Re: [PATCH] iio: adc: add driver for ti adc081s/adc101s/adc121s

From: Jonathan Cameron
Date: Fri Dec 29 2017 - 10:58:20 EST

Next message: Eric Biggers: "[PATCH] crypto: algapi - convert cra_refcnt to refcount_t"
Previous message: Pavel Machek: "v4.15-rc5: resume took way too long, warning in syslog"
In reply to: Milan Stevanovic: "Re: [PATCH] iio: adc: add driver for ti adc081s/adc101s/adc121s"
Next in thread: Milan Stevanovic: "Re: [PATCH] iio: adc: add driver for ti adc081s/adc101s/adc121s"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, 25 Dec 2017 21:42:38 +0100
Milan Stevanovic <milan.o.stevanovic@xxxxxxxxx> wrote:

+CC Lars-Peter and Michael to take a look and see if supporting this
in the AD7746 driver is viable.

> On 12/25/2017 03:21 PM, Philippe Ombredanne wrote:
> > On Sun, Dec 24, 2017 at 8:02 PM, Milan Stevanovic
> > <milan.o.stevanovic@xxxxxxxxx> wrote:
> >> On 12/23/2017 03:13 PM, Philippe Ombredanne wrote:
> >>> Milan,
> >>>
> >>> On Sat, Dec 23, 2017 at 12:19 AM, Milan Stevanovic
> >>> <milan.o.stevanovic@xxxxxxxxx> wrote:
> >>>> Signed-off-by: Milan Stevanovic <milan.o.stevanovic@xxxxxxxxx>
> >>> <snip>
> >>>
> >>> May be it is just me, but you may be missing a commit message?
> >>>
> >>> <snip>
> >> Yes... Sorry I missed this
> >>>> --- /dev/null
> >>>> +++ b/drivers/iio/adc/ti-adc081s.c
> >>>> @@ -0,0 +1,239 @@
> >>>> +/*
> >>>> + * TI ADC081S/ADC101S/ADC121S 8/10/12-bit ADC driver
> >>>> + *
> >>>> + *
> >>>> + * 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.
> >>> Could you consider using an SPDX tag here as documented by Thomas in
> >>> this patch set [1] ?
> >>> This is the new thing for this end of of year and a must have for 2018!
> >>>
> >>> [1] https://lkml.org/lkml/2017/12/4/934
> >> Thanks for this... I added it...
> >>
> >> Best regards
> >> Milan
> >>
> >> From 039f53aa702bd524e53c61b0365d048e2624ef02 Mon Sep 17 00:00:00 2001
> >> From: Milan Stevanovic <milan.o.stevanovic@xxxxxxxxx>
> >> Date: Sat, 23 Dec 2017 00:06:32 +0100
> >> Subject: [PATCH v2] iio: adc: add driver for ti adc081s/adc101s/adc121s
> >>
> >> Linux device driver for single-channel CMOS
> >> 8/10/12-bit analog-to-digital converter with a
> >> high-speed serial interface.
> >>
> >> Signed-off-by: Milan Stevanovic <milan.o.stevanovic@xxxxxxxxx>
> >>
> >> Changes in v2:
> >> - Add SPDX tag
> >> - Add correct git commit message
> > <snip>
> >
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/ti-adc081s.c
> >> @@ -0,0 +1,242 @@
> >> +/*
> >> + * Released under the GPLv2 only.
> >> + * SPDX-License-Identifier: GPL-2.0
> >> + *
> > The SPDX tag should be on the first line using a C++ style comment
> > when in a .c file as explained in Thomas doc
> > And when using an SPDx id, do no repeat a license notice: this would
> > defeat the whole purpose of getting rid of boilerplate.
> >
> >> +// SPDX-License-Identifier: GPL-2.0
> Thanks
>
> Best regards
> Milan

Please don't do this - it makes the patch really hard to review (as it's
not immediately obvious where the latest version is) and very tricky
to apply. New version, new email thread.

>
> From 108841eb108275081958610f03d8c8b0da280666 Mon Sep 17 00:00:00 2001
> From: Milan Stevanovic <milan.o.stevanovic@xxxxxxxxx>
> Date: Sat, 23 Dec 2017 00:06:32 +0100
> Subject: [PATCH v3] iio: adc: add driver for ti adc081s/adc101s/adc121s
>
> Linux device driver for single-channel CMOS
> 8/10/12-bit analog-to-digital converter with a
> high-speed serial interface.
>
> Signed-off-by: Milan Stevanovic <milan.o.stevanovic@xxxxxxxxx>
>
Change log goes below the --- cut line.

The point is that whilst this is interest during the review process it
isn't typically of interest after the patch has been applied and text above
the cut line ends up in the commit log for ever more.

Hmm. You could probably easily add support for this to another simple
spi 1 channel adc driver (ad7746 is probably best). There are a few
things here that driver doesn't yet support, but should be easy to add
them I think.

Take a look at it and see what you think. There are only so many
ways to design a 'dumb' single channel SPI adc and I'd really like to avoid
proliferating drivers just on the basis the devices are made by different
companies. (it took me a while to track down which was the simple
SPI driver so I'm not surprised if you didn't see it!)

Various comments inline.

> Changes in v2:
> - Add SPDX tag
> - Add correct git commit message
>
> Changes in v3:
> - C++ style comment for SPDX
> - update Kconfig file with name of devices
> - Add device tree txt file
> ---
> .../devicetree/bindings/iio/adc/ti-adc081s.txt | 18 ++
> drivers/iio/adc/Kconfig | 10 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-adc081s.c | 238 +++++++++++++++++++++
> 4 files changed, 267 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc081s.txt
> create mode 100644 drivers/iio/adc/ti-adc081s.c
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc081s.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc081s.txt
> new file mode 100644
> index 0000000..7d2a10d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc081s.txt
> @@ -0,0 +1,18 @@
> +* Texas Instruments' ADC081S/ADC101S/ADC121S

As there is a new binding in here this needs to be cc'd to the
device tree maintainers and the devicetree mailing list.

It is simple enough that I can take it without their ack, but
it is always good to give them the opportunity to comment!

> +
> +Required properties:
> + - compatible: Should be one of
> + * "ti,adc081s"
> + * "ti,adc101s"
> + * "ti,adc121s"
> + - reg: spi chip select number for the device
> + - vref-supply: The regulator supply for ADC reference voltage
> + - spi-max-frequency: Max SPI frequency to use (< 400000)
> +
> +Example:
> +adc@0 {
> + compatible = "ti,adc081s";
> + reg = <0>;
> + vref-supply = <&vdd_supply>;
> + spi-max-frequency = <200000>;
> +};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index ef86296..10366d6 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -701,6 +701,16 @@ config TI_ADC081C
> This driver can also be built as a module. If so, the module will be
> called ti-adc081c.
>
> +config TI_ADC081S
> + tristate "Texas Instruments ADC081S/ADC101S/ADC121S family"
> + depends on SPI
> + help
> + If you say yes here you get support for Texas Instruments
> + ADC081S/ADC101S/ADC121S family ADC chips.
> +
> + This driver can also be built as a module. If so, the module will be
> + called ti-adc081s.
> +
> config TI_ADC0832
> tristate "Texas Instruments ADC0831/ADC0832/ADC0834/ADC0838"
> depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 9572c10..020fecf 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
> obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
> obj-$(CONFIG_STM32_ADC) += stm32-adc.o
> obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> +obj-$(CONFIG_TI_ADC081S) += ti-adc081s.o
> obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
> obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
> obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
> diff --git a/drivers/iio/adc/ti-adc081s.c b/drivers/iio/adc/ti-adc081s.c
> new file mode 100644
> index 0000000..e0db4fd
> --- /dev/null
> +++ b/drivers/iio/adc/ti-adc081s.c
> @@ -0,0 +1,238 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * TI ADC081S/ADC101S/ADC121S 8/10/12-bit ADC driver
> + *
> + * Linux device driver for single-channel CMOS
> + * 8/10/12-bit analog-to-digital converter with a
> + * high-speed serial interface.
> + *
> + * Datasheets:
> + * http://www.ti.com/lit/ds/symlink/adc081s021.pdf
> + * http://www.ti.com/lit/ds/symlink/adc101s021.pdf
> + * http://www.ti.com/lit/ds/symlink/adc121s021.pdf
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/spi/spi.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/acpi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>

I don't think you are using anything from buffer.h yet.

> +#include <linux/regulator/consumer.h>
> +
> +struct adc081s {
> + struct spi_device *spi;
> + struct regulator *reg;
> + struct mutex lock;
> +
> + /* 8, 10 or 12 */
> + int bits;
> +};
> +
> +static int adc081s_read_raw(struct iio_dev *iio,
> + struct iio_chan_spec const *channel, int *value,
> + int *shift, long mask)
> +{
> + struct adc081s *adc = iio_priv(iio);
> + int ret;
> + __be16 buf;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&adc->lock);
> + ret = spi_read(adc->spi, (void *) &buf, 2);
Unlikely many spi hosts will use DMA for such a small transfer but you never
know. spi_read passes the buf pointer straight through to the host driver
which can then do dma. As such, buf needs to be dma safe - that is it needs
to be in in it's own cacheline.

2 ways fo doing this:
1) Locally allocate it on the heap and free it. All stack allocations start
on a new cacheline
2) Put it at the end of your adc801s structure and mark it __cacheline_aligned.
The private data is very carefully padded on the end of struct iio_dev to ensure
that any so marked fields will be at the start of a new cacheline.

Option 2 preferred as it avoids lots of allocate and free cycles.

> + mutex_unlock(&adc->lock);
> + if (ret)
> + return ret;
> + *value = (be16_to_cpu(buf) & 0x0FFF) >> (12 - adc->bits);
> + *value = sign_extend32(*value, channel->scan_type.realbits - 1);

It's not signed that I can see so why are we sign extending.

> +
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *value = regulator_get_voltage(adc->reg);
> + if (*value < 0)
> + return *value;
> +
> + /* convert regulator output voltage to mV */
> + *value /= 1000;
> + *shift = adc->bits;
> +
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + default:
> + break;
> + }
> +
> + return -EINVAL;
> +}
> +
> +#define ADCxx1S_CHAN(_bits) { \
> + .type = IIO_VOLTAGE, \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = (_bits), \
> + .storagebits = 16, \
> + .shift = 12 - (_bits), \
> + .endianness = IIO_CPU, \

These are only needed if you support buffered mode which your driver
doesn't (yet). For now just don't bother specifying them.

> + }, \
> +}
> +
> +#define DEFINE_ADCxx1S_CHANNELS(_name, _bits) \
> + static const struct iio_chan_spec _name ## _channels[] = { \
> + ADCxx1S_CHAN((_bits)), \

Don't need the double brackets.

> + IIO_CHAN_SOFT_TIMESTAMP(1), \
> + } \
> +
> +#define ADC081S_NUM_CHANNELS 2
> +
> +struct adcxx1s_model {
> + const struct iio_chan_spec *channels;
> + int bits;
> +};
> +
> +#define ADCxx1S_MODEL(_name, _bits) \
> + { \
> + .channels = _name ## _channels, \
> + .bits = (_bits), \
> + }
> +
> +DEFINE_ADCxx1S_CHANNELS(adc081s, 8);

Usual convention is to have upper case XXs for wild
cards. Actually preferred to just use a part number.
The chances of support being added to the driver for a part that does
not work with these wild cards is very high. As you have gone with
ADC081S for the driver name, just that throughout.

> +DEFINE_ADCxx1S_CHANNELS(adc101s, 10);
> +DEFINE_ADCxx1S_CHANNELS(adc121s, 12);
> +
> +/* Model ids are indexes in _models array */
> +enum adcxx1s_model_id {
> + ADC081S = 0,
> + ADC101S = 1,
> + ADC121S = 2,
> +};
> +
> +static struct adcxx1s_model adcxx1s_models[] = {
> + ADCxx1S_MODEL(adc081s, 8),
> + ADCxx1S_MODEL(adc101s, 10),
> + ADCxx1S_MODEL(adc121s, 12),
> +};
> +
> +static const struct iio_info adc081s_info = {
> + .read_raw = adc081s_read_raw,
> +};
> +
> +static int adc081s_probe(struct spi_device *spi)
> +{
> + struct iio_dev *iio;
> + struct adc081s *adc;
> + struct adcxx1s_model *model;
> + int err;
> +
> + if (ACPI_COMPANION(&spi->dev)) {
> + const struct acpi_device_id *ad_id;
> +
> + ad_id = acpi_match_device(spi->dev.driver->acpi_match_table,
> + &spi->dev);
> + if (!ad_id)
> + return -ENODEV;
> + model = &adcxx1s_models[ad_id->driver_data];
> + } else {
> + model = &adcxx1s_models[spi_get_device_id(spi)->driver_data];
> + }
> +
> + iio = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> + if (!iio)
> + return -ENOMEM;
> +
> + adc = iio_priv(iio);
> + adc->spi = spi;
> + adc->bits = model->bits;
> + mutex_init(&adc->lock);
> +
> + adc->reg = devm_regulator_get(&spi->dev, "vref");
> + if (IS_ERR(adc->reg))
> + return PTR_ERR(adc->reg);
> +
> + err = regulator_enable(adc->reg);
> + if (err < 0)
> + return err;
> +
> + iio->dev.parent = &spi->dev;
> + iio->dev.of_node = spi->dev.of_node;
> + iio->name = dev_name(&spi->dev);
> + iio->modes = INDIO_DIRECT_MODE;
> + iio->info = &adc081s_info;
> +
> + iio->channels = model->channels;
> + iio->num_channels = ADC081S_NUM_CHANNELS;
> +
> + err = iio_device_register(iio);
> + if (err < 0)
> + goto err_regulator_disable;
> +
> + spi_set_drvdata(spi, iio);
> +
> + return 0;
> +
> +err_regulator_disable:
> + regulator_disable(adc->reg);
> +
> + return err;
> +}
> +
> +static int adc081s_remove(struct spi_device *spi)
> +{
> + struct iio_dev *iio = spi_get_drvdata(spi);
> + struct adc081s *adc = iio_priv(iio);
> +
> + iio_device_unregister(iio);
> + regulator_disable(adc->reg);
> +
> + return 0;
> +}
> +
> +static const struct spi_device_id adc081s_id[] = {
> + { "adc081s", ADC081S },
> + { "adc101s", ADC101S },
> + { "adc121s", ADC121S },
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, adc081s_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id adc081s_of_match[] = {
> + { .compatible = "ti,adc081s" },
> + { .compatible = "ti,adc101s" },
> + { .compatible = "ti,adc121s" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, adc081s_of_match);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id adc081s_acpi_match[] = {
> + { "ADC081S", ADC081S },
> + { "ADC101S", ADC101S },
> + { "ADC121S", ADC121S },
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, adc081s_acpi_match);
> +#endif
> +
> +static struct spi_driver adc081s_driver = {
> + .driver = {
> + .name = "adc081s",
> + .of_match_table = of_match_ptr(adc081s_of_match),
> + .acpi_match_table = ACPI_PTR(adc081s_acpi_match),
> + },
> + .probe = adc081s_probe,
> + .remove = adc081s_remove,
> + .id_table = adc081s_id,
> +};
> +module_spi_driver(adc081s_driver);
> +
> +MODULE_AUTHOR("Milan Stevanovic <milan.o.stevanovic@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Texas Instruments ADC081S/ADC101S/ADC121S driver");
> +MODULE_LICENSE("GPL v2");

Next message: Eric Biggers: "[PATCH] crypto: algapi - convert cra_refcnt to refcount_t"
Previous message: Pavel Machek: "v4.15-rc5: resume took way too long, warning in syslog"
In reply to: Milan Stevanovic: "Re: [PATCH] iio: adc: add driver for ti adc081s/adc101s/adc121s"
Next in thread: Milan Stevanovic: "Re: [PATCH] iio: adc: add driver for ti adc081s/adc101s/adc121s"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]