From 6ae7537517f551540121ca6fb3b99080b7580410 Mon Sep 17 00:00:00 2001
From: Petar Stoykov <[email protected]>
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 <[email protected]>
---
drivers/iio/pressure/Kconfig | 9 +++
drivers/iio/pressure/Makefile | 1 +
drivers/iio/pressure/sdp500.c | 144 ++++++++++++++++++++++++++++++++++
3 files changed, 154 insertions(+)
create mode 100644 drivers/iio/pressure/sdp500.c
diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index 95efa32e4289..5debdfbd5324 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -212,6 +212,15 @@ config MS5637
This driver can also be built as a module. If so, the module will
be called ms5637.
+config SDP500
+ tristate "Sensirion SDP500 differential pressure sensor I2C driver"
+ depends on I2C
+ help
+ Say Y here to build support for Sensirion SDP500 differential pressure
+ sensor I2C driver.
+ To compile this driver as a module, choose M here: the core module
+ will be called sdp500.
+
config IIO_ST_PRESS
tristate "STMicroelectronics pressure sensor Driver"
depends on (I2C || SPI_MASTER) && SYSFS
diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
index 436aec7e65f3..489ef7b7befa 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_MS5611) += ms5611_core.o
obj-$(CONFIG_MS5611_I2C) += ms5611_i2c.o
obj-$(CONFIG_MS5611_SPI) += ms5611_spi.o
obj-$(CONFIG_MS5637) += ms5637.o
+obj-$(CONFIG_SDP500) += sdp500.o
obj-$(CONFIG_IIO_ST_PRESS) += st_pressure.o
st_pressure-y := st_pressure_core.o
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
+#define SDP500_CRC8_INIT 0x00
+
+#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);
+}
+
+static const struct iio_chan_spec sdp500_channels[] = {
+ {
+ .type = IIO_PRESSURE,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ },
+};
+
+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];
+ 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");
+ return -EIO;
+ }
+
+ rec_crc = rxbuf[2];
+ calculated_crc = crc8(sdp500_crc8_table, rxbuf,
SDP500_CRC8_WORD_LENGTH,
+ 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);
+
+ *val = dec_value;
+ *val2 = SDP500_SCALE_FACTOR;
+ 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");
+ /* 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");
+
+ 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,
+};
+module_i2c_driver(sdp500_driver);
+
+MODULE_AUTHOR("Thomas Sioutas <[email protected]>");
+MODULE_DESCRIPTION("Driver for Sensirion SDP500 differential pressure sensor");
+MODULE_LICENSE("GPL");
--
2.30.2
On Tue, Apr 30, 2024 at 05:27:24PM +0200, Petar Stoykov wrote:
> From 6ae7537517f551540121ca6fb3b99080b7580410 Mon Sep 17 00:00:00 2001
> From: Petar Stoykov <[email protected]>
> 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.
Any Datasheet: tag can be added?
..
> +config SDP500
> + tristate "Sensirion SDP500 differential pressure sensor I2C driver"
> + depends on I2C
> + help
> + Say Y here to build support for Sensirion SDP500 differential pressure
> + sensor I2C driver.
> + To compile this driver as a module, choose M here: the core module
> + will be called sdp500.
You patch is broken. Fix the way how you send patches.
..
> +static int sdp500_start_measurement(struct sdp500_data *data, const
> struct iio_dev *indio_dev)
Here is more visible.
--
With Best Regards,
Andy Shevchenko
On 30/04/2024 17:27, Petar Stoykov wrote:
> From 6ae7537517f551540121ca6fb3b99080b7580410 Mon Sep 17 00:00:00 2001
> From: Petar Stoykov <[email protected]>
> 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 <[email protected]>
> ---
> drivers/iio/pressure/Kconfig | 9 +++
> drivers/iio/pressure/Makefile | 1 +
> drivers/iio/pressure/sdp500.c | 144 ++++++++++++++++++++++++++++++++++
> 3 files changed, 154 insertions(+)
> create mode 100644 drivers/iio/pressure/sdp500.c
>
> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> index 95efa32e4289..5debdfbd5324 100644
> --- a/drivers/iio/pressure/Kconfig
> +++ b/drivers/iio/pressure/Kconfig
> @@ -212,6 +212,15 @@ config MS5637
> This driver can also be built as a module. If so, the module will
> be called ms5637.
>
> +config SDP500
> + tristate "Sensirion SDP500 differential pressure sensor I2C driver"
Nothing improved. Indentation is even more broken.
Use b4 to send patches. If you decide to send them manually, be sure you
use proper process, so git format-patch -3 --cover-letter && git
send-email. All threading is broken, all patches are not real patches
and have some headers pasted. That's not how it works.
Best regards,
Krzysztof
On Tue, Apr 30, 2024 at 5:40 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Tue, Apr 30, 2024 at 05:27:24PM +0200, Petar Stoykov wrote:
> > From 6ae7537517f551540121ca6fb3b99080b7580410 Mon Sep 17 00:00:00 2001
> > From: Petar Stoykov <[email protected]>
> > 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.
>
> Any Datasheet: tag can be added?
>
Ok. I see some drivers also include the pdf link in the driver's code.
I can do that as well.
> ...
>
> > +config SDP500
> > + tristate "Sensirion SDP500 differential pressure sensor I2C driver"
> > + depends on I2C
> > + help
> > + Say Y here to build support for Sensirion SDP500 differential pressure
> > + sensor I2C driver.
> > + To compile this driver as a module, choose M here: the core module
> > + will be called sdp500.
>
> You patch is broken. Fix the way how you send patches.
>
> ...
>
> > +static int sdp500_start_measurement(struct sdp500_data *data, const
> > struct iio_dev *indio_dev)
>
> Here is more visible.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
I finally figured it out. Gmail has a hard word-wrap at 80 characters per line.
At first I thought it was word-wrap on the receiving side but I was wrong.
I will try to convince IT to change things so I can use b4 or git send e-mail.
If that doesn't work then I guess my code will have shorter lines in next patch.
On Wed, May 01, 2024 at 01:48:24PM GMT, Petar Stoykov wrote:
> I finally figured it out. Gmail has a hard word-wrap at 80 characters
> per line. At first I thought it was word-wrap on the receiving side
> but I was wrong. I will try to convince IT to change things so I can
> use b4 or git send e-mail. If that doesn't work then I guess my code
> will have shorter lines in next patch.
With b4, you can send via the web submission endpoint:
https://b4.docs.kernel.org/en/stable-0.13.y/contributor/send.html
-K
On Tue, 30 Apr 2024 17:27:24 +0200
Petar Stoykov <[email protected]> wrote:
> From 6ae7537517f551540121ca6fb3b99080b7580410 Mon Sep 17 00:00:00 2001
> From: Petar Stoykov <[email protected]>
> 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 <[email protected]>
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 <[email protected]>");
> +MODULE_DESCRIPTION("Driver for Sensirion SDP500 differential pressure sensor");
> +MODULE_LICENSE("GPL");
On Sun, May 5, 2024 at 7:18 PM Jonathan Cameron <[email protected]> wrote:
>
> On Tue, 30 Apr 2024 17:27:24 +0200
> Petar Stoykov <[email protected]> wrote:
>
> > From 6ae7537517f551540121ca6fb3b99080b7580410 Mon Sep 17 00:00:00 2001
> > From: Petar Stoykov <[email protected]>
> > 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 <[email protected]>
> 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 <[email protected]>");
> > +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!