[Intel-gfx] [RFC v3 3/4] drm/i915: Add new panel driver based on crystal cove pmic

[Shobhit Kumar]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/03/2015 06:46 PM, Thierry Reding wrote:
> On Wed, Jan 21, 2015 at 04:48:12PM +0530, Shobhit Kumar wrote:
>> This driver provides support for the "crystal_cove_panel" cell
>> device. On BYT-T pmic has to be used to enable/disable panel.
>> 
>> v2: Addressed Jani's comments - Moved inside i915 - Correct
>> licensing - Remove unused stuff - Do not initialize
>> prepare/unprepare as they are not needed as of now - Correct
>> backlight off delay
>> 
>> Signed-off-by: Shobhit Kumar <shobhit.kumar at intel.com> --- 
>> drivers/gpu/drm/i915/Kconfig                   |  12 ++ 
>> drivers/gpu/drm/i915/Makefile                  |   3 + 
>> drivers/gpu/drm/i915/intel-panel-crystalcove.c | 159
>> +++++++++++++++++++++++++ 3 files changed, 174 insertions(+) 
>> create mode 100644
>> drivers/gpu/drm/i915/intel-panel-crystalcove.c
>> 
>> diff --git a/drivers/gpu/drm/i915/Kconfig
>> b/drivers/gpu/drm/i915/Kconfig index 4e39ab3..0510ef0 100644 ---
>> a/drivers/gpu/drm/i915/Kconfig +++
>> b/drivers/gpu/drm/i915/Kconfig @@ -69,3 +69,15 @@ config
>> DRM_I915_PRELIMINARY_HW_SUPPORT option changes the default for
>> that module option.
>> 
>> If in doubt, say "N". + +config DRM_I915_PANEL_CRYSTALCOVE_PMIC +
>> bool "Enable drm panel for crystal cove pmic based control" +
>> depends on DRM_I915 +	depends on DRM_PANEL +	default n
> 
> n is the default default.

Okay.

> 
>> diff --git a/drivers/gpu/drm/i915/intel-panel-crystalcove.c
>> b/drivers/gpu/drm/i915/intel-panel-crystalcove.c
> [...]
>> +#define PMIC_PANEL_EN		0x52 +#define PMIC_PWM_EN		0x51 +#define
>> PMIC_BKL_EN		0x4B +#define PMIC_PWM_LEVEL		0x4E
> 
> These look like they should be GPIOs/regulators and a PWM instead.
> So I think you'd need to further split up the MFD device to
> accomodate for this.

Yes these are I2C addresses for the PWM and Panel control registers on
PMIC. The PMIC driver is a I2C slave device and all control for
Crystal cove is through i2c. Till now there is no situation where
either of the two PWM and Panel control are through interfaces other
than PMIC. Either both are through PMIC or none. With that in mind, I
think can have one common MFD device for PWM and Panel control. Also I
was planning to add Backlight control via backlight class driver in
this same panel driver.

> 
>> +static inline struct crystalcove_panel
>> *to_crystalcove_panel(struct drm_panel *panel)
> 
> Please wrap this so it doesn't cross the 80-character boundary.
> 
>> +static int crystalcove_panel_disable(struct drm_panel *panel) 
>> +{ +	struct crystalcove_panel *p = to_crystalcove_panel(panel); 
>> + +	if (!p->enabled) +		return 0; + +	DRM_DEBUG_KMS("\n"); + +	/*
>> invoke the pmic driver */ +	regmap_write(p->regmap,
>> PMIC_PANEL_EN, 0x00);
> 
> A datasheet would be really good to determine whether this is the 
> correct place to write this. There are ->prepare() and
> ->unprepare() callbacks that get the panel into a state where you
> can communicate with it. This being a DSI panel I would assume that
> you need to enable the panel to some degree so you can send DSI
> commands. So most likely this enable "GPIO" should be set in
> ->unprepare().

This is just a mechanism to enable needed signals which are routed
through PMIC, I kept the logic as per enable and disable signals in
enable/disable. But yes actually it is preparing the panel to receive
DCS commands. I can move this into prepare/unprepare. In that case
there would be no need for any enable/disable callbacks.

> 
>> +static int crystalcove_panel_enable(struct drm_panel *panel) +{ 
>> +	struct crystalcove_panel *p = to_crystalcove_panel(panel); + +
>> if (p->enabled) +		return 0; + +	DRM_DEBUG_KMS("\n"); + +	/*
>> invoke the pmic driver */ +	regmap_write(p->regmap,
>> PMIC_PANEL_EN, 0x01);
> 
> Similarly I'd expect this to go into ->prepare() to make sure that
> you can communicate with the panel after ->prepare().

Can do as I mentioned above.

> 
>> +	/* Enable BKL as well */ +	regmap_write(p->regmap, PMIC_BKL_EN,
>> 0xFF);
> 
> Writing 0xff to an "enable" register seems weird. Again the
> datasheet would help in determining the right thing to do here.

Actually, I think I made a mistake in naming the register. It is
actually the PWMCLKDIV which has to be programmed as per needed
divider. I will correct this. Spec is not available in public.

> 
>> +	regmap_write(p->regmap, PMIC_PWM_EN, 0x01); +	msleep(20); +
>> regmap_write(p->regmap, PMIC_PWM_LEVEL, 255);
> 
> This definitely looks like it should be a PWM driver. Also how do
> you handle backlight brightness control? You only set things to
> either full off or full on in this driver.

As of now I did just enable and disable to save power leakage during
Suspend/Resume. I am planning to add a backlight class driver as well
as pert of this itself. As I said this should be okay as either both
Panel and PWM control is thorough PMIC or none in our platforms as of now.

> 
>> + +	drm_panel_init(&panel->base); +	panel->base.dev = dev; +
>> panel->base.funcs = &crystalcove_panel_funcs; + +
>> drm_panel_add(&panel->base);
> 
> This function can theoretically fail, although it doesn't (at
> present), so checking the error might be a good idea.

Okay.

> 
>> +static int crystalcove_panel_remove(struct platform_device
>> *pdev) +{ +	struct crystalcove_panel *panel =
>> platform_get_drvdata(pdev); + +	DRM_DEBUG_KMS("\n"); + +
>> drm_panel_detach(&panel->base); +
>> drm_panel_remove(&panel->base); + +
>> crystalcove_panel_disable(&panel->base); + +	return 0; +} + 
>> +static struct platform_driver crystalcove_panel_driver = { +
>> .probe = crystalcove_panel_probe, +	.remove =
>> crystalcove_panel_remove, +	.driver = { +		.name =
>> "crystal_cove_panel", +	}, +}; + 
>> +module_platform_driver(crystalcove_panel_driver);
> 
> What's also weird here is that you claim this to be a DSI panel,
> yet you use a platform driver. The right thing to do would be to
> instantiate the device as mipi_dsi_device, with the DSI controller
> being the parent.

This is not a DSI panel driver but just to control the Panel
EN/Disbale signals and PWM on BYT platform with CRC PMIC.
mipi_dsi_device conversion for the actual DSI panel driver is recently
done on i915.

Regards
Shobhit

> 
> Thierry
> 
> 
> 
> _______________________________________________ dri-devel mailing
> list dri-devel at lists.freedesktop.org 
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU0cJJAAoJEHuQFv2//5KqjBkH/35Md/V3bpdNzA0+hjrTy7ap
GPbApTUhyurYm32jAqQwtoVsYHl4Wysyv8lTxQnz+j1VcvWcq2e3a25j1Qo0ybbq
dZeDTr2r3X7Fgj+arFboLxEQSvIWLaT0ML+xaNorbsCaEGxXl/frX/fTCuWXeJet
VX1dII9UbTKtxV/Sc1lcXruL9O4pJSnH02tP+sXqev1gyPhm8EU2Wjz3MReUsvVh
roWnaYTUk7GS15ihuvD2mbm4+o5H8GD8wEP2F5telcKoD4C49HKFbmWVyXFKjHSe
kojntvgYAUGBQY2g6ytTk+nuE/56e3cIFSr27v8RwffWLw+VOWELT30gtKLF6ww=
=Jqac
-----END PGP SIGNATURE-----