[PATCH v2 5/5] drivers: iio: adc: Add MAX77541 ADC Support

[PATCH v2 5/5] drivers: iio: adc: Add MAX77541 ADC Support

[Okan Sahin]

The MAX77541 has an 8-bit Successive Approximation Register (SAR) ADC
with four multiplexers for supporting the telemetry feature

Signed-off-by: Okan Sahin <okan.sahin@analog.com>
---
 MAINTAINERS                    |   1 +
 drivers/iio/adc/Kconfig        |  11 ++
 drivers/iio/adc/Makefile       |   1 +
 drivers/iio/adc/max77541-adc.c | 199 +++++++++++++++++++++++++++++++++
 4 files changed, 212 insertions(+)
 create mode 100644 drivers/iio/adc/max77541-adc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8e5572b28a8c..18ce4644cc75 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12503,6 +12503,7 @@ L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/mfd/adi,max77541.yaml
 F:	Documentation/devicetree/bindings/regulator/adi,max77541.yaml
+F:	drivers/iio/adc/max77541-adc.c
 F:	drivers/mfd/max77541.c
 F:	drivers/regulator/max77541-regulator.c
 F:	include/linux/mfd/max77541.h
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 791612ca6012..9716225b50da 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -696,6 +696,17 @@ config MAX1363
 	  To compile this driver as a module, choose M here: the module will be
 	  called max1363.
 
+config MAX77541_ADC
+	tristate "Analog Devices MAX77541 ADC driver"
+	depends on MFD_MAX77541
+	help
+	  This driver controls a Analog Devices MAX77541 ADC
+	  via I2C bus. This device has one adc. Say yes here to build
+	  support for Analog Devices MAX77541 ADC interface.
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called max77541-adc.
+
 config MAX9611
 	tristate "Maxim max9611/max9612 ADC driver"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 46caba7a010c..03774cccbb4b 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_MAX1118) += max1118.o
 obj-$(CONFIG_MAX11205) += max11205.o
 obj-$(CONFIG_MAX1241) += max1241.o
 obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MAX77541_ADC) += max77541-adc.o
 obj-$(CONFIG_MAX9611) += max9611.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_MCP3422) += mcp3422.o
diff --git a/drivers/iio/adc/max77541-adc.c b/drivers/iio/adc/max77541-adc.c
new file mode 100644
index 000000000000..29adcdbd96ae
--- /dev/null
+++ b/drivers/iio/adc/max77541-adc.c
@@ -0,0 +1,199 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 Analog Devices, Inc.
+ * ADI MAX77541 ADC Driver with IIO interface
+ */
+
+#include <linux/bitfield.h>
+#include <linux/iio/iio.h>
+#include <linux/mfd/max77541.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/units.h>
+
+#define MAX77541_ADC_CHANNEL(_channel, _name, _type, _reg) \
+	{							\
+		.type = _type,					\
+		.indexed = 1,					\
+		.channel = _channel,				\
+		.address = _reg,				\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
+				      BIT(IIO_CHAN_INFO_SCALE) |\
+				      BIT(IIO_CHAN_INFO_OFFSET),\
+		.datasheet_name = _name,			\
+	}
+
+enum max77541_adc_range {
+	LOW_RANGE,
+	MID_RANGE,
+	HIGH_RANGE,
+};
+
+struct max77541_adc_iio {
+	struct regmap	*regmap;
+};
+
+enum max77541_adc_channel {
+	MAX77541_ADC_VSYS_V = 0,
+	MAX77541_ADC_VOUT1_V,
+	MAX77541_ADC_VOUT2_V,
+	MAX77541_ADC_TEMP,
+};
+
+static int max77541_adc_offset(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int *val, int *val2)
+{
+	switch (chan->channel) {
+	case MAX77541_ADC_VSYS_V:
+	case MAX77541_ADC_VOUT1_V:
+	case MAX77541_ADC_VOUT2_V:
+		*val = 0;
+		*val2 = 0;
+		return IIO_VAL_INT_PLUS_MICRO;
+	case MAX77541_ADC_TEMP:
+		*val = DIV_ROUND_CLOSEST(ABSOLUTE_ZERO_MILLICELSIUS,
+					 MILLIDEGREE_PER_DEGREE);
+		*val2 = 0;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int max77541_adc_scale(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int *val, int *val2)
+{
+	struct max77541_adc_iio *info = iio_priv(indio_dev);
+	unsigned int reg_val;
+	int ret;
+
+	switch (chan->channel) {
+	case MAX77541_ADC_VSYS_V:
+		*val = 0;
+		*val2 = 25000;
+		return IIO_VAL_INT_PLUS_MICRO;
+	case MAX77541_ADC_VOUT1_V:
+	case MAX77541_ADC_VOUT2_V:
+		ret = regmap_read(info->regmap, MAX77541_REG_M2_CFG1, &reg_val);
+		if (ret)
+			return ret;
+		reg_val = FIELD_GET(MAX77541_BITS_MX_CFG1_RNG, reg_val);
+
+		*val = 0;
+
+		switch (reg_val) {
+		case LOW_RANGE:
+			*val2 = 6250;
+			break;
+		case MID_RANGE:
+			*val2 = 12500;
+			break;
+		case HIGH_RANGE:
+			*val2 = 25000;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	case MAX77541_ADC_TEMP:
+		*val = 1;
+		*val2 = 725000;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int max77541_adc_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val)
+{
+	struct max77541_adc_iio *info = iio_priv(indio_dev);
+	int ret;
+
+	ret = regmap_read(info->regmap, chan->address, val);
+	if (ret)
+		return ret;
+
+	return IIO_VAL_INT;
+}
+
+static const struct iio_chan_spec max77541_adc_channels[] = {
+	MAX77541_ADC_CHANNEL(MAX77541_ADC_VSYS_V, "vsys_v", IIO_VOLTAGE,
+			     MAX77541_REG_ADC_DATA_CH1),
+	MAX77541_ADC_CHANNEL(MAX77541_ADC_VOUT1_V, "vout1_v", IIO_VOLTAGE,
+			     MAX77541_REG_ADC_DATA_CH2),
+	MAX77541_ADC_CHANNEL(MAX77541_ADC_VOUT2_V, "vout2_v", IIO_VOLTAGE,
+			     MAX77541_REG_ADC_DATA_CH3),
+	MAX77541_ADC_CHANNEL(MAX77541_ADC_TEMP, "temp", IIO_TEMP,
+			     MAX77541_REG_ADC_DATA_CH6),
+};
+
+static int max77541_adc_read_raw(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan,
+				 int *val, int *val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_OFFSET:
+		return max77541_adc_offset(indio_dev, chan, val, val2);
+	case IIO_CHAN_INFO_SCALE:
+		return max77541_adc_scale(indio_dev, chan, val, val2);
+	case IIO_CHAN_INFO_RAW:
+		return max77541_adc_raw(indio_dev, chan, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info max77541_adc_info = {
+	.read_raw = max77541_adc_read_raw,
+};
+
+static int max77541_adc_probe(struct platform_device *pdev)
+{
+	struct max77541_dev *max77541 = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct max77541_adc_iio *info;
+	struct iio_dev *indio_dev;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+
+	info->regmap = max77541->regmap;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	indio_dev->name = platform_get_device_id(pdev)->name;
+	indio_dev->info = &max77541_adc_info;
+	indio_dev->channels = max77541_adc_channels;
+	indio_dev->num_channels = ARRAY_SIZE(max77541_adc_channels);
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct platform_device_id max77541_adc_platform_id[] = {
+	{ "max77541-adc", },
+	{  /* sentinel */  }
+};
+MODULE_DEVICE_TABLE(platform, max77541_adc_platform_id);
+
+static struct platform_driver max77541_adc_driver = {
+	.driver = {
+		.name = "max77541-adc",
+	},
+	.probe = max77541_adc_probe,
+	.id_table = max77541_adc_platform_id,
+};
+module_platform_driver(max77541_adc_driver);
+
+MODULE_AUTHOR("Okan Sahin <Okan.Sahin@analog.com>");
+MODULE_DESCRIPTION("MAX77541 ADC driver");
+MODULE_LICENSE("GPL");
-- 
2.30.2

Re: [PATCH v2 5/5] drivers: iio: adc: Add MAX77541 ADC Support

[Jonathan Cameron]

On Tue, 27 Dec 2022 01:38:39 +0300
Okan Sahin <okan.sahin@analog.com> wrote:

> The MAX77541 has an 8-bit Successive Approximation Register (SAR) ADC
> with four multiplexers for supporting the telemetry feature
> 
> Signed-off-by: Okan Sahin <okan.sahin@analog.com>

If there is a v3 please send the whole series to everyone who is cc'd on
any of the patches in this version.  For a driver like this, it's much better
to let people pick and choose which bits they are interested in than
to only send part of it to each list / reviewer.

I took a closer look at the offsets / scales this time.  They don't appear
to comply with the ABI.  Whilst 'most' IIO units are SI units, a few have
units borrowed from hwmon including temperature and voltage - this brings
some multiplication factors that need to be taken into account.

Jonathan

> ---
>  MAINTAINERS                    |   1 +
>  drivers/iio/adc/Kconfig        |  11 ++
>  drivers/iio/adc/Makefile       |   1 +
>  drivers/iio/adc/max77541-adc.c | 199 +++++++++++++++++++++++++++++++++
>  4 files changed, 212 insertions(+)
>  create mode 100644 drivers/iio/adc/max77541-adc.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8e5572b28a8c..18ce4644cc75 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12503,6 +12503,7 @@ L:	linux-kernel@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/mfd/adi,max77541.yaml
>  F:	Documentation/devicetree/bindings/regulator/adi,max77541.yaml
> +F:	drivers/iio/adc/max77541-adc.c
>  F:	drivers/mfd/max77541.c
>  F:	drivers/regulator/max77541-regulator.c
>  F:	include/linux/mfd/max77541.h
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 791612ca6012..9716225b50da 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -696,6 +696,17 @@ config MAX1363
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max1363.
>  
> +config MAX77541_ADC
> +	tristate "Analog Devices MAX77541 ADC driver"
> +	depends on MFD_MAX77541
> +	help
> +	  This driver controls a Analog Devices MAX77541 ADC
> +	  via I2C bus. This device has one adc. Say yes here to build
> +	  support for Analog Devices MAX77541 ADC interface.
> +
> +	  To compile this driver as a module, choose M here:
> +	  the module will be called max77541-adc.
> +
>  config MAX9611
>  	tristate "Maxim max9611/max9612 ADC driver"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 46caba7a010c..03774cccbb4b 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_MAX1118) += max1118.o
>  obj-$(CONFIG_MAX11205) += max11205.o
>  obj-$(CONFIG_MAX1241) += max1241.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> +obj-$(CONFIG_MAX77541_ADC) += max77541-adc.o
>  obj-$(CONFIG_MAX9611) += max9611.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
> diff --git a/drivers/iio/adc/max77541-adc.c b/drivers/iio/adc/max77541-adc.c
> new file mode 100644
> index 000000000000..29adcdbd96ae
> --- /dev/null
> +++ b/drivers/iio/adc/max77541-adc.c
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 Analog Devices, Inc.
> + * ADI MAX77541 ADC Driver with IIO interface
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/iio/iio.h>
> +#include <linux/mfd/max77541.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/units.h>
> +
> +#define MAX77541_ADC_CHANNEL(_channel, _name, _type, _reg) \
> +	{							\
> +		.type = _type,					\
> +		.indexed = 1,					\
> +		.channel = _channel,				\
> +		.address = _reg,				\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
> +				      BIT(IIO_CHAN_INFO_SCALE) |\
> +				      BIT(IIO_CHAN_INFO_OFFSET),\
> +		.datasheet_name = _name,			\
> +	}

Move this macro down to just above where it is used.  Given the purpose
of this is to reduce boilerplate repetition we want to review what
it does based on the values provided.  That's much easier if we don't
have to go look for the macro.

> +
> +enum max77541_adc_range {
> +	LOW_RANGE,
> +	MID_RANGE,
> +	HIGH_RANGE,
> +};
> +
> +struct max77541_adc_iio {
> +	struct regmap	*regmap;
> +};
> +
> +enum max77541_adc_channel {
> +	MAX77541_ADC_VSYS_V = 0,
> +	MAX77541_ADC_VOUT1_V,
> +	MAX77541_ADC_VOUT2_V,
> +	MAX77541_ADC_TEMP,
> +};
> +
> +static int max77541_adc_offset(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       int *val, int *val2)
> +{
> +	switch (chan->channel) {
> +	case MAX77541_ADC_VSYS_V:
> +	case MAX77541_ADC_VOUT1_V:
> +	case MAX77541_ADC_VOUT2_V:
> +		*val = 0;
> +		*val2 = 0;
> +		return IIO_VAL_INT_PLUS_MICRO;

I thought I mentioned this before.  For cases where the offset is 0 don't 
set the bit in info_mask_seperate and don't provide an implementation to
read it.
 
> +	case MAX77541_ADC_TEMP:
> +		*val = DIV_ROUND_CLOSEST(ABSOLUTE_ZERO_MILLICELSIUS,
> +					 MILLIDEGREE_PER_DEGREE);
> +		*val2 = 0;

If *val2 == 0
then the return type should be IIO_VAL_INT allowing any
code using this to handle it as an integer, not a fixed point number.

I'd also like a comment here to explain the maths being done.
Base units of temperature are mili degrees Celsius and the offset looks wrong anyway as it doesn't
take into account that in datasheet it is applied after scale, whereas in IIO ABI it needs
to be applied first (thus requiring it to be divided by scale)
https://elixir.bootlin.com/linux/v6.2-rc1/source/Documentation/ABI/testing/sysfs-bus-iio#L244

So what you currently have is
1LSB = 1.725*(raw + -273/1000) 
I think it should be - based on equation (-273 + 1.725 * ADC_DATA6) deg C
1LSB = 1725*(raw - 273/1725) so offset should be -273/1725 and scale should be 1725.

Also check scaling is right for voltage channels - noting that voltage channels are
expressed in milivolts I suspect the sale should therefore be 25 but I haven't checked
it closely.



> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +



As noted above, put the macro definition here so it is easy to see what
this does.

> +static const struct iio_chan_spec max77541_adc_channels[] = {
> +	MAX77541_ADC_CHANNEL(MAX77541_ADC_VSYS_V, "vsys_v", IIO_VOLTAGE,
> +			     MAX77541_REG_ADC_DATA_CH1),
> +	MAX77541_ADC_CHANNEL(MAX77541_ADC_VOUT1_V, "vout1_v", IIO_VOLTAGE,
> +			     MAX77541_REG_ADC_DATA_CH2),
> +	MAX77541_ADC_CHANNEL(MAX77541_ADC_VOUT2_V, "vout2_v", IIO_VOLTAGE,
> +			     MAX77541_REG_ADC_DATA_CH3),
> +	MAX77541_ADC_CHANNEL(MAX77541_ADC_TEMP, "temp", IIO_TEMP,
> +			     MAX77541_REG_ADC_DATA_CH6),
> +};
> +

> +
> +static int max77541_adc_probe(struct platform_device *pdev)
> +{
> +	struct max77541_dev *max77541 = dev_get_drvdata(pdev->dev.parent);

I can't easily see other parts of the mfd (as not cc'd on rest of series),
but from what is here it might be nice to set the drvdata to the regmap rather
than the mfd driver specific structure.

> +	struct device *dev = &pdev->dev;
> +	struct max77541_adc_iio *info;
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +
> +	info->regmap = max77541->regmap;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	indio_dev->name = platform_get_device_id(pdev)->name;

I would prefer to see that hard coded.  Also, from an IIO point
of view this should just be the part number which is "max77541"
without the -adc postfix.

The advantage of hard coding it is I don't need to think about what
indio_dev->name = "max77541"; 
sets it to thus reducing the complexity of reviewing a little.
Note that similar cases have gone wrong in the past and we've
ended up stuck with hideous ABI for a few device names so I'm
paranoid about this now.


> +	indio_dev->info = &max77541_adc_info;
> +	indio_dev->channels = max77541_adc_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max77541_adc_channels);
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct platform_device_id max77541_adc_platform_id[] = {
> +	{ "max77541-adc", },
> +	{  /* sentinel */  }
> +};
> +MODULE_DEVICE_TABLE(platform, max77541_adc_platform_id);
> +
> +static struct platform_driver max77541_adc_driver = {
> +	.driver = {
> +		.name = "max77541-adc",
> +	},
> +	.probe = max77541_adc_probe,
> +	.id_table = max77541_adc_platform_id,
> +};
> +module_platform_driver(max77541_adc_driver);
> +
> +MODULE_AUTHOR("Okan Sahin <Okan.Sahin@analog.com>");
> +MODULE_DESCRIPTION("MAX77541 ADC driver");
> +MODULE_LICENSE("GPL");

RE: [PATCH v2 5/5] drivers: iio: adc: Add MAX77541 ADC Support

[Sahin, Okan]

Hi Jonathan,

Thank you for your feedback and efforts. I apologize for some missing points of v2 patch. I will be more careful to fix all feedback before sending new patch so I want to ask something before sending v3 patch. 

On Fri, 30 Dec 2022 8:50 PM
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 27 Dec 2022 01:38:39 +0300
> Okan Sahin <okan.sahin@analog.com> wrote:
> 
> > The MAX77541 has an 8-bit Successive Approximation Register (SAR) ADC
> > with four multiplexers for supporting the telemetry feature
> >
> > Signed-off-by: Okan Sahin <okan.sahin@analog.com>
> 
> If there is a v3 please send the whole series to everyone who is cc'd on any of
> the patches in this version.  For a driver like this, it's much better to let people
> pick and choose which bits they are interested in than to only send part of it to
> each list / reviewer.
> 
> I took a closer look at the offsets / scales this time.  They don't appear to
> comply with the ABI.  Whilst 'most' IIO units are SI units, a few have units
> borrowed from hwmon including temperature and voltage - this brings some
> multiplication factors that need to be taken into account.
> 
> Jonathan
> 
> > ---
> >  MAINTAINERS                    |   1 +
> >  drivers/iio/adc/Kconfig        |  11 ++
> >  drivers/iio/adc/Makefile       |   1 +
> >  drivers/iio/adc/max77541-adc.c | 199
> > +++++++++++++++++++++++++++++++++
> >  4 files changed, 212 insertions(+)
> >  create mode 100644 drivers/iio/adc/max77541-adc.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 8e5572b28a8c..18ce4644cc75 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12503,6 +12503,7 @@ L:	linux-kernel@vger.kernel.org
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/mfd/adi,max77541.yaml
> >  F:	Documentation/devicetree/bindings/regulator/adi,max77541.yaml
> > +F:	drivers/iio/adc/max77541-adc.c
> >  F:	drivers/mfd/max77541.c
> >  F:	drivers/regulator/max77541-regulator.c
> >  F:	include/linux/mfd/max77541.h
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index
> > 791612ca6012..9716225b50da 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -696,6 +696,17 @@ config MAX1363
> >  	  To compile this driver as a module, choose M here: the module will be
> >  	  called max1363.
> >
> > +config MAX77541_ADC
> > +	tristate "Analog Devices MAX77541 ADC driver"
> > +	depends on MFD_MAX77541
> > +	help
> > +	  This driver controls a Analog Devices MAX77541 ADC
> > +	  via I2C bus. This device has one adc. Say yes here to build
> > +	  support for Analog Devices MAX77541 ADC interface.
> > +
> > +	  To compile this driver as a module, choose M here:
> > +	  the module will be called max77541-adc.
> > +
> >  config MAX9611
> >  	tristate "Maxim max9611/max9612 ADC driver"
> >  	depends on I2C
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index
> > 46caba7a010c..03774cccbb4b 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -64,6 +64,7 @@ obj-$(CONFIG_MAX1118) += max1118.o
> >  obj-$(CONFIG_MAX11205) += max11205.o
> >  obj-$(CONFIG_MAX1241) += max1241.o
> >  obj-$(CONFIG_MAX1363) += max1363.o
> > +obj-$(CONFIG_MAX77541_ADC) += max77541-adc.o
> >  obj-$(CONFIG_MAX9611) += max9611.o
> >  obj-$(CONFIG_MCP320X) += mcp320x.o
> >  obj-$(CONFIG_MCP3422) += mcp3422.o
> > diff --git a/drivers/iio/adc/max77541-adc.c
> > b/drivers/iio/adc/max77541-adc.c new file mode 100644 index
> > 000000000000..29adcdbd96ae
> > --- /dev/null
> > +++ b/drivers/iio/adc/max77541-adc.c
> > @@ -0,0 +1,199 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2022 Analog Devices, Inc.
> > + * ADI MAX77541 ADC Driver with IIO interface  */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/mfd/max77541.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/driver.h>
> > +#include <linux/regulator/of_regulator.h> #include <linux/units.h>
> > +
> > +#define MAX77541_ADC_CHANNEL(_channel, _name, _type, _reg) \
> > +	{							\
> > +		.type = _type,					\
> > +		.indexed = 1,					\
> > +		.channel = _channel,				\
> > +		.address = _reg,				\
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
> > +				      BIT(IIO_CHAN_INFO_SCALE) |\
> > +				      BIT(IIO_CHAN_INFO_OFFSET),\
> > +		.datasheet_name = _name,			\
> > +	}
> 
> Move this macro down to just above where it is used.  Given the purpose of this
> is to reduce boilerplate repetition we want to review what it does based on the
> values provided.  That's much easier if we don't have to go look for the macro.
> 
> > +
> > +enum max77541_adc_range {
> > +	LOW_RANGE,
> > +	MID_RANGE,
> > +	HIGH_RANGE,
> > +};
> > +
> > +struct max77541_adc_iio {
> > +	struct regmap	*regmap;
> > +};
> > +
> > +enum max77541_adc_channel {
> > +	MAX77541_ADC_VSYS_V = 0,
> > +	MAX77541_ADC_VOUT1_V,
> > +	MAX77541_ADC_VOUT2_V,
> > +	MAX77541_ADC_TEMP,
> > +};
> > +
> > +static int max77541_adc_offset(struct iio_dev *indio_dev,
> > +			       struct iio_chan_spec const *chan,
> > +			       int *val, int *val2)
> > +{
> > +	switch (chan->channel) {
> > +	case MAX77541_ADC_VSYS_V:
> > +	case MAX77541_ADC_VOUT1_V:
> > +	case MAX77541_ADC_VOUT2_V:
> > +		*val = 0;
> > +		*val2 = 0;
> > +		return IIO_VAL_INT_PLUS_MICRO;
> 
> I thought I mentioned this before.  For cases where the offset is 0 don't set the
> bit in info_mask_seperate and don't provide an implementation to read it.
> 
> > +	case MAX77541_ADC_TEMP:
> > +		*val = DIV_ROUND_CLOSEST(ABSOLUTE_ZERO_MILLICELSIUS,
> > +					 MILLIDEGREE_PER_DEGREE);
> > +		*val2 = 0;
> 
> If *val2 == 0
> then the return type should be IIO_VAL_INT allowing any code using this to
> handle it as an integer, not a fixed point number.
> 
> I'd also like a comment here to explain the maths being done.
> Base units of temperature are mili degrees Celsius and the offset looks wrong
> anyway as it doesn't take into account that in datasheet it is applied after scale,
> whereas in IIO ABI it needs to be applied first (thus requiring it to be divided by
> scale) https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.2-
> rc1/source/Documentation/ABI/testing/sysfs-bus-
> iio*L244__;Iw!!A3Ni8CS0y2Y!7FLhRoK07ZzeEcEw_H5bPRrK-Dpnrs-mBOV-
> mWXiEWRQQZDa3F89L6WaGw0St2sFxPgtGhIznNbdig$
> 
> So what you currently have is
> 1LSB = 1.725*(raw + -273/1000)
> I think it should be - based on equation (-273 + 1.725 * ADC_DATA6) deg C 1LSB
> = 1725*(raw - 273/1725) so offset should be -273/1725 and scale should be
> 1725.
> 
> Also check scaling is right for voltage channels - noting that voltage channels are
> expressed in milivolts I suspect the sale should therefore be 25 but I haven't
> checked it closely.
> 
> 
> 
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> 
> 
> 
> As noted above, put the macro definition here so it is easy to see what this does.
> 
> > +static const struct iio_chan_spec max77541_adc_channels[] = {
> > +	MAX77541_ADC_CHANNEL(MAX77541_ADC_VSYS_V, "vsys_v",
> IIO_VOLTAGE,
> > +			     MAX77541_REG_ADC_DATA_CH1),
> > +	MAX77541_ADC_CHANNEL(MAX77541_ADC_VOUT1_V, "vout1_v",
> IIO_VOLTAGE,
> > +			     MAX77541_REG_ADC_DATA_CH2),
> > +	MAX77541_ADC_CHANNEL(MAX77541_ADC_VOUT2_V, "vout2_v",
> IIO_VOLTAGE,
> > +			     MAX77541_REG_ADC_DATA_CH3),
> > +	MAX77541_ADC_CHANNEL(MAX77541_ADC_TEMP, "temp", IIO_TEMP,
> > +			     MAX77541_REG_ADC_DATA_CH6),
> > +};
> > +
> 
> > +
> > +static int max77541_adc_probe(struct platform_device *pdev) {
> > +	struct max77541_dev *max77541 = dev_get_drvdata(pdev-
> >dev.parent);
> 
> I can't easily see other parts of the mfd (as not cc'd on rest of series), but from
> what is here it might be nice to set the drvdata to the regmap rather than the
> mfd driver specific structure.
> 
This is my mistake, I will send whole patches to people who related them at once. As you said I just need regmap from parent mfd device. I think below is what you expect, right? I will also remove dev_get_drv_data.
struct device *dev = &pdev->dev;
struct regmap *map = dev_get_regmap(dev->parent, NULL);

> > +	struct device *dev = &pdev->dev;
> > +	struct max77541_adc_iio *info;
> > +	struct iio_dev *indio_dev;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	info = iio_priv(indio_dev);
> > +
> > +	info->regmap = max77541->regmap;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	indio_dev->name = platform_get_device_id(pdev)->name;
> 
> I would prefer to see that hard coded.  Also, from an IIO point of view this
> should just be the part number which is "max77541"
> without the -adc postfix.
> 
> The advantage of hard coding it is I don't need to think about what indio_dev-
> >name = "max77541"; sets it to thus reducing the complexity of reviewing a
> little.
> Note that similar cases have gone wrong in the past and we've ended up stuck
> with hideous ABI for a few device names so I'm paranoid about this now.
> 
> 
> > +	indio_dev->info = &max77541_adc_info;
> > +	indio_dev->channels = max77541_adc_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(max77541_adc_channels);
> > +
> > +	return devm_iio_device_register(dev, indio_dev);
> > +}
> > +
> > +static const struct platform_device_id max77541_adc_platform_id[] = {
> > +	{ "max77541-adc", },
> > +	{  /* sentinel */  }
> > +};
> > +MODULE_DEVICE_TABLE(platform, max77541_adc_platform_id);
> > +
> > +static struct platform_driver max77541_adc_driver = {
> > +	.driver = {
> > +		.name = "max77541-adc",
> > +	},
> > +	.probe = max77541_adc_probe,
> > +	.id_table = max77541_adc_platform_id,
> > +};
> > +module_platform_driver(max77541_adc_driver);
> > +
> > +MODULE_AUTHOR("Okan Sahin <Okan.Sahin@analog.com>");
> > +MODULE_DESCRIPTION("MAX77541 ADC driver");
> > +MODULE_LICENSE("GPL");