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
