Re: [PATCH v2 2/3] iio: pressure: Add driver for Sensirion SDP500

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 10 Jun 2024 10:58:35 +0200
Petar Stoykov <pd.pstoykov@gmail.com> wrote:

> On Sun, May 5, 2024 at 7:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 30 Apr 2024 17:27:24 +0200
> > Petar Stoykov <pd.pstoykov@gmail.com> wrote:
> >  
> > > From 6ae7537517f551540121ca6fb3b99080b7580410 Mon Sep 17 00:00:00 2001
> > > From: Petar Stoykov <pd.pstoykov@gmail.com>
> > > 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@gmail.com>  
> > 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@prodrive-technologies.com>");
> > > +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