Re: [PATCH v2 5/6] staging:iio:ad2s90: Add IIO_CHAN_INFO_SCALE to channel spec and read_raw

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 3 Nov 2018 13:04:04 -0300
Matheus Tavares Bernardino <matheus.bernardino@usp.br> wrote:

> On Sun, Oct 28, 2018 at 1:50 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 26 Oct 2018 23:00:04 -0300
> > Matheus Tavares <matheus.bernardino@usp.br> wrote:
> >  
> > > This patch adds the IIO_CHAN_INFO_SCALE mask to ad2s90_chan and
> > > implements the relative read behavior at ad2s90_read_raw.
> > >
> > > Signed-off-by: Victor Colombo <victorcolombo@gmail.com>
> > > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>  
> >
> > Hi,
> >
> > A suggestion inline.  This is a common case that we have infrastructure
> > to simplify.  + I think your scale factor is very slightly wrong.
> >
> > Jonathan
> >  
> > > ---
> > >  drivers/staging/iio/resolver/ad2s90.c | 32 ++++++++++++++++++---------
> > >  1 file changed, 22 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> > > index b4a6a89c11b0..52b656875ed1 100644
> > > --- a/drivers/staging/iio/resolver/ad2s90.c
> > > +++ b/drivers/staging/iio/resolver/ad2s90.c
> > > @@ -34,19 +34,31 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
> > >       int ret;
> > >       struct ad2s90_state *st = iio_priv(indio_dev);
> > >
> > > -     mutex_lock(&st->lock);
> > > +     switch (m) {
> > > +     case IIO_CHAN_INFO_SCALE:
> > > +             /* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
> > > +             *val = 0;
> > > +             *val2 = 1534355;  
> > Definitely 2^12 - 1?  That's a bit unusual if true as it would imply
> > that 2^12 - 1 and 0 were the same value.
> >
> > Imagine a smaller version with on 2^2 bits so 0, 1, 2, 3
> > Values of each are
> >
> > 0, M_PI/2, M_PI, 3*M_PI/2
> >
> > So the multiplier is 2*M_PI/(2^2) not 2*M_PI/(2^2 - 1)
> > 1/2 vs 2/3 * M_PI  
> 
> Oh, that makes a lot of sense! We used 2^12 - 1 here based on driver
> drivers/iio/resolver/ad2s1200.c, whose resolution is also 12 bits, as
> the ad2s90.c. Do you think this section is, perhaps, wrong on
> ad2s1200.c too, or maybe there is some difference between these two
> drivers that I didn't catch regarding the resolution?
Certainly seems likely to be wrong.  Difference is tiny so no one
probably ever noticed :(

Feel free to post a patch fixing that one as well!

Thanks,

Jonathan

> 
> Matheus.
> 
> > Now this is a very common case so we have the return type
> > IIO_VAL_FRACTIONAL_LOG2 to give a more obvious and potentially
> > more accurate representation.
> >  
> > > +             return IIO_VAL_INT_PLUS_NANO;
> > > +     case IIO_CHAN_INFO_RAW:
> > > +             mutex_lock(&st->lock);
> > > +
> > > +             ret = spi_read(st->sdev, st->rx, 2);
> > > +             if (ret < 0) {
> > > +                     mutex_unlock(&st->lock);
> > > +                     return ret;
> > > +             }
> > > +
> > > +             *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
> > >
> > > -     ret = spi_read(st->sdev, st->rx, 2);
> > > -     if (ret < 0) {
> > >               mutex_unlock(&st->lock);
> > > -             return ret;
> > > -     }
> > >
> > > -     *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
> > > -
> > > -     mutex_unlock(&st->lock);
> > > +             return IIO_VAL_INT;
> > > +     default:
> > > +             break;
> > > +     }
> > >
> > > -     return IIO_VAL_INT;
> > > +     return -EINVAL;
> > >  }
> > >
> > >  static const struct iio_info ad2s90_info = {
> > > @@ -57,7 +69,7 @@ static const struct iio_chan_spec ad2s90_chan = {
> > >       .type = IIO_ANGL,
> > >       .indexed = 1,
> > >       .channel = 0,
> > > -     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > > +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> > >  };
> > >
> > >  static int ad2s90_probe(struct spi_device *spi)  
> >