Message-ID: <CADFWO8FGqD5GyrRtvFptjMdYBhfFFwOzgZ1XnVVEPeY3E8CZPg@mail.gmail.com>
Date: Mon, 10 Jun 2024 10:58:35 +0200
From: Petar Stoykov <pd.pstoykov@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: linux-iio@...r.kernel.org, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Lars-Peter Clausen <lars@...afoo.de>, 
	Rob Herring <robh+dt@...nel.org>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Angel Iglesias <ang.iglesiasg@...il.com>, Conor Dooley <conor+dt@...nel.org>, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 2/3] iio: pressure: Add driver for Sensirion SDP500

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

>
> > +
> > +#define SDP500_SCALE_FACTOR 60
> > +
> > +#define SDP500_I2C_START_MEAS 0xF1
> > +
> > +struct sdp500_data {
> > +    struct device *dev;
> > +};
> > +
> > +DECLARE_CRC8_TABLE(sdp500_crc8_table);
> > +
> > +static int sdp500_start_measurement(struct sdp500_data *data, const
> > struct iio_dev *indio_dev)
> > +{
> > +    struct i2c_client *client = to_i2c_client(data->dev);
> > +
> > +    return i2c_smbus_write_byte(client, SDP500_I2C_START_MEAS);
> Doesn't seem worth a wrapper function. I would just put this code inline.
> > +}
> > +
> > +static const struct iio_chan_spec sdp500_channels[] = {
> > +    {
> > +        .type = IIO_PRESSURE,
> > +        .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>
> As below. It's a linear scale factor, so I would prefer _RAW and _SCALE
> to let userspace deal with the maths.

I saw your other e-mail with further explanation and I begrudgingly agree.
I would much prefer if the "SDP500_SCALE_FACTOR" return code is not
accepted for the "IIO_CHAN_INFO_PROCESSED" case. And I also saw
other drivers do the same as me which gave me confidence it was the right
thing to do.
But, again, it makes sense to avoid this so I'll change it as suggested.

>
> > +    },
> > +};
> > +
> > +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.

> > +    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.
The rec_crc is again for readability, like the SDP500_CRC8_INIT define.
I will change it to "received_crc" which is clearer though.

>
> > +            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);
>
> When you move to returning scale and _raw this print won't add anything so
> drop it.
>
> > +
> > +        *val = dec_value;
> > +        *val2 = SDP500_SCALE_FACTOR;
> For linear transforms like this it is normally better to provide separate
> raw and scale interfaces.
>
> Then if anyone does want to add buffered support in the future that is easier
> to do as it is much more compact + userspace has floating point which is always
> going to be better for division than we can do in kernel.
>
> > +        return IIO_VAL_FRACTIONAL;
> > +    default:
> > +        return -EINVAL;
> > +    }
> > +}
> > +
> > +static const struct iio_info sdp500_info = {
> > +    .read_raw = &sdp500_read_raw,
> > +};
> > +
> > +static int sdp500_probe(struct i2c_client *client)
> > +{
> > +    struct iio_dev *indio_dev;
> > +    struct sdp500_data *data;
> > +    struct device *dev = &client->dev;
> > +    int ret;
> > +    u8 rxbuf[SDP500_READ_SIZE];
> > +
> > +    indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +    if (!indio_dev)
> > +        return -ENOMEM;
> > +
> > +    /* has to be done before the first i2c communication */
> > +    crc8_populate_msb(sdp500_crc8_table, SDP500_CRC8_POLYNOMIAL);
> > +
> > +    data = iio_priv(indio_dev);
> > +    data->dev = dev;
> > +
> > +    indio_dev->name = "sdp500";
> > +    indio_dev->channels = sdp500_channels;
> > +    indio_dev->info = &sdp500_info;
> > +    indio_dev->modes = INDIO_DIRECT_MODE;
> > +    indio_dev->num_channels = ARRAY_SIZE(sdp500_channels);
> > +
> > +    ret = sdp500_start_measurement(data, indio_dev);
> > +    if (ret)
> > +        return dev_err_probe(dev, ret, "Failed to start measurement");
>
> Blank line here would help readability a tiny bit.
>
> > +    /* First measurement is not correct, read it out to get rid of it */
> > +    i2c_master_recv(client, rxbuf, SDP500_READ_SIZE);
> > +
> > +    ret = devm_iio_device_register(dev, indio_dev);
> > +    if (ret < 0)
> > +        return dev_err_probe(dev, ret, "Failed to register indio_dev");
>
> We rarely bother with error prints on failure to register as it is unlikely
> to fail because of something that happened at runtime and if it does, that
> is easy to track down.  So I'd drop this print.
> If you really want to keep it I don't mind that much.
>
> > +
> > +    return 0;
> > +}
> > +
> > +static const struct i2c_device_id sdp500_id[] = {
> > +    { "sdp500" },
> > +    { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, sdp500_id);
> > +
> > +static const struct of_device_id sdp500_of_match[] = {
> > +    { .compatible = "sensirion,sdp500" },
> > +    { }
> > +};
> > +MODULE_DEVICE_TABLE(of, sdp500_of_match);
> > +
> > +static struct i2c_driver sdp500_driver = {
> > +    .driver = {
> > +        .name    = "sensirion,sdp500",
> > +        .of_match_table = sdp500_of_match,
> > +    },
> > +    .probe       = sdp500_probe,
> > +    .id_table    = sdp500_id,
> I'd not bother with aligning = signs. It just tends to create noise
> as drivers evolve and people try to keep things aligned (resulting in realigning
> everything).
>

Good point. It does also bothers me when commits are muddied by
realigning things.

> > +};
> > +module_i2c_driver(sdp500_driver);
> > +
> > +MODULE_AUTHOR("Thomas Sioutas <thomas.sioutas@...drive-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!

Powered by blists - more mailing lists