On Tue, 20 Dec 2022 22:49:59 +0100
Mårten Lindahl <marten.lindahl@xxxxxxxx> wrote:
> Add support to configure proximity sensor interrupts and threshold
> limits for vcnl4040. If an interrupt is detected an event will be
> pushed to the event interface.
>
> Signed-off-by: Mårten Lindahl <marten.lindahl@xxxxxxxx>
Hi,
Code looks good in general. A few readability related suggestions inline.
Thanks,
Jonathan
> ---
> drivers/iio/light/vcnl4000.c | 163 +++++++++++++++++++++++++++++++++++
> 1 file changed, 163 insertions(+)
>
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 142d1760f65d..61d18c404ea6 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -60,8 +60,11 @@
...
> /* Bit masks for interrupt registers. */
> #define VCNL4010_INT_THR_SEL BIT(0) /* Select threshold interrupt source */
> @@ -138,6 +144,7 @@ struct vcnl4000_data {
> enum vcnl4000_device_ids id;
> int rev;
> int al_scale;
> + int ps_int;
Bit big for 2 bits ;) Maybe size it same as register size.
Also, probably benefit from a comment as ps_int isn't a particularly obviously name.
> const struct vcnl4000_chip_spec *chip_spec;
> struct mutex vcnl4000_lock;
> struct vcnl4200_channel vcnl4200_al;
...
>
> +static int vcnl4040_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct vcnl4000_data *data = iio_priv(indio_dev);
> +
> + return (dir == IIO_EV_DIR_RISING) ?
> + (data->ps_int & 0x01) : (data->ps_int & 0x02) >> 1;
Add some field definitions and FIELD_GET() to extract them.
> +}
> +
> +static int vcnl4040_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir, int state)
> +{
> + int ret;
> + u16 val;
> + struct vcnl4000_data *data = iio_priv(indio_dev);
> +
> + mutex_lock(&data->vcnl4000_lock);
> +
> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> + if (ret < 0)
> + goto out;
> +
> + val = FIELD_GET(VCNL4040_PS_CONF2_PS_INT, ret);
> +
> + if (dir == IIO_EV_DIR_RISING)
> + val = state ? (val | 0x1) : (val & 0x2);
Whilst I'm sure this is correct, it's not easy to follow. Perhaps
val = state ? (val | 0x1) : (val & ~0x1);
to make it clear you are turning on an off one bit?
Also as above, some field definitions may make this easier to follow.
> + else
> + val = state ? (val | 0x2) : (val & 0x1);
> +
> + data->ps_int = val;
> + val = (ret & ~VCNL4040_PS_CONF2_PS_INT) |
It's been quite a few lines. Probably better to put that ret into
a reg_val or similarly named field to make it slightly easier to see it
is retained from above.
> + FIELD_PREP(VCNL4040_PS_CONF2_PS_INT, val);
> + ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, val);
> +
> +out:
> + mutex_unlock(&data->vcnl4000_lock);
> + data->chip_spec->set_power_state(data, (bool)data->ps_int);
the bool cast is a little nasty. Perhaps != 0 is clearer?
> +
> + return ret;
> +}
> +
