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).
> + 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.
> + 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.
> + 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.
> [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,
> };
>
