* [PATCH 0/2] Add the property to make ocp level selectable
@ 2022-05-26 3:16 cy_huang
2022-05-26 3:16 ` [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection cy_huang
2022-05-26 3:16 ` [PATCH 2/2] backlight: rt4831: Add the property parsing " cy_huang
0 siblings, 2 replies; 15+ messages in thread
From: cy_huang @ 2022-05-26 3:16 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, lee.jones, daniel.thompson, jingoohan1
Cc: pavel, deller, cy_huang, lucas_tsai, devicetree, dri-devel,
linux-fbdev, linux-leds, linux-kernel
From: ChiYuan Huang <cy_huang@richtek.com>
This patch series is to make ocp level selectable.
Some application design use small inductor. And it's easy to trigger the
over current protection. Due to the OCP limit, It make the brightness current
not match the configured value.
To meet the HW design, the ocp level have to be selectable.
ChiYuan Huang (2):
dt-bindings: backlight: rt4831: Add the new property for ocp level
selection
backlight: rt4831: Add the property parsing for ocp level selection
.../bindings/leds/backlight/richtek,rt4831-backlight.yaml | 8 ++++++++
drivers/video/backlight/rt4831-backlight.c | 13 +++++++++++++
include/dt-bindings/leds/rt4831-backlight.h | 5 +++++
3 files changed, 26 insertions(+)
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection 2022-05-26 3:16 [PATCH 0/2] Add the property to make ocp level selectable cy_huang @ 2022-05-26 3:16 ` cy_huang 2022-05-26 8:06 ` Krzysztof Kozlowski 2022-05-26 3:16 ` [PATCH 2/2] backlight: rt4831: Add the property parsing " cy_huang 1 sibling, 1 reply; 15+ messages in thread From: cy_huang @ 2022-05-26 3:16 UTC (permalink / raw) To: robh+dt, krzysztof.kozlowski+dt, lee.jones, daniel.thompson, jingoohan1 Cc: pavel, deller, cy_huang, lucas_tsai, devicetree, dri-devel, linux-fbdev, linux-leds, linux-kernel From: ChiYuan Huang <cy_huang@richtek.com> Add the new property for ocp level selection. Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> --- .../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: + description: | + Backlight OCP level selection, currently support 0.9A/1.2A/1.5A/1.8A + $ref: /schemas/types.yaml#/definitions/uint8 + default: 1 + minimum: 0 + maximum: 3 + richtek,channel-use: description: | Backlight LED channel to be used. diff --git a/include/dt-bindings/leds/rt4831-backlight.h b/include/dt-bindings/leds/rt4831-backlight.h index 125c635..e8b8609 100644 --- a/include/dt-bindings/leds/rt4831-backlight.h +++ b/include/dt-bindings/leds/rt4831-backlight.h @@ -14,6 +14,11 @@ #define RT4831_BLOVPLVL_25V 2 #define RT4831_BLOVPLVL_29V 3 +#define RT4831_BLOCPLVL_0P9A 0 +#define RT4831_BLOCPLVL_1P2A 1 +#define RT4831_BLOCPLVL_1P5A 2 +#define RT4831_BLOCPLVL_1P8A 3 + #define RT4831_BLED_CH1EN (1 << 0) #define RT4831_BLED_CH2EN (1 << 1) #define RT4831_BLED_CH3EN (1 << 2) -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection 2022-05-26 3:16 ` [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection cy_huang @ 2022-05-26 8:06 ` Krzysztof Kozlowski 2022-05-26 8:13 ` ChiYuan Huang 0 siblings, 1 reply; 15+ messages in thread From: Krzysztof Kozlowski @ 2022-05-26 8:06 UTC (permalink / raw) To: cy_huang, robh+dt, krzysztof.kozlowski+dt, lee.jones, daniel.thompson, jingoohan1 Cc: pavel, deller, cy_huang, lucas_tsai, devicetree, dri-devel, linux-fbdev, linux-leds, linux-kernel On 26/05/2022 05:16, cy_huang wrote: > From: ChiYuan Huang <cy_huang@richtek.com> > > Add the new property for ocp level selection. > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > --- > .../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" > + 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)? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection 2022-05-26 8:06 ` Krzysztof Kozlowski @ 2022-05-26 8:13 ` ChiYuan Huang 2022-05-26 10:32 ` Krzysztof Kozlowski 0 siblings, 1 reply; 15+ messages in thread From: ChiYuan Huang @ 2022-05-26 8:13 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Daniel Thompson, jingoohan1, Pavel Machek, deller, cy_huang, lucas_tsai, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, dri-devel, linux-fbdev, Linux LED Subsystem, lkml Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年5月26日 週四 下午4:06寫道： > > On 26/05/2022 05:16, cy_huang wrote: > > From: ChiYuan Huang <cy_huang@richtek.com> > > > > Add the new property for ocp level selection. > > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > > --- > > .../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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection 2022-05-26 8:13 ` ChiYuan Huang @ 2022-05-26 10:32 ` Krzysztof Kozlowski 2022-06-02 13:56 ` Rob Herring 0 siblings, 1 reply; 15+ messages in thread From: Krzysztof Kozlowski @ 2022-05-26 10:32 UTC (permalink / raw) To: ChiYuan Huang Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Daniel Thompson, jingoohan1, Pavel Machek, deller, cy_huang, lucas_tsai, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, dri-devel, linux-fbdev, Linux LED Subsystem, lkml On 26/05/2022 10:13, ChiYuan Huang wrote: > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年5月26日 週四 下午4:06寫道： >> >> On 26/05/2022 05:16, cy_huang wrote: >>> From: ChiYuan Huang <cy_huang@richtek.com> >>> >>> Add the new property for ocp level selection. >>> >>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> >>> --- >>> .../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". I see you already did some register-style for voltage. It's wrong but it was done, so let it be. But let's don't make that a pattern... >> >> >> Best regards, >> Krzysztof Best regards, Krzysztof ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection 2022-05-26 10:32 ` Krzysztof Kozlowski @ 2022-06-02 13:56 ` Rob Herring 2022-06-02 13:58 ` Krzysztof Kozlowski 0 siblings, 1 reply; 15+ messages in thread From: Rob Herring @ 2022-06-02 13:56 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: ChiYuan Huang, Krzysztof Kozlowski, Lee Jones, Daniel Thompson, jingoohan1, Pavel Machek, deller, cy_huang, lucas_tsai, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, dri-devel, linux-fbdev, Linux LED Subsystem, lkml On Thu, May 26, 2022 at 12:32:12PM +0200, Krzysztof Kozlowski wrote: > On 26/05/2022 10:13, ChiYuan Huang wrote: > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年5月26日 週四 下午4:06寫道： > >> > >> On 26/05/2022 05:16, cy_huang wrote: > >>> From: ChiYuan Huang <cy_huang@richtek.com> > >>> > >>> Add the new property for ocp level selection. > >>> > >>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > >>> --- > >>> .../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. Rob ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection 2022-06-02 13:56 ` Rob Herring @ 2022-06-02 13:58 ` Krzysztof Kozlowski 2022-06-02 15:31 ` ChiYuan Huang 0 siblings, 1 reply; 15+ messages in thread From: Krzysztof Kozlowski @ 2022-06-02 13:58 UTC (permalink / raw) To: Rob Herring Cc: ChiYuan Huang, Krzysztof Kozlowski, Lee Jones, Daniel Thompson, jingoohan1, Pavel Machek, deller, cy_huang, lucas_tsai, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, dri-devel, linux-fbdev, Linux LED Subsystem, lkml 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 <krzysztof.kozlowski@linaro.org> 於 2022年5月26日 週四 下午4:06寫道： >>>> >>>> On 26/05/2022 05:16, cy_huang wrote: >>>>> From: ChiYuan Huang <cy_huang@richtek.com> >>>>> >>>>> Add the new property for ocp level selection. >>>>> >>>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> >>>>> --- >>>>> .../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. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection 2022-06-02 13:58 ` Krzysztof Kozlowski @ 2022-06-02 15:31 ` ChiYuan Huang 2022-06-05 16:11 ` Krzysztof Kozlowski 0 siblings, 1 reply; 15+ messages in thread From: ChiYuan Huang @ 2022-06-02 15:31 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Helge Deller, cy_huang, lucas_tsai, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, dri-devel, open list:FRAMEBUFFER LAYER, Linux LED Subsystem, lkml Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 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 <krzysztof.kozlowski@linaro.org> 於 2022年5月26日 週四 下午4:06寫道： > >>>> > >>>> On 26/05/2022 05:16, cy_huang wrote: > >>>>> From: ChiYuan Huang <cy_huang@richtek.com> > >>>>> > >>>>> Add the new property for ocp level selection. > >>>>> > >>>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > >>>>> --- > >>>>> .../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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection 2022-06-02 15:31 ` ChiYuan Huang @ 2022-06-05 16:11 ` Krzysztof Kozlowski 2022-06-06 1:39 ` ChiYuan Huang 0 siblings, 1 reply; 15+ messages in thread From: Krzysztof Kozlowski @ 2022-06-05 16:11 UTC (permalink / raw) To: ChiYuan Huang Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Helge Deller, cy_huang, lucas_tsai, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, dri-devel, open list:FRAMEBUFFER LAYER, Linux LED Subsystem, lkml On 02/06/2022 17:31, ChiYuan Huang wrote: > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 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 <krzysztof.kozlowski@linaro.org> 於 2022年5月26日 週四 下午4:06寫道： >>>>>> >>>>>> On 26/05/2022 05:16, cy_huang wrote: >>>>>>> From: ChiYuan Huang <cy_huang@richtek.com> >>>>>>> >>>>>>> Add the new property for ocp level selection. >>>>>>> >>>>>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> >>>>>>> --- >>>>>>> .../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? > 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? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection 2022-06-05 16:11 ` Krzysztof Kozlowski @ 2022-06-06 1:39 ` ChiYuan Huang 2022-06-06 6:20 ` Krzysztof Kozlowski 0 siblings, 1 reply; 15+ messages in thread From: ChiYuan Huang @ 2022-06-06 1:39 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Helge Deller, cy_huang, lucas_tsai, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, dri-devel, open list:FRAMEBUFFER LAYER, Linux LED Subsystem, lkml Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月6日 週一 上午12:11寫道： > > On 02/06/2022 17:31, ChiYuan Huang wrote: > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 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 <krzysztof.kozlowski@linaro.org> 於 2022年5月26日 週四 下午4:06寫道： > >>>>>> > >>>>>> On 26/05/2022 05:16, cy_huang wrote: > >>>>>>> From: ChiYuan Huang <cy_huang@richtek.com> > >>>>>>> > >>>>>>> Add the new property for ocp level selection. > >>>>>>> > >>>>>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > >>>>>>> --- > >>>>>>> .../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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection 2022-06-06 1:39 ` ChiYuan Huang @ 2022-06-06 6:20 ` Krzysztof Kozlowski 0 siblings, 0 replies; 15+ messages in thread From: Krzysztof Kozlowski @ 2022-06-06 6:20 UTC (permalink / raw) To: ChiYuan Huang Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Helge Deller, cy_huang, lucas_tsai, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, dri-devel, open list:FRAMEBUFFER LAYER, Linux LED Subsystem, lkml On 06/06/2022 03:39, ChiYuan Huang wrote: > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月6日 週一 上午12:11寫道： >> >> On 02/06/2022 17:31, ChiYuan Huang wrote: >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 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 <krzysztof.kozlowski@linaro.org> 於 2022年5月26日 週四 下午4:06寫道： >>>>>>>> >>>>>>>> On 26/05/2022 05:16, cy_huang wrote: >>>>>>>>> From: ChiYuan Huang <cy_huang@richtek.com> >>>>>>>>> >>>>>>>>> Add the new property for ocp level selection. >>>>>>>>> >>>>>>>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> >>>>>>>>> --- >>>>>>>>> .../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? This sounds differently so I propose to use this dedicated property with the changes I asked for. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] backlight: rt4831: Add the property parsing for ocp level selection 2022-05-26 3:16 [PATCH 0/2] Add the property to make ocp level selectable cy_huang 2022-05-26 3:16 ` [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection cy_huang @ 2022-05-26 3:16 ` cy_huang 2022-05-26 10:05 ` Daniel Thompson 1 sibling, 1 reply; 15+ messages in thread From: cy_huang @ 2022-05-26 3:16 UTC (permalink / raw) To: robh+dt, krzysztof.kozlowski+dt, lee.jones, daniel.thompson, jingoohan1 Cc: pavel, deller, cy_huang, lucas_tsai, devicetree, dri-devel, linux-fbdev, linux-leds, linux-kernel From: ChiYuan Huang <cy_huang@richtek.com> Add the property parsing for ocp level selection. Reported-by: Lucas Tsai <lucas_tsai@richtek.com> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> --- 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_BLDIML 0x04 #define RT4831_REG_ENABLE 0x08 +#define RT4831_REG_BLOPT2 0x11 #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_MASK GENMASK(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; + + 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] backlight: rt4831: Add the property parsing for ocp level selection 2022-05-26 3:16 ` [PATCH 2/2] backlight: rt4831: Add the property parsing " cy_huang @ 2022-05-26 10:05 ` Daniel Thompson 2022-05-27 2:24 ` ChiYuan Huang 0 siblings, 1 reply; 15+ messages in thread From: Daniel Thompson @ 2022-05-26 10:05 UTC (permalink / raw) To: cy_huang Cc: robh+dt, krzysztof.kozlowski+dt, lee.jones, jingoohan1, pavel, deller, cy_huang, lucas_tsai, devicetree, dri-devel, linux-fbdev, linux-leds, linux-kernel On Thu, May 26, 2022 at 11:16:35AM +0800, cy_huang wrote: > From: ChiYuan Huang <cy_huang@richtek.com> > > Add the property parsing for ocp level selection. Isn't this just restating the Subject: line? 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, <whatever>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... > > Reported-by: Lucas Tsai <lucas_tsai@richtek.com> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > --- > 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_BLDIML 0x04 > #define RT4831_REG_ENABLE 0x08 > +#define RT4831_REG_BLOPT2 0x11 > > #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_MASK GENMASK(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? 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? 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] backlight: rt4831: Add the property parsing for ocp level selection 2022-05-26 10:05 ` Daniel Thompson @ 2022-05-27 2:24 ` ChiYuan Huang 2022-05-27 10:56 ` Daniel Thompson 0 siblings, 1 reply; 15+ messages in thread From: ChiYuan Huang @ 2022-05-27 2:24 UTC (permalink / raw) To: Daniel Thompson Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, jingoohan1, Pavel Machek, deller, cy_huang, lucas_tsai, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, dri-devel, linux-fbdev, Linux LED Subsystem, lkml Daniel Thompson <daniel.thompson@linaro.org> 於 2022年5月26日 週四 下午6:05寫道： > > On Thu, May 26, 2022 at 11:16:35AM +0800, cy_huang wrote: > > From: ChiYuan Huang <cy_huang@richtek.com> > > > > 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, <whatever>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 <lucas_tsai@richtek.com> > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > > --- > > 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_BLDIML 0x04 > > #define RT4831_REG_ENABLE 0x08 > > +#define RT4831_REG_BLOPT2 0x11 > > > > #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_MASK GENMASK(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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] backlight: rt4831: Add the property parsing for ocp level selection 2022-05-27 2:24 ` ChiYuan Huang @ 2022-05-27 10:56 ` Daniel Thompson 0 siblings, 0 replies; 15+ messages in thread From: Daniel Thompson @ 2022-05-27 10:56 UTC (permalink / raw) To: ChiYuan Huang Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, jingoohan1, Pavel Machek, deller, cy_huang, lucas_tsai, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, dri-devel, linux-fbdev, Linux LED Subsystem, lkml On Fri, May 27, 2022 at 10:24:42AM +0800, ChiYuan Huang wrote: > Daniel Thompson <daniel.thompson@linaro.org> 於 2022年5月26日 週四 下午6:05寫道： > > > > On Thu, May 26, 2022 at 11:16:35AM +0800, cy_huang wrote: > > > From: ChiYuan Huang <cy_huang@richtek.com> > > > > > > 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, <whatever>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 <lucas_tsai@richtek.com> > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > > > --- > > > 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_BLDIML 0x04 > > > #define RT4831_REG_ENABLE 0x08 > > > +#define RT4831_REG_BLOPT2 0x11 > > > > > > #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_MASK GENMASK(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. Ok, great. When you make this change can you make sure there is a comment in the source code explaining that concerns about older firmware is *why* we treat bled-ocp-sel differently to bled-ovp-sel! Thanks Daniel. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-06-06 6:21 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-26 3:16 [PATCH 0/2] Add the property to make ocp level selectable cy_huang 2022-05-26 3:16 ` [PATCH 1/2] dt-bindings: backlight: rt4831: Add the new property for ocp level selection cy_huang 2022-05-26 8:06 ` Krzysztof Kozlowski 2022-05-26 8:13 ` ChiYuan Huang 2022-05-26 10:32 ` Krzysztof Kozlowski 2022-06-02 13:56 ` Rob Herring 2022-06-02 13:58 ` Krzysztof Kozlowski 2022-06-02 15:31 ` ChiYuan Huang 2022-06-05 16:11 ` Krzysztof Kozlowski 2022-06-06 1:39 ` ChiYuan Huang 2022-06-06 6:20 ` Krzysztof Kozlowski 2022-05-26 3:16 ` [PATCH 2/2] backlight: rt4831: Add the property parsing " cy_huang 2022-05-26 10:05 ` Daniel Thompson 2022-05-27 2:24 ` ChiYuan Huang 2022-05-27 10:56 ` Daniel Thompson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).