[PATCH v1 0/2] mfd: lm3533: convert to use OF

[PATCH v1 0/2] mfd: lm3533: convert to use OF

[Svyatoslav Ryhel]

Add schema and add support for lm3533 mfd to use device
tree bindings.

Svyatoslav Ryhel (2):
  dt-bindings: mfd: Document TI LM3533 MFD
  mfd: lm3533: convert to use OF

 .../devicetree/bindings/mfd/ti,lm3533.yaml    | 221 ++++++++++++++++++
 drivers/iio/light/lm3533-als.c                |  58 ++++-
 drivers/leds/leds-lm3533.c                    |  69 +++++-
 drivers/mfd/lm3533-core.c                     |  79 +++++--
 drivers/video/backlight/lm3533_bl.c           |  72 +++++-
 include/dt-bindings/mfd/lm3533.h              |  19 ++
 include/linux/mfd/lm3533.h                    |   1 +
 7 files changed, 496 insertions(+), 23 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
 create mode 100644 include/dt-bindings/mfd/lm3533.h

-- 
2.43.0

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

[Svyatoslav Ryhel]

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@gmail.com>
---
 .../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@gmail.com>
+
+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
+
+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
+
+      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
+
+    required:
+      - compatible
+      - reg
+
+    additionalProperties: false
+
+  "^led@[0-3]$":
+    type: object
+    description:
+      Properties for a led device.
+
+    properties:
+      compatible:
+        const: lm3533-leds
+
+      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
+
+    required:
+      - compatible
+      - reg
+
+    additionalProperties: false
+
+  "^light-sensor@[0]$":
+    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
+
+      pwm-mode:
+        type: boolean
+        description: Mode in which ALS is running
+
+    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>;
+            };
+        };
+    };
+...
diff --git a/include/dt-bindings/mfd/lm3533.h b/include/dt-bindings/mfd/lm3533.h
new file mode 100644
index 000000000000..929988190c52
--- /dev/null
+++ b/include/dt-bindings/mfd/lm3533.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * This header provides macros for TI LM3533 device bindings.
+ */
+
+#ifndef _DT_BINDINGS_MFD_LM3533_H
+#define _DT_BINDINGS_MFD_LM3533_H
+
+/* LM3533 boost freq */
+#define LM3533_BOOST_FREQ_500KHZ	0
+#define LM3533_BOOST_FREQ_1000KHZ	1
+
+/* LM3533 boost ovp */
+#define LM3533_BOOST_OVP_16V		0
+#define LM3533_BOOST_OVP_24V		1
+#define LM3533_BOOST_OVP_32V		2
+#define LM3533_BOOST_OVP_40V		3
+
+#endif
-- 
2.43.0

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 8469 bytes --]

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@gmail.com>
> ---
>  .../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@gmail.com>
> +
> +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.


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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Svyatoslav Ryhel]

ср, 12 лют. 2025 р. о 21:49 Conor Dooley <conor@kernel.org> пише:
>
> 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@gmail.com>
> > ---
> >  .../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@gmail.com>
> > +
> > +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>;
> > +            };
> > +        };
> > +    };
>

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

[Daniel Thompson]

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@gmail.com>
> ---
>  .../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@gmail.com>

This looks like it has been copied from the lm3533 driver. Did Johan
agree to this?


Daniel.

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

[Svyatoslav Ryhel]

чт, 13 лют. 2025 р. о 23:32 Daniel Thompson <daniel@riscstar.com> пише:
>
> 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@gmail.com>
> > ---
> >  .../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@gmail.com>
>
> This looks like it has been copied from the lm3533 driver. Did Johan
> agree to this?
>

Thank you for pointing this out, maintainers field should have been
amended with my name. It seems that this slipped from me on
submitting. I initially though that maintainers should contain driver
author hence set Johan, but that is obviously not correct.

>
> Daniel.

[PATCH v1 2/2] mfd: lm3533: convert to use OF

[Svyatoslav Ryhel]

Add ability to fill pdata from device tree. Common stuff is
filled from core driver and then pdata is filled per-device
since all cells are optional.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/iio/light/lm3533-als.c      | 58 ++++++++++++++++++++-
 drivers/leds/leds-lm3533.c          | 69 +++++++++++++++++++++++--
 drivers/mfd/lm3533-core.c           | 79 ++++++++++++++++++++++++-----
 drivers/video/backlight/lm3533_bl.c | 72 ++++++++++++++++++++++++--
 include/linux/mfd/lm3533.h          |  1 +
 5 files changed, 256 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
index 99f0b903018c..21fc5f215c80 100644
--- a/drivers/iio/light/lm3533-als.c
+++ b/drivers/iio/light/lm3533-als.c
@@ -16,7 +16,9 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/mfd/core.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 
@@ -826,6 +828,50 @@ static const struct iio_info lm3533_als_info = {
 	.read_raw	= &lm3533_als_read_raw,
 };
 
+static void lm3533_parse_als(struct platform_device *pdev,
+			     struct lm3533_als_platform_data *pdata)
+{
+	struct device *dev = &pdev->dev;
+	int val, ret;
+
+	/* 1 - 127 (ignored in PWM-mode) */
+	ret = device_property_read_u32(dev, "resistor-value", &val);
+	if (ret)
+		val = 1;
+	pdata->r_select = val;
+
+	pdata->pwm_mode = device_property_read_bool(dev, "pwm-mode");
+}
+
+static int lm3533_pass_of_node(struct platform_device *pdev,
+			       struct lm3533_als_platform_data *pdata)
+{
+	struct device *parent_dev = pdev->dev.parent;
+	struct device *dev = &pdev->dev;
+	struct fwnode_handle *node;
+	const char *label;
+	int val, ret;
+
+	device_for_each_child_node(parent_dev, node) {
+		fwnode_property_read_string(node, "compatible", &label);
+
+		if (!strcmp(label, pdev->name)) {
+			ret = fwnode_property_read_u32(node, "reg", &val);
+			if (ret) {
+				dev_err(dev, "reg property is missing: ret %d\n", ret);
+				return ret;
+			}
+
+			if (val == pdev->id) {
+				dev->fwnode = node;
+				dev->of_node = to_of_node(node);
+			}
+		}
+	}
+
+	return 0;
+}
+
 static int lm3533_als_probe(struct platform_device *pdev)
 {
 	const struct lm3533_als_platform_data *pdata;
@@ -840,8 +886,16 @@ static int lm3533_als_probe(struct platform_device *pdev)
 
 	pdata = dev_get_platdata(&pdev->dev);
 	if (!pdata) {
-		dev_err(&pdev->dev, "no platform data\n");
-		return -EINVAL;
+		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+
+		ret = lm3533_pass_of_node(pdev, pdata);
+		if (ret)
+			return dev_err_probe(&pdev->dev, ret,
+					     "failed to get als device node\n");
+
+		lm3533_parse_als(pdev, pdata);
 	}
 
 	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*als));
diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
index 45795f2a1042..587bc27b17c3 100644
--- a/drivers/leds/leds-lm3533.c
+++ b/drivers/leds/leds-lm3533.c
@@ -11,7 +11,9 @@
 #include <linux/leds.h>
 #include <linux/mfd/core.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 
 #include <linux/mfd/lm3533.h>
@@ -644,6 +646,59 @@ static int lm3533_led_setup(struct lm3533_led *led,
 	return lm3533_ctrlbank_set_pwm(&led->cb, pdata->pwm);
 }
 
+static void lm3533_parse_led(struct platform_device *pdev,
+			     struct lm3533_led_platform_data *pdata)
+{
+	struct device *dev = &pdev->dev;
+	int val, ret;
+
+	ret = device_property_read_string(dev, "default-trigger",
+					  &pdata->default_trigger);
+	if (ret)
+		pdata->default_trigger = "none";
+
+	/* 5000 - 29800 uA (800 uA step) */
+	ret = device_property_read_u32(dev, "max-current-microamp", &val);
+	if (ret)
+		val = 5000;
+	pdata->max_current = val;
+
+	/* 0 - 0x3f */
+	ret = device_property_read_u32(dev, "pwm", &val);
+	if (ret)
+		val = 0;
+	pdata->pwm = val;
+}
+
+static int lm3533_pass_of_node(struct platform_device *pdev,
+			       struct lm3533_led_platform_data *pdata)
+{
+	struct device *parent_dev = pdev->dev.parent;
+	struct device *dev = &pdev->dev;
+	struct fwnode_handle *node;
+	const char *label;
+	int val, ret;
+
+	device_for_each_child_node(parent_dev, node) {
+		fwnode_property_read_string(node, "compatible", &label);
+
+		if (!strcmp(label, pdev->name)) {
+			ret = fwnode_property_read_u32(node, "reg", &val);
+			if (ret) {
+				dev_info(dev, "reg property is missing: ret %d\n", ret);
+				return ret;
+			}
+
+			if (val == pdev->id) {
+				dev->fwnode = node;
+				dev->of_node = to_of_node(node);
+			}
+		}
+	}
+
+	return 0;
+}
+
 static int lm3533_led_probe(struct platform_device *pdev)
 {
 	struct lm3533 *lm3533;
@@ -659,8 +714,16 @@ static int lm3533_led_probe(struct platform_device *pdev)
 
 	pdata = dev_get_platdata(&pdev->dev);
 	if (!pdata) {
-		dev_err(&pdev->dev, "no platform data\n");
-		return -EINVAL;
+		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+
+		ret = lm3533_pass_of_node(pdev, pdata);
+		if (ret)
+			return dev_err_probe(&pdev->dev, ret,
+					     "failed to get led device node\n");
+
+		lm3533_parse_led(pdev, pdata);
 	}
 
 	if (pdev->id < 0 || pdev->id >= LM3533_LVCTRLBANK_COUNT) {
@@ -673,7 +736,7 @@ static int lm3533_led_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	led->lm3533 = lm3533;
-	led->cdev.name = pdata->name;
+	led->cdev.name = dev_name(&pdev->dev);
 	led->cdev.default_trigger = pdata->default_trigger;
 	led->cdev.brightness_set_blocking = lm3533_led_set;
 	led->cdev.brightness_get = lm3533_led_get;
diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
index 0a2409d00b2e..cab60c25e3c7 100644
--- a/drivers/mfd/lm3533-core.c
+++ b/drivers/mfd/lm3533-core.c
@@ -381,11 +381,16 @@ static int lm3533_device_als_init(struct lm3533 *lm3533)
 	struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
 	int ret;
 
-	if (!pdata->als)
+	if (!pdata->num_als)
 		return 0;
 
-	lm3533_als_devs[0].platform_data = pdata->als;
-	lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
+	if (pdata->num_als > ARRAY_SIZE(lm3533_als_devs))
+		pdata->num_als = ARRAY_SIZE(lm3533_als_devs);
+
+	if (pdata->als) {
+		lm3533_als_devs[0].platform_data = pdata->als;
+		lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
+	}
 
 	ret = mfd_add_devices(lm3533->dev, 0, lm3533_als_devs, 1, NULL,
 			      0, NULL);
@@ -405,15 +410,17 @@ static int lm3533_device_bl_init(struct lm3533 *lm3533)
 	int i;
 	int ret;
 
-	if (!pdata->backlights || pdata->num_backlights == 0)
+	if (!pdata->num_backlights)
 		return 0;
 
 	if (pdata->num_backlights > ARRAY_SIZE(lm3533_bl_devs))
 		pdata->num_backlights = ARRAY_SIZE(lm3533_bl_devs);
 
-	for (i = 0; i < pdata->num_backlights; ++i) {
-		lm3533_bl_devs[i].platform_data = &pdata->backlights[i];
-		lm3533_bl_devs[i].pdata_size = sizeof(pdata->backlights[i]);
+	if (pdata->backlights) {
+		for (i = 0; i < pdata->num_backlights; ++i) {
+			lm3533_bl_devs[i].platform_data = &pdata->backlights[i];
+			lm3533_bl_devs[i].pdata_size = sizeof(pdata->backlights[i]);
+		}
 	}
 
 	ret = mfd_add_devices(lm3533->dev, 0, lm3533_bl_devs,
@@ -434,15 +441,17 @@ static int lm3533_device_led_init(struct lm3533 *lm3533)
 	int i;
 	int ret;
 
-	if (!pdata->leds || pdata->num_leds == 0)
+	if (!pdata->num_leds)
 		return 0;
 
 	if (pdata->num_leds > ARRAY_SIZE(lm3533_led_devs))
 		pdata->num_leds = ARRAY_SIZE(lm3533_led_devs);
 
-	for (i = 0; i < pdata->num_leds; ++i) {
-		lm3533_led_devs[i].platform_data = &pdata->leds[i];
-		lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
+	if (pdata->leds) {
+		for (i = 0; i < pdata->num_leds; ++i) {
+			lm3533_led_devs[i].platform_data = &pdata->leds[i];
+			lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
+		}
 	}
 
 	ret = mfd_add_devices(lm3533->dev, 0, lm3533_led_devs,
@@ -469,6 +478,26 @@ static int lm3533_device_setup(struct lm3533 *lm3533,
 	return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
 }
 
+static void lm3533_parse_nodes(struct lm3533 *lm3533,
+			       struct lm3533_platform_data *pdata)
+{
+	struct fwnode_handle *node;
+	const char *label;
+
+	device_for_each_child_node(lm3533->dev, node) {
+		fwnode_property_read_string(node, "compatible", &label);
+
+		if (!strcmp(label, lm3533_bl_devs[pdata->num_backlights].name))
+			pdata->num_backlights++;
+
+		if (!strcmp(label, lm3533_led_devs[pdata->num_leds].name))
+			pdata->num_leds++;
+
+		if (!strcmp(label, lm3533_als_devs[pdata->num_als].name))
+			pdata->num_als++;
+	}
+}
+
 static int lm3533_device_init(struct lm3533 *lm3533)
 {
 	struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
@@ -477,8 +506,25 @@ static int lm3533_device_init(struct lm3533 *lm3533)
 	dev_dbg(lm3533->dev, "%s\n", __func__);
 
 	if (!pdata) {
-		dev_err(lm3533->dev, "no platform data\n");
-		return -EINVAL;
+		pdata = devm_kzalloc(lm3533->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+
+		ret = device_property_read_u32(lm3533->dev,
+					       "ti,boost-ovp",
+					       &pdata->boost_ovp);
+		if (ret)
+			pdata->boost_ovp = LM3533_BOOST_OVP_16V;
+
+		ret = device_property_read_u32(lm3533->dev,
+					       "ti,boost-freq",
+					       &pdata->boost_freq);
+		if (ret)
+			pdata->boost_freq = LM3533_BOOST_FREQ_500KHZ;
+
+		lm3533_parse_nodes(lm3533, pdata);
+
+		lm3533->dev->platform_data = pdata;
 	}
 
 	lm3533->hwen = devm_gpiod_get(lm3533->dev, NULL, GPIOD_OUT_LOW);
@@ -603,6 +649,12 @@ static void lm3533_i2c_remove(struct i2c_client *i2c)
 	lm3533_device_exit(lm3533);
 }
 
+static const struct of_device_id lm3533_match_table[] = {
+	{ .compatible = "ti,lm3533" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, lm3533_match_table);
+
 static const struct i2c_device_id lm3533_i2c_ids[] = {
 	{ "lm3533" },
 	{ }
@@ -612,6 +664,7 @@ MODULE_DEVICE_TABLE(i2c, lm3533_i2c_ids);
 static struct i2c_driver lm3533_i2c_driver = {
 	.driver = {
 		   .name = "lm3533",
+		   .of_match_table = lm3533_match_table,
 	},
 	.id_table	= lm3533_i2c_ids,
 	.probe		= lm3533_i2c_probe,
diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
index babfd3ceec86..bc568916f1b1 100644
--- a/drivers/video/backlight/lm3533_bl.c
+++ b/drivers/video/backlight/lm3533_bl.c
@@ -258,6 +258,60 @@ static int lm3533_bl_setup(struct lm3533_bl *bl,
 	return lm3533_ctrlbank_set_pwm(&bl->cb, pdata->pwm);
 }
 
+static void lm3533_parse_backlight(struct platform_device *pdev,
+				   struct lm3533_bl_platform_data *pdata)
+{
+	struct device *dev = &pdev->dev;
+	int val, ret;
+
+	/* 5000 - 29800 uA (800 uA step) */
+	ret = device_property_read_u32(dev, "max-current-microamp", &val);
+	if (ret)
+		val = 5000;
+	pdata->max_current = val;
+
+	/* 0 - 255 */
+	ret = device_property_read_u32(dev, "default-brightness", &val);
+	if (ret)
+		val = LM3533_BL_MAX_BRIGHTNESS;
+	pdata->default_brightness = val;
+
+	/* 0 - 0x3f */
+	ret = device_property_read_u32(dev, "pwm", &val);
+	if (ret)
+		val = 0;
+	pdata->pwm = val;
+}
+
+static int lm3533_pass_of_node(struct platform_device *pdev,
+			       struct lm3533_bl_platform_data *pdata)
+{
+	struct device *parent_dev = pdev->dev.parent;
+	struct device *dev = &pdev->dev;
+	struct fwnode_handle *node;
+	const char *label;
+	int val, ret;
+
+	device_for_each_child_node(parent_dev, node) {
+		fwnode_property_read_string(node, "compatible", &label);
+
+		if (!strcmp(label, pdev->name)) {
+			ret = fwnode_property_read_u32(node, "reg", &val);
+			if (ret) {
+				dev_info(dev, "reg property is missing: ret %d\n", ret);
+				return ret;
+			}
+
+			if (val == pdev->id) {
+				dev->fwnode = node;
+				dev->of_node = to_of_node(node);
+			}
+		}
+	}
+
+	return 0;
+}
+
 static int lm3533_bl_probe(struct platform_device *pdev)
 {
 	struct lm3533 *lm3533;
@@ -275,8 +329,16 @@ static int lm3533_bl_probe(struct platform_device *pdev)
 
 	pdata = dev_get_platdata(&pdev->dev);
 	if (!pdata) {
-		dev_err(&pdev->dev, "no platform data\n");
-		return -EINVAL;
+		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+
+		ret = lm3533_pass_of_node(pdev, pdata);
+		if (ret)
+			return dev_err_probe(&pdev->dev, ret,
+					     "failed to get backlight device node\n");
+
+		lm3533_parse_backlight(pdev, pdata);
 	}
 
 	if (pdev->id < 0 || pdev->id >= LM3533_HVCTRLBANK_COUNT) {
@@ -299,9 +361,9 @@ static int lm3533_bl_probe(struct platform_device *pdev)
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = LM3533_BL_MAX_BRIGHTNESS;
 	props.brightness = pdata->default_brightness;
-	bd = devm_backlight_device_register(&pdev->dev, pdata->name,
-					pdev->dev.parent, bl, &lm3533_bl_ops,
-					&props);
+	bd = devm_backlight_device_register(&pdev->dev, dev_name(&pdev->dev),
+					    pdev->dev.parent, bl,
+					    &lm3533_bl_ops, &props);
 	if (IS_ERR(bd)) {
 		dev_err(&pdev->dev, "failed to register backlight device\n");
 		return PTR_ERR(bd);
diff --git a/include/linux/mfd/lm3533.h b/include/linux/mfd/lm3533.h
index 69059a7a2ce5..e22fd2093a95 100644
--- a/include/linux/mfd/lm3533.h
+++ b/include/linux/mfd/lm3533.h
@@ -74,6 +74,7 @@ struct lm3533_platform_data {
 	enum lm3533_boost_freq boost_freq;
 
 	struct lm3533_als_platform_data *als;
+	int num_als;
 
 	struct lm3533_bl_platform_data *backlights;
 	int num_backlights;
-- 
2.43.0

Re: [PATCH v1 2/2] mfd: lm3533: convert to use OF

[Andy Shevchenko]

On Wed, Feb 12, 2025 at 09:58:42AM +0200, Svyatoslav Ryhel wrote:
> Add ability to fill pdata from device tree. Common stuff is
> filled from core driver and then pdata is filled per-device
> since all cells are optional.

...

>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/mfd/core.h>

> +#include <linux/of.h>

Is it used? In any case, please no OF-centric APIs in a new (feature) code.

>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>

...

> +static int lm3533_pass_of_node(struct platform_device *pdev,

pass_of_node sounds a bit awkward.
Perhaps you thought about parse_fwnode ?

> +			       struct lm3533_als_platform_data *pdata)
> +{
> +	struct device *parent_dev = pdev->dev.parent;
> +	struct device *dev = &pdev->dev;
> +	struct fwnode_handle *node;
> +	const char *label;
> +	int val, ret;
> +
> +	device_for_each_child_node(parent_dev, node) {
> +		fwnode_property_read_string(node, "compatible", &label);
> +
> +		if (!strcmp(label, pdev->name)) {

This is a bit strange. Why one need to compare platform device instance (!)
name with compatible which is part of ABI. This looks really wrong approach.
Needs a very good explanation on what's going on here.

Besides that the label is usually filled by LEDS core, why do we need to handle
it in a special way?

> +			ret = fwnode_property_read_u32(node, "reg", &val);
> +			if (ret) {
> +				dev_err(dev, "reg property is missing: ret %d\n", ret);
> +				return ret;
> +			}
> +
> +			if (val == pdev->id) {

> +				dev->fwnode = node;
> +				dev->of_node = to_of_node(node);

No direct access to fwnode in struct device, please use device_set_node().

> +			}
> +		}
> +	}
> +
> +	return 0;
> +}

...

>  	pdata = dev_get_platdata(&pdev->dev);
>  	if (!pdata) {
> -		dev_err(&pdev->dev, "no platform data\n");
> -		return -EINVAL;
> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		ret = lm3533_pass_of_node(pdev, pdata);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "failed to get als device node\n");

With

	struct device *dev = &pdev->dev;

at the top of the function will help a lot in making the code neater and easier
to read.

> +		lm3533_parse_als(pdev, pdata);
>  	}

...

>  #include <linux/leds.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mutex.h>

> +#include <linux/of.h>

Cargo cult? "Proxy" header? Please follow IWYU principle.

>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/slab.h>

...

> +static void lm3533_parse_led(struct platform_device *pdev,
> +			     struct lm3533_led_platform_data *pdata)
> +{
> +	struct device *dev = &pdev->dev;
> +	int val, ret;
> +
> +	ret = device_property_read_string(dev, "default-trigger",
> +					  &pdata->default_trigger);
> +	if (ret)
> +		pdata->default_trigger = "none";

Isn't this done already in LEDS core?

> +	/* 5000 - 29800 uA (800 uA step) */
> +	ret = device_property_read_u32(dev, "max-current-microamp", &val);
> +	if (ret)
> +		val = 5000;
> +	pdata->max_current = val;
> +
> +	/* 0 - 0x3f */
> +	ret = device_property_read_u32(dev, "pwm", &val);
> +	if (ret)
> +		val = 0;
> +	pdata->pwm = val;
> +}

...

> +static int lm3533_pass_of_node(struct platform_device *pdev,
> +			       struct lm3533_led_platform_data *pdata)

I think I already saw exactly the same piece of code. Please make sure
you have no duplications.

> +}

...

>  	pdata = dev_get_platdata(&pdev->dev);
>  	if (!pdata) {
> -		dev_err(&pdev->dev, "no platform data\n");
> -		return -EINVAL;
> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		ret = lm3533_pass_of_node(pdev, pdata);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "failed to get led device node\n");
> +
> +		lm3533_parse_led(pdev, pdata);

Ditto.

>  	}

...

> -	led->cdev.name = pdata->name;
> +	led->cdev.name = dev_name(&pdev->dev);

Are you sure it's a good idea?

...

> -	if (!pdata->als)
> +	if (!pdata->num_als)
>  		return 0;
>  
> -	lm3533_als_devs[0].platform_data = pdata->als;
> -	lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
> +	if (pdata->num_als > ARRAY_SIZE(lm3533_als_devs))
> +		pdata->num_als = ARRAY_SIZE(lm3533_als_devs);

Looks like you want

	pdata->num_als = clamp(pdata->num_als, 0, ARRAY_SIZE(lm3533_als_devs));
	if (!pdata->num_als)
		return 0;

instead of the above. You would need minmax.h for that.

...

> +	if (pdata->leds) {

This is strange. I would expect num_leds == 0 in this case

> +		for (i = 0; i < pdata->num_leds; ++i) {
> +			lm3533_led_devs[i].platform_data = &pdata->leds[i];
> +			lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
> +		}
>  	}

...

> +static void lm3533_parse_nodes(struct lm3533 *lm3533,
> +			       struct lm3533_platform_data *pdata)
> +{
> +	struct fwnode_handle *node;
> +	const char *label;
> +
> +	device_for_each_child_node(lm3533->dev, node) {
> +		fwnode_property_read_string(node, "compatible", &label);
> +
> +		if (!strcmp(label, lm3533_bl_devs[pdata->num_backlights].name))
> +			pdata->num_backlights++;
> +
> +		if (!strcmp(label, lm3533_led_devs[pdata->num_leds].name))
> +			pdata->num_leds++;
> +
> +		if (!strcmp(label, lm3533_als_devs[pdata->num_als].name))
> +			pdata->num_als++;
> +	}
> +}

Oh, I don't like this approach. If you have compatible, you have driver_data
available, all this is not needed as it may be hard coded.

...

>  	if (!pdata) {

I would expect actually that legacy platform data support will be simply killed
by this patch(es). Do we have in-kernel users? If so, they can be easily
converted to use software nodes, otherwise we even don't need to care.

> -		dev_err(lm3533->dev, "no platform data\n");
> -		return -EINVAL;
> +		pdata = devm_kzalloc(lm3533->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		ret = device_property_read_u32(lm3533->dev,
> +					       "ti,boost-ovp",
> +					       &pdata->boost_ovp);
> +		if (ret)
> +			pdata->boost_ovp = LM3533_BOOST_OVP_16V;
> +
> +		ret = device_property_read_u32(lm3533->dev,
> +					       "ti,boost-freq",
> +					       &pdata->boost_freq);
> +		if (ret)
> +			pdata->boost_freq = LM3533_BOOST_FREQ_500KHZ;
> +
> +		lm3533_parse_nodes(lm3533, pdata);
> +
> +		lm3533->dev->platform_data = pdata;
>  	}

...

> +static const struct of_device_id lm3533_match_table[] = {
> +	{ .compatible = "ti,lm3533" },
> +	{ },

No comma in the terminator entry.

> +};

...

> +static void lm3533_parse_backlight(struct platform_device *pdev,

pdev is not actually used, just pass struct device *dev directly.
Same comment to other functions in this change. It will make code more
bus independent and reusable.

> +				   struct lm3533_bl_platform_data *pdata)
> +{
> +	struct device *dev = &pdev->dev;
> +	int val, ret;
> +
> +	/* 5000 - 29800 uA (800 uA step) */
> +	ret = device_property_read_u32(dev, "max-current-microamp", &val);
> +	if (ret)
> +		val = 5000;
> +	pdata->max_current = val;

> +	/* 0 - 255 */
> +	ret = device_property_read_u32(dev, "default-brightness", &val);
> +	if (ret)
> +		val = LM3533_BL_MAX_BRIGHTNESS;
> +	pdata->default_brightness = val;

Isn't handled by LEDS core?

> +	/* 0 - 0x3f */
> +	ret = device_property_read_u32(dev, "pwm", &val);
> +	if (ret)
> +		val = 0;
> +	pdata->pwm = val;
> +}

...

> +static int lm3533_pass_of_node(struct platform_device *pdev,
> +			       struct lm3533_bl_platform_data *pdata)
> +{

3rd dup?

> +}

...

>  	pdata = dev_get_platdata(&pdev->dev);
>  	if (!pdata) {
> -		dev_err(&pdev->dev, "no platform data\n");
> -		return -EINVAL;
> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		ret = lm3533_pass_of_node(pdev, pdata);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "failed to get backlight device node\n");
> +
> +		lm3533_parse_backlight(pdev, pdata);
>  	}

Ditto.

> -	bd = devm_backlight_device_register(&pdev->dev, pdata->name,
> -					pdev->dev.parent, bl, &lm3533_bl_ops,
> -					&props);
> +	bd = devm_backlight_device_register(&pdev->dev, dev_name(&pdev->dev),

I'm not sure the dev_name() is a good idea. We usually try to rely on the
predictable outcome, something like what "%pfw" prints against a certain fwnode.

> +					    pdev->dev.parent, bl,
> +					    &lm3533_bl_ops, &props);

...

Also I feel that this change may be split to a few separate logical changes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] mfd: lm3533: convert to use OF

[Svyatoslav Ryhel]

чт, 13 лют. 2025 р. о 16:57 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> пише:
>
> On Wed, Feb 12, 2025 at 09:58:42AM +0200, Svyatoslav Ryhel wrote:
> > Add ability to fill pdata from device tree. Common stuff is
> > filled from core driver and then pdata is filled per-device
> > since all cells are optional.
>
> ...
>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/mfd/core.h>
>
> > +#include <linux/of.h>
>
> Is it used? In any case, please no OF-centric APIs in a new (feature) code.
>
> >  #include <linux/platform_device.h>
> > +#include <linux/property.h>
> >  #include <linux/slab.h>
> >  #include <linux/uaccess.h>
>
> ...
>
> > +static int lm3533_pass_of_node(struct platform_device *pdev,
>
> pass_of_node sounds a bit awkward.
> Perhaps you thought about parse_fwnode ?
>
> > +                            struct lm3533_als_platform_data *pdata)
> > +{
> > +     struct device *parent_dev = pdev->dev.parent;
> > +     struct device *dev = &pdev->dev;
> > +     struct fwnode_handle *node;
> > +     const char *label;
> > +     int val, ret;
> > +
> > +     device_for_each_child_node(parent_dev, node) {
> > +             fwnode_property_read_string(node, "compatible", &label);
> > +
> > +             if (!strcmp(label, pdev->name)) {
>
> This is a bit strange. Why one need to compare platform device instance (!)
> name with compatible which is part of ABI. This looks really wrong approach.
> Needs a very good explanation on what's going on here.
>
> Besides that the label is usually filled by LEDS core, why do we need to handle
> it in a special way?
>
> > +                     ret = fwnode_property_read_u32(node, "reg", &val);
> > +                     if (ret) {
> > +                             dev_err(dev, "reg property is missing: ret %d\n", ret);
> > +                             return ret;
> > +                     }
> > +
> > +                     if (val == pdev->id) {
>
> > +                             dev->fwnode = node;
> > +                             dev->of_node = to_of_node(node);
>
> No direct access to fwnode in struct device, please use device_set_node().
>
> > +                     }
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
>
> ...
>
> >       pdata = dev_get_platdata(&pdev->dev);
> >       if (!pdata) {
> > -             dev_err(&pdev->dev, "no platform data\n");
> > -             return -EINVAL;
> > +             pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > +             if (!pdata)
> > +                     return -ENOMEM;
> > +
> > +             ret = lm3533_pass_of_node(pdev, pdata);
> > +             if (ret)
> > +                     return dev_err_probe(&pdev->dev, ret,
> > +                                          "failed to get als device node\n");
>
> With
>
>         struct device *dev = &pdev->dev;
>
> at the top of the function will help a lot in making the code neater and easier
> to read.
>
> > +             lm3533_parse_als(pdev, pdata);
> >       }
>
> ...
>
> >  #include <linux/leds.h>
> >  #include <linux/mfd/core.h>
> >  #include <linux/mutex.h>
>
> > +#include <linux/of.h>
>
> Cargo cult? "Proxy" header? Please follow IWYU principle.
>
> >  #include <linux/platform_device.h>
> > +#include <linux/property.h>
> >  #include <linux/slab.h>
>
> ...
>
> > +static void lm3533_parse_led(struct platform_device *pdev,
> > +                          struct lm3533_led_platform_data *pdata)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     int val, ret;
> > +
> > +     ret = device_property_read_string(dev, "default-trigger",
> > +                                       &pdata->default_trigger);
> > +     if (ret)
> > +             pdata->default_trigger = "none";
>
> Isn't this done already in LEDS core?
>
> > +     /* 5000 - 29800 uA (800 uA step) */
> > +     ret = device_property_read_u32(dev, "max-current-microamp", &val);
> > +     if (ret)
> > +             val = 5000;
> > +     pdata->max_current = val;
> > +
> > +     /* 0 - 0x3f */
> > +     ret = device_property_read_u32(dev, "pwm", &val);
> > +     if (ret)
> > +             val = 0;
> > +     pdata->pwm = val;
> > +}
>
> ...
>
> > +static int lm3533_pass_of_node(struct platform_device *pdev,
> > +                            struct lm3533_led_platform_data *pdata)
>
> I think I already saw exactly the same piece of code. Please make sure
> you have no duplications.
>
> > +}
>
> ...
>
> >       pdata = dev_get_platdata(&pdev->dev);
> >       if (!pdata) {
> > -             dev_err(&pdev->dev, "no platform data\n");
> > -             return -EINVAL;
> > +             pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > +             if (!pdata)
> > +                     return -ENOMEM;
> > +
> > +             ret = lm3533_pass_of_node(pdev, pdata);
> > +             if (ret)
> > +                     return dev_err_probe(&pdev->dev, ret,
> > +                                          "failed to get led device node\n");
> > +
> > +             lm3533_parse_led(pdev, pdata);
>
> Ditto.
>
> >       }
>
> ...
>
> > -     led->cdev.name = pdata->name;
> > +     led->cdev.name = dev_name(&pdev->dev);
>
> Are you sure it's a good idea?
>
> ...
>
> > -     if (!pdata->als)
> > +     if (!pdata->num_als)
> >               return 0;
> >
> > -     lm3533_als_devs[0].platform_data = pdata->als;
> > -     lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
> > +     if (pdata->num_als > ARRAY_SIZE(lm3533_als_devs))
> > +             pdata->num_als = ARRAY_SIZE(lm3533_als_devs);
>
> Looks like you want
>
>         pdata->num_als = clamp(pdata->num_als, 0, ARRAY_SIZE(lm3533_als_devs));
>         if (!pdata->num_als)
>                 return 0;
>
> instead of the above. You would need minmax.h for that.
>
> ...
>
> > +     if (pdata->leds) {
>
> This is strange. I would expect num_leds == 0 in this case
>
> > +             for (i = 0; i < pdata->num_leds; ++i) {
> > +                     lm3533_led_devs[i].platform_data = &pdata->leds[i];
> > +                     lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
> > +             }
> >       }
>
> ...
>
> > +static void lm3533_parse_nodes(struct lm3533 *lm3533,
> > +                            struct lm3533_platform_data *pdata)
> > +{
> > +     struct fwnode_handle *node;
> > +     const char *label;
> > +
> > +     device_for_each_child_node(lm3533->dev, node) {
> > +             fwnode_property_read_string(node, "compatible", &label);
> > +
> > +             if (!strcmp(label, lm3533_bl_devs[pdata->num_backlights].name))
> > +                     pdata->num_backlights++;
> > +
> > +             if (!strcmp(label, lm3533_led_devs[pdata->num_leds].name))
> > +                     pdata->num_leds++;
> > +
> > +             if (!strcmp(label, lm3533_als_devs[pdata->num_als].name))
> > +                     pdata->num_als++;
> > +     }
> > +}
>
> Oh, I don't like this approach. If you have compatible, you have driver_data
> available, all this is not needed as it may be hard coded.
>
> ...
>
> >       if (!pdata) {
>
> I would expect actually that legacy platform data support will be simply killed
> by this patch(es). Do we have in-kernel users? If so, they can be easily
> converted to use software nodes, otherwise we even don't need to care.
>
> > -             dev_err(lm3533->dev, "no platform data\n");
> > -             return -EINVAL;
> > +             pdata = devm_kzalloc(lm3533->dev, sizeof(*pdata), GFP_KERNEL);
> > +             if (!pdata)
> > +                     return -ENOMEM;
> > +
> > +             ret = device_property_read_u32(lm3533->dev,
> > +                                            "ti,boost-ovp",
> > +                                            &pdata->boost_ovp);
> > +             if (ret)
> > +                     pdata->boost_ovp = LM3533_BOOST_OVP_16V;
> > +
> > +             ret = device_property_read_u32(lm3533->dev,
> > +                                            "ti,boost-freq",
> > +                                            &pdata->boost_freq);
> > +             if (ret)
> > +                     pdata->boost_freq = LM3533_BOOST_FREQ_500KHZ;
> > +
> > +             lm3533_parse_nodes(lm3533, pdata);
> > +
> > +             lm3533->dev->platform_data = pdata;
> >       }
>
> ...
>
> > +static const struct of_device_id lm3533_match_table[] = {
> > +     { .compatible = "ti,lm3533" },
> > +     { },
>
> No comma in the terminator entry.
>
> > +};
>
> ...
>
> > +static void lm3533_parse_backlight(struct platform_device *pdev,
>
> pdev is not actually used, just pass struct device *dev directly.
> Same comment to other functions in this change. It will make code more
> bus independent and reusable.
>
> > +                                struct lm3533_bl_platform_data *pdata)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     int val, ret;
> > +
> > +     /* 5000 - 29800 uA (800 uA step) */
> > +     ret = device_property_read_u32(dev, "max-current-microamp", &val);
> > +     if (ret)
> > +             val = 5000;
> > +     pdata->max_current = val;
>
> > +     /* 0 - 255 */
> > +     ret = device_property_read_u32(dev, "default-brightness", &val);
> > +     if (ret)
> > +             val = LM3533_BL_MAX_BRIGHTNESS;
> > +     pdata->default_brightness = val;
>
> Isn't handled by LEDS core?
>
> > +     /* 0 - 0x3f */
> > +     ret = device_property_read_u32(dev, "pwm", &val);
> > +     if (ret)
> > +             val = 0;
> > +     pdata->pwm = val;
> > +}
>
> ...
>
> > +static int lm3533_pass_of_node(struct platform_device *pdev,
> > +                            struct lm3533_bl_platform_data *pdata)
> > +{
>
> 3rd dup?
>
> > +}
>
> ...
>
> >       pdata = dev_get_platdata(&pdev->dev);
> >       if (!pdata) {
> > -             dev_err(&pdev->dev, "no platform data\n");
> > -             return -EINVAL;
> > +             pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > +             if (!pdata)
> > +                     return -ENOMEM;
> > +
> > +             ret = lm3533_pass_of_node(pdev, pdata);
> > +             if (ret)
> > +                     return dev_err_probe(&pdev->dev, ret,
> > +                                          "failed to get backlight device node\n");
> > +
> > +             lm3533_parse_backlight(pdev, pdata);
> >       }
>
> Ditto.
>
> > -     bd = devm_backlight_device_register(&pdev->dev, pdata->name,
> > -                                     pdev->dev.parent, bl, &lm3533_bl_ops,
> > -                                     &props);
> > +     bd = devm_backlight_device_register(&pdev->dev, dev_name(&pdev->dev),
>
> I'm not sure the dev_name() is a good idea. We usually try to rely on the
> predictable outcome, something like what "%pfw" prints against a certain fwnode.
>
> > +                                         pdev->dev.parent, bl,
> > +                                         &lm3533_bl_ops, &props);
>
> ...
>
> Also I feel that this change may be split to a few separate logical changes.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Acknowledged, thank you.

Re: [PATCH v1 2/2] mfd: lm3533: convert to use OF

[Jonathan Cameron]

On Wed, 12 Feb 2025 09:58:42 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> Add ability to fill pdata from device tree. Common stuff is
> filled from core driver and then pdata is filled per-device
> since all cells are optional.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>

Focusing on just the IIO bit. (backlog of review is a bit high
for me this weekend!)

> ---
>  drivers/iio/light/lm3533-als.c      | 58 ++++++++++++++++++++-
>  drivers/leds/leds-lm3533.c          | 69 +++++++++++++++++++++++--
>  drivers/mfd/lm3533-core.c           | 79 ++++++++++++++++++++++++-----
>  drivers/video/backlight/lm3533_bl.c | 72 ++++++++++++++++++++++++--
>  include/linux/mfd/lm3533.h          |  1 +
>  5 files changed, 256 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> index 99f0b903018c..21fc5f215c80 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c
> @@ -16,7 +16,9 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/mfd/core.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  
> @@ -826,6 +828,50 @@ static const struct iio_info lm3533_als_info = {
>  	.read_raw	= &lm3533_als_read_raw,
>  };
>  
> +static void lm3533_parse_als(struct platform_device *pdev,
> +			     struct lm3533_als_platform_data *pdata)
> +{
> +	struct device *dev = &pdev->dev;
> +	int val, ret;
> +
> +	/* 1 - 127 (ignored in PWM-mode) */
> +	ret = device_property_read_u32(dev, "resistor-value", &val);
> +	if (ret)
> +		val = 1;
For defaults that apply on any error, the pattern

	val = 1;
	device_property_read_u32(dev, "resistor-value", &val);
is cleaner.

What are the units? If it's a resistor then they should be ohms
or similar and that should be part of the naming.

You should be taking whatever the value is in ohms and mapping
it to appropriate r_select in this function.

> +	pdata->r_select = val;
> +
> +	pdata->pwm_mode = device_property_read_bool(dev, "pwm-mode");
> +}
> +
> +static int lm3533_pass_of_node(struct platform_device *pdev,
> +			       struct lm3533_als_platform_data *pdata)
> +{
> +	struct device *parent_dev = pdev->dev.parent;
> +	struct device *dev = &pdev->dev;
> +	struct fwnode_handle *node;
> +	const char *label;
> +	int val, ret;
> +
> +	device_for_each_child_node(parent_dev, node) {

This devices should have direct access to the correct child node.
Otherwise something odd is going on...

> +		fwnode_property_read_string(node, "compatible", &label);
> +
> +		if (!strcmp(label, pdev->name)) {
> +			ret = fwnode_property_read_u32(node, "reg", &val);
> +			if (ret) {
> +				dev_err(dev, "reg property is missing: ret %d\n", ret);

use return dev_err_probe() for these.

> +				return ret;
> +			}
> +
> +			if (val == pdev->id) {
> +				dev->fwnode = node;
> +				dev->of_node = to_of_node(node);
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int lm3533_als_probe(struct platform_device *pdev)
>  {
>  	const struct lm3533_als_platform_data *pdata;
> @@ -840,8 +886,16 @@ static int lm3533_als_probe(struct platform_device *pdev)
>  
>  	pdata = dev_get_platdata(&pdev->dev);
>  	if (!pdata) {
> -		dev_err(&pdev->dev, "no platform data\n");
> -		return -EINVAL;
> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		ret = lm3533_pass_of_node(pdev, pdata);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "failed to get als device node\n");
> +
> +		lm3533_parse_als(pdev, pdata);
>  	}
>  
>  	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*als));

Re: [PATCH v1 0/2] mfd: lm3533: convert to use OF

[Andy Shevchenko]

On Wed, Feb 12, 2025 at 09:58:40AM +0200, Svyatoslav Ryhel wrote:
> Add schema and add support for lm3533 mfd to use device
> tree bindings.

Thank you! I'm going to review the series this week, I definitely have
the comments. Stay tuned.

-- 
With Best Regards,
Andy Shevchenko