Message-ID: <CADFWO8EF0WxAV=k-ZAJ1McmaYv4SD5G+O4FhoMDsVQaRqe6sdg@mail.gmail.com>
Date: Tue, 30 Apr 2024 17:11:08 +0200
From: Petar Stoykov <pd.pstoykov@...il.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: linux-iio@...r.kernel.org, Jonathan Cameron <jic23@...nel.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>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 2/3] iio: pressure: Add driver for Sensirion SDP500

On Tue, Jan 16, 2024 at 6:03 PM Jonathan Cameron
<Jonathan.Cameron@...wei.com> wrote:
>
> On Tue, 16 Jan 2024 16:24:56 +0100
> Petar Stoykov <pd.pstoykov@...il.com> wrote:
>
> > 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,
>
> Welcome to IIO.
>
> A few quick comments inline to add to Krzysztof's review
>
> Jonathan
>
> > diff --git a/drivers/iio/pressure/sdp500.c b/drivers/iio/pressure/sdp500.c
> > new file mode 100644
> > index 000000000000..bc492ef3ef3e
> > --- /dev/null
> > +++ b/drivers/iio/pressure/sdp500.c
> > @@ -0,0 +1,201 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +
> > +#define SDP500_CRC8_POLYNOMIAL  0x31   // x8 + x5 + x4 + 1 (normalized to 0x31)
> > +#define SDP500_READ_SIZE        3
> > +
> > +#define SDP500_SCALE_FACTOR 60
> > +
> > +#define SDP500_I2C_START_MEAS 0xF1
> > +
> > +#define sdp500_err(idev, fmt, ...)                    \
> > +    dev_err(idev->dev.parent, fmt "\n", ##__VA_ARGS__)
> > +
> > +#define sdp500_dbg(idev, fmt, ...)                    \
> > +    dev_dbg(idev->dev.parent, fmt "\n", ##__VA_ARGS__)
> > +
> > +#define sdp500_info(idev, fmt, ...)                    \
> > +    dev_info(idev->dev.parent, fmt "\n", ##__VA_ARGS__)
> Agree with Krzysztof - should never wrap these up.
>
> > +
> > +struct sdp500_data {
> > +    struct device *dev;
> > +};
> > +
> > +uint8_t calculate_crc8(uint8_t *data, uint32_t len, uint8_t poly)
> > +{
> > +    uint8_t    count = 0;
> > +    uint8_t    value = 0;
> > +    uint8_t    temp = 0;
> > +
> > +    while (len--) {
> > +        temp = *(data);
> > +        data++;
> > +        value ^= temp;
> > +        for (count = 0; count < BITS_PER_BYTE; count++) {
> > +            if (value & 0x80)
> > +                value = (value << 1) ^ poly;
> > +            else
> > +                value = value << 1;
> > +        }
> > +    }
> As pointed out in other review - should be no need to reinvent the wheel.
> > +
> > +    return value;
> > +}
> > +
> > +static int sdp500_xfer(struct sdp500_data *data, u8 *txbuf, size_t txsize,
> > +              u8 *rxbuf, size_t rxsize, const struct iio_dev *indio_dev)
> > +{
> > +    struct i2c_client *client = to_i2c_client(data->dev);
> > +    int ret;
> > +
> > +    ret = i2c_master_send(client, txbuf, txsize);
> > +    if (ret < 0) {
> > +        sdp500_err(indio_dev, "Failed to send data");
> > +        return ret;
> > +    }
> > +    if (ret != txsize) {
> > +        sdp500_err(indio_dev, "Data is sent wrongly");
> > +        return -EIO;
> > +    }
> > +
> > +    if (!rxsize)
> > +        return 0;
> > +
> > +    ret = i2c_master_recv(client, rxbuf, rxsize);
> > +    if (ret < 0) {
> > +        sdp500_err(indio_dev, "Failed to receive data");
> > +        return ret;
> > +    }
> > +    if (ret != rxsize) {
> > +        sdp500_err(indio_dev, "Data is received wrongly");
> > +        return -EIO;
> > +    }
>
> This looks wrong from my reading of the datasheet and what
> the datasheet shows can be done with standard functions
> that handle these corner cases for you.
>
> > +
> > +    return 0;
> > +}
> > +
> > +static int sdp500_start_measurement(struct sdp500_data *data, const
> > struct iio_dev *indio_dev)
> > +{
> > +    u8 txbuf = SDP500_I2C_START_MEAS;
> > +
> > +    return sdp500_xfer(data, &txbuf, 1, NULL, 0, indio_dev);
>
> Just call i2c_master_send() here directly.
> However this looks like
> i2c_smbus_write_byte() ?
>
> > +}
> > +
> > +static const struct iio_chan_spec sdp500_channels[] = {
> > +    {
> > +        .type = IIO_PRESSURE,
> > +        .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>
> Looks like a simple linear scale.  Would be better to make scaling
> a userspace / consumer problem and provide IIO_CHAN_INFO_RAW
> and IIO_CHAN_INFO_SCALE.

I prefer returning the pressure directly because there is no other calculation
that the user of this driver can do. If they make the calculation differently
then their pressure value would be wrong.

>
> > +    },
> > +};
> > +
> > +static int sdp500_read_raw(struct iio_dev *indio_dev,
> > +              struct iio_chan_spec const *chan,
> > +              int *val, int *val2, long mask)
> > +{
> > +    int ret = -EINVAL;
>
> Rarely a good idea. Better to return this where it is
> clear why this value makes sense.
> > +    u8 rxbuf[SDP500_READ_SIZE];
> > +    u8 rec_crc, calculated_crc;
> > +    s16 dec_value;
> > +    struct sdp500_data *data = iio_priv(indio_dev);
> > +
> > +    switch (mask) {
> > +    case IIO_CHAN_INFO_PROCESSED:
> > +        sdp500_xfer(data, NULL, 0, rxbuf, SDP500_READ_SIZE, indio_dev);
>
> A zero size send is unusual. I'm not really seeing how it lines
> up with the datasheet either.
> The sequence shown there looks more like an
> i2c_smbus_read_i2c_block_data() call as it shows it happening
> as one transaction (figure in 4.1 doesn't have a NA after the
> command is sent).
> https://sensirion.com/media/documents/63859DD0/6166CF0E/Sensirion_Differential_Pressure_Datasheet_SDP600Series.pdf
> (also note that this appears to say the sdp600 is
> covered as well)
>
>
>
> > +        rec_crc = rxbuf[2];
> > +        calculated_crc = calculate_crc8(rxbuf, SDP500_READ_SIZE - 1,
> > +                        SDP500_CRC8_POLYNOMIAL);
> > +        if (rec_crc != calculated_crc) {
> > +            sdp500_err(indio_dev, "calculated crc = 0x%.2X but
> > received 0x%.2X",
> > +                calculated_crc, rec_crc);
> > +            return -EIO;
> > +        }
> > +
> > +        dec_value = ((rxbuf[0] << 8) & 0xFF00) | rxbuf[1];
>
> Look like a get_unaligned_be16() call opencoded - use that instead
> of this.
>
>
> > +        sdp500_dbg(indio_dev, "dec value = %d", dec_value);
> > +
> > +        *val = dec_value;
> > +        *val2 = SDP500_SCALE_FACTOR;
> > +        ret = IIO_VAL_FRACTIONAL;
> > +        break;
>                 return IIO_VAL_FRACTIONAL;
>         default:
>                 return -EINVAL;
> and drop the return that follows.
>
>
> > +    }
> > +    return ret;
> > +}
> > +
> > +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;
> > +
> > +    indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +    if (!indio_dev) {
> > +        dev_err(dev->parent, "Failed to allocate iio device\n");
> > +        return -ENOMEM;
> > +    }
> > +
> > +    i2c_set_clientdata(client, indio_dev);
>
> Only if you need it, which I suspect you don't once you've dropped
> remove as suggested below.
>
> > +
> > +    data = iio_priv(indio_dev);
> > +    data->dev = dev;
> When supporting only one bus type, we tend to use i2c_client instead
> to get access to that and the dev.
>
> > +
> > +    indio_dev->dev.parent = dev;
>
> The IIO core does that for you so no need to duplicate.
>
> > +    indio_dev->name = client->name;
> Hard code the name.  What exactly goes in client->name
> isn't obvious for all types of firmware etc.
> This just wants to be the part number so "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) {
> > +        sdp500_err(indio_dev, "Failed to start measurement");
> > +        return ret;
> > +    }
> You will want to read back one result here as datasheet notes
> first one is garbage and we want to make sure we ate it!
>
> > +
> > +    ret = iio_device_register(indio_dev);
> > +    if (ret < 0) {
> > +        sdp500_err(indio_dev, "Failed to register indio_dev");
>
> spaces rather than tabs in here it seems.
> Run checkpatch.pl

I did that before sending the patches and there were no issues.
I still have the patch files and there are no spaces. Who knows what happened.

>
> Also,
>                 return dev_error_probe() for any error messages
> in probe or functions only called from probe.
>
>
> > +        return ret;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static const struct i2c_device_id sdp500_id[] = {
> > +    { "sdp500" },
> > +    { },
> No comma after 'terminating' entries like this.
>
> > +};
> > +MODULE_DEVICE_TABLE(i2c, sdp500_id);
> > +
> > +static void sdp500_remove(struct i2c_client *client)
> > +{
> > +    struct iio_dev *indio_dev = dev_get_drvdata(&client->dev);
> > +
> > +    iio_device_unregister(indio_dev);
>
> As Krysztof pointed out, devm version of register will mean you don't
> need this.
>
> J

Sorry for the slow reply, I finally found some time to continue this task.
Thank you for the clear review. I didn't know many of the existing functions
that you suggested. I updated the driver with them and it looks nicer now.

Powered by blists - more mailing lists