diff mbox series

[2/2] drm/bridge: ti-sn65dsi86: Expose backlight controls

Message ID 20200930223532.77755-3-bjorn.andersson@linaro.org (mailing list archive)
State New, archived
Headers show
Series drm/bridge: ti-sn65dsi86: Support backlight controls | expand

Commit Message

Bjorn Andersson Sept. 30, 2020, 10:35 p.m. UTC
The TI SN65DSI86 can be configured to generate a PWM pulse on GPIO4,
to be used to drive a backlight driver.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/bridge/Kconfig        |   1 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 143 +++++++++++++++++++++++++-
 2 files changed, 140 insertions(+), 4 deletions(-)

Comments

Steev Klimaszewski Sept. 30, 2020, 11:07 p.m. UTC | #1
On 9/30/20 5:35 PM, Bjorn Andersson wrote:
> The TI SN65DSI86 can be configured to generate a PWM pulse on GPIO4,
> to be used to drive a backlight driver.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   1 +
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 143 +++++++++++++++++++++++++-
>  2 files changed, 140 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 43271c21d3fc..eea310bd88e1 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -195,6 +195,7 @@ config DRM_TI_SN65DSI86
>  	select REGMAP_I2C
>  	select DRM_PANEL
>  	select DRM_MIPI_DSI
> +	select BACKLIGHT_CLASS_DEVICE
>  	help
>  	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
>  
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 5b6e19ecbc84..41e24d0dbd18 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -68,6 +68,7 @@
>  #define  SN_GPIO_MUX_OUTPUT			1
>  #define  SN_GPIO_MUX_SPECIAL			2
>  #define  SN_GPIO_MUX_MASK			0x3
> +#define  SN_GPIO_MUX_SHIFT(gpio)		((gpio) * 2)
>  #define SN_AUX_WDATA_REG(x)			(0x64 + (x))
>  #define SN_AUX_ADDR_19_16_REG			0x74
>  #define SN_AUX_ADDR_15_8_REG			0x75
> @@ -86,6 +87,12 @@
>  #define SN_ML_TX_MODE_REG			0x96
>  #define  ML_TX_MAIN_LINK_OFF			0
>  #define  ML_TX_NORMAL_MODE			BIT(0)
> +#define SN_PWM_PRE_DIV_REG			0xA0
> +#define SN_BACKLIGHT_SCALE_REG			0xA1
> +#define SN_BACKLIGHT_REG			0xA3
> +#define SN_PWM_CTL_REG				0xA5
> +#define  SN_PWM_ENABLE				BIT(1)
> +#define  SN_PWM_INVERT				BIT(0)
>  #define SN_AUX_CMD_STATUS_REG			0xF4
>  #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
>  #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
> @@ -155,6 +162,8 @@ struct ti_sn_bridge {
>  	struct gpio_chip		gchip;
>  	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
>  #endif
> +	u32				brightness;
> +	u32				max_brightness;
>  };
>  
>  static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> @@ -173,6 +182,18 @@ static const struct regmap_config ti_sn_bridge_regmap_config = {
>  	.cache_type = REGCACHE_NONE,
>  };
>  
> +static void ti_sn_bridge_read_u16(struct ti_sn_bridge *pdata,
> +				  unsigned int reg, u16 *val)
> +{
> +	unsigned int high;
> +	unsigned int low;
> +
> +	regmap_read(pdata->regmap, reg, &low);
> +	regmap_read(pdata->regmap, reg + 1, &high);
> +
> +	*val = high << 8 | low;
> +}
> +
>  static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
>  				   unsigned int reg, u16 val)
>  {
> @@ -180,6 +201,50 @@ static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
>  	regmap_write(pdata->regmap, reg + 1, val >> 8);
>  }
>  
> +static int ti_sn_backlight_update(struct ti_sn_bridge *pdata)
> +{
> +	unsigned int pre_div;
> +
> +	if (!pdata->max_brightness)
> +		return 0;
> +
> +	/* Enable PWM on GPIO4 */
> +	regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> +			   SN_GPIO_MUX_MASK << SN_GPIO_MUX_SHIFT(4 - 1),
> +			   SN_GPIO_MUX_SPECIAL << SN_GPIO_MUX_SHIFT(4 - 1));
> +
> +	if (pdata->brightness) {
> +		/* Set max brightness */
> +		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, pdata->max_brightness);
> +
> +		/* Set brightness */
> +		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, pdata->brightness);
> +
> +		/*
> +		 * The PWM frequency is derived from the refclk as:
> +		 * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
> +		 *
> +		 * A hand wavy estimate based on 12MHz refclk and 500Hz desired
> +		 * PWM frequency gives us a pre_div resulting in a PWM
> +		 * frequency of between 500 and 1600Hz, depending on the actual
> +		 * refclk rate.
> +		 *
> +		 * One is added to avoid high BACKLIGHT_SCALE values to produce
> +		 * a pre_div of 0 - which cancels out the large BACKLIGHT_SCALE
> +		 * value.
> +		 */
> +		pre_div = 12000000 / (500 * pdata->max_brightness) + 1;
> +		regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
> +
> +		/* Enable PWM */
> +		regmap_update_bits(pdata->regmap, SN_PWM_CTL_REG, SN_PWM_ENABLE, SN_PWM_ENABLE);
> +	} else {
> +		regmap_update_bits(pdata->regmap, SN_PWM_CTL_REG, SN_PWM_ENABLE, 0);
> +	}
> +
> +	return 0;
> +}
> +
>  static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
>  {
>  	struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
> @@ -193,7 +258,7 @@ static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
>  
>  	gpiod_set_value(pdata->enable_gpio, 1);
>  
> -	return ret;
> +	return ti_sn_backlight_update(pdata);
>  }
>  
>  static int __maybe_unused ti_sn_bridge_suspend(struct device *dev)
> @@ -1010,7 +1075,7 @@ static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip,
>  					     unsigned int offset)
>  {
>  	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
> -	int shift = offset * 2;
> +	int shift = SN_GPIO_MUX_SHIFT(offset);
>  	int ret;
>  
>  	if (!test_and_clear_bit(offset, pdata->gchip_output))
> @@ -1038,7 +1103,7 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
>  					      unsigned int offset, int val)
>  {
>  	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
> -	int shift = offset * 2;
> +	int shift = SN_GPIO_MUX_SHIFT(offset);
>  	int ret;
>  
>  	if (test_and_set_bit(offset, pdata->gchip_output))
> @@ -1073,12 +1138,17 @@ static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = {
>  
>  static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
>  {
> +	int ngpio = SN_NUM_GPIOS;
>  	int ret;
>  
>  	/* Only init if someone is going to use us as a GPIO controller */
>  	if (!of_property_read_bool(pdata->dev->of_node, "gpio-controller"))
>  		return 0;
>  
> +	/* If GPIO4 is used for backlight, reduce number of gpios */
> +	if (pdata->max_brightness)
> +		ngpio--;
> +
>  	pdata->gchip.label = dev_name(pdata->dev);
>  	pdata->gchip.parent = pdata->dev;
>  	pdata->gchip.owner = THIS_MODULE;
> @@ -1092,7 +1162,7 @@ static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
>  	pdata->gchip.set = ti_sn_bridge_gpio_set;
>  	pdata->gchip.can_sleep = true;
>  	pdata->gchip.names = ti_sn_bridge_gpio_names;
> -	pdata->gchip.ngpio = SN_NUM_GPIOS;
> +	pdata->gchip.ngpio = ngpio;
>  	pdata->gchip.base = -1;
>  	ret = devm_gpiochip_add_data(pdata->dev, &pdata->gchip, pdata);
>  	if (ret)
> @@ -1159,6 +1229,65 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn_bridge *pdata,
>  	pdata->ln_polrs = ln_polrs;
>  }
>  
> +static int ti_sn_backlight_update_status(struct backlight_device *bl)
> +{
> +	struct ti_sn_bridge *pdata = bl_get_data(bl);
> +	int brightness = bl->props.brightness;
> +
> +	if (bl->props.power != FB_BLANK_UNBLANK ||
> +	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
> +	    bl->props.state & BL_CORE_FBBLANK) {
> +		pdata->brightness = 0;
> +	}
> +
> +	pdata->brightness = brightness;
> +
> +	return ti_sn_backlight_update(pdata);
> +}
> +
> +static int ti_sn_backlight_get_brightness(struct backlight_device *bl)
> +{
> +	struct ti_sn_bridge *pdata = bl_get_data(bl);
> +	u16 val;
> +
> +	ti_sn_bridge_read_u16(pdata, SN_BACKLIGHT_REG, &val);
> +
> +	return val;
> +}
> +
> +const struct backlight_ops ti_sn_backlight_ops = {
> +	.update_status = ti_sn_backlight_update_status,
> +	.get_brightness = ti_sn_backlight_get_brightness,
> +};
> +
> +static int ti_sn_backlight_init(struct ti_sn_bridge *pdata)
> +{
> +	struct backlight_properties props = {};
> +	struct backlight_device	*bl;
> +	struct device *dev = pdata->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "ti,backlight-scale", &pdata->max_brightness);
> +	if (ret == -EINVAL) {
> +		return 0;
> +	} else if (ret || pdata->max_brightness >= 0xffff) {
> +		DRM_ERROR("invalid max-brightness\n");
> +		return -EINVAL;
> +	}
> +
> +	props.type = BACKLIGHT_RAW;
> +	props.max_brightness = pdata->max_brightness;
> +	bl = devm_backlight_device_register(dev, "sn65dsi86", dev, pdata,
> +					    &ti_sn_backlight_ops, &props);
> +	if (IS_ERR(bl)) {
> +		DRM_ERROR("failed to register backlight device\n");
> +		return PTR_ERR(bl);
> +	}
> +
> +	return 0;
> +}
> +
>  static int ti_sn_bridge_probe(struct i2c_client *client,
>  			      const struct i2c_device_id *id)
>  {
> @@ -1224,6 +1353,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>  
>  	pm_runtime_enable(pdata->dev);
>  
> +	ret = ti_sn_backlight_init(pdata);
> +	if (ret) {
> +		pm_runtime_disable(pdata->dev);
> +		return ret;
> +	}
> +
>  	ret = ti_sn_setup_gpio_controller(pdata);
>  	if (ret) {
>  		pm_runtime_disable(pdata->dev);


Tested-By: Steev Klimaszewski <steev@kali.org>
Doug Anderson Oct. 2, 2020, 8:42 p.m. UTC | #2
Hi,

On Wed, Sep 30, 2020 at 3:40 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> The TI SN65DSI86 can be configured to generate a PWM pulse on GPIO4,
> to be used to drive a backlight driver.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   1 +
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 143 +++++++++++++++++++++++++-
>  2 files changed, 140 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 43271c21d3fc..eea310bd88e1 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -195,6 +195,7 @@ config DRM_TI_SN65DSI86
>         select REGMAP_I2C
>         select DRM_PANEL
>         select DRM_MIPI_DSI
> +       select BACKLIGHT_CLASS_DEVICE
>         help
>           Texas Instruments SN65DSI86 DSI to eDP Bridge driver
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 5b6e19ecbc84..41e24d0dbd18 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -68,6 +68,7 @@
>  #define  SN_GPIO_MUX_OUTPUT                    1
>  #define  SN_GPIO_MUX_SPECIAL                   2
>  #define  SN_GPIO_MUX_MASK                      0x3
> +#define  SN_GPIO_MUX_SHIFT(gpio)               ((gpio) * 2)
>  #define SN_AUX_WDATA_REG(x)                    (0x64 + (x))
>  #define SN_AUX_ADDR_19_16_REG                  0x74
>  #define SN_AUX_ADDR_15_8_REG                   0x75
> @@ -86,6 +87,12 @@
>  #define SN_ML_TX_MODE_REG                      0x96
>  #define  ML_TX_MAIN_LINK_OFF                   0
>  #define  ML_TX_NORMAL_MODE                     BIT(0)
> +#define SN_PWM_PRE_DIV_REG                     0xA0
> +#define SN_BACKLIGHT_SCALE_REG                 0xA1
> +#define SN_BACKLIGHT_REG                       0xA3
> +#define SN_PWM_CTL_REG                         0xA5
> +#define  SN_PWM_ENABLE                         BIT(1)
> +#define  SN_PWM_INVERT                         BIT(0)
>  #define SN_AUX_CMD_STATUS_REG                  0xF4
>  #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT          BIT(3)
>  #define  AUX_IRQ_STATUS_AUX_SHORT              BIT(5)
> @@ -155,6 +162,8 @@ struct ti_sn_bridge {
>         struct gpio_chip                gchip;
>         DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
>  #endif
> +       u32                             brightness;
> +       u32                             max_brightness;

You missed adding to the docstring for brightness and max_brightness.

Also: why do you need your own copy of these two values?  Couldn't you
just store the "struct backlight_device *" that came back from
"devm_backlight_device_register()" and then reference
bl->props.brightness / bl->props.max_brightness?


>  };
>
>  static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> @@ -173,6 +182,18 @@ static const struct regmap_config ti_sn_bridge_regmap_config = {
>         .cache_type = REGCACHE_NONE,
>  };
>
> +static void ti_sn_bridge_read_u16(struct ti_sn_bridge *pdata,
> +                                 unsigned int reg, u16 *val)
> +{
> +       unsigned int high;
> +       unsigned int low;
> +
> +       regmap_read(pdata->regmap, reg, &low);
> +       regmap_read(pdata->regmap, reg + 1, &high);
> +
> +       *val = high << 8 | low;
> +}

Ideally you should be error checking your reads.  I know this driver
isn't very good about error checking the regmap reads in general, but
probably that should be fixed.  Certainly i2c-backed regmaps can have
failures and you will then do your math on whatever uninitialized
memory was on the stack.  That seems bad.

Presumably you'll then want to return the error code from this
function?  If for some reason you don't, your function should just
return the val instead of passing by reference.


>  static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
>                                    unsigned int reg, u16 val)
>  {
> @@ -180,6 +201,50 @@ static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
>         regmap_write(pdata->regmap, reg + 1, val >> 8);
>  }
>
> +static int ti_sn_backlight_update(struct ti_sn_bridge *pdata)
> +{
> +       unsigned int pre_div;
> +
> +       if (!pdata->max_brightness)
> +               return 0;
> +
> +       /* Enable PWM on GPIO4 */
> +       regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> +                          SN_GPIO_MUX_MASK << SN_GPIO_MUX_SHIFT(4 - 1),
> +                          SN_GPIO_MUX_SPECIAL << SN_GPIO_MUX_SHIFT(4 - 1));
> +
> +       if (pdata->brightness) {
> +               /* Set max brightness */
> +               ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, pdata->max_brightness);
> +
> +               /* Set brightness */
> +               ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, pdata->brightness);
> +
> +               /*
> +                * The PWM frequency is derived from the refclk as:
> +                * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
> +                *
> +                * A hand wavy estimate based on 12MHz refclk and 500Hz desired
> +                * PWM frequency gives us a pre_div resulting in a PWM
> +                * frequency of between 500 and 1600Hz, depending on the actual
> +                * refclk rate.
> +                *
> +                * One is added to avoid high BACKLIGHT_SCALE values to produce
> +                * a pre_div of 0 - which cancels out the large BACKLIGHT_SCALE
> +                * value.
> +                */
> +               pre_div = 12000000 / (500 * pdata->max_brightness) + 1;
> +               regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);

Different panels have different requirements for PWM frequency.  Some
may also have different duty-cycle to brightness curves that differ
based on the PWM frequency and it would be nice to make sure we know
what frequency we're at rather than getting something random-ish.  It
feels like you need to be less hand-wavy.  You should presumably
specify the desired frequency in the device tree and then do the math.


> +               /* Enable PWM */
> +               regmap_update_bits(pdata->regmap, SN_PWM_CTL_REG, SN_PWM_ENABLE, SN_PWM_ENABLE);
> +       } else {
> +               regmap_update_bits(pdata->regmap, SN_PWM_CTL_REG, SN_PWM_ENABLE, 0);
> +       }

While technically it works OK to conflate brightness = 0 with
backlight disabled (the PWM driver exposed by the Chrome OS EC does,
at least), I believe the API in Linux does make a difference.  Why not
match the Linux API.  If Linux says that the backlight should be at
brightness 50 but should be off, set the brightness to 50 and turn the
backlight off.  If it says set the brightness to 0 and turn it on,
honor it.

I believe (but haven't tested) one side effect of the way you're doing
is is that:

set_brightness(50)
blank()
unblank()
get_brightness()

...will return 0, not 50.  I believe (but haven't tested) that if you
don't implement get_brightness() it would fix things,


> +static int ti_sn_backlight_update_status(struct backlight_device *bl)
> +{
> +       struct ti_sn_bridge *pdata = bl_get_data(bl);
> +       int brightness = bl->props.brightness;
> +
> +       if (bl->props.power != FB_BLANK_UNBLANK ||
> +           bl->props.fb_blank != FB_BLANK_UNBLANK ||
> +           bl->props.state & BL_CORE_FBBLANK) {

backlight_is_blank() instead of open-coding?  ...or you somehow don't
want the extra test for "BL_CORE_SUSPENDED" ?


> +               pdata->brightness = 0;

As per comments in ti_sn_backlight_update(), IMO you want to keep
enabled / disabled state separate from brightness.


> +       }
> +
> +       pdata->brightness = brightness;
> +
> +       return ti_sn_backlight_update(pdata);
> +}

Just to be neat and tidy, I'd expect something in the above would do a
pm_runtime_get_sync() when the backlight first turns on and
pm_runtime_put() when the backlight goes blank.  Right now you're
relying on the fact that the backlight is usually turned on later in
the sequence, but it shouldn't hurt to add an extra pm_runtime
reference and means you're no longer relying on the implicitness.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 43271c21d3fc..eea310bd88e1 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -195,6 +195,7 @@  config DRM_TI_SN65DSI86
 	select REGMAP_I2C
 	select DRM_PANEL
 	select DRM_MIPI_DSI
+	select BACKLIGHT_CLASS_DEVICE
 	help
 	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
 
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 5b6e19ecbc84..41e24d0dbd18 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -68,6 +68,7 @@ 
 #define  SN_GPIO_MUX_OUTPUT			1
 #define  SN_GPIO_MUX_SPECIAL			2
 #define  SN_GPIO_MUX_MASK			0x3
+#define  SN_GPIO_MUX_SHIFT(gpio)		((gpio) * 2)
 #define SN_AUX_WDATA_REG(x)			(0x64 + (x))
 #define SN_AUX_ADDR_19_16_REG			0x74
 #define SN_AUX_ADDR_15_8_REG			0x75
@@ -86,6 +87,12 @@ 
 #define SN_ML_TX_MODE_REG			0x96
 #define  ML_TX_MAIN_LINK_OFF			0
 #define  ML_TX_NORMAL_MODE			BIT(0)
+#define SN_PWM_PRE_DIV_REG			0xA0
+#define SN_BACKLIGHT_SCALE_REG			0xA1
+#define SN_BACKLIGHT_REG			0xA3
+#define SN_PWM_CTL_REG				0xA5
+#define  SN_PWM_ENABLE				BIT(1)
+#define  SN_PWM_INVERT				BIT(0)
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
@@ -155,6 +162,8 @@  struct ti_sn_bridge {
 	struct gpio_chip		gchip;
 	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
 #endif
+	u32				brightness;
+	u32				max_brightness;
 };
 
 static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
@@ -173,6 +182,18 @@  static const struct regmap_config ti_sn_bridge_regmap_config = {
 	.cache_type = REGCACHE_NONE,
 };
 
+static void ti_sn_bridge_read_u16(struct ti_sn_bridge *pdata,
+				  unsigned int reg, u16 *val)
+{
+	unsigned int high;
+	unsigned int low;
+
+	regmap_read(pdata->regmap, reg, &low);
+	regmap_read(pdata->regmap, reg + 1, &high);
+
+	*val = high << 8 | low;
+}
+
 static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
 				   unsigned int reg, u16 val)
 {
@@ -180,6 +201,50 @@  static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
 	regmap_write(pdata->regmap, reg + 1, val >> 8);
 }
 
+static int ti_sn_backlight_update(struct ti_sn_bridge *pdata)
+{
+	unsigned int pre_div;
+
+	if (!pdata->max_brightness)
+		return 0;
+
+	/* Enable PWM on GPIO4 */
+	regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
+			   SN_GPIO_MUX_MASK << SN_GPIO_MUX_SHIFT(4 - 1),
+			   SN_GPIO_MUX_SPECIAL << SN_GPIO_MUX_SHIFT(4 - 1));
+
+	if (pdata->brightness) {
+		/* Set max brightness */
+		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, pdata->max_brightness);
+
+		/* Set brightness */
+		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, pdata->brightness);
+
+		/*
+		 * The PWM frequency is derived from the refclk as:
+		 * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
+		 *
+		 * A hand wavy estimate based on 12MHz refclk and 500Hz desired
+		 * PWM frequency gives us a pre_div resulting in a PWM
+		 * frequency of between 500 and 1600Hz, depending on the actual
+		 * refclk rate.
+		 *
+		 * One is added to avoid high BACKLIGHT_SCALE values to produce
+		 * a pre_div of 0 - which cancels out the large BACKLIGHT_SCALE
+		 * value.
+		 */
+		pre_div = 12000000 / (500 * pdata->max_brightness) + 1;
+		regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
+
+		/* Enable PWM */
+		regmap_update_bits(pdata->regmap, SN_PWM_CTL_REG, SN_PWM_ENABLE, SN_PWM_ENABLE);
+	} else {
+		regmap_update_bits(pdata->regmap, SN_PWM_CTL_REG, SN_PWM_ENABLE, 0);
+	}
+
+	return 0;
+}
+
 static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
 {
 	struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
@@ -193,7 +258,7 @@  static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
 
 	gpiod_set_value(pdata->enable_gpio, 1);
 
-	return ret;
+	return ti_sn_backlight_update(pdata);
 }
 
 static int __maybe_unused ti_sn_bridge_suspend(struct device *dev)
@@ -1010,7 +1075,7 @@  static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip,
 					     unsigned int offset)
 {
 	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
-	int shift = offset * 2;
+	int shift = SN_GPIO_MUX_SHIFT(offset);
 	int ret;
 
 	if (!test_and_clear_bit(offset, pdata->gchip_output))
@@ -1038,7 +1103,7 @@  static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
 					      unsigned int offset, int val)
 {
 	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
-	int shift = offset * 2;
+	int shift = SN_GPIO_MUX_SHIFT(offset);
 	int ret;
 
 	if (test_and_set_bit(offset, pdata->gchip_output))
@@ -1073,12 +1138,17 @@  static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = {
 
 static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
 {
+	int ngpio = SN_NUM_GPIOS;
 	int ret;
 
 	/* Only init if someone is going to use us as a GPIO controller */
 	if (!of_property_read_bool(pdata->dev->of_node, "gpio-controller"))
 		return 0;
 
+	/* If GPIO4 is used for backlight, reduce number of gpios */
+	if (pdata->max_brightness)
+		ngpio--;
+
 	pdata->gchip.label = dev_name(pdata->dev);
 	pdata->gchip.parent = pdata->dev;
 	pdata->gchip.owner = THIS_MODULE;
@@ -1092,7 +1162,7 @@  static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
 	pdata->gchip.set = ti_sn_bridge_gpio_set;
 	pdata->gchip.can_sleep = true;
 	pdata->gchip.names = ti_sn_bridge_gpio_names;
-	pdata->gchip.ngpio = SN_NUM_GPIOS;
+	pdata->gchip.ngpio = ngpio;
 	pdata->gchip.base = -1;
 	ret = devm_gpiochip_add_data(pdata->dev, &pdata->gchip, pdata);
 	if (ret)
@@ -1159,6 +1229,65 @@  static void ti_sn_bridge_parse_lanes(struct ti_sn_bridge *pdata,
 	pdata->ln_polrs = ln_polrs;
 }
 
+static int ti_sn_backlight_update_status(struct backlight_device *bl)
+{
+	struct ti_sn_bridge *pdata = bl_get_data(bl);
+	int brightness = bl->props.brightness;
+
+	if (bl->props.power != FB_BLANK_UNBLANK ||
+	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
+	    bl->props.state & BL_CORE_FBBLANK) {
+		pdata->brightness = 0;
+	}
+
+	pdata->brightness = brightness;
+
+	return ti_sn_backlight_update(pdata);
+}
+
+static int ti_sn_backlight_get_brightness(struct backlight_device *bl)
+{
+	struct ti_sn_bridge *pdata = bl_get_data(bl);
+	u16 val;
+
+	ti_sn_bridge_read_u16(pdata, SN_BACKLIGHT_REG, &val);
+
+	return val;
+}
+
+const struct backlight_ops ti_sn_backlight_ops = {
+	.update_status = ti_sn_backlight_update_status,
+	.get_brightness = ti_sn_backlight_get_brightness,
+};
+
+static int ti_sn_backlight_init(struct ti_sn_bridge *pdata)
+{
+	struct backlight_properties props = {};
+	struct backlight_device	*bl;
+	struct device *dev = pdata->dev;
+	struct device_node *np = dev->of_node;
+	int ret;
+
+	ret = of_property_read_u32(np, "ti,backlight-scale", &pdata->max_brightness);
+	if (ret == -EINVAL) {
+		return 0;
+	} else if (ret || pdata->max_brightness >= 0xffff) {
+		DRM_ERROR("invalid max-brightness\n");
+		return -EINVAL;
+	}
+
+	props.type = BACKLIGHT_RAW;
+	props.max_brightness = pdata->max_brightness;
+	bl = devm_backlight_device_register(dev, "sn65dsi86", dev, pdata,
+					    &ti_sn_backlight_ops, &props);
+	if (IS_ERR(bl)) {
+		DRM_ERROR("failed to register backlight device\n");
+		return PTR_ERR(bl);
+	}
+
+	return 0;
+}
+
 static int ti_sn_bridge_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -1224,6 +1353,12 @@  static int ti_sn_bridge_probe(struct i2c_client *client,
 
 	pm_runtime_enable(pdata->dev);
 
+	ret = ti_sn_backlight_init(pdata);
+	if (ret) {
+		pm_runtime_disable(pdata->dev);
+		return ret;
+	}
+
 	ret = ti_sn_setup_gpio_controller(pdata);
 	if (ret) {
 		pm_runtime_disable(pdata->dev);