| Message ID | 20231209105217.3630-1-579lpy@gmail.com |
|---|---|
| Headers | show |
| Series | iio: humidity: Add driver for ti HDC302x humidity sensors | expand |
On Sat, 9 Dec 2023 18:58:16 +0800 Li peiyu <579lpy@gmail.com> wrote: > Add support for HDC302x integrated capacitive based relative > humidity (RH) and temperature sensor. > This driver supports reading values, reading the maximum and > minimum of values and controlling the integrated heater of > the sensor. > > Co-developed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > Signed-off-by: Li peiyu <579lpy@gmail.com> A few follow up comments as you are going to be doing a v6 to resolve the dt-binding feedback on v5. Thanks, Jonathan > diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile > index f19ff3de97c5..5fbeef299f61 100644 > --- a/drivers/iio/humidity/Makefile > +++ b/drivers/iio/humidity/Makefile > @@ -7,6 +7,7 @@ obj-$(CONFIG_AM2315) += am2315.o > obj-$(CONFIG_DHT11) += dht11.o > obj-$(CONFIG_HDC100X) += hdc100x.o > obj-$(CONFIG_HDC2010) += hdc2010.o > +obj-$(CONFIG_HDC3020) += hdc3020.o > obj-$(CONFIG_HID_SENSOR_HUMIDITY) += hid-sensor-humidity.o > > hts221-y := hts221_core.o \ > diff --git a/drivers/iio/humidity/hdc3020.c b/drivers/iio/humidity/hdc3020.c > new file mode 100644 > index 000000000000..da7a7990656c > --- /dev/null > +++ b/drivers/iio/humidity/hdc3020.c > @@ -0,0 +1,470 @@ .. > +static const struct iio_chan_spec hdc3020_channels[] = { > + { > + .type = IIO_TEMP, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_PEAK) | > + BIT(IIO_CHAN_INFO_TROUGH) | 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_PEAK) | > + BIT(IIO_CHAN_INFO_TROUGH) | BIT(IIO_CHAN_INFO_OFFSET), The offset is 0 for this channel. Convention for that is don't provide it as that is the assumed default. So drop BIT(IIO_CHAN_INFO_OFFSET) from here and return an error in read_raw if offset is requested for channels of type other than IIO_TEMP > + }, > + { > + /* > + * For setting the internal heater, which can be switched on to > + * prevent or remove any condensation that may develop when the > + * ambient environment approaches its dew point temperature. > + */ > + .type = IIO_CURRENT, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW), > + .output = 1, > + }, > +}; > + > + > +static int hdc3020_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + struct hdc3020_data *data = iio_priv(indio_dev); > + int ret; > + > + if (chan->type != IIO_TEMP && chan->type != IIO_HUMIDITYRELATIVE) > + return -EINVAL; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: { > + guard(mutex)(&data->lock); > + ret = hdc3020_read_measurement(data, chan->type, val); > + if (ret < 0) > + return ret; > + > + return IIO_VAL_INT; > + } > + case IIO_CHAN_INFO_PEAK: { > + guard(mutex)(&data->lock); > + if (chan->type == IIO_TEMP) { > + ret = hdc3020_read_high_peak_t(data, val); > + if (ret < 0) > + return ret; > + } else { > + ret = hdc3020_read_high_peak_rh(data, val); > + if (ret < 0) > + return ret; > + } > + return IIO_VAL_INT; > + } > + case IIO_CHAN_INFO_TROUGH: { > + guard(mutex)(&data->lock); > + if (chan->type == IIO_TEMP) { > + ret = hdc3020_read_low_peak_t(data, val); > + if (ret < 0) > + return ret; > + } else { > + ret = hdc3020_read_low_peak_rh(data, val); > + if (ret < 0) > + return ret; > + } > + return IIO_VAL_INT; > + } > + case IIO_CHAN_INFO_SCALE: > + *val2 = 65536; > + if (chan->type == IIO_TEMP) > + *val = 175; > + else > + *val = 100; > + return IIO_VAL_FRACTIONAL; > + > + case IIO_CHAN_INFO_OFFSET: > + if (chan->type == IIO_TEMP) > + *val = 16852; > + else > + *val = 0; > + return IIO_VAL_INT; > + } > + > + return -EINVAL; Where we expect all cases in a switch statement to return (no breaks) like here, I think we can make that explicit and let the compiler catch any that break this intent, by moving this final return into default: return -EINVAL; > +} > + > + > +static int hdc3020_update_heater(struct hdc3020_data *data, int val) > +{ > + u8 buf[5]; > + int ret; > + > + if (val < hdc3020_heater_vals[0] || val > hdc3020_heater_vals[2]) > + return -EINVAL; > + > + buf[0] = HDC3020_HEATER_CMD_MSB; > + > + if (!val) { > + buf[1] = HDC3020_HEATER_DISABLE; > + return hdc3020_write_bytes(data, buf, 2); > + } > + > + buf[1] = HDC3020_HEATER_CONFIG; > + buf[2] = (val & 0x3F00) >> 8; > + buf[3] = val & 0xFF; You could do this as a put_unaligned_be16(val & GENMASK(13, 0), &buf[2]); (at least I think that's what it is doing). That makes it a little more explicit that a 14 bit value is being written. > + buf[4] = crc8(hdc3020_crc8_table, buf + 2, 2, CRC8_INIT_VALUE); > + ret = hdc3020_write_bytes(data, buf, 5); > + if (ret < 0) > + return ret; > + > + buf[1] = HDC3020_HEATER_ENABLE; > + > + return hdc3020_write_bytes(data, buf, 2); > +} > + > +static int hdc3020_probe(struct i2c_client *client) > +{ > + struct iio_dev *indio_dev; > + struct hdc3020_data *data; > + int ret; > + > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) > + return -EOPNOTSUPP; > + > + 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); > + > + crc8_populate_msb(hdc3020_crc8_table, HDC3020_CRC8_POLYNOMIAL); > + > + indio_dev->name = "hdc3020"; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &hdc3020_info; > + indio_dev->channels = hdc3020_channels; > + indio_dev->num_channels = ARRAY_SIZE(hdc3020_channels); > + > + ret = hdc3020_write_bytes(data, HDC3020_S_AUTO_10HZ_MOD0, 2); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "Unable to set up measurement\n"); > + > + ret = devm_add_action_or_reset(&data->client->dev, hdc3020_stop, data); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "Failed to add device\n"); Ah. I was talking about this bit above in previous review as the devm_add_action_or_reset() is the call that is unlikely to fail and isn't "add device". Having an error for devm_iio_device_regiser() was fine and that kind of is adding a device so the error message was fine. Generally comments, in my reviews at least, come immediately after the code. In this example it was indeed ambiguous, so sorry about that! > + > + return devm_iio_device_register(&data->client->dev, indio_dev); > +}
Add support for HDC302x integrated capacitive based relative humidity (RH) and temperature sensor. This driver supports reading values, reading the maximum and minimum of values and controlling the integrated heater of the sensor. Signed-off-by: Li peiyu <579lpy@gmail.com> --- changes in v5: iio ABI: - Document _TROUGH as an info element. sensor driver: - Correct heater enable/disable commands - Rearrang header files in alphabetical order. - Change .info_mask_separate to BIT(IIO_CHAN_INFO_RAW). - Add details to mutex comment. - Add error handling for chan->type in read_raw call. - Remove error message for devm_iio_device_register. changes in v4: iio core: - Add an IIO_CHAN_INFO_TROUGH modifier for minimum values. iio ABI: - Document the new _TROUGH modifier. sensor driver: - Add MAINTAINERS. - Use new IIO_CHAN_INFO_TROUGH modifier. - Support the complete heater range. - Remove measurement values from the data structure. - Use guard(mutex)(...), make the code simpler - Removed buffer mode and direct mode conversion code - Minor coding-style fixes. dt-bindings: - removed unnecessary example - add vdd-supply to the example changes in v3: sensor driver: - Removed the custom ABI - Give up calculating values in the driver - Use read_avail callback to get available parameters - Changed the scope of the lock to make the code more concise - Fixed the code format issue dt-bindings: - Use a fallback compatible changes in v2: sensor driver: - Added static modification to global variables - change the methord to read peak value dt-bindings: - change the maintainers to me. - hdc3020,hdc3021,hdc3022 are compatible,I've changed the dirver. - change the node name to humidity-sensor. --- Javier Carrasco (2): iio: core: introduce trough modifier for minimum values iio: ABI: document temperature and humidity peak/trough raw attributes Li peiyu (2): dt-bindings: iio: humidity: Add TI HDC302x support iio: humidity: Add driver for TI HDC302x humidity sensors Documentation/ABI/testing/sysfs-bus-iio | 13 +- .../bindings/iio/humidity/ti,hdc3020.yaml | 55 +++ MAINTAINERS | 8 + drivers/iio/humidity/Kconfig | 12 + drivers/iio/humidity/Makefile | 1 + drivers/iio/humidity/hdc3020.c | 470 +++++++++++++++++++++ drivers/iio/industrialio-core.c | 1 + include/linux/iio/types.h | 1 + 8 files changed, 560 insertions(+), 1 deletion(-) --- base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a Best regards,