| Message ID | 1421839093-7635-4-git-send-email-shobhit.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
-----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@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@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-----
On Tue, Feb 03, 2015 at 02:16:53PM +0100, Thierry Reding wrote: > On Wed, Jan 21, 2015 at 04:48:12PM +0530, Shobhit Kumar wrote: > > 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. I've had some extension discussion with Thierry on irc and I agree that using gpio/pwm interfaces will suit better. In addition both gpio and pwm subsystem already have magic register/lookup support (which works outside of devicetree or the new acpi tables, which I didn't know). So we'll avoid writing our own inter-module boilerplate. Shobhit, Jani&I also chatted about this topic on irc and I think the rough direction is hopefully clear now. Just a quick mail here for the record so that the irc discussion won't get lost. -Daniel
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 + help + Choose this option if you have BYT-T based device with DSI panel. On + BYT-T there a crystal cove PMIC which controls the PANEL EN/DISABLE + signals. + + If in doubt, say "N". diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 1849ffa..cc2f10d 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -88,4 +88,7 @@ i915-y += i915_dma.o \ obj-$(CONFIG_DRM_I915) += i915.o +# I915 DRM Panels +obj-$(CONFIG_DRM_I915_PANEL_CRYSTALCOVE_PMIC) += intel-panel-crystalcove.o + CFLAGS_i915_trace_points.o := -I$(src) diff --git a/drivers/gpu/drm/i915/intel-panel-crystalcove.c b/drivers/gpu/drm/i915/intel-panel-crystalcove.c new file mode 100644 index 0000000..a6aefa3 --- /dev/null +++ b/drivers/gpu/drm/i915/intel-panel-crystalcove.c @@ -0,0 +1,159 @@ +/* + * Copyright © 2014 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Authors: + * Shobhit Kumar <shobhit.kumar@intel.com> + */ + +#include <drm/drmP.h> +#include <drm/drm_crtc.h> +#include <drm/drm_panel.h> +#include <linux/mfd/intel_soc_pmic.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#define PMIC_PANEL_EN 0x52 +#define PMIC_PWM_EN 0x51 +#define PMIC_BKL_EN 0x4B +#define PMIC_PWM_LEVEL 0x4E + +struct crystalcove_panel { + struct drm_panel base; + bool enabled; + + /* crystal cove pmic regmap */ + struct regmap *regmap; +}; + +static inline struct crystalcove_panel *to_crystalcove_panel(struct drm_panel *panel) +{ + return container_of(panel, struct crystalcove_panel, base); +} + +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); + + /* Disable backlight as well */ + regmap_write(p->regmap, PMIC_PWM_LEVEL, 0); + msleep(20); + regmap_write(p->regmap, PMIC_PWM_EN, 0x00); + regmap_write(p->regmap, PMIC_BKL_EN, 0x7F); + + p->enabled = false; + + return 0; +} + +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); + + /* Enable BKL as well */ + regmap_write(p->regmap, PMIC_BKL_EN, 0xFF); + regmap_write(p->regmap, PMIC_PWM_EN, 0x01); + msleep(20); + regmap_write(p->regmap, PMIC_PWM_LEVEL, 255); + + p->enabled = true; + + return 0; +} + +static const struct drm_panel_funcs crystalcove_panel_funcs = { + .disable = crystalcove_panel_disable, + .enable = crystalcove_panel_enable, +}; + +static int crystalcove_panel_probe(struct platform_device *pdev) +{ + struct crystalcove_panel *panel; + int retval; + struct device *dev = pdev->dev.parent; + struct intel_soc_pmic *pmic = dev_get_drvdata(dev); + + panel = devm_kzalloc(&pdev->dev, sizeof(*panel), GFP_KERNEL); + if (!panel) + return -ENOMEM; + + DRM_DEBUG_KMS("\n"); + + platform_set_drvdata(pdev, panel); + + strcpy(panel->base.name, "crystal_cove_panel"); + panel->regmap = pmic->regmap; + + regmap_read(panel->regmap, PMIC_PANEL_EN, &retval); + panel->enabled = retval; + + drm_panel_init(&panel->base); + panel->base.dev = dev; + panel->base.funcs = &crystalcove_panel_funcs; + + drm_panel_add(&panel->base); + + return 0; +} + +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); + +MODULE_AUTHOR("Shobhit Kumar <shobhit.kumar@linux.intel.com"); +MODULE_DESCRIPTION("Intel Crystal Cove Panel Driver"); +MODULE_LICENSE("GPL and additional rights");
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