On Tue, 3 May 2022 20:13:54 +0530
Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx> wrote:
> From: Zhigang Shi <Zhigang.Shi@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> Signed-off-by: Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx>
> Signed-off-by: Zhigang Shi <Zhigang.Shi@xxxxxxxxxx>
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;
> +}
> +
> +
