Hi Jonathan,
On Sat, 8 Feb 2020 15:03:08 +0000
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Wed, 5 Feb 2020 11:16:53 +0100
> Tomas Novotny <tomas@xxxxxxxxxx> wrote:
>
> > Proximity integration time handling and available values are added. The
> > integration time affects sampling rate, so it is computed now. The
> > settings should be changed only on the disabled channel. The check is
> > performed and user is notified in case of enabled channel. The helper
> > function is added as it will be used also for other settings.
> >
> > Integration time values are taken from the Vishay's documents "Designing
> > the VCNL4200 Into an Application" from Oct-2019 and "Designing the
> > VCNL4040 Into an Application" from Nov-2019.
> >
> > Duty ratio will be made configurable in the next patch.
> A few comments inline.
>
> Jonathan
>
> >
> > Signed-off-by: Tomas Novotny <tomas@xxxxxxxxxx>
> > ---
> > drivers/iio/light/vcnl4000.c | 188 +++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 183 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index f351b100a165..0bad082d762d 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -60,6 +60,8 @@
> >
> > /* Bit masks for various configuration registers */
> > #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_SD BIT(0) /* Proximity shutdown */
> >
> > enum vcnl4000_device_ids {
> > @@ -74,6 +76,9 @@ struct vcnl4200_channel {
> > ktime_t last_measurement;
> > ktime_t sampling_rate;
> > struct mutex lock;
> > + const int *int_time;
> > + size_t int_time_size;
> > + int ps_duty_ratio; /* Proximity specific */
> > };
> >
> > struct vcnl4000_data {
> > @@ -100,6 +105,10 @@ struct vcnl4000_chip_spec {
> > int *val);
> > int (*enable)(struct vcnl4000_data *data, enum iio_chan_type type,
> > bool val);
> > + int (*get_int_time)(struct vcnl4000_data *data, enum iio_chan_type type,
> > + int *val, int *val2);
> > + int (*set_int_time)(struct vcnl4000_data *data, enum iio_chan_type type,
> > + int val, int val2);
> > };
> >
> > static const struct i2c_device_id vcnl4000_id[] = {
> > @@ -132,10 +141,36 @@ static const struct iio_chan_spec vcnl4200_channels[] = {
> > }, {
> > .type = IIO_PROXIMITY,
> > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > - BIT(IIO_CHAN_INFO_ENABLE),
> > + BIT(IIO_CHAN_INFO_ENABLE) |
> > + BIT(IIO_CHAN_INFO_INT_TIME),
> > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> > }
> > };
> >
> > +/* Values are directly mapped to register values. */
> > +/* Integration time in us; 1T, 1.5T, 2T, 2.5T, 3T, 3.5T, 4T, 8T */
> > +static const int vcnl4040_ps_int_time[] = {
> > + 0, 125,
> > + 0, 188, /* 187.5 */
> > + 0, 250,
> > + 0, 313, /* 312.5 */
> > + 0, 375,
> > + 0, 438, /* 437.5 */
> > + 0, 500,
> > + 0, 1000
> > +};
> > +
> > +/* Values are directly mapped to register values. */
> > +/* Integration time in us; 1T, 1.5T, 2T, 4T, 8T, 9T */
> > +static const int vcnl4200_ps_int_time[] = {
> > + 0, 30,
> > + 0, 45,
> > + 0, 60,
> > + 0, 120,
> > + 0, 240,
> > + 0, 270
> > +};
> > +
> > static const struct regmap_config vcnl4000_regmap_config = {
> > .reg_bits = 8,
> > .val_bits = 8,
> > @@ -179,6 +214,34 @@ static int vcnl4000_init(struct vcnl4000_data *data)
> > return 0;
> > };
> >
> > +static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
> > + enum iio_chan_type type)
> > +{
> > + int ret;
> > + int it_val, it_val2;
> > + int duty_ratio;
> > +
> > + switch (type) {
> > + case IIO_PROXIMITY:
> > + ret = data->chip_spec->get_int_time(data, IIO_PROXIMITY,
> > + &it_val, &it_val2);
> > + if (ret < 0)
> > + return ret;
> > +
> > + duty_ratio = data->vcnl4200_ps.ps_duty_ratio;
> > + /*
> > + * Integration time multiplied by duty ratio.
> > + * Add 20% of part to part tolerance.
> > + */
> > + data->vcnl4200_ps.sampling_rate =
>
> sampling_period perhaps? It's a time rather than a rate (so many per second).
well, yes. This attribute was introduced in the past. Should I prepend
cleanup patch at the beginning of this series?
> > + ktime_set(((it_val * duty_ratio) * 6) / 5,
> > + (((it_val2 * duty_ratio) * 6) / 5) * 1000);
> > + return 0;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > static int vcnl4200_init(struct vcnl4000_data *data)
> > {
> > int ret, id;
> > @@ -218,18 +281,25 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> > case VCNL4200_PROD_ID:
> > /* Default wait time is 50ms, add 20% tolerance. */
> > data->vcnl4200_al.sampling_rate = ktime_set(0, 60000 * 1000);
> > - /* Default wait time is 4.8ms, add 20% tolerance. */
> > - data->vcnl4200_ps.sampling_rate = ktime_set(0, 5760 * 1000);
> > + 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->al_scale = 24000;
> > break;
> > case VCNL4040_PROD_ID:
> > /* Default wait time is 80ms, add 20% tolerance. */
> > data->vcnl4200_al.sampling_rate = ktime_set(0, 96000 * 1000);
> > - /* Default wait time is 5ms, add 20% tolerance. */
> > - data->vcnl4200_ps.sampling_rate = ktime_set(0, 6000 * 1000);
> > + 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->al_scale = 120000;
> > break;
> > }
> > + ret = vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
> > + if (ret < 0)
> > + return ret;
> > data->vcnl4200_al.last_measurement = ktime_set(0, 0);
> > data->vcnl4200_ps.last_measurement = ktime_set(0, 0);
> > mutex_init(&data->vcnl4200_al.lock);
> > @@ -368,6 +438,80 @@ static int vcnl4200_enable(struct vcnl4000_data *data, enum iio_chan_type type,
> > }
> > }
> >
> > +static int vcnl4200_check_enabled(struct vcnl4000_data *data,
> > + enum iio_chan_type type)
> > +{
> > + int ret, val;
> > +
> > + ret = data->chip_spec->is_enabled(data, type, &val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (val) {
> > + dev_warn(&data->client->dev,
> > + "Channel is enabled. Parameter cannot be changed.\n");
>
> For a basic parameter like this, userspace isn't going to typically
> expect that it can't be changed live. Hence you should be looking
> to go through a disable / modify / enable sequence.
ok, I will handle it.
> > + return -EBUSY;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int vcnl4200_get_int_time(struct vcnl4000_data *data,
> > + enum iio_chan_type type, int *val, int *val2)
> > +{
> > + int ret;
> > + unsigned int reg;
> > +
> > + switch (type) {
> > + case IIO_PROXIMITY:
> > + ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, ®);
> > + if (ret < 0)
> > + return ret;
> > +
> > + reg &= VCNL4200_PS_IT_MASK;
> > + reg >>= VCNL4200_PS_IT_SHIFT;
> > +
> > + *val = data->vcnl4200_ps.int_time[reg * 2];
> > + *val2 = data->vcnl4200_ps.int_time[reg * 2 + 1];
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +
> > +static int vcnl4200_set_int_time(struct vcnl4000_data *data,
> > + enum iio_chan_type type, int val, int val2)
> > +{
> > + int i, ret;
> > +
> > + ret = vcnl4200_check_enabled(data, type);
> > + if (ret < 0)
> > + return ret;
> > +
> > + switch (type) {
>
> This check is rather paranoid as it shouldn't be possible to get here
> without this being IIO_PROXIMITY.
I made it confusing. The switch was added as a skeleton because of int. time
for ambient light. I will drop it completely (including the "check effect").
> > + case IIO_PROXIMITY:
> > + for (i = 0; i < data->vcnl4200_ps.int_time_size; i += 2) {
> > + if (val == data->vcnl4200_ps.int_time[i] &&
> > + data->vcnl4200_ps.int_time[i + 1] == val2) {
>
> > + ret = regmap_update_bits(data->regmap,
> > + VCNL4200_PS_CONF1,
> > + VCNL4200_PS_IT_MASK,
> > + (i / 2) <<
> > + VCNL4200_PS_IT_SHIFT);
> > + if (ret < 0)
> > + return ret;
> > + return vcnl4200_set_samp_rate(data,
> > + IIO_PROXIMITY);
> > + }
> > + }
> > + break;
>
> return -EINVAL here and no need for break or handling below.
>
> > + default:
> > + return -EINVAL;
> > + }
> > + return -EINVAL;
> > +}
> > +
> > static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > [VCNL4000] = {
> > .prod = "VCNL4000",
> > @@ -397,6 +541,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > .measure_proximity = vcnl4200_measure_proximity,
> > .is_enabled = vcnl4200_is_enabled,
> > .enable = vcnl4200_enable,
> > + .get_int_time = vcnl4200_get_int_time,
> > + .set_int_time = vcnl4200_set_int_time,
> > },
>
> We now support more devices in this driver.
You mean that vcnl4000 - vcnl4020 are missing it? There is no integration
time for them. It is guarded by different iio_chan_spec.
Thanks,
Tomas
> > [VCNL4200] = {
> > .prod = "VCNL4200",
> > @@ -408,6 +554,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > .measure_proximity = vcnl4200_measure_proximity,
> > .is_enabled = vcnl4200_is_enabled,
> > .enable = vcnl4200_enable,
> > + .get_int_time = vcnl4200_get_int_time,
> > + .set_int_time = vcnl4200_set_int_time,
> > },
> > };
> >
> > @@ -443,6 +591,32 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
> > return IIO_VAL_INT_PLUS_MICRO;
> > case IIO_CHAN_INFO_ENABLE:
> > return data->chip_spec->is_enabled(data, chan->type, val);
> > + case IIO_CHAN_INFO_INT_TIME:
> > + return data->chip_spec->get_int_time(data, chan->type,
> > + val, val2);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int vcnl4000_read_avail(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + const int **vals, int *type, int *length,
> > + long mask)
> > +{
> > + struct vcnl4000_data *data = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_INT_TIME:
> > + switch (chan->type) {
> > + case IIO_PROXIMITY:
> > + *vals = data->vcnl4200_ps.int_time;
> > + *length = data->vcnl4200_ps.int_time_size;
> > + *type = IIO_VAL_INT_PLUS_MICRO;
> > + return IIO_AVAIL_LIST;
> > + default:
> > + return -EINVAL;
> > + }
> > default:
> > return -EINVAL;
> > }
> > @@ -457,6 +631,9 @@ static int vcnl4000_write_raw(struct iio_dev *indio_dev,
> > switch (mask) {
> > case IIO_CHAN_INFO_ENABLE:
> > return data->chip_spec->enable(data, chan->type, val);
> > + case IIO_CHAN_INFO_INT_TIME:
> > + return data->chip_spec->set_int_time(data, chan->type,
> > + val, val2);
> > default:
> > return -EINVAL;
> > }
> > @@ -464,6 +641,7 @@ static int vcnl4000_write_raw(struct iio_dev *indio_dev,
> >
> > static const struct iio_info vcnl4000_info = {
> > .read_raw = vcnl4000_read_raw,
> > + .read_avail = vcnl4000_read_avail,
> > .write_raw = vcnl4000_write_raw,
> > };
> >
>
