Re: [PATCH v2 10/13] staging: iio: ad2s1200: Add scaling factor for angular velocity channel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 21, April 2018 19:07, Jonathan Cameron wrote:

> On Fri, 20 Apr 2018 21:31:09 +0200
> David Veenstra <davidjulianveenstra@xxxxxxxxx> wrote:
>
>> The sysfs iio ABI states radians per second is expected as the unit for
>> angular velocity, but the 12-bit angular velocity register has rps
>> as its unit. So a fractional scaling factor of approximately 2 * Pi is
>> added to the angular velocity channel.
>> 
>> The added comments will also be relevant for the scaling factor of
>> the angle channel.
>> 
>> Signed-off-by: David Veenstra <davidjulianveenstra@xxxxxxxxx>
> Comment inline.  The function you are talking about isn't used
> in the majority of likely use cases for this part.  The maths will actually
> be done in userspace (which can use floating point).
>
> Thanks,
>
> Jonathan
>
>> ---
>> Changes in v2:
>>   - Move explanation of Pi approximation to top of switch statement,
>>     as this will also be relevant to angle channel.
>>   - Replaced 33102 / 2 with 16551 on line 84.
>> 
>>  drivers/staging/iio/resolver/ad2s1200.c | 84 +++++++++++++++++++++++----------
>>  1 file changed, 59 insertions(+), 25 deletions(-)
>> 
>> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
>> index 29a9bb666e7b..6c56257be3b1 100644
>> --- a/drivers/staging/iio/resolver/ad2s1200.c
>> +++ b/drivers/staging/iio/resolver/ad2s1200.c
>> @@ -60,38 +60,71 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>>  	int ret = 0;
>>  	u16 vel;
>>  
>> -	mutex_lock(&st->lock);
>> -	gpiod_set_value(st->sample, 0);
>> +	/*
>> +	 * Below a fractional approximation of Pi is needed.
>> +	 * The following approximation will be used: 103993 / 33102.
>> +	 * This is accurate in 9 decimals places.
>> +	 *
>> +	 * This fraction is based on OEIS series of nominator/denominator
>> +	 * of convergents to Pi (A002485 and A002486).
>> +	 */
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->type) {
>> +		case IIO_ANGL_VEL:
>> +			/*
>> +			 * 2 * Pi ~= 2 * 103993 / 33102
>> +			 *
>> +			 * iio_convert_raw_to_processed uses integer
>> +			 * division. This will cause at most 5% error
>> +			 * (for very small values). But for 99.5% of the values
>> +			 * it will cause less that 1% error.
> This is an interesting comment, but relies on anyone actually
> using iio_convert_raw_to_processed with this device.
>
> I would imagine that 'in kernel' users of a resolver (who would use
> that function) will be few and far between.  Mostly this will just
> get passed to userspace.  That involves this being converted to
> a decimal.  I would just specify it as one in the first place.

Hmm, I didn't realize that it might never be used in kernel. A
decimal representation is indeed a lot more clear. I'll change
scale value type to IIO_VAL_INT_PLUS_MICRO for both the angular
velocity and angel channel.

Best regards,
David Veenstra

>
>> +			 */
>> +			*val = 103993;
>> +			*val2 = 16551;
>> +			return IIO_VAL_FRACTIONAL;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +		break;
>> +	case IIO_CHAN_INFO_RAW:
>> +		mutex_lock(&st->lock);
>> +		gpiod_set_value(st->sample, 0);
>> +
>> +		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
>> +		udelay(1);
>> +		gpiod_set_value(st->sample, 1);
>> +		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
>> +
>> +		ret = spi_read(st->sdev, st->rx, 2);
>> +		if (ret < 0) {
>> +			mutex_unlock(&st->lock);
>> +			return ret;
>> +		}
>>  
>> -	/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
>> -	udelay(1);
>> -	gpiod_set_value(st->sample, 1);
>> -	gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
>> +		vel = be16_to_cpup((__be16 *)st->rx);
>> +		switch (chan->type) {
>> +		case IIO_ANGL:
>> +			*val = vel >> 4;
>> +			break;
>> +		case IIO_ANGL_VEL:
>> +			*val = sign_extend32((s16)vel >> 4, 11);
>> +			break;
>> +		default:
>> +			mutex_unlock(&st->lock);
>> +			return -EINVAL;
>> +		}
>>  
>> -	ret = spi_read(st->sdev, st->rx, 2);
>> -	if (ret < 0) {
>> +		/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
>> +		udelay(1);
>>  		mutex_unlock(&st->lock);
>> -		return ret;
>> -	}
>>  
>> -	vel = be16_to_cpup((__be16 *)st->rx);
>> -	switch (chan->type) {
>> -	case IIO_ANGL:
>> -		*val = vel >> 4;
>> -		break;
>> -	case IIO_ANGL_VEL:
>> -		*val = sign_extend32((s16)vel >> 4, 11);
>> -		break;
>> +		return IIO_VAL_INT;
>>  	default:
>> -		mutex_unlock(&st->lock);
>> -		return -EINVAL;
>> +		break;
>>  	}
>>  
>> -	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
>> -	udelay(1);
>> -	mutex_unlock(&st->lock);
>> -
>> -	return IIO_VAL_INT;
>> +	return -EINVAL;
>>  }
>>  
>>  static const struct iio_chan_spec ad2s1200_channels[] = {
>> @@ -105,6 +138,7 @@ static const struct iio_chan_spec ad2s1200_channels[] = {
>>  		.indexed = 1,
>>  		.channel = 0,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>>  	}
>>  };
>>  

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux