drm/dsi: Implement dcs set/get display brightness
Subsystems (3)
PATCH v2 1/2 drm/dsi: Implement dcs set/get display brightness
Provide a small convenience wrapper that set/get the display brightness value
---
v1:
*tested in nexus7 2nd gen.
v2:
* implemented jani review comments
-functions name mapped accordingly
-bl value increased from 0xff to 0xffff -backlight interface will be handled in panel driver,
so it is moved from the mipi_dsi helper function
--- drivers/gpu/drm/drm_mipi_dsi.c | 49 ++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_mipi_dsi.h | 4 ++++ 2 files changed, 53 insertions(+)
drivers/gpu/drm/drm_mipi_dsi.c +49 -0
@@ -1041,6 +1041,55 @@ int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format) }EXPORT_SYMBOL (mipi_dsi_dcs_set_pixel_format); +/** +* mipi_dsi_dcs_get_display_brightness() - gets the current brightness value +* of the display +* @dsi: DSI peripheral device +* @brightness: brightness value +* +* Return: 0 on success or a negative error code on failure. +*/ +int mipi_dsi_dcs_get_display_brightness (struct mipi_dsi_device *dsi, +u16 *brightness) +{ +ssize_t err; + + err =mipi_dsi_dcs_read (dsi,MIPI_DCS_GET_DISPLAY_BRIGHTNESS , + brightness,sizeof (*brightness)); +if (err <0 ) { +if (err ==0 ) + err = -ENODATA ; + +return err; + } + +return 0 ; +} +EXPORT_SYMBOL (mipi_dsi_dcs_get_display_brightness); + +/** +* mipi_dsi_dcs_set_display_brightness() - sets the brightness value of +* the display +* @dsi: DSI peripheral device +* @brightness: brightness value +* +* Return: 0 on success or a negative error code on failure. +*/ +int mipi_dsi_dcs_set_display_brightness (struct mipi_dsi_device *dsi, +u16 brightness) +{ +ssize_t err; +u8 bl_value[2 ] = { brightness &0xff , brightness >>8 }; + + err =mipi_dsi_dcs_write (dsi,MIPI_DCS_SET_DISPLAY_BRIGHTNESS , + bl_value,sizeof (bl_value)); +if (err <0 ) +return err; + +return 0 ; +} +EXPORT_SYMBOL (mipi_dsi_dcs_set_display_brightness); +static int mipi_dsi_drv_probe (struct device *dev) {struct mipi_dsi_driver *drv =to_mipi_dsi_driver (dev->driver);
include/drm/drm_mipi_dsi.h +4 -0
@@ -270,6 +270,10 @@ int mipi_dsi_dcs_set_tear_off(struct mipi_dsi_device *dsi);int mipi_dsi_dcs_set_tear_on (struct mipi_dsi_device *dsi,enum mipi_dsi_dcs_tear_mode mode);int mipi_dsi_dcs_set_pixel_format (struct mipi_dsi_device *dsi,u8 format); +int mipi_dsi_dcs_get_display_brightness (struct mipi_dsi_device *dsi, +u16 *brightness); +int mipi_dsi_dcs_set_display_brightness (struct mipi_dsi_device *dsi, +u16 brightness);/** * struct mipi_dsi_driver - DSI driver
Signature · 1 line
1.9.1
PATCH v5 2/2 drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel
Add support for the JDI LT070ME05000 WUXGA DSI panel used in Nexus 7 2013 devices.
Programming sequence for the panel is was originally found in the android-msm-flo-3.4-lollipop-release branch from:
https://android.googlesource.com/kernel/msm.git
And video mode setting is from dsi-panel-jdi-dualmipi1-video.dtsi file in:
git://codeaurora.org/kernel/msm-3.10.git LNX.LA.3.6_rb1.27
[jstultz: Cherry-picked to mainline, folded down other fixes
from Vinay and Archit]
[vinay simha bn: removed interface setting cmd mode, video mode panel setting selection]
---
v2:
* incorporated code reviews from theiry, archit
code style, alphabetical soring in Makefile, Kconfig, regulator_bulk, arrays of u8, generic helper function, documentation bindings,
v3:
* dcs backlight support added
* tested this panel driver in nexus7 2013 device
v4:
* backlight interface added in the panel driver
* incorporated width_mm and height_mm suggested by rob herring
v5:
* theirry review comments incorporated
panel model naming consistent, alphabetical soring in Kconfig Makefile, MAX_BRIGHTNESS dropped, regulator_names, parameterize panel width and height, descprition for control display, cabc and interface setting, temporary variable removed, consistent error reporting and commit message
* removed tear on/off, scanline, since these are required only for command mode panels
--- drivers/gpu/drm/panel/Kconfig | 11 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 515 +++++++++++++++++++++++++ 3 files changed, 527 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
drivers/gpu/drm/panel/Kconfig +11 -0
@@ -18,6 +18,17 @@ config DRM_PANEL_SIMPLE that it can be automatically turned off when the panel goes into a low power state. +config DRM_PANEL_JDI_LT070ME05000 + tristate "JDI LT070ME05000 WUXGA DSI panel" + depends on OF + depends on DRM_MIPI_DSI + depends on BACKLIGHT_CLASS_DEVICE + help + Say Y here if you want to enable support for JDI DSI video mode + panel as found in Google Nexus 7 (2013) devices. + The panel has a 1200(RGB)×1920 (WUXGA) resolution and uses + 24 bit per pixel. + config DRM_PANEL_SAMSUNG_LD9040 tristate "Samsung LD9040 RGB/SPI panel" depends on OF && SPI
drivers/gpu/drm/panel/Makefile +1 -0
@@ -1,4 +1,5 @@ obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o +obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +515 -0
515 lines not on this page · raw message
Signature · 1 line
1.9.1
Re: [PATCH v5 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel
Hi Vinay,
I belive I've spotted a few issues. If my understanding is correct, then I'll defer to Thierry if he'd like them fixed here, or as follow-ups.
Review
Quoted 2 lines · Vinay Simha BN
(quoted diff) +2
-0+#define PANEL_NUM_REGULATORS 3 +
Nit: #define PANEL_NUM_REGULATORS ARRAY_SIZE(regulator_names) or just drop the extra define and use the latter directly ?
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 132
Quoted 8 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +6
-0+static int jdi_panel_init (struct jdi_panel *jdi) +{ +if (ret <0 ) +return ret; + +return 0 ;
Nit: The above three lines can become one - "return ret;"
Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4
-0+} + +static int jdi_panel_on (struct jdi_panel *jdi) +{
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 146
Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10
-0+struct mipi_dsi_device *dsi = jdi->dsi; +int ret; + + dsi->mode_flags |=MIPI_DSI_MODE_LPM ; + + ret =mipi_dsi_dcs_set_display_on (dsi); +if (ret <0 ) +return ret; + +return 0 ;
Ditto.
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 175
Quoted 7 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +7
-0+static int jdi_panel_disable (struct drm_panel *panel) +{ +struct jdi_panel *jdi =to_jdi_panel (panel); + +if (!jdi->enabled) +return 0 ; +
Thinking out loud:
Thierry,
Shouldn't we fold 'enabled' and 'prepared' in struct drm_panel and tweak the helpers respectively ? Is there any specific reason for keeping these in the drivers ?
Quoted 1 line
+ if (
jdi->backlight) {
We seems to be bailing out of jdi_panel_add() when this is NULL. Thus we can omit the check.
Quoted 11 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +11
-0+static int jdi_panel_unprepare (struct drm_panel *panel) +{ +struct jdi_panel *jdi =to_jdi_panel (panel); +struct device *dev = &jdi->dsi->dev; +int ret; + +if (!jdi->prepared) +return 0 ; + + ret =jdi_panel_off (jdi); +if (ret) {
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 206
Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10
-0+dev_err (panel->dev,"failed to set panel off: %d\n" , ret); +return ret; + } + + ret =regulator_bulk_disable (ARRAY_SIZE (jdi->supplies), jdi->supplies); +if (ret <0 ) { +dev_err (dev,"regulator disable failed, %d\n" , ret); +return ret; + } +
Since we cannot recover from most/all of the above I'm thinking if one shouldn't drop the "return ret" lines. Same goes for jdi_panel_off().
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 210
Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4
-0+if (jdi->reset_gpio) +gpiod_set_value (jdi->reset_gpio,0 ); + +if (jdi->enable_gpio)
Drop these two checks. The gpios are required, thus one should bail out in jdi_panel_add()
Quoted 11 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10
-0+static int jdi_panel_prepare (struct drm_panel *panel) +{ +if (jdi->reset_gpio) { +gpiod_set_value (jdi->reset_gpio,1 ); +usleep_range (10 ,20 ); + } + +if (jdi->enable_gpio) { +gpiod_set_value (jdi->enable_gpio,1 ); +usleep_range (10 ,20 );
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 266
Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +8
-0+ } + +poweroff: +if (jdi->reset_gpio) +gpiod_set_value (jdi->reset_gpio,0 ); +if (jdi->enable_gpio) +gpiod_set_value (jdi->enable_gpio,0 ); +
Generic suggestion/nitpick: Please keep the teardown order the inverse of the setup one. In here one could/should handle enable_gpio first and then reset_gpio.
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 277
Quoted 11 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +11
-0+return ret; +} + +static int jdi_panel_enable (struct drm_panel *panel) +{ +struct jdi_panel *jdi =to_jdi_panel (panel); + +if (jdi->enabled) +return 0 ; + +if (jdi->backlight) {
Analogous to jdi_panel_disable - drop the check ?
Quoted 12 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10
-0+static int jdi_panel_add (struct jdi_panel *jdi) +{ + jdi->reset_gpio =devm_gpiod_get (dev,"reset" ,GPIOD_OUT_LOW ); +if (IS_ERR (jdi->reset_gpio)) { +dev_err (dev,"cannot get reset-gpios %ld\n" , +PTR_ERR (jdi->reset_gpio)); + jdi->reset_gpio =NULL ; + }else { +gpiod_direction_output (jdi->reset_gpio,0 ); + }
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 427
Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10
-0+ + jdi->enable_gpio =devm_gpiod_get (dev,"enable" ,GPIOD_OUT_LOW ); +if (IS_ERR (jdi->enable_gpio)) { +dev_err (dev,"cannot get enable-gpio %ld\n" , +PTR_ERR (jdi->enable_gpio)); + jdi->enable_gpio =NULL ; + }else { +gpiod_direction_output (jdi->enable_gpio,0 ); + } +
As mentioned above - since these two are required, thus we should error out of this function. Right ?
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 440
Quoted 13 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +13
-0+ jdi->backlight =drm_panel_create_dsi_backlight (jdi->dsi); +if (!jdi->backlight) +return -EPROBE_DEFER ; + +drm_panel_init (&jdi->base); + jdi->base.funcs = &jdi_panel_funcs; + jdi->base.dev = &jdi->dsi->dev; + + ret =drm_panel_add (&jdi->base); +if (ret <0 ) +return ret; + +return 0 ;
return ret;
Regards,
Emil
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel
Quoted 1 line · Emil Velikov
On 16 June 2016 at 04:00, Vinay Simha BN <[email protected]> wrote:
[...]
Quoted 9 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 175
Quoted 7 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +7
-0+static int jdi_panel_disable (struct drm_panel *panel) +{ +struct jdi_panel *jdi =to_jdi_panel (panel); + +if (!jdi->enabled) +return 0 ; +Thinking out loud:
Quoted 4 lines
Thierry,
Shouldn't we fold 'enabled' and 'prepared' in structdrm_paneland tweak the helpers respectively ? Is there any specific reason for keeping these in the drivers ?
Yes, I think that would make sense eventually. It's clearly a recurring pattern. Ideally nothing would be calling these functions more than once and thereby making the checks unnecessary. In practice that may mean that we need to put the variables and checks into the drm/panel core because display drivers (as opposed to a sane core implementation) call these. I suppose we could encourage proper usage by adding a couple of WARNs here and there if expectations aren't met.
I don't think doing this is terribly urgent because it's easy to rip out of drivers once the drm/panel core supports it. And it's something that we could even leave within drivers when the core supports it, so trivial to remove one by one after the core patches have landed.
Thierry
Re: [PATCH v5 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel
Quoted 25 lines · Thierry Reding
Quoted 1 line · Emil Velikov
On 16 June 2016 at 04:00, Vinay Simha BN <[email protected]> wrote:
[...]
Quoted 9 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 175
Quoted 7 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +7
-0+static int jdi_panel_disable (struct drm_panel *panel) +{ +struct jdi_panel *jdi =to_jdi_panel (panel); + +if (!jdi->enabled) +return 0 ; +Thinking out loud:
Quoted 4 lines
Thierry,
Shouldn't we fold 'enabled' and 'prepared' in structdrm_paneland tweak the helpers respectively ? Is there any specific reason for keeping these in the drivers ?Yes, I think that would make sense eventually. It's clearly a recurring pattern. Ideally nothing would be calling these functions more than once and thereby making the checks unnecessary. In practice that may mean that we need to put the variables and checks into the drm/panel core because display drivers (as opposed to a sane core implementation) call these. I suppose we could encourage proper usage by adding a couple of WARNs here and there if expectations aren't met.
Quoted 4 lines
I don't think doing this is terribly urgent because it's easy to rip out of drivers once the drm/panel core supports it. And it's something that we could even leave within drivers when the core supports it, so trivial to remove one by one after the core patches have landed.
As long as we have non-atomic drm drivers using this multiple enable/disable calls can happen. Atomic drivers should screw this up (ignoring a few misguided ones that mix atomic and legacy helpers in bad ways, but those are getting fixed).
I think a good plan would be:
1. Move this tracking into drm panel helpers, ditch it from all drivers.
2. Add WARN_ON for multiple enables/disables, but only for DRIVER_ATOMIC.
Makes sure we can remove this boilerplate, makes sure that atomic drivers are consistent, leaves existing drivers unharmed.
Cheers, Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel
Quoted 36 lines · Daniel Vetter
Quoted 25 lines · Thierry Reding
Quoted 1 line · Emil Velikov
On 16 June 2016 at 04:00, Vinay Simha BN <[email protected]> wrote:
[...]
Quoted 9 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 175
Quoted 7 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +7
-0+static int jdi_panel_disable (struct drm_panel *panel) +{ +struct jdi_panel *jdi =to_jdi_panel (panel); + +if (!jdi->enabled) +return 0 ; +Thinking out loud:
Quoted 4 lines
Thierry,
Shouldn't we fold 'enabled' and 'prepared' in structdrm_paneland tweak the helpers respectively ? Is there any specific reason for keeping these in the drivers ?Yes, I think that would make sense eventually. It's clearly a recurring pattern. Ideally nothing would be calling these functions more than once and thereby making the checks unnecessary. In practice that may mean that we need to put the variables and checks into the drm/panel core because display drivers (as opposed to a sane core implementation) call these. I suppose we could encourage proper usage by adding a couple of WARNs here and there if expectations aren't met.
Quoted 4 lines
I don't think doing this is terribly urgent because it's easy to rip out of drivers once the drm/panel core supports it. And it's something that we could even leave within drivers when the core supports it, so trivial to remove one by one after the core patches have landed.
As long as we have non-atomic drm drivers using this multiple enable/disable calls can happen. Atomic drivers should screw this up (ignoring a few misguided ones that mix atomic and legacy helpers in bad ways, but those are getting fixed).
Quoted 6 lines
I think a good plan would be:
1. Move this tracking into drm panel helpers, ditch it from all drivers.
2. AddWARN_ONfor multiple enables/disables, but only forDRIVER_ATOMIC.Makes sure we can remove this boilerplate, makes sure that atomic drivers are consistent, leaves existing drivers unharmed.
Yeah, that sounds like a reasonable plan.
Thierry
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel