Re: [PATCH v2 1/2] staging: iio: ad7780: add gain & filter gpio support
On 02/01, Popa, Stefan Serban wrote: Adding support for gpios, normally doesn't have anything to do with the information stored in the iio_chan_spec struct. Also, we need to try to work with the already defined attributes. In your case, setting the gain GPIO is equivalent to modifying the scale, while setting the filter GPIO results in a change in sampling frequency. Therefore, we need use a macro which has the IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_FREQUENCY flags set. I think AD_SD_CHANNEL should do just fine. I hope this answers your question. -Stefan It does. Thank you. :) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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,
> > > + 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.
> You're right, I'll fix this in v3.
> >
> >
> > >
> > > + full_scale = 1 << (chip_info-
> > > >channel.scane_type.realbis
> > > - 1);
> > > + gain = DIV_ROUND_CLOSEST(vref, full_scale);
> > > + gain =
Re: [PATCH v2 1/2] staging: iio: ad7780: add gain & filter gpio support
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_info
Hi,
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?
+
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.
You're right, I'll fix this in v3.
+ 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
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
[PATCH v2 1/2] staging: iio: ad7780: add gain & filter gpio support
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
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 };
+
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;
+ 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;
+ 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->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 (chip_info->is_ad778x) {
- if (raw_sample & AD7780_GAIN)
- st->gain = 1;
- else
- st->gain = 128;
+ st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
+ st->filter = ad778x_filter[raw_sample & AD7780_FILTER];
}
return 0;
@@ -141,18 +200,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)
