lkml.org 
[lkml]   [2019]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 4/4] staging: iio: ad7780: moving ad7780 out of staging
On 02/09, Jonathan Cameron wrote:
>On Tue, 5 Feb 2019 15:14:03 -0200
>Renato Lui Geh <renatogeh@gmail.com> wrote:
>
>> Move ad7780 ADC driver out of staging and into the mainline.
>>
>> The ad7780 is a sigma-delta analog to digital converter. This driver provides
>> reading voltage values and status bits from both the ad778x and ad717x series.
>> Its interface also allows writing on the FILTER and GAIN GPIO pins on the
>> ad778x.
>>
>> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
>> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
>> Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
>
>This needs a device tree binding doc which should be reviewed before we move
>the driver out of staging. Make sure to cc the dt-binding maintainers and
>list. Doesn't really matter if that patch is before or after this one
>in the series but needs to be in the same series.

Ok! I see that some Analog dt-bindings are prefixed by adi and some are
not. Should I follow any naming standard?
>
>There are a few more minor tidy ups that would be nice to have inline
>given you are doing a v4. Stuff like this could have been cleaned up
>after moving out of staging (nothing wrong with improving non staging
>drivers after all) but always better to do it whilst we remember!
>
>> ---
>> Changes in v3:
>> - Changes unrelated to moving the driver to main tree were resent as
>> individual patches
>>
>> drivers/iio/adc/Kconfig | 13 ++
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/ad7780.c | 359 +++++++++++++++++++++++++++++++
>> drivers/staging/iio/adc/Kconfig | 13 --
>> drivers/staging/iio/adc/Makefile | 1 -
>> drivers/staging/iio/adc/ad7780.c | 359 -------------------------------
>> 6 files changed, 373 insertions(+), 373 deletions(-)
>> create mode 100644 drivers/iio/adc/ad7780.c
>> delete mode 100644 drivers/staging/iio/adc/ad7780.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index f3cc7a31bce5..2cdee166d0e9 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -108,6 +108,19 @@ config AD7766
>> To compile this driver as a module, choose M here: the module will be
>> called ad7766.
>>
>> +config AD7780
>> + tristate "Analog Devices AD7780 and similar ADCs driver"
>> + depends on SPI
>> + depends on GPIOLIB || COMPILE_TEST
>> + select AD_SIGMA_DELTA
>> + help
>> + Say yes here to build support for Analog Devices AD7170, AD7171,
>> + AD7780 and AD7781 SPI analog to digital converters (ADC).
>> + If unsure, say N (but it's safe to say "Y").
>
>I wouldn't bother with this statement, doesn't add any real info!
>
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called ad7780.
>> +
>> config AD7791
>> tristate "Analog Devices AD7791 ADC driver"
>> depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index ea5031348052..b48852157115 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -15,6 +15,7 @@ obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
>> obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
>> obj-$(CONFIG_AD7606) += ad7606.o
>> obj-$(CONFIG_AD7766) += ad7766.o
>> +obj-$(CONFIG_AD7780) += ad7780.o
>> obj-$(CONFIG_AD7791) += ad7791.o
>> obj-$(CONFIG_AD7793) += ad7793.o
>> obj-$(CONFIG_AD7887) += ad7887.o
>> diff --git a/drivers/iio/adc/ad7780.c b/drivers/iio/adc/ad7780.c
>> new file mode 100644
>> index 000000000000..163e3c983598
>> --- /dev/null
>> +++ b/drivers/iio/adc/ad7780.c
>> @@ -0,0 +1,359 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
>> + *
>> + * Copyright 2011 Analog Devices Inc.
>
>I think you have done more than enough to this driver to add
>an additional copyright line if you want to!

Oh wow, that'd be awesome! Thanks! Should I just add my full name there?
>
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/err.h>
>> +#include <linux/sched.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/adc/ad_sigma_delta.h>
>> +
>> +#define AD7780_RDY BIT(7)
>> +#define AD7780_FILTER BIT(6)
>> +#define AD7780_ERR BIT(5)
>> +#define AD7780_ID1 BIT(4)
>> +#define AD7780_ID0 BIT(3)
>> +#define AD7780_GAIN BIT(2)
>> +#define AD7780_PAT1 BIT(1)
>> +#define AD7780_PAT0 BIT(0)
>These two bits of pattern don't really add anything. I'd drop them in
>favour of something like
>
>#define AD7780_PATTERN_GOOD 1
>#define AD7780_PATTERN_MASK GENMASK(1, 0)
>
>Same for ID for that matter. These aren't one bit fields, so we shouldn't
>ever present them as such (though the datasheet confusingly sort of does
>so!)
>
>> +
>> +#define AD7780_PATTERN (AD7780_PAT0)
>> +#define AD7780_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1)
>> +
>> +#define AD7170_PAT2 BIT(2)
>> +
>> +#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
>> +#define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
>I'd use a value for the pattern directly and
>GENMASK for the mask.
>
>> +
>> +#define AD7780_GAIN_GPIO 0
>> +#define AD7780_FILTER_GPIO 1
>> +
>> +#define AD7780_GAIN_MIDPOINT 64
>> +#define AD7780_FILTER_MIDPOINT 13350
>> +
>> +static const unsigned int ad778x_gain[2] = { 1, 128 };
>> +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
>> +
>> +struct ad7780_chip_info {
>> + struct iio_chan_spec channel;
>> + unsigned int pattern_mask;
>> + unsigned int pattern;
>> + bool is_ad778x;
>> +};
>> +
>> +struct ad7780_state {
>> + const struct ad7780_chip_info *chip_info;
>> + struct regulator *reg;
>> + struct gpio_desc *powerdown_gpio;
>> + struct gpio_desc *gain_gpio;
>> + struct gpio_desc *filter_gpio;
>> + unsigned int gain;
>> + unsigned int odr;
>> + unsigned int int_vref_mv;
>> +
>> + struct ad_sigma_delta sd;
>> +};
>> +
>> +enum ad7780_supported_device_ids {
>> + ID_AD7170,
>> + ID_AD7171,
>> + ID_AD7780,
>> + ID_AD7781,
>> +};
>> +
>> +static struct ad7780_state *ad_sigma_delta_to_ad7780(struct ad_sigma_delta *sd)
>> +{
>> + return container_of(sd, struct ad7780_state, sd);
>> +}
>> +
>> +static int ad7780_set_mode(struct ad_sigma_delta *sigma_delta,
>> + enum ad_sigma_delta_mode mode)
>> +{
>> + struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
>> + unsigned int val;
>> +
>> + switch (mode) {
>> + case AD_SD_MODE_SINGLE:
>> + case AD_SD_MODE_CONTINUOUS:
>> + val = 1;
>> + break;
>> + default:
>> + val = 0;
>> + break;
>> + }
>> +
>> + gpiod_set_value(st->powerdown_gpio, val);
>> +
>> + return 0;
>> +}
>> +
>> +static int ad7780_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val,
>> + int *val2,
>> + long m)
>> +{
>> + struct ad7780_state *st = iio_priv(indio_dev);
>> + int voltage_uv;
>> +
>> + switch (m) {
>> + case IIO_CHAN_INFO_RAW:
>> + return ad_sigma_delta_single_conversion(indio_dev, chan, val);
>> + case IIO_CHAN_INFO_SCALE:
>> + voltage_uv = regulator_get_voltage(st->reg);
>> + if (voltage_uv < 0)
>> + return voltage_uv;
>> + voltage_uv /= 1000;
>> + *val = voltage_uv * st->gain;
>> + *val2 = chan->scan_type.realbits - 1;
>> + st->int_vref_mv = voltage_uv;
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> + case IIO_CHAN_INFO_OFFSET:
>> + *val = -(1 << (chan->scan_type.realbits - 1));
>> + return IIO_VAL_INT;
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + *val = st->odr;
>> + return IIO_VAL_INT;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int ad7780_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int val,
>> + int val2,
>> + long m)
>> +{
>> + struct ad7780_state *st = iio_priv(indio_dev);
>> + const struct ad7780_chip_info *chip_info = st->chip_info;
>> + unsigned long long vref;
>> + unsigned int full_scale, gain;
>> +
>> + if (!chip_info->is_ad778x)
>> + return 0;
>> +
>> + switch (m) {
>> + case IIO_CHAN_INFO_SCALE:
>> + if (val != 0)
>> + return -EINVAL;
>> +
>> + vref = st->int_vref_mv * 1000000LL;
>> + full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
>> + gain = DIV_ROUND_CLOSEST(vref, full_scale);
>> + gain = DIV_ROUND_CLOSEST(gain, val2);
>> + st->gain = gain;
>> + if (gain < AD7780_GAIN_MIDPOINT)
>> + gain = 0;
>> + else
>> + gain = 1;
>> + gpiod_set_value(st->gain_gpio, gain);
>> + break;
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
>> + val = 0;
>> + else
>> + val = 1;
>> + st->odr = ad778x_odr_avail[val];
>> + gpiod_set_value(st->filter_gpio, val);
>> + break;
>We'll get a warning here due to the lack of a default handler.
>It's pointless except to suppress the warning, but best to add one.
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
>> + unsigned int raw_sample)
>> +{
>> + struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
>> + const struct ad7780_chip_info *chip_info = st->chip_info;
>> +
>> + if ((raw_sample & AD7780_ERR) ||
>> + ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
>> + return -EIO;
>> +
>> + if (chip_info->is_ad778x) {
>> + st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
>> + st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
>> + .set_mode = ad7780_set_mode,
>> + .postprocess_sample = ad7780_postprocess_sample,
>> + .has_registers = false,
>> +};
>> +
>> +#define AD7780_CHANNEL(bits, wordsize) \
>> + AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
>> +
>> +static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
>> + [ID_AD7170] = {
>> + .channel = AD7780_CHANNEL(12, 24),
>> + .pattern = AD7170_PATTERN,
>> + .pattern_mask = AD7170_PATTERN_MASK,
>> + .is_ad778x = false,
>> + },
>> + [ID_AD7171] = {
>> + .channel = AD7780_CHANNEL(16, 24),
>> + .pattern = AD7170_PATTERN,
>> + .pattern_mask = AD7170_PATTERN_MASK,
>> + .is_ad778x = false,
>> + },
>> + [ID_AD7780] = {
>> + .channel = AD7780_CHANNEL(24, 32),
>> + .pattern = AD7780_PATTERN,
>> + .pattern_mask = AD7780_PATTERN_MASK,
>> + .is_ad778x = true,
>> + },
>> + [ID_AD7781] = {
>> + .channel = AD7780_CHANNEL(20, 32),
>> + .pattern = AD7780_PATTERN,
>> + .pattern_mask = AD7780_PATTERN_MASK,
>> + .is_ad778x = true,
>> + },
>> +};
>> +
>> +static const struct iio_info ad7780_info = {
>> + .read_raw = ad7780_read_raw,
>> + .write_raw = ad7780_write_raw,
>> +};
>> +
>> +static int ad7780_probe(struct spi_device *spi)
>> +{
>> + struct ad7780_state *st;
>> + struct iio_dev *indio_dev;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + st = iio_priv(indio_dev);
>> + st->gain = 1;
>> +
>> + ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info);
>> +
>> + st->chip_info =
>> + &ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
>> +
>> + spi_set_drvdata(spi, indio_dev);
>> +
>> + indio_dev->dev.parent = &spi->dev;
>> + indio_dev->name = spi_get_device_id(spi)->name;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> + indio_dev->channels = &st->chip_info->channel;
>> + indio_dev->num_channels = 1;
>> + indio_dev->info = &ad7780_info;
>> +
>> + st->powerdown_gpio = devm_gpiod_get_optional(&spi->dev,
>> + "powerdown",
>> + GPIOD_OUT_LOW);
>> + if (IS_ERR(st->powerdown_gpio)) {
>> + ret = PTR_ERR(st->powerdown_gpio);
>> + dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + if (st->chip_info->is_ad778x) {
>> + st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
>> + "gain",
>> + GPIOD_OUT_HIGH);
>> + if (IS_ERR(st->gain_gpio)) {
>> + ret = PTR_ERR(st->gain_gpio);
>> + dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
>> + "filter",
>> + GPIOD_OUT_HIGH);
>> + if (IS_ERR(st->filter_gpio)) {
>> + ret = PTR_ERR(st->filter_gpio);
>> + dev_err(&spi->dev,
>> + "Failed to request filter GPIO: %d\n",
>> + ret);
>> + return ret;
>> + }
>> + }
>> +
>> + st->reg = devm_regulator_get(&spi->dev, "avdd");
>> + if (IS_ERR(st->reg))
>> + return PTR_ERR(st->reg);
>> +
>> + ret = regulator_enable(st->reg);
>> + if (ret) {
>> + dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
>> + return ret;
>> + }
>> +
>> + ret = ad_sd_setup_buffer_and_trigger(indio_dev);
>> + if (ret)
>> + goto error_disable_reg;
>> +
>> + ret = iio_device_register(indio_dev);
>> + if (ret)
>> + goto error_cleanup_buffer_and_trigger;
>> +
>> + return 0;
>> +
>> +error_cleanup_buffer_and_trigger:
>> + ad_sd_cleanup_buffer_and_trigger(indio_dev);
>> +error_disable_reg:
>> + regulator_disable(st->reg);
>> +
>> + return ret;
>> +}
>> +
>> +static int ad7780_remove(struct spi_device *spi)
>> +{
>> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> + struct ad7780_state *st = iio_priv(indio_dev);
>> +
>> + iio_device_unregister(indio_dev);
>> + ad_sd_cleanup_buffer_and_trigger(indio_dev);
>> +
>> + regulator_disable(st->reg);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct spi_device_id ad7780_id[] = {
>> + {"ad7170", ID_AD7170},
>> + {"ad7171", ID_AD7171},
>> + {"ad7780", ID_AD7780},
>> + {"ad7781", ID_AD7781},
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(spi, ad7780_id);
>> +
>> +static struct spi_driver ad7780_driver = {
>> + .driver = {
>> + .name = "ad7780",
>> + },
>> + .probe = ad7780_probe,
>> + .remove = ad7780_remove,
>> + .id_table = ad7780_id,
>> +};
>> +module_spi_driver(ad7780_driver);
>> +
>> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
>> +MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs");
>> +MODULE_LICENSE("GPL v2");

\
 
 \ /
  Last update: 2019-02-14 21:49    [W:0.139 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site