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

[Jonathan Cameron <jic23@kernel.org>]

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

> From: Zhigang Shi <Zhigang.Shi@liteon.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@collabora.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.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;
> +}
> +

> +