Message-ID: <fd8a8f09-7661-42c2-ace6-0a0e4845184d@linaro.org>
Date: Tue, 16 Jan 2024 16:30:51 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Petar Stoykov <pd.pstoykov@...il.com>, linux-iio@...r.kernel.org
Cc: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen
 <lars@...afoo.de>, Rob Herring <robh+dt@...nel.org>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Angel Iglesias <ang.iglesiasg@...il.com>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH 2/3] iio: pressure: Add driver for Sensirion SDP500

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

Powered by blists - more mailing lists