On 10/30/21 1:18 PM, Gwendal Grignou wrote:
[...]
Driver looks really great! Just a few small comments.
diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index 7c7203ca3ac63..aaddf97f9b219 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -112,11 +112,15 @@ config SRF04
To compile this driver as a module, choose M here: the
module will be called srf04.
+config SX_COMMON
+ tristate
+
config SX9310
tristate "SX9310/SX9311 Semtech proximity sensor"
select IIO_BUFFER
select IIO_TRIGGERED_BUFFER
select REGMAP_I2C
+ select SX_COMMON
This part belongs int the previous patch. Same with the Makefile update
for the common code.
depends on I2C
help
Say Y here to build a driver for Semtech's SX9310/SX9311 capacitive
[...]
diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
new file mode 100644
index 0000000000000..41d9c950c5abd
--- /dev/null
+++ b/drivers/iio/proximity/sx9324.c
@@ -0,0 +1,931 @@
[...]
+
+#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>
No GPIOs in this driver.
+#include <linux/kernel.h>
+#include <linux/log2.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
[..]
+
+static const char * const sx9324_cs_pin_usage[] = { "HZ", "MI", "DS", "GD" };
+
+static ssize_t sx9324_phase_configuration_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
+ unsigned int val;
+ int i, ret, pin_idx;
+ size_t len = 0;
+
+ ret = regmap_read(data->regmap, SX9324_REG_AFE_PH0 + this_attr->address, &val);
+
+ for (i = 0; i < SX9324_NUM_PINS; i++) {
+ pin_idx = (val & SX9324_REG_AFE_PH0_PIN_MASK(i)) >> (2 * i);
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%s,",
+ sx9324_cs_pin_usage[pin_idx]);
+ }
+ buf[len - 1] = '\n';
+ return len;
+}
We have IIO_ENUM, which seems like a good candidate for this.
+
+static ssize_t sx9324_phase_configuration_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t len)
+{
+ return -EINVAL;
+}
+
+#define IIO_DEV_ATTR_PHASE_CONFIG(_idx) \
+IIO_DEVICE_ATTR(in_proximity_configuration##_idx, 0644, \
+ sx9324_phase_configuration_show, \
+ sx9324_phase_configuration_store, _idx)
+
+static IIO_DEV_ATTR_PHASE_CONFIG(0);
+static IIO_DEV_ATTR_PHASE_CONFIG(1);
+static IIO_DEV_ATTR_PHASE_CONFIG(2);
+static IIO_DEV_ATTR_PHASE_CONFIG(3);
+
+/*
+ * 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 ssize_t sx9324_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(sx9324_samp_freq_table); i++)
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%d ",
+ sx9324_samp_freq_table[i].val,
+ sx9324_samp_freq_table[i].val2);
+ buf[len - 1] = '\n';
+ return len;
+}
+static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(sx9324_show_samp_freq_avail);
Consider implementing the `read_avail`. This means you just have to
return your table and the IIO core will handle the formatting.
+
[...]
+
+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);
Does this lock here actually protect against anything?
regmap_update_bits() has its own internal lock. Same with other locks
around regmap calls.
+
+ ret = regmap_update_bits(data->regmap,
+ SX9324_REG_GNRL_CTRL0,
+ SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, i);
+
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
