* [PATCH v4 1/5] iio: sx9310: Add frequency in read_avail
2021-11-20 10:14 [PATCH v4 0/5] Expand Semtech SAR Sensors support Gwendal Grignou
@ 2021-11-20 10:14 ` Gwendal Grignou
2021-11-20 10:14 ` [PATCH v4 2/5] iio: sx9310: Extract common Semtech sensor logic Gwendal Grignou
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Gwendal Grignou @ 2021-11-20 10:14 UTC (permalink / raw)
To: jic23, lars, swboyd; +Cc: andy.shevchenko, linux-iio, Gwendal Grignou
Instead of using IIO_DEV_ATTR_SAMP_FREQ_AVAIL sysfs attribute,
add a _FREQ case in read_avail() to display the frequency table.
No change to |sampling_frequency_available| was observed.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
No changes in v4
No changes in v3
New in v2.
drivers/iio/proximity/sx9310.c | 33 +++++++--------------------------
1 file changed, 7 insertions(+), 26 deletions(-)
diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index a3fdb59b06d220..1647268b6471f1 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -196,6 +196,8 @@ static const struct iio_event_spec sx9310_events[] = {
.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, \
@@ -251,22 +253,6 @@ static const unsigned int sx9310_scan_period_table[] = {
400, 600, 800, 1000, 2000, 3000, 4000, 5000,
};
-static ssize_t sx9310_show_samp_freq_avail(struct device *dev,
- struct device_attribute *attr,
- char *buf)
-{
- size_t len = 0;
- int i;
-
- for (i = 0; i < ARRAY_SIZE(sx9310_samp_freq_table); i++)
- len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%d ",
- sx9310_samp_freq_table[i].val,
- sx9310_samp_freq_table[i].val2);
- buf[len - 1] = '\n';
- return len;
-}
-static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(sx9310_show_samp_freq_avail);
-
static const struct regmap_range sx9310_writable_reg_ranges[] = {
regmap_reg_range(SX9310_REG_IRQ_MSK, SX9310_REG_IRQ_FUNC),
regmap_reg_range(SX9310_REG_PROX_CTRL0, SX9310_REG_PROX_CTRL19),
@@ -562,6 +548,11 @@ static int sx9310_read_avail(struct iio_dev *indio_dev,
*length = ARRAY_SIZE(sx9310_gain_vals);
*vals = sx9310_gain_vals;
return IIO_AVAIL_LIST;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ *length = ARRAY_SIZE(sx9310_samp_freq_table) * 2;
+ *vals = (int *)sx9310_samp_freq_table;
+ return IIO_AVAIL_LIST;
}
return -EINVAL;
@@ -1031,17 +1022,7 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev,
return ret;
}
-static struct attribute *sx9310_attributes[] = {
- &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
- NULL
-};
-
-static const struct attribute_group sx9310_attribute_group = {
- .attrs = sx9310_attributes,
-};
-
static const struct iio_info sx9310_info = {
- .attrs = &sx9310_attribute_group,
.read_raw = sx9310_read_raw,
.read_avail = sx9310_read_avail,
.read_event_value = sx9310_read_event_val,
--
2.34.0.rc2.393.gf8c9666880-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v4 2/5] iio: sx9310: Extract common Semtech sensor logic
2021-11-20 10:14 [PATCH v4 0/5] Expand Semtech SAR Sensors support Gwendal Grignou
2021-11-20 10:14 ` [PATCH v4 1/5] iio: sx9310: Add frequency in read_avail Gwendal Grignou
@ 2021-11-20 10:14 ` Gwendal Grignou
2021-11-20 15:04 ` Jonathan Cameron
2021-11-20 10:14 ` [PATCH v4 3/5] iio: proximity: Add SX9324 support Gwendal Grignou
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Gwendal Grignou @ 2021-11-20 10:14 UTC (permalink / raw)
To: jic23, lars, swboyd; +Cc: andy.shevchenko, linux-iio, Gwendal Grignou
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>
---
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>
#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"
/* Register definitions. */
-#define SX9310_REG_IRQ_SRC 0x00
+#define SX9310_REG_IRQ_SRC SX_COMMON_REG_IRQ_SRC
#define SX9310_REG_STAT0 0x01
#define SX9310_REG_STAT1 0x02
#define SX9310_REG_STAT1_COMPSTAT_MASK GENMASK(3, 0)
@@ -135,83 +133,36 @@
#define SX9310_WHOAMI_VALUE 0x01
#define SX9311_WHOAMI_VALUE 0x02
#define SX9310_REG_RESET 0x7f
-#define SX9310_SOFT_RESET 0xde
/* 4 hardware channels, as defined in STAT0: COMB, CS2, CS1 and CS0. */
#define SX9310_NUM_CHANNELS 4
-static_assert(SX9310_NUM_CHANNELS < BITS_PER_LONG);
-
-struct sx9310_data {
- /* Serialize access to registers and channel configuration */
- struct mutex mutex;
- struct i2c_client *client;
- struct iio_trigger *trig;
- struct regmap *regmap;
- struct regulator_bulk_data supplies[2];
- /*
- * Last reading of the proximity status for each channel.
- * We only send an event to user space when this changes.
- */
- unsigned long chan_prox_stat;
- bool trigger_enabled;
- /* Ensure correct alignment of timestamp when present. */
- struct {
- __be16 channels[SX9310_NUM_CHANNELS];
- s64 ts __aligned(8);
- } buffer;
- /* Remember enabled channels and sample rate during suspend. */
- unsigned int suspend_ctrl0;
- struct completion completion;
- unsigned long chan_read;
- unsigned long chan_event;
- unsigned int whoami;
-};
-
-static const struct iio_event_spec sx9310_events[] = {
- {
- .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),
- },
-};
-
-#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 = sx9310_events, \
- .num_event_specs = ARRAY_SIZE(sx9310_events), \
- .scan_index = idx, \
- .scan_type = { \
- .sign = 's', \
- .realbits = 12, \
- .storagebits = 16, \
- .endianness = IIO_BE, \
- }, \
- }
+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, \
+ .num_event_specs = ARRAY_SIZE(sx_common_events), \
+ .scan_index = idx, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 12, \
+ .storagebits = 16, \
+ .endianness = IIO_BE, \
+ }, \
+}
#define SX9310_CHANNEL(idx) SX9310_NAMED_CHANNEL(idx, NULL)
static const struct iio_chan_spec sx9310_channels[] = {
@@ -306,64 +257,7 @@ static const struct regmap_config sx9310_regmap_config = {
.volatile_table = &sx9310_volatile_regs,
};
-static int sx9310_update_chan_en(struct sx9310_data *data,
- unsigned long chan_read,
- unsigned long chan_event)
-{
- int ret;
- unsigned long channels = chan_read | chan_event;
-
- if ((data->chan_read | data->chan_event) != channels) {
- ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL0,
- SX9310_REG_PROX_CTRL0_SENSOREN_MASK,
- channels);
- if (ret)
- return ret;
- }
- data->chan_read = chan_read;
- data->chan_event = chan_event;
- return 0;
-}
-
-static int sx9310_get_read_channel(struct sx9310_data *data, int channel)
-{
- return sx9310_update_chan_en(data, data->chan_read | BIT(channel),
- data->chan_event);
-}
-
-static int sx9310_put_read_channel(struct sx9310_data *data, int channel)
-{
- return sx9310_update_chan_en(data, data->chan_read & ~BIT(channel),
- data->chan_event);
-}
-
-static int sx9310_get_event_channel(struct sx9310_data *data, int channel)
-{
- return sx9310_update_chan_en(data, data->chan_read,
- data->chan_event | BIT(channel));
-}
-
-static int sx9310_put_event_channel(struct sx9310_data *data, int channel)
-{
- return sx9310_update_chan_en(data, data->chan_read,
- data->chan_event & ~BIT(channel));
-}
-
-static int sx9310_enable_irq(struct sx9310_data *data, unsigned int irq)
-{
- if (!data->client->irq)
- return 0;
- return regmap_update_bits(data->regmap, SX9310_REG_IRQ_MSK, irq, irq);
-}
-
-static int sx9310_disable_irq(struct sx9310_data *data, unsigned int irq)
-{
- if (!data->client->irq)
- return 0;
- return regmap_update_bits(data->regmap, SX9310_REG_IRQ_MSK, irq, 0);
-}
-
-static int sx9310_read_prox_data(struct sx9310_data *data,
+static int sx9310_read_prox_data(struct sx_common_data *data,
const struct iio_chan_spec *chan, __be16 *val)
{
int ret;
@@ -379,7 +273,7 @@ static int sx9310_read_prox_data(struct sx9310_data *data,
* If we have no interrupt support, we have to wait for a scan period
* after enabling a channel to get a result.
*/
-static int sx9310_wait_for_sample(struct sx9310_data *data)
+static int sx9310_wait_for_sample(struct sx_common_data *data)
{
int ret;
unsigned int val;
@@ -395,66 +289,7 @@ static int sx9310_wait_for_sample(struct sx9310_data *data)
return 0;
}
-static int sx9310_read_proximity(struct sx9310_data *data,
- const struct iio_chan_spec *chan, int *val)
-{
- int ret;
- __be16 rawval;
-
- mutex_lock(&data->mutex);
-
- ret = sx9310_get_read_channel(data, chan->channel);
- if (ret)
- goto out;
-
- ret = sx9310_enable_irq(data, SX9310_CONVDONE_IRQ);
- if (ret)
- goto out_put_channel;
-
- mutex_unlock(&data->mutex);
-
- if (data->client->irq) {
- ret = wait_for_completion_interruptible(&data->completion);
- reinit_completion(&data->completion);
- } else {
- ret = sx9310_wait_for_sample(data);
- }
-
- mutex_lock(&data->mutex);
-
- if (ret)
- goto out_disable_irq;
-
- ret = sx9310_read_prox_data(data, chan, &rawval);
- if (ret)
- goto out_disable_irq;
-
- *val = sign_extend32(be16_to_cpu(rawval),
- chan->address == SX9310_REG_DIFF_MSB ? 11 : 15);
-
- ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ);
- if (ret)
- goto out_put_channel;
-
- ret = sx9310_put_read_channel(data, chan->channel);
- if (ret)
- goto out;
-
- mutex_unlock(&data->mutex);
-
- return IIO_VAL_INT;
-
-out_disable_irq:
- sx9310_disable_irq(data, SX9310_CONVDONE_IRQ);
-out_put_channel:
- sx9310_put_read_channel(data, chan->channel);
-out:
- mutex_unlock(&data->mutex);
-
- return ret;
-}
-
-static int sx9310_read_gain(struct sx9310_data *data,
+static int sx9310_read_gain(struct sx_common_data *data,
const struct iio_chan_spec *chan, int *val)
{
unsigned int regval, gain;
@@ -482,7 +317,7 @@ static int sx9310_read_gain(struct sx9310_data *data,
return IIO_VAL_INT;
}
-static int sx9310_read_samp_freq(struct sx9310_data *data, int *val, int *val2)
+static int sx9310_read_samp_freq(struct sx_common_data *data, int *val, int *val2)
{
unsigned int regval;
int ret;
@@ -502,7 +337,7 @@ static int sx9310_read_raw(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan, int *val,
int *val2, long mask)
{
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
int ret;
if (chan->type != IIO_PROXIMITY)
@@ -514,7 +349,7 @@ static int sx9310_read_raw(struct iio_dev *indio_dev,
if (ret)
return ret;
- ret = sx9310_read_proximity(data, chan, val);
+ ret = sx_common_read_proximity(data, chan, val);
iio_device_release_direct_mode(indio_dev);
return ret;
case IIO_CHAN_INFO_HARDWAREGAIN:
@@ -577,7 +412,7 @@ static int sx9310_get_thresh_reg(unsigned int channel)
return -EINVAL;
}
-static int sx9310_read_thresh(struct sx9310_data *data,
+static int sx9310_read_thresh(struct sx_common_data *data,
const struct iio_chan_spec *chan, int *val)
{
unsigned int reg;
@@ -600,7 +435,7 @@ static int sx9310_read_thresh(struct sx9310_data *data,
return IIO_VAL_INT;
}
-static int sx9310_read_hysteresis(struct sx9310_data *data,
+static int sx9310_read_hysteresis(struct sx_common_data *data,
const struct iio_chan_spec *chan, int *val)
{
unsigned int regval, pthresh;
@@ -624,7 +459,7 @@ static int sx9310_read_hysteresis(struct sx9310_data *data,
return IIO_VAL_INT;
}
-static int sx9310_read_far_debounce(struct sx9310_data *data, int *val)
+static int sx9310_read_far_debounce(struct sx_common_data *data, int *val)
{
unsigned int regval;
int ret;
@@ -642,7 +477,7 @@ static int sx9310_read_far_debounce(struct sx9310_data *data, int *val)
return IIO_VAL_INT;
}
-static int sx9310_read_close_debounce(struct sx9310_data *data, int *val)
+static int sx9310_read_close_debounce(struct sx_common_data *data, int *val)
{
unsigned int regval;
int ret;
@@ -666,7 +501,7 @@ static int sx9310_read_event_val(struct iio_dev *indio_dev,
enum iio_event_direction dir,
enum iio_event_info info, int *val, int *val2)
{
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
if (chan->type != IIO_PROXIMITY)
return -EINVAL;
@@ -690,7 +525,7 @@ static int sx9310_read_event_val(struct iio_dev *indio_dev,
}
}
-static int sx9310_write_thresh(struct sx9310_data *data,
+static int sx9310_write_thresh(struct sx_common_data *data,
const struct iio_chan_spec *chan, int val)
{
unsigned int reg;
@@ -720,7 +555,7 @@ static int sx9310_write_thresh(struct sx9310_data *data,
return ret;
}
-static int sx9310_write_hysteresis(struct sx9310_data *data,
+static int sx9310_write_hysteresis(struct sx_common_data *data,
const struct iio_chan_spec *chan, int _val)
{
unsigned int hyst, val = _val;
@@ -750,7 +585,7 @@ static int sx9310_write_hysteresis(struct sx9310_data *data,
return ret;
}
-static int sx9310_write_far_debounce(struct sx9310_data *data, int val)
+static int sx9310_write_far_debounce(struct sx_common_data *data, int val)
{
int ret;
unsigned int regval;
@@ -771,7 +606,7 @@ static int sx9310_write_far_debounce(struct sx9310_data *data, int val)
return ret;
}
-static int sx9310_write_close_debounce(struct sx9310_data *data, int val)
+static int sx9310_write_close_debounce(struct sx_common_data *data, int val)
{
int ret;
unsigned int regval;
@@ -798,7 +633,7 @@ static int sx9310_write_event_val(struct iio_dev *indio_dev,
enum iio_event_direction dir,
enum iio_event_info info, int val, int val2)
{
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
if (chan->type != IIO_PROXIMITY)
return -EINVAL;
@@ -822,7 +657,7 @@ static int sx9310_write_event_val(struct iio_dev *indio_dev,
}
}
-static int sx9310_set_samp_freq(struct sx9310_data *data, int val, int val2)
+static int sx9310_set_samp_freq(struct sx_common_data *data, int val, int val2)
{
int i, ret;
@@ -846,8 +681,8 @@ static int sx9310_set_samp_freq(struct sx9310_data *data, int val, int val2)
return ret;
}
-static int sx9310_write_gain(struct sx9310_data *data,
- const struct iio_chan_spec *chan, int val)
+static int sx9310_write_gain(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int val)
{
unsigned int gain, mask;
int ret;
@@ -881,7 +716,7 @@ static int sx9310_write_raw(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan, int val, int val2,
long mask)
{
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
if (chan->type != IIO_PROXIMITY)
return -EINVAL;
@@ -896,238 +731,7 @@ static int sx9310_write_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
-static irqreturn_t sx9310_irq_handler(int irq, void *private)
-{
- struct iio_dev *indio_dev = private;
- struct sx9310_data *data = iio_priv(indio_dev);
-
- if (data->trigger_enabled)
- iio_trigger_poll(data->trig);
-
- /*
- * Even if no event is enabled, we need to wake the thread to clear the
- * interrupt state by reading SX9310_REG_IRQ_SRC.
- * It is not possible to do that here because regmap_read takes a mutex.
- */
- return IRQ_WAKE_THREAD;
-}
-
-static void sx9310_push_events(struct iio_dev *indio_dev)
-{
- int ret;
- unsigned int val, chan;
- struct sx9310_data *data = iio_priv(indio_dev);
- s64 timestamp = iio_get_time_ns(indio_dev);
- unsigned long prox_changed;
-
- /* Read proximity state on all channels */
- ret = regmap_read(data->regmap, SX9310_REG_STAT0, &val);
- if (ret) {
- dev_err(&data->client->dev, "i2c transfer error in irq\n");
- return;
- }
-
- /*
- * Only iterate over channels with changes on proximity status that have
- * events enabled.
- */
- prox_changed = (data->chan_prox_stat ^ val) & data->chan_event;
-
- for_each_set_bit(chan, &prox_changed, SX9310_NUM_CHANNELS) {
- int dir;
- u64 ev;
-
- dir = (val & BIT(chan)) ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
- ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan,
- IIO_EV_TYPE_THRESH, dir);
-
- iio_push_event(indio_dev, ev, timestamp);
- }
- data->chan_prox_stat = val;
-}
-
-static irqreturn_t sx9310_irq_thread_handler(int irq, void *private)
-{
- struct iio_dev *indio_dev = private;
- struct sx9310_data *data = iio_priv(indio_dev);
- int ret;
- unsigned int val;
-
- mutex_lock(&data->mutex);
-
- ret = regmap_read(data->regmap, SX9310_REG_IRQ_SRC, &val);
- if (ret) {
- dev_err(&data->client->dev, "i2c transfer error in irq\n");
- goto out;
- }
-
- if (val & (SX9310_FAR_IRQ | SX9310_CLOSE_IRQ))
- sx9310_push_events(indio_dev);
-
- if (val & SX9310_CONVDONE_IRQ)
- complete(&data->completion);
-
-out:
- mutex_unlock(&data->mutex);
-
- return IRQ_HANDLED;
-}
-
-static int sx9310_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 sx9310_data *data = iio_priv(indio_dev);
-
- return !!(data->chan_event & BIT(chan->channel));
-}
-
-static int sx9310_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)
-{
- struct sx9310_data *data = iio_priv(indio_dev);
- unsigned int eventirq = SX9310_FAR_IRQ | SX9310_CLOSE_IRQ;
- int ret;
-
- /* If the state hasn't changed, there's nothing to do. */
- if (!!(data->chan_event & BIT(chan->channel)) == state)
- return 0;
-
- mutex_lock(&data->mutex);
- if (state) {
- ret = sx9310_get_event_channel(data, chan->channel);
- if (ret)
- goto out_unlock;
- if (!(data->chan_event & ~BIT(chan->channel))) {
- ret = sx9310_enable_irq(data, eventirq);
- if (ret)
- sx9310_put_event_channel(data, chan->channel);
- }
- } else {
- ret = sx9310_put_event_channel(data, chan->channel);
- if (ret)
- goto out_unlock;
- if (!data->chan_event) {
- ret = sx9310_disable_irq(data, eventirq);
- if (ret)
- sx9310_get_event_channel(data, chan->channel);
- }
- }
-
-out_unlock:
- mutex_unlock(&data->mutex);
- return ret;
-}
-
-static const struct iio_info sx9310_info = {
- .read_raw = sx9310_read_raw,
- .read_avail = sx9310_read_avail,
- .read_event_value = sx9310_read_event_val,
- .write_event_value = sx9310_write_event_val,
- .write_raw = sx9310_write_raw,
- .read_event_config = sx9310_read_event_config,
- .write_event_config = sx9310_write_event_config,
-};
-
-static int sx9310_set_trigger_state(struct iio_trigger *trig, bool state)
-{
- struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
- struct sx9310_data *data = iio_priv(indio_dev);
- int ret = 0;
-
- mutex_lock(&data->mutex);
-
- if (state)
- ret = sx9310_enable_irq(data, SX9310_CONVDONE_IRQ);
- else if (!data->chan_read)
- ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ);
- if (ret)
- goto out;
-
- data->trigger_enabled = state;
-
-out:
- mutex_unlock(&data->mutex);
-
- return ret;
-}
-
-static const struct iio_trigger_ops sx9310_trigger_ops = {
- .set_trigger_state = sx9310_set_trigger_state,
-};
-
-static irqreturn_t sx9310_trigger_handler(int irq, void *private)
-{
- struct iio_poll_func *pf = private;
- struct iio_dev *indio_dev = pf->indio_dev;
- struct sx9310_data *data = iio_priv(indio_dev);
- __be16 val;
- int bit, ret, i = 0;
-
- mutex_lock(&data->mutex);
-
- for_each_set_bit(bit, indio_dev->active_scan_mask,
- indio_dev->masklength) {
- ret = sx9310_read_prox_data(data, &indio_dev->channels[bit],
- &val);
- if (ret)
- goto out;
-
- data->buffer.channels[i++] = val;
- }
-
- iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
- pf->timestamp);
-
-out:
- mutex_unlock(&data->mutex);
-
- iio_trigger_notify_done(indio_dev->trig);
-
- return IRQ_HANDLED;
-}
-
-static int sx9310_buffer_preenable(struct iio_dev *indio_dev)
-{
- struct sx9310_data *data = iio_priv(indio_dev);
- unsigned long channels = 0;
- int bit, ret;
-
- mutex_lock(&data->mutex);
- for_each_set_bit(bit, indio_dev->active_scan_mask,
- indio_dev->masklength)
- __set_bit(indio_dev->channels[bit].channel, &channels);
-
- ret = sx9310_update_chan_en(data, channels, data->chan_event);
- mutex_unlock(&data->mutex);
- return ret;
-}
-
-static int sx9310_buffer_postdisable(struct iio_dev *indio_dev)
-{
- struct sx9310_data *data = iio_priv(indio_dev);
- int ret;
-
- mutex_lock(&data->mutex);
- ret = sx9310_update_chan_en(data, 0, data->chan_event);
- mutex_unlock(&data->mutex);
- return ret;
-}
-
-static const struct iio_buffer_setup_ops sx9310_buffer_setup_ops = {
- .preenable = sx9310_buffer_preenable,
- .postdisable = sx9310_buffer_postdisable,
-};
-
-struct sx9310_reg_default {
- u8 reg;
- u8 def;
-};
-
-static const struct sx9310_reg_default sx9310_default_regs[] = {
+static const struct sx_common_reg_default sx9310_default_regs[] = {
{ SX9310_REG_IRQ_MSK, 0x00 },
{ SX9310_REG_IRQ_FUNC, 0x00 },
/*
@@ -1172,7 +776,7 @@ static const struct sx9310_reg_default sx9310_default_regs[] = {
/* Activate all channels and perform an initial compensation. */
static int sx9310_init_compensation(struct iio_dev *indio_dev)
{
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
int ret;
unsigned int val;
unsigned int ctrl0;
@@ -1202,9 +806,9 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev)
return ret;
}
-static const struct sx9310_reg_default *
+static const struct sx_common_reg_default *
sx9310_get_default_reg(struct device *dev, int idx,
- struct sx9310_reg_default *reg_def)
+ struct sx_common_reg_default *reg_def)
{
u32 combined[SX9310_NUM_CHANNELS];
u32 start = 0, raw = 0, pos = 0;
@@ -1305,42 +909,20 @@ sx9310_get_default_reg(struct device *dev, int idx,
return reg_def;
}
-static int sx9310_init_device(struct iio_dev *indio_dev)
+static int sx9310_check_whoami(struct device *dev,
+ struct iio_dev *indio_dev)
{
- struct sx9310_data *data = iio_priv(indio_dev);
- struct sx9310_reg_default tmp;
- const struct sx9310_reg_default *initval;
+ struct sx_common_data *data = iio_priv(indio_dev);
+ unsigned int long ddata;
+ unsigned int whoami;
int ret;
- unsigned int i, val;
- ret = regmap_write(data->regmap, SX9310_REG_RESET, SX9310_SOFT_RESET);
- if (ret)
- return ret;
-
- usleep_range(1000, 2000); /* power-up time is ~1ms. */
-
- /* Clear reset interrupt state by reading SX9310_REG_IRQ_SRC. */
- ret = regmap_read(data->regmap, SX9310_REG_IRQ_SRC, &val);
- if (ret)
+ ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &whoami);
+ if (ret) {
+ dev_err(dev, "error in reading WHOAMI register: %d", ret);
return ret;
-
- /* Program some sane defaults. */
- for (i = 0; i < ARRAY_SIZE(sx9310_default_regs); i++) {
- initval = sx9310_get_default_reg(&indio_dev->dev, i, &tmp);
- ret = regmap_write(data->regmap, initval->reg, initval->def);
- if (ret)
- return ret;
}
- return sx9310_init_compensation(indio_dev);
-}
-
-static int sx9310_set_indio_dev_name(struct device *dev,
- struct iio_dev *indio_dev,
- unsigned int whoami)
-{
- unsigned int long ddata;
-
ddata = (uintptr_t)device_get_match_data(dev);
if (ddata != whoami) {
dev_err(dev, "WHOAMI does not match device data: %u\n", whoami);
@@ -1362,108 +944,47 @@ static int sx9310_set_indio_dev_name(struct device *dev,
return 0;
}
-static void sx9310_regulator_disable(void *_data)
-{
- struct sx9310_data *data = _data;
+static const struct sx_common_chip_info sx9310_chip_info = {
+ .reg_stat = SX9310_REG_STAT0,
+ .reg_irq_msk = SX9310_REG_IRQ_MSK,
+ .reg_enable_chan = SX9310_REG_PROX_CTRL0,
+ .reg_reset = SX9310_REG_RESET,
+
+ .mask_enable_chan = SX9310_REG_STAT1_COMPSTAT_MASK,
+ .irq_msk_offset = 3,
+ .num_channels = SX9310_NUM_CHANNELS,
+ .num_default_regs = ARRAY_SIZE(sx9310_default_regs),
+
+ .ops = {
+ .read_prox_data = sx9310_read_prox_data,
+ .check_whoami = sx9310_check_whoami,
+ .init_compensation = sx9310_init_compensation,
+ .wait_for_sample = sx9310_wait_for_sample,
+ .get_default_reg = sx9310_get_default_reg,
+ },
- regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
-}
+ .iio_channels = sx9310_channels,
+ .num_iio_channels = ARRAY_SIZE(sx9310_channels),
+ .iio_info = {
+ .read_raw = sx9310_read_raw,
+ .read_avail = sx9310_read_avail,
+ .read_event_value = sx9310_read_event_val,
+ .write_event_value = sx9310_write_event_val,
+ .write_raw = sx9310_write_raw,
+ .read_event_config = sx_common_read_event_config,
+ .write_event_config = sx_common_write_event_config,
+ },
+};
static int sx9310_probe(struct i2c_client *client)
{
- int ret;
- struct device *dev = &client->dev;
- struct iio_dev *indio_dev;
- struct sx9310_data *data;
-
- indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
- if (!indio_dev)
- return -ENOMEM;
-
- data = iio_priv(indio_dev);
- data->client = client;
- data->supplies[0].supply = "vdd";
- data->supplies[1].supply = "svdd";
- mutex_init(&data->mutex);
- init_completion(&data->completion);
-
- data->regmap = devm_regmap_init_i2c(client, &sx9310_regmap_config);
- if (IS_ERR(data->regmap))
- return PTR_ERR(data->regmap);
-
- ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
- data->supplies);
- if (ret)
- return ret;
-
- ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
- if (ret)
- return ret;
- /* Must wait for Tpor time after initial power up */
- usleep_range(1000, 1100);
-
- ret = devm_add_action_or_reset(dev, sx9310_regulator_disable, data);
- if (ret)
- return ret;
-
- ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami);
- if (ret) {
- dev_err(dev, "error in reading WHOAMI register: %d", ret);
- return ret;
- }
-
- ret = sx9310_set_indio_dev_name(dev, indio_dev, data->whoami);
- if (ret)
- return ret;
-
- ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(dev));
- indio_dev->channels = sx9310_channels;
- indio_dev->num_channels = ARRAY_SIZE(sx9310_channels);
- indio_dev->info = &sx9310_info;
- indio_dev->modes = INDIO_DIRECT_MODE;
- i2c_set_clientdata(client, indio_dev);
-
- ret = sx9310_init_device(indio_dev);
- if (ret)
- return ret;
-
- if (client->irq) {
- ret = devm_request_threaded_irq(dev, client->irq,
- sx9310_irq_handler,
- sx9310_irq_thread_handler,
- IRQF_ONESHOT,
- "sx9310_event", indio_dev);
- if (ret)
- return ret;
-
- data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
- indio_dev->name,
- iio_device_id(indio_dev));
- if (!data->trig)
- return -ENOMEM;
-
- data->trig->ops = &sx9310_trigger_ops;
- iio_trigger_set_drvdata(data->trig, indio_dev);
-
- ret = devm_iio_trigger_register(dev, data->trig);
- if (ret)
- return ret;
- }
-
- ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
- iio_pollfunc_store_time,
- sx9310_trigger_handler,
- &sx9310_buffer_setup_ops);
- if (ret)
- return ret;
-
- return devm_iio_device_register(dev, indio_dev);
+ return sx_common_probe(client, &sx9310_chip_info, &sx9310_regmap_config);
}
static int __maybe_unused sx9310_suspend(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
u8 ctrl0;
int ret;
@@ -1471,11 +992,11 @@ static int __maybe_unused sx9310_suspend(struct device *dev)
mutex_lock(&data->mutex);
ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0,
- &data->suspend_ctrl0);
+ &data->suspend_ctrl);
if (ret)
goto out;
- ctrl0 = data->suspend_ctrl0 & ~SX9310_REG_PROX_CTRL0_SENSOREN_MASK;
+ ctrl0 = data->suspend_ctrl & ~SX9310_REG_PROX_CTRL0_SENSOREN_MASK;
ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0, ctrl0);
if (ret)
goto out;
@@ -1490,7 +1011,7 @@ static int __maybe_unused sx9310_suspend(struct device *dev)
static int __maybe_unused sx9310_resume(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
int ret;
mutex_lock(&data->mutex);
@@ -1499,7 +1020,7 @@ static int __maybe_unused sx9310_resume(struct device *dev)
goto out;
ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0,
- data->suspend_ctrl0);
+ data->suspend_ctrl);
out:
mutex_unlock(&data->mutex);
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.
+ *
+ */
+
+#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>
+
+#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"
+
+/* 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);
+
+static irqreturn_t sx_common_irq_handler(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct sx_common_data *data = iio_priv(indio_dev);
+
+ if (data->trigger_enabled)
+ iio_trigger_poll(data->trig);
+
+ /*
+ * Even if no event is enabled, we need to wake the thread to clear the
+ * interrupt state by reading SX_COMMON_REG_IRQ_SRC.
+ * It is not possible to do that here because regmap_read takes a mutex.
+ */
+ return IRQ_WAKE_THREAD;
+}
+
+static void sx_common_push_events(struct iio_dev *indio_dev)
+{
+ int ret;
+ unsigned int val, chan;
+ struct sx_common_data *data = iio_priv(indio_dev);
+ s64 timestamp = iio_get_time_ns(indio_dev);
+ unsigned long prox_changed;
+
+ /* Read proximity state on all channels */
+ ret = regmap_read(data->regmap, data->chip_info->reg_stat, &val);
+ if (ret) {
+ dev_err(&data->client->dev, "i2c transfer error in irq\n");
+ return;
+ }
+
+ /*
+ * Only iterate over channels with changes on proximity status that have
+ * events enabled.
+ */
+ prox_changed = (data->chan_prox_stat ^ val) & data->chan_event;
+
+ for_each_set_bit(chan, &prox_changed, data->chip_info->num_channels) {
+ int dir;
+ u64 ev;
+
+ dir = (val & BIT(chan)) ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
+ ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan,
+ IIO_EV_TYPE_THRESH, dir);
+
+ iio_push_event(indio_dev, ev, timestamp);
+ }
+ data->chan_prox_stat = val;
+}
+
+static int sx_common_enable_irq(struct sx_common_data *data, unsigned int irq)
+{
+ if (!data->client->irq)
+ return 0;
+ return regmap_update_bits(data->regmap, data->chip_info->reg_irq_msk,
+ irq << data->chip_info->irq_msk_offset,
+ irq << data->chip_info->irq_msk_offset);
+}
+
+static int sx_common_disable_irq(struct sx_common_data *data, unsigned int irq)
+{
+ if (!data->client->irq)
+ return 0;
+ return regmap_update_bits(data->regmap, data->chip_info->reg_irq_msk,
+ irq << data->chip_info->irq_msk_offset, 0);
+}
+
+static int sx_common_update_chan_en(struct sx_common_data *data,
+ unsigned long chan_read,
+ unsigned long chan_event)
+{
+ int ret;
+ unsigned long channels = chan_read | chan_event;
+
+ if ((data->chan_read | data->chan_event) != channels) {
+ ret = regmap_update_bits(data->regmap,
+ data->chip_info->reg_enable_chan,
+ data->chip_info->mask_enable_chan,
+ channels);
+ if (ret)
+ return ret;
+ }
+ data->chan_read = chan_read;
+ data->chan_event = chan_event;
+ return 0;
+}
+
+static int sx_common_get_read_channel(struct sx_common_data *data, int channel)
+{
+ return sx_common_update_chan_en(data, data->chan_read | BIT(channel),
+ data->chan_event);
+}
+
+static int sx_common_put_read_channel(struct sx_common_data *data, int channel)
+{
+ return sx_common_update_chan_en(data, data->chan_read & ~BIT(channel),
+ data->chan_event);
+}
+
+static int sx_common_get_event_channel(struct sx_common_data *data, int channel)
+{
+ return sx_common_update_chan_en(data, data->chan_read,
+ data->chan_event | BIT(channel));
+}
+
+static int sx_common_put_event_channel(struct sx_common_data *data, int channel)
+{
+ return sx_common_update_chan_en(data, data->chan_read,
+ data->chan_event & ~BIT(channel));
+}
+
+/**
+ * sx_common_read_proximity() - Read raw proximity value.
+ *
+ * @data: Internal data
+ * @chan: Channel to read
+ * @val: pointer to return read value.
+ *
+ * Request a conversion, wait for the sensor to be ready and
+ * return the raw proximity value.
+ */
+int sx_common_read_proximity(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int *val)
+{
+ int ret;
+ __be16 rawval;
+
+ mutex_lock(&data->mutex);
+
+ ret = sx_common_get_read_channel(data, chan->channel);
+ if (ret)
+ goto out;
+
+ ret = sx_common_enable_irq(data, SX_COMMON_CONVDONE_IRQ);
+ if (ret)
+ goto out_put_channel;
+
+ mutex_unlock(&data->mutex);
+
+ if (data->client->irq) {
+ ret = wait_for_completion_interruptible(&data->completion);
+ reinit_completion(&data->completion);
+ } else {
+ ret = data->chip_info->ops.wait_for_sample(data);
+ }
+
+ mutex_lock(&data->mutex);
+
+ if (ret)
+ goto out_disable_irq;
+
+ ret = data->chip_info->ops.read_prox_data(data, chan, &rawval);
+ if (ret)
+ goto out_disable_irq;
+
+ *val = sign_extend32(be16_to_cpu(rawval), chan->scan_type.realbits - 1);
+
+ ret = sx_common_disable_irq(data, SX_COMMON_CONVDONE_IRQ);
+ if (ret)
+ goto out_put_channel;
+
+ ret = sx_common_put_read_channel(data, chan->channel);
+ if (ret)
+ goto out;
+
+ mutex_unlock(&data->mutex);
+
+ return IIO_VAL_INT;
+
+out_disable_irq:
+ sx_common_disable_irq(data, SX_COMMON_CONVDONE_IRQ);
+out_put_channel:
+ sx_common_put_read_channel(data, chan->channel);
+out:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(sx_common_read_proximity);
+
+/**
+ * sx_common_read_event_config() - Configure event setting.
+ *
+ * @indio_dev: iio device object
+ * @chan: Channel to read
+ * @type: Type of event (unused)
+ * @dir: Direction of event (unused)
+ *
+ * return if the given channel is used for event gathering.
+ */
+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)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+
+ return !!(data->chan_event & BIT(chan->channel));
+}
+EXPORT_SYMBOL_GPL(sx_common_read_event_config);
+
+/**
+ * sx_common_write_event_config() - Configure event setting.
+ *
+ * @indio_dev: iio device object
+ * @chan: Channel to enable
+ * @type: Type of event (unused)
+ * @dir: Direction of event (unused)
+ * @state: State of the event.
+ *
+ * Enable/Disable event on a given channel.
+ */
+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)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+ unsigned int eventirq = SX_COMMON_FAR_IRQ | SX_COMMON_CLOSE_IRQ;
+ int ret;
+
+ /* If the state hasn't changed, there's nothing to do. */
+ if (!!(data->chan_event & BIT(chan->channel)) == state)
+ return 0;
+
+ mutex_lock(&data->mutex);
+ if (state) {
+ ret = sx_common_get_event_channel(data, chan->channel);
+ if (ret)
+ goto out_unlock;
+ if (!(data->chan_event & ~BIT(chan->channel))) {
+ ret = sx_common_enable_irq(data, eventirq);
+ if (ret)
+ sx_common_put_event_channel(data, chan->channel);
+ }
+ } else {
+ ret = sx_common_put_event_channel(data, chan->channel);
+ if (ret)
+ goto out_unlock;
+ if (!data->chan_event) {
+ ret = sx_common_disable_irq(data, eventirq);
+ if (ret)
+ sx_common_get_event_channel(data, chan->channel);
+ }
+ }
+
+out_unlock:
+ mutex_unlock(&data->mutex);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(sx_common_write_event_config);
+
+static int sx_common_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+ struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+ struct sx_common_data *data = iio_priv(indio_dev);
+ int ret = 0;
+
+ mutex_lock(&data->mutex);
+
+ if (state)
+ ret = sx_common_enable_irq(data, SX_COMMON_CONVDONE_IRQ);
+ else if (!data->chan_read)
+ ret = sx_common_disable_irq(data, SX_COMMON_CONVDONE_IRQ);
+ if (ret)
+ goto out;
+
+ data->trigger_enabled = state;
+
+out:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static const struct iio_trigger_ops sx_common_trigger_ops = {
+ .set_trigger_state = sx_common_set_trigger_state,
+};
+
+static irqreturn_t sx_common_irq_thread_handler(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct sx_common_data *data = iio_priv(indio_dev);
+ int ret;
+ unsigned int val;
+
+ mutex_lock(&data->mutex);
+
+ ret = regmap_read(data->regmap, SX_COMMON_REG_IRQ_SRC, &val);
+ if (ret) {
+ dev_err(&data->client->dev, "i2c transfer error in irq\n");
+ goto out;
+ }
+
+ if (val & ((SX_COMMON_FAR_IRQ | SX_COMMON_CLOSE_IRQ) << data->chip_info->irq_msk_offset))
+ sx_common_push_events(indio_dev);
+
+ if (val & (SX_COMMON_CONVDONE_IRQ << data->chip_info->irq_msk_offset))
+ complete(&data->completion);
+
+out:
+ mutex_unlock(&data->mutex);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t sx_common_trigger_handler(int irq, void *private)
+{
+ struct iio_poll_func *pf = private;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct sx_common_data *data = iio_priv(indio_dev);
+ __be16 val;
+ int bit, ret, i = 0;
+
+ mutex_lock(&data->mutex);
+
+ for_each_set_bit(bit, indio_dev->active_scan_mask,
+ indio_dev->masklength) {
+ ret = data->chip_info->ops.read_prox_data(data,
+ &indio_dev->channels[bit],
+ &val);
+ if (ret)
+ goto out;
+
+ data->buffer.channels[i++] = val;
+ }
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
+ pf->timestamp);
+
+out:
+ mutex_unlock(&data->mutex);
+
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static int sx_common_buffer_preenable(struct iio_dev *indio_dev)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+ unsigned long channels = 0;
+ int bit, ret;
+
+ mutex_lock(&data->mutex);
+ for_each_set_bit(bit, indio_dev->active_scan_mask,
+ indio_dev->masklength)
+ __set_bit(indio_dev->channels[bit].channel, &channels);
+
+ ret = sx_common_update_chan_en(data, channels, data->chan_event);
+ mutex_unlock(&data->mutex);
+ return ret;
+}
+
+static int sx_common_buffer_postdisable(struct iio_dev *indio_dev)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = sx_common_update_chan_en(data, 0, data->chan_event);
+ mutex_unlock(&data->mutex);
+ return ret;
+}
+
+static const struct iio_buffer_setup_ops sx_common_buffer_setup_ops = {
+ .preenable = sx_common_buffer_preenable,
+ .postdisable = sx_common_buffer_postdisable,
+};
+
+static void sx_common_regulator_disable(void *_data)
+{
+ struct sx_common_data *data = _data;
+
+ regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
+}
+
+#define SX_COMMON_SOFT_RESET 0xde
+
+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);
+ 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);
+}
+
+/**
+ * sx_common_probe() - Common setup for Semtech SAR sensor
+ *
+ * @client: I2C client object
+ * @chip_info: Semtech sensor chip information.
+ * @regmap_config: Sensor registers map configuration.
+ */
+int sx_common_probe(struct i2c_client *client,
+ const struct sx_common_chip_info *chip_info,
+ const struct regmap_config *regmap_config)
+{
+ struct device *dev = &client->dev;
+ struct iio_dev *indio_dev;
+ struct sx_common_data *data;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+
+ data->chip_info = chip_info;
+ data->client = client;
+ data->supplies[0].supply = "vdd";
+ data->supplies[1].supply = "svdd";
+ mutex_init(&data->mutex);
+ init_completion(&data->completion);
+
+ data->regmap = devm_regmap_init_i2c(client, regmap_config);
+ if (IS_ERR(data->regmap))
+ return PTR_ERR(data->regmap);
+
+ ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
+ data->supplies);
+ if (ret)
+ return ret;
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
+ if (ret)
+ return ret;
+ /* Must wait for Tpor time after initial power up */
+ usleep_range(1000, 1100);
+
+ ret = devm_add_action_or_reset(dev, sx_common_regulator_disable, data);
+ if (ret)
+ return ret;
+
+ ret = data->chip_info->ops.check_whoami(dev, indio_dev);
+ if (ret) {
+ dev_err(dev, "error in reading WHOAMI register: %d", ret);
+ return ret;
+ }
+
+ ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(dev));
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ indio_dev->channels = data->chip_info->iio_channels;
+ indio_dev->num_channels = data->chip_info->num_iio_channels;
+ indio_dev->info = &data->chip_info->iio_info;
+
+ i2c_set_clientdata(client, indio_dev);
+
+ ret = sx_common_init_device(indio_dev);
+ if (ret)
+ return ret;
+
+ if (client->irq) {
+ ret = devm_request_threaded_irq(dev, client->irq,
+ sx_common_irq_handler,
+ sx_common_irq_thread_handler,
+ IRQF_ONESHOT,
+ "sx_event", indio_dev);
+ if (ret)
+ return ret;
+
+ data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+ indio_dev->name,
+ iio_device_id(indio_dev));
+ if (!data->trig)
+ return -ENOMEM;
+
+ data->trig->ops = &sx_common_trigger_ops;
+ iio_trigger_set_drvdata(data->trig, indio_dev);
+
+ ret = devm_iio_trigger_register(dev, data->trig);
+ if (ret)
+ return ret;
+ }
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+ iio_pollfunc_store_time,
+ sx_common_trigger_handler,
+ &sx_common_buffer_setup_ops);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+EXPORT_SYMBOL_GPL(sx_common_probe);
+
+MODULE_AUTHOR("Gwendal Grignou <gwendal@chromium.org>");
+MODULE_DESCRIPTION("Common functions and structures for Semtech sensor");
+MODULE_LICENSE("GPL v2");
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>
+
+#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 */
--
2.34.0.rc2.393.gf8c9666880-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v4 2/5] iio: sx9310: Extract common Semtech sensor logic
2021-11-20 10:14 ` [PATCH v4 2/5] iio: sx9310: Extract common Semtech sensor logic Gwendal Grignou
@ 2021-11-20 15:04 ` Jonathan Cameron
2021-11-20 15:19 ` Jonathan Cameron
0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2021-11-20 15:04 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: lars, swboyd, andy.shevchenko, linux-iio
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
> ---
> 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 */
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4 2/5] iio: sx9310: Extract common Semtech sensor logic
2021-11-20 15:04 ` Jonathan Cameron
@ 2021-11-20 15:19 ` Jonathan Cameron
2021-12-08 7:18 ` Gwendal Grignou
0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2021-11-20 15:19 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: lars, swboyd, andy.shevchenko, linux-iio
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 */
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4 2/5] iio: sx9310: Extract common Semtech sensor logic
2021-11-20 15:19 ` Jonathan Cameron
@ 2021-12-08 7:18 ` Gwendal Grignou
0 siblings, 0 replies; 11+ messages in thread
From: Gwendal Grignou @ 2021-12-08 7:18 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: lars, swboyd, andy.shevchenko, linux-iio
On Sat, Nov 20, 2021 at 7:14 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> 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.
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 */
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4 3/5] iio: proximity: Add SX9324 support
2021-11-20 10:14 [PATCH v4 0/5] Expand Semtech SAR Sensors support Gwendal Grignou
2021-11-20 10:14 ` [PATCH v4 1/5] iio: sx9310: Add frequency in read_avail Gwendal Grignou
2021-11-20 10:14 ` [PATCH v4 2/5] iio: sx9310: Extract common Semtech sensor logic Gwendal Grignou
@ 2021-11-20 10:14 ` Gwendal Grignou
2021-11-20 15:27 ` Jonathan Cameron
2021-11-20 10:15 ` [PATCH v4 4/5] dt-bindings: iio: Add sx9324 binding Gwendal Grignou
2021-11-20 10:15 ` [PATCH v4 5/5] iio: sx9324: Add dt_binding support Gwendal Grignou
4 siblings, 1 reply; 11+ messages in thread
From: Gwendal Grignou @ 2021-11-20 10:14 UTC (permalink / raw)
To: jic23, lars, swboyd; +Cc: andy.shevchenko, linux-iio, Gwendal Grignou
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>
---
Changes in v4:
- Remove SX9324_DRIVER_NAME define
- Drop comma after sentinel
- Handle negative values when setting parameters.
- Simpliy whoami function.
Changes in v3:
- Fix constant names
- Disable IRQ at suspend.
- Concatenate 2 lines.
Changes in v2:
- Use new interface with sx_common.
- Document phase configuration parameter.
- Define phase configuration parameter in channel.
- Only Add needed changes in Kconfig and Makefile.
- Use read_avail to define available frequencies
.../ABI/testing/sysfs-bus-iio-sx9324 | 28 +
drivers/iio/proximity/Kconfig | 14 +
drivers/iio/proximity/Makefile | 1 +
drivers/iio/proximity/sx9324.c | 916 ++++++++++++++++++
4 files changed, 959 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-sx9324
create mode 100644 drivers/iio/proximity/sx9324.c
diff --git a/Documentation/ABI/testing/sysfs-bus-iio-sx9324 b/Documentation/ABI/testing/sysfs-bus-iio-sx9324
new file mode 100644
index 00000000000000..632e3321f5a3df
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-sx9324
@@ -0,0 +1,28 @@
+What: /sys/bus/iio/devices/iio:deviceX/in_proximity<id>_setup
+Date: November 2021
+KernelVersion: 5.17
+Contact: Gwendal Grignou <gwendal@chromium.org>
+Description:
+ SX9324 has 3 inputs, CS0, CS1 and CS2. Hardware layout
+ defines if the input is
+ + not connected (HZ),
+ + grounded (GD),
+ + connected to an antenna where it can act as a base
+ (DS - data shield), or measured input (MI).
+
+ The sensor rotates measurement across 4 phases
+ (PH0, PH1, PH2, PH3), where the inputs are configured
+ and then measured.
+
+ By default, during the first phase, [PH0], CS0 is measured,
+ while CS1 and CS2 are used as shields.
+ `cat in_proximity0_setup` returns "MI,DS,DS".
+ [PH1], CS1 is measured, CS0 and CS2 are shield:
+ `cat in_proximity1_setup` returns "DS,MI,DS".
+ [PH2], CS2 is measured, CS0 and CS1 are shield:
+ `cat in_proximity1_setup` returns "DS,DS,MI".
+ [PH3], CS1 and CS2 are measured (combo mode):
+ `cat in_proximity1_setup` returns "DS,MI,MI".
+
+ Note, these are the chip default. Hardware layout will most
+ likely dictate different output. The entry is read-only.
diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index e88fc373c2c903..aaddf97f9b2192 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -129,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 2577fbce4144e5..cffe962b352718 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_SX9324) += sx9324.o
obj-$(CONFIG_SX_COMMON) += sx_common.o
obj-$(CONFIG_SX9500) += sx9500.o
obj-$(CONFIG_VCNL3020) += vcnl3020.o
diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
new file mode 100644
index 00000000000000..3858e70d7a86f6
--- /dev/null
+++ b/drivers/iio/proximity/sx9324.c
@@ -0,0 +1,916 @@
+// 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
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/irq.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/log2.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/regmap.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"
+
+/* Register definitions. */
+#define SX9324_REG_IRQ_SRC SX_COMMON_REG_IRQ_SRC
+#define SX9324_REG_STAT0 0x01
+#define SX9324_REG_STAT1 0x02
+#define SX9324_REG_STAT2 0x03
+#define SX9324_REG_STAT2_COMPSTAT_MASK GENMASK(3, 0)
+#define SX9324_REG_STAT3 0x04
+#define SX9324_REG_IRQ_MSK 0x05
+#define SX9324_CONVDONE_IRQ BIT(3)
+#define SX9324_FAR_IRQ BIT(5)
+#define SX9324_CLOSE_IRQ BIT(6)
+#define SX9324_REG_IRQ_CFG0 0x06
+#define SX9324_REG_IRQ_CFG1 0x07
+#define SX9324_REG_IRQ_CFG1_FAILCOND 0x80
+#define SX9324_REG_IRQ_CFG2 0x08
+
+#define SX9324_REG_GNRL_CTRL0 0x10
+#define SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK GENMASK(4, 0)
+#define SX9324_REG_GNRL_CTRL0_SCANPERIOD_100MS 0x16
+#define SX9324_REG_GNRL_CTRL1 0x11
+#define SX9324_REG_GNRL_CTRL1_PHEN_MASK GENMASK(3, 0)
+#define SX9324_REG_GNRL_CTRL1_PAUSECTRL 0x20
+
+#define SX9324_REG_I2C_ADDR 0x14
+#define SX9324_REG_CLK_SPRD 0x15
+
+#define SX9324_REG_AFE_CTRL0 0x20
+#define SX9324_REG_AFE_CTRL1 0x21
+#define SX9324_REG_AFE_CTRL2 0x22
+#define SX9324_REG_AFE_CTRL3 0x23
+#define SX9324_REG_AFE_CTRL4 0x24
+#define SX9324_REG_AFE_CTRL4_FREQ_83_33HZ 0x40
+#define SX9324_REG_AFE_CTRL4_RESOLUTION_MASK GENMASK(2, 0)
+#define SX9324_REG_AFE_CTRL4_RES_100 0x04
+#define SX9324_REG_AFE_CTRL5 0x25
+#define SX9324_REG_AFE_CTRL6 0x26
+#define SX9324_REG_AFE_CTRL7 0x27
+#define SX9324_REG_AFE_PH0 0x28
+#define SX9324_REG_AFE_PH0_PIN_MASK(_pin) \
+ GENMASK(2 * (_pin) + 1, 2 * (_pin))
+
+#define SX9324_REG_AFE_PH1 0x29
+#define SX9324_REG_AFE_PH2 0x2a
+#define SX9324_REG_AFE_PH3 0x2b
+#define SX9324_REG_AFE_CTRL8 0x2c
+#define SX9324_REG_AFE_CTRL8_RESFILTN_4KOHM 0x02
+#define SX9324_REG_AFE_CTRL9 0x2d
+#define SX9324_REG_AFE_CTRL9_AGAIN_1 0x08
+
+#define SX9324_REG_PROX_CTRL0 0x30
+#define SX9324_REG_PROX_CTRL0_GAIN_MASK GENMASK(5, 3)
+#define SX9324_REG_PROX_CTRL0_GAIN_1 0x80
+#define SX9324_REG_PROX_CTRL0_RAWFILT_1P50 0x01
+#define SX9324_REG_PROX_CTRL1 0x31
+#define SX9324_REG_PROX_CTRL2 0x32
+#define SX9324_REG_PROX_CTRL2_AVGNEG_THRESH_16K 0x20
+#define SX9324_REG_PROX_CTRL3 0x33
+#define SX9324_REG_PROX_CTRL3_AVGDEB_2SAMPLES 0x40
+#define SX9324_REG_PROX_CTRL3_AVGPOS_THRESH_16K 0x20
+#define SX9324_REG_PROX_CTRL4 0x34
+#define SX9324_REG_PROX_CTRL4_AVGNEGFILT_MASK GENMASK(5, 3)
+#define SX9324_REG_PROX_CTRL4_AVGNEG_FILT_2 0x08
+#define SX9324_REG_PROX_CTRL4_AVGPOSFILT_MASK GENMASK(2, 0)
+#define SX9324_REG_PROX_CTRL3_AVGPOS_FILT_256 0x04
+#define SX9324_REG_PROX_CTRL5 0x35
+#define SX9324_REG_PROX_CTRL5_HYST_MASK GENMASK(5, 4)
+#define SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK GENMASK(3, 2)
+#define SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK GENMASK(1, 0)
+#define SX9324_REG_PROX_CTRL6 0x36
+#define SX9324_REG_PROX_CTRL6_PROXTHRESH_32 0x08
+#define SX9324_REG_PROX_CTRL7 0x37
+
+#define SX9324_REG_ADV_CTRL0 0x40
+#define SX9324_REG_ADV_CTRL1 0x41
+#define SX9324_REG_ADV_CTRL2 0x42
+#define SX9324_REG_ADV_CTRL3 0x43
+#define SX9324_REG_ADV_CTRL4 0x44
+#define SX9324_REG_ADV_CTRL5 0x45
+#define SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK GENMASK(3, 2)
+#define SX9324_REG_ADV_CTRL5_STARTUP_SENSOR_1 0x04
+#define SX9324_REG_ADV_CTRL5_STARTUP_METHOD_1 0x01
+#define SX9324_REG_ADV_CTRL6 0x46
+#define SX9324_REG_ADV_CTRL7 0x47
+#define SX9324_REG_ADV_CTRL8 0x48
+#define SX9324_REG_ADV_CTRL9 0x49
+#define SX9324_REG_ADV_CTRL10 0x4a
+#define SX9324_REG_ADV_CTRL11 0x4b
+#define SX9324_REG_ADV_CTRL12 0x4c
+#define SX9324_REG_ADV_CTRL13 0x4d
+#define SX9324_REG_ADV_CTRL14 0x4e
+#define SX9324_REG_ADV_CTRL15 0x4f
+#define SX9324_REG_ADV_CTRL16 0x50
+#define SX9324_REG_ADV_CTRL17 0x51
+#define SX9324_REG_ADV_CTRL18 0x52
+#define SX9324_REG_ADV_CTRL19 0x53
+#define SX9324_REG_ADV_CTRL20 0x54
+#define SX9324_REG_ADV_CTRL19_HIGHT_FAILURE_THRESH_SATURATION 0xf0
+
+#define SX9324_REG_PHASE_SEL 0x60
+
+#define SX9324_REG_USEFUL_MSB 0x61
+#define SX9324_REG_USEFUL_LSB 0x62
+
+#define SX9324_REG_AVG_MSB 0x63
+#define SX9324_REG_AVG_LSB 0x64
+
+#define SX9324_REG_DIFF_MSB 0x65
+#define SX9324_REG_DIFF_LSB 0x66
+
+#define SX9324_REG_OFFSET_MSB 0x67
+#define SX9324_REG_OFFSET_LSB 0x68
+
+#define SX9324_REG_SAR_MSB 0x69
+#define SX9324_REG_SAR_LSB 0x6a
+
+#define SX9324_REG_RESET 0x9f
+/* Write this to REG_RESET to do a soft reset. */
+#define SX9324_SOFT_RESET 0xde
+
+#define SX9324_REG_WHOAMI 0xfa
+#define SX9324_WHOAMI_VALUE 0x23
+
+#define SX9324_REG_REVISION 0xfe
+
+/* 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
+
+static const char * const sx9324_cs_pin_usage[] = { "HZ", "MI", "DS", "GD" };
+
+static ssize_t sx9324_phase_configuration_show(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ char *buf)
+{
+ 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 + chan->channel, &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 const struct iio_chan_spec_ext_info sx9324_channel_ext_info[] = {
+ {
+ .name = "setting",
+ .shared = IIO_SEPARATE,
+ .read = sx9324_phase_configuration_show,
+ },
+ {}
+};
+
+#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), \
+ .info_mask_shared_by_all_available = \
+ BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+ .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, \
+ }, \
+ .ext_info = sx9324_channel_ext_info, \
+}
+
+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),
+};
+
+/*
+ * Each entry contains the integer part (val) and the fractional part, in micro
+ * seconds. It conforms to the IIO output IIO_VAL_INT_PLUS_MICRO.
+ */
+static const struct {
+ int val;
+ int val2;
+} sx9324_samp_freq_table[] = {
+ {1000, 0}, /* 00000: Min (no idle time) */
+ {500, 0}, /* 00001: 2 ms */
+ {250, 0}, /* 00010: 4 ms */
+ {166, 666666}, /* 00011: 6 ms */
+ {125, 0}, /* 00100: 8 ms */
+ {100, 0}, /* 00101: 10 ms */
+ {71, 428571}, /* 00110: 14 ms */
+ {55, 555556}, /* 00111: 18 ms */
+ {45, 454545}, /* 01000: 22 ms */
+ {38, 461538}, /* 01001: 26 ms */
+ {33, 333333}, /* 01010: 30 ms */
+ {29, 411765}, /* 01011: 34 ms */
+ {26, 315789}, /* 01100: 38 ms */
+ {23, 809524}, /* 01101: 42 ms */
+ {21, 739130}, /* 01110: 46 ms */
+ {20, 0}, /* 01111: 50 ms */
+ {17, 857143}, /* 10000: 56 ms */
+ {16, 129032}, /* 10001: 62 ms */
+ {14, 705882}, /* 10010: 68 ms */
+ {13, 513514}, /* 10011: 74 ms */
+ {12, 500000}, /* 10100: 80 ms */
+ {11, 111111}, /* 10101: 90 ms */
+ {10, 0}, /* 10110: 100 ms (Typ.) */
+ {5, 0}, /* 10111: 200 ms */
+ {3, 333333}, /* 11000: 300 ms */
+ {2, 500000}, /* 11001: 400 ms */
+ {1, 666667}, /* 11010: 600 ms */
+ {1, 250000}, /* 11011: 800 ms */
+ {1, 0}, /* 11100: 1 s */
+ {0, 500000}, /* 11101: 2 s */
+ {0, 333333}, /* 11110: 3 s */
+ {0, 250000}, /* 11111: 4 s */
+};
+
+static const unsigned int sx9324_scan_period_table[] = {
+ 2, 15, 30, 45, 60, 90, 120, 200,
+ 400, 600, 800, 1000, 2000, 3000, 4000, 5000,
+};
+
+static const struct regmap_range sx9324_writable_reg_ranges[] = {
+ /*
+ * To set COMPSTAT for compensation, even if datasheet says register is
+ * RO.
+ */
+ regmap_reg_range(SX9324_REG_STAT2, SX9324_REG_STAT2),
+ regmap_reg_range(SX9324_REG_IRQ_MSK, SX9324_REG_IRQ_CFG2),
+ regmap_reg_range(SX9324_REG_GNRL_CTRL0, SX9324_REG_GNRL_CTRL1),
+ /* Leave i2c and clock spreading as unavailable */
+ regmap_reg_range(SX9324_REG_AFE_CTRL0, SX9324_REG_AFE_CTRL9),
+ regmap_reg_range(SX9324_REG_PROX_CTRL0, SX9324_REG_PROX_CTRL7),
+ regmap_reg_range(SX9324_REG_ADV_CTRL0, SX9324_REG_ADV_CTRL20),
+ regmap_reg_range(SX9324_REG_PHASE_SEL, SX9324_REG_PHASE_SEL),
+ regmap_reg_range(SX9324_REG_OFFSET_MSB, SX9324_REG_OFFSET_LSB),
+ regmap_reg_range(SX9324_REG_RESET, SX9324_REG_RESET),
+};
+
+static const struct regmap_access_table sx9324_writeable_regs = {
+ .yes_ranges = sx9324_writable_reg_ranges,
+ .n_yes_ranges = ARRAY_SIZE(sx9324_writable_reg_ranges),
+};
+
+/*
+ * All allocated registers are readable, so we just list unallocated
+ * ones.
+ */
+static const struct regmap_range sx9324_non_readable_reg_ranges[] = {
+ regmap_reg_range(SX9324_REG_IRQ_CFG2 + 1, SX9324_REG_GNRL_CTRL0 - 1),
+ regmap_reg_range(SX9324_REG_GNRL_CTRL1 + 1, SX9324_REG_AFE_CTRL0 - 1),
+ regmap_reg_range(SX9324_REG_AFE_CTRL9 + 1, SX9324_REG_PROX_CTRL0 - 1),
+ regmap_reg_range(SX9324_REG_PROX_CTRL7 + 1, SX9324_REG_ADV_CTRL0 - 1),
+ regmap_reg_range(SX9324_REG_ADV_CTRL20 + 1, SX9324_REG_PHASE_SEL - 1),
+ regmap_reg_range(SX9324_REG_SAR_LSB + 1, SX9324_REG_RESET - 1),
+ regmap_reg_range(SX9324_REG_RESET + 1, SX9324_REG_WHOAMI - 1),
+ regmap_reg_range(SX9324_REG_WHOAMI + 1, SX9324_REG_REVISION - 1),
+};
+
+static const struct regmap_access_table sx9324_readable_regs = {
+ .no_ranges = sx9324_non_readable_reg_ranges,
+ .n_no_ranges = ARRAY_SIZE(sx9324_non_readable_reg_ranges),
+};
+
+static const struct regmap_range sx9324_volatile_reg_ranges[] = {
+ regmap_reg_range(SX9324_REG_IRQ_SRC, SX9324_REG_STAT3),
+ regmap_reg_range(SX9324_REG_USEFUL_MSB, SX9324_REG_DIFF_LSB),
+ regmap_reg_range(SX9324_REG_SAR_MSB, SX9324_REG_SAR_LSB),
+ regmap_reg_range(SX9324_REG_WHOAMI, SX9324_REG_WHOAMI),
+ regmap_reg_range(SX9324_REG_REVISION, SX9324_REG_REVISION),
+};
+
+static const struct regmap_access_table sx9324_volatile_regs = {
+ .yes_ranges = sx9324_volatile_reg_ranges,
+ .n_yes_ranges = ARRAY_SIZE(sx9324_volatile_reg_ranges),
+};
+
+static const struct regmap_config sx9324_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+
+ .max_register = SX9324_REG_REVISION,
+ .cache_type = REGCACHE_RBTREE,
+
+ .wr_table = &sx9324_writeable_regs,
+ .rd_table = &sx9324_readable_regs,
+ .volatile_table = &sx9324_volatile_regs,
+};
+
+static int sx9324_read_prox_data(struct sx_common_data *data,
+ const struct iio_chan_spec *chan,
+ __be16 *val)
+{
+ int ret;
+
+ ret = regmap_write(data->regmap, SX9324_REG_PHASE_SEL, chan->channel);
+ if (ret < 0)
+ return ret;
+
+ return regmap_bulk_read(data->regmap, chan->address, val, sizeof(*val));
+}
+
+/*
+ * If we have no interrupt support, we have to wait for a scan period
+ * after enabling a channel to get a result.
+ */
+static int sx9324_wait_for_sample(struct sx_common_data *data)
+{
+ int ret;
+ unsigned int val;
+
+ ret = regmap_read(data->regmap, SX9324_REG_GNRL_CTRL0, &val);
+ if (ret < 0)
+ return ret;
+ val = FIELD_GET(SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, val);
+
+ msleep(sx9324_scan_period_table[val]);
+
+ return 0;
+}
+
+static int sx9324_read_gain(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int *val)
+{
+ unsigned int reg, regval;
+ int ret;
+
+ reg = SX9324_REG_PROX_CTRL0 + chan->channel / 2;
+ ret = regmap_read(data->regmap, reg, ®val);
+ if (ret)
+ return ret;
+
+ *val = 1 << FIELD_GET(SX9324_REG_PROX_CTRL0_GAIN_MASK, regval);
+
+ return IIO_VAL_INT;
+}
+
+static int sx9324_read_samp_freq(struct sx_common_data *data,
+ int *val, int *val2)
+{
+ int ret;
+ unsigned int regval;
+
+ ret = regmap_read(data->regmap, SX9324_REG_GNRL_CTRL0, ®val);
+ if (ret)
+ return ret;
+
+ regval = FIELD_GET(SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, regval);
+ *val = sx9324_samp_freq_table[regval].val;
+ *val2 = sx9324_samp_freq_table[regval].val2;
+
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int sx9324_read_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);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ ret = sx_common_read_proximity(data, chan, val);
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
+ case IIO_CHAN_INFO_HARDWAREGAIN:
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ ret = sx9324_read_gain(data, chan, val);
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return sx9324_read_samp_freq(data, val, val2);
+ default:
+ return -EINVAL;
+ }
+}
+
+static const int sx9324_gain_vals[] = { 1, 2, 4, 8 };
+
+static int sx9324_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ if (chan->type != IIO_PROXIMITY)
+ return -EINVAL;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_HARDWAREGAIN:
+ *type = IIO_VAL_INT;
+ *length = ARRAY_SIZE(sx9324_gain_vals);
+ *vals = sx9324_gain_vals;
+ return IIO_AVAIL_LIST;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ *length = ARRAY_SIZE(sx9324_samp_freq_table) * 2;
+ *vals = (int *)sx9324_samp_freq_table;
+ return IIO_AVAIL_LIST;
+ }
+
+ return -EINVAL;
+}
+
+static int sx9324_set_samp_freq(struct sx_common_data *data,
+ int val, int val2)
+{
+ int i, ret;
+
+ for (i = 0; i < ARRAY_SIZE(sx9324_samp_freq_table); i++)
+ if (val == sx9324_samp_freq_table[i].val &&
+ val2 == sx9324_samp_freq_table[i].val2)
+ break;
+
+ if (i == ARRAY_SIZE(sx9324_samp_freq_table))
+ return -EINVAL;
+
+ mutex_lock(&data->mutex);
+
+ ret = regmap_update_bits(data->regmap,
+ SX9324_REG_GNRL_CTRL0,
+ SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, i);
+
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int sx9324_read_thresh(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int *val)
+{
+ unsigned int regval;
+ unsigned int reg;
+ int ret;
+
+ /*
+ * TODO(gwendal): Depending on the phase function
+ * (proximity/table/body), retrieve the right threshold.
+ * For now, return the proximity threshold.
+ */
+ reg = SX9324_REG_PROX_CTRL6 + chan->channel / 2;
+ ret = regmap_read(data->regmap, reg, ®val);
+ if (ret)
+ return ret;
+
+ if (regval <= 1)
+ *val = regval;
+ else
+ *val = (regval * regval) / 2;
+
+ return IIO_VAL_INT;
+}
+
+static int sx9324_read_hysteresis(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int *val)
+{
+ unsigned int regval, pthresh;
+ int ret;
+
+ ret = sx9324_read_thresh(data, chan, &pthresh);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_read(data->regmap, SX9324_REG_PROX_CTRL5, ®val);
+ if (ret)
+ return ret;
+
+ regval = FIELD_GET(SX9324_REG_PROX_CTRL5_HYST_MASK, regval);
+ if (!regval)
+ *val = 0;
+ else
+ *val = pthresh >> (5 - regval);
+
+ return IIO_VAL_INT;
+}
+
+static int sx9324_read_far_debounce(struct sx_common_data *data, int *val)
+{
+ unsigned int regval;
+ int ret;
+
+ ret = regmap_read(data->regmap, SX9324_REG_PROX_CTRL5, ®val);
+ if (ret)
+ return ret;
+
+ regval = FIELD_GET(SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK, regval);
+ if (regval)
+ *val = 1 << regval;
+ else
+ *val = 0;
+
+ return IIO_VAL_INT;
+}
+
+static int sx9324_read_close_debounce(struct sx_common_data *data, int *val)
+{
+ unsigned int regval;
+ int ret;
+
+ ret = regmap_read(data->regmap, SX9324_REG_PROX_CTRL5, ®val);
+ if (ret)
+ return ret;
+
+ regval = FIELD_GET(SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK, regval);
+ if (regval)
+ *val = 1 << regval;
+ else
+ *val = 0;
+
+ return IIO_VAL_INT;
+}
+
+static int sx9324_read_event_val(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)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+
+ if (chan->type != IIO_PROXIMITY)
+ return -EINVAL;
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ return sx9324_read_thresh(data, chan, val);
+ case IIO_EV_INFO_PERIOD:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return sx9324_read_far_debounce(data, val);
+ case IIO_EV_DIR_FALLING:
+ return sx9324_read_close_debounce(data, val);
+ default:
+ return -EINVAL;
+ }
+ case IIO_EV_INFO_HYSTERESIS:
+ return sx9324_read_hysteresis(data, chan, val);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int sx9324_write_thresh(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int _val)
+{
+ unsigned int reg, val = _val;
+ int ret;
+
+ reg = SX9324_REG_PROX_CTRL6 + chan->channel / 2;
+
+ if (val >= 1)
+ val = int_sqrt(2 * val);
+
+ if (val > 0xff)
+ return -EINVAL;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_write(data->regmap, reg, val);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int sx9324_write_hysteresis(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int _val)
+{
+ unsigned int hyst, val = _val;
+ int ret, pthresh;
+
+ ret = sx9324_read_thresh(data, chan, &pthresh);
+ if (ret < 0)
+ return ret;
+
+ if (val == 0)
+ hyst = 0;
+ else if (val >= pthresh >> 2)
+ hyst = 3;
+ else if (val >= pthresh >> 3)
+ hyst = 2;
+ else if (val >= pthresh >> 4)
+ hyst = 1;
+ else
+ return -EINVAL;
+
+ hyst = FIELD_PREP(SX9324_REG_PROX_CTRL5_HYST_MASK, hyst);
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
+ SX9324_REG_PROX_CTRL5_HYST_MASK, hyst);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int sx9324_write_far_debounce(struct sx_common_data *data, int _val)
+{
+ unsigned int regval, val = _val;
+ int ret;
+
+ if (val > 0)
+ val = ilog2(val);
+ if (!FIELD_FIT(SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK, val))
+ return -EINVAL;
+
+ regval = FIELD_PREP(SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK, val);
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
+ SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK,
+ regval);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int sx9324_write_close_debounce(struct sx_common_data *data, int _val)
+{
+ unsigned int regval, val = _val;
+ int ret;
+
+ if (val > 0)
+ val = ilog2(val);
+ if (!FIELD_FIT(SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK, val))
+ return -EINVAL;
+
+ regval = FIELD_PREP(SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK, val);
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
+ SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK,
+ regval);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int sx9324_write_event_val(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)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+
+ if (chan->type != IIO_PROXIMITY)
+ return -EINVAL;
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ return sx9324_write_thresh(data, chan, val);
+ case IIO_EV_INFO_PERIOD:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return sx9324_write_far_debounce(data, val);
+ case IIO_EV_DIR_FALLING:
+ return sx9324_write_close_debounce(data, val);
+ default:
+ return -EINVAL;
+ }
+ case IIO_EV_INFO_HYSTERESIS:
+ return sx9324_write_hysteresis(data, chan, val);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int sx9324_write_gain(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int val)
+{
+ unsigned int gain, reg;
+ int ret;
+
+ gain = ilog2(val);
+ reg = SX9324_REG_PROX_CTRL0 + chan->channel / 2;
+ gain = FIELD_PREP(SX9324_REG_PROX_CTRL0_GAIN_MASK, gain);
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, reg,
+ SX9324_REG_PROX_CTRL0_GAIN_MASK,
+ gain);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+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;
+}
+
+/* Activate all channels and perform an initial compensation. */
+static int sx9324_init_compensation(struct iio_dev *indio_dev)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+ unsigned int val;
+ int ret;
+
+ /* run the compensation phase on all channels */
+ ret = regmap_update_bits(data->regmap, SX9324_REG_STAT2,
+ SX9324_REG_STAT2_COMPSTAT_MASK,
+ SX9324_REG_STAT2_COMPSTAT_MASK);
+ if (ret)
+ return ret;
+
+ ret = regmap_read_poll_timeout(data->regmap, SX9324_REG_STAT2, val,
+ !(val & SX9324_REG_STAT2_COMPSTAT_MASK),
+ 20000, 2000000);
+ if (ret == -ETIMEDOUT)
+ dev_err(&data->client->dev,
+ "initial compensation timed out: 0x%02x\n",
+ val);
+ return ret;
+}
+
+static int sx9324_check_whoami(struct device *dev,
+ struct iio_dev *indio_dev)
+{
+ /*
+ * Only one sensor for this driver. Assuming the device tree
+ * is correct, just set the sensor name.
+ */
+ indio_dev->name = "sx9324";
+ return 0;
+}
+
+static const struct sx_common_chip_info sx9324_chip_info = {
+ .reg_stat = SX9324_REG_STAT0,
+ .reg_irq_msk = SX9324_REG_IRQ_MSK,
+ .reg_enable_chan = SX9324_REG_GNRL_CTRL1,
+ .reg_reset = SX9324_REG_RESET,
+
+ .mask_enable_chan = SX9324_REG_GNRL_CTRL1_PHEN_MASK,
+ .irq_msk_offset = 3,
+ .num_channels = SX9324_NUM_CHANNELS,
+
+ .ops = {
+ .read_prox_data = sx9324_read_prox_data,
+ .check_whoami = sx9324_check_whoami,
+ .init_compensation = sx9324_init_compensation,
+ .wait_for_sample = sx9324_wait_for_sample,
+ },
+
+ .iio_channels = sx9324_channels,
+ .num_iio_channels = ARRAY_SIZE(sx9324_channels),
+ .iio_info = {
+ .read_raw = sx9324_read_raw,
+ .read_avail = sx9324_read_avail,
+ .read_event_value = sx9324_read_event_val,
+ .write_event_value = sx9324_write_event_val,
+ .write_raw = sx9324_write_raw,
+ .read_event_config = sx_common_read_event_config,
+ .write_event_config = sx_common_write_event_config,
+ },
+};
+
+static int sx9324_probe(struct i2c_client *client)
+{
+ return sx_common_probe(client, &sx9324_chip_info, &sx9324_regmap_config);
+}
+
+static int __maybe_unused sx9324_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct sx_common_data *data = iio_priv(indio_dev);
+ unsigned int regval;
+ int ret;
+
+ disable_irq_nosync(data->client->irq);
+
+ mutex_lock(&data->mutex);
+ ret = regmap_read(data->regmap, SX9324_REG_GNRL_CTRL1, ®val);
+
+ data->suspend_ctrl =
+ FIELD_GET(SX9324_REG_GNRL_CTRL1_PHEN_MASK, regval);
+
+ if (ret < 0)
+ goto out;
+
+ /* Disable all phases, send the device to sleep. */
+ ret = regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1, 0);
+
+out:
+ mutex_unlock(&data->mutex);
+ return ret;
+}
+
+static int __maybe_unused sx9324_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct sx_common_data *data = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1,
+ data->suspend_ctrl | SX9324_REG_GNRL_CTRL1_PAUSECTRL);
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+
+ enable_irq(data->client->irq);
+ return 0;
+}
+
+static const struct dev_pm_ops sx9324_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(sx9324_suspend, sx9324_resume)
+};
+
+static const struct acpi_device_id sx9324_acpi_match[] = {
+ { "STH9324", SX9324_WHOAMI_VALUE},
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, sx9324_acpi_match);
+
+static const struct of_device_id sx9324_of_match[] = {
+ { .compatible = "semtech,sx9324", (void *)SX9324_WHOAMI_VALUE},
+ { }
+};
+MODULE_DEVICE_TABLE(of, sx9324_of_match);
+
+static const struct i2c_device_id sx9324_id[] = {
+ {"sx9324", SX9324_WHOAMI_VALUE},
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, sx9324_id);
+
+static struct i2c_driver sx9324_driver = {
+ .driver = {
+ .name = "sx9324",
+ .acpi_match_table = ACPI_PTR(sx9324_acpi_match),
+ .of_match_table = of_match_ptr(sx9324_of_match),
+ .pm = &sx9324_pm_ops,
+
+ /*
+ * Lots of i2c transfers in probe + over 200 ms waiting in
+ * sx9324_init_compensation() mean a slow probe; prefer async
+ * so we don't delay boot if we're builtin to the kernel.
+ */
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+ .probe_new = sx9324_probe,
+ .id_table = sx9324_id,
+};
+module_i2c_driver(sx9324_driver);
+
+MODULE_AUTHOR("Gwendal Grignou <gwendal@chromium.org>");
+MODULE_DESCRIPTION("Driver for Semtech SX9324 proximity sensor");
+MODULE_LICENSE("GPL v2");
--
2.34.0.rc2.393.gf8c9666880-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v4 3/5] iio: proximity: Add SX9324 support
2021-11-20 10:14 ` [PATCH v4 3/5] iio: proximity: Add SX9324 support Gwendal Grignou
@ 2021-11-20 15:27 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2021-11-20 15:27 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: lars, swboyd, andy.shevchenko, linux-iio
On Sat, 20 Nov 2021 02:14:59 -0800
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>
> ---
A few little things inline. Looks good otherwise,
Jonathan
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index e88fc373c2c903..aaddf97f9b2192 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -129,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 2577fbce4144e5..cffe962b352718 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_SX9324) += sx9324.o
> obj-$(CONFIG_SX_COMMON) += sx_common.o
> obj-$(CONFIG_SX9500) += sx9500.o
> obj-$(CONFIG_VCNL3020) += vcnl3020.o
> diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> new file mode 100644
> index 00000000000000..3858e70d7a86f6
> --- /dev/null
> +++ b/drivers/iio/proximity/sx9324.c
> @@ -0,0 +1,916 @@
> +// 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
> + *
Unneeded blank line..
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.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>
As before, take a look at these (or run IWYU if you can) to prune
out the unused headers.
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#include "sx_common.h"
> +
> +static int sx9324_read_prox_data(struct sx_common_data *data,
> + const struct iio_chan_spec *chan,
> + __be16 *val)
> +{
> + int ret;
> +
> + ret = regmap_write(data->regmap, SX9324_REG_PHASE_SEL, chan->channel);
> + if (ret < 0)
> + return ret;
A little inconsistent to check for < 0 for error here, but return directly the
value of the function below. Cleaner to have if (ret) here as well as regmap
never returns positive from its read and write functions.
> +
> + return regmap_bulk_read(data->regmap, chan->address, val, sizeof(*val));
> +}
> +
> +/*
> + * If we have no interrupt support, we have to wait for a scan period
> + * after enabling a channel to get a result.
> + */
> +static int sx9324_wait_for_sample(struct sx_common_data *data)
> +{
> + int ret;
> + unsigned int val;
> +
> + ret = regmap_read(data->regmap, SX9324_REG_GNRL_CTRL0, &val);
> + if (ret < 0)
> + return ret;
> + val = FIELD_GET(SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, val);
> +
> + msleep(sx9324_scan_period_table[val]);
> +
> + return 0;
> +}
> +
> +
> +static int sx9324_write_thresh(struct sx_common_data *data,
> + const struct iio_chan_spec *chan, int _val)
> +{
> + unsigned int reg, val = _val;
> + int ret;
> +
> + reg = SX9324_REG_PROX_CTRL6 + chan->channel / 2;
> +
> + if (val >= 1)
> + val = int_sqrt(2 * val);
This is unusual enough that perhaps a comment on what the threshold maths is
would be useful (I can't find the datasheet online to check this).
> +
> + if (val > 0xff)
> + return -EINVAL;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_write(data->regmap, reg, val);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +
> +static int sx9324_check_whoami(struct device *dev,
> + struct iio_dev *indio_dev)
> +{
> + /*
> + * Only one sensor for this driver. Assuming the device tree
> + * is correct, just set the sensor name.
If there is a whoami register, would be nice to check it. If not then
say that here as it's a better reason than the driver only supporting one
sensor..
> + */
> + indio_dev->name = "sx9324";
> + return 0;
> +}
> +
> +static const struct sx_common_chip_info sx9324_chip_info = {
> + .reg_stat = SX9324_REG_STAT0,
> + .reg_irq_msk = SX9324_REG_IRQ_MSK,
> + .reg_enable_chan = SX9324_REG_GNRL_CTRL1,
> + .reg_reset = SX9324_REG_RESET,
> +
> + .mask_enable_chan = SX9324_REG_GNRL_CTRL1_PHEN_MASK,
> + .irq_msk_offset = 3,
> + .num_channels = SX9324_NUM_CHANNELS,
> +
> + .ops = {
> + .read_prox_data = sx9324_read_prox_data,
> + .check_whoami = sx9324_check_whoami,
> + .init_compensation = sx9324_init_compensation,
> + .wait_for_sample = sx9324_wait_for_sample,
> + },
> +
> + .iio_channels = sx9324_channels,
> + .num_iio_channels = ARRAY_SIZE(sx9324_channels),
> + .iio_info = {
> + .read_raw = sx9324_read_raw,
> + .read_avail = sx9324_read_avail,
> + .read_event_value = sx9324_read_event_val,
> + .write_event_value = sx9324_write_event_val,
> + .write_raw = sx9324_write_raw,
> + .read_event_config = sx_common_read_event_config,
> + .write_event_config = sx_common_write_event_config,
> + },
> +};
> +
> +static int sx9324_probe(struct i2c_client *client)
> +{
> + return sx_common_probe(client, &sx9324_chip_info, &sx9324_regmap_config);
> +}
> +
> +static int __maybe_unused sx9324_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct sx_common_data *data = iio_priv(indio_dev);
> + unsigned int regval;
> + int ret;
> +
> + disable_irq_nosync(data->client->irq);
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, SX9324_REG_GNRL_CTRL1, ®val);
> +
> + data->suspend_ctrl =
> + FIELD_GET(SX9324_REG_GNRL_CTRL1_PHEN_MASK, regval);
Needs a comment on why it makes sense to use a value when the read failed particularly
as the value is potentially uninitialized. I'm guessing this should be after the
error check.
> +
> + if (ret < 0)
> + goto out;
> +
> + /* Disable all phases, send the device to sleep. */
> + ret = regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1, 0);
> +
> +out:
> + mutex_unlock(&data->mutex);
> + return ret;
> +}
> +
> +static int __maybe_unused sx9324_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct sx_common_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1,
> + data->suspend_ctrl | SX9324_REG_GNRL_CTRL1_PAUSECTRL);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> +
> + enable_irq(data->client->irq);
Slight preference for blank lines before returns like this to distinguish
them from being related to the previous block of code (unlikely error returns)
Not important though if you'd rather not for some reason.
> + return 0;
> +}
> +
> +static const struct dev_pm_ops sx9324_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(sx9324_suspend, sx9324_resume)
> +};
> +
> +static const struct acpi_device_id sx9324_acpi_match[] = {
> + { "STH9324", SX9324_WHOAMI_VALUE},
Good, it's a probably valid ACPI entry as the PNP id is actually semtech :)
As you are using ACPI_PTR() this should be __maybe_unused to avoid warnings.
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, sx9324_acpi_match);
> +
> +static const struct of_device_id sx9324_of_match[] = {
> + { .compatible = "semtech,sx9324", (void *)SX9324_WHOAMI_VALUE},
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, sx9324_of_match);
> +
> +static const struct i2c_device_id sx9324_id[] = {
> + {"sx9324", SX9324_WHOAMI_VALUE},
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, sx9324_id);
> +
> +static struct i2c_driver sx9324_driver = {
> + .driver = {
> + .name = "sx9324",
> + .acpi_match_table = ACPI_PTR(sx9324_acpi_match),
> + .of_match_table = of_match_ptr(sx9324_of_match),
Please don't introduce of_match_ptr() I'm still trying to get rid of the
existing cases of this. It breaks the ACPI / PRP0001 based probing for
no significant advantage.
Also, if you build this with CONFIG_OF disabled you'd get a warning about
the unused table..
> + .pm = &sx9324_pm_ops,
> +
> + /*
> + * Lots of i2c transfers in probe + over 200 ms waiting in
> + * sx9324_init_compensation() mean a slow probe; prefer async
> + * so we don't delay boot if we're builtin to the kernel.
> + */
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .probe_new = sx9324_probe,
> + .id_table = sx9324_id,
> +};
> +module_i2c_driver(sx9324_driver);
> +
> +MODULE_AUTHOR("Gwendal Grignou <gwendal@chromium.org>");
> +MODULE_DESCRIPTION("Driver for Semtech SX9324 proximity sensor");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4 4/5] dt-bindings: iio: Add sx9324 binding
2021-11-20 10:14 [PATCH v4 0/5] Expand Semtech SAR Sensors support Gwendal Grignou
` (2 preceding siblings ...)
2021-11-20 10:14 ` [PATCH v4 3/5] iio: proximity: Add SX9324 support Gwendal Grignou
@ 2021-11-20 10:15 ` Gwendal Grignou
2021-11-20 15:32 ` Jonathan Cameron
2021-11-20 10:15 ` [PATCH v4 5/5] iio: sx9324: Add dt_binding support Gwendal Grignou
4 siblings, 1 reply; 11+ messages in thread
From: Gwendal Grignou @ 2021-11-20 10:15 UTC (permalink / raw)
To: jic23, lars, swboyd; +Cc: andy.shevchenko, linux-iio, Gwendal Grignou
Similar to SX9310, add biddings to setup sx9324 hardware properties.
SX9324 is a little different, introduce 4 phases to be configured in 2
pairs over 3 antennas.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
Changes in v4:
- Use const instead of single enum
- Specify ph0-pin better
- Recopy type information for phX-pin
- Fix cut and paste errors.
Changes in v3:
- Remove duplicate information.
- Use intervals instead of enum.
- Fix filter description.
Changes in v2:
- Fix interrupt documentation wording.
.../iio/proximity/semtech,sx9324.yaml | 161 ++++++++++++++++++
1 file changed, 161 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
new file mode 100644
index 00000000000000..70e4736fb1cc9a
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
@@ -0,0 +1,161 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/proximity/semtech,sx9324.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Semtech's SX9324 capacitive proximity sensor
+
+maintainers:
+ - Gwendal Grignou <gwendal@chromium.org>
+ - Daniel Campello <campello@chromium.org>
+
+description: |
+ Semtech's SX9324 proximity sensor.
+
+properties:
+ compatible:
+ const: semtech,sx9324
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ description:
+ Generated by device to announce preceding read request has finished
+ and data is available or that a close/far proximity event has happened.
+ maxItems: 1
+
+ vdd-supply:
+ description: Main power supply
+
+ svdd-supply:
+ description: Host interface power supply
+
+ "#io-channel-cells":
+ const: 1
+
+ semtech,ph0-pin:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description: |
+ Array of 3 entries. Index represent the id of the CS pin.
+ Value indicates how each CS pin is used during phase 0.
+ Each of the 3 pins have the following value -
+ 0 : unused (high impedance)
+ 1 : measured input
+ 2 : dynamic shield
+ 3 : grounded.
+ For instance, CS0 measured, CS1 shield and CS2 ground is [1, 2, 3]
+ items:
+ enum: [ 0, 1, 2, 3 ]
+ minItems: 3
+ maxItems: 3
+
+ semtech,ph1-pin:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description: Same as ph0-pin for phase 1.
+ items:
+ enum: [ 0, 1, 2, 3 ]
+ minItems: 3
+ maxItems: 3
+
+ semtech,ph2-pin:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description: Same as ph0-pin for phase 2.
+ items:
+ enum: [ 0, 1, 2, 3 ]
+ minItems: 3
+ maxItems: 3
+
+ semtech,ph3-pin:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description: Same as ph0-pin for phase 3.
+ items:
+ enum: [ 0, 1, 2, 3 ]
+ minItems: 3
+ maxItems: 3
+
+
+ semtech,resolution01:
+ $ref: /schemas/types.yaml#definitions/uint32
+ enum: [8, 16, 32, 64, 128, 256, 512, 1024]
+ description:
+ Capacitance measurement resolution. For phase 0 and 1.
+ Higher the number, higher the resolution.
+ default: 128
+
+ semtech,resolution23:
+ $ref: /schemas/types.yaml#definitions/uint32
+ enum: [8, 16, 32, 64, 128, 256, 512, 1024]
+ description:
+ Capacitance measurement resolution. For phase 2 and 3
+ default: 128
+
+ semtech,startup-sensor:
+ $ref: /schemas/types.yaml#definitions/uint32
+ enum: [0, 1, 2, 3]
+ default: 0
+ description: |
+ Phase used for start-up proximity detection.
+ It is used when we enable a phase to remove static offset and measure
+ only capacitance changes introduced by the user.
+
+ semtech,proxraw-strength01:
+ $ref: /schemas/types.yaml#definitions/uint32
+ min: 0
+ max: 7
+ default: 1
+ description:
+ PROXRAW filter strength for phase 0 and 1. A value of 0 represents off,
+ and other values represent 1-1/2^N.
+
+ semtech,proxraw-strength23:
+ $ref: /schemas/types.yaml#definitions/uint32
+ min: 0
+ max: 7
+ default: 1
+ description:
+ Same as proxraw-strength01, for phase 2 and 3.
+
+ semtech,avg-pos-strength:
+ $ref: /schemas/types.yaml#definitions/uint32
+ enum: [0, 16, 64, 128, 256, 512, 1024, 4294967295]
+ default: 16
+ description: |
+ Average positive filter strength. A value of 0 represents off and
+ UINT_MAX (4294967295) represents infinite. Other values
+ represent 1-1/N.
+
+required:
+ - compatible
+ - reg
+ - "#io-channel-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ proximity@28 {
+ compatible = "semtech,sx9324";
+ reg = <0x28>;
+ interrupt-parent = <&pio>;
+ interrupts = <5 IRQ_TYPE_LEVEL_LOW 5>;
+ vdd-supply = <&pp3300_a>;
+ svdd-supply = <&pp1800_prox>;
+ #io-channel-cells = <1>;
+ semtech,ph0-pin = <1, 2, 3>;
+ semtech,ph1-pin = <3, 2, 1>;
+ semtech,ph2-pin = <1, 2, 3>;
+ semtech,ph3-pin = <3, 2, 1>;
+ semtech,resolution01 = 2;
+ semtech,resolution23 = 2;
+ semtech,startup-sensor = <1>;
+ semtech,proxraw-strength01 = <2>;
+ semtech,proxraw-strength23 = <2>;
+ semtech,avg-pos-strength = <64>;
+ };
+ };
--
2.34.0.rc2.393.gf8c9666880-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v4 4/5] dt-bindings: iio: Add sx9324 binding
2021-11-20 10:15 ` [PATCH v4 4/5] dt-bindings: iio: Add sx9324 binding Gwendal Grignou
@ 2021-11-20 15:32 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2021-11-20 15:32 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: lars, swboyd, andy.shevchenko, linux-iio
On Sat, 20 Nov 2021 02:15:00 -0800
Gwendal Grignou <gwendal@chromium.org> wrote:
> Similar to SX9310, add biddings to setup sx9324 hardware properties.
> SX9324 is a little different, introduce 4 phases to be configured in 2
> pairs over 3 antennas.
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
No devicetree list or RobH in the to list so this won't get the review
from them that it needs. Fix that for v5.
A few suggestions inline about consistency of property naming.
I don't have particularly strong views on that though so could be persuaded
what you have is better perhaps...
> ---
> Changes in v4:
> - Use const instead of single enum
> - Specify ph0-pin better
> - Recopy type information for phX-pin
> - Fix cut and paste errors.
>
> Changes in v3:
> - Remove duplicate information.
> - Use intervals instead of enum.
> - Fix filter description.
>
> Changes in v2:
> - Fix interrupt documentation wording.
>
> .../iio/proximity/semtech,sx9324.yaml | 161 ++++++++++++++++++
> 1 file changed, 161 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> new file mode 100644
> index 00000000000000..70e4736fb1cc9a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> @@ -0,0 +1,161 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/proximity/semtech,sx9324.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Semtech's SX9324 capacitive proximity sensor
> +
> +maintainers:
> + - Gwendal Grignou <gwendal@chromium.org>
> + - Daniel Campello <campello@chromium.org>
> +
> +description: |
> + Semtech's SX9324 proximity sensor.
> +
> +properties:
> + compatible:
> + const: semtech,sx9324
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + description:
> + Generated by device to announce preceding read request has finished
> + and data is available or that a close/far proximity event has happened.
> + maxItems: 1
> +
> + vdd-supply:
> + description: Main power supply
> +
> + svdd-supply:
> + description: Host interface power supply
> +
> + "#io-channel-cells":
> + const: 1
> +
> + semtech,ph0-pin:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description: |
> + Array of 3 entries. Index represent the id of the CS pin.
> + Value indicates how each CS pin is used during phase 0.
> + Each of the 3 pins have the following value -
> + 0 : unused (high impedance)
> + 1 : measured input
> + 2 : dynamic shield
> + 3 : grounded.
> + For instance, CS0 measured, CS1 shield and CS2 ground is [1, 2, 3]
> + items:
> + enum: [ 0, 1, 2, 3 ]
> + minItems: 3
> + maxItems: 3
> +
> + semtech,ph1-pin:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description: Same as ph0-pin for phase 1.
> + items:
> + enum: [ 0, 1, 2, 3 ]
> + minItems: 3
> + maxItems: 3
> +
> + semtech,ph2-pin:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description: Same as ph0-pin for phase 2.
> + items:
> + enum: [ 0, 1, 2, 3 ]
> + minItems: 3
> + maxItems: 3
> +
> + semtech,ph3-pin:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description: Same as ph0-pin for phase 3.
> + items:
> + enum: [ 0, 1, 2, 3 ]
> + minItems: 3
> + maxItems: 3
> +
One blank line only
> +
> + semtech,resolution01:
Given phX naming above, maybe this should be
semtech,ph01-resolution
etc for consistency? Rob may have a better idea for what would be consistent
with other dt bindings here.
> + $ref: /schemas/types.yaml#definitions/uint32
> + enum: [8, 16, 32, 64, 128, 256, 512, 1024]
> + description:
> + Capacitance measurement resolution. For phase 0 and 1.
> + Higher the number, higher the resolution.
> + default: 128
> +
> + semtech,resolution23:
> + $ref: /schemas/types.yaml#definitions/uint32
> + enum: [8, 16, 32, 64, 128, 256, 512, 1024]
> + description:
> + Capacitance measurement resolution. For phase 2 and 3
> + default: 128
> +
> + semtech,startup-sensor:
Perhaps include ph in the name to make it clear this is setting the initial
phase as per the descriptions above.
> + $ref: /schemas/types.yaml#definitions/uint32
> + enum: [0, 1, 2, 3]
> + default: 0
> + description: |
> + Phase used for start-up proximity detection.
> + It is used when we enable a phase to remove static offset and measure
> + only capacitance changes introduced by the user.
> +
> + semtech,proxraw-strength01:
> + $ref: /schemas/types.yaml#definitions/uint32
> + min: 0
> + max: 7
> + default: 1
> + description:
> + PROXRAW filter strength for phase 0 and 1. A value of 0 represents off,
> + and other values represent 1-1/2^N.
> +
> + semtech,proxraw-strength23:
> + $ref: /schemas/types.yaml#definitions/uint32
> + min: 0
> + max: 7
> + default: 1
> + description:
> + Same as proxraw-strength01, for phase 2 and 3.
> +
> + semtech,avg-pos-strength:
> + $ref: /schemas/types.yaml#definitions/uint32
> + enum: [0, 16, 64, 128, 256, 512, 1024, 4294967295]
> + default: 16
> + description: |
> + Average positive filter strength. A value of 0 represents off and
> + UINT_MAX (4294967295) represents infinite. Other values
> + represent 1-1/N.
> +
> +required:
> + - compatible
> + - reg
> + - "#io-channel-cells"
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + proximity@28 {
> + compatible = "semtech,sx9324";
> + reg = <0x28>;
> + interrupt-parent = <&pio>;
> + interrupts = <5 IRQ_TYPE_LEVEL_LOW 5>;
> + vdd-supply = <&pp3300_a>;
> + svdd-supply = <&pp1800_prox>;
> + #io-channel-cells = <1>;
> + semtech,ph0-pin = <1, 2, 3>;
> + semtech,ph1-pin = <3, 2, 1>;
> + semtech,ph2-pin = <1, 2, 3>;
> + semtech,ph3-pin = <3, 2, 1>;
> + semtech,resolution01 = 2;
> + semtech,resolution23 = 2;
> + semtech,startup-sensor = <1>;
> + semtech,proxraw-strength01 = <2>;
> + semtech,proxraw-strength23 = <2>;
> + semtech,avg-pos-strength = <64>;
> + };
> + };
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4 5/5] iio: sx9324: Add dt_binding support
2021-11-20 10:14 [PATCH v4 0/5] Expand Semtech SAR Sensors support Gwendal Grignou
` (3 preceding siblings ...)
2021-11-20 10:15 ` [PATCH v4 4/5] dt-bindings: iio: Add sx9324 binding Gwendal Grignou
@ 2021-11-20 10:15 ` Gwendal Grignou
4 siblings, 0 replies; 11+ messages in thread
From: Gwendal Grignou @ 2021-11-20 10:15 UTC (permalink / raw)
To: jic23, lars, swboyd; +Cc: andy.shevchenko, linux-iio, Gwendal Grignou
Based on bindings/iio/proximity/semtech,sx9324.yaml, implement
retrieving sensor hardware property and alter default values.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
Changes in v4:
- Fix spelling in commit title.
Changes in v3:
- Remove duplicate information.
- Use intervals instead of enum.
- Fix filter description.
Changes in v2:
- Fix interrupt documentation wording.
drivers/iio/proximity/sx9324.c | 158 +++++++++++++++++++++++++++++++++
1 file changed, 158 insertions(+)
diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
index 3858e70d7a86f6..26ab91676aba9e 100644
--- a/drivers/iio/proximity/sx9324.c
+++ b/drivers/iio/proximity/sx9324.c
@@ -83,6 +83,7 @@
#define SX9324_REG_PROX_CTRL0 0x30
#define SX9324_REG_PROX_CTRL0_GAIN_MASK GENMASK(5, 3)
#define SX9324_REG_PROX_CTRL0_GAIN_1 0x80
+#define SX9324_REG_PROX_CTRL0_RAWFILT_MASK GENMASK(2, 0)
#define SX9324_REG_PROX_CTRL0_RAWFILT_1P50 0x01
#define SX9324_REG_PROX_CTRL1 0x31
#define SX9324_REG_PROX_CTRL2 0x32
@@ -757,6 +758,74 @@ static int sx9324_write_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
+static const struct sx_common_reg_default sx9324_default_regs[] = {
+ { SX9324_REG_IRQ_MSK, 0x00 },
+ { SX9324_REG_IRQ_CFG0, 0x00 },
+ { SX9324_REG_IRQ_CFG1, SX9324_REG_IRQ_CFG1_FAILCOND },
+ { SX9324_REG_IRQ_CFG2, 0x00 },
+ { SX9324_REG_GNRL_CTRL0, SX9324_REG_GNRL_CTRL0_SCANPERIOD_100MS },
+ /*
+ * The lower 4 bits should not be set as it enable sensors measurements.
+ * Turning the detection on before the configuration values are set to
+ * good values can cause the device to return erroneous readings.
+ */
+ { SX9324_REG_GNRL_CTRL1, SX9324_REG_GNRL_CTRL1_PAUSECTRL },
+
+ { SX9324_REG_AFE_CTRL0, 0x00 },
+ { SX9324_REG_AFE_CTRL3, 0x00 },
+ { SX9324_REG_AFE_CTRL4, SX9324_REG_AFE_CTRL4_FREQ_83_33HZ |
+ SX9324_REG_AFE_CTRL4_RES_100 },
+ { SX9324_REG_AFE_CTRL6, 0x00 },
+ { SX9324_REG_AFE_CTRL7, SX9324_REG_AFE_CTRL4_FREQ_83_33HZ |
+ SX9324_REG_AFE_CTRL4_RES_100 },
+
+ /* TODO(gwendal): PHx use chip default or all grounded? */
+ { SX9324_REG_AFE_PH0, 0x29 },
+ { SX9324_REG_AFE_PH1, 0x26 },
+ { SX9324_REG_AFE_PH2, 0x1a },
+ { SX9324_REG_AFE_PH3, 0x16 },
+
+ { SX9324_REG_AFE_CTRL8, SX9324_REG_AFE_CTRL8_RESFILTN_4KOHM },
+ { SX9324_REG_AFE_CTRL9, SX9324_REG_AFE_CTRL9_AGAIN_1 },
+
+ { SX9324_REG_PROX_CTRL0, SX9324_REG_PROX_CTRL0_GAIN_1 |
+ SX9324_REG_PROX_CTRL0_RAWFILT_1P50 },
+ { SX9324_REG_PROX_CTRL1, SX9324_REG_PROX_CTRL0_GAIN_1 |
+ SX9324_REG_PROX_CTRL0_RAWFILT_1P50 },
+ { SX9324_REG_PROX_CTRL2, SX9324_REG_PROX_CTRL2_AVGNEG_THRESH_16K },
+ { SX9324_REG_PROX_CTRL3, SX9324_REG_PROX_CTRL3_AVGDEB_2SAMPLES |
+ SX9324_REG_PROX_CTRL3_AVGPOS_THRESH_16K },
+ { SX9324_REG_PROX_CTRL4, SX9324_REG_PROX_CTRL4_AVGNEG_FILT_2 |
+ SX9324_REG_PROX_CTRL3_AVGPOS_FILT_256 },
+ { SX9324_REG_PROX_CTRL5, 0x00 },
+ { SX9324_REG_PROX_CTRL6, SX9324_REG_PROX_CTRL6_PROXTHRESH_32 },
+ { SX9324_REG_PROX_CTRL7, SX9324_REG_PROX_CTRL6_PROXTHRESH_32 },
+ { SX9324_REG_ADV_CTRL0, 0x00 },
+ { SX9324_REG_ADV_CTRL1, 0x00 },
+ { SX9324_REG_ADV_CTRL2, 0x00 },
+ { SX9324_REG_ADV_CTRL3, 0x00 },
+ { SX9324_REG_ADV_CTRL4, 0x00 },
+ { SX9324_REG_ADV_CTRL5, SX9324_REG_ADV_CTRL5_STARTUP_SENSOR_1 |
+ SX9324_REG_ADV_CTRL5_STARTUP_METHOD_1 },
+ { SX9324_REG_ADV_CTRL6, 0x00 },
+ { SX9324_REG_ADV_CTRL7, 0x00 },
+ { SX9324_REG_ADV_CTRL8, 0x00 },
+ { SX9324_REG_ADV_CTRL9, 0x00 },
+ /* Body/Table threshold */
+ { SX9324_REG_ADV_CTRL10, 0x00 },
+ { SX9324_REG_ADV_CTRL11, 0x00 },
+ { SX9324_REG_ADV_CTRL12, 0x00 },
+ /* TODO(gwendal): SAR currenly disabled */
+ { SX9324_REG_ADV_CTRL13, 0x00 },
+ { SX9324_REG_ADV_CTRL14, 0x00 },
+ { SX9324_REG_ADV_CTRL15, 0x00 },
+ { SX9324_REG_ADV_CTRL16, 0x00 },
+ { SX9324_REG_ADV_CTRL17, 0x00 },
+ { SX9324_REG_ADV_CTRL18, 0x00 },
+ { SX9324_REG_ADV_CTRL19, SX9324_REG_ADV_CTRL19_HIGHT_FAILURE_THRESH_SATURATION },
+ { SX9324_REG_ADV_CTRL20, SX9324_REG_ADV_CTRL19_HIGHT_FAILURE_THRESH_SATURATION },
+};
+
/* Activate all channels and perform an initial compensation. */
static int sx9324_init_compensation(struct iio_dev *indio_dev)
{
@@ -781,6 +850,93 @@ static int sx9324_init_compensation(struct iio_dev *indio_dev)
return ret;
}
+static const struct sx_common_reg_default *
+sx9324_get_default_reg(struct device *dev, int idx,
+ struct sx_common_reg_default *reg_def)
+{
+#define SX9324_PIN_DEF "semtech,ph0-pin"
+#define SX9324_RESOLUTION_DEF "semtech,resolution01"
+#define SX9324_PROXRAW_DEF "semtech,proxraw-strength01"
+ unsigned int pin_defs[SX9324_NUM_PINS];
+ char prop[] = SX9324_PROXRAW_DEF;
+ u32 start = 0, raw = 0, pos = 0;
+ int ret, count, ph, pin;
+
+ memcpy(reg_def, &sx9324_default_regs[idx], sizeof(*reg_def));
+ switch (reg_def->reg) {
+ case SX9324_REG_AFE_PH0:
+ case SX9324_REG_AFE_PH1:
+ case SX9324_REG_AFE_PH2:
+ case SX9324_REG_AFE_PH3:
+ ph = reg_def->reg - SX9324_REG_AFE_PH0;
+ scnprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
+
+ count = device_property_count_u32(dev, prop);
+ if (count != ARRAY_SIZE(pin_defs))
+ break;
+ ret = device_property_read_u32_array(dev, prop, pin_defs,
+ ARRAY_SIZE(pin_defs));
+ for (pin = 0; pin < SX9324_NUM_PINS; pin++)
+ raw |= (pin_defs[pin] << (2 * pin)) &
+ SX9324_REG_AFE_PH0_PIN_MASK(pin);
+ reg_def->def = raw;
+ break;
+ case SX9324_REG_AFE_CTRL4:
+ case SX9324_REG_AFE_CTRL7:
+ if (reg_def->reg == SX9324_REG_AFE_CTRL4)
+ strncpy(prop, "semtech,resolution01", ARRAY_SIZE(prop));
+ else
+ strncpy(prop, "semtech,resolution23", ARRAY_SIZE(prop));
+
+ ret = device_property_read_u32(dev, prop, &raw);
+ if (ret)
+ break;
+
+ raw = ilog2(raw) - 3;
+
+ reg_def->def &= ~SX9324_REG_AFE_CTRL4_RESOLUTION_MASK;
+ reg_def->def |= FIELD_PREP(SX9324_REG_AFE_CTRL4_RESOLUTION_MASK,
+ raw);
+ break;
+ case SX9324_REG_ADV_CTRL5:
+ ret = device_property_read_u32(dev, "semtech,startup-sensor", &start);
+ if (ret)
+ break;
+
+ reg_def->def &= ~SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK;
+ reg_def->def |= FIELD_PREP(SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK,
+ start);
+ break;
+ case SX9324_REG_PROX_CTRL4:
+ ret = device_property_read_u32(dev, "semtech,avg-pos-strength", &pos);
+ if (ret)
+ break;
+
+ /* Powers of 2, except for a gap between 16 and 64 */
+ raw = clamp(ilog2(pos), 3, 11) - (pos >= 32 ? 4 : 3);
+
+ reg_def->def &= ~SX9324_REG_PROX_CTRL4_AVGPOSFILT_MASK;
+ reg_def->def |= FIELD_PREP(SX9324_REG_PROX_CTRL4_AVGPOSFILT_MASK,
+ raw);
+ break;
+ case SX9324_REG_PROX_CTRL0:
+ case SX9324_REG_PROX_CTRL1:
+ if (reg_def->reg == SX9324_REG_PROX_CTRL0)
+ strncpy(prop, "semtech,proxraw-strength01", ARRAY_SIZE(prop));
+ else
+ strncpy(prop, "semtech,proxraw-strength23", ARRAY_SIZE(prop));
+ ret = device_property_read_u32(dev, prop, &raw);
+ if (ret)
+ break;
+
+ reg_def->def &= ~SX9324_REG_PROX_CTRL0_RAWFILT_MASK;
+ reg_def->def |= FIELD_PREP(SX9324_REG_PROX_CTRL0_RAWFILT_MASK,
+ raw);
+ break;
+ }
+ return reg_def;
+}
+
static int sx9324_check_whoami(struct device *dev,
struct iio_dev *indio_dev)
{
@@ -801,12 +957,14 @@ static const struct sx_common_chip_info sx9324_chip_info = {
.mask_enable_chan = SX9324_REG_GNRL_CTRL1_PHEN_MASK,
.irq_msk_offset = 3,
.num_channels = SX9324_NUM_CHANNELS,
+ .num_default_regs = ARRAY_SIZE(sx9324_default_regs),
.ops = {
.read_prox_data = sx9324_read_prox_data,
.check_whoami = sx9324_check_whoami,
.init_compensation = sx9324_init_compensation,
.wait_for_sample = sx9324_wait_for_sample,
+ .get_default_reg = sx9324_get_default_reg,
},
.iio_channels = sx9324_channels,
--
2.34.0.rc2.393.gf8c9666880-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread