The 3 parameters in question in the datasheet:
- PS_CAN_DIG : This is just a digital substraction
- PS_CAN_ANA_DURATION: The duration of the short cancellation light pulse
- PS_CAN_ANA_CURRENT: The light pulse current used
I used a standard calibration attribute name for all of those, respectively:
- in_proximity_calibscale
- in_proxmiity_calibbias
- out_current_calibbias
I don't know if this is a correct use or not.
See my other comments inline.
Thank you,
Mikael
On Thu, Dec 19, 2024 at 04:34:54PM +0000, Jonathan Cameron wrote:
> On Mon, 16 Dec 2024 17:55:41 -0500
> Mikael Gonella-Bolduc via B4 Relay <devnull+mgonellabolduc.dimonoff.com@xxxxxxxxxx> wrote:
>
> > From: Mikael Gonella-Bolduc <mgonellabolduc@xxxxxxxxxxxx>
> >
> > APDS9160 is a combination of ALS and proximity sensors.
> >
> > This patch add supports for:
> > - Intensity clear data and illuminance data
> > - Proximity data
> > - Gain control, rate control
> > - Event thresholds
> >
> > Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@xxxxxxxxxxxx>
>
> Hi Mikael,
>
> A couple of questions on the calib* parts. I hadn't looked closely those
> before and the datasheet is not very helpful!
>
> Jonathan
>
>
> > diff --git a/drivers/iio/light/apds9160.c b/drivers/iio/light/apds9160.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..0c93ab847d9a36aac7aa6a1893bba0fe819d9e28
> > --- /dev/null
> > +++ b/drivers/iio/light/apds9160.c
>
> > +
> > +/*
> > + * The PS intelligent cancellation level register allows
> > + * for an on-chip substraction of the ADC count caused by
> > + * unwanted reflected light from PS ADC output.
> As it's subtraction, why calibscale? Sounds more suitable to make this to calibbias.
> > + */
> > +static int apds9160_set_ps_cancellation_level(struct apds9160_chip *data,
> > + int val)
> > +{
> > + int ret;
> > + __le16 buf;
> > +
> > + if (val < 0 || val > 0xFFFF)
> > + return -EINVAL;
> > +
> > + buf = cpu_to_le16(val);
> > + ret = regmap_bulk_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_DIG_LSB,
> > + &buf, 2);
> > + if (ret)
> > + return ret;
> > +
> > + data->ps_cancellation_level = val;
> > +
> > + return ret;
> > +}
> > +
> > +/*
> > + * This parameter determines the cancellation pulse duration
> > + * in each of the PWM pulse. The cancellation is applied during the
> > + * integration phase of the PS measurement.
> > + * Duration is programmed in half clock cycles
> > + * A duration value of 0 or 1 will not generate any cancellation pulse
>
> Perhaps add some details on why this is a calibbias type control?
>
> Whilst I can sort of grasp it might have a similar affect to a conventional
> calibration bias by removing some offset, it's not totally obvious.
>
After looking at all possible types for a proxmity channel this is what I though was the most sensible choice.
Is it possible to use a custom attribute type here if nothing fits?
Or maybe we should drop this entirely since it will probably be rarely used.
> > + */
> > +static int apds9160_set_ps_analog_cancellation(struct apds9160_chip *data,
> > + int val)
> > +{
> > + int ret;
> > +
> > + if (val < 0 || val > 0x7F)
> > + return -EINVAL;
> > +
> > + ret = regmap_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_ANA_DUR,
> > + val);
> > + if (ret)
> > + return ret;
> > +
> > + data->ps_cancellation_analog = val;
> > +
> > + return ret;
> > +}
> > +
> > +/*
> > + * This parameter works in conjunction with the cancellation pulse duration
> > + * The value determines the current used for crosstalk cancellation
> > + * B4-B5: The coarse value defines cancellation current in steps of 60nA
> > + * B0-B3: The fine value adjusts the cancellation current in steps of 2.4nA
>
> Whilst I'm failing to actually understand what this is doing and maybe never will
> we can make the interface more intuitive by not using the encoded value.
> Just use a value in nA with both the val and val2 parts.
>
> it is rather odd given 15 * 2.4 is only 36 so there are holes.. PRobably a case
> for getting as close as you can to the requested value.
>
> Calibration parameters aren't guaranteed to have a simple interpretation but
> this current case is worse that it needs to be.
>
Ok, noted I will make this change for v4 if it's best to keep this.
This works in conjunction with the cancellation duration.
> > + case IIO_CHAN_INFO_CALIBSCALE:
> > + if (val2 != 0)
> > + return -EINVAL;
> > + switch (chan->type) {
> > + case IIO_PROXIMITY:
> > + return apds9160_set_ps_cancellation_level(data, val);
> > + case IIO_CURRENT:
> > + return apds9160_set_ps_cancellation_current(data, val);
>
> I can't figure out what this one actually is.
>
This is the current used by the cancellation light pulse (PS_CAN_ANA_CURRENT).
I might not be using the correct type name here, since it's so specific to this chip
I just don't know which one to use.
