On Tue, 2020-03-17 at 22:17 +0200, Alexandru Lazar wrote: > [External] > > Add driver for the Maxim MAX1241 12-bit, single-channel ADC. The driver > includes support for this device's low-power operation mode. hey, overall looks good; i'd run ./scripts/checpatch.pl on the patches a bit; you can run it on the patch file, or on the git commit with ./scripts/checpatch.pl -g <git-commits> i usually do ./scripts/checpatch.pl -g HEAD~2.. [or something like that] before generating patches; i sometimes forget to do that; some more comments inline > > Signed-off-by: Alexandru Lazar <alazar@xxxxxxxxxxxxx> > --- > drivers/iio/adc/Kconfig | 12 +++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/max1241.c | 215 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 228 insertions(+) > create mode 100644 drivers/iio/adc/max1241.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 5d8540b7b427..3a55beec69c9 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -566,6 +566,18 @@ config MAX1118 > To compile this driver as a module, choose M here: the module will be > called max1118. > > +config MAX1241 > + tristate "Maxim max1241 ADC driver" > + depends on SPI > + select IIO_BUFFER > + select IIO_TRIGGERED_BUFFER > + help > + Say yes here to build support for Maxim max1241 12-bit, single-channel > + ADC. nitpick: this looks inconsistently indented > + > + To compile this driver as a module, choose M here: the module will be > + called max1118. > + > config MAX1363 > tristate "Maxim max1363 ADC driver" > depends on I2C > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index a1f1fbec0f87..37d6f17559dc 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -54,6 +54,7 @@ obj-$(CONFIG_LTC2497) += ltc2497.o > obj-$(CONFIG_MAX1027) += max1027.o > obj-$(CONFIG_MAX11100) += max11100.o > obj-$(CONFIG_MAX1118) += max1118.o > +obj-$(CONFIG_MAX1241) += max1241.o > obj-$(CONFIG_MAX1363) += max1363.o > obj-$(CONFIG_MAX9611) += max9611.o > obj-$(CONFIG_MCP320X) += mcp320x.o > diff --git a/drivers/iio/adc/max1241.c b/drivers/iio/adc/max1241.c > new file mode 100644 > index 000000000000..2bd31f22fb2c > --- /dev/null > +++ b/drivers/iio/adc/max1241.c > @@ -0,0 +1,215 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * MAX1241 low-power, 12-bit serial ADC > + * > + * Copyright (c) 2020 Ioan-Alexandru Lazar <alazar@xxxxxxxxxxxxx> > + * > + * This file is subject to the terms and conditions of version 2 of > + * the GNU General Public License. See the file COPYING in the main > + * directory of this archive for more details. This license text is no longer needed. The SPDX-License-Identifier header should handle that. > + * > + * Datasheet: > https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf > + */ > + > +#include <linux/regulator/consumer.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/gpio/driver.h> > +#include <linux/gpio.h> > +#include <linux/iio/iio.h> > +#include <linux/module.h> > +#include <linux/spi/spi.h> > + > +#define MAX1241_VAL_MASK 0xFFF > +#define MAX1241_SHDN_DELAY_USEC 4 > + > +enum max1241_id { > + max1241, > +}; > + > +struct max1241 { > + struct spi_device *spi; > + struct mutex lock; > + struct regulator *reg; > + struct gpio_desc *shdn; > + > + __be16 data ____cacheline_aligned; Jonathan may know better than me here, but you could technically avoid needing to explicitly use the __be16 datatype; and just use u16; i think the SPI framework should have some handling for that; maybe using the 'bits_per_word' field; you'd probably still need to do the shifting though; i remember some discussion about this on ad7949.c though there may be other drivers doing this as well though, this isn't a big deal; and i don't feel strongly about doing like this or some other way; this comment tries to be more informative [or just noisy] > +}; > + > +static const struct iio_chan_spec max1241_channels[] = { > + { > + .type = IIO_VOLTAGE, > + .indexed = 1, > + .channel = 0, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + .scan_index = 0, > + .scan_type = { > + .sign = 'u', > + .realbits = 12, > + .storagebits = 16, > + }, > + }, > +}; > + > +static int max1241_read(struct max1241 *adc) > +{ > + int ret; > + struct spi_device *spi = adc->spi; > + struct spi_transfer xfers[] = { > + /* > + * Begin conversion by bringing /CS low for at least > + * tconv us. > + */ > + { > + .len = 0, > + .delay_usecs = 8, 'delay_usecs' is going to go away. Can you change this to? .delay.value = 8, .delay.unit = SPI_DELAY_UNIT_USECS. SPI_DELAY_UNIT_USECS is 0, so if you don't assign it's fine, but it's a good idea to assign it, to make it clear it's usecs > + }, > + /* > + * Then read two bytes of data in our RX buffer. > + */ > + { > + .rx_buf = &adc->data, > + .len = 2, > + }, > + }; > + > + ret = spi_sync_transfer(spi, xfers, 2); > + > + return ret; > +} > + > +static int max1241_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + int ret, vref_uV; > + struct max1241 *adc = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + mutex_lock(&adc->lock); > + > + if (adc->shdn) { > + gpiod_set_value(adc->shdn, 0); > + udelay(MAX1241_SHDN_DELAY_USEC); > + } > + > + ret = max1241_read(adc); > + > + if (adc->shdn) > + gpiod_set_value(adc->shdn, 1); > + > + if (ret) { > + mutex_unlock(&adc->lock); > + return ret; > + } > + > + *val = (be16_to_cpu(adc->data) >> 3) & MAX1241_VAL_MASK; > + > + mutex_unlock(&adc->lock); > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + vref_uV = regulator_get_voltage(adc->reg); > + > + if (vref_uV < 0) > + return vref_uV; > + > + *val = vref_uV / 1000; > + *val2 = 12; > + > + return IIO_VAL_FRACTIONAL_LOG2; > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_info max1241_info = { > + .read_raw = max1241_read_raw, > +}; > + > +static int max1241_probe(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev; > + struct max1241 *adc; > + int ret = 0; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc)); > + if (!indio_dev) > + return -ENOMEM; > + > + adc = iio_priv(indio_dev); > + adc->spi = spi; > + mutex_init(&adc->lock); > + > + spi_set_drvdata(spi, indio_dev); > + > + adc->reg = devm_regulator_get(&spi->dev, "vref"); > + if (IS_ERR(adc->reg)) { > + dev_err(&spi->dev, "failed to get vref regulator\n"); > + return PTR_ERR(adc->reg); > + } > + > + ret = regulator_enable(adc->reg); > + if (ret) > + return ret; > + [1] > + adc->shdn = devm_gpiod_get_optional(&spi->dev, "shdn", GPIOD_OUT_HIGH); > + if (!adc->shdn) > + dev_info(&spi->dev, "no shdn pin passed, low-power mode > disabled"); > + else > + dev_info(&spi->dev, "shdn pin passed, low-power mode enabled"); > + > + indio_dev->name = spi_get_device_id(spi)->name; > + indio_dev->dev.parent = &spi->dev; > + indio_dev->info = &max1241_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = max1241_channels; > + indio_dev->num_channels = ARRAY_SIZE(max1241_channels); > + > + ret = iio_device_register(indio_dev); This should disable the regulator on the error path. if (ret) { regulator_disable(adc->reg); return ret; } return 0; Though, I would argue in favor of adding a devm_add_action_or_reset() callback to disable the regulator on error & remove. This should be placed at [1] Example: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/isl29018.c#n762 I've started to grow quite fond of these type of callbacks. For one part, you can remove the 'remove' part of the driver. On another part you can do neat stuff to simplify/reduce error paths in probe. We typically suggest these during our internal reviews. > + > + return ret; > +} > + > +static int max1241_remove(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct max1241 *adc = iio_priv(indio_dev); > + int ret = 0; > + > + iio_device_unregister(indio_dev); > + ret = regulator_disable(adc->reg); > + > + return ret; > +} > + > +static const struct spi_device_id max1241_id[] = { > + { "max1241", max1241 }, > + {}, > +}; > + > +#ifdef CONFIG_OF i'd remove this CONFIG_OF i guess this was copied from max1118.c see [2] > + > +static const struct of_device_id max1241_dt_ids[] = { > + { .compatible = "maxim,max1241" }, the datasheet mentions also max1240 you could add that to the list as well typically, it's a good idea, since some people get hung-up on the naming [which is not a bad idea] so, if you add max1240 to this list, the driver officially supports both max1240 & max1241 > + {}, > +}; > +MODULE_DEVICE_TABLE(of, max1241_dt_ids); > + > +#endif > + > +static struct spi_driver max1241_spi_driver = { > + .driver = { > + .name = "max1241", > + .of_match_table = of_match_ptr(max1241_dt_ids), [2] i'd remove of_match_ptr() and make it just .of_match_table = max1241_dt_ids, there's this code in the kernel that parses of_match_table for ACPI as well; might as well allow it to work > + }, > + .probe = max1241_probe, > + .remove = max1241_remove, > + .id_table = max1241_id, > +}; > +module_spi_driver(max1241_spi_driver); > + > +MODULE_AUTHOR("Alexandru Lazar <alazar@xxxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("MAX1241 ADC driver"); > +MODULE_LICENSE("GPL v2");
