On Sat, Nov 20, 2021 at 7:14 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Sat, 20 Nov 2021 15:04:27 +0000
> Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> > On Sat, 20 Nov 2021 02:14:58 -0800
> > Gwendal Grignou <gwendal@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >
> > 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.
Done. I remove dev_err() from whoami() calls to prevent dev_err()
pollution in the probe code.
>
> 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
Done. I removed some from sx924 as well.
> >
> > > +
> > > +#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.
Done
> >
> > > +#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.
My concern is the event management is done in the common code
(sx_common_push_events)), so if we were to add events, that function
would likely move as well. The event structure is more tied to the
function than the channels, so I would prefer to leave it in
sx_common.
> >
> > > +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.
Done
> >
> > 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.
Checked the other files as well.
> >
> >
> > > + 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).
Done.
> >
> > > +
> > > +#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 */
> >
>
