Hi Jonathan,
On Sat, 8 Feb 2020 15:17:10 +0000
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Wed, 5 Feb 2020 11:16:55 +0100
> Tomas Novotny <tomas@xxxxxxxxxx> wrote:
>
> > Multi pulse settings allow to emit more pulses during one measurement
> > (up to 8 on vcnl4040 and vcnl4200). The raw reading is approximately
> > multiplied by the multi pulse settings. More information is available in
> > the added documentation.
> >
> > Multi pulse is specific only for proximity sensor. Only the vcnl4040 and
> > vcnl4200 hardware supports this settings.
>
> A few comments but this one is more or less good.
>
> Thanks,
>
> Jonathan
>
> >
> > Signed-off-by: Tomas Novotny <tomas@xxxxxxxxxx>
> > ---
> > Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 | 21 +++++++++
> > drivers/iio/light/vcnl4000.c | 60 ++++++++++++++++++++++++
> > 2 files changed, 81 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > index 4860f409dbf0..923a78dc9869 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > @@ -16,3 +16,24 @@ KernelVersion: 5.7
> > Contact: linux-iio@xxxxxxxxxxxxxxx
> > Description:
> > The on/off available duty ratios.
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/in_proximity_multi_pulse
>
> This is less ambiguous than duty_cycle, but perhaps something
> in_proximity_led_pulse_count is more specific?
ok.
> > +Date: February 2020
> > +KernelVersion: 5.7
> > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > + Instead of one single pulse per every measurement, more pulses
> > + may be programmed. This leads to a longer led current on-time
> > + for each proximity measurement, which also results in a higher
> > + detection range. The raw reading is approximately multiplied by
> > + the multi pulse settings. The duty ration is not changed. The
oh, typo.
> Hmm. Normal meaning of duty ratio would be changed by this... It's uneven but
> multiple pulses == more on time hence lower duty ratio.
>
> > + settings cannot be changed when the proximity channel is
> > + enabled. Valid values are in the respective '_available'
> > + attribute.
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/in_proximity_multi_pulse_available
> > +Date: February 2020
> > +KernelVersion: 5.7
> > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > + List of multi pulse values.
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index a8c2ce1056a6..cc75e5e7e634 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -46,6 +46,7 @@
> >
> > #define VCNL4200_AL_CONF 0x00 /* Ambient light configuration */
> > #define VCNL4200_PS_CONF1 0x03 /* Proximity configuration */
> > +#define VCNL4200_PS_CONF3 0x04 /* Proximity conf., white channel, LED I */
> > #define VCNL4200_PS_DATA 0x08 /* Proximity data */
> > #define VCNL4200_AL_DATA 0x09 /* Ambient light data */
> > #define VCNL4200_DEV_ID 0x0e /* Device ID, slave address and version */
> > @@ -65,6 +66,8 @@
> > #define VCNL4200_PS_DUTY_MASK GENMASK(7, 6) /* Proximity duty ratio */
> > #define VCNL4200_PS_DUTY_SHIFT 6
> > #define VCNL4200_PS_SD BIT(0) /* Proximity shutdown */
> > +#define VCNL4200_PS_MPS_MASK GENMASK(6, 5)
> > +#define VCNL4200_PS_MPS_SHIFT 5
>
> You could probably use the FIELD_PREP macros etc to avoid having both mask and
> shift defines.
As Marco suggested, I will use reg_field.
Thanks a lot for the whole review.
Tomas
> > enum vcnl4000_device_ids {
> > VCNL4000,
> > @@ -223,11 +226,24 @@ static const char * const vcnl4200_ps_duty_ratio_items[] = {
> > "1/1280"
> > };
> >
> > +/* Values are directly mapped to register values. */
> > +static const char * const vcnl4200_ps_multi_pulse_items[] = {
> > + "1",
> > + "2",
> > + "4",
> > + "8"
> > +};
> > +
> > static int vcnl4200_get_ps_duty_ratio(struct iio_dev *indio_dev,
> > const struct iio_chan_spec *chan);
> > static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
> > const struct iio_chan_spec *chan,
> > unsigned int mode);
> > +static int vcnl4200_get_ps_multi_pulse(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan);
> > +static int vcnl4200_set_ps_multi_pulse(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + unsigned int mode);
> >
> > static const struct iio_enum vcnl4040_ps_duty_ratio_enum = {
> > .items = vcnl4040_ps_duty_ratio_items,
> > @@ -243,15 +259,26 @@ static const struct iio_enum vcnl4200_ps_duty_ratio_enum = {
> > .set = vcnl4200_set_ps_duty_ratio,
> > };
> >
> > +static const struct iio_enum vcnl4200_ps_multi_pulse_enum = {
> > + .items = vcnl4200_ps_multi_pulse_items,
> > + .num_items = ARRAY_SIZE(vcnl4200_ps_multi_pulse_items),
> > + .get = vcnl4200_get_ps_multi_pulse,
> > + .set = vcnl4200_set_ps_multi_pulse,
> > +};
> > +
> > static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[] = {
> > IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4040_ps_duty_ratio_enum),
> > IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4040_ps_duty_ratio_enum),
> > + IIO_ENUM("multi_pulse", IIO_SEPARATE, &vcnl4200_ps_multi_pulse_enum),
> > + IIO_ENUM_AVAILABLE("multi_pulse", &vcnl4200_ps_multi_pulse_enum),
> > { },
> > };
> >
> > static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[] = {
> > IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4200_ps_duty_ratio_enum),
> > IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4200_ps_duty_ratio_enum),
> > + IIO_ENUM("multi_pulse", IIO_SEPARATE, &vcnl4200_ps_multi_pulse_enum),
> > + IIO_ENUM_AVAILABLE("multi_pulse", &vcnl4200_ps_multi_pulse_enum),
> > { },
> > };
> >
> > @@ -638,6 +665,39 @@ static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
> > return vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
> > };
> >
> > +static int vcnl4200_get_ps_multi_pulse(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan)
> > +{
> > + int ret;
> > + unsigned int val;
> > + struct vcnl4000_data *data = iio_priv(indio_dev);
> > +
> > + ret = regmap_read(data->regmap, VCNL4200_PS_CONF3, &val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + val &= VCNL4200_PS_MPS_MASK;
> > + val >>= VCNL4200_PS_MPS_SHIFT;
> > +
> > + return val;
> > +};
> > +
> > +static int vcnl4200_set_ps_multi_pulse(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + unsigned int mode)
> > +{
> > + int ret;
> > + struct vcnl4000_data *data = iio_priv(indio_dev);
> > +
> > + ret = vcnl4200_check_enabled(data, IIO_PROXIMITY);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return regmap_update_bits(data->regmap, VCNL4200_PS_CONF3,
> > + VCNL4200_PS_MPS_MASK,
> > + mode << VCNL4200_PS_MPS_SHIFT);
> > +};
> > +
> > static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > [VCNL4000] = {
> > .prod = "VCNL4000",
>
