* [PATCH v3 01/11] dt-bindings: leds: Document TI LM3533 LED controller
2026-06-01 15:18 [PATCH v3 00/11] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
@ 2026-06-01 15:18 ` Svyatoslav Ryhel
2026-06-02 13:49 ` Daniel Thompson
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
2026-06-01 15:18 ` [PATCH v3 02/11] mfd: lm3533: Remove driver specific regmap wrappers Svyatoslav Ryhel
` (10 subsequent siblings)
11 siblings, 2 replies; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-01 15:18 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Svyatoslav Ryhel
Cc: Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
Document 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>
Reviewed-by: Jonathan Cameron <jic23@kernel.org> #for light sensor
---
.../leds/backlight/ti,lm3533-backlight.yaml | 69 +++++++
.../bindings/leds/ti,lm3533-leds.yaml | 67 +++++++
.../devicetree/bindings/leds/ti,lm3533.yaml | 169 ++++++++++++++++++
3 files changed, 305 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
create mode 100644 Documentation/devicetree/bindings/leds/ti,lm3533-leds.yaml
create mode 100644 Documentation/devicetree/bindings/leds/ti,lm3533.yaml
diff --git a/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
new file mode 100644
index 000000000000..a25846e9b375
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/ti,lm3533-backlight.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI LM3533 high voltage series LED strings
+
+description:
+ This is part of the TI LM3533 MFD device. It represents two high voltage series
+ LED strings for display backlight controlled by the TI LM3533.
+
+maintainers:
+ - Svyatoslav Ryhel <clamor95@gmail.com>
+
+allOf:
+ - $ref: /schemas/leds/backlight/common.yaml#
+
+properties:
+ compatible:
+ const: ti,lm3533-backlight
+
+ reg:
+ description: Control bank selection (0 = bank A, 1 = bank B).
+ maximum: 1
+
+ led-max-microamp:
+ description: maximum current in uA with a 800 uA step.
+ minimum: 5000
+ maximum: 29800
+ default: 5000
+
+ led-sources:
+ description: |
+ HVLED strings associated with this control bank:
+ 0 - HVLED1
+ 1 - HVLED2
+ minItems: 1
+ maxItems: 2
+ items:
+ maximum: 1
+
+ ti,pwm-config-mask:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ Control Bank PWM Configuration Register mask that allows to configure
+ PWM input in Zones 0-4
+ BIT(0) - PWM Input is enabled
+ BIT(1) - PWM Input is enabled in Zone 0
+ BIT(2) - PWM Input is enabled in Zone 1
+ BIT(3) - PWM Input is enabled in Zone 2
+ BIT(4) - PWM Input is enabled in Zone 3
+ BIT(5) - PWM Input is enabled in Zone 4
+ default: 0
+
+ ti,linear-mapping-mode:
+ description:
+ Enable linear mapping mode. If disabled, then it will use exponential
+ mapping mode in which the ramp up/down appears to have a more uniform
+ transition to the human eye.
+ type: boolean
+
+required:
+ - compatible
+ - reg
+
+unevaluatedProperties: false
+
+# see ti,lm3533.yaml for an example
diff --git a/Documentation/devicetree/bindings/leds/ti,lm3533-leds.yaml b/Documentation/devicetree/bindings/leds/ti,lm3533-leds.yaml
new file mode 100644
index 000000000000..d8dbefef3323
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/ti,lm3533-leds.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/ti,lm3533-leds.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI LM3533 low voltage control banks for individual LEDs
+
+description:
+ This is part of the TI LM3533 MFD device. It represents four low voltage
+ control banks for individual LEDs provided by the TI LM3533.
+
+maintainers:
+ - Svyatoslav Ryhel <clamor95@gmail.com>
+
+allOf:
+ - $ref: /schemas/leds/common.yaml#
+
+properties:
+ compatible:
+ const: ti,lm3533-leds
+
+ reg:
+ description:
+ Control bank selection (2 = bank C, 3 = bank D, 4 = bank E, 5 = bank F).
+ minimum: 2
+ maximum: 5
+
+ led-max-microamp:
+ description: maximum current in uA with a 800 uA step.
+ minimum: 5000
+ maximum: 29800
+ default: 5000
+
+ led-sources:
+ description: |
+ LVLED associated with this control bank. May be more than 1 source per bank.
+ 0 - LVLED1
+ 1 - LVLED2
+ 2 - LVLED3
+ 3 - LVLED4
+ 4 - LVLED5
+ minItems: 1
+ maxItems: 5
+ items:
+ maximum: 4
+
+ ti,pwm-config-mask:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ Control Bank PWM Configuration Register mask that allows to configure
+ PWM input in Zones 0-4
+ BIT(0) - PWM Input is enabled
+ BIT(1) - PWM Input is enabled in Zone 0
+ BIT(2) - PWM Input is enabled in Zone 1
+ BIT(3) - PWM Input is enabled in Zone 2
+ BIT(4) - PWM Input is enabled in Zone 3
+ BIT(5) - PWM Input is enabled in Zone 4
+ default: 0
+
+required:
+ - compatible
+ - reg
+
+unevaluatedProperties: false
+
+# see ti,lm3533.yaml for an example
diff --git a/Documentation/devicetree/bindings/leds/ti,lm3533.yaml b/Documentation/devicetree/bindings/leds/ti,lm3533.yaml
new file mode 100644
index 000000000000..bdc05a9f3d66
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/ti,lm3533.yaml
@@ -0,0 +1,169 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/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 high voltage series LED strings for display backlight and four low
+ voltage control banks for individual LEDs. Additionally, LM3533 features an
+ interface for an external light sensor.
+
+ https://www.ti.com/product/LM3533
+
+maintainers:
+ - Svyatoslav Ryhel <clamor95@gmail.com>
+
+properties:
+ compatible:
+ const: ti,lm3533
+
+ reg:
+ maxItems: 1
+
+ enable-gpios:
+ description: GPIO connected to the HWEN pin.
+ maxItems: 1
+
+ vin-supply:
+ description: Supply connected to the IN line (2.7 V to 5.5 V).
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ ti,boost-ovp-microvolt:
+ description: boost OVP select (16V, 24V, 32V, 40V)
+ enum: [ 16000000, 24000000, 32000000, 40000000 ]
+ default: 16000000
+
+ ti,boost-freq-hz:
+ description: boost frequency select (500KHz or 1MHz)
+ enum: [ 500000, 1000000 ]
+ default: 500000
+
+ light-sensor:
+ type: object
+ additionalProperties: false
+
+ properties:
+ compatible:
+ const: ti,lm3533-als
+
+ interrupts:
+ maxItems: 1
+
+ ti,resistor-ohms:
+ description:
+ Internal configuration resistor value when light sensor is in
+ Analog Sensor mode and PWM mode is disabled. The expectation is
+ the input is a current from the external analog light sensor and
+ this is used to convert it to a voltage within the target range.
+ minimum: 1575
+ maximum: 200000
+
+ ti,pwm-mode:
+ type: boolean
+ description:
+ Switch for mode in which light sensor interface is running. If
+ this property is set then the light sensor interface is running
+ in PWM mode, internal resistor value is set to high-impedance (0)
+ and ti,resistor-ohms property is ignored.
+
+ required:
+ - compatible
+
+ oneOf:
+ - required:
+ - ti,resistor-ohms
+ - required:
+ - ti,pwm-mode
+
+patternProperties:
+ "^backlight@[01]$":
+ $ref: /schemas/leds/backlight/ti,lm3533-backlight.yaml#
+
+ "^led@[2-5]$":
+ $ref: /schemas/leds/ti,lm3533-leds.yaml#
+
+required:
+ - compatible
+ - reg
+ - '#address-cells'
+ - '#size-cells'
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led-controller@36 {
+ compatible = "ti,lm3533";
+ reg = <0x36>;
+
+ enable-gpios = <&gpio 110 GPIO_ACTIVE_HIGH>;
+ vin-supply = <&vdd_3v3_bat>;
+
+ ti,boost-ovp-microvolt = <24000000>;
+ ti,boost-freq-hz = <500000>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ backlight@0 {
+ compatible = "ti,lm3533-backlight";
+ reg = <0>;
+
+ default-brightness = <113>;
+
+ led-max-microamp = <23400>;
+ led-sources = <0 1>;
+ };
+
+ led@2 {
+ compatible = "ti,lm3533-leds";
+ reg = <2>;
+
+ led-max-microamp = <23400>;
+ led-sources = <0 1>;
+ };
+
+ led@4 {
+ compatible = "ti,lm3533-leds";
+ reg = <4>;
+
+ led-max-microamp = <23400>;
+ led-sources = <2>;
+ };
+
+ led@5 {
+ compatible = "ti,lm3533-leds";
+ reg = <5>;
+
+ led-max-microamp = <23400>;
+ led-sources = <3 4>;
+ };
+
+ light-sensor {
+ compatible = "ti,lm3533-als";
+
+ interrupt-parent = <&gpio>;
+ interrupts = <80 IRQ_TYPE_LEVEL_LOW>;
+
+ ti,pwm-mode;
+ };
+ };
+ };
+...
--
2.51.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 01/11] dt-bindings: leds: Document TI LM3533 LED controller
2026-06-01 15:18 ` [PATCH v3 01/11] dt-bindings: leds: Document TI LM3533 LED controller Svyatoslav Ryhel
@ 2026-06-02 13:49 ` Daniel Thompson
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
1 sibling, 0 replies; 47+ messages in thread
From: Daniel Thompson @ 2026-06-02 13:49 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
On Mon, Jun 01, 2026 at 06:18:21PM +0300, Svyatoslav Ryhel wrote:
> Document 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>
> Reviewed-by: Jonathan Cameron <jic23@kernel.org> #for light sensor
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org> #for backlight
Daniel.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Claude review: dt-bindings: leds: Document TI LM3533 LED controller
2026-06-01 15:18 ` [PATCH v3 01/11] dt-bindings: leds: Document TI LM3533 LED controller Svyatoslav Ryhel
2026-06-02 13:49 ` Daniel Thompson
@ 2026-06-04 3:59 ` Claude Code Review Bot
1 sibling, 0 replies; 47+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
The three binding schemas (ti,lm3533.yaml, ti,lm3533-backlight.yaml, ti,lm3533-leds.yaml) are well-structured. The `oneOf` constraint for ALS requiring either `ti,resistor-ohms` or `ti,pwm-mode` is correct. The example is comprehensive and covers backlight, LEDs, and ALS nodes.
Minor nit: The `ti,pwm-config-mask` property is duplicated identically in both the backlight and LED schemas. Consider defining it once in a shared schema fragment, though this is cosmetic.
**No blocking issues.**
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 02/11] mfd: lm3533: Remove driver specific regmap wrappers
2026-06-01 15:18 [PATCH v3 00/11] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
2026-06-01 15:18 ` [PATCH v3 01/11] dt-bindings: leds: Document TI LM3533 LED controller Svyatoslav Ryhel
@ 2026-06-01 15:18 ` Svyatoslav Ryhel
2026-06-02 8:05 ` Andy Shevchenko
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
2026-06-01 15:18 ` [PATCH v3 03/11] iio: light: lm3533-als: Remove redundant pdata helpers Svyatoslav Ryhel
` (9 subsequent siblings)
11 siblings, 2 replies; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-01 15:18 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Svyatoslav Ryhel
Cc: Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
Remove driver-specific regmap wrappers in favor of using regmap helpers
directly. The wrappers are mostly equivalent to the standard helpers, with
two exceptions: regmap_read requires an unsigned int pointer, and
regmap_update_bits has the mask and value arguments swapped. These
differences were accounted for and adjusted accordingly.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/iio/light/lm3533-als.c | 57 ++++++++++++----------
drivers/leds/leds-lm3533.c | 34 +++++++-------
drivers/mfd/lm3533-core.c | 73 +++++------------------------
drivers/mfd/lm3533-ctrlbank.c | 22 +++++----
drivers/video/backlight/lm3533_bl.c | 21 +++++----
include/linux/mfd/lm3533.h | 8 +---
6 files changed, 86 insertions(+), 129 deletions(-)
diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
index 99f0b903018c..fb61904f110f 100644
--- a/drivers/iio/light/lm3533-als.c
+++ b/drivers/iio/light/lm3533-als.c
@@ -17,6 +17,7 @@
#include <linux/mutex.h>
#include <linux/mfd/core.h>
#include <linux/platform_device.h>
+#include <linux/regmap.h>
#include <linux/slab.h>
#include <linux/uaccess.h>
@@ -64,7 +65,7 @@ static int lm3533_als_get_adc(struct iio_dev *indio_dev, bool average,
{
struct lm3533_als *als = iio_priv(indio_dev);
u8 reg;
- u8 val;
+ u32 val;
int ret;
if (average)
@@ -72,7 +73,7 @@ static int lm3533_als_get_adc(struct iio_dev *indio_dev, bool average,
else
reg = LM3533_REG_ALS_READ_ADC_RAW;
- ret = lm3533_read(als->lm3533, reg, &val);
+ ret = regmap_read(als->lm3533->regmap, reg, &val);
if (ret) {
dev_err(&indio_dev->dev, "failed to read adc\n");
return ret;
@@ -86,10 +87,10 @@ static int lm3533_als_get_adc(struct iio_dev *indio_dev, bool average,
static int _lm3533_als_get_zone(struct iio_dev *indio_dev, u8 *zone)
{
struct lm3533_als *als = iio_priv(indio_dev);
- u8 val;
+ u32 val;
int ret;
- ret = lm3533_read(als->lm3533, LM3533_REG_ALS_ZONE_INFO, &val);
+ ret = regmap_read(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO, &val);
if (ret) {
dev_err(&indio_dev->dev, "failed to read zone\n");
return ret;
@@ -127,7 +128,7 @@ static inline u8 lm3533_als_get_target_reg(unsigned channel, unsigned zone)
}
static int lm3533_als_get_target(struct iio_dev *indio_dev, unsigned channel,
- unsigned zone, u8 *val)
+ unsigned zone, u32 *val)
{
struct lm3533_als *als = iio_priv(indio_dev);
u8 reg;
@@ -140,7 +141,7 @@ static int lm3533_als_get_target(struct iio_dev *indio_dev, unsigned channel,
return -EINVAL;
reg = lm3533_als_get_target_reg(channel, zone);
- ret = lm3533_read(als->lm3533, reg, val);
+ ret = regmap_read(als->lm3533->regmap, reg, val);
if (ret)
dev_err(&indio_dev->dev, "failed to get target current\n");
@@ -161,7 +162,7 @@ static int lm3533_als_set_target(struct iio_dev *indio_dev, unsigned channel,
return -EINVAL;
reg = lm3533_als_get_target_reg(channel, zone);
- ret = lm3533_write(als->lm3533, reg, val);
+ ret = regmap_write(als->lm3533->regmap, reg, val);
if (ret)
dev_err(&indio_dev->dev, "failed to set target current\n");
@@ -172,7 +173,7 @@ static int lm3533_als_get_current(struct iio_dev *indio_dev, unsigned channel,
int *val)
{
u8 zone;
- u8 target;
+ u32 target;
int ret;
ret = lm3533_als_get_zone(indio_dev, &zone);
@@ -280,7 +281,8 @@ static int lm3533_als_set_int_mode(struct iio_dev *indio_dev, int enable)
else
val = 0;
- ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, val, mask);
+ ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
+ val, mask);
if (ret) {
dev_err(&indio_dev->dev, "failed to set int mode %d\n",
enable);
@@ -294,10 +296,10 @@ static int lm3533_als_get_int_mode(struct iio_dev *indio_dev, int *enable)
{
struct lm3533_als *als = iio_priv(indio_dev);
u8 mask = LM3533_ALS_INT_ENABLE_MASK;
- u8 val;
+ u32 val;
int ret;
- ret = lm3533_read(als->lm3533, LM3533_REG_ALS_ZONE_INFO, &val);
+ ret = regmap_read(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO, &val);
if (ret) {
dev_err(&indio_dev->dev, "failed to get int mode\n");
return ret;
@@ -316,7 +318,7 @@ static inline u8 lm3533_als_get_threshold_reg(unsigned nr, bool raising)
}
static int lm3533_als_get_threshold(struct iio_dev *indio_dev, unsigned nr,
- bool raising, u8 *val)
+ bool raising, u32 *val)
{
struct lm3533_als *als = iio_priv(indio_dev);
u8 reg;
@@ -326,7 +328,7 @@ static int lm3533_als_get_threshold(struct iio_dev *indio_dev, unsigned nr,
return -EINVAL;
reg = lm3533_als_get_threshold_reg(nr, raising);
- ret = lm3533_read(als->lm3533, reg, val);
+ ret = regmap_read(als->lm3533->regmap, reg, val);
if (ret)
dev_err(&indio_dev->dev, "failed to get threshold\n");
@@ -337,7 +339,7 @@ static int lm3533_als_set_threshold(struct iio_dev *indio_dev, unsigned nr,
bool raising, u8 val)
{
struct lm3533_als *als = iio_priv(indio_dev);
- u8 val2;
+ u32 val2;
u8 reg, reg2;
int ret;
@@ -348,7 +350,7 @@ static int lm3533_als_set_threshold(struct iio_dev *indio_dev, unsigned nr,
reg2 = lm3533_als_get_threshold_reg(nr, !raising);
mutex_lock(&als->thresh_mutex);
- ret = lm3533_read(als->lm3533, reg2, &val2);
+ ret = regmap_read(als->lm3533->regmap, reg2, &val2);
if (ret) {
dev_err(&indio_dev->dev, "failed to get threshold\n");
goto out;
@@ -363,7 +365,7 @@ static int lm3533_als_set_threshold(struct iio_dev *indio_dev, unsigned nr,
goto out;
}
- ret = lm3533_write(als->lm3533, reg, val);
+ ret = regmap_write(als->lm3533->regmap, reg, val);
if (ret) {
dev_err(&indio_dev->dev, "failed to set threshold\n");
goto out;
@@ -375,11 +377,11 @@ static int lm3533_als_set_threshold(struct iio_dev *indio_dev, unsigned nr,
}
static int lm3533_als_get_hysteresis(struct iio_dev *indio_dev, unsigned nr,
- u8 *val)
+ u32 *val)
{
struct lm3533_als *als = iio_priv(indio_dev);
- u8 falling;
- u8 raising;
+ u32 falling;
+ u32 raising;
int ret;
if (nr > LM3533_ALS_THRESH_MAX)
@@ -503,7 +505,7 @@ static ssize_t show_als_attr(struct device *dev,
{
struct iio_dev *indio_dev = dev_to_iio_dev(dev);
struct lm3533_als_attribute *als_attr = to_lm3533_als_attr(attr);
- u8 val;
+ u32 val;
int ret;
switch (als_attr->type) {
@@ -725,7 +727,8 @@ static int lm3533_als_set_input_mode(struct lm3533_als *als, bool pwm_mode)
else
val = 0; /* analog input */
- ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, val, mask);
+ ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
+ mask, val);
if (ret) {
dev_err(&als->pdev->dev, "failed to set input mode %d\n",
pwm_mode);
@@ -744,7 +747,8 @@ static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
return -EINVAL;
}
- ret = lm3533_write(als->lm3533, LM3533_REG_ALS_RESISTOR_SELECT, val);
+ ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
+ val);
if (ret) {
dev_err(&als->pdev->dev, "failed to set resistor\n");
return ret;
@@ -778,7 +782,8 @@ static int lm3533_als_setup_irq(struct lm3533_als *als, void *dev)
int ret;
/* Make sure interrupts are disabled. */
- ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, 0, mask);
+ ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
+ mask, 0);
if (ret) {
dev_err(&als->pdev->dev, "failed to disable interrupts\n");
return ret;
@@ -801,7 +806,8 @@ static int lm3533_als_enable(struct lm3533_als *als)
u8 mask = LM3533_ALS_ENABLE_MASK;
int ret;
- ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, mask, mask);
+ ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
+ mask, mask);
if (ret)
dev_err(&als->pdev->dev, "failed to enable ALS\n");
@@ -813,7 +819,8 @@ static int lm3533_als_disable(struct lm3533_als *als)
u8 mask = LM3533_ALS_ENABLE_MASK;
int ret;
- ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, 0, mask);
+ ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
+ mask, 0);
if (ret)
dev_err(&als->pdev->dev, "failed to disable ALS\n");
diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
index 45795f2a1042..51ff61aee75c 100644
--- a/drivers/leds/leds-lm3533.c
+++ b/drivers/leds/leds-lm3533.c
@@ -12,6 +12,7 @@
#include <linux/mfd/core.h>
#include <linux/mutex.h>
#include <linux/platform_device.h>
+#include <linux/regmap.h>
#include <linux/slab.h>
#include <linux/mfd/lm3533.h>
@@ -101,7 +102,8 @@ static int lm3533_led_pattern_enable(struct lm3533_led *led, int enable)
else
val = 0;
- ret = lm3533_update(led->lm3533, LM3533_REG_PATTERN_ENABLE, val, mask);
+ ret = regmap_update_bits(led->lm3533->regmap,
+ LM3533_REG_PATTERN_ENABLE, mask, val);
if (ret) {
dev_err(led->cdev.dev, "failed to enable pattern %d (%d)\n",
pattern, enable);
@@ -131,7 +133,7 @@ static int lm3533_led_set(struct led_classdev *cdev,
static enum led_brightness lm3533_led_get(struct led_classdev *cdev)
{
struct lm3533_led *led = to_lm3533_led(cdev);
- u8 val;
+ u32 val;
int ret;
ret = lm3533_ctrlbank_get_brightness(&led->cb, &val);
@@ -259,7 +261,7 @@ static u8 lm3533_led_delay_set(struct lm3533_led *led, u8 base,
dev_dbg(led->cdev.dev, "%s - %lu: %u (0x%02x)\n", __func__,
*delay, t, val);
reg = lm3533_led_get_pattern_reg(led, base);
- ret = lm3533_write(led->lm3533, reg, val);
+ ret = regmap_write(led->lm3533->regmap, reg, val);
if (ret)
dev_err(led->cdev.dev, "failed to set delay (%02x)\n", reg);
@@ -337,10 +339,10 @@ static ssize_t show_risefalltime(struct device *dev,
struct lm3533_led *led = to_lm3533_led(led_cdev);
ssize_t ret;
u8 reg;
- u8 val;
+ u32 val;
reg = lm3533_led_get_pattern_reg(led, base);
- ret = lm3533_read(led->lm3533, reg, &val);
+ ret = regmap_read(led->lm3533->regmap, reg, &val);
if (ret)
return ret;
@@ -375,7 +377,7 @@ static ssize_t store_risefalltime(struct device *dev,
return -EINVAL;
reg = lm3533_led_get_pattern_reg(led, base);
- ret = lm3533_write(led->lm3533, reg, val);
+ ret = regmap_write(led->lm3533->regmap, reg, val);
if (ret)
return ret;
@@ -405,11 +407,11 @@ static ssize_t show_als_channel(struct device *dev,
struct lm3533_led *led = to_lm3533_led(led_cdev);
unsigned channel;
u8 reg;
- u8 val;
+ u32 val;
int ret;
reg = lm3533_led_get_lv_reg(led, LM3533_REG_CTRLBANK_BCONF_BASE);
- ret = lm3533_read(led->lm3533, reg, &val);
+ ret = regmap_read(led->lm3533->regmap, reg, &val);
if (ret)
return ret;
@@ -441,7 +443,7 @@ static ssize_t store_als_channel(struct device *dev,
mask = LM3533_REG_CTRLBANK_BCONF_ALS_CHANNEL_MASK;
val = channel - 1;
- ret = lm3533_update(led->lm3533, reg, val, mask);
+ ret = regmap_update_bits(led->lm3533->regmap, reg, mask, val);
if (ret)
return ret;
@@ -455,11 +457,11 @@ static ssize_t show_als_en(struct device *dev,
struct lm3533_led *led = to_lm3533_led(led_cdev);
bool enable;
u8 reg;
- u8 val;
+ u32 val;
int ret;
reg = lm3533_led_get_lv_reg(led, LM3533_REG_CTRLBANK_BCONF_BASE);
- ret = lm3533_read(led->lm3533, reg, &val);
+ ret = regmap_read(led->lm3533->regmap, reg, &val);
if (ret)
return ret;
@@ -491,7 +493,7 @@ static ssize_t store_als_en(struct device *dev,
else
val = 0;
- ret = lm3533_update(led->lm3533, reg, val, mask);
+ ret = regmap_update_bits(led->lm3533->regmap, reg, mask, val);
if (ret)
return ret;
@@ -504,12 +506,12 @@ static ssize_t show_linear(struct device *dev,
struct led_classdev *led_cdev = dev_get_drvdata(dev);
struct lm3533_led *led = to_lm3533_led(led_cdev);
u8 reg;
- u8 val;
+ u32 val;
int linear;
int ret;
reg = lm3533_led_get_lv_reg(led, LM3533_REG_CTRLBANK_BCONF_BASE);
- ret = lm3533_read(led->lm3533, reg, &val);
+ ret = regmap_read(led->lm3533->regmap, reg, &val);
if (ret)
return ret;
@@ -544,7 +546,7 @@ static ssize_t store_linear(struct device *dev,
else
val = 0;
- ret = lm3533_update(led->lm3533, reg, val, mask);
+ ret = regmap_update_bits(led->lm3533->regmap, reg, mask, val);
if (ret)
return ret;
@@ -557,7 +559,7 @@ static ssize_t show_pwm(struct device *dev,
{
struct led_classdev *led_cdev = dev_get_drvdata(dev);
struct lm3533_led *led = to_lm3533_led(led_cdev);
- u8 val;
+ u32 val;
int ret;
ret = lm3533_ctrlbank_get_pwm(&led->cb, &val);
diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
index 0a2409d00b2e..f9bdc0a1d9be 100644
--- a/drivers/mfd/lm3533-core.c
+++ b/drivers/mfd/lm3533-core.c
@@ -80,66 +80,14 @@ static struct mfd_cell lm3533_led_devs[] = {
},
};
-int lm3533_read(struct lm3533 *lm3533, u8 reg, u8 *val)
-{
- int tmp;
- int ret;
-
- ret = regmap_read(lm3533->regmap, reg, &tmp);
- if (ret < 0) {
- dev_err(lm3533->dev, "failed to read register %02x: %d\n",
- reg, ret);
- return ret;
- }
-
- *val = tmp;
-
- dev_dbg(lm3533->dev, "read [%02x]: %02x\n", reg, *val);
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(lm3533_read);
-
-int lm3533_write(struct lm3533 *lm3533, u8 reg, u8 val)
-{
- int ret;
-
- dev_dbg(lm3533->dev, "write [%02x]: %02x\n", reg, val);
-
- ret = regmap_write(lm3533->regmap, reg, val);
- if (ret < 0) {
- dev_err(lm3533->dev, "failed to write register %02x: %d\n",
- reg, ret);
- }
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(lm3533_write);
-
-int lm3533_update(struct lm3533 *lm3533, u8 reg, u8 val, u8 mask)
-{
- int ret;
-
- dev_dbg(lm3533->dev, "update [%02x]: %02x/%02x\n", reg, val, mask);
-
- ret = regmap_update_bits(lm3533->regmap, reg, mask, val);
- if (ret < 0) {
- dev_err(lm3533->dev, "failed to update register %02x: %d\n",
- reg, ret);
- }
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(lm3533_update);
-
static int lm3533_set_boost_freq(struct lm3533 *lm3533,
enum lm3533_boost_freq freq)
{
int ret;
- ret = lm3533_update(lm3533, LM3533_REG_BOOST_PWM,
- freq << LM3533_BOOST_FREQ_SHIFT,
- LM3533_BOOST_FREQ_MASK);
+ ret = regmap_update_bits(lm3533->regmap, LM3533_REG_BOOST_PWM,
+ LM3533_BOOST_FREQ_MASK,
+ freq << LM3533_BOOST_FREQ_SHIFT);
if (ret)
dev_err(lm3533->dev, "failed to set boost frequency\n");
@@ -152,9 +100,9 @@ static int lm3533_set_boost_ovp(struct lm3533 *lm3533,
{
int ret;
- ret = lm3533_update(lm3533, LM3533_REG_BOOST_PWM,
- ovp << LM3533_BOOST_OVP_SHIFT,
- LM3533_BOOST_OVP_MASK);
+ ret = regmap_update_bits(lm3533->regmap, LM3533_REG_BOOST_PWM,
+ LM3533_BOOST_OVP_MASK,
+ ovp << LM3533_BOOST_OVP_SHIFT);
if (ret)
dev_err(lm3533->dev, "failed to set boost ovp\n");
@@ -181,7 +129,8 @@ static int lm3533_set_hvled_config(struct lm3533 *lm3533, u8 hvled, u8 bl)
mask = LM3533_BL_ID_MASK << shift;
val = bl << shift;
- ret = lm3533_update(lm3533, LM3533_REG_OUTPUT_CONF1, val, mask);
+ ret = regmap_update_bits(lm3533->regmap, LM3533_REG_OUTPUT_CONF1,
+ mask, val);
if (ret)
dev_err(lm3533->dev, "failed to set hvled config\n");
@@ -216,7 +165,7 @@ static int lm3533_set_lvled_config(struct lm3533 *lm3533, u8 lvled, u8 led)
mask = LM3533_LED_ID_MASK << shift;
val = led << shift;
- ret = lm3533_update(lm3533, reg, val, mask);
+ ret = regmap_update_bits(lm3533->regmap, reg, mask, val);
if (ret)
dev_err(lm3533->dev, "failed to set lvled config\n");
@@ -258,7 +207,7 @@ static ssize_t show_output(struct device *dev,
struct lm3533_device_attribute *lattr = to_lm3533_dev_attr(attr);
int id = lattr->u.output.id;
u8 reg;
- u8 val;
+ u32 val;
u8 mask;
int shift;
int ret;
@@ -278,7 +227,7 @@ static ssize_t show_output(struct device *dev,
mask = LM3533_LED_ID_MASK << shift;
}
- ret = lm3533_read(lm3533, reg, &val);
+ ret = regmap_read(lm3533->regmap, reg, &val);
if (ret)
return ret;
diff --git a/drivers/mfd/lm3533-ctrlbank.c b/drivers/mfd/lm3533-ctrlbank.c
index 2537dfade51c..37bae279f41e 100644
--- a/drivers/mfd/lm3533-ctrlbank.c
+++ b/drivers/mfd/lm3533-ctrlbank.c
@@ -9,6 +9,7 @@
#include <linux/device.h>
#include <linux/module.h>
+#include <linux/regmap.h>
#include <linux/mfd/lm3533.h>
@@ -38,8 +39,8 @@ int lm3533_ctrlbank_enable(struct lm3533_ctrlbank *cb)
dev_dbg(cb->dev, "%s - %d\n", __func__, cb->id);
mask = 1 << cb->id;
- ret = lm3533_update(cb->lm3533, LM3533_REG_CTRLBANK_ENABLE,
- mask, mask);
+ ret = regmap_update_bits(cb->lm3533->regmap, LM3533_REG_CTRLBANK_ENABLE,
+ mask, mask);
if (ret)
dev_err(cb->dev, "failed to enable ctrlbank %d\n", cb->id);
@@ -55,7 +56,8 @@ int lm3533_ctrlbank_disable(struct lm3533_ctrlbank *cb)
dev_dbg(cb->dev, "%s - %d\n", __func__, cb->id);
mask = 1 << cb->id;
- ret = lm3533_update(cb->lm3533, LM3533_REG_CTRLBANK_ENABLE, 0, mask);
+ ret = regmap_update_bits(cb->lm3533->regmap, LM3533_REG_CTRLBANK_ENABLE,
+ mask, 0);
if (ret)
dev_err(cb->dev, "failed to disable ctrlbank %d\n", cb->id);
@@ -80,7 +82,7 @@ int lm3533_ctrlbank_set_max_current(struct lm3533_ctrlbank *cb, u16 imax)
val = (imax - LM3533_MAX_CURRENT_MIN) / LM3533_MAX_CURRENT_STEP;
reg = lm3533_ctrlbank_get_reg(cb, LM3533_REG_MAX_CURRENT_BASE);
- ret = lm3533_write(cb->lm3533, reg, val);
+ ret = regmap_write(cb->lm3533->regmap, reg, val);
if (ret)
dev_err(cb->dev, "failed to set max current\n");
@@ -94,7 +96,7 @@ int lm3533_ctrlbank_set_brightness(struct lm3533_ctrlbank *cb, u8 val)
int ret;
reg = lm3533_ctrlbank_get_reg(cb, LM3533_REG_BRIGHTNESS_BASE);
- ret = lm3533_write(cb->lm3533, reg, val);
+ ret = regmap_write(cb->lm3533->regmap, reg, val);
if (ret)
dev_err(cb->dev, "failed to set brightness\n");
@@ -102,13 +104,13 @@ int lm3533_ctrlbank_set_brightness(struct lm3533_ctrlbank *cb, u8 val)
}
EXPORT_SYMBOL_GPL(lm3533_ctrlbank_set_brightness);
-int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u8 *val)
+int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u32 *val)
{
u8 reg;
int ret;
reg = lm3533_ctrlbank_get_reg(cb, LM3533_REG_BRIGHTNESS_BASE);
- ret = lm3533_read(cb->lm3533, reg, val);
+ ret = regmap_read(cb->lm3533->regmap, reg, val);
if (ret)
dev_err(cb->dev, "failed to get brightness\n");
@@ -135,7 +137,7 @@ int lm3533_ctrlbank_set_pwm(struct lm3533_ctrlbank *cb, u8 val)
return -EINVAL;
reg = lm3533_ctrlbank_get_reg(cb, LM3533_REG_PWM_BASE);
- ret = lm3533_write(cb->lm3533, reg, val);
+ ret = regmap_write(cb->lm3533->regmap, reg, val);
if (ret)
dev_err(cb->dev, "failed to set PWM mask\n");
@@ -143,13 +145,13 @@ int lm3533_ctrlbank_set_pwm(struct lm3533_ctrlbank *cb, u8 val)
}
EXPORT_SYMBOL_GPL(lm3533_ctrlbank_set_pwm);
-int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u8 *val)
+int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u32 *val)
{
u8 reg;
int ret;
reg = lm3533_ctrlbank_get_reg(cb, LM3533_REG_PWM_BASE);
- ret = lm3533_read(cb->lm3533, reg, val);
+ ret = regmap_read(cb->lm3533->regmap, reg, val);
if (ret)
dev_err(cb->dev, "failed to get PWM mask\n");
diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
index babfd3ceec86..5c8e69087860 100644
--- a/drivers/video/backlight/lm3533_bl.c
+++ b/drivers/video/backlight/lm3533_bl.c
@@ -11,6 +11,7 @@
#include <linux/init.h>
#include <linux/platform_device.h>
#include <linux/backlight.h>
+#include <linux/regmap.h>
#include <linux/slab.h>
#include <linux/mfd/lm3533.h>
@@ -45,7 +46,7 @@ static int lm3533_bl_update_status(struct backlight_device *bd)
static int lm3533_bl_get_brightness(struct backlight_device *bd)
{
struct lm3533_bl *bl = bl_get_data(bd);
- u8 val;
+ u32 val;
int ret;
ret = lm3533_ctrlbank_get_brightness(&bl->cb, &val);
@@ -82,12 +83,12 @@ static ssize_t show_als_en(struct device *dev,
{
struct lm3533_bl *bl = dev_get_drvdata(dev);
int ctrlbank = lm3533_bl_get_ctrlbank_id(bl);
- u8 val;
+ u32 val;
u8 mask;
bool enable;
int ret;
- ret = lm3533_read(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF, &val);
+ ret = regmap_read(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF, &val);
if (ret)
return ret;
@@ -118,8 +119,8 @@ static ssize_t store_als_en(struct device *dev,
else
val = 0;
- ret = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF, val,
- mask);
+ ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
+ mask, val);
if (ret)
return ret;
@@ -130,12 +131,12 @@ static ssize_t show_linear(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct lm3533_bl *bl = dev_get_drvdata(dev);
- u8 val;
+ u32 val;
u8 mask;
int linear;
int ret;
- ret = lm3533_read(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF, &val);
+ ret = regmap_read(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF, &val);
if (ret)
return ret;
@@ -169,8 +170,8 @@ static ssize_t store_linear(struct device *dev,
else
val = 0;
- ret = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF, val,
- mask);
+ ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
+ mask, val);
if (ret)
return ret;
@@ -182,7 +183,7 @@ static ssize_t show_pwm(struct device *dev,
char *buf)
{
struct lm3533_bl *bl = dev_get_drvdata(dev);
- u8 val;
+ u32 val;
int ret;
ret = lm3533_ctrlbank_get_pwm(&bl->cb, &val);
diff --git a/include/linux/mfd/lm3533.h b/include/linux/mfd/lm3533.h
index 69059a7a2ce5..db0a5e5c9e39 100644
--- a/include/linux/mfd/lm3533.h
+++ b/include/linux/mfd/lm3533.h
@@ -86,14 +86,10 @@ extern int lm3533_ctrlbank_enable(struct lm3533_ctrlbank *cb);
extern int lm3533_ctrlbank_disable(struct lm3533_ctrlbank *cb);
extern int lm3533_ctrlbank_set_brightness(struct lm3533_ctrlbank *cb, u8 val);
-extern int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u8 *val);
+extern int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u32 *val);
extern int lm3533_ctrlbank_set_max_current(struct lm3533_ctrlbank *cb,
u16 imax);
extern int lm3533_ctrlbank_set_pwm(struct lm3533_ctrlbank *cb, u8 val);
-extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u8 *val);
-
-extern int lm3533_read(struct lm3533 *lm3533, u8 reg, u8 *val);
-extern int lm3533_write(struct lm3533 *lm3533, u8 reg, u8 val);
-extern int lm3533_update(struct lm3533 *lm3533, u8 reg, u8 val, u8 mask);
+extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u32 *val);
#endif /* __LINUX_MFD_LM3533_H */
--
2.51.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 02/11] mfd: lm3533: Remove driver specific regmap wrappers
2026-06-01 15:18 ` [PATCH v3 02/11] mfd: lm3533: Remove driver specific regmap wrappers Svyatoslav Ryhel
@ 2026-06-02 8:05 ` Andy Shevchenko
2026-06-02 10:18 ` Svyatoslav Ryhel
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
1 sibling, 1 reply; 47+ messages in thread
From: Andy Shevchenko @ 2026-06-02 8:05 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
On Mon, Jun 01, 2026 at 06:18:22PM +0300, Svyatoslav Ryhel wrote:
> Remove driver-specific regmap wrappers in favor of using regmap helpers
> directly. The wrappers are mostly equivalent to the standard helpers, with
> two exceptions: regmap_read requires an unsigned int pointer, and
> regmap_update_bits has the mask and value arguments swapped. These
> differences were accounted for and adjusted accordingly.
We refer to functions as func(), exempli gratia, regmap_read().
...
> static int lm3533_als_get_current(struct iio_dev *indio_dev, unsigned channel,
> int *val)
> {
> u8 zone;
> - u8 target;
> + u32 target;
> int ret;
While at it, move towards reversed xmas tree order
u32 target;
u8 zone;
int ret;
...
> - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, val, mask);
> + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
> + val, mask);
It's better to replace this to use _set_bits()/_clear_bits() or even move from
the above conditional (not in this context) to _assign_bits().
...
> else
> val = 0; /* analog input */
>
> - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, val, mask);
> + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> + mask, val);
Ditto.
> if (ret) {
> dev_err(&als->pdev->dev, "failed to set input mode %d\n",
> pwm_mode);
...
> /* Make sure interrupts are disabled. */
> - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, 0, mask);
> + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
> + mask, 0);
_clear_bits().
> if (ret) {
> dev_err(&als->pdev->dev, "failed to disable interrupts\n");
> return ret;
...
> u8 mask = LM3533_ALS_ENABLE_MASK;
> int ret;
>
> - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, mask, mask);
> + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> + mask, mask);
_set_bits()
> if (ret)
> dev_err(&als->pdev->dev, "failed to enable ALS\n");
>
...
> u8 mask = LM3533_ALS_ENABLE_MASK;
> int ret;
>
> - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, 0, mask);
> + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> + mask, 0);
_clear_bits()
> if (ret)
> dev_err(&als->pdev->dev, "failed to disable ALS\n");
...
> else
> val = 0;
>
> - ret = lm3533_update(led->lm3533, LM3533_REG_PATTERN_ENABLE, val, mask);
> + ret = regmap_update_bits(led->lm3533->regmap,
> + LM3533_REG_PATTERN_ENABLE, mask, val);
_assign_bits() and so on...
> if (ret) {
> dev_err(led->cdev.dev, "failed to enable pattern %d (%d)\n",
> pattern, enable);
...
> extern int lm3533_ctrlbank_set_brightness(struct lm3533_ctrlbank *cb, u8 val);
> -extern int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u8 *val);
> +extern int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u32 *val);
We don't need to keep 'extern' for ages.
> extern int lm3533_ctrlbank_set_max_current(struct lm3533_ctrlbank *cb,
> u16 imax);
> extern int lm3533_ctrlbank_set_pwm(struct lm3533_ctrlbank *cb, u8 val);
> -extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u8 *val);
> -
> -extern int lm3533_read(struct lm3533 *lm3533, u8 reg, u8 *val);
> -extern int lm3533_write(struct lm3533 *lm3533, u8 reg, u8 val);
> -extern int lm3533_update(struct lm3533 *lm3533, u8 reg, u8 val, u8 mask);
> +extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u32 *val);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 02/11] mfd: lm3533: Remove driver specific regmap wrappers
2026-06-02 8:05 ` Andy Shevchenko
@ 2026-06-02 10:18 ` Svyatoslav Ryhel
0 siblings, 0 replies; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-02 10:18 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
вт, 2 черв. 2026 р. о 11:05 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
>
> On Mon, Jun 01, 2026 at 06:18:22PM +0300, Svyatoslav Ryhel wrote:
> > Remove driver-specific regmap wrappers in favor of using regmap helpers
> > directly. The wrappers are mostly equivalent to the standard helpers, with
> > two exceptions: regmap_read requires an unsigned int pointer, and
> > regmap_update_bits has the mask and value arguments swapped. These
> > differences were accounted for and adjusted accordingly.
>
> We refer to functions as func(), exempli gratia, regmap_read().
>
Noted.
> ...
>
> > static int lm3533_als_get_current(struct iio_dev *indio_dev, unsigned channel,
> > int *val)
> > {
> > u8 zone;
> > - u8 target;
> > + u32 target;
> > int ret;
>
> While at it, move towards reversed xmas tree order
>
> u32 target;
> u8 zone;
> int ret;
>
Noted.
>
> ...
>
> > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, val, mask);
> > + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
> > + val, mask);
>
> It's better to replace this to use _set_bits()/_clear_bits() or even move from
> the above conditional (not in this context) to _assign_bits().
>
I will take a look.
> ...
>
> > else
> > val = 0; /* analog input */
> >
> > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, val, mask);
> > + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> > + mask, val);
>
> Ditto.
>
> > if (ret) {
> > dev_err(&als->pdev->dev, "failed to set input mode %d\n",
> > pwm_mode);
>
> ...
>
> > /* Make sure interrupts are disabled. */
> > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, 0, mask);
> > + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
> > + mask, 0);
>
> _clear_bits().
>
> > if (ret) {
> > dev_err(&als->pdev->dev, "failed to disable interrupts\n");
> > return ret;
>
> ...
>
> > u8 mask = LM3533_ALS_ENABLE_MASK;
> > int ret;
> >
> > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, mask, mask);
> > + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> > + mask, mask);
>
> _set_bits()
>
> > if (ret)
> > dev_err(&als->pdev->dev, "failed to enable ALS\n");
> >
>
> ...
>
> > u8 mask = LM3533_ALS_ENABLE_MASK;
> > int ret;
> >
> > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, 0, mask);
> > + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> > + mask, 0);
>
> _clear_bits()
>
> > if (ret)
> > dev_err(&als->pdev->dev, "failed to disable ALS\n");
>
> ...
>
> > else
> > val = 0;
> >
> > - ret = lm3533_update(led->lm3533, LM3533_REG_PATTERN_ENABLE, val, mask);
> > + ret = regmap_update_bits(led->lm3533->regmap,
> > + LM3533_REG_PATTERN_ENABLE, mask, val);
>
> _assign_bits() and so on...
>
> > if (ret) {
> > dev_err(led->cdev.dev, "failed to enable pattern %d (%d)\n",
> > pattern, enable);
>
> ...
>
> > extern int lm3533_ctrlbank_set_brightness(struct lm3533_ctrlbank *cb, u8 val);
> > -extern int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u8 *val);
> > +extern int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u32 *val);
>
> We don't need to keep 'extern' for ages.
>
I will no inflate this patchset further
> > extern int lm3533_ctrlbank_set_max_current(struct lm3533_ctrlbank *cb,
> > u16 imax);
> > extern int lm3533_ctrlbank_set_pwm(struct lm3533_ctrlbank *cb, u8 val);
> > -extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u8 *val);
> > -
> > -extern int lm3533_read(struct lm3533 *lm3533, u8 reg, u8 *val);
> > -extern int lm3533_write(struct lm3533 *lm3533, u8 reg, u8 val);
> > -extern int lm3533_update(struct lm3533 *lm3533, u8 reg, u8 val, u8 mask);
> > +extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u32 *val);
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Claude review: mfd: lm3533: Remove driver specific regmap wrappers
2026-06-01 15:18 ` [PATCH v3 02/11] mfd: lm3533: Remove driver specific regmap wrappers Svyatoslav Ryhel
2026-06-02 8:05 ` Andy Shevchenko
@ 2026-06-04 3:59 ` Claude Code Review Bot
1 sibling, 0 replies; 47+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Bug: Swapped mask/val in lm3533_als_set_int_mode.**
The old `lm3533_update(lm3533, reg, val, mask)` wrapper internally calls `regmap_update_bits(regmap, reg, mask, val)` — the wrapper takes `(val, mask)` while regmap takes `(mask, val)`. Most call sites are converted correctly, but one is not:
```c
- ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, val, mask);
+ ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
+ val, mask);
```
This should be `mask, val` (not `val, mask`). When `enable=false`, `val=0` and `mask=LM3533_ALS_INT_ENABLE_MASK`. The direct call becomes `regmap_update_bits(regmap, reg, 0, mask)` — a mask of 0 means no bits are touched, so disabling ALS interrupts becomes a no-op. The correct conversion is:
```c
ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
mask, val);
```
All other `lm3533_update` → `regmap_update_bits` conversions in this patch are correct.
The `u8` → `u32` type changes for regmap_read output pointers are correct and necessary since `regmap_read` requires `unsigned int *`.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 03/11] iio: light: lm3533-als: Remove redundant pdata helpers
2026-06-01 15:18 [PATCH v3 00/11] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
2026-06-01 15:18 ` [PATCH v3 01/11] dt-bindings: leds: Document TI LM3533 LED controller Svyatoslav Ryhel
2026-06-01 15:18 ` [PATCH v3 02/11] mfd: lm3533: Remove driver specific regmap wrappers Svyatoslav Ryhel
@ 2026-06-01 15:18 ` Svyatoslav Ryhel
2026-06-02 8:07 ` Andy Shevchenko
` (2 more replies)
2026-06-01 15:18 ` [PATCH v3 04/11] mfd: lm3533-core: " Svyatoslav Ryhel
` (8 subsequent siblings)
11 siblings, 3 replies; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-01 15:18 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Svyatoslav Ryhel
Cc: Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
The lm3533_als_set_input_mode and lm3533_als_set_resistor functions are
used only in lm3533_als_setup. Incorporate their code into
lm3533_als_setup directly to simplify driver readability.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/iio/light/lm3533-als.c | 61 +++++++++-------------------------
1 file changed, 16 insertions(+), 45 deletions(-)
diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
index fb61904f110f..52136ca1abc9 100644
--- a/drivers/iio/light/lm3533-als.c
+++ b/drivers/iio/light/lm3533-als.c
@@ -716,61 +716,32 @@ static const struct attribute_group lm3533_als_attribute_group = {
.attrs = lm3533_als_attributes
};
-static int lm3533_als_set_input_mode(struct lm3533_als *als, bool pwm_mode)
-{
- u8 mask = LM3533_ALS_INPUT_MODE_MASK;
- u8 val;
- int ret;
-
- if (pwm_mode)
- val = mask; /* pwm input */
- else
- val = 0; /* analog input */
-
- ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
- mask, val);
- if (ret) {
- dev_err(&als->pdev->dev, "failed to set input mode %d\n",
- pwm_mode);
- return ret;
- }
-
- return 0;
-}
-
-static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
-{
- int ret;
-
- if (val < LM3533_ALS_RESISTOR_MIN || val > LM3533_ALS_RESISTOR_MAX) {
- dev_err(&als->pdev->dev, "invalid resistor value\n");
- return -EINVAL;
- }
-
- ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
- val);
- if (ret) {
- dev_err(&als->pdev->dev, "failed to set resistor\n");
- return ret;
- }
-
- return 0;
-}
-
static int lm3533_als_setup(struct lm3533_als *als,
const struct lm3533_als_platform_data *pdata)
{
+ struct device *dev = &als->pdev->dev;
int ret;
- ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
+ ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
+ LM3533_ALS_INPUT_MODE_MASK,
+ pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
if (ret)
- return ret;
+ return dev_err_probe(dev, ret, "failed to set input mode %d\n",
+ pdata->pwm_mode);
+
/* ALS input is always high impedance in PWM-mode. */
if (!pdata->pwm_mode) {
- ret = lm3533_als_set_resistor(als, pdata->r_select);
+ if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
+ pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
+ dev_err(&als->pdev->dev, "invalid resistor value\n");
+ return -EINVAL;
+ }
+
+ ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
+ pdata->r_select);
if (ret)
- return ret;
+ return dev_err_probe(dev, ret, "failed to set resistor\n");
}
return 0;
--
2.51.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 03/11] iio: light: lm3533-als: Remove redundant pdata helpers
2026-06-01 15:18 ` [PATCH v3 03/11] iio: light: lm3533-als: Remove redundant pdata helpers Svyatoslav Ryhel
@ 2026-06-02 8:07 ` Andy Shevchenko
2026-06-02 13:42 ` Jonathan Cameron
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
2 siblings, 0 replies; 47+ messages in thread
From: Andy Shevchenko @ 2026-06-02 8:07 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
On Mon, Jun 01, 2026 at 06:18:23PM +0300, Svyatoslav Ryhel wrote:
> The lm3533_als_set_input_mode and lm3533_als_set_resistor functions are
> used only in lm3533_als_setup. Incorporate their code into
> lm3533_als_setup directly to simplify driver readability.
Same comments as per previous patch.
I stop my review here as it seems may be more of this.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 03/11] iio: light: lm3533-als: Remove redundant pdata helpers
2026-06-01 15:18 ` [PATCH v3 03/11] iio: light: lm3533-als: Remove redundant pdata helpers Svyatoslav Ryhel
2026-06-02 8:07 ` Andy Shevchenko
@ 2026-06-02 13:42 ` Jonathan Cameron
2026-06-02 13:45 ` Svyatoslav Ryhel
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
2 siblings, 1 reply; 47+ messages in thread
From: Jonathan Cameron @ 2026-06-02 13:42 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, David Lechner, Nuno Sá,
Andy Shevchenko, Helge Deller, Johan Hovold, dri-devel,
linux-leds, devicetree, linux-kernel, linux-iio, linux-fbdev
On Mon, 1 Jun 2026 18:18:23 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> The lm3533_als_set_input_mode and lm3533_als_set_resistor functions are
> used only in lm3533_als_setup. Incorporate their code into
> lm3533_als_setup directly to simplify driver readability.
Minor stuff inline.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> drivers/iio/light/lm3533-als.c | 61 +++++++++-------------------------
> 1 file changed, 16 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> index fb61904f110f..52136ca1abc9 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c
> static int lm3533_als_setup(struct lm3533_als *als,
> const struct lm3533_als_platform_data *pdata)
> {
> + struct device *dev = &als->pdev->dev;
> int ret;
>
> - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
Maybe a local struct regmap pointer given dereferenced in a couple of places.
> + LM3533_ALS_INPUT_MODE_MASK,
> + pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
Andy raised this in previous patch but in the interests of being specific
regmap_assign_bits() is going to be cleaner here.
> if (ret)
> - return ret;
> + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> + pdata->pwm_mode);
> +
>
> /* ALS input is always high impedance in PWM-mode. */
> if (!pdata->pwm_mode) {
> - ret = lm3533_als_set_resistor(als, pdata->r_select);
> + if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> + pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> + dev_err(&als->pdev->dev, "invalid resistor value\n");
> + return -EINVAL;
> + }
> +
> + ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> + pdata->r_select);
> if (ret)
> - return ret;
> + return dev_err_probe(dev, ret, "failed to set resistor\n");
> }
>
> return 0;
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 03/11] iio: light: lm3533-als: Remove redundant pdata helpers
2026-06-02 13:42 ` Jonathan Cameron
@ 2026-06-02 13:45 ` Svyatoslav Ryhel
0 siblings, 0 replies; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-02 13:45 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, David Lechner, Nuno Sá,
Andy Shevchenko, Helge Deller, Johan Hovold, dri-devel,
linux-leds, devicetree, linux-kernel, linux-iio, linux-fbdev
вт, 2 черв. 2026 р. о 16:42 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Mon, 1 Jun 2026 18:18:23 +0300
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > The lm3533_als_set_input_mode and lm3533_als_set_resistor functions are
> > used only in lm3533_als_setup. Incorporate their code into
> > lm3533_als_setup directly to simplify driver readability.
> Minor stuff inline.
>
>
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> > drivers/iio/light/lm3533-als.c | 61 +++++++++-------------------------
> > 1 file changed, 16 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > index fb61904f110f..52136ca1abc9 100644
> > --- a/drivers/iio/light/lm3533-als.c
> > +++ b/drivers/iio/light/lm3533-als.c
>
> > static int lm3533_als_setup(struct lm3533_als *als,
> > const struct lm3533_als_platform_data *pdata)
> > {
> > + struct device *dev = &als->pdev->dev;
> > int ret;
> >
> > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > + ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
>
> Maybe a local struct regmap pointer given dereferenced in a couple of places.
>
sure, why not
> > + LM3533_ALS_INPUT_MODE_MASK,
> > + pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
>
> Andy raised this in previous patch but in the interests of being specific
> regmap_assign_bits() is going to be cleaner here.
>
I am currently adjusting accordingly.
> > if (ret)
> > - return ret;
> > + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > + pdata->pwm_mode);
> > +
> >
> > /* ALS input is always high impedance in PWM-mode. */
> > if (!pdata->pwm_mode) {
> > - ret = lm3533_als_set_resistor(als, pdata->r_select);
> > + if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> > + pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> > + dev_err(&als->pdev->dev, "invalid resistor value\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> > + pdata->r_select);
> > if (ret)
> > - return ret;
> > + return dev_err_probe(dev, ret, "failed to set resistor\n");
> > }
> >
> > return 0;
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Claude review: iio: light: lm3533-als: Remove redundant pdata helpers
2026-06-01 15:18 ` [PATCH v3 03/11] iio: light: lm3533-als: Remove redundant pdata helpers Svyatoslav Ryhel
2026-06-02 8:07 ` Andy Shevchenko
2026-06-02 13:42 ` Jonathan Cameron
@ 2026-06-04 3:59 ` Claude Code Review Bot
2 siblings, 0 replies; 47+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Straightforward inlining of `lm3533_als_set_input_mode` and `lm3533_als_set_resistor` into `lm3533_als_setup`. The `dev_err_probe` conversion is a nice touch.
Minor: there's a stale extra blank line after the first `dev_err_probe` call:
```c
if (ret)
return dev_err_probe(dev, ret, "failed to set input mode %d\n",
pdata->pwm_mode);
<-- extra blank line
/* ALS input is always high impedance in PWM-mode. */
```
**No blocking issues.**
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 04/11] mfd: lm3533-core: Remove redundant pdata helpers
2026-06-01 15:18 [PATCH v3 00/11] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
` (2 preceding siblings ...)
2026-06-01 15:18 ` [PATCH v3 03/11] iio: light: lm3533-als: Remove redundant pdata helpers Svyatoslav Ryhel
@ 2026-06-01 15:18 ` Svyatoslav Ryhel
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
2026-06-01 15:18 ` [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings Svyatoslav Ryhel
` (7 subsequent siblings)
11 siblings, 1 reply; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-01 15:18 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Svyatoslav Ryhel
Cc: Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
The lm3533_set_boost_freq and lm3533_set_boost_ovp functions are used only
in lm3533_device_setup, which in turn is only called by
lm3533_device_init. Incorporate their code directly into
lm3533_device_init to simplify driver readability.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/mfd/lm3533-core.c | 57 ++++++++++-----------------------------
1 file changed, 14 insertions(+), 43 deletions(-)
diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
index f9bdc0a1d9be..45e7f7481aa0 100644
--- a/drivers/mfd/lm3533-core.c
+++ b/drivers/mfd/lm3533-core.c
@@ -80,35 +80,6 @@ static struct mfd_cell lm3533_led_devs[] = {
},
};
-static int lm3533_set_boost_freq(struct lm3533 *lm3533,
- enum lm3533_boost_freq freq)
-{
- int ret;
-
- ret = regmap_update_bits(lm3533->regmap, LM3533_REG_BOOST_PWM,
- LM3533_BOOST_FREQ_MASK,
- freq << LM3533_BOOST_FREQ_SHIFT);
- if (ret)
- dev_err(lm3533->dev, "failed to set boost frequency\n");
-
- return ret;
-}
-
-
-static int lm3533_set_boost_ovp(struct lm3533 *lm3533,
- enum lm3533_boost_ovp ovp)
-{
- int ret;
-
- ret = regmap_update_bits(lm3533->regmap, LM3533_REG_BOOST_PWM,
- LM3533_BOOST_OVP_MASK,
- ovp << LM3533_BOOST_OVP_SHIFT);
- if (ret)
- dev_err(lm3533->dev, "failed to set boost ovp\n");
-
- return ret;
-}
-
/*
* HVLED output config -- output hvled controlled by backlight bl
*/
@@ -406,18 +377,6 @@ static int lm3533_device_led_init(struct lm3533 *lm3533)
return 0;
}
-static int lm3533_device_setup(struct lm3533 *lm3533,
- struct lm3533_platform_data *pdata)
-{
- int ret;
-
- ret = lm3533_set_boost_freq(lm3533, pdata->boost_freq);
- if (ret)
- return ret;
-
- return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
-}
-
static int lm3533_device_init(struct lm3533 *lm3533)
{
struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
@@ -437,9 +396,21 @@ static int lm3533_device_init(struct lm3533 *lm3533)
lm3533_enable(lm3533);
- ret = lm3533_device_setup(lm3533, pdata);
- if (ret)
+ ret = regmap_update_bits(lm3533->regmap, LM3533_REG_BOOST_PWM,
+ LM3533_BOOST_FREQ_MASK,
+ pdata->boost_freq << LM3533_BOOST_FREQ_SHIFT);
+ if (ret) {
+ dev_err(lm3533->dev, "failed to set boost frequency\n");
goto err_disable;
+ }
+
+ ret = regmap_update_bits(lm3533->regmap, LM3533_REG_BOOST_PWM,
+ LM3533_BOOST_OVP_MASK,
+ pdata->boost_ovp << LM3533_BOOST_OVP_SHIFT);
+ if (ret) {
+ dev_err(lm3533->dev, "failed to set boost ovp\n");
+ goto err_disable;
+ }
lm3533_device_als_init(lm3533);
lm3533_device_bl_init(lm3533);
--
2.51.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
2026-06-01 15:18 [PATCH v3 00/11] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
` (3 preceding siblings ...)
2026-06-01 15:18 ` [PATCH v3 04/11] mfd: lm3533-core: " Svyatoslav Ryhel
@ 2026-06-01 15:18 ` Svyatoslav Ryhel
2026-06-02 8:23 ` Andy Shevchenko
` (3 more replies)
2026-06-01 15:18 ` [PATCH v3 06/11] mfd: lm3533: Add support for VIN power supply Svyatoslav Ryhel
` (6 subsequent siblings)
11 siblings, 4 replies; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-01 15:18 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Svyatoslav Ryhel
Cc: Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
Since there are no users of this driver via platform data, remove the
platform data support and switch to using Device Tree bindings.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/iio/light/lm3533-als.c | 69 +++++----
drivers/leds/leds-lm3533.c | 51 ++++--
drivers/mfd/lm3533-core.c | 232 ++++++++++++----------------
drivers/mfd/lm3533-ctrlbank.c | 5 -
drivers/video/backlight/lm3533_bl.c | 58 ++++---
include/linux/mfd/lm3533.h | 52 +------
6 files changed, 222 insertions(+), 245 deletions(-)
diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
index 52136ca1abc9..55b35467a722 100644
--- a/drivers/iio/light/lm3533-als.c
+++ b/drivers/iio/light/lm3533-als.c
@@ -16,16 +16,19 @@
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/mfd/core.h>
+#include <linux/mod_devicetable.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/regmap.h>
#include <linux/slab.h>
#include <linux/uaccess.h>
+#include <linux/units.h>
#include <linux/mfd/lm3533.h>
-#define LM3533_ALS_RESISTOR_MIN 1
-#define LM3533_ALS_RESISTOR_MAX 127
+#define LM3533_ALS_RESISTOR_MIN 1575
+#define LM3533_ALS_RESISTOR_MAX 200000
#define LM3533_ALS_CHANNEL_CURRENT_MAX 2
#define LM3533_ALS_THRESH_MAX 3
#define LM3533_ALS_ZONE_MAX 4
@@ -57,6 +60,9 @@ struct lm3533_als {
atomic_t zone;
struct mutex thresh_mutex;
+
+ bool pwm_mode;
+ u32 r_select;
};
@@ -411,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev,
int enable;
int ret;
- if (als->irq) {
+ if (als->irq > 0) {
ret = lm3533_als_get_int_mode(indio_dev, &enable);
if (ret)
return ret;
@@ -716,30 +722,34 @@ static const struct attribute_group lm3533_als_attribute_group = {
.attrs = lm3533_als_attributes
};
-static int lm3533_als_setup(struct lm3533_als *als,
- const struct lm3533_als_platform_data *pdata)
+static int lm3533_als_setup(struct lm3533_als *als)
{
struct device *dev = &als->pdev->dev;
int ret;
+ als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
+
ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
LM3533_ALS_INPUT_MODE_MASK,
- pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
+ als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
if (ret)
return dev_err_probe(dev, ret, "failed to set input mode %d\n",
- pdata->pwm_mode);
-
+ als->pwm_mode);
/* ALS input is always high impedance in PWM-mode. */
- if (!pdata->pwm_mode) {
- if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
- pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
- dev_err(&als->pdev->dev, "invalid resistor value\n");
- return -EINVAL;
- }
+ if (!als->pwm_mode) {
+ ret = device_property_read_u32(dev, "ti,resistor-value-ohms",
+ &als->r_select);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to ger resistor value\n");
+
+ als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
+ LM3533_ALS_RESISTOR_MAX);
+ als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
- pdata->r_select);
+ als->r_select);
if (ret)
return dev_err_probe(dev, ret, "failed to set resistor\n");
}
@@ -806,7 +816,6 @@ static const struct iio_info lm3533_als_info = {
static int lm3533_als_probe(struct platform_device *pdev)
{
- const struct lm3533_als_platform_data *pdata;
struct lm3533 *lm3533;
struct lm3533_als *als;
struct iio_dev *indio_dev;
@@ -816,12 +825,6 @@ static int lm3533_als_probe(struct platform_device *pdev)
if (!lm3533)
return -EINVAL;
- pdata = dev_get_platdata(&pdev->dev);
- if (!pdata) {
- dev_err(&pdev->dev, "no platform data\n");
- return -EINVAL;
- }
-
indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*als));
if (!indio_dev)
return -ENOMEM;
@@ -830,25 +833,28 @@ static int lm3533_als_probe(struct platform_device *pdev)
indio_dev->channels = lm3533_als_channels;
indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
indio_dev->name = dev_name(&pdev->dev);
- iio_device_set_parent(indio_dev, pdev->dev.parent);
indio_dev->modes = INDIO_DIRECT_MODE;
als = iio_priv(indio_dev);
als->lm3533 = lm3533;
als->pdev = pdev;
- als->irq = lm3533->irq;
+ als->irq = platform_get_irq_optional(pdev, 0);
+
+ if (als->irq == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
atomic_set(&als->zone, 0);
mutex_init(&als->thresh_mutex);
platform_set_drvdata(pdev, indio_dev);
- if (als->irq) {
+ if (als->irq > 0) {
ret = lm3533_als_setup_irq(als, indio_dev);
if (ret)
return ret;
}
- ret = lm3533_als_setup(als, pdata);
+ ret = lm3533_als_setup(als);
if (ret)
goto err_free_irq;
@@ -867,7 +873,7 @@ static int lm3533_als_probe(struct platform_device *pdev)
err_disable:
lm3533_als_disable(als);
err_free_irq:
- if (als->irq)
+ if (als->irq > 0)
free_irq(als->irq, indio_dev);
return ret;
@@ -881,13 +887,20 @@ static void lm3533_als_remove(struct platform_device *pdev)
lm3533_als_set_int_mode(indio_dev, false);
iio_device_unregister(indio_dev);
lm3533_als_disable(als);
- if (als->irq)
+ if (als->irq > 0)
free_irq(als->irq, indio_dev);
}
+static const struct of_device_id lm3533_als_match_table[] = {
+ { .compatible = "ti,lm3533-als" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, lm3533_als_match_table);
+
static struct platform_driver lm3533_als_driver = {
.driver = {
.name = "lm3533-als",
+ .of_match_table = lm3533_als_match_table,
},
.probe = lm3533_als_probe,
.remove = lm3533_als_remove,
diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
index 51ff61aee75c..f2502daf450d 100644
--- a/drivers/leds/leds-lm3533.c
+++ b/drivers/leds/leds-lm3533.c
@@ -10,8 +10,10 @@
#include <linux/module.h>
#include <linux/leds.h>
#include <linux/mfd/core.h>
+#include <linux/mod_devicetable.h>
#include <linux/mutex.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/regmap.h>
#include <linux/slab.h>
@@ -49,6 +51,9 @@ struct lm3533_led {
struct mutex mutex;
unsigned long flags;
+
+ u32 max_current;
+ u32 pwm;
};
@@ -634,22 +639,20 @@ static const struct attribute_group *lm3533_led_attribute_groups[] = {
NULL
};
-static int lm3533_led_setup(struct lm3533_led *led,
- struct lm3533_led_platform_data *pdata)
+static int lm3533_led_setup(struct lm3533_led *led)
{
int ret;
- ret = lm3533_ctrlbank_set_max_current(&led->cb, pdata->max_current);
+ ret = lm3533_ctrlbank_set_max_current(&led->cb, led->max_current);
if (ret)
return ret;
- return lm3533_ctrlbank_set_pwm(&led->cb, pdata->pwm);
+ return lm3533_ctrlbank_set_pwm(&led->cb, led->pwm);
}
static int lm3533_led_probe(struct platform_device *pdev)
{
struct lm3533 *lm3533;
- struct lm3533_led_platform_data *pdata;
struct lm3533_led *led;
int ret;
@@ -659,12 +662,6 @@ static int lm3533_led_probe(struct platform_device *pdev)
if (!lm3533)
return -EINVAL;
- pdata = dev_get_platdata(&pdev->dev);
- if (!pdata) {
- dev_err(&pdev->dev, "no platform data\n");
- return -EINVAL;
- }
-
if (pdev->id < 0 || pdev->id >= LM3533_LVCTRLBANK_COUNT) {
dev_err(&pdev->dev, "illegal LED id %d\n", pdev->id);
return -EINVAL;
@@ -675,8 +672,6 @@ static int lm3533_led_probe(struct platform_device *pdev)
return -ENOMEM;
led->lm3533 = lm3533;
- led->cdev.name = pdata->name;
- led->cdev.default_trigger = pdata->default_trigger;
led->cdev.brightness_set_blocking = lm3533_led_set;
led->cdev.brightness_get = lm3533_led_get;
led->cdev.blink_set = lm3533_led_blink_set;
@@ -684,6 +679,15 @@ static int lm3533_led_probe(struct platform_device *pdev)
led->cdev.groups = lm3533_led_attribute_groups;
led->id = pdev->id;
+ led->cdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
+ pdev->name, led->id);
+ if (!led->cdev.name)
+ return -ENOMEM;
+
+ led->cdev.default_trigger = "none";
+ device_property_read_string(&pdev->dev, "linux,default-trigger",
+ &led->cdev.default_trigger);
+
mutex_init(&led->mutex);
/* The class framework makes a callback to get brightness during
@@ -696,15 +700,23 @@ static int lm3533_led_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, led);
- ret = led_classdev_register(pdev->dev.parent, &led->cdev);
+ ret = led_classdev_register(&pdev->dev, &led->cdev);
if (ret) {
- dev_err(&pdev->dev, "failed to register LED %d\n", pdev->id);
+ dev_err(&pdev->dev, "failed to register LED %d\n", led->id);
return ret;
}
led->cb.dev = led->cdev.dev;
- ret = lm3533_led_setup(led, pdata);
+ device_property_read_u32(&pdev->dev, "led-max-microamp",
+ &led->max_current);
+ led->max_current = clamp(led->max_current, LM3533_MAX_CURRENT_MIN,
+ LM3533_MAX_CURRENT_MAX);
+
+ led->pwm = 0;
+ device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);
+
+ ret = lm3533_led_setup(led);
if (ret)
goto err_deregister;
@@ -741,9 +753,16 @@ static void lm3533_led_shutdown(struct platform_device *pdev)
lm3533_led_set(&led->cdev, LED_OFF); /* disable blink */
}
+static const struct of_device_id lm3533_led_match_table[] = {
+ { .compatible = "ti,lm3533-leds" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, lm3533_led_match_table);
+
static struct platform_driver lm3533_led_driver = {
.driver = {
.name = "lm3533-leds",
+ .of_match_table = lm3533_led_match_table,
},
.probe = lm3533_led_probe,
.remove = lm3533_led_remove,
diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
index 45e7f7481aa0..d57c2cdf11c5 100644
--- a/drivers/mfd/lm3533-core.c
+++ b/drivers/mfd/lm3533-core.c
@@ -14,19 +14,26 @@
#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/mfd/core.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
#include <linux/regmap.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
#include <linux/uaccess.h>
+#include <linux/units.h>
#include <linux/mfd/lm3533.h>
#define LM3533_BOOST_OVP_MASK 0x06
#define LM3533_BOOST_OVP_SHIFT 1
+#define LM3533_BOOST_OVP_MIN 16000000
+#define LM3533_BOOST_OVP_MAX 40000000
#define LM3533_BOOST_FREQ_MASK 0x01
#define LM3533_BOOST_FREQ_SHIFT 0
+#define LM3533_BOOST_FREQ_MIN 500000
+#define LM3533_BOOST_FREQ_MAX 1000000
#define LM3533_BL_ID_MASK 1
#define LM3533_LED_ID_MASK 3
@@ -35,6 +42,7 @@
#define LM3533_HVLED_ID_MAX 2
#define LM3533_LVLED_ID_MAX 5
+#define LM3533_CELLS_MAX 7
#define LM3533_REG_OUTPUT_CONF1 0x10
#define LM3533_REG_OUTPUT_CONF2 0x11
@@ -42,44 +50,6 @@
#define LM3533_REG_MAX 0xb2
-
-static struct mfd_cell lm3533_als_devs[] = {
- {
- .name = "lm3533-als",
- .id = -1,
- },
-};
-
-static struct mfd_cell lm3533_bl_devs[] = {
- {
- .name = "lm3533-backlight",
- .id = 0,
- },
- {
- .name = "lm3533-backlight",
- .id = 1,
- },
-};
-
-static struct mfd_cell lm3533_led_devs[] = {
- {
- .name = "lm3533-leds",
- .id = 0,
- },
- {
- .name = "lm3533-leds",
- .id = 1,
- },
- {
- .name = "lm3533-leds",
- .id = 2,
- },
- {
- .name = "lm3533-leds",
- .id = 3,
- },
-};
-
/*
* HVLED output config -- output hvled controlled by backlight bl
*/
@@ -296,129 +266,101 @@ static struct attribute_group lm3533_attribute_group = {
.attrs = lm3533_attributes
};
-static int lm3533_device_als_init(struct lm3533 *lm3533)
-{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
- int ret;
-
- if (!pdata->als)
- return 0;
-
- 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);
- if (ret) {
- dev_err(lm3533->dev, "failed to add ALS device\n");
- return ret;
- }
-
- lm3533->have_als = 1;
-
- return 0;
-}
-
-static int lm3533_device_bl_init(struct lm3533 *lm3533)
+static int lm3533_device_init(struct lm3533 *lm3533)
{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
- int i;
+ struct device *dev = lm3533->dev;
+ struct mfd_cell *lm3533_devices;
+ u32 count = 0, reg, nchilds;
int ret;
- if (!pdata->backlights || pdata->num_backlights == 0)
- 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]);
+ nchilds = device_get_child_node_count(dev);
+ if (!nchilds || nchilds > LM3533_CELLS_MAX) {
+ dev_err(dev, "num of child nodes is not supported\n");
+ return -ENODEV;
}
- ret = mfd_add_devices(lm3533->dev, 0, lm3533_bl_devs,
- pdata->num_backlights, NULL, 0, NULL);
- if (ret) {
- dev_err(lm3533->dev, "failed to add backlight devices\n");
- return ret;
- }
-
- lm3533->have_backlights = 1;
-
- return 0;
-}
-
-static int lm3533_device_led_init(struct lm3533 *lm3533)
-{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
- int i;
- int ret;
-
- if (!pdata->leds || pdata->num_leds == 0)
- return 0;
+ lm3533_devices = devm_kcalloc(dev, nchilds, sizeof(*lm3533_devices),
+ GFP_KERNEL);
+ if (!lm3533_devices)
+ return -ENOMEM;
- if (pdata->num_leds > ARRAY_SIZE(lm3533_led_devs))
- pdata->num_leds = ARRAY_SIZE(lm3533_led_devs);
+ device_for_each_child_node_scoped(lm3533->dev, child) {
+ if (!fwnode_device_is_available(child))
+ continue;
- 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 (count >= nchilds)
+ break;
- ret = mfd_add_devices(lm3533->dev, 0, lm3533_led_devs,
- pdata->num_leds, NULL, 0, NULL);
- if (ret) {
- dev_err(lm3533->dev, "failed to add LED devices\n");
- return ret;
- }
+ if (fwnode_device_is_compatible(child, "ti,lm3533-als")) {
+ lm3533_devices[count].name = "lm3533-als";
+ lm3533_devices[count].id = PLATFORM_DEVID_NONE;
+ lm3533_devices[count].of_compatible = "ti,lm3533-als";
- lm3533->have_leds = 1;
+ lm3533->have_als = true;
+ count++;
+ }
- return 0;
-}
+ if (fwnode_device_is_compatible(child, "ti,lm3533-backlight")) {
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ if (ret || reg >= LM3533_HVLED_ID_MAX) {
+ dev_err(dev, "invalid backlight node %s\n",
+ fwnode_get_name(child));
+ continue;
+ }
-static int lm3533_device_init(struct lm3533 *lm3533)
-{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
- int ret;
+ lm3533_devices[count].name = "lm3533-backlight";
+ lm3533_devices[count].id = reg;
+ lm3533_devices[count].of_compatible = "ti,lm3533-backlight";
- dev_dbg(lm3533->dev, "%s\n", __func__);
+ lm3533->have_backlights = true;
+ count++;
+ }
- if (!pdata) {
- dev_err(lm3533->dev, "no platform data\n");
- return -EINVAL;
+ if (fwnode_device_is_compatible(child, "ti,lm3533-leds")) {
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ if (ret || reg < LM3533_HVLED_ID_MAX ||
+ reg > LM3533_LVLED_ID_MAX) {
+ dev_err(dev, "invalid LED node %s\n",
+ fwnode_get_name(child));
+ continue;
+ }
+
+ lm3533_devices[count].name = "lm3533-leds";
+ lm3533_devices[count].id = reg - LM3533_HVLED_ID_MAX;
+ lm3533_devices[count].of_compatible = "ti,lm3533-leds";
+
+ lm3533->have_leds = true;
+ count++;
+ }
}
- lm3533->hwen = devm_gpiod_get(lm3533->dev, NULL, GPIOD_OUT_LOW);
- if (IS_ERR(lm3533->hwen))
- return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen), "failed to request HWEN GPIO\n");
- gpiod_set_consumer_name(lm3533->hwen, "lm3533-hwen");
-
lm3533_enable(lm3533);
ret = regmap_update_bits(lm3533->regmap, LM3533_REG_BOOST_PWM,
LM3533_BOOST_FREQ_MASK,
- pdata->boost_freq << LM3533_BOOST_FREQ_SHIFT);
+ lm3533->boost_freq << LM3533_BOOST_FREQ_SHIFT);
if (ret) {
- dev_err(lm3533->dev, "failed to set boost frequency\n");
+ dev_err(dev, "failed to set boost frequency\n");
goto err_disable;
}
ret = regmap_update_bits(lm3533->regmap, LM3533_REG_BOOST_PWM,
LM3533_BOOST_OVP_MASK,
- pdata->boost_ovp << LM3533_BOOST_OVP_SHIFT);
+ lm3533->boost_ovp << LM3533_BOOST_OVP_SHIFT);
if (ret) {
- dev_err(lm3533->dev, "failed to set boost ovp\n");
+ dev_err(dev, "failed to set boost ovp\n");
goto err_disable;
}
- lm3533_device_als_init(lm3533);
- lm3533_device_bl_init(lm3533);
- lm3533_device_led_init(lm3533);
+ ret = mfd_add_devices(dev, 0, lm3533_devices, count, NULL, 0, NULL);
+ if (ret) {
+ dev_err(dev, "failed to add MFD devices: %d\n", ret);
+ goto err_disable;
+ }
- ret = sysfs_create_group(&lm3533->dev->kobj, &lm3533_attribute_group);
- if (ret < 0) {
- dev_err(lm3533->dev, "failed to create sysfs attributes\n");
+ ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);
+ if (ret) {
+ dev_err(dev, "failed to create sysfs attributes\n");
goto err_unregister;
}
@@ -509,7 +451,26 @@ static int lm3533_i2c_probe(struct i2c_client *i2c)
return PTR_ERR(lm3533->regmap);
lm3533->dev = &i2c->dev;
- lm3533->irq = i2c->irq;
+
+ lm3533->hwen = devm_gpiod_get_optional(lm3533->dev, "enable",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(lm3533->hwen))
+ return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen),
+ "failed to get HWEN GPIO\n");
+
+ device_property_read_u32(lm3533->dev, "ti,boost-ovp-microvolt",
+ &lm3533->boost_ovp);
+
+ lm3533->boost_ovp = clamp(lm3533->boost_ovp, LM3533_BOOST_OVP_MIN,
+ LM3533_BOOST_OVP_MAX);
+ lm3533->boost_ovp = lm3533->boost_ovp / (8 * MICRO) - 2;
+
+ device_property_read_u32(lm3533->dev, "ti,boost-freq-hz",
+ &lm3533->boost_freq);
+
+ lm3533->boost_freq = clamp(lm3533->boost_freq, LM3533_BOOST_FREQ_MIN,
+ LM3533_BOOST_FREQ_MAX);
+ lm3533->boost_freq = lm3533->boost_freq / (500 * KILO) - 1;
return lm3533_device_init(lm3533);
}
@@ -523,6 +484,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" },
{ }
@@ -532,6 +499,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/mfd/lm3533-ctrlbank.c b/drivers/mfd/lm3533-ctrlbank.c
index 37bae279f41e..63543ecd815f 100644
--- a/drivers/mfd/lm3533-ctrlbank.c
+++ b/drivers/mfd/lm3533-ctrlbank.c
@@ -13,11 +13,6 @@
#include <linux/mfd/lm3533.h>
-
-#define LM3533_MAX_CURRENT_MIN 5000
-#define LM3533_MAX_CURRENT_MAX 29800
-#define LM3533_MAX_CURRENT_STEP 800
-
#define LM3533_PWM_MAX 0x3f
#define LM3533_REG_PWM_BASE 0x14
diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
index 5c8e69087860..c70b8a3bb301 100644
--- a/drivers/video/backlight/lm3533_bl.c
+++ b/drivers/video/backlight/lm3533_bl.c
@@ -9,7 +9,9 @@
#include <linux/module.h>
#include <linux/init.h>
+#include <linux/mod_devicetable.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/backlight.h>
#include <linux/regmap.h>
#include <linux/slab.h>
@@ -28,6 +30,9 @@ struct lm3533_bl {
struct lm3533_ctrlbank cb;
struct backlight_device *bd;
int id;
+
+ u32 max_current;
+ u32 pwm;
};
@@ -247,25 +252,24 @@ static struct attribute_group lm3533_bl_attribute_group = {
.attrs = lm3533_bl_attributes
};
-static int lm3533_bl_setup(struct lm3533_bl *bl,
- struct lm3533_bl_platform_data *pdata)
+static int lm3533_bl_setup(struct lm3533_bl *bl)
{
int ret;
- ret = lm3533_ctrlbank_set_max_current(&bl->cb, pdata->max_current);
+ ret = lm3533_ctrlbank_set_max_current(&bl->cb, bl->max_current);
if (ret)
return ret;
- return lm3533_ctrlbank_set_pwm(&bl->cb, pdata->pwm);
+ return lm3533_ctrlbank_set_pwm(&bl->cb, bl->pwm);
}
static int lm3533_bl_probe(struct platform_device *pdev)
{
struct lm3533 *lm3533;
- struct lm3533_bl_platform_data *pdata;
struct lm3533_bl *bl;
struct backlight_device *bd;
struct backlight_properties props;
+ char *name = NULL;
int ret;
dev_dbg(&pdev->dev, "%s\n", __func__);
@@ -274,12 +278,6 @@ static int lm3533_bl_probe(struct platform_device *pdev)
if (!lm3533)
return -EINVAL;
- pdata = dev_get_platdata(&pdev->dev);
- if (!pdata) {
- dev_err(&pdev->dev, "no platform data\n");
- return -EINVAL;
- }
-
if (pdev->id < 0 || pdev->id >= LM3533_HVCTRLBANK_COUNT) {
dev_err(&pdev->dev, "illegal backlight id %d\n", pdev->id);
return -EINVAL;
@@ -296,13 +294,20 @@ static int lm3533_bl_probe(struct platform_device *pdev)
bl->cb.id = lm3533_bl_get_ctrlbank_id(bl);
bl->cb.dev = NULL; /* until registered */
+ name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
+ pdev->name, pdev->id);
+ if (!name)
+ return -ENOMEM;
+
memset(&props, 0, sizeof(props));
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);
+ props.brightness = LM3533_BL_MAX_BRIGHTNESS;
+ device_property_read_u32(&pdev->dev, "default-brightness",
+ &props.brightness);
+
+ bd = devm_backlight_device_register(&pdev->dev, name, &pdev->dev,
+ bl, &lm3533_bl_ops, &props);
if (IS_ERR(bd)) {
dev_err(&pdev->dev, "failed to register backlight device\n");
return PTR_ERR(bd);
@@ -313,6 +318,18 @@ static int lm3533_bl_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, bl);
+ device_property_read_u32(&pdev->dev, "led-max-microamp",
+ &bl->max_current);
+ bl->max_current = clamp(bl->max_current, LM3533_MAX_CURRENT_MIN,
+ LM3533_MAX_CURRENT_MAX);
+
+ bl->pwm = 0;
+ device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &bl->pwm);
+
+ ret = lm3533_bl_setup(bl);
+ if (ret)
+ return ret;
+
ret = sysfs_create_group(&bd->dev.kobj, &lm3533_bl_attribute_group);
if (ret < 0) {
dev_err(&pdev->dev, "failed to create sysfs attributes\n");
@@ -321,10 +338,6 @@ static int lm3533_bl_probe(struct platform_device *pdev)
backlight_update_status(bd);
- ret = lm3533_bl_setup(bl, pdata);
- if (ret)
- goto err_sysfs_remove;
-
ret = lm3533_ctrlbank_enable(&bl->cb);
if (ret)
goto err_sysfs_remove;
@@ -382,10 +395,17 @@ static void lm3533_bl_shutdown(struct platform_device *pdev)
lm3533_ctrlbank_disable(&bl->cb);
}
+static const struct of_device_id lm3533_bl_match_table[] = {
+ { .compatible = "ti,lm3533-backlight" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, lm3533_bl_match_table);
+
static struct platform_driver lm3533_bl_driver = {
.driver = {
.name = "lm3533-backlight",
.pm = &lm3533_bl_pm_ops,
+ .of_match_table = lm3533_bl_match_table,
},
.probe = lm3533_bl_probe,
.remove = lm3533_bl_remove,
diff --git a/include/linux/mfd/lm3533.h b/include/linux/mfd/lm3533.h
index db0a5e5c9e39..3fd8d4737ea5 100644
--- a/include/linux/mfd/lm3533.h
+++ b/include/linux/mfd/lm3533.h
@@ -15,6 +15,10 @@
#define LM3533_ATTR_RW(_name) \
DEVICE_ATTR(_name, S_IRUGO | S_IWUSR , show_##_name, store_##_name)
+#define LM3533_MAX_CURRENT_MIN 5000
+#define LM3533_MAX_CURRENT_MAX 29800
+#define LM3533_MAX_CURRENT_STEP 800
+
struct device;
struct gpio_desc;
struct regmap;
@@ -25,7 +29,9 @@ struct lm3533 {
struct regmap *regmap;
struct gpio_desc *hwen;
- int irq;
+
+ u32 boost_ovp;
+ u32 boost_freq;
unsigned have_als:1;
unsigned have_backlights:1;
@@ -38,50 +44,6 @@ struct lm3533_ctrlbank {
int id;
};
-struct lm3533_als_platform_data {
- unsigned pwm_mode:1; /* PWM input mode (default analog) */
- u8 r_select; /* 1 - 127 (ignored in PWM-mode) */
-};
-
-struct lm3533_bl_platform_data {
- char *name;
- u16 max_current; /* 5000 - 29800 uA (800 uA step) */
- u8 default_brightness; /* 0 - 255 */
- u8 pwm; /* 0 - 0x3f */
-};
-
-struct lm3533_led_platform_data {
- char *name;
- const char *default_trigger;
- u16 max_current; /* 5000 - 29800 uA (800 uA step) */
- u8 pwm; /* 0 - 0x3f */
-};
-
-enum lm3533_boost_freq {
- LM3533_BOOST_FREQ_500KHZ,
- LM3533_BOOST_FREQ_1000KHZ,
-};
-
-enum lm3533_boost_ovp {
- LM3533_BOOST_OVP_16V,
- LM3533_BOOST_OVP_24V,
- LM3533_BOOST_OVP_32V,
- LM3533_BOOST_OVP_40V,
-};
-
-struct lm3533_platform_data {
- enum lm3533_boost_ovp boost_ovp;
- enum lm3533_boost_freq boost_freq;
-
- struct lm3533_als_platform_data *als;
-
- struct lm3533_bl_platform_data *backlights;
- int num_backlights;
-
- struct lm3533_led_platform_data *leds;
- int num_leds;
-};
-
extern int lm3533_ctrlbank_enable(struct lm3533_ctrlbank *cb);
extern int lm3533_ctrlbank_disable(struct lm3533_ctrlbank *cb);
--
2.51.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
2026-06-01 15:18 ` [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings Svyatoslav Ryhel
@ 2026-06-02 8:23 ` Andy Shevchenko
2026-06-02 10:31 ` Svyatoslav Ryhel
2026-06-02 13:46 ` Jonathan Cameron
` (2 subsequent siblings)
3 siblings, 1 reply; 47+ messages in thread
From: Andy Shevchenko @ 2026-06-02 8:23 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
On Mon, Jun 01, 2026 at 06:18:25PM +0300, Svyatoslav Ryhel wrote:
> Since there are no users of this driver via platform data, remove the
> platform data support and switch to using Device Tree bindings.
...
> @@ -57,6 +60,9 @@ struct lm3533_als {
>
> atomic_t zone;
> struct mutex thresh_mutex;
> +
> + bool pwm_mode;
> + u32 r_select;
> };
Have you run `pahole`? Does it agree with the layout you made here?
...
> - als->irq = lm3533->irq;
> + als->irq = platform_get_irq_optional(pdev, 0);
> +
Redundant blank line.
> + if (als->irq == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
What about other error codes when IRQ is found by can't be retrieved for some
reasons? IIRC we check against ENXIO in similar cases
als->irq = platform_get_irq_optional(pdev, 0);
if (als->irq == -ENXIO)
als->irq = 0;
if (als->irq < 0)
return als->irq;
...
> + led->pwm = 0;
Isn't it 0 by zalloc ?
> + device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);
...
> #define LM3533_BOOST_FREQ_MASK 0x01
> #define LM3533_BOOST_FREQ_SHIFT 0
> +#define LM3533_BOOST_FREQ_MIN 500000
> +#define LM3533_BOOST_FREQ_MAX 1000000
HZ_PER_KHZ (since you included units.h)?
...
> + nchilds = device_get_child_node_count(dev);
> + if (!nchilds || nchilds > LM3533_CELLS_MAX) {
> + dev_err(dev, "num of child nodes is not supported\n");
> + return -ENODEV;
Why not dev_err_probe() here and elsewhere? It looks inconsistent with this
patch.
> }
...
> + device_for_each_child_node_scoped(lm3533->dev, child) {
> + if (!fwnode_device_is_available(child))
> + continue;
Do we need this check?
...
> + dev_err(dev, "invalid LED node %s\n",
> + fwnode_get_name(child));
%pfw
...
> + ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);
No way. You should use .dev_groups.
> + if (ret) {
> + dev_err(dev, "failed to create sysfs attributes\n");
> goto err_unregister;
> }
...
Can you think on how to split this change to smaller steps? I believe it's
possible.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
2026-06-02 8:23 ` Andy Shevchenko
@ 2026-06-02 10:31 ` Svyatoslav Ryhel
2026-06-02 11:05 ` Andy Shevchenko
0 siblings, 1 reply; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-02 10:31 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
вт, 2 черв. 2026 р. о 11:24 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
>
> On Mon, Jun 01, 2026 at 06:18:25PM +0300, Svyatoslav Ryhel wrote:
> > Since there are no users of this driver via platform data, remove the
> > platform data support and switch to using Device Tree bindings.
>
> ...
>
> > @@ -57,6 +60,9 @@ struct lm3533_als {
> >
> > atomic_t zone;
> > struct mutex thresh_mutex;
> > +
> > + bool pwm_mode;
> > + u32 r_select;
> > };
>
> Have you run `pahole`? Does it agree with the layout you made here?
>
Noted.
> ...
>
> > - als->irq = lm3533->irq;
> > + als->irq = platform_get_irq_optional(pdev, 0);
>
> > +
>
> Redundant blank line.
>
Simplifies code perception, whatever.
> > + if (als->irq == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
>
> What about other error codes when IRQ is found by can't be retrieved for some
> reasons? IIRC we check against ENXIO in similar cases
>
Then we treat it as no IRQ. Original implementation cares only if IRQ
is present or no.
> als->irq = platform_get_irq_optional(pdev, 0);
> if (als->irq == -ENXIO)
> als->irq = 0;
> if (als->irq < 0)
> return als->irq;
>
> ...
>
> > + led->pwm = 0;
>
> Isn't it 0 by zalloc ?
It is, thanks.
>
> > + device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);
>
> ...
>
> > #define LM3533_BOOST_FREQ_MASK 0x01
> > #define LM3533_BOOST_FREQ_SHIFT 0
> > +#define LM3533_BOOST_FREQ_MIN 500000
> > +#define LM3533_BOOST_FREQ_MAX 1000000
>
> HZ_PER_KHZ (since you included units.h)?
>
500 * HZ_PER_KHZ
1000 * HZ_PER_KHZ
You meant this? Sure.
> ...
>
> > + nchilds = device_get_child_node_count(dev);
> > + if (!nchilds || nchilds > LM3533_CELLS_MAX) {
> > + dev_err(dev, "num of child nodes is not supported\n");
> > + return -ENODEV;
>
> Why not dev_err_probe() here and elsewhere? It looks inconsistent with this
> patch.
>
I must have overlooked it, thanks. WDYM elsewhere, this is the only occurance.
> > }
>
> ...
>
> > + device_for_each_child_node_scoped(lm3533->dev, child) {
>
> > + if (!fwnode_device_is_available(child))
> > + continue;
>
> Do we need this check?
>
This is nice to have if the node is disabled. If we assume that there
are no disabled nodes, I can remove it.
> ...
>
> > + dev_err(dev, "invalid LED node %s\n",
> > + fwnode_get_name(child));
>
> %pfw
>
Noted.
> ...
>
> > + ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);
>
> No way. You should use .dev_groups.
>
I did not change how driver does this, just swapped lm3533->dev to
dev. I will set is back as it was.
> > + if (ret) {
> > + dev_err(dev, "failed to create sysfs attributes\n");
> > goto err_unregister;
> > }
>
> ...
>
> Can you think on how to split this change to smaller steps? I believe it's
> possible.
>
No, I am done with tinkering with this patchset. It is broken enough
and it has inflated enough.
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
2026-06-02 10:31 ` Svyatoslav Ryhel
@ 2026-06-02 11:05 ` Andy Shevchenko
2026-06-02 12:00 ` Svyatoslav Ryhel
0 siblings, 1 reply; 47+ messages in thread
From: Andy Shevchenko @ 2026-06-02 11:05 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
On Tue, Jun 02, 2026 at 01:31:44PM +0300, Svyatoslav Ryhel wrote:
> вт, 2 черв. 2026 р. о 11:24 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > On Mon, Jun 01, 2026 at 06:18:25PM +0300, Svyatoslav Ryhel wrote:
...
> > > + device_for_each_child_node_scoped(lm3533->dev, child) {
> >
> > > + if (!fwnode_device_is_available(child))
> > > + continue;
> >
> > Do we need this check?
>
> This is nice to have if the node is disabled. If we assume that there
> are no disabled nodes, I can remove it.
It's already implied. See
static struct fwnode_handle *
of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode, struct fwnode_handle *child)
{
return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode), to_of_node(child)));
}
And I believe it's written somewhere in the documentation (if not, feel free to
patch that).
...
> > > + ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);
> >
> > No way. You should use .dev_groups.
>
> I did not change how driver does this, just swapped lm3533->dev to
> dev. I will set is back as it was.
This is a serious race condition that needs to be addressed. Since you are
touching this driver the fixes against known issues probably are the first
things that have to be done.
> > > + if (ret) {
> > > + dev_err(dev, "failed to create sysfs attributes\n");
> > > goto err_unregister;
> > > }
...
> > Can you think on how to split this change to smaller steps? I believe it's
> > possible.
>
> No, I am done with tinkering with this patchset. It is broken enough
> and it has inflated enough.
Probably you don't want this to be reviewed then? I believe other kernel
developers and maintainers will ask you the same.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
2026-06-02 11:05 ` Andy Shevchenko
@ 2026-06-02 12:00 ` Svyatoslav Ryhel
0 siblings, 0 replies; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-02 12:00 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
вт, 2 черв. 2026 р. о 14:05 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
>
> On Tue, Jun 02, 2026 at 01:31:44PM +0300, Svyatoslav Ryhel wrote:
> > вт, 2 черв. 2026 р. о 11:24 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > > On Mon, Jun 01, 2026 at 06:18:25PM +0300, Svyatoslav Ryhel wrote:
>
>
> ...
>
> > > > + device_for_each_child_node_scoped(lm3533->dev, child) {
> > >
> > > > + if (!fwnode_device_is_available(child))
> > > > + continue;
> > >
> > > Do we need this check?
> >
> > This is nice to have if the node is disabled. If we assume that there
> > are no disabled nodes, I can remove it.
>
> It's already implied. See
>
> static struct fwnode_handle *
> of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode, struct fwnode_handle *child)
> {
> return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode), to_of_node(child)));
> }
>
> And I believe it's written somewhere in the documentation (if not, feel free to
> patch that).
>
Very nice. Thank you.
> ...
>
> > > > + ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);
> > >
> > > No way. You should use .dev_groups.
> >
> > I did not change how driver does this, just swapped lm3533->dev to
> > dev. I will set is back as it was.
>
> This is a serious race condition that needs to be addressed. Since you are
> touching this driver the fixes against known issues probably are the first
> things that have to be done.
>
Fine, I will have a look.
> > > > + if (ret) {
> > > > + dev_err(dev, "failed to create sysfs attributes\n");
> > > > goto err_unregister;
> > > > }
>
> ...
>
> > > Can you think on how to split this change to smaller steps? I believe it's
> > > possible.
> >
> > No, I am done with tinkering with this patchset. It is broken enough
> > and it has inflated enough.
>
> Probably you don't want this to be reviewed then? I believe other kernel
> developers and maintainers will ask you the same.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
2026-06-01 15:18 ` [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings Svyatoslav Ryhel
2026-06-02 8:23 ` Andy Shevchenko
@ 2026-06-02 13:46 ` Jonathan Cameron
2026-06-02 13:50 ` Svyatoslav Ryhel
2026-06-02 13:49 ` Daniel Thompson
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
3 siblings, 1 reply; 47+ messages in thread
From: Jonathan Cameron @ 2026-06-02 13:46 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, David Lechner, Nuno Sá,
Andy Shevchenko, Helge Deller, Johan Hovold, dri-devel,
linux-leds, devicetree, linux-kernel, linux-iio, linux-fbdev
On Mon, 1 Jun 2026 18:18:25 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> Since there are no users of this driver via platform data, remove the
> platform data support and switch to using Device Tree bindings.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> index 52136ca1abc9..55b35467a722 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c
> @@ -16,16 +16,19 @@
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/regmap.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> +#include <linux/units.h>
>
> #include <linux/mfd/lm3533.h>
>
>
> -#define LM3533_ALS_RESISTOR_MIN 1
> -#define LM3533_ALS_RESISTOR_MAX 127
> +#define LM3533_ALS_RESISTOR_MIN 1575
> +#define LM3533_ALS_RESISTOR_MAX 200000
> #define LM3533_ALS_CHANNEL_CURRENT_MAX 2
> #define LM3533_ALS_THRESH_MAX 3
> #define LM3533_ALS_ZONE_MAX 4
> @@ -57,6 +60,9 @@ struct lm3533_als {
>
> atomic_t zone;
> struct mutex thresh_mutex;
> +
> + bool pwm_mode;
> + u32 r_select;
> };
>
>
> @@ -411,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev,
> int enable;
> int ret;
>
> - if (als->irq) {
> + if (als->irq > 0) {
> ret = lm3533_als_get_int_mode(indio_dev, &enable);
> if (ret)
> return ret;
> @@ -716,30 +722,34 @@ static const struct attribute_group lm3533_als_attribute_group = {
> .attrs = lm3533_als_attributes
> };
>
> -static int lm3533_als_setup(struct lm3533_als *als,
> - const struct lm3533_als_platform_data *pdata)
> +static int lm3533_als_setup(struct lm3533_als *als)
> {
> struct device *dev = &als->pdev->dev;
> int ret;
>
> + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> +
> ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> LM3533_ALS_INPUT_MODE_MASK,
> - pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> + als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> if (ret)
> return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> - pdata->pwm_mode);
> -
> + als->pwm_mode);
>
> /* ALS input is always high impedance in PWM-mode. */
> - if (!pdata->pwm_mode) {
> - if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> - pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> - dev_err(&als->pdev->dev, "invalid resistor value\n");
> - return -EINVAL;
> - }
> + if (!als->pwm_mode) {
> + ret = device_property_read_u32(dev, "ti,resistor-value-ohms",
> + &als->r_select);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to ger resistor value\n");
> +
> + als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> + LM3533_ALS_RESISTOR_MAX);
If we are getting garbage from DT I think I'd rather error out that paper over
that problem. So similar to before, check valid value and if not fail probe
so that hopefully someone goes and fixes it!
> + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
Why do we need this when we didn't before? The range checks are the same
so it smells like it shouldn't need transforming. I'd also rather we didn't do
rewriting of the meaning of r_select like this. Just use a local variable for
the intermediate result.
>
> ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> - pdata->r_select);
> + als->r_select);
> if (ret)
> return dev_err_probe(dev, ret, "failed to set resistor\n");
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
2026-06-02 13:46 ` Jonathan Cameron
@ 2026-06-02 13:50 ` Svyatoslav Ryhel
2026-06-02 14:20 ` Jonathan Cameron
0 siblings, 1 reply; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-02 13:50 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, David Lechner, Nuno Sá,
Andy Shevchenko, Helge Deller, Johan Hovold, dri-devel,
linux-leds, devicetree, linux-kernel, linux-iio, linux-fbdev
вт, 2 черв. 2026 р. о 16:46 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Mon, 1 Jun 2026 18:18:25 +0300
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > Since there are no users of this driver via platform data, remove the
> > platform data support and switch to using Device Tree bindings.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>
>
> > index 52136ca1abc9..55b35467a722 100644
> > --- a/drivers/iio/light/lm3533-als.c
> > +++ b/drivers/iio/light/lm3533-als.c
> > @@ -16,16 +16,19 @@
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/mfd/core.h>
> > +#include <linux/mod_devicetable.h>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/regmap.h>
> > #include <linux/slab.h>
> > #include <linux/uaccess.h>
> > +#include <linux/units.h>
> >
> > #include <linux/mfd/lm3533.h>
> >
> >
> > -#define LM3533_ALS_RESISTOR_MIN 1
> > -#define LM3533_ALS_RESISTOR_MAX 127
> > +#define LM3533_ALS_RESISTOR_MIN 1575
> > +#define LM3533_ALS_RESISTOR_MAX 200000
> > #define LM3533_ALS_CHANNEL_CURRENT_MAX 2
> > #define LM3533_ALS_THRESH_MAX 3
> > #define LM3533_ALS_ZONE_MAX 4
> > @@ -57,6 +60,9 @@ struct lm3533_als {
> >
> > atomic_t zone;
> > struct mutex thresh_mutex;
> > +
> > + bool pwm_mode;
> > + u32 r_select;
> > };
> >
> >
> > @@ -411,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev,
> > int enable;
> > int ret;
> >
> > - if (als->irq) {
> > + if (als->irq > 0) {
> > ret = lm3533_als_get_int_mode(indio_dev, &enable);
> > if (ret)
> > return ret;
> > @@ -716,30 +722,34 @@ static const struct attribute_group lm3533_als_attribute_group = {
> > .attrs = lm3533_als_attributes
> > };
> >
> > -static int lm3533_als_setup(struct lm3533_als *als,
> > - const struct lm3533_als_platform_data *pdata)
> > +static int lm3533_als_setup(struct lm3533_als *als)
> > {
> > struct device *dev = &als->pdev->dev;
> > int ret;
> >
> > + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > +
> > ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> > LM3533_ALS_INPUT_MODE_MASK,
> > - pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> > + als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> > if (ret)
> > return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > - pdata->pwm_mode);
> > -
> > + als->pwm_mode);
> >
> > /* ALS input is always high impedance in PWM-mode. */
> > - if (!pdata->pwm_mode) {
> > - if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> > - pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> > - dev_err(&als->pdev->dev, "invalid resistor value\n");
> > - return -EINVAL;
> > - }
> > + if (!als->pwm_mode) {
> > + ret = device_property_read_u32(dev, "ti,resistor-value-ohms",
> > + &als->r_select);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "failed to ger resistor value\n");
> > +
> > + als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> > + LM3533_ALS_RESISTOR_MAX);
>
> If we are getting garbage from DT I think I'd rather error out that paper over
> that problem. So similar to before, check valid value and if not fail probe
> so that hopefully someone goes and fixes it!
>
sure
> > + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
> Why do we need this when we didn't before? The range checks are the same
> so it smells like it shouldn't need transforming. I'd also rather we didn't do
> rewriting of the meaning of r_select like this. Just use a local variable for
> the intermediate result.
>
before pdata passed resistor value as actual register value, not we
are getting the actual resistance in ohms from the tree and must
convert it into register value.
> >
> > ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> > - pdata->r_select);
> > + als->r_select);
> > if (ret)
> > return dev_err_probe(dev, ret, "failed to set resistor\n");
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
2026-06-02 13:50 ` Svyatoslav Ryhel
@ 2026-06-02 14:20 ` Jonathan Cameron
2026-06-02 14:28 ` Svyatoslav Ryhel
0 siblings, 1 reply; 47+ messages in thread
From: Jonathan Cameron @ 2026-06-02 14:20 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, David Lechner, Nuno Sá,
Andy Shevchenko, Helge Deller, Johan Hovold, dri-devel,
linux-leds, devicetree, linux-kernel, linux-iio, linux-fbdev
On Tue, 2 Jun 2026 16:50:16 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> вт, 2 черв. 2026 р. о 16:46 Jonathan Cameron <jic23@kernel.org> пише:
> >
> > On Mon, 1 Jun 2026 18:18:25 +0300
> > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >
> > > Since there are no users of this driver via platform data, remove the
> > > platform data support and switch to using Device Tree bindings.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> >
> >
> > > index 52136ca1abc9..55b35467a722 100644
> > > --- a/drivers/iio/light/lm3533-als.c
> > > +++ b/drivers/iio/light/lm3533-als.c
> > > @@ -16,16 +16,19 @@
> > > #include <linux/module.h>
> > > #include <linux/mutex.h>
> > > #include <linux/mfd/core.h>
> > > +#include <linux/mod_devicetable.h>
> > > #include <linux/platform_device.h>
> > > +#include <linux/property.h>
> > > #include <linux/regmap.h>
> > > #include <linux/slab.h>
> > > #include <linux/uaccess.h>
> > > +#include <linux/units.h>
> > >
> > > #include <linux/mfd/lm3533.h>
> > >
> > >
> > > -#define LM3533_ALS_RESISTOR_MIN 1
> > > -#define LM3533_ALS_RESISTOR_MAX 127
> > > +#define LM3533_ALS_RESISTOR_MIN 1575
> > > +#define LM3533_ALS_RESISTOR_MAX 200000
> > > #define LM3533_ALS_CHANNEL_CURRENT_MAX 2
> > > #define LM3533_ALS_THRESH_MAX 3
> > > #define LM3533_ALS_ZONE_MAX 4
> > > @@ -57,6 +60,9 @@ struct lm3533_als {
> > >
> > > atomic_t zone;
> > > struct mutex thresh_mutex;
> > > +
> > > + bool pwm_mode;
> > > + u32 r_select;
> > > };
> > >
> > >
> > > @@ -411,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev,
> > > int enable;
> > > int ret;
> > >
> > > - if (als->irq) {
> > > + if (als->irq > 0) {
> > > ret = lm3533_als_get_int_mode(indio_dev, &enable);
> > > if (ret)
> > > return ret;
> > > @@ -716,30 +722,34 @@ static const struct attribute_group lm3533_als_attribute_group = {
> > > .attrs = lm3533_als_attributes
> > > };
> > >
> > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > - const struct lm3533_als_platform_data *pdata)
> > > +static int lm3533_als_setup(struct lm3533_als *als)
> > > {
> > > struct device *dev = &als->pdev->dev;
> > > int ret;
> > >
> > > + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > > +
> > > ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> > > LM3533_ALS_INPUT_MODE_MASK,
> > > - pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> > > + als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> > > if (ret)
> > > return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > > - pdata->pwm_mode);
> > > -
> > > + als->pwm_mode);
> > >
> > > /* ALS input is always high impedance in PWM-mode. */
> > > - if (!pdata->pwm_mode) {
> > > - if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> > > - pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> > > - dev_err(&als->pdev->dev, "invalid resistor value\n");
> > > - return -EINVAL;
> > > - }
> > > + if (!als->pwm_mode) {
> > > + ret = device_property_read_u32(dev, "ti,resistor-value-ohms",
> > > + &als->r_select);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret,
> > > + "failed to ger resistor value\n");
> > > +
> > > + als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> > > + LM3533_ALS_RESISTOR_MAX);
> >
> > If we are getting garbage from DT I think I'd rather error out that paper over
> > that problem. So similar to before, check valid value and if not fail probe
> > so that hopefully someone goes and fixes it!
> >
>
> sure
>
> > > + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
> > Why do we need this when we didn't before? The range checks are the same
> > so it smells like it shouldn't need transforming. I'd also rather we didn't do
> > rewriting of the meaning of r_select like this. Just use a local variable for
> > the intermediate result.
> >
>
> before pdata passed resistor value as actual register value, not we
> are getting the actual resistance in ohms from the tree and must
> convert it into register value.
ah. I missed the change of values. Can you make them explicitly now _OHMS or something
along those lines rather than reusing the macro name for a different thing.
>
> > >
> > > ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> > > - pdata->r_select);
> > > + als->r_select);
> > > if (ret)
> > > return dev_err_probe(dev, ret, "failed to set resistor\n");
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
2026-06-02 14:20 ` Jonathan Cameron
@ 2026-06-02 14:28 ` Svyatoslav Ryhel
0 siblings, 0 replies; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-02 14:28 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, David Lechner, Nuno Sá,
Andy Shevchenko, Helge Deller, Johan Hovold, dri-devel,
linux-leds, devicetree, linux-kernel, linux-iio, linux-fbdev
вт, 2 черв. 2026 р. о 17:20 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Tue, 2 Jun 2026 16:50:16 +0300
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > вт, 2 черв. 2026 р. о 16:46 Jonathan Cameron <jic23@kernel.org> пише:
> > >
> > > On Mon, 1 Jun 2026 18:18:25 +0300
> > > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> > >
> > > > Since there are no users of this driver via platform data, remove the
> > > > platform data support and switch to using Device Tree bindings.
> > > >
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > >
> > >
> > > > index 52136ca1abc9..55b35467a722 100644
> > > > --- a/drivers/iio/light/lm3533-als.c
> > > > +++ b/drivers/iio/light/lm3533-als.c
> > > > @@ -16,16 +16,19 @@
> > > > #include <linux/module.h>
> > > > #include <linux/mutex.h>
> > > > #include <linux/mfd/core.h>
> > > > +#include <linux/mod_devicetable.h>
> > > > #include <linux/platform_device.h>
> > > > +#include <linux/property.h>
> > > > #include <linux/regmap.h>
> > > > #include <linux/slab.h>
> > > > #include <linux/uaccess.h>
> > > > +#include <linux/units.h>
> > > >
> > > > #include <linux/mfd/lm3533.h>
> > > >
> > > >
> > > > -#define LM3533_ALS_RESISTOR_MIN 1
> > > > -#define LM3533_ALS_RESISTOR_MAX 127
> > > > +#define LM3533_ALS_RESISTOR_MIN 1575
> > > > +#define LM3533_ALS_RESISTOR_MAX 200000
> > > > #define LM3533_ALS_CHANNEL_CURRENT_MAX 2
> > > > #define LM3533_ALS_THRESH_MAX 3
> > > > #define LM3533_ALS_ZONE_MAX 4
> > > > @@ -57,6 +60,9 @@ struct lm3533_als {
> > > >
> > > > atomic_t zone;
> > > > struct mutex thresh_mutex;
> > > > +
> > > > + bool pwm_mode;
> > > > + u32 r_select;
> > > > };
> > > >
> > > >
> > > > @@ -411,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev,
> > > > int enable;
> > > > int ret;
> > > >
> > > > - if (als->irq) {
> > > > + if (als->irq > 0) {
> > > > ret = lm3533_als_get_int_mode(indio_dev, &enable);
> > > > if (ret)
> > > > return ret;
> > > > @@ -716,30 +722,34 @@ static const struct attribute_group lm3533_als_attribute_group = {
> > > > .attrs = lm3533_als_attributes
> > > > };
> > > >
> > > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > > - const struct lm3533_als_platform_data *pdata)
> > > > +static int lm3533_als_setup(struct lm3533_als *als)
> > > > {
> > > > struct device *dev = &als->pdev->dev;
> > > > int ret;
> > > >
> > > > + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > > > +
> > > > ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> > > > LM3533_ALS_INPUT_MODE_MASK,
> > > > - pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> > > > + als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> > > > if (ret)
> > > > return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > > > - pdata->pwm_mode);
> > > > -
> > > > + als->pwm_mode);
> > > >
> > > > /* ALS input is always high impedance in PWM-mode. */
> > > > - if (!pdata->pwm_mode) {
> > > > - if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> > > > - pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> > > > - dev_err(&als->pdev->dev, "invalid resistor value\n");
> > > > - return -EINVAL;
> > > > - }
> > > > + if (!als->pwm_mode) {
> > > > + ret = device_property_read_u32(dev, "ti,resistor-value-ohms",
> > > > + &als->r_select);
> > > > + if (ret)
> > > > + return dev_err_probe(dev, ret,
> > > > + "failed to ger resistor value\n");
> > > > +
> > > > + als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> > > > + LM3533_ALS_RESISTOR_MAX);
> > >
> > > If we are getting garbage from DT I think I'd rather error out that paper over
> > > that problem. So similar to before, check valid value and if not fail probe
> > > so that hopefully someone goes and fixes it!
> > >
> >
> > sure
> >
> > > > + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
> > > Why do we need this when we didn't before? The range checks are the same
> > > so it smells like it shouldn't need transforming. I'd also rather we didn't do
> > > rewriting of the meaning of r_select like this. Just use a local variable for
> > > the intermediate result.
> > >
> >
> > before pdata passed resistor value as actual register value, not we
> > are getting the actual resistance in ohms from the tree and must
> > convert it into register value.
>
> ah. I missed the change of values. Can you make them explicitly now _OHMS or something
> along those lines rather than reusing the macro name for a different thing.
>
Acknowledged.
> >
> > > >
> > > > ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> > > > - pdata->r_select);
> > > > + als->r_select);
> > > > if (ret)
> > > > return dev_err_probe(dev, ret, "failed to set resistor\n");
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings
2026-06-01 15:18 ` [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings Svyatoslav Ryhel
2026-06-02 8:23 ` Andy Shevchenko
2026-06-02 13:46 ` Jonathan Cameron
@ 2026-06-02 13:49 ` Daniel Thompson
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
3 siblings, 0 replies; 47+ messages in thread
From: Daniel Thompson @ 2026-06-02 13:49 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
On Mon, Jun 01, 2026 at 06:18:25PM +0300, Svyatoslav Ryhel wrote:
> Since there are no users of this driver via platform data, remove the
> platform data support and switch to using Device Tree bindings.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org> #for backlight
Daniel.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Claude review: mfd: lm3533: Convert to use OF bindings
2026-06-01 15:18 ` [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings Svyatoslav Ryhel
` (2 preceding siblings ...)
2026-06-02 13:49 ` Daniel Thompson
@ 2026-06-04 3:59 ` Claude Code Review Bot
3 siblings, 0 replies; 47+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the largest patch. The overall conversion from pdata to DT is well done — dynamic `mfd_cell` allocation from child nodes, parsing DT properties, removing all pdata structs.
**Bug: DT property name mismatch for ALS resistor.**
The binding schema (patch 1) defines the property as `ti,resistor-ohms`, but the driver reads:
```c
ret = device_property_read_u32(dev, "ti,resistor-value-ohms",
&als->r_select);
```
This should be `"ti,resistor-ohms"` to match the binding.
**Typo in error message:**
```c
return dev_err_probe(dev, ret,
"failed to ger resistor value\n");
```
"ger" should be "get".
**Minor observations:**
- The resistor ohms-to-register conversion `DIV_ROUND_UP(2 * MICRO, 10 * als->r_select)` is correct: for 1575 ohms → register 127, for 200000 ohms → register 1.
- `led->cdev.default_trigger = "none"` is unconventional; the standard approach is to leave it NULL when no trigger is specified. The subsequent `device_property_read_string` will overwrite it only if the DT property exists.
- Moving `LM3533_MAX_CURRENT_*` defines from `lm3533-ctrlbank.c` to the shared header is necessary since both LED and backlight drivers now clamp current values before calling `lm3533_ctrlbank_set_max_current`.
- The `boost_ovp` and `boost_freq` values are not initialized before the `clamp()` call. If the DT property is missing, `device_property_read_u32` leaves the variable uninitialized. Since `boost_ovp` and `boost_freq` are struct members (zero-initialized via devm_kzalloc), the default value 0 will be clamped to the minimum. This works but could be more explicit.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 06/11] mfd: lm3533: Add support for VIN power supply
2026-06-01 15:18 [PATCH v3 00/11] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
` (4 preceding siblings ...)
2026-06-01 15:18 ` [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings Svyatoslav Ryhel
@ 2026-06-01 15:18 ` Svyatoslav Ryhel
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
2026-06-01 15:18 ` [PATCH v3 07/11] mfd: lm3533: Set DMA mask Svyatoslav Ryhel
` (5 subsequent siblings)
11 siblings, 1 reply; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-01 15:18 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Svyatoslav Ryhel
Cc: Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
Add support for 2.7V-5.5V VIN power supply.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/mfd/lm3533-core.c | 23 +++++++++++++++++++++--
include/linux/mfd/lm3533.h | 2 ++
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
index d57c2cdf11c5..4dca5641b774 100644
--- a/drivers/mfd/lm3533-core.c
+++ b/drivers/mfd/lm3533-core.c
@@ -17,6 +17,7 @@
#include <linux/mod_devicetable.h>
#include <linux/property.h>
#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
#include <linux/uaccess.h>
@@ -113,14 +114,25 @@ static int lm3533_set_lvled_config(struct lm3533 *lm3533, u8 lvled, u8 led)
return ret;
}
-static void lm3533_enable(struct lm3533 *lm3533)
+static int lm3533_enable(struct lm3533 *lm3533)
{
+ int ret;
+
+ ret = regulator_enable(lm3533->vin_supply);
+ if (ret) {
+ dev_err(lm3533->dev, "failed to enable vin power supply\n");
+ return ret;
+ }
+
gpiod_set_value(lm3533->hwen, 1);
+
+ return 0;
}
static void lm3533_disable(struct lm3533 *lm3533)
{
gpiod_set_value(lm3533->hwen, 0);
+ regulator_disable(lm3533->vin_supply);
}
enum lm3533_attribute_type {
@@ -334,7 +346,9 @@ static int lm3533_device_init(struct lm3533 *lm3533)
}
}
- lm3533_enable(lm3533);
+ ret = lm3533_enable(lm3533);
+ if (ret)
+ return ret;
ret = regmap_update_bits(lm3533->regmap, LM3533_REG_BOOST_PWM,
LM3533_BOOST_FREQ_MASK,
@@ -458,6 +472,11 @@ static int lm3533_i2c_probe(struct i2c_client *i2c)
return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen),
"failed to get HWEN GPIO\n");
+ lm3533->vin_supply = devm_regulator_get(lm3533->dev, "vin");
+ if (IS_ERR(lm3533->vin_supply))
+ return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->vin_supply),
+ "failed to get vin-supply\n");
+
device_property_read_u32(lm3533->dev, "ti,boost-ovp-microvolt",
&lm3533->boost_ovp);
diff --git a/include/linux/mfd/lm3533.h b/include/linux/mfd/lm3533.h
index 3fd8d4737ea5..9ee99be93d9b 100644
--- a/include/linux/mfd/lm3533.h
+++ b/include/linux/mfd/lm3533.h
@@ -22,6 +22,7 @@
struct device;
struct gpio_desc;
struct regmap;
+struct regulator;
struct lm3533 {
struct device *dev;
@@ -29,6 +30,7 @@ struct lm3533 {
struct regmap *regmap;
struct gpio_desc *hwen;
+ struct regulator *vin_supply;
u32 boost_ovp;
u32 boost_freq;
--
2.51.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v3 07/11] mfd: lm3533: Set DMA mask
2026-06-01 15:18 [PATCH v3 00/11] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
` (5 preceding siblings ...)
2026-06-01 15:18 ` [PATCH v3 06/11] mfd: lm3533: Add support for VIN power supply Svyatoslav Ryhel
@ 2026-06-01 15:18 ` Svyatoslav Ryhel
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
2026-06-01 15:18 ` [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic Svyatoslav Ryhel
` (4 subsequent siblings)
11 siblings, 1 reply; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-01 15:18 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Svyatoslav Ryhel
Cc: Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
Missing coherent_dma_mask assigning triggers the following warning in
dmesg:
[ 3.287872] platform lm3533-backlight.0: DMA mask not set
Since this warning might be elevated to an error in the future, set
coherent_dma_mask to zero because both the core and cells do not utilize
DMA.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/mfd/lm3533-core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
index 4dca5641b774..b597d338d6f7 100644
--- a/drivers/mfd/lm3533-core.c
+++ b/drivers/mfd/lm3533-core.c
@@ -491,6 +491,10 @@ static int lm3533_i2c_probe(struct i2c_client *i2c)
LM3533_BOOST_FREQ_MAX);
lm3533->boost_freq = lm3533->boost_freq / (500 * KILO) - 1;
+ /* LM3533 and child devices do not use DMA */
+ i2c->dev.coherent_dma_mask = 0;
+ i2c->dev.dma_mask = &i2c->dev.coherent_dma_mask;
+
return lm3533_device_init(lm3533);
}
--
2.51.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* Claude review: mfd: lm3533: Set DMA mask
2026-06-01 15:18 ` [PATCH v3 07/11] mfd: lm3533: Set DMA mask Svyatoslav Ryhel
@ 2026-06-04 3:59 ` Claude Code Review Bot
0 siblings, 0 replies; 47+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
```c
i2c->dev.coherent_dma_mask = 0;
i2c->dev.dma_mask = &i2c->dev.coherent_dma_mask;
```
This approach is questionable. The warning "DMA mask not set" comes from the child MFD platform devices, not the i2c parent. Setting the DMA mask on the i2c device may not propagate correctly to MFD children created by `mfd_add_devices()`. The proper fix is likely in the MFD core or by using `dma_coerce_mask_and_coherent()` on the child devices. I'd suggest confirming this actually silences the warning.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic
2026-06-01 15:18 [PATCH v3 00/11] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
` (6 preceding siblings ...)
2026-06-01 15:18 ` [PATCH v3 07/11] mfd: lm3533: Set DMA mask Svyatoslav Ryhel
@ 2026-06-01 15:18 ` Svyatoslav Ryhel
2026-06-02 8:09 ` Andy Shevchenko
` (2 more replies)
2026-06-01 15:18 ` [PATCH v3 09/11] video: backlight: lm3533_bl: Set initial mapping mode from DT Svyatoslav Ryhel
` (3 subsequent siblings)
11 siblings, 3 replies; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-01 15:18 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Svyatoslav Ryhel
Cc: Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
Simplify the sysfs logic of the linear property by switching to a macro
and a ternary operator.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/video/backlight/lm3533_bl.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
index c70b8a3bb301..36e6f027613a 100644
--- a/drivers/video/backlight/lm3533_bl.c
+++ b/drivers/video/backlight/lm3533_bl.c
@@ -23,6 +23,7 @@
#define LM3533_BL_MAX_BRIGHTNESS 255
#define LM3533_REG_CTRLBANK_AB_BCONF 0x1a
+#define CTRLBANK_AB_BCONF_MODE(n) BIT(2 * (n) + 1)
struct lm3533_bl {
@@ -136,8 +137,9 @@ static ssize_t show_linear(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct lm3533_bl *bl = dev_get_drvdata(dev);
+ int id = lm3533_bl_get_ctrlbank_id(bl);
+ u8 mask = CTRLBANK_AB_BCONF_MODE(id);
u32 val;
- u8 mask;
int linear;
int ret;
@@ -145,8 +147,6 @@ static ssize_t show_linear(struct device *dev,
if (ret)
return ret;
- mask = 1 << (2 * lm3533_bl_get_ctrlbank_id(bl) + 1);
-
if (val & mask)
linear = 1;
else
@@ -160,23 +160,16 @@ static ssize_t store_linear(struct device *dev,
const char *buf, size_t len)
{
struct lm3533_bl *bl = dev_get_drvdata(dev);
+ int id = lm3533_bl_get_ctrlbank_id(bl);
unsigned long linear;
- u8 mask;
- u8 val;
int ret;
if (kstrtoul(buf, 0, &linear))
return -EINVAL;
- mask = 1 << (2 * lm3533_bl_get_ctrlbank_id(bl) + 1);
-
- if (linear)
- val = mask;
- else
- val = 0;
-
ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
- mask, val);
+ CTRLBANK_AB_BCONF_MODE(id),
+ linear ? CTRLBANK_AB_BCONF_MODE(id) : 0);
if (ret)
return ret;
--
2.51.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic
2026-06-01 15:18 ` [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic Svyatoslav Ryhel
@ 2026-06-02 8:09 ` Andy Shevchenko
2026-06-02 10:19 ` Svyatoslav Ryhel
2026-06-02 13:55 ` Daniel Thompson
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
2 siblings, 1 reply; 47+ messages in thread
From: Andy Shevchenko @ 2026-06-02 8:09 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
On Mon, Jun 01, 2026 at 06:18:28PM +0300, Svyatoslav Ryhel wrote:
> Simplify the sysfs logic of the linear property by switching to a macro
> and a ternary operator.
...
> if (kstrtoul(buf, 0, &linear))
> return -EINVAL;
Besides _assign_bits() in the below, side note here to unshadow error codes:
ret = kstrtoul(buf, 0, &linear);
if (ret)
return ret;
(obviously in a separate change).
...
> ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> - mask, val);
> + CTRLBANK_AB_BCONF_MODE(id),
> + linear ? CTRLBANK_AB_BCONF_MODE(id) : 0);
> if (ret)
> return ret;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic
2026-06-02 8:09 ` Andy Shevchenko
@ 2026-06-02 10:19 ` Svyatoslav Ryhel
2026-06-02 11:07 ` Andy Shevchenko
0 siblings, 1 reply; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-02 10:19 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
вт, 2 черв. 2026 р. о 11:09 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
>
> On Mon, Jun 01, 2026 at 06:18:28PM +0300, Svyatoslav Ryhel wrote:
> > Simplify the sysfs logic of the linear property by switching to a macro
> > and a ternary operator.
>
> ...
>
> > if (kstrtoul(buf, 0, &linear))
> > return -EINVAL;
>
> Besides _assign_bits() in the below, side note here to unshadow error codes:
>
> ret = kstrtoul(buf, 0, &linear);
> if (ret)
> return ret;
>
> (obviously in a separate change).
Won't happen in this patches.
>
> ...
>
> > ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> > - mask, val);
> > + CTRLBANK_AB_BCONF_MODE(id),
> > + linear ? CTRLBANK_AB_BCONF_MODE(id) : 0);
> > if (ret)
> > return ret;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic
2026-06-02 10:19 ` Svyatoslav Ryhel
@ 2026-06-02 11:07 ` Andy Shevchenko
2026-06-02 11:59 ` Svyatoslav Ryhel
0 siblings, 1 reply; 47+ messages in thread
From: Andy Shevchenko @ 2026-06-02 11:07 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
On Tue, Jun 02, 2026 at 01:19:00PM +0300, Svyatoslav Ryhel wrote:
> вт, 2 черв. 2026 р. о 11:09 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > On Mon, Jun 01, 2026 at 06:18:28PM +0300, Svyatoslav Ryhel wrote:
...
> > > if (kstrtoul(buf, 0, &linear))
> > > return -EINVAL;
> >
> > Besides _assign_bits() in the below, side note here to unshadow error codes:
> >
> > ret = kstrtoul(buf, 0, &linear);
> > if (ret)
> > return ret;
> >
> > (obviously in a separate change).
>
> Won't happen in this patches.
You mean both suggestions or you are talking about kstrotoul() only? If it's
only about the latter, it's fine with me, but _assign_bits() makes sense to do
in this patch as you already change the parameters enough to make it better.
...
> > > ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> > > - mask, val);
> > > + CTRLBANK_AB_BCONF_MODE(id),
> > > + linear ? CTRLBANK_AB_BCONF_MODE(id) : 0);
> > > if (ret)
> > > return ret;
^^^ left for the context.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic
2026-06-02 11:07 ` Andy Shevchenko
@ 2026-06-02 11:59 ` Svyatoslav Ryhel
0 siblings, 0 replies; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-02 11:59 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
вт, 2 черв. 2026 р. о 14:07 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
>
> On Tue, Jun 02, 2026 at 01:19:00PM +0300, Svyatoslav Ryhel wrote:
> > вт, 2 черв. 2026 р. о 11:09 Andy Shevchenko <andriy.shevchenko@intel.com> пише:
> > > On Mon, Jun 01, 2026 at 06:18:28PM +0300, Svyatoslav Ryhel wrote:
>
> ...
>
> > > > if (kstrtoul(buf, 0, &linear))
> > > > return -EINVAL;
> > >
> > > Besides _assign_bits() in the below, side note here to unshadow error codes:
> > >
> > > ret = kstrtoul(buf, 0, &linear);
> > > if (ret)
> > > return ret;
> > >
> > > (obviously in a separate change).
> >
> > Won't happen in this patches.
>
> You mean both suggestions or you are talking about kstrotoul() only? If it's
> only about the latter, it's fine with me, but _assign_bits() makes sense to do
> in this patch as you already change the parameters enough to make it better.
>
only kstrotoul()
> ...
>
> > > > ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> > > > - mask, val);
> > > > + CTRLBANK_AB_BCONF_MODE(id),
> > > > + linear ? CTRLBANK_AB_BCONF_MODE(id) : 0);
> > > > if (ret)
> > > > return ret;
>
> ^^^ left for the context.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic
2026-06-01 15:18 ` [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic Svyatoslav Ryhel
2026-06-02 8:09 ` Andy Shevchenko
@ 2026-06-02 13:55 ` Daniel Thompson
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
2 siblings, 0 replies; 47+ messages in thread
From: Daniel Thompson @ 2026-06-02 13:55 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
On Mon, Jun 01, 2026 at 06:18:28PM +0300, Svyatoslav Ryhel wrote:
> Simplify the sysfs logic of the linear property by switching to a macro
> and a ternary operator.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
With the change to regmap_assign_bits():
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org>
Daniel.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Claude review: video: backlight: lm3533_bl: Improve linear sysfs logic
2026-06-01 15:18 ` [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic Svyatoslav Ryhel
2026-06-02 8:09 ` Andy Shevchenko
2026-06-02 13:55 ` Daniel Thompson
@ 2026-06-04 3:59 ` Claude Code Review Bot
2 siblings, 0 replies; 47+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Clean refactoring with the `CTRLBANK_AB_BCONF_MODE(n)` macro and ternary operator. Functionally equivalent.
**No issues.**
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 09/11] video: backlight: lm3533_bl: Set initial mapping mode from DT
2026-06-01 15:18 [PATCH v3 00/11] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
` (7 preceding siblings ...)
2026-06-01 15:18 ` [PATCH v3 08/11] video: backlight: lm3533_bl: Improve linear sysfs logic Svyatoslav Ryhel
@ 2026-06-01 15:18 ` Svyatoslav Ryhel
2026-06-02 14:05 ` Daniel Thompson
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
2026-06-01 15:18 ` [PATCH v3 10/11] video: backlight: lm3533_bl: Implement backlight_scale property Svyatoslav Ryhel
` (2 subsequent siblings)
11 siblings, 2 replies; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-01 15:18 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Svyatoslav Ryhel
Cc: Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
Add support to obtain the initial mapping mode from DT instead of leaving
it unconfigured.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/video/backlight/lm3533_bl.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
index 36e6f027613a..f0d88b7bc229 100644
--- a/drivers/video/backlight/lm3533_bl.c
+++ b/drivers/video/backlight/lm3533_bl.c
@@ -34,6 +34,7 @@ struct lm3533_bl {
u32 max_current;
u32 pwm;
+ bool linear;
};
@@ -247,8 +248,15 @@ static struct attribute_group lm3533_bl_attribute_group = {
static int lm3533_bl_setup(struct lm3533_bl *bl)
{
+ int id = lm3533_bl_get_ctrlbank_id(bl);
int ret;
+ ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
+ CTRLBANK_AB_BCONF_MODE(id),
+ bl->linear ? CTRLBANK_AB_BCONF_MODE(id) : 0);
+ if (ret)
+ return ret;
+
ret = lm3533_ctrlbank_set_max_current(&bl->cb, bl->max_current);
if (ret)
return ret;
@@ -299,6 +307,9 @@ static int lm3533_bl_probe(struct platform_device *pdev)
device_property_read_u32(&pdev->dev, "default-brightness",
&props.brightness);
+ bl->linear = device_property_read_bool(&pdev->dev,
+ "ti,linear-mapping-mode");
+
bd = devm_backlight_device_register(&pdev->dev, name, &pdev->dev,
bl, &lm3533_bl_ops, &props);
if (IS_ERR(bd)) {
--
2.51.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 09/11] video: backlight: lm3533_bl: Set initial mapping mode from DT
2026-06-01 15:18 ` [PATCH v3 09/11] video: backlight: lm3533_bl: Set initial mapping mode from DT Svyatoslav Ryhel
@ 2026-06-02 14:05 ` Daniel Thompson
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
1 sibling, 0 replies; 47+ messages in thread
From: Daniel Thompson @ 2026-06-02 14:05 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
On Mon, Jun 01, 2026 at 06:18:29PM +0300, Svyatoslav Ryhel wrote:
> Add support to obtain the initial mapping mode from DT instead of leaving
> it unconfigured.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> drivers/video/backlight/lm3533_bl.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> index 36e6f027613a..f0d88b7bc229 100644
> --- a/drivers/video/backlight/lm3533_bl.c
> +++ b/drivers/video/backlight/lm3533_bl.c
> @@ -34,6 +34,7 @@ struct lm3533_bl {
>
> u32 max_current;
> u32 pwm;
> + bool linear;
> };
>
>
> @@ -247,8 +248,15 @@ static struct attribute_group lm3533_bl_attribute_group = {
>
> static int lm3533_bl_setup(struct lm3533_bl *bl)
> {
> + int id = lm3533_bl_get_ctrlbank_id(bl);
> int ret;
>
> + ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> + CTRLBANK_AB_BCONF_MODE(id),
> + bl->linear ? CTRLBANK_AB_BCONF_MODE(id) : 0);
I guess this is another candidate for regmap_assign_bits() but with that
change:
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org>
Daniel.
^ permalink raw reply [flat|nested] 47+ messages in thread* Claude review: video: backlight: lm3533_bl: Set initial mapping mode from DT
2026-06-01 15:18 ` [PATCH v3 09/11] video: backlight: lm3533_bl: Set initial mapping mode from DT Svyatoslav Ryhel
2026-06-02 14:05 ` Daniel Thompson
@ 2026-06-04 3:59 ` Claude Code Review Bot
1 sibling, 0 replies; 47+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Adds `ti,linear-mapping-mode` boolean property reading and applies the mode in `lm3533_bl_setup` before registering the sysfs group. The property reading is done before backlight device registration, which is correct for the initial state.
**No issues.**
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 10/11] video: backlight: lm3533_bl: Implement backlight_scale property
2026-06-01 15:18 [PATCH v3 00/11] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
` (8 preceding siblings ...)
2026-06-01 15:18 ` [PATCH v3 09/11] video: backlight: lm3533_bl: Set initial mapping mode from DT Svyatoslav Ryhel
@ 2026-06-01 15:18 ` Svyatoslav Ryhel
2026-06-02 14:06 ` Daniel Thompson
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
2026-06-01 15:18 ` [PATCH v3 11/11] video: leds: backlight: lm3533: Support getting LED sources from DT Svyatoslav Ryhel
2026-06-04 3:59 ` Claude review: mfd: lm3533: convert to OF bindings, improve support Claude Code Review Bot
11 siblings, 2 replies; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-01 15:18 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Svyatoslav Ryhel
Cc: Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
Since the device supports linear and non-linear modes, implement the
backlight_scale property to describe this state.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/video/backlight/lm3533_bl.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
index f0d88b7bc229..945625c54fc4 100644
--- a/drivers/video/backlight/lm3533_bl.c
+++ b/drivers/video/backlight/lm3533_bl.c
@@ -161,6 +161,7 @@ static ssize_t store_linear(struct device *dev,
const char *buf, size_t len)
{
struct lm3533_bl *bl = dev_get_drvdata(dev);
+ struct backlight_device *bd = bl->bd;
int id = lm3533_bl_get_ctrlbank_id(bl);
unsigned long linear;
int ret;
@@ -174,6 +175,9 @@ static ssize_t store_linear(struct device *dev,
if (ret)
return ret;
+ bd->props.scale = linear ? BACKLIGHT_SCALE_LINEAR :
+ BACKLIGHT_SCALE_NON_LINEAR;
+
return len;
}
@@ -309,6 +313,8 @@ static int lm3533_bl_probe(struct platform_device *pdev)
bl->linear = device_property_read_bool(&pdev->dev,
"ti,linear-mapping-mode");
+ props.scale = bl->linear ? BACKLIGHT_SCALE_LINEAR :
+ BACKLIGHT_SCALE_NON_LINEAR;
bd = devm_backlight_device_register(&pdev->dev, name, &pdev->dev,
bl, &lm3533_bl_ops, &props);
--
2.51.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 10/11] video: backlight: lm3533_bl: Implement backlight_scale property
2026-06-01 15:18 ` [PATCH v3 10/11] video: backlight: lm3533_bl: Implement backlight_scale property Svyatoslav Ryhel
@ 2026-06-02 14:06 ` Daniel Thompson
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
1 sibling, 0 replies; 47+ messages in thread
From: Daniel Thompson @ 2026-06-02 14:06 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
On Mon, Jun 01, 2026 at 06:18:30PM +0300, Svyatoslav Ryhel wrote:
> Since the device supports linear and non-linear modes, implement the
> backlight_scale property to describe this state.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Thanks!
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org>
Daniel.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Claude review: video: backlight: lm3533_bl: Implement backlight_scale property
2026-06-01 15:18 ` [PATCH v3 10/11] video: backlight: lm3533_bl: Implement backlight_scale property Svyatoslav Ryhel
2026-06-02 14:06 ` Daniel Thompson
@ 2026-06-04 3:59 ` Claude Code Review Bot
1 sibling, 0 replies; 47+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Sets `props.scale` during probe and updates it in the `store_linear` sysfs handler. Clean integration with the backlight framework.
**No issues.**
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 11/11] video: leds: backlight: lm3533: Support getting LED sources from DT
2026-06-01 15:18 [PATCH v3 00/11] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
` (9 preceding siblings ...)
2026-06-01 15:18 ` [PATCH v3 10/11] video: backlight: lm3533_bl: Implement backlight_scale property Svyatoslav Ryhel
@ 2026-06-01 15:18 ` Svyatoslav Ryhel
2026-06-04 3:59 ` Claude review: " Claude Code Review Bot
2026-06-04 3:59 ` Claude review: mfd: lm3533: convert to OF bindings, improve support Claude Code Review Bot
11 siblings, 1 reply; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-06-01 15:18 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Svyatoslav Ryhel
Cc: Johan Hovold, dri-devel, linux-leds, devicetree, linux-kernel,
linux-iio, linux-fbdev
Add Control Bank to HVLED/LVLED muxing support based on the led-sources
defined in the device tree.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/leds/leds-lm3533.c | 55 ++++++++++++++++++++++++++++-
drivers/video/backlight/lm3533_bl.c | 40 ++++++++++++++++++++-
2 files changed, 93 insertions(+), 2 deletions(-)
diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
index f2502daf450d..6db1c1fd2e20 100644
--- a/drivers/leds/leds-lm3533.c
+++ b/drivers/leds/leds-lm3533.c
@@ -7,6 +7,7 @@
* Author: Johan Hovold <jhovold@gmail.com>
*/
+#include <linux/bits.h>
#include <linux/module.h>
#include <linux/leds.h>
#include <linux/mfd/core.h>
@@ -27,6 +28,12 @@
#define LM3533_ALS_CHANNEL_LV_MIN 1
#define LM3533_ALS_CHANNEL_LV_MAX 2
+#define LM3533_REG_OUTPUT_CONF1 0x10
+#define OUTPUT_CONF1_MASK GENMASK(7, 2)
+#define OUTPUT_CONF1_SHIFT 2
+#define LM3533_REG_OUTPUT_CONF2 0x11
+#define OUTPUT_CONF2_MASK GENMASK(3, 0)
+#define OUTPUT_CONF2_SHIFT 6
#define LM3533_REG_CTRLBANK_BCONF_BASE 0x1b
#define LM3533_REG_PATTERN_ENABLE 0x28
#define LM3533_REG_PATTERN_LOW_TIME_BASE 0x71
@@ -54,6 +61,9 @@ struct lm3533_led {
u32 max_current;
u32 pwm;
+
+ int num_leds;
+ u32 leds[LM3533_LVCTRLBANK_MAX];
};
@@ -641,7 +651,33 @@ static const struct attribute_group *lm3533_led_attribute_groups[] = {
static int lm3533_led_setup(struct lm3533_led *led)
{
- int ret;
+ u32 output_cfg_shift = 0;
+ u32 output_cfg_val = 0;
+ int ret, i;
+
+ if (led->num_leds) {
+ for (i = 0; i < led->num_leds; i++) {
+ if (led->leds[i] >= LM3533_LVCTRLBANK_MAX)
+ continue;
+
+ output_cfg_shift = led->leds[i] * 2;
+ output_cfg_val |= led->id << output_cfg_shift;
+ }
+
+ /* LVLED1, LVLED2 and LVLED3 */
+ ret = regmap_update_bits(led->lm3533->regmap, LM3533_REG_OUTPUT_CONF1,
+ OUTPUT_CONF1_MASK,
+ output_cfg_val << OUTPUT_CONF1_SHIFT);
+ if (ret)
+ return ret;
+
+ /* LVLED4 and LVLED5 */
+ ret = regmap_update_bits(led->lm3533->regmap, LM3533_REG_OUTPUT_CONF2,
+ OUTPUT_CONF2_MASK,
+ output_cfg_val >> OUTPUT_CONF2_SHIFT);
+ if (ret)
+ return ret;
+ }
ret = lm3533_ctrlbank_set_max_current(&led->cb, led->max_current);
if (ret)
@@ -716,6 +752,23 @@ static int lm3533_led_probe(struct platform_device *pdev)
led->pwm = 0;
device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);
+ led->num_leds = device_property_count_u32(&pdev->dev, "led-sources");
+
+ /*
+ * If led-sources property is not set then either this Control Bank uses
+ * its default LVLED or is not linked to any LVLED at all.
+ */
+ if (led->num_leds > 0 && led->num_leds <= LM3533_LVCTRLBANK_MAX) {
+ ret = device_property_read_u32_array(&pdev->dev, "led-sources",
+ led->leds, led->num_leds);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to get led-sources\n");
+ goto err_deregister;
+ }
+ } else {
+ led->num_leds = 0;
+ }
+
ret = lm3533_led_setup(led);
if (ret)
goto err_deregister;
diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
index 945625c54fc4..2119433bae45 100644
--- a/drivers/video/backlight/lm3533_bl.c
+++ b/drivers/video/backlight/lm3533_bl.c
@@ -7,6 +7,7 @@
* Author: Johan Hovold <jhovold@gmail.com>
*/
+#include <linux/bits.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/mod_devicetable.h>
@@ -22,6 +23,8 @@
#define LM3533_HVCTRLBANK_COUNT 2
#define LM3533_BL_MAX_BRIGHTNESS 255
+#define LM3533_REG_OUTPUT_CONF1 0x10
+#define OUTPUT_CONF1_MASK GENMASK(1, 0)
#define LM3533_REG_CTRLBANK_AB_BCONF 0x1a
#define CTRLBANK_AB_BCONF_MODE(n) BIT(2 * (n) + 1)
@@ -35,6 +38,9 @@ struct lm3533_bl {
u32 max_current;
u32 pwm;
bool linear;
+
+ int num_leds;
+ u32 led_strings[LM3533_HVCTRLBANK_COUNT];
};
@@ -253,7 +259,8 @@ static struct attribute_group lm3533_bl_attribute_group = {
static int lm3533_bl_setup(struct lm3533_bl *bl)
{
int id = lm3533_bl_get_ctrlbank_id(bl);
- int ret;
+ u32 output_cfg_val = 0;
+ int ret, i;
ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
CTRLBANK_AB_BCONF_MODE(id),
@@ -261,6 +268,20 @@ static int lm3533_bl_setup(struct lm3533_bl *bl)
if (ret)
return ret;
+ if (bl->num_leds) {
+ for (i = 0; i < bl->num_leds; i++) {
+ if (bl->led_strings[i] >= LM3533_HVCTRLBANK_COUNT)
+ continue;
+
+ output_cfg_val |= id << bl->led_strings[i];
+ }
+
+ ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_OUTPUT_CONF1,
+ OUTPUT_CONF1_MASK, output_cfg_val);
+ if (ret)
+ return ret;
+ }
+
ret = lm3533_ctrlbank_set_max_current(&bl->cb, bl->max_current);
if (ret)
return ret;
@@ -336,6 +357,23 @@ static int lm3533_bl_probe(struct platform_device *pdev)
bl->pwm = 0;
device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &bl->pwm);
+ bl->num_leds = device_property_count_u32(&pdev->dev, "led-sources");
+
+ /*
+ * If led-sources property is not set then either this Control Bank uses
+ * its default HVLED or is not linked to any HVLED at all.
+ */
+ if (bl->num_leds > 0 && bl->num_leds <= LM3533_HVCTRLBANK_COUNT) {
+ ret = device_property_read_u32_array(&pdev->dev, "led-sources",
+ bl->led_strings,
+ bl->num_leds);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "failed to get led-sources\n");
+ } else {
+ bl->num_leds = 0;
+ }
+
ret = lm3533_bl_setup(bl);
if (ret)
return ret;
--
2.51.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* Claude review: video: leds: backlight: lm3533: Support getting LED sources from DT
2026-06-01 15:18 ` [PATCH v3 11/11] video: leds: backlight: lm3533: Support getting LED sources from DT Svyatoslav Ryhel
@ 2026-06-04 3:59 ` Claude Code Review Bot
0 siblings, 0 replies; 47+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:59 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Bug: Wide mask clobbers other banks' LED source configurations.**
In `lm3533_bl_setup`:
```c
ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_OUTPUT_CONF1,
OUTPUT_CONF1_MASK, output_cfg_val);
```
`OUTPUT_CONF1_MASK = GENMASK(1, 0) = 0x03` covers BOTH HVLED1 and HVLED2 fields. If backlight bank B only configures HVLED1 (led-sources = <0>), the mask still writes bit 1 as 0, resetting HVLED2's assignment to bank A — even if bank A had previously configured HVLED2 differently. The same issue exists on the LED side:
```c
ret = regmap_update_bits(led->lm3533->regmap, LM3533_REG_OUTPUT_CONF1,
OUTPUT_CONF1_MASK,
output_cfg_val << OUTPUT_CONF1_SHIFT);
```
`OUTPUT_CONF1_MASK = GENMASK(7, 2)` covers all three LVLED1/2/3 fields. Configuring one LVLED clobbers the other two. The original `lm3533_set_hvled_config` and `lm3533_set_lvled_config` correctly computed per-LED masks to avoid this. The fix is to build the mask dynamically alongside the value:
```c
for (i = 0; i < bl->num_leds; i++) {
if (bl->led_strings[i] >= LM3533_HVCTRLBANK_COUNT)
continue;
output_cfg_mask |= LM3533_BL_ID_MASK << bl->led_strings[i];
output_cfg_val |= id << bl->led_strings[i];
}
ret = regmap_update_bits(..., output_cfg_mask, output_cfg_val);
```
**Possible undefined symbol:** `LM3533_LVCTRLBANK_MAX` is used in `leds-lm3533.c` for the `leds[]` array size and bounds check, but I don't see its definition in any patch diff. Verify this compiles — it may need to be added to the header or as a local define.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 47+ messages in thread
* Claude review: mfd: lm3533: convert to OF bindings, improve support
2026-06-01 15:18 [PATCH v3 00/11] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
` (10 preceding siblings ...)
2026-06-01 15:18 ` [PATCH v3 11/11] video: leds: backlight: lm3533: Support getting LED sources from DT Svyatoslav Ryhel
@ 2026-06-04 3:59 ` Claude Code Review Bot
11 siblings, 0 replies; 47+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 3:59 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: mfd: lm3533: convert to OF bindings, improve support
Author: Svyatoslav Ryhel <clamor95@gmail.com>
Patches: 34
Reviewed: 2026-06-04T13:59:53.443885
---
This is a v3 series converting the LM3533 MFD driver from platform data to Device Tree bindings, adding VIN supply, DMA mask, mapping mode, and LED source configuration. The overall direction is good and the DT binding schemas are well-structured. However, there are several bugs that need to be fixed:
1. **Patch 2 has a regmap_update_bits argument swap bug** in one call site that would break ALS interrupt disable.
2. **Patch 5 has a DT property name mismatch** between the binding schema and driver code for the ALS resistor property.
3. **Patch 11 has a wide-mask bug** in the output config register writes that will clobber other banks' LED source assignments.
The series needs another spin to fix these issues before it can be merged.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 2/6] mfd: lm3533: Convert to use OF bindings
2026-05-17 7:43 [PATCH v1 0/6] " Svyatoslav Ryhel
@ 2026-05-17 7:43 ` Svyatoslav Ryhel
2026-05-18 6:19 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 47+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-17 7:43 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Helge Deller,
Svyatoslav Ryhel
Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-iio,
linux-fbdev
Since there are no users of this driver via platform data, remove the
platform data support and switch to using Device Tree bindings.
Additionally, optimize functions used only by platform data.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/iio/light/lm3533-als.c | 123 +++++--------
drivers/leds/leds-lm3533.c | 60 ++++---
drivers/mfd/lm3533-core.c | 257 +++++++++-------------------
drivers/video/backlight/lm3533_bl.c | 52 ++++--
include/linux/mfd/lm3533.h | 51 +-----
5 files changed, 202 insertions(+), 341 deletions(-)
diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
index 99f0b903018c..853abb96e13f 100644
--- a/drivers/iio/light/lm3533-als.c
+++ b/drivers/iio/light/lm3533-als.c
@@ -16,15 +16,18 @@
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/mfd/core.h>
+#include <linux/mod_devicetable.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/slab.h>
#include <linux/uaccess.h>
+#include <linux/units.h>
#include <linux/mfd/lm3533.h>
-#define LM3533_ALS_RESISTOR_MIN 1
-#define LM3533_ALS_RESISTOR_MAX 127
+#define LM3533_ALS_RESISTOR_MIN 1575
+#define LM3533_ALS_RESISTOR_MAX 200000
#define LM3533_ALS_CHANNEL_CURRENT_MAX 2
#define LM3533_ALS_THRESH_MAX 3
#define LM3533_ALS_ZONE_MAX 4
@@ -56,6 +59,9 @@ struct lm3533_als {
atomic_t zone;
struct mutex thresh_mutex;
+
+ bool pwm_mode;
+ u32 r_select;
};
@@ -714,64 +720,6 @@ static const struct attribute_group lm3533_als_attribute_group = {
.attrs = lm3533_als_attributes
};
-static int lm3533_als_set_input_mode(struct lm3533_als *als, bool pwm_mode)
-{
- u8 mask = LM3533_ALS_INPUT_MODE_MASK;
- u8 val;
- int ret;
-
- if (pwm_mode)
- val = mask; /* pwm input */
- else
- val = 0; /* analog input */
-
- ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, val, mask);
- if (ret) {
- dev_err(&als->pdev->dev, "failed to set input mode %d\n",
- pwm_mode);
- return ret;
- }
-
- return 0;
-}
-
-static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
-{
- int ret;
-
- if (val < LM3533_ALS_RESISTOR_MIN || val > LM3533_ALS_RESISTOR_MAX) {
- dev_err(&als->pdev->dev, "invalid resistor value\n");
- return -EINVAL;
- }
-
- ret = lm3533_write(als->lm3533, LM3533_REG_ALS_RESISTOR_SELECT, val);
- if (ret) {
- dev_err(&als->pdev->dev, "failed to set resistor\n");
- return ret;
- }
-
- return 0;
-}
-
-static int lm3533_als_setup(struct lm3533_als *als,
- const struct lm3533_als_platform_data *pdata)
-{
- int ret;
-
- ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
- if (ret)
- return ret;
-
- /* ALS input is always high impedance in PWM-mode. */
- if (!pdata->pwm_mode) {
- ret = lm3533_als_set_resistor(als, pdata->r_select);
- if (ret)
- return ret;
- }
-
- return 0;
-}
-
static int lm3533_als_setup_irq(struct lm3533_als *als, void *dev)
{
u8 mask = LM3533_ALS_INT_ENABLE_MASK;
@@ -784,7 +732,8 @@ static int lm3533_als_setup_irq(struct lm3533_als *als, void *dev)
return ret;
}
- ret = request_threaded_irq(als->irq, NULL, lm3533_als_isr,
+ ret = devm_request_threaded_irq(&als->pdev->dev, als->irq, NULL,
+ lm3533_als_isr,
IRQF_TRIGGER_LOW | IRQF_ONESHOT,
dev_name(&als->pdev->dev), dev);
if (ret) {
@@ -828,7 +777,6 @@ static const struct iio_info lm3533_als_info = {
static int lm3533_als_probe(struct platform_device *pdev)
{
- const struct lm3533_als_platform_data *pdata;
struct lm3533 *lm3533;
struct lm3533_als *als;
struct iio_dev *indio_dev;
@@ -838,12 +786,6 @@ static int lm3533_als_probe(struct platform_device *pdev)
if (!lm3533)
return -EINVAL;
- pdata = dev_get_platdata(&pdev->dev);
- if (!pdata) {
- dev_err(&pdev->dev, "no platform data\n");
- return -EINVAL;
- }
-
indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*als));
if (!indio_dev)
return -ENOMEM;
@@ -852,31 +794,52 @@ static int lm3533_als_probe(struct platform_device *pdev)
indio_dev->channels = lm3533_als_channels;
indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
indio_dev->name = dev_name(&pdev->dev);
- iio_device_set_parent(indio_dev, pdev->dev.parent);
indio_dev->modes = INDIO_DIRECT_MODE;
als = iio_priv(indio_dev);
als->lm3533 = lm3533;
als->pdev = pdev;
- als->irq = lm3533->irq;
+ als->irq = platform_get_irq_optional(pdev, 0);
atomic_set(&als->zone, 0);
mutex_init(&als->thresh_mutex);
platform_set_drvdata(pdev, indio_dev);
- if (als->irq) {
+ if (als->irq > 0) {
ret = lm3533_als_setup_irq(als, indio_dev);
if (ret)
return ret;
}
- ret = lm3533_als_setup(als, pdata);
+ device_property_read_u32(&pdev->dev, "ti,resistor-value-ohm",
+ &als->r_select);
+
+ als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
+ LM3533_ALS_RESISTOR_MAX);
+ als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
+
+ als->pwm_mode = device_property_read_bool(&pdev->dev, "ti,pwm-mode");
+
+ ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, als->pwm_mode ?
+ LM3533_ALS_INPUT_MODE_MASK : 0,
+ LM3533_ALS_INPUT_MODE_MASK);
if (ret)
- goto err_free_irq;
+ return dev_err_probe(&pdev->dev, ret,
+ "failed to set input mode %d\n",
+ als->pwm_mode);
+
+ /* ALS input is always high impedance in PWM-mode. */
+ if (!als->pwm_mode) {
+ ret = lm3533_write(lm3533, LM3533_REG_ALS_RESISTOR_SELECT,
+ (u8)als->r_select);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "failed to set resistor\n");
+ }
ret = lm3533_als_enable(als);
if (ret)
- goto err_free_irq;
+ return ret;
ret = iio_device_register(indio_dev);
if (ret) {
@@ -888,9 +851,6 @@ static int lm3533_als_probe(struct platform_device *pdev)
err_disable:
lm3533_als_disable(als);
-err_free_irq:
- if (als->irq)
- free_irq(als->irq, indio_dev);
return ret;
}
@@ -903,13 +863,18 @@ static void lm3533_als_remove(struct platform_device *pdev)
lm3533_als_set_int_mode(indio_dev, false);
iio_device_unregister(indio_dev);
lm3533_als_disable(als);
- if (als->irq)
- free_irq(als->irq, indio_dev);
}
+static const struct of_device_id lm3533_als_match_table[] = {
+ { .compatible = "ti,lm3533-als" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, lm3533_als_match_table);
+
static struct platform_driver lm3533_als_driver = {
.driver = {
.name = "lm3533-als",
+ .of_match_table = lm3533_als_match_table,
},
.probe = lm3533_als_probe,
.remove = lm3533_als_remove,
diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
index 45795f2a1042..f6345bc1f443 100644
--- a/drivers/leds/leds-lm3533.c
+++ b/drivers/leds/leds-lm3533.c
@@ -10,8 +10,10 @@
#include <linux/module.h>
#include <linux/leds.h>
#include <linux/mfd/core.h>
+#include <linux/mod_devicetable.h>
#include <linux/mutex.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/slab.h>
#include <linux/mfd/lm3533.h>
@@ -48,6 +50,9 @@ struct lm3533_led {
struct mutex mutex;
unsigned long flags;
+
+ u32 max_current;
+ u32 pwm;
};
@@ -632,22 +637,20 @@ static const struct attribute_group *lm3533_led_attribute_groups[] = {
NULL
};
-static int lm3533_led_setup(struct lm3533_led *led,
- struct lm3533_led_platform_data *pdata)
+static int lm3533_led_setup(struct lm3533_led *led)
{
int ret;
- ret = lm3533_ctrlbank_set_max_current(&led->cb, pdata->max_current);
+ ret = lm3533_ctrlbank_set_max_current(&led->cb, led->max_current);
if (ret)
return ret;
- return lm3533_ctrlbank_set_pwm(&led->cb, pdata->pwm);
+ return lm3533_ctrlbank_set_pwm(&led->cb, led->pwm);
}
static int lm3533_led_probe(struct platform_device *pdev)
{
struct lm3533 *lm3533;
- struct lm3533_led_platform_data *pdata;
struct lm3533_led *led;
int ret;
@@ -657,30 +660,30 @@ static int lm3533_led_probe(struct platform_device *pdev)
if (!lm3533)
return -EINVAL;
- pdata = dev_get_platdata(&pdev->dev);
- if (!pdata) {
- dev_err(&pdev->dev, "no platform data\n");
- return -EINVAL;
- }
-
- if (pdev->id < 0 || pdev->id >= LM3533_LVCTRLBANK_COUNT) {
- dev_err(&pdev->dev, "illegal LED id %d\n", pdev->id);
- return -EINVAL;
- }
+ if (pdev->id < LM3533_LVCTRLBANK_MIN || pdev->id > LM3533_LVCTRLBANK_MAX)
+ return dev_err_probe(&pdev->dev, -EINVAL,
+ "illegal LED id %d\n", pdev->id);
led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
if (!led)
return -ENOMEM;
led->lm3533 = lm3533;
- led->cdev.name = pdata->name;
- led->cdev.default_trigger = pdata->default_trigger;
led->cdev.brightness_set_blocking = lm3533_led_set;
led->cdev.brightness_get = lm3533_led_get;
led->cdev.blink_set = lm3533_led_blink_set;
led->cdev.brightness = LED_OFF;
led->cdev.groups = lm3533_led_attribute_groups;
- led->id = pdev->id;
+ led->id = pdev->id - LM3533_LVCTRLBANK_MIN;
+
+ led->cdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
+ pdev->name, led->id);
+ if (!led->cdev.name)
+ return -ENOMEM;
+
+ led->cdev.default_trigger = "none";
+ device_property_read_string(&pdev->dev, "linux,default-trigger",
+ &led->cdev.default_trigger);
mutex_init(&led->mutex);
@@ -694,15 +697,23 @@ static int lm3533_led_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, led);
- ret = led_classdev_register(pdev->dev.parent, &led->cdev);
+ ret = led_classdev_register(&pdev->dev, &led->cdev);
if (ret) {
- dev_err(&pdev->dev, "failed to register LED %d\n", pdev->id);
+ dev_err(&pdev->dev, "failed to register LED %d\n", led->id);
return ret;
}
led->cb.dev = led->cdev.dev;
- ret = lm3533_led_setup(led, pdata);
+ device_property_read_u32(&pdev->dev, "led-max-microamp",
+ &led->max_current);
+ led->max_current = clamp(led->max_current, LM3533_LED_MAX_CURRENT_MIN,
+ LM3533_LED_MAX_CURRENT_MAX);
+
+ led->pwm = 0;
+ device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);
+
+ ret = lm3533_led_setup(led);
if (ret)
goto err_deregister;
@@ -739,9 +750,16 @@ static void lm3533_led_shutdown(struct platform_device *pdev)
lm3533_led_set(&led->cdev, LED_OFF); /* disable blink */
}
+static const struct of_device_id lm3533_led_match_table[] = {
+ { .compatible = "ti,lm3533-leds" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, lm3533_led_match_table);
+
static struct platform_driver lm3533_led_driver = {
.driver = {
.name = "lm3533-leds",
+ .of_match_table = lm3533_led_match_table,
},
.probe = lm3533_led_probe,
.remove = lm3533_led_remove,
diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
index 0a2409d00b2e..83ebd780f39d 100644
--- a/drivers/mfd/lm3533-core.c
+++ b/drivers/mfd/lm3533-core.c
@@ -14,19 +14,26 @@
#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/mfd/core.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
#include <linux/regmap.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
#include <linux/uaccess.h>
+#include <linux/units.h>
#include <linux/mfd/lm3533.h>
#define LM3533_BOOST_OVP_MASK 0x06
#define LM3533_BOOST_OVP_SHIFT 1
+#define LM3533_BOOST_OVP_MIN 16000000
+#define LM3533_BOOST_OVP_MAX 40000000
#define LM3533_BOOST_FREQ_MASK 0x01
#define LM3533_BOOST_FREQ_SHIFT 0
+#define LM3533_BOOST_FREQ_MIN 500000
+#define LM3533_BOOST_FREQ_MAX 1000000
#define LM3533_BL_ID_MASK 1
#define LM3533_LED_ID_MASK 3
@@ -42,42 +49,14 @@
#define LM3533_REG_MAX 0xb2
-
-static struct mfd_cell lm3533_als_devs[] = {
- {
- .name = "lm3533-als",
- .id = -1,
- },
-};
-
-static struct mfd_cell lm3533_bl_devs[] = {
- {
- .name = "lm3533-backlight",
- .id = 0,
- },
- {
- .name = "lm3533-backlight",
- .id = 1,
- },
-};
-
-static struct mfd_cell lm3533_led_devs[] = {
- {
- .name = "lm3533-leds",
- .id = 0,
- },
- {
- .name = "lm3533-leds",
- .id = 1,
- },
- {
- .name = "lm3533-leds",
- .id = 2,
- },
- {
- .name = "lm3533-leds",
- .id = 3,
- },
+static struct mfd_cell lm3533_child_devices[] = {
+ MFD_CELL_OF("lm3533-als", NULL, NULL, 0, 0, "ti,lm3533-als"),
+ MFD_CELL_OF_REG("lm3533-backlight", NULL, NULL, 0, 0, "ti,lm3533-backlight", 0),
+ MFD_CELL_OF_REG("lm3533-backlight", NULL, NULL, 0, 1, "ti,lm3533-backlight", 1),
+ MFD_CELL_OF_REG("lm3533-leds", NULL, NULL, 0, 0, "ti,lm3533-leds", 2),
+ MFD_CELL_OF_REG("lm3533-leds", NULL, NULL, 0, 1, "ti,lm3533-leds", 3),
+ MFD_CELL_OF_REG("lm3533-leds", NULL, NULL, 0, 2, "ti,lm3533-leds", 4),
+ MFD_CELL_OF_REG("lm3533-leds", NULL, NULL, 0, 3, "ti,lm3533-leds", 5),
};
int lm3533_read(struct lm3533 *lm3533, u8 reg, u8 *val)
@@ -132,35 +111,6 @@ int lm3533_update(struct lm3533 *lm3533, u8 reg, u8 val, u8 mask)
}
EXPORT_SYMBOL_GPL(lm3533_update);
-static int lm3533_set_boost_freq(struct lm3533 *lm3533,
- enum lm3533_boost_freq freq)
-{
- int ret;
-
- ret = lm3533_update(lm3533, LM3533_REG_BOOST_PWM,
- freq << LM3533_BOOST_FREQ_SHIFT,
- LM3533_BOOST_FREQ_MASK);
- if (ret)
- dev_err(lm3533->dev, "failed to set boost frequency\n");
-
- return ret;
-}
-
-
-static int lm3533_set_boost_ovp(struct lm3533 *lm3533,
- enum lm3533_boost_ovp ovp)
-{
- int ret;
-
- ret = lm3533_update(lm3533, LM3533_REG_BOOST_PWM,
- ovp << LM3533_BOOST_OVP_SHIFT,
- LM3533_BOOST_OVP_MASK);
- if (ret)
- dev_err(lm3533->dev, "failed to set boost ovp\n");
-
- return ret;
-}
-
/*
* HVLED output config -- output hvled controlled by backlight bl
*/
@@ -376,136 +326,45 @@ static struct attribute_group lm3533_attribute_group = {
.attrs = lm3533_attributes
};
-static int lm3533_device_als_init(struct lm3533 *lm3533)
+static int lm3533_device_init(struct lm3533 *lm3533)
{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
+ struct device *dev = lm3533->dev;
int ret;
- if (!pdata->als)
- return 0;
-
- lm3533_als_devs[0].platform_data = pdata->als;
- lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
+ lm3533_enable(lm3533);
- ret = mfd_add_devices(lm3533->dev, 0, lm3533_als_devs, 1, NULL,
- 0, NULL);
+ ret = lm3533_update(lm3533, LM3533_REG_BOOST_PWM,
+ lm3533->boost_freq << LM3533_BOOST_FREQ_SHIFT,
+ LM3533_BOOST_FREQ_MASK);
if (ret) {
- dev_err(lm3533->dev, "failed to add ALS device\n");
- return ret;
- }
-
- lm3533->have_als = 1;
-
- return 0;
-}
-
-static int lm3533_device_bl_init(struct lm3533 *lm3533)
-{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
- int i;
- int ret;
-
- if (!pdata->backlights || pdata->num_backlights == 0)
- 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]);
+ dev_err(dev, "failed to set boost frequency\n");
+ goto err_disable;
}
- ret = mfd_add_devices(lm3533->dev, 0, lm3533_bl_devs,
- pdata->num_backlights, NULL, 0, NULL);
+ ret = lm3533_update(lm3533, LM3533_REG_BOOST_PWM,
+ lm3533->boost_ovp << LM3533_BOOST_OVP_SHIFT,
+ LM3533_BOOST_OVP_MASK);
if (ret) {
- dev_err(lm3533->dev, "failed to add backlight devices\n");
- return ret;
- }
-
- lm3533->have_backlights = 1;
-
- return 0;
-}
-
-static int lm3533_device_led_init(struct lm3533 *lm3533)
-{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
- int i;
- int ret;
-
- if (!pdata->leds || pdata->num_leds == 0)
- 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]);
+ dev_err(dev, "failed to set boost ovp\n");
+ goto err_disable;
}
- ret = mfd_add_devices(lm3533->dev, 0, lm3533_led_devs,
- pdata->num_leds, NULL, 0, NULL);
+ ret = devm_mfd_add_devices(dev, 0, lm3533_child_devices,
+ ARRAY_SIZE(lm3533_child_devices),
+ NULL, 0, NULL);
if (ret) {
- dev_err(lm3533->dev, "failed to add LED devices\n");
- return ret;
- }
-
- lm3533->have_leds = 1;
-
- return 0;
-}
-
-static int lm3533_device_setup(struct lm3533 *lm3533,
- struct lm3533_platform_data *pdata)
-{
- int ret;
-
- ret = lm3533_set_boost_freq(lm3533, pdata->boost_freq);
- if (ret)
- return ret;
-
- return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
-}
-
-static int lm3533_device_init(struct lm3533 *lm3533)
-{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
- int ret;
-
- dev_dbg(lm3533->dev, "%s\n", __func__);
-
- if (!pdata) {
- dev_err(lm3533->dev, "no platform data\n");
- return -EINVAL;
+ dev_err(dev, "failed to add MFD devices: %d\n", ret);
+ goto err_disable;
}
- lm3533->hwen = devm_gpiod_get(lm3533->dev, NULL, GPIOD_OUT_LOW);
- if (IS_ERR(lm3533->hwen))
- return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen), "failed to request HWEN GPIO\n");
- gpiod_set_consumer_name(lm3533->hwen, "lm3533-hwen");
-
- lm3533_enable(lm3533);
-
- ret = lm3533_device_setup(lm3533, pdata);
- if (ret)
+ ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);
+ if (ret) {
+ dev_err(dev, "failed to create sysfs attributes\n");
goto err_disable;
-
- lm3533_device_als_init(lm3533);
- lm3533_device_bl_init(lm3533);
- lm3533_device_led_init(lm3533);
-
- ret = sysfs_create_group(&lm3533->dev->kobj, &lm3533_attribute_group);
- if (ret < 0) {
- dev_err(lm3533->dev, "failed to create sysfs attributes\n");
- goto err_unregister;
}
return 0;
-err_unregister:
- mfd_remove_devices(lm3533->dev);
err_disable:
lm3533_disable(lm3533);
@@ -517,8 +376,6 @@ static void lm3533_device_exit(struct lm3533 *lm3533)
dev_dbg(lm3533->dev, "%s\n", __func__);
sysfs_remove_group(&lm3533->dev->kobj, &lm3533_attribute_group);
-
- mfd_remove_devices(lm3533->dev);
lm3533_disable(lm3533);
}
@@ -589,7 +446,40 @@ static int lm3533_i2c_probe(struct i2c_client *i2c)
return PTR_ERR(lm3533->regmap);
lm3533->dev = &i2c->dev;
- lm3533->irq = i2c->irq;
+
+ lm3533->hwen = devm_gpiod_get_optional(lm3533->dev, "enable",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(lm3533->hwen))
+ return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen),
+ "failed to get HWEN GPIO\n");
+
+ device_property_read_u32(lm3533->dev, "ti,boost-ovp-microvolt",
+ &lm3533->boost_ovp);
+
+ lm3533->boost_ovp = clamp(lm3533->boost_ovp, LM3533_BOOST_OVP_MIN,
+ LM3533_BOOST_OVP_MAX);
+ lm3533->boost_ovp = lm3533->boost_ovp / (8 * MICRO) - 2;
+
+ device_property_read_u32(lm3533->dev, "ti,boost-freq-hz",
+ &lm3533->boost_freq);
+
+ lm3533->boost_freq = clamp(lm3533->boost_freq, LM3533_BOOST_FREQ_MIN,
+ LM3533_BOOST_FREQ_MAX);
+ lm3533->boost_freq = lm3533->boost_freq / (500 * KILO) - 1;
+
+ device_for_each_child_node_scoped(lm3533->dev, child) {
+ if (!fwnode_device_is_available(child))
+ continue;
+
+ if (fwnode_device_is_compatible(child, "ti,lm3533-als"))
+ lm3533->have_als = true;
+
+ if (fwnode_device_is_compatible(child, "ti,lm3533-backlight"))
+ lm3533->have_backlights = true;
+
+ if (fwnode_device_is_compatible(child, "ti,lm3533-leds"))
+ lm3533->have_leds = true;
+ }
return lm3533_device_init(lm3533);
}
@@ -603,6 +493,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 +508,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..42da652df58d 100644
--- a/drivers/video/backlight/lm3533_bl.c
+++ b/drivers/video/backlight/lm3533_bl.c
@@ -9,7 +9,9 @@
#include <linux/module.h>
#include <linux/init.h>
+#include <linux/mod_devicetable.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/backlight.h>
#include <linux/slab.h>
@@ -27,6 +29,9 @@ struct lm3533_bl {
struct lm3533_ctrlbank cb;
struct backlight_device *bd;
int id;
+
+ u32 max_current;
+ u32 pwm;
};
@@ -246,25 +251,24 @@ static struct attribute_group lm3533_bl_attribute_group = {
.attrs = lm3533_bl_attributes
};
-static int lm3533_bl_setup(struct lm3533_bl *bl,
- struct lm3533_bl_platform_data *pdata)
+static int lm3533_bl_setup(struct lm3533_bl *bl)
{
int ret;
- ret = lm3533_ctrlbank_set_max_current(&bl->cb, pdata->max_current);
+ ret = lm3533_ctrlbank_set_max_current(&bl->cb, bl->max_current);
if (ret)
return ret;
- return lm3533_ctrlbank_set_pwm(&bl->cb, pdata->pwm);
+ return lm3533_ctrlbank_set_pwm(&bl->cb, bl->pwm);
}
static int lm3533_bl_probe(struct platform_device *pdev)
{
struct lm3533 *lm3533;
- struct lm3533_bl_platform_data *pdata;
struct lm3533_bl *bl;
struct backlight_device *bd;
struct backlight_properties props;
+ char *name = NULL;
int ret;
dev_dbg(&pdev->dev, "%s\n", __func__);
@@ -273,12 +277,6 @@ static int lm3533_bl_probe(struct platform_device *pdev)
if (!lm3533)
return -EINVAL;
- pdata = dev_get_platdata(&pdev->dev);
- if (!pdata) {
- dev_err(&pdev->dev, "no platform data\n");
- return -EINVAL;
- }
-
if (pdev->id < 0 || pdev->id >= LM3533_HVCTRLBANK_COUNT) {
dev_err(&pdev->dev, "illegal backlight id %d\n", pdev->id);
return -EINVAL;
@@ -295,13 +293,20 @@ static int lm3533_bl_probe(struct platform_device *pdev)
bl->cb.id = lm3533_bl_get_ctrlbank_id(bl);
bl->cb.dev = NULL; /* until registered */
+ name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
+ pdev->name, pdev->id);
+ if (!name)
+ return -ENOMEM;
+
memset(&props, 0, sizeof(props));
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);
+ props.brightness = LM3533_BL_MAX_BRIGHTNESS;
+ device_property_read_u32(&pdev->dev, "default-brightness",
+ &props.brightness);
+
+ bd = devm_backlight_device_register(&pdev->dev, name, &pdev->dev,
+ bl, &lm3533_bl_ops, &props);
if (IS_ERR(bd)) {
dev_err(&pdev->dev, "failed to register backlight device\n");
return PTR_ERR(bd);
@@ -320,7 +325,15 @@ static int lm3533_bl_probe(struct platform_device *pdev)
backlight_update_status(bd);
- ret = lm3533_bl_setup(bl, pdata);
+ device_property_read_u32(&pdev->dev, "led-max-microamp",
+ &bl->max_current);
+ bl->max_current = clamp(bl->max_current, LM3533_LED_MAX_CURRENT_MIN,
+ LM3533_LED_MAX_CURRENT_MAX);
+
+ bl->pwm = 0;
+ device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &bl->pwm);
+
+ ret = lm3533_bl_setup(bl);
if (ret)
goto err_sysfs_remove;
@@ -381,10 +394,17 @@ static void lm3533_bl_shutdown(struct platform_device *pdev)
lm3533_ctrlbank_disable(&bl->cb);
}
+static const struct of_device_id lm3533_bl_match_table[] = {
+ { .compatible = "ti,lm3533-backlight" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, lm3533_bl_match_table);
+
static struct platform_driver lm3533_bl_driver = {
.driver = {
.name = "lm3533-backlight",
.pm = &lm3533_bl_pm_ops,
+ .of_match_table = lm3533_bl_match_table,
},
.probe = lm3533_bl_probe,
.remove = lm3533_bl_remove,
diff --git a/include/linux/mfd/lm3533.h b/include/linux/mfd/lm3533.h
index 69059a7a2ce5..3aa962d4c747 100644
--- a/include/linux/mfd/lm3533.h
+++ b/include/linux/mfd/lm3533.h
@@ -15,6 +15,9 @@
#define LM3533_ATTR_RW(_name) \
DEVICE_ATTR(_name, S_IRUGO | S_IWUSR , show_##_name, store_##_name)
+#define LM3533_LED_MAX_CURRENT_MIN 5000
+#define LM3533_LED_MAX_CURRENT_MAX 29800
+
struct device;
struct gpio_desc;
struct regmap;
@@ -25,7 +28,9 @@ struct lm3533 {
struct regmap *regmap;
struct gpio_desc *hwen;
- int irq;
+
+ u32 boost_ovp;
+ u32 boost_freq;
unsigned have_als:1;
unsigned have_backlights:1;
@@ -38,50 +43,6 @@ struct lm3533_ctrlbank {
int id;
};
-struct lm3533_als_platform_data {
- unsigned pwm_mode:1; /* PWM input mode (default analog) */
- u8 r_select; /* 1 - 127 (ignored in PWM-mode) */
-};
-
-struct lm3533_bl_platform_data {
- char *name;
- u16 max_current; /* 5000 - 29800 uA (800 uA step) */
- u8 default_brightness; /* 0 - 255 */
- u8 pwm; /* 0 - 0x3f */
-};
-
-struct lm3533_led_platform_data {
- char *name;
- const char *default_trigger;
- u16 max_current; /* 5000 - 29800 uA (800 uA step) */
- u8 pwm; /* 0 - 0x3f */
-};
-
-enum lm3533_boost_freq {
- LM3533_BOOST_FREQ_500KHZ,
- LM3533_BOOST_FREQ_1000KHZ,
-};
-
-enum lm3533_boost_ovp {
- LM3533_BOOST_OVP_16V,
- LM3533_BOOST_OVP_24V,
- LM3533_BOOST_OVP_32V,
- LM3533_BOOST_OVP_40V,
-};
-
-struct lm3533_platform_data {
- enum lm3533_boost_ovp boost_ovp;
- enum lm3533_boost_freq boost_freq;
-
- struct lm3533_als_platform_data *als;
-
- struct lm3533_bl_platform_data *backlights;
- int num_backlights;
-
- struct lm3533_led_platform_data *leds;
- int num_leds;
-};
-
extern int lm3533_ctrlbank_enable(struct lm3533_ctrlbank *cb);
extern int lm3533_ctrlbank_disable(struct lm3533_ctrlbank *cb);
--
2.51.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* Claude review: mfd: lm3533: Convert to use OF bindings
2026-05-17 7:43 ` [PATCH v1 2/6] mfd: lm3533: Convert to use OF bindings Svyatoslav Ryhel
@ 2026-05-18 6:19 ` Claude Code Review Bot
0 siblings, 0 replies; 47+ messages in thread
From: Claude Code Review Bot @ 2026-05-18 6:19 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the largest patch and has several bugs:
**Critical — MFD cell ID mismatch**: The LED MFD cells are defined with `cell->id` = 0,1,2,3:
```c
MFD_CELL_OF_REG("lm3533-leds", NULL, NULL, 0, 0, "ti,lm3533-leds", 2),
MFD_CELL_OF_REG("lm3533-leds", NULL, NULL, 0, 1, "ti,lm3533-leds", 3),
MFD_CELL_OF_REG("lm3533-leds", NULL, NULL, 0, 2, "ti,lm3533-leds", 4),
MFD_CELL_OF_REG("lm3533-leds", NULL, NULL, 0, 3, "ti,lm3533-leds", 5),
```
Since `devm_mfd_add_devices` is called with `id=0`, `pdev->id = 0 + cell->id`, giving 0,1,2,3. But in `lm3533_led_probe`, the check is:
```c
if (pdev->id < LM3533_LVCTRLBANK_MIN || pdev->id > LM3533_LVCTRLBANK_MAX)
```
where `LM3533_LVCTRLBANK_MIN=2`. So LED cells with `pdev->id` 0 and 1 will **always fail probe with `-EINVAL`**. And `led->id = pdev->id - LM3533_LVCTRLBANK_MIN` will produce wrong/negative values for cells 0 and 1.
Fix: The cell IDs should be 2,3,4,5 to match the DT reg values.
**DT property name mismatch**: The binding defines `ti,resistor-ohm`, but the driver reads a different name:
```c
device_property_read_u32(&pdev->dev, "ti,resistor-value-ohm",
&als->r_select);
```
This will silently fail to read the property on all boards that use the documented binding name.
**Dead code**: The `have_als`, `have_backlights`, `have_leds` flags are computed by scanning child nodes but never read anywhere in the new code — they're dead stores. Since `devm_mfd_add_devices` always registers all 7 cells regardless of these flags, the iteration and flag-setting should either be removed or used to conditionally register cells.
**Removed `iio_device_set_parent`**: The line `iio_device_set_parent(indio_dev, pdev->dev.parent)` is deleted without comment. This changes the IIO device's parent from the i2c device to the platform device. If this is intentional (for proper OF node association), it should be mentioned in the commit message.
**Minor — `linux,default-trigger` property**: The LEDs driver reads `linux,default-trigger` via `device_property_read_string`, but the DT binding in Patch 1 doesn't document this property (though it inherits from `common.yaml` which may cover it).
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 47+ messages in thread