Re: [PATCH 2/2] iio: light: vcnl4000: Add interrupt support for vcnl4040

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 20 Dec 2022 22:49:59 +0100
Mårten Lindahl <marten.lindahl@axis.com> 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@axis.com>
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;
> +}
> +