Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED
On Mi, 2019-02-13 at 22:25 -0200, Rodrigo Ribeiro wrote: > [External] > > > Em ter, 29 de jan de 2019 às 07:10, Alexandru Ardelean l.com> escreveu: > > > > On Sat, Jan 26, 2019 at 8:13 PM Jonathan Cameron > wrote: > > > > > > On Fri, 25 Jan 2019 22:14:32 -0200 > > > Rodrigo Ribeiro wrote: > > > > > > > Em sex, 25 de jan de 2019 às 21:46, Rodrigo Ribeiro > > > > escreveu: > > > > > > > > > > Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean > > > > > escreveu: > > > > > > > > > > > > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro ail.com> wrote: > > > > > > > > > > > > > > Remove the checkpatch.pl check: > > > > > > > > > > > > > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'? > > > > > > > > > > > > Hey, > > > > > > > > > > Hi, > > > > > > > > > > Thanks for answering. > > > > > > > > > > > A bit curios about this one. > > > > > > Are you using this chip/driver ? > > > > > > > > > > No, I am just a student at USP (University of São Paulo) starting > in Linux > > > > > Kernel and a new member of the USP Linux Kernel group that has > contributed > > > > > for a few months. > > > > > > > > > > > Thing is: the part is nearing EOL, and it could be an idea to > be > > > > > > marked for removal (since it's still in staging). > > > > > > But if there are users for this driver, it could be left around > for a while. > > > > > > > > > > This is my first patch in Linux Kernel, but if the driver will be > removed, I > > > > > can send a patch for another driver. Is there any driver that I > can > > > > > fix a style warning? > > > > > > > > Maybe, one checkstyle patch is enough, right? Which drivers can I > truly > > > > contribute to? > > > > > > How about the ad7150? That one is still listed as production. > > > What do you think Alex, you probably have better visibility on the > > > status of these parts and their drivers than I do! > > > > > > (note I haven't even opened that one for a few years so no idea > > > what state the driver is in!) > > > > > > > ad7150 is a good alternative. > > At a first glance over the driver it looks like it could do with some > > polish/conversions to newer IIO constructs (like IIO triggers, maybe > > some newer event handling mechanisms?). > > I'll sync with Stefan [Popa] to see about this stuff at a later point > in time. > > > > I'd also add here the adxl345 driver. > > This is mostly informational for anyone who'd find this interesting. > > There are 2 drivers for this chip, one in IIO > > [drivers/iio/accel/adxl345] and another one in > > "drivers/misc/adxl34x.c" as part of the input sub-system. > > What would be interesting here is to finalize the IIO driver [ I think > > some features are lacking behind the input driver], and make the input > > driver a consumer of the IIO driver. > > > > From my side, both these variants are fine to take on. > > The ad7150 is a good idea as a starter project, and the adxl one is > > more of a long-term medium-level project. > > > > Thanks > > Alex > > > > Hi Alex, thanks for suggestions. > > I read the IIO trigger documentation > (https://www.kernel.org/doc/html/v4.12/driver-api/iio/triggers.html) and > ask one question: What is the difference between events and triggers? > They are sounds me same things. > > I am trying to understand how to implement a IIO trigger by reading > the IIO Simple Dummy code but this driver does not implements IIO > triggers > but only IIO events. Is there a didactic example like IIO Simple Dummy > that implements IIO triggers? > > Thanks > Rodrigo > Hi Rodrigo, From what I know, IIO Events are not used for passing readings from devices to userspace, but rather out of bounds information such as crossing of voltage thresholds, proximity detection, motion detection, etc. Triggers are typically used to determine when valid data can be read from a device which is then stored in the buffer. After a quick look over the AD7150, I think that using IIO events, might be the correct approach, since it is a proximity sensing device. -Stefan > > > Jonathan > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean > > > > > escreveu: > > > > > > > > > > > > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro ail.com> wrote: > > > > > > > > > > > > > > Remove the checkpatch.pl check: > > > > > > > > > > > > > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'? > > > > > > > > > > > > Hey, > > > > > > > > > > > > A bit curios about this one. > > > > > > Are you using this chip/driver ? > > > > > > > > > > > > Thing is: the part is nearing EOL, and it could be an idea to > be > > > > > > marked for removal (since it's still in staging). > > > > > > But if there are users for this driver, it could be left around > for a while. > > > > > > > > > > > > Thanks > > > > > > Alex > > > > > > > > > > > > > > > > > > > > Signed-off-by: Rodrigo Ribeiro > > > > > > > Signed-off-by: Rafael Tsuha > > > > > > > --- > > > > > > > This
Re: [PATCH v2 1/2] staging: iio: ad7780: add gain & filter gpio support
On Vi, 2019-02-01 at 12:55 -0200, Renato Lui Geh wrote:
> On 01/30, Popa, Stefan Serban wrote:
> >
> > On Du, 2019-01-27 at 18:30 -0200, Renato Lui Geh wrote:
> > >
> > > Previously, the AD7780 driver only supported gpio for the 'powerdown'
> > > pin. This commit adds suppport for the 'gain' and 'filter' pin.
> > >
> > > Signed-off-by: Renato Lui Geh
> > > Signed-off-by: Giuliano Belinassi
> > > Co-developed-by: Giuliano Belinassi
> > > ---
> > > Changes in v2:
> > > - Filter reading changed to mHz
> > > - Storing filter, gain and voltage to chip_infoHi,
> Hi Stefan,
>
> Thanks for the review. Comments inline.
>
> Renato
> >
> >
> > Comments inline.
> >
> > Regards,
> > Stefan
> >
> > >
> > >
> > > drivers/staging/iio/adc/ad7780.c | 103 ++-
> > > --
> > > include/linux/iio/adc/ad_sigma_delta.h | 5 ++
> > > 2 files changed, 99 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > b/drivers/staging/iio/adc/ad7780.c
> > > index c4a85789c2db..82394e31b168 100644
> > > --- a/drivers/staging/iio/adc/ad7780.c
> > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > @@ -39,6 +39,15 @@
> > > #define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> > > #define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 |
> > > AD7170_PAT2)
> > >
> > > +#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_filter[2] = { 1, 16700 };
> > I would name this array ad778x_odr_avail or something like that.
> Sure
> >
> > We should also consider adding the option to read the available
> > sampling frequencies from user space, but let's leave this for another
> > commit.
> Do you mean returning 10 and 16.7 Hz?
>
> Should this be done before sending the driver to the main tree, or is it
> ok to do something like that after it has left staging?
I think, we should leave it for a future commit.
> >
> >
> > >
> > > +
> > > struct ad7780_chip_info {
> > > struct iio_chan_specchannel;
> > > unsigned intpattern_mask;
> > > @@ -50,7 +59,11 @@ struct ad7780_state {
> > > const struct ad7780_chip_info *chip_info;
> > > struct regulator*reg;
> > > struct gpio_desc*powerdown_gpio;
> > > - unsigned intgain;
> > > + struct gpio_desc*gain_gpio;
> > > + struct gpio_desc*filter_gpio;
> > > + unsigned intgain;
> > > + unsigned intfilter;
> > Also, this could be renamed as odr.
> >
> > >
> > > + unsigned intint_vref_mv;
> > >
> > > struct ad_sigma_delta sd;
> > > };
> > > @@ -104,17 +117,65 @@ static int ad7780_read_raw(struct iio_dev
> > > *indio_dev,
> > > voltage_uv = regulator_get_voltage(st->reg);
> > > if (voltage_uv < 0)
> > > return voltage_uv;
> > > - *val = (voltage_uv / 1000) * st->gain;
> > > + 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->filter;
> > > + 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,
> > > +
Re: [PATCH v2 1/2] staging: iio: ad7780: add gain & filter gpio support
On Du, 2019-01-27 at 18:30 -0200, Renato Lui Geh wrote:
> Previously, the AD7780 driver only supported gpio for the 'powerdown'
> pin. This commit adds suppport for the 'gain' and 'filter' pin.
>
> Signed-off-by: Renato Lui Geh
> Signed-off-by: Giuliano Belinassi
> Co-developed-by: Giuliano Belinassi
> ---
> Changes in v2:
> - Filter reading changed to mHz
> - Storing filter, gain and voltage to chip_info
Hi,
Comments inline.
Regards,
Stefan
>
> drivers/staging/iio/adc/ad7780.c | 103 ++---
> include/linux/iio/adc/ad_sigma_delta.h | 5 ++
> 2 files changed, 99 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index c4a85789c2db..82394e31b168 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -39,6 +39,15 @@
> #define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> #define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
>
> +#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_filter[2] = { 1, 16700 };
I would name this array ad778x_odr_avail or something like that. We should
also consider adding the option to read the available sampling frequencies
from user space, but let's leave this for another commit.
> +
> struct ad7780_chip_info {
> struct iio_chan_specchannel;
> unsigned intpattern_mask;
> @@ -50,7 +59,11 @@ struct ad7780_state {
> const struct ad7780_chip_info *chip_info;
> struct regulator*reg;
> struct gpio_desc*powerdown_gpio;
> - unsigned intgain;
> + struct gpio_desc*gain_gpio;
> + struct gpio_desc*filter_gpio;
> + unsigned intgain;
> + unsigned intfilter;
Also, this could be renamed as odr.
> + unsigned intint_vref_mv;
>
> struct ad_sigma_delta sd;
> };
> @@ -104,17 +117,65 @@ static int ad7780_read_raw(struct iio_dev
> *indio_dev,
> voltage_uv = regulator_get_voltage(st->reg);
> if (voltage_uv < 0)
> return voltage_uv;
> - *val = (voltage_uv / 1000) * st->gain;
> + 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->filter;
> + 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;
> + int vref, gain;
> + unsigned int full_scale;
> +
> + if (!chip_info->is_ad778x)
> + return 0;
> +
> + switch (m) {
> + case IIO_CHAN_INFO_SCALE:
> + if (val != 0)
> + return -EINVAL;
> +
> + vref = st->int_vref_mv * 100LL;
From the datasheet, the VREF is ±5 V, therefore your vref variable will
overflow. You probably need unsigned long long.
> + full_scale = 1 << (chip_info->channel.scane_type.realbis
> - 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->filter = ad778x_filter[val];
> + gpiod_set_value(st->filter_gpio, val);
> + break;
> + }
> +
> + return 0;
> +}
> +
> static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> unsigned int raw_sample)
> {
> @@ -126,10 +187,8 @@ static int ad7780_postprocess_sample(struct
> ad_sigma_delta *sigma_delta,
> return -EIO;
>
> if
Re: [PATCH 05/11] staging: iio: adc: ad7606: Add support for threaded irq
On Du, 2018-12-16 at 13:49 +, Jonathan Cameron wrote:
> On Thu, 13 Dec 2018 14:46:17 +0200
> Stefan Popa wrote:
>
> >
> > This patch replaces the use of a polling ring buffer with a threaded
> > interrupt.
> >
> > Enabling the buffer sets the CONVST signal to high. When the rising
> > edge
> > of the CONVST is applied, BUSY signal goes logic high and transitions
> > low
> > at the end of the entire conversion process. The falling edge of the
> > BUSY
> > signal triggers the interrupt.
> >
> > ad7606_trigger_handler() is used as bottom half of the poll function.
> > It reads data from the device and stores it in the internal buffer.
> >
> > Signed-off-by: Stefan Popa
> I'd like a little more info as comments in this one. See below.
> Which is another way of saying I have no idea what is going on :)
>
> Thanks,
>
> Jonathan.
>
Hi Jonathan,
Thank you for the review. It turns out that there is no reason to trigger a
conversion before disabling the buffer. I will remove it in v2.
Thank you!
-Stefan
> >
> > ---
> > drivers/staging/iio/adc/ad7606.c | 116 +
> > --
> > drivers/staging/iio/adc/ad7606.h | 6 +-
> > 2 files changed, 89 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7606.c
> > b/drivers/staging/iio/adc/ad7606.c
> > index 7191d51..13aeeec 100644
> > --- a/drivers/staging/iio/adc/ad7606.c
> > +++ b/drivers/staging/iio/adc/ad7606.c
> > @@ -21,6 +21,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "ad7606.h"
> > @@ -81,36 +82,24 @@ static int ad7606_read_samples(struct ad7606_state
> > *st)
> > static irqreturn_t ad7606_trigger_handler(int irq, void *p)
> > {
> > struct iio_poll_func *pf = p;
> > - struct ad7606_state *st = iio_priv(pf->indio_dev);
> > -
> > - gpiod_set_value(st->gpio_convst, 1);
> > -
> > - return IRQ_HANDLED;
> > -}
> > -
> > -/**
> > - * ad7606_poll_bh_to_ring() bh of trigger launched polling to ring
> > buffer
> > - * @work_s:the work struct through which this was scheduled
> > - *
> > - * Currently there is no option in this driver to disable the saving
> > of
> > - * timestamps within the ring.
> > - * I think the one copy of this at a time was to avoid problems if the
> > - * trigger was set far too high and the reads then locked up the
> > computer.
> > - **/
> > -static void ad7606_poll_bh_to_ring(struct work_struct *work_s)
> > -{
> > - struct ad7606_state *st = container_of(work_s, struct
> > ad7606_state,
> > - poll_work);
> > - struct iio_dev *indio_dev = iio_priv_to_dev(st);
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct ad7606_state *st = iio_priv(indio_dev);
> > int ret;
> >
> > + mutex_lock(>lock);
> > +
> > ret = ad7606_read_samples(st);
> > if (ret == 0)
> > iio_push_to_buffers_with_timestamp(indio_dev, st-
> > >data,
> > iio_get_time_ns(ind
> > io_dev));
> >
> > - gpiod_set_value(st->gpio_convst, 0);
> > iio_trigger_notify_done(indio_dev->trig);
> > + /* The rising edge of the CONVST signal starts a new
> > conversion. */
> > + gpiod_set_value(st->gpio_convst, 1);
> > +
> > + mutex_unlock(>lock);
> > +
> > + return IRQ_HANDLED;
> > }
> >
> > static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int
> > ch)
> > @@ -378,8 +367,11 @@ static int ad7606_request_gpios(struct
> > ad7606_state *st)
> > return PTR_ERR_OR_ZERO(st->gpio_os);
> > }
> >
> > -/**
> > - * Interrupt handler
> > +/*
> > + * The BUSY signal indicates when conversions are in progress, so when
> > a rising
> > + * edge of CONVST is applied, BUSY goes logic high and transitions low
> > at the
> > + * end of the entire conversion process. The falling edge of the BUSY
> > signal
> > + * triggers this interrupt.
> > */
> > static irqreturn_t ad7606_interrupt(int irq, void *dev_id)
> > {
> > @@ -387,7 +379,8 @@ static irqreturn_t ad7606_interrupt(int irq, void
> > *dev_id)
> > struct ad7606_state *st = iio_priv(indio_dev);
> >
> > if (iio_buffer_enabled(indio_dev)) {
> > - schedule_work(>poll_work);
> > + gpiod_set_value(st->gpio_convst, 0);
> > + iio_trigger_poll_chained(st->trig);
> > } else {
> > complete(>completion);
> > }
> > @@ -395,26 +388,74 @@ static irqreturn_t ad7606_interrupt(int irq, void
> > *dev_id)
> > return IRQ_HANDLED;
> > };
> >
> > +static int ad7606_validate_trigger(struct iio_dev *indio_dev,
> > + struct iio_trigger *trig)
> > +{
> > + struct ad7606_state *st = iio_priv(indio_dev);
> > +
> > + if (st->trig != trig)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int ad7606_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > + struct ad7606_state *st = iio_priv(indio_dev);
> > +
> > +
Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support
On Mi, 2018-11-28 at 16:15 -0200, Giuliano Belinassi wrote:
> Previously, the AD7780 driver only supported gpio for the 'powerdown'
> pin. This commit adds suppport for the 'gain' and 'filter' pin.
>
> Signed-off-by: Giuliano Belinassi
> ---
> Changes in v2:
> - Now this patch is part of the patchset that aims to remove ad7780
> out of staging. https://marc.info/?l=linux-iio=154282349808890=2
> - Also, now it reads voltage and filter values from the userspace
> instead of gpio pin states.
Hello,
Please see bellow.
>
> drivers/staging/iio/adc/ad7780.c | 78 --
> include/linux/iio/adc/ad_sigma_delta.h | 5 ++
> 2 files changed, 79 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index c4a85789c2db..05979a79fda3 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -39,6 +39,12 @@
> #define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> #define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 |
> AD7170_PAT2)
>
> +#define AD7780_GAIN_GPIO 0
> +#define AD7780_FILTER_GPIO 1
> +
> +#define AD7780_GAIN_MIDPOINT 64
> +#define AD7780_FILTER_MIDPOINT 13350
> +
> struct ad7780_chip_info {
> struct iio_chan_specchannel;
> unsigned intpattern_mask;
> @@ -50,6 +56,8 @@ 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 intgain;
>
> struct ad_sigma_delta sd;
> @@ -115,18 +123,65 @@ static int ad7780_read_raw(struct iio_dev
> *indio_dev,
> 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;
> + int uvref, gain;
> + unsigned int full_scale;
> +
> + if (!chip_info->is_ad778x)
> + return 0;
> +
> + switch (m) {
> + case IIO_CHAN_INFO_SCALE:
> + if (val != 0)
> + return -EINVAL;
> +
> + uvref = regulator_get_voltage(st->reg);
regulator_get_voltage() has already been called in the probe function and
the result is stored in st->int_vref_mv.
My suggestion would be to use a local vref variable declared as unsigned
int. It is my fault that I haven't explained correctly in the previous
email, but you need to multiply vref_mv with 100LL in order to get the
right precision: vref = st->int_vref_mv * 100LL. Afterwards you will be
able to perform the divisions.
> +
> + if (uvref < 0)
> + return uvref;
> +
> + full_scale = 1 << (chip_info->channel.scan_type.realbits
> - 1);
> + gain = DIV_ROUND_CLOSEST(uvref, full_scale);
> + gain = DIV_ROUND_CLOSEST(gain, val2);
> +
> + gpiod_set_value(st->gain_gpio, gain <
> AD7780_GAIN_MIDPOINT ? 0 : 1);
Once the gain is set, you can store it in st->gain variable.
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (val2 != 0)
> + return -EINVAL;
> +
> + gpiod_set_value(st->filter_gpio, val <
> AD7780_FILTER_MIDPOINT ? 0 : 1);
This is probably fine, although I am not a big fan of the ternary operator.
A simple if else statement would do. However, I don't feel strongly about
it, so feel free to disagree.
> + break;
> + }
> +
> + 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;
> + int val;
>
> if ((raw_sample & AD7780_ERR) ||
> ((raw_sample & chip_info->pattern_mask) != chip_info-
> >pattern))
> return -EIO;
>
> if (chip_info->is_ad778x) {
> - if (raw_sample & AD7780_GAIN)
> + val = raw_sample & AD7780_GAIN;
> +
> + if (val != gpiod_get_value(st->gain_gpio))
> + return -EIO;
It is not obvious to me what is the point of this check. Maybe you can add
a comment?
> +
> + if (val)
> st->gain = 1;
> else
> st->gain = 128;
Do we still need this? I am not convinced.
> @@ -141,18 +196,20 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
> .has_registers = false,
> };
>
> -#define AD7780_CHANNEL(bits, wordsize) \
> +#define AD7170_CHANNEL(bits,
Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support
On Lu, 2018-11-26 at 17:24 -0200, Giuliano Belinassi wrote: Hi, please see bellow > Hi, thank you for the review > > > > > On Thu, 22 Nov 2018 11:01:00 + > > "Popa, Stefan Serban" wrote: > > > > > > I think that instead of setting the gain directly, we should use > > > the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 datasheet > > > there > > > is a formula from which the output code can be calculated: > > > Code = 2^(N − 1) > > > × [(AIN × Gain /VREF) + 1]. So, by setting the scale from user space, > > > the > > > driver can calculate the correct gain by using the formula above. > > > Also, it > > > would be useful to introduce scale available. > > > Furthermore, there is a new > > > ad7124 adc driver which does this exact thing. Take a look here: http > > > s://gi > > > thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.c# > > > L337. > We have some questions about the code you provided to us: > 1-) What is exactly the inputs for the write_raw function? In your write_raw() function you need to add IIO_CHAN_INFO_SCALE case. Setting the scale from user space looks something like this: root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale . Furthermore, in your write_raw() function, val=0 and val2=298. Knowing that full_scale_voltage = Vref / (gain * scale), we can calculate the gain = Vref / (full_scale_voltage * scale). We only support two gains (1 and 128), so we need to determine which one fits better with the desired scale. Finally, all we have left to do is to set the gain. > 2-) Could you give more details about the math around lines 346-348? > Is it correct to assume that the multiplication at line 346 won't > overflow? (vref is an uint) It is correct that Vref is in microvolts, so for example, Vref of 2.5V = 25uV. It won't overflow since we use the Vref as nominator, while full_scale_voltage and scale are the denominators. > > And regarding our code: > 1-) The val in our write_raw function should be, in case of GAIN, a > number that best approximate the actual gain value of 1 or 128? For > instance, if the user inputs 126, we should default to 128? We should not allow the the user to input the gain, he needs to input the scale (see the mail from Jonathan and the above explanation). However, if the calculated gain is one that is not supported, such as 126, we will set the closest matching value, in this case 128. > 2-) In the case of FILTER, is it the same? Is the user sending the > value in mHz (milihertz)? Yes, it is the same with the FILTER. You need to add a IIO_CHAN_INFO_SAMP_FREQ case in your write_raw() function. From user space, the input value should be in Hz, something like this: root:/sys/bus/iio/devices/iio:device0> echo 10 > in_voltage_sampling_frequency > > Thank you > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support
On Mi, 2018-11-21 at 16:04 -0200, Giuliano Belinassi wrote:
> Previously, the AD7780 driver only supported gpio for the 'powerdown'
> pin. This commit adds suppport for the 'gain' and 'filter' pin.
Hey,
Comments inline.
>
> Signed-off-by: Giuliano Belinassi
> ---
> drivers/staging/iio/adc/ad7780.c | 61 --
> include/linux/iio/adc/ad_sigma_delta.h | 5 +++
> 2 files changed, 62 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index c4a85789c2db..69794f06dbcd 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -39,6 +39,9 @@
> #define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> #define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 |
> AD7170_PAT2)
>
> +#define AD7780_GAIN_GPIO 0
> +#define AD7780_FILTER_GPIO 1
> +
> struct ad7780_chip_info {
> struct iio_chan_specchannel;
> unsigned intpattern_mask;
> @@ -50,6 +53,8 @@ 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 intgain;
>
> struct ad_sigma_delta sd;
> @@ -115,18 +120,51 @@ static int ad7780_read_raw(struct iio_dev
> *indio_dev,
> 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);
> +
> + if (m != IIO_CHAN_INFO_RAW)
> + return -EINVAL;
> +
> + if (st->chip_info->is_ad778x) {
> + switch(val) {
> + case AD7780_GAIN_GPIO:
I think that instead of setting the gain directly, we should use
the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 datasheet there
is a formula from which the output code can be calculated:
Code = 2^(N − 1)
× [(AIN × Gain /VREF) + 1]. So, by setting the scale from user space, the
driver can calculate the correct gain by using the formula above. Also, it
would be useful to introduce scale available.
Furthermore, there is a new
ad7124 adc driver which does this exact thing. Take a look here: https://gi
thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.c#L337.
> + gpiod_set_value(st->gain_gpio, val2);
> + break;
> + case AD7780_FILTER_GPIO:
The attribute that should be used to configure the filter gpio is
IIO_CHAN_INFO_SAMP_FREQ. So, we should have 10 Hz and 16.7 Hz available
sampling frequencies. If from user space the 10 Hz sampling freq is
requested, then we set the FILTER pin high, while for 16.7 Hz the FILTER
pin will be low.
> + gpiod_set_value(st->filter_gpio, val2);
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> +
> + 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;
> + int val;
>
> if ((raw_sample & AD7780_ERR) ||
> ((raw_sample & chip_info->pattern_mask) != chip_info-
> >pattern))
> return -EIO;
>
> if (chip_info->is_ad778x) {
> - if (raw_sample & AD7780_GAIN)
> + val = raw_sample & AD7780_GAIN;
> +
> + if (val != gpiod_get_value(st->gain_gpio))
> + return -EIO;
> +
> + if (val)
> st->gain = 1;
> else
> st->gain = 128;
> @@ -141,18 +179,20 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
> .has_registers = false,
> };
>
> -#define AD7780_CHANNEL(bits, wordsize) \
> +#define AD7170_CHANNEL(bits, wordsize) \
> AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
> +#define AD7780_CHANNEL(bits, wordsize) \
> + AD_SD_CHANNEL_GAIN_FILTER(1, 0, 0, bits, 32, wordsize - bits)
>
> static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> [ID_AD7170] = {
> - .channel = AD7780_CHANNEL(12, 24),
> + .channel = AD7170_CHANNEL(12, 24),
> .pattern = AD7170_PATTERN,
> .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7171] = {
> - .channel = AD7780_CHANNEL(16, 24),
> + .channel = AD7170_CHANNEL(16, 24),
> .pattern = AD7170_PATTERN,
> .pattern_mask = AD7170_PATTERN_MASK,
>
