Re: [PATCH 2/2] iio: light: vcnl4000: Add ps high definition for vcnl4040

[Mårten Lindahl <martenli@xxxxxxxx>]

On 12/17/23 15:15, Jonathan Cameron wrote:
> On Fri, 15 Dec 2023 13:43:05 +0100
> Mårten Lindahl <marten.lindahl@xxxxxxxx> wrote:
>
> > The vcnl4040 proximity sensor defaults to 12 bit data resolution, but
> > the chip also supports 16 bit data resolution, which is called proximity
> > high definition (PS_HD).
> >
> > Add read/write attribute for proximity resolution, and read attribute
> > for available proximity resolution values for the vcnl4040 chip.
> >
> > Signed-off-by: Mårten Lindahl <marten.lindahl@xxxxxxxx>
Hi Jonathan!
> I'll review this on basis the usecase is clear (see reply to cover letter)

I'll skip this patch (see reply to cover letter comment)

Your are right about the paired register manipulation. Better to 
read/write byte instead of word.

Kind regards

Mårten

> The manipulation of the CONF1 and CONF2 registers in a pair is rather odd given you
> only want to change one bit here.
>
> Why is that done?
> > ---
> >   drivers/iio/light/vcnl4000.c | 87 +++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 85 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index fdf763a04b0b..2addff635b79 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -90,6 +90,7 @@
> >   #define VCNL4040_PS_CONF1_PS_SHUTDOWN	BIT(0)
> >   #define VCNL4040_PS_CONF2_PS_IT	GENMASK(3, 1) /* Proximity integration time */
> >   #define VCNL4040_CONF1_PS_PERS	GENMASK(5, 4) /* Proximity interrupt persistence setting */
> > +#define VCNL4040_PS_CONF2_PS_HD		BIT(11)	/* Proximity high definition */
> >   #define VCNL4040_PS_CONF2_PS_INT	GENMASK(9, 8) /* Proximity interrupt mode */
> >   #define VCNL4040_PS_CONF3_MPS		GENMASK(6, 5) /* Proximity multi pulse number */
> >   #define VCNL4040_PS_MS_LED_I		GENMASK(10, 8) /* Proximity current */
> > @@ -170,6 +171,11 @@ static const int vcnl4040_ps_calibbias_ua[][2] = {
> >   	{0, 200000},
> >   };
> >   
> > +static const int vcnl4040_ps_resolutions[2] = {
> > +	12,
> > +	16
> > +};
> > +
> >   static const int vcnl4040_als_persistence[] = {1, 2, 4, 8};
> >   static const int vcnl4040_ps_persistence[] = {1, 2, 3, 4};
> >   static const int vcnl4040_ps_oversampling_ratio[] = {1, 2, 4, 8};
> > @@ -880,6 +886,54 @@ static ssize_t vcnl4040_write_ps_calibbias(struct vcnl4000_data *data, int val)
> >   	return ret;
> >   }
> >   
> > +static ssize_t vcnl4040_read_ps_resolution(struct vcnl4000_data *data, int *val, int *val2)
> > +{
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> The field seems to be in PS_CONF2.  So you are reading a word and I guess that
> gets you two registers.  Can we not do a byte read to get just CONF2?
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = FIELD_GET(VCNL4040_PS_CONF2_PS_HD, ret);
> > +	if (ret >= ARRAY_SIZE(vcnl4040_ps_resolutions))
> > +		return -EINVAL;
> > +
> > +	*val = vcnl4040_ps_resolutions[ret];
> > +	*val2 = 0;
> > +
> > +	return ret;
> > +}
> > +
> > +static ssize_t vcnl4040_write_ps_resolution(struct vcnl4000_data *data, int val)
> > +{
> > +	unsigned int i;
> > +	int ret;
> > +	u16 regval;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(vcnl4040_ps_resolutions); i++) {
> > +		if (val == vcnl4040_ps_resolutions[i])
> > +			break;
> > +	}
> > +
> > +	if (i >= ARRAY_SIZE(vcnl4040_ps_resolutions))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&data->vcnl4000_lock);
> > +
> > +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> > +	if (ret < 0)
> > +		goto out_unlock;
> > +
> > +	regval = (ret & ~VCNL4040_PS_CONF2_PS_HD);
> > +	regval |= FIELD_PREP(VCNL4040_PS_CONF2_PS_HD, i);
> > +	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1,
> > +					regval);
> > +
> > +out_unlock:
> > +	mutex_unlock(&data->vcnl4000_lock);
> > +	return ret;
> > +}
> c),