On 3/17/20 9:17 PM, Alexandru Lazar wrote:
Add driver for the Maxim MAX1241 12-bit, single-channel ADC. The driver
includes support for this device's low-power operation mode.
Signed-off-by: Alexandru Lazar <alazar@xxxxxxxxxxxxx>
Hi,
Looks very good, small clean driver. Few comments inline
---
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
depends on SPI_MASTER
There is also SPI_SLAVE support no in the kernel and just SPI does not
imply SPI_MASTER.
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ Say yes here to build support for Maxim max1241 12-bit, single-channel
+ ADC.
+
+ To compile this driver as a module, choose M here: the module will be
+ called max1118.
+
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.
+ *
+ * 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>
Probably only needs the gpio/consumer.h, but not the other two gpio
includes.
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+
[...]
+
+static int max1241_read(struct max1241 *adc)
+{
+ int ret;
+ struct spi_device *spi = adc->spi;
I'd go without the spi variable here and just use adc->spi directly when
calling spi_sync_transfer().
+ struct spi_transfer xfers[] = {
+ /*
+ * Begin conversion by bringing /CS low for at least
+ * tconv us.
+ */
+ {
+ .len = 0,
+ .delay_usecs = 8,
+ },
+ /*
+ * Then read two bytes of data in our RX buffer.
+ */
+ {
+ .rx_buf = &adc->data,
+ .len = 2,
+ },
+ };
+
+ ret = spi_sync_transfer(spi, xfers, 2);
ARRAY_SIZE(xfers) instead of 2.
Maybe also directly 'return spi_sync_transfer()'.
+
+ return ret;
+}
[...]
+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;
+
+ adc->shdn = devm_gpiod_get_optional(&spi->dev, "shdn", GPIOD_OUT_HIGH);
Makes sense to check for IS_ERR(adc->shdn) here and return if there is
an error. This allows you to handle both probe deferral as well as if
there is a mistake in the GPIO specification in the devicetree.
+ 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");
I can see how these message above are useful during development, but I'd
remove them or turn them into dev_dbg() for the "production" version of
the driver. Imagine every driver printed something like this, there
would be a lot of spam in the bootlog.
+
+ 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);
+
+ return ret;
I'd go with just "return iio_device_register()".
+}
+
+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;
Remove can't really fail, the return type is only int for historic
reasons. The function should always return 0.
+}
