RE: [PATCH v1 1/2] dt-bindings: backlight: Add MPS MP3309C

[Flavio Suligoi]

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 <dt-bindings/gpio/gpio.h>
> > +    i2c3 {
> 
> i2c
> 
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        clock-frequency = <100000>;
> 
> Drop
> 
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&pinctrl_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 = <&pinctrl_fpga_reset>;
> > +            pwms = <&pwm1 0 3333333 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 = <&gpio4 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>;
> > +            mps,reset-on-length-ms = <10>;
> > +        };
> > +    };
> > +
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    /* Backlight with analog control via I2C bus */
> > +    i2c3 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        clock-frequency = <100000>;
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&pinctrl_i2c3>;
> > +        status = "okay";
> 
> Drop entire example. It differs by one property - missing pwms.

Ok

> 
> 
> Best regards,
> Krzysztof

Thanks and best regards,
Flavio