| Message ID | 20220722102407.2205-14-peterwu.pub@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,01/13] dt-bindings: usb: Add MediaTek MT6370 TCPC | expand |
On Fri, Jul 22, 2022 at 12:25 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > From: ChiaEn Wu <chiaen_wu@richtek.com> > > MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger > with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight > driver, display bias voltage supply, one general purpose LDO, and the > USB Type-C & PD controller complies with the latest USB Type-C and PD > standards. > > This adds support for MediaTek MT6370 Backlight driver. It's commonly used Read Submitting Patches, please! (In this case, find "This patch" in the above mentioned document, read and act accordingly) > to drive the display WLED. There are 4 channels inside, and each channel > supports up to 30mA of current capability with 2048 current steps in > exponential or linear mapping curves. ... > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); (see below) ... > + /* > + * To make MT6372 using 14 bits to control the brightness > + * backward compatible with 11 bits brightness control > + * (like MT6370 and MT6371 do), we left shift the value > + * and pad with 1 to remaining bits. Hence, the MT6372's to the remaining > + * backlight brightness will be almost the same as MT6370's > + * and MT6371's. > + */ > + if (priv->vid_type == MT6370_VID_6372) { > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > + } Nice! Why not... ... > + gpiod_set_value(priv->enable_gpio, brightness ? 1 : 0); !!brightness will do as well. ... > + brightness = brightness_val[1] << fls(MT6370_BL_DIM2_MASK); > + val |= prop_val << (ffs(MT6370_BL_PWM_HYS_SEL_MASK) - 1); > + val |= ovp_uV << (ffs(MT6370_BL_OVP_SEL_MASK) - 1); > + val |= ocp_uA << (ffs(MT6370_BL_OC_SEL_MASK) - 1); > + val = prop_val << (ffs(MT6370_BL_CH_MASK) - 1); ...to use respective _SHIFTs in all these? ... > + priv->enable_gpio = devm_gpiod_get_optional(dev, "enable", > + GPIOD_OUT_HIGH); > + if (IS_ERR(priv->enable_gpio)) > + dev_err(dev, "Failed to get 'enable' gpio\n"); What does this mean? Shouldn't be return dev_err_probe()?
On Fri, Jul 22, 2022 at 06:24:07PM +0800, ChiaEn Wu wrote: > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > index a003e02..846dbe7 100644 > --- a/drivers/video/backlight/Kconfig > +++ b/drivers/video/backlight/Kconfig > @@ -268,6 +268,18 @@ 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 > + This enables support for Mediatek MT6370 Backlight driver. > + It's commonly used to drive the display WLED. There are 4 channels > + inside, and each channel supports up to 30mA of current capability > + with 2048 current steps in exponential or linear mapping curves. Does the MT6372 support more steps than this? In other words does it use a fourteen bit scale or does it use an 11-bit scale at a different register location? > + > + This driver can also be built as a module. If so, the module > + will be called "mt6370-backlight". > + > [...] > diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c > new file mode 100644 > index 0000000..ba00a8f > --- /dev/null > +++ b/drivers/video/backlight/mt6370-backlight.c > [...] > +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); > + > + /* > + * To make MT6372 using 14 bits to control the brightness > + * backward compatible with 11 bits brightness control > + * (like MT6370 and MT6371 do), we left shift the value > + * and pad with 1 to remaining bits. Hence, the MT6372's > + * backlight brightness will be almost the same as MT6370's > + * and MT6371's. > + */ > + if (priv->vid_type == MT6370_VID_6372) { > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > + } This somewhat depends on the answer to the first question above, but what is the point of this shifting? If the range is 14-bit then the driver should set max_brightness to 16384 and present the full range of the MT6372 to the user. Especially when using linear mappings (which are a totally pointless scale to use for a backlight) the extra steps are useful for backlight animation. Daniel.
On Mon, Jul 25, 2022 at 6:31 PM Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Fri, Jul 22, 2022 at 06:24:07PM +0800, ChiaEn Wu wrote: > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > > index a003e02..846dbe7 100644 > > --- a/drivers/video/backlight/Kconfig > > +++ b/drivers/video/backlight/Kconfig > > @@ -268,6 +268,18 @@ 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 > > + This enables support for Mediatek MT6370 Backlight driver. > > + It's commonly used to drive the display WLED. There are 4 channels > > + inside, and each channel supports up to 30mA of current capability > > + with 2048 current steps in exponential or linear mapping curves. > > Does the MT6372 support more steps than this? In other words does it use > a fourteen bit scale or does it use an 11-bit scale at a different > register location? Hi Daniel, Thanks for your reply. Yes, MT6372 can support 16384 steps and uses a 14-bit scale register location. But the maximum current of each channel of MT6372 is the same as MT6370 and MT6371, both 30mA. The main reason why MT6372 is designed this way is that one of the customers asked for a more delicate adjustment of the backlight brightness. But other customers actually do not have such requirements. Therefore, we designed it this way for maximum compatibility in software. > > > > + > > + This driver can also be built as a module. If so, the module > > + will be called "mt6370-backlight". > > + > > [...] > > diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c > > new file mode 100644 > > index 0000000..ba00a8f > > --- /dev/null > > +++ b/drivers/video/backlight/mt6370-backlight.c > > [...] > > +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); > > + > > + /* > > + * To make MT6372 using 14 bits to control the brightness > > + * backward compatible with 11 bits brightness control > > + * (like MT6370 and MT6371 do), we left shift the value > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > + * backlight brightness will be almost the same as MT6370's > > + * and MT6371's. > > + */ > > + if (priv->vid_type == MT6370_VID_6372) { > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > + } > > This somewhat depends on the answer to the first question above, but > what is the point of this shifting? If the range is 14-bit then the > driver should set max_brightness to 16384 and present the full range of > the MT6372 to the user. So should we make all 16384 steps of MT6372 available to users? Does that mean the DTS needs to be modified as well? Or, for the reasons, I have just explained (just one customer has this requirement), then we do not make any changes for compatibility reasons? Thanks. > > Especially when using linear mappings (which are a totally pointless > scale to use for a backlight) the extra steps are useful for backlight > animation. > > > Daniel.
On Tue, Jul 26, 2022 at 10:20:02AM +0800, ChiaEn Wu wrote: > On Mon, Jul 25, 2022 at 6:31 PM Daniel Thompson > <daniel.thompson@linaro.org> wrote: > > > > On Fri, Jul 22, 2022 at 06:24:07PM +0800, ChiaEn Wu wrote: > > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > > > index a003e02..846dbe7 100644 > > > --- a/drivers/video/backlight/Kconfig > > > +++ b/drivers/video/backlight/Kconfig > > > @@ -268,6 +268,18 @@ 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 > > > + This enables support for Mediatek MT6370 Backlight driver. > > > + It's commonly used to drive the display WLED. There are 4 channels > > > + inside, and each channel supports up to 30mA of current capability > > > + with 2048 current steps in exponential or linear mapping curves. > > > > Does the MT6372 support more steps than this? In other words does it use > > a fourteen bit scale or does it use an 11-bit scale at a different > > register location? > > Hi Daniel, > > Thanks for your reply. > Yes, MT6372 can support 16384 steps and uses a 14-bit scale register > location. But the maximum current of each > channel of MT6372 is the same as MT6370 and MT6371, both 30mA. > The main reason why MT6372 is designed this way is that one of the > customers asked for a more delicate > adjustment of the backlight brightness. But other customers actually > do not have such requirements. > Therefore, we designed it this way for maximum compatibility in software. I don't think that is an acceptable approach for the upstream kernel. To be "compatible" with (broken) software this driver ends up reducing the capability of the upstream kernel to the point it becomes unable to meet requirements for delicate adjustment (requirements that were sufficiently important to change the hardware design so you could meet them). > > > + > > > + This driver can also be built as a module. If so, the module > > > + will be called "mt6370-backlight". > > > + > > > [...] > > > diff --git a/drivers/video/backlight/mt6370-backlight.c b/drivers/video/backlight/mt6370-backlight.c > > > new file mode 100644 > > > index 0000000..ba00a8f > > > --- /dev/null > > > +++ b/drivers/video/backlight/mt6370-backlight.c > > > [...] > > > +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); > > > + > > > + /* > > > + * To make MT6372 using 14 bits to control the brightness > > > + * backward compatible with 11 bits brightness control > > > + * (like MT6370 and MT6371 do), we left shift the value > > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > > + * backlight brightness will be almost the same as MT6370's > > > + * and MT6371's. > > > + */ > > > + if (priv->vid_type == MT6370_VID_6372) { > > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > > + } > > > > This somewhat depends on the answer to the first question above, but > > what is the point of this shifting? If the range is 14-bit then the > > driver should set max_brightness to 16384 and present the full range of > > the MT6372 to the user. > > So should we make all 16384 steps of MT6372 available to users? Yes. > Does that mean the DTS needs to be modified as well? Yes... the property to set initial brightness needs a 14-bit range. It would also be a good idea to discuss with the DT maintainers whether you should introduce a second compatible string (ending 6372) in order to allow the DT validation checks to detect accidental use of MT6372 ranges on MT6370 hardware. > Or, for the reasons, I have just explained (just one customer has this > requirement), then we do not make any changes for compatibility > reasons? I'd be curious what the compatiblity reasons are. In other words what software breaks? Normally the userspace backlight code reads the max_brightness property and configures things accordingly (and therefore if you the component that breaks is something like an Android HAL then fix the HAL instead). Daniel.
On Tue, Jul 26, 2022 at 5:31 PM Daniel Thompson <daniel.thompson@linaro.org> wrote: ... > > > Does the MT6372 support more steps than this? In other words does it use > > > a fourteen bit scale or does it use an 11-bit scale at a different > > > register location? > > > > Hi Daniel, > > > > Thanks for your reply. > > Yes, MT6372 can support 16384 steps and uses a 14-bit scale register > > location. But the maximum current of each > > channel of MT6372 is the same as MT6370 and MT6371, both 30mA. > > The main reason why MT6372 is designed this way is that one of the > > customers asked for a more delicate > > adjustment of the backlight brightness. But other customers actually > > do not have such requirements. > > Therefore, we designed it this way for maximum compatibility in software. Sorry for I used of the wrong word, I mean is 'driver', not higher-level software > > I don't think that is an acceptable approach for the upstream kernel. > > To be "compatible" with (broken) software this driver ends up reducing > the capability of the upstream kernel to the point it becomes unable to > meet requirements for delicate adjustment (requirements that were > sufficiently important to change the hardware design so you could meet > them). Originally, we just wanted to use one version of the driver to cover all the SubPMIC of the 6370 series(6370~6372). And, the users who use this series SubPMIC can directly apply this driver to drive the backlight device without knowing the underlying hardware. To achieve this goal, we have designed it to look like this. > > ... > > > > + > > > > + if (brightness) { > > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); > > > > + > > > > + /* > > > > + * To make MT6372 using 14 bits to control the brightness > > > > + * backward compatible with 11 bits brightness control > > > > + * (like MT6370 and MT6371 do), we left shift the value > > > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > > > + * backlight brightness will be almost the same as MT6370's > > > > + * and MT6371's. > > > > + */ > > > > + if (priv->vid_type == MT6370_VID_6372) { > > > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > > > + } > > > > > > This somewhat depends on the answer to the first question above, but > > > what is the point of this shifting? If the range is 14-bit then the > > > driver should set max_brightness to 16384 and present the full range of > > > the MT6372 to the user. > > > > So should we make all 16384 steps of MT6372 available to users? > > Yes. > > > > Does that mean the DTS needs to be modified as well? > > Yes... the property to set initial brightness needs a 14-bit range. > > It would also be a good idea to discuss with the DT maintainers whether > you should introduce a second compatible string (ending 6372) in order > to allow the DT validation checks to detect accidental use of MT6372 > ranges on MT6370 hardware. hmmm... I have just thought about it, maybe I can just modify the maximum value of default-brightness and max-brightness in DT to 16384, modify the description and add some comments. And then on the driver side, we can use mt6370_check_vendor_info( ) to determine whether it is MT6372. If no, then in mt6370_bl_update_status(), first brightness_val / 8 and then set. In mt6370_bl_get_brightness(), first brightness_val * 8 and then return; If I do this change, does this meet your requirements? > > > > Or, for the reasons, I have just explained (just one customer has this > > requirement), then we do not make any changes for compatibility > > reasons? > > I'd be curious what the compatiblity reasons are. In other words what > software breaks? The reason is as above. We just hope the users who use this series SubPMIC can directly apply this driver to drive the backlight device without knowing the underlying hardware. Not software breaks. Thanks! > > Normally the userspace backlight code reads the max_brightness property > and configures things accordingly (and therefore if you the component > that breaks is something like an Android HAL then fix the HAL instead). > > > Daniel.
On Tue, Jul 26, 2022 at 07:28:48PM +0800, ChiaEn Wu wrote: > On Tue, Jul 26, 2022 at 5:31 PM Daniel Thompson > <daniel.thompson@linaro.org> wrote: > ... > > > > Does the MT6372 support more steps than this? In other words does it use > > > > a fourteen bit scale or does it use an 11-bit scale at a different > > > > register location? > > > > > > Hi Daniel, > > > > > > Thanks for your reply. > > > Yes, MT6372 can support 16384 steps and uses a 14-bit scale register > > > location. But the maximum current of each > > > channel of MT6372 is the same as MT6370 and MT6371, both 30mA. > > > The main reason why MT6372 is designed this way is that one of the > > > customers asked for a more delicate > > > adjustment of the backlight brightness. But other customers actually > > > do not have such requirements. > > > Therefore, we designed it this way for maximum compatibility in software. > > Sorry for I used of the wrong word, I mean is 'driver', not > higher-level software > > > > > I don't think that is an acceptable approach for the upstream kernel. > > > > To be "compatible" with (broken) software this driver ends up reducing > > the capability of the upstream kernel to the point it becomes unable to > > meet requirements for delicate adjustment (requirements that were > > sufficiently important to change the hardware design so you could meet > > them). > > Originally, we just wanted to use one version of the driver to cover > all the SubPMIC of the 6370 series(6370~6372). > And, the users who use this series SubPMIC can directly apply this > driver to drive the backlight device without knowing the underlying > hardware. > To achieve this goal, we have designed it to look like this. You don't need a second driver to support two different values for max-brightness. The same driver can support both ranges with nothing but a small tweak during the driver probe function. > ... > > > > > + > > > > > + if (brightness) { > > > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > > > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); > > > > > + > > > > > + /* > > > > > + * To make MT6372 using 14 bits to control the brightness > > > > > + * backward compatible with 11 bits brightness control > > > > > + * (like MT6370 and MT6371 do), we left shift the value > > > > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > > > > + * backlight brightness will be almost the same as MT6370's > > > > > + * and MT6371's. > > > > > + */ > > > > > + if (priv->vid_type == MT6370_VID_6372) { > > > > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > > > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > > > > + } > > > > > > > > This somewhat depends on the answer to the first question above, but > > > > what is the point of this shifting? If the range is 14-bit then the > > > > driver should set max_brightness to 16384 and present the full range of > > > > the MT6372 to the user. > > > > > > So should we make all 16384 steps of MT6372 available to users? > > > > Yes. > > > > > > > Does that mean the DTS needs to be modified as well? > > > > Yes... the property to set initial brightness needs a 14-bit range. > > > > It would also be a good idea to discuss with the DT maintainers whether > > you should introduce a second compatible string (ending 6372) in order > > to allow the DT validation checks to detect accidental use of MT6372 > > ranges on MT6370 hardware. > > hmmm... I have just thought about it, > maybe I can just modify the maximum value of default-brightness and > max-brightness in DT to 16384, > modify the description and add some comments. What for? All the other backlight drivers (there are >130 of them) expose the hardware range[1]. Most userspaces will already know how to handle that (by reading the max_brightness and, if it is recent enough, also the scale properties in sysfs). I'm still don't understand why one should fix a bug in the userspace by implementing a hack in the driver. [1] Well almost. The PWM backlight driver does contain support for dead-spot avoidance and to allow the adoption of exponential scale. However this purpose of these is based on how PWM backlights work > And then on the driver side, > we can use mt6370_check_vendor_info( ) to determine whether it is MT6372. > If no, then in mt6370_bl_update_status(), first brightness_val / 8 and then set. > In mt6370_bl_get_brightness(), first brightness_val * 8 and then return; > > If I do this change, does this meet your requirements? Not really. It's still misleading a sophisticated userspace, which may make bad rounding decisions for backlight animation, in order to support a broken one. > > > Or, for the reasons, I have just explained (just one customer has this > > > requirement), then we do not make any changes for compatibility > > > reasons? > > > > I'd be curious what the compatiblity reasons are. In other words what > > software breaks? > > The reason is as above. We just hope the users who use this series SubPMIC can > directly apply this driver to drive the backlight device without > knowing the underlying hardware. > Not software breaks. As above, ignoring the max_brightness property is a bug in the userspace. I'm still unclear why sending faked ranges to userspace it a better solution than fixing the userspace. Daniel.
On Tue, Jul 26, 2022 at 7:59 PM Daniel Thompson <daniel.thompson@linaro.org> wrote: ... > > > > > Does the MT6372 support more steps than this? In other words does it use > > > > > a fourteen bit scale or does it use an 11-bit scale at a different > > > > > register location? > > > > > > > > Hi Daniel, > > > > > > > > Thanks for your reply. > > > > Yes, MT6372 can support 16384 steps and uses a 14-bit scale register > > > > location. But the maximum current of each > > > > channel of MT6372 is the same as MT6370 and MT6371, both 30mA. > > > > The main reason why MT6372 is designed this way is that one of the > > > > customers asked for a more delicate > > > > adjustment of the backlight brightness. But other customers actually > > > > do not have such requirements. > > > > Therefore, we designed it this way for maximum compatibility in software. > > > > Sorry for I used of the wrong word, I mean is 'driver', not > > higher-level software > > > > > > > > I don't think that is an acceptable approach for the upstream kernel. > > > > > > To be "compatible" with (broken) software this driver ends up reducing > > > the capability of the upstream kernel to the point it becomes unable to > > > meet requirements for delicate adjustment (requirements that were > > > sufficiently important to change the hardware design so you could meet > > > them). > > > > Originally, we just wanted to use one version of the driver to cover > > all the SubPMIC of the 6370 series(6370~6372). > > And, the users who use this series SubPMIC can directly apply this > > driver to drive the backlight device without knowing the underlying > > hardware. > > To achieve this goal, we have designed it to look like this. > > You don't need a second driver to support two different values for > max-brightness. The same driver can support both ranges with nothing but > a small tweak during the driver probe function. > > > > ... > > > > > > + > > > > > > + if (brightness) { > > > > > > + brightness_val[0] = (brightness - 1) & MT6370_BL_DIM2_MASK; > > > > > > + brightness_val[1] = (brightness - 1) >> fls(MT6370_BL_DIM2_MASK); > > > > > > + > > > > > > + /* > > > > > > + * To make MT6372 using 14 bits to control the brightness > > > > > > + * backward compatible with 11 bits brightness control > > > > > > + * (like MT6370 and MT6371 do), we left shift the value > > > > > > + * and pad with 1 to remaining bits. Hence, the MT6372's > > > > > > + * backlight brightness will be almost the same as MT6370's > > > > > > + * and MT6371's. > > > > > > + */ > > > > > > + if (priv->vid_type == MT6370_VID_6372) { > > > > > > + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT; > > > > > > + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK; > > > > > > + } > > > > > > > > > > This somewhat depends on the answer to the first question above, but > > > > > what is the point of this shifting? If the range is 14-bit then the > > > > > driver should set max_brightness to 16384 and present the full range of > > > > > the MT6372 to the user. > > > > > > > > So should we make all 16384 steps of MT6372 available to users? > > > > > > Yes. > > > > > > > > > > Does that mean the DTS needs to be modified as well? > > > > > > Yes... the property to set initial brightness needs a 14-bit range. > > > > > > It would also be a good idea to discuss with the DT maintainers whether > > > you should introduce a second compatible string (ending 6372) in order > > > to allow the DT validation checks to detect accidental use of MT6372 > > > ranges on MT6370 hardware. > > > > hmmm... I have just thought about it, > > maybe I can just modify the maximum value of default-brightness and > > max-brightness in DT to 16384, > > modify the description and add some comments. > > What for? > > All the other backlight drivers (there are >130 of them) expose the hardware > range[1]. Most userspaces will already know how to handle that (by reading > the max_brightness and, if it is recent enough, also the scale > properties in sysfs). > > I'm still don't understand why one should fix a bug in the userspace by > implementing a hack in the driver. > > > [1] Well almost. The PWM backlight driver does contain support for > dead-spot avoidance and to allow the adoption of exponential scale. > However this purpose of these is based on how PWM backlights work > > > And then on the driver side, > > we can use mt6370_check_vendor_info() to determine whether it is MT6372. > > If no, then in mt6370_bl_update_status(), first brightness_val / 8 and then set. > > In mt6370_bl_get_brightness(), first brightness_val * 8 and then return; > > > > If I do this change, does this meet your requirements? > > Not really. > > It's still misleading a sophisticated userspace, which may make bad > rounding decisions for backlight animation, in order to support a broken > one. > > > > > > Or, for the reasons, I have just explained (just one customer has this > > > > requirement), then we do not make any changes for compatibility > > > > reasons? > > > > > > I'd be curious what the compatiblity reasons are. In other words what > > > software breaks? > > > > The reason is as above. We just hope the users who use this series SubPMIC can > > directly apply this driver to drive the backlight device without > > knowing the underlying hardware. > > Not software breaks. > > As above, ignoring the max_brightness property is a bug in the > userspace. I'm still unclear why sending faked ranges to userspace > it a better solution than fixing the userspace. Ok, I got it! If I add a second compatible string (like 'mediatek,mt6372-backlight') in the DT section, and append 'if-then-else' to determine the correct maximum value of 'default-brightness' and 'max-brightness', Also, I will append 'bled exponential mode' to make user control using linear or exponential mode. These changes I will explain to DT's maintainer again. Back to the driver section, do I still need to use the register to confirm again whether this SubPMIC used now is MT6372 and record this information? (using 'mt6370_check_vendor_info()') I am afraid that the user who uses the MT6370 hardware, but the DT compatible string is set to 'mediatek,mt6372-backlight'. This may cause errors when update/get brightness values. So I hope the driver here can check again to make sure the 'default-brightness', 'max-brightness', can be updated/got correctly. I don't know if this will make you feel redundant if I do this?? Thank you so much!
On Wed, Jul 27, 2022 at 02:24:46PM +0800, ChiaEn Wu wrote: > On Tue, Jul 26, 2022 at 7:59 PM Daniel Thompson > <daniel.thompson@linaro.org> wrote: > > > > > So should we make all 16384 steps of MT6372 available to users? > > > > > > > > Yes. > > > > > > > > > > > > > Does that mean the DTS needs to be modified as well? > > > > > > > > Yes... the property to set initial brightness needs a 14-bit range. > > > > > > > > It would also be a good idea to discuss with the DT maintainers whether > > > > you should introduce a second compatible string (ending 6372) in order > > > > to allow the DT validation checks to detect accidental use of MT6372 > > > > ranges on MT6370 hardware. [snip] > > > > I'd be curious what the compatiblity reasons are. In other words what > > > > software breaks? > > > > > > The reason is as above. We just hope the users who use this series SubPMIC can > > > directly apply this driver to drive the backlight device without > > > knowing the underlying hardware. > > > Not software breaks. > > > > As above, ignoring the max_brightness property is a bug in the > > userspace. I'm still unclear why sending faked ranges to userspace > > it a better solution than fixing the userspace. > > Ok, I got it! > If I add a second compatible string (like 'mediatek,mt6372-backlight') > in the DT section, > and append 'if-then-else' to determine the correct maximum value of > 'default-brightness' and 'max-brightness', > Also, I will append 'bled exponential mode' to make user control using > linear or exponential mode. I'd be very pleased to see support for exponential mode added: it's a much better way to control LEDs and backlights. > These changes I will explain to DT's maintainer again. Excellent. I know DT maintainers are copied into this thread but they will probably not be following this patch's thread so it is better to discuss in the mail thread for the DT bindings! > Back to the driver section, > do I still need to use the register to confirm again whether this > SubPMIC used now is MT6372 and record this information? (using > 'mt6370_check_vendor_info()') > I am afraid that the user who uses the MT6370 hardware, but the DT > compatible string is set to 'mediatek,mt6372-backlight'. > This may cause errors when update/get brightness values. > So I hope the driver here can check again to make sure the > 'default-brightness', 'max-brightness', can be updated/got correctly. > I don't know if this will make you feel redundant if I do this?? Yes, it's good idea to check the hardware model during probe. I'd suggest just reporting this as an error ("Buggy DT, wrong compatible string") rather than trying to re-intepret the parameters. Daniel.
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig index a003e02..846dbe7 100644 --- a/drivers/video/backlight/Kconfig +++ b/drivers/video/backlight/Kconfig @@ -268,6 +268,18 @@ 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 + This enables support for Mediatek MT6370 Backlight driver. + It's commonly used to drive the display WLED. There are 4 channels + inside, and each channel supports up to 30mA of current capability + with 2048 current steps in exponential or linear mapping curves. + + This driver can also be built as a module. If so, the module + will be called "mt6370-backlight". + config BACKLIGHT_APPLE tristate "Apple Backlight Driver" depends on X86 && ACPI diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile index cae2c83..e815f3f 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 0000000..ba00a8f --- /dev/null +++ b/drivers/video/backlight/mt6370-backlight.c @@ -0,0 +1,339 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2022 Richtek Technology Corp. + * + * Author: ChiaEn Wu <chiaen_wu@richtek.com> + */ + +#include <linux/backlight.h> +#include <linux/bitfield.h> +#include <linux/bits.h> +#include <linux/gpio/consumer.h> +#include <linux/kernel.h> +#include <linux/minmax.h> +#include <linux/mod_devicetable.h> +#include <linux/module.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_PWM_HYS_TH_MIN_STEP 1 +#define MT6370_BL_PWM_HYS_TH_MAX_STEP 64 +#define MT6370_BL_OVP_MIN_UV 17000000 +#define MT6370_BL_OVP_MAX_UV 29000000 +#define MT6370_BL_OVP_STEP_UV 4000000 +#define MT6370_BL_OCP_MIN_UA 900000 +#define MT6370_BL_OCP_MAX_UA 1800000 +#define MT6370_BL_OCP_STEP_UA 300000 +#define MT6370_BL_MAX_BRIGHTNESS 2048 +#define MT6370_BL_MAX_CH 15 + +enum { + MT6370_VID_COMMON = 0, + MT6370_VID_6372, +}; + +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); + + /* + * To make MT6372 using 14 bits to control the brightness + * backward compatible with 11 bits brightness control + * (like MT6370 and MT6371 do), we left shift the value + * and pad with 1 to remaining bits. Hence, the MT6372's + * backlight brightness will be almost the same as MT6370's + * and MT6371's. + */ + 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; + } + + 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, +}; + +static int mt6370_init_backlight_properties(struct mt6370_priv *priv, + struct backlight_properties *props) +{ + struct device *dev = priv->dev; + u8 prop_val; + u32 brightness, ovp_uV, ocp_uA; + unsigned int mask, val; + int ret; + + /* Vendor optional properties */ + val = 0; + if (device_property_read_bool(dev, "mediatek,bled-pwm-enable")) + val |= MT6370_BL_PWM_EN_MASK; + + if (device_property_read_bool(dev, "mediatek,bled-pwm-hys-enable")) + val |= MT6370_BL_PWM_HYS_EN_MASK; + + ret = device_property_read_u8(dev, + "mediatek,bled-pwm-hys-input-th-steps", + &prop_val); + if (!ret) { + prop_val = clamp_val(prop_val, + MT6370_BL_PWM_HYS_TH_MIN_STEP, + MT6370_BL_PWM_HYS_TH_MAX_STEP); + prop_val = prop_val <= 1 ? 0 : + prop_val <= 4 ? 1 : + prop_val <= 16 ? 2 : 3; + val |= prop_val << (ffs(MT6370_BL_PWM_HYS_SEL_MASK) - 1); + } + + ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_PWM, + val, val); + if (ret) + return ret; + + val = 0; + if (device_property_read_bool(dev, "mediatek,bled-ovp-shutdown")) + val |= MT6370_BL_OVP_EN_MASK; + + ret = device_property_read_u32(dev, "mediatek,bled-ovp-microvolt", + &ovp_uV); + if (!ret) { + ovp_uV = clamp_val(ovp_uV, MT6370_BL_OVP_MIN_UV, + MT6370_BL_OVP_MAX_UV); + ovp_uV = DIV_ROUND_UP(ovp_uV - MT6370_BL_OVP_MIN_UV, + MT6370_BL_OVP_STEP_UV); + val |= ovp_uV << (ffs(MT6370_BL_OVP_SEL_MASK) - 1); + } + + if (device_property_read_bool(dev, "mediatek,bled-ocp-shutdown")) + val |= MT6370_BL_OC_EN_MASK; + + ret = device_property_read_u32(dev, "mediatek,bled-ocp-microamp", + &ocp_uA); + if (!ret) { + ocp_uA = clamp_val(ocp_uA, MT6370_BL_OCP_MIN_UA, + MT6370_BL_OCP_MAX_UA); + ocp_uA = DIV_ROUND_UP(ocp_uA - MT6370_BL_OCP_MIN_UA, + MT6370_BL_OCP_STEP_UA); + val |= ocp_uA << (ffs(MT6370_BL_OC_SEL_MASK) - 1); + } + + ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BSTCTRL, + val, 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 || prop_val > MT6370_BL_MAX_CH) { + dev_err(dev, + "No channel specified or over than upper bound (%d)\n", + prop_val); + 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) +{ + /* + * MT6372 uses 14 bits to control the brightness but MT6370 and MT6371 + * use 11 bits. They are different so we have to use this function to + * check the vendor ID and use different methods to calculate the + * brightness. + */ + 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 backlight_properties props = { + .type = BACKLIGHT_RAW, + .scale = BACKLIGHT_SCALE_LINEAR, + }; + struct device *dev = &pdev->dev; + struct mt6370_priv *priv; + int ret; + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->dev = dev; + + priv->regmap = dev_get_regmap(dev->parent, NULL); + if (!priv->regmap) + return dev_err_probe(dev, -ENODEV, "Failed to get regmap\n"); + + ret = mt6370_check_vendor_info(priv); + if (ret) + return dev_err_probe(dev, ret, "Failed to check vendor info\n"); + + priv->enable_gpio = devm_gpiod_get_optional(dev, "enable", + GPIOD_OUT_HIGH); + if (IS_ERR(priv->enable_gpio)) + dev_err(dev, "Failed to get 'enable' gpio\n"); + + ret = mt6370_init_backlight_properties(priv, &props); + if (ret) + return dev_err_probe(dev, ret, + "Failed to init backlight properties\n"); + + priv->bl = devm_backlight_device_register(dev, pdev->name, dev, priv, + &mt6370_bl_ops, &props); + if (IS_ERR(priv->bl)) + return dev_err_probe(dev, PTR_ERR(priv->bl), + "Failed to register backlight\n"); + + 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");