[2/2] leds: max597x: Add support for max597x
Commit Message
From: Patrick Rudolph <patrick.rudolph@9elements.com>
max597x is hot swap controller with indication led support.
This driver uses DT property to configure led during boot time &
also provide the led control in sysfs.
Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
drivers/leds/Kconfig | 11 +++
drivers/leds/Makefile | 1 +
drivers/leds/leds-max597x.c | 132 ++++++++++++++++++++++++++++++++++++
3 files changed, 144 insertions(+)
create mode 100644 drivers/leds/leds-max597x.c
Comments
Hi Naresh,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 368eb79f738a21e16c2bdbcac2444dfa96b01aaa]
url: https://github.com/intel-lab-lkp/linux/commits/Naresh-Solanki/leds-max597x-Add-support-for-max597x/20230322-204408
base: 368eb79f738a21e16c2bdbcac2444dfa96b01aaa
patch link: https://lore.kernel.org/r/20230322124316.2147143-2-Naresh.Solanki%409elements.com
patch subject: [PATCH 2/2] leds: max597x: Add support for max597x
reproduce:
make versioncheck
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303230037.O7fPpT2X-lkp@intel.com/
versioncheck warnings: (new ones prefixed by >>)
INFO PATH=/opt/cross/clang/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
/usr/bin/timeout -k 100 3h /usr/bin/make W=1 --keep-going HOSTCC=gcc-11 CC=gcc-11 -j32 ARCH=x86_64 versioncheck
find ./* \( -name SCCS -o -name BitKeeper -o -name .svn -o -name CVS -o -name .pc -o -name .hg -o -name .git \) -prune -o \
-name '*.[hcS]' -type f -print | sort \
| xargs perl -w ./scripts/checkversion.pl
./drivers/accessibility/speakup/genmap.c: 13 linux/version.h not needed.
./drivers/accessibility/speakup/makemapdata.c: 13 linux/version.h not needed.
./drivers/iio/adc/max597x-iio.c: 20 linux/version.h not needed.
>> ./drivers/leds/leds-max597x.c: 20 linux/version.h not needed.
./drivers/net/ethernet/qlogic/qede/qede.h: 10 linux/version.h not needed.
./drivers/net/ethernet/qlogic/qede/qede_ethtool.c: 7 linux/version.h not needed.
./drivers/soc/tegra/cbb/tegra-cbb.c: 19 linux/version.h not needed.
./drivers/soc/tegra/cbb/tegra194-cbb.c: 26 linux/version.h not needed.
./drivers/soc/tegra/cbb/tegra234-cbb.c: 27 linux/version.h not needed.
./drivers/staging/media/atomisp/include/linux/atomisp.h: 23 linux/version.h not needed.
./samples/trace_events/trace_custom_sched.c: 11 linux/version.h not needed.
./sound/soc/codecs/cs42l42.c: 14 linux/version.h not needed.
./tools/lib/bpf/bpf_helpers.h: 289: need linux/version.h
./tools/perf/tests/bpf-script-example.c: 60: need linux/version.h
./tools/perf/tests/bpf-script-test-kbuild.c: 21: need linux/version.h
./tools/perf/tests/bpf-script-test-prologue.c: 49: need linux/version.h
./tools/perf/tests/bpf-script-test-relocation.c: 51: need linux/version.h
./tools/testing/selftests/bpf/progs/dev_cgroup.c: 9 linux/version.h not needed.
./tools/testing/selftests/bpf/progs/netcnt_prog.c: 3 linux/version.h not needed.
./tools/testing/selftests/bpf/progs/test_map_lock.c: 4 linux/version.h not needed.
./tools/testing/selftests/bpf/progs/test_send_signal_kern.c: 4 linux/version.h not needed.
./tools/testing/selftests/bpf/progs/test_spin_lock.c: 4 linux/version.h not needed.
./tools/testing/selftests/bpf/progs/test_tcp_estats.c: 37 linux/version.h not needed.
./tools/testing/selftests/wireguard/qemu/init.c: 27 linux/version.h not needed.
On Wed 2023-03-22 13:43:16, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>
> max597x is hot swap controller with indication led support.
"indicator LED"?
> This driver uses DT property to configure led during boot time &
> also provide the led control in sysfs.
LED.
> +++ 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 597x smart switch indication LEDs
> + via the I2C bus.
Strange whitespace.
> --- /dev/null
> +++ b/drivers/leds/leds-max597x.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device driver for regulators in MAX5970 and MAX5978 IC
Regulators go elsewhere.
> +static int max597x_led_set_brightness(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + struct max597x_led *led = ldev_to_maxled(cdev);
> + int ret;
> +
> + if (!led || !led->regmap)
> + return -ENODEV;
> +
> + ret = regmap_update_bits(led->regmap, MAX5970_REG_LED_FLASH,
> + 1 << led->index, ~brightness << led->index);
~brightness << led->index is quite confusing. Can we get something
else?
> + led = devm_kzalloc(dev, sizeof(struct max597x_led),
> + GFP_KERNEL);
> + if (!led)
> + 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 = led_classdev_register(dev, &led->led);
> + if (ret) {
> + dev_err(dev, "Error in initializing led %s", led->led.name);
> + devm_kfree(dev, led);
> + return ret;
> + }
You don't need to do the kfree.
> + if (!of_property_read_string(nc, "default-state", &state)) {
> + if (!strcmp(state, "on")) {
> + led->led.brightness = 1;
> + led_set_brightness(&led->led, led->led.brightness);
> + }
> + }
Lets get rid of this unless you really need it.
BR,
Pavel
Hi!
> max597x is hot swap controller with indication led support.
> This driver uses DT property to configure led during boot time &
> also provide the led control in sysfs.
Can you provide dts example showing how you'll use it?
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> +static int max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc,
> + u32 reg)
> +{
...
> + ret = led_classdev_register(dev, &led->led);
> + return 0;
> +}
> +
> +static int max597x_led_probe(struct platform_device *pdev)
> +{
...
> + 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(&i2c->dev, "invalid LED (%u >= %d)\n", reg,
> + MAX597X_NUM_LEDS);
> + continue;
> + }
> +
> + ret = max597x_setup_led(&i2c->dev, regmap, child, reg);
> + if (ret < 0) {
> + of_node_put(child);
> + return ret;
> + }
This will cause crashes. After you successfully registered one LED,
you can't just bail out.
BR,
Pavel
@@ -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 597x smart switch indication LEDs
+ via the I2C bus.
+
+ To compile this driver as a module, choose M here: the module will
+ be called max597x-led.
+
config LEDS_MC13783
tristate "LED Support for MC13XXX PMIC"
depends on LEDS_CLASS
@@ -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
new file mode 100644
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device driver for regulators in MAX5970 and MAX5978 IC
+ *
+ * Copyright (c) 2022 9elements GmbH
+ *
+ * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/i2c.h>
+#include <linux/mfd/max597x.h>
+#include <linux/regmap.h>
+#include <linux/version.h>
+#include <linux/platform_device.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;
+
+ if (!led || !led->regmap)
+ return -ENODEV;
+
+ ret = regmap_update_bits(led->regmap, MAX5970_REG_LED_FLASH,
+ 1 << led->index, ~brightness << led->index);
+ if (ret < 0)
+ dev_err(cdev->dev, "failed to set brightness %d\n", ret);
+ return ret;
+}
+
+static int max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc,
+ u32 reg)
+{
+ struct max597x_led *led;
+ const char *state;
+ int ret = 0;
+
+ led = devm_kzalloc(dev, sizeof(struct max597x_led),
+ GFP_KERNEL);
+ if (!led)
+ 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 = led_classdev_register(dev, &led->led);
+ if (ret) {
+ dev_err(dev, "Error in initializing led %s", led->led.name);
+ devm_kfree(dev, led);
+ return ret;
+ }
+
+ if (!of_property_read_string(nc, "default-state", &state)) {
+ if (!strcmp(state, "on")) {
+ led->led.brightness = 1;
+ led_set_brightness(&led->led, led->led.brightness);
+ }
+ }
+ return 0;
+}
+
+static int max597x_led_probe(struct platform_device *pdev)
+{
+ struct device_node *np = dev_of_node(pdev->dev.parent);
+ struct i2c_client *i2c = to_i2c_client(pdev->dev.parent);
+ struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ struct device_node *led_node;
+ struct device_node *child;
+ int ret = 0;
+
+ if (!regmap)
+ return -EPROBE_DEFER;
+
+ led_node = of_get_child_by_name(np, "leds");
+ if (!led_node)
+ return -ENODEV;
+
+ 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(&i2c->dev, "invalid LED (%u >= %d)\n", reg,
+ MAX597X_NUM_LEDS);
+ continue;
+ }
+
+ ret = max597x_setup_led(&i2c->dev, regmap, child, reg);
+ if (ret < 0) {
+ of_node_put(child);
+ return ret;
+ }
+ }
+
+ return ret;
+}
+
+static struct platform_driver max597x_led_driver = {
+ .driver = {
+ .name = "max597x-led",
+ },
+ .probe = max597x_led_probe,
+};
+
+module_platform_driver(max597x_led_driver);
+
+MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
+MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
+MODULE_LICENSE("GPL");