On Mon, 21 Jun 2021 17:30:51 +0300
Ivan Mikhaylov <i.mikhaylov@xxxxxxxxx> wrote:
> Add the low/high threshold options.
>
> Signed-off-by: Ivan Mikhaylov <i.mikhaylov@xxxxxxxxx>
Hi,
A few comments inline.
Thanks,
Jonathan
> ---
> drivers/iio/proximity/vcnl3020.c | 95 ++++++++++++++++++++++++++++++++
> 1 file changed, 95 insertions(+)
>
> diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c
> index 2e65127d5359..f3320de014e4 100644
> --- a/drivers/iio/proximity/vcnl3020.c
> +++ b/drivers/iio/proximity/vcnl3020.c
> @@ -255,6 +255,91 @@ static bool vcnl3020_is_thr_enabled(struct vcnl3020_data *data)
> return !!(icr & VCNL_ICR_THRES_EN);
> }
>
> +static int vcnl3020_read_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + int rc;
> + struct vcnl3020_data *data = iio_priv(indio_dev);
> + __be16 res;
> +
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + rc = regmap_bulk_read(data->regmap, VCNL_PS_HI_THR_HI,
> + &res, sizeof(res));
> + if (rc < 0)
> + return rc;
> + *val = be16_to_cpu(res);
> + return IIO_VAL_INT;
> + case IIO_EV_DIR_FALLING:
> + rc = regmap_bulk_read(data->regmap, VCNL_PS_LO_THR_HI,
> + &res, sizeof(res));
> + if (rc < 0)
> + return rc;
> + *val = be16_to_cpu(res);
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int vcnl3020_write_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + int rc;
> + __be16 buf;
> + struct vcnl3020_data *data = iio_priv(indio_dev);
> +
> + rc = iio_device_claim_direct_mode(indio_dev);
Why?
The intent of that function is to protect against mode transitions, so we
can't move from sysfs type captures to interrupt driven ones whilst
a sysfs read is in progress. That's not the case here (or in the existing
driver where this is used). So I think what you really want is a locally
defined lock that allows you to ensure device state is consistent if
you have any read / modify / write cycles.
For these particular registers I'm not even seeing that. Regmap has it's
own locking to avoid concurrency issues inside it's functions, so I'm not
sure you need a lock at all.
> + if (rc)
> + return rc;
> +
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + /* 16 bit word/ low * high */
> + buf = cpu_to_be16(val);
> + rc = regmap_bulk_write(data->regmap, VCNL_PS_HI_THR_HI,
> + &buf, sizeof(buf));
> + if (rc < 0)
> + goto out_release_direct_mode;
> + rc = IIO_VAL_INT;
> + goto out_release_direct_mode;
> + case IIO_EV_DIR_FALLING:
> + buf = cpu_to_be16(val);
> + rc = regmap_bulk_write(data->regmap, VCNL_PS_LO_THR_HI,
> + &buf, sizeof(buf));
> + if (rc < 0)
> + goto out_release_direct_mode;
> + rc = IIO_VAL_INT;
> + goto out_release_direct_mode;
> + default:
> + rc = -EINVAL;
> + goto out_release_direct_mode;
> + }
> + default:
> + rc = -EINVAL;
> + goto out_release_direct_mode;
> + }
> +out_release_direct_mode:
> + iio_device_release_direct_mode(indio_dev);
> +
> + return rc;
> +}
> +
> static int vcnl3020_enable_periodic(struct iio_dev *indio_dev,
> struct vcnl3020_data *data)
> {
> @@ -356,6 +441,14 @@ static int vcnl3020_read_event_config(struct iio_dev *indio_dev,
>
> static const struct iio_event_spec vcnl3020_event_spec[] = {
> {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE),
> + }, {
> .type = IIO_EV_TYPE_THRESH,
> .dir = IIO_EV_DIR_EITHER,
> .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> @@ -445,6 +538,8 @@ static const struct iio_info vcnl3020_info = {
> .read_raw = vcnl3020_read_raw,
> .write_raw = vcnl3020_write_raw,
> .read_avail = vcnl3020_read_avail,
> + .read_event_value = vcnl3020_read_event,
> + .write_event_value = vcnl3020_write_event,
> .read_event_config = vcnl3020_read_event_config,
> .write_event_config = vcnl3020_write_event_config,
> };
