On Mon, 10 Jun 2024 10:58:35 +0200
Petar Stoykov <pd.pstoykov@xxxxxxxxx> wrote:
> On Sun, May 5, 2024 at 7:18 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Tue, 30 Apr 2024 17:27:24 +0200
> > Petar Stoykov <pd.pstoykov@xxxxxxxxx> wrote:
> >
> > > From 6ae7537517f551540121ca6fb3b99080b7580410 Mon Sep 17 00:00:00 2001
> > > From: Petar Stoykov <pd.pstoykov@xxxxxxxxx>
> > > Date: Mon, 15 Jan 2024 12:21:26 +0100
> > > Subject: [PATCH 2/3] iio: pressure: Add driver for Sensirion SDP500
> > >
> > > Sensirion SDP500 is a digital differential pressure sensor. The sensor is
> > > accessed over I2C.
> > >
> > > Signed-off-by: Petar Stoykov <pd.pstoykov@xxxxxxxxx>
> > Hi Petar
> >
> > Ignoring the patch formatting which others have already given feedback on,
> > a few minor comments inline.
> >
> > Also, I'd expect some regulator handling to turn the power on.
> > Obviously on your particular board there may be nothing to do but good to
> > have the support in place anyway and it will be harmless if the power
> > is always on.
> >
> > Jonathan
> >
> Hi Jonathan,
>
> Thank you for looking past the formatting!
>
> I wrongly assumed the power regulator would be handled automatically :)
> I see examples of how to do it in other pressure drivers now.
>
> > > st_pressure-$(CONFIG_IIO_BUFFER) += st_pressure_buffer.o
> > > diff --git a/drivers/iio/pressure/sdp500.c b/drivers/iio/pressure/sdp500.c
> > > new file mode 100644
> > > index 000000000000..7efcc69e829c
> > > --- /dev/null
> > > +++ b/drivers/iio/pressure/sdp500.c
> > > @@ -0,0 +1,144 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +#include <linux/i2c.h>
> > > +#include <linux/crc8.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <asm/unaligned.h>
> > > +
> > > +#define SDP500_CRC8_POLYNOMIAL 0x31 // x8 + x5 + x4 + 1 (normalized to 0x31)
> > > +#define SDP500_READ_SIZE 3
> > > +#define SDP500_CRC8_WORD_LENGTH 2
> >
> > As below. I'd establish these off the data the are the lengths of by using
> > a structure definition. That will be more obvious and less fragile than
> > defines hiding up here.
> >
> >
> > > +#define SDP500_CRC8_INIT 0x00
> >
> > I'd just use the number inline. Can't see what the define is adding.
>
> I've been taught to avoid magic numbers as much as possible.
> Giving it a define directly explains what the number is, even if it's used once.
> But I'll follow the community (in this case, you) for this.
Normally I agree with the magic number case, but this
is an actual value. We are saying continue the CRC from
0 (i.e. nothing). It's kind of the logical default value
so seeing it in line makes it clear we aren't continuing form
a prior crc etc.
...
> >
> > > + },
> > > +};
> > > +
> > > +static int sdp500_read_raw(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan,
> > > + int *val, int *val2, long mask)
> > > +{
> > > + int ret;
> > > + u8 rxbuf[SDP500_READ_SIZE];
> > You could define this as a struct so all the data types are obvious.
> >
> > struct {
> > __be16 data;
> > u8 crc;
> > } __packed rxbuf;
> > The __packed let's you use sizeof(rxbuf) for the transfer size.
> > Beware though as IIRC that will mean data is not necessarily aligned
> > so you'll still need the unaligned accessors.
> >
>
> I know, but I prefer to receive data in simple arrays and then deal with it.
The disadvantage is you loose the readability a structure brings, but
meh, I don't care that much.
>
> > > + u8 rec_crc, calculated_crc;
> > > + s16 dec_value;
> > > + struct sdp500_data *data = iio_priv(indio_dev);
> > > + struct i2c_client *client = to_i2c_client(data->dev);
> > > +
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_PROCESSED:
> > > + ret = i2c_master_recv(client, rxbuf, SDP500_READ_SIZE);
> > > + if (ret < 0) {
> > > + dev_err(indio_dev->dev.parent, "Failed to receive data");
> > > + return ret;
> > > + }
> > > + if (ret != SDP500_READ_SIZE) {
> > > + dev_err(indio_dev->dev.parent, "Data is received wrongly");
> >
> > I'd guess indio_dev->dev.parent == data->dev
> > If so use data->dev as more compact and that's where you are getting the
> > i2c_client from.
> >
>
> Makes sense.
>
> > > + return -EIO;
> > > + }
> > > +
> > > + rec_crc = rxbuf[2];
> > > + calculated_crc = crc8(sdp500_crc8_table, rxbuf,
> > > SDP500_CRC8_WORD_LENGTH,
> >
> > I'd use the number 2 for length directly as it's useful to know this is the
> > __be16 only, or sizeof(__be16)
> > What is the point in rec_crc local variable?
>
> Ok, I will use sizeof(rxbuff) - 1 instead of the define.
That's obscure and another reason I'd rather see a structure so this
becomes sizeof(a.data)
> The rec_crc is again for readability, like the SDP500_CRC8_INIT define.
> I will change it to "received_crc" which is clearer though.
The fact you compare it with the crc makes that pretty obvious, but
fair enough I guess if you think it helps.
>
> >
> > > + SDP500_CRC8_INIT);
> > > + if (rec_crc != calculated_crc) {
> > > + dev_err(indio_dev->dev.parent, "calculated crc = 0x%.2X,
> > > received 0x%.2X",
> > > + calculated_crc, rec_crc);
> > > + return -EIO;
> > > + }
> > > +
> > > + dec_value = get_unaligned_be16(rxbuf);
> > > + dev_dbg(indio_dev->dev.parent, "dec value = %d", dec_value);
> >
>
> > > +};
> > > +module_i2c_driver(sdp500_driver);
> > > +
> > > +MODULE_AUTHOR("Thomas Sioutas <thomas.sioutas@xxxxxxxxxxxxxxxxxxxxxxxxx>");
> > > +MODULE_DESCRIPTION("Driver for Sensirion SDP500 differential pressure sensor");
> > > +MODULE_LICENSE("GPL");
> >
>
> I will test the driver with the suggested changes as soon as I get the
> hardware again
> and I will try using the b4 tool with "web submission endpoint". Thanks again!
>
Good luck! (it should be fine but I've never tried it :)
Jonathan
