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.
