On 14/12/16 12:00, jacopo@xxxxxxxxxx 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@xxxxxxxxxx>
>>> ---
>>>
>>> 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@xxxxxxxxxx>");
>>> +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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
