| Message ID | CADFWO8EQUkGcbE=RXjxXbub2tZge9+ss=gB-Q6wngFAvwFygRg@mail.gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for Sensirion SDP500 | expand |
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 <pd.pstoykov@gmail.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. 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.
On 30/04/2024 17:27, Petar Stoykov wrote: > From 6ae7537517f551540121ca6fb3b99080b7580410 Mon Sep 17 00:00:00 2001 > From: Petar Stoykov <pd.pstoykov@gmail.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@gmail.com> > --- > 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 <andriy.shevchenko@linux.intel.com> 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 <pd.pstoykov@gmail.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. > > 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 <pd.pstoykov@gmail.com> wrote: > From 6ae7537517f551540121ca6fb3b99080b7580410 Mon Sep 17 00:00:00 2001 > From: Petar Stoykov <pd.pstoykov@gmail.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@gmail.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@prodrive-technologies.com>"); > +MODULE_DESCRIPTION("Driver for Sensirion SDP500 differential pressure sensor"); > +MODULE_LICENSE("GPL");
On Sun, May 5, 2024 at 7:18 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Tue, 30 Apr 2024 17:27:24 +0200 > Petar Stoykov <pd.pstoykov@gmail.com> wrote: > > > From 6ae7537517f551540121ca6fb3b99080b7580410 Mon Sep 17 00:00:00 2001 > > From: Petar Stoykov <pd.pstoykov@gmail.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@gmail.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@prodrive-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!
On Mon, 10 Jun 2024 10:58:35 +0200 Petar Stoykov <pd.pstoykov@gmail.com> wrote: > On Sun, May 5, 2024 at 7:18 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Tue, 30 Apr 2024 17:27:24 +0200 > > Petar Stoykov <pd.pstoykov@gmail.com> wrote: > > > > > From 6ae7537517f551540121ca6fb3b99080b7580410 Mon Sep 17 00:00:00 2001 > > > From: Petar Stoykov <pd.pstoykov@gmail.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@gmail.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. Normally I agree with the magic number case, but this is an actual value. We are saying continue the CRC from 0 (i.e. nothing). It's kind of the logical default value so seeing it in line makes it clear we aren't continuing form a prior crc etc. ... > > > > > + }, > > > +}; > > > + > > > +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. The disadvantage is you loose the readability a structure brings, but meh, I don't care that much. > > > > + 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. That's obscure and another reason I'd rather see a structure so this becomes sizeof(a.data) > The rec_crc is again for readability, like the SDP500_CRC8_INIT define. > I will change it to "received_crc" which is clearer though. The fact you compare it with the crc makes that pretty obvious, but fair enough I guess if you think it helps. > > > > > > + 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); > > > > > > +}; > > > +module_i2c_driver(sdp500_driver); > > > + > > > +MODULE_AUTHOR("Thomas Sioutas <thomas.sioutas@prodrive-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! > Good luck! (it should be fine but I've never tried it :) Jonathan
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); +