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

[Gustavo Silva]

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

b) the driver had been previously removed

This is documented in the state diagram on page 24 of the datasheet.

I'll remove this reset.

> > +
> > +	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?

> 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?

> > +	data->regmap = regmap;
> > +
> > +	indio_dev->name = name;
> > +	indio_dev->info = &ens160_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->channels = ens160_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(ens160_channels);
> > +
> > +	ret = ens160_chip_init(data);
> > +	if (ret) {
> > +		dev_err_probe(dev, ret, "chip initialization failed\n");
> > +		return ret;
> 		return dev_err_probe();
> 
> > +	}
> > +
> > +	return devm_iio_device_register(dev, indio_dev);
> > +}
> > +EXPORT_SYMBOL_NS(ens160_core_probe, IIO_ENS160);
> > +
> > +void ens160_core_remove(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct ens160_data *data = iio_priv(indio_dev);
> > +
> > +	ens160_set_mode(data, ENS160_REG_MODE_IDLE);
> 
> This looks to be mixing devm and manual cleanup.
> My guess is this is the unwind for code in ens160_chip_init()
> If so that unwind should definitely happen after we unregister
> the userspace intefaces in the unwind of devm_iio_device_register().
> Currently it happens before this.
> 
> This is an even stronger reason to use devm_add_action_or_reset()
> for this than tidying up as mentioned below (note I tend to
> review backwards through patches so my comments may make more
> sense read that way around).
>
The intention was to transition the chip into idle mode upon driver
removal, ensuring the sensor ceased data readings.
I'll use devm_add_action_or_reset() as suggested.