From: Patrick Rudolph <[email protected]>
max597x has 10bit ADC for voltage & current monitoring.
Use iio framework to expose the same in sysfs.
Signed-off-by: Patrick Rudolph <[email protected]>
Signed-off-by: Naresh Solanki <[email protected]>
---
drivers/iio/adc/Kconfig | 15 ++++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/max597x-iio.c | 162 ++++++++++++++++++++++++++++++++++
3 files changed, 178 insertions(+)
create mode 100644 drivers/iio/adc/max597x-iio.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 45af2302be53..0d1a3dea0b7d 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -735,6 +735,21 @@ config MAX1363
To compile this driver as a module, choose M here: the module will be
called max1363.
+config MAX597X_IIO
+ tristate "Maxim 597x power switch and monitor"
+ depends on I2C && OF
+ select MFD_MAX597X
+ help
+ This driver enables support for the Maxim 597x smart switch and
+ voltage/current monitoring interface using the Industrial I/O (IIO)
+ framework. The Maxim 597x is a power switch and monitor that can
+ provide voltage and current measurements via the I2C bus. Enabling
+ this driver will allow user space applications to read the voltage
+ and current measurements using IIO interfaces.
+
+ To compile this driver as a module, choose M here: the module will be
+ called max597x-iio.
+
config MAX9611
tristate "Maxim max9611/max9612 ADC driver"
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 36c18177322a..7ec0c2cf7bbb 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_MAX11205) += max11205.o
obj-$(CONFIG_MAX11410) += max11410.o
obj-$(CONFIG_MAX1241) += max1241.o
obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MAX597X_IIO) += max597x-iio.o
obj-$(CONFIG_MAX9611) += max9611.o
obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_MCP3422) += mcp3422.o
diff --git a/drivers/iio/adc/max597x-iio.c b/drivers/iio/adc/max597x-iio.c
new file mode 100644
index 000000000000..a35bab926313
--- /dev/null
+++ b/drivers/iio/adc/max597x-iio.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device driver for regulators in MAX5970 and MAX5978 IC
+ *
+ * Copyright (c) 2022 9elements GmbH
+ *
+ * Author: Patrick Rudolph <[email protected]>
+ */
+
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/iio/iio.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>
+
+struct max597x_iio {
+ struct regmap *regmap;
+ int shunt_micro_ohms[MAX5970_NUM_SWITCHES];
+ unsigned int irng[MAX5970_NUM_SWITCHES];
+ unsigned int mon_rng[MAX5970_NUM_SWITCHES];
+};
+
+#define MAX597X_ADC_CHANNEL(_idx, _type) { \
+ .type = IIO_ ## _type, \
+ .indexed = 1, \
+ .channel = (_idx), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .address = MAX5970_REG_ ## _type ## _L(_idx), \
+}
+
+static const struct iio_chan_spec max5978_adc_iio_channels[] = {
+ MAX597X_ADC_CHANNEL(0, VOLTAGE),
+ MAX597X_ADC_CHANNEL(0, CURRENT),
+};
+
+static const struct iio_chan_spec max5970_adc_iio_channels[] = {
+ MAX597X_ADC_CHANNEL(0, VOLTAGE),
+ MAX597X_ADC_CHANNEL(0, CURRENT),
+ MAX597X_ADC_CHANNEL(1, VOLTAGE),
+ MAX597X_ADC_CHANNEL(1, CURRENT),
+};
+
+static int max597x_iio_read_raw(struct iio_dev *iio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long info)
+{
+ int ret;
+ struct max597x_iio *data = iio_priv(iio_dev);
+ unsigned int reg_l, reg_h;
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ ret = regmap_read(data->regmap, chan->address, ®_l);
+ if (ret < 0)
+ return ret;
+ ret = regmap_read(data->regmap, chan->address - 1, ®_h);
+ if (ret < 0)
+ return ret;
+ *val = (reg_h << 2) | (reg_l & 3);
+
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+
+ switch (chan->address) {
+ case MAX5970_REG_CURRENT_L(0):
+ fallthrough;
+ case MAX5970_REG_CURRENT_L(1):
+ /* in A, convert to mA */
+ *val = data->irng[chan->channel] * 1000;
+ *val2 =
+ data->shunt_micro_ohms[chan->channel] * ADC_MASK;
+ return IIO_VAL_FRACTIONAL;
+
+ case MAX5970_REG_VOLTAGE_L(0):
+ fallthrough;
+ case MAX5970_REG_VOLTAGE_L(1):
+ /* in uV, convert to mV */
+ *val = data->mon_rng[chan->channel];
+ *val2 = ADC_MASK * 1000;
+ return IIO_VAL_FRACTIONAL;
+ }
+
+ break;
+ }
+ return -EINVAL;
+}
+
+static const struct iio_info max597x_adc_iio_info = {
+ .read_raw = &max597x_iio_read_raw,
+};
+
+static int max597x_iio_probe(struct platform_device *pdev)
+{
+ struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
+ struct i2c_client *i2c = to_i2c_client(pdev->dev.parent);
+ struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ struct iio_dev *indio_dev;
+ struct max597x_iio *priv;
+ int ret, i;
+
+ if (!regmap)
+ return -EPROBE_DEFER;
+
+ if (!max597x || !max597x->num_switches)
+ return -EPROBE_DEFER;
+
+ /* registering iio */
+ indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*priv));
+ if (!indio_dev) {
+ dev_err(&i2c->dev, "failed allocating iio device\n");
+ return -ENOMEM;
+ }
+ indio_dev->name = dev_name(&i2c->dev);
+ indio_dev->info = &max597x_adc_iio_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ switch (max597x->num_switches) {
+ case MAX597x_TYPE_MAX5970:
+ indio_dev->channels = max5970_adc_iio_channels;
+ indio_dev->num_channels = ARRAY_SIZE(max5970_adc_iio_channels);
+ break;
+ case MAX597x_TYPE_MAX5978:
+ indio_dev->channels = max5978_adc_iio_channels;
+ indio_dev->num_channels = ARRAY_SIZE(max5978_adc_iio_channels);
+ break;
+ }
+
+ priv = iio_priv(indio_dev);
+ priv->regmap = regmap;
+ for (i = 0; i < indio_dev->num_channels; i++) {
+ priv->irng[i] = max597x->irng[i];
+ priv->mon_rng[i] = max597x->mon_rng[i];
+ priv->shunt_micro_ohms[i] = max597x->shunt_micro_ohms[i];
+ }
+
+ ret = devm_iio_device_register(&i2c->dev, indio_dev);
+ if (ret)
+ dev_err(&i2c->dev, "could not register iio device");
+
+ return ret;
+}
+
+static struct platform_driver max597x_iio_driver = {
+ .driver = {
+ .name = "max597x-iio",
+ },
+ .probe = max597x_iio_probe,
+};
+
+module_platform_driver(max597x_iio_driver);
+
+MODULE_AUTHOR("Patrick Rudolph <[email protected]>");
+MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
+MODULE_LICENSE("GPL");
base-commit: 368eb79f738a21e16c2bdbcac2444dfa96b01aaa
--
2.39.1
From: Patrick Rudolph <[email protected]>
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 <[email protected]>
Signed-off-by: Naresh Solanki <[email protected]>
---
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
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 <[email protected]>
+ */
+
+#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 <[email protected]>");
+MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
+MODULE_LICENSE("GPL");
--
2.39.1
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-1-Naresh.Solanki%409elements.com
patch subject: [PATCH 1/2] iio: max597x: Add support for max597x
reproduce:
make versioncheck
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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/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.
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi,
This looks really good. A few minor comments inline.
On 3/22/23 05:43, Naresh Solanki wrote:
> [...]
> +static int max597x_iio_read_raw(struct iio_dev *iio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long info)
> +{
> + int ret;
> + struct max597x_iio *data = iio_priv(iio_dev);
> + unsigned int reg_l, reg_h;
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + ret = regmap_read(data->regmap, chan->address, ®_l);
> + if (ret < 0)
> + return ret;
> + ret = regmap_read(data->regmap, chan->address - 1, ®_h);
> + if (ret < 0)
> + return ret;
Is there any chance of a race condition of getting inconsistent data
when splitting this over two reads? I.e. registers being updated with
new values in between the two reads.
> + *val = (reg_h << 2) | (reg_l & 3);
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> +
> + switch (chan->address) {
> + case MAX5970_REG_CURRENT_L(0):
> + fallthrough;
`fallthrough` should not be needed for multiple case statements right on top of each other with no code in between. Same below
> + case MAX5970_REG_CURRENT_L(1):
> + /* in A, convert to mA */
> + *val = data->irng[chan->channel] * 1000;
> + *val2 =
> + data->shunt_micro_ohms[chan->channel] * ADC_MASK;
ADC_MASK should really have a MAX5970_ prefix, but I guess it is defined
in max597x.h
> + return IIO_VAL_FRACTIONAL;
> +
> + case MAX5970_REG_VOLTAGE_L(0):
> + fallthrough;
> + case MAX5970_REG_VOLTAGE_L(1):
> + /* in uV, convert to mV */
> + *val = data->mon_rng[chan->channel];
> + *val2 = ADC_MASK * 1000;
> + return IIO_VAL_FRACTIONAL;
> + }
> +
> + break;
> + }
> + return -EINVAL;
> +}
> [..]
> +static int max597x_iio_probe(struct platform_device *pdev)
> +{
> + struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
> + struct i2c_client *i2c = to_i2c_client(pdev->dev.parent);
> + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + struct iio_dev *indio_dev;
> + struct max597x_iio *priv;
> + int ret, i;
> +
> + if (!regmap)
> + return -EPROBE_DEFER;
> +
> + if (!max597x || !max597x->num_switches)
> + return -EPROBE_DEFER;
> +
> + /* registering iio */
> + indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*priv));
For the devm allocations we should be using &pdev->dev and not the I2C
device, since this is the device to which the allocations belong and
where they should be freed when the device is removed.
> + if (!indio_dev) {
> + dev_err(&i2c->dev, "failed allocating iio device\n");
Consider using dev_err_probe() for error message printing. This will
give a consistent formatting of the messages. Also again use &pdev->dev
instead of I2C device to get the right device listed in the error messages.
> + return -ENOMEM;
> + }
> + indio_dev->name = dev_name(&i2c->dev);
The IIO ABI wants the type of the chip for the name. E.g. "max5970",
using dev_name() of the parent I2C device will result in something else.
> [...]
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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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.
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Wed 2023-03-22 13:43:16, Naresh Solanki wrote:
> From: Patrick Rudolph <[email protected]>
>
> 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
--
People of Russia, stop Putin before his war on Ukraine escalates.
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 <[email protected]>
> Signed-off-by: Naresh Solanki <[email protected]>
> +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
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi,
On 22-03-2023 09:28 pm, Lars-Peter Clausen wrote:
> Hi,
>
> This looks really good. A few minor comments inline.
>
> On 3/22/23 05:43, Naresh Solanki wrote:
>> [...]
>> +static int max597x_iio_read_raw(struct iio_dev *iio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long info)
>> +{
>> + int ret;
>> + struct max597x_iio *data = iio_priv(iio_dev);
>> + unsigned int reg_l, reg_h;
>> +
>> + switch (info) {
>> + case IIO_CHAN_INFO_RAW:
>> + ret = regmap_read(data->regmap, chan->address, ®_l);
>> + if (ret < 0)
>> + return ret;
>> + ret = regmap_read(data->regmap, chan->address - 1, ®_h);
>> + if (ret < 0)
>> + return ret;
> Is there any chance of a race condition of getting inconsistent data
> when splitting this over two reads? I.e. registers being updated with
> new values in between the two reads.
yes, reg_l holds lower 2 bits. due to latency in reads, value may differ.
>> + *val = (reg_h << 2) | (reg_l & 3);
>> +
>> + return IIO_VAL_INT;
>> + case IIO_CHAN_INFO_SCALE:
>> +
>> + switch (chan->address) {
>> + case MAX5970_REG_CURRENT_L(0):
>> + fallthrough;
>
> `fallthrough` should not be needed for multiple case statements right on
> top of each other with no code in between. Same below
Sure.
>
>> + case MAX5970_REG_CURRENT_L(1):
>> + /* in A, convert to mA */
>> + *val = data->irng[chan->channel] * 1000;
>> + *val2 =
>> + data->shunt_micro_ohms[chan->channel] * ADC_MASK;
> ADC_MASK should really have a MAX5970_ prefix, but I guess it is defined
> in max597x.h
Yes its taken from max597x.h
>> + return IIO_VAL_FRACTIONAL;
>> +
>> + case MAX5970_REG_VOLTAGE_L(0):
>> + fallthrough;
>> + case MAX5970_REG_VOLTAGE_L(1):
>> + /* in uV, convert to mV */
>> + *val = data->mon_rng[chan->channel];
>> + *val2 = ADC_MASK * 1000;
>> + return IIO_VAL_FRACTIONAL;
>> + }
>> +
>> + break;
>> + }
>> + return -EINVAL;
>> +}
>> [..]
>> +static int max597x_iio_probe(struct platform_device *pdev)
>> +{
>> + struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
>> + struct i2c_client *i2c = to_i2c_client(pdev->dev.parent);
>> + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> + struct iio_dev *indio_dev;
>> + struct max597x_iio *priv;
>> + int ret, i;
>> +
>> + if (!regmap)
>> + return -EPROBE_DEFER;
>> +
>> + if (!max597x || !max597x->num_switches)
>> + return -EPROBE_DEFER;
>> +
>> + /* registering iio */
>> + indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*priv));
> For the devm allocations we should be using &pdev->dev and not the I2C
> device, since this is the device to which the allocations belong and
> where they should be freed when the device is removed.
Sure. Will use &pdev->dev
>> + if (!indio_dev) {
>> + dev_err(&i2c->dev, "failed allocating iio device\n");
> Consider using dev_err_probe() for error message printing. This will
> give a consistent formatting of the messages. Also again use &pdev->dev
> instead of I2C device to get the right device listed in the error messages.
Sure. Will use
dev_err_probe(&pdev->dev, ret, "could not register iio device");
>> + return -ENOMEM;
>> + }
>> + indio_dev->name = dev_name(&i2c->dev);
> The IIO ABI wants the type of the chip for the name. E.g. "max5970",
> using dev_name() of the parent I2C device will result in something else.
Sure. Will make it:
indio_dev->name = dev_name(&pdev->dev);
>> [...]
Regards,
Naresh
On 3/23/23 05:01, Naresh Solanki wrote:
> Hi,
>
> On 22-03-2023 09:28 pm, Lars-Peter Clausen wrote:
>> Hi,
>>
>> This looks really good. A few minor comments inline.
>>
>> On 3/22/23 05:43, Naresh Solanki wrote:
>>> [...]
>>> +static int max597x_iio_read_raw(struct iio_dev *iio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val, int *val2, long info)
>>> +{
>>> + int ret;
>>> + struct max597x_iio *data = iio_priv(iio_dev);
>>> + unsigned int reg_l, reg_h;
>>> +
>>> + switch (info) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + ret = regmap_read(data->regmap, chan->address, ®_l);
>>> + if (ret < 0)
>>> + return ret;
>>> + ret = regmap_read(data->regmap, chan->address - 1, ®_h);
>>> + if (ret < 0)
>>> + return ret;
>> Is there any chance of a race condition of getting inconsistent data
>> when splitting this over two reads? I.e. registers being updated with
>> new values in between the two reads.
> yes, reg_l holds lower 2 bits. due to latency in reads, value may differ.
>>> + *val = (reg_h << 2) | (reg_l & 3);
>>> +
>>> + return IIO_VAL_INT;
>>> + case IIO_CHAN_INFO_SCALE:
>>> +
>>> + switch (chan->address) {
>>> + case MAX5970_REG_CURRENT_L(0):
>>> + fallthrough;
>>
>> `fallthrough` should not be needed for multiple case statements right
>> on top of each other with no code in between. Same below
> Sure.
>>
>>> + case MAX5970_REG_CURRENT_L(1):
>>> + /* in A, convert to mA */
>>> + *val = data->irng[chan->channel] * 1000;
>>> + *val2 =
>>> + data->shunt_micro_ohms[chan->channel] * ADC_MASK;
>> ADC_MASK should really have a MAX5970_ prefix, but I guess it is
>> defined in max597x.h
> Yes its taken from max597x.h
>>> + return IIO_VAL_FRACTIONAL;
>>> +
>>> + case MAX5970_REG_VOLTAGE_L(0):
>>> + fallthrough;
>>> + case MAX5970_REG_VOLTAGE_L(1):
>>> + /* in uV, convert to mV */
>>> + *val = data->mon_rng[chan->channel];
>>> + *val2 = ADC_MASK * 1000;
>>> + return IIO_VAL_FRACTIONAL;
>>> + }
>>> +
>>> + break;
>>> + }
>>> + return -EINVAL;
>>> +}
>>> [..]
>>> +static int max597x_iio_probe(struct platform_device *pdev)
>>> +{
>>> + struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
>>> + struct i2c_client *i2c = to_i2c_client(pdev->dev.parent);
>>> + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
>>> + struct iio_dev *indio_dev;
>>> + struct max597x_iio *priv;
>>> + int ret, i;
>>> +
>>> + if (!regmap)
>>> + return -EPROBE_DEFER;
>>> +
>>> + if (!max597x || !max597x->num_switches)
>>> + return -EPROBE_DEFER;
>>> +
>>> + /* registering iio */
>>> + indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*priv));
>> For the devm allocations we should be using &pdev->dev and not the
>> I2C device, since this is the device to which the allocations belong
>> and where they should be freed when the device is removed.
> Sure. Will use &pdev->dev
>>> + if (!indio_dev) {
>>> + dev_err(&i2c->dev, "failed allocating iio device\n");
>> Consider using dev_err_probe() for error message printing. This will
>> give a consistent formatting of the messages. Also again use
>> &pdev->dev instead of I2C device to get the right device listed in
>> the error messages.
> Sure. Will use
> dev_err_probe(&pdev->dev, ret, "could not register iio device");
>>> + return -ENOMEM;
>>> + }
>>> + indio_dev->name = dev_name(&i2c->dev);
>> The IIO ABI wants the type of the chip for the name. E.g. "max5970",
>> using dev_name() of the parent I2C device will result in something else.
> Sure. Will make it:
> indio_dev->name = dev_name(&pdev->dev);
>
dev_name() in general should not be used for indio_dev->name, it does
not meet the ABI requirements for the IIO ABI. Move this into the switch
block below and then assign "max5970" or "max5978" depending on the
device type.
Hi
On 23-03-2023 06:37 pm, Lars-Peter Clausen wrote:
> On 3/23/23 05:01, Naresh Solanki wrote:
>> Hi,
>>
>> On 22-03-2023 09:28 pm, Lars-Peter Clausen wrote:
>>> Hi,
>>>
>>> This looks really good. A few minor comments inline.
>>>
>>> On 3/22/23 05:43, Naresh Solanki wrote:
>>>> [...]
>>>> +static int max597x_iio_read_raw(struct iio_dev *iio_dev,
>>>> + struct iio_chan_spec const *chan,
>>>> + int *val, int *val2, long info)
>>>> +{
>>>> + int ret;
>>>> + struct max597x_iio *data = iio_priv(iio_dev);
>>>> + unsigned int reg_l, reg_h;
>>>> +
>>>> + switch (info) {
>>>> + case IIO_CHAN_INFO_RAW:
>>>> + ret = regmap_read(data->regmap, chan->address, ®_l);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + ret = regmap_read(data->regmap, chan->address - 1, ®_h);
>>>> + if (ret < 0)
>>>> + return ret;
>>> Is there any chance of a race condition of getting inconsistent data
>>> when splitting this over two reads? I.e. registers being updated with
>>> new values in between the two reads.
>> yes, reg_l holds lower 2 bits. due to latency in reads, value may differ.
>>>> + *val = (reg_h << 2) | (reg_l & 3);
>>>> +
>>>> + return IIO_VAL_INT;
>>>> + case IIO_CHAN_INFO_SCALE:
>>>> +
>>>> + switch (chan->address) {
>>>> + case MAX5970_REG_CURRENT_L(0):
>>>> + fallthrough;
>>>
>>> `fallthrough` should not be needed for multiple case statements right
>>> on top of each other with no code in between. Same below
>> Sure.
>>>
>>>> + case MAX5970_REG_CURRENT_L(1):
>>>> + /* in A, convert to mA */
>>>> + *val = data->irng[chan->channel] * 1000;
>>>> + *val2 =
>>>> + data->shunt_micro_ohms[chan->channel] * ADC_MASK;
>>> ADC_MASK should really have a MAX5970_ prefix, but I guess it is
>>> defined in max597x.h
>> Yes its taken from max597x.h
>>>> + return IIO_VAL_FRACTIONAL;
>>>> +
>>>> + case MAX5970_REG_VOLTAGE_L(0):
>>>> + fallthrough;
>>>> + case MAX5970_REG_VOLTAGE_L(1):
>>>> + /* in uV, convert to mV */
>>>> + *val = data->mon_rng[chan->channel];
>>>> + *val2 = ADC_MASK * 1000;
>>>> + return IIO_VAL_FRACTIONAL;
>>>> + }
>>>> +
>>>> + break;
>>>> + }
>>>> + return -EINVAL;
>>>> +}
>>>> [..]
>>>> +static int max597x_iio_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
>>>> + struct i2c_client *i2c = to_i2c_client(pdev->dev.parent);
>>>> + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
>>>> + struct iio_dev *indio_dev;
>>>> + struct max597x_iio *priv;
>>>> + int ret, i;
>>>> +
>>>> + if (!regmap)
>>>> + return -EPROBE_DEFER;
>>>> +
>>>> + if (!max597x || !max597x->num_switches)
>>>> + return -EPROBE_DEFER;
>>>> +
>>>> + /* registering iio */
>>>> + indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*priv));
>>> For the devm allocations we should be using &pdev->dev and not the
>>> I2C device, since this is the device to which the allocations belong
>>> and where they should be freed when the device is removed.
>> Sure. Will use &pdev->dev
>>>> + if (!indio_dev) {
>>>> + dev_err(&i2c->dev, "failed allocating iio device\n");
>>> Consider using dev_err_probe() for error message printing. This will
>>> give a consistent formatting of the messages. Also again use
>>> &pdev->dev instead of I2C device to get the right device listed in
>>> the error messages.
>> Sure. Will use
>> dev_err_probe(&pdev->dev, ret, "could not register iio device");
>>>> + return -ENOMEM;
>>>> + }
>>>> + indio_dev->name = dev_name(&i2c->dev);
>>> The IIO ABI wants the type of the chip for the name. E.g. "max5970",
>>> using dev_name() of the parent I2C device will result in something else.
>> Sure. Will make it:
>> indio_dev->name = dev_name(&pdev->dev);
>>
> dev_name() in general should not be used for indio_dev->name, it does
> not meet the ABI requirements for the IIO ABI. Move this into the switch
> block below and then assign "max5970" or "max5978" depending on the
> device type.
Sure.
>
>
Thanks,
Naresh
On Thu, 23 Mar 2023 17:31:18 +0530
Naresh Solanki <[email protected]> wrote:
> Hi,
>
> On 22-03-2023 09:28 pm, Lars-Peter Clausen wrote:
> > Hi,
> >
> > This looks really good. A few minor comments inline.
> >
> > On 3/22/23 05:43, Naresh Solanki wrote:
> >> [...]
> >> +static int max597x_iio_read_raw(struct iio_dev *iio_dev,
> >> + struct iio_chan_spec const *chan,
> >> + int *val, int *val2, long info)
> >> +{
> >> + int ret;
> >> + struct max597x_iio *data = iio_priv(iio_dev);
> >> + unsigned int reg_l, reg_h;
> >> +
> >> + switch (info) {
> >> + case IIO_CHAN_INFO_RAW:
> >> + ret = regmap_read(data->regmap, chan->address, ®_l);
> >> + if (ret < 0)
> >> + return ret;
> >> + ret = regmap_read(data->regmap, chan->address - 1, ®_h);
> >> + if (ret < 0)
> >> + return ret;
> > Is there any chance of a race condition of getting inconsistent data
> > when splitting this over two reads? I.e. registers being updated with
> > new values in between the two reads.
> yes, reg_l holds lower 2 bits. due to latency in reads, value may differ.
Normally there is a way to avoid the tearing via a bulk read of some type.
Is that not possible here? If not, there are various tricks such as
repeated reads until stable that can be used.
Looks like the device has a block read format that might work.