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

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

On Tue, 16 Jan 2024 16:24:56 +0100
Petar Stoykov <pd.pstoykov@gmail.com> wrote:

> 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,

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.

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

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