Re: [PATCH 1/2] hwmon: add driver for HTU31

[Christophe JAILLET]

Le 23/01/2025 à 21:25, Andrei Lalaev a écrit :
> Add base support for HTU31 temperature and humidity sensor.
>
> Besides temperature and humidity values, the driver also exports a 24-bit
> serial number to sysfs.
>
> Signed-off-by: Andrei Lalaev <andrey.lalaev@xxxxxxxxx>
> ---

...

> +static int htu31_read(struct device *dev, enum hwmon_sensor_types type,
> +		u32 attr, int channel, long *val)
> +{
> +	struct htu31_data *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = htu31_data_fetch_command(data);
> +

Unneeded empty line.

> +	if (ret < 0)
> +		return ret;
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		if (attr != hwmon_temp_input)
> +			return -EINVAL;
> +
> +		*val = data->temperature;
> +		break;
> +	case hwmon_humidity:
> +		if (attr != hwmon_humidity_input)
> +			return -EINVAL;
> +
> +		*val = data->humidity;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int serial_number_populate(struct htu31_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	u8 read_sn_cmd = HTU31_READ_SERIAL_CMD;
> +	u8 sn_buf[HTU31_SERIAL_NUMBER_LEN + HTU31_SERIAL_NUMBER_CRC_LEN];
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = client->addr,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = &read_sn_cmd,
> +		},
> +		{
> +			.addr = client->addr,
> +			.flags = I2C_M_RD,
> +			.len = sizeof(sn_buf),
> +			.buf = sn_buf,
> +		},
> +	};
> +	int ret;
> +	u8 crc;
> +
> +	guard(mutex)(&data->i2c_lock);
> +
> +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (ret < 0)
> +		return -EIO;

Why not return ret?

> +
> +	crc = crc8(htu31_crc8_table, sn_buf, HTU31_SERIAL_NUMBER_LEN, HTU31_CRC8_INIT_VAL);
> +	if (crc != sn_buf[HTU31_SERIAL_NUMBER_CRC_OFFSET]) {
> +		dev_err(&client->dev, "Serial number CRC mismatch\n");
> +		return -EIO;
> +	}
> +
> +	memcpy(data->serial_number, sn_buf, HTU31_SERIAL_NUMBER_LEN);
> +
> +	return 0;
> +}

...

> +static ssize_t heater_enable_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf,
> +				   size_t count)
> +{
> +	struct htu31_data *data = dev_get_drvdata(dev);
> +	u8 heater_cmd;
> +	bool status;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &status);
> +	if (ret)
> +		return ret;
> +
> +	heater_cmd = status ? HTU31_HEATER_ON_CMD : HTU31_HEATER_OFF_CMD;
> +
> +	guard(mutex)(&data->i2c_lock);
> +
> +	ret = i2c_master_send(data->client, &heater_cmd, 1);
> +	if (ret < 0)
> +		return -EIO;

Why not return ret?

> +
> +	data->heater_enable = status;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(heater_enable);
> +
> +static ssize_t serial_number_show(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	struct htu31_data *data = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%X%X%X\n", data->serial_number[0],
> +					   data->serial_number[1],
> +					   data->serial_number[2]);
> +}
> +
> +static DEVICE_ATTR_RO(serial_number);
> +
> +static struct attribute *htu31_attrs[] = {
> +	&dev_attr_heater_enable.attr,
> +	&dev_attr_serial_number.attr,
> +	NULL,

No need for trailing comma after a terminator

> +};
> +
> +ATTRIBUTE_GROUPS(htu31);
> +
> +static const struct hwmon_channel_info * const htu31_info[] = {
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> +	HWMON_CHANNEL_INFO(humidity, HWMON_H_INPUT),
> +	NULL,

No need for trailing comma after a terminator

> +};

...

> +static int htu31_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon_dev;
> +	struct htu31_data *data;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +	data->wait_time = HTU31_TEMPERATURE_CONV_TIME + HTU31_HUMIDITY_CONV_TIME;
> +
> +	mutex_init(&data->i2c_lock);

Maybe devm_mutex_init()?

> +
> +	crc8_populate_msb(htu31_crc8_table, HTU31_CRC8_POLYNOMIAL);
> +
> +	if (serial_number_populate(data)) {

Maybe:
    ret = serial_number_populate(data);
    if (ret) {
        ...
        return ret;
    }

> +		dev_err(dev, "Failed to read serial number\n");
> +		return -EIO;
> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev,
> +							 client->name,
> +							 data,
> +							 &htu31_chip_info,
> +							 htu31_groups);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id htu31_id[] = {
> +	{ "htu31" },
> +	{ },

No need for trailing comma after a terminator

> +};
> +MODULE_DEVICE_TABLE(i2c, htu31_id);
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id htu31_of_match[] = {
> +	{ .compatible = "meas,htu31" },
> +	{ },

No need for trailing comma after a terminator

> +};
> +MODULE_DEVICE_TABLE(of, htu31_of_match);

...

CJ