LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] iio: cros_ec: Fix gyro scale calculation
@ 2019-02-20 15:03 Enric Balletbo i Serra
  2019-02-20 16:01 ` Jonathan Cameron
  0 siblings, 1 reply; 5+ messages in thread
From: Enric Balletbo i Serra @ 2019-02-20 15:03 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio, linux-kernel
  Cc: Gwendal Grignou, Peter Meerwald-Stadler, Guenter Roeck,
	Benson Leung, Lars-Peter Clausen, kernel, Hartmut Knaack

From: Gwendal Grignou <gwendal@chromium.org>

Calculation was copied from IIO_DEGREE_TO_RAD, but offset added to avoid
rounding error is wrong. It should be only half of the divider.

Fixes: c14dca07a31d ("iio: cros_ec_sensors: add ChromeOS EC Contiguous Sensors driver")
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
index 89cb0066a6e0..600942af9f9c 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
@@ -103,7 +103,7 @@ static int cros_ec_sensors_read(struct iio_dev *indio_dev,
 			 * Do not use IIO_DEGREE_TO_RAD to avoid precision
 			 * loss. Round to the nearest integer.
 			 */
-			*val = div_s64(val64 * 314159 + 9000000ULL, 1000);
+			*val = div_s64(val64 * 314159 + 500ULL, 1000);
 			*val2 = 18000 << (CROS_EC_SENSOR_BITS - 1);
 			ret = IIO_VAL_FRACTIONAL;
 			break;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio: cros_ec: Fix gyro scale calculation
  2019-02-20 15:03 [PATCH] iio: cros_ec: Fix gyro scale calculation Enric Balletbo i Serra
@ 2019-02-20 16:01 ` Jonathan Cameron
  2019-02-22 10:24   ` Enric Balletbo i Serra
  0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2019-02-20 16:01 UTC (permalink / raw)
  To: Enric Balletbo i Serra
  Cc: linux-iio, linux-kernel, Gwendal Grignou, Peter Meerwald-Stadler,
	Guenter Roeck, Benson Leung, Lars-Peter Clausen, kernel,
	Hartmut Knaack

On Wed, 20 Feb 2019 16:03:00 +0100
Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:

> From: Gwendal Grignou <gwendal@chromium.org>
> 
> Calculation was copied from IIO_DEGREE_TO_RAD, but offset added to avoid
> rounding error is wrong. It should be only half of the divider.
> 
> Fixes: c14dca07a31d ("iio: cros_ec_sensors: add ChromeOS EC Contiguous Sensors driver")
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

This one is kind of interesting. See below.

> ---
> 
>  drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> index 89cb0066a6e0..600942af9f9c 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> @@ -103,7 +103,7 @@ static int cros_ec_sensors_read(struct iio_dev *indio_dev,
>  			 * Do not use IIO_DEGREE_TO_RAD to avoid precision
>  			 * loss. Round to the nearest integer.
>  			 */
> -			*val = div_s64(val64 * 314159 + 9000000ULL, 1000);
> +			*val = div_s64(val64 * 314159 + 500ULL, 1000);
That is only one of two divides going on.  Firstly we divide by 1000 here,
then we provide it in fractional form which means that the actual value you get
from sysfs etc is 
val/val2.  It's this one we are protecting against rounding error on I guess.
Now this is even less obviously because it's not 18000 either, but 
18000 * 2^CROS_EC_SENSOR_BITS.

Which ultimately means neither answer is correct. Hmm.
Not totally sure what the right answer actually is..

Jonathan

>  			*val2 = 18000 << (CROS_EC_SENSOR_BITS - 1);
>  			ret = IIO_VAL_FRACTIONAL;
>  			break;


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio: cros_ec: Fix gyro scale calculation
  2019-02-20 16:01 ` Jonathan Cameron
@ 2019-02-22 10:24   ` Enric Balletbo i Serra
  2019-03-03 16:47     ` Jonathan Cameron
  0 siblings, 1 reply; 5+ messages in thread
From: Enric Balletbo i Serra @ 2019-02-22 10:24 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, linux-kernel, Gwendal Grignou, Peter Meerwald-Stadler,
	Guenter Roeck, Benson Leung, Lars-Peter Clausen, kernel,
	Hartmut Knaack

Hi Jonathan,

On 20/2/19 17:01, Jonathan Cameron wrote:
> On Wed, 20 Feb 2019 16:03:00 +0100
> Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
> 
>> From: Gwendal Grignou <gwendal@chromium.org>
>>
>> Calculation was copied from IIO_DEGREE_TO_RAD, but offset added to avoid
>> rounding error is wrong. It should be only half of the divider.
>>
>> Fixes: c14dca07a31d ("iio: cros_ec_sensors: add ChromeOS EC Contiguous Sensors driver")
>> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> 
> This one is kind of interesting. See below.
> 
>> ---
>>
>>  drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
>> index 89cb0066a6e0..600942af9f9c 100644
>> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
>> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
>> @@ -103,7 +103,7 @@ static int cros_ec_sensors_read(struct iio_dev *indio_dev,
>>  			 * Do not use IIO_DEGREE_TO_RAD to avoid precision
>>  			 * loss. Round to the nearest integer.
>>  			 */
>> -			*val = div_s64(val64 * 314159 + 9000000ULL, 1000);
>> +			*val = div_s64(val64 * 314159 + 500ULL, 1000);
> That is only one of two divides going on.  Firstly we divide by 1000 here,
> then we provide it in fractional form which means that the actual value you get
> from sysfs etc is 
> val/val2.  It's this one we are protecting against rounding error on I guess.
> Now this is even less obviously because it's not 18000 either, but 
> 18000 * 2^CROS_EC_SENSOR_BITS.
> 
> Which ultimately means neither answer is correct. Hmm.
> Not totally sure what the right answer actually is..
> 

If I understood well the Gwendal's patch the problem that we're trying to solve
is that current calculation is not closer from the float calculation.

For 1000dps, the result should be:

    (1000 * pi ) / 180 >> 15 ~= 0.000532632218

But with current calculation we get

    $ cat scale
    0.000547890

With that patch (modifying the offset to avoid the rounding error) we get a
closer result

    $ cat scale
    0.000532631

So, what we're trying to do is have val/val2 closer to the real value. Makes
this sense to you or I'm missing something? I can improve the commit message if
it's not clear.

-- Enric

> Jonathan
> 
>>  			*val2 = 18000 << (CROS_EC_SENSOR_BITS - 1);
>>  			ret = IIO_VAL_FRACTIONAL;
>>  			break;
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio: cros_ec: Fix gyro scale calculation
  2019-02-22 10:24   ` Enric Balletbo i Serra
@ 2019-03-03 16:47     ` Jonathan Cameron
  2019-03-06 11:16       ` Enric Balletbo i Serra
  0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2019-03-03 16:47 UTC (permalink / raw)
  To: Enric Balletbo i Serra
  Cc: linux-iio, linux-kernel, Gwendal Grignou, Peter Meerwald-Stadler,
	Guenter Roeck, Benson Leung, Lars-Peter Clausen, kernel,
	Hartmut Knaack

On Fri, 22 Feb 2019 11:24:24 +0100
Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:

> Hi Jonathan,
> 
> On 20/2/19 17:01, Jonathan Cameron wrote:
> > On Wed, 20 Feb 2019 16:03:00 +0100
> > Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
> >   
> >> From: Gwendal Grignou <gwendal@chromium.org>
> >>
> >> Calculation was copied from IIO_DEGREE_TO_RAD, but offset added to avoid
> >> rounding error is wrong. It should be only half of the divider.
> >>
> >> Fixes: c14dca07a31d ("iio: cros_ec_sensors: add ChromeOS EC Contiguous Sensors driver")
> >> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>  
> > 
> > This one is kind of interesting. See below.
> >   
> >> ---
> >>
> >>  drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> >> index 89cb0066a6e0..600942af9f9c 100644
> >> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> >> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> >> @@ -103,7 +103,7 @@ static int cros_ec_sensors_read(struct iio_dev *indio_dev,
> >>  			 * Do not use IIO_DEGREE_TO_RAD to avoid precision
> >>  			 * loss. Round to the nearest integer.
> >>  			 */
> >> -			*val = div_s64(val64 * 314159 + 9000000ULL, 1000);
> >> +			*val = div_s64(val64 * 314159 + 500ULL, 1000);  
> > That is only one of two divides going on.  Firstly we divide by 1000 here,
> > then we provide it in fractional form which means that the actual value you get
> > from sysfs etc is 
> > val/val2.  It's this one we are protecting against rounding error on I guess.
> > Now this is even less obviously because it's not 18000 either, but 
> > 18000 * 2^CROS_EC_SENSOR_BITS.
> > 
> > Which ultimately means neither answer is correct. Hmm.
> > Not totally sure what the right answer actually is..
> >   
> 
> If I understood well the Gwendal's patch the problem that we're trying to solve
> is that current calculation is not closer from the float calculation.
> 
> For 1000dps, the result should be:
> 
>     (1000 * pi ) / 180 >> 15 ~= 0.000532632218
> 
> But with current calculation we get
> 
>     $ cat scale
>     0.000547890
> 
> With that patch (modifying the offset to avoid the rounding error) we get a
> closer result
> 
>     $ cat scale
>     0.000532631
> 
> So, what we're trying to do is have val/val2 closer to the real value. Makes
> this sense to you or I'm missing something? I can improve the commit message if
> it's not clear.

I think we are in enough of a mess here with the different dividers that we
should just do the maths here, then we can avoid the bia.

aiming for nano value.
val * pi * 10e12 / (180 * 2^15)
div_s64(val * 3141592653000 + 2949120, 5898240) = 532632
vs 532632 for floating point division.
Then use IIO_INT_PLUS_NANO to return it.

Even then I suspect the +2949120 is only effecting the last digit so
you could probably drop it safely enough.

I'd certainly rather we had all the magic in one place rather than
trying to correct for divisions that aren't apparent here.

> 
> -- Enric
> 
> > Jonathan
> >   
> >>  			*val2 = 18000 << (CROS_EC_SENSOR_BITS - 1);
> >>  			ret = IIO_VAL_FRACTIONAL;
> >>  			break;  
> >   


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio: cros_ec: Fix gyro scale calculation
  2019-03-03 16:47     ` Jonathan Cameron
@ 2019-03-06 11:16       ` Enric Balletbo i Serra
  0 siblings, 0 replies; 5+ messages in thread
From: Enric Balletbo i Serra @ 2019-03-06 11:16 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, linux-kernel, Gwendal Grignou, Peter Meerwald-Stadler,
	Guenter Roeck, Benson Leung, Lars-Peter Clausen, kernel,
	Hartmut Knaack

Hi Jonathan,

On 3/3/19 17:47, Jonathan Cameron wrote:
> On Fri, 22 Feb 2019 11:24:24 +0100
> Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
> 
>> Hi Jonathan,
>>
>> On 20/2/19 17:01, Jonathan Cameron wrote:
>>> On Wed, 20 Feb 2019 16:03:00 +0100
>>> Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
>>>   
>>>> From: Gwendal Grignou <gwendal@chromium.org>
>>>>
>>>> Calculation was copied from IIO_DEGREE_TO_RAD, but offset added to avoid
>>>> rounding error is wrong. It should be only half of the divider.
>>>>
>>>> Fixes: c14dca07a31d ("iio: cros_ec_sensors: add ChromeOS EC Contiguous Sensors driver")
>>>> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>  
>>>
>>> This one is kind of interesting. See below.
>>>   
>>>> ---
>>>>
>>>>  drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
>>>> index 89cb0066a6e0..600942af9f9c 100644
>>>> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
>>>> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
>>>> @@ -103,7 +103,7 @@ static int cros_ec_sensors_read(struct iio_dev *indio_dev,
>>>>  			 * Do not use IIO_DEGREE_TO_RAD to avoid precision
>>>>  			 * loss. Round to the nearest integer.
>>>>  			 */
>>>> -			*val = div_s64(val64 * 314159 + 9000000ULL, 1000);
>>>> +			*val = div_s64(val64 * 314159 + 500ULL, 1000);  
>>> That is only one of two divides going on.  Firstly we divide by 1000 here,
>>> then we provide it in fractional form which means that the actual value you get
>>> from sysfs etc is 
>>> val/val2.  It's this one we are protecting against rounding error on I guess.
>>> Now this is even less obviously because it's not 18000 either, but 
>>> 18000 * 2^CROS_EC_SENSOR_BITS.
>>>
>>> Which ultimately means neither answer is correct. Hmm.
>>> Not totally sure what the right answer actually is..
>>>   
>>
>> If I understood well the Gwendal's patch the problem that we're trying to solve
>> is that current calculation is not closer from the float calculation.
>>
>> For 1000dps, the result should be:
>>
>>     (1000 * pi ) / 180 >> 15 ~= 0.000532632218
>>
>> But with current calculation we get
>>
>>     $ cat scale
>>     0.000547890
>>
>> With that patch (modifying the offset to avoid the rounding error) we get a
>> closer result
>>
>>     $ cat scale
>>     0.000532631
>>
>> So, what we're trying to do is have val/val2 closer to the real value. Makes
>> this sense to you or I'm missing something? I can improve the commit message if
>> it's not clear.
> 
> I think we are in enough of a mess here with the different dividers that we
> should just do the maths here, then we can avoid the bia.
> 
> aiming for nano value.
> val * pi * 10e12 / (180 * 2^15)
> div_s64(val * 3141592653000 + 2949120, 5898240) = 532632
> vs 532632 for floating point division.
> Then use IIO_INT_PLUS_NANO to return it.
> 
> Even then I suspect the +2949120 is only effecting the last digit so
> you could probably drop it safely enough.
> 
> I'd certainly rather we had all the magic in one place rather than
> trying to correct for divisions that aren't apparent here.
> 

Thanks for the clear explanation, yes, this looks better. I'll do some tests and
submit a second version.

Thanks!

>>
>> -- Enric
>>
>>> Jonathan
>>>   
>>>>  			*val2 = 18000 << (CROS_EC_SENSOR_BITS - 1);
>>>>  			ret = IIO_VAL_FRACTIONAL;
>>>>  			break;  
>>>   
> 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-03-06 11:16 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-20 15:03 [PATCH] iio: cros_ec: Fix gyro scale calculation Enric Balletbo i Serra
2019-02-20 16:01 ` Jonathan Cameron
2019-02-22 10:24   ` Enric Balletbo i Serra
2019-03-03 16:47     ` Jonathan Cameron
2019-03-06 11:16       ` Enric Balletbo i Serra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).