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@xxxxxxxxxx> wrote:

> On Tue, 27 Dec 2022 01:38:39 +0300
> Okan Sahin <okan.sahin@xxxxxxxxxx> 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@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
> >  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@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("MAX77541 ADC driver");
> > +MODULE_LICENSE("GPL");