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
