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