Re: [PATCH v5 2/2] iio: light: as73211: New driver
From: Christian Eggers
Date: Tue Aug 04 2020 - 03:42:32 EST
On Sunday, 2 August 2020, 20:02:35 CEST, Andy Shevchenko wrote:
> Thanks for an update, my comments below.
Thanks for the review. Please see below for my questions.
> On Sun, Aug 2, 2020 at 7:40 PM Christian Eggers <ceggers@xxxxxxx> wrote:
> > Datasheet:
> > https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0-
> > b302-c2fd-e30a-c98df87616df
> Do we need the UUID after the document file name?
I have send AMS an inquiry. Not sure whether I will get an answer. I will wait
a few days until sending v6.
> > +#define AS73211_OFFSET_TEMP (-66.9)
> > +#define AS73211_SCALE_TEMP 0.05
> In the kernel we don't do float arithmetic. How these are being used?
Does this restriction also apply for compile time constants? I am quite
sure that all calculations using these defines will be evaluated at compile
time. If found a number of other places where probably the same is done:
find . -name '*.c' | xargs grep "#define.*[0-9]\.[0-9]" | grep -v '"' | grep -v "\/\*.*[0-9]\.[0-9]"
> > + *val2 = (AS73211_OFFSET_TEMP - (int)AS73211_OFFSET_TEMP) *
> > 1000000;
> > + *val2 = (AS73211_SCALE_TEMP -
> > (int)AS73211_SCALE_TEMP) * 1000000;
> Magic 1000000 multiplier.
I think that in the context of IIO_VAL_INT_PLUS_MICRO this isn't quite magic. Using
1000000 directly seems quite usual:
find drivers/iio/ -type f | xargs grep "val2 = .*1000000"
> I think here you got them always 0. And to fix that you need to
> redefine (with also units included in the name) above constants like
> #define ..._OFFSET_TEMP_mC 66500
> ... _SCALE_TEMP_?? 50
a scale factor has no unit
> Consider to use definitions from
There are only definition for milli celsius. For IIO_VAL_INT_PLUS_MICRO I would
require micro celsius.
If I have the freedom, I would keep it as it is. Else I would suggest the following:
#define AS73211_OFFSET_TEMP_INT (-66)
#define AS73211_OFFSET_TEMP_MICRO 900000
#define AS73211_SCALE_TEMP_INT 0
#define AS73211_SCALE_TEMP_MICRO 50000
> > + }}
> > +
> > + return -EINVAL;
> Make it default case.
changed. Is there any benefit? My IDE's syntax checker now complains
"No return, in a function returning non-void". But gcc is happy with this.
> > + ret = devm_iio_device_register(dev, indio_dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> return devm_iio_device_register();
changed. I prefer the original pattern as it would produce less changed lines
if something needs to inserted later.