* [PATCH v2 0/6] mfd: lm3533: convert to OF bindings, improve support
@ 2026-05-28 13:51 Svyatoslav Ryhel
2026-05-28 13:51 ` [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller Svyatoslav Ryhel
` (5 more replies)
0 siblings, 6 replies; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-28 13:51 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
Convert LM3533 to OF bindings, add missing VIN supply, add support for
setting mapping mode and LED sources based on device tree.
---
Changes in v2:
schema
- maximum led sources for leds set to 4
- anyOf > oneOf in ALS
- improved ALS descriptions
- adjusted example
drivers
- dropped devm convertion of irq and mfd helpers
- all als configuration moved into lm3533_als_setup
- added regulator/consumer.h
- lm3533_bl_setup set before sysfs_create_group in backlight
- added check if LVLED is valid
- LM3533_REG_OUTPUT_CONF1 > LM3533_REG_OUTPUT_CONF2 for LVLED4 and LVLED5
---
Svyatoslav Ryhel (6):
dt-bindings: leds: Document TI LM3533 LED controller
mfd: lm3533: Convert to use OF bindings
mfd: lm3533: Add support for VIN power supply
mfd: lm3533: Set DMA mask
video: backlight: lm3533_bl: Set initial mapping mode from DT
video: leds: backlight: lm3533: Support getting LED sources from DT
.../leds/backlight/ti,lm3533-backlight.yaml | 68 ++++
.../bindings/leds/ti,lm3533-leds.yaml | 66 ++++
.../devicetree/bindings/leds/ti,lm3533.yaml | 170 ++++++++++
drivers/iio/light/lm3533-als.c | 95 +++---
drivers/leds/leds-lm3533.c | 108 +++++--
drivers/mfd/lm3533-core.c | 291 +++++++-----------
drivers/video/backlight/lm3533_bl.c | 127 ++++++--
include/linux/mfd/lm3533.h | 53 +---
8 files changed, 653 insertions(+), 325 deletions(-)
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
--
2.51.0
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller
2026-05-28 13:51 [PATCH v2 0/6] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
@ 2026-05-28 13:51 ` Svyatoslav Ryhel
2026-05-28 14:54 ` Jonathan Cameron
2026-05-29 9:51 ` Daniel Thompson
2026-05-28 13:51 ` [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings Svyatoslav Ryhel
` (4 subsequent siblings)
5 siblings, 2 replies; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-28 13:51 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>
---
.../leds/backlight/ti,lm3533-backlight.yaml | 68 +++++++
.../bindings/leds/ti,lm3533-leds.yaml | 66 +++++++
.../devicetree/bindings/leds/ti,lm3533.yaml | 170 ++++++++++++++++++
3 files changed, 304 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..866b0fb8ed04
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
@@ -0,0 +1,68 @@
+# 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
+
+ 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..a321926de62e
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/ti,lm3533-leds.yaml
@@ -0,0 +1,66 @@
+# 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
+
+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..6e12e12be08e
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/ti,lm3533.yaml
@@ -0,0 +1,170 @@
+# 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-ohm:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Internal configuration resister 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-ohm property is ignored.
+
+ required:
+ - compatible
+
+ oneOf:
+ - required:
+ - ti,resistor-ohm
+ - 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] 24+ messages in thread
* [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
2026-05-28 13:51 [PATCH v2 0/6] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
2026-05-28 13:51 ` [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller Svyatoslav Ryhel
@ 2026-05-28 13:51 ` Svyatoslav Ryhel
2026-05-28 14:50 ` Jonathan Cameron
2026-05-29 11:00 ` Daniel Thompson
2026-05-28 13:51 ` [PATCH v2 3/6] mfd: lm3533: Add support for VIN power supply Svyatoslav Ryhel
` (3 subsequent siblings)
5 siblings, 2 replies; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-28 13:51 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.
Additionally, optimize functions used only by platform data.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/iio/light/lm3533-als.c | 95 ++++------
drivers/leds/leds-lm3533.c | 51 ++++--
drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
drivers/video/backlight/lm3533_bl.c | 52 ++++--
include/linux/mfd/lm3533.h | 51 +-----
5 files changed, 212 insertions(+), 305 deletions(-)
diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
index 99f0b903018c..cbd337b73bd9 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,59 +720,33 @@ 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)
+static int lm3533_als_setup(struct lm3533_als *als)
{
- u8 mask = LM3533_ALS_INPUT_MODE_MASK;
- u8 val;
+ struct device *dev = &als->pdev.dev;
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;
- }
+ device_property_read_u32(dev, "ti,resistor-value-ohm",
+ &als->r_select);
- return 0;
-}
+ 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);
-static int lm3533_als_setup(struct lm3533_als *als,
- const struct lm3533_als_platform_data *pdata)
-{
- int ret;
+ als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
- ret = lm3533_als_set_input_mode(als, pdata->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)
- return ret;
+ return dev_err_probe(dev, ret, "failed to set input mode %d\n",
+ als->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 (!als->pwm_mode) {
+ ret = lm3533_write(lm3533, LM3533_REG_ALS_RESISTOR_SELECT,
+ (u8)als->r_select);
if (ret)
- return ret;
+ return dev_err_probe(dev, ret, "failed to set resistor\n");
}
return 0;
@@ -828,7 +808,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 +817,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,25 +825,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;
@@ -907,9 +883,16 @@ static void lm3533_als_remove(struct platform_device *pdev)
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..d707d43d5526 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, (u8)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,12 +660,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;
@@ -673,8 +670,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;
@@ -682,6 +677,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
@@ -694,15 +698,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 +751,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..8495e9119871 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,
- },
-};
-
int lm3533_read(struct lm3533 *lm3533, u8 reg, u8 *val)
{
int tmp;
@@ -132,35 +102,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,135 +317,93 @@ 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;
+ struct mfd_cell lm3533_cells[LM3533_CELLS_MAX];
+ u32 count = 0, reg;
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;
-}
+ lm3533_enable(lm3533);
-static int lm3533_device_bl_init(struct lm3533 *lm3533)
-{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
- int i;
- int ret;
+ device_for_each_child_node_scoped(lm3533->dev, child) {
+ if (!fwnode_device_is_available(child))
+ continue;
- if (!pdata->backlights || pdata->num_backlights == 0)
- return 0;
+ if (count >= LM3533_CELLS_MAX)
+ break;
- if (pdata->num_backlights > ARRAY_SIZE(lm3533_bl_devs))
- pdata->num_backlights = ARRAY_SIZE(lm3533_bl_devs);
+ if (fwnode_device_is_compatible(child, "ti,lm3533-als")) {
+ lm3533_cells[count].name = "lm3533-als";
+ lm3533_cells[count].id = PLATFORM_DEVID_NONE;
+ lm3533_cells[count].of_compatible = "ti,lm3533-als";
- 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]);
- }
+ lm3533->have_als = true;
+ }
- 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;
- }
+ 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 reg %d\n", reg);
+ continue;
+ }
- lm3533->have_backlights = 1;
+ lm3533_cells[count].name = "lm3533-backlight";
+ lm3533_cells[count].id = reg;
+ lm3533_cells[count].of_compatible = "ti,lm3533-backlight";
- return 0;
-}
+ lm3533->have_backlights = true;
+ }
-static int lm3533_device_led_init(struct lm3533 *lm3533)
-{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
- int i;
- int ret;
+ 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 reg %d\n", reg);
+ continue;
+ }
- if (!pdata->leds || pdata->num_leds == 0)
- return 0;
+ lm3533_cells[count].name = "lm3533-leds";
+ lm3533_cells[count].id = reg - LM3533_HVLED_ID_MAX;
+ lm3533_cells[count].of_compatible = "ti,lm3533-leds";
- if (pdata->num_leds > ARRAY_SIZE(lm3533_led_devs))
- pdata->num_leds = ARRAY_SIZE(lm3533_led_devs);
+ lm3533->have_leds = true;
+ }
- 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]);
+ count++;
}
- ret = mfd_add_devices(lm3533->dev, 0, lm3533_led_devs,
- pdata->num_leds, 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 LED devices\n");
- return ret;
+ dev_err(dev, "failed to set boost frequency\n");
+ goto err_disable;
}
- 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;
+ ret = lm3533_update(lm3533, LM3533_REG_BOOST_PWM,
+ lm3533->boost_ovp << LM3533_BOOST_OVP_SHIFT,
+ LM3533_BOOST_OVP_MASK);
+ if (ret) {
+ dev_err(dev, "failed to set boost ovp\n");
+ 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 = mfd_add_devices(dev, 0, lm3533_cells, count, NULL, 0, NULL);
+ if (ret) {
+ dev_err(dev, "failed to add MFD devices: %d\n", ret);
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;
+ ret = sysfs_create_group(&dev->kobj, &lm3533_attribute_group);
+ if (ret) {
+ dev_err(dev, "failed to create sysfs attributes\n");
+ goto err_remove_devices;
}
return 0;
-err_unregister:
+err_remove_devices:
mfd_remove_devices(lm3533->dev);
err_disable:
lm3533_disable(lm3533);
@@ -517,8 +416,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 +486,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);
}
@@ -600,9 +516,16 @@ static void lm3533_i2c_remove(struct i2c_client *i2c)
dev_dbg(&i2c->dev, "%s\n", __func__);
+ mfd_remove_devices(lm3533->dev);
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 +535,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] 24+ messages in thread
* [PATCH v2 3/6] mfd: lm3533: Add support for VIN power supply
2026-05-28 13:51 [PATCH v2 0/6] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
2026-05-28 13:51 ` [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller Svyatoslav Ryhel
2026-05-28 13:51 ` [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings Svyatoslav Ryhel
@ 2026-05-28 13:51 ` Svyatoslav Ryhel
2026-05-28 13:51 ` [PATCH v2 4/6] mfd: lm3533: Set DMA mask Svyatoslav Ryhel
` (2 subsequent siblings)
5 siblings, 0 replies; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-28 13:51 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 8495e9119871..519f8c16a3f3 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>
@@ -164,14 +165,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 {
@@ -324,7 +336,9 @@ static int lm3533_device_init(struct lm3533 *lm3533)
u32 count = 0, reg;
int ret;
- lm3533_enable(lm3533);
+ ret = lm3533_enable(lm3533);
+ if (ret)
+ return ret;
device_for_each_child_node_scoped(lm3533->dev, child) {
if (!fwnode_device_is_available(child))
@@ -493,6 +507,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 3aa962d4c747..d751773fc92d 100644
--- a/include/linux/mfd/lm3533.h
+++ b/include/linux/mfd/lm3533.h
@@ -21,6 +21,7 @@
struct device;
struct gpio_desc;
struct regmap;
+struct regulator;
struct lm3533 {
struct device *dev;
@@ -28,6 +29,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] 24+ messages in thread
* [PATCH v2 4/6] mfd: lm3533: Set DMA mask
2026-05-28 13:51 [PATCH v2 0/6] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
` (2 preceding siblings ...)
2026-05-28 13:51 ` [PATCH v2 3/6] mfd: lm3533: Add support for VIN power supply Svyatoslav Ryhel
@ 2026-05-28 13:51 ` Svyatoslav Ryhel
2026-05-28 13:51 ` [PATCH v2 5/6] video: backlight: lm3533_bl: Set initial mapping mode from DT Svyatoslav Ryhel
2026-05-28 13:51 ` [PATCH v2 6/6] video: leds: backlight: lm3533: Support getting LED sources " Svyatoslav Ryhel
5 siblings, 0 replies; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-28 13:51 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 519f8c16a3f3..3cfdebf5fb52 100644
--- a/drivers/mfd/lm3533-core.c
+++ b/drivers/mfd/lm3533-core.c
@@ -526,6 +526,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] 24+ messages in thread
* [PATCH v2 5/6] video: backlight: lm3533_bl: Set initial mapping mode from DT
2026-05-28 13:51 [PATCH v2 0/6] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
` (3 preceding siblings ...)
2026-05-28 13:51 ` [PATCH v2 4/6] mfd: lm3533: Set DMA mask Svyatoslav Ryhel
@ 2026-05-28 13:51 ` Svyatoslav Ryhel
2026-05-29 11:10 ` Daniel Thompson
2026-05-28 13:51 ` [PATCH v2 6/6] video: leds: backlight: lm3533: Support getting LED sources " Svyatoslav Ryhel
5 siblings, 1 reply; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-28 13:51 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. Additionally, update the linear sysfs code, which uses a
similar coding pattern.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/video/backlight/lm3533_bl.c | 50 ++++++++++++++++-------------
1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
index 42da652df58d..c03d0d1667e4 100644
--- a/drivers/video/backlight/lm3533_bl.c
+++ b/drivers/video/backlight/lm3533_bl.c
@@ -22,6 +22,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 {
@@ -32,6 +33,7 @@ struct lm3533_bl {
u32 max_current;
u32 pwm;
+ bool linear;
};
@@ -135,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);
u8 val;
- u8 mask;
int linear;
int ret;
@@ -144,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
@@ -159,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 = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF, val,
- mask);
+ ret = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF,
+ linear ? CTRLBANK_AB_BCONF_MODE(id) : 0,
+ CTRLBANK_AB_BCONF_MODE(id));
if (ret)
return ret;
@@ -253,8 +247,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 = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF,
+ bl->linear ? CTRLBANK_AB_BCONF_MODE(id) : 0,
+ CTRLBANK_AB_BCONF_MODE(id));
+ if (ret)
+ return ret;
+
ret = lm3533_ctrlbank_set_max_current(&bl->cb, bl->max_current);
if (ret)
return ret;
@@ -317,14 +318,6 @@ static int lm3533_bl_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, bl);
- ret = sysfs_create_group(&bd->dev.kobj, &lm3533_bl_attribute_group);
- if (ret < 0) {
- dev_err(&pdev->dev, "failed to create sysfs attributes\n");
- return ret;
- }
-
- backlight_update_status(bd);
-
device_property_read_u32(&pdev->dev, "led-max-microamp",
&bl->max_current);
bl->max_current = clamp(bl->max_current, LM3533_LED_MAX_CURRENT_MIN,
@@ -333,9 +326,20 @@ 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->linear = device_property_read_bool(&pdev->dev,
+ "ti,linear-mapping-mode");
+
ret = lm3533_bl_setup(bl);
if (ret)
- goto err_sysfs_remove;
+ 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");
+ return ret;
+ }
+
+ backlight_update_status(bd);
ret = lm3533_ctrlbank_enable(&bl->cb);
if (ret)
--
2.51.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 6/6] video: leds: backlight: lm3533: Support getting LED sources from DT
2026-05-28 13:51 [PATCH v2 0/6] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
` (4 preceding siblings ...)
2026-05-28 13:51 ` [PATCH v2 5/6] video: backlight: lm3533_bl: Set initial mapping mode from DT Svyatoslav Ryhel
@ 2026-05-28 13:51 ` Svyatoslav Ryhel
5 siblings, 0 replies; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-28 13:51 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 | 37 ++++++++++++++++++-
2 files changed, 90 insertions(+), 2 deletions(-)
diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
index d707d43d5526..07390bba9a48 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>
@@ -26,6 +27,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
@@ -53,6 +60,9 @@ struct lm3533_led {
u32 max_current;
u32 pwm;
+
+ int num_leds;
+ u32 leds[LM3533_LVCTRLBANK_MAX];
};
@@ -639,7 +649,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 = lm3533_update(led->lm3533, LM3533_REG_OUTPUT_CONF1,
+ output_cfg_val << OUTPUT_CONF1_SHIFT,
+ OUTPUT_CONF1_MASK);
+ if (ret)
+ return ret;
+
+ /* LVLED4 and LVLED5 */
+ ret = lm3533_update(led->lm3533, LM3533_REG_OUTPUT_CONF2,
+ output_cfg_val >> OUTPUT_CONF2_SHIFT,
+ OUTPUT_CONF2_MASK);
+ if (ret)
+ return ret;
+ }
ret = lm3533_ctrlbank_set_max_current(&led->cb, led->max_current);
if (ret)
@@ -714,6 +750,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 c03d0d1667e4..82b46a531dd2 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>
@@ -21,6 +22,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)
@@ -34,6 +37,9 @@ struct lm3533_bl {
u32 max_current;
u32 pwm;
bool linear;
+
+ u32 num_leds;
+ u32 led_strings[LM3533_HVCTRLBANK_COUNT];
};
@@ -248,7 +254,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 = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF,
bl->linear ? CTRLBANK_AB_BCONF_MODE(id) : 0,
@@ -256,6 +263,16 @@ 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++)
+ output_cfg_val |= id << bl->led_strings[i];
+
+ ret = lm3533_update(bl->lm3533, LM3533_REG_OUTPUT_CONF1,
+ output_cfg_val, OUTPUT_CONF1_MASK);
+ if (ret)
+ return ret;
+ }
+
ret = lm3533_ctrlbank_set_max_current(&bl->cb, bl->max_current);
if (ret)
return ret;
@@ -329,6 +346,24 @@ static int lm3533_bl_probe(struct platform_device *pdev)
bl->linear = device_property_read_bool(&pdev->dev,
"ti,linear-mapping-mode");
+ 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) {
+ dev_err(&pdev->dev, "failed to get led-sources\n");
+ goto err_sysfs_remove;
+ }
+ } else {
+ bl->num_leds = 0;
+ }
+
ret = lm3533_bl_setup(bl);
if (ret)
return ret;
--
2.51.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
2026-05-28 13:51 ` [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings Svyatoslav Ryhel
@ 2026-05-28 14:50 ` Jonathan Cameron
2026-05-28 15:03 ` Svyatoslav Ryhel
2026-05-29 11:00 ` Daniel Thompson
1 sibling, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2026-05-28 14:50 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 Thu, 28 May 2026 16:51:19 +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.
> Additionally, optimize functions used only by platform data.
At least the IIO ones would have made much the same amount of sense for
dt, just that they weren't having in the first place. I'd prefer that
as a precursor patch to make the rest much more readable.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
I only looked in detail at the iio bit. A few changes requested.
> ---
> drivers/iio/light/lm3533-als.c | 95 ++++------
> drivers/leds/leds-lm3533.c | 51 ++++--
> drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
> drivers/video/backlight/lm3533_bl.c | 52 ++++--
> include/linux/mfd/lm3533.h | 51 +-----
> 5 files changed, 212 insertions(+), 305 deletions(-)
>
> diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> index 99f0b903018c..cbd337b73bd9 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c
> @@ -714,59 +720,33 @@ 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)
> +static int lm3533_als_setup(struct lm3533_als *als)
> {
> - u8 mask = LM3533_ALS_INPUT_MODE_MASK;
> - u8 val;
> + struct device *dev = &als->pdev.dev;
> 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;
> - }
> + device_property_read_u32(dev, "ti,resistor-value-ohm",
> + &als->r_select);
Does this have a default? If so the pattern we've recently be setting on for IIO
is
if (device_property_present(dev, "ti,resistor-value-ohm"))
ret = device_property_read_u32();
if (ret) //corrupt property in some fashion
return ret;
} else {
//set default
}
If there is no default then check it unconditionally.
>
> - return 0;
> -}
> + 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);
>
> -static int lm3533_als_setup(struct lm3533_als *als,
> - const struct lm3533_als_platform_data *pdata)
> -{
> - int ret;
> + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
>
> - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> + ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, als->pwm_mode ?
> + LM3533_ALS_INPUT_MODE_MASK : 0,
That's ugly. Better as
ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF,
als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0,
Though if there wasn't a layer hiding the regmap, it could just have been
ret = regmap_assign_bits(lm3533->regmap, LM3533_REG_ALS_CONF,
LM3533_ALS_INPUT_MODE_MASK, als->pwm_mode);;
which would have been nicer.
I'm not particularly keen on the swashing of the helpers being in a patch
that is about switching the binding type as feels largely unrelated.
Should really have been a precursor, easier to review patch.
> + LM3533_ALS_INPUT_MODE_MASK);
> if (ret)
> - return ret;
> + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> + als->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 (!als->pwm_mode) {
> + ret = lm3533_write(lm3533, LM3533_REG_ALS_RESISTOR_SELECT,
> + (u8)als->r_select);
Same applies here. Mostly an unrelated change as the only thing switching that
is related to the patch is one parameter.
> if (ret)
> - return ret;
> + return dev_err_probe(dev, ret, "failed to set resistor\n");
> }
>
> return 0;
> @@ -852,25 +825,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);
I'm not sure why this was there in the first place. Hence not sure if it
is safe to remove.
> diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> index 45795f2a1042..d707d43d5526 100644
> --- a/drivers/leds/leds-lm3533.c
> +++ b/drivers/leds/leds-lm3533.c
>
> led->cb.dev = led->cdev.dev;
>
> - ret = lm3533_led_setup(led, pdata);
> + device_property_read_u32(&pdev->dev, "led-max-microamp",
> + &led->max_current);
I'd prefer explicit setting of the default to be visible before this, or
the property_present pattern I mention in the IIO review above.
> + led->max_current = clamp(led->max_current, LM3533_LED_MAX_CURRENT_MIN,
> + LM3533_LED_MAX_CURRENT_MAX);
I didn't look any further (busy day!)
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller
2026-05-28 13:51 ` [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller Svyatoslav Ryhel
@ 2026-05-28 14:54 ` Jonathan Cameron
2026-05-29 9:51 ` Daniel Thompson
1 sibling, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2026-05-28 14:54 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 Thu, 28 May 2026 16:51:18 +0300
Svyatoslav Ryhel <clamor95@gmail.com> 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>
> ---
The light sensor binding looks fine to me. I didn't check the rest.
Reviewed-by: Jonathan Cameron <jic23@kernel.org> #for light sensor
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
2026-05-28 14:50 ` Jonathan Cameron
@ 2026-05-28 15:03 ` Svyatoslav Ryhel
2026-05-29 9:08 ` Jonathan Cameron
0 siblings, 1 reply; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-28 15:03 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
чт, 28 трав. 2026 р. о 17:50 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Thu, 28 May 2026 16:51:19 +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.
> > Additionally, optimize functions used only by platform data.
>
>
> At least the IIO ones would have made much the same amount of sense for
> dt, just that they weren't having in the first place. I'd prefer that
> as a precursor patch to make the rest much more readable.
>
I can add you preferences into this commit, I don't mind.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>
> I only looked in detail at the iio bit. A few changes requested.
>
> > ---
> > drivers/iio/light/lm3533-als.c | 95 ++++------
> > drivers/leds/leds-lm3533.c | 51 ++++--
> > drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
> > drivers/video/backlight/lm3533_bl.c | 52 ++++--
> > include/linux/mfd/lm3533.h | 51 +-----
> > 5 files changed, 212 insertions(+), 305 deletions(-)
> >
> > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > index 99f0b903018c..cbd337b73bd9 100644
> > --- a/drivers/iio/light/lm3533-als.c
> > +++ b/drivers/iio/light/lm3533-als.c
>
> > @@ -714,59 +720,33 @@ 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)
> > +static int lm3533_als_setup(struct lm3533_als *als)
> > {
> > - u8 mask = LM3533_ALS_INPUT_MODE_MASK;
> > - u8 val;
> > + struct device *dev = &als->pdev.dev;
> > 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;
> > - }
> > + device_property_read_u32(dev, "ti,resistor-value-ohm",
> > + &als->r_select);
> Does this have a default? If so the pattern we've recently be setting on for IIO
> is
> if (device_property_present(dev, "ti,resistor-value-ohm"))
> ret = device_property_read_u32();
> if (ret) //corrupt property in some fashion
> return ret;
> } else {
> //set default
> }
> If there is no default then check it unconditionally.
default value is LM3533_ALS_RESISTOR_MIN and if no property is present
clamp will ensure that als->r_select will be set to
LM3533_ALS_RESISTOR_MIN
>
> >
> > - return 0;
> > -}
> > + 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);
> >
> > -static int lm3533_als_setup(struct lm3533_als *als,
> > - const struct lm3533_als_platform_data *pdata)
> > -{
> > - int ret;
> > + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> >
> > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > + ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, als->pwm_mode ?
> > + LM3533_ALS_INPUT_MODE_MASK : 0,
>
> That's ugly. Better as
>
> ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF,
> als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0,
>
Yes sure, just followed 80 char limit.
> Though if there wasn't a layer hiding the regmap, it could just have been
>
> ret = regmap_assign_bits(lm3533->regmap, LM3533_REG_ALS_CONF,
> LM3533_ALS_INPUT_MODE_MASK, als->pwm_mode);;
>
> which would have been nicer.
>
> I'm not particularly keen on the swashing of the helpers being in a patch
> that is about switching the binding type as feels largely unrelated.
> Should really have been a precursor, easier to review patch.
>
Removing of lm3533_update layer is not the scope of this patchset.
>
> > + LM3533_ALS_INPUT_MODE_MASK);
> > if (ret)
> > - return ret;
> > + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > + als->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 (!als->pwm_mode) {
> > + ret = lm3533_write(lm3533, LM3533_REG_ALS_RESISTOR_SELECT,
> > + (u8)als->r_select);
>
> Same applies here. Mostly an unrelated change as the only thing switching that
> is related to the patch is one parameter.
>
Removing of lm3533_write layer is not the scope of this patchset.
> > if (ret)
> > - return ret;
> > + return dev_err_probe(dev, ret, "failed to set resistor\n");
> > }
> >
> > return 0;
>
> > @@ -852,25 +825,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);
>
> I'm not sure why this was there in the first place. Hence not sure if it
> is safe to remove.
>
This is directly related to OF conversion. The iio_device_set_parent
bound indio_dev to parent, and it causes problems with OF now since
als output has its own node and binding it to parent if wrong. Same
story for backlight and leds btw.
>
> > diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> > index 45795f2a1042..d707d43d5526 100644
> > --- a/drivers/leds/leds-lm3533.c
> > +++ b/drivers/leds/leds-lm3533.c
>
> >
> > led->cb.dev = led->cdev.dev;
> >
> > - ret = lm3533_led_setup(led, pdata);
> > + device_property_read_u32(&pdev->dev, "led-max-microamp",
> > + &led->max_current);
>
> I'd prefer explicit setting of the default to be visible before this, or
> the property_present pattern I mention in the IIO review above.
>
clamp will ensure that led->max_current will be set to
LM3533_LED_MAX_CURRENT_MIN regardless if it it present
> > + led->max_current = clamp(led->max_current, LM3533_LED_MAX_CURRENT_MIN,
> > + LM3533_LED_MAX_CURRENT_MAX);
>
> I didn't look any further (busy day!)
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
2026-05-28 15:03 ` Svyatoslav Ryhel
@ 2026-05-29 9:08 ` Jonathan Cameron
2026-05-29 9:39 ` Svyatoslav Ryhel
0 siblings, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2026-05-29 9:08 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 Thu, 28 May 2026 18:03:31 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> чт, 28 трав. 2026 р. о 17:50 Jonathan Cameron <jic23@kernel.org> пише:
> >
> > On Thu, 28 May 2026 16:51:19 +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.
> > > Additionally, optimize functions used only by platform data.
> >
> >
> > At least the IIO ones would have made much the same amount of sense for
> > dt, just that they weren't having in the first place. I'd prefer that
Gah. I write gibberish after too much reviewing. having/helping!
> > as a precursor patch to make the rest much more readable.
> >
>
> I can add you preferences into this commit, I don't mind.
>
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> >
> > I only looked in detail at the iio bit. A few changes requested.
> >
> > > ---
> > > drivers/iio/light/lm3533-als.c | 95 ++++------
> > > drivers/leds/leds-lm3533.c | 51 ++++--
> > > drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
> > > drivers/video/backlight/lm3533_bl.c | 52 ++++--
> > > include/linux/mfd/lm3533.h | 51 +-----
> > > 5 files changed, 212 insertions(+), 305 deletions(-)
> > >
> > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > > index 99f0b903018c..cbd337b73bd9 100644
> > > --- a/drivers/iio/light/lm3533-als.c
> > > +++ b/drivers/iio/light/lm3533-als.c
> >
> > > @@ -714,59 +720,33 @@ 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)
> > > +static int lm3533_als_setup(struct lm3533_als *als)
> > > {
> > > - u8 mask = LM3533_ALS_INPUT_MODE_MASK;
> > > - u8 val;
> > > + struct device *dev = &als->pdev.dev;
> > > 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;
> > > - }
> > > + device_property_read_u32(dev, "ti,resistor-value-ohm",
> > > + &als->r_select);
> > Does this have a default? If so the pattern we've recently be setting on for IIO
> > is
> > if (device_property_present(dev, "ti,resistor-value-ohm"))
> > ret = device_property_read_u32();
> > if (ret) //corrupt property in some fashion
> > return ret;
> > } else {
> > //set default
> > }
> > If there is no default then check it unconditionally.
>
> default value is LM3533_ALS_RESISTOR_MIN and if no property is present
> clamp will ensure that als->r_select will be set to
> LM3533_ALS_RESISTOR_MIN
I don't see that default in the binding doc and relying in the 0 being clamped
isn't particularly readable - I'd set it explicitly.
>
> >
> > >
> > > - return 0;
> > > -}
> > > + 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);
> > >
> > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > - const struct lm3533_als_platform_data *pdata)
> > > -{
> > > - int ret;
> > > + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > >
> > > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > > + ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, als->pwm_mode ?
> > > + LM3533_ALS_INPUT_MODE_MASK : 0,
> >
> > That's ugly. Better as
> >
> > ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF,
> > als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0,
> >
>
> Yes sure, just followed 80 char limit.
>
> > Though if there wasn't a layer hiding the regmap, it could just have been
> >
> > ret = regmap_assign_bits(lm3533->regmap, LM3533_REG_ALS_CONF,
> > LM3533_ALS_INPUT_MODE_MASK, als->pwm_mode);;
> >
> > which would have been nicer.
> >
> > I'm not particularly keen on the swashing of the helpers being in a patch
smashing. (this definitely wasn't my best effort at English!)
> > that is about switching the binding type as feels largely unrelated.
> > Should really have been a precursor, easier to review patch.
> >
>
> Removing of lm3533_update layer is not the scope of this patchset.
Understood. I'm fine with just the refactor you are doing brought out as a precursor
patch.
>
> >
> > > + LM3533_ALS_INPUT_MODE_MASK);
> > > if (ret)
> > > - return ret;
> > > + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > > + als->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 (!als->pwm_mode) {
> > > + ret = lm3533_write(lm3533, LM3533_REG_ALS_RESISTOR_SELECT,
> > > + (u8)als->r_select);
> >
> > Same applies here. Mostly an unrelated change as the only thing switching that
> > is related to the patch is one parameter.
> >
>
> Removing of lm3533_write layer is not the scope of this patchset.
>
> > > if (ret)
> > > - return ret;
> > > + return dev_err_probe(dev, ret, "failed to set resistor\n");
> > > }
> > >
> > > return 0;
> >
> > > @@ -852,25 +825,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);
> >
> > I'm not sure why this was there in the first place. Hence not sure if it
> > is safe to remove.
> >
>
> This is directly related to OF conversion. The iio_device_set_parent
> bound indio_dev to parent, and it causes problems with OF now since
> als output has its own node and binding it to parent if wrong. Same
> story for backlight and leds btw.
Is there any risk anyone was using the canonical path to get to the iio dev?
/sys/bus/platform/devices/..../iio\:deviceX
This is technically an ABI change be it a subtle one.
>
> >
> > > diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> > > index 45795f2a1042..d707d43d5526 100644
> > > --- a/drivers/leds/leds-lm3533.c
> > > +++ b/drivers/leds/leds-lm3533.c
> >
> > >
> > > led->cb.dev = led->cdev.dev;
> > >
> > > - ret = lm3533_led_setup(led, pdata);
> > > + device_property_read_u32(&pdev->dev, "led-max-microamp",
> > > + &led->max_current);
> >
> > I'd prefer explicit setting of the default to be visible before this, or
> > the property_present pattern I mention in the IIO review above.
> >
>
> clamp will ensure that led->max_current will be set to
> LM3533_LED_MAX_CURRENT_MIN regardless if it it present
As above, I'd prefer it set explicitly.
>
> > > + led->max_current = clamp(led->max_current, LM3533_LED_MAX_CURRENT_MIN,
> > > + LM3533_LED_MAX_CURRENT_MAX);
> >
> > I didn't look any further (busy day!)
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
2026-05-29 9:08 ` Jonathan Cameron
@ 2026-05-29 9:39 ` Svyatoslav Ryhel
2026-05-29 10:48 ` Jonathan Cameron
0 siblings, 1 reply; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-29 9:39 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
пт, 29 трав. 2026 р. о 12:08 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Thu, 28 May 2026 18:03:31 +0300
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > чт, 28 трав. 2026 р. о 17:50 Jonathan Cameron <jic23@kernel.org> пише:
> > >
> > > On Thu, 28 May 2026 16:51:19 +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.
> > > > Additionally, optimize functions used only by platform data.
> > >
> > >
> > > At least the IIO ones would have made much the same amount of sense for
> > > dt, just that they weren't having in the first place. I'd prefer that
>
> Gah. I write gibberish after too much reviewing. having/helping!
>
> > > as a precursor patch to make the rest much more readable.
> > >
> >
> > I can add you preferences into this commit, I don't mind.
> >
> > > >
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > >
> > > I only looked in detail at the iio bit. A few changes requested.
> > >
> > > > ---
> > > > drivers/iio/light/lm3533-als.c | 95 ++++------
> > > > drivers/leds/leds-lm3533.c | 51 ++++--
> > > > drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
> > > > drivers/video/backlight/lm3533_bl.c | 52 ++++--
> > > > include/linux/mfd/lm3533.h | 51 +-----
> > > > 5 files changed, 212 insertions(+), 305 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > > > index 99f0b903018c..cbd337b73bd9 100644
> > > > --- a/drivers/iio/light/lm3533-als.c
> > > > +++ b/drivers/iio/light/lm3533-als.c
> > >
> > > > @@ -714,59 +720,33 @@ 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)
> > > > +static int lm3533_als_setup(struct lm3533_als *als)
> > > > {
> > > > - u8 mask = LM3533_ALS_INPUT_MODE_MASK;
> > > > - u8 val;
> > > > + struct device *dev = &als->pdev.dev;
> > > > 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;
> > > > - }
> > > > + device_property_read_u32(dev, "ti,resistor-value-ohm",
> > > > + &als->r_select);
> > > Does this have a default? If so the pattern we've recently be setting on for IIO
> > > is
> > > if (device_property_present(dev, "ti,resistor-value-ohm"))
> > > ret = device_property_read_u32();
> > > if (ret) //corrupt property in some fashion
> > > return ret;
> > > } else {
> > > //set default
> > > }
> > > If there is no default then check it unconditionally.
> >
> > default value is LM3533_ALS_RESISTOR_MIN and if no property is present
> > clamp will ensure that als->r_select will be set to
> > LM3533_ALS_RESISTOR_MIN
>
> I don't see that default in the binding doc and relying in the 0 being clamped
> isn't particularly readable - I'd set it explicitly.
>
Oh, ye, my bad. Schema enforces one of props to be present and if pwn
is present then resistor is ignored. What if I move resistor reading,
clamping and conversion under !als->pwm_mode check? Then resistor must
be present and hence must be checked unconditionally.
Additionally, I can comment original lm3533_als_setup with #if 0
#endif then git formatting will be much cleaner and easier to review,
and once we all come to result I will just remove entire commented
block and Lee can pick clean commits.
>
> >
> > >
> > > >
> > > > - return 0;
> > > > -}
> > > > + 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);
> > > >
> > > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > > - const struct lm3533_als_platform_data *pdata)
> > > > -{
> > > > - int ret;
> > > > + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > > >
> > > > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > > > + ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, als->pwm_mode ?
> > > > + LM3533_ALS_INPUT_MODE_MASK : 0,
> > >
> > > That's ugly. Better as
> > >
> > > ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF,
> > > als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0,
> > >
> >
> > Yes sure, just followed 80 char limit.
> >
> > > Though if there wasn't a layer hiding the regmap, it could just have been
> > >
> > > ret = regmap_assign_bits(lm3533->regmap, LM3533_REG_ALS_CONF,
> > > LM3533_ALS_INPUT_MODE_MASK, als->pwm_mode);;
> > >
> > > which would have been nicer.
> > >
> > > I'm not particularly keen on the swashing of the helpers being in a patch
>
> smashing. (this definitely wasn't my best effort at English!)
>
> > > that is about switching the binding type as feels largely unrelated.
> > > Should really have been a precursor, easier to review patch.
> > >
> >
> > Removing of lm3533_update layer is not the scope of this patchset.
>
> Understood. I'm fine with just the refactor you are doing brought out as a precursor
> patch.
>
I have looked into removing wrappers too. That seems to be less a
hassle that I anticipated, so I will include regmap switch in the v2.
> >
> > >
> > > > + LM3533_ALS_INPUT_MODE_MASK);
> > > > if (ret)
> > > > - return ret;
> > > > + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > > > + als->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 (!als->pwm_mode) {
> > > > + ret = lm3533_write(lm3533, LM3533_REG_ALS_RESISTOR_SELECT,
> > > > + (u8)als->r_select);
> > >
> > > Same applies here. Mostly an unrelated change as the only thing switching that
> > > is related to the patch is one parameter.
> > >
> >
> > Removing of lm3533_write layer is not the scope of this patchset.
> >
> > > > if (ret)
> > > > - return ret;
> > > > + return dev_err_probe(dev, ret, "failed to set resistor\n");
> > > > }
> > > >
> > > > return 0;
> > >
> > > > @@ -852,25 +825,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);
> > >
> > > I'm not sure why this was there in the first place. Hence not sure if it
> > > is safe to remove.
> > >
> >
> > This is directly related to OF conversion. The iio_device_set_parent
> > bound indio_dev to parent, and it causes problems with OF now since
> > als output has its own node and binding it to parent if wrong. Same
> > story for backlight and leds btw.
>
> Is there any risk anyone was using the canonical path to get to the iio dev?
> /sys/bus/platform/devices/..../iio\:deviceX
> This is technically an ABI change be it a subtle one.
>
Linux kernel has no users of this driver, and it is in "stale" state
for more then 2 years (maybe even longer). I have cc'd Johan Hovold.
https://lore.kernel.org/lkml/ZmBcvtLCzllQDWVX@hovoldconsulting.com/
This this 2 y. o. discussion and there were no actions ore movements.
I assume this driver in its current form has no more users. This does
not mean that it cannot be revived though.
>
> >
> > >
> > > > diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> > > > index 45795f2a1042..d707d43d5526 100644
> > > > --- a/drivers/leds/leds-lm3533.c
> > > > +++ b/drivers/leds/leds-lm3533.c
> > >
> > > >
> > > > led->cb.dev = led->cdev.dev;
> > > >
> > > > - ret = lm3533_led_setup(led, pdata);
> > > > + device_property_read_u32(&pdev->dev, "led-max-microamp",
> > > > + &led->max_current);
> > >
> > > I'd prefer explicit setting of the default to be visible before this, or
> > > the property_present pattern I mention in the IIO review above.
> > >
> >
> > clamp will ensure that led->max_current will be set to
> > LM3533_LED_MAX_CURRENT_MIN regardless if it it present
>
> As above, I'd prefer it set explicitly.
>
I understand your position and I am not denying it for ALS part, but
LEDs don't belong to IIO subsystem and different subsystem maintainers
may have drastically different preferences and requirements (ugh, PTSD
in its full glory).
> >
> > > > + led->max_current = clamp(led->max_current, LM3533_LED_MAX_CURRENT_MIN,
> > > > + LM3533_LED_MAX_CURRENT_MAX);
> > >
> > > I didn't look any further (busy day!)
> >
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller
2026-05-28 13:51 ` [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller Svyatoslav Ryhel
2026-05-28 14:54 ` Jonathan Cameron
@ 2026-05-29 9:51 ` Daniel Thompson
2026-05-29 10:07 ` Svyatoslav Ryhel
1 sibling, 1 reply; 24+ messages in thread
From: Daniel Thompson @ 2026-05-29 9:51 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 Thu, May 28, 2026 at 04:51:18PM +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>
> ---
> .../leds/backlight/ti,lm3533-backlight.yaml | 68 +++++++
> .../bindings/leds/ti,lm3533-leds.yaml | 66 +++++++
> .../devicetree/bindings/leds/ti,lm3533.yaml | 170 ++++++++++++++++++
> 3 files changed, 304 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..866b0fb8ed04
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
> @@ -0,0 +1,68 @@
> +# 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
> <snip>
> + 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
This is optional and the drive implements a default (zero) that is not
documented here.
Is zero a sane default from a DT binding point of view?
Daniel.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller
2026-05-29 9:51 ` Daniel Thompson
@ 2026-05-29 10:07 ` Svyatoslav Ryhel
2026-05-29 10:46 ` Daniel Thompson
0 siblings, 1 reply; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-29 10:07 UTC (permalink / raw)
To: Daniel Thompson
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
пт, 29 трав. 2026 р. о 12:51 Daniel Thompson <daniel@riscstar.com> пише:
>
> On Thu, May 28, 2026 at 04:51:18PM +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>
> > ---
> > .../leds/backlight/ti,lm3533-backlight.yaml | 68 +++++++
> > .../bindings/leds/ti,lm3533-leds.yaml | 66 +++++++
> > .../devicetree/bindings/leds/ti,lm3533.yaml | 170 ++++++++++++++++++
> > 3 files changed, 304 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..866b0fb8ed04
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
> > @@ -0,0 +1,68 @@
> > +# 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
> > <snip>
> > + 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
>
> This is optional and the drive implements a default (zero) that is not
> documented here.
>
> Is zero a sane default from a DT binding point of view?
>
Yes, if property is missing then PWM input is disabled which is
equivalent to setting all bits to 0.
>
> Daniel.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller
2026-05-29 10:07 ` Svyatoslav Ryhel
@ 2026-05-29 10:46 ` Daniel Thompson
2026-05-29 10:56 ` Svyatoslav Ryhel
0 siblings, 1 reply; 24+ messages in thread
From: Daniel Thompson @ 2026-05-29 10:46 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 Fri, May 29, 2026 at 01:07:50PM +0300, Svyatoslav Ryhel wrote:
> пт, 29 трав. 2026 р. о 12:51 Daniel Thompson <daniel@riscstar.com> пише:
> >
> > On Thu, May 28, 2026 at 04:51:18PM +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>
> > > ---
> > > .../leds/backlight/ti,lm3533-backlight.yaml | 68 +++++++
> > > .../bindings/leds/ti,lm3533-leds.yaml | 66 +++++++
> > > .../devicetree/bindings/leds/ti,lm3533.yaml | 170 ++++++++++++++++++
> > > 3 files changed, 304 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..866b0fb8ed04
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
> > > @@ -0,0 +1,68 @@
> > > +# 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
> > > <snip>
> > > + 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
> >
> > This is optional and the drive implements a default (zero) that is not
> > documented here.
> >
> > Is zero a sane default from a DT binding point of view?
> >
>
> Yes, if property is missing then PWM input is disabled which is
> equivalent to setting all bits to 0.
So the default should be documented in the bindings?
Daniel.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
2026-05-29 9:39 ` Svyatoslav Ryhel
@ 2026-05-29 10:48 ` Jonathan Cameron
2026-05-29 11:02 ` Svyatoslav Ryhel
0 siblings, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2026-05-29 10:48 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 Fri, 29 May 2026 12:39:56 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> пт, 29 трав. 2026 р. о 12:08 Jonathan Cameron <jic23@kernel.org> пише:
> >
> > On Thu, 28 May 2026 18:03:31 +0300
> > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >
> > > чт, 28 трав. 2026 р. о 17:50 Jonathan Cameron <jic23@kernel.org> пише:
> > > >
> > > > On Thu, 28 May 2026 16:51:19 +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.
> > > > > Additionally, optimize functions used only by platform data.
> > > >
> > > >
> > > > At least the IIO ones would have made much the same amount of sense for
> > > > dt, just that they weren't having in the first place. I'd prefer that
> >
> > Gah. I write gibberish after too much reviewing. having/helping!
> >
> > > > as a precursor patch to make the rest much more readable.
> > > >
> > >
> > > I can add you preferences into this commit, I don't mind.
> > >
> > > > >
> > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > >
> > > > I only looked in detail at the iio bit. A few changes requested.
> > > >
> > > > > ---
> > > > > drivers/iio/light/lm3533-als.c | 95 ++++------
> > > > > drivers/leds/leds-lm3533.c | 51 ++++--
> > > > > drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
> > > > > drivers/video/backlight/lm3533_bl.c | 52 ++++--
> > > > > include/linux/mfd/lm3533.h | 51 +-----
> > > > > 5 files changed, 212 insertions(+), 305 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > > > > index 99f0b903018c..cbd337b73bd9 100644
> > > > > --- a/drivers/iio/light/lm3533-als.c
> > > > > +++ b/drivers/iio/light/lm3533-als.c
> > > >
> > > > > @@ -714,59 +720,33 @@ 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)
> > > > > +static int lm3533_als_setup(struct lm3533_als *als)
> > > > > {
> > > > > - u8 mask = LM3533_ALS_INPUT_MODE_MASK;
> > > > > - u8 val;
> > > > > + struct device *dev = &als->pdev.dev;
> > > > > 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;
> > > > > - }
> > > > > + device_property_read_u32(dev, "ti,resistor-value-ohm",
> > > > > + &als->r_select);
> > > > Does this have a default? If so the pattern we've recently be setting on for IIO
> > > > is
> > > > if (device_property_present(dev, "ti,resistor-value-ohm"))
> > > > ret = device_property_read_u32();
> > > > if (ret) //corrupt property in some fashion
> > > > return ret;
> > > > } else {
> > > > //set default
> > > > }
> > > > If there is no default then check it unconditionally.
> > >
> > > default value is LM3533_ALS_RESISTOR_MIN and if no property is present
> > > clamp will ensure that als->r_select will be set to
> > > LM3533_ALS_RESISTOR_MIN
> >
> > I don't see that default in the binding doc and relying in the 0 being clamped
> > isn't particularly readable - I'd set it explicitly.
> >
>
> Oh, ye, my bad. Schema enforces one of props to be present and if pwn
> is present then resistor is ignored. What if I move resistor reading,
> clamping and conversion under !als->pwm_mode check? Then resistor must
> be present and hence must be checked unconditionally.
Sounds good.
>
> Additionally, I can comment original lm3533_als_setup with #if 0
> #endif then git formatting will be much cleaner and easier to review,
> and once we all come to result I will just remove entire commented
> block and Lee can pick clean commits.
No don't do that. If you flatten the two helpers as a precursor patch
then the changes in here will be easier to review anyway.
>
> >
> > >
> > > >
> > > > >
> > > > > - return 0;
> > > > > -}
> > > > > + 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);
> > > > >
> > > > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > > > - const struct lm3533_als_platform_data *pdata)
> > > > > -{
> > > > > - int ret;
> > > > > + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > > > >
> > > > > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > > > > + ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, als->pwm_mode ?
> > > > > + LM3533_ALS_INPUT_MODE_MASK : 0,
> > > >
> > > > That's ugly. Better as
> > > >
> > > > ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF,
> > > > als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0,
> > > >
> > >
> > > Yes sure, just followed 80 char limit.
> > >
> > > > Though if there wasn't a layer hiding the regmap, it could just have been
> > > >
> > > > ret = regmap_assign_bits(lm3533->regmap, LM3533_REG_ALS_CONF,
> > > > LM3533_ALS_INPUT_MODE_MASK, als->pwm_mode);;
> > > >
> > > > which would have been nicer.
> > > >
> > > > I'm not particularly keen on the swashing of the helpers being in a patch
> >
> > smashing. (this definitely wasn't my best effort at English!)
> >
> > > > that is about switching the binding type as feels largely unrelated.
> > > > Should really have been a precursor, easier to review patch.
> > > >
> > >
> > > Removing of lm3533_update layer is not the scope of this patchset.
> >
> > Understood. I'm fine with just the refactor you are doing brought out as a precursor
> > patch.
> >
>
> I have looked into removing wrappers too. That seems to be less a
> hassle that I anticipated, so I will include regmap switch in the v2.
Ah ok. Even better.
>
> > >
> > > >
> > > > > + LM3533_ALS_INPUT_MODE_MASK);
> > > > > if (ret)
> > > > > - return ret;
> > > > > + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > > > > + als->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 (!als->pwm_mode) {
> > > > > + ret = lm3533_write(lm3533, LM3533_REG_ALS_RESISTOR_SELECT,
> > > > > + (u8)als->r_select);
> > > >
> > > > Same applies here. Mostly an unrelated change as the only thing switching that
> > > > is related to the patch is one parameter.
> > > >
> > >
> > > Removing of lm3533_write layer is not the scope of this patchset.
> > >
> > > > > if (ret)
> > > > > - return ret;
> > > > > + return dev_err_probe(dev, ret, "failed to set resistor\n");
> > > > > }
> > > > >
> > > > > return 0;
> > > >
> > > > > @@ -852,25 +825,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);
> > > >
> > > > I'm not sure why this was there in the first place. Hence not sure if it
> > > > is safe to remove.
> > > >
> > >
> > > This is directly related to OF conversion. The iio_device_set_parent
> > > bound indio_dev to parent, and it causes problems with OF now since
> > > als output has its own node and binding it to parent if wrong. Same
> > > story for backlight and leds btw.
> >
> > Is there any risk anyone was using the canonical path to get to the iio dev?
> > /sys/bus/platform/devices/..../iio\:deviceX
> > This is technically an ABI change be it a subtle one.
> >
>
> Linux kernel has no users of this driver, and it is in "stale" state
> for more then 2 years (maybe even longer). I have cc'd Johan Hovold.
>
> https://lore.kernel.org/lkml/ZmBcvtLCzllQDWVX@hovoldconsulting.com/
>
> This this 2 y. o. discussion and there were no actions ore movements.
> I assume this driver in its current form has no more users. This does
> not mean that it cannot be revived though.
So, just to check, are you a user of this code or is this more trying to
help out with old code?
Jonathan
>
> >
> > >
> > > >
> > > > > diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> > > > > index 45795f2a1042..d707d43d5526 100644
> > > > > --- a/drivers/leds/leds-lm3533.c
> > > > > +++ b/drivers/leds/leds-lm3533.c
> > > >
> > > > >
> > > > > led->cb.dev = led->cdev.dev;
> > > > >
> > > > > - ret = lm3533_led_setup(led, pdata);
> > > > > + device_property_read_u32(&pdev->dev, "led-max-microamp",
> > > > > + &led->max_current);
> > > >
> > > > I'd prefer explicit setting of the default to be visible before this, or
> > > > the property_present pattern I mention in the IIO review above.
> > > >
> > >
> > > clamp will ensure that led->max_current will be set to
> > > LM3533_LED_MAX_CURRENT_MIN regardless if it it present
> >
> > As above, I'd prefer it set explicitly.
> >
>
> I understand your position and I am not denying it for ALS part, but
> LEDs don't belong to IIO subsystem and different subsystem maintainers
> may have drastically different preferences and requirements (ugh, PTSD
> in its full glory).
>
> > >
> > > > > + led->max_current = clamp(led->max_current, LM3533_LED_MAX_CURRENT_MIN,
> > > > > + LM3533_LED_MAX_CURRENT_MAX);
> > > >
> > > > I didn't look any further (busy day!)
> > >
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller
2026-05-29 10:46 ` Daniel Thompson
@ 2026-05-29 10:56 ` Svyatoslav Ryhel
0 siblings, 0 replies; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-29 10:56 UTC (permalink / raw)
To: Daniel Thompson
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
пт, 29 трав. 2026 р. о 13:46 Daniel Thompson <daniel@riscstar.com> пише:
>
> On Fri, May 29, 2026 at 01:07:50PM +0300, Svyatoslav Ryhel wrote:
> > пт, 29 трав. 2026 р. о 12:51 Daniel Thompson <daniel@riscstar.com> пише:
> > >
> > > On Thu, May 28, 2026 at 04:51:18PM +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>
> > > > ---
> > > > .../leds/backlight/ti,lm3533-backlight.yaml | 68 +++++++
> > > > .../bindings/leds/ti,lm3533-leds.yaml | 66 +++++++
> > > > .../devicetree/bindings/leds/ti,lm3533.yaml | 170 ++++++++++++++++++
> > > > 3 files changed, 304 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..866b0fb8ed04
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/leds/backlight/ti,lm3533-backlight.yaml
> > > > @@ -0,0 +1,68 @@
> > > > +# 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
> > > > <snip>
> > > > + 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
> > >
> > > This is optional and the drive implements a default (zero) that is not
> > > documented here.
> > >
> > > Is zero a sane default from a DT binding point of view?
> > >
> >
> > Yes, if property is missing then PWM input is disabled which is
> > equivalent to setting all bits to 0.
>
> So the default should be documented in the bindings?
>
Ye, sure, I can do that.
>
> Daniel.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
2026-05-28 13:51 ` [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings Svyatoslav Ryhel
2026-05-28 14:50 ` Jonathan Cameron
@ 2026-05-29 11:00 ` Daniel Thompson
2026-05-29 11:06 ` Svyatoslav Ryhel
1 sibling, 1 reply; 24+ messages in thread
From: Daniel Thompson @ 2026-05-29 11:00 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 Thu, May 28, 2026 at 04:51:19PM +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.
> Additionally, optimize functions used only by platform data.
The last sentence is a little vague and makes us have to hunt for the
changes in a relatively large patch. If it is referring to the change to
common up the init and update code then it's would better to say that
explicitly!
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> drivers/iio/light/lm3533-als.c | 95 ++++------
> drivers/leds/leds-lm3533.c | 51 ++++--
> drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
> drivers/video/backlight/lm3533_bl.c | 52 ++++--
> include/linux/mfd/lm3533.h | 51 +-----
Just one comment for backlight, below:
> 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
> @@ -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;
Given the big changes to the driver is there any chance of putting a
good value in props.scale (BACKLIGHT_SCALE_LINEAR or
BACKLIGHT_SCALE_NON_LINEAR)?
If the difference between 50% and 100% *looks* like 50% then the scale is
non-linear (since humn perception of brightness is not linear).
Daniel.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
2026-05-29 10:48 ` Jonathan Cameron
@ 2026-05-29 11:02 ` Svyatoslav Ryhel
2026-05-29 13:10 ` Jonathan Cameron
0 siblings, 1 reply; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-29 11:02 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
пт, 29 трав. 2026 р. о 13:48 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Fri, 29 May 2026 12:39:56 +0300
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > пт, 29 трав. 2026 р. о 12:08 Jonathan Cameron <jic23@kernel.org> пише:
> > >
> > > On Thu, 28 May 2026 18:03:31 +0300
> > > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> > >
> > > > чт, 28 трав. 2026 р. о 17:50 Jonathan Cameron <jic23@kernel.org> пише:
> > > > >
> > > > > On Thu, 28 May 2026 16:51:19 +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.
> > > > > > Additionally, optimize functions used only by platform data.
> > > > >
> > > > >
> > > > > At least the IIO ones would have made much the same amount of sense for
> > > > > dt, just that they weren't having in the first place. I'd prefer that
> > >
> > > Gah. I write gibberish after too much reviewing. having/helping!
> > >
> > > > > as a precursor patch to make the rest much more readable.
> > > > >
> > > >
> > > > I can add you preferences into this commit, I don't mind.
> > > >
> > > > > >
> > > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > >
> > > > > I only looked in detail at the iio bit. A few changes requested.
> > > > >
> > > > > > ---
> > > > > > drivers/iio/light/lm3533-als.c | 95 ++++------
> > > > > > drivers/leds/leds-lm3533.c | 51 ++++--
> > > > > > drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
> > > > > > drivers/video/backlight/lm3533_bl.c | 52 ++++--
> > > > > > include/linux/mfd/lm3533.h | 51 +-----
> > > > > > 5 files changed, 212 insertions(+), 305 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > > > > > index 99f0b903018c..cbd337b73bd9 100644
> > > > > > --- a/drivers/iio/light/lm3533-als.c
> > > > > > +++ b/drivers/iio/light/lm3533-als.c
> > > > >
> > > > > > @@ -714,59 +720,33 @@ 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)
> > > > > > +static int lm3533_als_setup(struct lm3533_als *als)
> > > > > > {
> > > > > > - u8 mask = LM3533_ALS_INPUT_MODE_MASK;
> > > > > > - u8 val;
> > > > > > + struct device *dev = &als->pdev.dev;
> > > > > > 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;
> > > > > > - }
> > > > > > + device_property_read_u32(dev, "ti,resistor-value-ohm",
> > > > > > + &als->r_select);
> > > > > Does this have a default? If so the pattern we've recently be setting on for IIO
> > > > > is
> > > > > if (device_property_present(dev, "ti,resistor-value-ohm"))
> > > > > ret = device_property_read_u32();
> > > > > if (ret) //corrupt property in some fashion
> > > > > return ret;
> > > > > } else {
> > > > > //set default
> > > > > }
> > > > > If there is no default then check it unconditionally.
> > > >
> > > > default value is LM3533_ALS_RESISTOR_MIN and if no property is present
> > > > clamp will ensure that als->r_select will be set to
> > > > LM3533_ALS_RESISTOR_MIN
> > >
> > > I don't see that default in the binding doc and relying in the 0 being clamped
> > > isn't particularly readable - I'd set it explicitly.
> > >
> >
> > Oh, ye, my bad. Schema enforces one of props to be present and if pwn
> > is present then resistor is ignored. What if I move resistor reading,
> > clamping and conversion under !als->pwm_mode check? Then resistor must
> > be present and hence must be checked unconditionally.
>
> Sounds good.
>
> >
> > Additionally, I can comment original lm3533_als_setup with #if 0
> > #endif then git formatting will be much cleaner and easier to review,
> > and once we all come to result I will just remove entire commented
> > block and Lee can pick clean commits.
>
> No don't do that. If you flatten the two helpers as a precursor patch
> then the changes in here will be easier to review anyway.
>
By "flatten the two helpers" you mean incorporate
lm3533_als_set_input_mode and lm3533_als_set_resistor into
lm3533_als_setup first and then convert it to use DT? I am asking,
just to be sure.
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > - return 0;
> > > > > > -}
> > > > > > + 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);
> > > > > >
> > > > > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > > > > - const struct lm3533_als_platform_data *pdata)
> > > > > > -{
> > > > > > - int ret;
> > > > > > + als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > > > > >
> > > > > > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > > > > > + ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, als->pwm_mode ?
> > > > > > + LM3533_ALS_INPUT_MODE_MASK : 0,
> > > > >
> > > > > That's ugly. Better as
> > > > >
> > > > > ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF,
> > > > > als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0,
> > > > >
> > > >
> > > > Yes sure, just followed 80 char limit.
> > > >
> > > > > Though if there wasn't a layer hiding the regmap, it could just have been
> > > > >
> > > > > ret = regmap_assign_bits(lm3533->regmap, LM3533_REG_ALS_CONF,
> > > > > LM3533_ALS_INPUT_MODE_MASK, als->pwm_mode);;
> > > > >
> > > > > which would have been nicer.
> > > > >
> > > > > I'm not particularly keen on the swashing of the helpers being in a patch
> > >
> > > smashing. (this definitely wasn't my best effort at English!)
> > >
> > > > > that is about switching the binding type as feels largely unrelated.
> > > > > Should really have been a precursor, easier to review patch.
> > > > >
> > > >
> > > > Removing of lm3533_update layer is not the scope of this patchset.
> > >
> > > Understood. I'm fine with just the refactor you are doing brought out as a precursor
> > > patch.
> > >
> >
> > I have looked into removing wrappers too. That seems to be less a
> > hassle that I anticipated, so I will include regmap switch in the v2.
>
> Ah ok. Even better.
>
> >
> > > >
> > > > >
> > > > > > + LM3533_ALS_INPUT_MODE_MASK);
> > > > > > if (ret)
> > > > > > - return ret;
> > > > > > + return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > > > > > + als->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 (!als->pwm_mode) {
> > > > > > + ret = lm3533_write(lm3533, LM3533_REG_ALS_RESISTOR_SELECT,
> > > > > > + (u8)als->r_select);
> > > > >
> > > > > Same applies here. Mostly an unrelated change as the only thing switching that
> > > > > is related to the patch is one parameter.
> > > > >
> > > >
> > > > Removing of lm3533_write layer is not the scope of this patchset.
> > > >
> > > > > > if (ret)
> > > > > > - return ret;
> > > > > > + return dev_err_probe(dev, ret, "failed to set resistor\n");
> > > > > > }
> > > > > >
> > > > > > return 0;
> > > > >
> > > > > > @@ -852,25 +825,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);
> > > > >
> > > > > I'm not sure why this was there in the first place. Hence not sure if it
> > > > > is safe to remove.
> > > > >
> > > >
> > > > This is directly related to OF conversion. The iio_device_set_parent
> > > > bound indio_dev to parent, and it causes problems with OF now since
> > > > als output has its own node and binding it to parent if wrong. Same
> > > > story for backlight and leds btw.
> > >
> > > Is there any risk anyone was using the canonical path to get to the iio dev?
> > > /sys/bus/platform/devices/..../iio\:deviceX
> > > This is technically an ABI change be it a subtle one.
> > >
> >
> > Linux kernel has no users of this driver, and it is in "stale" state
> > for more then 2 years (maybe even longer). I have cc'd Johan Hovold.
> >
> > https://lore.kernel.org/lkml/ZmBcvtLCzllQDWVX@hovoldconsulting.com/
> >
> > This this 2 y. o. discussion and there were no actions ore movements.
> > I assume this driver in its current form has no more users. This does
> > not mean that it cannot be revived though.
>
> So, just to check, are you a user of this code or is this more trying to
> help out with old code?
>
I am not insane enough to get myself into all this conversion if I did
not need it. This is one of 2 remaining gaps in support of LG
P880/P895 I own and support. I would really like to finally mainline
all the patches I have locally since maintaining them becomes quite
troublesome with time and additional patches layering on top.
> Jonathan
>
> >
> > >
> > > >
> > > > >
> > > > > > diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> > > > > > index 45795f2a1042..d707d43d5526 100644
> > > > > > --- a/drivers/leds/leds-lm3533.c
> > > > > > +++ b/drivers/leds/leds-lm3533.c
> > > > >
> > > > > >
> > > > > > led->cb.dev = led->cdev.dev;
> > > > > >
> > > > > > - ret = lm3533_led_setup(led, pdata);
> > > > > > + device_property_read_u32(&pdev->dev, "led-max-microamp",
> > > > > > + &led->max_current);
> > > > >
> > > > > I'd prefer explicit setting of the default to be visible before this, or
> > > > > the property_present pattern I mention in the IIO review above.
> > > > >
> > > >
> > > > clamp will ensure that led->max_current will be set to
> > > > LM3533_LED_MAX_CURRENT_MIN regardless if it it present
> > >
> > > As above, I'd prefer it set explicitly.
> > >
> >
> > I understand your position and I am not denying it for ALS part, but
> > LEDs don't belong to IIO subsystem and different subsystem maintainers
> > may have drastically different preferences and requirements (ugh, PTSD
> > in its full glory).
> >
> > > >
> > > > > > + led->max_current = clamp(led->max_current, LM3533_LED_MAX_CURRENT_MIN,
> > > > > > + LM3533_LED_MAX_CURRENT_MAX);
> > > > >
> > > > > I didn't look any further (busy day!)
> > > >
> > >
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
2026-05-29 11:00 ` Daniel Thompson
@ 2026-05-29 11:06 ` Svyatoslav Ryhel
0 siblings, 0 replies; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-29 11:06 UTC (permalink / raw)
To: Daniel Thompson
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
пт, 29 трав. 2026 р. о 14:00 Daniel Thompson <daniel@riscstar.com> пише:
>
> On Thu, May 28, 2026 at 04:51:19PM +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.
> > Additionally, optimize functions used only by platform data.
>
> The last sentence is a little vague and makes us have to hunt for the
> changes in a relatively large patch. If it is referring to the change to
> common up the init and update code then it's would better to say that
> explicitly!
>
If I understood Jonathan properly, the last sentence will get its own patch.
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> > drivers/iio/light/lm3533-als.c | 95 ++++------
> > drivers/leds/leds-lm3533.c | 51 ++++--
> > drivers/mfd/lm3533-core.c | 268 ++++++++++------------------
> > drivers/video/backlight/lm3533_bl.c | 52 ++++--
> > include/linux/mfd/lm3533.h | 51 +-----
>
> Just one comment for backlight, below:
>
> > 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
> > @@ -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;
>
> Given the big changes to the driver is there any chance of putting a
> good value in props.scale (BACKLIGHT_SCALE_LINEAR or
> BACKLIGHT_SCALE_NON_LINEAR)?
>
> If the difference between 50% and 100% *looks* like 50% then the scale is
> non-linear (since humn perception of brightness is not linear).
>
Yes! But not in this patch. This patchset has a dedicated patch
implementing linear and non-linear configuration from tree which may
include this configuration as well. No guarantees though, but I will
keep in mind this request. Thanks!
>
> Daniel.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 5/6] video: backlight: lm3533_bl: Set initial mapping mode from DT
2026-05-28 13:51 ` [PATCH v2 5/6] video: backlight: lm3533_bl: Set initial mapping mode from DT Svyatoslav Ryhel
@ 2026-05-29 11:10 ` Daniel Thompson
2026-05-29 11:17 ` Svyatoslav Ryhel
0 siblings, 1 reply; 24+ messages in thread
From: Daniel Thompson @ 2026-05-29 11:10 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 Thu, May 28, 2026 at 04:51:22PM +0300, Svyatoslav Ryhel wrote:
> Add support to obtain the initial mapping mode from DT instead of leaving
> it unconfigured. Additionally, update the linear sysfs code, which uses a
> similar coding pattern.
Words like "additionally" in a patch description can be a sign the patch
should actually be two patches. In this case the patch would be a lot
easier to read if you cleaned up the linear sysfs code (patch N) and then
added the new DT logic (patch N+1).
Daniel.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 5/6] video: backlight: lm3533_bl: Set initial mapping mode from DT
2026-05-29 11:10 ` Daniel Thompson
@ 2026-05-29 11:17 ` Svyatoslav Ryhel
2026-05-29 11:40 ` Daniel Thompson
0 siblings, 1 reply; 24+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-29 11:17 UTC (permalink / raw)
To: Daniel Thompson
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
пт, 29 трав. 2026 р. о 14:10 Daniel Thompson <daniel@riscstar.com> пише:
>
> On Thu, May 28, 2026 at 04:51:22PM +0300, Svyatoslav Ryhel wrote:
> > Add support to obtain the initial mapping mode from DT instead of leaving
> > it unconfigured. Additionally, update the linear sysfs code, which uses a
> > similar coding pattern.
>
> Words like "additionally" in a patch description can be a sign the patch
> should actually be two patches. In this case the patch would be a lot
> easier to read if you cleaned up the linear sysfs code (patch N) and then
> added the new DT logic (patch N+1).
>
I looked into this in reverse. My goal was to add DT logic I don't
case how sysfs works. My code matched with what sysfs does I just
included sysfs change as well. I might better drop sysfs changes
entirely since with such pace this patchset will inflate from 6 to 15
and beyond.
>
> Daniel.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 5/6] video: backlight: lm3533_bl: Set initial mapping mode from DT
2026-05-29 11:17 ` Svyatoslav Ryhel
@ 2026-05-29 11:40 ` Daniel Thompson
0 siblings, 0 replies; 24+ messages in thread
From: Daniel Thompson @ 2026-05-29 11:40 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 Fri, May 29, 2026 at 02:17:00PM +0300, Svyatoslav Ryhel wrote:
> пт, 29 трав. 2026 р. о 14:10 Daniel Thompson <daniel@riscstar.com> пише:
> >
> > On Thu, May 28, 2026 at 04:51:22PM +0300, Svyatoslav Ryhel wrote:
> > > Add support to obtain the initial mapping mode from DT instead of leaving
> > > it unconfigured. Additionally, update the linear sysfs code, which uses a
> > > similar coding pattern.
> >
> > Words like "additionally" in a patch description can be a sign the patch
> > should actually be two patches. In this case the patch would be a lot
> > easier to read if you cleaned up the linear sysfs code (patch N) and then
> > added the new DT logic (patch N+1).
> >
>
> I looked into this in reverse. My goal was to add DT logic I don't
> case how sysfs works. My code matched with what sysfs does I just
> included sysfs change as well. I might better drop sysfs changes
> entirely since with such pace this patchset will inflate from 6 to 15
> and beyond.
Not sure about that. The clean up is good and introducing the new
BIT(2 * n + 1) macro does need to be used pervasively or there's no
point in adding it.
Daniel.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings
2026-05-29 11:02 ` Svyatoslav Ryhel
@ 2026-05-29 13:10 ` Jonathan Cameron
0 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2026-05-29 13:10 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
> > > > > > if (device_property_present(dev, "ti,resistor-value-ohm"))
> > > > > > ret = device_property_read_u32();
> > > > > > if (ret) //corrupt property in some fashion
> > > > > > return ret;
> > > > > > } else {
> > > > > > //set default
> > > > > > }
> > > > > > If there is no default then check it unconditionally.
> > > > >
> > > > > default value is LM3533_ALS_RESISTOR_MIN and if no property is present
> > > > > clamp will ensure that als->r_select will be set to
> > > > > LM3533_ALS_RESISTOR_MIN
> > > >
> > > > I don't see that default in the binding doc and relying in the 0 being clamped
> > > > isn't particularly readable - I'd set it explicitly.
> > > >
> > >
> > > Oh, ye, my bad. Schema enforces one of props to be present and if pwn
> > > is present then resistor is ignored. What if I move resistor reading,
> > > clamping and conversion under !als->pwm_mode check? Then resistor must
> > > be present and hence must be checked unconditionally.
> >
> > Sounds good.
> >
> > >
> > > Additionally, I can comment original lm3533_als_setup with #if 0
> > > #endif then git formatting will be much cleaner and easier to review,
> > > and once we all come to result I will just remove entire commented
> > > block and Lee can pick clean commits.
> >
> > No don't do that. If you flatten the two helpers as a precursor patch
> > then the changes in here will be easier to review anyway.
> >
>
> By "flatten the two helpers" you mean incorporate
> lm3533_als_set_input_mode and lm3533_als_set_resistor into
> lm3533_als_setup first and then convert it to use DT? I am asking,
> just to be sure.
>
yes
> > > > > > > @@ -852,25 +825,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);
> > > > > >
> > > > > > I'm not sure why this was there in the first place. Hence not sure if it
> > > > > > is safe to remove.
> > > > > >
> > > > >
> > > > > This is directly related to OF conversion. The iio_device_set_parent
> > > > > bound indio_dev to parent, and it causes problems with OF now since
> > > > > als output has its own node and binding it to parent if wrong. Same
> > > > > story for backlight and leds btw.
> > > >
> > > > Is there any risk anyone was using the canonical path to get to the iio dev?
> > > > /sys/bus/platform/devices/..../iio\:deviceX
> > > > This is technically an ABI change be it a subtle one.
> > > >
> > >
> > > Linux kernel has no users of this driver, and it is in "stale" state
> > > for more then 2 years (maybe even longer). I have cc'd Johan Hovold.
> > >
> > > https://lore.kernel.org/lkml/ZmBcvtLCzllQDWVX@hovoldconsulting.com/
> > >
> > > This this 2 y. o. discussion and there were no actions ore movements.
> > > I assume this driver in its current form has no more users. This does
> > > not mean that it cannot be revived though.
> >
> > So, just to check, are you a user of this code or is this more trying to
> > help out with old code?
> >
>
> I am not insane enough to get myself into all this conversion if I did
> not need it. This is one of 2 remaining gaps in support of LG
> P880/P895 I own and support. I would really like to finally mainline
> all the patches I have locally since maintaining them becomes quite
> troublesome with time and additional patches layering on top.
Excellent! There are some odd people out there who do start on this
sort of thing despite no personal use case :)
Jonathan
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2026-05-29 13:10 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-28 13:51 [PATCH v2 0/6] mfd: lm3533: convert to OF bindings, improve support Svyatoslav Ryhel
2026-05-28 13:51 ` [PATCH v2 1/6] dt-bindings: leds: Document TI LM3533 LED controller Svyatoslav Ryhel
2026-05-28 14:54 ` Jonathan Cameron
2026-05-29 9:51 ` Daniel Thompson
2026-05-29 10:07 ` Svyatoslav Ryhel
2026-05-29 10:46 ` Daniel Thompson
2026-05-29 10:56 ` Svyatoslav Ryhel
2026-05-28 13:51 ` [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings Svyatoslav Ryhel
2026-05-28 14:50 ` Jonathan Cameron
2026-05-28 15:03 ` Svyatoslav Ryhel
2026-05-29 9:08 ` Jonathan Cameron
2026-05-29 9:39 ` Svyatoslav Ryhel
2026-05-29 10:48 ` Jonathan Cameron
2026-05-29 11:02 ` Svyatoslav Ryhel
2026-05-29 13:10 ` Jonathan Cameron
2026-05-29 11:00 ` Daniel Thompson
2026-05-29 11:06 ` Svyatoslav Ryhel
2026-05-28 13:51 ` [PATCH v2 3/6] mfd: lm3533: Add support for VIN power supply Svyatoslav Ryhel
2026-05-28 13:51 ` [PATCH v2 4/6] mfd: lm3533: Set DMA mask Svyatoslav Ryhel
2026-05-28 13:51 ` [PATCH v2 5/6] video: backlight: lm3533_bl: Set initial mapping mode from DT Svyatoslav Ryhel
2026-05-29 11:10 ` Daniel Thompson
2026-05-29 11:17 ` Svyatoslav Ryhel
2026-05-29 11:40 ` Daniel Thompson
2026-05-28 13:51 ` [PATCH v2 6/6] video: leds: backlight: lm3533: Support getting LED sources " Svyatoslav Ryhel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox