Re: [PATCH 10/11] staging: iio: ad2s1200: Replace angle channel with inclination channel

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 24 Mar 2018 15:57:19 +0100
David Julian Veenstra <davidjulianveenstra@gmail.com> wrote:

> On 24, March 2018 14:12, Jonathan Cameron wrote:
> 
> > On Sat, 24 Mar 2018 13:36:44 +0100
> > David Julian Veenstra <davidjulianveenstra@gmail.com> wrote:
> >  
> >> On 23, March 2018 14:27, Jonathan Cameron wrote:
> >>   
> >> > On Sun, 18 Mar 2018 14:37:04 +0100
> >> > David Veenstra <davidjulianveenstra@gmail.com> 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@gmail.com>    
> >> > 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@vger.kernel.org
> >> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html