Re: [PATCH v4 2/5] iio: sx9310: Extract common Semtech sensor logic

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 20 Nov 2021 15:04:27 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sat, 20 Nov 2021 02:14:58 -0800
> Gwendal Grignou <gwendal@chromium.org> wrote:
> 
> > Before adding new Semtech sensors, move common logic to all Semtech SAR
> > sensor in its own file:
> > - interface with IIO subsystem,
> > - interrupt management,
> > - channel access conrol,
> > - event processing.
> > 
> > The change adds a bidirectional interface between sx93xx and sx_common.
> > 
> > The change is quite mechanical, as the impacted functions are moved
> > and renamed.
> > 
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>  
> 
> Hi Gwendal,
> 
> Some trivial bits and bobs inline.  Biggest one is I'd like the new header to
> stand alone such that it is include order independent and doesn't rely on
> deeply nested includes, which means you need a few more includes and some
> forwards definitions.
> 
> Also, one cleanup that applies to the previous patch and here (I think).
> 
> Looks good to me otherwise.
> 
> Jonathan

One additional thing. Would be good to add dev_err_probe() to relevant error
paths in probe so we get deferred probing debug info registered for us.

It'll take a us a while to do this for all the IIO drivers, but as you
are touching these it's a nice to have.

Jonathan

> 
> > ---
> > Changes in v4:
> > - Fix credit in sx_common.c
> > - Use chip_info instead of info in common data structure.
> > - Remove unnecessary includes in sx_common.c
> > - Remove WHOAMI register values from sx_common.h
> > - Fix kerneldoc comments.
> > 
> > Changes in v3:
> > - No line change in Makefile
> > - Leave interrupt.h include, needed at suspend/resume.
> > 
> > Changes in v2:
> > - Add Kconfig/Makefile that were in the wrong patch
> > - Define a better interface between common code and driver code
> > - Move back read_avail() to driver code:
> >   As it now contains frequency table, it will differ from sensor to
> >   sensor.
> > - Check whoami and set driver name back in the driver code.
> > - Use a common probe routine.
> > - Fix kernel doc comments.
> > 
> >  drivers/iio/proximity/Kconfig     |   4 +
> >  drivers/iio/proximity/Makefile    |   1 +
> >  drivers/iio/proximity/sx9310.c    | 675 +++++-------------------------
> >  drivers/iio/proximity/sx_common.c | 568 +++++++++++++++++++++++++
> >  drivers/iio/proximity/sx_common.h | 155 +++++++
> >  5 files changed, 826 insertions(+), 577 deletions(-)
> >  create mode 100644 drivers/iio/proximity/sx_common.c
> >  create mode 100644 drivers/iio/proximity/sx_common.h
> > 
> > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > index 7c7203ca3ac638..e88fc373c2c903 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
> > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > index cbdac09433eb51..2577fbce4144e5 100644
> > --- a/drivers/iio/proximity/Makefile
> > +++ b/drivers/iio/proximity/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_RFD77402)		+= rfd77402.o
> >  obj-$(CONFIG_SRF04)		+= srf04.o
> >  obj-$(CONFIG_SRF08)		+= srf08.o
> >  obj-$(CONFIG_SX9310)		+= sx9310.o
> > +obj-$(CONFIG_SX_COMMON) 	+= sx_common.o
> >  obj-$(CONFIG_SX9500)		+= sx9500.o
> >  obj-$(CONFIG_VCNL3020)		+= vcnl3020.o
> >  obj-$(CONFIG_VL53L0X_I2C)	+= vl53l0x-i2c.o
> > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > index 1647268b6471f1..6376ffe17726f6 100644
> > --- a/drivers/iio/proximity/sx9310.c
> > +++ b/drivers/iio/proximity/sx9310.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/bitfield.h>
> >  #include <linux/delay.h>
> >  #include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/irq.h>  
> 
> An iwyu parse on this file before your changes suggested neither irq.h nor
> slab.h are directly used, so clean those out whilst here. Also possible a
> bunch of these other headers are also no longer directly used after you add the
> library but I haven't checked.
> 
> >  #include <linux/kernel.h>
> >  #include <linux/log2.h>
> > @@ -22,19 +23,16 @@
> >  #include <linux/pm.h>
> >  #include <linux/property.h>
> >  #include <linux/regmap.h>
> > -#include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >  
> > -#include <linux/iio/buffer.h>
> >  #include <linux/iio/events.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > -#include <linux/iio/trigger.h>
> > -#include <linux/iio/triggered_buffer.h>
> > -#include <linux/iio/trigger_consumer.h>
> > +
> > +#include "sx_common.h"
> >    
> 
> > +static_assert(SX9310_NUM_CHANNELS <= SX_COMMON_MAX_NUM_CHANNELS);
> > +
> > +#define SX9310_NAMED_CHANNEL(idx, name)				 \
> > +{								 \
> > +	.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),		 \
> > +	.info_mask_shared_by_all_available =			 \
> > +		BIT(IIO_CHAN_INFO_SAMP_FREQ),			 \
> > +	.indexed = 1,						 \
> > +	.channel = idx,						 \
> > +	.extend_name = name,					 \
> > +	.address = SX9310_REG_DIFF_MSB,				 \
> > +	.event_spec = sx_common_events,				 \  
> 
> As mentioned later, I'd be tempted to have a local copy of the event_spec.
> It's rather odd to have everything in the channel description local except
> for that part. 
> 
> > +	.num_event_specs = ARRAY_SIZE(sx_common_events),	 \
> > +	.scan_index = idx,					 \
> > +	.scan_type = {						 \
> > +		.sign = 's',					 \
> > +		.realbits = 12,					 \
> > +		.storagebits = 16,				 \
> > +		.endianness = IIO_BE,				 \
> > +	},							 \
> > +}  
> 
> 
> ...
> 
> 
> > diff --git a/drivers/iio/proximity/sx_common.c b/drivers/iio/proximity/sx_common.c
> > new file mode 100644
> > index 00000000000000..fc2062a1d95c74
> > --- /dev/null
> > +++ b/drivers/iio/proximity/sx_common.c
> > @@ -0,0 +1,568 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2021 Google LLC.
> > + *
> > + * Common part of most Semtech SAR sensor.
> > + *  
> One of my favourite trivial things to comment on.  No need for the trailing blank line
> before the */
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/i2c.h>
> > +#include <linux/irq.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>  
> 
> Used?  This was one of the most frequent 'unneeded includes' in the iwyu
> work.  Given all the wrappers for allocations in IIO, there are relatively
> few calls to stuff in slab.h
> 
> > +
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>  
> 
> I might be wrong, but my suspicion is that there are no users of anything in
> iio/sysfs.h any more since you converted the _available in the earlier patch.
> So probably drop this include in that patch and don't have it here.
> 
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +
> > +#include "sx_common.h"
> > +
> > +/* All Semtech SAR sensor have IRQ bit in same order. */
> > +#define   SX_COMMON_CONVDONE_IRQ			BIT(0)
> > +#define   SX_COMMON_FAR_IRQ				BIT(2)
> > +#define   SX_COMMON_CLOSE_IRQ				BIT(3)
> > +
> > +const struct iio_event_spec sx_common_events[3] = {
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_RISING,
> > +		.mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
> > +	},
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +		.mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
> > +	},
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_EITHER,
> > +		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> > +				 BIT(IIO_EV_INFO_HYSTERESIS) |
> > +				 BIT(IIO_EV_INFO_VALUE),
> > +	},
> > +};
> > +EXPORT_SYMBOL_GPL(sx_common_events);
> > +  
> 
> Seems likely this might become driver dependent at some point but I guess you can
> move it when needed.  Just feels a bit odd as it's tightly coupled to the channels spec.
> 
> Maybe you would be better off just replicating this small amount of data in each driver?
> That way it is readily available when looking at the rest of the chan_spec.
> 
> > +static int sx_common_init_device(struct iio_dev *indio_dev)
> > +{
> > +	struct sx_common_data *data = iio_priv(indio_dev);
> > +	struct sx_common_reg_default tmp;
> > +	const struct sx_common_reg_default *initval;
> > +	int ret;
> > +	unsigned int i, val;
> > +
> > +	ret = regmap_write(data->regmap, data->chip_info->reg_reset, SX_COMMON_SOFT_RESET);  
> 
> Rather long line - where it doesn't hurt readability I'd prefer to keep things under 80 chars.
> 
> Here it would be fine to break the long line. Might be worth considering a local variable
> for chip_info to short lines where it's used - there are other places this might help.
> 
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	usleep_range(1000, 2000); /* power-up time is ~1ms. */
> > +
> > +	/* Clear reset interrupt state by reading SX_COMMON_REG_IRQ_SRC. */
> > +	ret = regmap_read(data->regmap, SX_COMMON_REG_IRQ_SRC, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Program defaults from constant or BIOS. */
> > +	for (i = 0; i < data->chip_info->num_default_regs; i++) {
> > +		initval = data->chip_info->ops.get_default_reg(&indio_dev->dev,
> > +							  i, &tmp);
> > +		ret = regmap_write(data->regmap, initval->reg, initval->def);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return data->chip_info->ops.init_compensation(indio_dev);
> > +}
> > +  
> 
> ...
> 
> > diff --git a/drivers/iio/proximity/sx_common.h b/drivers/iio/proximity/sx_common.h
> > new file mode 100644
> > index 00000000000000..808c5fe44cf8eb
> > --- /dev/null
> > +++ b/drivers/iio/proximity/sx_common.h
> > @@ -0,0 +1,155 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Code shared between most Semtech SAR sensor driver.
> > + */
> > +
> > +#ifndef IIO_SX_COMMON_H
> > +#define IIO_SX_COMMON_H
> > +
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>  
> 
> A bunch of forwards definitions should be in here to avoid any potential
> header ordering dependencies. If you have a pointer and it's not defined
> directly in the two headers above there should be a forwards definition.
> 
> e.g.
> 
> struct device;
> struct iio_chan_spec;
> struct iio_dev;
> there are othes but given there is a struct iio_info this needs to
> include iio.h (which will reduce what else you need come to think of it).
> 
> > +
> > +#define SX_COMMON_REG_IRQ_SRC				0x00
> > +
> > +#define SX_COMMON_MAX_NUM_CHANNELS	4
> > +static_assert(SX_COMMON_MAX_NUM_CHANNELS < BITS_PER_LONG);
> > +
> > +struct sx_common_data;
> > +
> > +struct sx_common_reg_default {
> > +	u8 reg;
> > +	u8 def;
> > +};
> > +
> > +/**
> > + * struct sx_common_ops: function pointers needed by common code
> > + *
> > + * List functions needed by common code to gather information or configure
> > + * the sensor.
> > + *
> > + * @read_prox_data:	Function to read raw proximity data.
> > + * @check_whoami:	Set device name based on whoami register.
> > + * @init_compensation:	Function to set initial compensation.
> > + * @wait_for_sample:	When there are no physical IRQ, function to wait for a
> > + *			sample to be ready.
> > + * @get_default_reg:	Populate the initial value for a given register.
> > + */
> > +struct sx_common_ops {
> > +	int (*read_prox_data)(struct sx_common_data *data,
> > +			      const struct iio_chan_spec *chan, __be16 *val);
> > +	int (*check_whoami)(struct device *dev, struct iio_dev *indio_dev);
> > +	int (*init_compensation)(struct iio_dev *indio_dev);
> > +	int (*wait_for_sample)(struct sx_common_data *data);
> > +	const struct sx_common_reg_default  *
> > +		(*get_default_reg)(struct device *dev, int idx,
> > +				   struct sx_common_reg_default *reg_def);
> > +};
> > +
> > +/**
> > + * struct sx_common_chip_info: Semtech Sensor private chip information
> > + *
> > + * @reg_stat:		Main status register address.
> > + * @reg_irq_msk:	IRQ mask register address.
> > + * @reg_enable_chan:	Address to enable/disable channels/phases.
> > + * @reg_reset:		Reset register address.
> > + *
> > + * @mask_enable_chan:	Mask over the channels bits in the enable channel
> > + *			register.
> > + * @irq_msk_offset:	Offset to enable interrupt in the IRQ mask
> > + *			register.
> > + *
> > + * @num_channels:	Number of channel/phase.
> > + * @num_default_regs:	Number of internal registers that can be configured.
> > + *
> > + * @ops:		Private functions pointers.
> > + *
> > + * @iio_channels:	Description of exposed iio channels.
> > + * @num_iio_channels:	Number of iio_channels.
> > + * @iio_info:		iio_info structure for this driver.
> > + *
> > + */
> > +struct sx_common_chip_info {
> > +	unsigned int reg_stat;
> > +	unsigned int reg_irq_msk;
> > +	unsigned int reg_enable_chan;
> > +	unsigned int reg_reset;
> > +
> > +	unsigned int mask_enable_chan;
> > +	unsigned int irq_msk_offset;
> > +	unsigned int num_channels;
> > +	int num_default_regs;
> > +
> > +	struct sx_common_ops ops;
> > +
> > +	const struct iio_chan_spec *iio_channels;
> > +	int num_iio_channels;
> > +	struct iio_info iio_info;
> > +};
> > +
> > +/**
> > + * struct sx_common_data: Semtech Sensor private data structure.
> > + *
> > + * @chip_info:		Structure defining sensor internals.
> > + * @mutex:		Serialize access to registers and channel configuration.
> > + * @num_channels:	Number of channel/phase.
> > + * @completion:		completion object to wait for data acquisition.
> > + * @client:		I2C client structure.
> > + * @trig:		IIO trigger object.
> > + * @regmap:		Register map.
> > + *
> > + * @num_default_regs:	Number of default registers to set at init.
> > + * @supplies:		Power supplies object.
> > + * @chan_prox_stat:	Last reading of the proximity status for each channel.
> > + *			We only send an event to user space when this changes.
> > + * @trigger_enabled:	True when the device trigger is enabled.
> > + *
> > + * @buffer:		Bufffer to store raw samples.
> > + * @suspend_ctrl:	Remember enabled channels and sample rate during suspend.
> > + * @chan_read:		Bit field for each raw channel enabled.
> > + * @chan_event:		Bit field for each event enabled.
> > + *
> > + */
> > +struct sx_common_data {
> > +	const struct sx_common_chip_info *chip_info;
> > +
> > +	struct mutex mutex;
> > +	struct completion completion;
> > +	struct i2c_client *client;
> > +	struct iio_trigger *trig;
> > +	struct regmap *regmap;
> > +
> > +	struct regulator_bulk_data supplies[2];
> > +	unsigned long chan_prox_stat;
> > +	bool trigger_enabled;
> > +
> > +	/* Ensure correct alignment of timestamp when present. */
> > +	struct {
> > +		__be16 channels[SX_COMMON_MAX_NUM_CHANNELS];
> > +		s64 ts __aligned(8);
> > +	} buffer;
> > +
> > +	unsigned int suspend_ctrl;
> > +	unsigned long chan_read;
> > +	unsigned long chan_event;
> > +};
> > +
> > +int sx_common_read_proximity(struct sx_common_data *data,
> > +			     const struct iio_chan_spec *chan, int *val);
> > +
> > +int sx_common_read_event_config(struct iio_dev *indio_dev,
> > +				const struct iio_chan_spec *chan,
> > +				enum iio_event_type type,
> > +				enum iio_event_direction dir);
> > +int sx_common_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);
> > +
> > +int sx_common_probe(struct i2c_client *client,
> > +		    const struct sx_common_chip_info *chip_info,
> > +		    const struct regmap_config *regmap_config);
> > +
> > +/* 3 is the number of events defined by a single phase. */
> > +extern const struct iio_event_spec sx_common_events[3];
> > +
> > +#endif  /* IIO_SX_COMMON_H */  
>