| Message ID | 20221207090906.5896-6-okan.sahin@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | staging: drivers: mfd: Add MAX77541 MFD and related device drivers | expand |
On Wed, Dec 07, 2022 at 12:08:44PM +0300, Okan Sahin wrote: > This patch add adc support for MAX77541. > > The MAX77541 has an 8-bit Successive Approximation Register (SAR) ADC > with four multiplexers for supporting the telemetry feature Same comment as per patch 2. ... > +#include <linux/bitfield.h> > +#include <linux/iio/iio.h> > +#include <include/linux/mfd/max77541.h> Hmm... > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/regulator/driver.h> > +#include <linux/regulator/of_regulator.h> ... > +enum { > + MAX77541_ADC_CH1_I = 0, > + MAX77541_ADC_CH2_I, > + MAX77541_ADC_CH3_I, > + MAX77541_ADC_CH6_I, > + > + MAX77541_ADC_IRQMAX_I, If it's a terminator, drop the trailing comma. > +}; ... > + case MAX77541_ADC_TEMP: > + *val = -273; I believe we have definition for this in units.h. Can you use it? > + *val2 = 0; > + return IIO_VAL_INT_PLUS_MICRO; > + } > +} ... > + *val = 0; > + > + if (reg_val == LOW_RANGE) > + *val2 = 6250; > + else if (reg_val == MID_RANGE) > + *val2 = 12500; > + else if (reg_val == HIGH_RANGE) > + *val2 = 25000; > + else > + return -EINVAL; Can it be provided as a table? ... > + *val = 0; > + > + if (reg_val == LOW_RANGE) > + *val2 = 6250; > + else if (reg_val == MID_RANGE) > + *val2 = 12500; > + else if (reg_val == HIGH_RANGE) > + *val2 = 25000; > + else > + return -EINVAL; Ditto. ... > + Redundant blank line. > +module_platform_driver(max77541_adc_driver);
On Wed, 7 Dec 2022 12:08:44 +0300 Okan Sahin <okan.sahin@analog.com> wrote: > This patch add adc support for MAX77541. > > The MAX77541 has an 8-bit Successive Approximation Register (SAR) ADC > with four multiplexers for supporting the telemetry feature Good to add a little detail on what features the driver currently supports. > > Signed-off-by: Okan Sahin <okan.sahin@analog.com> ... > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 791612ca6012..2e7833b33f12 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 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. > + ... > diff --git a/drivers/iio/adc/max77541-adc.c b/drivers/iio/adc/max77541-adc.c > new file mode 100644 > index 000000000000..7ca8576bd4e1 > --- /dev/null > +++ b/drivers/iio/adc/max77541-adc.c > @@ -0,0 +1,224 @@ > +// 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 <include/linux/mfd/max77541.h> Move this driver specific header include to below the main block. We want to make it clear it is special. > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/regulator/driver.h> > +#include <linux/regulator/of_regulator.h> Would be very very surprised to see off_regulator.h correctly included in an IIO driver. Check all of these for whether they are necessary (rather than cut and paste from another driver). > + > +#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_CH1_I = 0, > + MAX77541_ADC_CH2_I, > + MAX77541_ADC_CH3_I, > + MAX77541_ADC_CH6_I, > + > + MAX77541_ADC_IRQMAX_I, > +}; > + > +struct max77541_adc_iio { > + struct regmap *regmap; > + int irq; > + int irq_arr[MAX77541_ADC_IRQMAX_I]; Clear out unused elements until the follow up patch that actually makes us of them. For now they are just distracting noise. > +}; > + > +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; If the offset is zero, then don't have the interface. Default assumption if OFFSET is not provided is that the offset is 0. > + case MAX77541_ADC_TEMP: > + *val = -273; > + *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: > + ret = regmap_read(info->regmap, MAX77541_REG_M2_CFG1, ®_val); > + if (ret) > + return ret; > + > + reg_val = FIELD_GET(MAX77541_BITS_MX_CFG1_RNG, reg_val); > + > + *val = 0; > + > + if (reg_val == LOW_RANGE) Ah. Here is the mysterious macro from patch 1. Bring that definition forwards to this patch. Also switch statement would be more readable here. > + *val2 = 6250; > + else if (reg_val == MID_RANGE) > + *val2 = 12500; > + else if (reg_val == HIGH_RANGE) > + *val2 = 25000; > + else > + return -EINVAL; > + > + return IIO_VAL_INT_PLUS_MICRO; > + case MAX77541_ADC_VOUT2_V: > + ret = regmap_read(info->regmap, MAX77541_REG_M2_CFG1, ®_val); Umm. Is this identical to the above case? If so, just have one block for both. > + if (ret) > + return ret; > + reg_val = FIELD_GET(MAX77541_BITS_MX_CFG1_RNG, reg_val); > + > + *val = 0; > + > + if (reg_val == LOW_RANGE) > + *val2 = 6250; > + else if (reg_val == MID_RANGE) > + *val2 = 12500; > + else if (reg_val == HIGH_RANGE) > + *val2 = 25000; > + else > + 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 const struct iio_chan_spec max77541_adc_channels[] = { Bring the macro definition down to just above this array. That will make it easier to review as we can see the right parameters are being passed in. For 4 channels, I'm not sure I'd bother wrapping it in a macro at all (particularly as that macro needs to be more complex - see above), but I don't mind if you want to keep it that way. I would also move this down below the iio_info definition because that will keep all the code relevant to that in one place rather than throwing this in the middle. > + 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; > + struct max77541_adc_iio *info; > + struct iio_dev *indio_dev; > + > + indio_dev = devm_iio_device_alloc(&pdev->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(&pdev->dev, indio_dev); > +} > + > +static const struct platform_device_id max77541_adc_platform_id[] = { > + { "max77541-adc", MAX77541, }, Better to introduce the complexity given by the type only when it is needed. So for now drop the .data values here and below. If / when that does happen I'll be asking you to use a chip_info structure anyway - right now that structure would be empty so no point in adding it yet. > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(platform, max77541_adc_platform_id); > + > +static const struct of_device_id max77541_adc_of_id[] = { > + { > + .compatible = "adi,max77541-adc", > + .data = (void *)MAX77541, > + }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, max77541_adc_of_id); > + > +static struct platform_driver max77541_adc_driver = { > + .driver = { > + .name = "max77541-adc", > + .of_match_table = max77541_adc_of_id, > + }, > + .probe = max77541_adc_probe, > + .id_table = max77541_adc_platform_id, > +}; > + Drop blank line. Macro and structure are very closely associated so it's good to make that obvious by not separating them. > +module_platform_driver(max77541_adc_driver); > + > +MODULE_AUTHOR("Okan Sahin <Okan.Sahin@analog.com>"); > +MODULE_DESCRIPTION("MAX77541 ADC driver"); > +MODULE_LICENSE("GPL");
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..2e7833b33f12 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..7ca8576bd4e1 --- /dev/null +++ b/drivers/iio/adc/max77541-adc.c @@ -0,0 +1,224 @@ +// 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 <include/linux/mfd/max77541.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/of_regulator.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_CH1_I = 0, + MAX77541_ADC_CH2_I, + MAX77541_ADC_CH3_I, + MAX77541_ADC_CH6_I, + + MAX77541_ADC_IRQMAX_I, +}; + +struct max77541_adc_iio { + struct regmap *regmap; + int irq; + int irq_arr[MAX77541_ADC_IRQMAX_I]; +}; + +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 = -273; + *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: + ret = regmap_read(info->regmap, MAX77541_REG_M2_CFG1, ®_val); + if (ret) + return ret; + + reg_val = FIELD_GET(MAX77541_BITS_MX_CFG1_RNG, reg_val); + + *val = 0; + + if (reg_val == LOW_RANGE) + *val2 = 6250; + else if (reg_val == MID_RANGE) + *val2 = 12500; + else if (reg_val == HIGH_RANGE) + *val2 = 25000; + else + return -EINVAL; + + return IIO_VAL_INT_PLUS_MICRO; + case MAX77541_ADC_VOUT2_V: + ret = regmap_read(info->regmap, MAX77541_REG_M2_CFG1, ®_val); + if (ret) + return ret; + reg_val = FIELD_GET(MAX77541_BITS_MX_CFG1_RNG, reg_val); + + *val = 0; + + if (reg_val == LOW_RANGE) + *val2 = 6250; + else if (reg_val == MID_RANGE) + *val2 = 12500; + else if (reg_val == HIGH_RANGE) + *val2 = 25000; + else + 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; + struct max77541_adc_iio *info; + struct iio_dev *indio_dev; + + indio_dev = devm_iio_device_alloc(&pdev->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(&pdev->dev, indio_dev); +} + +static const struct platform_device_id max77541_adc_platform_id[] = { + { "max77541-adc", MAX77541, }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(platform, max77541_adc_platform_id); + +static const struct of_device_id max77541_adc_of_id[] = { + { + .compatible = "adi,max77541-adc", + .data = (void *)MAX77541, + }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, max77541_adc_of_id); + +static struct platform_driver max77541_adc_driver = { + .driver = { + .name = "max77541-adc", + .of_match_table = max77541_adc_of_id, + }, + .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");
This patch add adc support for MAX77541. 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 | 224 +++++++++++++++++++++++++++++++++ 4 files changed, 237 insertions(+) create mode 100644 drivers/iio/adc/max77541-adc.c