On Sat, 24 Mar 2018 15:57:19 +0100 David Julian Veenstra <davidjulianveenstra@xxxxxxxxx> wrote: > On 24, March 2018 14:12, Jonathan Cameron wrote: > > > On Sat, 24 Mar 2018 13:36:44 +0100 > > David Julian Veenstra <davidjulianveenstra@xxxxxxxxx> wrote: > > > >> On 23, March 2018 14:27, Jonathan Cameron wrote: > >> > >> > On Sun, 18 Mar 2018 14:37:04 +0100 > >> > David Veenstra <davidjulianveenstra@xxxxxxxxx> wrote: > >> > > >> >> The angle channel is not defined in sysfs iio ABI. So it is replaced > >> >> with an inclination channel, because it is defined in the ABI, and has the > >> >> semantics of an angle. > >> >> > >> >> In addition, a fractional scaling factor of 360 / (2^12 -1) is added, > >> >> to scale the 12-bits angular position to degrees, conform the ABI. > >> >> > >> >> Signed-off-by: David Veenstra <davidjulianveenstra@xxxxxxxxx> > >> > No, please do not blugeon a device into the existing documented ABI. > >> > > >> > A resolver does not measure something that can be remotely > >> > sensibly mapped to inclination. Inclination is relative to 'down'. > >> > A resolver doesn't care about down. > >> > > >> > Add an angle type to the ABI docs. It simply hasn't been documented > >> > before because there were no drivers outside staging that use it. > >> > >> I'm sorry, I misunderstood our previous discussing on this topic > >> (https://marc.info/?l=linux-iio&m=152087432322446&w=2) as saying: > >> "there already exists another channel type that is a good enough match". > > Ah, no I was making the point we need to match with the 'units' not > > use the channel type. > > > > Hmm. We have an an unfortunate inconsistency between use of radians/sec > > and degrees for different types. > > > > Radians feels perfectly sensible for a rotary device, but not so much > > for an angle of tilt. > > > > I'm not sure how we resolve this cleanly or if we can. > > My gut feeling is we need to go with radians for angle measures on > > a rotating devices (to match against anglvel) and leave inclination > > in degrees. > > I agree that radians doesn't make sense for inclination, and that it > makes sense to have consistency between the unit of angle and that of > angular velocity. > > However, if ABI consistency is desired, another option would be to > change the unit of angular velocity to degrees per seconds. Then > everything would be the same. But this sounds like a very painful > change... It's effectively impossible without ABI breakage and getting shouted at by users (and potentially Linus forcing a revert). The only way we could do it would be to introduce a new 'similarly' named type and deprecate the old one, removing it some years in the future. Unfortunately we (where I meant I :() mess up sometimes and have to live with the result. Jonathan > > For now, I'll use radians for the angle. > > Best regards, > David Veenstra > > > > > Sorry for the confusion, I'd missed that the units were inconsistent > > which would have made that comment harder to interpret. > > > > Jonathan > > > >> > >> The in_incli and in_rot_from_* family all use degrees as their unit. > >> For V2 I'll change it back to an angle channel and use angle as its > >> unit. > >> > >> Best regards, > >> David Veenstra > >> > >> > > >> > Jonathan > >> > > >> >> --- > >> >> drivers/staging/iio/resolver/ad2s1200.c | 11 ++++++++--- > >> >> 1 file changed, 8 insertions(+), 3 deletions(-) > >> >> > >> >> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c > >> >> index 4b97a106012c..937676bcc0d4 100644 > >> >> --- a/drivers/staging/iio/resolver/ad2s1200.c > >> >> +++ b/drivers/staging/iio/resolver/ad2s1200.c > >> >> @@ -64,6 +64,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev, > >> >> switch (m) { > >> >> case IIO_CHAN_INFO_SCALE: > >> >> switch (chan->type) { > >> >> + case IIO_INCLI: > >> >> + *val = 360; > >> >> + *val2 = 0xFFF; > >> >> + return IIO_VAL_FRACTIONAL; > >> >> case IIO_ANGL_VEL: > >> >> /* > >> >> * 2 * Pi is almost equal to > >> >> @@ -91,7 +95,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev, > >> >> /* delay (6 * AD2S1200_TSCLK + 20) nano seconds */ > >> >> udelay(1); > >> >> gpiod_set_value(st->sample, 1); > >> >> - gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL)); > >> >> + gpiod_set_value(st->rdvel, !!(chan->type == IIO_INCLI)); > >> >> > >> >> ret = spi_read(st->sdev, st->rx, 2); > >> >> if (ret < 0) { > >> >> @@ -101,7 +105,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev, > >> >> > >> >> vel = be16_to_cpup((__be16 *)st->rx); > >> >> switch (chan->type) { > >> >> - case IIO_ANGL: > >> >> + case IIO_INCLI: > >> >> *val = vel >> 4; > >> >> ret = IIO_VAL_INT; > >> >> break; > >> >> @@ -128,10 +132,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev, > >> >> > >> >> static const struct iio_chan_spec ad2s1200_channels[] = { > >> >> { > >> >> - .type = IIO_ANGL, > >> >> + .type = IIO_INCLI, > >> >> .indexed = 1, > >> >> .channel = 0, > >> >> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > >> >> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), > >> >> }, { > >> >> .type = IIO_ANGL_VEL, > >> >> .indexed = 1, > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-iio" in > >> the body of a message to majordomo@xxxxxxxxxxxxxxx > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
