On 29/08/2023 12:15, Flavio Suligoi wrote:

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