[PATCH v5 0/2] iio: adc: Add Maxim MAX11100 driver

[PATCH v5 0/2] iio: adc: Add Maxim MAX11100 driver

[Jacopo Mondi]

Hello everyone,
  hopefully we have reached last iteration on this simple driver.

After having clarified the read_raw() measure unit question, I'm now sending
out v5 addressing Jonathan's comment on having spi destination aligned to
their own cache lines to support DMA-capable SPI controllers.

Also, I added indio_dev->name attribute, as suggested by Lars-Peter even if
the same value can be retrieved from of_node/name.


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

v2 -> v3:
    - add _SCALE bit of read_raw function and change _RAW bit accordingly
    - call regulator_get_voltage when accessing the _SCALE part of read_raw
      and not during probe
    - add back remove function as regulator has to be disabled when detaching
      the module. Do not use devm_ version of iio_register/unregister functions
      anymore but do unregister in the remove.
    - remove mutex as access to SPI bus is protected by SPI core. Thanks marex

v3 -> v4:
    - split device tree binding documentation and actual ADC driver
    - add "reg" to the list of required properties and use a better
      namimg for the adc device node in bindings documentation as suggested
      by Geert.

v4 -> v5:
    - make spi_read() destination buffer cacheline aligned
    - add indio_dev->name attribute.

Jacopo Mondi (2):
  iio: adc: Add Maxim MAX11100 driver
  dt-bindings: iio: document MAX11100 ADC

 .../devicetree/bindings/iio/adc/max11100.txt       |  19 ++
 drivers/iio/adc/Kconfig                            |   9 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/max11100.c                         | 192 +++++++++++++++++++++
 4 files changed, 221 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
 create mode 100644 drivers/iio/adc/max11100.c

-- 
2.7.4

[PATCHv5 1/2] iio: adc: Add Maxim MAX11100 driver

[Jacopo Mondi]

From: Jacopo Mondi <jacopo@jmondi.org>

Add iio driver for Maxim MAX11100 single-channel ADC.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Tested-by: Marek Vasut <marek.vasut@gmail.com>
---
 drivers/iio/adc/Kconfig    |   9 +++
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/max11100.c | 192 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 202 insertions(+)
 create mode 100644 drivers/iio/adc/max11100.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 38bc319..c32bc7a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -307,6 +307,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_MASTER
+	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 d36c4be..5684369 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -31,6 +31,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..b738ecf
--- /dev/null
+++ b/drivers/iio/adc/max11100.c
@@ -0,0 +1,192 @@
+/*
+ * iio/adc/max11100.c
+ * Maxim max11100 ADC Driver with IIO interface
+ *
+ * Copyright (C) 2016-17 Renesas Electronics Corporation
+ * Copyright (C) 2016-17 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/driver.h>
+
+/*
+ * LSB is the ADC single digital step
+ * 1 LSB = (vref_mv / 2 ^ 16)
+ *
+ * LSB is used to calculate analog voltage value
+ * from the number of ADC steps count
+ *
+ * Ain = (count * LSB)
+ */
+#define MAX11100_LSB_DIV		(1 << 16)
+
+struct max11100_state {
+	const struct max11100_chip_desc *desc;
+	struct regulator *vref_reg;
+	struct spi_device *spi;
+
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	u8 buffer[3] ____cacheline_aligned;
+};
+
+static struct iio_chan_spec max11100_channels[] = {
+	{ /* [0] */
+		.type = IIO_VOLTAGE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+	},
+};
+
+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_single(struct iio_dev *indio_dev, int *val)
+{
+	int ret;
+	struct max11100_state *state = iio_priv(indio_dev);
+
+	ret = spi_read(state->spi, state->buffer, sizeof(state->buffer));
+	if (ret) {
+		dev_err(&indio_dev->dev, "SPI transfer failed\n");
+		return ret;
+	}
+
+	/* the first 8 bits sent out from ADC must be 0s */
+	if (state->buffer[0]) {
+		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
+		return -EINVAL;
+	}
+
+	*val = (state->buffer[1] << 8) | state->buffer[2];
+
+	return 0;
+}
+
+static int max11100_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long info)
+{
+	int ret, vref_uv;
+	struct max11100_state *state = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		ret = max11100_read_single(indio_dev, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		vref_uv = regulator_get_voltage(state->vref_reg);
+		if (vref_uv < 0)
+			/* dummy regulator "get_voltage" returns -EINVAL */
+			return -EINVAL;
+
+		*val =  vref_uv / 1000;
+		*val2 = MAX11100_LSB_DIV;
+		return IIO_VAL_FRACTIONAL;
+	}
+
+	return -EINVAL;
+}
+
+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)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, indio_dev);
+
+	state = iio_priv(indio_dev);
+	state->spi = spi;
+	state->desc = &max11100_desc;
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->dev.of_node = spi->dev.of_node;
+	indio_dev->name = "max11100";
+	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;
+
+	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
+	if (IS_ERR(state->vref_reg))
+		return PTR_ERR(state->vref_reg);
+
+	ret = regulator_enable(state->vref_reg);
+	if (ret)
+		return ret;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto disable_regulator;
+
+	return 0;
+
+disable_regulator:
+	regulator_disable(state->vref_reg);
+
+	return ret;
+}
+
+static int max11100_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct max11100_state *state = iio_priv(indio_dev);
+
+	regulator_disable(state->vref_reg);
+
+	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: [PATCHv5 1/2] iio: adc: Add Maxim MAX11100 driver

[Peter Meerwald-Stadler]

> Add iio driver for Maxim MAX11100 single-channel ADC.

minor comments, maybe Jonathan can fix it up when taking this...
 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Tested-by: Marek Vasut <marek.vasut@gmail.com>
> ---
>  drivers/iio/adc/Kconfig    |   9 +++
>  drivers/iio/adc/Makefile   |   1 +
>  drivers/iio/adc/max11100.c | 192 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 202 insertions(+)
>  create mode 100644 drivers/iio/adc/max11100.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 38bc319..c32bc7a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -307,6 +307,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_MASTER
> +	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 d36c4be..5684369 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -31,6 +31,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..b738ecf
> --- /dev/null
> +++ b/drivers/iio/adc/max11100.c
> @@ -0,0 +1,192 @@
> +/*
> + * iio/adc/max11100.c
> + * Maxim max11100 ADC Driver with IIO interface
> + *
> + * Copyright (C) 2016-17 Renesas Electronics Corporation
> + * Copyright (C) 2016-17 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/driver.h>
> +
> +/*
> + * LSB is the ADC single digital step
> + * 1 LSB = (vref_mv / 2 ^ 16)
> + *
> + * LSB is used to calculate analog voltage value
> + * from the number of ADC steps count
> + *
> + * Ain = (count * LSB)
> + */
> +#define MAX11100_LSB_DIV		(1 << 16)
> +
> +struct max11100_state {
> +	const struct max11100_chip_desc *desc;
> +	struct regulator *vref_reg;
> +	struct spi_device *spi;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 */
> +	u8 buffer[3] ____cacheline_aligned;
> +};
> +
> +static struct iio_chan_spec max11100_channels[] = {
> +	{ /* [0] */
> +		.type = IIO_VOLTAGE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +};
> +
> +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,
> +};

for just one supported chip, this is more complicated than necessary...

> +
> +static int max11100_read_single(struct iio_dev *indio_dev, int *val)
> +{
> +	int ret;
> +	struct max11100_state *state = iio_priv(indio_dev);
> +
> +	ret = spi_read(state->spi, state->buffer, sizeof(state->buffer));
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "SPI transfer failed\n");
> +		return ret;
> +	}
> +
> +	/* the first 8 bits sent out from ADC must be 0s */
> +	if (state->buffer[0]) {
> +		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
> +		return -EINVAL;
> +	}
> +
> +	*val = (state->buffer[1] << 8) | state->buffer[2];
> +
> +	return 0;
> +}
> +
> +static int max11100_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long info)
> +{
> +	int ret, vref_uv;
> +	struct max11100_state *state = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = max11100_read_single(indio_dev, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		vref_uv = regulator_get_voltage(state->vref_reg);
> +		if (vref_uv < 0)
> +			/* dummy regulator "get_voltage" returns -EINVAL */
> +			return -EINVAL;
> +
> +		*val =  vref_uv / 1000;
> +		*val2 = MAX11100_LSB_DIV;
> +		return IIO_VAL_FRACTIONAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +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)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	state = iio_priv(indio_dev);
> +	state->spi = spi;
> +	state->desc = &max11100_desc;
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->dev.of_node = spi->dev.of_node;
> +	indio_dev->name = "max11100";
> +	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;
> +
> +	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
> +	if (IS_ERR(state->vref_reg))
> +		return PTR_ERR(state->vref_reg);
> +
> +	ret = regulator_enable(state->vref_reg);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto disable_regulator;
> +
> +	return 0;
> +
> +disable_regulator:
> +	regulator_disable(state->vref_reg);
> +
> +	return ret;
> +}
> +
> +static int max11100_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct max11100_state *state = iio_priv(indio_dev);
> +
> +	regulator_disable(state->vref_reg);

should be the other way around, first _unregister(), then 
regulator_disable()

reverse order as in _probe()

> +
> +	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: [PATCHv5 1/2] iio: adc: Add Maxim MAX11100 driver

[jacopo mondi]

Hi Peter,

On 19/01/2017 19:15, Peter Meerwald-Stadler wrote:
>
>> Add iio driver for Maxim MAX11100 single-channel ADC.
>
> minor comments, maybe Jonathan can fix it up when taking this...
>
>> +struct max11100_state {
>> +	const struct max11100_chip_desc *desc;
>> +	struct regulator *vref_reg;
>> +	struct spi_device *spi;
>> +
>> +	/*
>> +	 * DMA (thus cache coherency maintenance) requires the
>> +	 * transfer buffers to live in their own cache lines.
>> +	 */
>> +	u8 buffer[3] ____cacheline_aligned;
>> +};
>> +
>> +static struct iio_chan_spec max11100_channels[] = {
>> +	{ /* [0] */
>> +		.type = IIO_VOLTAGE,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +				      BIT(IIO_CHAN_INFO_SCALE),
>> +	},
>> +};
>> +
>> +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,
>> +};
>
> for just one supported chip, this is more complicated than necessary...
>

I can get rid of this, yes, but it's much nicer than hard-coding those 
values in  indio_dev fields ;)

>> +
>> +static int max11100_read_single(struct iio_dev *indio_dev, int *val)
>> +{
>> +	int ret;
>> +	struct max11100_state *state = iio_priv(indio_dev);
>> +
>> +	ret = spi_read(state->spi, state->buffer, sizeof(state->buffer));
>> +	if (ret) {
>> +		dev_err(&indio_dev->dev, "SPI transfer failed\n");
>> +		return ret;
>> +	}
>> +
>> +	/* the first 8 bits sent out from ADC must be 0s */
>> +	if (state->buffer[0]) {
>> +		dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	*val = (state->buffer[1] << 8) | state->buffer[2];
>> +
>> +	return 0;
>> +}
>> +
>> +static int max11100_read_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int *val, int *val2, long info)
>> +{
>> +	int ret, vref_uv;
>> +	struct max11100_state *state = iio_priv(indio_dev);
>> +
>> +	switch (info) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = max11100_read_single(indio_dev, val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		vref_uv = regulator_get_voltage(state->vref_reg);
>> +		if (vref_uv < 0)
>> +			/* dummy regulator "get_voltage" returns -EINVAL */
>> +			return -EINVAL;
>> +
>> +		*val =  vref_uv / 1000;
>> +		*val2 = MAX11100_LSB_DIV;
>> +		return IIO_VAL_FRACTIONAL;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +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)
>> +		return -ENOMEM;
>> +
>> +	spi_set_drvdata(spi, indio_dev);
>> +
>> +	state = iio_priv(indio_dev);
>> +	state->spi = spi;
>> +	state->desc = &max11100_desc;
>> +
>> +	indio_dev->dev.parent = &spi->dev;
>> +	indio_dev->dev.of_node = spi->dev.of_node;
>> +	indio_dev->name = "max11100";
>> +	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;
>> +
>> +	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
>> +	if (IS_ERR(state->vref_reg))
>> +		return PTR_ERR(state->vref_reg);
>> +
>> +	ret = regulator_enable(state->vref_reg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret)
>> +		goto disable_regulator;
>> +
>> +	return 0;
>> +
>> +disable_regulator:
>> +	regulator_disable(state->vref_reg);
>> +
>> +	return ret;
>> +}
>> +
>> +static int max11100_remove(struct spi_device *spi)
>> +{
>> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +	struct max11100_state *state = iio_priv(indio_dev);
>> +
>> +	regulator_disable(state->vref_reg);
>
> should be the other way around, first _unregister(), then
> regulator_disable()
>
> reverse order as in _probe()
>

Yeah, that's right...

Let's see, I can send v6 with no issues, or Jonathan can do that if he 
prefers to.
Just let me know :)

Thanks
    j


>> +
>> +	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");
>>
>

Re: [PATCHv5 1/2] iio: adc: Add Maxim MAX11100 driver

[Jonathan Cameron]

On 19/01/17 21:47, jacopo mondi wrote:
> Hi Peter,
> 
> On 19/01/2017 19:15, Peter Meerwald-Stadler wrote:
>>
>>> Add iio driver for Maxim MAX11100 single-channel ADC.
>>
>> minor comments, maybe Jonathan can fix it up when taking this...
>>
>>> +struct max11100_state {
>>> +    const struct max11100_chip_desc *desc;
>>> +    struct regulator *vref_reg;
>>> +    struct spi_device *spi;
>>> +
>>> +    /*
>>> +     * DMA (thus cache coherency maintenance) requires the
>>> +     * transfer buffers to live in their own cache lines.
>>> +     */
>>> +    u8 buffer[3] ____cacheline_aligned;
>>> +};
>>> +
>>> +static struct iio_chan_spec max11100_channels[] = {
>>> +    { /* [0] */
>>> +        .type = IIO_VOLTAGE,
>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>> +                      BIT(IIO_CHAN_INFO_SCALE),
>>> +    },
>>> +};
>>> +
>>> +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,
>>> +};
>>
>> for just one supported chip, this is more complicated than necessary...
>>
> 
> I can get rid of this, yes, but it's much nicer than hard-coding
> those values in indio_dev fields ;)
It's abstraction for something that doesn't need to be abstracted.  Adds
a few lines of code that needs checking for no gain.  Hence
it's gone.
>>> +
>>> +static int max11100_read_single(struct iio_dev *indio_dev, int *val)
>>> +{
>>> +    int ret;
>>> +    struct max11100_state *state = iio_priv(indio_dev);
>>> +
>>> +    ret = spi_read(state->spi, state->buffer, sizeof(state->buffer));
>>> +    if (ret) {
>>> +        dev_err(&indio_dev->dev, "SPI transfer failed\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    /* the first 8 bits sent out from ADC must be 0s */
>>> +    if (state->buffer[0]) {
>>> +        dev_err(&indio_dev->dev, "Invalid value: buffer[0] != 0\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    *val = (state->buffer[1] << 8) | state->buffer[2];
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int max11100_read_raw(struct iio_dev *indio_dev,
>>> +                 struct iio_chan_spec const *chan,
>>> +                 int *val, int *val2, long info)
>>> +{
>>> +    int ret, vref_uv;
>>> +    struct max11100_state *state = iio_priv(indio_dev);
>>> +
>>> +    switch (info) {
>>> +    case IIO_CHAN_INFO_RAW:
>>> +        ret = max11100_read_single(indio_dev, val);
>>> +        if (ret)
>>> +            return ret;
>>> +
>>> +        return IIO_VAL_INT;
>>> +
>>> +    case IIO_CHAN_INFO_SCALE:
>>> +        vref_uv = regulator_get_voltage(state->vref_reg);
>>> +        if (vref_uv < 0)
>>> +            /* dummy regulator "get_voltage" returns -EINVAL */
>>> +            return -EINVAL;
>>> +
>>> +        *val =  vref_uv / 1000;
>>> +        *val2 = MAX11100_LSB_DIV;
>>> +        return IIO_VAL_FRACTIONAL;
>>> +    }
>>> +
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +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)
>>> +        return -ENOMEM;
>>> +
>>> +    spi_set_drvdata(spi, indio_dev);
>>> +
>>> +    state = iio_priv(indio_dev);
>>> +    state->spi = spi;
>>> +    state->desc = &max11100_desc;
>>> +
>>> +    indio_dev->dev.parent = &spi->dev;
>>> +    indio_dev->dev.of_node = spi->dev.of_node;
>>> +    indio_dev->name = "max11100";
>>> +    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;
>>> +
>>> +    state->vref_reg = devm_regulator_get(&spi->dev, "vref");
>>> +    if (IS_ERR(state->vref_reg))
>>> +        return PTR_ERR(state->vref_reg);
>>> +
>>> +    ret = regulator_enable(state->vref_reg);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = iio_device_register(indio_dev);
>>> +    if (ret)
>>> +        goto disable_regulator;
>>> +
>>> +    return 0;
>>> +
>>> +disable_regulator:
>>> +    regulator_disable(state->vref_reg);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int max11100_remove(struct spi_device *spi)
>>> +{
>>> +    struct iio_dev *indio_dev = spi_get_drvdata(spi);
>>> +    struct max11100_state *state = iio_priv(indio_dev);
>>> +
>>> +    regulator_disable(state->vref_reg);
>>
>> should be the other way around, first _unregister(), then
>> regulator_disable()
>>
>> reverse order as in _probe()
>>
> 
> Yeah, that's right...
Fixed up.
> 
> Let's see, I can send v6 with no issues, or Jonathan can do that if he prefers to.
> Just let me know :)
I'll do it as minor stuff.

Applied with changes as above to the togreg branch of iio.git and pushed out
as testing for the autobuilders to play with it.

Thanks,

Jonathan
> 
> Thanks
>    j
> 
> 
>>> +
>>> +    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");
>>>
>>
> 

[PATCHv5 2/2] dt-bindings: iio: document MAX11100 ADC

[Jacopo Mondi]

Add device tree bindings documentation for Maxim MAX11100 single-channel
ADC

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 .../devicetree/bindings/iio/adc/max11100.txt          | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt b/Documentation/devicetree/bindings/iio/adc/max11100.txt
new file mode 100644
index 0000000..ad0bc31
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
@@ -0,0 +1,19 @@
+* Maxim max11100 Analog to Digital Converter (ADC)
+
+Required properties:
+  - compatible: Should be "maxim,max11100"
+  - reg: the adc unit address
+  - vref-supply: phandle to the regulator that provides reference voltage
+
+Optional properties:
+  - spi-max-frequency: SPI maximum frequency
+
+Example:
+
+max11100: adc@0 {
+        compatible = "maxim,max11100";
+        reg = <0>;
+        vref-supply = <&adc0_vref>;
+        spi-max-frequency = <240000>;
+};
+
-- 
2.7.4

Re: [PATCHv5 2/2] dt-bindings: iio: document MAX11100 ADC

[Jonathan Cameron]

On 18/01/17 16:30, Jacopo Mondi wrote:
> Add device tree bindings documentation for Maxim MAX11100 single-channel
> ADC
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Simple enough I'm happy to take it without bothering Rob or Mark. 
However, any device tree bindings at all should be cc'd to them and to the devicetree
list.

Done in case anyone wants to comment.
> ---
>  .../devicetree/bindings/iio/adc/max11100.txt          | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt b/Documentation/devicetree/bindings/iio/adc/max11100.txt
> new file mode 100644
> index 0000000..ad0bc31
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
> @@ -0,0 +1,19 @@
> +* Maxim max11100 Analog to Digital Converter (ADC)
> +
> +Required properties:
> +  - compatible: Should be "maxim,max11100"
> +  - reg: the adc unit address
> +  - vref-supply: phandle to the regulator that provides reference voltage
> +
> +Optional properties:
> +  - spi-max-frequency: SPI maximum frequency
> +
> +Example:
> +
> +max11100: adc@0 {
> +        compatible = "maxim,max11100";
> +        reg = <0>;
> +        vref-supply = <&adc0_vref>;
> +        spi-max-frequency = <240000>;
> +};
> +
> 

Re: [PATCHv5 2/2] dt-bindings: iio: document MAX11100 ADC

[Jonathan Cameron]

On 21/01/17 13:22, Jonathan Cameron wrote:
> On 18/01/17 16:30, Jacopo Mondi wrote:
>> Add device tree bindings documentation for Maxim MAX11100 single-channel
>> ADC
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Simple enough I'm happy to take it without bothering Rob or Mark. 
> However, any device tree bindings at all should be cc'd to them and to the devicetree
> list.
> 
> Done in case anyone wants to comment.

Also applied to the togreg branch of iio.git which is pushed out as testing for
the autobuilders to play with everything in it.

Thanks,

Jonathan
>> ---
>>  .../devicetree/bindings/iio/adc/max11100.txt          | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt b/Documentation/devicetree/bindings/iio/adc/max11100.txt
>> new file mode 100644
>> index 0000000..ad0bc31
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
>> @@ -0,0 +1,19 @@
>> +* Maxim max11100 Analog to Digital Converter (ADC)
>> +
>> +Required properties:
>> +  - compatible: Should be "maxim,max11100"
>> +  - reg: the adc unit address
>> +  - vref-supply: phandle to the regulator that provides reference voltage
>> +
>> +Optional properties:
>> +  - spi-max-frequency: SPI maximum frequency
>> +
>> +Example:
>> +
>> +max11100: adc@0 {
>> +        compatible = "maxim,max11100";
>> +        reg = <0>;
>> +        vref-supply = <&adc0_vref>;
>> +        spi-max-frequency = <240000>;
>> +};
>> +
>>
> 
> --
> 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: [PATCHv5 2/2] dt-bindings: iio: document MAX11100 ADC

[Rob Herring]

On Wed, Jan 18, 2017 at 05:30:53PM +0100, Jacopo Mondi wrote:
> Add device tree bindings documentation for Maxim MAX11100 single-channel
> ADC
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  .../devicetree/bindings/iio/adc/max11100.txt          | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCHv5 2/2] dt-bindings: iio: document MAX11100 ADC

[Jonathan Cameron]

On 21/01/17 20:33, Rob Herring wrote:
> On Wed, Jan 18, 2017 at 05:30:53PM +0100, Jacopo Mondi wrote:
>> Add device tree bindings documentation for Maxim MAX11100 single-channel
>> ADC
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> ---
>>  .../devicetree/bindings/iio/adc/max11100.txt          | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
> 
> Acked-by: Rob Herring <robh@kernel.org>
Thanks and added.

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