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

[Marten Lindahl <martenli@axis.com>]

On Fri, Dec 23, 2022 at 05:00:54PM +0100, Jonathan Cameron wrote:
> 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

Hi Jonathan!

Thank you. Please see my reflections below.
> 
> > ---
> >  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.

Ok, I'll do so.
> 
> >  	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.

I will do that.
> 
> > +}
> > +
> > +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.

I will do that to make it more clear.
> 
> > +	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?

I will rework these lines using field definitions. I'll send v2 shortly.

Kind regards
Mårten

> 
> > +
> > +	return ret;
> > +}
> > +
> 
>