diff mbox series

[1/2] iio: humidity: Add support for Sensirion SHTC3 sensor

Message ID 20211114132335.47651-2-gilles.talis@gmail.com (mailing list archive)
State Not Applicable
Headers show
Series iio: humidity: add support for Sensirion SHTC3 | expand

Commit Message

Gilles Talis Nov. 14, 2021, 1:23 p.m. UTC 
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@gmail.com>
---
 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

Comments

Lars-Peter Clausen Nov. 14, 2021, 1:47 p.m. UTC | #1 
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@gmail.com>

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

> ---
>   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@gmail.com>
> + *
> + * 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)
> +		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;
> +}
> +
> +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, &reg_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;
> +
> +	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@gmail.com>");
> +MODULE_LICENSE("GPL");
Jonathan Cameron Nov. 14, 2021, 4:49 p.m. UTC | #2 
On Sun, 14 Nov 2021 15:58:15 +0100
Gilles Talis <gilles.talis@gmail.com> wrote:

> Hi Lars,
> 
> Le dim. 14 nov. 2021 à 14:47, Lars-Peter Clausen <lars@metafoo.de> 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@gmail.com>  
> >
> > 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!

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.

Thanks,

Jonathan

> 
> 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@gmail.com>
> > > + *
> > > + * 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, &reg_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@gmail.com>");
> > > +MODULE_LICENSE("GPL");  
> >
> >
> >
kernel test robot Nov. 15, 2021, 3:08 a.m. UTC | #3 
Hi Gilles,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next v5.16-rc1 next-20211112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Gilles-Talis/iio-humidity-add-support-for-Sensirion-SHTC3/20211114-212618
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: s390-randconfig-s032-20211115 (attached as .config)
compiler: s390-linux-gcc (GCC) 11.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/d4b528fcce840650354227c99ec7f6c0bdd076b0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Gilles-Talis/iio-humidity-add-support-for-Sensirion-SHTC3/20211114-212618
        git checkout d4b528fcce840650354227c99ec7f6c0bdd076b0
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/iio/humidity/shtc3.c:88:39: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected unsigned short [addressable] [usertype] cmd @@     got restricted __be16 @@
   drivers/iio/humidity/shtc3.c:88:39: sparse:     expected unsigned short [addressable] [usertype] cmd
   drivers/iio/humidity/shtc3.c:88:39: sparse:     got restricted __be16
>> drivers/iio/humidity/shtc3.c:88:56: sparse: sparse: Using plain integer as NULL pointer
   drivers/iio/humidity/shtc3.c:93:36: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected unsigned short [addressable] [usertype] cmd @@     got restricted __be16 @@
   drivers/iio/humidity/shtc3.c:93:36: sparse:     expected unsigned short [addressable] [usertype] cmd
   drivers/iio/humidity/shtc3.c:93:36: sparse:     got restricted __be16
   drivers/iio/humidity/shtc3.c:93:54: sparse: sparse: Using plain integer as NULL pointer
>> drivers/iio/humidity/shtc3.c:122:13: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned short [usertype] cmd @@     got restricted __be16 @@
   drivers/iio/humidity/shtc3.c:122:13: sparse:     expected unsigned short [usertype] cmd
   drivers/iio/humidity/shtc3.c:122:13: sparse:     got restricted __be16
   drivers/iio/humidity/shtc3.c:189:38: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected unsigned short [addressable] [usertype] cmd @@     got restricted __be16 @@
   drivers/iio/humidity/shtc3.c:189:38: sparse:     expected unsigned short [addressable] [usertype] cmd
   drivers/iio/humidity/shtc3.c:189:38: sparse:     got restricted __be16
   drivers/iio/humidity/shtc3.c:204:38: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected unsigned short [addressable] [usertype] cmd @@     got restricted __be16 @@
   drivers/iio/humidity/shtc3.c:204:38: sparse:     expected unsigned short [addressable] [usertype] cmd
   drivers/iio/humidity/shtc3.c:204:38: sparse:     got restricted __be16
   drivers/iio/humidity/shtc3.c:204:60: sparse: sparse: Using plain integer as NULL pointer

vim +88 drivers/iio/humidity/shtc3.c

    85	
    86	static int shtc3_sleep(struct i2c_client *client)
    87	{
  > 88		return shtc3_send_cmd(client, SHTC3_CMD_SLEEP, 0);
    89	}
    90	
    91	static int shtc3_wakeup(struct i2c_client *client)
    92	{
    93		if (shtc3_send_cmd(client, SHTC3_CMD_WAKEUP, 0) < 0)
    94			return -EIO;
    95	
    96		/* Wait for device to wake up */
    97		usleep_range(180, 240);
    98	
    99		return 0;
   100	}
   101	
   102	static int shtc3_read_channel(struct i2c_client *client, bool temp)
   103	{
   104		int ret;
   105		u16 cmd;
   106		u16 meas;
   107	
   108		ret = shtc3_wakeup(client);
   109		if (ret < 0)
   110			return ret;
   111	
   112		/*
   113		 * Sensor sends back measurement results after measurement command
   114		 * has been issued by the host.
   115		 * Sensor sends 3 bytes (2 bytes of data + 1 byte of CRC) for each
   116		 * channel sequentially
   117		 * The command issued by the host determines the channel for which
   118		 * the sensor will first send the data.
   119		 * We select the channel for which we need the results
   120		 * then only read back the 2 bytes corresponding to this channel.
   121		 */
 > 122		cmd = temp ? SHTC3_CMD_TEMP_MEAS_LP_CS : SHTC3_CMD_RH_MEAS_LP_CS;
   123		ret = shtc3_send_cmd(client, cmd, &meas);
   124		if (ret < 0)
   125			return ret;
   126	
   127		/* Go back to sleep */
   128		shtc3_sleep(client);
   129	
   130		return meas;
   131	}
   132	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Gilles Talis Nov. 15, 2021, 7:29 a.m. UTC | #4 
Hi Jonathan,

Le dim. 14 nov. 2021 à 17:44, Jonathan Cameron <jic23@kernel.org> a écrit :
>
> On Sun, 14 Nov 2021 15:58:15 +0100
> Gilles Talis <gilles.talis@gmail.com> wrote:
>
> > Hi Lars,
> >
> > Le dim. 14 nov. 2021 à 14:47, Lars-Peter Clausen <lars@metafoo.de> 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@gmail.com>
> > >
> > > 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@gmail.com>
> > > > + *
> > > > + * 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, &reg_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@gmail.com>");
> > > > +MODULE_LICENSE("GPL");
> > >
> > >
> > >
>
diff mbox series

Patch

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@gmail.com>
+ *
+ * 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)
+		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;
+}
+
+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, &reg_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;
+
+	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@gmail.com>");
+MODULE_LICENSE("GPL");