RE: [PATCH v1 1/2] dt-bindings: backlight: Add MPS MP3309C
Hi Krzysztof, ... > >>> + > >>> + reg: > >>> +maxItems: 1 > >>> + > >>> + mps,dimming-mode: > >>> +description: The dimming mode (PWM or analog by I2C commands). > >>> +$ref: '/schemas/types.yaml#/definitions/string' > >> > >> Drop quotes, you should see warnings for this. > >> > >> It does not look like you tested the bindings, at least after quick > >> look. Please run `make dt_binding_check` (see > >> Documentation/devicetree/bindings/writing-schema.rst for instructions). > >> Maybe you need to update your dtschema and yamllint. > >> > >>> +enum: > >>> + - pwm > >>> + - analog-i2c > >> > >> Why do you think this is a property of a board? Is PWM signal optional? > >> If so, its presence would define it. Otherwise it seems you want to > >> control the driver. > >> > > > > The MP3309C device always need a I2C bus to rd/wr its internal registers. > > But the brightness can be controlled in one of the following ways > > (mutually exclusive, but mandatory): > > - a PWM input signal > > or > > - a I2C command > > So, the driver needs a property to select the dimming mode used; this > property is mandatory. > > No, it's not a proof. Don't mix properties and hardware signals. > > > This is the reason of the existence of the ' mps,dimming-mode' property. > > PWM signal is not optional, it is required if and only if the 'pwm' dimming > mode is used. > > So the pwms determine the mode. That's it, no need for this property. > > > > If the 'analog-i2c' dimming mode is used, instead, the PWM signal must not > be used. > > So the property 'mps,dimming-mode' controls how the MP3309C is used. > > I can add more details about this in the description section. > > No, drop the property or explain more, e.g. is I2C mode of control used while > having PWMs signals connected? Ok, I think I understand what you mean: since the I2C bus is always present to configure the chip, the 'analog-i2c' dimming mode is the default mode. The other type of dimming mode, the 'pwm' dimming mode, is an option. When the pwm is present in the DT, the pwm signal is used as dimming control mode instead of the i2c dimming control mode. In this way the ' mps,dimming-mode' property is not more necessary and can be eliminated. > > > ... > > > >>> + > >>> + mps,overvoltage-protection-13v: > >>> +description: overvoltage protection set to 13.5V. > >>> +type: boolean > >>> + mps,overvoltage-protection-24v: > >>> +description: overvoltage protection set to 24V. > >>> +type: boolean > >>> + mps,overvoltage-protection-35v: > >>> +description: overvoltage protection set to 35.5V. > >>> +type: boolean > >> > >> Nope for these three. Use -microvolt suffix for one property. > > > > Ok > > > >> > >>> + > >>> + mps,reset-gpios: > >>> +description: optional GPIO to reset an external device (LCD panel, > FPGA, > >>> + etc.) when the backlight is switched on. > >>> +maxItems: 1 > >> > >> No, you should not add here GPIOs for other devices. > > > > Do you mean that I have to remove this property or that I have to move it > somewhere else? > > I added this feature because sometimes, in embedded boards, you need a > > pulse signal to > > How you described it, this is not the property of this device. > > > use after the backlight probing, for example to reset another device > > in sync with the backlight probe. > > There is no term as "probe" in hardware, so you describe drivers. > > > Do you think I have to remove this feature from the driver? > > You cannot request GPIO after removing it from the bindings, obviously, but > whether your backlight should reset something else? Don't care, don't know. I > talk about bindings. This GPIO was used in one of our boards to solve a sync problem, but it is no more necessary. I'll eliminate it, thanks. > > > > > ... > > > >>> +allOf: > >>> + - $ref: common.yaml# > >>> + - if: > >>> + properties: > >>> +mps,dimming-mode: > >>> + contains: > >>> +enum: > >>> + - pwm > >>> +then: > >>> + required: > >>> +- pwms > >> > >> So this proves the point - mps,dimming-mode looks redundant and not > >> hardware related. > > > > See my previous comment. > > No, it still proves the point till you explain why pwms cannot be used to > determine this. Read my messages. > > Best regards, > Krzysztof Thanks for your help! Flavio
RE: [PATCH v1 1/2] dt-bindings: backlight: Add MPS MP3309C
Hi Krzysztof,
Thanks for your quick replay and corrections!
Just some questions about some of your remarks:
> > @@ -0,0 +1,202 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
>
> > +
> > + reg:
> > +maxItems: 1
> > +
> > + mps,dimming-mode:
> > +description: The dimming mode (PWM or analog by I2C commands).
> > +$ref: '/schemas/types.yaml#/definitions/string'
>
> Drop quotes, you should see warnings for this.
>
> It does not look like you tested the bindings, at least after quick look.
> Please
> run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
>
> > +enum:
> > + - pwm
> > + - analog-i2c
>
> Why do you think this is a property of a board? Is PWM signal optional?
> If so, its presence would define it. Otherwise it seems you want to control
> the
> driver.
>
The MP3309C device always need a I2C bus to rd/wr its internal registers.
But the brightness can be controlled in one of the following ways (mutually
exclusive,
but mandatory):
- a PWM input signal
or
- a I2C command
So, the driver needs a property to select the dimming mode used; this property
is mandatory.
This is the reason of the existence of the ' mps,dimming-mode' property.
PWM signal is not optional, it is required if and only if the 'pwm' dimming
mode is used.
If the 'analog-i2c' dimming mode is used, instead, the PWM signal must not be
used.
So the property 'mps,dimming-mode' controls how the MP3309C is used.
I can add more details about this in the description section.
...
> > +
> > + mps,overvoltage-protection-13v:
> > +description: overvoltage protection set to 13.5V.
> > +type: boolean
> > + mps,overvoltage-protection-24v:
> > +description: overvoltage protection set to 24V.
> > +type: boolean
> > + mps,overvoltage-protection-35v:
> > +description: overvoltage protection set to 35.5V.
> > +type: boolean
>
> Nope for these three. Use -microvolt suffix for one property.
Ok
>
> > +
> > + mps,reset-gpios:
> > +description: optional GPIO to reset an external device (LCD panel,
> > FPGA,
> > + etc.) when the backlight is switched on.
> > +maxItems: 1
>
> No, you should not add here GPIOs for other devices.
Do you mean that I have to remove this property or that I have to move it
somewhere else?
I added this feature because sometimes, in embedded boards, you need a pulse
signal to
use after the backlight probing, for example to reset another device in sync
with the backlight
probe.
Do you think I have to remove this feature from the driver?
...
> > +allOf:
> > + - $ref: common.yaml#
> > + - if:
> > + properties:
> > +mps,dimming-mode:
> > + contains:
> > +enum:
> > + - pwm
> > +then:
> > + required:
> > +- pwms
>
> So this proves the point - mps,dimming-mode looks redundant and not
> hardware related.
See my previous comment.
>
> > + not:
> > +required:
> > + - enable-gpios
> > +
> > + - if:
> > + properties:
> > +mps,dimming-mode:
> > + contains:
> > +enum:
> > + - analog-i2c
> > +then:
> > + required:
> > +- enable-gpios
> > + not:
> > +required:
> > + - pwms
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - mps,dimming-mode
> > + - max-brightness
> > + - default-brightness
> > +
> > +additionalProperties: false
>
> Instead:
> unevaluatedProperties: false
>
Ok
> > +
> > +examples:
> > + - |
> > +#include
> > +i2c3 {
>
> i2c
>
> > +#address-cells = <1>;
> > +#size-cells = <0>;
> > +
> > +clock-frequency = <10>;
>
> Drop
>
> > +pinctrl-names = "default";
> > +pinctrl-0 = <_i2c3>;
> > +status = "okay";
>
> Drop all except of cells.
Ok
>
> > +
> > +/* Backlight with PWM control */
> > +backlight_pwm: backlight@17 {
> > +compatible = "mps,mp3309c-backlight";
> > +reg = <0x17>;
> > +mps,dimming-mode = "pwm";
> > +pinctrl-names = "default";
> > +pinctrl-0 = <_fpga_reset>;
> > +pwms = < 0 333 0>; /* 300 Hz --> (1/f) * 1*10^9 */
> > +max-brightness = <100>;
> > +default-brightness = <80>;
> > +mps,switch-on-delay-ms = <800>;
> > +mps,switch-off-delay-ms = <10>;
> > +mps,overvoltage-protection-24v;
> > +
> > +/*
> > + * Enable an FPGA reset pulse when MIPI data are stable,
> > + * before switch on the backlight
> > + */
> > +mps,reset-gpios = < 20 GPIO_ACTIVE_HIGH>;
>
> Nope, nope. FPGA reset pin is not related to this device.
See my previous comment/question about this feature.
>
> > +mps,reset-on-delay-ms = <100>;
> > +
Re: [PATCH v1 1/2] dt-bindings: backlight: Add MPS MP3309C
On 29/08/2023 16:18, Flavio Suligoi wrote: > Hi Krzysztof, > > Thanks for your quick replay and corrections! > Just some questions about some of your remarks: > >>> @@ -0,0 +1,202 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2 >>> +--- >> >>> + >>> + reg: >>> +maxItems: 1 >>> + >>> + mps,dimming-mode: >>> +description: The dimming mode (PWM or analog by I2C commands). >>> +$ref: '/schemas/types.yaml#/definitions/string' >> >> Drop quotes, you should see warnings for this. >> >> It does not look like you tested the bindings, at least after quick look. >> Please >> run `make dt_binding_check` (see >> Documentation/devicetree/bindings/writing-schema.rst for instructions). >> Maybe you need to update your dtschema and yamllint. >> >>> +enum: >>> + - pwm >>> + - analog-i2c >> >> Why do you think this is a property of a board? Is PWM signal optional? >> If so, its presence would define it. Otherwise it seems you want to control >> the >> driver. >> > > The MP3309C device always need a I2C bus to rd/wr its internal registers. > But the brightness can be controlled in one of the following ways (mutually > exclusive, > but mandatory): > - a PWM input signal > or > - a I2C command > So, the driver needs a property to select the dimming mode used; this > property is mandatory. No, it's not a proof. Don't mix properties and hardware signals. > This is the reason of the existence of the ' mps,dimming-mode' property. > PWM signal is not optional, it is required if and only if the 'pwm' dimming > mode is used. So the pwms determine the mode. That's it, no need for this property. > If the 'analog-i2c' dimming mode is used, instead, the PWM signal must not be > used. > So the property 'mps,dimming-mode' controls how the MP3309C is used. > I can add more details about this in the description section. No, drop the property or explain more, e.g. is I2C mode of control used while having PWMs signals connected? > ... > >>> + >>> + mps,overvoltage-protection-13v: >>> +description: overvoltage protection set to 13.5V. >>> +type: boolean >>> + mps,overvoltage-protection-24v: >>> +description: overvoltage protection set to 24V. >>> +type: boolean >>> + mps,overvoltage-protection-35v: >>> +description: overvoltage protection set to 35.5V. >>> +type: boolean >> >> Nope for these three. Use -microvolt suffix for one property. > > Ok > >> >>> + >>> + mps,reset-gpios: >>> +description: optional GPIO to reset an external device (LCD panel, >>> FPGA, >>> + etc.) when the backlight is switched on. >>> +maxItems: 1 >> >> No, you should not add here GPIOs for other devices. > > Do you mean that I have to remove this property or that I have to move it > somewhere else? > I added this feature because sometimes, in embedded boards, you need a pulse > signal to How you described it, this is not the property of this device. > use after the backlight probing, for example to reset another device in sync > with the backlight > probe. There is no term as "probe" in hardware, so you describe drivers. > Do you think I have to remove this feature from the driver? You cannot request GPIO after removing it from the bindings, obviously, but whether your backlight should reset something else? Don't care, don't know. I talk about bindings. > > ... > >>> +allOf: >>> + - $ref: common.yaml# >>> + - if: >>> + properties: >>> +mps,dimming-mode: >>> + contains: >>> +enum: >>> + - pwm >>> +then: >>> + required: >>> +- pwms >> >> So this proves the point - mps,dimming-mode looks redundant and not >> hardware related. > > See my previous comment. No, it still proves the point till you explain why pwms cannot be used to determine this. Read my messages. Best regards, Krzysztof
Re: [PATCH v1 1/2] dt-bindings: backlight: Add MPS MP3309C
On 29/08/2023 12:15, Flavio Suligoi wrote: > The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a > programmable switching frequency to optimize efficiency. > The brightness can be controlled either by I2C commands (called "analog" > mode) or by a PWM input signal (PWM mode). > This driver supports both modes. > > For device driver details, please refer to: > - drivers/video/backlight/mp3309c_bl.c > > The datasheet is available at: > - https://www.monolithicpower.com/en/mp3309c.html > > Signed-off-by: Flavio Suligoi > --- > .../bindings/leds/backlight/mps,mp3309c.yaml | 202 ++ > 1 file changed, 202 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > > diff --git > a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > new file mode 100644 > index ..a58904f2a271 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml > @@ -0,0 +1,202 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/leds/backlight/mps,mp3309c.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: MPS MP3309C backlight > + > +maintainers: > + - Flavio Suligoi > + > +description: | > + The Monolithic Power (MPS) MP3309C is a WLED step-up converter, featuring a > + programmable switching frequency to optimize efficiency. > + It supports both analog (via I2C commands) and PWM dimming mode. > + > + The datasheet is available at: > + https://www.monolithicpower.com/en/mp3309c.html > + > +properties: > + compatible: > +const: mps,mp3309c-backlight Drop "-backlight". Can it be anything else? > + > + reg: > +maxItems: 1 > + > + mps,dimming-mode: > +description: The dimming mode (PWM or analog by I2C commands). > +$ref: '/schemas/types.yaml#/definitions/string' Drop quotes, you should see warnings for this. It does not look like you tested the bindings, at least after quick look. Please run `make dt_binding_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Maybe you need to update your dtschema and yamllint. > +enum: > + - pwm > + - analog-i2c Why do you think this is a property of a board? Is PWM signal optional? If so, its presence would define it. Otherwise it seems you want to control the driver. > + > + pinctrl-names: > +items: > + - const: default Drop > + > + pinctrl-0: true Drop > + > + pwms: > +description: PWM channel used for controlling the backlight in "pwm" > dimming > + mode. > +maxItems: 1 > + > + default-brightness: > +minimum: 0 0 is minimum. Provide rather maximum? or just skip the property. > + > + max-brightness: > +minimum: 1 Same concerns. > + > + enable-gpios: > +description: GPIO used to enable the backlight in "analog-i2c" dimming > mode. > +maxItems: 1 > + > + mps,switch-on-delay-ms: > +description: delay (in ms) before switch on the backlight, to wait for > image > + stabilization. > +default: 10 > + > + mps,switch-off-delay-ms: > +description: delay (in ms) after the switch off command to the backlight. > +default: 0 > + > + mps,overvoltage-protection-13v: > +description: overvoltage protection set to 13.5V. > +type: boolean > + mps,overvoltage-protection-24v: > +description: overvoltage protection set to 24V. > +type: boolean > + mps,overvoltage-protection-35v: > +description: overvoltage protection set to 35.5V. > +type: boolean Nope for these three. Use -microvolt suffix for one property. > + > + mps,reset-gpios: > +description: optional GPIO to reset an external device (LCD panel, FPGA, > + etc.) when the backlight is switched on. > +maxItems: 1 No, you should not add here GPIOs for other devices. > + > + mps,reset-on-delay-ms: > +description: delay (in s) before generating the reset-gpios. in ms > +default: 10 > + > + mps,reset-on-length-ms: > +description: pulse length (in ms) for reset-gpios. > +default: 10 > + > +oneOf: > + - required: > + - mps,overvoltage-protection-13v > + - required: > + - mps,overvoltage-protection-24v > + - required: > + - mps,overvoltage-protection-35.5v > + > +allOf: > + - $ref: common.yaml# > + - if: > + properties: > +mps,dimming-mode: > + contains: > +enum: > + - pwm > +then: > + required: > +- pwms So this proves the point - mps,dimming-mode looks redundant and not hardware related. > + not: > +required: > + - enable-gpios > + > + - if: > + properties: > +mps,dimming-mode: > + contains: > +enum: > + - analog-i2c > +then: > + required: > +- enable-gpios > + not: > +
