Re: [PATCH v3 2/2] leds: max597x: Add support for max597x
From: Naresh Solanki
Date: Wed Apr 12 2023 - 05:00:41 EST
Hi Lee,
On 05-04-2023 08:37 pm, Lee Jones wrote:
On Tue, 28 Mar 2023, Naresh Solanki wrote:
From: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
max597x is hot swap controller with indicator LED support.
This driver uses DT property to configure led during boot time &
also provide the LED control in sysfs.
DTS example:
i2c {
#address-cells = <1>;
#size-cells = <0>;
regulator@3a {
compatible = "maxim,max5978";
reg = <0x3a>;
vss1-supply = <&p3v3>;
regulators {
sw0_ref_0: sw0 {
shunt-resistor-micro-ohms = <12000>;
};
};
leds {
#address-cells = <1>;
#size-cells = <0>;
led@0 {
reg = <0>;
label = "led0";
default-state = "on";
};
led@1 {
reg = <1>;
label = "led1";
default-state = "on";
};
};
};
};
Signed-off-by: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
...
Changes in V3:
- Remove of_node_put as its handled by for loop
- Print error if an LED fails to register.
- Update driver name in Kconfig description
- Remove unneeded variable assignment
- Use devm_led_classdev_register to reget led
Changes in V2:
- Fix regmap update
- Remove devm_kfree
- Remove default-state
- Add example dts in commit message
- Fix whitespace in Kconfig
- Fix comment
---
drivers/leds/Kconfig | 11 ++++
drivers/leds/Makefile | 1 +
drivers/leds/leds-max597x.c | 112 ++++++++++++++++++++++++++++++++++++
3 files changed, 124 insertions(+)
create mode 100644 drivers/leds/leds-max597x.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9dbce09eabac..60004cb8c257 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -590,6 +590,17 @@ config LEDS_ADP5520
To compile this driver as a module, choose M here: the module will
be called leds-adp5520.
+config LEDS_MAX597X
+ tristate "LED Support for Maxim 597x"
+ depends on LEDS_CLASS
+ depends on MFD_MAX597X
+ help
+ This option enables support for the Maxim MAX5970 & MAX5978 smart
+ switch indication LEDs via the I2C bus.
+
+ To compile this driver as a module, choose M here: the module will
+ be called leds-max597x.
+
config LEDS_MC13783
tristate "LED Support for MC13XXX PMIC"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d30395d11fd8..da1192e40268 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_LP8501) += leds-lp8501.o
obj-$(CONFIG_LEDS_LP8788) += leds-lp8788.o
obj-$(CONFIG_LEDS_LP8860) += leds-lp8860.o
obj-$(CONFIG_LEDS_LT3593) += leds-lt3593.o
+obj-$(CONFIG_LEDS_MAX597X) += leds-max597x.o
obj-$(CONFIG_LEDS_MAX77650) += leds-max77650.o
obj-$(CONFIG_LEDS_MAX8997) += leds-max8997.o
obj-$(CONFIG_LEDS_MC13783) += leds-mc13783.o
diff --git a/drivers/leds/leds-max597x.c b/drivers/leds/leds-max597x.c
new file mode 100644
index 000000000000..83e4dfb617fb
--- /dev/null
+++ b/drivers/leds/leds-max597x.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device driver for leds in MAX5970 and MAX5978 IC
+ *
+ * Copyright (c) 2022 9elements GmbH
+ *
+ * Author: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
+ */
+
+#include <linux/leds.h>
+#include <linux/mfd/max597x.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define ldev_to_maxled(c) container_of(c, struct max597x_led, led)
+
+struct max597x_led {
+ struct regmap *regmap;
+ struct led_classdev led;
+ unsigned int index;
+};
+
+static int max597x_led_set_brightness(struct led_classdev *cdev,
+ enum led_brightness brightness)
+{
+ struct max597x_led *led = ldev_to_maxled(cdev);
+ int ret, val = 0;
Why preinitialise?
Ack. Not needed so will remove.
+ if (!led || !led->regmap)
+ return -ENODEV;
Can !led actually happen?
Ack. Will remove !led .
+ val = !brightness ? BIT(led->index) : 0;
Perhaps a comment?
Sure. Will add "Set/Clear Led index bit"
+ ret = regmap_update_bits(led->regmap, MAX5970_REG_LED_FLASH, BIT(led->index), val);
+ if (ret < 0)
+ dev_err(cdev->dev, "failed to set brightness %d\n", ret);
'\n'
Ack
+ return ret;
+}
+
+static int max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc,
+ u32 reg)
+{
+ struct max597x_led *led;
+ int ret;
+
+ led = devm_kzalloc(dev, sizeof(struct max597x_led),
+ GFP_KERNEL);
Consistently break at 100-chars please.
You have lines wayyyy longer than this elsewhere.
Ack. Will align with 100-chars. Thanks
+ if (!led)
'led' is confusing. Either this or the member 'led' should be changed.
Perhaps ddata here and cdev for the member?
Sure.
+ return -ENOMEM;
+
+ if (of_property_read_string(nc, "label", &led->led.name))
+ led->led.name = nc->name;
+
+ led->led.max_brightness = 1;
+ led->led.brightness_set_blocking = max597x_led_set_brightness;
+ led->led.default_trigger = "none";
+ led->index = reg;
+ led->regmap = regmap;
+ ret = devm_led_classdev_register(dev, &led->led);
+ if (ret)
+ dev_err(dev, "Error in initializing led %s", led->led.name);
Drop the "in" and s/led/LED/
Ack
+
+ return ret;
+}
+
+static int max597x_led_probe(struct platform_device *pdev)
+{
+ struct device_node *np = dev_of_node(pdev->dev.parent);
Why not have your own compatible string?
This is leaf driver & MFD driver does has compatible string.
+ struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
These "big" API calls are usually done outside of the allocation block.
Please move it to just above the check for !regmap.
+ struct device_node *led_node;
+ struct device_node *child;
+ int ret = 0;
Is it okay for an LED driver to not to register any LEDs?
Yes. Usage of indication LED on the max5970/5978 is optional.
Perhaps -ENODEV?
This driver is loaded only if MFD driver is included. remap is setup by
MFD driver & hence defer probe till MFD driver is loaded.
+ if (!regmap)
+ return -EPROBE_DEFER;
+
+ led_node = of_get_child_by_name(np, "leds");
+ if (!led_node)
+ return -ENODEV;
Ah, that's better. So set ret to -ENODEV and use it here.
Yes.
+ for_each_available_child_of_node(led_node, child) {
+ u32 reg;
+
+ if (of_property_read_u32(child, "reg", ®))
+ continue;
+
+ if (reg >= MAX597X_NUM_LEDS) {
+ dev_err(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
+ MAX597X_NUM_LEDS);
+ continue;
+ }
+
+ ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
+ if (ret < 0)
+ dev_err(&pdev->dev, "Failed to initialize LED %u\n", reg);
I think you (or I) are missing the point of the previous reviews. It's
not okay to error out and continue executing. Either this is okay (you
can warn and carry on) or it's not (return an error). Your first
submission suggested that this was an error. In which case you do need
to return. I think Pavel was suggesting that you should unwind
(de-register) before retuning, rather than leaving things in an odd
half-registered state. Not that you should blindly carry on as if the
issue never occurred.
I did refer to other such implementations & some have used return on
error & some just print on error & continue. I felt that continue
executing with warning(on error) is better approach.
+ }
+
+ return ret;
+}
+
+static struct platform_driver max597x_led_driver = {
+ .driver = {
+ .name = "max597x-led",
+ },
+ .probe = max597x_led_probe,
+};
+
Remove this line.
+module_platform_driver(max597x_led_driver);
+
+MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
Odd. I thought this was a LED driver?
This also has LED. will update the strign to:
MAX5970_hot-swap controller LED driver
+MODULE_LICENSE("GPL");
--
2.39.1
--
Lee Jones [李琼斯]
Regards,
Naresh