On Mon, 22 Jul 2024 17:58:43 +0100
Joshua Felmeden <jfelmeden@xxxxxxxxxxxxxxxxxxxx> wrote:
> Add support for ENS210/ENS210A/ENS211/ENS212/ENS213A/ENS215.
>
> The ENS21x is a family of temperature and relative humidity sensors with
> accuracies tailored to the needs of specific applications.
>
> Signed-off-by: Joshua Felmeden <jfelmeden@xxxxxxxxxxxxxxxxxxxx>
Hi Joshua,
I nearly took this with a few tweaks, but the address / casting issue for the
*_get_measurement looks to me like a more significant issue and the fix will need testing.
Various other minor bits and pieces inline.
Thanks,
Jonathan
> diff --git a/drivers/iio/humidity/ens210.c b/drivers/iio/humidity/ens210.c
> new file mode 100644
> index 000000000000..9fe60a9eeef0
> --- /dev/null
> +++ b/drivers/iio/humidity/ens210.c
> @@ -0,0 +1,344 @@
...
> +
> +/**
> + * struct ens210_data - Humidity/Temperature sensor device structure
> + * @client: i2c client
> + * @chip_info: chip specific information
> + * @lock: lock protecting the i2c conversion
That's not enough detail. I2C has it's own locks so we don't need to protect the
bus. I assume this is really about protecting a particular sequence of multiple
accesses?
> + */
> +struct ens210_data {
> + struct i2c_client *client;
> + const struct ens210_chip_info *chip_info;
> + struct mutex lock;
> +};
> +
> +/* calculate 17-bit crc7 */
> +static u8 ens210_crc7(u32 val)
> +{
> + unsigned int val_be = (val & 0x1ffff) >> 0x8;
> +
> + return crc7_be(0xde, (u8 *)&val_be, 3) >> 1;
> +}
> +
> +static int ens210_get_measurement(struct iio_dev *indio_dev, bool temp, int *val)
> +{
> + struct ens210_data *data = iio_priv(indio_dev);
> + struct device *dev = &data->client->dev;
> + u32 regval;
> + u8 regval_le[3];
> + int ret;
> +
> + /* assert read */
> + ret = i2c_smbus_write_byte_data(data->client, ENS210_REG_SENS_START,
> + temp ? ENS210_SENS_START_T_START :
> + ENS210_SENS_START_H_START);
> + if (ret)
> + return ret;
> +
> + /* wait for conversion to be ready */
> + msleep(data->chip_info->conv_time_msec);
> +
> + ret = i2c_smbus_read_byte_data(data->client,
> + ENS210_REG_SENS_STAT);
Under 80 chars on one line.
> + if (ret < 0)
> + return ret;
> +
> + /* perform read */
> + ret = i2c_smbus_read_i2c_block_data(
> + data->client, temp ? ENS210_REG_T_VAL : ENS210_REG_H_VAL, 3,
> + (u8 *)®val_le);
This cast looks wrong and the need to have it indicates
a bigger issue. regval_le is already u8 * so it's address is u8 ** and you
will be writing to the completely the wrong place.
> + if (ret < 0) {
> + dev_err(dev, "failed to read register");
> + return -EIO;
you've returned so no need for an else for the next check.
if (ret != 3)
> + } else if (ret != 3) {
> + dev_err(dev, "expected 3 bytes, received %d\n", ret);
> + return -EIO;
> + }
> +
> + regval = get_unaligned_le24(regval_le);
> + if (ens210_crc7(regval) != ((regval >> 17) & 0x7f)) {
> + /* crc fail */
Comment very obvious. Drop it as such comments are just an opportunity for
becoming wrong in future as they bring no real value.
> + dev_err(dev, "ens invalid crc\n");
> + return -EIO;
> + }
> +
> + if (!((regval >> 16) & 0x1)) {
> + dev_err(dev, "ens data is not valid");
> + return -EIO;
> + }
> +
> + *val = regval & 0xffff;
*val = regval & GENMASK(15, 0);
is, I think, slightly more readable.
> + return IIO_VAL_INT;
> +}
> +
> +static int ens210_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel, int *val,
> + int *val2, long mask)
> +{
> + struct ens210_data *data = iio_priv(indio_dev);
> + int ret = -EINVAL;
If we hit this value something went wrong.. Indeed see missing unreachable
below. If that is confusing the compiler add a comment to say so.
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + scoped_guard(mutex, &data->lock) {
> + ret = ens210_get_measurement(indio_dev,
> + channel->type == IIO_TEMP, val);
> + if (ret)
> + return ret;
> + return IIO_VAL_INT;
> + }
> + return ret;
unreachable()
> + case IIO_CHAN_INFO_SCALE:
> + if (channel->type == IIO_TEMP) {
> + *val = 15;
> + *val2 = 625000;
> + } else {
> + *val = 1;
> + *val2 = 953125;
> + }
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_OFFSET:
> + if (channel->type == IIO_TEMP) {
> + *val = -17481;
> + *val2 = 600000;
> + ret = IIO_VAL_INT_PLUS_MICRO;
> + break;
return IIO_VAL_INT_PLUS_MICRO;
not break to an unreachable()!
> + }
> + *val = 0;
The default offset is 0 so don't report one for the humidity channel.
Just drop bit being set in info_mask_separate below.
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + unreachable();
> +}
> +
> +static const struct iio_chan_spec ens210_channels[] = {
> + {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_OFFSET),
> + },
> + {
> + .type = IIO_HUMIDITYRELATIVE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_OFFSET),
As above, drop the offset bit.
> + }
> +};
> +
> +static const struct iio_info ens210_info = {
> + .read_raw = ens210_read_raw,
> +};
> +
> +static int ens210_probe(struct i2c_client *client)
> +{
> + struct ens210_data *data;
> + struct iio_dev *indio_dev;
> + uint16_t part_id;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> + I2C_FUNC_SMBUS_WRITE_BYTE |
> + I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> + return dev_err_probe(&client->dev, -EOPNOTSUPP,
> + "adapter does not support some i2c transactions\n");
> + }
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->client = client;
> + mutex_init(&data->lock);
> + data->chip_info = i2c_get_match_data(client);
> +
> + ret = devm_regulator_get_enable(&client->dev, "vdd");
> + if (ret)
> + return ret;
> +
> + /* reset device */
> + ret = i2c_smbus_write_byte_data(client, ENS210_REG_SYS_CTRL,
> + ENS210_SYS_CTRL_SYS_RESET);
> + if (ret)
> + return ret;
> +
> + /* wait for device to become active */
> + usleep_range(4000, 5000);
> +
> + /* disable low power mode */
> + ret = i2c_smbus_write_byte_data(client, ENS210_REG_SYS_CTRL, 0x00);
> + if (ret)
> + return ret;
> +
> + /* wait for device to finish */
> + usleep_range(4000, 5000);
> +
> + /* get part_id */
> + part_id = i2c_smbus_read_word_data(client, ENS210_REG_PART_ID);
This may return an error. So should check negative value separately
as if that happens we don't want to carry on.
ret = i2c_...
if (ret < 0)
return ret;
part_id = ret;
...
> +
> + if (part_id != data->chip_info->part_id) {
> + dev_info(&client->dev,
> + "Part ID does not match (0x%04x != 0x%04x)\n", part_id,
> + data->chip_info->part_id);
> + }
> +
> + /* reenable low power */
> + ret = i2c_smbus_write_byte_data(client, ENS210_REG_SYS_CTRL,
> + ENS210_SYS_CTRL_LOW_POWER_ENABLE);
> + if (ret)
> + return ret;
> +
> + indio_dev->name = data->chip_info->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = ens210_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ens210_channels);
> + indio_dev->info = &ens210_info;
> +
> + return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +static const struct of_device_id ens210_of_match[] = {
> + { .compatible = "sciosense,ens210", .data = &ens210_chip_info_data},
> + { .compatible = "sciosense,ens210a", .data = &ens210a_chip_info_data },
> + { .compatible = "sciosense,ens211", .data = &ens211_chip_info_data},
> + { .compatible = "sciosense,ens212", .data = &ens212_chip_info_data},
Trivial: should always be a space before }
> + { .compatible = "sciosense,ens213a", .data = &ens213a_chip_info_data },
> + { .compatible = "sciosense,ens215", .data = &ens215_chip_info_data },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ens210_of_match);
> +
> +static const struct i2c_device_id ens210_id_table[] = {
> + { "ens210", (kernel_ulong_t)&ens210_chip_info_data },
> + { "ens210a", (kernel_ulong_t)&ens210a_chip_info_data },
> + { "ens211", (kernel_ulong_t)&ens211_chip_info_data },
> + { "ens212", (kernel_ulong_t)&ens212_chip_info_data },
> + { "ens213a", (kernel_ulong_t)&ens213a_chip_info_data },
> + { "ens215", (kernel_ulong_t)&ens215_chip_info_data },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ens210_id_table);
> +
> +static struct i2c_driver ens210_driver = {
> + .probe = ens210_probe,
> + .id_table = ens210_id_table,
> + .driver = {
> + .name = "ens210",
> + .of_match_table = ens210_of_match,
> + },
> +};
Trivial: Given close coupling between module_i2c_driver and the i2c_driver
structure it's common to not have a blank line here.
> +
> +module_i2c_driver(ens210_driver);
> +
> +MODULE_DESCRIPTION("ScioSense ENS210 temperature and humidity sensor driver");
> +MODULE_AUTHOR("Joshua Felmeden <jfelmeden@xxxxxxxxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
>
