Re: [PATCH v5 2/2] iio: light: as73211: New driver

[Christian Eggers <ceggers@arri.de>]

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.

Best regards
Christian

> On Sun, Aug 2, 2020 at 7:40 PM Christian Eggers <ceggers@arri.de> 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
> https://elixir.bootlin.com/linux/latest/source/include/linux/units.h
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.