On Sat, 3 Nov 2018 13:04:04 -0300
Matheus Tavares Bernardino <matheus.bernardino@xxxxxx> wrote:
> On Sun, Oct 28, 2018 at 1:50 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Fri, 26 Oct 2018 23:00:04 -0300
> > Matheus Tavares <matheus.bernardino@xxxxxx> 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@xxxxxxxxx>
> > > Signed-off-by: Matheus Tavares <matheus.bernardino@xxxxxx>
> >
> > 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)
> >
