* [PATCH v3 0/2] mfd: lm3533: convert to use OF
@ 2025-02-24 11:48 Svyatoslav Ryhel
2025-02-24 11:48 ` [PATCH v3 1/2] dt-bindings: mfd: Document TI LM3533 MFD Svyatoslav Ryhel
2025-02-24 11:48 ` [PATCH v3 2/2] mfd: lm3533: convert to use OF Svyatoslav Ryhel
0 siblings, 2 replies; 10+ messages in thread
From: Svyatoslav Ryhel @ 2025-02-24 11:48 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Lars-Peter Clausen, Pavel Machek,
Daniel Thompson, Jingoo Han, Helge Deller, Svyatoslav Ryhel,
Andy Shevchenko, Uwe Kleine-König
Cc: devicetree, linux-kernel, linux-iio, linux-leds, dri-devel,
linux-fbdev
Add schema and add support for lm3533 mfd to use device tree bindings.
---
Changes on switching from v2 to v3:
- wrapped lines in schema and commit messages arround 80 chars
- removed |
- switched to MFD binding style
- completed binding example
- restored MFD
Changes on switching from v1 to v2:
- added unit seffix where it is suitable
- added vendor prefixes where it is suitable
- light sensor mover out of pattern properties
- added references to common schemas
- added detailed descriptions of properties
- removed platform data use
- devices bind and configure themself entirely
using device tree
---
Svyatoslav Ryhel (2):
dt-bindings: mfd: Document TI LM3533 MFD
mfd: lm3533: convert to use OF
.../devicetree/bindings/mfd/ti,lm3533.yaml | 231 ++++++++++++++++++
drivers/iio/light/lm3533-als.c | 40 ++-
drivers/leds/leds-lm3533.c | 46 ++--
drivers/mfd/lm3533-core.c | 159 ++++--------
drivers/video/backlight/lm3533_bl.c | 71 ++++--
include/linux/mfd/lm3533.h | 35 +--
6 files changed, 395 insertions(+), 187 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
--
2.43.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/2] dt-bindings: mfd: Document TI LM3533 MFD
2025-02-24 11:48 [PATCH v3 0/2] mfd: lm3533: convert to use OF Svyatoslav Ryhel
@ 2025-02-24 11:48 ` Svyatoslav Ryhel
2025-02-24 20:31 ` Rob Herring
2025-02-24 11:48 ` [PATCH v3 2/2] mfd: lm3533: convert to use OF Svyatoslav Ryhel
1 sibling, 1 reply; 10+ messages in thread
From: Svyatoslav Ryhel @ 2025-02-24 11:48 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Lars-Peter Clausen, Pavel Machek,
Daniel Thompson, Jingoo Han, Helge Deller, Svyatoslav Ryhel,
Andy Shevchenko, Uwe Kleine-König
Cc: devicetree, linux-kernel, linux-iio, linux-leds, dri-devel,
linux-fbdev
Add bindings for the LM3533 - a complete power source for backlight, keypad
and indicator LEDs in smartphone handsets. The high-voltage inductive boost
converter provides the power for two series LED strings display backlight
and keypad functions.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
.../devicetree/bindings/mfd/ti,lm3533.yaml | 231 ++++++++++++++++++
1 file changed, 231 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
diff --git a/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
new file mode 100644
index 000000000000..c8ac6d4424aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
@@ -0,0 +1,231 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/ti,lm3533.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI LM3533 Complete Lighting Power Solution
+
+description: >
+ The LM3533 is a complete power source for backlight, keypad, and indicator LEDs
+ in smartphone handsets. The high-voltage inductive boost converter provides the
+ power for two series LED strings display backlight and keypad functions.
+
+ https://www.ti.com/product/LM3533
+
+maintainers:
+ - Svyatoslav Ryhel <clamor95@gmail.com>
+
+properties:
+ compatible:
+ const: ti,lm3533
+
+ reg:
+ maxItems: 1
+
+ enable-gpios:
+ description: GPIO to use to enable/disable the backlight (HWEN pin).
+ maxItems: 1
+
+ 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
+ description:
+ Properties for an illumination sensor.
+ additionalProperties: false
+
+ properties:
+ compatible:
+ const: ti,lm3533-als
+
+ ti,resistor-value-ohm:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Internal configuration resister value when ALS is in Analog Sensor
+ mode and PWM mode is disabled.
+ minimum: 1575
+ maximum: 200000
+
+ ti,pwm-mode:
+ type: boolean
+ description:
+ Switch for mode in which ALS is running. If this propertly is set
+ then ALS is running in PWM mode, internal resistor value is set to
+ high-impedance (0) and resistor-value-ohm propertly is ignored.
+
+ required:
+ - compatible
+
+required:
+ - compatible
+ - reg
+ - enable-gpios
+ - light-sensor
+ - backlight-0
+ - backlight-1
+ - led-0
+ - led-1
+ - led-2
+ - led-3
+
+patternProperties:
+ "^backlight-[01]$":
+ type: object
+ description:
+ Properties for a backlight device.
+
+ $ref: /schemas/leds/backlight/common.yaml#
+
+ properties:
+ compatible:
+ const: ti,lm3533-backlight
+
+ default-brightness: true
+
+ ti,max-current-microamp:
+ description:
+ Maximum current in µA with a 800 µA step.
+ enum: [ 5000, 5800, 6600, 7400, 8200, 9000, 9800,
+ 10600, 11400, 12200, 13000, 13800, 14600,
+ 15400, 16200, 17000, 17800, 18600, 19400,
+ 20200, 21000, 21800, 22600, 23400, 24200,
+ 25000, 25800, 26600, 27400, 28200, 29000,
+ 29800 ]
+ default: 5000
+
+ 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
+
+ ti,hardware-controlled:
+ description:
+ Each backlight has its own voltage Control Bank (A and B) and there are
+ two HVLED sinks which by default are linked to respective Bank. Setting
+ this property will link both sinks to a Control Bank of backlight where
+ property is defined.
+ type: boolean
+
+ required:
+ - compatible
+
+ additionalProperties: false
+
+ "^led-[0-3]$":
+ type: object
+ description:
+ Properties for a led device.
+
+ $ref: /schemas/leds/common.yaml#
+
+ properties:
+ compatible:
+ const: ti,lm3533-leds
+
+ linux,default-trigger: true
+
+ ti,max-current-microamp:
+ description:
+ Maximum current in µA with a 800 µA step.
+ enum: [ 5000, 5800, 6600, 7400, 8200, 9000, 9800,
+ 10600, 11400, 12200, 13000, 13800, 14600,
+ 15400, 16200, 17000, 17800, 18600, 19400,
+ 20200, 21000, 21800, 22600, 23400, 24200,
+ 25000, 25800, 26600, 27400, 28200, 29000,
+ 29800 ]
+ default: 5000
+
+ ti,pwm-config-mask:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Same descryption and function as for backlight.
+
+ required:
+ - compatible
+
+ additionalProperties: false
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led-controller@36 {
+ compatible = "ti,lm3533";
+ reg = <0x36>;
+
+ enable-gpios = <&gpio 110 GPIO_ACTIVE_HIGH>;
+
+ ti,boost-ovp-microvolt = <24000000>;
+ ti,boost-freq-hz = <500000>;
+
+ backlight-0 {
+ compatible = "ti,lm3533-backlight";
+
+ ti,max-current-microamp = <23400>;
+ default-brightness = <113>;
+ ti,hardware-controlled;
+ };
+
+ backlight-1 {
+ compatible = "ti,lm3533-backlight";
+ status = "disabled";
+ };
+
+ led-0 {
+ compatible = "ti,lm3533-leds";
+ status = "disabled";
+ };
+
+ led-1 {
+ compatible = "ti,lm3533-leds";
+ status = "disabled";
+ };
+
+ led-2 {
+ compatible = "ti,lm3533-leds";
+ status = "disabled";
+ };
+
+ led-3 {
+ compatible = "ti,lm3533-leds";
+ status = "disabled";
+ };
+
+ light-sensor {
+ compatible = "ti,lm3533-als";
+ status = "disabled";
+ };
+ };
+ };
+...
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 2/2] mfd: lm3533: convert to use OF
2025-02-24 11:48 [PATCH v3 0/2] mfd: lm3533: convert to use OF Svyatoslav Ryhel
2025-02-24 11:48 ` [PATCH v3 1/2] dt-bindings: mfd: Document TI LM3533 MFD Svyatoslav Ryhel
@ 2025-02-24 11:48 ` Svyatoslav Ryhel
2025-02-28 8:59 ` Lee Jones
1 sibling, 1 reply; 10+ messages in thread
From: Svyatoslav Ryhel @ 2025-02-24 11:48 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Lars-Peter Clausen, Pavel Machek,
Daniel Thompson, Jingoo Han, Helge Deller, Svyatoslav Ryhel,
Andy Shevchenko, Uwe Kleine-König
Cc: devicetree, linux-kernel, linux-iio, linux-leds, dri-devel,
linux-fbdev
Remove platform data and fully relay on OF and device tree
parsing and binding devices.
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
drivers/iio/light/lm3533-als.c | 40 ++++---
drivers/leds/leds-lm3533.c | 46 +++++---
drivers/mfd/lm3533-core.c | 159 ++++++++--------------------
drivers/video/backlight/lm3533_bl.c | 71 ++++++++++---
include/linux/mfd/lm3533.h | 35 +-----
5 files changed, 164 insertions(+), 187 deletions(-)
diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
index 99f0b903018c..cb52965e93c6 100644
--- a/drivers/iio/light/lm3533-als.c
+++ b/drivers/iio/light/lm3533-als.c
@@ -16,9 +16,12 @@
#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>
@@ -56,6 +59,9 @@ struct lm3533_als {
atomic_t zone;
struct mutex thresh_mutex;
+
+ unsigned pwm_mode:1; /* PWM input mode (default analog) */
+ u8 r_select; /* 1 - 127 (ignored in PWM-mode) */
};
@@ -753,18 +759,17 @@ static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
return 0;
}
-static int lm3533_als_setup(struct lm3533_als *als,
- const struct lm3533_als_platform_data *pdata)
+static int lm3533_als_setup(struct lm3533_als *als)
{
int ret;
- ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
+ ret = lm3533_als_set_input_mode(als, als->pwm_mode);
if (ret)
return ret;
/* ALS input is always high impedance in PWM-mode. */
- if (!pdata->pwm_mode) {
- ret = lm3533_als_set_resistor(als, pdata->r_select);
+ if (!als->pwm_mode) {
+ ret = lm3533_als_set_resistor(als, als->r_select);
if (ret)
return ret;
}
@@ -828,22 +833,16 @@ 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;
+ u32 val;
int ret;
lm3533 = dev_get_drvdata(pdev->dev.parent);
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;
@@ -864,13 +863,21 @@ static int lm3533_als_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, indio_dev);
+ val = 200 * KILO; /* 200kOhm */
+ device_property_read_u32(&pdev->dev, "ti,resistor-value-ohm", &val);
+
+ /* Convert resitance into R_ALS value with 2v / 10uA * R */
+ als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * val);
+
+ als->pwm_mode = device_property_read_bool(&pdev->dev, "ti,pwm-mode");
+
if (als->irq) {
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 +914,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..6e661a2a540f 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;
+
+ u16 max_current; /* 5000 - 29800 uA (800 uA step) */
+ u8 pwm; /* 0 - 0x3f */
};
@@ -632,23 +637,22 @@ static const struct attribute_group *lm3533_led_attribute_groups[] = {
NULL
};
-static int lm3533_led_setup(struct lm3533_led *led,
- struct lm3533_led_platform_data *pdata)
+static int lm3533_led_setup(struct lm3533_led *led)
{
int ret;
- ret = lm3533_ctrlbank_set_max_current(&led->cb, pdata->max_current);
+ ret = lm3533_ctrlbank_set_max_current(&led->cb, led->max_current);
if (ret)
return ret;
- return lm3533_ctrlbank_set_pwm(&led->cb, pdata->pwm);
+ return lm3533_ctrlbank_set_pwm(&led->cb, led->pwm);
}
static int lm3533_led_probe(struct platform_device *pdev)
{
struct lm3533 *lm3533;
- struct lm3533_led_platform_data *pdata;
struct lm3533_led *led;
+ u32 val;
int ret;
dev_dbg(&pdev->dev, "%s\n", __func__);
@@ -657,12 +661,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 +671,11 @@ 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.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
+ pdev->name, pdev->id);
+ led->cdev.default_trigger = "none";
+ device_property_read_string(&pdev->dev, "linux,default-trigger",
+ &led->cdev.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;
@@ -702,7 +703,17 @@ static int lm3533_led_probe(struct platform_device *pdev)
led->cb.dev = led->cdev.dev;
- ret = lm3533_led_setup(led, pdata);
+ /* 5000 - 29800 uA (800 uA step) */
+ val = 5000;
+ device_property_read_u32(&pdev->dev, "ti,max-current-microamp", &val);
+ led->max_current = val;
+
+ /* 0 - 0x3f */
+ val = 0;
+ device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &val);
+ led->pwm = val;
+
+ ret = lm3533_led_setup(led);
if (ret)
goto err_deregister;
@@ -739,9 +750,16 @@ static void lm3533_led_shutdown(struct platform_device *pdev)
lm3533_led_set(&led->cdev, LED_OFF); /* disable blink */
}
+static const struct of_device_id lm3533_led_match_table[] = {
+ { .compatible = "ti,lm3533-leds" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, lm3533_led_match_table);
+
static struct platform_driver lm3533_led_driver = {
.driver = {
.name = "lm3533-leds",
+ .of_match_table = lm3533_led_match_table,
},
.probe = lm3533_led_probe,
.remove = lm3533_led_remove,
diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
index 0a2409d00b2e..e1b8fe136af9 100644
--- a/drivers/mfd/lm3533-core.c
+++ b/drivers/mfd/lm3533-core.c
@@ -14,10 +14,13 @@
#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>
@@ -42,41 +45,41 @@
#define LM3533_REG_MAX 0xb2
-
-static struct mfd_cell lm3533_als_devs[] = {
+static struct mfd_cell lm3533_child_devices[] = {
{
.name = "lm3533-als",
.id = -1,
+ .of_compatible = "ti,lm3533-als",
},
-};
-
-static struct mfd_cell lm3533_bl_devs[] = {
{
.name = "lm3533-backlight",
.id = 0,
+ .of_compatible = "ti,lm3533-backlight",
},
{
.name = "lm3533-backlight",
.id = 1,
+ .of_compatible = "ti,lm3533-backlight",
},
-};
-
-static struct mfd_cell lm3533_led_devs[] = {
{
.name = "lm3533-leds",
.id = 0,
+ .of_compatible = "ti,lm3533-leds",
},
{
.name = "lm3533-leds",
.id = 1,
+ .of_compatible = "ti,lm3533-leds",
},
{
.name = "lm3533-leds",
.id = 2,
+ .of_compatible = "ti,lm3533-leds",
},
{
.name = "lm3533-leds",
.id = 3,
+ .of_compatible = "ti,lm3533-leds",
},
};
@@ -376,136 +379,60 @@ static struct attribute_group lm3533_attribute_group = {
.attrs = lm3533_attributes
};
-static int lm3533_device_als_init(struct lm3533 *lm3533)
-{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
- int ret;
-
- if (!pdata->als)
- return 0;
-
- lm3533_als_devs[0].platform_data = pdata->als;
- lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
-
- ret = mfd_add_devices(lm3533->dev, 0, lm3533_als_devs, 1, NULL,
- 0, NULL);
- if (ret) {
- dev_err(lm3533->dev, "failed to add ALS device\n");
- return ret;
- }
-
- lm3533->have_als = 1;
-
- return 0;
-}
-
-static int lm3533_device_bl_init(struct lm3533 *lm3533)
-{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
- int i;
- int ret;
-
- if (!pdata->backlights || pdata->num_backlights == 0)
- return 0;
-
- if (pdata->num_backlights > ARRAY_SIZE(lm3533_bl_devs))
- pdata->num_backlights = ARRAY_SIZE(lm3533_bl_devs);
-
- for (i = 0; i < pdata->num_backlights; ++i) {
- lm3533_bl_devs[i].platform_data = &pdata->backlights[i];
- lm3533_bl_devs[i].pdata_size = sizeof(pdata->backlights[i]);
- }
-
- ret = mfd_add_devices(lm3533->dev, 0, lm3533_bl_devs,
- pdata->num_backlights, NULL, 0, NULL);
- if (ret) {
- dev_err(lm3533->dev, "failed to add backlight devices\n");
- return ret;
- }
-
- lm3533->have_backlights = 1;
-
- return 0;
-}
-
-static int lm3533_device_led_init(struct lm3533 *lm3533)
-{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
- int i;
- int ret;
-
- if (!pdata->leds || pdata->num_leds == 0)
- return 0;
-
- if (pdata->num_leds > ARRAY_SIZE(lm3533_led_devs))
- pdata->num_leds = ARRAY_SIZE(lm3533_led_devs);
-
- for (i = 0; i < pdata->num_leds; ++i) {
- lm3533_led_devs[i].platform_data = &pdata->leds[i];
- lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
- }
-
- ret = mfd_add_devices(lm3533->dev, 0, lm3533_led_devs,
- pdata->num_leds, NULL, 0, NULL);
- if (ret) {
- dev_err(lm3533->dev, "failed to add LED devices\n");
- return ret;
- }
-
- lm3533->have_leds = 1;
-
- return 0;
-}
-
-static int lm3533_device_setup(struct lm3533 *lm3533,
- struct lm3533_platform_data *pdata)
+static int lm3533_device_setup(struct lm3533 *lm3533)
{
int ret;
- ret = lm3533_set_boost_freq(lm3533, pdata->boost_freq);
+ ret = lm3533_set_boost_freq(lm3533, lm3533->boost_freq);
if (ret)
return ret;
- return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
+ return lm3533_set_boost_ovp(lm3533, lm3533->boost_ovp);
}
static int lm3533_device_init(struct lm3533 *lm3533)
{
- struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
+ u32 val;
int ret;
- dev_dbg(lm3533->dev, "%s\n", __func__);
+ lm3533->hwen = devm_gpiod_get(lm3533->dev, "enable", GPIOD_OUT_LOW);
+ if (IS_ERR(lm3533->hwen))
+ return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen),
+ "failed to request HWEN GPIO\n");
- if (!pdata) {
- dev_err(lm3533->dev, "no platform data\n");
- return -EINVAL;
- }
+ val = 16 * MICRO; /* 16V */
+ device_property_read_u32(lm3533->dev, "ti,boost-ovp-microvolt", &val);
- 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");
+ /* boost_ovp is defined in microvolts, convert to enum value */
+ lm3533->boost_ovp = val / (8 * MICRO) - 2;
+
+ val = 500 * KILO; /* 500kHz */
+ device_property_read_u32(lm3533->dev, "ti,boost-freq-hz", &val);
+
+ /* boost_freq is defined in Hz, convert to enum value */
+ lm3533->boost_freq = val / (500 * KILO) - 1;
lm3533_enable(lm3533);
- ret = lm3533_device_setup(lm3533, pdata);
+ ret = lm3533_device_setup(lm3533);
if (ret)
goto err_disable;
- lm3533_device_als_init(lm3533);
- lm3533_device_bl_init(lm3533);
- lm3533_device_led_init(lm3533);
+ ret = devm_mfd_add_devices(lm3533->dev, 0, lm3533_child_devices,
+ ARRAY_SIZE(lm3533_child_devices), NULL, 0, NULL);
+ if (ret) {
+ dev_err(lm3533->dev, "failed to add MFD devices: %d\n", ret);
+ goto err_disable;
+ }
ret = sysfs_create_group(&lm3533->dev->kobj, &lm3533_attribute_group);
if (ret < 0) {
dev_err(lm3533->dev, "failed to create sysfs attributes\n");
- goto err_unregister;
+ goto err_disable;
}
return 0;
-err_unregister:
- mfd_remove_devices(lm3533->dev);
err_disable:
lm3533_disable(lm3533);
@@ -517,8 +444,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);
}
@@ -591,6 +516,9 @@ static int lm3533_i2c_probe(struct i2c_client *i2c)
lm3533->dev = &i2c->dev;
lm3533->irq = i2c->irq;
+ i2c->dev.coherent_dma_mask = 0;
+ i2c->dev.dma_mask = &i2c->dev.coherent_dma_mask;
+
return lm3533_device_init(lm3533);
}
@@ -603,6 +531,12 @@ static void lm3533_i2c_remove(struct i2c_client *i2c)
lm3533_device_exit(lm3533);
}
+static const struct of_device_id lm3533_match_table[] = {
+ { .compatible = "ti,lm3533" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, lm3533_match_table);
+
static const struct i2c_device_id lm3533_i2c_ids[] = {
{ "lm3533" },
{ }
@@ -612,6 +546,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..0827a5e98dbb 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>
@@ -19,6 +21,7 @@
#define LM3533_HVCTRLBANK_COUNT 2
#define LM3533_BL_MAX_BRIGHTNESS 255
+#define LM3533_REG_OUTPUT_CONF1 0x10
#define LM3533_REG_CTRLBANK_AB_BCONF 0x1a
@@ -27,6 +30,11 @@ struct lm3533_bl {
struct lm3533_ctrlbank cb;
struct backlight_device *bd;
int id;
+
+ u16 max_current; /* 5000 - 29800 uA (800 uA step) */
+ u8 pwm; /* 0 - 0x3f */
+ bool linear;
+ bool hvled;
};
@@ -246,25 +254,40 @@ 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 id = lm3533_bl_get_ctrlbank_id(bl);
int ret;
- ret = lm3533_ctrlbank_set_max_current(&bl->cb, pdata->max_current);
+ if (bl->linear) {
+ ret = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF,
+ BIT(2 * id + 1), BIT(2 * id + 1));
+ if (ret)
+ return ret;
+ }
+
+ if (bl->hvled) {
+ ret = lm3533_update(bl->lm3533, LM3533_REG_OUTPUT_CONF1,
+ id | id << 1, BIT(0) | BIT(1));
+ if (ret)
+ return ret;
+ }
+
+ 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;
+ u32 val;
int ret;
dev_dbg(&pdev->dev, "%s\n", __func__);
@@ -273,12 +296,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 +312,15 @@ 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);
+
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.parent,
+ 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 +339,20 @@ static int lm3533_bl_probe(struct platform_device *pdev)
backlight_update_status(bd);
- ret = lm3533_bl_setup(bl, pdata);
+ /* 5000 - 29800 uA (800 uA step) */
+ val = 5000;
+ device_property_read_u32(&pdev->dev, "ti,max-current-microamp", &val);
+ bl->max_current = val;
+
+ /* 0 - 0x3f */
+ val = 0;
+ device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &val);
+ bl->pwm = val;
+
+ bl->linear = device_property_read_bool(&pdev->dev, "ti,linear-mapping-mode");
+ bl->hvled = device_property_read_bool(&pdev->dev, "ti,hardware-controlled");
+
+ ret = lm3533_bl_setup(bl);
if (ret)
goto err_sysfs_remove;
@@ -381,10 +413,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..3b28fc0970f6 100644
--- a/include/linux/mfd/lm3533.h
+++ b/include/linux/mfd/lm3533.h
@@ -27,6 +27,9 @@ struct lm3533 {
struct gpio_desc *hwen;
int irq;
+ u32 boost_ovp;
+ u32 boost_freq;
+
unsigned have_als:1;
unsigned have_backlights:1;
unsigned have_leds:1;
@@ -38,25 +41,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,
@@ -69,19 +53,6 @@ enum lm3533_boost_ovp {
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.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: mfd: Document TI LM3533 MFD
2025-02-24 11:48 ` [PATCH v3 1/2] dt-bindings: mfd: Document TI LM3533 MFD Svyatoslav Ryhel
@ 2025-02-24 20:31 ` Rob Herring
0 siblings, 0 replies; 10+ messages in thread
From: Rob Herring @ 2025-02-24 20:31 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
Lars-Peter Clausen, Pavel Machek, Daniel Thompson, Jingoo Han,
Helge Deller, Andy Shevchenko, Uwe Kleine-König, devicetree,
linux-kernel, linux-iio, linux-leds, dri-devel, linux-fbdev
On Mon, Feb 24, 2025 at 01:48:13PM +0200, Svyatoslav Ryhel wrote:
> Add bindings for the LM3533 - a complete power source for backlight, keypad
> and indicator LEDs in smartphone handsets. The high-voltage inductive boost
> converter provides the power for two series LED strings display backlight
> and keypad functions.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> .../devicetree/bindings/mfd/ti,lm3533.yaml | 231 ++++++++++++++++++
> 1 file changed, 231 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> new file mode 100644
> index 000000000000..c8ac6d4424aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ti,lm3533.yaml
> @@ -0,0 +1,231 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/ti,lm3533.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI LM3533 Complete Lighting Power Solution
> +
> +description: >
> + The LM3533 is a complete power source for backlight, keypad, and indicator LEDs
> + in smartphone handsets. The high-voltage inductive boost converter provides the
> + power for two series LED strings display backlight and keypad functions.
> +
> + https://www.ti.com/product/LM3533
> +
> +maintainers:
> + - Svyatoslav Ryhel <clamor95@gmail.com>
> +
> +properties:
> + compatible:
> + const: ti,lm3533
> +
> + reg:
> + maxItems: 1
> +
> + enable-gpios:
> + description: GPIO to use to enable/disable the backlight (HWEN pin).
> + maxItems: 1
> +
> + 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
> + description:
> + Properties for an illumination sensor.
> + additionalProperties: false
> +
> + properties:
> + compatible:
> + const: ti,lm3533-als
> +
> + ti,resistor-value-ohm:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Internal configuration resister value when ALS is in Analog Sensor
> + mode and PWM mode is disabled.
> + minimum: 1575
> + maximum: 200000
> +
> + ti,pwm-mode:
> + type: boolean
> + description:
> + Switch for mode in which ALS is running. If this propertly is set
> + then ALS is running in PWM mode, internal resistor value is set to
> + high-impedance (0) and resistor-value-ohm propertly is ignored.
> +
> + required:
> + - compatible
> +
> +required:
> + - compatible
> + - reg
> + - enable-gpios
> + - light-sensor
> + - backlight-0
> + - backlight-1
> + - led-0
> + - led-1
> + - led-2
> + - led-3
> +
> +patternProperties:
> + "^backlight-[01]$":
> + type: object
> + description:
> + Properties for a backlight device.
> +
> + $ref: /schemas/leds/backlight/common.yaml#
> +
> + properties:
> + compatible:
> + const: ti,lm3533-backlight
> +
> + default-brightness: true
> +
> + ti,max-current-microamp:
> + description:
> + Maximum current in µA with a 800 µA step.
> + enum: [ 5000, 5800, 6600, 7400, 8200, 9000, 9800,
> + 10600, 11400, 12200, 13000, 13800, 14600,
> + 15400, 16200, 17000, 17800, 18600, 19400,
> + 20200, 21000, 21800, 22600, 23400, 24200,
> + 25000, 25800, 26600, 27400, 28200, 29000,
> + 29800 ]
> + default: 5000
> +
> + 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
> +
> + ti,hardware-controlled:
> + description:
> + Each backlight has its own voltage Control Bank (A and B) and there are
> + two HVLED sinks which by default are linked to respective Bank. Setting
> + this property will link both sinks to a Control Bank of backlight where
> + property is defined.
> + type: boolean
> +
> + required:
> + - compatible
> +
> + additionalProperties: false
> +
> + "^led-[0-3]$":
> + type: object
> + description:
> + Properties for a led device.
> +
> + $ref: /schemas/leds/common.yaml#
> +
> + properties:
> + compatible:
> + const: ti,lm3533-leds
> +
> + linux,default-trigger: true
> +
> + ti,max-current-microamp:
> + description:
> + Maximum current in µA with a 800 µA step.
> + enum: [ 5000, 5800, 6600, 7400, 8200, 9000, 9800,
> + 10600, 11400, 12200, 13000, 13800, 14600,
> + 15400, 16200, 17000, 17800, 18600, 19400,
> + 20200, 21000, 21800, 22600, 23400, 24200,
> + 25000, 25800, 26600, 27400, 28200, 29000,
> + 29800 ]
> + default: 5000
> +
> + ti,pwm-config-mask:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Same descryption and function as for backlight.
> +
> + required:
> + - compatible
> +
> + additionalProperties: false
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + led-controller@36 {
> + compatible = "ti,lm3533";
> + reg = <0x36>;
> +
> + enable-gpios = <&gpio 110 GPIO_ACTIVE_HIGH>;
> +
> + ti,boost-ovp-microvolt = <24000000>;
> + ti,boost-freq-hz = <500000>;
> +
> + backlight-0 {
> + compatible = "ti,lm3533-backlight";
> +
> + ti,max-current-microamp = <23400>;
> + default-brightness = <113>;
> + ti,hardware-controlled;
> + };
> +
> + backlight-1 {
> + compatible = "ti,lm3533-backlight";
> + status = "disabled";
Examples should be enabled. Drop status.
With those fixed,
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> + };
> +
> + led-0 {
> + compatible = "ti,lm3533-leds";
> + status = "disabled";
> + };
> +
> + led-1 {
> + compatible = "ti,lm3533-leds";
> + status = "disabled";
> + };
> +
> + led-2 {
> + compatible = "ti,lm3533-leds";
> + status = "disabled";
> + };
> +
> + led-3 {
> + compatible = "ti,lm3533-leds";
> + status = "disabled";
> + };
> +
> + light-sensor {
> + compatible = "ti,lm3533-als";
> + status = "disabled";
> + };
> + };
> + };
> +...
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] mfd: lm3533: convert to use OF
2025-02-24 11:48 ` [PATCH v3 2/2] mfd: lm3533: convert to use OF Svyatoslav Ryhel
@ 2025-02-28 8:59 ` Lee Jones
2025-02-28 9:30 ` Svyatoslav Ryhel
0 siblings, 1 reply; 10+ messages in thread
From: Lee Jones @ 2025-02-28 8:59 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
Lars-Peter Clausen, Pavel Machek, Daniel Thompson, Jingoo Han,
Helge Deller, Andy Shevchenko, Uwe Kleine-König, devicetree,
linux-kernel, linux-iio, linux-leds, dri-devel, linux-fbdev
On Mon, 24 Feb 2025, Svyatoslav Ryhel wrote:
> Remove platform data and fully relay on OF and device tree
> parsing and binding devices.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> drivers/iio/light/lm3533-als.c | 40 ++++---
> drivers/leds/leds-lm3533.c | 46 +++++---
> drivers/mfd/lm3533-core.c | 159 ++++++++--------------------
> drivers/video/backlight/lm3533_bl.c | 71 ++++++++++---
> include/linux/mfd/lm3533.h | 35 +-----
> 5 files changed, 164 insertions(+), 187 deletions(-)
>
> diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> index 99f0b903018c..cb52965e93c6 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c
> @@ -16,9 +16,12 @@
> #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>
>
> @@ -56,6 +59,9 @@ struct lm3533_als {
>
> atomic_t zone;
> struct mutex thresh_mutex;
> +
> + unsigned pwm_mode:1; /* PWM input mode (default analog) */
> + u8 r_select; /* 1 - 127 (ignored in PWM-mode) */
> };
>
>
> @@ -753,18 +759,17 @@ static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
> return 0;
> }
>
> -static int lm3533_als_setup(struct lm3533_als *als,
> - const struct lm3533_als_platform_data *pdata)
> +static int lm3533_als_setup(struct lm3533_als *als)
> {
> int ret;
>
> - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> + ret = lm3533_als_set_input_mode(als, als->pwm_mode);
> if (ret)
> return ret;
>
> /* ALS input is always high impedance in PWM-mode. */
> - if (!pdata->pwm_mode) {
> - ret = lm3533_als_set_resistor(als, pdata->r_select);
> + if (!als->pwm_mode) {
> + ret = lm3533_als_set_resistor(als, als->r_select);
You're already passing 'als'.
Just teach lm3533_als_set_resistor that 'r_select' is now contained.
> if (ret)
> return ret;
> }
> @@ -828,22 +833,16 @@ 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;
> + u32 val;
Value of what, potatoes?
> int ret;
>
> lm3533 = dev_get_drvdata(pdev->dev.parent);
> 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;
> @@ -864,13 +863,21 @@ static int lm3533_als_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, indio_dev);
>
> + val = 200 * KILO; /* 200kOhm */
Better to #define magic numbers; DEFAULT_{DESCRIPTION}_OHMS
> + device_property_read_u32(&pdev->dev, "ti,resistor-value-ohm", &val);
> +
> + /* Convert resitance into R_ALS value with 2v / 10uA * R */
Because ...
> + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * val);
> +
> + als->pwm_mode = device_property_read_bool(&pdev->dev, "ti,pwm-mode");
> +
> if (als->irq) {
> 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 +914,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..6e661a2a540f 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;
> +
> + u16 max_current; /* 5000 - 29800 uA (800 uA step) */
> + u8 pwm; /* 0 - 0x3f */
This comment doesn't add anything.
> };
>
>
> @@ -632,23 +637,22 @@ 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);
Why not make max_current and attribute of lm3533_ctrlbank and drop the
redundant parameter?
> if (ret)
> return ret;
>
> - return lm3533_ctrlbank_set_pwm(&led->cb, pdata->pwm);
> + return lm3533_ctrlbank_set_pwm(&led->cb, led->pwm);
As above.
> }
>
> static int lm3533_led_probe(struct platform_device *pdev)
> {
> struct lm3533 *lm3533;
> - struct lm3533_led_platform_data *pdata;
> struct lm3533_led *led;
> + u32 val;
> int ret;
>
> dev_dbg(&pdev->dev, "%s\n", __func__);
> @@ -657,12 +661,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 +671,11 @@ 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.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
> + pdev->name, pdev->id);
> + led->cdev.default_trigger = "none";
> + device_property_read_string(&pdev->dev, "linux,default-trigger",
> + &led->cdev.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;
> @@ -702,7 +703,17 @@ static int lm3533_led_probe(struct platform_device *pdev)
>
> led->cb.dev = led->cdev.dev;
>
> - ret = lm3533_led_setup(led, pdata);
> + /* 5000 - 29800 uA (800 uA step) */
> + val = 5000;
> + device_property_read_u32(&pdev->dev, "ti,max-current-microamp", &val);
> + led->max_current = val;
Why not just use 'led->max_current' from the offset and delete 'val'?
> +
> + /* 0 - 0x3f */
How does this improve readability? Why would use this info?
> + val = 0;
> + device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &val);
> + led->pwm = val;
> +
> + ret = lm3533_led_setup(led);
> if (ret)
> goto err_deregister;
>
> @@ -739,9 +750,16 @@ static void lm3533_led_shutdown(struct platform_device *pdev)
> lm3533_led_set(&led->cdev, LED_OFF); /* disable blink */
> }
>
> +static const struct of_device_id lm3533_led_match_table[] = {
> + { .compatible = "ti,lm3533-leds" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, lm3533_led_match_table);
> +
> static struct platform_driver lm3533_led_driver = {
> .driver = {
> .name = "lm3533-leds",
> + .of_match_table = lm3533_led_match_table,
> },
> .probe = lm3533_led_probe,
> .remove = lm3533_led_remove,
> diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
> index 0a2409d00b2e..e1b8fe136af9 100644
> --- a/drivers/mfd/lm3533-core.c
> +++ b/drivers/mfd/lm3533-core.c
> @@ -14,10 +14,13 @@
> #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>
>
> @@ -42,41 +45,41 @@
>
> #define LM3533_REG_MAX 0xb2
>
> -
> -static struct mfd_cell lm3533_als_devs[] = {
> +static struct mfd_cell lm3533_child_devices[] = {
Drop the child_ part.
> {
> .name = "lm3533-als",
> .id = -1,
> + .of_compatible = "ti,lm3533-als",
> },
> -};
> -
> -static struct mfd_cell lm3533_bl_devs[] = {
> {
> .name = "lm3533-backlight",
> .id = 0,
> + .of_compatible = "ti,lm3533-backlight",
> },
> {
> .name = "lm3533-backlight",
> .id = 1,
> + .of_compatible = "ti,lm3533-backlight",
> },
> -};
> -
> -static struct mfd_cell lm3533_led_devs[] = {
> {
> .name = "lm3533-leds",
> .id = 0,
Do you know if these are actually used for anything?
Any reason to not just use PLATFORM_DEVID_AUTO?
> + .of_compatible = "ti,lm3533-leds",
> },
> {
> .name = "lm3533-leds",
> .id = 1,
> + .of_compatible = "ti,lm3533-leds",
> },
> {
> .name = "lm3533-leds",
> .id = 2,
> + .of_compatible = "ti,lm3533-leds",
> },
> {
> .name = "lm3533-leds",
> .id = 3,
> + .of_compatible = "ti,lm3533-leds",
> },
> };
>
> @@ -376,136 +379,60 @@ static struct attribute_group lm3533_attribute_group = {
> .attrs = lm3533_attributes
> };
>
> -static int lm3533_device_als_init(struct lm3533 *lm3533)
> -{
> - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> - int ret;
> -
> - if (!pdata->als)
> - return 0;
> -
> - lm3533_als_devs[0].platform_data = pdata->als;
> - lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
> -
> - ret = mfd_add_devices(lm3533->dev, 0, lm3533_als_devs, 1, NULL,
> - 0, NULL);
> - if (ret) {
> - dev_err(lm3533->dev, "failed to add ALS device\n");
> - return ret;
> - }
> -
> - lm3533->have_als = 1;
> -
> - return 0;
> -}
> -
> -static int lm3533_device_bl_init(struct lm3533 *lm3533)
> -{
> - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> - int i;
> - int ret;
> -
> - if (!pdata->backlights || pdata->num_backlights == 0)
> - return 0;
> -
> - if (pdata->num_backlights > ARRAY_SIZE(lm3533_bl_devs))
> - pdata->num_backlights = ARRAY_SIZE(lm3533_bl_devs);
> -
> - for (i = 0; i < pdata->num_backlights; ++i) {
> - lm3533_bl_devs[i].platform_data = &pdata->backlights[i];
> - lm3533_bl_devs[i].pdata_size = sizeof(pdata->backlights[i]);
> - }
> -
> - ret = mfd_add_devices(lm3533->dev, 0, lm3533_bl_devs,
> - pdata->num_backlights, NULL, 0, NULL);
> - if (ret) {
> - dev_err(lm3533->dev, "failed to add backlight devices\n");
> - return ret;
> - }
> -
> - lm3533->have_backlights = 1;
> -
> - return 0;
> -}
> -
> -static int lm3533_device_led_init(struct lm3533 *lm3533)
> -{
> - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> - int i;
> - int ret;
> -
> - if (!pdata->leds || pdata->num_leds == 0)
> - return 0;
> -
> - if (pdata->num_leds > ARRAY_SIZE(lm3533_led_devs))
> - pdata->num_leds = ARRAY_SIZE(lm3533_led_devs);
> -
> - for (i = 0; i < pdata->num_leds; ++i) {
> - lm3533_led_devs[i].platform_data = &pdata->leds[i];
> - lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
> - }
> -
> - ret = mfd_add_devices(lm3533->dev, 0, lm3533_led_devs,
> - pdata->num_leds, NULL, 0, NULL);
> - if (ret) {
> - dev_err(lm3533->dev, "failed to add LED devices\n");
> - return ret;
> - }
> -
> - lm3533->have_leds = 1;
> -
> - return 0;
> -}
> -
> -static int lm3533_device_setup(struct lm3533 *lm3533,
> - struct lm3533_platform_data *pdata)
> +static int lm3533_device_setup(struct lm3533 *lm3533)
> {
> int ret;
>
> - ret = lm3533_set_boost_freq(lm3533, pdata->boost_freq);
> + ret = lm3533_set_boost_freq(lm3533, lm3533->boost_freq);
Same comments as before.
Teach lm3533_set_boost_freq() that 'boost_freq' is contained in 'lm3533'.
> if (ret)
> return ret;
>
> - return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> + return lm3533_set_boost_ovp(lm3533, lm3533->boost_ovp);
> }
>
> static int lm3533_device_init(struct lm3533 *lm3533)
> {
> - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> + u32 val;
'uV' and 'hz' could be easier to follow.
> int ret;
>
> - dev_dbg(lm3533->dev, "%s\n", __func__);
> + lm3533->hwen = devm_gpiod_get(lm3533->dev, "enable", GPIOD_OUT_LOW);
> + if (IS_ERR(lm3533->hwen))
> + return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen),
> + "failed to request HWEN GPIO\n");
>
> - if (!pdata) {
> - dev_err(lm3533->dev, "no platform data\n");
> - return -EINVAL;
> - }
> + val = 16 * MICRO; /* 16V */
> + device_property_read_u32(lm3533->dev, "ti,boost-ovp-microvolt", &val);
>
> - 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");
> + /* boost_ovp is defined in microvolts, convert to enum value */
> + lm3533->boost_ovp = val / (8 * MICRO) - 2;
Wait, what. Why?
Converting a useful microvolt value to an arbitrary enum sounds fragile.
> +
> + val = 500 * KILO; /* 500kHz */
> + device_property_read_u32(lm3533->dev, "ti,boost-freq-hz", &val);
> +
> + /* boost_freq is defined in Hz, convert to enum value */
> + lm3533->boost_freq = val / (500 * KILO) - 1;
>
> lm3533_enable(lm3533);
>
> - ret = lm3533_device_setup(lm3533, pdata);
> + ret = lm3533_device_setup(lm3533);
> if (ret)
> goto err_disable;
>
> - lm3533_device_als_init(lm3533);
> - lm3533_device_bl_init(lm3533);
> - lm3533_device_led_init(lm3533);
> + ret = devm_mfd_add_devices(lm3533->dev, 0, lm3533_child_devices,
> + ARRAY_SIZE(lm3533_child_devices), NULL, 0, NULL);
> + if (ret) {
> + dev_err(lm3533->dev, "failed to add MFD devices: %d\n", ret);
"child devices" or "sub-devices".
> + goto err_disable;
> + }
>
> ret = sysfs_create_group(&lm3533->dev->kobj, &lm3533_attribute_group);
> if (ret < 0) {
> dev_err(lm3533->dev, "failed to create sysfs attributes\n");
> - goto err_unregister;
> + goto err_disable;
> }
>
> return 0;
>
> -err_unregister:
> - mfd_remove_devices(lm3533->dev);
> err_disable:
> lm3533_disable(lm3533);
>
> @@ -517,8 +444,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);
> }
>
> @@ -591,6 +516,9 @@ static int lm3533_i2c_probe(struct i2c_client *i2c)
> lm3533->dev = &i2c->dev;
> lm3533->irq = i2c->irq;
>
> + i2c->dev.coherent_dma_mask = 0;
> + i2c->dev.dma_mask = &i2c->dev.coherent_dma_mask;
Why are you manually doing this?
The core should take care of this for you:
drivers/mfd/mfd-core.c: pdev->dev.dma_mask = parent->dma_mask;
drivers/mfd/mfd-core.c: pdev->dev.coherent_dma_mask = parent->coherent_dma_mask;
> +
> return lm3533_device_init(lm3533);
> }
>
> @@ -603,6 +531,12 @@ static void lm3533_i2c_remove(struct i2c_client *i2c)
> lm3533_device_exit(lm3533);
> }
>
> +static const struct of_device_id lm3533_match_table[] = {
> + { .compatible = "ti,lm3533" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, lm3533_match_table);
> +
> static const struct i2c_device_id lm3533_i2c_ids[] = {
> { "lm3533" },
> { }
> @@ -612,6 +546,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..0827a5e98dbb 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>
>
> @@ -19,6 +21,7 @@
> #define LM3533_HVCTRLBANK_COUNT 2
> #define LM3533_BL_MAX_BRIGHTNESS 255
>
> +#define LM3533_REG_OUTPUT_CONF1 0x10
> #define LM3533_REG_CTRLBANK_AB_BCONF 0x1a
>
>
> @@ -27,6 +30,11 @@ struct lm3533_bl {
> struct lm3533_ctrlbank cb;
> struct backlight_device *bd;
> int id;
> +
> + u16 max_current; /* 5000 - 29800 uA (800 uA step) */
> + u8 pwm; /* 0 - 0x3f */
Remove or improve this comment.
> + bool linear;
> + bool hvled;
> };
>
>
> @@ -246,25 +254,40 @@ 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 id = lm3533_bl_get_ctrlbank_id(bl);
> int ret;
>
> - ret = lm3533_ctrlbank_set_max_current(&bl->cb, pdata->max_current);
> + if (bl->linear) {
> + ret = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF,
> + BIT(2 * id + 1), BIT(2 * id + 1));
These need to be defined as SHIFT values or whatever they are.
> + if (ret)
> + return ret;
> + }
> +
> + if (bl->hvled) {
> + ret = lm3533_update(bl->lm3533, LM3533_REG_OUTPUT_CONF1,
> + id | id << 1, BIT(0) | BIT(1));
> + if (ret)
> + return ret;
> + }
> +
> + 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;
> + u32 val;
As above.
> int ret;
>
> dev_dbg(&pdev->dev, "%s\n", __func__);
> @@ -273,12 +296,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 +312,15 @@ 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);
Doesn't platform already provide enumerated names?
> +
> 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.parent,
> + 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 +339,20 @@ static int lm3533_bl_probe(struct platform_device *pdev)
>
> backlight_update_status(bd);
>
> - ret = lm3533_bl_setup(bl, pdata);
> + /* 5000 - 29800 uA (800 uA step) */
> + val = 5000;
> + device_property_read_u32(&pdev->dev, "ti,max-current-microamp", &val);
> + bl->max_current = val;
> +
> + /* 0 - 0x3f */
> + val = 0;
> + device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &val);
> + bl->pwm = val;
> +
> + bl->linear = device_property_read_bool(&pdev->dev, "ti,linear-mapping-mode");
> + bl->hvled = device_property_read_bool(&pdev->dev, "ti,hardware-controlled");
> +
> + ret = lm3533_bl_setup(bl);
> if (ret)
> goto err_sysfs_remove;
>
> @@ -381,10 +413,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..3b28fc0970f6 100644
> --- a/include/linux/mfd/lm3533.h
> +++ b/include/linux/mfd/lm3533.h
> @@ -27,6 +27,9 @@ struct lm3533 {
> struct gpio_desc *hwen;
> int irq;
>
> + u32 boost_ovp;
> + u32 boost_freq;
> +
> unsigned have_als:1;
> unsigned have_backlights:1;
> unsigned have_leds:1;
> @@ -38,25 +41,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,
> @@ -69,19 +53,6 @@ enum lm3533_boost_ovp {
> 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.43.0
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] mfd: lm3533: convert to use OF
2025-02-28 8:59 ` Lee Jones
@ 2025-02-28 9:30 ` Svyatoslav Ryhel
2025-03-05 13:44 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Svyatoslav Ryhel @ 2025-02-28 9:30 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
Lars-Peter Clausen, Pavel Machek, Daniel Thompson, Jingoo Han,
Helge Deller, Andy Shevchenko, Uwe Kleine-König, devicetree,
linux-kernel, linux-iio, linux-leds, dri-devel, linux-fbdev
пт, 28 лют. 2025 р. о 10:59 Lee Jones <lee@kernel.org> пише:
>
> On Mon, 24 Feb 2025, Svyatoslav Ryhel wrote:
>
> > Remove platform data and fully relay on OF and device tree
> > parsing and binding devices.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> > drivers/iio/light/lm3533-als.c | 40 ++++---
> > drivers/leds/leds-lm3533.c | 46 +++++---
> > drivers/mfd/lm3533-core.c | 159 ++++++++--------------------
> > drivers/video/backlight/lm3533_bl.c | 71 ++++++++++---
> > include/linux/mfd/lm3533.h | 35 +-----
> > 5 files changed, 164 insertions(+), 187 deletions(-)
> >
> > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > index 99f0b903018c..cb52965e93c6 100644
> > --- a/drivers/iio/light/lm3533-als.c
> > +++ b/drivers/iio/light/lm3533-als.c
> > @@ -16,9 +16,12 @@
> > #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>
> >
> > @@ -56,6 +59,9 @@ struct lm3533_als {
> >
> > atomic_t zone;
> > struct mutex thresh_mutex;
> > +
> > + unsigned pwm_mode:1; /* PWM input mode (default analog) */
> > + u8 r_select; /* 1 - 127 (ignored in PWM-mode) */
> > };
> >
> >
> > @@ -753,18 +759,17 @@ static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
> > return 0;
> > }
> >
> > -static int lm3533_als_setup(struct lm3533_als *als,
> > - const struct lm3533_als_platform_data *pdata)
> > +static int lm3533_als_setup(struct lm3533_als *als)
> > {
> > int ret;
> >
> > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > + ret = lm3533_als_set_input_mode(als, als->pwm_mode);
> > if (ret)
> > return ret;
> >
> > /* ALS input is always high impedance in PWM-mode. */
> > - if (!pdata->pwm_mode) {
> > - ret = lm3533_als_set_resistor(als, pdata->r_select);
> > + if (!als->pwm_mode) {
> > + ret = lm3533_als_set_resistor(als, als->r_select);
>
> You're already passing 'als'.
>
> Just teach lm3533_als_set_resistor that 'r_select' is now contained.
>
This is not scope of this patchset. I was already accused in too much
changes which make it unreadable. This patchset is dedicated to
swapping platform data to use of the device tree. NOT improving
functions, NOT rewriting arbitrary mechanics. If you feed a need for
this change, then propose a followup. I need from this driver only one
thing, that it could work with device tree. But it seems that it is
better that it just rots in the garbage bin until removed cause no one
cared.
> > if (ret)
> > return ret;
> > }
> > @@ -828,22 +833,16 @@ 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;
> > + u32 val;
>
> Value of what, potatoes?
>
Oranges.
> > int ret;
> >
> > lm3533 = dev_get_drvdata(pdev->dev.parent);
> > 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;
> > @@ -864,13 +863,21 @@ static int lm3533_als_probe(struct platform_device *pdev)
> >
> > platform_set_drvdata(pdev, indio_dev);
> >
> > + val = 200 * KILO; /* 200kOhm */
>
> Better to #define magic numbers; DEFAULT_{DESCRIPTION}_OHMS
>
Why? that is not needed.
> > + device_property_read_u32(&pdev->dev, "ti,resistor-value-ohm", &val);
> > +
> > + /* Convert resitance into R_ALS value with 2v / 10uA * R */
>
> Because ...
>
BACAUSE the device DOES NOT understand human readable values, only 0s
and 1s, hence mOhms must be converted into value lm3533 chip can
understand.
> > + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * val);
> > +
> > + als->pwm_mode = device_property_read_bool(&pdev->dev, "ti,pwm-mode");
> > +
> > if (als->irq) {
> > 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 +914,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..6e661a2a540f 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;
> > +
> > + u16 max_current; /* 5000 - 29800 uA (800 uA step) */
> > + u8 pwm; /* 0 - 0x3f */
>
> This comment doesn't add anything.
>
This comment is from pdata. Both values were just transferred from there.
> > };
> >
> >
> > @@ -632,23 +637,22 @@ 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);
>
> Why not make max_current and attribute of lm3533_ctrlbank and drop the
> redundant parameter?
>
> > if (ret)
> > return ret;
> >
> > - return lm3533_ctrlbank_set_pwm(&led->cb, pdata->pwm);
> > + return lm3533_ctrlbank_set_pwm(&led->cb, led->pwm);
>
> As above.
>
As above.
> > }
> >
> > static int lm3533_led_probe(struct platform_device *pdev)
> > {
> > struct lm3533 *lm3533;
> > - struct lm3533_led_platform_data *pdata;
> > struct lm3533_led *led;
> > + u32 val;
> > int ret;
> >
> > dev_dbg(&pdev->dev, "%s\n", __func__);
> > @@ -657,12 +661,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 +671,11 @@ 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.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
> > + pdev->name, pdev->id);
> > + led->cdev.default_trigger = "none";
> > + device_property_read_string(&pdev->dev, "linux,default-trigger",
> > + &led->cdev.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;
> > @@ -702,7 +703,17 @@ static int lm3533_led_probe(struct platform_device *pdev)
> >
> > led->cb.dev = led->cdev.dev;
> >
> > - ret = lm3533_led_setup(led, pdata);
> > + /* 5000 - 29800 uA (800 uA step) */
> > + val = 5000;
> > + device_property_read_u32(&pdev->dev, "ti,max-current-microamp", &val);
> > + led->max_current = val;
>
> Why not just use 'led->max_current' from the offset and delete 'val'?
>
BECAUSE, linux has no device_property_read_u32_default to pass default
value there. Additionally max_current is u16 and
device_property_read_u32 will not work for u16.
> > +
> > + /* 0 - 0x3f */
>
> How does this improve readability? Why would use this info?
>
> > + val = 0;
> > + device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &val);
> > + led->pwm = val;
> > +
> > + ret = lm3533_led_setup(led);
> > if (ret)
> > goto err_deregister;
> >
> > @@ -739,9 +750,16 @@ static void lm3533_led_shutdown(struct platform_device *pdev)
> > lm3533_led_set(&led->cdev, LED_OFF); /* disable blink */
> > }
> >
> > +static const struct of_device_id lm3533_led_match_table[] = {
> > + { .compatible = "ti,lm3533-leds" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, lm3533_led_match_table);
> > +
> > static struct platform_driver lm3533_led_driver = {
> > .driver = {
> > .name = "lm3533-leds",
> > + .of_match_table = lm3533_led_match_table,
> > },
> > .probe = lm3533_led_probe,
> > .remove = lm3533_led_remove,
> > diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
> > index 0a2409d00b2e..e1b8fe136af9 100644
> > --- a/drivers/mfd/lm3533-core.c
> > +++ b/drivers/mfd/lm3533-core.c
> > @@ -14,10 +14,13 @@
> > #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>
> >
> > @@ -42,41 +45,41 @@
> >
> > #define LM3533_REG_MAX 0xb2
> >
> > -
> > -static struct mfd_cell lm3533_als_devs[] = {
> > +static struct mfd_cell lm3533_child_devices[] = {
>
> Drop the child_ part.
>
Why? It seems that you are nitpicking.
> > {
> > .name = "lm3533-als",
> > .id = -1,
> > + .of_compatible = "ti,lm3533-als",
> > },
> > -};
> > -
> > -static struct mfd_cell lm3533_bl_devs[] = {
> > {
> > .name = "lm3533-backlight",
> > .id = 0,
> > + .of_compatible = "ti,lm3533-backlight",
> > },
> > {
> > .name = "lm3533-backlight",
> > .id = 1,
> > + .of_compatible = "ti,lm3533-backlight",
> > },
> > -};
> > -
> > -static struct mfd_cell lm3533_led_devs[] = {
> > {
> > .name = "lm3533-leds",
> > .id = 0,
>
> Do you know if these are actually used for anything?
>
Yes, they are used to select correct control bank
> Any reason to not just use PLATFORM_DEVID_AUTO?
>
> > + .of_compatible = "ti,lm3533-leds",
> > },
> > {
> > .name = "lm3533-leds",
> > .id = 1,
> > + .of_compatible = "ti,lm3533-leds",
> > },
> > {
> > .name = "lm3533-leds",
> > .id = 2,
> > + .of_compatible = "ti,lm3533-leds",
> > },
> > {
> > .name = "lm3533-leds",
> > .id = 3,
> > + .of_compatible = "ti,lm3533-leds",
> > },
> > };
> >
> > @@ -376,136 +379,60 @@ static struct attribute_group lm3533_attribute_group = {
> > .attrs = lm3533_attributes
> > };
> >
> > -static int lm3533_device_als_init(struct lm3533 *lm3533)
> > -{
> > - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> > - int ret;
> > -
> > - if (!pdata->als)
> > - return 0;
> > -
> > - lm3533_als_devs[0].platform_data = pdata->als;
> > - lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
> > -
> > - ret = mfd_add_devices(lm3533->dev, 0, lm3533_als_devs, 1, NULL,
> > - 0, NULL);
> > - if (ret) {
> > - dev_err(lm3533->dev, "failed to add ALS device\n");
> > - return ret;
> > - }
> > -
> > - lm3533->have_als = 1;
> > -
> > - return 0;
> > -}
> > -
> > -static int lm3533_device_bl_init(struct lm3533 *lm3533)
> > -{
> > - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> > - int i;
> > - int ret;
> > -
> > - if (!pdata->backlights || pdata->num_backlights == 0)
> > - return 0;
> > -
> > - if (pdata->num_backlights > ARRAY_SIZE(lm3533_bl_devs))
> > - pdata->num_backlights = ARRAY_SIZE(lm3533_bl_devs);
> > -
> > - for (i = 0; i < pdata->num_backlights; ++i) {
> > - lm3533_bl_devs[i].platform_data = &pdata->backlights[i];
> > - lm3533_bl_devs[i].pdata_size = sizeof(pdata->backlights[i]);
> > - }
> > -
> > - ret = mfd_add_devices(lm3533->dev, 0, lm3533_bl_devs,
> > - pdata->num_backlights, NULL, 0, NULL);
> > - if (ret) {
> > - dev_err(lm3533->dev, "failed to add backlight devices\n");
> > - return ret;
> > - }
> > -
> > - lm3533->have_backlights = 1;
> > -
> > - return 0;
> > -}
> > -
> > -static int lm3533_device_led_init(struct lm3533 *lm3533)
> > -{
> > - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> > - int i;
> > - int ret;
> > -
> > - if (!pdata->leds || pdata->num_leds == 0)
> > - return 0;
> > -
> > - if (pdata->num_leds > ARRAY_SIZE(lm3533_led_devs))
> > - pdata->num_leds = ARRAY_SIZE(lm3533_led_devs);
> > -
> > - for (i = 0; i < pdata->num_leds; ++i) {
> > - lm3533_led_devs[i].platform_data = &pdata->leds[i];
> > - lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
> > - }
> > -
> > - ret = mfd_add_devices(lm3533->dev, 0, lm3533_led_devs,
> > - pdata->num_leds, NULL, 0, NULL);
> > - if (ret) {
> > - dev_err(lm3533->dev, "failed to add LED devices\n");
> > - return ret;
> > - }
> > -
> > - lm3533->have_leds = 1;
> > -
> > - return 0;
> > -}
> > -
> > -static int lm3533_device_setup(struct lm3533 *lm3533,
> > - struct lm3533_platform_data *pdata)
> > +static int lm3533_device_setup(struct lm3533 *lm3533)
> > {
> > int ret;
> >
> > - ret = lm3533_set_boost_freq(lm3533, pdata->boost_freq);
> > + ret = lm3533_set_boost_freq(lm3533, lm3533->boost_freq);
>
> Same comments as before.
>
> Teach lm3533_set_boost_freq() that 'boost_freq' is contained in 'lm3533'.
>
Same answer as above.
> > if (ret)
> > return ret;
> >
> > - return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> > + return lm3533_set_boost_ovp(lm3533, lm3533->boost_ovp);
> > }
> >
> > static int lm3533_device_init(struct lm3533 *lm3533)
> > {
> > - struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
> > + u32 val;
>
> 'uV' and 'hz' could be easier to follow.
>
Add 2 temp values instead of one, nice.
> > int ret;
> >
> > - dev_dbg(lm3533->dev, "%s\n", __func__);
> > + lm3533->hwen = devm_gpiod_get(lm3533->dev, "enable", GPIOD_OUT_LOW);
> > + if (IS_ERR(lm3533->hwen))
> > + return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen),
> > + "failed to request HWEN GPIO\n");
> >
> > - if (!pdata) {
> > - dev_err(lm3533->dev, "no platform data\n");
> > - return -EINVAL;
> > - }
> > + val = 16 * MICRO; /* 16V */
> > + device_property_read_u32(lm3533->dev, "ti,boost-ovp-microvolt", &val);
> >
> > - 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");
> > + /* boost_ovp is defined in microvolts, convert to enum value */
> > + lm3533->boost_ovp = val / (8 * MICRO) - 2;
>
> Wait, what. Why?
>
> Converting a useful microvolt value to an arbitrary enum sounds fragile.
>
BACAUSE the device DOES NOT understand human readable values, only 0s
and 1s, hence uV and Hz must be converted into value lm3533 chip can
understand.
> > +
> > + val = 500 * KILO; /* 500kHz */
> > + device_property_read_u32(lm3533->dev, "ti,boost-freq-hz", &val);
> > +
> > + /* boost_freq is defined in Hz, convert to enum value */
> > + lm3533->boost_freq = val / (500 * KILO) - 1;
> >
> > lm3533_enable(lm3533);
> >
> > - ret = lm3533_device_setup(lm3533, pdata);
> > + ret = lm3533_device_setup(lm3533);
> > if (ret)
> > goto err_disable;
> >
> > - lm3533_device_als_init(lm3533);
> > - lm3533_device_bl_init(lm3533);
> > - lm3533_device_led_init(lm3533);
> > + ret = devm_mfd_add_devices(lm3533->dev, 0, lm3533_child_devices,
>
>
> > + ARRAY_SIZE(lm3533_child_devices), NULL, 0, NULL);
> > + if (ret) {
> > + dev_err(lm3533->dev, "failed to add MFD devices: %d\n", ret);
>
> "child devices" or "sub-devices".
>
Does this naming matters? I can call them any way I want to as long
this remains clear. Or you have created some twisted consensus in your
heads that mfd child devices MUST be called sub-devices?
> > + goto err_disable;
> > + }
> >
> > ret = sysfs_create_group(&lm3533->dev->kobj, &lm3533_attribute_group);
> > if (ret < 0) {
> > dev_err(lm3533->dev, "failed to create sysfs attributes\n");
> > - goto err_unregister;
> > + goto err_disable;
> > }
> >
> > return 0;
> >
> > -err_unregister:
> > - mfd_remove_devices(lm3533->dev);
> > err_disable:
> > lm3533_disable(lm3533);
> >
> > @@ -517,8 +444,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);
> > }
> >
> > @@ -591,6 +516,9 @@ static int lm3533_i2c_probe(struct i2c_client *i2c)
> > lm3533->dev = &i2c->dev;
> > lm3533->irq = i2c->irq;
> >
> > + i2c->dev.coherent_dma_mask = 0;
> > + i2c->dev.dma_mask = &i2c->dev.coherent_dma_mask;
>
> Why are you manually doing this?
>
> The core should take care of this for you:
>
> drivers/mfd/mfd-core.c: pdev->dev.dma_mask = parent->dma_mask;
> drivers/mfd/mfd-core.c: pdev->dev.coherent_dma_mask = parent->coherent_dma_mask;
>
Parent uses DMA, but mfd and its children do not,
> > +
> > return lm3533_device_init(lm3533);
> > }
> >
> > @@ -603,6 +531,12 @@ static void lm3533_i2c_remove(struct i2c_client *i2c)
> > lm3533_device_exit(lm3533);
> > }
> >
> > +static const struct of_device_id lm3533_match_table[] = {
> > + { .compatible = "ti,lm3533" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, lm3533_match_table);
> > +
> > static const struct i2c_device_id lm3533_i2c_ids[] = {
> > { "lm3533" },
> > { }
> > @@ -612,6 +546,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..0827a5e98dbb 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>
> >
> > @@ -19,6 +21,7 @@
> > #define LM3533_HVCTRLBANK_COUNT 2
> > #define LM3533_BL_MAX_BRIGHTNESS 255
> >
> > +#define LM3533_REG_OUTPUT_CONF1 0x10
> > #define LM3533_REG_CTRLBANK_AB_BCONF 0x1a
> >
> >
> > @@ -27,6 +30,11 @@ struct lm3533_bl {
> > struct lm3533_ctrlbank cb;
> > struct backlight_device *bd;
> > int id;
> > +
> > + u16 max_current; /* 5000 - 29800 uA (800 uA step) */
> > + u8 pwm; /* 0 - 0x3f */
>
> Remove or improve this comment.
>
Comment was moved from the platform data along with the value.
> > + bool linear;
> > + bool hvled;
> > };
> >
> >
> > @@ -246,25 +254,40 @@ 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 id = lm3533_bl_get_ctrlbank_id(bl);
> > int ret;
> >
> > - ret = lm3533_ctrlbank_set_max_current(&bl->cb, pdata->max_current);
> > + if (bl->linear) {
> > + ret = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF,
> > + BIT(2 * id + 1), BIT(2 * id + 1));
>
> These need to be defined as SHIFT values or whatever they are.
>
Why? IMHO looks perfectly fine and readable.
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (bl->hvled) {
> > + ret = lm3533_update(bl->lm3533, LM3533_REG_OUTPUT_CONF1,
> > + id | id << 1, BIT(0) | BIT(1));
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + 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;
> > + u32 val;
>
> As above.
>
As above.
> > int ret;
> >
> > dev_dbg(&pdev->dev, "%s\n", __func__);
> > @@ -273,12 +296,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 +312,15 @@ 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);
>
> Doesn't platform already provide enumerated names?
>
It does, but if you have 4 devices which use same driver there cannot
be 4 same names.
> > +
> > 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.parent,
> > + 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 +339,20 @@ static int lm3533_bl_probe(struct platform_device *pdev)
> >
> > backlight_update_status(bd);
> >
> > - ret = lm3533_bl_setup(bl, pdata);
> > + /* 5000 - 29800 uA (800 uA step) */
> > + val = 5000;
> > + device_property_read_u32(&pdev->dev, "ti,max-current-microamp", &val);
> > + bl->max_current = val;
> > +
> > + /* 0 - 0x3f */
> > + val = 0;
> > + device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &val);
> > + bl->pwm = val;
> > +
> > + bl->linear = device_property_read_bool(&pdev->dev, "ti,linear-mapping-mode");
> > + bl->hvled = device_property_read_bool(&pdev->dev, "ti,hardware-controlled");
> > +
> > + ret = lm3533_bl_setup(bl);
> > if (ret)
> > goto err_sysfs_remove;
> >
> > @@ -381,10 +413,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..3b28fc0970f6 100644
> > --- a/include/linux/mfd/lm3533.h
> > +++ b/include/linux/mfd/lm3533.h
> > @@ -27,6 +27,9 @@ struct lm3533 {
> > struct gpio_desc *hwen;
> > int irq;
> >
> > + u32 boost_ovp;
> > + u32 boost_freq;
> > +
> > unsigned have_als:1;
> > unsigned have_backlights:1;
> > unsigned have_leds:1;
> > @@ -38,25 +41,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,
> > @@ -69,19 +53,6 @@ enum lm3533_boost_ovp {
> > 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.43.0
> >
>
> --
> Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] mfd: lm3533: convert to use OF
2025-02-28 9:30 ` Svyatoslav Ryhel
@ 2025-03-05 13:44 ` Jonathan Cameron
2025-03-05 14:18 ` Svyatoslav Ryhel
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2025-03-05 13:44 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Lars-Peter Clausen, Pavel Machek, Daniel Thompson, Jingoo Han,
Helge Deller, Andy Shevchenko, Uwe Kleine-König, devicetree,
linux-kernel, linux-iio, linux-leds, dri-devel, linux-fbdev
On Fri, 28 Feb 2025 11:30:51 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> пт, 28 лют. 2025 р. о 10:59 Lee Jones <lee@kernel.org> пише:
> >
> > On Mon, 24 Feb 2025, Svyatoslav Ryhel wrote:
> >
> > > Remove platform data and fully relay on OF and device tree
> > > parsing and binding devices.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > > drivers/iio/light/lm3533-als.c | 40 ++++---
> > > drivers/leds/leds-lm3533.c | 46 +++++---
> > > drivers/mfd/lm3533-core.c | 159 ++++++++--------------------
> > > drivers/video/backlight/lm3533_bl.c | 71 ++++++++++---
> > > include/linux/mfd/lm3533.h | 35 +-----
> > > 5 files changed, 164 insertions(+), 187 deletions(-)
> > >
> > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > > index 99f0b903018c..cb52965e93c6 100644
> > > --- a/drivers/iio/light/lm3533-als.c
> > > +++ b/drivers/iio/light/lm3533-als.c
> > > @@ -16,9 +16,12 @@
> > > #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>
> > >
> > > @@ -56,6 +59,9 @@ struct lm3533_als {
> > >
> > > atomic_t zone;
> > > struct mutex thresh_mutex;
> > > +
> > > + unsigned pwm_mode:1; /* PWM input mode (default analog) */
> > > + u8 r_select; /* 1 - 127 (ignored in PWM-mode) */
> > > };
> > >
> > >
> > > @@ -753,18 +759,17 @@ static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
> > > return 0;
> > > }
> > >
> > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > - const struct lm3533_als_platform_data *pdata)
> > > +static int lm3533_als_setup(struct lm3533_als *als)
> > > {
> > > int ret;
> > >
> > > - ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > > + ret = lm3533_als_set_input_mode(als, als->pwm_mode);
> > > if (ret)
> > > return ret;
> > >
> > > /* ALS input is always high impedance in PWM-mode. */
> > > - if (!pdata->pwm_mode) {
> > > - ret = lm3533_als_set_resistor(als, pdata->r_select);
> > > + if (!als->pwm_mode) {
> > > + ret = lm3533_als_set_resistor(als, als->r_select);
> >
> > You're already passing 'als'.
> >
> > Just teach lm3533_als_set_resistor that 'r_select' is now contained.
> >
>
> This is not scope of this patchset. I was already accused in too much
> changes which make it unreadable. This patchset is dedicated to
> swapping platform data to use of the device tree. NOT improving
> functions, NOT rewriting arbitrary mechanics. If you feed a need for
> this change, then propose a followup. I need from this driver only one
> thing, that it could work with device tree. But it seems that it is
> better that it just rots in the garbage bin until removed cause no one
> cared.
This is not an unreasonable request as you added r_select to als.
Perhaps it belongs in a separate follow up patch. However
it is worth remembering the motivation here is that you want get
this code upstream, the maintainers don't have that motivation.
Greg KH has given various talks on the different motivations in the
past. It maybe worth a watch.
>
> > > if (ret)
> > > return ret;
> > > }
> > > @@ -828,22 +833,16 @@ 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;
> > > + u32 val;
> >
> > Value of what, potatoes?
> >
>
> Oranges.
A well named variable would avoid need for any discussion of
what it is the value of.
>
> > > int ret;
> > >
> > > lm3533 = dev_get_drvdata(pdev->dev.parent);
> > > 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;
> > > @@ -864,13 +863,21 @@ static int lm3533_als_probe(struct platform_device *pdev)
> > >
> > > platform_set_drvdata(pdev, indio_dev);
> > >
> > > + val = 200 * KILO; /* 200kOhm */
> >
> > Better to #define magic numbers; DEFAULT_{DESCRIPTION}_OHMS
> >
>
> Why? that is not needed.
If this variable had a more useful name there would be no need for
the comment either.
val_resitor_ohms = 200 * KILLO;
or similar.
>
> > > + device_property_read_u32(&pdev->dev, "ti,resistor-value-ohm", &val);
> > > +
> > > + /* Convert resitance into R_ALS value with 2v / 10uA * R */
> >
> > Because ...
> >
>
> BACAUSE the device DOES NOT understand human readable values, only 0s
> and 1s, hence mOhms must be converted into value lm3533 chip can
> understand.
A comment that gave the motivation would be much more useful than
repeating the maths.
/* Convert resistance to equivalent register value */
>
> > > + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * val);
> > > +
> > > + als->pwm_mode = device_property_read_bool(&pdev->dev, "ti,pwm-mode");
> > > +
> > > if (als->irq) {
> > > 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 +914,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,
Anyhow, I'm short on time so only looking at the IIO related part.
Jonathan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] mfd: lm3533: convert to use OF
2025-03-05 13:44 ` Jonathan Cameron
@ 2025-03-05 14:18 ` Svyatoslav Ryhel
2025-03-08 17:19 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Svyatoslav Ryhel @ 2025-03-05 14:18 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Lars-Peter Clausen, Pavel Machek, Daniel Thompson, Jingoo Han,
Helge Deller, Andy Shevchenko, Uwe Kleine-König, devicetree,
linux-kernel, linux-iio, linux-leds, dri-devel, linux-fbdev
ср, 5 бер. 2025 р. о 15:45 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Fri, 28 Feb 2025 11:30:51 +0200
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > пт, 28 лют. 2025 р. о 10:59 Lee Jones <lee@kernel.org> пише:
> > >
> > > On Mon, 24 Feb 2025, Svyatoslav Ryhel wrote:
> > >
> > > > Remove platform data and fully relay on OF and device tree
> > > > parsing and binding devices.
> > > >
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > ---
> > > > drivers/iio/light/lm3533-als.c | 40 ++++---
> > > > drivers/leds/leds-lm3533.c | 46 +++++---
> > > > drivers/mfd/lm3533-core.c | 159 ++++++++--------------------
> > > > drivers/video/backlight/lm3533_bl.c | 71 ++++++++++---
> > > > include/linux/mfd/lm3533.h | 35 +-----
> > > > 5 files changed, 164 insertions(+), 187 deletions(-)
> > > >
...
> > > > /* 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_als_set_resistor(als, als->r_select);
> > >
> > > You're already passing 'als'.
> > >
> > > Just teach lm3533_als_set_resistor that 'r_select' is now contained.
> > >
> >
> > This is not scope of this patchset. I was already accused in too much
> > changes which make it unreadable. This patchset is dedicated to
> > swapping platform data to use of the device tree. NOT improving
> > functions, NOT rewriting arbitrary mechanics. If you feed a need for
> > this change, then propose a followup. I need from this driver only one
> > thing, that it could work with device tree. But it seems that it is
> > better that it just rots in the garbage bin until removed cause no one
> > cared.
>
> This is not an unreasonable request as you added r_select to als.
> Perhaps it belongs in a separate follow up patch.
I have just moved values used in pdata to private structs of each
driver. Without changing names or purpose.
> However
> it is worth remembering the motivation here is that you want get
> this code upstream, the maintainers don't have that motivation.
This driver is already upstream and it is useless and incompatible
with majority of supported devices. Maintainers should encourage those
who try to help and instead we have what? A total discouragement. Well
defined path into nowhere.
>
> Greg KH has given various talks on the different motivations in the
> past. It maybe worth a watch.
>
>
> >
> > > > if (ret)
> > > > return ret;
> > > > }
> > > > @@ -828,22 +833,16 @@ 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;
> > > > + u32 val;
> > >
> > > Value of what, potatoes?
> > >
> >
> > Oranges.
>
> A well named variable would avoid need for any discussion of
> what it is the value of.
>
This is temporary placeholder used to get values from the tree and
then pass it driver struct.
> >
> > > > int ret;
> > > >
> > > > lm3533 = dev_get_drvdata(pdev->dev.parent);
> > > > 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;
> > > > @@ -864,13 +863,21 @@ static int lm3533_als_probe(struct platform_device *pdev)
> > > >
> > > > platform_set_drvdata(pdev, indio_dev);
> > > >
> > > > + val = 200 * KILO; /* 200kOhm */
> > >
> > > Better to #define magic numbers; DEFAULT_{DESCRIPTION}_OHMS
> > >
> >
> > Why? that is not needed.
> If this variable had a more useful name there would be no need for
> the comment either.
>
> val_resitor_ohms = 200 * KILLO;
>
> or similar.
>
So I have to add a "reasonably" named variable for each property I
want to get from device tree? Why? It seems to be a bit of overkill,
no? Maybe I am not aware, have variables stopped being reusable?
> >
> > > > + device_property_read_u32(&pdev->dev, "ti,resistor-value-ohm", &val);
> > > > +
> > > > + /* Convert resitance into R_ALS value with 2v / 10uA * R */
> > >
> > > Because ...
> > >
> >
> > BACAUSE the device DOES NOT understand human readable values, only 0s
> > and 1s, hence mOhms must be converted into value lm3533 chip can
> > understand.
> A comment that gave the motivation would be much more useful than
> repeating the maths.
>
> /* Convert resistance to equivalent register value */
>
ok, this is reasonable.
> >
> > > > + als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * val);
> > > > +
> > > > + als->pwm_mode = device_property_read_bool(&pdev->dev, "ti,pwm-mode");
> > > > +
> > > > if (als->irq) {
> > > > 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 +914,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,
>
> Anyhow, I'm short on time so only looking at the IIO related part.
>
> Jonathan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] mfd: lm3533: convert to use OF
2025-03-05 14:18 ` Svyatoslav Ryhel
@ 2025-03-08 17:19 ` Jonathan Cameron
2025-03-11 13:40 ` Lee Jones
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2025-03-08 17:19 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Lars-Peter Clausen, Pavel Machek, Daniel Thompson, Jingoo Han,
Helge Deller, Andy Shevchenko, Uwe Kleine-König, devicetree,
linux-kernel, linux-iio, linux-leds, dri-devel, linux-fbdev
On Wed, 5 Mar 2025 16:18:38 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> ср, 5 бер. 2025 р. о 15:45 Jonathan Cameron <jic23@kernel.org> пише:
> >
> > On Fri, 28 Feb 2025 11:30:51 +0200
> > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >
> > > пт, 28 лют. 2025 р. о 10:59 Lee Jones <lee@kernel.org> пише:
> > > >
> > > > On Mon, 24 Feb 2025, Svyatoslav Ryhel wrote:
> > > >
> > > > > Remove platform data and fully relay on OF and device tree
> > > > > parsing and binding devices.
> > > > >
> > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > > ---
> > > > > drivers/iio/light/lm3533-als.c | 40 ++++---
> > > > > drivers/leds/leds-lm3533.c | 46 +++++---
> > > > > drivers/mfd/lm3533-core.c | 159 ++++++++--------------------
> > > > > drivers/video/backlight/lm3533_bl.c | 71 ++++++++++---
> > > > > include/linux/mfd/lm3533.h | 35 +-----
> > > > > 5 files changed, 164 insertions(+), 187 deletions(-)
> > > > >
> ...
> > > > > /* 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_als_set_resistor(als, als->r_select);
> > > >
> > > > You're already passing 'als'.
> > > >
> > > > Just teach lm3533_als_set_resistor that 'r_select' is now contained.
> > > >
> > >
> > > This is not scope of this patchset. I was already accused in too much
> > > changes which make it unreadable. This patchset is dedicated to
> > > swapping platform data to use of the device tree. NOT improving
> > > functions, NOT rewriting arbitrary mechanics. If you feed a need for
> > > this change, then propose a followup. I need from this driver only one
> > > thing, that it could work with device tree. But it seems that it is
> > > better that it just rots in the garbage bin until removed cause no one
> > > cared.
> >
> > This is not an unreasonable request as you added r_select to als.
> > Perhaps it belongs in a separate follow up patch.
>
> I have just moved values used in pdata to private structs of each
> driver. Without changing names or purpose.
>
> > However
> > it is worth remembering the motivation here is that you want get
> > this code upstream, the maintainers don't have that motivation.
>
> This driver is already upstream and it is useless and incompatible
> with majority of supported devices. Maintainers should encourage those
> who try to help and instead we have what? A total discouragement. Well
> defined path into nowhere.
That is not how I read the situation. A simple request was made to
result in more maintainable code as a direct result of that
improvement being enabled by code changes you were making.
I'm sorry to hear that discouraged you.
>
> >
> > Greg KH has given various talks on the different motivations in the
> > past. It maybe worth a watch.
> >
> >
> > >
> > > > > if (ret)
> > > > > return ret;
> > > > > }
> > > > > @@ -828,22 +833,16 @@ 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;
> > > > > + u32 val;
> > > >
> > > > Value of what, potatoes?
> > > >
> > >
> > > Oranges.
> >
> > A well named variable would avoid need for any discussion of
> > what it is the value of.
> >
>
> This is temporary placeholder used to get values from the tree and
> then pass it driver struct.
Better if it is a temporary placeholder with a meaningful name.
>
> > >
> > > > > int ret;
> > > > >
> > > > > lm3533 = dev_get_drvdata(pdev->dev.parent);
> > > > > 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;
> > > > > @@ -864,13 +863,21 @@ static int lm3533_als_probe(struct platform_device *pdev)
> > > > >
> > > > > platform_set_drvdata(pdev, indio_dev);
> > > > >
> > > > > + val = 200 * KILO; /* 200kOhm */
> > > >
> > > > Better to #define magic numbers; DEFAULT_{DESCRIPTION}_OHMS
> > > >
> > >
> > > Why? that is not needed.
> > If this variable had a more useful name there would be no need for
> > the comment either.
> >
> > val_resitor_ohms = 200 * KILLO;
> >
> > or similar.
> >
>
> So I have to add a "reasonably" named variable for each property I
> want to get from device tree? Why? It seems to be a bit of overkill,
> no? Maybe I am not aware, have variables stopped being reusable?
Lets go with yes if you want a definitive answer. In reality it's
a question of how many are needed. If 10-100s sure reuse is fine,
if just a few sensible naming can remove the need for comments
and improve readability.
Jonathan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] mfd: lm3533: convert to use OF
2025-03-08 17:19 ` Jonathan Cameron
@ 2025-03-11 13:40 ` Lee Jones
0 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2025-03-11 13:40 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Svyatoslav Ryhel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Lars-Peter Clausen, Pavel Machek, Daniel Thompson, Jingoo Han,
Helge Deller, Andy Shevchenko, Uwe Kleine-König, devicetree,
linux-kernel, linux-iio, linux-leds, dri-devel, linux-fbdev
On Sat, 08 Mar 2025, Jonathan Cameron wrote:
> On Wed, 5 Mar 2025 16:18:38 +0200
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > ср, 5 бер. 2025 р. о 15:45 Jonathan Cameron <jic23@kernel.org> пише:
> > >
> > > On Fri, 28 Feb 2025 11:30:51 +0200
> > > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> > >
> > > > пт, 28 лют. 2025 р. о 10:59 Lee Jones <lee@kernel.org> пише:
> > > > >
> > > > > On Mon, 24 Feb 2025, Svyatoslav Ryhel wrote:
> > > > >
> > > > > > Remove platform data and fully relay on OF and device tree
> > > > > > parsing and binding devices.
> > > > > >
> > > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > > > ---
> > > > > > drivers/iio/light/lm3533-als.c | 40 ++++---
> > > > > > drivers/leds/leds-lm3533.c | 46 +++++---
> > > > > > drivers/mfd/lm3533-core.c | 159 ++++++++--------------------
> > > > > > drivers/video/backlight/lm3533_bl.c | 71 ++++++++++---
> > > > > > include/linux/mfd/lm3533.h | 35 +-----
> > > > > > 5 files changed, 164 insertions(+), 187 deletions(-)
> > > > > >
> > ...
> > > > > > /* 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_als_set_resistor(als, als->r_select);
> > > > >
> > > > > You're already passing 'als'.
> > > > >
> > > > > Just teach lm3533_als_set_resistor that 'r_select' is now contained.
> > > > >
> > > >
> > > > This is not scope of this patchset. I was already accused in too much
> > > > changes which make it unreadable. This patchset is dedicated to
> > > > swapping platform data to use of the device tree. NOT improving
> > > > functions, NOT rewriting arbitrary mechanics. If you feed a need for
> > > > this change, then propose a followup. I need from this driver only one
> > > > thing, that it could work with device tree. But it seems that it is
> > > > better that it just rots in the garbage bin until removed cause no one
> > > > cared.
> > >
> > > This is not an unreasonable request as you added r_select to als.
> > > Perhaps it belongs in a separate follow up patch.
> >
> > I have just moved values used in pdata to private structs of each
> > driver. Without changing names or purpose.
> >
> > > However
> > > it is worth remembering the motivation here is that you want get
> > > this code upstream, the maintainers don't have that motivation.
> >
> > This driver is already upstream and it is useless and incompatible
> > with majority of supported devices. Maintainers should encourage those
> > who try to help and instead we have what? A total discouragement. Well
> > defined path into nowhere.
>
> That is not how I read the situation. A simple request was made to
> result in more maintainable code as a direct result of that
> improvement being enabled by code changes you were making.
> I'm sorry to hear that discouraged you.
>
> >
> > >
> > > Greg KH has given various talks on the different motivations in the
> > > past. It maybe worth a watch.
> > >
> > >
> > > >
> > > > > > if (ret)
> > > > > > return ret;
> > > > > > }
> > > > > > @@ -828,22 +833,16 @@ 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;
> > > > > > + u32 val;
> > > > >
> > > > > Value of what, potatoes?
> > > > >
> > > >
> > > > Oranges.
> > >
> > > A well named variable would avoid need for any discussion of
> > > what it is the value of.
> > >
> >
> > This is temporary placeholder used to get values from the tree and
> > then pass it driver struct.
>
> Better if it is a temporary placeholder with a meaningful name.
>
> >
> > > >
> > > > > > int ret;
> > > > > >
> > > > > > lm3533 = dev_get_drvdata(pdev->dev.parent);
> > > > > > 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;
> > > > > > @@ -864,13 +863,21 @@ static int lm3533_als_probe(struct platform_device *pdev)
> > > > > >
> > > > > > platform_set_drvdata(pdev, indio_dev);
> > > > > >
> > > > > > + val = 200 * KILO; /* 200kOhm */
> > > > >
> > > > > Better to #define magic numbers; DEFAULT_{DESCRIPTION}_OHMS
> > > > >
> > > >
> > > > Why? that is not needed.
> > > If this variable had a more useful name there would be no need for
> > > the comment either.
> > >
> > > val_resitor_ohms = 200 * KILLO;
> > >
> > > or similar.
> > >
> >
> > So I have to add a "reasonably" named variable for each property I
> > want to get from device tree? Why? It seems to be a bit of overkill,
> > no? Maybe I am not aware, have variables stopped being reusable?
>
> Lets go with yes if you want a definitive answer. In reality it's
> a question of how many are needed. If 10-100s sure reuse is fine,
> if just a few sensible naming can remove the need for comments
> and improve readability.
>
> Jonathan
You clearly have more patience for this than I do! =;-)
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-03-11 13:40 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-24 11:48 [PATCH v3 0/2] mfd: lm3533: convert to use OF Svyatoslav Ryhel
2025-02-24 11:48 ` [PATCH v3 1/2] dt-bindings: mfd: Document TI LM3533 MFD Svyatoslav Ryhel
2025-02-24 20:31 ` Rob Herring
2025-02-24 11:48 ` [PATCH v3 2/2] mfd: lm3533: convert to use OF Svyatoslav Ryhel
2025-02-28 8:59 ` Lee Jones
2025-02-28 9:30 ` Svyatoslav Ryhel
2025-03-05 13:44 ` Jonathan Cameron
2025-03-05 14:18 ` Svyatoslav Ryhel
2025-03-08 17:19 ` Jonathan Cameron
2025-03-11 13:40 ` Lee Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).