Messages in this thread | | | From | Flavio Suligoi <> | Subject | RE: [PATCH v1 1/2] dt-bindings: backlight: Add MPS MP3309C | Date | Tue, 29 Aug 2023 14:18:48 +0000 |
| |
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
| |