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