From: Jonathan Cameron <jic23@kernel.org>
To: Gustavo Silva <gustavograzs@gmail.com>
Cc: robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org,
lars@metafoo.de, christophe.jaillet@wanadoo.fr,
gerald.loacker@wolfvision.net, devicetree@vger.kernel.org,
linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/6] iio: chemical: add driver for ENS160 sensor
Date: Sun, 26 May 2024 13:20:25 +0100 [thread overview]
Message-ID: <20240526131927.1ab92e13@jic23-huawei> (raw)
In-Reply-To: <wsaoofbe3gvwyejkhkqv3xx4q36a6wupn2yr7ntyklzwxovxhs@s6s2fcy5yebg>
On Sat, 25 May 2024 21:29:42 -0300
Gustavo Silva <gustavograzs@gmail.com> 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@gmail.com> 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@gmail.com>
> >
> > > +
> > > +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
next prev parent reply other threads:[~2024-05-26 12:21 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-12 21:04 [PATCH 0/6] Add driver for ENS160 sensor Gustavo Silva
2024-05-12 21:04 ` [PATCH 1/6] dt-bindings: vendor-prefixes: add ScioSense Gustavo Silva
2024-05-13 16:02 ` Conor Dooley
2024-05-12 21:04 ` [PATCH 2/6] dt-bindings: Add ENS160 as trivial device Gustavo Silva
2024-05-13 16:09 ` Conor Dooley
2024-05-19 12:33 ` Jonathan Cameron
2024-05-12 21:04 ` [PATCH 3/6] iio: chemical: add driver for ENS160 sensor Gustavo Silva
2024-05-13 19:12 ` Christophe JAILLET
2024-05-19 13:24 ` Jonathan Cameron
2024-05-22 23:41 ` Gustavo Silva
2024-05-19 14:01 ` Jonathan Cameron
2024-05-26 0:29 ` Gustavo Silva
2024-05-26 12:20 ` Jonathan Cameron [this message]
2024-05-12 21:04 ` [PATCH 4/6] iio: chemical: ens160: add triggered buffer support Gustavo Silva
2024-05-13 19:13 ` Christophe JAILLET
2024-05-19 14:03 ` Jonathan Cameron
2024-05-20 6:50 ` Christophe JAILLET
2024-05-13 23:50 ` kernel test robot
2024-05-19 14:18 ` Jonathan Cameron
2024-05-12 21:04 ` [PATCH 5/6] iio: chemical: ens160: add power management support Gustavo Silva
2024-05-19 14:18 ` Jonathan Cameron
2024-05-12 21:04 ` [PATCH 6/6] MAINTAINERS: Add ScioSense ENS160 Gustavo Silva
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240526131927.1ab92e13@jic23-huawei \
--to=jic23@kernel.org \
--cc=christophe.jaillet@wanadoo.fr \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gerald.loacker@wolfvision.net \
--cc=gustavograzs@gmail.com \
--cc=krzk+dt@kernel.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).