linux-kernel - Re: [PATCH v3 3/3] iio: light: Add support for ltrf216a sensor

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] [day] [month] [year] [list]

Message-ID: <20220507175912.10264563@jic23-huawei>
Date:   Sat, 7 May 2022 17:59:12 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Shreeya Patel <shreeya.patel@...labora.com>
Cc:     lars@...afoo.de, robh+dt@...nel.org, Zhigang.Shi@...eon.com,
        krzk@...nel.org, krisman@...labora.com, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...labora.com, alvaro.soliverez@...labora.com
Subject: Re: [PATCH v3 3/3] iio: light: Add support for ltrf216a sensor

On Tue,  3 May 2022 20:13:54 +0530
Shreeya Patel <shreeya.patel@...labora.com> wrote:

> From: Zhigang Shi <Zhigang.Shi@...eon.com>
> 
> Add initial support for ltrf216a ambient light sensor.
> 
> Datasheet: gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTRF216A.pdf
> Co-developed-by: Shreeya Patel <shreeya.patel@...labora.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@...labora.com>
> Signed-off-by: Zhigang Shi <Zhigang.Shi@...eon.com>

One locking bug, otherwise just the lock documentation to resolve.

Thanks,

Jonathan



> +
> +struct ltrf216a_data {
> +	struct i2c_client *client;
> +	u32 int_time;
> +	u16 int_time_fac;
> +	u8 als_gain_fac;
> +	struct mutex mutex; /* Protect read and write operations */

See other branch of thread for feedback on that comment

Thanks for highlighting that btw. I'd forgotten about it so might have
missed it on a fresh read through.

> +};
> +
> +/* open air. need to update based on TP transmission rate. */
> +#define WIN_FAC	1
> +

> +
> +static int ltrf216a_read_data(struct ltrf216a_data *data, u8 addr)
> +{
> +	int i, ret = -1, tries = 25;
> +	u8 buf[3];
> +
> +	while (tries--) {
> +		ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_STATUS);
> +		if (ret < 0)
> +			return ret;
> +		if (ret & LTRF216A_ALS_DATA_STATUS)
> +			break;
> +		msleep(20);
> +	}
> +
> +	for (i = 0; i < 3; i++) {
> +		ret = i2c_smbus_read_byte_data(data->client, addr);

Might be worth seeing if the device copes with
i2c_smbus_read_i2c_block_data()
The datasheet doesn't mention it though so it might well not work.


> +		if (ret < 0)
> +			return ret;
> +		buf[i] = ret;
> +		addr++;
> +	}
> +
> +	return get_unaligned_le24(&buf[0]);
> +}

> +
> +static int ltrf216a_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int *val,
> +			     int *val2, long mask)
> +{
> +	int ret;
> +	struct ltrf216a_data *data = iio_priv(indio_dev);
> +
> +	mutex_lock(&data->mutex);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_PROCESSED:
> +		ret = ltrf216a_get_lux(data);
> +		if (ret < 0)
> +			return ret;

Check your locking.  This path doesn't unlock the mutex.

> +		*val = ret;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		ret = ltrf216a_get_int_time(data, val, val2);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +

> +