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@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 > 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.
