Hi All Please find modifed set of patches using regmap interface to accedd the PMIC registers. These patches implement a drm_panel as a platform driver for the mfd_cell device declared in intel_soc_pmic_core.c.
DRM is extended to provide a find panel by name in absence of OF.
Backlight control is pending. For now I am doing Backlight Enable/Disable also during panel/enable as this will at least save power.
Regards Shobhit
Shobhit Kumar (4): drm: Add support to find drm_panel by name mfd: Add a new cell device for panel controlled by crystal cove pmic drm/i915: Add new panel driver based on crystal cove pmic drm/i915: Enable DSI panel enable/disable based on PMIC
drivers/gpu/drm/drm_panel.c | 18 +++ drivers/gpu/drm/i915/Kconfig | 13 ++ drivers/gpu/drm/i915/Makefile | 3 + drivers/gpu/drm/i915/intel-panel-crystalcove.c | 160 +++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dsi.c | 16 +++ drivers/gpu/drm/i915/intel_dsi.h | 6 + drivers/mfd/intel_soc_pmic_crc.c | 3 + include/drm/drm_panel.h | 3 + 8 files changed, 222 insertions(+) create mode 100644 drivers/gpu/drm/i915/intel-panel-crystalcove.c
For scenarios where OF is not available, we can use panel identification by name.
Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com --- drivers/gpu/drm/drm_panel.c | 18 ++++++++++++++++++ include/drm/drm_panel.h | 3 +++ 2 files changed, 21 insertions(+)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index 2ef988e..e1cb8cf 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -95,6 +95,24 @@ struct drm_panel *of_drm_find_panel(struct device_node *np) EXPORT_SYMBOL(of_drm_find_panel); #endif
+struct drm_panel *drm_find_panel_by_name(const char *name) +{ + struct drm_panel *panel; + + mutex_lock(&panel_lock); + + list_for_each_entry(panel, &panel_list, list) { + if (strcmp(panel->name, name) == 0) { + mutex_unlock(&panel_lock); + return panel; + } + } + + mutex_unlock(&panel_lock); + return NULL; +} +EXPORT_SYMBOL(drm_find_panel_by_name); + MODULE_AUTHOR("Thierry Reding treding@nvidia.com"); MODULE_DESCRIPTION("DRM panel infrastructure"); MODULE_LICENSE("GPL and additional rights"); diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 1fbcc96..1ef9ff3 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -74,6 +74,7 @@ struct drm_panel { struct drm_device *drm; struct drm_connector *connector; struct device *dev; + char name[NAME_MAX];
const struct drm_panel_funcs *funcs;
@@ -137,4 +138,6 @@ static inline struct drm_panel *of_drm_find_panel(struct device_node *np) } #endif
+struct drm_panel *drm_find_panel_by_name(const char *name); + #endif
On Wed, Jan 21, 2015 at 04:48:10PM +0530, Shobhit Kumar wrote:
For scenarios where OF is not available, we can use panel identification by name.
Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com
drivers/gpu/drm/drm_panel.c | 18 ++++++++++++++++++ include/drm/drm_panel.h | 3 +++ 2 files changed, 21 insertions(+)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index 2ef988e..e1cb8cf 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -95,6 +95,24 @@ struct drm_panel *of_drm_find_panel(struct device_node *np) EXPORT_SYMBOL(of_drm_find_panel); #endif
+struct drm_panel *drm_find_panel_by_name(const char *name) +{
- struct drm_panel *panel;
- mutex_lock(&panel_lock);
- list_for_each_entry(panel, &panel_list, list) {
if (strcmp(panel->name, name) == 0) {mutex_unlock(&panel_lock);return panel;}- }
- mutex_unlock(&panel_lock);
- return NULL;
+} +EXPORT_SYMBOL(drm_find_panel_by_name);
MODULE_AUTHOR("Thierry Reding treding@nvidia.com"); MODULE_DESCRIPTION("DRM panel infrastructure"); MODULE_LICENSE("GPL and additional rights"); diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 1fbcc96..1ef9ff3 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -74,6 +74,7 @@ struct drm_panel { struct drm_device *drm; struct drm_connector *connector; struct device *dev;
- char name[NAME_MAX];
I thought I had said this before, but I can't find any record, so here goes again: NAME_MAX is a pretty large number and it increases the size of this structure a lot. Why not use dev_name(dev) to do the matching? Or if that doesn't work for some reason, allocate a string of the right size, or use a static one.
Thierry
For scenarios where OF is not available, we can use panel identification by name.
v2: Use const char *name instead of name[NAME_MAX] (Thierry)
CC: Thierry Reding thierry.reding@gmail.com Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com --- drivers/gpu/drm/drm_panel.c | 18 ++++++++++++++++++ include/drm/drm_panel.h | 3 +++ 2 files changed, 21 insertions(+)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index 2ef988e..7b4f559 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -95,6 +95,24 @@ struct drm_panel *of_drm_find_panel(struct device_node *np) EXPORT_SYMBOL(of_drm_find_panel); #endif
+struct drm_panel *drm_find_panel_by_name(const char *name) +{ + struct drm_panel *panel; + + mutex_lock(&panel_lock); + + list_for_each_entry(panel, &panel_list, list) { + if (panel->name && strcmp(panel->name, name) == 0) { + mutex_unlock(&panel_lock); + return panel; + } + } + + mutex_unlock(&panel_lock); + return NULL; +} +EXPORT_SYMBOL(drm_find_panel_by_name); + MODULE_AUTHOR("Thierry Reding treding@nvidia.com"); MODULE_DESCRIPTION("DRM panel infrastructure"); MODULE_LICENSE("GPL and additional rights"); diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 1fbcc96..43b9004 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -74,6 +74,7 @@ struct drm_panel { struct drm_device *drm; struct drm_connector *connector; struct device *dev; + const char *name;
const struct drm_panel_funcs *funcs;
@@ -137,4 +138,6 @@ static inline struct drm_panel *of_drm_find_panel(struct device_node *np) } #endif
+struct drm_panel *drm_find_panel_by_name(const char *name); + #endif
On BYT-T configuration, panel enable/disable signals are routed through PMIC. Add a cell device for the same.
Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com --- drivers/mfd/intel_soc_pmic_crc.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c index c85e2ec..c8ccc24 100644 --- a/drivers/mfd/intel_soc_pmic_crc.c +++ b/drivers/mfd/intel_soc_pmic_crc.c @@ -109,6 +109,9 @@ static struct mfd_cell crystal_cove_dev[] = { { .name = "crystal_cove_pmic", }, + { + .name = "crystal_cove_panel", + }, };
static struct regmap_config crystal_cove_regmap_config = {
On Wed, Jan 21, 2015 at 04:48:11PM +0530, Shobhit Kumar wrote:
On BYT-T configuration, panel enable/disable signals are routed through PMIC. Add a cell device for the same.
Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com
drivers/mfd/intel_soc_pmic_crc.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c index c85e2ec..c8ccc24 100644 --- a/drivers/mfd/intel_soc_pmic_crc.c +++ b/drivers/mfd/intel_soc_pmic_crc.c @@ -109,6 +109,9 @@ static struct mfd_cell crystal_cove_dev[] = { { .name = "crystal_cove_pmic", },
- {
.name = "crystal_cove_panel",- },
};
static struct regmap_config crystal_cove_regmap_config = {
This doesn't look at all right. A PMIC doesn't typically contain a panel so this likely is a wrong description of the hardware. Is the datasheet for the Crystal Cove PMIC available somewhere? Google doesn't turn any- thing useful up.
Thierry
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/03/2015 06:35 PM, Thierry Reding wrote:
On Wed, Jan 21, 2015 at 04:48:11PM +0530, Shobhit Kumar wrote:
On BYT-T configuration, panel enable/disable signals are routed through PMIC. Add a cell device for the same.
Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com --- drivers/mfd/intel_soc_pmic_crc.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c index c85e2ec..c8ccc24 100644 --- a/drivers/mfd/intel_soc_pmic_crc.c +++ b/drivers/mfd/intel_soc_pmic_crc.c @@ -109,6 +109,9 @@ static struct mfd_cell crystal_cove_dev[] = { { .name = "crystal_cove_pmic", }, + { + .name = "crystal_cove_panel", + }, };
static struct regmap_config crystal_cove_regmap_config = {
This doesn't look at all right. A PMIC doesn't typically contain a panel so this likely is a wrong description of the hardware. Is the datasheet for the Crystal Cove PMIC available somewhere? Google doesn't turn any- thing useful up.
Perhaps the naming makes it look like wrong, but only the Panel En/Disable and Back light En/Disable/Control signals for any DSI panels on this platform are routed through PMIC. Not sure if any spec is available publicly.
Regards Shobhit
Thierry
_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
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 + 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");
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
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
This allows for proper PPS during enable/disable of BYT-T platforms where these signals are routed through PMIC. Needs DRM_PANEL to be selected by default as well
v2: Adapt to panel find function name change in drm_panel
Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com --- drivers/gpu/drm/i915/Kconfig | 1 + drivers/gpu/drm/i915/intel-panel-crystalcove.c | 1 + drivers/gpu/drm/i915/intel_dsi.c | 16 ++++++++++++++++ drivers/gpu/drm/i915/intel_dsi.h | 6 ++++++ 4 files changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig index 0510ef0..bacbc06 100644 --- a/drivers/gpu/drm/i915/Kconfig +++ b/drivers/gpu/drm/i915/Kconfig @@ -18,6 +18,7 @@ config DRM_I915 select INPUT if ACPI select ACPI_VIDEO if ACPI select ACPI_BUTTON if ACPI + select DRM_PANEL help Choose this option if you have a system that has "Intel Graphics Media Accelerator" or "HD Graphics" integrated graphics, diff --git a/drivers/gpu/drm/i915/intel-panel-crystalcove.c b/drivers/gpu/drm/i915/intel-panel-crystalcove.c index a6aefa3..0f129b2 100644 --- a/drivers/gpu/drm/i915/intel-panel-crystalcove.c +++ b/drivers/gpu/drm/i915/intel-panel-crystalcove.c @@ -154,6 +154,7 @@ static struct platform_driver crystalcove_panel_driver = {
module_platform_driver(crystalcove_panel_driver);
+MODULE_ALIAS(); MODULE_AUTHOR("Shobhit Kumar <shobhit.kumar@linux.intel.com"); MODULE_DESCRIPTION("Intel Crystal Cove Panel Driver"); MODULE_LICENSE("GPL and additional rights"); diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 42b6d6f..6857d19 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -26,6 +26,7 @@ #include <drm/drmP.h> #include <drm/drm_crtc.h> #include <drm/drm_edid.h> +#include <drm/drm_panel.h> #include <drm/i915_drm.h> #include <linux/slab.h> #include "i915_drv.h" @@ -230,6 +231,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
DRM_DEBUG_KMS("\n");
+ drm_panel_enable(intel_dsi->panel); + /* Disable DPOunit clock gating, can stall pipe * and we need DPLL REFA always enabled */ tmp = I915_READ(DPLL(pipe)); @@ -392,6 +395,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder)
msleep(intel_dsi->panel_off_delay); msleep(intel_dsi->panel_pwr_cycle_delay); + + drm_panel_disable(intel_dsi->panel); }
static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, @@ -896,6 +901,17 @@ void intel_dsi_init(struct drm_device *dev) fixed_mode->type |= DRM_MODE_TYPE_PREFERRED; intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
+ /* Initialize the PMIC based drm_panel if available on the platform */ + if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) { + intel_dsi->panel = drm_find_panel_by_name("crystal_cove_panel"); + if (!intel_dsi->panel) { + DRM_ERROR("PMIC Panel control will not work !!\n"); + return; + } + + drm_panel_attach(intel_dsi->panel, connector); + } + return;
err: diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index 8fe2064..4a9242d 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -33,6 +33,9 @@ #define DSI_DUAL_LINK_FRONT_BACK 1 #define DSI_DUAL_LINK_PIXEL_ALT 2
+#define PPS_BLC_PMIC 0 +#define PPS_BLC_SOC 1 + struct intel_dsi_device { unsigned int panel_id; const char *name; @@ -83,6 +86,8 @@ struct intel_dsi {
struct intel_connector *attached_connector;
+ struct drm_panel *panel; + /* bit mask of ports being driven */ u16 ports;
@@ -116,6 +121,7 @@ struct intel_dsi { u32 dphy_reg; u32 video_frmt_cfg_bits; u16 lp_byte_clk; + u8 pps_blc;
/* timeouts in byte clocks */ u16 lp_rx_timeout;
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Shobhit Kumar -
Shobhit Kumar -
Thierry Reding