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@xxxxxxxxx>

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@xxxxxxxxx>");
> +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
>