| Message ID | 20210216145346.18304-2-i.mikhaylov@yadro.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | add proximity rate option for vcnl3020 | expand |
On Tue, 16 Feb 2021 17:53:45 +0300 Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote: > Add the proximity rate optional option and handling of it for > vishay vcnl3020. > > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com> 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 > --- > 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! > +{ > + 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. > + 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 = {
On Sun, 2021-02-21 at 15:20 +0000, Jonathan Cameron wrote: > On Tue, 16 Feb 2021 17:53:45 +0300 > Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote: > > > Add the proximity rate optional option and handling of it for > > vishay vcnl3020. > > > > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com> > 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 = {
On Sun, 2021-02-21 at 22:34 +0200, Andy Shevchenko wrote: > > > On Tuesday, February 16, 2021, Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote: > > Add the proximity rate optional option and handling of it for > > vishay vcnl3020. > > > > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com> > > --- > > 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}, > > Can you confirm that’s the correct value and shan’t be 953125? Yes, It is described in the documentation for vcnl3020, also vcnl4000.c driver has same sampling frequency values for vcnl4020. > > + {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; > > }; > >
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) +{ + 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; + 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; + } + + rc = vcnl3020_get_and_apply_property(data, + vcnl3020_proximity_rate_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]) { + index = i; + break; + } + } + + 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: + 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 = {
Add the proximity rate optional option and handling of it for vishay vcnl3020. Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com> --- drivers/iio/proximity/vcnl3020.c | 123 ++++++++++++++++++++++++++++++- 1 file changed, 120 insertions(+), 3 deletions(-)