| Message ID | 20231020135434.2598578-2-f.suligoi@asem.it |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] dt-bindings: backlight: mp3309c: remove two required properties | expand |
On Mon, Oct 23, 2023 at 09:28:03AM +0000, Flavio Suligoi wrote: > > On Fri, Oct 20, 2023 at 03:54:33PM +0200, Flavio Suligoi wrote: > > > The two properties: > > > > > > - max-brightness > > > - default brightness > > > > > > are not really required, so they can be removed from the "required" > > > section. > > > > Why are they not required? You need to provide an explanation. > > The "max-brightness" is not more used now in the driver (I used it in the first version > of the driver). If it is not used any more, what happens when someone passes an old devicetree to the kernel, that contains max-brightness, but not any of your new properties? > The "default-brightness", if omitted in the DT, is managed by the device driver, > using a default value. This depends on the dimming mode used: For default-brightness, has here always been support in the driver for the property being omitted, or is this newly added? > - for the "analog mode", via I2C commands, this value is fixed by hardware (=31) > - while in case of pwm mode the default used is the last value of the > brightness-levels array. > > Also the brightness-levels array is not required; if it is omitted, the driver uses > a default array of 0..255 and the "default-brightness" is the last one, which is "255". Firstly, this is the sort of rationale that needs to be put into your commit messages, rather than bullet pointed lists of what you have done. Secondly, what about other operating systems etc, do any of those support this platform and depend on presence of these properties? > > > > Other changes: > > > > > > - improve the backlight working mode description, in the "description" > > > section > > > > > - update the example, removing the "max-brightness" and introducing the > > > "brightess-levels" property > > > > Why is this more useful? > > I introduced the "brightness-levels" instead of "max-brightness" for homogeneity, > since the "analog mode" dimming has a brightness-levels array fixed by hardware (0..31). > In this way also the "pwm" mode can use the same concepts of array of levels. What I would like is an explanation in the commit message as to why the revised example is more helpful than the existing (and must-remain-valid) one. Cheers, Conor. > > > > > > > > Signed-off-by: Flavio Suligoi <f.suligoi@asem.it> > > > --- > > > .../bindings/leds/backlight/mps,mp3309c.yaml | 10 ++++------ > > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > > > diff --git > > a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > > index 4191e33626f5..527a37368ed7 100644 > > > --- > > a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > > +++ > > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > > @@ -14,8 +14,8 @@ description: | > > > programmable switching frequency to optimize efficiency. > > > It supports two different dimming modes: > > > > > > - - analog mode, via I2C commands (default) > > > - - PWM controlled mode. > > > + - analog mode, via I2C commands, as default mode (32 dimming levels) > > > + - PWM controlled mode (optional) > > > > > > The datasheet is available at: > > > https://www.monolithicpower.com/en/mp3309c.html > > > @@ -50,8 +50,6 @@ properties: > > > required: > > > - compatible > > > - reg > > > - - max-brightness > > > - - default-brightness > > > > > > unevaluatedProperties: false > > > > > > @@ -66,8 +64,8 @@ examples: > > > compatible = "mps,mp3309c"; > > > reg = <0x17>; > > > pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */ > > > - max-brightness = <100>; > > > - default-brightness = <80>; > > > + brightness-levels = <0 4 8 16 32 64 128 255>; > > > + default-brightness = <6>; > > > mps,overvoltage-protection-microvolt = <24000000>; > > > }; > > > }; > > > -- > > > 2.34.1 > > >
Hi Conor, ... > On Mon, Oct 23, 2023 at 09:28:03AM +0000, Flavio Suligoi wrote: > > > On Fri, Oct 20, 2023 at 03:54:33PM +0200, Flavio Suligoi wrote: > > > > The two properties: > > > > > > > > - max-brightness > > > > - default brightness > > > > > > > > are not really required, so they can be removed from the "required" > > > > section. > > > > > > Why are they not required? You need to provide an explanation. > > > > The "max-brightness" is not more used now in the driver (I used it in > > the first version of the driver). > > If it is not used any more, what happens when someone passes an old > devicetree to the kernel, that contains max-brightness, but not any of your > new properties? This is not a problem, because the device driver has not yet been included in any kernel. My patch for the device driver is still being analyzed by the maintainers. Only this dt-binding yaml file is already included in the "for-backlight-next" branch of the "backlight" kernel repository. At the moment, this driver is used only in a i.MX8MM board produced in my company, under my full control. No other developer is using it now. > > The "default-brightness", if omitted in the DT, is managed by the > > device driver, using a default value. This depends on the dimming mode > used: > > For default-brightness, has here always been support in the driver for the > property being omitted, or is this newly added? In the first version of the driver this property was a "required property", but nobody has used this driver before, so this should be not a problem. > > > - for the "analog mode", via I2C commands, this value is fixed by > > hardware (=31) > > - while in case of pwm mode the default used is the last value of the > > brightness-levels array. > > > > Also the brightness-levels array is not required; if it is omitted, > > the driver uses a default array of 0..255 and the "default-brightness" is the > last one, which is "255". > > Firstly, this is the sort of rationale that needs to be put into your commit > messages, rather than bullet pointed lists of what you have done. You are absolutely right, I'll include these details in the next commit message. > > Secondly, what about other operating systems etc, do any of those support > this platform and depend on presence of these properties? I used this backlight driver in our i.MX8MM board only, with Linux only. > > > > > > > Other changes: > > > > > > > > - improve the backlight working mode description, in the "description" > > > > section > > > > > > > - update the example, removing the "max-brightness" and introducing > the > > > > "brightess-levels" property > > > > > > Why is this more useful? > > > > I introduced the "brightness-levels" instead of "max-brightness" for > > homogeneity, since the "analog mode" dimming has a brightness-levels array > fixed by hardware (0..31). > > In this way also the "pwm" mode can use the same concepts of array of > levels. > > What I would like is an explanation in the commit message as to why the > revised example is more helpful than the existing (and > must-remain-valid) one. As said before, no one may have ever used this device driver, so I would leave only this new version of the example. > > Cheers, > Conor. Thanks for your help and best regards, Flavio. > > > > > > > > > > > > Signed-off-by: Flavio Suligoi <f.suligoi@asem.it> > > > > --- > > > > .../bindings/leds/backlight/mps,mp3309c.yaml | 10 ++++------ > > > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > > > > > diff --git > > > a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > > > index 4191e33626f5..527a37368ed7 100644 > > > > --- > > > a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > > > +++ > > > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > > > @@ -14,8 +14,8 @@ description: | > > > > programmable switching frequency to optimize efficiency. > > > > It supports two different dimming modes: > > > > > > > > - - analog mode, via I2C commands (default) > > > > - - PWM controlled mode. > > > > + - analog mode, via I2C commands, as default mode (32 dimming > > > > + levels) > > > > + - PWM controlled mode (optional) > > > > > > > > The datasheet is available at: > > > > https://www.monolithicpower.com/en/mp3309c.html > > > > @@ -50,8 +50,6 @@ properties: > > > > required: > > > > - compatible > > > > - reg > > > > - - max-brightness > > > > - - default-brightness > > > > > > > > unevaluatedProperties: false > > > > > > > > @@ -66,8 +64,8 @@ examples: > > > > compatible = "mps,mp3309c"; > > > > reg = <0x17>; > > > > pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */ > > > > - max-brightness = <100>; > > > > - default-brightness = <80>; > > > > + brightness-levels = <0 4 8 16 32 64 128 255>; > > > > + default-brightness = <6>; > > > > mps,overvoltage-protection-microvolt = <24000000>; > > > > }; > > > > }; > > > > -- > > > > 2.34.1 > > > >
On Tue, Oct 24, 2023 at 07:53:38AM +0000, Flavio Suligoi wrote: > > On Mon, Oct 23, 2023 at 09:28:03AM +0000, Flavio Suligoi wrote: > > > > On Fri, Oct 20, 2023 at 03:54:33PM +0200, Flavio Suligoi wrote: > > > > > The two properties: > > > > > > > > > > - max-brightness > > > > > - default brightness > > > > > > > > > > are not really required, so they can be removed from the "required" > > > > > section. > > > > > > > > Why are they not required? You need to provide an explanation. > > > > > > The "max-brightness" is not more used now in the driver (I used it in > > > the first version of the driver). > > > > If it is not used any more, what happens when someone passes an old > > devicetree to the kernel, that contains max-brightness, but not any of your > > new properties? > > This is not a problem, because the device driver has not yet been included in any kernel. > My patch for the device driver is still being analyzed by the maintainers. > Only this dt-binding yaml file is already included in the "for-backlight-next" branch > of the "backlight" kernel repository. > At the moment, this driver is used only in a i.MX8MM board produced in my company, > under my full control. No other developer is using it now. Right. This is exactly the sort of commentary that you need to provide up front, to have us spent a bunch of time going back and forth to figure out :( > > > The "default-brightness", if omitted in the DT, is managed by the > > > device driver, using a default value. This depends on the dimming mode > > used: > > > > For default-brightness, has here always been support in the driver for the > > property being omitted, or is this newly added? > > In the first version of the driver this property was a "required property", > but nobody has used this driver before, so this should be not a problem. > > What I would like is an explanation in the commit message as to why the > > revised example is more helpful than the existing (and > > must-remain-valid) one. > > As said before, no one may have ever used this device driver, > so I would leave only this new version of the example. Okay. Please improve the commit message explaining why it is okay to make these changes & send a v2. The alternative is that Lee drops the dt-binding patch & you submit a revised version of the binding alongside the next iteration of the driver. Cheers, Conor.
diff --git a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml index 4191e33626f5..527a37368ed7 100644 --- a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml +++ b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml @@ -14,8 +14,8 @@ description: | programmable switching frequency to optimize efficiency. It supports two different dimming modes: - - analog mode, via I2C commands (default) - - PWM controlled mode. + - analog mode, via I2C commands, as default mode (32 dimming levels) + - PWM controlled mode (optional) The datasheet is available at: https://www.monolithicpower.com/en/mp3309c.html @@ -50,8 +50,6 @@ properties: required: - compatible - reg - - max-brightness - - default-brightness unevaluatedProperties: false @@ -66,8 +64,8 @@ examples: compatible = "mps,mp3309c"; reg = <0x17>; pwms = <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*10^9 */ - max-brightness = <100>; - default-brightness = <80>; + brightness-levels = <0 4 8 16 32 64 128 255>; + default-brightness = <6>; mps,overvoltage-protection-microvolt = <24000000>; }; };
The two properties: - max-brightness - default brightness are not really required, so they can be removed from the "required" section. Other changes: - improve the backlight working mode description, in the "description" section - update the example, removing the "max-brightness" and introducing the "brightess-levels" property Signed-off-by: Flavio Suligoi <f.suligoi@asem.it> --- .../bindings/leds/backlight/mps,mp3309c.yaml | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)