[Intel-gfx] [RFC v3 3/4] drm/i915: Add new panel driver based on crystal cove pmic
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_EN0x51 > > +#define PMIC_BKL_EN0x4B > > +#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 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[RFC v3 3/4] drm/i915: Add new panel driver based on crystal cove pmic
-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 ---
>> 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_LEVEL0x4E
>
> 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
[RFC v3 3/4] drm/i915: Add new panel driver based on crystal cove pmic
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
> ---
> 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_EN0x52
> +#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(>base);
> + panel->base.dev = dev;
> + panel->base.funcs = _panel_funcs;
> +
> + drm_panel_add(>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(>base);
> + drm_panel_remove(>base);
> +
> + crystalcove_panel_disable(>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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
[RFC v3 3/4] drm/i915: Add new panel driver based on crystal cove pmic
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
---
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
+ 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 000..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
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define PMIC_PANEL_EN 0x52
+#define PMIC_PWM_EN0x51
+#define PMIC_BKL_EN0x4B
+#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
[RFC v3 3/4] drm/i915: Add new panel driver based on crystal cove pmic
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
---
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
+ 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 000..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
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define PMIC_PANEL_EN 0x52
+#define PMIC_PWM_EN0x51
+#define PMIC_BKL_EN0x4B
+#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
