Thread (7 messages) 7 messages, 3 authors, 2020-08-04

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

From: Andy Shevchenko <hidden>
Date: 2020-08-02 18:02:55
Also in: linux-iio, lkml

On Sun, Aug 2, 2020 at 7:40 PM Christian Eggers [off-list ref] wrote:
Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor.

This driver has no built-in trigger. In order for making triggered
measurements, an external (software) trigger driver like
iio-trig-hrtimer or iio-trig-sysfs is required.

The sensor supports single and continuous measurement modes. The latter
is not used by design as this would require tight timing synchronization
between hardware and driver without much benefit.
Thanks for an update, my comments below.
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?

...
+/* Available sample frequencies are 1.024MHz multiplied by powers of two. */
+static const int as73211_samp_freq_avail[] = {
+       AS73211_SAMPLE_FREQ_BASE * 1,
+       AS73211_SAMPLE_FREQ_BASE * 2,
+       AS73211_SAMPLE_FREQ_BASE * 4,
+       AS73211_SAMPLE_FREQ_BASE * 8
+ Comma.
+};
...
+#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?

...
+               *val2 = (AS73211_OFFSET_TEMP - (int)AS73211_OFFSET_TEMP) * 1000000;
+                       *val2 = (AS73211_SCALE_TEMP - (int)AS73211_SCALE_TEMP) * 1000000;
Magic 1000000 multiplier.

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

Consider to use definitions from
https://elixir.bootlin.com/linux/latest/source/include/linux/units.h

...
+       }}
+
+       return -EINVAL;
Make it default case.
+       }
+
+       return -EINVAL;
Ditto.

...
+       }}
+
+       return -EINVAL;
Ditto.

...
+       ret = devm_iio_device_register(dev, indio_dev);
+       if (ret < 0)
+               return ret;
+
+       return 0;
  return devm_iio_device_register();

And consider to drop ' < 0' for devm_*() calls. As far as I understood
your intention to explicitly leave them because of i2c_*() calls,
though devm_*() and such are different.

-- 
With Best Regards,
Andy Shevchenko
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help