Hi Jonathan,
On Sat, 8 Feb 2020 15:11:18 +0000
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Wed, 5 Feb 2020 11:16:54 +0100
> Tomas Novotny <tomas@xxxxxxxxxx> wrote:
>
> > Duty ratio controls the proximity sensor response time. More information
> > is available in the added documentation.
>
> As always there is significant burden to defining custom ABI for
> a device. Generic userspace will have no idea what to do with it.
> In this particular case I can't really think of a straight forward
Would be usage of sampling frequency too confusing/dirty? Led on time is
controlled solely by integration time.
> way of mapping this to existing ABI so I guess we will have to
> go with custom. May be better to make it explicit what it is a
> duty ratio of. I initially thought it was of sampling for the
> proximity sensor.
>
> Perhaps something in_proximity_led_duty_cycle
>
> A few comments inline.
>
>
>
> >
> > Duty ratio is specific only for proximity sensor. Only the vcnl4040 and
> > vcnl4200 hardware supports this settings.
> >
> > Signed-off-by: Tomas Novotny <tomas@xxxxxxxxxx>
> > ---
> > Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 | 18 +++
> > drivers/iio/light/vcnl4000.c | 138 ++++++++++++++++++++++-
> > 2 files changed, 150 insertions(+), 6 deletions(-)
> > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > new file mode 100644
> > index 000000000000..4860f409dbf0
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > @@ -0,0 +1,18 @@
> > +What: /sys/bus/iio/devices/iio:deviceX/in_proximity_duty_ratio
> > +Date: February 2020
> > +KernelVersion: 5.7
> > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > + Duty ratio controls the proximity sensor response time. It is a
> > + control of on/off led current ratio. The final period depends
> > + also on integration time. The formula is simple: integration
> > + time * duty ratio off part. The settings cannot be changed when
> > + the proximity channel is enabled. Valid values are in the
> > + respective '_available' attribute.
>
> Fix the cannot be changed, by a disable / modify / enable cycle unless there
> is a very strong reason not to do that.
ok, I will do.
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/in_proximity_duty_ratio_available
> > +Date: February 2020
> > +KernelVersion: 5.7
> > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > + The on/off available duty ratios.
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index 0bad082d762d..a8c2ce1056a6 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -62,6 +62,8 @@
> > #define VCNL4200_AL_SD BIT(0) /* Ambient light shutdown */
> > #define VCNL4200_PS_IT_MASK GENMASK(3, 1) /* Proximity integration time */
> > #define VCNL4200_PS_IT_SHIFT 1
> > +#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 */
> >
> > enum vcnl4000_device_ids {
> > @@ -78,7 +80,7 @@ struct vcnl4200_channel {
> > struct mutex lock;
> > const int *int_time;
> > size_t int_time_size;
> > - int ps_duty_ratio; /* Proximity specific */
> > + const int *ps_duty_ratio; /* Proximity specific */
> > };
> >
> > struct vcnl4000_data {
> > @@ -132,6 +134,25 @@ static const struct iio_chan_spec vcnl4000_channels[] = {
> > }
> > };
> >
> > +static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[];
> > +
> > +static const struct iio_chan_spec vcnl4040_channels[] = {
> > + {
> > + .type = IIO_LIGHT,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_ENABLE),
> > + }, {
> > + .type = IIO_PROXIMITY,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_ENABLE) |
> > + BIT(IIO_CHAN_INFO_INT_TIME),
> > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> > + .ext_info = vcnl4040_ps_ext_info,
> > + }
> > +};
> > +static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[];
> > +
> > static const struct iio_chan_spec vcnl4200_channels[] = {
> > {
> > .type = IIO_LIGHT,
> > @@ -144,6 +165,7 @@ static const struct iio_chan_spec vcnl4200_channels[] = {
> > BIT(IIO_CHAN_INFO_ENABLE) |
> > BIT(IIO_CHAN_INFO_INT_TIME),
> > .info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> > + .ext_info = vcnl4200_ps_ext_info,
> > }
> > };
> >
> > @@ -171,6 +193,68 @@ static const int vcnl4200_ps_int_time[] = {
> > 0, 270
> > };
> >
> > +/* Values are directly mapped to register values. */
> > +static const int vcnl4040_ps_duty_ratio[] = {
> > + 40,
> > + 80,
> > + 160,
> > + 320
> > +};
> > +
> > +static const char * const vcnl4040_ps_duty_ratio_items[] = {
> > + "1/40",
> > + "1/80",
> > + "1/160",
> > + "1/320"
> > +};
> > +
> > +/* Values are directly mapped to register values. */
> > +static const int vcnl4200_ps_duty_ratio[] = {
> > + 160,
> > + 320,
> > + 640,
> > + 1280
> > +};
> > +
> > +static const char * const vcnl4200_ps_duty_ratio_items[] = {
> > + "1/160",
>
> We don't have any interfaces expressed as a fraction.
> Please use decimal, even if it is less compact.
ok.
> > + "1/320",
> > + "1/640",
> > + "1/1280"
> > +};
> > +
> > +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 const struct iio_enum vcnl4040_ps_duty_ratio_enum = {
> > + .items = vcnl4040_ps_duty_ratio_items,
> > + .num_items = ARRAY_SIZE(vcnl4040_ps_duty_ratio_items),
> > + .get = vcnl4200_get_ps_duty_ratio,
> > + .set = vcnl4200_set_ps_duty_ratio,
> > +};
> > +
> > +static const struct iio_enum vcnl4200_ps_duty_ratio_enum = {
> > + .items = vcnl4200_ps_duty_ratio_items,
> > + .num_items = ARRAY_SIZE(vcnl4200_ps_duty_ratio_items),
> > + .get = vcnl4200_get_ps_duty_ratio,
> > + .set = vcnl4200_set_ps_duty_ratio,
> > +};
> > +
> > +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),
> > + { },
> > +};
> > +
> > +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),
> > + { },
> > +};
> > +
> > static const struct regmap_config vcnl4000_regmap_config = {
> > .reg_bits = 8,
> > .val_bits = 8,
> > @@ -228,7 +312,11 @@ static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
> > if (ret < 0)
> > return ret;
> >
> > - duty_ratio = data->vcnl4200_ps.ps_duty_ratio;
> > + ret = vcnl4200_get_ps_duty_ratio(iio_priv_to_dev(data), NULL);
> > + if (ret < 0)
> > + return ret;
> > + duty_ratio = data->vcnl4200_ps.ps_duty_ratio[ret];
> > +
> > /*
> > * Integration time multiplied by duty ratio.
> > * Add 20% of part to part tolerance.
> > @@ -236,6 +324,7 @@ static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
> > data->vcnl4200_ps.sampling_rate =
> > ktime_set(((it_val * duty_ratio) * 6) / 5,
> > (((it_val2 * duty_ratio) * 6) / 5) * 1000);
> > +
> Please check patch series for stray whitespace changes like this one.
> They add noise and slow down acceptance of patches.
:(. Sorry, it wasn't intentional.
Thanks,
Tomas
> > return 0;
> > default:
> > return -EINVAL;
> > @@ -284,7 +373,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> > data->vcnl4200_ps.int_time = vcnl4200_ps_int_time;
> > data->vcnl4200_ps.int_time_size =
> > ARRAY_SIZE(vcnl4200_ps_int_time);
> > - data->vcnl4200_ps.ps_duty_ratio = 160;
> > + data->vcnl4200_ps.ps_duty_ratio = vcnl4200_ps_duty_ratio;
> > data->al_scale = 24000;
> > break;
> > case VCNL4040_PROD_ID:
> > @@ -293,7 +382,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> > data->vcnl4200_ps.int_time = vcnl4040_ps_int_time;
> > data->vcnl4200_ps.int_time_size =
> > ARRAY_SIZE(vcnl4040_ps_int_time);
> > - data->vcnl4200_ps.ps_duty_ratio = 40;
> > + data->vcnl4200_ps.ps_duty_ratio = vcnl4040_ps_duty_ratio;
> > data->al_scale = 120000;
> > break;
> > }
> > @@ -512,6 +601,43 @@ static int vcnl4200_set_int_time(struct vcnl4000_data *data,
> > return -EINVAL;
> > }
> >
> > +static int vcnl4200_get_ps_duty_ratio(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_CONF1, &val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + val &= VCNL4200_PS_DUTY_MASK;
> > + val >>= VCNL4200_PS_DUTY_SHIFT;
> > +
> > + return val;
> > +};
> > +
> > +static int vcnl4200_set_ps_duty_ratio(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;
> > +
> > + ret = regmap_update_bits(data->regmap, VCNL4200_PS_CONF1,
> > + VCNL4200_PS_DUTY_MASK,
> > + mode << VCNL4200_PS_DUTY_SHIFT);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
> > +};
> > +
> > static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > [VCNL4000] = {
> > .prod = "VCNL4000",
> > @@ -533,8 +659,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > },
> > [VCNL4040] = {
> > .prod = "VCNL4040",
> > - .channels = vcnl4200_channels,
> > - .num_channels = ARRAY_SIZE(vcnl4200_channels),
> > + .channels = vcnl4040_channels,
> > + .num_channels = ARRAY_SIZE(vcnl4040_channels),
> > .regmap_config = &vcnl4200_regmap_config,
> > .init = vcnl4200_init,
> > .measure_light = vcnl4200_measure_light,
>
