Re: [PATCH 1/2] iio: proximity: vcnl3020: add proximity rate
From: Ivan Mikhaylov
Date: Wed Feb 24 2021 - 10:41:09 EST
On Sun, 2021年02月21日 at 15:20 +0000, Jonathan Cameron wrote:
>
On 2021年2月16日 17:53:45 +0300
>
Ivan Mikhaylov <i.mikhaylov@xxxxxxxxx> wrote:
>
>
> Add the proximity rate optional option and handling of it for
>
> vishay vcnl3020.
>
>
>
> Signed-off-by: Ivan Mikhaylov <i.mikhaylov@xxxxxxxxx>
>
Hi Ivan,
>
>
Other than dropping the dt part this mostly looks fine.
>
Please also implement the read_avail callback to let userspace know the
>
valid set of values.
>
>
Thanks
>
>
Jonathan
>
Ok, I'll get rid of dt part then. Thanks for review.
>
> ---
>
> drivers/iio/proximity/vcnl3020.c | 123 ++++++++++++++++++++++++++++++-
>
> 1 file changed, 120 insertions(+), 3 deletions(-)
>
>
>
> diff --git a/drivers/iio/proximity/vcnl3020.c
>
> b/drivers/iio/proximity/vcnl3020.c
>
> index 37264f801ad0..833c5d5ac0a1 100644
>
> --- a/drivers/iio/proximity/vcnl3020.c
>
> +++ b/drivers/iio/proximity/vcnl3020.c
>
> @@ -40,6 +40,17 @@
>
> #define VCNL_ON_DEMAND_TIMEOUT_US 100000
>
> #define VCNL_POLL_US 20000
>
>
>
> +static const int vcnl3020_prox_sampling_frequency[][2] = {
>
> + {1, 950000},
>
> + {3, 906250},
>
> + {7, 812500},
>
> + {16, 625000},
>
> + {31, 250000},
>
> + {62, 500000},
>
> + {125, 0},
>
> + {250, 0},
>
> +};
>
> +
>
> /**
>
> * struct vcnl3020_data - vcnl3020 specific data.
>
> * @regmap: device register map.
>
> @@ -75,12 +86,37 @@ static u32 microamp_to_reg(u32 *val)
>
> return *val /= 10000;
>
> };
>
>
>
> +static u32 hz_to_reg(u32 *val)
>
Hmm. This is rather odd in the existing driver. It makes no sense
>
to have callbacks like this that both modify the value passed by pointer
>
and return it.
>
>
Much cleaner to just pass by value and make caller do the assignment.
>
>
Given I've suggested you drop this anyway probably not that important!
Good point anyways.
>
> +{
>
> + unsigned int i;
>
> + int index = -1;
>
> +
>
> + for (i = 0; i < ARRAY_SIZE(vcnl3020_prox_sampling_frequency); i++) {
>
> + if (*val == vcnl3020_prox_sampling_frequency[i][0]) {
>
> + index = i;
>
>
return i;
>
>
> + break;
>
> + }
>
> + }
>
> +
>
> + *val = index;
>
> + if (index < 0)
>
> + *val = 0;
>
> +
>
> + return *val;
>
> +};
>
> +
>
> static struct vcnl3020_property vcnl3020_led_current_property = {
>
> .name = "vishay,led-current-microamp",
>
> .reg = VCNL_LED_CURRENT,
>
> .conversion_func = microamp_to_reg,
>
> };
>
>
>
> +static struct vcnl3020_property vcnl3020_proximity_rate_property = {
>
> + .name = "vishay,proximity-rate-hz",
>
> + .reg = VCNL_PROXIMITY_RATE,
>
> + .conversion_func = hz_to_reg,
>
> +};
>
> +
>
> static int vcnl3020_get_and_apply_property(struct vcnl3020_data *data,
>
> struct vcnl3020_property prop)
>
> {
>
> @@ -125,8 +161,18 @@ static int vcnl3020_init(struct vcnl3020_data *data)
>
> data->rev = reg;
>
> mutex_init(&data->lock);
>
>
>
> - return vcnl3020_get_and_apply_property(data,
>
> - vcnl3020_led_current_property);
>
> + rc = vcnl3020_get_and_apply_property(data,
>
> + vcnl3020_led_current_property);
>
> + if (rc) {
>
> + goto err_prop_set;
>
>
Kernel style has not brackets around single line blocks like this.
>
if (rc)
>
goto err_prop_set;
>
>
> + }
>
> +
>
> + rc = vcnl3020_get_and_apply_property(data,
>
> + vcnl3020_proximity_rate_property);
>
>
From review of binding doc, I don't think this makes a much sense as a
>
dt property.
>
>
> +
>
> +err_prop_set:
>
> +
>
> + return rc;
>
> };
>
>
>
> static int vcnl3020_measure_proximity(struct vcnl3020_data *data, int *val)
>
> @@ -165,10 +211,50 @@ static int vcnl3020_measure_proximity(struct
>
> vcnl3020_data *data, int *val)
>
> return rc;
>
> }
>
>
>
> +static int vcnl3020_read_proxy_samp_freq(struct vcnl3020_data *data, int
>
> *val,
>
> + int *val2)
>
> +{
>
> + int rc;
>
> + unsigned int prox_rate;
>
> +
>
> + rc = regmap_read(data->regmap, VCNL_PROXIMITY_RATE, &prox_rate);
>
> + if (rc)
>
> + return rc;
>
> +
>
> + if (prox_rate >= ARRAY_SIZE(vcnl3020_prox_sampling_frequency))
>
> + return -EINVAL;
>
> +
>
> + *val = vcnl3020_prox_sampling_frequency[prox_rate][0];
>
> + *val2 = vcnl3020_prox_sampling_frequency[prox_rate][1];
>
> +
>
> + return 0;
>
> +}
>
> +
>
> +static int vcnl3020_write_proxy_samp_freq(struct vcnl3020_data *data, int
>
> val,
>
> + int val2)
>
> +{
>
> + unsigned int i;
>
> + int index = -1;
>
> +
>
> + for (i = 0; i < ARRAY_SIZE(vcnl3020_prox_sampling_frequency); i++) {
>
> + if (val == vcnl3020_prox_sampling_frequency[i][0] &&
>
> + val2 == vcnl3020_prox_sampling_frequency[i][1]) {
>
return regmap_write(data->regmap,
>
VCNL_PROXIMITY_RATE, index);
>
>
> + index = i;
>
> + break;
>
> + }
>
> + }
>
>
return -EINVAL;
>
>
Would probably be easier to read.
>
> +
>
> + if (index < 0)
>
> + return -EINVAL;
>
> +
>
> + return regmap_write(data->regmap, VCNL_PROXIMITY_RATE, index);
>
> +}
>
> +
>
> static const struct iio_chan_spec vcnl3020_channels[] = {
>
> {
>
> .type = IIO_PROXIMITY,
>
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
>
> },
>
> };
>
>
>
> @@ -185,13 +271,44 @@ static int vcnl3020_read_raw(struct iio_dev
>
> *indio_dev,
>
> if (rc)
>
> return rc;
>
> return IIO_VAL_INT;
>
> + case IIO_CHAN_INFO_SAMP_FREQ:
>
> + rc = vcnl3020_read_proxy_samp_freq(data, val, val2);
>
> + if (rc < 0)
>
> + return rc;
>
> + return IIO_VAL_INT_PLUS_MICRO;
>
> default:
>
> return -EINVAL;
>
> }
>
> }
>
>
>
> +static int vcnl3020_write_raw(struct iio_dev *indio_dev,
>
> + struct iio_chan_spec const *chan,
>
> + int val, int val2, long mask)
>
> +{
>
> + int rc;
>
> + struct vcnl3020_data *data = iio_priv(indio_dev);
>
> +
>
> + rc = iio_device_claim_direct_mode(indio_dev);
>
> + if (rc)
>
> + return rc;
>
> +
>
> + switch (mask) {
>
> + case IIO_CHAN_INFO_SAMP_FREQ:
>
>
You could simplify the flow by moving the iio_device_claim_direct_mode()
>
and iio_deivce_release_direct_mode() to this case statement. That way
>
the default path can return directly.
>
Yeap.
>
> + rc = vcnl3020_write_proxy_samp_freq(data, val, val2);
>
> + goto end;
>
> + default:
>
> + rc = -EINVAL;
>
> + goto end;
>
> + }
>
> +
>
> +end:
>
> + iio_device_release_direct_mode(indio_dev);
>
> + return rc;
>
> +}
>
> +
>
> static const struct iio_info vcnl3020_info = {
>
> .read_raw = vcnl3020_read_raw,
>
> + .write_raw = vcnl3020_write_raw,
>
> };
>
>
>
> static const struct regmap_config vcnl3020_regmap_config = {