Re: [PATCH v2 1/2] staging: iio: ad7780: add gain & filter gpio support

[Popa, Stefan Serban]

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 <renatogeh@xxxxxxxxx>
> > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@xxxxxx>
> > > Co-developed-by: Giuliano Belinassi <giuliano.belinassi@xxxxxx>
> > > ---
> > > 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]ÂÂ= { 10000, 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_specÂÂÂÂchannel;
> > > ÂÂÂÂÂÂÂÂunsigned intÂÂÂÂÂÂÂÂÂÂÂÂpattern_mask;
> > > @@ -50,7 +59,11 @@ struct ad7780_state {
> > > ÂÂÂÂÂÂÂÂconst struct ad7780_chip_infoÂÂÂ*chip_info;
> > > ÂÂÂÂÂÂÂÂstruct regulatorÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*reg;
> > > ÂÂÂÂÂÂÂÂstruct gpio_descÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*powerdown_gpio;
> > > -ÂÂÂÂÂÂÂunsigned intÂÂÂÂgain;
> > > +ÂÂÂÂÂÂÂstruct gpio_descÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*gain_gpio;
> > > +ÂÂÂÂÂÂÂstruct gpio_descÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*filter_gpio;
> > > +ÂÂÂÂÂÂÂunsigned intÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgain;
> > > +ÂÂÂÂÂÂÂunsigned intÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfilter;
> > Also, this could be renamed as odr.
> > 
> > > 
> > > +ÂÂÂÂÂÂÂunsigned intÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint_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 * 1000000LL;
> > 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
> > > 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)
> > > 
> > > Â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,
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.is_ad778x = false,
> > > @@ -173,6 +234,7 @@ static const struct ad7780_chip_info
> > > ad7780_chip_info_tbl[] = {
> > > 
> > > Â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)
> > > @@ -222,6 +284,29 @@ static int ad7780_probe(struct spi_device *spi)
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto error_disable_reg;
> > > ÂÂÂÂÂÂÂÂ}
> > > 
> > > +ÂÂÂÂÂÂÂif (st->chip_info->is_ad778x) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂst->gain_gpio = devm_gpiod_get_optional(&spi->dev,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"gain",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂGPIOD_OUT_HIG
> > > H);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (IS_ERR(st->gain_gpio)) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret = PTR_ERR(st->gain_gpio);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev_err(&spi->dev, "Failed to request gain
> > > GPIO:
> > > %d\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto error_disable_reg;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂst->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"filter",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂGPIOD_OUT_H
> > > IGH)
> > > ;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (IS_ERR(st->filter_gpio)) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret = PTR_ERR(st->filter_gpio);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev_err(&spi->dev,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"Failed to request filter GPIO:
> > > %d\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto error_disable_reg;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > > +ÂÂÂÂÂÂÂ}
> > > +
> > > ÂÂÂÂÂÂÂÂret = ad_sd_setup_buffer_and_trigger(indio_dev);
> > > ÂÂÂÂÂÂÂÂif (ret)
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto error_disable_reg;
> > > diff --git a/include/linux/iio/adc/ad_sigma_delta.h
> > > b/include/linux/iio/adc/ad_sigma_delta.h
> > > index 7e84351fa2c0..c01da869de83 100644
> > > --- a/include/linux/iio/adc/ad_sigma_delta.h
> > > +++ b/include/linux/iio/adc/ad_sigma_delta.h
> > > @@ -176,6 +176,11 @@ int ad_sd_validate_trigger(struct iio_dev
> > > *indio_dev, struct iio_trigger *trig);
> > > ÂÂÂÂÂÂÂÂ__AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ_storagebits, _shift, NULL, IIO_VOLTAGE, 0)
> > > 
> > > +#define AD_SD_CHANNEL_GAIN_FILTER(_si, _channel, _address, _bits, \
> > > +ÂÂÂÂÂÂÂ_storagebits, _shift) \
> > > +ÂÂÂÂÂÂÂ__AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ_storagebits, _shift, NULL, IIO_VOLTAGE,
> > > BIT(IIO_CHAN_INFO_RAW))
> > > +
> > Do you need to define a new macro?
> Oh yeah, that macro is useless. I'll remove this block. That was
> definitely an oversight. We thought we had to create a new macro for
> adding support for gain and filter GPIOs (which this macro definitely
> doesn't, as it is identical to the default one) on the driver's channel,
> but reading other drivers it seems you don't actually need to? Is this
> right?

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
> > 
> > 
> > > 
> > > Â#define AD_SD_TEMP_CHANNEL(_si, _address, _bits, _storagebits,
> > > _shift) \
> > > ÂÂÂÂÂÂÂÂ__AD_SD_CHANNEL(_si, 0, -1, _address, _bits, \
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ_storagebits, _shift, NULL, IIO_TEMP, \
> > > --
> > > 2.20.1
> > >