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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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 <f.suligoi@asem.it>
> ---
>  .../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 000000000000..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 <f.suligoi@asem.it>
> +
> +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:
> +        required:
> +          - pwms
> +
> +required:
> +  - compatible
> +  - reg
> +  - mps,dimming-mode
> +  - max-brightness
> +  - default-brightness
> +
> +additionalProperties: false

Instead:
unevaluatedProperties: false

> +
> +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.

> +
> +        /* 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.

> +            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.


Best regards,
Krzysztof