On Sat, 9 Dec 2023 18:58:16 +0800
Li peiyu <579lpy@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>
> Signed-off-by: Li peiyu <579lpy@xxxxxxxxx>
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);
> +}
