Date: Sun, 5 May 2024 18:18:29 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Petar Stoykov <pd.pstoykov@...il.com>
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 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

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

> +
> +#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.

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

> +    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.

> +            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?

> +            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).

> +};
> +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");

Powered by blists - more mailing lists