Re: [PATCH 1/2] iio: adc: Add MAX1241 driver

[Lars-Peter Clausen <lars@metafoo.de>]

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@startmail.com>

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@startmail.com>
> + *
> + * 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.

> +}