Re: [PATCH v1 1/2] dt-bindings: mfd: Document TI LM3533 MFD

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



ср, 12 лют. 2025 р. о 21:49 Conor Dooley <conor@xxxxxxxxxx> пише:
>
> On Wed, Feb 12, 2025 at 09:58:41AM +0200, Svyatoslav Ryhel wrote:
> > Add bindings for the LM3533 - a complete power source for
> > backlight, keypad, and indicator LEDs in smartphone handsets.
> > The high-voltage inductive boost converter provides the
> > power for two series LED strings display backlight and keypad
> > functions.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > ---
> >  .../devicetree/bindings/mfd/ti,lm3533.yaml    | 221 ++++++++++++++++++
> >  include/dt-bindings/mfd/lm3533.h              |  19 ++
> >  2 files changed, 240 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> >  create mode 100644 include/dt-bindings/mfd/lm3533.h
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> > new file mode 100644
> > index 000000000000..d0307e5894f8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> > @@ -0,0 +1,221 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/ti,lm3533.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: TI LM3533 Complete Lighting Power Solution
> > +
> > +description: |
> > +  The LM3533 is a complete power source for backlight,
> > +  keypad, and indicator LEDs in smartphone handsets. The
> > +  high-voltage inductive boost converter provides the
> > +  power for two series LED strings display backlight and
> > +  keypad functions.
> > +  https://www.ti.com/product/LM3533
> > +
> > +maintainers:
> > +  - Johan Hovold <jhovold@xxxxxxxxx>
> > +
> > +properties:
> > +  compatible:
> > +    const: ti,lm3533
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +  enable-gpios:
> > +    description: GPIO to use to enable/disable the backlight (HWEN pin).
> > +    maxItems: 1
> > +
> > +  ti,boost-ovp:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Boost OVP select (16V, 24V, 32V, 40V)
> > +    enum: [ 0, 1, 2, 3 ]
> > +    default: 0
> > +
> > +  ti,boost-freq:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Boost frequency select (500KHz or 1MHz)
> > +    enum: [ 0, 1 ]
> > +    default: 0
>
> Properties like these should be in units, so some standard voltage-based
> one for the boost and in hertz for the second. See property-units.yaml
> in dt-schema.
>
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#address-cells'
> > +  - '#size-cells'
> > +  - enable-gpios
> > +
> > +patternProperties:
> > +  "^backlight@[01]$":
> > +    type: object
> > +    description:
> > +      Properties for a backlight device.
> > +
> > +    properties:
> > +      compatible:
> > +        const: lm3533-backlight
>
> Missing vendor prefix
>
> > +
> > +      reg:
> > +        description: |
> > +          The control bank that is used to program the two current sinks. The
> > +          LM3533 has two control banks (A and B) and are represented as 0 or 1
> > +          in this property. The two current sinks can be controlled
> > +          independently with both banks, or bank A can be configured to control
> > +          both sinks with the led-sources property.
> > +        minimum: 0
> > +        maximum: 1
> > +
> > +      max-current-microamp:
> > +        description:
> > +          Maximum current in µA with a 800 µA step.
> > +        enum: [ 5000, 5800, 6600, 7400, 8200, 9000, 9800,
> > +                10600, 11400, 12200, 13000, 13800, 14600,
> > +                15400, 16200, 17000, 17800, 18600, 19400,
> > +                20200, 21000, 21800, 22600, 23400, 24200,
> > +                25000, 25800, 26600, 27400, 28200, 29000,
> > +                29800 ]
> > +        default: 5000
> > +
> > +      default-brightness:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description:
> > +          Default brightness level on boot.
> > +        minimum: 0
> > +        maximum: 255
> > +        default: 255
> > +
> > +      pwm:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description:
> > +          Default pwm level on boot.
> > +        minimum: 0
> > +        maximum: 63
> > +        default: 0
>
> Why is default-brightness /and/ a default for pwm needed? If the pwm
> drives the backlight, wouldn't you only need one of these two?
>
> If it stays, I think it needs a rename to be more clear about what it is
> doing. "pwm" is very close to "pwms", the property used for phandles to
> pwm providers but the use is rather different.
>
> backlight/common.yaml has standard properties that you could be using,
> so I'd expect to see a ref here, rather than redefining your own
> version.
>
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +
> > +    additionalProperties: false
> > +
> > +  "^led@[0-3]$":
> > +    type: object
> > +    description:
> > +      Properties for a led device.
> > +
> > +    properties:
> > +      compatible:
> > +        const: lm3533-leds
>
> Ditto here re: compatible.
>
> > +
> > +      reg:
> > +        description:
> > +          4 led banks
> > +        minimum: 0
> > +        maximum: 3
> > +
> > +      max-current-microamp:
> > +        description:
> > +          Maximum current in µA with a 800 µA step.
> > +        enum: [ 5000, 5800, 6600, 7400, 8200, 9000, 9800,
> > +                10600, 11400, 12200, 13000, 13800, 14600,
> > +                15400, 16200, 17000, 17800, 18600, 19400,
> > +                20200, 21000, 21800, 22600, 23400, 24200,
> > +                25000, 25800, 26600, 27400, 28200, 29000,
> > +                29800 ]
> > +        default: 5000
> > +
> > +      default-trigger:
> > +        $ref: /schemas/types.yaml#/definitions/string
> > +        description: |
> > +          This parameter, if present, is a string defining
> > +          the trigger assigned to the LED. Check linux,default-trigger.
> > +
> > +      pwm:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description:
> > +          Default pwm level on boot.
> > +        minimum: 0
> > +        maximum: 63
> > +        default: 0
>
> Same applies here, leds/common.yaml has some of these already.
>
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +
> > +    additionalProperties: false
> > +
> > +  "^light-sensor@[0]$":
>
> Not a pattern if it can only have 1 outcome.
>
> > +    type: object
> > +    description:
> > +      Properties for an illumination sensor.
> > +
> > +    properties:
> > +      compatible:
> > +        const: lm3533-als
> > +
> > +      reg:
> > +        const: 0
> > +
> > +      resistor-value:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: |
> > +          PWM resistor value a linear step in currents
> > +          of 10 µA per code based upon 2V/R_ALS.
> > +        minimum: 1
> > +        maximum: 127
> > +        default: 1
> > +
>
> I'd expect a resistor to be measured in ohms of some sort,
> hard to tell what the increments actually are here, they don't appear to
> be a real unit. Are they register values?
>
> This and all other custom properties need to have a vendor prefix on
> them btw.
>
> > +      pwm-mode:
> > +        type: boolean
> > +        description: Mode in which ALS is running
>
> Are there multiple pwm modes? Or is this trying to say that, if set, the
> sensor is in pwm mode? Hard to tell from the description here, sorry.
>
> Cheers,
> Conor.
>

Acknowledged, thank you.

>
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +
> > +    additionalProperties: false
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/mfd/lm3533.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        led-controller@36 {
> > +            compatible = "ti,lm3533";
> > +            reg = <0x36>;
> > +
> > +            enable-gpios = <&gpio 110 GPIO_ACTIVE_HIGH>;
> > +
> > +            ti,boost-ovp = <LM3533_BOOST_OVP_24V>;
> > +            ti,boost-freq = <LM3533_BOOST_FREQ_500KHZ>;
> > +
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            backlight@0 {
> > +                compatible = "lm3533-backlight";
> > +                reg = <0>;
> > +
> > +                max-current-microamp = <23400>;
> > +                default-brightness = <113>;
> > +                pwm = <0x00>;
> > +            };
> > +        };
> > +    };
>




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux