| Message ID | 20230322124316.2147143-2-Naresh.Solanki@9elements.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 9dbce09eabac..ba184a3f736e 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 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 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..e2844202a35b --- /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 + * + * 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");