Re: [PATCH] iio: Add Maxim MAX11100 driver
Hi Jonathan, On 10/12/2016 19:04, Jonathan Cameron wrote: On 07/12/16 08:29, jac...@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
Re: [PATCH] iio: Add Maxim MAX11100 driver
On 07/12/16 08:29, jac...@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
>>> ---
>>> 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 000..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
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#include
>>> +#include
>>> +
>>> +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
>>
>>> +0x,
>>> +};
>>> +
>>> +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;
Re: [PATCH] iio: Add Maxim MAX11100 driver
On 07/12/16 12:22, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Wed, Dec 7, 2016 at 9:29 AM, jac...@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 --
> ge...@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
Hi Jacopo,
On Wed, Dec 7, 2016 at 9:29 AM, jac...@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 -- ge...@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
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
---
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 000..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
+#include
+#include
+#include
+
+#include
+#include
+
+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
+ 0x,
+};
+
+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_un
Re: [PATCH] iio: Add Maxim MAX11100 driver
On Tue, Dec 6, 2016 at 10:00 PM, Peter Meerwald-Stadler
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 -- ge...@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
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
> ---
> 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 000..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
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +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
> + 0x,
> +};
> +
> +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 (bu
