[PATCH 0/2] Maxim MAX1241 driver

[PATCH 0/2] Maxim MAX1241 driver

[Alexandru Lazar]

Hello!

This patch series adds support for the Maxim MAX1241, a 12-bit,
single-channel, SPI-connected ADC. It's a pretty sturdy ADC that I've
used for some time now and I figured my driver may be useful to others
as well.

I originally thought I might get this under the max1027 driver but the
124x family is different enough that shoehorning it in there didn't seem
like a good idea. The 1241 has a single channel, no FIFO, uses a
different mechanism to signal conversion start and has a low-power
operation mode which it uses in a pretty different way. This is actually
closer to MAX111x, but those also have a pretty different signaling
convention.

Needless to say, though, if anyone who is more familiar with this
situation disagrees and wants to point me in the direction of the
appropriate driver, I'm happy to make the changes there instead of
providing a separate driver.

Also please note that I am somewhat unfamiliar with the idioms of the
IIO subsystem's tree. Jonathan Cameron was kind enough to give me a few
pointers but there are a few questions I didn't bother him with (so I'm
guess I'm gonna bother you instead now).

1. There are two ADCs in this family, MAX1240 and MAX1241. This driver
only supports the MAX1241. The scaffolding to get MAX1240 support is in
there, but I didn't have access to a MAX1240 and I didn't want to submit
untested code for review. Can we add MAX1240 support later, or should I
do it from the very beginning? Either way is fine by me (I'm just a
little concerned about how quickly I might *get* a MAX1240 these
days...)

2. Looking at other drivers, it seems to me that, on ADCs that require
an external reference, it's customary to make a voltage regulator a
required property in the DT bindings. Am I correct here?

3. checkpatch.pl warns me that the MAINTAINERS file might need
updating. I'm not sure what the appropriate thing to do is here -- I can
maintain this driver indefinitely (I am actively using it anways) but
it's a 200-line driver, does that warrant inclusion in MAINTAINERS?

My apologies if anything here is distinctly bad -- this isn't the first
time I'm writing kernel code but it's definitely the first time I'm
sending ay upstream. Any bugs are due to my own incompetence, everything
else is just temporary ignorance :-).

Thanks,
Alex

Alexandru Lazar (2):
  iio: adc: Add MAX1241 driver
  dt-bindings: iio: adc: Add MAX1241 device tree bindings in
    documentation

 .../bindings/iio/adc/maxim,max1241.yaml       |  60 +++++
 drivers/iio/adc/Kconfig                       |  12 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/max1241.c                     | 215 ++++++++++++++++++
 4 files changed, 288 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
 create mode 100644 drivers/iio/adc/max1241.c

-- 
2.25.1

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

[Alexandru Lazar]

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>
---
 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.
+
+	  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@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>
+#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;
+};
+
+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,
+		},
+		/*
+		 * 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;
+
+	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);
+
+	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
+
+static const struct of_device_id max1241_dt_ids[] = {
+	{ .compatible = "maxim,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),
+	},
+	.probe = max1241_probe,
+	.remove = max1241_remove,
+	.id_table = max1241_id,
+};
+module_spi_driver(max1241_spi_driver);
+
+MODULE_AUTHOR("Alexandru Lazar <alazar@startmail.com>");
+MODULE_DESCRIPTION("MAX1241 ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

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

[Lars-Peter Clausen]

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.

> +}

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

[Lars-Peter Clausen]

On 3/17/20 9:33 PM, Lars-Peter Clausen wrote:
> On 3/17/20 9:17 PM, Alexandru Lazar wrote:
>> 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.

Sorry, that is of course not true. SPI does imply SPI_MASTER. Still 
SPI_MASTER is the correct dependency here.

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

[Alexandru Lazar]

Hi Lars,

Thank you very much for your comments! I'll send a version with the
fixes in a day or two (ar as soon as there's no more feedback, anyways)
-- in the meantime I have a question about this one:

> > +	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.

I thought this should go under info, rather than debug, because it's
(possibly) relevant runtime information. It doesn't require any action,
but it's something that a user of this driver may want to be aware
of. The timing (and power consumption, of course) in low-power mode are
different. It's akin to e.g.:

./at91_adc.c:782: dev_info(&idev->dev, "Resolution used: %u bits\n", st->res);
./at91_adc.c:842: dev_info(dev, "ADC Touch screen is disabled.\n");
./at91_adc.c:955: dev_info(&idev->dev, "not support touchscreen in the adc compatible string.\n");

or:

./ti-ads124s08.c:320: dev_info(&spi->dev, "Reset GPIO not defined\n");

which is why I figured I'd do the same in my code (what can I say, I
wear my code monkey badge with pride!).

Needless to say, since you've seen more IIO subsystem drivers than I've
seen, I totally trust your assessment of whether this is debug or info
more than I trust mine. If this was a false positive on your "looks like
a leftover debug message", let me know (and while I'm at it I might make
the message more useful/friendly...). Otherwise it'll get bumped down to
dev_dbg in my next revision.

Thanks,
Alex

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

[Lars-Peter Clausen]

On 3/17/20 10:28 PM, Alexandru Lazar wrote:
> Hi Lars,
> 
> Thank you very much for your comments! I'll send a version with the
> fixes in a day or two (ar as soon as there's no more feedback, anyways)
> -- in the meantime I have a question about this one:
> 
>>> +	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.
> 
> I thought this should go under info, rather than debug, because it's
> (possibly) relevant runtime information. It doesn't require any action,
> but it's something that a user of this driver may want to be aware
> of. The timing (and power consumption, of course) in low-power mode are
> different. It's akin to e.g.:
> 
> ./at91_adc.c:782: dev_info(&idev->dev, "Resolution used: %u bits\n", st->res);
> ./at91_adc.c:842: dev_info(dev, "ADC Touch screen is disabled.\n");
> ./at91_adc.c:955: dev_info(&idev->dev, "not support touchscreen in the adc compatible string.\n");
> 
> or:
> 
> ./ti-ads124s08.c:320: dev_info(&spi->dev, "Reset GPIO not defined\n");
> 
> which is why I figured I'd do the same in my code (what can I say, I
> wear my code monkey badge with pride!).
> 
> Needless to say, since you've seen more IIO subsystem drivers than I've
> seen, I totally trust your assessment of whether this is debug or info
> more than I trust mine. If this was a false positive on your "looks like
> a leftover debug message", let me know (and while I'm at it I might make
> the message more useful/friendly...). Otherwise it'll get bumped down to
> dev_dbg in my next revision.

If I was to write this driver I would not make it dev_info(). In my 
opinion drivers should only print essential information during probe, 
like when something goes wrong. Otherwise the boot log gets very noisy 
quickly.

- Lars

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

[Lars-Peter Clausen]

On 3/17/20 9:17 PM, Alexandru Lazar wrote:

> --- 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

One more bit.

Your driver currently does not support buffered mode, so the two select 
statements above are not required.

> +	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.
> +
>   config MAX1363
>   	tristate "Maxim max1363 ADC driver"
>   	depends on I2C

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

[Ardelean, Alexandru]

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

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@startmail.com>");
> +MODULE_DESCRIPTION("MAX1241 ADC driver");
> +MODULE_LICENSE("GPL v2");

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

[Alexandru Lazar]

Hey,

Thanks for the comments! I'll make the changes that you suggested!

I did run checkpatch.pl -- the only thing it complained about was
that the length of a line in the commit message was too long, and I
don't think I can do anything about it because the line in question is a
file path :-).

otoh I definitely didn't do dt_binding_check, I had no idea that was a
thing. I'll run it and integrate any necessary changes in the revised
version. Thanks!

Best regards,
Alex

On Wed, Mar 18, 2020 at 06:50:41AM +0000, Ardelean, Alexandru wrote:
> 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@startmail.com>
> > ---
> >  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@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.
> 
> 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@startmail.com>");
> > +MODULE_DESCRIPTION("MAX1241 ADC driver");
> > +MODULE_LICENSE("GPL v2");

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

[Lars-Peter Clausen]

On 3/18/20 7:50 AM, Ardelean, Alexandru wrote:
> 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@startmail.com>
>> ---
>>   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@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.
> 
> 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

I'd keep it as it is. Which bits_per_word values are supported depends 
on the SPI master driver. All of them support 8-bit, but many don't 
support 16-bit.

- Lars

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

[Ardelean, Alexandru]

On Wed, 2020-03-18 at 10:31 +0100, Lars-Peter Clausen wrote:
> On 3/18/20 7:50 AM, Ardelean, Alexandru wrote:
> > 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@startmail.com>
> > > ---
> > >   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@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.
> > 
> > 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
> 
> I'd keep it as it is. Which bits_per_word values are supported depends 
> on the SPI master driver. All of them support 8-bit, but many don't 
> support 16-bit.

Fair enough.
I'm a bit vague on the details here.

> 
> - Lars

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

[Rohit Sarkar]

On Wed, Mar 18, 2020 at 06:50:41AM +0000, Ardelean, Alexandru wrote:
> 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;  
> 
Another good idea might be to set up a post-commit hook that runs
checkpatch for you! 

Reference: Git post-commit hooks section of https://kernelnewbies.org/FirstKernelPatch

Thanks,
Rohit

[PATCH 2/2] dt-bindings: iio: adc: Add MAX1241 device tree bindings in documentation

[Alexandru Lazar]

Add device-tree bindings documentation for the MAX1241 device driver.

Signed-off-by: Alexandru Lazar <alazar@startmail.com>
---
 .../bindings/iio/adc/maxim,max1241.yaml       | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
new file mode 100644
index 000000000000..abb90d382067
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7780.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim MAX1241 12-bit, single-channel analog to digital converter
+
+maintainers:
+  - Ioan-Alexandru Lazar <alazar@startmail.com>
+
+description: |
+  Bindings for the max1241 12-bit, single-channel ADC device. This
+  driver supports voltage reading and can optionally be configured for
+  power-down mode operation. The datasheet can be found at:
+    https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf
+
+properties:
+  compatible:
+    enum:
+      -maxim,max1241
+
+  reg:
+    maxItems: 1
+
+  vref-supply:
+    description:
+      Device tree identifier of the regulator that provides the external
+      reference voltage.
+    maxItems: 1
+
+  spi-max-frequency:
+    maxItems: 1
+
+  shdn-gpios:
+    description:
+      GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If
+      specified, the /SHDN pin will be asserted between conversions,
+      thus enabling power-down mode.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vref-supply
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    spi0 {
+        adc@0 {
+            compatible = "maxim,max1241";
+            reg = <0>;
+            vref-supply = <&vdd_3v3_reg>;
+            spi-max-frequency = <1000000>;
+            shdn-gpios = <&gpio 26 1>;
+        };
+    };
+
+
-- 
2.25.1

Re: [PATCH 2/2] dt-bindings: iio: adc: Add MAX1241 device tree bindings in documentation

[Ardelean, Alexandru]

On Tue, 2020-03-17 at 22:17 +0200, Alexandru Lazar wrote:
> [External]
> 
> Add device-tree bindings documentation for the MAX1241 device driver.
> 

curious: did you run dt_binding_check  ?

typically what i do is:

ARCH=arm make defconfig   # no idea why this is still needed for bindings check 
                          # i mean, i see the deps, but i don't feel is needed 
                          # ¯\_(ツ)_/¯

ARCH=arm make dt_binding_check \
    DT_SCHEMA_FILES=Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml

# should be on one line


# for dependencies, see 
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/writing-schema.rst#dependencies

It might be that Rob's bot will come back and tell us anything wrong with this
file.


> Signed-off-by: Alexandru Lazar <alazar@startmail.com>
> ---
>  .../bindings/iio/adc/maxim,max1241.yaml       | 60 +++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> new file mode 100644
> index 000000000000..abb90d382067
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7780.yaml#

This isn't AD7780 :p

adi,ad7780.yaml#  -> maxim,max1241.yaml#


> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX1241 12-bit, single-channel analog to digital converter
> +
> +maintainers:
> +  - Ioan-Alexandru Lazar <alazar@startmail.com>
> +
> +description: |
> +  Bindings for the max1241 12-bit, single-channel ADC device. This
> +  driver supports voltage reading and can optionally be configured for
> +  power-down mode operation. The datasheet can be found at:
> +    https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      -maxim,max1241
> +
> +  reg:
> +    maxItems: 1
> +
> +  vref-supply:
> +    description:
> +      Device tree identifier of the regulator that provides the external
> +      reference voltage.
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maxItems: 1

this could probably be omitted;
it's documented in some spi.yaml file and is considered a standard property

> +
> +  shdn-gpios:
> +    description:
> +      GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If
> +      specified, the /SHDN pin will be asserted between conversions,
> +      thus enabling power-down mode.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - vref-supply
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    spi0 {
> +        adc@0 {
> +            compatible = "maxim,max1241";
> +            reg = <0>;
> +            vref-supply = <&vdd_3v3_reg>;
> +            spi-max-frequency = <1000000>;
> +            shdn-gpios = <&gpio 26 1>;
> +        };
> +    };
> +
> +

Re: [PATCH 0/2] Maxim MAX1241 driver

[Lars-Peter Clausen]

On 3/17/20 9:17 PM, Alexandru Lazar wrote:
> Hello!
> 
> This patch series adds support for the Maxim MAX1241, a 12-bit,
> single-channel, SPI-connected ADC. It's a pretty sturdy ADC that I've
> used for some time now and I figured my driver may be useful to others
> as well.
> 
> I originally thought I might get this under the max1027 driver but the
> 124x family is different enough that shoehorning it in there didn't seem
> like a good idea. The 1241 has a single channel, no FIFO, uses a
> different mechanism to signal conversion start and has a low-power
> operation mode which it uses in a pretty different way. This is actually
> closer to MAX111x, but those also have a pretty different signaling
> convention.
> 
> Needless to say, though, if anyone who is more familiar with this
> situation disagrees and wants to point me in the direction of the
> appropriate driver, I'm happy to make the changes there instead of
> providing a separate driver.

I think your approach is the best approach here. Trying to build 
"generic" drivers that support lots of only partially related devices 
can get messy real quick.

> 
> Also please note that I am somewhat unfamiliar with the idioms of the
> IIO subsystem's tree. Jonathan Cameron was kind enough to give me a few
> pointers but there are a few questions I didn't bother him with (so I'm
> guess I'm gonna bother you instead now).
> 
> 1. There are two ADCs in this family, MAX1240 and MAX1241. This driver
> only supports the MAX1241. The scaffolding to get MAX1240 support is in
> there, but I didn't have access to a MAX1240 and I didn't want to submit
> untested code for review. Can we add MAX1240 support later, or should I
> do it from the very beginning? Either way is fine by me (I'm just a
> little concerned about how quickly I might *get* a MAX1240 these
> days...)
> 

Can be added later. Its always good to start with a small tested 
baseline. But either way it is up to you, no problem with having it 
supported by the initial patch either.

> 2. Looking at other drivers, it seems to me that, on ADCs that require
> an external reference, it's customary to make a voltage regulator a
> required property in the DT bindings. Am I correct here?

Yes.

> 
> 3. checkpatch.pl warns me that the MAINTAINERS file might need
> updating. I'm not sure what the appropriate thing to do is here -- I can
> maintain this driver indefinitely (I am actively using it anways) but
> it's a 200-line driver, does that warrant inclusion in MAINTAINERS?

Different people will have different options on this. My opinion is that 
it is not needed for small drivers. get_maintainers.pl will list your 
name anyway since you are the author for the commit.

> My apologies if anything here is distinctly bad -- this isn't the first
> time I'm writing kernel code but it's definitely the first time I'm
> sending ay upstream. Any bugs are due to my own incompetence, everything
> else is just temporary ignorance :-).

The driver's code looks very clean. Top job!

- Lars