lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240720175439.54b73f2c@jic23-huawei>
Date: Sat, 20 Jul 2024 17:54:39 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Joshua Felmeden <jfelmeden@...goodpenguin.co.uk>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] iio: humidity: Add support for ENS210

On Fri, 19 Jul 2024 13:50:54 +0100
Joshua Felmeden <jfelmeden@...goodpenguin.co.uk> wrote:

> Add support for ENS210/ENS210A/ENS211/ENS212/ENS213A/ENS215.
> 
> The ENS21x is a family of temperature and relative humidity sensors with
> accuracies tailored to the needs of specific applications.
> 
> Signed-off-by: Joshua Felmeden <jfelmeden@...goodpenguin.co.uk>
Hi Joshua

Looking pretty good, but a few more minor comments.

Jonathan


> +
> +/**
> + * struct ens210_data - Humidity/Temperature sensor device structure
> + * @client:	i2c client
> + * @lock:	lock protecting the i2c conversion
> + * @res_index:	index to selected sensor resolution
> + */
> +struct ens210_data {
> +	struct i2c_client *client;
> +	const struct ens210_chip_info *chip_info;
> +	struct mutex lock;

Docs rather different to content. Make sure to run kernel-doc script
over the files and fix warnings.

> +};
> +
> +/* calculate 17-bit crc7 */
> +static u8 ens210_crc7(u32 val)
> +{
> +	__be32 val_be = (cpu_to_be32(val & 0x1ffff) >> 0x8);

drop excess outer brackets.

> +
> +	return crc7_be(0xde, (u8 *)&val_be, 3) >> 1;
> +}
> +
> +static int ens210_get_measurement(struct iio_dev *indio_dev, bool temp, int *val)
> +{
> +	u32 regval;
> +	u8 regval_le[3];
> +	int ret;
> +	struct ens210_data *data = iio_priv(indio_dev);
> +
> +	/* assert read */
> +	ret = i2c_smbus_write_byte_data(data->client, ENS210_REG_SENS_START,
> +				  temp ? ENS210_SENS_START_T_START :
> +					 ENS210_SENS_START_H_START);
> +	if (ret)
> +		return ret;
> +
> +	/* wait for conversion to be ready */
> +	msleep(data->chip_info->conv_time_msec);
> +
> +	ret = i2c_smbus_read_byte_data(data->client,
> +				       ENS210_REG_SENS_STAT);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* perform read */
> +	ret = i2c_smbus_read_i2c_block_data(
> +		data->client, temp ? ENS210_REG_T_VAL : ENS210_REG_H_VAL, 3,
> +		(u8 *)&regval_le);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "failed to read register");
> +		return -EIO;
> +	} else if (ret != 3) {
> +		dev_err(&indio_dev->dev, "expected 3 bytes, received %d\n", ret);

Use the i2c dev for all error messages.
The indio_dev->dev one is only available in some paths and not generally
as useful.

Add a local 
struct device *dev = &data->client->dev;
in places where you use it lots of times.

> +		return -EIO;
> +	}
> +
> +	regval = get_unaligned_le24(regval_le);
> +	if (ens210_crc7(regval) != ((regval >> 17) & 0x7f)) {

> +		/* crc fail */
> +		dev_err(&indio_dev->dev, "ens invalid crc\n");
> +		return -EIO;
> +	}
> +
> +	*val = regval & 0xffff;
I wondered what the 17th bit is and it seems to be a valid flag.
Should we be checking that before returning the data?

> +	return IIO_VAL_INT;
> +}
> +
> +static int ens210_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *channel, int *val,
> +			   int *val2, long mask)
> +{
> +	struct ens210_data *data = iio_priv(indio_dev);
> +	int ret = -EINVAL;

> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		scoped_guard(mutex, &data->lock) {
> +			ret = ens210_get_measurement(
> +				indio_dev, channel->type == IIO_TEMP, val);
Odd line wrapping. I'd just use a slightly long line and format as
>			ret = ens210_get_measurement(indio_dev,
						     channel->type == IIO_TEMP,
						     val);


> +			if (ret)
> +				return ret;
> +			return IIO_VAL_INT;
> +		}
> +		return ret;
		unreachable();
The compiler may fail to figure out we can't get here so we tell it.
The drop the return ret;

> +	case IIO_CHAN_INFO_SCALE:
> +		if (channel->type == IIO_TEMP) {
> +			*val = 15;
> +			*val2 = 625000;
> +		} else {
> +			*val = 1;
> +			*val2 = 953125;
> +		}
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_OFFSET:
> +		if (channel->type == IIO_TEMP) {
> +			*val = -17481;
> +			*val2 = 600000;
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +			break;
> +		}
> +		*val = 0;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +	return ret;
Unreachable code I think, so drop it.

> +}
> +

> +static int ens210_probe(struct i2c_client *client)
> +{
> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
Don't think this is used any more. 
> +	struct ens210_data *data;
> +	struct iio_dev *indio_dev;
> +	uint16_t part_id;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +			I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> +			I2C_FUNC_SMBUS_WRITE_BYTE |
> +			I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> +		dev_err_probe(&client->dev, -EOPNOTSUPP,
> +			"adapter does not support some i2c transactions\n");
> +		return -EOPNOTSUPP;
		return dev_err_probe()
don't worry about the slightly long line.

> +	}
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);

Used?  If not drop it.

> +	data->client = client;
> +	mutex_init(&data->lock);
> +	data->chip_info = i2c_get_match_data(client);
> +
> +	ret = devm_regulator_get_enable(&client->dev, "vdd");
> +	if (ret)
> +		return ret;
> +
> +	/* reset device */
> +	ret = i2c_smbus_write_byte_data(client, ENS210_REG_SYS_CTRL,
> +					ENS210_SYS_CTRL_SYS_RESET);
> +	if (ret)
> +		return ret;
> +
> +	/* wait for device to become active */
> +	usleep_range(4000, 5000);
> +
> +	/* disable low power mode */
> +	ret = i2c_smbus_write_byte_data(client, ENS210_REG_SYS_CTRL, 0x00);
> +	if (ret)
> +		return ret;
> +
> +	/* wait for device to finish */
> +	usleep_range(4000, 5000);
> +
> +	/* get part_id */
> +	part_id = i2c_smbus_read_word_data(client, ENS210_REG_PART_ID);
> +
> +	if (part_id != data->chip_info->part_id) {
> +		dev_info(&client->dev,
> +			"Part ID does not match (0x%04x != 0x%04lx)\n", part_id,
%04x as per the build bot warning.

> +			data->chip_info->part_id);
> +	}
> +
> +	/* reenable low power */
> +	ret = i2c_smbus_write_byte_data(client, ENS210_REG_SYS_CTRL,
> +					ENS210_SYS_CTRL_LOW_POWER_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = data->chip_info->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ens210_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ens210_channels);
> +	indio_dev->info = &ens210_info;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ