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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4672 bytes --]

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@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.

> 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.

> +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().

> +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().

> +	/* 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.

> +	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.

> +
> +	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.

> +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.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx