diff mbox series

[RESEND,14/14] video: backlight: mt6370: Add Mediatek MT6370 support

Message ID 20220531111900.19422-15-peterwu.pub@gmail.com (mailing list archive)
State Handled Elsewhere
Headers show
Series Add Mediatek MT6370 PMIC support | expand

Commit Message

ChiaEn Wu May 31, 2022, 11:19 a.m. UTC
From: ChiaEn Wu <chiaen_wu@richtek.com>

Add Mediatek MT6370 Backlight support.

Signed-off-by: ChiaEn Wu <chiaen_wu@richtek.com>
---
 drivers/video/backlight/Kconfig            |   8 +
 drivers/video/backlight/Makefile           |   1 +
 drivers/video/backlight/mt6370-backlight.c | 338 +++++++++++++++++++++
 3 files changed, 347 insertions(+)
 create mode 100644 drivers/video/backlight/mt6370-backlight.c

Comments

Daniel Thompson June 1, 2022, 9:46 a.m. UTC | #1
On Tue, May 31, 2022 at 07:19:00PM +0800, ChiaEn Wu wrote:
> From: ChiaEn Wu <chiaen_wu@richtek.com>
> 
> Add Mediatek MT6370 Backlight support.
> 
> Signed-off-by: ChiaEn Wu <chiaen_wu@richtek.com>
> ---
>  drivers/video/backlight/Kconfig            |   8 +
>  drivers/video/backlight/Makefile           |   1 +
>  drivers/video/backlight/mt6370-backlight.c | 338 +++++++++++++++++++++
>  3 files changed, 347 insertions(+)
>  create mode 100644 drivers/video/backlight/mt6370-backlight.c
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index a003e02e13ce..d9868fbe7488 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -268,6 +268,14 @@ config BACKLIGHT_MAX8925
>  	  If you have a LCD backlight connected to the WLED output of MAX8925
>  	  WLED output, say Y here to enable this driver.
>  
> +config BACKLIGHT_MT6370

Is MT6370 really the best name for this driver? In other words, you
don't expect there to be any family resemblance between this backlight
and the backlight in the *next* Mediatak PMIC?

Moreover, 


> +	tristate "Mediatek MT6370 Backlight Driver"
> +	depends on MFD_MT6370
> +	help
> +	  Say Y here to enable MT6370 Backlight support.
> +	  It's commonly used to drive the display WLED. There're 4 channels
> +	  inisde, and each channel can provide up to 30mA current.

Nitpicking but this doesn't align well with other help texts in this
file.


> +
>  config BACKLIGHT_APPLE
>  	tristate "Apple Backlight Driver"
>  	depends on X86 && ACPI
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index cae2c83422ae..e815f3f1deff 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -44,6 +44,7 @@ obj-$(CONFIG_BACKLIGHT_LP855X)		+= lp855x_bl.o
>  obj-$(CONFIG_BACKLIGHT_LP8788)		+= lp8788_bl.o
>  obj-$(CONFIG_BACKLIGHT_LV5207LP)	+= lv5207lp.o
>  obj-$(CONFIG_BACKLIGHT_MAX8925)		+= max8925_bl.o
> +obj-$(CONFIG_BACKLIGHT_MT6370)		+= mt6370-backlight.o
>  obj-$(CONFIG_BACKLIGHT_OMAP1)		+= omap1_bl.o
>  obj-$(CONFIG_BACKLIGHT_PANDORA)		+= pandora_bl.o
>  obj-$(CONFIG_BACKLIGHT_PCF50633)	+= pcf50633-backlight.o
> diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c
> new file mode 100644
> index 000000000000..f8a8d33203ed
> --- /dev/null
> +++ b/drivers/video/backlight/mt6370-backlight.c
> @@ -0,0 +1,338 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/backlight.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define MT6370_REG_DEV_INFO		0x100
> +#define MT6370_REG_BL_EN		0x1A0
> +#define MT6370_REG_BL_BSTCTRL		0x1A1
> +#define MT6370_REG_BL_PWM		0x1A2
> +#define MT6370_REG_BL_DIM2		0x1A4
> +
> +#define MT6370_VENID_MASK		GENMASK(7, 4)
> +#define MT6370_BL_EXT_EN_MASK		BIT(7)
> +#define MT6370_BL_EN_MASK		BIT(6)
> +#define MT6370_BL_CONFIG_MASK		BIT(0)
> +#define MT6370_BL_CH_MASK		GENMASK(5, 2)
> +#define MT6370_BL_DIM2_MASK		GENMASK(2, 0)
> +#define MT6370_BL_DUMMY_6372_MASK	GENMASK(2, 0)
> +#define MT6370_BL_DIM2_6372_SHIFT	3
> +#define MT6370_BL_PWM_EN_MASK		BIT(7)
> +#define MT6370_BL_PWM_HYS_EN_MASK	BIT(2)
> +#define MT6370_BL_PWM_HYS_SEL_MASK	GENMASK(1, 0)
> +#define MT6370_BL_OVP_EN_MASK		BIT(7)
> +#define MT6370_BL_OVP_SEL_MASK		GENMASK(6, 5)
> +#define MT6370_BL_OC_EN_MASK		BIT(3)
> +#define MT6370_BL_OC_SEL_MASK		GENMASK(2, 1)
> +
> +#define MT6370_BL_MAX_BRIGHTNESS	2048
> +
> +enum {
> +	MT6370_VID_COMMON = 0,
> +	MT6370_VID_6372,
> +	MT6370_VID_MAX,

Unused.

> +};
> +
> +enum mt6370_prop_type {
> +	MT6370_PARSE_TYPE_BOOL = 0,
> +	MT6370_PARSE_TYPE_U8,
> +	MT6370_PARSE_TYPE_MAX,

Unused.

> +};
> +
> +struct mt6370_priv {
> +	int vid_type;
> +	struct backlight_device *bl;
> +	struct device *dev;
> +	struct gpio_desc *enable_gpio;
> +	struct regmap *regmap;
> +};
> +
> +static int mt6370_bl_update_status(struct backlight_device *bl_dev)
> +{
> +	struct mt6370_priv *priv = bl_get_data(bl_dev);
> +	int brightness = backlight_get_brightness(bl_dev);
> +	unsigned int enable_val;
> +	u8 brightness_val[2];
> +	int ret;
> +
> +	if (brightness) {
> +		brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK;
> +		brightness_val[1] = (brightness - 1)
> +					>> fls(MT6370_BL_DIM2_MASK);
> +
> +		if (priv->vid_type == MT6370_VID_6372) {
> +			brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT;
> +			brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK;

Comment explaining why we have to set these bits would be useful.


> +		}
> +
> +		ret = regmap_raw_write(priv->regmap, MT6370_REG_BL_DIM2,
> +				       brightness_val, sizeof(brightness_val));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (priv->enable_gpio)
> +		gpiod_set_value(priv->enable_gpio, brightness ? 1 : 0);
> +
> +	enable_val = brightness ? MT6370_BL_EN_MASK : 0;
> +	return regmap_update_bits(priv->regmap, MT6370_REG_BL_EN,
> +				  MT6370_BL_EN_MASK, enable_val);
> +}
> +

<snip>

> +#define MT6370_DT_PROP_DECL(_name, _type, _reg, _mask, _max, _inv)	\
> +{									\
> +	.name = "mediatek,bled-" #_name,				\

I'd rather have the whole DT property in the macro (because it helps
with grepability).


> +	.type = MT6370_PARSE_TYPE_##_type,				\
> +	.reg = _reg,							\
> +	.mask = _mask,							\
> +	.max_val = _max,						\
> +	.invert = _inv,							\
> +}
> +
> +static int mt6370_init_backlight_properties(struct mt6370_priv *priv,
> +					    struct backlight_properties *props)
> +{
> +	struct device *dev = priv->dev;
> +	u8 prop_val;
> +	u32 brightness;
> +	unsigned int mask, val;
> +	static const struct {
> +		char *name;
> +		enum mt6370_prop_type type;
> +		unsigned int reg;
> +		unsigned int mask;
> +		u8 max_val;
> +		bool invert;
> +	} vendor_opt_props[] = {
> +		MT6370_DT_PROP_DECL(pwm-enable, BOOL, MT6370_REG_BL_PWM,
> +				    MT6370_BL_PWM_EN_MASK, 1, false),
> +		MT6370_DT_PROP_DECL(pwm-hys-enable, BOOL, MT6370_REG_BL_PWM,
> +				    MT6370_BL_PWM_HYS_EN_MASK, 1, false),
> +		MT6370_DT_PROP_DECL(pwm-hys-sel, U8, MT6370_REG_BL_PWM,
> +				    MT6370_BL_PWM_HYS_SEL_MASK, 3, false),
> +		MT6370_DT_PROP_DECL(ovp-level-sel, U8, MT6370_REG_BL_BSTCTRL,
> +				    MT6370_BL_OVP_SEL_MASK, 3, false),
> +		MT6370_DT_PROP_DECL(ovp-shutdown, BOOL, MT6370_REG_BL_BSTCTRL,
> +				    MT6370_BL_OVP_EN_MASK, 1, true),
> +		MT6370_DT_PROP_DECL(ocp-level-sel, U8, MT6370_REG_BL_BSTCTRL,
> +				    MT6370_BL_OC_SEL_MASK, 3, false),
> +		MT6370_DT_PROP_DECL(ocp-shutdown, BOOL, MT6370_REG_BL_BSTCTRL,
> +				    MT6370_BL_OC_EN_MASK, 1, true),
> +	}, *prop_now;
> +	int i, ret;
> +
> +	/* vendor optional properties */
> +	for (i = 0; i < ARRAY_SIZE(vendor_opt_props); i++) {
> +		prop_now = vendor_opt_props + i;
> +
> +		switch (prop_now->type) {
> +		case MT6370_PARSE_TYPE_BOOL:
> +			if (device_property_read_bool(dev, prop_now->name))
> +				val = 1;
> +			else
> +				val = 0;
> +			break;
> +		case MT6370_PARSE_TYPE_U8:
> +			ret = device_property_read_u8(dev, prop_now->name,
> +						      &prop_val);
> +			/* Property not exist, keep value in default */
> +			if (ret)
> +				continue;
> +
> +			val = min_t(u8, prop_val, prop_now->max_val);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		if (prop_now->invert)
> +			val = prop_now->max_val - val;
> +
> +		val <<= ffs(prop_now->mask) - 1;
> +
> +		ret = regmap_update_bits(priv->regmap, prop_now->reg,
> +					 prop_now->mask, val);
> +		if (ret)
> +			return ret;
> +	}

Is it really worth all this tricky code for 7 properties?

The code would be much easier to read and maintain if it were coded
directly. For example, the inverted boolean code is hard to read and
can be written directly as:


        val = device_property_read_bool(dev, "mediatek,bled-ovp_shutdown");
	ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BST_CTRL,
	                         MT6370_BL_OVP_EN_MASK,
				 MT6370_BL_OVP_EN_MASK * !val);
	if (ret)
		return ret;

The direct coded approach will probably also pay off if you switch
the bindings over to microvolts/microamps since it becomes much more
natural to call out to a lookup function to convert it into a register
value.

> +
> +	/* common properties */
> +	ret = device_property_read_u32(dev, "max-brightness", &brightness);
> +	if (ret)
> +		brightness = MT6370_BL_MAX_BRIGHTNESS;
> +
> +	props->max_brightness = min_t(u32, brightness,
> +				      MT6370_BL_MAX_BRIGHTNESS);
> +
> +	ret = device_property_read_u32(dev, "default-brightness", &brightness);
> +	if (ret)
> +		brightness = props->max_brightness;
> +
> +	props->brightness = min_t(u32, brightness, props->max_brightness);
> +
> +
> +	ret = device_property_read_u8(dev, "mediatek,bled-channel-use",
> +				      &prop_val);
> +	if (ret) {
> +		dev_err(dev, "mediatek,bled-channel-use DT property missing\n");
> +		return ret;
> +	}
> +
> +	if (!prop_val) {
> +		dev_err(dev, "No channel specified\n");
> +		return -EINVAL;
> +	}

If we are going to validity check this property then it needs an upper
bounds check to (e.g. consider if property is set to 64).


> +
> +	mask = MT6370_BL_EXT_EN_MASK | MT6370_BL_CH_MASK;
> +	val = prop_val << (ffs(MT6370_BL_CH_MASK) - 1);
> +
> +	if (priv->enable_gpio)
> +		val |= MT6370_BL_EXT_EN_MASK;
> +
> +	return regmap_update_bits(priv->regmap, MT6370_REG_BL_EN, mask, val);
> +}
> +
> +static int mt6370_check_vendor_info(struct mt6370_priv *priv)

A comment explaining what variants this function is intended to
supported here would be good here.


> +{
> +	unsigned int dev_info, vid;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, MT6370_REG_DEV_INFO, &dev_info);
> +	if (ret)
> +		return ret;
> +
> +	vid = FIELD_GET(MT6370_VENID_MASK, dev_info);
> +	if (vid == 0x9 || vid == 0xb)
> +		priv->vid_type = MT6370_VID_6372;
> +	else
> +		priv->vid_type = MT6370_VID_COMMON;
> +
> +	return 0;
> +}


No furthers comments so I trimmed the rest.


Daniel.
ChiaEn Wu June 2, 2022, 7:14 p.m. UTC | #2
Hi Daniel,

Thanks for your valuable feedback!

Daniel Thompson <daniel.thompson@linaro.org> 於 2022年6月1日 週三 下午5:46寫道:
>
> On Tue, May 31, 2022 at 07:19:00PM +0800, ChiaEn Wu wrote:
> > From: ChiaEn Wu <chiaen_wu@richtek.com>
> >
> > Add Mediatek MT6370 Backlight support.
> >
> > Signed-off-by: ChiaEn Wu <chiaen_wu@richtek.com>
> > ---
> >  drivers/video/backlight/Kconfig            |   8 +
> >  drivers/video/backlight/Makefile           |   1 +
> >  drivers/video/backlight/mt6370-backlight.c | 338 +++++++++++++++++++++
> >  3 files changed, 347 insertions(+)
> >  create mode 100644 drivers/video/backlight/mt6370-backlight.c
> >
> > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> > index a003e02e13ce..d9868fbe7488 100644
> > --- a/drivers/video/backlight/Kconfig
> > +++ b/drivers/video/backlight/Kconfig
> > @@ -268,6 +268,14 @@ config BACKLIGHT_MAX8925
> >         If you have a LCD backlight connected to the WLED output of MAX8925
> >         WLED output, say Y here to enable this driver.
> >
> > +config BACKLIGHT_MT6370
>
> Is MT6370 really the best name for this driver? In other words, you
> don't expect there to be any family resemblance between this backlight
> and the backlight in the *next* Mediatak PMIC?
>
> Moreover,

MT6370, MT6371, MT6372 are very similar, their hardware design and
features are almost the same and this driver can be compatible with
them.
But, MT6374 is not the same serials PMIC as MT6370, but the name is
very similar. And It has no backlight feature, so I think MT6370 is
the best name for this driver now.

>
>
> > +     tristate "Mediatek MT6370 Backlight Driver"
> > +     depends on MFD_MT6370
> > +     help
> > +       Say Y here to enable MT6370 Backlight support.
> > +       It's commonly used to drive the display WLED. There're 4 channels
> > +       inisde, and each channel can provide up to 30mA current.
>
> Nitpicking but this doesn't align well with other help texts in this
> file.
>

OK! I will try to align well in the next patch.

>
> > +
> >  config BACKLIGHT_APPLE
> >       tristate "Apple Backlight Driver"
> >       depends on X86 && ACPI
> > diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> > index cae2c83422ae..e815f3f1deff 100644
> > --- a/drivers/video/backlight/Makefile
> > +++ b/drivers/video/backlight/Makefile
> > @@ -44,6 +44,7 @@ obj-$(CONFIG_BACKLIGHT_LP855X)              += lp855x_bl.o
> >  obj-$(CONFIG_BACKLIGHT_LP8788)               += lp8788_bl.o
> >  obj-$(CONFIG_BACKLIGHT_LV5207LP)     += lv5207lp.o
> >  obj-$(CONFIG_BACKLIGHT_MAX8925)              += max8925_bl.o
> > +obj-$(CONFIG_BACKLIGHT_MT6370)               += mt6370-backlight.o
> >  obj-$(CONFIG_BACKLIGHT_OMAP1)                += omap1_bl.o
> >  obj-$(CONFIG_BACKLIGHT_PANDORA)              += pandora_bl.o
> >  obj-$(CONFIG_BACKLIGHT_PCF50633)     += pcf50633-backlight.o
> > diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c
> > new file mode 100644
> > index 000000000000..f8a8d33203ed
> > --- /dev/null
> > +++ b/drivers/video/backlight/mt6370-backlight.c
> > @@ -0,0 +1,338 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/backlight.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +#define MT6370_REG_DEV_INFO          0x100
> > +#define MT6370_REG_BL_EN             0x1A0
> > +#define MT6370_REG_BL_BSTCTRL                0x1A1
> > +#define MT6370_REG_BL_PWM            0x1A2
> > +#define MT6370_REG_BL_DIM2           0x1A4
> > +
> > +#define MT6370_VENID_MASK            GENMASK(7, 4)
> > +#define MT6370_BL_EXT_EN_MASK                BIT(7)
> > +#define MT6370_BL_EN_MASK            BIT(6)
> > +#define MT6370_BL_CONFIG_MASK                BIT(0)
> > +#define MT6370_BL_CH_MASK            GENMASK(5, 2)
> > +#define MT6370_BL_DIM2_MASK          GENMASK(2, 0)
> > +#define MT6370_BL_DUMMY_6372_MASK    GENMASK(2, 0)
> > +#define MT6370_BL_DIM2_6372_SHIFT    3
> > +#define MT6370_BL_PWM_EN_MASK                BIT(7)
> > +#define MT6370_BL_PWM_HYS_EN_MASK    BIT(2)
> > +#define MT6370_BL_PWM_HYS_SEL_MASK   GENMASK(1, 0)
> > +#define MT6370_BL_OVP_EN_MASK                BIT(7)
> > +#define MT6370_BL_OVP_SEL_MASK               GENMASK(6, 5)
> > +#define MT6370_BL_OC_EN_MASK         BIT(3)
> > +#define MT6370_BL_OC_SEL_MASK                GENMASK(2, 1)
> > +
> > +#define MT6370_BL_MAX_BRIGHTNESS     2048
> > +
> > +enum {
> > +     MT6370_VID_COMMON = 0,
> > +     MT6370_VID_6372,
> > +     MT6370_VID_MAX,
>
> Unused.
>
> > +};
> > +
> > +enum mt6370_prop_type {
> > +     MT6370_PARSE_TYPE_BOOL = 0,
> > +     MT6370_PARSE_TYPE_U8,
> > +     MT6370_PARSE_TYPE_MAX,
>
> Unused.
>
> > +};
> > +
> > +struct mt6370_priv {
> > +     int vid_type;
> > +     struct backlight_device *bl;
> > +     struct device *dev;
> > +     struct gpio_desc *enable_gpio;
> > +     struct regmap *regmap;
> > +};
> > +
> > +static int mt6370_bl_update_status(struct backlight_device *bl_dev)
> > +{
> > +     struct mt6370_priv *priv = bl_get_data(bl_dev);
> > +     int brightness = backlight_get_brightness(bl_dev);
> > +     unsigned int enable_val;
> > +     u8 brightness_val[2];
> > +     int ret;
> > +
> > +     if (brightness) {
> > +             brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK;
> > +             brightness_val[1] = (brightness - 1)
> > +                                     >> fls(MT6370_BL_DIM2_MASK);
> > +
> > +             if (priv->vid_type == MT6370_VID_6372) {
> > +                     brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT;
> > +                     brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK;
>
> Comment explaining why we have to set these bits would be useful.

I got it, I will add the comment in the next patch.

>
>
> > +             }
> > +
> > +             ret = regmap_raw_write(priv->regmap, MT6370_REG_BL_DIM2,
> > +                                    brightness_val, sizeof(brightness_val));
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     if (priv->enable_gpio)
> > +             gpiod_set_value(priv->enable_gpio, brightness ? 1 : 0);
> > +
> > +     enable_val = brightness ? MT6370_BL_EN_MASK : 0;
> > +     return regmap_update_bits(priv->regmap, MT6370_REG_BL_EN,
> > +                               MT6370_BL_EN_MASK, enable_val);
> > +}
> > +
>
> <snip>
>
> > +#define MT6370_DT_PROP_DECL(_name, _type, _reg, _mask, _max, _inv)   \
> > +{                                                                    \
> > +     .name = "mediatek,bled-" #_name,                                \
>
> I'd rather have the whole DT property in the macro (because it helps
> with grepability).

Do you mean the _name parameter must be the full name of the DT
property and do not use "#" to concat like following example?

// in declare
            .name = _name,
// in use
            MT6370_DT_PROP_DECL(mediatek,bled-pwm-enable, ......)

>
>
> > +     .type = MT6370_PARSE_TYPE_##_type,                              \
> > +     .reg = _reg,                                                    \
> > +     .mask = _mask,                                                  \
> > +     .max_val = _max,                                                \
> > +     .invert = _inv,                                                 \
> > +}
> > +
> > +static int mt6370_init_backlight_properties(struct mt6370_priv *priv,
> > +                                         struct backlight_properties *props)
> > +{
> > +     struct device *dev = priv->dev;
> > +     u8 prop_val;
> > +     u32 brightness;
> > +     unsigned int mask, val;
> > +     static const struct {
> > +             char *name;
> > +             enum mt6370_prop_type type;
> > +             unsigned int reg;
> > +             unsigned int mask;
> > +             u8 max_val;
> > +             bool invert;
> > +     } vendor_opt_props[] = {
> > +             MT6370_DT_PROP_DECL(pwm-enable, BOOL, MT6370_REG_BL_PWM,
> > +                                 MT6370_BL_PWM_EN_MASK, 1, false),
> > +             MT6370_DT_PROP_DECL(pwm-hys-enable, BOOL, MT6370_REG_BL_PWM,
> > +                                 MT6370_BL_PWM_HYS_EN_MASK, 1, false),
> > +             MT6370_DT_PROP_DECL(pwm-hys-sel, U8, MT6370_REG_BL_PWM,
> > +                                 MT6370_BL_PWM_HYS_SEL_MASK, 3, false),
> > +             MT6370_DT_PROP_DECL(ovp-level-sel, U8, MT6370_REG_BL_BSTCTRL,
> > +                                 MT6370_BL_OVP_SEL_MASK, 3, false),
> > +             MT6370_DT_PROP_DECL(ovp-shutdown, BOOL, MT6370_REG_BL_BSTCTRL,
> > +                                 MT6370_BL_OVP_EN_MASK, 1, true),
> > +             MT6370_DT_PROP_DECL(ocp-level-sel, U8, MT6370_REG_BL_BSTCTRL,
> > +                                 MT6370_BL_OC_SEL_MASK, 3, false),
> > +             MT6370_DT_PROP_DECL(ocp-shutdown, BOOL, MT6370_REG_BL_BSTCTRL,
> > +                                 MT6370_BL_OC_EN_MASK, 1, true),
> > +     }, *prop_now;
> > +     int i, ret;
> > +
> > +     /* vendor optional properties */
> > +     for (i = 0; i < ARRAY_SIZE(vendor_opt_props); i++) {
> > +             prop_now = vendor_opt_props + i;
> > +
> > +             switch (prop_now->type) {
> > +             case MT6370_PARSE_TYPE_BOOL:
> > +                     if (device_property_read_bool(dev, prop_now->name))
> > +                             val = 1;
> > +                     else
> > +                             val = 0;
> > +                     break;
> > +             case MT6370_PARSE_TYPE_U8:
> > +                     ret = device_property_read_u8(dev, prop_now->name,
> > +                                                   &prop_val);
> > +                     /* Property not exist, keep value in default */
> > +                     if (ret)
> > +                             continue;
> > +
> > +                     val = min_t(u8, prop_val, prop_now->max_val);
> > +                     break;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> > +
> > +             if (prop_now->invert)
> > +                     val = prop_now->max_val - val;
> > +
> > +             val <<= ffs(prop_now->mask) - 1;
> > +
> > +             ret = regmap_update_bits(priv->regmap, prop_now->reg,
> > +                                      prop_now->mask, val);
> > +             if (ret)
> > +                     return ret;
> > +     }
>
> Is it really worth all this tricky code for 7 properties?
>
> The code would be much easier to read and maintain if it were coded
> directly. For example, the inverted boolean code is hard to read and
> can be written directly as:
>
>
>         val = device_property_read_bool(dev, "mediatek,bled-ovp_shutdown");
>         ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BST_CTRL,
>                                  MT6370_BL_OVP_EN_MASK,
>                                  MT6370_BL_OVP_EN_MASK * !val);
>         if (ret)
>                 return ret;
>
> The direct coded approach will probably also pay off if you switch
> the bindings over to microvolts/microamps since it becomes much more
> natural to call out to a lookup function to convert it into a register
> value.
>

The purpose of my code is trying to avoid the repeat code in this
function. And for loop can help to decrease the lines of code
effectively, that's why I use these code to parse the DT properties.

> > +
> > +     /* common properties */
> > +     ret = device_property_read_u32(dev, "max-brightness", &brightness);
> > +     if (ret)
> > +             brightness = MT6370_BL_MAX_BRIGHTNESS;
> > +
> > +     props->max_brightness = min_t(u32, brightness,
> > +                                   MT6370_BL_MAX_BRIGHTNESS);
> > +
> > +     ret = device_property_read_u32(dev, "default-brightness", &brightness);
> > +     if (ret)
> > +             brightness = props->max_brightness;
> > +
> > +     props->brightness = min_t(u32, brightness, props->max_brightness);
> > +
> > +
> > +     ret = device_property_read_u8(dev, "mediatek,bled-channel-use",
> > +                                   &prop_val);
> > +     if (ret) {
> > +             dev_err(dev, "mediatek,bled-channel-use DT property missing\n");
> > +             return ret;
> > +     }
> > +
> > +     if (!prop_val) {
> > +             dev_err(dev, "No channel specified\n");
> > +             return -EINVAL;
> > +     }
>
> If we are going to validity check this property then it needs an upper
> bounds check to (e.g. consider if property is set to 64).
>
OK, I will add the validation in the next patch.

>
> > +
> > +     mask = MT6370_BL_EXT_EN_MASK | MT6370_BL_CH_MASK;
> > +     val = prop_val << (ffs(MT6370_BL_CH_MASK) - 1);
> > +
> > +     if (priv->enable_gpio)
> > +             val |= MT6370_BL_EXT_EN_MASK;
> > +
> > +     return regmap_update_bits(priv->regmap, MT6370_REG_BL_EN, mask, val);
> > +}
> > +
> > +static int mt6370_check_vendor_info(struct mt6370_priv *priv)
>
> A comment explaining what variants this function is intended to
> supported here would be good here.

Ok, I got it! I'll add the comment in the next patch.

>
>
> > +{
> > +     unsigned int dev_info, vid;
> > +     int ret;
> > +
> > +     ret = regmap_read(priv->regmap, MT6370_REG_DEV_INFO, &dev_info);
> > +     if (ret)
> > +             return ret;
> > +
> > +     vid = FIELD_GET(MT6370_VENID_MASK, dev_info);
> > +     if (vid == 0x9 || vid == 0xb)
> > +             priv->vid_type = MT6370_VID_6372;
> > +     else
> > +             priv->vid_type = MT6370_VID_COMMON;
> > +
> > +     return 0;
> > +}
>
>
> No furthers comments so I trimmed the rest.
>
>
> Daniel.

Best regards,
ChiaEn Wu
Daniel Thompson June 7, 2022, 10:41 a.m. UTC | #3
On Fri, Jun 03, 2022 at 03:14:56AM +0800, ChiaEn Wu wrote:
> Daniel Thompson <daniel.thompson@linaro.org> 於 2022年6月1日 週三 下午5:46寫道:
> >
> > On Tue, May 31, 2022 at 07:19:00PM +0800, ChiaEn Wu wrote:
> > > +#define MT6370_DT_PROP_DECL(_name, _type, _reg, _mask, _max, _inv)   \
> > > +{                                                                    \
> > > +     .name = "mediatek,bled-" #_name,                                \
> >
> > I'd rather have the whole DT property in the macro (because it helps
> > with grepability).
> 
> Do you mean the _name parameter must be the full name of the DT
> property and do not use "#" to concat like following example?
> 
> // in declare
>             .name = _name,
> // in use
>             MT6370_DT_PROP_DECL(mediatek,bled-pwm-enable, ......)

Yes, I would prefer this form, although, as discussed below, I don't really
like MT6370_DT_PROP_DECL().


> > > +     .type = MT6370_PARSE_TYPE_##_type,                              \
> > > +     .reg = _reg,                                                    \
> > > +     .mask = _mask,                                                  \
> > > +     .max_val = _max,                                                \
> > > +     .invert = _inv,                                                 \
> > > +}
> > > +
> > > +static int mt6370_init_backlight_properties(struct mt6370_priv *priv,
> > > +                                         struct backlight_properties *props)
> > > +{
> > > +     struct device *dev = priv->dev;
> > > +     u8 prop_val;
> > > +     u32 brightness;
> > > +     unsigned int mask, val;
> > > +     static const struct {
> > > +             char *name;
> > > +             enum mt6370_prop_type type;
> > > +             unsigned int reg;
> > > +             unsigned int mask;
> > > +             u8 max_val;
> > > +             bool invert;
> > > +     } vendor_opt_props[] = {
> > > +             MT6370_DT_PROP_DECL(pwm-enable, BOOL, MT6370_REG_BL_PWM,
> > > +                                 MT6370_BL_PWM_EN_MASK, 1, false),
> > > +             MT6370_DT_PROP_DECL(pwm-hys-enable, BOOL, MT6370_REG_BL_PWM,
> > > +                                 MT6370_BL_PWM_HYS_EN_MASK, 1, false),
> > > +             MT6370_DT_PROP_DECL(pwm-hys-sel, U8, MT6370_REG_BL_PWM,
> > > +                                 MT6370_BL_PWM_HYS_SEL_MASK, 3, false),
> > > +             MT6370_DT_PROP_DECL(ovp-level-sel, U8, MT6370_REG_BL_BSTCTRL,
> > > +                                 MT6370_BL_OVP_SEL_MASK, 3, false),
> > > +             MT6370_DT_PROP_DECL(ovp-shutdown, BOOL, MT6370_REG_BL_BSTCTRL,
> > > +                                 MT6370_BL_OVP_EN_MASK, 1, true),
> > > +             MT6370_DT_PROP_DECL(ocp-level-sel, U8, MT6370_REG_BL_BSTCTRL,
> > > +                                 MT6370_BL_OC_SEL_MASK, 3, false),
> > > +             MT6370_DT_PROP_DECL(ocp-shutdown, BOOL, MT6370_REG_BL_BSTCTRL,
> > > +                                 MT6370_BL_OC_EN_MASK, 1, true),
> > > +     }, *prop_now;
> > > +     int i, ret;
> > > +
> > > +     /* vendor optional properties */
> > > +     for (i = 0; i < ARRAY_SIZE(vendor_opt_props); i++) {
> > > +             prop_now = vendor_opt_props + i;
> > > +
> > > +             switch (prop_now->type) {
> > > +             case MT6370_PARSE_TYPE_BOOL:
> > > +                     if (device_property_read_bool(dev, prop_now->name))
> > > +                             val = 1;
> > > +                     else
> > > +                             val = 0;
> > > +                     break;
> > > +             case MT6370_PARSE_TYPE_U8:
> > > +                     ret = device_property_read_u8(dev, prop_now->name,
> > > +                                                   &prop_val);
> > > +                     /* Property not exist, keep value in default */
> > > +                     if (ret)
> > > +                             continue;
> > > +
> > > +                     val = min_t(u8, prop_val, prop_now->max_val);
> > > +                     break;
> > > +             default:
> > > +                     return -EINVAL;
> > > +             }
> > > +
> > > +             if (prop_now->invert)
> > > +                     val = prop_now->max_val - val;
> > > +
> > > +             val <<= ffs(prop_now->mask) - 1;
> > > +
> > > +             ret = regmap_update_bits(priv->regmap, prop_now->reg,
> > > +                                      prop_now->mask, val);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> >
> > Is it really worth all this tricky code for 7 properties?
> >
> > The code would be much easier to read and maintain if it were coded
> > directly. For example, the inverted boolean code is hard to read and
> > can be written directly as:
> >
> >
> >         val = device_property_read_bool(dev, "mediatek,bled-ovp_shutdown");
> >         ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BST_CTRL,
> >                                  MT6370_BL_OVP_EN_MASK,
> >                                  MT6370_BL_OVP_EN_MASK * !val);
> >         if (ret)
> >                 return ret;
> >
> > The direct coded approach will probably also pay off if you switch
> > the bindings over to microvolts/microamps since it becomes much more
> > natural to call out to a lookup function to convert it into a register
> > value.
> >
> 
> The purpose of my code is trying to avoid the repeat code in this
> function. And for loop can help to decrease the lines of code
> effectively, that's why I use these code to parse the DT properties.

I'm not really convinced that is uses fewer lines of code. It
certainly would if there were a very large number of properties
but here there is only seven.

However I guess what I'm really complaining about is how hard it is to
read the for loop. We have to study the macros, keep track six different
arguments per property and review the complex logic of the for loop
(which for example handles inverted u8's that don't actually exist).

To be clear, it's not that loops aren't useful for reducing boilerplate
code. They can be. However trying to handle booleans and integers in the
*same* loop ends up needlessly hard to read.

Also, I think that if/when you adopt microamps/microvolts then the
hard-to-read problem will get even worse unless you get loops to do only
one thing!


Daniel.
diff mbox series

Patch

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index a003e02e13ce..d9868fbe7488 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -268,6 +268,14 @@  config BACKLIGHT_MAX8925
 	  If you have a LCD backlight connected to the WLED output of MAX8925
 	  WLED output, say Y here to enable this driver.
 
+config BACKLIGHT_MT6370
+	tristate "Mediatek MT6370 Backlight Driver"
+	depends on MFD_MT6370
+	help
+	  Say Y here to enable MT6370 Backlight support.
+	  It's commonly used to drive the display WLED. There're 4 channels
+	  inisde, and each channel can provide up to 30mA current.
+
 config BACKLIGHT_APPLE
 	tristate "Apple Backlight Driver"
 	depends on X86 && ACPI
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index cae2c83422ae..e815f3f1deff 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -44,6 +44,7 @@  obj-$(CONFIG_BACKLIGHT_LP855X)		+= lp855x_bl.o
 obj-$(CONFIG_BACKLIGHT_LP8788)		+= lp8788_bl.o
 obj-$(CONFIG_BACKLIGHT_LV5207LP)	+= lv5207lp.o
 obj-$(CONFIG_BACKLIGHT_MAX8925)		+= max8925_bl.o
+obj-$(CONFIG_BACKLIGHT_MT6370)		+= mt6370-backlight.o
 obj-$(CONFIG_BACKLIGHT_OMAP1)		+= omap1_bl.o
 obj-$(CONFIG_BACKLIGHT_PANDORA)		+= pandora_bl.o
 obj-$(CONFIG_BACKLIGHT_PCF50633)	+= pcf50633-backlight.o
diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c
new file mode 100644
index 000000000000..f8a8d33203ed
--- /dev/null
+++ b/drivers/video/backlight/mt6370-backlight.c
@@ -0,0 +1,338 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/backlight.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define MT6370_REG_DEV_INFO		0x100
+#define MT6370_REG_BL_EN		0x1A0
+#define MT6370_REG_BL_BSTCTRL		0x1A1
+#define MT6370_REG_BL_PWM		0x1A2
+#define MT6370_REG_BL_DIM2		0x1A4
+
+#define MT6370_VENID_MASK		GENMASK(7, 4)
+#define MT6370_BL_EXT_EN_MASK		BIT(7)
+#define MT6370_BL_EN_MASK		BIT(6)
+#define MT6370_BL_CONFIG_MASK		BIT(0)
+#define MT6370_BL_CH_MASK		GENMASK(5, 2)
+#define MT6370_BL_DIM2_MASK		GENMASK(2, 0)
+#define MT6370_BL_DUMMY_6372_MASK	GENMASK(2, 0)
+#define MT6370_BL_DIM2_6372_SHIFT	3
+#define MT6370_BL_PWM_EN_MASK		BIT(7)
+#define MT6370_BL_PWM_HYS_EN_MASK	BIT(2)
+#define MT6370_BL_PWM_HYS_SEL_MASK	GENMASK(1, 0)
+#define MT6370_BL_OVP_EN_MASK		BIT(7)
+#define MT6370_BL_OVP_SEL_MASK		GENMASK(6, 5)
+#define MT6370_BL_OC_EN_MASK		BIT(3)
+#define MT6370_BL_OC_SEL_MASK		GENMASK(2, 1)
+
+#define MT6370_BL_MAX_BRIGHTNESS	2048
+
+enum {
+	MT6370_VID_COMMON = 0,
+	MT6370_VID_6372,
+	MT6370_VID_MAX,
+};
+
+enum mt6370_prop_type {
+	MT6370_PARSE_TYPE_BOOL = 0,
+	MT6370_PARSE_TYPE_U8,
+	MT6370_PARSE_TYPE_MAX,
+};
+
+struct mt6370_priv {
+	int vid_type;
+	struct backlight_device *bl;
+	struct device *dev;
+	struct gpio_desc *enable_gpio;
+	struct regmap *regmap;
+};
+
+static int mt6370_bl_update_status(struct backlight_device *bl_dev)
+{
+	struct mt6370_priv *priv = bl_get_data(bl_dev);
+	int brightness = backlight_get_brightness(bl_dev);
+	unsigned int enable_val;
+	u8 brightness_val[2];
+	int ret;
+
+	if (brightness) {
+		brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK;
+		brightness_val[1] = (brightness - 1)
+					>> fls(MT6370_BL_DIM2_MASK);
+
+		if (priv->vid_type == MT6370_VID_6372) {
+			brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT;
+			brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK;
+		}
+
+		ret = regmap_raw_write(priv->regmap, MT6370_REG_BL_DIM2,
+				       brightness_val, sizeof(brightness_val));
+		if (ret)
+			return ret;
+	}
+
+	if (priv->enable_gpio)
+		gpiod_set_value(priv->enable_gpio, brightness ? 1 : 0);
+
+	enable_val = brightness ? MT6370_BL_EN_MASK : 0;
+	return regmap_update_bits(priv->regmap, MT6370_REG_BL_EN,
+				  MT6370_BL_EN_MASK, enable_val);
+}
+
+static int mt6370_bl_get_brightness(struct backlight_device *bl_dev)
+{
+	struct mt6370_priv *priv = bl_get_data(bl_dev);
+	unsigned int enable;
+	u8 brightness_val[2];
+	int brightness, ret;
+
+	ret = regmap_read(priv->regmap, MT6370_REG_BL_EN, &enable);
+	if (ret)
+		return ret;
+
+	if (!(enable & MT6370_BL_EN_MASK))
+		return 0;
+
+	ret = regmap_raw_read(priv->regmap, MT6370_REG_BL_DIM2,
+			      brightness_val, sizeof(brightness_val));
+	if (ret)
+		return ret;
+
+	if (priv->vid_type == MT6370_VID_6372)
+		brightness_val[0] >>= MT6370_BL_DIM2_6372_SHIFT;
+
+	brightness = brightness_val[1] << fls(MT6370_BL_DIM2_MASK);
+	brightness += (brightness_val[0] & MT6370_BL_DIM2_MASK);
+
+	return brightness + 1;
+}
+
+static const struct backlight_ops mt6370_bl_ops = {
+	.options = BL_CORE_SUSPENDRESUME,
+	.update_status = mt6370_bl_update_status,
+	.get_brightness = mt6370_bl_get_brightness,
+};
+
+#define MT6370_DT_PROP_DECL(_name, _type, _reg, _mask, _max, _inv)	\
+{									\
+	.name = "mediatek,bled-" #_name,				\
+	.type = MT6370_PARSE_TYPE_##_type,				\
+	.reg = _reg,							\
+	.mask = _mask,							\
+	.max_val = _max,						\
+	.invert = _inv,							\
+}
+
+static int mt6370_init_backlight_properties(struct mt6370_priv *priv,
+					    struct backlight_properties *props)
+{
+	struct device *dev = priv->dev;
+	u8 prop_val;
+	u32 brightness;
+	unsigned int mask, val;
+	static const struct {
+		char *name;
+		enum mt6370_prop_type type;
+		unsigned int reg;
+		unsigned int mask;
+		u8 max_val;
+		bool invert;
+	} vendor_opt_props[] = {
+		MT6370_DT_PROP_DECL(pwm-enable, BOOL, MT6370_REG_BL_PWM,
+				    MT6370_BL_PWM_EN_MASK, 1, false),
+		MT6370_DT_PROP_DECL(pwm-hys-enable, BOOL, MT6370_REG_BL_PWM,
+				    MT6370_BL_PWM_HYS_EN_MASK, 1, false),
+		MT6370_DT_PROP_DECL(pwm-hys-sel, U8, MT6370_REG_BL_PWM,
+				    MT6370_BL_PWM_HYS_SEL_MASK, 3, false),
+		MT6370_DT_PROP_DECL(ovp-level-sel, U8, MT6370_REG_BL_BSTCTRL,
+				    MT6370_BL_OVP_SEL_MASK, 3, false),
+		MT6370_DT_PROP_DECL(ovp-shutdown, BOOL, MT6370_REG_BL_BSTCTRL,
+				    MT6370_BL_OVP_EN_MASK, 1, true),
+		MT6370_DT_PROP_DECL(ocp-level-sel, U8, MT6370_REG_BL_BSTCTRL,
+				    MT6370_BL_OC_SEL_MASK, 3, false),
+		MT6370_DT_PROP_DECL(ocp-shutdown, BOOL, MT6370_REG_BL_BSTCTRL,
+				    MT6370_BL_OC_EN_MASK, 1, true),
+	}, *prop_now;
+	int i, ret;
+
+	/* vendor optional properties */
+	for (i = 0; i < ARRAY_SIZE(vendor_opt_props); i++) {
+		prop_now = vendor_opt_props + i;
+
+		switch (prop_now->type) {
+		case MT6370_PARSE_TYPE_BOOL:
+			if (device_property_read_bool(dev, prop_now->name))
+				val = 1;
+			else
+				val = 0;
+			break;
+		case MT6370_PARSE_TYPE_U8:
+			ret = device_property_read_u8(dev, prop_now->name,
+						      &prop_val);
+			/* Property not exist, keep value in default */
+			if (ret)
+				continue;
+
+			val = min_t(u8, prop_val, prop_now->max_val);
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		if (prop_now->invert)
+			val = prop_now->max_val - val;
+
+		val <<= ffs(prop_now->mask) - 1;
+
+		ret = regmap_update_bits(priv->regmap, prop_now->reg,
+					 prop_now->mask, val);
+		if (ret)
+			return ret;
+	}
+
+	/* common properties */
+	ret = device_property_read_u32(dev, "max-brightness", &brightness);
+	if (ret)
+		brightness = MT6370_BL_MAX_BRIGHTNESS;
+
+	props->max_brightness = min_t(u32, brightness,
+				      MT6370_BL_MAX_BRIGHTNESS);
+
+	ret = device_property_read_u32(dev, "default-brightness", &brightness);
+	if (ret)
+		brightness = props->max_brightness;
+
+	props->brightness = min_t(u32, brightness, props->max_brightness);
+
+
+	ret = device_property_read_u8(dev, "mediatek,bled-channel-use",
+				      &prop_val);
+	if (ret) {
+		dev_err(dev, "mediatek,bled-channel-use DT property missing\n");
+		return ret;
+	}
+
+	if (!prop_val) {
+		dev_err(dev, "No channel specified\n");
+		return -EINVAL;
+	}
+
+	mask = MT6370_BL_EXT_EN_MASK | MT6370_BL_CH_MASK;
+	val = prop_val << (ffs(MT6370_BL_CH_MASK) - 1);
+
+	if (priv->enable_gpio)
+		val |= MT6370_BL_EXT_EN_MASK;
+
+	return regmap_update_bits(priv->regmap, MT6370_REG_BL_EN, mask, val);
+}
+
+static int mt6370_check_vendor_info(struct mt6370_priv *priv)
+{
+	unsigned int dev_info, vid;
+	int ret;
+
+	ret = regmap_read(priv->regmap, MT6370_REG_DEV_INFO, &dev_info);
+	if (ret)
+		return ret;
+
+	vid = FIELD_GET(MT6370_VENID_MASK, dev_info);
+	if (vid == 0x9 || vid == 0xb)
+		priv->vid_type = MT6370_VID_6372;
+	else
+		priv->vid_type = MT6370_VID_COMMON;
+
+	return 0;
+}
+
+static int mt6370_bl_probe(struct platform_device *pdev)
+{
+	struct mt6370_priv *priv;
+	struct backlight_properties props = {
+		.type = BACKLIGHT_RAW,
+		.scale = BACKLIGHT_SCALE_LINEAR,
+	};
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+
+	priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!priv->regmap) {
+		dev_err(&pdev->dev, "Failed to get regmap\n");
+		return -ENODEV;
+	}
+
+	ret = mt6370_check_vendor_info(priv);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to check vendor info (%d)\n", ret);
+		return ret;
+	}
+
+	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
+						    GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->enable_gpio)) {
+		dev_err(&pdev->dev, "Failed to get 'enable' gpio\n");
+		return PTR_ERR(priv->enable_gpio);
+	}
+
+	ret = mt6370_init_backlight_properties(priv, &props);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to init backlight properties\n");
+		return ret;
+	}
+
+	priv->bl = devm_backlight_device_register(&pdev->dev, pdev->name,
+						  &pdev->dev, priv,
+						  &mt6370_bl_ops, &props);
+	if (IS_ERR(priv->bl)) {
+		dev_err(&pdev->dev, "Failed to register backlight\n");
+		return PTR_ERR(priv->bl);
+	}
+
+	backlight_update_status(priv->bl);
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static int mt6370_bl_remove(struct platform_device *pdev)
+{
+	struct mt6370_priv *priv = platform_get_drvdata(pdev);
+	struct backlight_device *bl_dev = priv->bl;
+
+	bl_dev->props.brightness = 0;
+	backlight_update_status(priv->bl);
+
+	return 0;
+}
+
+static const struct of_device_id __maybe_unused mt6370_bl_of_match[] = {
+	{ .compatible = "mediatek,mt6370-backlight", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mt6370_bl_of_match);
+
+static struct platform_driver mt6370_bl_driver = {
+	.driver = {
+		.name = "mt6370-backlight",
+		.of_match_table = mt6370_bl_of_match,
+	},
+	.probe = mt6370_bl_probe,
+	.remove = mt6370_bl_remove,
+};
+module_platform_driver(mt6370_bl_driver);
+
+MODULE_AUTHOR("ChiaEn Wu <chiaen_wu@richtek.com>");
+MODULE_DESCRIPTION("Mediatek MT6370 Backlight Driver");
+MODULE_LICENSE("GPL v2");