Re: [PATCH v5 11/13] leds: mt6370: Add MediaTek MT6370 current sink type LED Indicator support
On Sun, Jul 17, 2022 at 10:46:43AM +0200, Pavel Machek wrote: > Hi! > > > The MediaTek MT6370 is a highly-integrated smart power management IC, > > which includes a single cell Li-Ion/Li-Polymer switching battery > > charger, a USB Type-C & Power Delivery (PD) controller, dual > > Flash LED current sources, a RGB LED driver, a backlight WLED driver, > > a display bias driver and a general LDO for portable devices. > > > > In MediaTek MT6370, there are four channel current-sink RGB LEDs that > > support hardware pattern for constant current, PWM, and breath mode. > > Isink4 channel can also be used as a CHG_VIN power good indicator. > > > > Signed-off-by: ChiYuan Huang > > > index a49979f..71bacb5 100644 > > --- a/drivers/leds/Kconfig > > +++ b/drivers/leds/Kconfig > > @@ -244,6 +244,20 @@ config LEDS_MT6323 > > This option enables support for on-chip LED drivers found on > > Mediatek MT6323 PMIC. > > > > +config LEDS_MT6370_RGB > > + tristate "LED Support for MediaTek MT6370 PMIC" > > + depends on LEDS_CLASS > > + depends on MFD_MT6370 > > + select LINEAR_RANGE > > + help > > + Say Y here to enable support for MT6370_RGB LED device. > > + In MT6370, there are four channel current-sink LED drivers that > > + support hardware pattern for constant current, PWM, and breath mode. > > + Isink4 channel can also be used as a CHG_VIN power good > > Should this go to leds/rgb directory, and should it depend on > multicolor framework? Yes, and I may also want to change the file name from 'leds-mt6370' to 'leds-mt6370-rgb'. Is it ok? > > Best regards, > Pavel > -- > People of Russia, stop Putin before his war on Ukraine escalates.
Re: [PATCH v5 11/13] leds: mt6370: Add MediaTek MT6370 current sink type LED Indicator support
ChiYuan Huang 於 2022年7月20日 週三 下午5:48寫道：
>
> ChiYuan Huang 於 2022年7月20日 週三 下午5:45寫道：
> >
> > On Fri, Jul 15, 2022 at 08:29:42PM +0200, Andy Shevchenko wrote:
> > > On Fri, Jul 15, 2022 at 1:29 PM ChiaEn Wu wrote:
> > > >
> > > > From: ChiYuan Huang
> > > >
> > > > The MediaTek MT6370 is a highly-integrated smart power management IC,
> > > > which includes a single cell Li-Ion/Li-Polymer switching battery
> > > > charger, a USB Type-C & Power Delivery (PD) controller, dual
> > > > Flash LED current sources, a RGB LED driver, a backlight WLED driver,
> > > > a display bias driver and a general LDO for portable devices.
> > > >
> > > > In MediaTek MT6370, there are four channel current-sink RGB LEDs that
> > > > support hardware pattern for constant current, PWM, and breath mode.
> > > > Isink4 channel can also be used as a CHG_VIN power good indicator.
> > >
> > > ...
> > >
> > > > + This driver can also be built as a module. If so the module
> > >
> > > so, the
> > >
> > > > + will be called "leds-mt6370.ko".
> > >
> > > No ".ko".
> > >
> > > Why did you ignore these comments? Please go and fix _everywhere_ in
> > > your series.
> > > It's basically the rule of thumb, if the reviewer gives a comment
> > > against an occurrence of something, go through entire series and check
> > > if there are other places like commented one and address them all.
> > >
> > > ...
> > >
> > > > + * Author: Alice Chen
> > >
> > > Strange, the commit message doesn't have a corresponding SoB, why?
> > >
> > Yes, there're two authors Alice and me.
> > I'll correct it in next.
> > > ...
> > >
> > > > +#define MT6370_PWM_DUTY31
> > > > +#define MT6372_PMW_DUTY255
> > >
> > > Looks like these are limits by hardware?
> > > Check with the datasheet if (BIT(x) - 1) makes more sense here.
> > >
> > > ...
> > >
> > > > + switch (led_no) {
> > > > + case MT6370_LED_ISNK1:
> > > > + sel_field = F_LED1_DUTY;
> > > > + break;
> > > > + case MT6370_LED_ISNK2:
> > > > + sel_field = F_LED2_DUTY;
> > > > + break;
> > > > + case MT6370_LED_ISNK3:
> > > > + sel_field = F_LED3_DUTY;
> > > > + break;
> > > > + default:
> > > > + sel_field = F_LED4_DUTY;
> > >
> > > Missed break;
> > >
> > > > + }
> > >
> > > ...
> > >
> > > > + switch (led_no) {
> > > > + case MT6370_LED_ISNK1:
> > > > + sel_field = F_LED1_FREQ;
> > > > + break;
> > > > + case MT6370_LED_ISNK2:
> > > > + sel_field = F_LED2_FREQ;
> > > > + break;
> > > > + case MT6370_LED_ISNK3:
> > > > + sel_field = F_LED3_FREQ;
> > > > + break;
> > > > + default:
> > > > + sel_field = F_LED4_FREQ;
> > >
> > > Ditto.
> > >
> > > > + }
> > >
> > > ...
> > >
> > > > + switch (led_no) {
> > > > + case MT6370_LED_ISNK1:
> > > > + case MT6370_LED_ISNK2:
> > > > + case MT6370_LED_ISNK3:
> > > > + *base = MT6370_REG_RGB1_TR + led_no * 3;
> > > > + break;
> > > > + default:
> > > > + *base = MT6370_REG_RGB_CHRIND_TR;
> > >
> > > Ditto.
> > > It seems you dropped them for all switch-cases. It's not goot, please
> > > restore them back.
> > >
> > > > + }
> > >
> > > ...
> > >
> > > > + u8 val[P_MAX_PATTERNS / 2] = {0};
> > >
> > > { } should suffice
> > >
> > >
> > In the above range selector, we use the 'logic or' to generate the
> typo, it's 'below'.
> > pattern values.
> >
Ah, found in c11 standard 6.7.9 item 21
It is the same as 'static storage durat
Re: [PATCH v5 11/13] leds: mt6370: Add MediaTek MT6370 current sink type LED Indicator support
ChiYuan Huang 於 2022年7月20日 週三 下午5:45寫道：
>
> On Fri, Jul 15, 2022 at 08:29:42PM +0200, Andy Shevchenko wrote:
> > On Fri, Jul 15, 2022 at 1:29 PM ChiaEn Wu wrote:
> > >
> > > From: ChiYuan Huang
> > >
> > > The MediaTek MT6370 is a highly-integrated smart power management IC,
> > > which includes a single cell Li-Ion/Li-Polymer switching battery
> > > charger, a USB Type-C & Power Delivery (PD) controller, dual
> > > Flash LED current sources, a RGB LED driver, a backlight WLED driver,
> > > a display bias driver and a general LDO for portable devices.
> > >
> > > In MediaTek MT6370, there are four channel current-sink RGB LEDs that
> > > support hardware pattern for constant current, PWM, and breath mode.
> > > Isink4 channel can also be used as a CHG_VIN power good indicator.
> >
> > ...
> >
> > > + This driver can also be built as a module. If so the module
> >
> > so, the
> >
> > > + will be called "leds-mt6370.ko".
> >
> > No ".ko".
> >
> > Why did you ignore these comments? Please go and fix _everywhere_ in
> > your series.
> > It's basically the rule of thumb, if the reviewer gives a comment
> > against an occurrence of something, go through entire series and check
> > if there are other places like commented one and address them all.
> >
> > ...
> >
> > > + * Author: Alice Chen
> >
> > Strange, the commit message doesn't have a corresponding SoB, why?
> >
> Yes, there're two authors Alice and me.
> I'll correct it in next.
> > ...
> >
> > > +#define MT6370_PWM_DUTY31
> > > +#define MT6372_PMW_DUTY255
> >
> > Looks like these are limits by hardware?
> > Check with the datasheet if (BIT(x) - 1) makes more sense here.
> >
> > ...
> >
> > > + switch (led_no) {
> > > + case MT6370_LED_ISNK1:
> > > + sel_field = F_LED1_DUTY;
> > > + break;
> > > + case MT6370_LED_ISNK2:
> > > + sel_field = F_LED2_DUTY;
> > > + break;
> > > + case MT6370_LED_ISNK3:
> > > + sel_field = F_LED3_DUTY;
> > > + break;
> > > + default:
> > > + sel_field = F_LED4_DUTY;
> >
> > Missed break;
> >
> > > + }
> >
> > ...
> >
> > > + switch (led_no) {
> > > + case MT6370_LED_ISNK1:
> > > + sel_field = F_LED1_FREQ;
> > > + break;
> > > + case MT6370_LED_ISNK2:
> > > + sel_field = F_LED2_FREQ;
> > > + break;
> > > + case MT6370_LED_ISNK3:
> > > + sel_field = F_LED3_FREQ;
> > > + break;
> > > + default:
> > > + sel_field = F_LED4_FREQ;
> >
> > Ditto.
> >
> > > + }
> >
> > ...
> >
> > > + switch (led_no) {
> > > + case MT6370_LED_ISNK1:
> > > + case MT6370_LED_ISNK2:
> > > + case MT6370_LED_ISNK3:
> > > + *base = MT6370_REG_RGB1_TR + led_no * 3;
> > > + break;
> > > + default:
> > > + *base = MT6370_REG_RGB_CHRIND_TR;
> >
> > Ditto.
> > It seems you dropped them for all switch-cases. It's not goot, please
> > restore them back.
> >
> > > + }
> >
> > ...
> >
> > > + u8 val[P_MAX_PATTERNS / 2] = {0};
> >
> > { } should suffice
> >
> >
> In the above range selector, we use the 'logic or' to generate the
typo, it's 'below'.
> pattern values.
>
> If to change it from '{0} to '{ }', is it correct?
> > > + /*
> > > +* Pattern list
> > > +* tr1: byte 0, b'[7: 4]
> > > +* tr2: byte 0, b'[3: 0]
> > > +* tf1: byte 1, b'[7: 4]
> > > +* tf2: byte 1, b'[3: 0]
> > > +* ton: byte 2, b'[7: 4]
> > > +* toff: byte 2, b'[3: 0]
> > > +*/
> > > + for (i = 0; i < P_MAX_PATTERNS; i++) {
> > > + curr = pattern + i;
> > > +
> > > + sel_range = i == P_LED_TOFF ? R_LED_TOFF : R_L
Re: [PATCH v5 11/13] leds: mt6370: Add MediaTek MT6370 current sink type LED Indicator support
On Fri, Jul 15, 2022 at 08:29:42PM +0200, Andy Shevchenko wrote:
> On Fri, Jul 15, 2022 at 1:29 PM ChiaEn Wu wrote:
> >
> > From: ChiYuan Huang
> >
> > The MediaTek MT6370 is a highly-integrated smart power management IC,
> > which includes a single cell Li-Ion/Li-Polymer switching battery
> > charger, a USB Type-C & Power Delivery (PD) controller, dual
> > Flash LED current sources, a RGB LED driver, a backlight WLED driver,
> > a display bias driver and a general LDO for portable devices.
> >
> > In MediaTek MT6370, there are four channel current-sink RGB LEDs that
> > support hardware pattern for constant current, PWM, and breath mode.
> > Isink4 channel can also be used as a CHG_VIN power good indicator.
>
> ...
>
> > + This driver can also be built as a module. If so the module
>
> so, the
>
> > + will be called "leds-mt6370.ko".
>
> No ".ko".
>
> Why did you ignore these comments? Please go and fix _everywhere_ in
> your series.
> It's basically the rule of thumb, if the reviewer gives a comment
> against an occurrence of something, go through entire series and check
> if there are other places like commented one and address them all.
>
> ...
>
> > + * Author: Alice Chen
>
> Strange, the commit message doesn't have a corresponding SoB, why?
>
Yes, there're two authors Alice and me.
I'll correct it in next.
> ...
>
> > +#define MT6370_PWM_DUTY31
> > +#define MT6372_PMW_DUTY255
>
> Looks like these are limits by hardware?
> Check with the datasheet if (BIT(x) - 1) makes more sense here.
>
> ...
>
> > + switch (led_no) {
> > + case MT6370_LED_ISNK1:
> > + sel_field = F_LED1_DUTY;
> > + break;
> > + case MT6370_LED_ISNK2:
> > + sel_field = F_LED2_DUTY;
> > + break;
> > + case MT6370_LED_ISNK3:
> > + sel_field = F_LED3_DUTY;
> > + break;
> > + default:
> > + sel_field = F_LED4_DUTY;
>
> Missed break;
>
> > + }
>
> ...
>
> > + switch (led_no) {
> > + case MT6370_LED_ISNK1:
> > + sel_field = F_LED1_FREQ;
> > + break;
> > + case MT6370_LED_ISNK2:
> > + sel_field = F_LED2_FREQ;
> > + break;
> > + case MT6370_LED_ISNK3:
> > + sel_field = F_LED3_FREQ;
> > + break;
> > + default:
> > + sel_field = F_LED4_FREQ;
>
> Ditto.
>
> > + }
>
> ...
>
> > + switch (led_no) {
> > + case MT6370_LED_ISNK1:
> > + case MT6370_LED_ISNK2:
> > + case MT6370_LED_ISNK3:
> > + *base = MT6370_REG_RGB1_TR + led_no * 3;
> > + break;
> > + default:
> > + *base = MT6370_REG_RGB_CHRIND_TR;
>
> Ditto.
> It seems you dropped them for all switch-cases. It's not goot, please
> restore them back.
>
> > + }
>
> ...
>
> > + u8 val[P_MAX_PATTERNS / 2] = {0};
>
> { } should suffice
>
>
In the above range selector, we use the 'logic or' to generate the
pattern values.
If to change it from '{0} to '{ }', is it correct?
> > + /*
> > +* Pattern list
> > +* tr1: byte 0, b'[7: 4]
> > +* tr2: byte 0, b'[3: 0]
> > +* tf1: byte 1, b'[7: 4]
> > +* tf2: byte 1, b'[3: 0]
> > +* ton: byte 2, b'[7: 4]
> > +* toff: byte 2, b'[3: 0]
> > +*/
> > + for (i = 0; i < P_MAX_PATTERNS; i++) {
> > + curr = pattern + i;
> > +
> > + sel_range = i == P_LED_TOFF ? R_LED_TOFF : R_LED_TRFON;
> > +
> > + linear_range_get_selector_within(priv->ranges + sel_range,
> > +curr->delta_t, &sel);
> > +
> > + val[i / 2] |= sel << (4 * ((i + 1) % 2));
> > + }
> > +
> > + memcpy(pattern_val, val, 3);
> > + return 0;
> > +}
>
> ...
>
> > +out:
>
> out_unlock:
>
> > + mutex_unlock(&priv->lock);
> > +
> > + return ret;
>
> ...
>
> > +out:
>
> Ditto. And so on.
>
> > + mutex_unlock(&priv->lock);
> >
Re: [PATCH v5 08/13] usb: typec: tcpci_mt6370: Add MediaTek MT6370 tcpci driver
Andy Shevchenko 於 2022年7月18日 週一 晚上7:39寫道：
>
> On Mon, Jul 18, 2022 at 10:08 AM ChiYuan Huang wrote:
> > On Fri, Jul 15, 2022 at 03:10:42PM +0200, Andy Shevchenko wrote:
> > > On Fri, Jul 15, 2022 at 1:28 PM ChiaEn Wu wrote:
>
> ...
>
> > > > This commit add support for the Type-C & Power Delivery controller in
> > >
> > > This commit add -> Add
> > >
> > Upper case? Or rewrite it as 'This commit is to add .'?
>
> Please, read this documentation [1] for better understanding. It
> should clarify this and perhaps other possible questions.
>
> [1]:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>
I'm thinking why to change it from 'add' to "Add'.
Ah, I misunderstand it.
> > > > MediaTek MT6370 IC.
>
> ...
>
> > > > + ret = devm_request_threaded_irq(dev, priv->irq, NULL,
> > > > + mt6370_irq_handler,
> > > > IRQF_ONESHOT,
> > > > + dev_name(dev), priv);
> > > > + if (ret) {
> > >
> > > > + tcpci_unregister_port(priv->tcpci);
> > >
> > > This is wrong.
> > > You mixed devm_ with non-devm. Either drop devm_ *after* the first
> > > non-devm_ call, or convert everything to be managed.
> > >
> > How about to add 'devm_add_action_or_reset' for tcpci_unregister_port?
> > This will convert all as 'devm_' version.
>
> I think it would work, that wrapper was designed to cover cases like this.
>
> > > > + return dev_err_probe(dev, ret, "Failed to allocate
> > > > irq\n");
> > > > + }
>
> ...
>
> > > > +static int mt6370_tcpc_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct mt6370_priv *priv = platform_get_drvdata(pdev);
> > >
> > > > + disable_irq(priv->irq);
> > >
> > > Why?
> > > An ugly workaround due to ordering issues in ->probe()?
> > >
> > Yes, due to the ordering in probe.
> > 'bus remove' will be called before device resource releases.
> >
> > Like as you said, another way is to convert all as non-devm
> > version after 'tcpci_unregister_port'.
> >
> > If to keep the original order, 'disable_irq' before
> > 'tcpci_unregister_port' can make the flow more safe.
> >
> > Or you can think one case if irq triggers after
> > 'tcpci_unregister_port'. Null pointer occurs.
> >
> > Anyway, in next revision, I'll convert all to be 'devm_' version.
> > For this remove callback, only 'dev_pm_clear_wake_irq' and
> > 'device_init_wakeup' will be kept.
> >
> > Is this better?
>
> Sounds like a plan!
>
Already did. Just to double confirm the changes.
Thanks. All are clear.
> > > > + tcpci_unregister_port(priv->tcpci);
> > > > + dev_pm_clear_wake_irq(&pdev->dev);
> > > > + device_init_wakeup(&pdev->dev, false);
> > > > +
> > > > + return 0;
> > > > +}
>
> --
> With Best Regards,
> Andy Shevchenko
Re: [PATCH v5 08/13] usb: typec: tcpci_mt6370: Add MediaTek MT6370 tcpci driver
On Fri, Jul 15, 2022 at 03:10:42PM +0200, Andy Shevchenko wrote:
> On Fri, Jul 15, 2022 at 1:28 PM ChiaEn Wu wrote:
>
> > The MT6370 is a highly-integrated smart power management IC, which
> > includes a single cell Li-Ion/Li-Polymer switching battery charger,
> > a USB Type-C & Power Delivery (PD) controller, dual Flash LED current
> > sources, a RGB LED driver, a backlight WLED driver, a display bias
> > driver and a general LDO for portable devices.
> >
> > This commit add support for the Type-C & Power Delivery controller in
>
> This commit add -> Add
>
Upper case? Or rewrite it as 'This commit is to add .'?
>
> > MediaTek MT6370 IC.
>
>
> > +static int mt6370_tcpc_probe(struct platform_device *pdev)
> > +{
> > + struct mt6370_priv *priv;
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->dev = dev;
> > + platform_set_drvdata(pdev, priv);
> > +
> > + priv->tcpci_data.regmap = dev_get_regmap(dev->parent, NULL);
> > + if (!priv->tcpci_data.regmap)
> > + return dev_err_probe(dev, -ENODEV, "Failed to init
> > regmap\n");
> > +
> > + ret = mt6370_check_vendor_info(priv);
> > + if (ret)
> > + return ret;
> > +
> > + priv->irq = platform_get_irq(pdev, 0);
> > + if (priv->irq < 0)
> > + return priv->irq;
> > +
> > + /* Assign TCPCI feature and ops */
> > + priv->tcpci_data.auto_discharge_disconnect = 1;
> > + priv->tcpci_data.init = mt6370_tcpc_init;
> > + priv->tcpci_data.set_vconn = mt6370_tcpc_set_vconn;
> > +
> > + priv->vbus = devm_regulator_get_optional(dev, "vbus");
> > + if (!IS_ERR(priv->vbus))
> > + priv->tcpci_data.set_vbus = mt6370_tcpc_set_vbus;
> > +
> > + priv->tcpci = tcpci_register_port(dev, &priv->tcpci_data);
> > + if (IS_ERR(priv->tcpci))
> > + return dev_err_probe(dev, PTR_ERR(priv->tcpci),
> > +"Failed to register tcpci port\n");
> > +
> > + ret = devm_request_threaded_irq(dev, priv->irq, NULL,
> > + mt6370_irq_handler, IRQF_ONESHOT,
> > + dev_name(dev), priv);
> > + if (ret) {
>
> > + tcpci_unregister_port(priv->tcpci);
>
> This is wrong.
> You mixed devm_ with non-devm. Either drop devm_ *after* the first
> non-devm_ call, or convert everything to be managed.
>
How about to add 'devm_add_action_or_reset' for tcpci_unregister_port?
This will convert all as 'devm_' version.
> > + return dev_err_probe(dev, ret, "Failed to allocate irq\n");
> > + }
> > +
> > + device_init_wakeup(dev, true);
> > + dev_pm_set_wake_irq(dev, priv->irq);
> > +
> > + return 0;
> > +}
> > +
> > +static int mt6370_tcpc_remove(struct platform_device *pdev)
> > +{
> > + struct mt6370_priv *priv = platform_get_drvdata(pdev);
>
> > + disable_irq(priv->irq);
>
> Why?
> An ugly workaround due to ordering issues in ->probe()?
>
Yes, due to the ordering in probe.
'bus remove' will be called before device resource releases.
Like as you said, another way is to convert all as non-devm
version after 'tcpci_unregister_port'.
If to keep the original order, 'disable_irq' before
'tcpci_unregister_port' can make the flow more safe.
Or you can think one case if irq triggers after
'tcpci_unregister_port'. Null pointer occurs.
Anyway, in next revision, I'll convert all to be 'devm_' version.
For this remove callback, only 'dev_pm_clear_wake_irq' and
'device_init_wakeup' will be kept.
Is this better?
> > + tcpci_unregister_port(priv->tcpci);
> > + dev_pm_clear_wake_irq(&pdev->dev);
> > + device_init_wakeup(&pdev->dev, false);
> > +
> > + return 0;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
Re: [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection
Krzysztof Kozlowski 於 2022年6月6日 週一 上午12:11寫道： > > On 02/06/2022 17:31, ChiYuan Huang wrote: > > Krzysztof Kozlowski 於 2022年6月2日 週四 > > 下午9:58寫道： > >> > >> On 02/06/2022 15:56, Rob Herring wrote: > >>> On Thu, May 26, 2022 at 12:32:12PM +0200, Krzysztof Kozlowski wrote: > >>>> On 26/05/2022 10:13, ChiYuan Huang wrote: > >>>>> Krzysztof Kozlowski 於 2022年5月26日 週四 > >>>>> 下午4:06寫道： > >>>>>> > >>>>>> On 26/05/2022 05:16, cy_huang wrote: > >>>>>>> From: ChiYuan Huang > >>>>>>> > >>>>>>> Add the new property for ocp level selection. > >>>>>>> > >>>>>>> Signed-off-by: ChiYuan Huang > >>>>>>> --- > >>>>>>> .../bindings/leds/backlight/richtek,rt4831-backlight.yaml | > >>>>>>> 8 > >>>>>>> include/dt-bindings/leds/rt4831-backlight.h | > >>>>>>> 5 + > >>>>>>> 2 files changed, 13 insertions(+) > >>>>>>> > >>>>>>> diff --git > >>>>>>> a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > >>>>>>> > >>>>>>> b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > >>>>>>> index e0ac686..c1c59de 100644 > >>>>>>> --- > >>>>>>> a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > >>>>>>> +++ > >>>>>>> b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > >>>>>>> @@ -47,6 +47,14 @@ properties: > >>>>>>> minimum: 0 > >>>>>>> maximum: 3 > >>>>>>> > >>>>>>> + richtek,bled-ocp-sel: > >>>>>> > >>>>>> Skip "sel" as it is a shortcut of selection. Name instead: > >>>>>> "richtek,backlight-ocp" > >>>>>> > >>>>> OK, if so, do I need to rename all properties from 'bled' to > >>>>> 'backlight' ? > >>>>> If only this property is naming as 'backlight'. it may conflict with > >>>>> the others like as "richtek,bled-ovp-sel". > >>>> > >>>> Ah, no, no need. > >>>> > >>>>>> > >>>>>>> +description: | > >>>>>>> + Backlight OCP level selection, currently support > >>>>>>> 0.9A/1.2A/1.5A/1.8A > >>>>>> > >>>>>> Could you explain here what is OCP (unfold the acronym)? > >>>>> Yes. And the full name is 'over current protection'. > >>>> > >>>> Thanks and this leads to second thing - you encode register value > >>>> instead of logical value. This must be a logical value in mA, so > >>>> "richtek,bled-ocp-microamp". > >>> > >>> We already have common properties for setting current of LEDs. We should > >>> use that here I think. > >> > >> It might not be exactly the same. We have "led-max-microamp" which is > >> the maximum allowed current. I guess over-current protection level is > >> slightly higher (e.g. led-max-microamp + 1). IOW, led-max-microamp is > >> something which still can be set and used by system/hardware. OCP should > >> not. > >> > > Yap, you're right. > > So I am right or Rob? > As I know, both are incorrect. > > From the modern backlight IC design, it uses the boost converter > > architecture. > > This OCP level is to limit the inductor current when the internal MOS > > switch turn on. > > Details can refer to the below wiki link > > https://en.wikipedia.org/wiki/Boost_converter > > > > And based on it, OVP is used to limit the inductor output voltage. > > Each channel maximum current is based on the IC affordable limit. > > It is more like as what you said 'led-max-microamp'. > > > > So boost voltage level may depend on the LED VF. > > The different series of LED may cause different boost voltage. > > > > RT4831's OVP/OCP is not just the protection, more like as the limit. > > This suggests Rob is right, so let's use led-max-microamp property? > No, the meaning is different. 'led-max-microamp' always means the channel output current. It already can be adjusted by backlight brightness node. For example low voltage side (3.3~4.4V) to generate the boost voltage to 16~17V, even 20V for BLED Vout. This OCP is to limit the input current of low voltage side. After the explanation, do you still think it's the same thing? > Best regards, > Krzysztof
Re: [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection
Krzysztof Kozlowski 於 2022年6月2日 週四 下午9:58寫道： > > On 02/06/2022 15:56, Rob Herring wrote: > > On Thu, May 26, 2022 at 12:32:12PM +0200, Krzysztof Kozlowski wrote: > >> On 26/05/2022 10:13, ChiYuan Huang wrote: > >>> Krzysztof Kozlowski 於 2022年5月26日 週四 > >>> 下午4:06寫道： > >>>> > >>>> On 26/05/2022 05:16, cy_huang wrote: > >>>>> From: ChiYuan Huang > >>>>> > >>>>> Add the new property for ocp level selection. > >>>>> > >>>>> Signed-off-by: ChiYuan Huang > >>>>> --- > >>>>> .../bindings/leds/backlight/richtek,rt4831-backlight.yaml | 8 > >>>>> > >>>>> include/dt-bindings/leds/rt4831-backlight.h | 5 > >>>>> + > >>>>> 2 files changed, 13 insertions(+) > >>>>> > >>>>> diff --git > >>>>> a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > >>>>> > >>>>> b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > >>>>> index e0ac686..c1c59de 100644 > >>>>> --- > >>>>> a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > >>>>> +++ > >>>>> b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > >>>>> @@ -47,6 +47,14 @@ properties: > >>>>> minimum: 0 > >>>>> maximum: 3 > >>>>> > >>>>> + richtek,bled-ocp-sel: > >>>> > >>>> Skip "sel" as it is a shortcut of selection. Name instead: > >>>> "richtek,backlight-ocp" > >>>> > >>> OK, if so, do I need to rename all properties from 'bled' to 'backlight' ? > >>> If only this property is naming as 'backlight'. it may conflict with > >>> the others like as "richtek,bled-ovp-sel". > >> > >> Ah, no, no need. > >> > >>>> > >>>>> +description: | > >>>>> + Backlight OCP level selection, currently support > >>>>> 0.9A/1.2A/1.5A/1.8A > >>>> > >>>> Could you explain here what is OCP (unfold the acronym)? > >>> Yes. And the full name is 'over current protection'. > >> > >> Thanks and this leads to second thing - you encode register value > >> instead of logical value. This must be a logical value in mA, so > >> "richtek,bled-ocp-microamp". > > > > We already have common properties for setting current of LEDs. We should > > use that here I think. > > It might not be exactly the same. We have "led-max-microamp" which is > the maximum allowed current. I guess over-current protection level is > slightly higher (e.g. led-max-microamp + 1). IOW, led-max-microamp is > something which still can be set and used by system/hardware. OCP should > not. > Yap, you're right. >From the modern backlight IC design, it uses the boost converter architecture. This OCP level is to limit the inductor current when the internal MOS switch turn on. Details can refer to the below wiki link https://en.wikipedia.org/wiki/Boost_converter And based on it, OVP is used to limit the inductor output voltage. Each channel maximum current is based on the IC affordable limit. It is more like as what you said 'led-max-microamp'. So boost voltage level may depend on the LED VF. The different series of LED may cause different boost voltage. RT4831's OVP/OCP is not just the protection, more like as the limit. > > Best regards, > Krzysztof
Re: [PATCH 06/14] leds: mt6370: Add Mediatek MT6370 Indicator support
Andy Shevchenko 於 2022年6月2日 週四 下午5:18寫道：
>
> On Thu, Jun 2, 2022 at 8:27 AM ChiYuan Huang wrote:
> > On Wed, Jun 01, 2022 at 11:48:58AM +0200, Andy Shevchenko wrote:
> > > On Tue, May 31, 2022 at 1:16 PM ChiaEn Wu wrote:
>
> ...
>
> > > What indicator?
> > It's RGB curent sink type LED driver (maximum supported current is only
> > 24mA).
>
> Make your commit messages a slightly more verbose.
>
OK, will refine the commit message in next.
> ...
>
> > > > +#include
> > >
> > > Are you sure this is the correct header? Seems you need
> > > mod_devicetable.h instead.
> > >
> > It's the correct header and be used for the struct 'of_device_id'.
>
> Nope. Run the following command
> $ git grep -n 'struct of_device_id {' -- include/linux/
>
Got it, thanks.
> ...
>
> > > > +struct mt6370_priv {
> > > > + struct mutex lock;
> > >
> > > Do you use regmap locking?
> > >
> > MFD regmap register already the access lock.
> >
> > This lock is just to guarantee only one user can access the RGB register
> > part.
> >
> > Sorry, from the comment, do you want us to rename or remove this lock?
>
> My point is, since you have two locks, explain why you need each of them.
>
OK, will leave a comment line to explain the usage of this lock.
> > > > + struct device *dev;
> > >
> > > > + struct regmap *regmap;
> > >
> > > > + struct regmap_field *fields[F_MAX_FIELDS];
> > > > + const struct reg_field *reg_fields;
> > > > + const struct linear_range *ranges;
> > > > + struct reg_cfg *reg_cfgs;
> > > > + unsigned int leds_count;
> > > > + unsigned int leds_active;
> > > > + bool is_mt6372;
> > > > + struct mt6370_led leds[];
> > > > +};
>
>
> --
> With Best Regards,
> Andy Shevchenko
Re: [PATCH 06/14] leds: mt6370: Add Mediatek MT6370 Indicator support
On Wed, Jun 01, 2022 at 11:48:58AM +0200, Andy Shevchenko wrote:
> On Tue, May 31, 2022 at 1:16 PM ChiaEn Wu wrote:
> >
> > From: Alice Chen
>
> All below comments are applicable to the rest of the series as well
> (one way or another), so please fix all your patches where it's
> appropriate.
>
> >
> > Add Mediatek MT6370 Indicator support
>
> What indicator?
It's RGB curent sink type LED driver (maximum supported current is only 24mA).
> Please also keep attention on English punctuation (missed period).
>
Ack in next.
> ...
>
> > + help
> > + Support 4 channels and reg/pwm/breath mode.
> > + Isink4 can also use as a CHG_VIN power good Indicator.
>
> be used
>
Ack in next.
> > + Say Y here to enable support for
> > + MT6370_RGB_LED device.
>
> ...
>
> > +#include
> > +#include
> > +#include
>
> > +#include
>
> Are you sure this is the correct header? Seems you need
> mod_devicetable.h instead.
>
It's the correct header and be used for the struct 'of_device_id'.
> > +#include
> > +#include
>
> ...
>
> > +struct mt6370_priv {
> > + struct mutex lock;
>
> Do you use regmap locking?
>
MFD regmap register already the access lock.
This lock is just to guarantee only one user can access the RGB register
part.
Sorry, from the comment, do you want us to rename or remove this lock?
> > + struct device *dev;
>
> > + struct regmap *regmap;
>
> > + struct regmap_field *fields[F_MAX_FIELDS];
> > + const struct reg_field *reg_fields;
> > + const struct linear_range *ranges;
> > + struct reg_cfg *reg_cfgs;
> > + unsigned int leds_count;
> > + unsigned int leds_active;
> > + bool is_mt6372;
> > + struct mt6370_led leds[];
> > +};
>
> ...
>
> > +static const unsigned int common_tfreqs[] = {
> > + 1, 5000, 2000, 1000, 500, 200, 5, 1
>
> Leave a comma at the end.
>
Ack in next.
> > +};
> > +
> > +static const unsigned int mt6372_tfreqs[] = {
> > + 8000, 4000, 2000, 1000, 500, 250, 8, 4
>
> Ditto.
>
Ack in next.
> > +};
>
>
> --
> With Best Regards,
> Andy Shevchenko
Re: [PATCH 2/2] backlight: rt4831: Add the property parsing for ocp level selection
Daniel Thompson 於 2022年5月26日 週四 下午6:05寫道：
>
> On Thu, May 26, 2022 at 11:16:35AM +0800, cy_huang wrote:
> > From: ChiYuan Huang
> >
> > Add the property parsing for ocp level selection.
>
> Isn't this just restating the Subject: line?
>
Ah, that's my fault. I didn't state too much in the patch comment.
I only left it in the cover letter.
> It would be better to provide information useful to the reviewer here.
> Something like:
>
> "Currently this driver simply inherits whatever over-current protection
> level is programmed into the hardware when it is handed over. Typically
> this will be the reset value, A, although the bootloader could
> have established a different value.
>
> Allow the correct OCP value to be provided by the DT."
>
> BTW please don't uncritically copy the above into the patch header. It is
> just made something up as an example and I did no fact checking...
>
OK, got it.
>
> >
> > Reported-by: Lucas Tsai
> > Signed-off-by: ChiYuan Huang
> > ---
> > drivers/video/backlight/rt4831-backlight.c | 13 +
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/video/backlight/rt4831-backlight.c
> > b/drivers/video/backlight/rt4831-backlight.c
> > index 42155c7..c81f7d9 100644
> > --- a/drivers/video/backlight/rt4831-backlight.c
> > +++ b/drivers/video/backlight/rt4831-backlight.c
> > @@ -12,6 +12,7 @@
> > #define RT4831_REG_BLCFG 0x02
> > #define RT4831_REG_BLDIML0x04
> > #define RT4831_REG_ENABLE0x08
> > +#define RT4831_REG_BLOPT20x11
> >
> > #define RT4831_BLMAX_BRIGHTNESS 2048
> >
> > @@ -23,6 +24,8 @@
> > #define RT4831_BLDIML_MASK GENMASK(2, 0)
> > #define RT4831_BLDIMH_MASK GENMASK(10, 3)
> > #define RT4831_BLDIMH_SHIFT 3
> > +#define RT4831_BLOCP_MASKGENMASK(1, 0)
> > +#define RT4831_BLOCP_SHIFT 0
> >
> > struct rt4831_priv {
> > struct device *dev;
> > @@ -120,6 +123,16 @@ static int rt4831_parse_backlight_properties(struct
> > rt4831_priv *priv,
> > if (ret)
> > return ret;
> >
> > + ret = device_property_read_u8(dev, "richtek,bled-ocp-sel", &propval);
> > + if (ret)
> > + propval = RT4831_BLOCPLVL_1P2A;
>
> Is 1.2A the reset value for the register?
Yes, it's the HW default value.
>
> Additionally, it looks like adding a hard-coded default would cause
> problems for existing platforms where the bootloader doesn't use
> richtek,bled-ocp-sel and pre-configures a different value itself.
>
> Would it be safer (in terms of working nicely with older bootloaders)
> to only write to the RT4831_BLOCP_MASK if the new property is set?
>
Ah, my excuse. I really didn't consider the case that you mentioned.
It seems it's better to do the judgement here for two cases.
1) property not exist, keep the current HW value
2) property exist, clamp align and update the suitable selector to HW.
Thanks.
>
> Daniel.
>
>
>
> > +
> > + propval = min_t(u8, propval, RT4831_BLOCPLVL_1P8A);
> > + ret = regmap_update_bits(priv->regmap, RT4831_REG_BLOPT2,
> > RT4831_BLOCP_MASK,
> > + propval << RT4831_BLOCP_SHIFT);
> > + if (ret)
> > + return ret;
> > +
> > ret = device_property_read_u8(dev, "richtek,channel-use", &propval);
> > if (ret) {
> > dev_err(dev, "richtek,channel-use DT property missing\n");
> > --
> > 2.7.4
> >
Re: [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection
Krzysztof Kozlowski 於 2022年5月26日 週四 下午4:06寫道： > > On 26/05/2022 05:16, cy_huang wrote: > > From: ChiYuan Huang > > > > Add the new property for ocp level selection. > > > > Signed-off-by: ChiYuan Huang > > --- > > .../bindings/leds/backlight/richtek,rt4831-backlight.yaml | 8 > > > > include/dt-bindings/leds/rt4831-backlight.h | 5 + > > 2 files changed, 13 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > > > > b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > > index e0ac686..c1c59de 100644 > > --- > > a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > > +++ > > b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > > @@ -47,6 +47,14 @@ properties: > > minimum: 0 > > maximum: 3 > > > > + richtek,bled-ocp-sel: > > Skip "sel" as it is a shortcut of selection. Name instead: > "richtek,backlight-ocp" > OK, if so, do I need to rename all properties from 'bled' to 'backlight' ? If only this property is naming as 'backlight'. it may conflict with the others like as "richtek,bled-ovp-sel". > > > +description: | > > + Backlight OCP level selection, currently support 0.9A/1.2A/1.5A/1.8A > > Could you explain here what is OCP (unfold the acronym)? Yes. And the full name is 'over current protection'. > > > Best regards, > Krzysztof
Re: [RESEND PATCH v6 2/4] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
Hi, Rob Herring 於 2021年5月1日 週六 上午4:10寫道： > > On Mon, Apr 26, 2021 at 03:18:09PM +0800, cy_huang wrote: > > From: ChiYuan Huang > > > > Adds DT binding document for Richtek RT4831 backlight. > > > > Signed-off-by: ChiYuan Huang > > Reviewed-by: Daniel Thompson > > --- > > Resend this v6 patch series to loop devicetree reviewers. > > > > For next, need to add the below line > > Reviewed-by: Daniel Thompson > > --- > > .../leds/backlight/richtek,rt4831-backlight.yaml | 65 > > ++ > > include/dt-bindings/leds/rt4831-backlight.h| 23 > > 2 files changed, 88 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > > create mode 100644 include/dt-bindings/leds/rt4831-backlight.h > > > > diff --git > > a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > > > > b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > > new file mode 100644 > > index ..4da6a66 > > --- /dev/null > > +++ > > b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > > @@ -0,0 +1,65 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: > > http://devicetree.org/schemas/leds/backlight/richtek,rt4831-backlight.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Richtek RT4831 Backlight > > + > > +maintainers: > > + - ChiYuan Huang > > + > > +description: | > > + RT4831 is a mutifunctional device that can provide power to the LCD > > display > > + and LCD backlight. > > + > > + For the LCD backlight, it can provide four channel WLED driving > > capability. > > + Each channel driving current is up to 30mA > > + > > + Datasheet is available at > > + https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf > > + > > Need to reference common backlight schema: > > allOf: > - $ref: common.yaml# > > > +properties: > > + compatible: > > +const: richtek,rt4831-backlight > > + > > + default-brightness: > > +description: | > > + The default brightness that applied to the system on start-up. > > +$ref: /schemas/types.yaml#/definitions/uint32 > > Drop description and $ref. Covered in common.yaml. > OK > > +minimum: 0 > > +maximum: 2048 > > + > > + max-brightness: > > +description: | > > + The max brightness for the H/W limit > > +$ref: /schemas/types.yaml#/definitions/uint32 > > And here. > OK The above changes will be merged in next, thanks. After mfd code patch reviewed, I'll send v7 for all of review comments. > > +minimum: 0 > > +maximum: 2048 > > + > > + richtek,pwm-enable: > > +description: | > > + Specify the backlight dimming following by PWM duty or by SW control. > > +type: boolean > > + > > + richtek,bled-ovp-sel: > > +description: | > > + Backlight OVP level selection, currently support 17V/21V/25V/29V. > > +$ref: /schemas/types.yaml#/definitions/uint8 > > +default: 1 > > +minimum: 0 > > +maximum: 3 > > + > > + richtek,channel-use: > > +description: | > > + Backlight LED channel to be used. > > + BIT 0/1/2/3 is used to indicate led channel 1/2/3/4 enable or > > disable. > > +$ref: /schemas/types.yaml#/definitions/uint8 > > +minimum: 1 > > +maximum: 15 > > + > > +required: > > + - compatible > > + - richtek,channel-use > > + > > +additionalProperties: false > > diff --git a/include/dt-bindings/leds/rt4831-backlight.h > > b/include/dt-bindings/leds/rt4831-backlight.h > > new file mode 100644 > > index ..125c635 > > --- /dev/null > > +++ b/include/dt-bindings/leds/rt4831-backlight.h > > @@ -0,0 +1,23 @@ > > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ > > +/* > > + * This header provides constants for rt4831 backlight bindings. > > + * > > + * Copyright (C) 2020, Richtek Technology Corp. > > + * Author: ChiYuan Huang > > + */ > > + > > +#ifndef _DT_BINDINGS_RT4831_BACKLIGHT_H > > +#define _DT_BINDINGS_RT4831_BACKLIGHT_H > > + > > +#define RT4831_BLOVPLVL_17V 0 > > +#define RT4831_BLOVPLVL_21V 1 > > +#define RT4831_BLOVPLVL_25V 2 > > +#define RT4831_BLOVPLVL_29V 3 > > + > > +#define RT4831_BLED_CH1EN(1 << 0) > > +#define RT4831_BLED_CH2EN(1 << 1) > > +#define RT4831_BLED_CH3EN(1 << 2) > > +#define RT4831_BLED_CH4EN(1 << 3) > > +#define RT4831_BLED_ALLCHEN ((1 << 4) - 1) > > + > > +#endif /* _DT_BINDINGS_RT4831_BACKLIGHT_H */ > > -- > > 2.7.4 > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v6 3/4] mfd: rt4831: Adds DT binding document for Richtek RT4831
HI, Rob Herring 於 2021年5月1日 週六 上午4:10寫道： > > On Mon, 26 Apr 2021 15:18:10 +0800, cy_huang wrote: > > From: ChiYuan Huang > > > > Adds DT binding document for Richtek RT4831. > > > > Signed-off-by: ChiYuan Huang > > --- > > Resend this v6 patch series to loop devicetree reviewers. > > --- > > .../devicetree/bindings/mfd/richtek,rt4831.yaml| 90 > > ++ > > 1 file changed, 90 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml > > > > Reviewed-by: Rob Herring Will merge in next v7. Thanks. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v6 4/4] backlight: rt4831: Adds support for Richtek RT4831 backlight
Hi,
Daniel Thompson 於 2021年4月26日 週一 下午6:19寫道：
>
> On Mon, Apr 26, 2021 at 03:18:11PM +0800, cy_huang wrote:
> > From: ChiYuan Huang
> >
> > Adds support for Richtek RT4831 backlight.
> >
> > Signed-off-by: ChiYuan Huang
> > Reviewed-by: Daniel Thompson
> > ---
> > Resend this v6 patch series to loop devicetree reviewers.
> >
> > For next, if the typo in Kconfig 'common' to 'commonly' can be added the
> > below line
> > Reviewed-by: Daniel Thompson
> > ---
>
> This isn't the best way to handle feedback from multiple maintainers.
>
> It is great to see you are keeping track of feedback. However it doesn't
> make sense to RESEND an old patchset and acknowledge that you haven't
> fixed a typo yet.
>
> It would be better to fix the typo and to resend a v7.
>
You can refer to the below reply
https://lkml.org/lkml/2021/4/23/229
>
> Daniel.
>
>
> > drivers/video/backlight/Kconfig| 8 ++
> > drivers/video/backlight/Makefile | 1 +
> > drivers/video/backlight/rt4831-backlight.c | 203
> > +
> > 3 files changed, 212 insertions(+)
> > create mode 100644 drivers/video/backlight/rt4831-backlight.c
> >
> > diff --git a/drivers/video/backlight/Kconfig
> > b/drivers/video/backlight/Kconfig
> > index d83c87b..de96441 100644
> > --- a/drivers/video/backlight/Kconfig
> > +++ b/drivers/video/backlight/Kconfig
> > @@ -289,6 +289,14 @@ config BACKLIGHT_QCOM_WLED
> > If you have the Qualcomm PMIC, say Y to enable a driver for the
> > WLED block. Currently it supports PM8941 and PMI8998.
> >
> > +config BACKLIGHT_RT4831
> > + tristate "Richtek RT4831 Backlight Driver"
> > + depends on MFD_RT4831
> > + help
> > + This enables support for Richtek RT4831 Backlight driver.
> > + It's common used to drive the display WLED. There're four channels
> > + inisde, and each channel can provide up to 30mA current.
> > +
> > config BACKLIGHT_SAHARA
> > tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
> > depends on X86
> > diff --git a/drivers/video/backlight/Makefile
> > b/drivers/video/backlight/Makefile
> > index 685f3f1..cae2c83 100644
> > --- a/drivers/video/backlight/Makefile
> > +++ b/drivers/video/backlight/Makefile
> > @@ -49,6 +49,7 @@ obj-$(CONFIG_BACKLIGHT_PANDORA) +=
> > pandora_bl.o
> > obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
> > obj-$(CONFIG_BACKLIGHT_PWM) += pwm_bl.o
> > obj-$(CONFIG_BACKLIGHT_QCOM_WLED)+= qcom-wled.o
> > +obj-$(CONFIG_BACKLIGHT_RT4831) += rt4831-backlight.o
> > obj-$(CONFIG_BACKLIGHT_SAHARA) += kb3886_bl.o
> > obj-$(CONFIG_BACKLIGHT_SKY81452) += sky81452-backlight.o
> > obj-$(CONFIG_BACKLIGHT_TOSA) += tosa_bl.o
> > diff --git a/drivers/video/backlight/rt4831-backlight.c
> > b/drivers/video/backlight/rt4831-backlight.c
> > new file mode 100644
> > index ..42155c7
> > --- /dev/null
> > +++ b/drivers/video/backlight/rt4831-backlight.c
> > @@ -0,0 +1,203 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define RT4831_REG_BLCFG 0x02
> > +#define RT4831_REG_BLDIML0x04
> > +#define RT4831_REG_ENABLE0x08
> > +
> > +#define RT4831_BLMAX_BRIGHTNESS 2048
> > +
> > +#define RT4831_BLOVP_MASKGENMASK(7, 5)
> > +#define RT4831_BLOVP_SHIFT 5
> > +#define RT4831_BLPWMEN_MASK BIT(0)
> > +#define RT4831_BLEN_MASK BIT(4)
> > +#define RT4831_BLCH_MASK GENMASK(3, 0)
> > +#define RT4831_BLDIML_MASK GENMASK(2, 0)
> > +#define RT4831_BLDIMH_MASK GENMASK(10, 3)
> > +#define RT4831_BLDIMH_SHIFT 3
> > +
> > +struct rt4831_priv {
> > + struct device *dev;
> > + struct regmap *regmap;
> > + struct backlight_device *bl;
> > +};
> > +
> > +static int rt4831_bl_update_status(struct backlight_device *bl_dev)
> > +{
> > + struct rt4831_priv *priv = bl_get_data(bl_dev);
> > + int brightness = backlight_get_brightness(bl_dev);
> > + unsigned int enable = brightness ? RT4831_BLEN_MASK : 0;
> > + u8 v[2];
> > + int ret;
> > +
> > + if (brightness) {
> > + v[0] = (bri
Re: [PATCH v6 1/4] mfd: rt4831: Adds support for Richtek RT4831
Hi, Lee: ChiYuan Huang 於 2021年4月19日 週一 下午5:55寫道： > > Lee Jones 於 2021年4月19日 週一 下午3:24寫道： > > > > On Mon, 19 Apr 2021, Lee Jones wrote: > > > > > On Mon, 19 Apr 2021, Lee Jones wrote: > > > > > > > On Mon, 19 Apr 2021, ChiYuan Huang wrote: > > > > > > > > > Hi, Linux mfd reviewers: > > > > >It's been three weeks not to get any response from you. > > > > > Is there something wrong about this mfd patch? > > > > > If yes, please feel free to let me know. > > > > > > > > Couple of things: > > > > > > > > First, if you think a patch had fallen through the gaps, which does > > > > happen sometimes, it is generally considered acceptable to submit a > > > > [RESEND] ~2 weeks after the initial submission. FYI: This was such a > > > > patch. It was not on, or had fallen off of my radar for some reason. > > > > > > > > Secondly, we are really late in the release cycle. -rc8 has just been > > > > released. Quite a few maintainers slow down at ~-rc6. Particularly > > > > for new drivers. > > > > > > > > No need to resubmit this driver this time. It is now on my to-review > > > > list and I will tend to it shortly. > > > > > > > > Thanks for your patience. > > > > > > Also you are missing a DT review on patch 4. > > > > ... looks like you forgot to Cc them! > > > Yap, really. I''ll resend patch 4 and cc them. Thx. Should I resend the patch and loop DT reviewers? > > -- > > Lee Jones [李琼斯] > > Senior Technical Lead - Developer Services > > Linaro.org │ Open source software for Arm SoCs > > Follow Linaro: Facebook | Twitter | Blog ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 1/4] mfd: rt4831: Adds support for Richtek RT4831
Lee Jones 於 2021年4月19日 週一 下午3:24寫道： > > On Mon, 19 Apr 2021, Lee Jones wrote: > > > On Mon, 19 Apr 2021, Lee Jones wrote: > > > > > On Mon, 19 Apr 2021, ChiYuan Huang wrote: > > > > > > > Hi, Linux mfd reviewers: > > > >It's been three weeks not to get any response from you. > > > > Is there something wrong about this mfd patch? > > > > If yes, please feel free to let me know. > > > > > > Couple of things: > > > > > > First, if you think a patch had fallen through the gaps, which does > > > happen sometimes, it is generally considered acceptable to submit a > > > [RESEND] ~2 weeks after the initial submission. FYI: This was such a > > > patch. It was not on, or had fallen off of my radar for some reason. > > > > > > Secondly, we are really late in the release cycle. -rc8 has just been > > > released. Quite a few maintainers slow down at ~-rc6. Particularly > > > for new drivers. > > > > > > No need to resubmit this driver this time. It is now on my to-review > > > list and I will tend to it shortly. > > > > > > Thanks for your patience. > > > > Also you are missing a DT review on patch 4. > > ... looks like you forgot to Cc them! > Yap, really. I''ll resend patch 4 and cc them. Thx. > -- > Lee Jones [李琼斯] > Senior Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 1/4] mfd: rt4831: Adds support for Richtek RT4831
Hi, Linux mfd reviewers:
It's been three weeks not to get any response from you.
Is there something wrong about this mfd patch?
If yes, please feel free to let me know.
cy_huang 於 2021年3月28日 週日 下午11:24寫道：
>
> From: ChiYuan Huang
>
> This adds support Richtek RT4831 core. It includes four channel WLED driver
> and Display Bias Voltage outputs.
>
> Signed-off-by: ChiYuan Huang
> ---
> The RT4831 regulator patches are already on main stream, and can be referred
> to
> 9351ab8b0cb6 regulator: rt4831: Adds support for Richtek RT4831 DSV regulator
> 934b05e81862 regulator: rt4831: Adds DT binding document for Richtek RT4831
> DSV regulator
>
> since v6
> - Respin the date from 2020 to 2021.
> - Rmove the shutdown routine.
> - Change the macro OF_MFD_CELL to MFD_CELL_OF.
>
>
> since v5
> - Rename file name from rt4831-core.c to rt4831.c
> - Change RICHTEK_VID to RICHTEK_VENDOR_ID.
> - Change gpio_desc nameing from 'enable' to 'enable_gpio' in probe.
> - Change variable 'val' to the meaningful name 'chip_id'.
> - Refine the error log when vendor id is not matched.
> - Remove of_match_ptr.
>
> since v2
> - Refine Kconfig descriptions.
> - Add copyright.
> - Refine error logs in probe.
> - Refine comment lines in remove and shutdown.
> ---
> drivers/mfd/Kconfig | 10 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/rt4831.c | 115
> +++
> 3 files changed, 126 insertions(+)
> create mode 100644 drivers/mfd/rt4831.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b74efa4..3f43834 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1065,6 +1065,16 @@ config MFD_RDC321X
> southbridge which provides access to GPIOs and Watchdog using the
> southbridge PCI device configuration space.
>
> +config MFD_RT4831
> + tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> + depends on I2C
> + select MFD_CORE
> + select REGMAP_I2C
> + help
> + This enables support for the Richtek RT4831 that includes 4 channel
> + WLED driving and Display Bias Voltage. It's commonly used to provide
> + power to the LCD display and LCD backlight.
> +
> config MFD_RT5033
> tristate "Richtek RT5033 Power Management IC"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 834f546..5986914 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC) += menf21bmc.o
> obj-$(CONFIG_MFD_HI6421_PMIC) += hi6421-pmic-core.o
> obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
> obj-$(CONFIG_MFD_DLN2) += dln2.o
> +obj-$(CONFIG_MFD_RT4831) += rt4831.o
> obj-$(CONFIG_MFD_RT5033) += rt5033.o
> obj-$(CONFIG_MFD_SKY81452) += sky81452.o
>
> diff --git a/drivers/mfd/rt4831.c b/drivers/mfd/rt4831.c
> new file mode 100644
> index ..b169781
> --- /dev/null
> +++ b/drivers/mfd/rt4831.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021 Richtek Technology Corp.
> + *
> + * Author: ChiYuan Huang
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define RT4831_REG_REVISION0x01
> +#define RT4831_REG_ENABLE 0x08
> +#define RT4831_REG_I2CPROT 0x15
> +
> +#define RICHTEK_VENDOR_ID 0x03
> +#define RT4831_VID_MASKGENMASK(1, 0)
> +#define RT4831_RESET_MASK BIT(7)
> +#define RT4831_I2CSAFETMR_MASK BIT(0)
> +
> +static const struct mfd_cell rt4831_subdevs[] = {
> + MFD_CELL_OF("rt4831-backlight", NULL, NULL, 0, 0,
> "richtek,rt4831-backlight"),
> + MFD_CELL_NAME("rt4831-regulator")
> +};
> +
> +static bool rt4831_is_accessible_reg(struct device *dev, unsigned int reg)
> +{
> + if (reg >= RT4831_REG_REVISION && reg <= RT4831_REG_I2CPROT)
> + return true;
> + return false;
> +}
> +
> +static const struct regmap_config rt4831_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = RT4831_REG_I2CPROT,
> +
> + .readable_reg = rt4831_is_accessible_reg,
> + .writeable_reg = rt4831_is_accessible_reg,
> +};
> +
> +static int rt4831_probe(struct i2c_client *client)
> +{
> + struct gpio_desc *enable_gpio;
> + struct regmap *regmap;
> + unsigned int chip_id;
> + int ret;
> +
> + enable_gpio = devm_gpiod_get_optional(&c
Re: [PATCH v6 4/4] backlight: rt4831: Adds support for Richtek RT4831 backlight
Hi, Daniel: Daniel Thompson 於 2021年3月29日 週一 下午6:26寫道： > > On Sun, Mar 28, 2021 at 11:24:19PM +0800, cy_huang wrote: > > From: ChiYuan Huang > > > > Adds support for Richtek RT4831 backlight. > > > > Signed-off-by: ChiYuan Huang > > --- > > since v6 > > - Fix Kconfig typo. > > - Remove internal mutex lock. > > - Add the prefix for max brightness. > > - rename init_device_properties to parse_backlight_properties. > > - Remove some warning message if default value is adopted. > > - Add backlight property scale to LINEAR mapping. > > - Fix regmap get to check NULL not IS_ERR. > > --- > > drivers/video/backlight/Kconfig| 8 ++ > > drivers/video/backlight/Makefile | 1 + > > drivers/video/backlight/rt4831-backlight.c | 203 > > + > > 3 files changed, 212 insertions(+) > > create mode 100644 drivers/video/backlight/rt4831-backlight.c > > > > diff --git a/drivers/video/backlight/Kconfig > > b/drivers/video/backlight/Kconfig > > index d83c87b..de96441 100644 > > --- a/drivers/video/backlight/Kconfig > > +++ b/drivers/video/backlight/Kconfig > > @@ -289,6 +289,14 @@ config BACKLIGHT_QCOM_WLED > > If you have the Qualcomm PMIC, say Y to enable a driver for the > > WLED block. Currently it supports PM8941 and PMI8998. > > > > +config BACKLIGHT_RT4831 > > + tristate "Richtek RT4831 Backlight Driver" > > + depends on MFD_RT4831 > > + help > > + This enables support for Richtek RT4831 Backlight driver. > > + It's common used to drive the display WLED. There're four channels > > Nitpicking but I was expecting the original typo be converted to > "commonly". > OK, I'll correct this typo in v7 next. And will merge the reviewed-by line. Thx. > > With that addressed: > Reviewed-by: Daniel Thompson > > > Daniel. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight
Dear reviewers:
Didn't get any response about this backlight patch.
Is there any part need to be refined?
cy_huang 於 2020年12月17日 週四 下午11:01寫道：
>
> From: ChiYuan Huang
>
> Adds support for Richtek RT4831 backlight.
>
> Signed-off-by: ChiYuan Huang
> ---
> drivers/video/backlight/Kconfig| 8 ++
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/rt4831-backlight.c | 219
> +
> 3 files changed, 228 insertions(+)
> create mode 100644 drivers/video/backlight/rt4831-backlight.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index d83c87b..666bdb0 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -289,6 +289,14 @@ config BACKLIGHT_QCOM_WLED
> If you have the Qualcomm PMIC, say Y to enable a driver for the
> WLED block. Currently it supports PM8941 and PMI8998.
>
> +config BACKLIGHT_RT4831
> + tristate "Richtek RT4831 Backlight Driver"
> + depends on MFD_RT4831
> + help
> + This enables support for Richtek RT4831 Backlight driver.
> + It's commont used to drive the display WLED. There're four channels
> + inisde, and each channel can provide up to 30mA current.
> +
> config BACKLIGHT_SAHARA
> tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
> depends on X86
> diff --git a/drivers/video/backlight/Makefile
> b/drivers/video/backlight/Makefile
> index 685f3f1..cae2c83 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_BACKLIGHT_PANDORA) +=
> pandora_bl.o
> obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
> obj-$(CONFIG_BACKLIGHT_PWM)+= pwm_bl.o
> obj-$(CONFIG_BACKLIGHT_QCOM_WLED) += qcom-wled.o
> +obj-$(CONFIG_BACKLIGHT_RT4831) += rt4831-backlight.o
> obj-$(CONFIG_BACKLIGHT_SAHARA) += kb3886_bl.o
> obj-$(CONFIG_BACKLIGHT_SKY81452) += sky81452-backlight.o
> obj-$(CONFIG_BACKLIGHT_TOSA) += tosa_bl.o
> diff --git a/drivers/video/backlight/rt4831-backlight.c
> b/drivers/video/backlight/rt4831-backlight.c
> new file mode 100644
> index ..816c4d6
> --- /dev/null
> +++ b/drivers/video/backlight/rt4831-backlight.c
> @@ -0,0 +1,219 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define RT4831_REG_BLCFG 0x02
> +#define RT4831_REG_BLDIML 0x04
> +#define RT4831_REG_ENABLE 0x08
> +
> +#define BL_MAX_BRIGHTNESS 2048
> +
> +#define RT4831_BLOVP_MASK GENMASK(7, 5)
> +#define RT4831_BLOVP_SHIFT 5
> +#define RT4831_BLPWMEN_MASKBIT(0)
> +#define RT4831_BLEN_MASK BIT(4)
> +#define RT4831_BLCH_MASK GENMASK(3, 0)
> +#define RT4831_BLDIML_MASK GENMASK(2, 0)
> +#define RT4831_BLDIMH_MASK GENMASK(10, 3)
> +#define RT4831_BLDIMH_SHIFT3
> +
> +struct rt4831_priv {
> + struct regmap *regmap;
> + struct mutex lock;
> + struct backlight_device *bl;
> +};
> +
> +static int rt4831_bl_update_status(struct backlight_device *bl_dev)
> +{
> + struct rt4831_priv *priv = bl_get_data(bl_dev);
> + int brightness = backlight_get_brightness(bl_dev);
> + unsigned int enable = brightness ? RT4831_BLEN_MASK : 0;
> + u8 v[2];
> + int ret;
> +
> + mutex_lock(&priv->lock);
> +
> + if (brightness) {
> + v[0] = (brightness - 1) & RT4831_BLDIML_MASK;
> + v[1] = ((brightness - 1) & RT4831_BLDIMH_MASK) >>
> RT4831_BLDIMH_SHIFT;
> +
> + ret = regmap_raw_write(priv->regmap, RT4831_REG_BLDIML, v,
> sizeof(v));
> + if (ret)
> + goto unlock;
> + }
> +
> + ret = regmap_update_bits(priv->regmap, RT4831_REG_ENABLE,
> RT4831_BLEN_MASK, enable);
> +
> +unlock:
> + mutex_unlock(&priv->lock);
> + return ret;
> +}
> +
> +static int rt4831_bl_get_brightness(struct backlight_device *bl_dev)
> +{
> + struct rt4831_priv *priv = bl_get_data(bl_dev);
> + unsigned int val;
> + u8 v[2];
> + int ret;
> +
> + mutex_lock(&priv->lock);
> +
> + ret = regmap_read(priv->regmap, RT4831_REG_ENABLE, &val);
> + if (ret)
> + return ret;
> +
> + if (!(val & RT4831_BLEN_MASK)) {
> +
Re: [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core
HI, Lee:
ChiYuan Huang 於 2021年1月13日 週三 下午10:09寫道：
>
> Lee Jones 於 2021年1月13日 週三 下午8:21寫道：
> >
> > On Thu, 17 Dec 2020, cy_huang wrote:
> >
> > > From: ChiYuan Huang
> > >
> > > This adds support Richtek RT4831 core. It includes four channel WLED
> > > driver
> > > and Display Bias Voltage outputs.
> > >
> > > Signed-off-by: ChiYuan Huang
> > > ---
> > > since v5
> > > - Rename file name from rt4831-core.c to rt4831.c
> > > - Change RICHTEK_VID to RICHTEK_VENDOR_ID.
> > > - Change gpio_desc nameing from 'enable' to 'enable_gpio' in probe.
> > > - Change variable 'val' to the meaningful name 'chip_id'.
> > > - Refine the error log when vendor id is not matched.
> > > - Remove of_match_ptr.
> > >
> > > since v2
> > > - Refine Kconfig descriptions.
> > > - Add copyright.
> > > - Refine error logs in probe.
> > > - Refine comment lines in remove and shutdown.
> > > ---
> > > drivers/mfd/Kconfig | 10 +
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/rt4831.c | 124
> > > +++
> > > 3 files changed, 135 insertions(+)
> > > create mode 100644 drivers/mfd/rt4831.c
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 8b99a13..dfb2640 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1088,6 +1088,16 @@ config MFD_RDC321X
> > > southbridge which provides access to GPIOs and Watchdog using the
> > > southbridge PCI device configuration space.
> > >
> > > +config MFD_RT4831
> > > + tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> > > + depends on I2C
> > > + select MFD_CORE
> > > + select REGMAP_I2C
> > > + help
> > > + This enables support for the Richtek RT4831 that includes 4
> > > channel
> > > + WLED driving and Display Bias Voltage. It's commonly used to
> > > provide
> > > + power to the LCD display and LCD backlight.
> > > +
> > > config MFD_RT5033
> > > tristate "Richtek RT5033 Power Management IC"
> > > depends on I2C
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > index 1780019..28d247b 100644
> > > --- a/drivers/mfd/Makefile
> > > +++ b/drivers/mfd/Makefile
> > > @@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC) += menf21bmc.o
> > > obj-$(CONFIG_MFD_HI6421_PMIC)+= hi6421-pmic-core.o
> > > obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
> > > obj-$(CONFIG_MFD_DLN2) += dln2.o
> > > +obj-$(CONFIG_MFD_RT4831) += rt4831.o
> > > obj-$(CONFIG_MFD_RT5033) += rt5033.o
> > > obj-$(CONFIG_MFD_SKY81452) += sky81452.o
> > >
> > > diff --git a/drivers/mfd/rt4831.c b/drivers/mfd/rt4831.c
> > > new file mode 100644
> > > index ..2bf8364
> > > --- /dev/null
> > > +++ b/drivers/mfd/rt4831.c
> > > @@ -0,0 +1,124 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (c) 2020 Richtek Technology Corp.
> >
> > Nit: If you respin this, please bump the date.
> >
> Okay.
> > > + * Author: ChiYuan Huang
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#define RT4831_REG_REVISION 0x01
> > > +#define RT4831_REG_ENABLE0x08
> > > +#define RT4831_REG_I2CPROT 0x15
> > > +
> > > +#define RICHTEK_VENDOR_ID0x03
> > > +#define RT4831_VID_MASK GENMASK(1, 0)
> > > +#define RT4831_RESET_MASKBIT(7)
> > > +#define RT4831_I2CSAFETMR_MASK BIT(0)
> > > +
> > > +static const struct mfd_cell rt4831_subdevs[] = {
> > > + OF_MFD_CELL("rt4831-backlight", NULL, NULL, 0, 0,
> > > "richtek,rt4831-backlight"),
> > > + MFD_CELL_NAME("rt4831-regulator")
> > > +};
> > > +
> > > +static bool rt4831_is_accessible_reg(struct device *dev, unsigned int
> > > reg)
> > > +{
> > > + if (reg >= RT4831_REG_REVISION && reg <= RT4831_REG_I2CPROT)
> > > +
Re: [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core
Lee Jones 於 2021年1月13日 週三 下午8:21寫道：
>
> On Thu, 17 Dec 2020, cy_huang wrote:
>
> > From: ChiYuan Huang
> >
> > This adds support Richtek RT4831 core. It includes four channel WLED driver
> > and Display Bias Voltage outputs.
> >
> > Signed-off-by: ChiYuan Huang
> > ---
> > since v5
> > - Rename file name from rt4831-core.c to rt4831.c
> > - Change RICHTEK_VID to RICHTEK_VENDOR_ID.
> > - Change gpio_desc nameing from 'enable' to 'enable_gpio' in probe.
> > - Change variable 'val' to the meaningful name 'chip_id'.
> > - Refine the error log when vendor id is not matched.
> > - Remove of_match_ptr.
> >
> > since v2
> > - Refine Kconfig descriptions.
> > - Add copyright.
> > - Refine error logs in probe.
> > - Refine comment lines in remove and shutdown.
> > ---
> > drivers/mfd/Kconfig | 10 +
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/rt4831.c | 124
> > +++
> > 3 files changed, 135 insertions(+)
> > create mode 100644 drivers/mfd/rt4831.c
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 8b99a13..dfb2640 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1088,6 +1088,16 @@ config MFD_RDC321X
> > southbridge which provides access to GPIOs and Watchdog using the
> > southbridge PCI device configuration space.
> >
> > +config MFD_RT4831
> > + tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> > + depends on I2C
> > + select MFD_CORE
> > + select REGMAP_I2C
> > + help
> > + This enables support for the Richtek RT4831 that includes 4 channel
> > + WLED driving and Display Bias Voltage. It's commonly used to provide
> > + power to the LCD display and LCD backlight.
> > +
> > config MFD_RT5033
> > tristate "Richtek RT5033 Power Management IC"
> > depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 1780019..28d247b 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC) += menf21bmc.o
> > obj-$(CONFIG_MFD_HI6421_PMIC)+= hi6421-pmic-core.o
> > obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
> > obj-$(CONFIG_MFD_DLN2) += dln2.o
> > +obj-$(CONFIG_MFD_RT4831) += rt4831.o
> > obj-$(CONFIG_MFD_RT5033) += rt5033.o
> > obj-$(CONFIG_MFD_SKY81452) += sky81452.o
> >
> > diff --git a/drivers/mfd/rt4831.c b/drivers/mfd/rt4831.c
> > new file mode 100644
> > index ..2bf8364
> > --- /dev/null
> > +++ b/drivers/mfd/rt4831.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2020 Richtek Technology Corp.
>
> Nit: If you respin this, please bump the date.
>
Okay.
> > + * Author: ChiYuan Huang
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define RT4831_REG_REVISION 0x01
> > +#define RT4831_REG_ENABLE0x08
> > +#define RT4831_REG_I2CPROT 0x15
> > +
> > +#define RICHTEK_VENDOR_ID0x03
> > +#define RT4831_VID_MASK GENMASK(1, 0)
> > +#define RT4831_RESET_MASKBIT(7)
> > +#define RT4831_I2CSAFETMR_MASK BIT(0)
> > +
> > +static const struct mfd_cell rt4831_subdevs[] = {
> > + OF_MFD_CELL("rt4831-backlight", NULL, NULL, 0, 0,
> > "richtek,rt4831-backlight"),
> > + MFD_CELL_NAME("rt4831-regulator")
> > +};
> > +
> > +static bool rt4831_is_accessible_reg(struct device *dev, unsigned int reg)
> > +{
> > + if (reg >= RT4831_REG_REVISION && reg <= RT4831_REG_I2CPROT)
> > + return true;
> > + return false;
> > +}
> > +
> > +static const struct regmap_config rt4831_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = RT4831_REG_I2CPROT,
> > +
> > + .readable_reg = rt4831_is_accessible_reg,
> > + .writeable_reg = rt4831_is_accessible_reg,
> > +};
> > +
> > +static int rt4831_probe(struct i2c_client *client)
> > +{
> > + struct gpio_desc *enable_gpio;
> > + struct regmap *regmap;
> > + unsigned int chip_id;
> > + int ret;
> > +
> > + enable_
