Hi Jonathan, Le dim. 14 nov. 2021 à 17:44, Jonathan Cameron <jic23@xxxxxxxxxx> a écrit : > > On Sun, 14 Nov 2021 15:58:15 +0100 > Gilles Talis <gilles.talis@xxxxxxxxx> wrote: > > > Hi Lars, > > > > Le dim. 14 nov. 2021 à 14:47, Lars-Peter Clausen <lars@xxxxxxxxxx> a écrit : > > > > > On 11/14/21 2:23 PM, Gilles Talis wrote: > > > > The SHTC3 is a digital humidity and temperature sensor. > > > > It covers humidity measurement range from 0 to 100% relative humidity > > > > and temperature measurement range from -45 to 125 deg C. > > > > > > > > Datasheet: https://www.sensirion.com/file/datasheet_shtc3 > > > > > > > > Signed-off-by: Gilles Talis <gilles.talis@xxxxxxxxx> > > > > > > Hi, > > > > > > Thanks for the path. This looks really good, very well written driver. > > > > > > But we already have support for the shtc3 in the Linux kernel as part of > > > the hwmon framework, see drivers/hwmon/shtc1.c. > > > > > > - Lars > > > > > Thanks for the review. Oops, I should have been more careful. Next time, I > > will try to be. > > Sorry for the spamming. > > The fun question of whether humidity drivers should be in IIO or HWMON was my > fault many years ago. Pre IIO being anywhere near ready for mainline (and mostly > a concept rather than code) I wanted to get at least one of the drivers I was > working with upstream and the characteristics of humidity drivers were rather > different from ADCs and Accelerometers etc so I asked if Humidty counted as > hardware monitoring and got something like "sure it could be" as a reply so > upstreamed one driver in hwmon (sht15). > > Ever since it's been a bit random on where humidity drivers end up based on > who is submitting them and their usecase + most similar drivers already upstream. > > Sorry you fell into this historical quirk! No problem. Thanks for taking the time to explain the history. Next time, I will make sure I also look in other subsystems before submitting such a driver. > > I took a quick look and agree with Lars: nice clean driver, but as you can guess > we don't really want 2 drivers and there isn't a strong reason to propose > moving this one between subsystems. This obviously makes no sense having 2 drivers for this device. Thanks for the review and the kind words. > > Thanks, > > Jonathan Thanks, Gilles. > > > > > Thanks! > > Gilles. > > > > > > > > > > > > > --- > > > > drivers/iio/humidity/Kconfig | 11 ++ > > > > drivers/iio/humidity/Makefile | 1 + > > > > drivers/iio/humidity/shtc3.c | 286 ++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 298 insertions(+) > > > > create mode 100644 drivers/iio/humidity/shtc3.c > > > > > > > > diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig > > > > index 2de5494e7c22..7aab6141c64b 100644 > > > > --- a/drivers/iio/humidity/Kconfig > > > > +++ b/drivers/iio/humidity/Kconfig > > > > @@ -98,6 +98,17 @@ config HTU21 > > > > This driver can also be built as a module. If so, the module will > > > > be called htu21. > > > > > > > > +config SHTC3 > > > > + tristate "Sensirion SHTC3 humidity and temperature sensor" > > > > + depends on I2C > > > > + select CRC8 > > > > + help > > > > + Say yes here to build support for the Sensirion SHTC3 > > > > + humidity and temperature sensor. > > > > + > > > > + To compile this driver as a module, choose M here: the module > > > > + will be called shtc3. > > > > + > > > > config SI7005 > > > > tristate "SI7005 relative humidity and temperature sensor" > > > > depends on I2C > > > > diff --git a/drivers/iio/humidity/Makefile > > > b/drivers/iio/humidity/Makefile > > > > index f19ff3de97c5..13020dfad1b3 100644 > > > > --- a/drivers/iio/humidity/Makefile > > > > +++ b/drivers/iio/humidity/Makefile > > > > @@ -16,6 +16,7 @@ obj-$(CONFIG_HTS221_I2C) += hts221_i2c.o > > > > obj-$(CONFIG_HTS221_SPI) += hts221_spi.o > > > > > > > > obj-$(CONFIG_HTU21) += htu21.o > > > > +obj-$(CONFIG_SHTC3) += shtc3.o > > > > obj-$(CONFIG_SI7005) += si7005.o > > > > obj-$(CONFIG_SI7020) += si7020.o > > > > > > > > diff --git a/drivers/iio/humidity/shtc3.c b/drivers/iio/humidity/shtc3.c > > > > new file mode 100644 > > > > index 000000000000..ec3d7215e378 > > > > --- /dev/null > > > > +++ b/drivers/iio/humidity/shtc3.c > > > > @@ -0,0 +1,286 @@ > > > > +// SPDX-License-Identifier: GPL-2.0-only > > > > +/* > > > > + * Sensirion SHTC3 Humidity and Temperature Sensor > > > > + * > > > > + * Copyright (c) 2021 Gilles Talis <gilles.talis@xxxxxxxxx> > > > > + * > > > > + * Datasheet: https://www.sensirion.com/file/datasheet_shtc3 > > > > + * > > > > + * I2C slave address: 0x70 > > > > + */ > > > > + > > > > +#include <linux/crc8.h> > > > > +#include <linux/delay.h> > > > > +#include <linux/i2c.h> > > > > +#include <linux/module.h> > > > > +#include <linux/mod_devicetable.h> > > > > + > > > > +#include <linux/iio/iio.h> > > > > + > > > > +#define SHTC3_CMD(cmd_word) cpu_to_be16(cmd_word) > > > > +#define SHTC3_CMD_LEN 2 > > > > + > > > > +#define SHTC3_ID_MASK 0x083F > > > > +#define SHTC3_ID 0x0807 > > > > + > > > > +#define SHTC3_CRC8_POLYNOMIAL 0x31 > > > > + > > > > +enum shtc3_cmd { > > > > + SHTC3_CMD_GET_ID = SHTC3_CMD(0xEFC8), > > > > + SHTC3_CMD_SOFT_RESET = SHTC3_CMD(0x805D), > > > > + SHTC3_CMD_SLEEP = SHTC3_CMD(0xB098), > > > > + SHTC3_CMD_WAKEUP = SHTC3_CMD(0x3517), > > > > + /* > > > > + * Run measurement, low-power mode, clock stretching > > > > + * temperature first > > > > + */ > > > > + SHTC3_CMD_TEMP_MEAS_LP_CS = SHTC3_CMD(0x6458), > > > > + /* > > > > + * Run measurement, low-power mode, clock stretching > > > > + * relative humidity first > > > > + */ > > > > + SHTC3_CMD_RH_MEAS_LP_CS = SHTC3_CMD(0x44DE), > > > > +}; > > > > + > > > > +DECLARE_CRC8_TABLE(shtc3_crc8_tbl); > > > > + > > > > +struct shtc3_rx_data { > > > > + __be16 data; > > > > + u8 crc; > > > > +} __packed; > > > > + > > > > +static int shtc3_send_cmd(struct i2c_client *client, u16 cmd, u16 *data) > > > > +{ > > > > + int ret; > > > > + struct shtc3_rx_data rx_data; > > > > + u8 crc; > > > > + > > > > + ret = i2c_master_send(client, (const char *)&cmd, SHTC3_CMD_LEN); > > > > + if (ret != SHTC3_CMD_LEN) > > That might eat another error, so convention is to check ret < 0 first and then > check the length. That way you can return a more informative error if there > is one. I see you do that on the recv below. > > > > > + return -EIO; > > > > + > > > > + /* > > > > + * This is used to read temperature and humidity measurements > > > > + * as well as the sensor ID. > > > > + * Sensor sends 2 bytes of data followed by one byte of CRC > > > > + */ > > > > + if (data) { > > > > + ret = i2c_master_recv(client, (u8 *) &rx_data, > > > > + sizeof(struct shtc3_rx_data)); > > > > + if (ret < 0) > > > > + return ret; > > > > + if (ret != sizeof(struct shtc3_rx_data)) > > > > + return -EIO; > > > > + > > > > + crc = crc8(shtc3_crc8_tbl, (u8 *)&rx_data.data, > > > > + 2, CRC8_INIT_VALUE); > > > > + if (crc != rx_data.crc) > > > > + return -EIO; > > > > + > > > > + *data = be16_to_cpu(rx_data.data); > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int shtc3_sleep(struct i2c_client *client) > > > > +{ > > > > + return shtc3_send_cmd(client, SHTC3_CMD_SLEEP, 0); > > > > +} > > > > + > > > > +static int shtc3_wakeup(struct i2c_client *client) > > > > +{ > > > > + if (shtc3_send_cmd(client, SHTC3_CMD_WAKEUP, 0) < 0) > > > > + return -EIO; > > > > + > > > > + /* Wait for device to wake up */ > > > > + usleep_range(180, 240); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int shtc3_read_channel(struct i2c_client *client, bool temp) > > > > +{ > > > > + int ret; > > > > + u16 cmd; > > > > + u16 meas; > > > > + > > > > + ret = shtc3_wakeup(client); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + /* > > > > + * Sensor sends back measurement results after measurement command > > > > + * has been issued by the host. > > > > + * Sensor sends 3 bytes (2 bytes of data + 1 byte of CRC) for each > > > > + * channel sequentially > > > > + * The command issued by the host determines the channel for which > > > > + * the sensor will first send the data. > > > > + * We select the channel for which we need the results > > > > + * then only read back the 2 bytes corresponding to this channel. > > > > + */ > > > > + cmd = temp ? SHTC3_CMD_TEMP_MEAS_LP_CS : SHTC3_CMD_RH_MEAS_LP_CS; > > > > + ret = shtc3_send_cmd(client, cmd, &meas); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + /* Go back to sleep */ > > > > + shtc3_sleep(client); > > > > + > > > > + return meas; > > > > +} > > > > + > > > > +static int shtc3_read_raw(struct iio_dev *indio_dev, > > > > + struct iio_chan_spec const *chan, int *val, > > > > + int *val2, long mask) > > > > +{ > > > > + struct i2c_client **client = iio_priv(indio_dev); > > > > + int ret; > > > > + > > > > + switch (mask) { > > > > + case IIO_CHAN_INFO_RAW: > > > > + ret = shtc3_read_channel(*client, (chan->type == > > > IIO_TEMP)); > > > > + if (ret < 0) > > > > + return ret; > > > > + *val = ret; > > > > + return IIO_VAL_INT; > > > > + case IIO_CHAN_INFO_SCALE: > > > > + if (chan->type == IIO_TEMP) { > > > > + *val = 2; > > > > + *val2 = 670000; > > > > + } else { > > > > + *val = 0; > > > > + *val2 = 1525; > > > > + } > > > > + return IIO_VAL_INT_PLUS_MICRO; > > > > + case IIO_CHAN_INFO_OFFSET: > > > > + *val = -16852; > > > > + return IIO_VAL_INT; > > > > + default: > > > > + break; > > > > + } > > > > + > > > > + return -EINVAL; > > I'd move this into the default: statement as saves a few lines and makes it > slightly easier to read. > > > > > +} > > > > + > > > > +static const struct iio_chan_spec shtc3_channels[] = { > > > > + { > > > > + .type = IIO_HUMIDITYRELATIVE, > > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > > > + BIT(IIO_CHAN_INFO_SCALE), > > > > + }, > > > > + { > > > > + .type = IIO_TEMP, > > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > > > + BIT(IIO_CHAN_INFO_SCALE) | > > > BIT(IIO_CHAN_INFO_OFFSET), > > > > + } > > > > +}; > > > > + > > > > +static const struct iio_info shtc3_info = { > > > > + .read_raw = shtc3_read_raw, > > > > +}; > > > > + > > > > +static int shtc3_verify_id(struct i2c_client *client) > > > > +{ > > > > + int ret; > > > > + u16 device_id; > > > > + u16 reg_val; > > > > + > > > > + ret = shtc3_send_cmd(client, SHTC3_CMD_GET_ID, ®_val); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + device_id = reg_val & SHTC3_ID_MASK; > > > > + if (device_id != SHTC3_ID) > > > > + return -ENODEV; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int shtc3_reset(struct i2c_client *client) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = shtc3_send_cmd(client, SHTC3_CMD_SOFT_RESET, 0); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + /* Wait for device to enter idle state */ > > > > + usleep_range(180, 240); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int shtc3_setup(struct i2c_client *client) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = shtc3_verify_id(client); > > > > + if (ret < 0) { > > > > + dev_err(&client->dev, "SHTC3 not found\n"); > > > > + return -ENODEV; > > > > + } > > > > + > > > > + ret = shtc3_reset(client); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + return shtc3_sleep(client); > > > > +} > > > > + > > > > +static int shtc3_probe(struct i2c_client *client, > > > > + const struct i2c_device_id *id) > > > > +{ > > > > + struct iio_dev *indio_dev; > > > > + struct i2c_client **data; > > > > + int ret; > > > > + > > > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > > > > + if (!indio_dev) > > > > + return -ENOMEM; > > > > + > > > > + data = iio_priv(indio_dev); > > > > + *data = client; > > It's normally better to put a structure in there because the chances > of a later change requiring more data is rather high... > > Obviously for now it will only have one element so will end up > compiling to pretty much the same you have here. > > > > > + > > > > + indio_dev->name = dev_name(&client->dev); > > > > + indio_dev->modes = INDIO_DIRECT_MODE; > > > > + indio_dev->info = &shtc3_info; > > > > + indio_dev->channels = shtc3_channels; > > > > + indio_dev->num_channels = ARRAY_SIZE(shtc3_channels); > > > > + > > > > + crc8_populate_msb(shtc3_crc8_tbl, SHTC3_CRC8_POLYNOMIAL); > > > > + > > > > + ret = shtc3_setup(client); > > > > + if (ret < 0) { > > > > + dev_err(&client->dev, "SHTC3 setup failed\n"); > > > > + return ret; > > > > + } > > > > + > > > > + return devm_iio_device_register(&client->dev, indio_dev); > > > > +} > > > > + > > > > +static const struct i2c_device_id shtc3_id[] = { > > > > + { "shtc3", 0 }, > > > > + { } > > > > +}; > > > > +MODULE_DEVICE_TABLE(i2c, shtc3_id); > > > > + > > > > +static const struct of_device_id shtc3_dt_ids[] = { > > > > + { .compatible = "sensirion,shtc3" }, > > > > + { } > > > > +}; > > > > +MODULE_DEVICE_TABLE(of, shtc3_dt_ids); > > > > + > > > > +static struct i2c_driver shtc3_driver = { > > > > + .driver = { > > > > + .name = "shtc3", > > > > + .of_match_table = shtc3_dt_ids, > > > > + }, > > > > + .probe = shtc3_probe, > > > > + .id_table = shtc3_id, > > > > +}; > > > > + > > > > +module_i2c_driver(shtc3_driver); > > > > +MODULE_DESCRIPTION("Sensirion SHTC3 Humidity and Temperature Sensor"); > > > > +MODULE_AUTHOR("Gilles Talis <gilles.talis@xxxxxxxxx>"); > > > > +MODULE_LICENSE("GPL"); > > > > > > > > > >
