Re: [PATCH 3/6] iio: chemical: add driver for ENS160 sensor

[Jonathan Cameron]

On Sat, 25 May 2024 21:29:42 -0300
Gustavo Silva <gustavograzs@xxxxxxxxx> wrote:

> Hi Jonathan,
> 
> Thank you for your review. I've got a few questions inline.
> 
> On Sun, May 19, 2024 at 03:01:51PM GMT, Jonathan Cameron wrote:
> > On Sun, 12 May 2024 18:04:39 -0300
> > Gustavo Silva <gustavograzs@xxxxxxxxx> wrote:
> >   
> > > ScioSense ENS160 is a digital metal oxide multi-gas sensor, designed
> > > for indoor air quality monitoring. The driver supports readings of
> > > CO2 and VOC, and can be accessed via both SPI and I2C.
> > > 
> > > Signed-off-by: Gustavo Silva <gustavograzs@xxxxxxxxx>  
> >   
> > > +
> > > +static int ens160_read_raw(struct iio_dev *indio_dev,
> > > +			   struct iio_chan_spec const *chan,
> > > +			   int *val, int *val2, long mask)
> > > +{
> > > +	struct ens160_data *data = iio_priv(indio_dev);
> > > +	__le16 buf;
> > > +	int ret;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		ret = regmap_bulk_read(data->regmap, chan->address,
> > > +					&buf, sizeof(buf));  
> > 
> > As below, should use a DMA safe buffer.
> >   
> > > +static int ens160_chip_init(struct ens160_data *data)
> > > +{
> > > +	struct device *dev = regmap_get_device(data->regmap);
> > > +	u8 fw_version[3];
> > > +	__le16 part_id;
> > > +	unsigned int status;
> > > +	int ret;
> > > +
> > > +	ret = ens160_set_mode(data, ENS160_REG_MODE_RESET);
> > > +	if (ret)
> > > +		return ret;  
> > 
> > No docs that I can see on what this means for access to registers etc.
> > Good to add a comment if you have info on this.
> >   
> Performing a reset at this point isn't strictly necessary. When we reach
> this point the chip should be in idle state because:
> 
> a) it was just powered on

Maybe but we have no way of telling that
> 
> b) the driver had been previously removed

Maybe or maybe not - the device may have just done a soft reboot and switched
operating system. We have no idea what the hardware state is.
As such the reset is a good idea.
> 
> This is documented in the state diagram on page 24 of the datasheet.
> 
> I'll remove this reset.

Better to keep the reset and provide info on what it means wrt to accessing
registers etc if possible. If there is no information then obviously not
much you can add in the way of documentation!

> 
> > > +
> > > +	ret = regmap_bulk_read(data->regmap, ENS160_REG_PART_ID, &part_id,
> > > +			       sizeof(part_id));  
> > 
> > Ah. So this is a fun corner case.  Currently regmap makes not guarantees
> > to always bounce buffer things (though last time I checked it actually did
> > do so - there are optimisations that may make sense where it will again
> > not do so).  So given we have an SPI bus involved, we should ensure that
> > only DMA safe buffers are used. These need to ensure that no other data
> > that might be changed concurrently with DMA is in the same IIO_DMA_MINALIGN
> > of aligned data (traditionally a cacheline but it gets more complex in some
> > systems and is less in others).  Upshot is that if you are are doing
> > bulk accesses you need to use a buffer that is either on the heap (kzalloc etc)
> > or carefully placed at the end of the iio_priv() structure marked
> > __align(IIO_DMA_MINALIGN). Lots of examples of that in tree.
> > If you are curious, wolfram did a good talk on the i2c equivalent of this
> > a few years back. 
> > https://www.youtube.com/watch?v=JDwaMClvV-s I think.
> >  
> Interesting. Thank you for the detailed info.
> 
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (le16_to_cpu(part_id) != ENS160_PART_ID)
> > > +		return -ENODEV;
> > > +
> > > +	ret = ens160_set_mode(data, ENS160_REG_MODE_IDLE);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
> > > +			   ENS160_REG_COMMAND_CLRGPR);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = regmap_write(data->regmap, ENS160_REG_COMMAND,
> > > +			   ENS160_REG_COMMAND_GET_APPVER);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	msleep(ENS160_BOOTING_TIME_MS);  
> > 
> > Why here?  Not obviously associated with a boot command?
> > A comment might make this easier to follow.  I 'think' it is
> > because this next read is the first time it matters. If so that
> > isn't obvious.  Also, there is an existing sleep in the mode set,
> > so I'm not sure why we need another one.
> >  
> The usage of booting time is not documented in the datasheet. From
> ScioSense's arduino driver the booting time is necessary after setting
> the operation mode. I performed some tests that confirm this.
> 
> In this case in particular it is not necessary. Maybe I forgot to remove
> it after some testing.
> > > +
> > > +	ret = regmap_bulk_read(data->regmap, ENS160_REG_GPR_READ4,
> > > +			       fw_version, sizeof(fw_version));  
> >   
> Does this bulk read also need to be made DMA safe? I'm guessing in this
> case it would be best to devm_kzalloc() a buffer of three bytes?
All bulk accesses need dma safe buffers.
I would take the approach many IIO drivers do of not doing another allocation
(which has overheads etc) but instead just add a suitable __aligned(IIO_DMA_MINALIGN)
buffer to the iio_priv structure.  Note you normally only need to do mark
the first one like that as we don't care if various different DMA buffers
are in the same cacheline as the SPI controller should not cause DMA safety
issues with itself.

> 
> > The first datasheet that google provided me has this 
> > GPR_READ0/GPR_READ1 and only 2 bytes. I hope they have maintained backwards
> > compatibility with that earlier doc!
> > 
> > When you do a separate DT binding in v2, make sure to include a link
> > to the datasheet you are using.  Also use a Datasheet: tag
> > in this patch to make it easy to find that.
> > I dug a little deeper and found the one on sciosense own website
> > - ah, you do have it in the comments.  Add to the commit message
> > and DT binding as well.
> > 
> >   
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	msleep(ENS160_BOOTING_TIME_MS);  
> > Why again?  
> Again, not needed. I'll remove it.
> 
> > > +	data = iio_priv(indio_dev);
> > > +	dev_set_drvdata(dev, indio_dev);  
> > 
> > After you've moved to devm_add_action_or_reset() for the unwind of
> > ens160_chip_init() you probably don't need to set the drvdata.
> >   
> I don't get it. Could you please elaborate on why it isn't needed to
> set drvdata after the change?

No other users.  Only the remove() callback calls the matching get_drvdata()
and that function won't exist once you've added device managed callbacks
to handle everything it does.

Jonathan