| Message ID | 1464832099-22402-2-git-send-email-simhavcs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 02 Jun 2016, Vinay Simha BN <simhavcs@gmail.com> wrote: > Provide a small convenience wrapper that set/get the > backlight brightness control and creates the backlight > device for the panel interface To be pedantic, we should downplay "backlight" in the DSI DCS brightness control... there need not be a backlight, at all, for brightness control (see AMOLED). > Cc: John Stultz <john.stultz@linaro.org> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: Archit Taneja <archit.taneja@gmail.com> > Cc: Rob Clark <robdclark@gmail.com> > Signed-off-by: Vinay Simha BN <simhavcs@gmail.com> > > -- > v1: > *tested in nexus7 2nd gen. > --- > drivers/gpu/drm/drm_mipi_dsi.c | 63 ++++++++++++++++++++++++++++++++++++++++++ > include/drm/drm_mipi_dsi.h | 3 ++ > 2 files changed, 66 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c > index 2f0b85c..ac4521e 100644 > --- a/drivers/gpu/drm/drm_mipi_dsi.c > +++ b/drivers/gpu/drm/drm_mipi_dsi.c > @@ -1026,6 +1026,69 @@ int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format) > } > EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format); > > +static int dsi_dcs_bl_get_brightness(struct backlight_device *bl) I'd rather see convenience helpers that are not tied to struct backlight_device. You can add a one-line wrapper on top that takes struct backlight_device pointer. While at it, please name the helpers according to the DCS command. > +{ > + struct mipi_dsi_device *dsi = bl_get_data(bl); > + int ret; > + u8 brightness; > + > + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; > + > + ret = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_DISPLAY_BRIGHTNESS, > + &brightness, sizeof(brightness)); > + if (ret < 0) > + return ret; > + > + dsi->mode_flags |= MIPI_DSI_MODE_LPM; > + > + return brightness; > +} > + > +static int dsi_dcs_bl_update_status(struct backlight_device *bl) > +{ > + struct mipi_dsi_device *dsi = bl_get_data(bl); > + int ret; > + u8 brightness = bl->props.brightness; > + > + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; > + > + ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, > + &brightness, sizeof(brightness)); > + if (ret < 0) > + return ret; > + > + dsi->mode_flags |= MIPI_DSI_MODE_LPM; > + > + return 0; > +} > + > +static const struct backlight_ops dsi_bl_ops = { > + .update_status = dsi_dcs_bl_update_status, > + .get_brightness = dsi_dcs_bl_get_brightness, > +}; > + > +/** > + * drm_panel_create_dsi_backlight() - creates the backlight device for the panel > + * @dsi: DSI peripheral device > + * > + * @return a struct backlight on success, or an ERR_PTR on error > + */ > +struct backlight_device > + *drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi) > +{ > + struct device *dev = &dsi->dev; > + struct backlight_properties props; > + > + memset(&props, 0, sizeof(props)); > + props.type = BACKLIGHT_RAW; > + props.brightness = 255; > + props.max_brightness = 255; The DCS spec says the max is either 0xff or 0xffff depending on the implementation... this obviously affects the helpers as well. And how about the backlight bits in write_control_display? I just fear this is a simplistic view of brightness control on DSI, and this will grow hairy over time. I think I'd rather see generic DSI DCS brightness *helpers* in this file, and then *perhaps* a separate file for brightness control in general. BR, Jani. > + > + return devm_backlight_device_register(dev, dev_name(dev), dev, dsi, > + &dsi_bl_ops, &props); > +} > +EXPORT_SYMBOL(drm_panel_create_dsi_backlight); > + > static int mipi_dsi_drv_probe(struct device *dev) > { > struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver); > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h > index 2788dbe..9dd6a97 100644 > --- a/include/drm/drm_mipi_dsi.h > +++ b/include/drm/drm_mipi_dsi.h > @@ -12,6 +12,7 @@ > #ifndef __DRM_MIPI_DSI_H__ > #define __DRM_MIPI_DSI_H__ > > +#include <linux/backlight.h> > #include <linux/device.h> > > struct mipi_dsi_host; > @@ -269,6 +270,8 @@ 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); > +struct backlight_device > + *drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi); > > /** > * struct mipi_dsi_driver - DSI driver
On Mon, Jun 6, 2016 at 1:07 PM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Thu, 02 Jun 2016, Vinay Simha BN <simhavcs@gmail.com> wrote: >> Provide a small convenience wrapper that set/get the >> backlight brightness control and creates the backlight >> device for the panel interface > > To be pedantic, we should downplay "backlight" in the DSI DCS brightness > control... there need not be a backlight, at all, for brightness control > (see AMOLED). but this jdi display and few other dsi display can be controlled by dcs commands > >> Cc: John Stultz <john.stultz@linaro.org> >> Cc: Sumit Semwal <sumit.semwal@linaro.org> >> Cc: Archit Taneja <archit.taneja@gmail.com> >> Cc: Rob Clark <robdclark@gmail.com> >> Signed-off-by: Vinay Simha BN <simhavcs@gmail.com> >> >> -- >> v1: >> *tested in nexus7 2nd gen. >> --- >> drivers/gpu/drm/drm_mipi_dsi.c | 63 ++++++++++++++++++++++++++++++++++++++++++ >> include/drm/drm_mipi_dsi.h | 3 ++ >> 2 files changed, 66 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c >> index 2f0b85c..ac4521e 100644 >> --- a/drivers/gpu/drm/drm_mipi_dsi.c >> +++ b/drivers/gpu/drm/drm_mipi_dsi.c >> @@ -1026,6 +1026,69 @@ int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format) >> } >> EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format); >> >> +static int dsi_dcs_bl_get_brightness(struct backlight_device *bl) > > I'd rather see convenience helpers that are not tied to struct > backlight_device. You can add a one-line wrapper on top that takes > struct backlight_device pointer. > i need to move the backlight_ops, backlight struct in panel driver and need to set/get brightness in drm_mipi_dsi.c > While at it, please name the helpers according to the DCS command. i will changes this to in v2 version mipi_dsi_dcs_set_brightness mipi_dsi_dcs_get_brightness > >> +{ >> + struct mipi_dsi_device *dsi = bl_get_data(bl); >> + int ret; >> + u8 brightness; >> + >> + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; >> + >> + ret = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_DISPLAY_BRIGHTNESS, >> + &brightness, sizeof(brightness)); >> + if (ret < 0) >> + return ret; >> + >> + dsi->mode_flags |= MIPI_DSI_MODE_LPM; >> + >> + return brightness; >> +} >> + >> +static int dsi_dcs_bl_update_status(struct backlight_device *bl) >> +{ >> + struct mipi_dsi_device *dsi = bl_get_data(bl); >> + int ret; >> + u8 brightness = bl->props.brightness; >> + >> + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; >> + >> + ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, >> + &brightness, sizeof(brightness)); >> + if (ret < 0) >> + return ret; >> + >> + dsi->mode_flags |= MIPI_DSI_MODE_LPM; >> + >> + return 0; >> +} >> + >> +static const struct backlight_ops dsi_bl_ops = { >> + .update_status = dsi_dcs_bl_update_status, >> + .get_brightness = dsi_dcs_bl_get_brightness, >> +}; >> + >> +/** >> + * drm_panel_create_dsi_backlight() - creates the backlight device for the panel >> + * @dsi: DSI peripheral device >> + * >> + * @return a struct backlight on success, or an ERR_PTR on error >> + */ >> +struct backlight_device >> + *drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi) >> +{ >> + struct device *dev = &dsi->dev; >> + struct backlight_properties props; >> + >> + memset(&props, 0, sizeof(props)); >> + props.type = BACKLIGHT_RAW; >> + props.brightness = 255; >> + props.max_brightness = 255; > > The DCS spec says the max is either 0xff or 0xffff depending on the > implementation... this obviously affects the helpers as well. > > And how about the backlight bits in write_control_display? 8 bits I just fear > this is a simplistic view of brightness control on DSI, and this will > grow hairy over time. I think I'd rather see generic DSI DCS brightness > *helpers* in this file, and then *perhaps* a separate file for > brightness control in general. > > BR, > Jani. > >> + >> + return devm_backlight_device_register(dev, dev_name(dev), dev, dsi, >> + &dsi_bl_ops, &props); >> +} >> +EXPORT_SYMBOL(drm_panel_create_dsi_backlight); >> + >> static int mipi_dsi_drv_probe(struct device *dev) >> { >> struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver); >> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h >> index 2788dbe..9dd6a97 100644 >> --- a/include/drm/drm_mipi_dsi.h >> +++ b/include/drm/drm_mipi_dsi.h >> @@ -12,6 +12,7 @@ >> #ifndef __DRM_MIPI_DSI_H__ >> #define __DRM_MIPI_DSI_H__ >> >> +#include <linux/backlight.h> >> #include <linux/device.h> >> >> struct mipi_dsi_host; >> @@ -269,6 +270,8 @@ 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); >> +struct backlight_device >> + *drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi); >> >> /** >> * struct mipi_dsi_driver - DSI driver > > -- > Jani Nikula, Intel Open Source Technology Center
On Mon, 06 Jun 2016, Vinay Simha <simhavcs@gmail.com> wrote: > On Mon, Jun 6, 2016 at 1:07 PM, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> On Thu, 02 Jun 2016, Vinay Simha BN <simhavcs@gmail.com> wrote: >>> Provide a small convenience wrapper that set/get the >>> backlight brightness control and creates the backlight >>> device for the panel interface >> >> To be pedantic, we should downplay "backlight" in the DSI DCS brightness >> control... there need not be a backlight, at all, for brightness control >> (see AMOLED). > but this jdi display and few other dsi display can be controlled by > dcs commands The point I was trying to convey was that the DSI DCS interface is agnostic to the actual method of controlling brightness. Whether the panel has a backlight to control brightness is a panel implementation detail. So maybe we shouldn't have "backlight" in the interface nomenclature either. BR, Jani.
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 2f0b85c..ac4521e 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -1026,6 +1026,69 @@ int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format) } EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format); +static int dsi_dcs_bl_get_brightness(struct backlight_device *bl) +{ + struct mipi_dsi_device *dsi = bl_get_data(bl); + int ret; + u8 brightness; + + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; + + ret = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_DISPLAY_BRIGHTNESS, + &brightness, sizeof(brightness)); + if (ret < 0) + return ret; + + dsi->mode_flags |= MIPI_DSI_MODE_LPM; + + return brightness; +} + +static int dsi_dcs_bl_update_status(struct backlight_device *bl) +{ + struct mipi_dsi_device *dsi = bl_get_data(bl); + int ret; + u8 brightness = bl->props.brightness; + + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; + + ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, + &brightness, sizeof(brightness)); + if (ret < 0) + return ret; + + dsi->mode_flags |= MIPI_DSI_MODE_LPM; + + return 0; +} + +static const struct backlight_ops dsi_bl_ops = { + .update_status = dsi_dcs_bl_update_status, + .get_brightness = dsi_dcs_bl_get_brightness, +}; + +/** + * drm_panel_create_dsi_backlight() - creates the backlight device for the panel + * @dsi: DSI peripheral device + * + * @return a struct backlight on success, or an ERR_PTR on error + */ +struct backlight_device + *drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi) +{ + struct device *dev = &dsi->dev; + struct backlight_properties props; + + memset(&props, 0, sizeof(props)); + props.type = BACKLIGHT_RAW; + props.brightness = 255; + props.max_brightness = 255; + + return devm_backlight_device_register(dev, dev_name(dev), dev, dsi, + &dsi_bl_ops, &props); +} +EXPORT_SYMBOL(drm_panel_create_dsi_backlight); + static int mipi_dsi_drv_probe(struct device *dev) { struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver); diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index 2788dbe..9dd6a97 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -12,6 +12,7 @@ #ifndef __DRM_MIPI_DSI_H__ #define __DRM_MIPI_DSI_H__ +#include <linux/backlight.h> #include <linux/device.h> struct mipi_dsi_host; @@ -269,6 +270,8 @@ 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); +struct backlight_device + *drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi); /** * struct mipi_dsi_driver - DSI driver
Provide a small convenience wrapper that set/get the backlight brightness control and creates the backlight device for the panel interface Cc: John Stultz <john.stultz@linaro.org> Cc: Sumit Semwal <sumit.semwal@linaro.org> Cc: Archit Taneja <archit.taneja@gmail.com> Cc: Rob Clark <robdclark@gmail.com> Signed-off-by: Vinay Simha BN <simhavcs@gmail.com> -- v1: *tested in nexus7 2nd gen. --- drivers/gpu/drm/drm_mipi_dsi.c | 63 ++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_mipi_dsi.h | 3 ++ 2 files changed, 66 insertions(+)