From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752103AbcFFHhU (ORCPT ); Mon, 6 Jun 2016 03:37:20 -0400 Received: from mga04.intel.com ([192.55.52.120]:14276 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751702AbcFFHhS (ORCPT ); Mon, 6 Jun 2016 03:37:18 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,426,1459839600"; d="scan'208";a="995995772" From: Jani Nikula To: Vinay Simha BN Cc: open list , "open list\:DRM DRIVERS" , Vinay Simha BN , Archit Taneja Subject: Re: [PATCH 2/2] drm/dsi: Implement dcs backlight brightness In-Reply-To: <1464832099-22402-2-git-send-email-simhavcs@gmail.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <1464832099-22402-1-git-send-email-simhavcs@gmail.com> <1464832099-22402-2-git-send-email-simhavcs@gmail.com> User-Agent: Notmuch/0.22+12~gbdd9442 (http://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu) Date: Mon, 06 Jun 2016 10:37:15 +0300 Message-ID: <87eg8a7opg.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 02 Jun 2016, Vinay Simha BN 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 > Cc: Sumit Semwal > Cc: Archit Taneja > Cc: Rob Clark > Signed-off-by: Vinay Simha BN > > -- > 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 > #include > > 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