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@xxxxxxxxx>
> ---
> 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:
pgpMNSGeTiLQ8.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
