From: Patrick Rudolph <[email protected]>
max5970 & max5978 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 V3:
- Use bulk read
- Remove line split
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 | 148 ++++++++++++++++++++++++++++++++++
3 files changed, 164 insertions(+)
create mode 100644 drivers/iio/adc/max597x-iio.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 45af2302be53..69310af5c665 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 5970 & 5978 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..f158e49b5a56
--- /dev/null
+++ b/drivers/iio/adc/max597x-iio.c
@@ -0,0 +1,148 @@
+// 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);
+ u16 reg_l, reg_h;
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ 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;
+ 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;
+
+ 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)
+ return dev_err_probe(&pdev->dev, ret, "could not register iio device\n");
+
+ 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
--
2.39.1
On Tue, 28 Mar 2023 11:44:14 +0200
Naresh Solanki <[email protected]> wrote:
> From: Patrick Rudolph <[email protected]>
>
> max5970 & max5978 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]>
> ...
--- not ...
As I mentioned in my reply to v2 thread (which crossed with this v3)
I'd like this series to be cc'd to the list and maintainers for Hwmon
with a cover letter explaining the reasoning for it being an IIO driver
+ the restrictions that potentially brings.
A few other comments inline from taking another look.
Thanks,
Jonathan
> Changes in V3:
> - Use bulk read
> - Remove line split
> 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 | 148 ++++++++++++++++++++++++++++++++++
> 3 files changed, 164 insertions(+)
> create mode 100644 drivers/iio/adc/max597x-iio.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 45af2302be53..69310af5c665 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 5970 & 5978 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..f158e49b5a56
> --- /dev/null
> +++ b/drivers/iio/adc/max597x-iio.c
> @@ -0,0 +1,148 @@
> +// 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);
> + u16 reg_l, reg_h;
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + ret = regmap_bulk_read(data->regmap, chan->address - 1, ®_l, 2);
This crossed with my reply to the v2 thread. If reading two separate registers that
don't directly correspond to a large packed value, read them into an array
of u8. Then get the relevant parts from that.
The following use is not endian safe (it won't work on a big endian machine)
> + 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;
> + 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;
I'd prefer this pushed up as a default: in each of the two switch statements.
Makes it clear to compilers and readers that only listing some case values is
deliberate.
> +}
> +
> +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;
> +
> + 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) {
Having a value 'num_switches' that maps to a set of enums called _TYPE_ is unusual.
Perhaps rename it to type.
> + 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)
> + return dev_err_probe(&pdev->dev, ret, "could not register iio device\n");
> +
> + 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
Hi Jonathan,
On 02-04-2023 10:31 pm, Jonathan Cameron wrote:
> On Tue, 28 Mar 2023 11:44:14 +0200
> Naresh Solanki <[email protected]> wrote:
>
>> From: Patrick Rudolph <[email protected]>
>>
>> max5970 & max5978 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]>
>> ...
>
> --- not ...
>
> As I mentioned in my reply to v2 thread (which crossed with this v3)
> I'd like this series to be cc'd to the list and maintainers for Hwmon
> with a cover letter explaining the reasoning for it being an IIO driver
> + the restrictions that potentially brings.
Sure.
>
> A few other comments inline from taking another look.
>
> Thanks,
>
> Jonathan
>
>
>> Changes in V3:
>> - Use bulk read
>> - Remove line split
>> 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 | 148 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 164 insertions(+)
>> create mode 100644 drivers/iio/adc/max597x-iio.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 45af2302be53..69310af5c665 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 5970 & 5978 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..f158e49b5a56
>> --- /dev/null
>> +++ b/drivers/iio/adc/max597x-iio.c
>> @@ -0,0 +1,148 @@
>> +// 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);
>> + u16 reg_l, reg_h;
>> +
>> + switch (info) {
>> + case IIO_CHAN_INFO_RAW:
>> + ret = regmap_bulk_read(data->regmap, chan->address - 1, ®_l, 2);
>
> This crossed with my reply to the v2 thread. If reading two separate registers that
> don't directly correspond to a large packed value, read them into an array
> of u8. Then get the relevant parts from that.
Sure
>
> The following use is not endian safe (it won't work on a big endian machine)
Sure
>
>> + 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;
>> + 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;
>
> I'd prefer this pushed up as a default: in each of the two switch statements.
> Makes it clear to compilers and readers that only listing some case values is
> deliberate.
Sure
>
>> +}
>> +
>> +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;
>> +
>> + 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) {
>
> Having a value 'num_switches' that maps to a set of enums called _TYPE_ is unusual.
> Perhaps rename it to type.
Will add a local variable type to track the same within with driver.
>
>
>> + 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)
>> + return dev_err_probe(&pdev->dev, ret, "could not register iio device\n");
>> +
>> + 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
>
Regards,
Naresh
On Tue, 4 Apr 2023 15:37:21 +0530
Naresh Solanki <[email protected]> wrote:
> Hi Jonathan,
>
> On 02-04-2023 10:31 pm, Jonathan Cameron wrote:
> > On Tue, 28 Mar 2023 11:44:14 +0200
> > Naresh Solanki <[email protected]> wrote:
> >
> >> From: Patrick Rudolph <[email protected]>
> >>
> >> max5970 & max5978 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]>
> >> ...
> >
> > --- not ...
> >
> > As I mentioned in my reply to v2 thread (which crossed with this v3)
> > I'd like this series to be cc'd to the list and maintainers for Hwmon
> > with a cover letter explaining the reasoning for it being an IIO driver
> > + the restrictions that potentially brings.
> Sure.
A minor request from me for future replies to make things a little
more efficient. Note I'm not always great at this myself - this
is a case of do as I say rather than as I do!
Don't bother agreeing with stuff - reviewers assume you agree
unless you comment!
Crop out any sections of the email reply that have no new content.
e.g.
...
> >> +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;
> >> +
> >> + 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) {
> >
> > Having a value 'num_switches' that maps to a set of enums called _TYPE_ is unusual.
> > Perhaps rename it to type.
> Will add a local variable type to track the same within with driver.
Not totally sure what you mean. I'll look for it in newer versions.
Thanks,
Jonathan