[PATCH] iio: Add Maxim MAX11100 driver

[PATCH] iio: Add Maxim MAX11100 driver

[Jacopo Mondi]

Add IIO driver for Maxim MAX11100 single-channel ADC.
Support raw_read mode only.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/iio/adc/Kconfig    |   9 +++
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/max11100.c | 165 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 175 insertions(+)
 create mode 100644 drivers/iio/adc/max11100.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 99c0514..e2f3340 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -285,6 +285,15 @@ config MAX1027
 	  To compile this driver as a module, choose M here: the module will be
 	  called max1027.
 
+config MAX11100
+	tristate "Maxim max11100 ADC driver"
+	depends on SPI
+	help
+	  Say yes here to build support for Maxim 11100 SPI ADC
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called max11100.
+
 config MAX1363
 	tristate "Maxim max1363 ADC driver"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 7a40c04..1463044 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
 obj-$(CONFIG_LTC2485) += ltc2485.o
 obj-$(CONFIG_MAX1027) += max1027.o
+obj-$(CONFIG_MAX11100) += max11100.o
 obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_MCP3422) += mcp3422.o
diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
new file mode 100644
index 0000000..fbce287
--- /dev/null
+++ b/drivers/iio/adc/max11100.c
@@ -0,0 +1,165 @@
+/*
+ * iio/adc/max11100.c
+ * Maxim MAX11100 ADC Driver with IIO interface
+ *
+ * Copyright (C) 2016 Renesas Electronics Corporation
+ * Copyright (C) 2016 Jacopo Mondi
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/delay.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+
+struct max11100_state {
+	const struct max11100_chip_desc *desc;
+	struct spi_device *spi;
+	struct mutex lock;
+};
+
+static struct iio_chan_spec max11100_channels[] = {
+	{ /* [0] */
+		.type = IIO_VOLTAGE,
+		.channel = 0,
+		.address = 0,
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 24,
+			.shift = 8,
+			.repeat = 1,
+			.endianness = IIO_BE,
+		},
+		.output = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	},
+};
+
+static const unsigned long max11100_scan_masks[] = {
+	0xffff,
+};
+
+static struct max11100_chip_desc {
+	uint32_t num_chan;
+	const unsigned long *scanmasks;
+	const struct iio_chan_spec *channels;
+} max11100_desc = {
+	.num_chan = 1,
+	.scanmasks = max11100_scan_masks,
+	.channels = max11100_channels,
+};
+
+static int max11100_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	int ret;
+	struct max11100_state *state = iio_priv(indio_dev);
+	uint8_t buffer[3] = { 0, 0, 0 };
+
+	mutex_lock(&state->lock);
+
+	ret = spi_read(state->spi, (void *) buffer, 3);
+	if (ret) {
+		dev_err(&indio_dev->dev, "SPI transfer failed\n");
+		return ret;
+	}
+
+	dev_dbg(&indio_dev->dev, "ADC output values: "	\
+		"[0]: 0x%2x [1]: 0x%2x [2]: 0x%2x\n",
+		buffer[0], buffer[1], buffer[2]);
+
+	/* the first 8 bits sent out from ADC must be 0s */
+	if (buffer[0]) {
+		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
+		return -EINVAL;
+	}
+
+	*val = (buffer[1] << 8) | buffer[2];
+	mutex_unlock(&state->lock);
+
+	return 0;
+}
+
+static const struct iio_info max11100_info = {
+	.driver_module = THIS_MODULE,
+	.read_raw = max11100_read_raw,
+};
+
+static int max11100_probe(struct spi_device *spi)
+{
+	int ret;
+	struct iio_dev *indio_dev;
+	struct max11100_state *state;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
+	if (!indio_dev) {
+		pr_err("Can't allocate iio device\n");
+		return -ENOMEM;
+	}
+	dev_dbg(&indio_dev->dev, "probe max11100 IIO SPI\n");
+
+	spi_set_drvdata(spi, indio_dev);
+
+	state = iio_priv(indio_dev);
+	state->spi = spi;
+	state->desc = &max11100_desc;
+
+	mutex_init(&state->lock);
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->dev.of_node = spi->dev.of_node;
+	indio_dev->info = &max11100_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = state->desc->channels;
+	indio_dev->num_channels = state->desc->num_chan;
+	indio_dev->available_scan_masks = state->desc->scanmasks;
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(&indio_dev->dev, "Failed to register iio device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int max11100_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+
+	dev_dbg(&indio_dev->dev, "remove max11100 IIO SPI\n");
+
+	iio_device_unregister(indio_dev);
+
+	return 0;
+}
+
+static const struct of_device_id max11100_ids[] = {
+	{.compatible = "maxim,max11100"},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, max11100_ids);
+
+static struct spi_driver max11100_driver = {
+	.driver = {
+		.name	= "max11100",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(max11100_ids),
+	},
+	.probe		= max11100_probe,
+	.remove		= max11100_remove,
+};
+
+module_spi_driver(max11100_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
+MODULE_DESCRIPTION("Maxim MAX11100 ADC Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH] iio: Add Maxim MAX11100 driver

[Peter Meerwald-Stadler]

On Tue, 6 Dec 2016, Jacopo Mondi wrote:

> Add IIO driver for Maxim MAX11100 single-channel ADC.
> Support raw_read mode only.

comments below; very minimal driver, but several easy issues...

the read_raw() is supposed to return millivolts (after application of 
offset and scale); maybe need _SCALE?
 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/iio/adc/Kconfig    |   9 +++
>  drivers/iio/adc/Makefile   |   1 +
>  drivers/iio/adc/max11100.c | 165 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 175 insertions(+)
>  create mode 100644 drivers/iio/adc/max11100.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 99c0514..e2f3340 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -285,6 +285,15 @@ config MAX1027
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max1027.
>  
> +config MAX11100
> +	tristate "Maxim max11100 ADC driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build support for Maxim 11100 SPI ADC

Maxim max11100

> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max11100.
> +
>  config MAX1363
>  	tristate "Maxim max1363 ADC driver"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 7a40c04..1463044 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
>  obj-$(CONFIG_MAX1027) += max1027.o
> +obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
> new file mode 100644
> index 0000000..fbce287
> --- /dev/null
> +++ b/drivers/iio/adc/max11100.c
> @@ -0,0 +1,165 @@
> +/*
> + * iio/adc/max11100.c
> + * Maxim MAX11100 ADC Driver with IIO interface

MAX11100 or max11100

> + *
> + * Copyright (C) 2016 Renesas Electronics Corporation
> + * Copyright (C) 2016 Jacopo Mondi
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/delay.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +
> +struct max11100_state {
> +	const struct max11100_chip_desc *desc;
> +	struct spi_device *spi;
> +	struct mutex lock;
> +};
> +
> +static struct iio_chan_spec max11100_channels[] = {
> +	{ /* [0] */
> +		.type = IIO_VOLTAGE,
> +		.channel = 0,

not indexed, so channel = 0 not needed

> +		.address = 0,

address not needed (and no need to initialize to 0 anyway)

> +		.scan_index = 0,

scan_index and scan_type not needed since no buffered support (yet); add 
this when needed

> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 24,
> +			.shift = 8,
> +			.repeat = 1,
> +			.endianness = IIO_BE,
> +		},
> +		.output = 1,

no, ADC is typically not an output channel 

> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	},
> +};
> +
> +static const unsigned long max11100_scan_masks[] = {

scan_masks not needed since no buffered support

> +	0xffff,
> +};
> +
> +static struct max11100_chip_desc {
> +	uint32_t num_chan;

why not just unsigned?

> +	const unsigned long *scanmasks;

not needed (yet)

> +	const struct iio_chan_spec *channels;
> +} max11100_desc = {
> +	.num_chan = 1,

ARRAY_SIZE(max11100_channels)

> +	.scanmasks = max11100_scan_masks,
> +	.channels = max11100_channels,
> +};
> +
> +static int max11100_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	int ret;
> +	struct max11100_state *state = iio_priv(indio_dev);
> +	uint8_t buffer[3] = { 0, 0, 0 };

no need to initialize buffer; consider alignment requirements for SPI

> +
> +	mutex_lock(&state->lock);

what is the mutex protecting?
must the spi_read() by protected? then the unlock can be placed right 
after the call...

> +
> +	ret = spi_read(state->spi, (void *) buffer, 3);

sizeof(buffer)

> +	if (ret) {
> +		dev_err(&indio_dev->dev, "SPI transfer failed\n");

mutex_unlock()!?

> +		return ret;
> +	}
> +
> +	dev_dbg(&indio_dev->dev, "ADC output values: "	\
> +		"[0]: 0x%2x [1]: 0x%2x [2]: 0x%2x\n",

probably 0x%02x if really needed

> +		buffer[0], buffer[1], buffer[2]);
> +
> +	/* the first 8 bits sent out from ADC must be 0s */
> +	if (buffer[0]) {
> +		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");

mutex_unlock()!?

> +		return -EINVAL;
> +	}
> +
> +	*val = (buffer[1] << 8) | buffer[2];
> +	mutex_unlock(&state->lock);
> +

shouldn't this return IIO_VAL_INT?

> +	return 0;
> +}
> +
> +static const struct iio_info max11100_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = max11100_read_raw,
> +};
> +
> +static int max11100_probe(struct spi_device *spi)
> +{
> +	int ret;
> +	struct iio_dev *indio_dev;
> +	struct max11100_state *state;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +	if (!indio_dev) {
> +		pr_err("Can't allocate iio device\n");

error message really needed?

> +		return -ENOMEM;
> +	}
> +	dev_dbg(&indio_dev->dev, "probe max11100 IIO SPI\n");

message really needed?

> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	state = iio_priv(indio_dev);
> +	state->spi = spi;
> +	state->desc = &max11100_desc;
> +
> +	mutex_init(&state->lock);
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->dev.of_node = spi->dev.of_node;
> +	indio_dev->info = &max11100_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = state->desc->channels;
> +	indio_dev->num_channels = state->desc->num_chan;
> +	indio_dev->available_scan_masks = state->desc->scanmasks;
> +
> +	ret = iio_device_register(indio_dev);

could use devm_ variant...

> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Failed to register iio device\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int max11100_remove(struct spi_device *spi)

function not needed if using devm_iio_device_register()

> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +
> +	dev_dbg(&indio_dev->dev, "remove max11100 IIO SPI\n");
> +
> +	iio_device_unregister(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id max11100_ids[] = {
> +	{.compatible = "maxim,max11100"},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, max11100_ids);
> +
> +static struct spi_driver max11100_driver = {
> +	.driver = {
> +		.name	= "max11100",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(max11100_ids),
> +	},
> +	.probe		= max11100_probe,
> +	.remove		= max11100_remove,
> +};
> +
> +module_spi_driver(max11100_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Maxim MAX11100 ADC Driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH] iio: Add Maxim MAX11100 driver

[Geert Uytterhoeven]

On Tue, Dec 6, 2016 at 10:00 PM, Peter Meerwald-Stadler
<pmeerw@pmeerw.net> wrote:
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig

>> +static int max11100_read_raw(struct iio_dev *indio_dev,
>> +                          struct iio_chan_spec const *chan,
>> +                          int *val, int *val2, long mask)
>> +{
>> +     int ret;
>> +     struct max11100_state *state = iio_priv(indio_dev);
>> +     uint8_t buffer[3] = { 0, 0, 0 };

>> +     ret = spi_read(state->spi, (void *) buffer, 3);

Cast not needed.

>> +static int max11100_probe(struct spi_device *spi)
>> +{
>> +     int ret;
>> +     struct iio_dev *indio_dev;
>> +     struct max11100_state *state;
>> +
>> +     indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
>> +     if (!indio_dev) {
>> +             pr_err("Can't allocate iio device\n");
>
> error message really needed?

No, OOM will scream anyway.

>
>> +             return -ENOMEM;
>> +     }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] iio: Add Maxim MAX11100 driver

[jacopo]

Hi Peter,
    thanks for review

On 06/12/2016 22:00, Peter Meerwald-Stadler wrote:
> On Tue, 6 Dec 2016, Jacopo Mondi wrote:
>
>> Add IIO driver for Maxim MAX11100 single-channel ADC.
>> Support raw_read mode only.
>
> comments below; very minimal driver, but several easy issues...
>
> the read_raw() is supposed to return millivolts (after application of
> offset and scale); maybe need _SCALE?

How can I return millivolts here? They vary in function of the supplied 
Vref as ( V = val * Vref / (2 ^ 16 - 1)) where "val" is digital value 
output by the ADC.
Since Vref can range from 3.8V to 5.25V, while it's typically 4.096V, 
isn't it more appropriate to let userspace perform conversion to 
millivolts, as it knows what Vref value is supplied to the ADC?

>
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>  drivers/iio/adc/Kconfig    |   9 +++
>>  drivers/iio/adc/Makefile   |   1 +
>>  drivers/iio/adc/max11100.c | 165 +++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 175 insertions(+)
>>  create mode 100644 drivers/iio/adc/max11100.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 99c0514..e2f3340 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -285,6 +285,15 @@ config MAX1027
>>  	  To compile this driver as a module, choose M here: the module will be
>>  	  called max1027.
>>
>> +config MAX11100
>> +	tristate "Maxim max11100 ADC driver"
>> +	depends on SPI
>> +	help
>> +	  Say yes here to build support for Maxim 11100 SPI ADC
>
> Maxim max11100
>
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called max11100.
>> +
>>  config MAX1363
>>  	tristate "Maxim max1363 ADC driver"
>>  	depends on I2C
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 7a40c04..1463044 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>>  obj-$(CONFIG_LTC2485) += ltc2485.o
>>  obj-$(CONFIG_MAX1027) += max1027.o
>> +obj-$(CONFIG_MAX11100) += max11100.o
>>  obj-$(CONFIG_MAX1363) += max1363.o
>>  obj-$(CONFIG_MCP320X) += mcp320x.o
>>  obj-$(CONFIG_MCP3422) += mcp3422.o
>> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
>> new file mode 100644
>> index 0000000..fbce287
>> --- /dev/null
>> +++ b/drivers/iio/adc/max11100.c
>> @@ -0,0 +1,165 @@
>> +/*
>> + * iio/adc/max11100.c
>> + * Maxim MAX11100 ADC Driver with IIO interface
>
> MAX11100 or max11100
>
>> + *
>> + * Copyright (C) 2016 Renesas Electronics Corporation
>> + * Copyright (C) 2016 Jacopo Mondi
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/delay.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +
>> +struct max11100_state {
>> +	const struct max11100_chip_desc *desc;
>> +	struct spi_device *spi;
>> +	struct mutex lock;
>> +};
>> +
>> +static struct iio_chan_spec max11100_channels[] = {
>> +	{ /* [0] */
>> +		.type = IIO_VOLTAGE,
>> +		.channel = 0,
>
> not indexed, so channel = 0 not needed
>
>> +		.address = 0,
>
> address not needed (and no need to initialize to 0 anyway)
>
>> +		.scan_index = 0,
>
> scan_index and scan_type not needed since no buffered support (yet); add
> this when needed
>
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 24,
>> +			.shift = 8,
>> +			.repeat = 1,
>> +			.endianness = IIO_BE,
>> +		},
>> +		.output = 1,
>
> no, ADC is typically not an output channel
>
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +	},
>> +};
>> +
>> +static const unsigned long max11100_scan_masks[] = {
>
> scan_masks not needed since no buffered support
>
>> +	0xffff,
>> +};
>> +
>> +static struct max11100_chip_desc {
>> +	uint32_t num_chan;
>
> why not just unsigned?
>
>> +	const unsigned long *scanmasks;
>
> not needed (yet)
>
>> +	const struct iio_chan_spec *channels;
>> +} max11100_desc = {
>> +	.num_chan = 1,
>
> ARRAY_SIZE(max11100_channels)
>
>> +	.scanmasks = max11100_scan_masks,
>> +	.channels = max11100_channels,
>> +};
>> +
>> +static int max11100_read_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int *val, int *val2, long mask)
>> +{
>> +	int ret;
>> +	struct max11100_state *state = iio_priv(indio_dev);
>> +	uint8_t buffer[3] = { 0, 0, 0 };
>
> no need to initialize buffer; consider alignment requirements for SPI
>
>> +
>> +	mutex_lock(&state->lock);
>
> what is the mutex protecting?
> must the spi_read() by protected? then the unlock can be placed right
> after the call...
>
>> +
>> +	ret = spi_read(state->spi, (void *) buffer, 3);
>
> sizeof(buffer)
>
>> +	if (ret) {
>> +		dev_err(&indio_dev->dev, "SPI transfer failed\n");
>
> mutex_unlock()!?
>
>> +		return ret;
>> +	}
>> +
>> +	dev_dbg(&indio_dev->dev, "ADC output values: "	\
>> +		"[0]: 0x%2x [1]: 0x%2x [2]: 0x%2x\n",
>
> probably 0x%02x if really needed
>
>> +		buffer[0], buffer[1], buffer[2]);
>> +
>> +	/* the first 8 bits sent out from ADC must be 0s */
>> +	if (buffer[0]) {
>> +		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
>
> mutex_unlock()!?

This is embarrassing. It got lost during last rebase, sorry about that.

>
>> +		return -EINVAL;
>> +	}
>> +
>> +	*val = (buffer[1] << 8) | buffer[2];
>> +	mutex_unlock(&state->lock);
>> +
>
> shouldn't this return IIO_VAL_INT?
>
>> +	return 0;
>> +}
>> +
>> +static const struct iio_info max11100_info = {
>> +	.driver_module = THIS_MODULE,
>> +	.read_raw = max11100_read_raw,
>> +};
>> +
>> +static int max11100_probe(struct spi_device *spi)
>> +{
>> +	int ret;
>> +	struct iio_dev *indio_dev;
>> +	struct max11100_state *state;
>> +
>> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
>> +	if (!indio_dev) {
>> +		pr_err("Can't allocate iio device\n");
>
> error message really needed?
>
>> +		return -ENOMEM;
>> +	}
>> +	dev_dbg(&indio_dev->dev, "probe max11100 IIO SPI\n");
>
> message really needed?
>
>> +
>> +	spi_set_drvdata(spi, indio_dev);
>> +
>> +	state = iio_priv(indio_dev);
>> +	state->spi = spi;
>> +	state->desc = &max11100_desc;
>> +
>> +	mutex_init(&state->lock);
>> +
>> +	indio_dev->dev.parent = &spi->dev;
>> +	indio_dev->dev.of_node = spi->dev.of_node;
>> +	indio_dev->info = &max11100_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = state->desc->channels;
>> +	indio_dev->num_channels = state->desc->num_chan;
>> +	indio_dev->available_scan_masks = state->desc->scanmasks;
>> +
>> +	ret = iio_device_register(indio_dev);
>
> could use devm_ variant...
>
>> +	if (ret) {
>> +		dev_err(&indio_dev->dev, "Failed to register iio device\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int max11100_remove(struct spi_device *spi)
>
> function not needed if using devm_iio_device_register()
>
>> +{
>> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +
>> +	dev_dbg(&indio_dev->dev, "remove max11100 IIO SPI\n");
>> +
>> +	iio_device_unregister(indio_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id max11100_ids[] = {
>> +	{.compatible = "maxim,max11100"},
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, max11100_ids);
>> +
>> +static struct spi_driver max11100_driver = {
>> +	.driver = {
>> +		.name	= "max11100",
>> +		.owner	= THIS_MODULE,
>> +		.of_match_table = of_match_ptr(max11100_ids),
>> +	},
>> +	.probe		= max11100_probe,
>> +	.remove		= max11100_remove,
>> +};
>> +
>> +module_spi_driver(max11100_driver);
>> +
>> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
>> +MODULE_DESCRIPTION("Maxim MAX11100 ADC Driver");
>> +MODULE_LICENSE("GPL v2");
>>
>

This driver is so minimal, I wonder if there are not drivers for similar 
devices to which I can add support for MAX11100 instead of writing a new 
one from scratch.
Anyone with more expertise than me in IIO codebase maybe can suggest 
something here...

Thanks
    j

Re: [PATCH] iio: Add Maxim MAX11100 driver

[Geert Uytterhoeven]

Hi Jacopo,

On Wed, Dec 7, 2016 at 9:29 AM, jacopo@jmondi.org <jacopo@jmondi.org> wrote:
> On 06/12/2016 22:00, Peter Meerwald-Stadler wrote:
>> On Tue, 6 Dec 2016, Jacopo Mondi wrote:
>>> Add IIO driver for Maxim MAX11100 single-channel ADC.
>>> Support raw_read mode only.
>>
>> the read_raw() is supposed to return millivolts (after application of
>> offset and scale); maybe need _SCALE?
>
> How can I return millivolts here? They vary in function of the supplied Vref
> as ( V = val * Vref / (2 ^ 16 - 1)) where "val" is digital value output by
> the ADC.
> Since Vref can range from 3.8V to 5.25V, while it's typically 4.096V, isn't
> it more appropriate to let userspace perform conversion to millivolts, as it
> knows what Vref value is supplied to the ADC?

Specify Vref in DT?

Which finally gives a good reason to add a DT binding document for
MAX11100 ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] iio: Add Maxim MAX11100 driver

[Jonathan Cameron]

On 07/12/16 12:22, Geert Uytterhoeven wrote:
> Hi Jacopo,
> 
> On Wed, Dec 7, 2016 at 9:29 AM, jacopo@jmondi.org <jacopo@jmondi.org> wrote:
>> On 06/12/2016 22:00, Peter Meerwald-Stadler wrote:
>>> On Tue, 6 Dec 2016, Jacopo Mondi wrote:
>>>> Add IIO driver for Maxim MAX11100 single-channel ADC.
>>>> Support raw_read mode only.
>>>
>>> the read_raw() is supposed to return millivolts (after application of
>>> offset and scale); maybe need _SCALE?
>>
>> How can I return millivolts here? They vary in function of the supplied Vref
>> as ( V = val * Vref / (2 ^ 16 - 1)) where "val" is digital value output by
>> the ADC.
>> Since Vref can range from 3.8V to 5.25V, while it's typically 4.096V, isn't
>> it more appropriate to let userspace perform conversion to millivolts, as it
>> knows what Vref value is supplied to the ADC?
> 
> Specify Vref in DT?
More specifically specify that it's supplied by a regulator.. 
If it's a fixed supply then DT should describe it as a fixed regulator.
> 
> Which finally gives a good reason to add a DT binding document for
> MAX11100 ;-)
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

Re: [PATCH] iio: Add Maxim MAX11100 driver

[Jonathan Cameron]

On 07/12/16 08:29, jacopo@jmondi.org wrote:
> Hi Peter,
>    thanks for review
> 
> On 06/12/2016 22:00, Peter Meerwald-Stadler wrote:
>> On Tue, 6 Dec 2016, Jacopo Mondi wrote:
>>
>>> Add IIO driver for Maxim MAX11100 single-channel ADC.
>>> Support raw_read mode only.
>>
>> comments below; very minimal driver, but several easy issues...
>>
>> the read_raw() is supposed to return millivolts (after application of
>> offset and scale); maybe need _SCALE?
> 
> How can I return millivolts here? They vary in function of the supplied Vref as ( V = val * Vref / (2 ^ 16 - 1)) where "val" is digital value output by the ADC.
> Since Vref can range from 3.8V to 5.25V, while it's typically 4.096V, isn't it more appropriate to let userspace perform conversion to millivolts, as it knows what Vref value is supplied to the ADC?
> 
>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  drivers/iio/adc/Kconfig    |   9 +++
>>>  drivers/iio/adc/Makefile   |   1 +
>>>  drivers/iio/adc/max11100.c | 165 +++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 175 insertions(+)
>>>  create mode 100644 drivers/iio/adc/max11100.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 99c0514..e2f3340 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -285,6 +285,15 @@ config MAX1027
>>>        To compile this driver as a module, choose M here: the module will be
>>>        called max1027.
>>>
>>> +config MAX11100
>>> +    tristate "Maxim max11100 ADC driver"
>>> +    depends on SPI
>>> +    help
>>> +      Say yes here to build support for Maxim 11100 SPI ADC
>>
>> Maxim max11100
>>
>>> +
>>> +      To compile this driver as a module, choose M here: the module will be
>>> +      called max11100.
>>> +
>>>  config MAX1363
>>>      tristate "Maxim max1363 ADC driver"
>>>      depends on I2C
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index 7a40c04..1463044 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>>>  obj-$(CONFIG_LTC2485) += ltc2485.o
>>>  obj-$(CONFIG_MAX1027) += max1027.o
>>> +obj-$(CONFIG_MAX11100) += max11100.o
>>>  obj-$(CONFIG_MAX1363) += max1363.o
>>>  obj-$(CONFIG_MCP320X) += mcp320x.o
>>>  obj-$(CONFIG_MCP3422) += mcp3422.o
>>> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
>>> new file mode 100644
>>> index 0000000..fbce287
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/max11100.c
>>> @@ -0,0 +1,165 @@
>>> +/*
>>> + * iio/adc/max11100.c
>>> + * Maxim MAX11100 ADC Driver with IIO interface
>>
>> MAX11100 or max11100
>>
>>> + *
>>> + * Copyright (C) 2016 Renesas Electronics Corporation
>>> + * Copyright (C) 2016 Jacopo Mondi
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/spi/spi.h>
>>> +#include <linux/delay.h>
>>> +
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/buffer.h>
>>> +
>>> +struct max11100_state {
>>> +    const struct max11100_chip_desc *desc;
>>> +    struct spi_device *spi;
>>> +    struct mutex lock;
>>> +};
>>> +
>>> +static struct iio_chan_spec max11100_channels[] = {
>>> +    { /* [0] */
>>> +        .type = IIO_VOLTAGE,
>>> +        .channel = 0,
>>
>> not indexed, so channel = 0 not needed
>>
>>> +        .address = 0,
>>
>> address not needed (and no need to initialize to 0 anyway)
>>
>>> +        .scan_index = 0,
>>
>> scan_index and scan_type not needed since no buffered support (yet); add
>> this when needed
>>
>>> +        .scan_type = {
>>> +            .sign = 'u',
>>> +            .realbits = 16,
>>> +            .storagebits = 24,
>>> +            .shift = 8,
>>> +            .repeat = 1,
>>> +            .endianness = IIO_BE,
>>> +        },
>>> +        .output = 1,
>>
>> no, ADC is typically not an output channel
>>
>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>> +    },
>>> +};
>>> +
>>> +static const unsigned long max11100_scan_masks[] = {
>>
>> scan_masks not needed since no buffered support
>>
>>> +    0xffff,
>>> +};
>>> +
>>> +static struct max11100_chip_desc {
>>> +    uint32_t num_chan;
>>
>> why not just unsigned?
>>
>>> +    const unsigned long *scanmasks;
>>
>> not needed (yet)
>>
>>> +    const struct iio_chan_spec *channels;
>>> +} max11100_desc = {
>>> +    .num_chan = 1,
>>
>> ARRAY_SIZE(max11100_channels)
>>
>>> +    .scanmasks = max11100_scan_masks,
>>> +    .channels = max11100_channels,
>>> +};
>>> +
>>> +static int max11100_read_raw(struct iio_dev *indio_dev,
>>> +                 struct iio_chan_spec const *chan,
>>> +                 int *val, int *val2, long mask)
>>> +{
>>> +    int ret;
>>> +    struct max11100_state *state = iio_priv(indio_dev);
>>> +    uint8_t buffer[3] = { 0, 0, 0 };
>>
>> no need to initialize buffer; consider alignment requirements for SPI
>>
>>> +
>>> +    mutex_lock(&state->lock);
>>
>> what is the mutex protecting?
>> must the spi_read() by protected? then the unlock can be placed right
>> after the call...
>>
>>> +
>>> +    ret = spi_read(state->spi, (void *) buffer, 3);
>>
>> sizeof(buffer)
>>
>>> +    if (ret) {
>>> +        dev_err(&indio_dev->dev, "SPI transfer failed\n");
>>
>> mutex_unlock()!?
>>
>>> +        return ret;
>>> +    }
>>> +
>>> +    dev_dbg(&indio_dev->dev, "ADC output values: "    \
>>> +        "[0]: 0x%2x [1]: 0x%2x [2]: 0x%2x\n",
>>
>> probably 0x%02x if really needed
>>
>>> +        buffer[0], buffer[1], buffer[2]);
>>> +
>>> +    /* the first 8 bits sent out from ADC must be 0s */
>>> +    if (buffer[0]) {
>>> +        dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
>>
>> mutex_unlock()!?
> 
> This is embarrassing. It got lost during last rebase, sorry about that.
> 
>>
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    *val = (buffer[1] << 8) | buffer[2];
>>> +    mutex_unlock(&state->lock);
>>> +
>>
>> shouldn't this return IIO_VAL_INT?
>>
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct iio_info max11100_info = {
>>> +    .driver_module = THIS_MODULE,
>>> +    .read_raw = max11100_read_raw,
>>> +};
>>> +
>>> +static int max11100_probe(struct spi_device *spi)
>>> +{
>>> +    int ret;
>>> +    struct iio_dev *indio_dev;
>>> +    struct max11100_state *state;
>>> +
>>> +    indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
>>> +    if (!indio_dev) {
>>> +        pr_err("Can't allocate iio device\n");
>>
>> error message really needed?
>>
>>> +        return -ENOMEM;
>>> +    }
>>> +    dev_dbg(&indio_dev->dev, "probe max11100 IIO SPI\n");
>>
>> message really needed?
>>
>>> +
>>> +    spi_set_drvdata(spi, indio_dev);
>>> +
>>> +    state = iio_priv(indio_dev);
>>> +    state->spi = spi;
>>> +    state->desc = &max11100_desc;
>>> +
>>> +    mutex_init(&state->lock);
>>> +
>>> +    indio_dev->dev.parent = &spi->dev;
>>> +    indio_dev->dev.of_node = spi->dev.of_node;
>>> +    indio_dev->info = &max11100_info;
>>> +    indio_dev->modes = INDIO_DIRECT_MODE;
>>> +    indio_dev->channels = state->desc->channels;
>>> +    indio_dev->num_channels = state->desc->num_chan;
>>> +    indio_dev->available_scan_masks = state->desc->scanmasks;
>>> +
>>> +    ret = iio_device_register(indio_dev);
>>
>> could use devm_ variant...
>>
>>> +    if (ret) {
>>> +        dev_err(&indio_dev->dev, "Failed to register iio device\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int max11100_remove(struct spi_device *spi)
>>
>> function not needed if using devm_iio_device_register()
>>
>>> +{
>>> +    struct iio_dev *indio_dev = spi_get_drvdata(spi);
>>> +
>>> +    dev_dbg(&indio_dev->dev, "remove max11100 IIO SPI\n");
>>> +
>>> +    iio_device_unregister(indio_dev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct of_device_id max11100_ids[] = {
>>> +    {.compatible = "maxim,max11100"},
>>> +    { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, max11100_ids);
>>> +
>>> +static struct spi_driver max11100_driver = {
>>> +    .driver = {
>>> +        .name    = "max11100",
>>> +        .owner    = THIS_MODULE,
>>> +        .of_match_table = of_match_ptr(max11100_ids),
>>> +    },
>>> +    .probe        = max11100_probe,
>>> +    .remove        = max11100_remove,
>>> +};
>>> +
>>> +module_spi_driver(max11100_driver);
>>> +
>>> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
>>> +MODULE_DESCRIPTION("Maxim MAX11100 ADC Driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
> 
> This driver is so minimal, I wonder if there are not drivers for
> similar devices to which I can add support for MAX11100 instead of
> writing a new one from scratch. Anyone with more expertise than me in
> IIO codebase maybe can suggest something here...
Could possibly blugeon it into the ad7476 code, but it wouldn't be pretty...
Some of the SPI TI parts perhaps, but again you'll end up with a fair refactoring of
the existing drivers to get it in.

Seems simple I agree, but somehow there are an awful lot of simple ways to interface
and ADC via a couple of spi transfers!

Jonathan
> Thanks
>    j
> 
> 

Re: [PATCH] iio: Add Maxim MAX11100 driver

[jacopo]

Hi Jonathan,

On 10/12/2016 19:04, Jonathan Cameron wrote:
> On 07/12/16 08:29, jacopo@jmondi.org wrote:
>>

  [snip]

>> This driver is so minimal, I wonder if there are not drivers for
>> similar devices to which I can add support for MAX11100 instead of
>> writing a new one from scratch. Anyone with more expertise than me in
>> IIO codebase maybe can suggest something here...
> Could possibly blugeon it into the ad7476 code, but it wouldn't be pretty...
> Some of the SPI TI parts perhaps, but again you'll end up with a fair refactoring of
> the existing drivers to get it in.
>
> Seems simple I agree, but somehow there are an awful lot of simple ways to interface
> and ADC via a couple of spi transfers!

Yes there are :)
I'll continue trying to submit this driver alone then as it seems 
merging with existing ones is not feasible at this time!

Thanks
    j


>
> Jonathan
>> Thanks
>>    j
>>
>>
>

[PATCHv2] iio: adc: Add Maxim MAX11100 driver

[Jacopo Mondi]

Add IIO driver for Maxim MAX11100 single-channel ADC.
Add DT bindings documentation.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---

v1 -> v2:
    - incorporated pmeerw's review comments
    - retrieve vref from dts and use that to convert read_raw result
      to mV
    - add device tree bindings documentation

---
 .../devicetree/bindings/iio/adc/max11100.txt       |  17 +++
 drivers/iio/adc/Kconfig                            |   9 ++
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/max11100.c                         | 166 +++++++++++++++++++++
 4 files changed, 193 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
 create mode 100644 drivers/iio/adc/max11100.c

diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt b/Documentation/devicetree/bindings/iio/adc/max11100.txt
new file mode 100644
index 0000000..6877c11
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
@@ -0,0 +1,17 @@
+* Maxim max11100 Analog to Digital Converter (ADC)
+
+Required properties:
+  - compatible: Should be "maxim,max11100"
+  - vref-supply: phandle to the regulator that provides reference voltage
+
+Optional properties:
+  - spi-max-frequency: SPI maximum frequency
+
+Example:
+
+adc0: max11100@0 {
+        compatible = "maxim,max11100";
+        vref-supply = <&adc0_vref>;
+        spi-max-frequency = <240000>;
+};
+
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 99c0514..a909484 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -285,6 +285,15 @@ config MAX1027
 	  To compile this driver as a module, choose M here: the module will be
 	  called max1027.
 
+config MAX11100
+	tristate "Maxim max11100 ADC driver"
+	depends on SPI
+	help
+	  Say yes here to build support for Maxim max11100 SPI ADC
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called max11100.
+
 config MAX1363
 	tristate "Maxim max1363 ADC driver"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 7a40c04..1463044 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
 obj-$(CONFIG_LTC2485) += ltc2485.o
 obj-$(CONFIG_MAX1027) += max1027.o
+obj-$(CONFIG_MAX11100) += max11100.o
 obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_MCP3422) += mcp3422.o
diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
new file mode 100644
index 0000000..f372ad8
--- /dev/null
+++ b/drivers/iio/adc/max11100.c
@@ -0,0 +1,166 @@
+/*
+ * iio/adc/max11100.c
+ * Maxim max11100 ADC Driver with IIO interface
+ *
+ * Copyright (C) 2016 Renesas Electronics Corporation
+ * Copyright (C) 2016 Jacopo Mondi
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/driver.h>
+
+/*
+ * LSB is the ADC single digital step
+ * 1 LSB = (vref / 2 ^ 16)
+ * AIN = (DIN * LSB)
+ */
+#define MAX11100_LSB_DIV		(1 << 16)
+#define MAX11100_LSB(vref) 		(vref / MAX11100_LSB_DIV)
+
+struct max11100_state {
+	const struct max11100_chip_desc *desc;
+	struct spi_device *spi;
+	int vref_uv;
+	struct mutex lock;
+};
+
+static struct iio_chan_spec max11100_channels[] = {
+	{ /* [0] */
+		.type = IIO_VOLTAGE,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 24,
+			.shift = 8,
+			.repeat = 1,
+			.endianness = IIO_BE,
+		},
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	},
+};
+
+static struct max11100_chip_desc {
+	unsigned int num_chan;
+	const struct iio_chan_spec *channels;
+} max11100_desc = {
+	.num_chan = ARRAY_SIZE(max11100_channels),
+	.channels = max11100_channels,
+};
+
+static int max11100_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	int ret;
+	struct max11100_state *state = iio_priv(indio_dev);
+	uint8_t buffer[3];
+
+	mutex_lock(&state->lock);
+
+	ret = spi_read(state->spi, buffer, sizeof(buffer));
+	if (ret) {
+		mutex_unlock(&state->lock);
+		dev_err(&indio_dev->dev, "SPI transfer failed\n");
+		return ret;
+	}
+	mutex_unlock(&state->lock);
+
+	/* the first 8 bits sent out from ADC must be 0s */
+	if (buffer[0]) {
+		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
+		return -EINVAL;
+	}
+
+	*val = be16_to_cpu(*(uint16_t *)&buffer[1]);
+	*val = *val * MAX11100_LSB(state->vref_uv) / 1000;
+
+	return IIO_VAL_INT;
+}
+
+static const struct iio_info max11100_info = {
+	.driver_module = THIS_MODULE,
+	.read_raw = max11100_read_raw,
+};
+
+static int max11100_probe(struct spi_device *spi)
+{
+	int ret;
+	struct iio_dev *indio_dev;
+	struct regulator *vref_reg;
+	struct max11100_state *state;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, indio_dev);
+
+	state = iio_priv(indio_dev);
+	state->spi = spi;
+	state->desc = &max11100_desc;
+
+	mutex_init(&state->lock);
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->dev.of_node = spi->dev.of_node;
+	indio_dev->info = &max11100_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = state->desc->channels;
+	indio_dev->num_channels = state->desc->num_chan;
+
+	vref_reg = devm_regulator_get(&spi->dev, "vref");
+	if (IS_ERR(vref_reg))
+		return PTR_ERR(vref_reg);
+
+	ret = regulator_enable(vref_reg);
+	if (ret)
+		return ret;
+
+	state->vref_uv = regulator_get_voltage(vref_reg);
+	if (state->vref_uv < 0) {
+		/* dummy regulator "get_voltage" returns -EINVAL as well */
+		ret = -EINVAL;
+		goto disable_regulator;
+	}
+
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret)
+		goto disable_regulator;
+
+	return 0;
+
+disable_regulator:
+	regulator_disable(vref_reg);
+	return ret;
+}
+
+static const struct of_device_id max11100_ids[] = {
+	{.compatible = "maxim,max11100"},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, max11100_ids);
+
+static struct spi_driver max11100_driver = {
+	.driver = {
+		.name	= "max11100",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(max11100_ids),
+	},
+	.probe		= max11100_probe,
+};
+
+module_spi_driver(max11100_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
+MODULE_DESCRIPTION("Maxim max11100 ADC Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[Geert Uytterhoeven]

Hi Jacopo,

On Tue, Dec 13, 2016 at 1:37 PM, Jacopo Mondi <jacopo@jmondi.org> wrote:
> Add IIO driver for Maxim MAX11100 single-channel ADC.
> Add DT bindings documentation.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
>  obj-$(CONFIG_MAX1027) += max1027.o
> +obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
> new file mode 100644
> index 0000000..f372ad8
> --- /dev/null
> +++ b/drivers/iio/adc/max11100.c

> +/*
> + * LSB is the ADC single digital step
> + * 1 LSB = (vref / 2 ^ 16)
> + * AIN = (DIN * LSB)
> + */
> +#define MAX11100_LSB_DIV               (1 << 16)
> +#define MAX11100_LSB(vref)             (vref / MAX11100_LSB_DIV)

DIV_ROUND_CLOSEST()?

> +struct max11100_state {
> +       const struct max11100_chip_desc *desc;
> +       struct spi_device *spi;
> +       int vref_uv;

unsi
It's good practice to move the smaller members to the end of the structure,
to avoid gaps due to alignment rules (struct mutex needs to be 64-bit aligned
on 64-bit platforms).

> +       struct mutex lock;
> +};

> +static struct max11100_chip_desc {
> +       unsigned int num_chan;
> +       const struct iio_chan_spec *channels;

Same here (but it won't have any effect for now).

> +} max11100_desc = {
> +       .num_chan = ARRAY_SIZE(max11100_channels),
> +       .channels = max11100_channels,
> +};

> +static int max11100_read_raw(struct iio_dev *indio_dev,
> +                            struct iio_chan_spec const *chan,
> +                            int *val, int *val2, long mask)
> +{
> +       int ret;
> +       struct max11100_state *state = iio_priv(indio_dev);
> +       uint8_t buffer[3];

> +       *val = be16_to_cpu(*(uint16_t *)&buffer[1]);

Reading the uint16_t will be an unaligned load, which is not supported on all
platforms. So you either have to use get_unaligned_le16(), or assemble the
value yourself, like you did in v1.

> +       *val = *val * MAX11100_LSB(state->vref_uv) / 1000;

DIV_ROUND_CLOSEST()?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]


> > +struct max11100_state {
> > +       const struct max11100_chip_desc *desc;
> > +       struct spi_device *spi;
> > +       int vref_uv;
> 
> unsi
> It's good practice to move the smaller members to the end of the structure,
> to avoid gaps due to alignment rules (struct mutex needs to be 64-bit aligned
> on 64-bit platforms).

One option. Another idea is to put the most used members to the front to
increase chances of being in the same cacheline.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[Peter Meerwald-Stadler]

> Add IIO driver for Maxim MAX11100 single-channel ADC.
> Add DT bindings documentation.

some more comments
 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> 
> v1 -> v2:
>     - incorporated pmeerw's review comments
>     - retrieve vref from dts and use that to convert read_raw result
>       to mV
>     - add device tree bindings documentation
> 
> ---
>  .../devicetree/bindings/iio/adc/max11100.txt       |  17 +++
>  drivers/iio/adc/Kconfig                            |   9 ++
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/max11100.c                         | 166 +++++++++++++++++++++
>  4 files changed, 193 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>  create mode 100644 drivers/iio/adc/max11100.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt b/Documentation/devicetree/bindings/iio/adc/max11100.txt
> new file mode 100644
> index 0000000..6877c11
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
> @@ -0,0 +1,17 @@
> +* Maxim max11100 Analog to Digital Converter (ADC)
> +
> +Required properties:
> +  - compatible: Should be "maxim,max11100"
> +  - vref-supply: phandle to the regulator that provides reference voltage
> +
> +Optional properties:
> +  - spi-max-frequency: SPI maximum frequency
> +
> +Example:
> +
> +adc0: max11100@0 {
> +        compatible = "maxim,max11100";
> +        vref-supply = <&adc0_vref>;
> +        spi-max-frequency = <240000>;
> +};
> +
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 99c0514..a909484 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -285,6 +285,15 @@ config MAX1027
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max1027.
>  
> +config MAX11100
> +	tristate "Maxim max11100 ADC driver"
> +	depends on SPI

SPI_MASTER is more precise I think

> +	help
> +	  Say yes here to build support for Maxim max11100 SPI ADC
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max11100.
> +
>  config MAX1363
>  	tristate "Maxim max1363 ADC driver"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 7a40c04..1463044 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
>  obj-$(CONFIG_MAX1027) += max1027.o
> +obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
> new file mode 100644
> index 0000000..f372ad8
> --- /dev/null
> +++ b/drivers/iio/adc/max11100.c
> @@ -0,0 +1,166 @@
> +/*
> + * iio/adc/max11100.c
> + * Maxim max11100 ADC Driver with IIO interface
> + *
> + * Copyright (C) 2016 Renesas Electronics Corporation
> + * Copyright (C) 2016 Jacopo Mondi
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/driver.h>
> +
> +/*
> + * LSB is the ADC single digital step
> + * 1 LSB = (vref / 2 ^ 16)
> + * AIN = (DIN * LSB)
> + */
> +#define MAX11100_LSB_DIV		(1 << 16)
> +#define MAX11100_LSB(vref) 		(vref / MAX11100_LSB_DIV)

maybe parenthesis around vref

> +
> +struct max11100_state {
> +	const struct max11100_chip_desc *desc;
> +	struct spi_device *spi;
> +	int vref_uv;
> +	struct mutex lock;
> +};
> +
> +static struct iio_chan_spec max11100_channels[] = {
> +	{ /* [0] */
> +		.type = IIO_VOLTAGE,
> +		.scan_type = {

scan_type not needed since driver does not support buffered reads

> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 24,
> +			.shift = 8,
> +			.repeat = 1,
> +			.endianness = IIO_BE,
> +		},
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	},
> +};
> +
> +static struct max11100_chip_desc {
> +	unsigned int num_chan;
> +	const struct iio_chan_spec *channels;
> +} max11100_desc = {
> +	.num_chan = ARRAY_SIZE(max11100_channels),
> +	.channels = max11100_channels,
> +};
> +
> +static int max11100_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	int ret;
> +	struct max11100_state *state = iio_priv(indio_dev);
> +	uint8_t buffer[3];
> +
> +	mutex_lock(&state->lock);
> +
> +	ret = spi_read(state->spi, buffer, sizeof(buffer));
> +	if (ret) {
> +		mutex_unlock(&state->lock);
> +		dev_err(&indio_dev->dev, "SPI transfer failed\n");
> +		return ret;
> +	}
> +	mutex_unlock(&state->lock);
> +
> +	/* the first 8 bits sent out from ADC must be 0s */
> +	if (buffer[0]) {
> +		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
> +		return -EINVAL;
> +	}
> +
> +	*val = be16_to_cpu(*(uint16_t *)&buffer[1]);
> +	*val = *val * MAX11100_LSB(state->vref_uv) / 1000;

no, INFO_RAW shall not perform such scaling, use _PROCESSED or add an 
INFO_SCALE to indicate the scaling

> +
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info max11100_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = max11100_read_raw,
> +};
> +
> +static int max11100_probe(struct spi_device *spi)
> +{
> +	int ret;
> +	struct iio_dev *indio_dev;
> +	struct regulator *vref_reg;
> +	struct max11100_state *state;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	state = iio_priv(indio_dev);
> +	state->spi = spi;
> +	state->desc = &max11100_desc;
> +
> +	mutex_init(&state->lock);
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->dev.of_node = spi->dev.of_node;
> +	indio_dev->info = &max11100_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = state->desc->channels;
> +	indio_dev->num_channels = state->desc->num_chan;
> +
> +	vref_reg = devm_regulator_get(&spi->dev, "vref");
> +	if (IS_ERR(vref_reg))
> +		return PTR_ERR(vref_reg);
> +
> +	ret = regulator_enable(vref_reg);
> +	if (ret)
> +		return ret;
> +
> +	state->vref_uv = regulator_get_voltage(vref_reg);
> +	if (state->vref_uv < 0) {
> +		/* dummy regulator "get_voltage" returns -EINVAL as well */
> +		ret = -EINVAL;
> +		goto disable_regulator;
> +	}
> +
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);

the regulator needs to be disabled in a _remove() function and since you 
need a remove function, devm_iio_device_register() should not be used

> +	if (ret)
> +		goto disable_regulator;
> +
> +	return 0;
> +
> +disable_regulator:
> +	regulator_disable(vref_reg);
> +	return ret;
> +}
> +
> +static const struct of_device_id max11100_ids[] = {
> +	{.compatible = "maxim,max11100"},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, max11100_ids);
> +
> +static struct spi_driver max11100_driver = {
> +	.driver = {
> +		.name	= "max11100",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(max11100_ids),
> +	},
> +	.probe		= max11100_probe,
> +};
> +
> +module_spi_driver(max11100_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Maxim max11100 ADC Driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[jacopo]

Hi Geert,

On 13/12/2016 14:21, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Tue, Dec 13, 2016 at 1:37 PM, Jacopo Mondi <jacopo@jmondi.org> wrote:
>> Add IIO driver for Maxim MAX11100 single-channel ADC.
>> Add DT bindings documentation.
>>
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>>  obj-$(CONFIG_LTC2485) += ltc2485.o
>>  obj-$(CONFIG_MAX1027) += max1027.o
>> +obj-$(CONFIG_MAX11100) += max11100.o
>>  obj-$(CONFIG_MAX1363) += max1363.o
>>  obj-$(CONFIG_MCP320X) += mcp320x.o
>>  obj-$(CONFIG_MCP3422) += mcp3422.o
>> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
>> new file mode 100644
>> index 0000000..f372ad8
>> --- /dev/null
>> +++ b/drivers/iio/adc/max11100.c
>
>> +/*
>> + * LSB is the ADC single digital step
>> + * 1 LSB = (vref / 2 ^ 16)
>> + * AIN = (DIN * LSB)
>> + */
>> +#define MAX11100_LSB_DIV               (1 << 16)
>> +#define MAX11100_LSB(vref)             (vref / MAX11100_LSB_DIV)
>
> DIV_ROUND_CLOSEST()?
>
>> +struct max11100_state {
>> +       const struct max11100_chip_desc *desc;
>> +       struct spi_device *spi;
>> +       int vref_uv;
>
> unsi

Was this a suggestion to turn "vref_uv" into unsigned?
As you can see in probe function it can get assigned to -EINVAL when a 
dummy regulator is returned from regulator-core.
I had to make it signed for this reason

Thanks
   j


> It's good practice to move the smaller members to the end of the structure,
> to avoid gaps due to alignment rules (struct mutex needs to be 64-bit aligned
> on 64-bit platforms).
>
>> +       struct mutex lock;
>> +};
>
>> +static struct max11100_chip_desc {
>> +       unsigned int num_chan;
>> +       const struct iio_chan_spec *channels;
>
> Same here (but it won't have any effect for now).
>
>> +} max11100_desc = {
>> +       .num_chan = ARRAY_SIZE(max11100_channels),
>> +       .channels = max11100_channels,
>> +};
>
>> +static int max11100_read_raw(struct iio_dev *indio_dev,
>> +                            struct iio_chan_spec const *chan,
>> +                            int *val, int *val2, long mask)
>> +{
>> +       int ret;
>> +       struct max11100_state *state = iio_priv(indio_dev);
>> +       uint8_t buffer[3];
>
>> +       *val = be16_to_cpu(*(uint16_t *)&buffer[1]);
>
> Reading the uint16_t will be an unaligned load, which is not supported on all
> platforms. So you either have to use get_unaligned_le16(), or assemble the
> value yourself, like you did in v1.
>
>> +       *val = *val * MAX11100_LSB(state->vref_uv) / 1000;
>
> DIV_ROUND_CLOSEST()?
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[jacopo]

Hello Peter,
    thanks for review

On 13/12/2016 21:33, Peter Meerwald-Stadler wrote:
>
>> Add IIO driver for Maxim MAX11100 single-channel ADC.
>> Add DT bindings documentation.
>
> some more comments
>
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>
>> v1 -> v2:
>>     - incorporated pmeerw's review comments
>>     - retrieve vref from dts and use that to convert read_raw result
>>       to mV
>>     - add device tree bindings documentation
>>
>> ---
>>  .../devicetree/bindings/iio/adc/max11100.txt       |  17 +++
>>  drivers/iio/adc/Kconfig                            |   9 ++
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/max11100.c                         | 166 +++++++++++++++++++++
>>  4 files changed, 193 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>>  create mode 100644 drivers/iio/adc/max11100.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt b/Documentation/devicetree/bindings/iio/adc/max11100.txt
>> new file mode 100644
>> index 0000000..6877c11
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
>> @@ -0,0 +1,17 @@
>> +* Maxim max11100 Analog to Digital Converter (ADC)
>> +
>> +Required properties:
>> +  - compatible: Should be "maxim,max11100"
>> +  - vref-supply: phandle to the regulator that provides reference voltage
>> +
>> +Optional properties:
>> +  - spi-max-frequency: SPI maximum frequency
>> +
>> +Example:
>> +
>> +adc0: max11100@0 {
>> +        compatible = "maxim,max11100";
>> +        vref-supply = <&adc0_vref>;
>> +        spi-max-frequency = <240000>;
>> +};
>> +
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 99c0514..a909484 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -285,6 +285,15 @@ config MAX1027
>>  	  To compile this driver as a module, choose M here: the module will be
>>  	  called max1027.
>>
>> +config MAX11100
>> +	tristate "Maxim max11100 ADC driver"
>> +	depends on SPI
>
> SPI_MASTER is more precise I think
>
>> +	help
>> +	  Say yes here to build support for Maxim max11100 SPI ADC
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called max11100.
>> +
>>  config MAX1363
>>  	tristate "Maxim max1363 ADC driver"
>>  	depends on I2C
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 7a40c04..1463044 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>>  obj-$(CONFIG_LTC2485) += ltc2485.o
>>  obj-$(CONFIG_MAX1027) += max1027.o
>> +obj-$(CONFIG_MAX11100) += max11100.o
>>  obj-$(CONFIG_MAX1363) += max1363.o
>>  obj-$(CONFIG_MCP320X) += mcp320x.o
>>  obj-$(CONFIG_MCP3422) += mcp3422.o
>> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
>> new file mode 100644
>> index 0000000..f372ad8
>> --- /dev/null
>> +++ b/drivers/iio/adc/max11100.c
>> @@ -0,0 +1,166 @@
>> +/*
>> + * iio/adc/max11100.c
>> + * Maxim max11100 ADC Driver with IIO interface
>> + *
>> + * Copyright (C) 2016 Renesas Electronics Corporation
>> + * Copyright (C) 2016 Jacopo Mondi
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include <linux/delay.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/driver.h>
>> +
>> +/*
>> + * LSB is the ADC single digital step
>> + * 1 LSB = (vref / 2 ^ 16)
>> + * AIN = (DIN * LSB)
>> + */
>> +#define MAX11100_LSB_DIV		(1 << 16)
>> +#define MAX11100_LSB(vref) 		(vref / MAX11100_LSB_DIV)
>
> maybe parenthesis around vref
>
>> +
>> +struct max11100_state {
>> +	const struct max11100_chip_desc *desc;
>> +	struct spi_device *spi;
>> +	int vref_uv;
>> +	struct mutex lock;
>> +};
>> +
>> +static struct iio_chan_spec max11100_channels[] = {
>> +	{ /* [0] */
>> +		.type = IIO_VOLTAGE,
>> +		.scan_type = {
>
> scan_type not needed since driver does not support buffered reads
>
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 24,
>> +			.shift = 8,
>> +			.repeat = 1,
>> +			.endianness = IIO_BE,
>> +		},
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +	},
>> +};
>> +
>> +static struct max11100_chip_desc {
>> +	unsigned int num_chan;
>> +	const struct iio_chan_spec *channels;
>> +} max11100_desc = {
>> +	.num_chan = ARRAY_SIZE(max11100_channels),
>> +	.channels = max11100_channels,
>> +};
>> +
>> +static int max11100_read_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int *val, int *val2, long mask)
>> +{
>> +	int ret;
>> +	struct max11100_state *state = iio_priv(indio_dev);
>> +	uint8_t buffer[3];
>> +
>> +	mutex_lock(&state->lock);
>> +
>> +	ret = spi_read(state->spi, buffer, sizeof(buffer));
>> +	if (ret) {
>> +		mutex_unlock(&state->lock);
>> +		dev_err(&indio_dev->dev, "SPI transfer failed\n");
>> +		return ret;
>> +	}
>> +	mutex_unlock(&state->lock);
>> +
>> +	/* the first 8 bits sent out from ADC must be 0s */
>> +	if (buffer[0]) {
>> +		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	*val = be16_to_cpu(*(uint16_t *)&buffer[1]);
>> +	*val = *val * MAX11100_LSB(state->vref_uv) / 1000;
>
> no, INFO_RAW shall not perform such scaling, use _PROCESSED or add an
> INFO_SCALE to indicate the scaling

Here I am not scaling the result, just converting the digital value read 
from ADC into millivolts.
The transfer function from Din to Ain depends on vref, in the form 
reported in comments in file header:

Ain = Din * (vref / 2^16)

I am using microvolts as "vref" unit otherwise I would have been forced 
to deal with floating point arithmetic.

Thanks
    j

>
>> +
>> +	return IIO_VAL_INT;
>> +}
>> +
>> +static const struct iio_info max11100_info = {
>> +	.driver_module = THIS_MODULE,
>> +	.read_raw = max11100_read_raw,
>> +};
>> +
>> +static int max11100_probe(struct spi_device *spi)
>> +{
>> +	int ret;
>> +	struct iio_dev *indio_dev;
>> +	struct regulator *vref_reg;
>> +	struct max11100_state *state;
>> +
>> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	spi_set_drvdata(spi, indio_dev);
>> +
>> +	state = iio_priv(indio_dev);
>> +	state->spi = spi;
>> +	state->desc = &max11100_desc;
>> +
>> +	mutex_init(&state->lock);
>> +
>> +	indio_dev->dev.parent = &spi->dev;
>> +	indio_dev->dev.of_node = spi->dev.of_node;
>> +	indio_dev->info = &max11100_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = state->desc->channels;
>> +	indio_dev->num_channels = state->desc->num_chan;
>> +
>> +	vref_reg = devm_regulator_get(&spi->dev, "vref");
>> +	if (IS_ERR(vref_reg))
>> +		return PTR_ERR(vref_reg);
>> +
>> +	ret = regulator_enable(vref_reg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	state->vref_uv = regulator_get_voltage(vref_reg);
>> +	if (state->vref_uv < 0) {
>> +		/* dummy regulator "get_voltage" returns -EINVAL as well */
>> +		ret = -EINVAL;
>> +		goto disable_regulator;
>> +	}
>> +
>> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
>
> the regulator needs to be disabled in a _remove() function and since you
> need a remove function, devm_iio_device_register() should not be used
>
>> +	if (ret)
>> +		goto disable_regulator;
>> +
>> +	return 0;
>> +
>> +disable_regulator:
>> +	regulator_disable(vref_reg);
>> +	return ret;
>> +}
>> +
>> +static const struct of_device_id max11100_ids[] = {
>> +	{.compatible = "maxim,max11100"},
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, max11100_ids);
>> +
>> +static struct spi_driver max11100_driver = {
>> +	.driver = {
>> +		.name	= "max11100",
>> +		.owner	= THIS_MODULE,
>> +		.of_match_table = of_match_ptr(max11100_ids),
>> +	},
>> +	.probe		= max11100_probe,
>> +};
>> +
>> +module_spi_driver(max11100_driver);
>> +
>> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
>> +MODULE_DESCRIPTION("Maxim max11100 ADC Driver");
>> +MODULE_LICENSE("GPL v2");
>>
>

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[Geert Uytterhoeven]

Hi Jacopo,

On Wed, Dec 14, 2016 at 12:54 PM, jacopo@jmondi.org <jacopo@jmondi.org> wrote:
>>> +struct max11100_state {
>>> +       const struct max11100_chip_desc *desc;
>>> +       struct spi_device *spi;
>>> +       int vref_uv;
>>
>> unsi
>
> Was this a suggestion to turn "vref_uv" into unsigned?

It was a suggestion I started writing, and aborted improperly when noticing
the below ;-)

> As you can see in probe function it can get assigned to -EINVAL when a dummy
> regulator is returned from regulator-core.
> I had to make it signed for this reason

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[Jonathan Cameron]

On 13/12/16 12:37, Jacopo Mondi wrote:
> Add IIO driver for Maxim MAX11100 single-channel ADC.
> Add DT bindings documentation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
A quick process related thing.  Please don't post a new
version of a driver as a reply to an old version. It very rapidly
leads to deep and difficult to follow threads.

Much easier to just start a new thread.

> ---
> 
> v1 -> v2:
>     - incorporated pmeerw's review comments
>     - retrieve vref from dts and use that to convert read_raw result
>       to mV
>     - add device tree bindings documentation
> 
> ---
>  .../devicetree/bindings/iio/adc/max11100.txt       |  17 +++
>  drivers/iio/adc/Kconfig                            |   9 ++
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/max11100.c                         | 166 +++++++++++++++++++++
>  4 files changed, 193 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>  create mode 100644 drivers/iio/adc/max11100.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt b/Documentation/devicetree/bindings/iio/adc/max11100.txt
> new file mode 100644
> index 0000000..6877c11
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
> @@ -0,0 +1,17 @@
> +* Maxim max11100 Analog to Digital Converter (ADC)
> +
> +Required properties:
> +  - compatible: Should be "maxim,max11100"
> +  - vref-supply: phandle to the regulator that provides reference voltage
> +
> +Optional properties:
> +  - spi-max-frequency: SPI maximum frequency
> +
> +Example:
> +
> +adc0: max11100@0 {
> +        compatible = "maxim,max11100";
> +        vref-supply = <&adc0_vref>;
> +        spi-max-frequency = <240000>;
> +};
> +
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 99c0514..a909484 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -285,6 +285,15 @@ config MAX1027
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max1027.
>  
> +config MAX11100
> +	tristate "Maxim max11100 ADC driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build support for Maxim max11100 SPI ADC
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max11100.
> +
>  config MAX1363
>  	tristate "Maxim max1363 ADC driver"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 7a40c04..1463044 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
>  obj-$(CONFIG_MAX1027) += max1027.o
> +obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
> new file mode 100644
> index 0000000..f372ad8
> --- /dev/null
> +++ b/drivers/iio/adc/max11100.c
> @@ -0,0 +1,166 @@
> +/*
> + * iio/adc/max11100.c
> + * Maxim max11100 ADC Driver with IIO interface
> + *
> + * Copyright (C) 2016 Renesas Electronics Corporation
> + * Copyright (C) 2016 Jacopo Mondi
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/driver.h>
> +
> +/*
> + * LSB is the ADC single digital step
> + * 1 LSB = (vref / 2 ^ 16)
> + * AIN = (DIN * LSB)
> + */
> +#define MAX11100_LSB_DIV		(1 << 16)
> +#define MAX11100_LSB(vref) 		(vref / MAX11100_LSB_DIV)
> +
> +struct max11100_state {
> +	const struct max11100_chip_desc *desc;
> +	struct spi_device *spi;
> +	int vref_uv;
> +	struct mutex lock;
> +};
> +
> +static struct iio_chan_spec max11100_channels[] = {
> +	{ /* [0] */
> +		.type = IIO_VOLTAGE,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 24,
> +			.shift = 8,
> +			.repeat = 1,
> +			.endianness = IIO_BE,
> +		},
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	},
> +};
> +
> +static struct max11100_chip_desc {
> +	unsigned int num_chan;
> +	const struct iio_chan_spec *channels;
> +} max11100_desc = {
> +	.num_chan = ARRAY_SIZE(max11100_channels),
> +	.channels = max11100_channels,
> +};
> +
> +static int max11100_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	int ret;
> +	struct max11100_state *state = iio_priv(indio_dev);
> +	uint8_t buffer[3];
> +
> +	mutex_lock(&state->lock);
> +
> +	ret = spi_read(state->spi, buffer, sizeof(buffer));
> +	if (ret) {
> +		mutex_unlock(&state->lock);
> +		dev_err(&indio_dev->dev, "SPI transfer failed\n");
> +		return ret;
> +	}
> +	mutex_unlock(&state->lock);
> +
> +	/* the first 8 bits sent out from ADC must be 0s */
> +	if (buffer[0]) {
> +		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
> +		return -EINVAL;
> +	}
> +
> +	*val = be16_to_cpu(*(uint16_t *)&buffer[1]);
> +	*val = *val * MAX11100_LSB(state->vref_uv) / 1000;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info max11100_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = max11100_read_raw,
> +};
> +
> +static int max11100_probe(struct spi_device *spi)
> +{
> +	int ret;
> +	struct iio_dev *indio_dev;
> +	struct regulator *vref_reg;
> +	struct max11100_state *state;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	state = iio_priv(indio_dev);
> +	state->spi = spi;
> +	state->desc = &max11100_desc;
> +
> +	mutex_init(&state->lock);
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->dev.of_node = spi->dev.of_node;
> +	indio_dev->info = &max11100_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = state->desc->channels;
> +	indio_dev->num_channels = state->desc->num_chan;
> +
> +	vref_reg = devm_regulator_get(&spi->dev, "vref");
> +	if (IS_ERR(vref_reg))
> +		return PTR_ERR(vref_reg);
> +
> +	ret = regulator_enable(vref_reg);
> +	if (ret)
> +		return ret;
> +
> +	state->vref_uv = regulator_get_voltage(vref_reg);
> +	if (state->vref_uv < 0) {
> +		/* dummy regulator "get_voltage" returns -EINVAL as well */
> +		ret = -EINVAL;
> +		goto disable_regulator;
> +	}
> +
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret)
> +		goto disable_regulator;
> +
> +	return 0;
> +
> +disable_regulator:
> +	regulator_disable(vref_reg);
> +	return ret;
> +}
> +
> +static const struct of_device_id max11100_ids[] = {
> +	{.compatible = "maxim,max11100"},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, max11100_ids);
> +
> +static struct spi_driver max11100_driver = {
> +	.driver = {
> +		.name	= "max11100",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(max11100_ids),
> +	},
> +	.probe		= max11100_probe,
> +};
> +
> +module_spi_driver(max11100_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Maxim max11100 ADC Driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[Jonathan Cameron]

On 13/12/16 15:53, Wolfram Sang wrote:
> 
>>> +struct max11100_state {
>>> +       const struct max11100_chip_desc *desc;
>>> +       struct spi_device *spi;
>>> +       int vref_uv;
>>
>> unsi
>> It's good practice to move the smaller members to the end of the structure,
>> to avoid gaps due to alignment rules (struct mutex needs to be 64-bit aligned
>> on 64-bit platforms).
> 
> One option. Another idea is to put the most used members to the front to
> increase chances of being in the same cacheline.
> 
Or more cynically, just put them in the order that makes most logical sense
as that gives you fewest grumpy reviewers.
We aren't really talking high performance stuff here!

Jonathan

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[Jonathan Cameron]

On 14/12/16 12:00, jacopo@jmondi.org wrote:
> Hello Peter,
>    thanks for review
> 
> On 13/12/2016 21:33, Peter Meerwald-Stadler wrote:
>>
>>> Add IIO driver for Maxim MAX11100 single-channel ADC.
>>> Add DT bindings documentation.
>>
>> some more comments
>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>
>>> v1 -> v2:
>>>     - incorporated pmeerw's review comments
>>>     - retrieve vref from dts and use that to convert read_raw result
>>>       to mV
>>>     - add device tree bindings documentation
>>>
>>> ---
>>>  .../devicetree/bindings/iio/adc/max11100.txt       |  17 +++
>>>  drivers/iio/adc/Kconfig                            |   9 ++
>>>  drivers/iio/adc/Makefile                           |   1 +
>>>  drivers/iio/adc/max11100.c                         | 166 +++++++++++++++++++++
>>>  4 files changed, 193 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>>>  create mode 100644 drivers/iio/adc/max11100.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt b/Documentation/devicetree/bindings/iio/adc/max11100.txt
>>> new file mode 100644
>>> index 0000000..6877c11
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
>>> @@ -0,0 +1,17 @@
>>> +* Maxim max11100 Analog to Digital Converter (ADC)
>>> +
>>> +Required properties:
>>> +  - compatible: Should be "maxim,max11100"
>>> +  - vref-supply: phandle to the regulator that provides reference voltage
>>> +
>>> +Optional properties:
>>> +  - spi-max-frequency: SPI maximum frequency
>>> +
>>> +Example:
>>> +
>>> +adc0: max11100@0 {
>>> +        compatible = "maxim,max11100";
>>> +        vref-supply = <&adc0_vref>;
>>> +        spi-max-frequency = <240000>;
>>> +};
>>> +
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 99c0514..a909484 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -285,6 +285,15 @@ config MAX1027
>>>        To compile this driver as a module, choose M here: the module will be
>>>        called max1027.
>>>
>>> +config MAX11100
>>> +    tristate "Maxim max11100 ADC driver"
>>> +    depends on SPI
>>
>> SPI_MASTER is more precise I think
>>
>>> +    help
>>> +      Say yes here to build support for Maxim max11100 SPI ADC
>>> +
>>> +      To compile this driver as a module, choose M here: the module will be
>>> +      called max11100.
>>> +
>>>  config MAX1363
>>>      tristate "Maxim max1363 ADC driver"
>>>      depends on I2C
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index 7a40c04..1463044 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>>>  obj-$(CONFIG_LTC2485) += ltc2485.o
>>>  obj-$(CONFIG_MAX1027) += max1027.o
>>> +obj-$(CONFIG_MAX11100) += max11100.o
>>>  obj-$(CONFIG_MAX1363) += max1363.o
>>>  obj-$(CONFIG_MCP320X) += mcp320x.o
>>>  obj-$(CONFIG_MCP3422) += mcp3422.o
>>> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
>>> new file mode 100644
>>> index 0000000..f372ad8
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/max11100.c
>>> @@ -0,0 +1,166 @@
>>> +/*
>>> + * iio/adc/max11100.c
>>> + * Maxim max11100 ADC Driver with IIO interface
>>> + *
>>> + * Copyright (C) 2016 Renesas Electronics Corporation
>>> + * Copyright (C) 2016 Jacopo Mondi
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +#include <linux/delay.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/spi/spi.h>
>>> +
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/buffer.h>
>>> +#include <linux/iio/driver.h>
>>> +
>>> +/*
>>> + * LSB is the ADC single digital step
>>> + * 1 LSB = (vref / 2 ^ 16)
>>> + * AIN = (DIN * LSB)
>>> + */
>>> +#define MAX11100_LSB_DIV        (1 << 16)
>>> +#define MAX11100_LSB(vref)         (vref / MAX11100_LSB_DIV)
>>
>> maybe parenthesis around vref
>>
>>> +
>>> +struct max11100_state {
>>> +    const struct max11100_chip_desc *desc;
>>> +    struct spi_device *spi;
>>> +    int vref_uv;
>>> +    struct mutex lock;
>>> +};
>>> +
>>> +static struct iio_chan_spec max11100_channels[] = {
>>> +    { /* [0] */
>>> +        .type = IIO_VOLTAGE,
>>> +        .scan_type = {
>>
>> scan_type not needed since driver does not support buffered reads
>>
>>> +            .sign = 'u',
>>> +            .realbits = 16,
>>> +            .storagebits = 24,
>>> +            .shift = 8,
>>> +            .repeat = 1,
>>> +            .endianness = IIO_BE,
>>> +        },
>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>> +    },
>>> +};
>>> +
>>> +static struct max11100_chip_desc {
>>> +    unsigned int num_chan;
>>> +    const struct iio_chan_spec *channels;
>>> +} max11100_desc = {
>>> +    .num_chan = ARRAY_SIZE(max11100_channels),
>>> +    .channels = max11100_channels,
>>> +};
>>> +
>>> +static int max11100_read_raw(struct iio_dev *indio_dev,
>>> +                 struct iio_chan_spec const *chan,
>>> +                 int *val, int *val2, long mask)
>>> +{
>>> +    int ret;
>>> +    struct max11100_state *state = iio_priv(indio_dev);
>>> +    uint8_t buffer[3];
>>> +
>>> +    mutex_lock(&state->lock);
>>> +
>>> +    ret = spi_read(state->spi, buffer, sizeof(buffer));
>>> +    if (ret) {
>>> +        mutex_unlock(&state->lock);
>>> +        dev_err(&indio_dev->dev, "SPI transfer failed\n");
>>> +        return ret;
>>> +    }
>>> +    mutex_unlock(&state->lock);
>>> +
>>> +    /* the first 8 bits sent out from ADC must be 0s */
>>> +    if (buffer[0]) {
>>> +        dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    *val = be16_to_cpu(*(uint16_t *)&buffer[1]);
>>> +    *val = *val * MAX11100_LSB(state->vref_uv) / 1000;
>>
>> no, INFO_RAW shall not perform such scaling, use _PROCESSED or add an
>> INFO_SCALE to indicate the scaling
> 
> Here I am not scaling the result, just converting the digital value read from ADC into millivolts.
> The transfer function from Din to Ain depends on vref, in the form reported in comments in file header:
> 
> Ain = Din * (vref / 2^16)
> 
> I am using microvolts as "vref" unit otherwise I would have been forced to deal with floating point arithmetic.
That's what the _scale elements of the IIO ABI meant for.  Make even this simple
conversion the job of userspace which is better placed to do any magic it wishes
to do with how it does the conversion (or whether it does depending on the
application).  So expectation is that raw means whatever came from the ADC so
ADC 'counts' not mV or similar.

Jonathan
> 
> Thanks
>    j
> 
>>
>>> +
>>> +    return IIO_VAL_INT;
>>> +}
>>> +
>>> +static const struct iio_info max11100_info = {
>>> +    .driver_module = THIS_MODULE,
>>> +    .read_raw = max11100_read_raw,
>>> +};
>>> +
>>> +static int max11100_probe(struct spi_device *spi)
>>> +{
>>> +    int ret;
>>> +    struct iio_dev *indio_dev;
>>> +    struct regulator *vref_reg;
>>> +    struct max11100_state *state;
>>> +
>>> +    indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
>>> +    if (!indio_dev)
>>> +        return -ENOMEM;
>>> +
>>> +    spi_set_drvdata(spi, indio_dev);
>>> +
>>> +    state = iio_priv(indio_dev);
>>> +    state->spi = spi;
>>> +    state->desc = &max11100_desc;
>>> +
>>> +    mutex_init(&state->lock);
>>> +
>>> +    indio_dev->dev.parent = &spi->dev;
>>> +    indio_dev->dev.of_node = spi->dev.of_node;
>>> +    indio_dev->info = &max11100_info;
>>> +    indio_dev->modes = INDIO_DIRECT_MODE;
>>> +    indio_dev->channels = state->desc->channels;
>>> +    indio_dev->num_channels = state->desc->num_chan;
>>> +
>>> +    vref_reg = devm_regulator_get(&spi->dev, "vref");
>>> +    if (IS_ERR(vref_reg))
>>> +        return PTR_ERR(vref_reg);
>>> +
>>> +    ret = regulator_enable(vref_reg);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    state->vref_uv = regulator_get_voltage(vref_reg);
>>> +    if (state->vref_uv < 0) {
>>> +        /* dummy regulator "get_voltage" returns -EINVAL as well */
>>> +        ret = -EINVAL;
>>> +        goto disable_regulator;
>>> +    }
>>> +
>>> +    ret = devm_iio_device_register(&spi->dev, indio_dev);
>>
>> the regulator needs to be disabled in a _remove() function and since you
>> need a remove function, devm_iio_device_register() should not be used
>>
>>> +    if (ret)
>>> +        goto disable_regulator;
>>> +
>>> +    return 0;
>>> +
>>> +disable_regulator:
>>> +    regulator_disable(vref_reg);
>>> +    return ret;
>>> +}
>>> +
>>> +static const struct of_device_id max11100_ids[] = {
>>> +    {.compatible = "maxim,max11100"},
>>> +    { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, max11100_ids);
>>> +
>>> +static struct spi_driver max11100_driver = {
>>> +    .driver = {
>>> +        .name    = "max11100",
>>> +        .owner    = THIS_MODULE,
>>> +        .of_match_table = of_match_ptr(max11100_ids),
>>> +    },
>>> +    .probe        = max11100_probe,
>>> +};
>>> +
>>> +module_spi_driver(max11100_driver);
>>> +
>>> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
>>> +MODULE_DESCRIPTION("Maxim max11100 ADC Driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[Jonathan Cameron]

On 13/12/16 12:37, Jacopo Mondi wrote:
> Add IIO driver for Maxim MAX11100 single-channel ADC.
> Add DT bindings documentation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Nothing significant to add, but a few comments inline.

Jonathan
> ---
> 
> v1 -> v2:
>     - incorporated pmeerw's review comments
>     - retrieve vref from dts and use that to convert read_raw result
>       to mV
>     - add device tree bindings documentation
> 
> ---
>  .../devicetree/bindings/iio/adc/max11100.txt       |  17 +++
>  drivers/iio/adc/Kconfig                            |   9 ++
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/max11100.c                         | 166 +++++++++++++++++++++
>  4 files changed, 193 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>  create mode 100644 drivers/iio/adc/max11100.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt b/Documentation/devicetree/bindings/iio/adc/max11100.txt
> new file mode 100644
> index 0000000..6877c11
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
> @@ -0,0 +1,17 @@
> +* Maxim max11100 Analog to Digital Converter (ADC)
> +
> +Required properties:
> +  - compatible: Should be "maxim,max11100"
> +  - vref-supply: phandle to the regulator that provides reference voltage
> +
> +Optional properties:
> +  - spi-max-frequency: SPI maximum frequency
> +
> +Example:
> +
> +adc0: max11100@0 {
> +        compatible = "maxim,max11100";
> +        vref-supply = <&adc0_vref>;
> +        spi-max-frequency = <240000>;
> +};
> +
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 99c0514..a909484 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -285,6 +285,15 @@ config MAX1027
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max1027.
>  
> +config MAX11100
> +	tristate "Maxim max11100 ADC driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build support for Maxim max11100 SPI ADC
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max11100.
> +
>  config MAX1363
>  	tristate "Maxim max1363 ADC driver"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 7a40c04..1463044 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
>  obj-$(CONFIG_MAX1027) += max1027.o
> +obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
> new file mode 100644
> index 0000000..f372ad8
> --- /dev/null
> +++ b/drivers/iio/adc/max11100.c
> @@ -0,0 +1,166 @@
> +/*
> + * iio/adc/max11100.c
> + * Maxim max11100 ADC Driver with IIO interface
> + *
> + * Copyright (C) 2016 Renesas Electronics Corporation
> + * Copyright (C) 2016 Jacopo Mondi
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/driver.h>
> +
> +/*
> + * LSB is the ADC single digital step
> + * 1 LSB = (vref / 2 ^ 16)
> + * AIN = (DIN * LSB)
> + */
> +#define MAX11100_LSB_DIV		(1 << 16)
> +#define MAX11100_LSB(vref) 		(vref / MAX11100_LSB_DIV)
> +
> +struct max11100_state {
> +	const struct max11100_chip_desc *desc;
> +	struct spi_device *spi;
> +	int vref_uv;
> +	struct mutex lock;
> +};
> +
> +static struct iio_chan_spec max11100_channels[] = {
> +	{ /* [0] */
> +		.type = IIO_VOLTAGE,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 24,
> +			.shift = 8,
> +			.repeat = 1,
> +			.endianness = IIO_BE,
> +		},
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	},
> +};
> +
> +static struct max11100_chip_desc {
> +	unsigned int num_chan;
> +	const struct iio_chan_spec *channels;
> +} max11100_desc = {
> +	.num_chan = ARRAY_SIZE(max11100_channels),
> +	.channels = max11100_channels,
> +};
> +
> +static int max11100_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	int ret;
> +	struct max11100_state *state = iio_priv(indio_dev);
> +	uint8_t buffer[3];
> +
> +	mutex_lock(&state->lock);
> +
> +	ret = spi_read(state->spi, buffer, sizeof(buffer));
> +	if (ret) {
> +		mutex_unlock(&state->lock);
> +		dev_err(&indio_dev->dev, "SPI transfer failed\n");
> +		return ret;
> +	}
> +	mutex_unlock(&state->lock);
> +
> +	/* the first 8 bits sent out from ADC must be 0s */
> +	if (buffer[0]) {
> +		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
> +		return -EINVAL;
> +	}
> +
> +	*val = be16_to_cpu(*(uint16_t *)&buffer[1]);
> +	*val = *val * MAX11100_LSB(state->vref_uv) / 1000;
What Peter said.
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info max11100_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = max11100_read_raw,
> +};
> +
> +static int max11100_probe(struct spi_device *spi)
> +{
> +	int ret;
> +	struct iio_dev *indio_dev;
> +	struct regulator *vref_reg;
> +	struct max11100_state *state;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	state = iio_priv(indio_dev);
> +	state->spi = spi;
> +	state->desc = &max11100_desc;
> +
> +	mutex_init(&state->lock);
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->dev.of_node = spi->dev.of_node;
> +	indio_dev->info = &max11100_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = state->desc->channels;
> +	indio_dev->num_channels = state->desc->num_chan;
> +
> +	vref_reg = devm_regulator_get(&spi->dev, "vref");
> +	if (IS_ERR(vref_reg))
> +		return PTR_ERR(vref_reg);
> +
> +	ret = regulator_enable(vref_reg);
> +	if (ret)
> +		return ret;
> +
> +	state->vref_uv = regulator_get_voltage(vref_reg);
If possible, do this as late as you can.  There are boards out there
were, to save hardware, reference voltages are fed from power supply lines
and and exactly what is on those lines may change as the board boots up.

Once you've moved the use of this value to the the _scale bit of read_raw
then it won't get called very often so just do the actual voltage request
there.  Do leave the enabling etc here though as otherwise power management
code might turn it off as unused.
> +	if (state->vref_uv < 0) {
> +		/* dummy regulator "get_voltage" returns -EINVAL as well */
> +		ret = -EINVAL;
> +		goto disable_regulator;
> +	}
> +
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret)
> +		goto disable_regulator;
> +
> +	return 0;
> +
> +disable_regulator:
> +	regulator_disable(vref_reg);
> +	return ret;
> +}
> +
> +static const struct of_device_id max11100_ids[] = {
> +	{.compatible = "maxim,max11100"},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, max11100_ids);
> +
> +static struct spi_driver max11100_driver = {
> +	.driver = {
> +		.name	= "max11100",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(max11100_ids),
> +	},
> +	.probe		= max11100_probe,
> +};
> +
> +module_spi_driver(max11100_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Maxim max11100 ADC Driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[jacopo mondi]

Hi Jonathan,

On 30/12/2016 17:08, Jonathan Cameron wrote:
> On 13/12/16 12:37, Jacopo Mondi wrote:
>> Add IIO driver for Maxim MAX11100 single-channel ADC.
>> Add DT bindings documentation.
>>
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> A quick process related thing.  Please don't post a new
> version of a driver as a reply to an old version. It very rapidly
> leads to deep and difficult to follow threads.
>
> Much easier to just start a new thread.
>

Noted!
Thanks for suggestion ;)
    j


>> ---
>>
>> v1 -> v2:
>>     - incorporated pmeerw's review comments
>>     - retrieve vref from dts and use that to convert read_raw result
>>       to mV
>>     - add device tree bindings documentation
>>
>> ---
>>  .../devicetree/bindings/iio/adc/max11100.txt       |  17 +++
>>  drivers/iio/adc/Kconfig                            |   9 ++
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/max11100.c                         | 166 +++++++++++++++++++++
>>  4 files changed, 193 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>>  create mode 100644 drivers/iio/adc/max11100.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt b/Documentation/devicetree/bindings/iio/adc/max11100.txt
>> new file mode 100644
>> index 0000000..6877c11
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
>> @@ -0,0 +1,17 @@
>> +* Maxim max11100 Analog to Digital Converter (ADC)
>> +
>> +Required properties:
>> +  - compatible: Should be "maxim,max11100"
>> +  - vref-supply: phandle to the regulator that provides reference voltage
>> +
>> +Optional properties:
>> +  - spi-max-frequency: SPI maximum frequency
>> +
>> +Example:
>> +
>> +adc0: max11100@0 {
>> +        compatible = "maxim,max11100";
>> +        vref-supply = <&adc0_vref>;
>> +        spi-max-frequency = <240000>;
>> +};
>> +
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 99c0514..a909484 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -285,6 +285,15 @@ config MAX1027
>>  	  To compile this driver as a module, choose M here: the module will be
>>  	  called max1027.
>>
>> +config MAX11100
>> +	tristate "Maxim max11100 ADC driver"
>> +	depends on SPI
>> +	help
>> +	  Say yes here to build support for Maxim max11100 SPI ADC
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called max11100.
>> +
>>  config MAX1363
>>  	tristate "Maxim max1363 ADC driver"
>>  	depends on I2C
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 7a40c04..1463044 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>>  obj-$(CONFIG_LTC2485) += ltc2485.o
>>  obj-$(CONFIG_MAX1027) += max1027.o
>> +obj-$(CONFIG_MAX11100) += max11100.o
>>  obj-$(CONFIG_MAX1363) += max1363.o
>>  obj-$(CONFIG_MCP320X) += mcp320x.o
>>  obj-$(CONFIG_MCP3422) += mcp3422.o
>> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
>> new file mode 100644
>> index 0000000..f372ad8
>> --- /dev/null
>> +++ b/drivers/iio/adc/max11100.c
>> @@ -0,0 +1,166 @@
>> +/*
>> + * iio/adc/max11100.c
>> + * Maxim max11100 ADC Driver with IIO interface
>> + *
>> + * Copyright (C) 2016 Renesas Electronics Corporation
>> + * Copyright (C) 2016 Jacopo Mondi
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include <linux/delay.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/driver.h>
>> +
>> +/*
>> + * LSB is the ADC single digital step
>> + * 1 LSB = (vref / 2 ^ 16)
>> + * AIN = (DIN * LSB)
>> + */
>> +#define MAX11100_LSB_DIV		(1 << 16)
>> +#define MAX11100_LSB(vref) 		(vref / MAX11100_LSB_DIV)
>> +
>> +struct max11100_state {
>> +	const struct max11100_chip_desc *desc;
>> +	struct spi_device *spi;
>> +	int vref_uv;
>> +	struct mutex lock;
>> +};
>> +
>> +static struct iio_chan_spec max11100_channels[] = {
>> +	{ /* [0] */
>> +		.type = IIO_VOLTAGE,
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 24,
>> +			.shift = 8,
>> +			.repeat = 1,
>> +			.endianness = IIO_BE,
>> +		},
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +	},
>> +};
>> +
>> +static struct max11100_chip_desc {
>> +	unsigned int num_chan;
>> +	const struct iio_chan_spec *channels;
>> +} max11100_desc = {
>> +	.num_chan = ARRAY_SIZE(max11100_channels),
>> +	.channels = max11100_channels,
>> +};
>> +
>> +static int max11100_read_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int *val, int *val2, long mask)
>> +{
>> +	int ret;
>> +	struct max11100_state *state = iio_priv(indio_dev);
>> +	uint8_t buffer[3];
>> +
>> +	mutex_lock(&state->lock);
>> +
>> +	ret = spi_read(state->spi, buffer, sizeof(buffer));
>> +	if (ret) {
>> +		mutex_unlock(&state->lock);
>> +		dev_err(&indio_dev->dev, "SPI transfer failed\n");
>> +		return ret;
>> +	}
>> +	mutex_unlock(&state->lock);
>> +
>> +	/* the first 8 bits sent out from ADC must be 0s */
>> +	if (buffer[0]) {
>> +		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	*val = be16_to_cpu(*(uint16_t *)&buffer[1]);
>> +	*val = *val * MAX11100_LSB(state->vref_uv) / 1000;
>> +
>> +	return IIO_VAL_INT;
>> +}
>> +
>> +static const struct iio_info max11100_info = {
>> +	.driver_module = THIS_MODULE,
>> +	.read_raw = max11100_read_raw,
>> +};
>> +
>> +static int max11100_probe(struct spi_device *spi)
>> +{
>> +	int ret;
>> +	struct iio_dev *indio_dev;
>> +	struct regulator *vref_reg;
>> +	struct max11100_state *state;
>> +
>> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	spi_set_drvdata(spi, indio_dev);
>> +
>> +	state = iio_priv(indio_dev);
>> +	state->spi = spi;
>> +	state->desc = &max11100_desc;
>> +
>> +	mutex_init(&state->lock);
>> +
>> +	indio_dev->dev.parent = &spi->dev;
>> +	indio_dev->dev.of_node = spi->dev.of_node;
>> +	indio_dev->info = &max11100_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = state->desc->channels;
>> +	indio_dev->num_channels = state->desc->num_chan;
>> +
>> +	vref_reg = devm_regulator_get(&spi->dev, "vref");
>> +	if (IS_ERR(vref_reg))
>> +		return PTR_ERR(vref_reg);
>> +
>> +	ret = regulator_enable(vref_reg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	state->vref_uv = regulator_get_voltage(vref_reg);
>> +	if (state->vref_uv < 0) {
>> +		/* dummy regulator "get_voltage" returns -EINVAL as well */
>> +		ret = -EINVAL;
>> +		goto disable_regulator;
>> +	}
>> +
>> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
>> +	if (ret)
>> +		goto disable_regulator;
>> +
>> +	return 0;
>> +
>> +disable_regulator:
>> +	regulator_disable(vref_reg);
>> +	return ret;
>> +}
>> +
>> +static const struct of_device_id max11100_ids[] = {
>> +	{.compatible = "maxim,max11100"},
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, max11100_ids);
>> +
>> +static struct spi_driver max11100_driver = {
>> +	.driver = {
>> +		.name	= "max11100",
>> +		.owner	= THIS_MODULE,
>> +		.of_match_table = of_match_ptr(max11100_ids),
>> +	},
>> +	.probe		= max11100_probe,
>> +};
>> +
>> +module_spi_driver(max11100_driver);
>> +
>> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
>> +MODULE_DESCRIPTION("Maxim max11100 ADC Driver");
>> +MODULE_LICENSE("GPL v2");
>>
>

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[jacopo mondi]

Hi Jonathan,
    thanks for review

On 30/12/2016 17:31, Jonathan Cameron wrote:
> On 14/12/16 12:00, jacopo@jmondi.org wrote:
>> Hello Peter,
>>    thanks for review
>>
>> On 13/12/2016 21:33, Peter Meerwald-Stadler wrote:
>>>
>>>> Add IIO driver for Maxim MAX11100 single-channel ADC.
>>>> Add DT bindings documentation.
>>>
>>> some more comments
>>>
>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>>     - incorporated pmeerw's review comments
>>>>     - retrieve vref from dts and use that to convert read_raw result
>>>>       to mV
>>>>     - add device tree bindings documentation
>>>>
>>>> ---
>>>>  .../devicetree/bindings/iio/adc/max11100.txt       |  17 +++
>>>>  drivers/iio/adc/Kconfig                            |   9 ++
>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>  drivers/iio/adc/max11100.c                         | 166 +++++++++++++++++++++
>>>>  4 files changed, 193 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>>>>  create mode 100644 drivers/iio/adc/max11100.c
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt b/Documentation/devicetree/bindings/iio/adc/max11100.txt
>>>> new file mode 100644
>>>> index 0000000..6877c11
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
>>>> @@ -0,0 +1,17 @@
>>>> +* Maxim max11100 Analog to Digital Converter (ADC)
>>>> +
>>>> +Required properties:
>>>> +  - compatible: Should be "maxim,max11100"
>>>> +  - vref-supply: phandle to the regulator that provides reference voltage
>>>> +
>>>> +Optional properties:
>>>> +  - spi-max-frequency: SPI maximum frequency
>>>> +
>>>> +Example:
>>>> +
>>>> +adc0: max11100@0 {
>>>> +        compatible = "maxim,max11100";
>>>> +        vref-supply = <&adc0_vref>;
>>>> +        spi-max-frequency = <240000>;
>>>> +};
>>>> +
>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>> index 99c0514..a909484 100644
>>>> --- a/drivers/iio/adc/Kconfig
>>>> +++ b/drivers/iio/adc/Kconfig
>>>> @@ -285,6 +285,15 @@ config MAX1027
>>>>        To compile this driver as a module, choose M here: the module will be
>>>>        called max1027.
>>>>
>>>> +config MAX11100
>>>> +    tristate "Maxim max11100 ADC driver"
>>>> +    depends on SPI
>>>
>>> SPI_MASTER is more precise I think
>>>
>>>> +    help
>>>> +      Say yes here to build support for Maxim max11100 SPI ADC
>>>> +
>>>> +      To compile this driver as a module, choose M here: the module will be
>>>> +      called max11100.
>>>> +
>>>>  config MAX1363
>>>>      tristate "Maxim max1363 ADC driver"
>>>>      depends on I2C
>>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>>> index 7a40c04..1463044 100644
>>>> --- a/drivers/iio/adc/Makefile
>>>> +++ b/drivers/iio/adc/Makefile
>>>> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>>>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>>>>  obj-$(CONFIG_LTC2485) += ltc2485.o
>>>>  obj-$(CONFIG_MAX1027) += max1027.o
>>>> +obj-$(CONFIG_MAX11100) += max11100.o
>>>>  obj-$(CONFIG_MAX1363) += max1363.o
>>>>  obj-$(CONFIG_MCP320X) += mcp320x.o
>>>>  obj-$(CONFIG_MCP3422) += mcp3422.o
>>>> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
>>>> new file mode 100644
>>>> index 0000000..f372ad8
>>>> --- /dev/null
>>>> +++ b/drivers/iio/adc/max11100.c
>>>> @@ -0,0 +1,166 @@
>>>> +/*
>>>> + * iio/adc/max11100.c
>>>> + * Maxim max11100 ADC Driver with IIO interface
>>>> + *
>>>> + * Copyright (C) 2016 Renesas Electronics Corporation
>>>> + * Copyright (C) 2016 Jacopo Mondi
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + */
>>>> +#include <linux/delay.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <linux/spi/spi.h>
>>>> +
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/buffer.h>
>>>> +#include <linux/iio/driver.h>
>>>> +
>>>> +/*
>>>> + * LSB is the ADC single digital step
>>>> + * 1 LSB = (vref / 2 ^ 16)
>>>> + * AIN = (DIN * LSB)
>>>> + */
>>>> +#define MAX11100_LSB_DIV        (1 << 16)
>>>> +#define MAX11100_LSB(vref)         (vref / MAX11100_LSB_DIV)
>>>
>>> maybe parenthesis around vref
>>>
>>>> +
>>>> +struct max11100_state {
>>>> +    const struct max11100_chip_desc *desc;
>>>> +    struct spi_device *spi;
>>>> +    int vref_uv;
>>>> +    struct mutex lock;
>>>> +};
>>>> +
>>>> +static struct iio_chan_spec max11100_channels[] = {
>>>> +    { /* [0] */
>>>> +        .type = IIO_VOLTAGE,
>>>> +        .scan_type = {
>>>
>>> scan_type not needed since driver does not support buffered reads
>>>
>>>> +            .sign = 'u',
>>>> +            .realbits = 16,
>>>> +            .storagebits = 24,
>>>> +            .shift = 8,
>>>> +            .repeat = 1,
>>>> +            .endianness = IIO_BE,
>>>> +        },
>>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>> +    },
>>>> +};
>>>> +
>>>> +static struct max11100_chip_desc {
>>>> +    unsigned int num_chan;
>>>> +    const struct iio_chan_spec *channels;
>>>> +} max11100_desc = {
>>>> +    .num_chan = ARRAY_SIZE(max11100_channels),
>>>> +    .channels = max11100_channels,
>>>> +};
>>>> +
>>>> +static int max11100_read_raw(struct iio_dev *indio_dev,
>>>> +                 struct iio_chan_spec const *chan,
>>>> +                 int *val, int *val2, long mask)
>>>> +{
>>>> +    int ret;
>>>> +    struct max11100_state *state = iio_priv(indio_dev);
>>>> +    uint8_t buffer[3];
>>>> +
>>>> +    mutex_lock(&state->lock);
>>>> +
>>>> +    ret = spi_read(state->spi, buffer, sizeof(buffer));
>>>> +    if (ret) {
>>>> +        mutex_unlock(&state->lock);
>>>> +        dev_err(&indio_dev->dev, "SPI transfer failed\n");
>>>> +        return ret;
>>>> +    }
>>>> +    mutex_unlock(&state->lock);
>>>> +
>>>> +    /* the first 8 bits sent out from ADC must be 0s */
>>>> +    if (buffer[0]) {
>>>> +        dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    *val = be16_to_cpu(*(uint16_t *)&buffer[1]);
>>>> +    *val = *val * MAX11100_LSB(state->vref_uv) / 1000;
>>>
>>> no, INFO_RAW shall not perform such scaling, use _PROCESSED or add an
>>> INFO_SCALE to indicate the scaling
>>
>> Here I am not scaling the result, just converting the digital value read from ADC into millivolts.
>> The transfer function from Din to Ain depends on vref, in the form reported in comments in file header:
>>
>> Ain = Din * (vref / 2^16)
>>
>> I am using microvolts as "vref" unit otherwise I would have been forced to deal with floating point arithmetic.
> That's what the _scale elements of the IIO ABI meant for.  Make even this simple
> conversion the job of userspace which is better placed to do any magic it wishes
> to do with how it does the conversion (or whether it does depending on the
> application).  So expectation is that raw means whatever came from the ADC so
> ADC 'counts' not mV or similar.
>

Ok, fine, I now get it.
Userspace perform conversion using values returned from _RAW and _SCALE.

As the conversion process is device-specific (in my case is the simple 
transfer function reported some lines above), should it be documented 
somewhere?

Thanks
    j

> Jonathan
>>
>> Thanks
>>    j
>>
>>>
>>>> +
>>>> +    return IIO_VAL_INT;
>>>> +}
>>>> +
>>>> +static const struct iio_info max11100_info = {
>>>> +    .driver_module = THIS_MODULE,
>>>> +    .read_raw = max11100_read_raw,
>>>> +};
>>>> +
>>>> +static int max11100_probe(struct spi_device *spi)
>>>> +{
>>>> +    int ret;
>>>> +    struct iio_dev *indio_dev;
>>>> +    struct regulator *vref_reg;
>>>> +    struct max11100_state *state;
>>>> +
>>>> +    indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
>>>> +    if (!indio_dev)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    spi_set_drvdata(spi, indio_dev);
>>>> +
>>>> +    state = iio_priv(indio_dev);
>>>> +    state->spi = spi;
>>>> +    state->desc = &max11100_desc;
>>>> +
>>>> +    mutex_init(&state->lock);
>>>> +
>>>> +    indio_dev->dev.parent = &spi->dev;
>>>> +    indio_dev->dev.of_node = spi->dev.of_node;
>>>> +    indio_dev->info = &max11100_info;
>>>> +    indio_dev->modes = INDIO_DIRECT_MODE;
>>>> +    indio_dev->channels = state->desc->channels;
>>>> +    indio_dev->num_channels = state->desc->num_chan;
>>>> +
>>>> +    vref_reg = devm_regulator_get(&spi->dev, "vref");
>>>> +    if (IS_ERR(vref_reg))
>>>> +        return PTR_ERR(vref_reg);
>>>> +
>>>> +    ret = regulator_enable(vref_reg);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    state->vref_uv = regulator_get_voltage(vref_reg);
>>>> +    if (state->vref_uv < 0) {
>>>> +        /* dummy regulator "get_voltage" returns -EINVAL as well */
>>>> +        ret = -EINVAL;
>>>> +        goto disable_regulator;
>>>> +    }
>>>> +
>>>> +    ret = devm_iio_device_register(&spi->dev, indio_dev);
>>>
>>> the regulator needs to be disabled in a _remove() function and since you
>>> need a remove function, devm_iio_device_register() should not be used
>>>
>>>> +    if (ret)
>>>> +        goto disable_regulator;
>>>> +
>>>> +    return 0;
>>>> +
>>>> +disable_regulator:
>>>> +    regulator_disable(vref_reg);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static const struct of_device_id max11100_ids[] = {
>>>> +    {.compatible = "maxim,max11100"},
>>>> +    { },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, max11100_ids);
>>>> +
>>>> +static struct spi_driver max11100_driver = {
>>>> +    .driver = {
>>>> +        .name    = "max11100",
>>>> +        .owner    = THIS_MODULE,
>>>> +        .of_match_table = of_match_ptr(max11100_ids),
>>>> +    },
>>>> +    .probe        = max11100_probe,
>>>> +};
>>>> +
>>>> +module_spi_driver(max11100_driver);
>>>> +
>>>> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
>>>> +MODULE_DESCRIPTION("Maxim max11100 ADC Driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[Jonathan Cameron]

On 02/01/17 09:19, jacopo mondi wrote:
> Hi Jonathan,
>    thanks for review
> 
> On 30/12/2016 17:31, Jonathan Cameron wrote:
>> On 14/12/16 12:00, jacopo@jmondi.org wrote:
>>> Hello Peter,
>>>    thanks for review
>>>
>>> On 13/12/2016 21:33, Peter Meerwald-Stadler wrote:
>>>>
>>>>> Add IIO driver for Maxim MAX11100 single-channel ADC.
>>>>> Add DT bindings documentation.
>>>>
>>>> some more comments
>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>> ---
>>>>>
>>>>> v1 -> v2:
>>>>>     - incorporated pmeerw's review comments
>>>>>     - retrieve vref from dts and use that to convert read_raw result
>>>>>       to mV
>>>>>     - add device tree bindings documentation
>>>>>
>>>>> ---
>>>>>  .../devicetree/bindings/iio/adc/max11100.txt       |  17 +++
>>>>>  drivers/iio/adc/Kconfig                            |   9 ++
>>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>>  drivers/iio/adc/max11100.c                         | 166 +++++++++++++++++++++
>>>>>  4 files changed, 193 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>>>>>  create mode 100644 drivers/iio/adc/max11100.c
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt b/Documentation/devicetree/bindings/iio/adc/max11100.txt
>>>>> new file mode 100644
>>>>> index 0000000..6877c11
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
>>>>> @@ -0,0 +1,17 @@
>>>>> +* Maxim max11100 Analog to Digital Converter (ADC)
>>>>> +
>>>>> +Required properties:
>>>>> +  - compatible: Should be "maxim,max11100"
>>>>> +  - vref-supply: phandle to the regulator that provides reference voltage
>>>>> +
>>>>> +Optional properties:
>>>>> +  - spi-max-frequency: SPI maximum frequency
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +adc0: max11100@0 {
>>>>> +        compatible = "maxim,max11100";
>>>>> +        vref-supply = <&adc0_vref>;
>>>>> +        spi-max-frequency = <240000>;
>>>>> +};
>>>>> +
>>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>>> index 99c0514..a909484 100644
>>>>> --- a/drivers/iio/adc/Kconfig
>>>>> +++ b/drivers/iio/adc/Kconfig
>>>>> @@ -285,6 +285,15 @@ config MAX1027
>>>>>        To compile this driver as a module, choose M here: the module will be
>>>>>        called max1027.
>>>>>
>>>>> +config MAX11100
>>>>> +    tristate "Maxim max11100 ADC driver"
>>>>> +    depends on SPI
>>>>
>>>> SPI_MASTER is more precise I think
>>>>
>>>>> +    help
>>>>> +      Say yes here to build support for Maxim max11100 SPI ADC
>>>>> +
>>>>> +      To compile this driver as a module, choose M here: the module will be
>>>>> +      called max11100.
>>>>> +
>>>>>  config MAX1363
>>>>>      tristate "Maxim max1363 ADC driver"
>>>>>      depends on I2C
>>>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>>>> index 7a40c04..1463044 100644
>>>>> --- a/drivers/iio/adc/Makefile
>>>>> +++ b/drivers/iio/adc/Makefile
>>>>> @@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>>>>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>>>>>  obj-$(CONFIG_LTC2485) += ltc2485.o
>>>>>  obj-$(CONFIG_MAX1027) += max1027.o
>>>>> +obj-$(CONFIG_MAX11100) += max11100.o
>>>>>  obj-$(CONFIG_MAX1363) += max1363.o
>>>>>  obj-$(CONFIG_MCP320X) += mcp320x.o
>>>>>  obj-$(CONFIG_MCP3422) += mcp3422.o
>>>>> diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
>>>>> new file mode 100644
>>>>> index 0000000..f372ad8
>>>>> --- /dev/null
>>>>> +++ b/drivers/iio/adc/max11100.c
>>>>> @@ -0,0 +1,166 @@
>>>>> +/*
>>>>> + * iio/adc/max11100.c
>>>>> + * Maxim max11100 ADC Driver with IIO interface
>>>>> + *
>>>>> + * Copyright (C) 2016 Renesas Electronics Corporation
>>>>> + * Copyright (C) 2016 Jacopo Mondi
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License version 2 as
>>>>> + * published by the Free Software Foundation.
>>>>> + */
>>>>> +#include <linux/delay.h>
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/regulator/consumer.h>
>>>>> +#include <linux/spi/spi.h>
>>>>> +
>>>>> +#include <linux/iio/iio.h>
>>>>> +#include <linux/iio/buffer.h>
>>>>> +#include <linux/iio/driver.h>
>>>>> +
>>>>> +/*
>>>>> + * LSB is the ADC single digital step
>>>>> + * 1 LSB = (vref / 2 ^ 16)
>>>>> + * AIN = (DIN * LSB)
>>>>> + */
>>>>> +#define MAX11100_LSB_DIV        (1 << 16)
>>>>> +#define MAX11100_LSB(vref)         (vref / MAX11100_LSB_DIV)
>>>>
>>>> maybe parenthesis around vref
>>>>
>>>>> +
>>>>> +struct max11100_state {
>>>>> +    const struct max11100_chip_desc *desc;
>>>>> +    struct spi_device *spi;
>>>>> +    int vref_uv;
>>>>> +    struct mutex lock;
>>>>> +};
>>>>> +
>>>>> +static struct iio_chan_spec max11100_channels[] = {
>>>>> +    { /* [0] */
>>>>> +        .type = IIO_VOLTAGE,
>>>>> +        .scan_type = {
>>>>
>>>> scan_type not needed since driver does not support buffered reads
>>>>
>>>>> +            .sign = 'u',
>>>>> +            .realbits = 16,
>>>>> +            .storagebits = 24,
>>>>> +            .shift = 8,
>>>>> +            .repeat = 1,
>>>>> +            .endianness = IIO_BE,
>>>>> +        },
>>>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>>> +    },
>>>>> +};
>>>>> +
>>>>> +static struct max11100_chip_desc {
>>>>> +    unsigned int num_chan;
>>>>> +    const struct iio_chan_spec *channels;
>>>>> +} max11100_desc = {
>>>>> +    .num_chan = ARRAY_SIZE(max11100_channels),
>>>>> +    .channels = max11100_channels,
>>>>> +};
>>>>> +
>>>>> +static int max11100_read_raw(struct iio_dev *indio_dev,
>>>>> +                 struct iio_chan_spec const *chan,
>>>>> +                 int *val, int *val2, long mask)
>>>>> +{
>>>>> +    int ret;
>>>>> +    struct max11100_state *state = iio_priv(indio_dev);
>>>>> +    uint8_t buffer[3];
>>>>> +
>>>>> +    mutex_lock(&state->lock);
>>>>> +
>>>>> +    ret = spi_read(state->spi, buffer, sizeof(buffer));
>>>>> +    if (ret) {
>>>>> +        mutex_unlock(&state->lock);
>>>>> +        dev_err(&indio_dev->dev, "SPI transfer failed\n");
>>>>> +        return ret;
>>>>> +    }
>>>>> +    mutex_unlock(&state->lock);
>>>>> +
>>>>> +    /* the first 8 bits sent out from ADC must be 0s */
>>>>> +    if (buffer[0]) {
>>>>> +        dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    *val = be16_to_cpu(*(uint16_t *)&buffer[1]);
>>>>> +    *val = *val * MAX11100_LSB(state->vref_uv) / 1000;
>>>>
>>>> no, INFO_RAW shall not perform such scaling, use _PROCESSED or add an
>>>> INFO_SCALE to indicate the scaling
>>>
>>> Here I am not scaling the result, just converting the digital value read from ADC into millivolts.
>>> The transfer function from Din to Ain depends on vref, in the form reported in comments in file header:
>>>
>>> Ain = Din * (vref / 2^16)
>>>
>>> I am using microvolts as "vref" unit otherwise I would have been forced to deal with floating point arithmetic.
>> That's what the _scale elements of the IIO ABI meant for.  Make even this simple
>> conversion the job of userspace which is better placed to do any magic it wishes
>> to do with how it does the conversion (or whether it does depending on the
>> application).  So expectation is that raw means whatever came from the ADC so
>> ADC 'counts' not mV or similar.
>>
> 
> Ok, fine, I now get it.
> Userspace perform conversion using values returned from _RAW and _SCALE.
> 
> As the conversion process is device-specific (in my case is the
> simple transfer function reported some lines above), should it be
> documented somewhere?
It should conform to the documentation in 
Documentation/ABI/test/sysfs-bus-iio

Userspace needs to be sure that is the case, otherwise it'll never know what
to do with it.

Jonathan
> 
> Thanks j
> 
>> Jonathan
>>>
>>> Thanks
>>>    j
>>>
>>>>
>>>>> +
>>>>> +    return IIO_VAL_INT;
>>>>> +}
>>>>> +
>>>>> +static const struct iio_info max11100_info = {
>>>>> +    .driver_module = THIS_MODULE,
>>>>> +    .read_raw = max11100_read_raw,
>>>>> +};
>>>>> +
>>>>> +static int max11100_probe(struct spi_device *spi)
>>>>> +{
>>>>> +    int ret;
>>>>> +    struct iio_dev *indio_dev;
>>>>> +    struct regulator *vref_reg;
>>>>> +    struct max11100_state *state;
>>>>> +
>>>>> +    indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
>>>>> +    if (!indio_dev)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    spi_set_drvdata(spi, indio_dev);
>>>>> +
>>>>> +    state = iio_priv(indio_dev);
>>>>> +    state->spi = spi;
>>>>> +    state->desc = &max11100_desc;
>>>>> +
>>>>> +    mutex_init(&state->lock);
>>>>> +
>>>>> +    indio_dev->dev.parent = &spi->dev;
>>>>> +    indio_dev->dev.of_node = spi->dev.of_node;
>>>>> +    indio_dev->info = &max11100_info;
>>>>> +    indio_dev->modes = INDIO_DIRECT_MODE;
>>>>> +    indio_dev->channels = state->desc->channels;
>>>>> +    indio_dev->num_channels = state->desc->num_chan;
>>>>> +
>>>>> +    vref_reg = devm_regulator_get(&spi->dev, "vref");
>>>>> +    if (IS_ERR(vref_reg))
>>>>> +        return PTR_ERR(vref_reg);
>>>>> +
>>>>> +    ret = regulator_enable(vref_reg);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    state->vref_uv = regulator_get_voltage(vref_reg);
>>>>> +    if (state->vref_uv < 0) {
>>>>> +        /* dummy regulator "get_voltage" returns -EINVAL as well */
>>>>> +        ret = -EINVAL;
>>>>> +        goto disable_regulator;
>>>>> +    }
>>>>> +
>>>>> +    ret = devm_iio_device_register(&spi->dev, indio_dev);
>>>>
>>>> the regulator needs to be disabled in a _remove() function and since you
>>>> need a remove function, devm_iio_device_register() should not be used
>>>>
>>>>> +    if (ret)
>>>>> +        goto disable_regulator;
>>>>> +
>>>>> +    return 0;
>>>>> +
>>>>> +disable_regulator:
>>>>> +    regulator_disable(vref_reg);
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static const struct of_device_id max11100_ids[] = {
>>>>> +    {.compatible = "maxim,max11100"},
>>>>> +    { },
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, max11100_ids);
>>>>> +
>>>>> +static struct spi_driver max11100_driver = {
>>>>> +    .driver = {
>>>>> +        .name    = "max11100",
>>>>> +        .owner    = THIS_MODULE,
>>>>> +        .of_match_table = of_match_ptr(max11100_ids),
>>>>> +    },
>>>>> +    .probe        = max11100_probe,
>>>>> +};
>>>>> +
>>>>> +module_spi_driver(max11100_driver);
>>>>> +
>>>>> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
>>>>> +MODULE_DESCRIPTION("Maxim max11100 ADC Driver");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>>
>>>>
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>