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

Hi Krzysztof,

...

quoted

quoted

quoted

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

quoted

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

quoted

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.

quoted

If the 'analog-i2c' dimming mode is used, instead, the PWM signal must not

be used.

quoted

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.

quoted

...

quoted

quoted

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

quoted

quoted

+
+  mps,reset-gpios:
+    description: optional GPIO to reset an external device (LCD panel,

FPGA,

quoted

quoted

quoted

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

quoted

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.

quoted

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.

quoted

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.

quoted

...

quoted

quoted

+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