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 | 201 ++++++++++++++++++++++++++++++++++
3 files changed, 211 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..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__)
+
+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;
+ }
+ }
+
+ 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;
+ }
+
+ 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);
+}
+
+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 = -EINVAL;
+ 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);
+ 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];
+ sdp500_dbg(indio_dev, "dec value = %d", dec_value);
+
+ *val = dec_value;
+ *val2 = SDP500_SCALE_FACTOR;
+ ret = IIO_VAL_FRACTIONAL;
+ break;
+ }
+ 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);
+
+ data = iio_priv(indio_dev);
+ data->dev = dev;
+
+ indio_dev->dev.parent = dev;
+ indio_dev->name = client->name;
+ 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;
+ }
+
+ ret = iio_device_register(indio_dev);
+ if (ret < 0) {
+ sdp500_err(indio_dev, "Failed to register indio_dev");
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct i2c_device_id sdp500_id[] = {
+ { "sdp500" },
+ { },
+};
+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);
+}
+
+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,
+ .remove = sdp500_remove,
+ .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 16/01/2024 16:24, Petar Stoykov wrote:
> Sensirion SDP500 is a digital differential pressure sensor. The sensor is
> accessed over I2C.
>
> 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..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__)
Nope, drop
> +
> +#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__)
Drop all three.
> +
> +struct sdp500_data {
> + struct device *dev;
> +};
> +
> +uint8_t calculate_crc8(uint8_t *data, uint32_t len, uint8_t poly)
Why this is not static?
> +{
> + uint8_t count = 0;
> + uint8_t value = 0;
> + uint8_t temp = 0;
Weird indentation.
You should not implement your own CRC functions. Don't we have CRC8 in
the kernel?
> +
> + 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;
> + }
> + }
> +
> + 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;
> + }
> +
> + return 0;
> +}
> +
> +static int sdp500_start_measurement(struct sdp500_data *data, const
> struct iio_dev *indio_dev)
Your patchset is corrupted.
> +{
> + u8 txbuf = SDP500_I2C_START_MEAS;
> +
> + return sdp500_xfer(data, &txbuf, 1, NULL, 0, indio_dev);
> +}
> +
> +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 = -EINVAL;
> + 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);
> + 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);
Your patchset is corrupted.
> + return -EIO;
> + }
> +
> + dec_value = ((rxbuf[0] << 8) & 0xFF00) | rxbuf[1];
> + sdp500_dbg(indio_dev, "dec value = %d", dec_value);
> +
> + *val = dec_value;
> + *val2 = SDP500_SCALE_FACTOR;
> + ret = IIO_VAL_FRACTIONAL;
> + break;
> + }
> + 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");
No printing on ENOMEM. Run coccinelle/coccicheck on your patchset. There
is no code in kernel doing this, so please take existing code as base
for your driver, instead of upstreaming ancient out-of-tree poor code.
> + return -ENOMEM;
> + }
> +
> + i2c_set_clientdata(client, indio_dev);
> +
> + data = iio_priv(indio_dev);
> + data->dev = dev;
> +
> + indio_dev->dev.parent = dev;
> + indio_dev->name = client->name;
> + 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");
You must use dev_err, not own print methods.
> + return ret;
> + }
> +
> + ret = iio_device_register(indio_dev);
Why not devm?
> + if (ret < 0) {
> + sdp500_err(indio_dev, "Failed to register indio_dev");
dev_err
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id sdp500_id[] = {
> + { "sdp500" },
> + { },
> +};
> +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);
> +}
> +
> +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,
> + .remove = sdp500_remove,
Some random wrapping here..
Best regards,
Krzysztof
On 16/01/2024 16:30, Krzysztof Kozlowski wrote:
>> +
>> +static struct i2c_driver sdp500_driver = {
>> + .driver = {
>> + .name = "sensirion,sdp500",
>> + .of_match_table = sdp500_of_match,
>> + },
>> + .probe = sdp500_probe,
>> + .remove = sdp500_remove,
>
> Some random wrapping here..
I meant: indentation before =.
Best regards,
Krzysztof
On Tue, Jan 16, 2024 at 6:03 PM Jonathan Cameron
<[email protected]> wrote:
>
> On Tue, 16 Jan 2024 16:24:56 +0100
> Petar Stoykov <[email protected]> wrote:
>
> > Sensirion SDP500 is a digital differential pressure sensor. The sensor is
> > accessed over I2C.
> >
> > Signed-off-by: Petar Stoykov <[email protected]>
> 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.
> > > +}
> > > +
> > > +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.
Ah. I missed this and just made the same comment on v2.
Let me give some more info than in the original review.
The documentation on how to apply scale is simple and this scaling is
pretty hard to get wrong.
There are a couple of reasons we prefer to make it a userspace problem
to do linear scaling and keep the actual channel value raw (if possible).
1) Logging applications typically store the scale once, and each data
point is then much cheaper to store as a u16 than as a floating point
number.
2) If you ever add buffered support, we do not support floating point
values in the buffer. That basically means we have to have both
_PROCESSED and _RAW provided so that _SCALE makes sense for the buffer.
Horribly messy ABI is the result.
Hence, push the scaling to userspace.
Note that we can't always do this as some conversion functions are
non linear and very hard to describe. In those cases _PROCESSED makes
sense. That's not true here.
Jonathan