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