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