On Sat, Sep 03, 2016 at 04:13:30PM +0100, Jonathan Cameron wrote:
> On 02/09/16 19:27, Gregor Boirie wrote:
> > 7985e7c100 ("iio: Introduce a new fractional value type") introduced a
> > new IIO_VAL_FRACTIONAL value type meant to represent rational type numbers
> > expressed by a numerator and denominator combination.
> >
> > Formating of IIO_VAL_FRACTIONAL values relies upon do_div() usage. This
> > fails handling negative values properly since parameters are reevaluated
> > as unsigned values.
> > Fix this by using div_s64_rem() instead. Computed integer part will carry
> > properly signed value. Formatted fractional part will always be positive.
> >
> > Fixes: 7985e7c100 ("iio: Introduce a new fractional value type")
> > Signed-off-by: Gregor Boirie <gregor.boirie@xxxxxxxxxx>
>
> Hi Gregor,
Hi,
>
> While this looks sensible to me, I always gain an almighty headache when
> I hit the various divide functions.
So am I.
>
> Lars, the fractional code was yours in the first place.
> If you have time can you sanity check this please.
This patch may break a lot things indeed. I'd feel much more comfortable if
someone else could perform additional testing too.
Grégor.
>
> Jonathan
>
> > ---
> > drivers/iio/industrialio-core.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index f914d5d..d2b8899 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -613,9 +613,8 @@ ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals)
> > return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
> > case IIO_VAL_FRACTIONAL:
> > tmp = div_s64((s64)vals[0] * 1000000000LL, vals[1]);
> > - vals[1] = do_div(tmp, 1000000000LL);
> > - vals[0] = tmp;
> > - return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
> > + vals[0] = (int)div_s64_rem(tmp, 1000000000, &vals[1]);
> > + return sprintf(buf, "%d.%09u\n", vals[0], abs(vals[1]));
> > case IIO_VAL_FRACTIONAL_LOG2:
> > tmp = (s64)vals[0] * 1000000000LL >> vals[1];
> > vals[1] = do_div(tmp, 1000000000LL);
> >
>
--
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
