Re: [PATCH 3/5] iio: proximity: Add SX9324 support

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 30 Oct 2021 04:18:25 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> Semtech SAR sensor SX9324 is an evolution of the SX9310:
> It has 4 phases that can be configure to capture and process data
> from any of 3 CS pins and provide independent detection:
> proximity, table proximity or body proximity.
> 
> Gather antenna data:
> echo sx9324-dev3 > trigger/current_trigger
> echo 1 > scan_elements/in_proximity0_en
> echo 1 > buffer/enable
> od -v -An --endian=big -t d2 -w2 /dev/iio\:device3
> (at 10Hz, the default).
> 
> Trigger events:
> Setting:
> thresh_falling_period: 2 (events)
> thresh_rising_period: 2 (events)
> in_proximity0_thresh_either_value: 300
> in_proximity0_thresh_either_hysteresis: 72
> 
> using iio_event_monitor /dev/iio\:deviceX, approaching my hand to the
> antenna pad, I see:
> ...
> Event: time: 1634763907532035297, type: proximity, channel: 0, evtype:
> thresh, direction: falling
> Event: time: 1634763910138104640, type: proximity, channel: 0, evtype:
> thresh, direction: rising
> ...
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>

Various comment inline, but biggest one is lack of docs of non standard ABI
which means discussion is very difficult.

Jonathan

> ---
>  drivers/iio/proximity/Kconfig  |  18 +
>  drivers/iio/proximity/Makefile |   3 +-
>  drivers/iio/proximity/sx9324.c | 931 +++++++++++++++++++++++++++++++++
>  3 files changed, 951 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/iio/proximity/sx9324.c
> 
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index 7c7203ca3ac63..aaddf97f9b219 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -112,11 +112,15 @@ config SRF04
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called srf04.
>  
> +config SX_COMMON
> +	tristate
> +
>  config SX9310
>  	tristate "SX9310/SX9311 Semtech proximity sensor"
>  	select IIO_BUFFER
>  	select IIO_TRIGGERED_BUFFER
>  	select REGMAP_I2C
> +	select SX_COMMON
>  	depends on I2C
>  	help
>  	  Say Y here to build a driver for Semtech's SX9310/SX9311 capacitive
> @@ -125,6 +129,20 @@ config SX9310
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called sx9310.
>  
> +config SX9324
> +	tristate "SX9324 Semtech proximity sensor"
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	select REGMAP_I2C
> +	select SX_COMMON
> +	depends on I2C
> +	help
> +	  Say Y here to build a driver for Semtech's SX9324
> +	  proximity/button sensor.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sx9324.
> +
>  config SX9500
>  	tristate "SX9500 Semtech proximity sensor"
>  	select IIO_BUFFER
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index cbdac09433eb5..1b026fedc396c 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -14,7 +14,8 @@ obj-$(CONFIG_RFD77402)		+= rfd77402.o
>  obj-$(CONFIG_SRF04)		+= srf04.o
>  obj-$(CONFIG_SRF08)		+= srf08.o
>  obj-$(CONFIG_SX9310)		+= sx9310.o
> +obj-$(CONFIG_SX9324)		+= sx9324.o
> +obj-$(CONFIG_SX_COMMON) 	+= sx_common.o
Previous patch...

>  obj-$(CONFIG_SX9500)		+= sx9500.o
>  obj-$(CONFIG_VCNL3020)		+= vcnl3020.o
>  obj-$(CONFIG_VL53L0X_I2C)	+= vl53l0x-i2c.o
> -
stray

> diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> new file mode 100644
> index 0000000000000..41d9c950c5abd
> --- /dev/null
> +++ b/drivers/iio/proximity/sx9324.c
> @@ -0,0 +1,931 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2021 Google LLC.
> + *
> + * Driver for Semtech's SX9324 capacitive proximity/button solution.
> + * Based on SX9324 driver and copy of datasheet at:
> + * https://edit.wpgdadawant.com/uploads/news_file/program/2019/30184/tech_files/program_30184_suggest_other_file.pdf
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */

SPDX tag usually means we can drop the GPL license blurb.

> +

> +
> +/* 4 channels, as defined in STAT0: PH0, PH1, PH2 and PH3. */
> +#define SX9324_NUM_CHANNELS		4
> +/* 3 CS pins: CS0, CS1, CS2. */
> +#define SX9324_NUM_PINS			3
> +
> +#define SX9324_CHANNEL(idx)						\
> +	{								\
> +		.type = IIO_PROXIMITY,					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +				      BIT(IIO_CHAN_INFO_HARDWAREGAIN),	\
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> +		.info_mask_separate_available =				\
> +			BIT(IIO_CHAN_INFO_HARDWAREGAIN),		\
> +		.indexed = 1,						\
> +		.channel = idx,						\
> +		.address = SX9324_REG_DIFF_MSB,				\
> +		.event_spec = sx_common_events,				\
> +		.num_event_specs = ARRAY_SIZE(sx_common_events),	\
> +		.scan_index = idx,					\
> +		.scan_type = {						\
> +			.sign = 's',					\
> +			.realbits = 12,					\
> +			.storagebits = 16,				\
> +			.endianness = IIO_BE,				\
> +		},							\
> +	}
> +
> +static const struct iio_chan_spec sx9324_channels[] = {
> +	SX9324_CHANNEL(0),			/* Phase 0 */
> +	SX9324_CHANNEL(1),			/* Phase 1 */
> +	SX9324_CHANNEL(2),			/* Phase 2 */
> +	SX9324_CHANNEL(3),			/* Phase 3 */
> +	IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +static const char * const sx9324_cs_pin_usage[] = { "HZ", "MI", "DS", "GD" };
> +
> +static ssize_t sx9324_phase_configuration_show(struct device *dev,
> +					       struct device_attribute *attr,
> +					       char *buf)
> +{
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct sx_common_data *data = iio_priv(indio_dev);
> +	unsigned int val;
> +	int i, ret, pin_idx;
> +	size_t len = 0;
> +
> +	ret = regmap_read(data->regmap, SX9324_REG_AFE_PH0 + this_attr->address, &val);
> +
> +	for (i = 0; i < SX9324_NUM_PINS; i++) {
> +		pin_idx = (val & SX9324_REG_AFE_PH0_PIN_MASK(i)) >> (2 * i);
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%s,",
> +				 sx9324_cs_pin_usage[pin_idx]);
> +	}
> +	buf[len - 1] = '\n';
> +	return len;
> +}
> +
> +static ssize_t sx9324_phase_configuration_store(struct device *dev,
> +						struct device_attribute *attr,
> +						const char *buf,
> +						size_t len)
> +{
> +	return -EINVAL;

Don't do this. If they are read only treat them correctly as such
with appropriate permissions etc.

> +}
> +
> +#define IIO_DEV_ATTR_PHASE_CONFIG(_idx) \
> +IIO_DEVICE_ATTR(in_proximity_configuration##_idx, 0644, \
> +		sx9324_phase_configuration_show, \
> +		sx9324_phase_configuration_store, _idx)
> +
> +static IIO_DEV_ATTR_PHASE_CONFIG(0);
> +static IIO_DEV_ATTR_PHASE_CONFIG(1);
> +static IIO_DEV_ATTR_PHASE_CONFIG(2);
> +static IIO_DEV_ATTR_PHASE_CONFIG(3);

Documentation of these? I'm not going to comment on them without appropriate
docs in

Documentation/ABI/testing/

Just wastes time figuring out what they are.


...



> +static int sx9324_write_raw(struct iio_dev *indio_dev,
> +			    const struct iio_chan_spec *chan, int val, int val2,
> +			    long mask)
> +{
> +	struct sx_common_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return sx9324_set_samp_freq(data, val, val2);
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		return sx9324_write_gain(data, chan, val);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static struct attribute *sx9324_attributes[] = {
> +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,

As mentioned in previous review, ideally move this to using the
read_avail callback.

> +	&iio_dev_attr_in_proximity_configuration0.dev_attr.attr,
> +	&iio_dev_attr_in_proximity_configuration1.dev_attr.attr,
> +	&iio_dev_attr_in_proximity_configuration2.dev_attr.attr,
> +	&iio_dev_attr_in_proximity_configuration3.dev_attr.attr,
> +	NULL
> +};

...

> +
> +static const struct acpi_device_id sx9324_acpi_match[] = {
> +	{ "STH9324", SX9324_WHOAMI_VALUE},

Any reference to a board, preferably with a dump of the DSDT/SSDT
that uses this or an official doc etc to justify that it's valid?

STH isn't in the pnp database so if it's in the wild we need a comment
to say where.

> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, sx9324_acpi_match);
> +
...