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]>
...
Changes in V2:
- Remove fallthrough
- Use pdev->dev instead of i2c->dev
- Init indio_dev->name based on device type.
---
drivers/iio/adc/Kconfig | 15 ++++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/max597x-iio.c | 152 ++++++++++++++++++++++++++++++++++
3 files changed, 168 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..8a9fc27ff71e
--- /dev/null
+++ b/drivers/iio/adc/max597x-iio.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device driver for IIO in MAX5970 and MAX5978 IC
+ *
+ * Copyright (c) 2022 9elements GmbH
+ *
+ * Author: Patrick Rudolph <[email protected]>
+ */
+
+#include <linux/iio/iio.h>
+#include <linux/mfd/max597x.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.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):
+ 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):
+ 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 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(&pdev->dev, sizeof(*priv));
+ if (!indio_dev)
+ return dev_err_probe(&pdev->dev, -ENOMEM,
+ "failed to allocate iio device\n");
+
+ 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);
+ indio_dev->name = "max5970";
+ break;
+ case MAX597x_TYPE_MAX5978:
+ indio_dev->channels = max5978_adc_iio_channels;
+ indio_dev->num_channels = ARRAY_SIZE(max5978_adc_iio_channels);
+ indio_dev->name = "max5978";
+ 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(&pdev->dev, indio_dev);
+ if (ret)
+ dev_err_probe(&pdev->dev, ret, "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 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 <[email protected]>
Signed-off-by: Naresh Solanki <[email protected]>
...
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..ec2b731ae545 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..3e1747c8693e
--- /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 <[email protected]>
+ */
+
+#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;
+
+ if (!led || !led->regmap)
+ return -ENODEV;
+
+ val = !brightness ? BIT(led->index) : 0;
+ 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);
+ 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 = 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);
+
+ return ret;
+}
+
+static int max597x_led_probe(struct platform_device *pdev)
+{
+ struct device_node *np = dev_of_node(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(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
+ MAX597X_NUM_LEDS);
+ continue;
+ }
+
+ ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
+ if (ret < 0)
+ of_node_put(child);
+ }
+
+ 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
Le 23/03/2023 à 20:45, Naresh Solanki a écrit :
> 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]>
Hi, a few nits below, should there be a v3.
CJ
> ...
> Changes in V2:
> - Remove fallthrough
> - Use pdev->dev instead of i2c->dev
> - Init indio_dev->name based on device type.
> ---
> drivers/iio/adc/Kconfig | 15 ++++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/max597x-iio.c | 152 ++++++++++++++++++++++++++++++++++
> 3 files changed, 168 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..8a9fc27ff71e
> --- /dev/null
> +++ b/drivers/iio/adc/max597x-iio.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device driver for IIO in MAX5970 and MAX5978 IC
> + *
> + * Copyright (c) 2022 9elements GmbH
> + *
> + * Author: Patrick Rudolph <[email protected]>
> + */
> +
> +#include <linux/iio/iio.h>
> +#include <linux/mfd/max597x.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.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:
> +
Nit: This blank line would look nicer if above the case:
> + switch (chan->address) {
> + case MAX5970_REG_CURRENT_L(0):
> + 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):
> + 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 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(&pdev->dev, sizeof(*priv));
> + if (!indio_dev)
> + return dev_err_probe(&pdev->dev, -ENOMEM,
> + "failed to allocate iio device\n");
> +
> + 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);
> + indio_dev->name = "max5970";
> + break;
> + case MAX597x_TYPE_MAX5978:
> + indio_dev->channels = max5978_adc_iio_channels;
> + indio_dev->num_channels = ARRAY_SIZE(max5978_adc_iio_channels);
> + indio_dev->name = "max5978";
> + 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(&pdev->dev, indio_dev);
> + if (ret)
> + dev_err_probe(&pdev->dev, ret, "could not register iio device");
Nit: \n missing
> +
> + return ret;
Nit: return 0;
> +}
> +
> +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
Le 23/03/2023 à 20:45, Naresh Solanki a écrit :
> From: Patrick Rudolph <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Naresh Solanki <[email protected]>
> ...
> 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..ec2b731ae545 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.
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..3e1747c8693e
> --- /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 <[email protected]>
> + */
> +
> +#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;
> +
> + if (!led || !led->regmap)
> + return -ENODEV;
> +
> + val = !brightness ? BIT(led->index) : 0;
> + 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);
> + 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 = 0;
Nit: useless "= 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);
devm_led_classdev_register?
> + if (ret)
> + dev_err(dev, "Error in initializing led %s", led->led.name);
> +
> + return ret;
> +}
> +
> +static int max597x_led_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = dev_of_node(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(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
> + MAX597X_NUM_LEDS);
> + continue;
> + }
> +
> + ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
> + if (ret < 0)
> + of_node_put(child);
This of_node_put() looks odd to me.
"return ret;" or "break;" missing ?
> + }
> +
> + 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");
Hi,
On 24-03-2023 01:36 am, Christophe JAILLET wrote:
> Le 23/03/2023 à 20:45, Naresh Solanki a écrit :
>> 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]>
>
> Hi, a few nits below, should there be a v3.
>
> CJ
>
>> ...
>> Changes in V2:
>> - Remove fallthrough
>> - Use pdev->dev instead of i2c->dev
>> - Init indio_dev->name based on device type.
>> ---
>> drivers/iio/adc/Kconfig | 15 ++++
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/max597x-iio.c | 152 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 168 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..8a9fc27ff71e
>> --- /dev/null
>> +++ b/drivers/iio/adc/max597x-iio.c
>> @@ -0,0 +1,152 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device driver for IIO in MAX5970 and MAX5978 IC
>> + *
>> + * Copyright (c) 2022 9elements GmbH
>> + *
>> + * Author: Patrick Rudolph
>> <[email protected]>
>> + */
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/mfd/max597x.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.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:
>> +
>
> Nit: This blank line would look nicer if above the case:
Oh yes. Will do that in V3
>
>> + switch (chan->address) {
>> + case MAX5970_REG_CURRENT_L(0):
>> + 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):
>> + 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 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(&pdev->dev, sizeof(*priv));
>> + if (!indio_dev)
>> + return dev_err_probe(&pdev->dev, -ENOMEM,
>> + "failed to allocate iio device\n");
>> +
>> + 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);
>> + indio_dev->name = "max5970";
>> + break;
>> + case MAX597x_TYPE_MAX5978:
>> + indio_dev->channels = max5978_adc_iio_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(max5978_adc_iio_channels);
>> + indio_dev->name = "max5978";
>> + 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(&pdev->dev, indio_dev);
>> + if (ret)
>> + dev_err_probe(&pdev->dev, ret, "could not register iio device");
>
> Nit: \n missing
>
Sure will make it like this:
if (ret)
return dev_err_probe(&pdev->dev, ret, "could not register iio device\n");
>> +
>> + return ret;
>
> Nit: return 0;
Sure
>
>> +}
>> +
>> +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
>
Hi,
On 24-03-2023 01:48 am, Christophe JAILLET wrote:
> Le 23/03/2023 à 20:45, Naresh Solanki a écrit :
>> From: Patrick Rudolph <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Naresh Solanki <[email protected]>
>> ...
>> 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..ec2b731ae545 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.
>
> leds-max597x?
As per struct max597x_led_driver, driver name is 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..3e1747c8693e
>> --- /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 <[email protected]>
>> + */
>> +
>> +#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;
>> +
>> + if (!led || !led->regmap)
>> + return -ENODEV;
>> +
>> + val = !brightness ? BIT(led->index) : 0;
>> + 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);
>> + 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 = 0;
>
> Nit: useless "= 0"
Ack. Will be removing ' = 0' in V3
>
>> +
>> + 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);
>
> devm_led_classdev_register?
Ack. Will update in V3
>
>> + if (ret)
>> + dev_err(dev, "Error in initializing led %s", led->led.name);
>> +
>> + return ret;
>> +}
>> +
>> +static int max597x_led_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = dev_of_node(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(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
>> + MAX597X_NUM_LEDS);
>> + continue;
>> + }
>> +
>> + ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
>> + if (ret < 0)
>> + of_node_put(child);
>
> This of_node_put() looks odd to me.
Not sure if I get this right but if led setup fails of_node_put should
be called.
> "return ret;" or "break;" missing ?
>
Didn't add a break so that it can continue initializing remaining led if
any.
>> + }
>> +
>> + 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");
>
Will push V3 based on feedback.
Regards,
Naresh
Hi!
> > > +++ 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.
> >
> > leds-max597x?
> As per struct max597x_led_driver, driver name is max597x-led
Please test the modular build and double check the module name.
BR,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
Hi Christophe, Pavel,
On 24-03-2023 06:37 pm, Pavel Machek wrote:
> Hi!
>
>>>> +++ 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.
>>>
>>> leds-max597x?
>> As per struct max597x_led_driver, driver name is max597x-led
>
> Please test the modular build and double check the module name.
Thank you for your suggestion, it turns out that you were right. The
correct module name is leds-max597x(./drivers/leds/leds-max597x.ko).
Will update Kconfig message accordingly. Thanks!
>
> BR,
> Pavel
>
Regards,
Naresh
Le 24/03/2023 à 11:54, Naresh Solanki a écrit :
> Hi,
>
> On 24-03-2023 01:48 am, Christophe JAILLET wrote:
>> Le 23/03/2023 à 20:45, Naresh Solanki a écrit :
>>> From: Patrick Rudolph <[email protected]>
>>>
>>> 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.
>>>
[...]
>>> +static int max597x_led_probe(struct platform_device *pdev)
>>> +{
>>> + struct device_node *np = dev_of_node(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(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
>>> + MAX597X_NUM_LEDS);
>>> + continue;
>>> + }
>>> +
>>> + ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
>>> + if (ret < 0)
>>> + of_node_put(child);
>>
>> This of_node_put() looks odd to me.
> Not sure if I get this right but if led setup fails of_node_put should
> be called.
My understanding is that this of_node_put() is there in case of error,
to release what would otherwise never be released by
for_each_available_child_of_node() if we exit early from the loop.
If the purpose is to release a reference taken in max597x_setup_led() in
case of error:
- this should be done IMHO within max597x_setup_led() directly
- there should be a of_node_get() somewhere in max597x_setup_led()
(after:
if (of_property_read_string(nc, "label", &led->led.name))
led->led.name = nc->name;
+ error handling path, *or*
just before the final return ret; when we know that everything is
fine,
if I understand correctly the code)
Is the reference taken elsewhere?
Did I miss something obvious?
>> "return ret;" or "break;" missing ?
>>
> Didn't add a break so that it can continue initializing remaining led if
> any.
Ok. Don't know the code enough to see if correct or not, but based on my
comment above, I think that something is missing in max597x_setup_led()
and that errors should be silently ignored here.
CJ
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
[...]
On Thu, 23 Mar 2023 20:45:48 +0100
Naresh Solanki <[email protected]> wrote:
> 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]>
I'm not a fan of wild cards in driver names. This doesn't
for example support the max5974, max5971 etc
Much better to name it after one of the supported parts.
Obviously can't do much about the mfd driver now, but I'd prefer
not to carry that through to the IIO driver if possible.
One concern I have here is that from the max5978 datasheet I see
this device supports features that are very much directed at hwmon
type usecases. In particular warning and critical threshold detection.
We don't support multiple thresholds (in same direction) for a single
channel via IIO. If you want those features in the future you may want
to consider using the hwmon subsystem.
We tend to be flexible with devices that sit near the boundary of IIO
and hwmon because we can bridge many of the features using the iio-hwmon
bridge driver. That doesn't work for more complex event handling and
I suspect some of the other features this device provides.
> ...
> Changes in V2:
> - Remove fallthrough
> - Use pdev->dev instead of i2c->dev
> - Init indio_dev->name based on device type.
> ---
> drivers/iio/adc/Kconfig | 15 ++++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/max597x-iio.c | 152 ++++++++++++++++++++++++++++++++++
> 3 files changed, 168 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.
Call out the actual part numbers supported in this help text to make it easy
to grep for them.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called max597x-iio.
> +
...
> +
> +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);
I replied late to previous patch, but I'd prefer to see a bulk read if
possible. It might ensure a matched pair, or if not reduce the chance of
tearing (when reg_l & 3 transitions from 3 to 0 for example and
reg_h & 1 is going from 0 to 1)
You could try a repeated read if the sampling rate is fairly low as
simply getting same high bits on either side of the low bit read is probably
enough to say tearing didn't happen.
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> +
> + switch (chan->address) {
> + case MAX5970_REG_CURRENT_L(0):
> + 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;
Don't worry about 80 char limit when it hurts readability. Just put that
on one line.
> + return IIO_VAL_FRACTIONAL;
> +
> + case MAX5970_REG_VOLTAGE_L(0):
> + 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 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 */
Comment doesn't add anything is is wrong anyway as this doesn't do the
majority of the registration. Dropt he comment.
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
> + if (!indio_dev)
> + return dev_err_probe(&pdev->dev, -ENOMEM,
> + "failed to allocate iio device\n");
...
Hi,
On 24-03-2023 09:06 pm, Christophe JAILLET wrote:
> Le 24/03/2023 à 11:54, Naresh Solanki a écrit :
>> Hi,
>>
>> On 24-03-2023 01:48 am, Christophe JAILLET wrote:
>>> Le 23/03/2023 à 20:45, Naresh Solanki a écrit :
>>>> From: Patrick Rudolph <[email protected]>
>>>>
>>>> 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.
>>>>
>
> [...]
>
>
>>>> +static int max597x_led_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct device_node *np = dev_of_node(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(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
>>>> + MAX597X_NUM_LEDS);
>>>> + continue;
>>>> + }
>>>> +
>>>> + ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
>>>> + if (ret < 0)
>>>> + of_node_put(child);
>>>
>>> This of_node_put() looks odd to me.
>> Not sure if I get this right but if led setup fails of_node_put should
>> be called.
>
> My understanding is that this of_node_put() is there in case of error,
> to release what would otherwise never be released by
> for_each_available_child_of_node() if we exit early from the loop.
>
> If the purpose is to release a reference taken in max597x_setup_led() in
> case of error:
> - this should be done IMHO within max597x_setup_led() directly
> - there should be a of_node_get() somewhere in max597x_setup_led()
> (after:
> if (of_property_read_string(nc, "label", &led->led.name))
> led->led.name = nc->name;
> + error handling path, *or*
> just before the final return ret; when we know that everything is
> fine,
> if I understand correctly the code)
>
> Is the reference taken elsewhere?
> Did I miss something obvious?
>
>
One of the reference is "drivers/leds/leds-sc27xx-bltc.c" line 311
Please do let me know if I have to do anything about it.
>>> "return ret;" or "break;" missing ?
>>>
>> Didn't add a break so that it can continue initializing remaining led
>> if any.
>
> Ok. Don't know the code enough to see if correct or not, but based on my
> comment above, I think that something is missing in max597x_setup_led()
> and that errors should be silently ignored here.
In my implementation, I have chosen to continue with the next LED if an
error occurs, rather than aborting the 'for loop' with an error. I have
seen other implementations also done in a similar way.
Do you have any further inputs or suggestions on this approach.
>
> CJ
>
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>
> [...]
>
Regards,
Naresh
Le 27/03/2023 à 17:47, Naresh Solanki a écrit :
> Hi,
>
> On 24-03-2023 09:06 pm, Christophe JAILLET wrote:
>> Le 24/03/2023 à 11:54, Naresh Solanki a écrit :
>>> Hi,
>>>
>>> On 24-03-2023 01:48 am, Christophe JAILLET wrote:
>>>> Le 23/03/2023 à 20:45, Naresh Solanki a écrit :
>>>>> From: Patrick Rudolph <[email protected]>
>>>>>
>>>>> 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.
>>>>>
>>
>> [...]
>>
>>
>>>>> +static int max597x_led_probe(struct platform_device *pdev)
>>>>> +{
>>>>> + struct device_node *np = dev_of_node(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(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
>>>>> + MAX597X_NUM_LEDS);
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>> + ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
>>>>> + if (ret < 0)
>>>>> + of_node_put(child);
>>>>
>>>> This of_node_put() looks odd to me.
>>> Not sure if I get this right but if led setup fails of_node_put
>>> should be called.
>>
>> My understanding is that this of_node_put() is there in case of error,
>> to release what would otherwise never be released by
>> for_each_available_child_of_node() if we exit early from the loop.
>>
>> If the purpose is to release a reference taken in max597x_setup_led()
>> in case of error:
>> - this should be done IMHO within max597x_setup_led() directly
>> - there should be a of_node_get() somewhere in max597x_setup_led()
>> (after:
>> if (of_property_read_string(nc, "label", &led->led.name))
>> led->led.name = nc->name;
>> + error handling path, *or*
>> just before the final return ret; when we know that everything
>> is fine,
>> if I understand correctly the code)
>>
>> Is the reference taken elsewhere?
>> Did I miss something obvious?
>>
>>
> One of the reference is "drivers/leds/leds-sc27xx-bltc.c" line 311
> Please do let me know if I have to do anything about it.
By reference, I was speaking of reference taken by a of_node_get() call
and released by a of_node_put() call.
Anyway, I do agree with leds-sc27xx-bltc.c.
There is a of_node_put() because for_each_available_child_of_node()
won't be able to do it by itself *in case of early return* ("return err;")
In all other paths (when the loop goes to the end), the reference taken
by for_each_available_child_of_node() is also released, on the next
iteration, by for_each_available_child_of_node().
In *your* case, if you don't break or return, there is no need to call
of_node_put() explicitly. It would lead to a double put. (yours and the
one that will be done by for_each_available_child_of_node()).
Have a look at for_each_available_child_of_node() and more specifically
at of_get_next_available_child().
At the first call 'child' is NULL. A ref is taken [1]. Nothing is released.
For following calls, a new ref is taken on a new node [1], and the
previous reference is released [2].
On the last call, the 'for' loop will not be executed because there is
nothing to scan anymore. No new reference is taken, and the previous
(and last) refence is finally released [2].
[1]: https://elixir.bootlin.com/linux/v6.3-rc3/source/drivers/of/base.c#L808
[2]: https://elixir.bootlin.com/linux/v6.3-rc3/source/drivers/of/base.c#L811
>
>>>> "return ret;" or "break;" missing ?
>>>>
>>> Didn't add a break so that it can continue initializing remaining led
>>> if any.
>>
>> Ok. Don't know the code enough to see if correct or not, but based on
>> my comment above, I think that something is missing in
>> max597x_setup_led() and that errors should be silently ignored here.
> In my implementation, I have chosen to continue with the next LED if an
> error occurs, rather than aborting the 'for loop' with an error. I have
> seen other implementations also done in a similar way.
> Do you have any further inputs or suggestions on this approach.
No, sorry, I won't be of any help on what design is the best.
CJ
Hi,
On 27-03-2023 10:50 pm, Christophe JAILLET wrote:
> Le 27/03/2023 à 17:47, Naresh Solanki a écrit :
>> Hi,
>>
>> On 24-03-2023 09:06 pm, Christophe JAILLET wrote:
>>> Le 24/03/2023 à 11:54, Naresh Solanki a écrit :
>>>> Hi,
>>>>
>>>> On 24-03-2023 01:48 am, Christophe JAILLET wrote:
>>>>> Le 23/03/2023 à 20:45, Naresh Solanki a écrit :
>>>>>> From: Patrick Rudolph <[email protected]>
>>>>>>
>>>>>> 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.
>>>>>>
>>>
>>> [...]
>>>
>>>
>>>>>> +static int max597x_led_probe(struct platform_device *pdev)
>>>>>> +{
>>>>>> + struct device_node *np = dev_of_node(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(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
>>>>>> + MAX597X_NUM_LEDS);
>>>>>> + continue;
>>>>>> + }
>>>>>> +
>>>>>> + ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
>>>>>> + if (ret < 0)
>>>>>> + of_node_put(child);
>>>>>
>>>>> This of_node_put() looks odd to me.
>>>> Not sure if I get this right but if led setup fails of_node_put
>>>> should be called.
>>>
>>> My understanding is that this of_node_put() is there in case of
>>> error, to release what would otherwise never be released by
>>> for_each_available_child_of_node() if we exit early from the loop.
>>>
>>> If the purpose is to release a reference taken in max597x_setup_led()
>>> in case of error:
>>> - this should be done IMHO within max597x_setup_led() directly
>>> - there should be a of_node_get() somewhere in max597x_setup_led()
>>> (after:
>>> if (of_property_read_string(nc, "label", &led->led.name))
>>> led->led.name = nc->name;
>>> + error handling path, *or*
>>> just before the final return ret; when we know that everything
>>> is fine,
>>> if I understand correctly the code)
>>>
>>> Is the reference taken elsewhere?
>>> Did I miss something obvious?
>>>
>>>
>> One of the reference is "drivers/leds/leds-sc27xx-bltc.c" line 311
>> Please do let me know if I have to do anything about it.
> By reference, I was speaking of reference taken by a of_node_get() call
> and released by a of_node_put() call.
>
> Anyway, I do agree with leds-sc27xx-bltc.c.
> There is a of_node_put() because for_each_available_child_of_node()
> won't be able to do it by itself *in case of early return* ("return err;")
>
> In all other paths (when the loop goes to the end), the reference taken
> by for_each_available_child_of_node() is also released, on the next
> iteration, by for_each_available_child_of_node().
>
> In *your* case, if you don't break or return, there is no need to call
> of_node_put() explicitly. It would lead to a double put. (yours and the
> one that will be done by for_each_available_child_of_node()).
>
> Have a look at for_each_available_child_of_node() and more specifically
> at of_get_next_available_child().
>
> At the first call 'child' is NULL. A ref is taken [1]. Nothing is released.
> For following calls, a new ref is taken on a new node [1], and the
> previous reference is released [2].
> On the last call, the 'for' loop will not be executed because there is
> nothing to scan anymore. No new reference is taken, and the previous
> (and last) refence is finally released [2].
Yes you are right. That of_node_put would be duplicate as it is already
taken care by the for loop.
Will remove that in next revision.
>
>
> [1]:
> https://elixir.bootlin.com/linux/v6.3-rc3/source/drivers/of/base.c#L808
> [2]:
> https://elixir.bootlin.com/linux/v6.3-rc3/source/drivers/of/base.c#L811
>
>>
>>>>> "return ret;" or "break;" missing ?
>>>>>
>>>> Didn't add a break so that it can continue initializing remaining
>>>> led if any.
>>>
>>> Ok. Don't know the code enough to see if correct or not, but based on
>>> my comment above, I think that something is missing in
>>> max597x_setup_led() and that errors should be silently ignored here.
>> In my implementation, I have chosen to continue with the next LED if
>> an error occurs, rather than aborting the 'for loop' with an error. I
>> have seen other implementations also done in a similar way.
>> Do you have any further inputs or suggestions on this approach.
>
> No, sorry, I won't be of any help on what design is the best.
>
> CJ
>
Regards,
Naresh
Hi,
On 26-03-2023 01:06 am, Jonathan Cameron wrote:
> On Thu, 23 Mar 2023 20:45:48 +0100
> Naresh Solanki <[email protected]> wrote:
>
>> 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]>
>
> I'm not a fan of wild cards in driver names. This doesn't
> for example support the max5974, max5971 etc
>
> Much better to name it after one of the supported parts.
> Obviously can't do much about the mfd driver now, but I'd prefer
> not to carry that through to the IIO driver if possible.
>
> One concern I have here is that from the max5978 datasheet I see
> this device supports features that are very much directed at hwmon
> type usecases. In particular warning and critical threshold detection.
> We don't support multiple thresholds (in same direction) for a single
> channel via IIO. If you want those features in the future you may want
> to consider using the hwmon subsystem.
>
> We tend to be flexible with devices that sit near the boundary of IIO
> and hwmon because we can bridge many of the features using the iio-hwmon
> bridge driver. That doesn't work for more complex event handling and
> I suspect some of the other features this device provides.
I believe it is the most appropriate approach for our use case at the
moment. If we decide to incorporate more complex event handling or need
to support multiple thresholds in the future, we will definitely
consider using the hwmon subsystem. Thank for your input.
>
>> ...
>> Changes in V2:
>> - Remove fallthrough
>> - Use pdev->dev instead of i2c->dev
>> - Init indio_dev->name based on device type.
>> ---
>> drivers/iio/adc/Kconfig | 15 ++++
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/max597x-iio.c | 152 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 168 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.
>
> Call out the actual part numbers supported in this help text to make it easy
> to grep for them.
Sure. Will mention max5970 & max5978 in help section.
>
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called max597x-iio.
>> +
>
> ...
>
>
>> +
>> +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);
>
> I replied late to previous patch, but I'd prefer to see a bulk read if
> possible. It might ensure a matched pair, or if not reduce the chance of
> tearing (when reg_l & 3 transitions from 3 to 0 for example and
> reg_h & 1 is going from 0 to 1)
>
> You could try a repeated read if the sampling rate is fairly low as
> simply getting same high bits on either side of the low bit read is probably
> enough to say tearing didn't happen.
Yes. will use something like:
ret = regmap_bulk_read(data->regmap, chan->address - 1, ®_l, 2);
if (ret < 0)
return ret;
reg_h = reg_l & 0xff;
reg_l = (reg_l >> 8) & 0xff;
*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):
>> + 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;
> Don't worry about 80 char limit when it hurts readability. Just put that
> on one line.
Sure
>
>> + return IIO_VAL_FRACTIONAL;
>> +
>> + case MAX5970_REG_VOLTAGE_L(0):
>> + 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 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 */
>
> Comment doesn't add anything is is wrong anyway as this doesn't do the
> majority of the registration. Dropt he comment.
Sure.
>
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
>> + if (!indio_dev)
>> + return dev_err_probe(&pdev->dev, -ENOMEM,
>> + "failed to allocate iio device\n");
>
> ...
>
BR,
Naresh
On Tue, 28 Mar 2023 00:14:16 +0530
Naresh Solanki <[email protected]> wrote:
> Hi,
>
> On 26-03-2023 01:06 am, Jonathan Cameron wrote:
> > On Thu, 23 Mar 2023 20:45:48 +0100
> > Naresh Solanki <[email protected]> wrote:
> >
> >> 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]>
> >
> > I'm not a fan of wild cards in driver names. This doesn't
> > for example support the max5974, max5971 etc
> >
> > Much better to name it after one of the supported parts.
> > Obviously can't do much about the mfd driver now, but I'd prefer
> > not to carry that through to the IIO driver if possible.
> >
> > One concern I have here is that from the max5978 datasheet I see
> > this device supports features that are very much directed at hwmon
> > type usecases. In particular warning and critical threshold detection.
> > We don't support multiple thresholds (in same direction) for a single
> > channel via IIO. If you want those features in the future you may want
> > to consider using the hwmon subsystem.
> >
> > We tend to be flexible with devices that sit near the boundary of IIO
> > and hwmon because we can bridge many of the features using the iio-hwmon
> > bridge driver. That doesn't work for more complex event handling and
> > I suspect some of the other features this device provides.
> I believe it is the most appropriate approach for our use case at the
> moment. If we decide to incorporate more complex event handling or need
> to support multiple thresholds in the future, we will definitely
> consider using the hwmon subsystem. Thank for your input.
It's not easy to move a driver (because of need to maintain ABI compatibility
in most cases). Hence I'd suggest at least CCing the hwmon list and maintainers
on future versions with a cover letter than explains your reasoning on why
this particular support should use IIO.
> >
> >
> >> +
> >> +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);
> >
> > I replied late to previous patch, but I'd prefer to see a bulk read if
> > possible. It might ensure a matched pair, or if not reduce the chance of
> > tearing (when reg_l & 3 transitions from 3 to 0 for example and
> > reg_h & 1 is going from 0 to 1)
> >
> > You could try a repeated read if the sampling rate is fairly low as
> > simply getting same high bits on either side of the low bit read is probably
> > enough to say tearing didn't happen.
> Yes. will use something like:
> ret = regmap_bulk_read(data->regmap, chan->address - 1, ®_l, 2);
> if (ret < 0)
> return ret;
> reg_h = reg_l & 0xff;
> reg_l = (reg_l >> 8) & 0xff;
> *val = (reg_h << 2) | (reg_l & 3);
As you are going to handle them as separate registers (which makes sense under the
circumstances) read into a u8 regs[2] then express this as the following which also
deals with endian issues by make the registering ordering explicit.
*val = (reg[0] << 2) | (reg[1] & 3);
Thanks,
Jonathan