[v5,0/2] iio: light: Support AMS AS73211 digital XYZ sensor

Message

Christian Eggers Aug. 2, 2020, 4:37 p.m. UTC

This series adds support for the AMS AS73211 digital XYZ sensor. Some
comments to the review emails below...

Again, many thanks for the comprehensive reviews.

Changes in v5:
---------------
- [1/2] Reviewed by Rob Herring
- [2/2] Added KHZ_PER_HR define
- [2/2] Added AS73211_SAMPLE_FREQ_BASE define
- [2/2] Slight changes in comments
- [2/2] Claim direct mode in write_raw()
- [2/2] Saturate only in case of overflow
- [2/2] Don't set indio_dev->dev.parent
- [2/2] Fix error path (order) in probe()

Changes in v4:
---------------
- Integrated 2nd review from Andy Shevchenko
- Use more devm_ functions in as73211_probe()

Changes in v3:
---------------
- Integrated comments from Andy Shevchenko
- Integrated comments from Jonathan Cameron

Changes in v2:
---------------
- Fix $id in dt binding
- Document full I2C address range in "reg" property
- Move "buffer" member out of "struct as73211_data"
- Fix sparse warnings by using correct data types


On Friday, 31 July 2020, 17:41:47 CEST, Andy Shevchenko wrote:
> On Fri, Jul 31, 2020 at 1:52 PM Christian Eggers <[e-mail]> wrote:
> > devm_iio_device_alloc() doesn't pass 'dev' to iio_device_alloc().
> > I already looked around, but I didn't find. And after debugging
> > v5.4, devm_iio_device_alloc() definitely doesn't do it.
> 
> Why are you talking about v5.4? We are in v5.8 cycle contributing to v5.9.
v5.4-rt is the version for my product release. Current base for these
patches is 5.8-rc6


On Friday, 31 July 2020, 18:19:55 CEST, Jonathan Cameron wrote:
> > On Fri, Jul 31, 2020 at 1:52 PM Christian Eggers <[e-mail]> wrote:
> > Recently IIO gained some features among which I think the one that
> > assigns parent devices.
> 
> yup. This should be in linux-next now for the coming merge window (which
> probably opens on Sunday).
Thanks for the hint. Is there a particular tree which is preferred for
IIO development?


On Saturday, 1 August 2020, 17:29:58 CEST, Jonathan Cameron wrote:
> On Fri, 31 Jul 2020 09:01:14 +0200 Christian Eggers <[e-mail]> wrote:
> > +static int as73211_write_raw(struct iio_dev *indio_dev, struct
> > iio_chan_spec const *chan, +			      int val, int val2, long mask)
> > +{
> > +	...
> > +	/* Need to switch to config mode ... */
> 
> Is this safe whilst we are doing a capture?
> You may want to claim_direct_mode for write_raw to ensure we don't get such
> a race.
> 
That's an important point! As I use iio-trig-sysfs, I never had any
problems with this. But if for instance iio-trig-hrtimer is active, this
could become a problem.

In v5, I have claimed direct mode. For the application this means, that
buffer/enable has to be set to '0' before any settings can be
changed. As libiio doesn't support enabling/disabling a buffer directly,
the buffer needs to be destroyed and newly constructed.