[PATCH 0/4] add periodic mode, threshold options and hwmon

[PATCH 0/4] add periodic mode, threshold options and hwmon

[Ivan Mikhaylov]

Add periodic mode enablement, high/low threshold options. Remove mutex
from vcnl3020_data structure, change mutex_lock/unlock's appropriately on
iio_device_claim/release_direct_mode.

Add hwmon driver as intrusion sensor which provides information about
any triggers within thresholds, works in periodic mode of proximity
sensor.

Ivan Mikhaylov (4):
  iio: proximity: vcnl3020: add periodic mode
  iio: proximity: vcnl3020: add threshold options
  iio: proximity: vncl3020: remove mutex from vcnl3020_data
  hwmon: vcnl3020: add hwmon driver for intrusion sensor

 drivers/hwmon/Kconfig                  |   7 +
 drivers/hwmon/Makefile                 |   1 +
 drivers/hwmon/vcnl3020-hwmon.c         |  57 +++++
 drivers/iio/proximity/vcnl3020.c       | 335 ++++++++++++++++++++++---
 include/linux/iio/proximity/vcnl3020.h |  26 ++
 5 files changed, 395 insertions(+), 31 deletions(-)
 create mode 100644 drivers/hwmon/vcnl3020-hwmon.c
 create mode 100644 include/linux/iio/proximity/vcnl3020.h

-- 
2.26.3

[PATCH 1/4] iio: proximity: vcnl3020: add periodic mode

[Ivan Mikhaylov]

Add the possibility to run proximity sensor in periodic measurement
mode.

Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
---
 drivers/iio/proximity/vcnl3020.c | 138 +++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)

diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c
index 43817f6b3086..25c6bdba3ede 100644
--- a/drivers/iio/proximity/vcnl3020.c
+++ b/drivers/iio/proximity/vcnl3020.c
@@ -36,6 +36,21 @@
 #define VCNL_PS_OD		BIT(3) /* start on-demand proximity
 					* measurement
 					*/
+#define VCNL_PS_EN		BIT(1) /* Enables periodic proximity
+					* measurement
+					*/
+#define VCNL_PS_SELFTIMED_EN	BIT(0) /* Enables state machine and LP
+					* oscillator for self timed
+					* measurements
+					*/
+
+/* Bit masks for ICR */
+#define  VCNL_ICR_THRES_EN	BIT(1) /* Enable interrupts on low or high
+					* thresholds */
+
+/* Bit masks for ISR */
+#define VCNL_INT_TH_HI		BIT(0)	/* High threshold hit */
+#define VCNL_INT_TH_LOW		BIT(1)	/* Low threshold hit */
 
 #define VCNL_ON_DEMAND_TIMEOUT_US	100000
 #define VCNL_POLL_US			20000
@@ -215,12 +230,124 @@ static int vcnl3020_write_proxy_samp_freq(struct vcnl3020_data *data, int val,
 	return regmap_write(data->regmap, VCNL_PROXIMITY_RATE, index);
 }
 
+static bool vcnl3020_is_in_periodic_mode(struct vcnl3020_data *data)
+{
+	int rc;
+	unsigned int cmd;
+
+	rc = regmap_read(data->regmap, VCNL_COMMAND, &cmd);
+	if (rc)
+		return false;
+
+	return !!(cmd & VCNL_PS_SELFTIMED_EN);
+}
+
+static bool vcnl3020_is_thr_enabled(struct vcnl3020_data *data)
+{
+	int rc;
+	unsigned int icr;
+
+	rc = regmap_read(data->regmap, VCNL_PS_ICR, &icr);
+	if (rc)
+		return false;
+
+	return !!(icr & VCNL_ICR_THRES_EN);
+}
+
+static int vcnl3020_config_threshold(struct iio_dev *indio_dev, bool state)
+{
+	struct vcnl3020_data *data = iio_priv(indio_dev);
+	int rc;
+	int icr;
+	int cmd;
+	int isr;
+
+	if (state) {
+		rc = iio_device_claim_direct_mode(indio_dev);
+		if (rc)
+			return rc;
+
+		/* Enable periodic measurement of proximity data. */
+		cmd = VCNL_PS_EN | VCNL_PS_SELFTIMED_EN;
+
+		/*
+		 * Enable interrupts on threshold, for proximity data by
+		 * default.
+		 */
+		icr = VCNL_ICR_THRES_EN;
+	} else {
+		if (!vcnl3020_is_thr_enabled(data))
+			return 0;
+
+		cmd = 0;
+		icr = 0;
+		isr = 0;
+	}
+
+	rc = regmap_write(data->regmap, VCNL_COMMAND, cmd);
+	if (rc)
+		goto end;
+
+	rc = regmap_write(data->regmap, VCNL_PS_ICR, icr);
+	if (rc)
+		goto end;
+
+	if (!state)
+		/* Clear interrupts */
+		rc = regmap_write(data->regmap, VCNL_ISR, isr);
+
+end:
+	if (state)
+		iio_device_release_direct_mode(indio_dev);
+
+	return rc;
+}
+
+static int vcnl3020_write_event_config(struct iio_dev *indio_dev,
+				       const struct iio_chan_spec *chan,
+				       enum iio_event_type type,
+				       enum iio_event_direction dir,
+				       int state)
+{
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		return vcnl3020_config_threshold(indio_dev, state);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int vcnl3020_read_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir)
+{
+	struct vcnl3020_data *data = iio_priv(indio_dev);
+
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		return vcnl3020_is_thr_enabled(data);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_event_spec vcnl3020_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
 static const struct iio_chan_spec vcnl3020_channels[] = {
 	{
 		.type = IIO_PROXIMITY,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
 		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.event_spec = vcnl3020_event_spec,
+		.num_event_specs = ARRAY_SIZE(vcnl3020_event_spec),
 	},
 };
 
@@ -233,6 +360,11 @@ static int vcnl3020_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
+
+		/* Protect against event capture. */
+		if (vcnl3020_is_in_periodic_mode(data))
+			return -EBUSY;
+
 		rc = vcnl3020_measure_proximity(data, val);
 		if (rc)
 			return rc;
@@ -254,6 +386,10 @@ static int vcnl3020_write_raw(struct iio_dev *indio_dev,
 	int rc;
 	struct vcnl3020_data *data = iio_priv(indio_dev);
 
+	/* Protect against event capture. */
+	if (vcnl3020_is_in_periodic_mode(data))
+		return -EBUSY;
+
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		rc = iio_device_claim_direct_mode(indio_dev);
@@ -287,6 +423,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_config = vcnl3020_read_event_config,
+	.write_event_config = vcnl3020_write_event_config,
 };
 
 static const struct regmap_config vcnl3020_regmap_config = {
-- 
2.26.3

Re: [PATCH 1/4] iio: proximity: vcnl3020: add periodic mode

[Andy Shevchenko]

On Fri, Apr 30, 2021 at 6:17 PM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:
>
> Add the possibility to run proximity sensor in periodic measurement
> mode.

...

> +static int vcnl3020_config_threshold(struct iio_dev *indio_dev, bool state)
> +{
> +       struct vcnl3020_data *data = iio_priv(indio_dev);
> +       int rc;
> +       int icr;
> +       int cmd;
> +       int isr;
> +
> +       if (state) {
> +               rc = iio_device_claim_direct_mode(indio_dev);
> +               if (rc)
> +                       return rc;
> +
> +               /* Enable periodic measurement of proximity data. */
> +               cmd = VCNL_PS_EN | VCNL_PS_SELFTIMED_EN;
> +
> +               /*
> +                * Enable interrupts on threshold, for proximity data by
> +                * default.
> +                */
> +               icr = VCNL_ICR_THRES_EN;
> +       } else {
> +               if (!vcnl3020_is_thr_enabled(data))
> +                       return 0;
> +
> +               cmd = 0;
> +               icr = 0;
> +               isr = 0;
> +       }
> +
> +       rc = regmap_write(data->regmap, VCNL_COMMAND, cmd);
> +       if (rc)
> +               goto end;
> +
> +       rc = regmap_write(data->regmap, VCNL_PS_ICR, icr);
> +       if (rc)
> +               goto end;
> +
> +       if (!state)
> +               /* Clear interrupts */
> +               rc = regmap_write(data->regmap, VCNL_ISR, isr);
> +
> +end:
> +       if (state)
> +               iio_device_release_direct_mode(indio_dev);
> +
> +       return rc;
> +}

The code will benefit in case you split above to two helpers, i.e.
_on() and _off().
It will gain better readability.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] iio: proximity: vcnl3020: add periodic mode

[Jonathan Cameron]

On Fri, 30 Apr 2021 18:24:16 +0300
Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:

> Add the possibility to run proximity sensor in periodic measurement
> mode.

Without an interrupt?  Unusual and perhaps best left to userspace.

> 
> Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
> ---
>  drivers/iio/proximity/vcnl3020.c | 138 +++++++++++++++++++++++++++++++
>  1 file changed, 138 insertions(+)
> 
> diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c
> index 43817f6b3086..25c6bdba3ede 100644
> --- a/drivers/iio/proximity/vcnl3020.c
> +++ b/drivers/iio/proximity/vcnl3020.c
> @@ -36,6 +36,21 @@
>  #define VCNL_PS_OD		BIT(3) /* start on-demand proximity
>  					* measurement
>  					*/
> +#define VCNL_PS_EN		BIT(1) /* Enables periodic proximity
> +					* measurement
> +					*/
> +#define VCNL_PS_SELFTIMED_EN	BIT(0) /* Enables state machine and LP
> +					* oscillator for self timed
> +					* measurements

This rather suggests that you should just put comments on their own lines
as it will involve less wrapping and ultimately be more compact and readable!

> +					*/
> +
> +/* Bit masks for ICR */
> +#define  VCNL_ICR_THRES_EN	BIT(1) /* Enable interrupts on low or high
> +					* thresholds */
> +
> +/* Bit masks for ISR */
> +#define VCNL_INT_TH_HI		BIT(0)	/* High threshold hit */
> +#define VCNL_INT_TH_LOW		BIT(1)	/* Low threshold hit */
>  
>  #define VCNL_ON_DEMAND_TIMEOUT_US	100000
>  #define VCNL_POLL_US			20000
> @@ -215,12 +230,124 @@ static int vcnl3020_write_proxy_samp_freq(struct vcnl3020_data *data, int val,
>  	return regmap_write(data->regmap, VCNL_PROXIMITY_RATE, index);
>  }
>  
> +static bool vcnl3020_is_in_periodic_mode(struct vcnl3020_data *data)
> +{
> +	int rc;
> +	unsigned int cmd;
> +
> +	rc = regmap_read(data->regmap, VCNL_COMMAND, &cmd);
> +	if (rc)
> +		return false;
> +
> +	return !!(cmd & VCNL_PS_SELFTIMED_EN);
> +}
> +
> +static bool vcnl3020_is_thr_enabled(struct vcnl3020_data *data)
> +{
> +	int rc;
> +	unsigned int icr;
> +
> +	rc = regmap_read(data->regmap, VCNL_PS_ICR, &icr);
> +	if (rc)
> +		return false;
> +
> +	return !!(icr & VCNL_ICR_THRES_EN);
> +}
> +
> +static int vcnl3020_config_threshold(struct iio_dev *indio_dev, bool state)
> +{
> +	struct vcnl3020_data *data = iio_priv(indio_dev);
> +	int rc;
> +	int icr;
> +	int cmd;
> +	int isr;
> +
> +	if (state) {
> +		rc = iio_device_claim_direct_mode(indio_dev);

Device doesn't support buffered mode, so this is a lock as in patch 3.
Don't do that as that isn't the intended use

> +		if (rc)
> +			return rc;
> +
> +		/* Enable periodic measurement of proximity data. */
> +		cmd = VCNL_PS_EN | VCNL_PS_SELFTIMED_EN;
> +
> +		/*
> +		 * Enable interrupts on threshold, for proximity data by
> +		 * default.
> +		 */
> +		icr = VCNL_ICR_THRES_EN;
> +	} else {
> +		if (!vcnl3020_is_thr_enabled(data))
> +			return 0;
> +
> +		cmd = 0;
> +		icr = 0;
> +		isr = 0;
> +	}
> +
> +	rc = regmap_write(data->regmap, VCNL_COMMAND, cmd);
> +	if (rc)
> +		goto end;
> +
> +	rc = regmap_write(data->regmap, VCNL_PS_ICR, icr);
> +	if (rc)
> +		goto end;
> +
> +	if (!state)
> +		/* Clear interrupts */

Given you don't seem to have an interrupt. I guess this is clearing
a status flag?

> +		rc = regmap_write(data->regmap, VCNL_ISR, isr);
> +
> +end:
> +	if (state)
> +		iio_device_release_direct_mode(indio_dev);

I would just allow for the small amount of repeated code and do only
one condition on if (state) in this function.

> +
> +	return rc;
> +}
> +
> +static int vcnl3020_write_event_config(struct iio_dev *indio_dev,
> +				       const struct iio_chan_spec *chan,
> +				       enum iio_event_type type,
> +				       enum iio_event_direction dir,
> +				       int state)
> +{
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		return vcnl3020_config_threshold(indio_dev, state);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int vcnl3020_read_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir)
> +{
> +	struct vcnl3020_data *data = iio_priv(indio_dev);
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		return vcnl3020_is_thr_enabled(data);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_event_spec vcnl3020_event_spec[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_EITHER,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
>  static const struct iio_chan_spec vcnl3020_channels[] = {
>  	{
>  		.type = IIO_PROXIMITY,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.event_spec = vcnl3020_event_spec,
> +		.num_event_specs = ARRAY_SIZE(vcnl3020_event_spec),
>  	},
>  };
>  
> @@ -233,6 +360,11 @@ static int vcnl3020_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +
> +		/* Protect against event capture. */
> +		if (vcnl3020_is_in_periodic_mode(data))
> +			return -EBUSY;
> +
>  		rc = vcnl3020_measure_proximity(data, val);
>  		if (rc)
>  			return rc;
> @@ -254,6 +386,10 @@ static int vcnl3020_write_raw(struct iio_dev *indio_dev,
>  	int rc;
>  	struct vcnl3020_data *data = iio_priv(indio_dev);
>  
> +	/* Protect against event capture. */
> +	if (vcnl3020_is_in_periodic_mode(data))
> +		return -EBUSY;
> +
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		rc = iio_device_claim_direct_mode(indio_dev);
> @@ -287,6 +423,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_config = vcnl3020_read_event_config,
> +	.write_event_config = vcnl3020_write_event_config,
>  };
>  
>  static const struct regmap_config vcnl3020_regmap_config = {

Re: [PATCH 1/4] iio: proximity: vcnl3020: add periodic mode

[Ivan Mikhaylov]

On Sun, 2021-05-02 at 19:00 +0100, Jonathan Cameron wrote:
> On Fri, 30 Apr 2021 18:24:16 +0300
> Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:
> 
> > Add the possibility to run proximity sensor in periodic measurement
> > mode.
> 
> Without an interrupt?  Unusual and perhaps best left to userspace.

Do you mean without interrupt handler in driver for this particular interrupt?
If it's need to be added here, I can add it. In this patch I just added trigger
to enable/disable periodic measurement mode without interrupt handler.

> 
> > +		if (rc)
> > +			return rc;
> > +
> > +		/* Enable periodic measurement of proximity data. */
> > +		cmd = VCNL_PS_EN | VCNL_PS_SELFTIMED_EN;
> > +
> > +		/*
> > +		 * Enable interrupts on threshold, for proximity data by
> > +		 * default.
> > +		 */
> > +		icr = VCNL_ICR_THRES_EN;
> > +	} else {
> > +		if (!vcnl3020_is_thr_enabled(data))
> > +			return 0;
> > +
> > +		cmd = 0;
> > +		icr = 0;
> > +		isr = 0;
> > +	}
> > +
> > +	rc = regmap_write(data->regmap, VCNL_COMMAND, cmd);
> > +	if (rc)
> > +		goto end;
> > +
> > +	rc = regmap_write(data->regmap, VCNL_PS_ICR, icr);
> > +	if (rc)
> > +		goto end;
> > +
> > +	if (!state)
> > +		/* Clear interrupts */
> 
> Given you don't seem to have an interrupt. I guess this is clearing
> a status flag?

Yes, it is clearing flag in interrupt status register.

> 
> > +		rc = regmap_write(data->regmap, VCNL_ISR, isr);
> > +
> > +end:
> > +	if (state)
> > +		iio_device_release_direct_mode(indio_dev);
> 

Re: [PATCH 1/4] iio: proximity: vcnl3020: add periodic mode

[Jonathan Cameron]

On Tue, 4 May 2021 22:07:53 +0300
Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:

> On Sun, 2021-05-02 at 19:00 +0100, Jonathan Cameron wrote:
> > On Fri, 30 Apr 2021 18:24:16 +0300
> > Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:
> >   
> > > Add the possibility to run proximity sensor in periodic measurement
> > > mode.  
> > 
> > Without an interrupt?  Unusual and perhaps best left to userspace.  
> 
> Do you mean without interrupt handler in driver for this particular interrupt?
> If it's need to be added here, I can add it. In this patch I just added trigger
> to enable/disable periodic measurement mode without interrupt handler.

The model for events in IIO is that they are 'pushed' to userspace.
So it is possible to do this with a polling thread, but on a device
with an interrupt line tied to the event, it makes much more sense
to do it that way.

We don't have read on demand event as there has been little real
demand for them (and they can be implemented in userspase).

Jonathan


> 
> >   
> > > +		if (rc)
> > > +			return rc;
> > > +
> > > +		/* Enable periodic measurement of proximity data. */
> > > +		cmd = VCNL_PS_EN | VCNL_PS_SELFTIMED_EN;
> > > +
> > > +		/*
> > > +		 * Enable interrupts on threshold, for proximity data by
> > > +		 * default.
> > > +		 */
> > > +		icr = VCNL_ICR_THRES_EN;
> > > +	} else {
> > > +		if (!vcnl3020_is_thr_enabled(data))
> > > +			return 0;
> > > +
> > > +		cmd = 0;
> > > +		icr = 0;
> > > +		isr = 0;
> > > +	}
> > > +
> > > +	rc = regmap_write(data->regmap, VCNL_COMMAND, cmd);
> > > +	if (rc)
> > > +		goto end;
> > > +
> > > +	rc = regmap_write(data->regmap, VCNL_PS_ICR, icr);
> > > +	if (rc)
> > > +		goto end;
> > > +
> > > +	if (!state)
> > > +		/* Clear interrupts */  
> > 
> > Given you don't seem to have an interrupt. I guess this is clearing
> > a status flag?  
> 
> Yes, it is clearing flag in interrupt status register.
> 
> >   
> > > +		rc = regmap_write(data->regmap, VCNL_ISR, isr);
> > > +
> > > +end:
> > > +	if (state)
> > > +		iio_device_release_direct_mode(indio_dev);  
> >   
> 
> 

[PATCH 2/4] iio: proximity: vcnl3020: add threshold options

[Ivan Mikhaylov]

Add the low/high threshold options.

Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
---
 drivers/iio/proximity/vcnl3020.c | 94 ++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c
index 25c6bdba3ede..0dfa6a0b5eec 100644
--- a/drivers/iio/proximity/vcnl3020.c
+++ b/drivers/iio/proximity/vcnl3020.c
@@ -254,6 +254,90 @@ 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, 2);
+			*val = be16_to_cpu(res);
+			if (rc < 0)
+				return rc;
+			return IIO_VAL_INT;
+		case IIO_EV_DIR_FALLING:
+			rc = regmap_bulk_read(data->regmap, VCNL_PS_LO_THR_HI,
+					      &res, 2);
+			*val = be16_to_cpu(res);
+			if (rc < 0)
+				return rc;
+			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);
+	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, 2);
+			if (rc < 0)
+				goto end;
+			rc = IIO_VAL_INT;
+			goto end;
+		case IIO_EV_DIR_FALLING:
+			buf = cpu_to_be16(val);
+			rc = regmap_bulk_write(data->regmap, VCNL_PS_LO_THR_HI,
+					       &buf, 2);
+			if (rc < 0)
+				goto end;
+			rc = IIO_VAL_INT;
+			goto end;
+		default:
+			rc = -EINVAL;
+			goto end;
+		}
+	default:
+		rc = -EINVAL;
+		goto end;
+	}
+end:
+	iio_device_release_direct_mode(indio_dev);
+	return rc;
+}
+
 static int vcnl3020_config_threshold(struct iio_dev *indio_dev, bool state)
 {
 	struct vcnl3020_data *data = iio_priv(indio_dev);
@@ -334,6 +418,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),
@@ -423,6 +515,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,
 };
-- 
2.26.3

Re: [PATCH 2/4] iio: proximity: vcnl3020: add threshold options

[Andy Shevchenko]

On Fri, Apr 30, 2021 at 6:16 PM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:
>
> Add the low/high threshold options.

...

> +                       rc = regmap_bulk_read(data->regmap, VCNL_PS_HI_THR_HI,
> +                                             &res, 2);

sizeof(res)

> +                       *val = be16_to_cpu(res);

So, the rule of thumb is not putting anything to the output, until you
know that there is no error.

> +                       if (rc < 0)
> +                               return rc;

...

> +                       rc = regmap_bulk_read(data->regmap, VCNL_PS_LO_THR_HI,
> +                                             &res, 2);
> +                       *val = be16_to_cpu(res);
> +                       if (rc < 0)
> +                               return rc;

As per above.

...

> +                       rc = regmap_bulk_write(data->regmap, VCNL_PS_HI_THR_HI,
> +                                              &buf, 2);

sizeof(buf) ?

...

> +                       rc = regmap_bulk_write(data->regmap, VCNL_PS_LO_THR_HI,
> +                                              &buf, 2);

Ditto.

...

> +end:

out_release_direct_mode:

> +       iio_device_release_direct_mode(indio_dev);
> +       return rc;

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/4] iio: proximity: vncl3020: remove mutex from vcnl3020_data

[Ivan Mikhaylov]

Remove the mutex from vcnl3020_data structure and change it on
iio_device_claim_direct_mode/iio_device_release_direct_mode.

Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
---
 drivers/iio/proximity/vcnl3020.c | 40 ++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c
index 0dfa6a0b5eec..bff59c7af966 100644
--- a/drivers/iio/proximity/vcnl3020.c
+++ b/drivers/iio/proximity/vcnl3020.c
@@ -71,13 +71,11 @@ static const int vcnl3020_prox_sampling_frequency[][2] = {
  * @regmap:	device register map.
  * @dev:	vcnl3020 device.
  * @rev:	revision id.
- * @lock:	lock for protecting access to device hardware registers.
  */
 struct vcnl3020_data {
 	struct regmap *regmap;
 	struct device *dev;
 	u8 rev;
-	struct mutex lock;
 };
 
 /**
@@ -149,7 +147,6 @@ 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);
@@ -161,11 +158,9 @@ static int vcnl3020_measure_proximity(struct vcnl3020_data *data, int *val)
 	unsigned int reg;
 	__be16 res;
 
-	mutex_lock(&data->lock);
-
 	rc = regmap_write(data->regmap, VCNL_COMMAND, VCNL_PS_OD);
 	if (rc)
-		goto err_unlock;
+		return rc;
 
 	/* wait for data to become ready */
 	rc = regmap_read_poll_timeout(data->regmap, VCNL_COMMAND, reg,
@@ -174,20 +169,17 @@ static int vcnl3020_measure_proximity(struct vcnl3020_data *data, int *val)
 	if (rc) {
 		dev_err(data->dev,
 			"Error (%d) reading vcnl3020 command register\n", rc);
-		goto err_unlock;
+		return rc;
 	}
 
 	/* high & low result bytes read */
 	rc = regmap_bulk_read(data->regmap, VCNL_PS_RESULT_HI, &res,
 			      sizeof(res));
 	if (rc)
-		goto err_unlock;
+		return rc;
 
 	*val = be16_to_cpu(res);
 
-err_unlock:
-	mutex_unlock(&data->lock);
-
 	return rc;
 }
 
@@ -450,25 +442,37 @@ static int vcnl3020_read_raw(struct iio_dev *indio_dev,
 	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_RAW:
 
 		/* Protect against event capture. */
-		if (vcnl3020_is_in_periodic_mode(data))
-			return -EBUSY;
+		if (vcnl3020_is_in_periodic_mode(data)) {
+			rc = -EBUSY;
+			goto end;
+		}
 
 		rc = vcnl3020_measure_proximity(data, val);
 		if (rc)
-			return rc;
-		return IIO_VAL_INT;
+			goto end;
+		rc = IIO_VAL_INT;
+		goto end;
 	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;
+			goto end;
+		rc = IIO_VAL_INT_PLUS_MICRO;
+		goto end;
 	default:
-		return -EINVAL;
+		rc = -EINVAL;
 	}
+
+end:
+	iio_device_release_direct_mode(indio_dev);
+	return rc;
 }
 
 static int vcnl3020_write_raw(struct iio_dev *indio_dev,
-- 
2.26.3

Re: [PATCH 3/4] iio: proximity: vncl3020: remove mutex from vcnl3020_data

[Andy Shevchenko]

On Fri, Apr 30, 2021 at 6:16 PM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:
>
> Remove the mutex from vcnl3020_data structure and change it on
> iio_device_claim_direct_mode/iio_device_release_direct_mode.

iio_device_claim_direct_mode()/iio_device_release_direct_mode().

I'm wondering if it should be the first patch in the series.

...

> +end:

out_release_direct_mode:

> +       iio_device_release_direct_mode(indio_dev);
> +       return rc;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] iio: proximity: vncl3020: remove mutex from vcnl3020_data

[Jonathan Cameron]

On Fri, 30 Apr 2021 18:24:18 +0300
Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:

> Remove the mutex from vcnl3020_data structure and change it on
> iio_device_claim_direct_mode/iio_device_release_direct_mode.
> 
> Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
I'm not keen on doing this.  The reason is that
iio_device_claim_direct_mode() is today implemented via a mutex which could
be used as you have it here, but... It might not be in the future.
I can see we might for example optimize it to allow multiple concurrent
accesses that assume direct mode.

So don't make assumptions about what it does.  It should be used if
you are protecting against the device entering (or already being in) a buffered
mode, but nothing else.

If other scope is needed, then stick to a local lock.  It's perfectly
fine to claim direct mode and take a lock of course if you are protecting
against different things.

Jonathan

> ---
>  drivers/iio/proximity/vcnl3020.c | 40 ++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c
> index 0dfa6a0b5eec..bff59c7af966 100644
> --- a/drivers/iio/proximity/vcnl3020.c
> +++ b/drivers/iio/proximity/vcnl3020.c
> @@ -71,13 +71,11 @@ static const int vcnl3020_prox_sampling_frequency[][2] = {
>   * @regmap:	device register map.
>   * @dev:	vcnl3020 device.
>   * @rev:	revision id.
> - * @lock:	lock for protecting access to device hardware registers.
>   */
>  struct vcnl3020_data {
>  	struct regmap *regmap;
>  	struct device *dev;
>  	u8 rev;
> -	struct mutex lock;
>  };
>  
>  /**
> @@ -149,7 +147,6 @@ 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);
> @@ -161,11 +158,9 @@ static int vcnl3020_measure_proximity(struct vcnl3020_data *data, int *val)
>  	unsigned int reg;
>  	__be16 res;
>  
> -	mutex_lock(&data->lock);
> -
>  	rc = regmap_write(data->regmap, VCNL_COMMAND, VCNL_PS_OD);
>  	if (rc)
> -		goto err_unlock;
> +		return rc;
>  
>  	/* wait for data to become ready */
>  	rc = regmap_read_poll_timeout(data->regmap, VCNL_COMMAND, reg,
> @@ -174,20 +169,17 @@ static int vcnl3020_measure_proximity(struct vcnl3020_data *data, int *val)
>  	if (rc) {
>  		dev_err(data->dev,
>  			"Error (%d) reading vcnl3020 command register\n", rc);
> -		goto err_unlock;
> +		return rc;
>  	}
>  
>  	/* high & low result bytes read */
>  	rc = regmap_bulk_read(data->regmap, VCNL_PS_RESULT_HI, &res,
>  			      sizeof(res));
>  	if (rc)
> -		goto err_unlock;
> +		return rc;
>  
>  	*val = be16_to_cpu(res);
>  
> -err_unlock:
> -	mutex_unlock(&data->lock);
> -
>  	return rc;
>  }
>  
> @@ -450,25 +442,37 @@ static int vcnl3020_read_raw(struct iio_dev *indio_dev,
>  	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_RAW:
>  
>  		/* Protect against event capture. */
> -		if (vcnl3020_is_in_periodic_mode(data))
> -			return -EBUSY;
> +		if (vcnl3020_is_in_periodic_mode(data)) {
> +			rc = -EBUSY;
> +			goto end;
> +		}
>  
>  		rc = vcnl3020_measure_proximity(data, val);
>  		if (rc)
> -			return rc;
> -		return IIO_VAL_INT;
> +			goto end;
> +		rc = IIO_VAL_INT;
> +		goto end;
>  	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;
> +			goto end;
> +		rc = IIO_VAL_INT_PLUS_MICRO;
> +		goto end;
>  	default:
> -		return -EINVAL;
> +		rc = -EINVAL;
>  	}
> +
> +end:
> +	iio_device_release_direct_mode(indio_dev);
> +	return rc;
>  }
>  
>  static int vcnl3020_write_raw(struct iio_dev *indio_dev,

[PATCH 4/4] hwmon: vcnl3020: add hwmon driver for intrusion sensor

[Ivan Mikhaylov]

Intrusion status detection via Interrupt Status Register.

Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
---
 drivers/hwmon/Kconfig                  |  7 +++
 drivers/hwmon/Makefile                 |  1 +
 drivers/hwmon/vcnl3020-hwmon.c         | 57 ++++++++++++++++++++++
 drivers/iio/proximity/vcnl3020.c       | 67 ++++++++++++++++++++------
 include/linux/iio/proximity/vcnl3020.h | 26 ++++++++++
 5 files changed, 143 insertions(+), 15 deletions(-)
 create mode 100644 drivers/hwmon/vcnl3020-hwmon.c
 create mode 100644 include/linux/iio/proximity/vcnl3020.h

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 1ecf697d8d99..862205bbb3bf 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1916,6 +1916,13 @@ config SENSORS_TMP513
 	  This driver can also be built as a module. If so, the module
 	  will be called tmp513.
 
+config SENSORS_VCNL3020
+	tristate "VCNL3020"
+	depends on I2C && VCNL3020
+	help
+	  If you say yes here you get support for the intrusion
+	  sensor via VCNL3020 proximity sensor.
+
 config SENSORS_VEXPRESS
 	tristate "Versatile Express"
 	depends on VEXPRESS_CONFIG
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 09a86c5e1d29..1d96212587aa 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -186,6 +186,7 @@ obj-$(CONFIG_SENSORS_TMP108)	+= tmp108.o
 obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
 obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
 obj-$(CONFIG_SENSORS_TMP513)	+= tmp513.o
+obj-$(CONFIG_SENSORS_VCNL3020)	+= vcnl3020-hwmon.o
 obj-$(CONFIG_SENSORS_VEXPRESS)	+= vexpress-hwmon.o
 obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
diff --git a/drivers/hwmon/vcnl3020-hwmon.c b/drivers/hwmon/vcnl3020-hwmon.c
new file mode 100644
index 000000000000..199bea25723b
--- /dev/null
+++ b/drivers/hwmon/vcnl3020-hwmon.c
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * vcnl3020-hwmon.c - intrusion sensor.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/iio/proximity/vcnl3020.h>
+
+static ssize_t vcnl3020_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct vcnl3020_data *vcnl3020_data = dev_get_drvdata(dev);
+
+	bool data = vcnl3020_is_thr_triggered(vcnl3020_data);
+
+	return sprintf(buf, "%u\n", data);
+}
+
+static SENSOR_DEVICE_ATTR_2_RO(intrusion0_alarm, vcnl3020, 0, 0);
+
+static struct attribute *vcnl3020_attrs[] = {
+	&sensor_dev_attr_intrusion0_alarm.dev_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(vcnl3020);
+
+static int32_t vcnl3020_hwmon_probe(struct platform_device *pdev)
+{
+	struct vcnl3020_data *vcnl3020_data = platform_get_drvdata(pdev);
+	struct device *hwmon_dev;
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
+							   VCNL3020_DRV,
+							   vcnl3020_data,
+							   vcnl3020_groups);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct platform_driver vcnl3020_hwmon_driver = {
+	.probe = vcnl3020_hwmon_probe,
+	.driver = {
+		.name = VCNL3020_DRV_HWMON,
+	},
+};
+
+module_platform_driver(vcnl3020_hwmon_driver);
+
+MODULE_AUTHOR("Ivan Mikhaylov <i.mikhaylov@yadro.com>");
+MODULE_DESCRIPTION("Intrusion sensor for VCNL3020");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c
index bff59c7af966..67baa14cc900 100644
--- a/drivers/iio/proximity/vcnl3020.c
+++ b/drivers/iio/proximity/vcnl3020.c
@@ -11,9 +11,11 @@
 #include <linux/err.h>
 #include <linux/delay.h>
 #include <linux/regmap.h>
+#include <linux/platform_device.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/proximity/vcnl3020.h>
 
 #define VCNL3020_PROD_ID	0x21
 
@@ -66,18 +68,6 @@ static const int vcnl3020_prox_sampling_frequency[][2] = {
 	{250, 0},
 };
 
-/**
- * struct vcnl3020_data - vcnl3020 specific data.
- * @regmap:	device register map.
- * @dev:	vcnl3020 device.
- * @rev:	revision id.
- */
-struct vcnl3020_data {
-	struct regmap *regmap;
-	struct device *dev;
-	u8 rev;
-};
-
 /**
  * struct vcnl3020_property - vcnl3020 property.
  * @name:	property name.
@@ -330,6 +320,23 @@ static int vcnl3020_write_event(struct iio_dev *indio_dev,
 	return rc;
 }
 
+#ifdef CONFIG_SENSORS_VCNL3020
+
+bool vcnl3020_is_thr_triggered(struct vcnl3020_data *data)
+{
+	int rc;
+	unsigned int isr;
+
+	rc = regmap_read(data->regmap, VCNL_ISR, &isr);
+	if (rc)
+		return false;
+
+	return !!((isr & VCNL_INT_TH_LOW) || (isr & VCNL_INT_TH_HI));
+}
+EXPORT_SYMBOL_GPL(vcnl3020_is_thr_triggered);
+
+#endif
+
 static int vcnl3020_config_threshold(struct iio_dev *indio_dev, bool state)
 {
 	struct vcnl3020_data *data = iio_priv(indio_dev);
@@ -536,6 +543,7 @@ static int vcnl3020_probe(struct i2c_client *client)
 	struct vcnl3020_data *data;
 	struct iio_dev *indio_dev;
 	struct regmap *regmap;
+	struct platform_device *pdev;
 	int rc;
 
 	regmap = devm_regmap_init_i2c(client, &vcnl3020_regmap_config);
@@ -560,10 +568,39 @@ static int vcnl3020_probe(struct i2c_client *client)
 	indio_dev->info = &vcnl3020_info;
 	indio_dev->channels = vcnl3020_channels;
 	indio_dev->num_channels = ARRAY_SIZE(vcnl3020_channels);
-	indio_dev->name = "vcnl3020";
+	indio_dev->name = VCNL3020_DRV;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	return devm_iio_device_register(&client->dev, indio_dev);
+	rc = devm_iio_device_register(&client->dev, indio_dev);
+	if (rc != 0)
+		goto err_out;
+
+#ifdef CONFIG_SENSORS_VCNL3020
+
+	pdev = platform_device_alloc(VCNL3020_DRV_HWMON, -1);
+	if (!pdev) {
+		dev_err(&client->dev, "Failed to allocate %s\n",
+			VCNL3020_DRV_HWMON);
+		rc = -ENOMEM;
+		goto err_out;
+	}
+
+	pdev->dev.parent = &indio_dev->dev;
+	platform_set_drvdata(pdev, data);
+	rc = platform_device_add(pdev);
+	if (rc != 0) {
+		dev_err(&client->dev, "Failed to register %s: %d\n",
+			VCNL3020_DRV_HWMON, rc);
+		platform_device_put(pdev);
+		pdev = NULL;
+		goto err_out;
+	}
+
+#endif
+
+err_out:
+
+	return rc;
 }
 
 static const struct of_device_id vcnl3020_of_match[] = {
@@ -576,7 +613,7 @@ MODULE_DEVICE_TABLE(of, vcnl3020_of_match);
 
 static struct i2c_driver vcnl3020_driver = {
 	.driver = {
-		.name   = "vcnl3020",
+		.name   = VCNL3020_DRV,
 		.of_match_table = vcnl3020_of_match,
 	},
 	.probe_new  = vcnl3020_probe,
diff --git a/include/linux/iio/proximity/vcnl3020.h b/include/linux/iio/proximity/vcnl3020.h
new file mode 100644
index 000000000000..d99783298e6e
--- /dev/null
+++ b/include/linux/iio/proximity/vcnl3020.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This file describe API for VCNL3020 proximity sensor.
+ */
+
+#ifndef VCNL3020_PROXIMITY_H
+#define VCNL3020_PROXIMITY_H
+
+#define VCNL3020_DRV_HWMON	"vcnl3020-hwmon"
+#define VCNL3020_DRV		"vcnl3020"
+
+/**
+ * struct vcnl3020_data - vcnl3020 specific data.
+ * @regmap:	device register map.
+ * @dev:	vcnl3020 device.
+ * @rev:	revision id.
+ */
+struct vcnl3020_data {
+	struct regmap *regmap;
+	struct device *dev;
+	u8 rev;
+};
+
+bool vcnl3020_is_thr_triggered(struct vcnl3020_data *data);
+
+#endif
-- 
2.26.3

Re: [PATCH 4/4] hwmon: vcnl3020: add hwmon driver for intrusion sensor

[Guenter Roeck]

On Fri, Apr 30, 2021 at 06:24:19PM +0300, Ivan Mikhaylov wrote:
> Intrusion status detection via Interrupt Status Register.
> 
> Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>

I think this should, if at all, be handled using the
iio->hwmon bridge (or, in other words, require a solution
which is not chip specific).

I am also not sure if "proximity" is really appropriate to use
for intrusion detection in the sense of hardware monitoring.
This would require a proximity sensor within a chassis, which
would be both overkill and unlikely to happen in the real world.
"Intrusion", in hardware monitoring context, means "someone
opened the chassis", not "someone got [too] close".

> ---
>  drivers/hwmon/Kconfig                  |  7 +++
>  drivers/hwmon/Makefile                 |  1 +
>  drivers/hwmon/vcnl3020-hwmon.c         | 57 ++++++++++++++++++++++
>  drivers/iio/proximity/vcnl3020.c       | 67 ++++++++++++++++++++------
>  include/linux/iio/proximity/vcnl3020.h | 26 ++++++++++
>  5 files changed, 143 insertions(+), 15 deletions(-)
>  create mode 100644 drivers/hwmon/vcnl3020-hwmon.c
>  create mode 100644 include/linux/iio/proximity/vcnl3020.h
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 1ecf697d8d99..862205bbb3bf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1916,6 +1916,13 @@ config SENSORS_TMP513
>  	  This driver can also be built as a module. If so, the module
>  	  will be called tmp513.
>  
> +config SENSORS_VCNL3020
> +	tristate "VCNL3020"
> +	depends on I2C && VCNL3020
> +	help
> +	  If you say yes here you get support for the intrusion
> +	  sensor via VCNL3020 proximity sensor.
> +
>  config SENSORS_VEXPRESS
>  	tristate "Versatile Express"
>  	depends on VEXPRESS_CONFIG
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 09a86c5e1d29..1d96212587aa 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -186,6 +186,7 @@ obj-$(CONFIG_SENSORS_TMP108)	+= tmp108.o
>  obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
>  obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
>  obj-$(CONFIG_SENSORS_TMP513)	+= tmp513.o
> +obj-$(CONFIG_SENSORS_VCNL3020)	+= vcnl3020-hwmon.o
>  obj-$(CONFIG_SENSORS_VEXPRESS)	+= vexpress-hwmon.o
>  obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>  obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
> diff --git a/drivers/hwmon/vcnl3020-hwmon.c b/drivers/hwmon/vcnl3020-hwmon.c
> new file mode 100644
> index 000000000000..199bea25723b
> --- /dev/null
> +++ b/drivers/hwmon/vcnl3020-hwmon.c
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * vcnl3020-hwmon.c - intrusion sensor.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/iio/proximity/vcnl3020.h>
> +
> +static ssize_t vcnl3020_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct vcnl3020_data *vcnl3020_data = dev_get_drvdata(dev);
> +
> +	bool data = vcnl3020_is_thr_triggered(vcnl3020_data);
> +
> +	return sprintf(buf, "%u\n", data);
> +}
> +
> +static SENSOR_DEVICE_ATTR_2_RO(intrusion0_alarm, vcnl3020, 0, 0);
> +
> +static struct attribute *vcnl3020_attrs[] = {
> +	&sensor_dev_attr_intrusion0_alarm.dev_attr.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(vcnl3020);
> +
> +static int32_t vcnl3020_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct vcnl3020_data *vcnl3020_data = platform_get_drvdata(pdev);
> +	struct device *hwmon_dev;
> +
> +	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
> +							   VCNL3020_DRV,
> +							   vcnl3020_data,
> +							   vcnl3020_groups);

Please note that new drivers must register using
[devm_]hwmon_device_register_with_info().

Thanks,
Guenter

> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static struct platform_driver vcnl3020_hwmon_driver = {
> +	.probe = vcnl3020_hwmon_probe,
> +	.driver = {
> +		.name = VCNL3020_DRV_HWMON,
> +	},
> +};
> +
> +module_platform_driver(vcnl3020_hwmon_driver);
> +
> +MODULE_AUTHOR("Ivan Mikhaylov <i.mikhaylov@yadro.com>");
> +MODULE_DESCRIPTION("Intrusion sensor for VCNL3020");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c
> index bff59c7af966..67baa14cc900 100644
> --- a/drivers/iio/proximity/vcnl3020.c
> +++ b/drivers/iio/proximity/vcnl3020.c
> @@ -11,9 +11,11 @@
>  #include <linux/err.h>
>  #include <linux/delay.h>
>  #include <linux/regmap.h>
> +#include <linux/platform_device.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/proximity/vcnl3020.h>
>  
>  #define VCNL3020_PROD_ID	0x21
>  
> @@ -66,18 +68,6 @@ static const int vcnl3020_prox_sampling_frequency[][2] = {
>  	{250, 0},
>  };
>  
> -/**
> - * struct vcnl3020_data - vcnl3020 specific data.
> - * @regmap:	device register map.
> - * @dev:	vcnl3020 device.
> - * @rev:	revision id.
> - */
> -struct vcnl3020_data {
> -	struct regmap *regmap;
> -	struct device *dev;
> -	u8 rev;
> -};
> -
>  /**
>   * struct vcnl3020_property - vcnl3020 property.
>   * @name:	property name.
> @@ -330,6 +320,23 @@ static int vcnl3020_write_event(struct iio_dev *indio_dev,
>  	return rc;
>  }
>  
> +#ifdef CONFIG_SENSORS_VCNL3020
> +
> +bool vcnl3020_is_thr_triggered(struct vcnl3020_data *data)
> +{
> +	int rc;
> +	unsigned int isr;
> +
> +	rc = regmap_read(data->regmap, VCNL_ISR, &isr);
> +	if (rc)
> +		return false;
> +
> +	return !!((isr & VCNL_INT_TH_LOW) || (isr & VCNL_INT_TH_HI));
> +}
> +EXPORT_SYMBOL_GPL(vcnl3020_is_thr_triggered);
> +
> +#endif
> +
>  static int vcnl3020_config_threshold(struct iio_dev *indio_dev, bool state)
>  {
>  	struct vcnl3020_data *data = iio_priv(indio_dev);
> @@ -536,6 +543,7 @@ static int vcnl3020_probe(struct i2c_client *client)
>  	struct vcnl3020_data *data;
>  	struct iio_dev *indio_dev;
>  	struct regmap *regmap;
> +	struct platform_device *pdev;
>  	int rc;
>  
>  	regmap = devm_regmap_init_i2c(client, &vcnl3020_regmap_config);
> @@ -560,10 +568,39 @@ static int vcnl3020_probe(struct i2c_client *client)
>  	indio_dev->info = &vcnl3020_info;
>  	indio_dev->channels = vcnl3020_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(vcnl3020_channels);
> -	indio_dev->name = "vcnl3020";
> +	indio_dev->name = VCNL3020_DRV;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	return devm_iio_device_register(&client->dev, indio_dev);
> +	rc = devm_iio_device_register(&client->dev, indio_dev);
> +	if (rc != 0)
> +		goto err_out;
> +
> +#ifdef CONFIG_SENSORS_VCNL3020
> +
> +	pdev = platform_device_alloc(VCNL3020_DRV_HWMON, -1);
> +	if (!pdev) {
> +		dev_err(&client->dev, "Failed to allocate %s\n",
> +			VCNL3020_DRV_HWMON);
> +		rc = -ENOMEM;
> +		goto err_out;
> +	}
> +
> +	pdev->dev.parent = &indio_dev->dev;
> +	platform_set_drvdata(pdev, data);
> +	rc = platform_device_add(pdev);
> +	if (rc != 0) {
> +		dev_err(&client->dev, "Failed to register %s: %d\n",
> +			VCNL3020_DRV_HWMON, rc);
> +		platform_device_put(pdev);
> +		pdev = NULL;
> +		goto err_out;
> +	}
> +
> +#endif
> +
> +err_out:
> +
> +	return rc;
>  }
>  
>  static const struct of_device_id vcnl3020_of_match[] = {
> @@ -576,7 +613,7 @@ MODULE_DEVICE_TABLE(of, vcnl3020_of_match);
>  
>  static struct i2c_driver vcnl3020_driver = {
>  	.driver = {
> -		.name   = "vcnl3020",
> +		.name   = VCNL3020_DRV,
>  		.of_match_table = vcnl3020_of_match,
>  	},
>  	.probe_new  = vcnl3020_probe,
> diff --git a/include/linux/iio/proximity/vcnl3020.h b/include/linux/iio/proximity/vcnl3020.h
> new file mode 100644
> index 000000000000..d99783298e6e
> --- /dev/null
> +++ b/include/linux/iio/proximity/vcnl3020.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This file describe API for VCNL3020 proximity sensor.
> + */
> +
> +#ifndef VCNL3020_PROXIMITY_H
> +#define VCNL3020_PROXIMITY_H
> +
> +#define VCNL3020_DRV_HWMON	"vcnl3020-hwmon"
> +#define VCNL3020_DRV		"vcnl3020"
> +
> +/**
> + * struct vcnl3020_data - vcnl3020 specific data.
> + * @regmap:	device register map.
> + * @dev:	vcnl3020 device.
> + * @rev:	revision id.
> + */
> +struct vcnl3020_data {
> +	struct regmap *regmap;
> +	struct device *dev;
> +	u8 rev;
> +};
> +
> +bool vcnl3020_is_thr_triggered(struct vcnl3020_data *data);
> +
> +#endif
> -- 
> 2.26.3
> 

Re: [PATCH 4/4] hwmon: vcnl3020: add hwmon driver for intrusion sensor

[Ivan Mikhaylov]

On Fri, 2021-04-30 at 09:38 -0700, Guenter Roeck wrote:
> On Fri, Apr 30, 2021 at 06:24:19PM +0300, Ivan Mikhaylov wrote:
> > Intrusion status detection via Interrupt Status Register.
> > 
> > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
> 
> I think this should, if at all, be handled using the
> iio->hwmon bridge (or, in other words, require a solution
> which is not chip specific).

Thanks a lot for suggestion, it's actually looks what's needed here instead of
this driver. Anyways, there is no IIO_PROXIMITY support inside supported types
in iio_hwmon.c. Should I add additional case inside this driver for
IIO_PROXIMITY type?

> I am also not sure if "proximity" is really appropriate to use
> for intrusion detection in the sense of hardware monitoring.
> This would require a proximity sensor within a chassis, which
> would be both overkill and unlikely to happen in the real world.
> "Intrusion", in hardware monitoring context, means "someone
> opened the chassis", not "someone got [too] close".
> 

I'm not sure either but it exists :) And it's exactly for this purpose:
"someone opened the chassis", "how near/far is cover?".

Re: [PATCH 4/4] hwmon: vcnl3020: add hwmon driver for intrusion sensor

[Jonathan Cameron]

On Tue, 4 May 2021 22:46:53 +0300
Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:

> On Fri, 2021-04-30 at 09:38 -0700, Guenter Roeck wrote:
> > On Fri, Apr 30, 2021 at 06:24:19PM +0300, Ivan Mikhaylov wrote:  
> > > Intrusion status detection via Interrupt Status Register.
> > > 
> > > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>  
> > 
> > I think this should, if at all, be handled using the
> > iio->hwmon bridge (or, in other words, require a solution
> > which is not chip specific).  
> 
> Thanks a lot for suggestion, it's actually looks what's needed here instead of
> this driver. Anyways, there is no IIO_PROXIMITY support inside supported types
> in iio_hwmon.c. Should I add additional case inside this driver for
> IIO_PROXIMITY type?
> 
> > I am also not sure if "proximity" is really appropriate to use
> > for intrusion detection in the sense of hardware monitoring.
> > This would require a proximity sensor within a chassis, which
> > would be both overkill and unlikely to happen in the real world.
> > "Intrusion", in hardware monitoring context, means "someone
> > opened the chassis", not "someone got [too] close".
> >   
> 
> I'm not sure either but it exists :) And it's exactly for this purpose:
> "someone opened the chassis", "how near/far is cover?".
> 

Hmm. So we will have somewhat of an impedance mismatch.

In IIO events are push based (typically interrupt driven).
There is also the issue that we don't currently have in kernel
interfaces to allow drivers like iio-hwmon to use them (there
has never been enough demand though it has been discussed a few
times).   As such we'd need to implement the core support for
that as well.   We might get away with some simplifications that
make this not too painful - e.g. avoid the need to filter events
by stating that a consumer may well get events it's not interested
in and it is up to the consumer to check (a later optimization could
then add filtering similar to what we do for main data flows).

Jonathan

Re: [PATCH 4/4] hwmon: vcnl3020: add hwmon driver for intrusion sensor

[Guenter Roeck]

On Tue, May 04, 2021 at 10:46:53PM +0300, Ivan Mikhaylov wrote:
> On Fri, 2021-04-30 at 09:38 -0700, Guenter Roeck wrote:
> > On Fri, Apr 30, 2021 at 06:24:19PM +0300, Ivan Mikhaylov wrote:
> > > Intrusion status detection via Interrupt Status Register.
> > > 
> > > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
> > 
> > I think this should, if at all, be handled using the
> > iio->hwmon bridge (or, in other words, require a solution
> > which is not chip specific).
> 
> Thanks a lot for suggestion, it's actually looks what's needed here instead of
> this driver. Anyways, there is no IIO_PROXIMITY support inside supported types
> in iio_hwmon.c. Should I add additional case inside this driver for
> IIO_PROXIMITY type?
> 
> > I am also not sure if "proximity" is really appropriate to use
> > for intrusion detection in the sense of hardware monitoring.
> > This would require a proximity sensor within a chassis, which
> > would be both overkill and unlikely to happen in the real world.
> > "Intrusion", in hardware monitoring context, means "someone
> > opened the chassis", not "someone got [too] close".
> > 
> 
> I'm not sure either but it exists :) And it's exactly for this purpose:
> "someone opened the chassis", "how near/far is cover?".
> 

The cost for VCNL3020, for a full reel with 3,300 chips, is $1.17 per chip
at Mouser. A mechanical switch costs a couple of cents. A single proximity
sensor won't cover all parts of a chassis; one would likely need several
chips to be sure that are no blind spots (if that is even possible - I don't
think it is in any of my PC chassis due to mechanical limitations). This
is on top of programming, which would be sensitive to generating false
alarms (or missing alarms, for that matter). That sounds quite impractical
and expensive to me. I'd really like to see the actual use case where a
proximity sensor (or set of proximity sensors) is used for intrusion
detection in the sense of hardware monitoring - not just the technical
possibility of doing so, but an actual use case (as in "this vendor,
in this chassis, is doing it").

Thanks,
Guenter

Re: [PATCH 4/4] hwmon: vcnl3020: add hwmon driver for intrusion sensor

[Ivan Mikhaylov]

On Wed, 2021-05-05 at 07:02 -0700, Guenter Roeck wrote:
> On Tue, May 04, 2021 at 10:46:53PM +0300, Ivan Mikhaylov wrote:
> > On Fri, 2021-04-30 at 09:38 -0700, Guenter Roeck wrote:
> > > On Fri, Apr 30, 2021 at 06:24:19PM +0300, Ivan Mikhaylov wrote:
> > > > Intrusion status detection via Interrupt Status Register.
> > > > 
> > > > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
> > > 
> > > I think this should, if at all, be handled using the
> > > iio->hwmon bridge (or, in other words, require a solution
> > > which is not chip specific).
> > 
> > Thanks a lot for suggestion, it's actually looks what's needed here instead
> > of
> > this driver. Anyways, there is no IIO_PROXIMITY support inside supported
> > types
> > in iio_hwmon.c. Should I add additional case inside this driver for
> > IIO_PROXIMITY type?
> > 
> > > I am also not sure if "proximity" is really appropriate to use
> > > for intrusion detection in the sense of hardware monitoring.
> > > This would require a proximity sensor within a chassis, which
> > > would be both overkill and unlikely to happen in the real world.
> > > "Intrusion", in hardware monitoring context, means "someone
> > > opened the chassis", not "someone got [too] close".
> > > 
> > 
> > I'm not sure either but it exists :) And it's exactly for this purpose:
> > "someone opened the chassis", "how near/far is cover?".
> > 
> 
> The cost for VCNL3020, for a full reel with 3,300 chips, is $1.17 per chip
> at Mouser. A mechanical switch costs a couple of cents. A single proximity
> sensor won't cover all parts of a chassis; one would likely need several
> chips to be sure that are no blind spots (if that is even possible - I don't
> think it is in any of my PC chassis due to mechanical limitations). This
> is on top of programming, which would be sensitive to generating false
> alarms (or missing alarms, for that matter). That sounds quite impractical
> and expensive to me. I'd really like to see the actual use case where a
> proximity sensor (or set of proximity sensors) is used for intrusion
> detection in the sense of hardware monitoring - not just the technical
> possibility of doing so, but an actual use case (as in "this vendor,
> in this chassis, is doing it").
> 
> Thanks,
> Guenter


Guenter, VCNL3020 is indeed used as an intrusion detection sensor at least in
one real design. That is YADRO VESNIN Rev. C where the proximity sensor is
installed in a very tight space on an nvme switch board where installation of a
mechanical switch was not possible without substantial redesign of the existing
other components that would cost a lot more than the price of VCNL3020.

VESNIN is a very tight-packed design of 4 x POWER8 CPUs, up to 8TB of RAM, and 26 nvme disks, all that in just 2U.
* https://imgur.com/a/wU9wEd4

Re: [PATCH 4/4] hwmon: vcnl3020: add hwmon driver for intrusion sensor

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2327 bytes --]

Hi Ivan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on iio/togreg]
[also build test ERROR on next-20210430]
[cannot apply to hwmon/hwmon-next v5.12]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ivan-Mikhaylov/add-periodic-mode-threshold-options-and-hwmon/20210430-231929
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/6c71dd96a8b45aa67f6ad388b5e67269f02b854c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ivan-Mikhaylov/add-periodic-mode-threshold-options-and-hwmon/20210430-231929
        git checkout 6c71dd96a8b45aa67f6ad388b5e67269f02b854c
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=sh 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

ERROR: modpost: "remap_vmalloc_range_partial" [samples/vfio-mdev/mdpy.ko] undefined!
>> ERROR: modpost: "vcnl3020_is_thr_triggered" [drivers/hwmon/vcnl3020-hwmon.ko] undefined!
ERROR: modpost: "__delay" [drivers/net/mdio/mdio-cavium.ko] undefined!
ERROR: modpost: "__udivdi3" [fs/btrfs/btrfs.ko] undefined!
ERROR: modpost: "__umoddi3" [fs/btrfs/btrfs.ko] undefined!

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for SND_ATMEL_SOC_PDC
   Depends on SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && HAS_DMA
   Selected by
   - SND_ATMEL_SOC_SSC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC
   - SND_ATMEL_SOC_SSC_PDC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && ATMEL_SSC

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54255 bytes --]