* [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer
@ 2023-03-16 23:48 Mehdi Djait
2023-03-16 23:48 ` [PATCH 1/3] dt-bindings: iio: Add " Mehdi Djait
` (4 more replies)
0 siblings, 5 replies; 30+ messages in thread
From: Mehdi Djait @ 2023-03-16 23:48 UTC (permalink / raw)
To: jic23, mazziesaccount
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, linux-iio,
linux-kernel, devicetree, Mehdi Djait
KX132 accelerometer is a sensor which:
- supports G-ranges of (+/-) 2, 4, 8, and 16G
- can be connected to I2C or SPI
- has internal HW FIFO buffer
- supports various ODRs (output data rates)
The KX132 accelerometer is very similair to the KX022A.
One key difference is number of bits to report the number of data bytes that
have been stored in the sample buffer: 8 bits for KX022A vs 10 bits for KX132.
A complete list of differences is listed in [1]
[1] https://kionixfs.azureedge.net/en/document/AN112-Transitioning-to-KX132-1211-Accelerometer.pdf1
Mehdi Djait (3):
dt-bindings: iio: Add KX132 accelerometer
iio: accel: kionix-kx022a: Add chip_info structure
iio: accel: Add support for Kionix/ROHM KX132 accelerometer
.../bindings/iio/accel/kionix,kx022a.yaml | 13 +-
drivers/iio/accel/kionix-kx022a-i2c.c | 21 +-
drivers/iio/accel/kionix-kx022a-spi.c | 24 +-
drivers/iio/accel/kionix-kx022a.c | 413 +++++++++++-------
drivers/iio/accel/kionix-kx022a.h | 181 +++++++-
5 files changed, 464 insertions(+), 188 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/3] dt-bindings: iio: Add KX132 accelerometer
2023-03-16 23:48 [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
@ 2023-03-16 23:48 ` Mehdi Djait
2023-03-19 15:54 ` Jonathan Cameron
2023-03-16 23:48 ` [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure Mehdi Djait
` (3 subsequent siblings)
4 siblings, 1 reply; 30+ messages in thread
From: Mehdi Djait @ 2023-03-16 23:48 UTC (permalink / raw)
To: jic23, mazziesaccount
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, linux-iio,
linux-kernel, devicetree, Mehdi Djait
Extend the kionix,kx022a.yaml file to support the
kx132 device
Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
.../bindings/iio/accel/kionix,kx022a.yaml | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml b/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
index 986df1a6ff0a..ac1e27402d5e 100644
--- a/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
@@ -4,19 +4,22 @@
$id: http://devicetree.org/schemas/iio/accel/kionix,kx022a.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: ROHM/Kionix KX022A Accelerometer
+title: ROHM/Kionix KX022A and KX132 Accelerometers
maintainers:
- Matti Vaittinen <mazziesaccount@gmail.com>
description: |
- KX022A is a 3-axis accelerometer supporting +/- 2G, 4G, 8G and 16G ranges,
- output data-rates from 0.78Hz to 1600Hz and a hardware-fifo buffering.
- KX022A can be accessed either via I2C or SPI.
+ KX022A and KX132 are 3-axis accelerometers supporting +/- 2G, 4G, 8G and
+ 16G ranges, output data-rates from 0.78Hz to 1600Hz and a hardware-fifo
+ buffering.
+ KX022A and KX132 can be accessed either via I2C or SPI.
properties:
compatible:
- const: kionix,kx022a
+ enum:
+ - kionix,kx022a
+ - kionix,kx132
reg:
maxItems: 1
--
2.30.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-16 23:48 [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
2023-03-16 23:48 ` [PATCH 1/3] dt-bindings: iio: Add " Mehdi Djait
@ 2023-03-16 23:48 ` Mehdi Djait
2023-03-17 1:01 ` kernel test robot
` (5 more replies)
2023-03-16 23:48 ` [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
` (2 subsequent siblings)
4 siblings, 6 replies; 30+ messages in thread
From: Mehdi Djait @ 2023-03-16 23:48 UTC (permalink / raw)
To: jic23, mazziesaccount
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, linux-iio,
linux-kernel, devicetree, Mehdi Djait
Refactor the kx022a driver implementation to make it more
generic and extensible.
Add the chip_info structure will to the driver's private
data to hold all the device specific infos.
Move the enum, struct and constants definitions to the header
file.
Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
drivers/iio/accel/kionix-kx022a-i2c.c | 19 +-
drivers/iio/accel/kionix-kx022a-spi.c | 22 +-
drivers/iio/accel/kionix-kx022a.c | 289 ++++++++++++--------------
drivers/iio/accel/kionix-kx022a.h | 128 ++++++++++--
4 files changed, 274 insertions(+), 184 deletions(-)
diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
index e6fd02d931b6..21c4c0ae1a68 100644
--- a/drivers/iio/accel/kionix-kx022a-i2c.c
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -15,23 +15,35 @@
static int kx022a_i2c_probe(struct i2c_client *i2c)
{
struct device *dev = &i2c->dev;
+ struct kx022a_chip_info *chip_info;
struct regmap *regmap;
+ const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
if (!i2c->irq) {
dev_err(dev, "No IRQ configured\n");
return -EINVAL;
}
- regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
+ chip_info = device_get_match_data(&i2c->dev);
+ if (!chip_info)
+ chip_info = (const struct kx022a_chip_info *) id->driver_data;
+
+ regmap = devm_regmap_init_i2c(i2c, chip_info->regmap_config);
if (IS_ERR(regmap))
return dev_err_probe(dev, PTR_ERR(regmap),
"Failed to initialize Regmap\n");
- return kx022a_probe_internal(dev);
+ return kx022a_probe_internal(dev, chip_info);
}
+static const struct i2c_device_id kx022a_i2c_id[] = {
+ { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
+
static const struct of_device_id kx022a_of_match[] = {
- { .compatible = "kionix,kx022a", },
+ { .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] },
{ }
};
MODULE_DEVICE_TABLE(of, kx022a_of_match);
@@ -42,6 +54,7 @@ static struct i2c_driver kx022a_i2c_driver = {
.of_match_table = kx022a_of_match,
},
.probe_new = kx022a_i2c_probe,
+ .id_table = kx022a_i2c_id,
};
module_i2c_driver(kx022a_i2c_driver);
diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c
index 9cd047f7b346..ec076af0f261 100644
--- a/drivers/iio/accel/kionix-kx022a-spi.c
+++ b/drivers/iio/accel/kionix-kx022a-spi.c
@@ -15,40 +15,46 @@
static int kx022a_spi_probe(struct spi_device *spi)
{
struct device *dev = &spi->dev;
+ struct kx022a_chip_info *chip_info;
struct regmap *regmap;
+ const struct spi_device_id *id = spi_get_device_id(spi);
if (!spi->irq) {
dev_err(dev, "No IRQ configured\n");
return -EINVAL;
}
- regmap = devm_regmap_init_spi(spi, &kx022a_regmap);
+ chip_info = device_get_match_data(&spi->dev);
+ if (!chip_info)
+ chip_info = (const struct kx022a_chip_info *) id->driver_data;
+
+ regmap = devm_regmap_init_spi(spi, chip_info->regmap_config);
if (IS_ERR(regmap))
return dev_err_probe(dev, PTR_ERR(regmap),
"Failed to initialize Regmap\n");
- return kx022a_probe_internal(dev);
+ return kx022a_probe_internal(dev, chip_info);
}
-static const struct spi_device_id kx022a_id[] = {
- { "kx022a" },
+static const struct spi_device_id kx022a_spi_id[] = {
+ { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
{ }
};
-MODULE_DEVICE_TABLE(spi, kx022a_id);
+MODULE_DEVICE_TABLE(spi, kx022a_spi_id);
static const struct of_device_id kx022a_of_match[] = {
- { .compatible = "kionix,kx022a", },
+ { .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] },
{ }
};
MODULE_DEVICE_TABLE(of, kx022a_of_match);
static struct spi_driver kx022a_spi_driver = {
.driver = {
- .name = "kx022a-spi",
+ .name = "kx022a-spi",
.of_match_table = kx022a_of_match,
},
.probe = kx022a_spi_probe,
- .id_table = kx022a_id,
+ .id_table = kx022a_spi_id,
};
module_spi_driver(kx022a_spi_driver);
diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index 8dac0337c249..27e8642aa8f5 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -26,29 +26,7 @@
#include "kionix-kx022a.h"
-/*
- * The KX022A has FIFO which can store 43 samples of HiRes data from 2
- * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
- * 258 bytes of sample data. The quirk to know is that the amount of bytes in
- * the FIFO is advertised via 8 bit register (max value 255). The thing to note
- * is that full 258 bytes of data is indicated using the max value 255.
- */
-#define KX022A_FIFO_LENGTH 43
-#define KX022A_FIFO_FULL_VALUE 255
-#define KX022A_SOFT_RESET_WAIT_TIME_US (5 * USEC_PER_MSEC)
-#define KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US (500 * USEC_PER_MSEC)
-
-/* 3 axis, 2 bytes of data for each of the axis */
-#define KX022A_FIFO_SAMPLES_SIZE_BYTES 6
-#define KX022A_FIFO_MAX_BYTES \
- (KX022A_FIFO_LENGTH * KX022A_FIFO_SAMPLES_SIZE_BYTES)
-
-enum {
- KX022A_STATE_SAMPLE,
- KX022A_STATE_FIFO,
-};
-
-/* Regmap configs */
+/* kx022a Regmap configs */
static const struct regmap_range kx022a_volatile_ranges[] = {
{
.range_min = KX022A_REG_XHP_L,
@@ -138,7 +116,7 @@ static const struct regmap_access_table kx022a_nir_regs = {
.n_yes_ranges = ARRAY_SIZE(kx022a_noinc_read_ranges),
};
-const struct regmap_config kx022a_regmap = {
+static const struct regmap_config kx022a_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
.volatile_table = &kx022a_volatile_regs,
@@ -149,39 +127,6 @@ const struct regmap_config kx022a_regmap = {
.max_register = KX022A_MAX_REGISTER,
.cache_type = REGCACHE_RBTREE,
};
-EXPORT_SYMBOL_NS_GPL(kx022a_regmap, IIO_KX022A);
-
-struct kx022a_data {
- struct regmap *regmap;
- struct iio_trigger *trig;
- struct device *dev;
- struct iio_mount_matrix orientation;
- int64_t timestamp, old_timestamp;
-
- int irq;
- int inc_reg;
- int ien_reg;
-
- unsigned int state;
- unsigned int odr_ns;
-
- bool trigger_enabled;
- /*
- * Prevent toggling the sensor stby/active state (PC1 bit) in the
- * middle of a configuration, or when the fifo is enabled. Also,
- * protect the data stored/retrieved from this structure from
- * concurrent accesses.
- */
- struct mutex mutex;
- u8 watermark;
-
- /* 3 x 16bit accel data + timestamp */
- __le16 buffer[8] __aligned(IIO_DMA_MINALIGN);
- struct {
- __le16 channels[3];
- s64 ts __aligned(8);
- } scan;
-};
static const struct iio_mount_matrix *
kx022a_get_mount_matrix(const struct iio_dev *idev,
@@ -192,13 +137,6 @@ kx022a_get_mount_matrix(const struct iio_dev *idev,
return &data->orientation;
}
-enum {
- AXIS_X,
- AXIS_Y,
- AXIS_Z,
- AXIS_MAX
-};
-
static const unsigned long kx022a_scan_masks[] = {
BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z), 0
};
@@ -208,7 +146,7 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
{ }
};
-#define KX022A_ACCEL_CHAN(axis, index) \
+#define KX022A_ACCEL_CHAN(axis, index, device) \
{ \
.type = IIO_ACCEL, \
.modified = 1, \
@@ -220,7 +158,7 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
BIT(IIO_CHAN_INFO_SCALE) | \
BIT(IIO_CHAN_INFO_SAMP_FREQ), \
.ext_info = kx022a_ext_info, \
- .address = KX022A_REG_##axis##OUT_L, \
+ .address = device##_REG_##axis##OUT_L, \
.scan_index = index, \
.scan_type = { \
.sign = 's', \
@@ -231,9 +169,9 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
}
static const struct iio_chan_spec kx022a_channels[] = {
- KX022A_ACCEL_CHAN(X, 0),
- KX022A_ACCEL_CHAN(Y, 1),
- KX022A_ACCEL_CHAN(Z, 2),
+ KX022A_ACCEL_CHAN(X, 0, KX022A),
+ KX022A_ACCEL_CHAN(Y, 1, KX022A),
+ KX022A_ACCEL_CHAN(Z, 2, KX022A),
IIO_CHAN_SOFT_TIMESTAMP(3),
};
@@ -286,6 +224,34 @@ static const int kx022a_scale_table[][2] = {
{ 4788, 403320 },
};
+const struct kx022a_chip_info kx_chip_info[] = {
+ [KX022A] = {
+ .name = "kx022a",
+ .type = KX022A,
+ .regmap_config = &kx022a_regmap_config,
+ .channels = kx022a_channels,
+ .num_channels = ARRAY_SIZE(kx022a_channels),
+ .fifo_length = KX022A_FIFO_LENGTH,
+ .who = KX022A_REG_WHO,
+ .id = KX022A_ID,
+ .cntl = KX022A_REG_CNTL,
+ .cntl2 = KX022A_REG_CNTL2,
+ .odcntl = KX022A_REG_ODCNTL,
+ .buf_cntl1 = KX022A_REG_BUF_CNTL1,
+ .buf_cntl2 = KX022A_REG_BUF_CNTL2,
+ .buf_clear = KX022A_REG_BUF_CLEAR,
+ .buf_status1 = KX022A_REG_BUF_STATUS_1,
+ .buf_smp_lvl_mask = KX022A_MASK_BUF_SMP_LVL,
+ .buf_read = KX022A_REG_BUF_READ,
+ .inc1 = KX022A_REG_INC1,
+ .inc4 = KX022A_REG_INC4,
+ .inc5 = KX022A_REG_INC5,
+ .inc6 = KX022A_REG_INC6,
+ .xout_l = KX022A_REG_XOUT_L,
+ },
+};
+EXPORT_SYMBOL_NS_GPL(kx_chip_info, IIO_KX022A);
+
static int kx022a_read_avail(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
const int **vals, int *type, int *length,
@@ -309,19 +275,17 @@ static int kx022a_read_avail(struct iio_dev *indio_dev,
}
}
-#define KX022A_DEFAULT_PERIOD_NS (20 * NSEC_PER_MSEC)
-
static void kx022a_reg2freq(unsigned int val, int *val1, int *val2)
{
- *val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][0];
- *val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][1];
+ *val1 = kx022a_accel_samp_freq_table[val & KX_MASK_ODR][0];
+ *val2 = kx022a_accel_samp_freq_table[val & KX_MASK_ODR][1];
}
static void kx022a_reg2scale(unsigned int val, unsigned int *val1,
unsigned int *val2)
{
- val &= KX022A_MASK_GSEL;
- val >>= KX022A_GSEL_SHIFT;
+ val &= KX_MASK_GSEL;
+ val >>= KX_GSEL_SHIFT;
*val1 = kx022a_scale_table[val][0];
*val2 = kx022a_scale_table[val][1];
@@ -332,11 +296,11 @@ static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
int ret;
if (on)
- ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL,
- KX022A_MASK_PC1);
+ ret = regmap_set_bits(data->regmap, data->chip_info->cntl,
+ KX_MASK_PC1);
else
- ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
- KX022A_MASK_PC1);
+ ret = regmap_clear_bits(data->regmap, data->chip_info->cntl,
+ KX_MASK_PC1);
if (ret)
dev_err(data->dev, "Turn %s fail %d\n", str_on_off(on), ret);
@@ -403,8 +367,8 @@ static int kx022a_write_raw(struct iio_dev *idev,
break;
ret = regmap_update_bits(data->regmap,
- KX022A_REG_ODCNTL,
- KX022A_MASK_ODR, n);
+ data->chip_info->odcntl,
+ KX_MASK_ODR, n);
data->odr_ns = kx022a_odrs[n];
kx022a_turn_on_unlock(data);
break;
@@ -424,9 +388,9 @@ static int kx022a_write_raw(struct iio_dev *idev,
if (ret)
break;
- ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
- KX022A_MASK_GSEL,
- n << KX022A_GSEL_SHIFT);
+ ret = regmap_update_bits(data->regmap, data->chip_info->cntl,
+ KX_MASK_GSEL,
+ n << KX_GSEL_SHIFT);
kx022a_turn_on_unlock(data);
break;
default:
@@ -446,8 +410,8 @@ static int kx022a_fifo_set_wmi(struct kx022a_data *data)
threshold = data->watermark;
- return regmap_update_bits(data->regmap, KX022A_REG_BUF_CNTL1,
- KX022A_MASK_WM_TH, threshold);
+ return regmap_update_bits(data->regmap, data->chip_info->buf_cntl1,
+ KX_MASK_WM_TH, threshold);
}
static int kx022a_get_axis(struct kx022a_data *data,
@@ -489,11 +453,11 @@ static int kx022a_read_raw(struct iio_dev *idev,
return ret;
case IIO_CHAN_INFO_SAMP_FREQ:
- ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, ®val);
+ ret = regmap_read(data->regmap, data->chip_info->odcntl, ®val);
if (ret)
return ret;
- if ((regval & KX022A_MASK_ODR) >
+ if ((regval & KX_MASK_ODR) >
ARRAY_SIZE(kx022a_accel_samp_freq_table)) {
dev_err(data->dev, "Invalid ODR\n");
return -EINVAL;
@@ -504,7 +468,7 @@ static int kx022a_read_raw(struct iio_dev *idev,
return IIO_VAL_INT_PLUS_MICRO;
case IIO_CHAN_INFO_SCALE:
- ret = regmap_read(data->regmap, KX022A_REG_CNTL, ®val);
+ ret = regmap_read(data->regmap, data->chip_info->cntl, ®val);
if (ret < 0)
return ret;
@@ -531,8 +495,8 @@ static int kx022a_set_watermark(struct iio_dev *idev, unsigned int val)
{
struct kx022a_data *data = iio_priv(idev);
- if (val > KX022A_FIFO_LENGTH)
- val = KX022A_FIFO_LENGTH;
+ if (val > data->chip_info->fifo_length)
+ val = data->chip_info->fifo_length;
mutex_lock(&data->mutex);
data->watermark = val;
@@ -580,6 +544,37 @@ static const struct iio_dev_attr *kx022a_fifo_attributes[] = {
NULL
};
+static int kx022a_get_fifo_bytes(struct kx022a_data *data)
+{
+ struct device *dev = regmap_get_device(data->regmap);
+ __le16 buf_status;
+ int ret, fifo_bytes;
+
+ ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
+ if (ret) {
+ dev_err(dev, "Error reading buffer status\n");
+ return ret;
+ }
+
+ buf_status &= data->chip_info->buf_smp_lvl_mask;
+ fifo_bytes = le16_to_cpu(buf_status);
+
+ /*
+ * The KX022A has FIFO which can store 43 samples of HiRes data from 2
+ * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
+ * 258 bytes of sample data. The quirk to know is that the amount of bytes in
+ * the FIFO is advertised via 8 bit register (max value 255). The thing to note
+ * is that full 258 bytes of data is indicated using the max value 255.
+ */
+ if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
+ fifo_bytes = KX022A_FIFO_MAX_BYTES;
+
+ if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)
+ dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
+
+ return fifo_bytes;
+}
+
static int kx022a_drop_fifo_contents(struct kx022a_data *data)
{
/*
@@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
*/
data->timestamp = 0;
- return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
+ return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
}
static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
bool irq)
{
struct kx022a_data *data = iio_priv(idev);
- struct device *dev = regmap_get_device(data->regmap);
- __le16 buffer[KX022A_FIFO_LENGTH * 3];
+ __le16 buffer[data->chip_info->fifo_length * 3];
uint64_t sample_period;
int count, fifo_bytes;
bool renable = false;
int64_t tstamp;
int ret, i;
- ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
- if (ret) {
- dev_err(dev, "Error reading buffer status\n");
- return ret;
- }
-
- /* Let's not overflow if we for some reason get bogus value from i2c */
- if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
- fifo_bytes = KX022A_FIFO_MAX_BYTES;
-
- if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES)
- dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
-
- count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES;
+ fifo_bytes = kx022a_get_fifo_bytes(data);
+ count = fifo_bytes / KX_FIFO_SAMPLES_SIZE_BYTES;
if (!count)
return 0;
@@ -679,8 +661,8 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
count = samples;
}
- fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES;
- ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
+ fifo_bytes = count * KX_FIFO_SAMPLES_SIZE_BYTES;
+ ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read,
&buffer[0], fifo_bytes);
if (ret)
goto renable_out;
@@ -733,20 +715,18 @@ static const struct iio_info kx022a_info = {
static int kx022a_set_drdy_irq(struct kx022a_data *data, bool en)
{
if (en)
- return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
- KX022A_MASK_DRDY);
+ return regmap_set_bits(data->regmap, data->chip_info->cntl,
+ KX_MASK_DRDY);
- return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
- KX022A_MASK_DRDY);
+ return regmap_clear_bits(data->regmap, data->chip_info->cntl,
+ KX_MASK_DRDY);
}
static int kx022a_prepare_irq_pin(struct kx022a_data *data)
{
/* Enable IRQ1 pin. Set polarity to active low */
- int mask = KX022A_MASK_IEN | KX022A_MASK_IPOL |
- KX022A_MASK_ITYP;
- int val = KX022A_MASK_IEN | KX022A_IPOL_LOW |
- KX022A_ITYP_LEVEL;
+ int mask = KX_MASK_IEN | KX_MASK_IPOL | KX_MASK_ITYP;
+ int val = KX_MASK_IEN | KX_IPOL_LOW | KX_ITYP_LEVEL;
int ret;
ret = regmap_update_bits(data->regmap, data->inc_reg, mask, val);
@@ -754,7 +734,7 @@ static int kx022a_prepare_irq_pin(struct kx022a_data *data)
return ret;
/* We enable WMI to IRQ pin only at buffer_enable */
- mask = KX022A_MASK_INS2_DRDY;
+ mask = KX_MASK_INS2_DRDY;
return regmap_set_bits(data->regmap, data->ien_reg, mask);
}
@@ -767,16 +747,16 @@ static int kx022a_fifo_disable(struct kx022a_data *data)
if (ret)
return ret;
- ret = regmap_clear_bits(data->regmap, data->ien_reg, KX022A_MASK_WMI);
+ ret = regmap_clear_bits(data->regmap, data->ien_reg, KX_MASK_WMI);
if (ret)
goto unlock_out;
- ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
- KX022A_MASK_BUF_EN);
+ ret = regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
+ KX_MASK_BUF_EN);
if (ret)
goto unlock_out;
- data->state &= ~KX022A_STATE_FIFO;
+ data->state &= ~KX_STATE_FIFO;
kx022a_drop_fifo_contents(data);
@@ -812,14 +792,14 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
goto unlock_out;
/* Enable buffer */
- ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
- KX022A_MASK_BUF_EN);
+ ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
+ KX_MASK_BUF_EN);
if (ret)
goto unlock_out;
- data->state |= KX022A_STATE_FIFO;
+ data->state |= KX_STATE_FIFO;
ret = regmap_set_bits(data->regmap, data->ien_reg,
- KX022A_MASK_WMI);
+ KX_MASK_WMI);
if (ret)
goto unlock_out;
@@ -858,8 +838,8 @@ static irqreturn_t kx022a_trigger_handler(int irq, void *p)
struct kx022a_data *data = iio_priv(idev);
int ret;
- ret = regmap_bulk_read(data->regmap, KX022A_REG_XOUT_L, data->buffer,
- KX022A_FIFO_SAMPLES_SIZE_BYTES);
+ ret = regmap_bulk_read(data->regmap, data->chip_info->xout_l, data->buffer,
+ KX_FIFO_SAMPLES_SIZE_BYTES);
if (ret < 0)
goto err_read;
@@ -879,7 +859,7 @@ static irqreturn_t kx022a_irq_handler(int irq, void *private)
data->old_timestamp = data->timestamp;
data->timestamp = iio_get_time_ns(idev);
- if (data->state & KX022A_STATE_FIFO || data->trigger_enabled)
+ if (data->state & KX_STATE_FIFO || data->trigger_enabled)
return IRQ_WAKE_THREAD;
return IRQ_NONE;
@@ -903,10 +883,10 @@ static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
ret = IRQ_HANDLED;
}
- if (data->state & KX022A_STATE_FIFO) {
+ if (data->state & KX_STATE_FIFO) {
int ok;
- ok = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+ ok = __kx022a_fifo_flush(idev, data->chip_info->fifo_length, true);
if (ok > 0)
ret = IRQ_HANDLED;
}
@@ -927,7 +907,7 @@ static int kx022a_trigger_set_state(struct iio_trigger *trig,
if (data->trigger_enabled == state)
goto unlock_out;
- if (data->state & KX022A_STATE_FIFO) {
+ if (data->state & KX_STATE_FIFO) {
dev_warn(data->dev, "Can't set trigger when FIFO enabled\n");
ret = -EBUSY;
goto unlock_out;
@@ -959,7 +939,7 @@ static int kx022a_chip_init(struct kx022a_data *data)
int ret, val;
/* Reset the senor */
- ret = regmap_write(data->regmap, KX022A_REG_CNTL2, KX022A_MASK_SRST);
+ ret = regmap_write(data->regmap, data->chip_info->cntl2, KX_MASK_SRST);
if (ret)
return ret;
@@ -969,25 +949,25 @@ static int kx022a_chip_init(struct kx022a_data *data)
*/
msleep(1);
- ret = regmap_read_poll_timeout(data->regmap, KX022A_REG_CNTL2, val,
- !(val & KX022A_MASK_SRST),
- KX022A_SOFT_RESET_WAIT_TIME_US,
- KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US);
+ ret = regmap_read_poll_timeout(data->regmap, data->chip_info->cntl2, val,
+ !(val & KX_MASK_SRST),
+ KX_SOFT_RESET_WAIT_TIME_US,
+ KX_SOFT_RESET_TOTAL_WAIT_TIME_US);
if (ret) {
dev_err(data->dev, "Sensor reset %s\n",
- val & KX022A_MASK_SRST ? "timeout" : "fail#");
+ val & KX_MASK_SRST ? "timeout" : "fail#");
return ret;
}
- ret = regmap_reinit_cache(data->regmap, &kx022a_regmap);
+ ret = regmap_reinit_cache(data->regmap, data->chip_info->regmap_config);
if (ret) {
dev_err(data->dev, "Failed to reinit reg cache\n");
return ret;
}
/* set data res 16bit */
- ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
- KX022A_MASK_BRES16);
+ ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
+ KX_MASK_BRES16);
if (ret) {
dev_err(data->dev, "Failed to set data resolution\n");
return ret;
@@ -996,7 +976,7 @@ static int kx022a_chip_init(struct kx022a_data *data)
return kx022a_prepare_irq_pin(data);
}
-int kx022a_probe_internal(struct device *dev)
+int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
{
static const char * const regulator_names[] = {"io-vdd", "vdd"};
struct iio_trigger *indio_trig;
@@ -1023,6 +1003,7 @@ int kx022a_probe_internal(struct device *dev)
return -ENOMEM;
data = iio_priv(idev);
+ data->chip_info = chip_info;
/*
* VDD is the analog and digital domain voltage supply and
@@ -1033,37 +1014,37 @@ int kx022a_probe_internal(struct device *dev)
if (ret && ret != -ENODEV)
return dev_err_probe(dev, ret, "failed to enable regulator\n");
- ret = regmap_read(regmap, KX022A_REG_WHO, &chip_id);
+ ret = regmap_read(regmap, data->chip_info->who, &chip_id);
if (ret)
return dev_err_probe(dev, ret, "Failed to access sensor\n");
- if (chip_id != KX022A_ID) {
+ if (chip_id != data->chip_info->id) {
dev_err(dev, "unsupported device 0x%x\n", chip_id);
return -EINVAL;
}
irq = fwnode_irq_get_byname(fwnode, "INT1");
if (irq > 0) {
- data->inc_reg = KX022A_REG_INC1;
- data->ien_reg = KX022A_REG_INC4;
+ data->inc_reg = data->chip_info->inc1;
+ data->ien_reg = data->chip_info->inc4;
} else {
irq = fwnode_irq_get_byname(fwnode, "INT2");
if (irq <= 0)
return dev_err_probe(dev, irq, "No suitable IRQ\n");
- data->inc_reg = KX022A_REG_INC5;
- data->ien_reg = KX022A_REG_INC6;
+ data->inc_reg = data->chip_info->inc5;
+ data->ien_reg = data->chip_info->inc6;
}
data->regmap = regmap;
data->dev = dev;
data->irq = irq;
- data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
+ data->odr_ns = KX_DEFAULT_PERIOD_NS;
mutex_init(&data->mutex);
- idev->channels = kx022a_channels;
- idev->num_channels = ARRAY_SIZE(kx022a_channels);
- idev->name = "kx022-accel";
+ idev->channels = data->chip_info->channels;
+ idev->num_channels = data->chip_info->num_channels;
+ idev->name = data->chip_info->name;
idev->info = &kx022a_info;
idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
idev->available_scan_masks = kx022a_scan_masks;
@@ -1112,7 +1093,7 @@ int kx022a_probe_internal(struct device *dev)
* No need to check for NULL. request_threaded_irq() defaults to
* dev_name() should the alloc fail.
*/
- name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a",
+ name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-accel",
dev_name(data->dev));
ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
index 12424649d438..3bb40e9f5613 100644
--- a/drivers/iio/accel/kionix-kx022a.h
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -11,24 +11,48 @@
#include <linux/bits.h>
#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+
+/* Common for all supported devices */
+#define KX_FIFO_SAMPLES_SIZE_BYTES 6
+#define KX_SOFT_RESET_WAIT_TIME_US (5 * USEC_PER_MSEC)
+#define KX_SOFT_RESET_TOTAL_WAIT_TIME_US (500 * USEC_PER_MSEC)
+#define KX_DEFAULT_PERIOD_NS (20 * NSEC_PER_MSEC)
+#define KX_MASK_GSEL GENMASK(4, 3)
+#define KX_MASK_ODR GENMASK(3, 0)
+#define KX_MASK_WM_TH GENMASK(6, 0)
+#define KX_GSEL_SHIFT 3
+#define KX_MASK_IEN BIT(5)
+#define KX_MASK_DRDY BIT(5)
+#define KX_MASK_PC1 BIT(7)
+#define KX_MASK_IPOL BIT(4)
+#define KX_IPOL_LOW 0
+#define KX_MASK_ITYP BIT(3)
+#define KX_ITYP_LEVEL 0
+#define KX_MASK_INS2_DRDY BIT(4)
+#define KX_MASK_WMI BIT(5)
+#define KX_MASK_BUF_EN BIT(7)
+#define KX_MASK_SRST BIT(7)
+#define KX_MASK_BRES16 BIT(6)
+
+
#define KX022A_REG_WHO 0x0f
#define KX022A_ID 0xc8
+#define KX022A_FIFO_LENGTH 43
+#define KX022A_FIFO_FULL_VALUE 255
+#define KX022A_FIFO_MAX_BYTES \
+ (KX022A_FIFO_LENGTH * KX_FIFO_SAMPLES_SIZE_BYTES)
+
#define KX022A_REG_CNTL2 0x19
-#define KX022A_MASK_SRST BIT(7)
#define KX022A_REG_CNTL 0x18
-#define KX022A_MASK_PC1 BIT(7)
#define KX022A_MASK_RES BIT(6)
-#define KX022A_MASK_DRDY BIT(5)
-#define KX022A_MASK_GSEL GENMASK(4, 3)
-#define KX022A_GSEL_SHIFT 3
#define KX022A_GSEL_2 0x0
#define KX022A_GSEL_4 BIT(3)
#define KX022A_GSEL_8 BIT(4)
#define KX022A_GSEL_16 GENMASK(4, 3)
#define KX022A_REG_INS2 0x13
-#define KX022A_MASK_INS2_DRDY BIT(4)
#define KX122_MASK_INS2_WMI BIT(5)
#define KX022A_REG_XHP_L 0x0
@@ -45,38 +69,104 @@
#define KX022A_REG_MAN_WAKE 0x2c
#define KX022A_REG_BUF_CNTL1 0x3a
-#define KX022A_MASK_WM_TH GENMASK(6, 0)
#define KX022A_REG_BUF_CNTL2 0x3b
-#define KX022A_MASK_BUF_EN BIT(7)
-#define KX022A_MASK_BRES16 BIT(6)
#define KX022A_REG_BUF_STATUS_1 0x3c
#define KX022A_REG_BUF_STATUS_2 0x3d
+#define KX022A_MASK_BUF_SMP_LVL GENMASK(6, 0)
#define KX022A_REG_BUF_CLEAR 0x3e
#define KX022A_REG_BUF_READ 0x3f
-#define KX022A_MASK_ODR GENMASK(3, 0)
#define KX022A_ODR_SHIFT 3
#define KX022A_FIFO_MAX_WMI_TH 41
#define KX022A_REG_INC1 0x1c
#define KX022A_REG_INC5 0x20
#define KX022A_REG_INC6 0x21
-#define KX022A_MASK_IEN BIT(5)
-#define KX022A_MASK_IPOL BIT(4)
#define KX022A_IPOL_LOW 0
-#define KX022A_IPOL_HIGH KX022A_MASK_IPOL1
-#define KX022A_MASK_ITYP BIT(3)
-#define KX022A_ITYP_PULSE KX022A_MASK_ITYP
-#define KX022A_ITYP_LEVEL 0
+#define KX022A_IPOL_HIGH KX_MASK_IPOL
+#define KX022A_ITYP_PULSE KX_MASK_ITYP
#define KX022A_REG_INC4 0x1f
-#define KX022A_MASK_WMI BIT(5)
#define KX022A_REG_SELF_TEST 0x60
#define KX022A_MAX_REGISTER 0x60
+enum kx022a_device_type {
+ KX022A,
+};
+
+enum {
+ KX_STATE_SAMPLE,
+ KX_STATE_FIFO,
+};
+
+enum {
+ AXIS_X,
+ AXIS_Y,
+ AXIS_Z,
+ AXIS_MAX
+};
+
struct device;
-int kx022a_probe_internal(struct device *dev);
-extern const struct regmap_config kx022a_regmap;
+struct kx022a_chip_info {
+ const char *name;
+ enum kx022a_device_type type;
+ const struct regmap_config *regmap_config;
+ const struct iio_chan_spec *channels;
+ unsigned int num_channels;
+ unsigned int fifo_length;
+ u8 who;
+ u8 id;
+ u8 cntl;
+ u8 cntl2;
+ u8 odcntl;
+ u8 buf_cntl1;
+ u8 buf_cntl2;
+ u8 buf_clear;
+ u8 buf_status1;
+ u16 buf_smp_lvl_mask;
+ u8 buf_read;
+ u8 inc1;
+ u8 inc4;
+ u8 inc5;
+ u8 inc6;
+ u8 xout_l;
+};
+
+struct kx022a_data {
+ const struct kx022a_chip_info *chip_info;
+ struct regmap *regmap;
+ struct iio_trigger *trig;
+ struct device *dev;
+ struct iio_mount_matrix orientation;
+ int64_t timestamp, old_timestamp;
+
+ int irq;
+ int inc_reg;
+ int ien_reg;
+
+ unsigned int state;
+ unsigned int odr_ns;
+
+ bool trigger_enabled;
+ /*
+ * Prevent toggling the sensor stby/active state (PC1 bit) in the
+ * middle of a configuration, or when the fifo is enabled. Also,
+ * protect the data stored/retrieved from this structure from
+ * concurrent accesses.
+ */
+ struct mutex mutex;
+ u8 watermark;
+
+ /* 3 x 16bit accel data + timestamp */
+ __le16 buffer[8] __aligned(IIO_DMA_MINALIGN);
+ struct {
+ __le16 channels[3];
+ s64 ts __aligned(8);
+ } scan;
+};
+
+int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);
+extern const struct kx022a_chip_info kx_chip_info[];
#endif
--
2.30.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer
2023-03-16 23:48 [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
2023-03-16 23:48 ` [PATCH 1/3] dt-bindings: iio: Add " Mehdi Djait
2023-03-16 23:48 ` [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure Mehdi Djait
@ 2023-03-16 23:48 ` Mehdi Djait
2023-03-19 16:22 ` Jonathan Cameron
2023-03-20 9:57 ` Matti Vaittinen
2023-03-17 12:07 ` [PATCH 0/3] " Andy Shevchenko
2023-03-19 7:43 ` Matti Vaittinen
4 siblings, 2 replies; 30+ messages in thread
From: Mehdi Djait @ 2023-03-16 23:48 UTC (permalink / raw)
To: jic23, mazziesaccount
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, linux-iio,
linux-kernel, devicetree, Mehdi Djait
Add support for the basic accelerometer features such as getting the
acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
using the WMI IRQ).
Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
drivers/iio/accel/kionix-kx022a-i2c.c | 2 +
drivers/iio/accel/kionix-kx022a-spi.c | 2 +
drivers/iio/accel/kionix-kx022a.c | 126 ++++++++++++++++++++++++++
drivers/iio/accel/kionix-kx022a.h | 53 +++++++++++
4 files changed, 183 insertions(+)
diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
index 21c4c0ae1a68..f9b2383c43f1 100644
--- a/drivers/iio/accel/kionix-kx022a-i2c.c
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -38,12 +38,14 @@ static int kx022a_i2c_probe(struct i2c_client *i2c)
static const struct i2c_device_id kx022a_i2c_id[] = {
{ .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
+ { .name = "kx132", .driver_data = (kernel_ulong_t)&kx_chip_info[KX132] },
{ }
};
MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
static const struct of_device_id kx022a_of_match[] = {
{ .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] },
+ { .compatible = "kionix,kx132", .data = &kx_chip_info[KX132] },
{ }
};
MODULE_DEVICE_TABLE(of, kx022a_of_match);
diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c
index ec076af0f261..86a10d6d33ff 100644
--- a/drivers/iio/accel/kionix-kx022a-spi.c
+++ b/drivers/iio/accel/kionix-kx022a-spi.c
@@ -38,12 +38,14 @@ static int kx022a_spi_probe(struct spi_device *spi)
static const struct spi_device_id kx022a_spi_id[] = {
{ .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
+ { .name = "kx132", .driver_data = (kernel_ulong_t)&kx_chip_info[KX132] },
{ }
};
MODULE_DEVICE_TABLE(spi, kx022a_spi_id);
static const struct of_device_id kx022a_of_match[] = {
{ .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] },
+ { .compatible = "kionix,kx132", .data = &kx_chip_info[KX132] },
{ }
};
MODULE_DEVICE_TABLE(of, kx022a_of_match);
diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index 27e8642aa8f5..3cacec99f792 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -128,6 +128,101 @@ static const struct regmap_config kx022a_regmap_config = {
.cache_type = REGCACHE_RBTREE,
};
+/* Regmap configs kx132 */
+static const struct regmap_range kx132_volatile_ranges[] = {
+ {
+ .range_min = KX132_REG_XADP_L,
+ .range_max = KX132_REG_COTR,
+ }, {
+ .range_min = KX132_REG_TSCP,
+ .range_max = KX132_REG_INT_REL,
+ }, {
+ /* The reset bit will be cleared by sensor */
+ .range_min = KX132_REG_CNTL2,
+ .range_max = KX132_REG_CNTL2,
+ }, {
+ .range_min = KX132_REG_BUF_STATUS_1,
+ .range_max = KX132_REG_BUF_READ,
+ },
+};
+
+static const struct regmap_access_table kx132_volatile_regs = {
+ .yes_ranges = &kx132_volatile_ranges[0],
+ .n_yes_ranges = ARRAY_SIZE(kx132_volatile_ranges),
+};
+
+static const struct regmap_range kx132_precious_ranges[] = {
+ {
+ .range_min = KX132_REG_INT_REL,
+ .range_max = KX132_REG_INT_REL,
+ },
+};
+
+static const struct regmap_access_table kx132_precious_regs = {
+ .yes_ranges = &kx132_precious_ranges[0],
+ .n_yes_ranges = ARRAY_SIZE(kx132_precious_ranges),
+};
+
+static const struct regmap_range kx132_read_only_ranges[] = {
+ {
+ .range_min = KX132_REG_XADP_L,
+ .range_max = KX132_REG_INT_REL,
+ }, {
+ .range_min = KX132_REG_BUF_STATUS_1,
+ .range_max = KX132_REG_BUF_STATUS_2,
+ }, {
+ .range_min = KX132_REG_BUF_READ,
+ .range_max = KX132_REG_BUF_READ,
+ },
+};
+
+static const struct regmap_access_table kx132_ro_regs = {
+ .no_ranges = &kx132_read_only_ranges[0],
+ .n_no_ranges = ARRAY_SIZE(kx132_read_only_ranges),
+};
+
+static const struct regmap_range kx132_write_only_ranges[] = {
+ {
+ .range_min = KX132_REG_MAN_WAKE,
+ .range_max = KX132_REG_MAN_WAKE,
+ }, {
+ .range_min = KX132_REG_SELF_TEST,
+ .range_max = KX132_REG_SELF_TEST,
+ }, {
+ .range_min = KX132_REG_BUF_CLEAR,
+ .range_max = KX132_REG_BUF_CLEAR,
+ },
+};
+
+static const struct regmap_access_table kx132_wo_regs = {
+ .no_ranges = &kx132_write_only_ranges[0],
+ .n_no_ranges = ARRAY_SIZE(kx132_write_only_ranges),
+};
+
+static const struct regmap_range kx132_noinc_read_ranges[] = {
+ {
+ .range_min = KX132_REG_BUF_READ,
+ .range_max = KX132_REG_BUF_READ,
+ },
+};
+
+static const struct regmap_access_table kx132_nir_regs = {
+ .yes_ranges = &kx132_noinc_read_ranges[0],
+ .n_yes_ranges = ARRAY_SIZE(kx132_noinc_read_ranges),
+};
+
+static const struct regmap_config kx132_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .volatile_table = &kx132_volatile_regs,
+ .rd_table = &kx132_wo_regs,
+ .wr_table = &kx132_ro_regs,
+ .rd_noinc_table = &kx132_nir_regs,
+ .precious_table = &kx132_precious_regs,
+ .max_register = KX132_MAX_REGISTER,
+ .cache_type = REGCACHE_RBTREE,
+};
+
static const struct iio_mount_matrix *
kx022a_get_mount_matrix(const struct iio_dev *idev,
const struct iio_chan_spec *chan)
@@ -175,6 +270,13 @@ static const struct iio_chan_spec kx022a_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(3),
};
+static const struct iio_chan_spec kx132_channels[] = {
+ KX022A_ACCEL_CHAN(X, 0, KX132),
+ KX022A_ACCEL_CHAN(Y, 1, KX132),
+ KX022A_ACCEL_CHAN(Z, 2, KX132),
+ IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
/*
* The sensor HW can support ODR up to 1600 Hz, which is beyond what most of the
* Linux CPUs can handle without dropping samples. Also, the low power mode is
@@ -249,6 +351,30 @@ const struct kx022a_chip_info kx_chip_info[] = {
.inc6 = KX022A_REG_INC6,
.xout_l = KX022A_REG_XOUT_L,
},
+ [KX132] = {
+ .name = "kx132",
+ .type = KX132,
+ .regmap_config = &kx132_regmap_config,
+ .channels = kx132_channels,
+ .num_channels = ARRAY_SIZE(kx132_channels),
+ .fifo_length = KX132_FIFO_LENGTH,
+ .who = KX132_REG_WHO,
+ .id = KX132_ID,
+ .cntl = KX132_REG_CNTL,
+ .cntl2 = KX132_REG_CNTL2,
+ .odcntl = KX132_REG_ODCNTL,
+ .buf_cntl1 = KX132_REG_BUF_CNTL1,
+ .buf_cntl2 = KX132_REG_BUF_CNTL2,
+ .buf_clear = KX132_REG_BUF_CLEAR,
+ .buf_status1 = KX132_REG_BUF_STATUS_1,
+ .buf_smp_lvl_mask = KX132_MASK_BUF_SMP_LVL,
+ .buf_read = KX132_REG_BUF_READ,
+ .inc1 = KX132_REG_INC1,
+ .inc4 = KX132_REG_INC4,
+ .inc5 = KX132_REG_INC5,
+ .inc6 = KX132_REG_INC6,
+ .xout_l = KX132_REG_XOUT_L,
+ },
};
EXPORT_SYMBOL_NS_GPL(kx_chip_info, IIO_KX022A);
diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
index 3bb40e9f5613..7e43bdb37156 100644
--- a/drivers/iio/accel/kionix-kx022a.h
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -90,8 +90,61 @@
#define KX022A_REG_SELF_TEST 0x60
#define KX022A_MAX_REGISTER 0x60
+
+#define KX132_REG_WHO 0x13
+#define KX132_ID 0x3d
+
+#define KX132_FIFO_LENGTH 86
+
+#define KX132_REG_CNTL2 0x1c
+#define KX132_REG_CNTL 0x1b
+#define KX132_MASK_RES BIT(6)
+#define KX132_GSEL_2 0x0
+#define KX132_GSEL_4 BIT(3)
+#define KX132_GSEL_8 BIT(4)
+#define KX132_GSEL_16 GENMASK(4, 3)
+
+#define KX132_REG_INS2 0x17
+#define KX132_MASK_INS2_WMI BIT(5)
+
+#define KX132_REG_XADP_L 0x02
+#define KX132_REG_XOUT_L 0x08
+#define KX132_REG_YOUT_L 0x0a
+#define KX132_REG_ZOUT_L 0x0c
+#define KX132_REG_COTR 0x12
+#define KX132_REG_TSCP 0x14
+#define KX132_REG_INT_REL 0x1a
+
+#define KX132_REG_ODCNTL 0x21
+
+#define KX132_REG_BTS_WUF_TH 0x4a
+#define KX132_REG_MAN_WAKE 0x4d
+
+#define KX132_REG_BUF_CNTL1 0x5e
+#define KX132_REG_BUF_CNTL2 0x5f
+#define KX132_REG_BUF_STATUS_1 0x60
+#define KX132_REG_BUF_STATUS_2 0x61
+#define KX132_MASK_BUF_SMP_LVL GENMASK(9, 0)
+#define KX132_REG_BUF_CLEAR 0x62
+#define KX132_REG_BUF_READ 0x63
+#define KX132_ODR_SHIFT 3
+#define KX132_FIFO_MAX_WMI_TH 86
+
+#define KX132_REG_INC1 0x22
+#define KX132_REG_INC5 0x26
+#define KX132_REG_INC6 0x27
+#define KX132_IPOL_LOW 0
+#define KX132_IPOL_HIGH KX_MASK_IPOL
+#define KX132_ITYP_PULSE KX_MASK_ITYP
+
+#define KX132_REG_INC4 0x25
+
+#define KX132_REG_SELF_TEST 0x5d
+#define KX132_MAX_REGISTER 0x76
+
enum kx022a_device_type {
KX022A,
+ KX132,
};
enum {
--
2.30.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-16 23:48 ` [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure Mehdi Djait
@ 2023-03-17 1:01 ` kernel test robot
2023-03-17 4:57 ` kernel test robot
` (4 subsequent siblings)
5 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2023-03-17 1:01 UTC (permalink / raw)
To: Mehdi Djait, jic23, mazziesaccount
Cc: oe-kbuild-all, krzysztof.kozlowski+dt, andriy.shevchenko,
linux-iio, linux-kernel, devicetree, Mehdi Djait
Hi Mehdi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on next-20230316]
[cannot apply to linus/master v6.3-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mehdi-Djait/dt-bindings-iio-Add-KX132-accelerometer/20230317-075056
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/3ddca10a4c03c3a64afb831cc9dd1e01fe89d305.1679009443.git.mehdi.djait.k%40gmail.com
patch subject: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230317/202303170813.jSOLGCL5-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/40c75341c42d0e5bea5d73961202978a4be41cd2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mehdi-Djait/dt-bindings-iio-Add-KX132-accelerometer/20230317-075056
git checkout 40c75341c42d0e5bea5d73961202978a4be41cd2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/iio/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303170813.jSOLGCL5-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/iio/accel/kionix-kx022a.c: In function '__kx022a_fifo_flush':
>> drivers/iio/accel/kionix-kx022a.c:598:9: warning: ISO C90 forbids variable length array 'buffer' [-Wvla]
598 | __le16 buffer[data->chip_info->fifo_length * 3];
| ^~~~~~
--
drivers/iio/accel/kionix-kx022a-i2c.c: In function 'kx022a_i2c_probe':
>> drivers/iio/accel/kionix-kx022a-i2c.c:27:19: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
27 | chip_info = device_get_match_data(&i2c->dev);
| ^
drivers/iio/accel/kionix-kx022a-i2c.c:29:27: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
29 | chip_info = (const struct kx022a_chip_info *) id->driver_data;
| ^
--
drivers/iio/accel/kionix-kx022a-spi.c: In function 'kx022a_spi_probe':
>> drivers/iio/accel/kionix-kx022a-spi.c:27:19: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
27 | chip_info = device_get_match_data(&spi->dev);
| ^
drivers/iio/accel/kionix-kx022a-spi.c:29:27: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
29 | chip_info = (const struct kx022a_chip_info *) id->driver_data;
| ^
vim +/buffer +598 drivers/iio/accel/kionix-kx022a.c
593
594 static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
595 bool irq)
596 {
597 struct kx022a_data *data = iio_priv(idev);
> 598 __le16 buffer[data->chip_info->fifo_length * 3];
599 uint64_t sample_period;
600 int count, fifo_bytes;
601 bool renable = false;
602 int64_t tstamp;
603 int ret, i;
604
605 fifo_bytes = kx022a_get_fifo_bytes(data);
606 count = fifo_bytes / KX_FIFO_SAMPLES_SIZE_BYTES;
607 if (!count)
608 return 0;
609
610 /*
611 * If we are being called from IRQ handler we know the stored timestamp
612 * is fairly accurate for the last stored sample. Otherwise, if we are
613 * called as a result of a read operation from userspace and hence
614 * before the watermark interrupt was triggered, take a timestamp
615 * now. We can fall anywhere in between two samples so the error in this
616 * case is at most one sample period.
617 */
618 if (!irq) {
619 /*
620 * We need to have the IRQ disabled or we risk of messing-up
621 * the timestamps. If we are ran from IRQ, then the
622 * IRQF_ONESHOT has us covered - but if we are ran by the
623 * user-space read we need to disable the IRQ to be on a safe
624 * side. We do this usng synchronous disable so that if the
625 * IRQ thread is being ran on other CPU we wait for it to be
626 * finished.
627 */
628 disable_irq(data->irq);
629 renable = true;
630
631 data->old_timestamp = data->timestamp;
632 data->timestamp = iio_get_time_ns(idev);
633 }
634
635 /*
636 * Approximate timestamps for each of the sample based on the sampling
637 * frequency, timestamp for last sample and number of samples.
638 *
639 * We'd better not use the current bandwidth settings to compute the
640 * sample period. The real sample rate varies with the device and
641 * small variation adds when we store a large number of samples.
642 *
643 * To avoid this issue we compute the actual sample period ourselves
644 * based on the timestamp delta between the last two flush operations.
645 */
646 if (data->old_timestamp) {
647 sample_period = data->timestamp - data->old_timestamp;
648 do_div(sample_period, count);
649 } else {
650 sample_period = data->odr_ns;
651 }
652 tstamp = data->timestamp - (count - 1) * sample_period;
653
654 if (samples && count > samples) {
655 /*
656 * Here we leave some old samples to the buffer. We need to
657 * adjust the timestamp to match the first sample in the buffer
658 * or we will miscalculate the sample_period at next round.
659 */
660 data->timestamp -= (count - samples) * sample_period;
661 count = samples;
662 }
663
664 fifo_bytes = count * KX_FIFO_SAMPLES_SIZE_BYTES;
665 ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read,
666 &buffer[0], fifo_bytes);
667 if (ret)
668 goto renable_out;
669
670 for (i = 0; i < count; i++) {
671 __le16 *sam = &buffer[i * 3];
672 __le16 *chs;
673 int bit;
674
675 chs = &data->scan.channels[0];
676 for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
677 chs[bit] = sam[bit];
678
679 iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
680
681 tstamp += sample_period;
682 }
683
684 ret = count;
685
686 renable_out:
687 if (renable)
688 enable_irq(data->irq);
689
690 return ret;
691 }
692
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-16 23:48 ` [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure Mehdi Djait
2023-03-17 1:01 ` kernel test robot
@ 2023-03-17 4:57 ` kernel test robot
2023-03-17 12:06 ` Andy Shevchenko
` (3 subsequent siblings)
5 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2023-03-17 4:57 UTC (permalink / raw)
To: Mehdi Djait, jic23, mazziesaccount
Cc: llvm, oe-kbuild-all, krzysztof.kozlowski+dt, andriy.shevchenko,
linux-iio, linux-kernel, devicetree, Mehdi Djait
Hi Mehdi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on next-20230317]
[cannot apply to linus/master v6.3-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mehdi-Djait/dt-bindings-iio-Add-KX132-accelerometer/20230317-075056
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/3ddca10a4c03c3a64afb831cc9dd1e01fe89d305.1679009443.git.mehdi.djait.k%40gmail.com
patch subject: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
config: i386-randconfig-a011-20230313 (https://download.01.org/0day-ci/archive/20230317/202303171208.uYJzdkrv-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/40c75341c42d0e5bea5d73961202978a4be41cd2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mehdi-Djait/dt-bindings-iio-Add-KX132-accelerometer/20230317-075056
git checkout 40c75341c42d0e5bea5d73961202978a4be41cd2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/iio/accel/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303171208.uYJzdkrv-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
>> drivers/iio/accel/kionix-kx022a-i2c.c:27:12: error: assigning to 'struct kx022a_chip_info *' from 'const void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
chip_info = device_get_match_data(&i2c->dev);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/accel/kionix-kx022a-i2c.c:29:13: error: assigning to 'struct kx022a_chip_info *' from 'const struct kx022a_chip_info *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
chip_info = (const struct kx022a_chip_info *) id->driver_data;
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
--
>> drivers/iio/accel/kionix-kx022a.c:598:16: warning: variable length array used [-Wvla]
__le16 buffer[data->chip_info->fifo_length * 3];
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
--
>> drivers/iio/accel/kionix-kx022a-spi.c:27:12: error: assigning to 'struct kx022a_chip_info *' from 'const void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
chip_info = device_get_match_data(&spi->dev);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/accel/kionix-kx022a-spi.c:29:13: error: assigning to 'struct kx022a_chip_info *' from 'const struct kx022a_chip_info *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
chip_info = (const struct kx022a_chip_info *) id->driver_data;
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
vim +27 drivers/iio/accel/kionix-kx022a-i2c.c
14
15 static int kx022a_i2c_probe(struct i2c_client *i2c)
16 {
17 struct device *dev = &i2c->dev;
18 struct kx022a_chip_info *chip_info;
19 struct regmap *regmap;
20 const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
21
22 if (!i2c->irq) {
23 dev_err(dev, "No IRQ configured\n");
24 return -EINVAL;
25 }
26
> 27 chip_info = device_get_match_data(&i2c->dev);
28 if (!chip_info)
> 29 chip_info = (const struct kx022a_chip_info *) id->driver_data;
30
31 regmap = devm_regmap_init_i2c(i2c, chip_info->regmap_config);
32 if (IS_ERR(regmap))
33 return dev_err_probe(dev, PTR_ERR(regmap),
34 "Failed to initialize Regmap\n");
35
36 return kx022a_probe_internal(dev, chip_info);
37 }
38
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-16 23:48 ` [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure Mehdi Djait
2023-03-17 1:01 ` kernel test robot
2023-03-17 4:57 ` kernel test robot
@ 2023-03-17 12:06 ` Andy Shevchenko
2023-03-18 16:12 ` Mehdi Djait
2023-03-19 16:20 ` Jonathan Cameron
` (2 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-03-17 12:06 UTC (permalink / raw)
To: Mehdi Djait
Cc: jic23, mazziesaccount, krzysztof.kozlowski+dt, linux-iio,
linux-kernel, devicetree
On Fri, Mar 17, 2023 at 12:48:36AM +0100, Mehdi Djait wrote:
> Refactor the kx022a driver implementation to make it more
> generic and extensible.
> Add the chip_info structure will to the driver's private
> data to hold all the device specific infos.
> Move the enum, struct and constants definitions to the header
> file.
Please, compile and test before sending.
...
> .driver = {
> - .name = "kx022a-spi",
> + .name = "kx022a-spi",
> .of_match_table = kx022a_of_match,
> },
What was changed here?
...
> - .id_table = kx022a_id,
> + .id_table = kx022a_spi_id,
Why do we need this change?
...
> - name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a",
> + name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-accel",
> dev_name(data->dev));
Shouldn't you use the name from chip info?
...
> +#define KX_MASK_BRES16 BIT(6)
> +
> +
One blank line is enough.
> #define KX022A_REG_WHO 0x0f
> #define KX022A_ID 0xc8
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer
2023-03-16 23:48 [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
` (2 preceding siblings ...)
2023-03-16 23:48 ` [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
@ 2023-03-17 12:07 ` Andy Shevchenko
2023-03-18 15:55 ` Mehdi Djait
2023-03-19 7:43 ` Matti Vaittinen
4 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-03-17 12:07 UTC (permalink / raw)
To: Mehdi Djait
Cc: jic23, mazziesaccount, krzysztof.kozlowski+dt, linux-iio,
linux-kernel, devicetree
On Fri, Mar 17, 2023 at 12:48:34AM +0100, Mehdi Djait wrote:
> KX132 accelerometer is a sensor which:
> - supports G-ranges of (+/-) 2, 4, 8, and 16G
> - can be connected to I2C or SPI
> - has internal HW FIFO buffer
> - supports various ODRs (output data rates)
>
> The KX132 accelerometer is very similair to the KX022A.
> One key difference is number of bits to report the number of data bytes that
> have been stored in the sample buffer: 8 bits for KX022A vs 10 bits for KX132.
>
> A complete list of differences is listed in [1]
> [1] https://kionixfs.azureedge.net/en/document/AN112-Transitioning-to-KX132-1211-Accelerometer.pdf1
Is it really the first version of this contribution?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer
2023-03-17 12:07 ` [PATCH 0/3] " Andy Shevchenko
@ 2023-03-18 15:55 ` Mehdi Djait
0 siblings, 0 replies; 30+ messages in thread
From: Mehdi Djait @ 2023-03-18 15:55 UTC (permalink / raw)
To: Andy Shevchenko
Cc: jic23, mazziesaccount, krzysztof.kozlowski+dt, linux-iio,
linux-kernel, devicetree
Hi Andy,
On Fri, Mar 17, 2023 at 02:07:49PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 17, 2023 at 12:48:34AM +0100, Mehdi Djait wrote:
> > KX132 accelerometer is a sensor which:
> > - supports G-ranges of (+/-) 2, 4, 8, and 16G
> > - can be connected to I2C or SPI
> > - has internal HW FIFO buffer
> > - supports various ODRs (output data rates)
> >
> > The KX132 accelerometer is very similair to the KX022A.
> > One key difference is number of bits to report the number of data bytes that
> > have been stored in the sample buffer: 8 bits for KX022A vs 10 bits for KX132.
> >
> > A complete list of differences is listed in [1]
> > [1] https://kionixfs.azureedge.net/en/document/AN112-Transitioning-to-KX132-1211-Accelerometer.pdf1
>
> Is it really the first version of this contribution?
>
Yes this is my first series for the KX132 Support.
--
Kind Regards
Mehdi Djait
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-17 12:06 ` Andy Shevchenko
@ 2023-03-18 16:12 ` Mehdi Djait
0 siblings, 0 replies; 30+ messages in thread
From: Mehdi Djait @ 2023-03-18 16:12 UTC (permalink / raw)
To: Andy Shevchenko
Cc: jic23, mazziesaccount, krzysztof.kozlowski+dt, linux-iio,
linux-kernel, devicetree
Hi Andy,
On Fri, Mar 17, 2023 at 02:06:39PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 17, 2023 at 12:48:36AM +0100, Mehdi Djait wrote:
> > Refactor the kx022a driver implementation to make it more
> > generic and extensible.
> > Add the chip_info structure will to the driver's private
> > data to hold all the device specific infos.
> > Move the enum, struct and constants definitions to the header
> > file.
>
> Please, compile and test before sending.
My bad, I ignored the warnings...
I will fix it.
>
> ...
>
> > .driver = {
> > - .name = "kx022a-spi",
> > + .name = "kx022a-spi",
> > .of_match_table = kx022a_of_match,
> > },
>
> What was changed here?
Nothing. I will fix it
>
> ...
>
> > - .id_table = kx022a_id,
> > + .id_table = kx022a_spi_id,
>
> Why do we need this change?
>
For consistency:
i2c: .id_table = kx022a_i2c_id,
spi: .id_table = kx022a_spi_id,
> ...
>
> > - name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a",
> > + name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-accel",
> > dev_name(data->dev));
>
> Shouldn't you use the name from chip info?
I can use the name from chip info.
dev_name(data->dev) is the original implementation
>
> ...
>
> > +#define KX_MASK_BRES16 BIT(6)
> > +
> > +
>
> One blank line is enough.
I will change it in v2
--
Kind Regards
Mehdi Djait
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer
2023-03-16 23:48 [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
` (3 preceding siblings ...)
2023-03-17 12:07 ` [PATCH 0/3] " Andy Shevchenko
@ 2023-03-19 7:43 ` Matti Vaittinen
2023-03-21 13:16 ` Mehdi Djait
4 siblings, 1 reply; 30+ messages in thread
From: Matti Vaittinen @ 2023-03-19 7:43 UTC (permalink / raw)
To: Mehdi Djait, jic23
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, linux-iio,
linux-kernel, devicetree
Hi Mehdi,
Things have been piling up for me during last two weeks... I will do
proper review during next week.
On 3/17/23 01:48, Mehdi Djait wrote:
> KX132 accelerometer is a sensor which:
> - supports G-ranges of (+/-) 2, 4, 8, and 16G
> - can be connected to I2C or SPI
> - has internal HW FIFO buffer
> - supports various ODRs (output data rates)
>
> The KX132 accelerometer is very similair to the KX022A.
> One key difference is number of bits to report the number of data bytes that
> have been stored in the sample buffer: 8 bits for KX022A vs 10 bits for KX132.
The KX022A has 16bits of data in HiRes mode. This is the default for
kx022a driver.
> A complete list of differences is listed in [1]
>
>
> [1] https://kionixfs.azureedge.net/en/document/AN112-Transitioning-to-KX132-1211-Accelerometer.pdf1
This document is somewhat misleading. It does not contain KX022A but the
older KX022. Kionix has the somewhat confusing habit of having very
similar names for models with - occasionally significant - differences.
(My own opinion).
I the "Technical referene manual" is more interesting document than the
data-sheet:
https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
I have heard that there have been a few very different versions of KX132
as well. Not sure if they have "leaked" out to public though. In any
case, for the kx132 it might be safest to use the full model name -
especially in the DT compatibles.
Finally, AFAIK the key "thing" in KX132 is the "ADP" (Advanced Data
Path) feature which allows filtering the data "in sensor".
Unfortunately, I am not really familiar with this feature. Do you think
this is something that might get configured only once at start-up
depending on the purpose of the board? If yes, this might be something
that will end-up having properties in device-tree. If yes, then it might
be a good idea to have own binding doc for KX132. Currently it seems Ok
to have them in the same binding doc though.
Anyways, I'll have proper look at this series during the next week -
Thanks for the contribution! Much appreciated!
Yours,
-- Matti
> Mehdi Djait (3):
> dt-bindings: iio: Add KX132 accelerometer
> iio: accel: kionix-kx022a: Add chip_info structure
> iio: accel: Add support for Kionix/ROHM KX132 accelerometer
>
> .../bindings/iio/accel/kionix,kx022a.yaml | 13 +-
> drivers/iio/accel/kionix-kx022a-i2c.c | 21 +-
> drivers/iio/accel/kionix-kx022a-spi.c | 24 +-
> drivers/iio/accel/kionix-kx022a.c | 413 +++++++++++-------
> drivers/iio/accel/kionix-kx022a.h | 181 +++++++-
> 5 files changed, 464 insertions(+), 188 deletions(-)
>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: Add KX132 accelerometer
2023-03-16 23:48 ` [PATCH 1/3] dt-bindings: iio: Add " Mehdi Djait
@ 2023-03-19 15:54 ` Jonathan Cameron
2023-03-21 13:22 ` Mehdi Djait
0 siblings, 1 reply; 30+ messages in thread
From: Jonathan Cameron @ 2023-03-19 15:54 UTC (permalink / raw)
To: Mehdi Djait
Cc: mazziesaccount, krzysztof.kozlowski+dt, andriy.shevchenko,
linux-iio, linux-kernel, devicetree
On Fri, 17 Mar 2023 00:48:35 +0100
Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> Extend the kionix,kx022a.yaml file to support the
> kx132 device
>
> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
Pins and power supplies etc all look the same to me so indeed seems that
you have covered all that is needed. One small comment inline
and I think Matti's point about more specific compatibles probably
needs to be taken into account if there are known variants.
Kionix has done this for a long time. I remember that fun with the
kxsd9 lots of years back - that had lots of subtle variants.
> ---
> .../bindings/iio/accel/kionix,kx022a.yaml | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml b/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
> index 986df1a6ff0a..ac1e27402d5e 100644
> --- a/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
> @@ -4,19 +4,22 @@
> $id: http://devicetree.org/schemas/iio/accel/kionix,kx022a.yaml#
> $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> -title: ROHM/Kionix KX022A Accelerometer
> +title: ROHM/Kionix KX022A and KX132 Accelerometers
>
> maintainers:
> - Matti Vaittinen <mazziesaccount@gmail.com>
>
> description: |
> - KX022A is a 3-axis accelerometer supporting +/- 2G, 4G, 8G and 16G ranges,
> - output data-rates from 0.78Hz to 1600Hz and a hardware-fifo buffering.
> - KX022A can be accessed either via I2C or SPI.
> + KX022A and KX132 are 3-axis accelerometers supporting +/- 2G, 4G, 8G and
> + 16G ranges, output data-rates from 0.78Hz to 1600Hz and a hardware-fifo
This may be one of those 'there are many versions' of the chip issues, but
the random datasheet I got via digikey (kionix website was slow and I'm
impatient) has max as 25600Hz for the KX132-1211.
No particular reason the sampling rates need to be in this description so
if they are different I'd just remove the mention or just say
"variable output data-rates"
> + buffering.
> + KX022A and KX132 can be accessed either via I2C or SPI.
>
> properties:
> compatible:
> - const: kionix,kx022a
> + enum:
> + - kionix,kx022a
> + - kionix,kx132
>
> reg:
> maxItems: 1
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-16 23:48 ` [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure Mehdi Djait
` (2 preceding siblings ...)
2023-03-17 12:06 ` Andy Shevchenko
@ 2023-03-19 16:20 ` Jonathan Cameron
2023-03-20 7:17 ` Matti Vaittinen
2023-03-20 9:35 ` Matti Vaittinen
2023-03-21 1:05 ` kernel test robot
5 siblings, 1 reply; 30+ messages in thread
From: Jonathan Cameron @ 2023-03-19 16:20 UTC (permalink / raw)
To: Mehdi Djait
Cc: mazziesaccount, krzysztof.kozlowski+dt, andriy.shevchenko,
linux-iio, linux-kernel, devicetree
On Fri, 17 Mar 2023 00:48:36 +0100
Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> Refactor the kx022a driver implementation to make it more
> generic and extensible.
> Add the chip_info structure will to the driver's private
> data to hold all the device specific infos.
> Move the enum, struct and constants definitions to the header
> file.
You also introduce an i2c_device_id table
Without that I think module autoloading will be broken anyway so that's
definitely a good thing to add.
A few comments inline. Mostly around reducing the name changes.
Wild cards (or simply shorted 'generic' prefixes like KX_)
almost always bite back in the long run. Hence we generally just try
to name things after the first device that they were relevant to.
Thanks,
Jonathan
>
> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
> ---
> drivers/iio/accel/kionix-kx022a-i2c.c | 19 +-
> drivers/iio/accel/kionix-kx022a-spi.c | 22 +-
> drivers/iio/accel/kionix-kx022a.c | 289 ++++++++++++--------------
> drivers/iio/accel/kionix-kx022a.h | 128 ++++++++++--
> 4 files changed, 274 insertions(+), 184 deletions(-)
>
> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> index e6fd02d931b6..21c4c0ae1a68 100644
> --- a/drivers/iio/accel/kionix-kx022a-i2c.c
> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> @@ -15,23 +15,35 @@
> static int kx022a_i2c_probe(struct i2c_client *i2c)
> {
> struct device *dev = &i2c->dev;
> + struct kx022a_chip_info *chip_info;
> struct regmap *regmap;
> + const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
>
> if (!i2c->irq) {
> dev_err(dev, "No IRQ configured\n");
> return -EINVAL;
> }
>
> - regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
> + chip_info = device_get_match_data(&i2c->dev);
> + if (!chip_info)
> + chip_info = (const struct kx022a_chip_info *) id->driver_data;
Get id only if the device_get_match_data() fails.
if (!chip_info) {
const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
chip_info = (const struct kx...)
}
> +
> + regmap = devm_regmap_init_i2c(i2c, chip_info->regmap_config);
> if (IS_ERR(regmap))
> return dev_err_probe(dev, PTR_ERR(regmap),
> "Failed to initialize Regmap\n");
>
> - return kx022a_probe_internal(dev);
> + return kx022a_probe_internal(dev, chip_info);
> }
>
> +static const struct i2c_device_id kx022a_i2c_id[] = {
> + { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
If there are a small set and we aren't ever going to index the chip_info structure
we might be better off not bothering with the enum and instead using a separate
instance of the structure for each chip.
.driver_data = (kernel_ulong_t)&kx022a_chip_info,
etc.
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
> +
> static const struct of_device_id kx022a_of_match[] = {
> - { .compatible = "kionix,kx022a", },
> + { .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] },
> { }
> };
> MODULE_DEVICE_TABLE(of, kx022a_of_match);
> @@ -42,6 +54,7 @@ static struct i2c_driver kx022a_i2c_driver = {
> .of_match_table = kx022a_of_match,
> },
> .probe_new = kx022a_i2c_probe,
> + .id_table = kx022a_i2c_id,
> };
> module_i2c_driver(kx022a_i2c_driver);
>
> diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c
> index 9cd047f7b346..ec076af0f261 100644
> --- a/drivers/iio/accel/kionix-kx022a-spi.c
> +++ b/drivers/iio/accel/kionix-kx022a-spi.c
> @@ -15,40 +15,46 @@
> static int kx022a_spi_probe(struct spi_device *spi)
> {
> struct device *dev = &spi->dev;
> + struct kx022a_chip_info *chip_info;
> struct regmap *regmap;
> + const struct spi_device_id *id = spi_get_device_id(spi);
>
> if (!spi->irq) {
> dev_err(dev, "No IRQ configured\n");
> return -EINVAL;
> }
>
> - regmap = devm_regmap_init_spi(spi, &kx022a_regmap);
> + chip_info = device_get_match_data(&spi->dev);
> + if (!chip_info)
As above. Get the id only if we are going to use it.
> + chip_info = (const struct kx022a_chip_info *) id->driver_data;
> +
> + regmap = devm_regmap_init_spi(spi, chip_info->regmap_config);
> if (IS_ERR(regmap))
> return dev_err_probe(dev, PTR_ERR(regmap),
> "Failed to initialize Regmap\n");
>
> - return kx022a_probe_internal(dev);
> + return kx022a_probe_internal(dev, chip_info);
> }
>
> -static const struct spi_device_id kx022a_id[] = {
> - { "kx022a" },
> +static const struct spi_device_id kx022a_spi_id[] = {
> + { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
> { }
> };
> -MODULE_DEVICE_TABLE(spi, kx022a_id);
> +MODULE_DEVICE_TABLE(spi, kx022a_spi_id);
>
> static const struct of_device_id kx022a_of_match[] = {
> - { .compatible = "kionix,kx022a", },
> + { .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] },
> { }
> };
> MODULE_DEVICE_TABLE(of, kx022a_of_match);
>
> static struct spi_driver kx022a_spi_driver = {
> .driver = {
> - .name = "kx022a-spi",
> + .name = "kx022a-spi",
> .of_match_table = kx022a_of_match,
> },
> .probe = kx022a_spi_probe,
> - .id_table = kx022a_id,
> + .id_table = kx022a_spi_id,
> };
> module_spi_driver(kx022a_spi_driver);
>
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index 8dac0337c249..27e8642aa8f5 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -26,29 +26,7 @@
..
> -#define KX022A_ACCEL_CHAN(axis, index) \
> +#define KX022A_ACCEL_CHAN(axis, index, device) \
> { \
> .type = IIO_ACCEL, \
> .modified = 1, \
> @@ -220,7 +158,7 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
> BIT(IIO_CHAN_INFO_SCALE) | \
> BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> .ext_info = kx022a_ext_info, \
> - .address = KX022A_REG_##axis##OUT_L, \
> + .address = device##_REG_##axis##OUT_L, \
I'm not particularly keen on this because it is hard to search for.
It wasn't great before, but it's getting worse.
Perhaps just put the fully defined address in as a parameter to the macro.
> .scan_index = index, \
> .scan_type = { \
> .sign = 's', \
> @@ -231,9 +169,9 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
> }
>
> static const struct iio_chan_spec kx022a_channels[] = {
> - KX022A_ACCEL_CHAN(X, 0),
> - KX022A_ACCEL_CHAN(Y, 1),
> - KX022A_ACCEL_CHAN(Z, 2),
> + KX022A_ACCEL_CHAN(X, 0, KX022A),
> + KX022A_ACCEL_CHAN(Y, 1, KX022A),
> + KX022A_ACCEL_CHAN(Z, 2, KX022A),
> IIO_CHAN_SOFT_TIMESTAMP(3),
> };
>
> @@ -286,6 +224,34 @@ static const int kx022a_scale_table[][2] = {
> { 4788, 403320 },
> };
>
> +const struct kx022a_chip_info kx_chip_info[] = {
> + [KX022A] = {
> + .name = "kx022a",
> + .type = KX022A,
> + .regmap_config = &kx022a_regmap_config,
> + .channels = kx022a_channels,
> + .num_channels = ARRAY_SIZE(kx022a_channels),
> + .fifo_length = KX022A_FIFO_LENGTH,
> + .who = KX022A_REG_WHO,
> + .id = KX022A_ID,
> + .cntl = KX022A_REG_CNTL,
> + .cntl2 = KX022A_REG_CNTL2,
> + .odcntl = KX022A_REG_ODCNTL,
> + .buf_cntl1 = KX022A_REG_BUF_CNTL1,
> + .buf_cntl2 = KX022A_REG_BUF_CNTL2,
> + .buf_clear = KX022A_REG_BUF_CLEAR,
> + .buf_status1 = KX022A_REG_BUF_STATUS_1,
> + .buf_smp_lvl_mask = KX022A_MASK_BUF_SMP_LVL,
> + .buf_read = KX022A_REG_BUF_READ,
> + .inc1 = KX022A_REG_INC1,
> + .inc4 = KX022A_REG_INC4,
> + .inc5 = KX022A_REG_INC5,
> + .inc6 = KX022A_REG_INC6,
> + .xout_l = KX022A_REG_XOUT_L,
> + },
> +};
> +EXPORT_SYMBOL_NS_GPL(kx_chip_info, IIO_KX022A);
...
>
> +static int kx022a_get_fifo_bytes(struct kx022a_data *data)
Factoring this out looks like an unrelated change. Fine to do it but should be a
separate patch.
If you need a device type specific version of this add a function pointer
to your chip_info structure. Given you don't add one for the next patch
I think this is just refactoring and so fine to do, but needs to be in a separate
patch from this one with a description that says why you are doing it.
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + __le16 buf_status;
> + int ret, fifo_bytes;
> +
> + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
> + if (ret) {
> + dev_err(dev, "Error reading buffer status\n");
> + return ret;
> + }
> +
> + buf_status &= data->chip_info->buf_smp_lvl_mask;
> + fifo_bytes = le16_to_cpu(buf_status);
> +
> + /*
> + * The KX022A has FIFO which can store 43 samples of HiRes data from 2
> + * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
> + * 258 bytes of sample data. The quirk to know is that the amount of bytes in
> + * the FIFO is advertised via 8 bit register (max value 255). The thing to note
> + * is that full 258 bytes of data is indicated using the max value 255.
> + */
> + if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
> + fifo_bytes = KX022A_FIFO_MAX_BYTES;
> +
> + if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)
> + dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
> +
> + return fifo_bytes;
> +}
> +
> static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> {
> /*
> @@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> */
> data->timestamp = 0;
>
> - return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
> + return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
> }
>
> static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> bool irq)
> {
> struct kx022a_data *data = iio_priv(idev);
> - struct device *dev = regmap_get_device(data->regmap);
> - __le16 buffer[KX022A_FIFO_LENGTH * 3];
> + __le16 buffer[data->chip_info->fifo_length * 3];
> uint64_t sample_period;
> int count, fifo_bytes;
> bool renable = false;
> int64_t tstamp;
> int ret, i;
>
> - ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
> - if (ret) {
> - dev_err(dev, "Error reading buffer status\n");
> - return ret;
> - }
> -
> - /* Let's not overflow if we for some reason get bogus value from i2c */
> - if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
> - fifo_bytes = KX022A_FIFO_MAX_BYTES;
> -
> - if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES)
> - dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
> -
> - count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES;
> + fifo_bytes = kx022a_get_fifo_bytes(data);
> + count = fifo_bytes / KX_FIFO_SAMPLES_SIZE_BYTES;
> if (!count)
> return 0;
...
>
> @@ -969,25 +949,25 @@ static int kx022a_chip_init(struct kx022a_data *data)
> */
> msleep(1);
>
> - ret = regmap_read_poll_timeout(data->regmap, KX022A_REG_CNTL2, val,
> - !(val & KX022A_MASK_SRST),
> - KX022A_SOFT_RESET_WAIT_TIME_US,
> - KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US);
> + ret = regmap_read_poll_timeout(data->regmap, data->chip_info->cntl2, val,
> + !(val & KX_MASK_SRST),
> + KX_SOFT_RESET_WAIT_TIME_US,
> + KX_SOFT_RESET_TOTAL_WAIT_TIME_US);
Where ever there are lots of accesses to data->chip_info I'd add
a local chip_info variable to improve readabilty a bit. Might be
worth doing the same with regmap (in a different patch)
> if (ret) {
> dev_err(data->dev, "Sensor reset %s\n",
> - val & KX022A_MASK_SRST ? "timeout" : "fail#");
> + val & KX_MASK_SRST ? "timeout" : "fail#");
> return ret;
> }
>
> - ret = regmap_reinit_cache(data->regmap, &kx022a_regmap);
> + ret = regmap_reinit_cache(data->regmap, data->chip_info->regmap_config);
> if (ret) {
> dev_err(data->dev, "Failed to reinit reg cache\n");
> return ret;
> }
>
> /* set data res 16bit */
> - ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
> - KX022A_MASK_BRES16);
> + ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
> + KX_MASK_BRES16);
> if (ret) {
> dev_err(data->dev, "Failed to set data resolution\n");
> return ret;
> @@ -996,7 +976,7 @@ static int kx022a_chip_init(struct kx022a_data *data)
> return kx022a_prepare_irq_pin(data);
> }
>
> -int kx022a_probe_internal(struct device *dev)
> +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
> {
> static const char * const regulator_names[] = {"io-vdd", "vdd"};
> struct iio_trigger *indio_trig;
> @@ -1023,6 +1003,7 @@ int kx022a_probe_internal(struct device *dev)
> return -ENOMEM;
>
> data = iio_priv(idev);
> + data->chip_info = chip_info;
>
> /*
> * VDD is the analog and digital domain voltage supply and
> @@ -1033,37 +1014,37 @@ int kx022a_probe_internal(struct device *dev)
> if (ret && ret != -ENODEV)
> return dev_err_probe(dev, ret, "failed to enable regulator\n");
>
> - ret = regmap_read(regmap, KX022A_REG_WHO, &chip_id);
> + ret = regmap_read(regmap, data->chip_info->who, &chip_id);
I think you have chip_info available as a local variable.
Use that in this function to shorten lines with no loss of readability.
> if (ret)
> return dev_err_probe(dev, ret, "Failed to access sensor\n");
>
> - if (chip_id != KX022A_ID) {
> + if (chip_id != data->chip_info->id) {
Recently we've tried to avoid introduce error returns on a failure to match
and instead just warn and go ahead with assumption that we have a correct
dt-binding telling us that some new device with a different ID is backwards
compatible with the old one. Obviously not part of this patch though but
something to think about later (if you don't do it later in this series).
> dev_err(dev, "unsupported device 0x%x\n", chip_id);
> return -EINVAL;
> }
...
>
> data->regmap = regmap;
> data->dev = dev;
> data->irq = irq;
> - data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
> + data->odr_ns = KX_DEFAULT_PERIOD_NS;
> mutex_init(&data->mutex);
>
> - idev->channels = kx022a_channels;
> - idev->num_channels = ARRAY_SIZE(kx022a_channels);
> - idev->name = "kx022-accel";
Ah. Missed this naming in original driver review. We only normally
postfix with accel in devices that have multiple sensors with separate
drivers. Don't think that is true of the kx022a.
It's ABI so we are stuck with it, but avoid repeating that issue
for new devices.
> + idev->channels = data->chip_info->channels;
> + idev->num_channels = data->chip_info->num_channels;
> + idev->name = data->chip_info->name;
> idev->info = &kx022a_info;
> idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> idev->available_scan_masks = kx022a_scan_masks;
> @@ -1112,7 +1093,7 @@ int kx022a_probe_internal(struct device *dev)
> * No need to check for NULL. request_threaded_irq() defaults to
> * dev_name() should the alloc fail.
> */
> - name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a",
> + name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-accel",
> dev_name(data->dev));
Why name change here? It's not particularly important but if we can avoid
it that would be a nice to have.
>
> ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
> diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
> index 12424649d438..3bb40e9f5613 100644
> --- a/drivers/iio/accel/kionix-kx022a.h
> +++ b/drivers/iio/accel/kionix-kx022a.h
> @@ -11,24 +11,48 @@
> #include <linux/bits.h>
> #include <linux/regmap.h>
>
> +#include <linux/iio/iio.h>
> +
> +/* Common for all supported devices */
> +#define KX_FIFO_SAMPLES_SIZE_BYTES 6
Even if they are used across multiple parts, don't rename them to
be generic. It almost always causes churn / name clashes etc.
It is absolutely fine to have driver specific naming that is based
on the first supported part rather than trying to make it cover them
all.
> +#define KX_SOFT_RESET_WAIT_TIME_US (5 * USEC_PER_MSEC)
> +#define KX_SOFT_RESET_TOTAL_WAIT_TIME_US (500 * USEC_PER_MSEC)
> +#define KX_DEFAULT_PERIOD_NS (20 * NSEC_PER_MSEC)
> +#define KX_MASK_GSEL GENMASK(4, 3)
> +#define KX_MASK_ODR GENMASK(3, 0)
> +#define KX_MASK_WM_TH GENMASK(6, 0)
> +#define KX_GSEL_SHIFT 3
> +#define KX_MASK_IEN BIT(5)
> +#define KX_MASK_DRDY BIT(5)
> +#define KX_MASK_PC1 BIT(7)
> +#define KX_MASK_IPOL BIT(4)
> +#define KX_IPOL_LOW 0
> +#define KX_MASK_ITYP BIT(3)
> +#define KX_ITYP_LEVEL 0
> +#define KX_MASK_INS2_DRDY BIT(4)
> +#define KX_MASK_WMI BIT(5)
> +#define KX_MASK_BUF_EN BIT(7)
> +#define KX_MASK_SRST BIT(7)
> +#define KX_MASK_BRES16 BIT(6)
> +
> +
> #define KX022A_REG_WHO 0x0f
> #define KX022A_ID 0xc8
>
> +#define KX022A_FIFO_LENGTH 43
> +#define KX022A_FIFO_FULL_VALUE 255
> +#define KX022A_FIFO_MAX_BYTES \
> + (KX022A_FIFO_LENGTH * KX_FIFO_SAMPLES_SIZE_BYTES)
> +
> #define KX022A_REG_CNTL2 0x19
> -#define KX022A_MASK_SRST BIT(7)
> #define KX022A_REG_CNTL 0x18
> -#define KX022A_MASK_PC1 BIT(7)
> #define KX022A_MASK_RES BIT(6)
> -#define KX022A_MASK_DRDY BIT(5)
> -#define KX022A_MASK_GSEL GENMASK(4, 3)
> -#define KX022A_GSEL_SHIFT 3
> #define KX022A_GSEL_2 0x0
> #define KX022A_GSEL_4 BIT(3)
> #define KX022A_GSEL_8 BIT(4)
> #define KX022A_GSEL_16 GENMASK(4, 3)
>
> #define KX022A_REG_INS2 0x13
> -#define KX022A_MASK_INS2_DRDY BIT(4)
> #define KX122_MASK_INS2_WMI BIT(5)
>
> #define KX022A_REG_XHP_L 0x0
> @@ -45,38 +69,104 @@
> #define KX022A_REG_MAN_WAKE 0x2c
>
> #define KX022A_REG_BUF_CNTL1 0x3a
> -#define KX022A_MASK_WM_TH GENMASK(6, 0)
> #define KX022A_REG_BUF_CNTL2 0x3b
> -#define KX022A_MASK_BUF_EN BIT(7)
> -#define KX022A_MASK_BRES16 BIT(6)
> #define KX022A_REG_BUF_STATUS_1 0x3c
> #define KX022A_REG_BUF_STATUS_2 0x3d
> +#define KX022A_MASK_BUF_SMP_LVL GENMASK(6, 0)
> #define KX022A_REG_BUF_CLEAR 0x3e
> #define KX022A_REG_BUF_READ 0x3f
> -#define KX022A_MASK_ODR GENMASK(3, 0)
> #define KX022A_ODR_SHIFT 3
> #define KX022A_FIFO_MAX_WMI_TH 41
>
> #define KX022A_REG_INC1 0x1c
> #define KX022A_REG_INC5 0x20
> #define KX022A_REG_INC6 0x21
> -#define KX022A_MASK_IEN BIT(5)
> -#define KX022A_MASK_IPOL BIT(4)
> #define KX022A_IPOL_LOW 0
> -#define KX022A_IPOL_HIGH KX022A_MASK_IPOL1
> -#define KX022A_MASK_ITYP BIT(3)
> -#define KX022A_ITYP_PULSE KX022A_MASK_ITYP
> -#define KX022A_ITYP_LEVEL 0
> +#define KX022A_IPOL_HIGH KX_MASK_IPOL
> +#define KX022A_ITYP_PULSE KX_MASK_ITYP
>
> #define KX022A_REG_INC4 0x1f
> -#define KX022A_MASK_WMI BIT(5)
>
> #define KX022A_REG_SELF_TEST 0x60
> #define KX022A_MAX_REGISTER 0x60
>
> +enum kx022a_device_type {
> + KX022A,
> +};
As below. I'd avoid using the enum unless needed.
That can make sense where a driver supports lots of devices but I don't think
it does here.
> +
> +enum {
> + KX_STATE_SAMPLE,
> + KX_STATE_FIFO,
> +};
> +
> +enum {
> + AXIS_X,
> + AXIS_Y,
> + AXIS_Z,
> + AXIS_MAX
> +};
> +
> struct device;
>
> -int kx022a_probe_internal(struct device *dev);
> -extern const struct regmap_config kx022a_regmap;
> +struct kx022a_chip_info {
> + const char *name;
> + enum kx022a_device_type type;
> + const struct regmap_config *regmap_config;
> + const struct iio_chan_spec *channels;
> + unsigned int num_channels;
> + unsigned int fifo_length;
> + u8 who;
Some of these are no immediately obvious so either rename the
field so it is more obvious what it is, or add comments.
> + u8 id;
> + u8 cntl;
> + u8 cntl2;
> + u8 odcntl;
> + u8 buf_cntl1;
> + u8 buf_cntl2;
> + u8 buf_clear;
> + u8 buf_status1;
> + u16 buf_smp_lvl_mask;
> + u8 buf_read;
> + u8 inc1;
> + u8 inc4;
> + u8 inc5;
> + u8 inc6;
> + u8 xout_l;
> +};
> +
> +struct kx022a_data {
Why move this to the header? Unless there is a strong reason
I'd prefer this to stay down in the .c file.
> + const struct kx022a_chip_info *chip_info;
> + struct regmap *regmap;
> + struct iio_trigger *trig;
> + struct device *dev;
> + struct iio_mount_matrix orientation;
> + int64_t timestamp, old_timestamp;
> +
> + int irq;
> + int inc_reg;
> + int ien_reg;
> +
> + unsigned int state;
> + unsigned int odr_ns;
> +
> + bool trigger_enabled;
> + /*
> + * Prevent toggling the sensor stby/active state (PC1 bit) in the
> + * middle of a configuration, or when the fifo is enabled. Also,
> + * protect the data stored/retrieved from this structure from
> + * concurrent accesses.
> + */
> + struct mutex mutex;
> + u8 watermark;
> +
> + /* 3 x 16bit accel data + timestamp */
> + __le16 buffer[8] __aligned(IIO_DMA_MINALIGN);
> + struct {
> + __le16 channels[3];
> + s64 ts __aligned(8);
> + } scan;
> +};
> +
> +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);
> +extern const struct kx022a_chip_info kx_chip_info[];
As mentioned in the bus specific driver, given there is a small set and we need the enum
just to index this, I'd just have one per supported device.
extern const struct kx022a_chip_info kx022a_chip_info;
extern const struct kx022a_chip_info kx321_chip_info;
etc
>
> #endif
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer
2023-03-16 23:48 ` [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
@ 2023-03-19 16:22 ` Jonathan Cameron
2023-03-21 16:34 ` Mehdi Djait
2023-03-20 9:57 ` Matti Vaittinen
1 sibling, 1 reply; 30+ messages in thread
From: Jonathan Cameron @ 2023-03-19 16:22 UTC (permalink / raw)
To: Mehdi Djait
Cc: mazziesaccount, krzysztof.kozlowski+dt, andriy.shevchenko,
linux-iio, linux-kernel, devicetree
On Fri, 17 Mar 2023 00:48:37 +0100
Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> Add support for the basic accelerometer features such as getting the
> acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
> using the WMI IRQ).
>
> Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
Nothing much specific to this patch, most changes will be as a result
of bringing this inline with the changes suggested for patch 2.
thanks,
Jonathan
> ---
> drivers/iio/accel/kionix-kx022a-i2c.c | 2 +
> drivers/iio/accel/kionix-kx022a-spi.c | 2 +
> drivers/iio/accel/kionix-kx022a.c | 126 ++++++++++++++++++++++++++
> drivers/iio/accel/kionix-kx022a.h | 53 +++++++++++
> 4 files changed, 183 insertions(+)
>
> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> index 21c4c0ae1a68..f9b2383c43f1 100644
> --- a/drivers/iio/accel/kionix-kx022a-i2c.c
> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> @@ -38,12 +38,14 @@ static int kx022a_i2c_probe(struct i2c_client *i2c)
>
> static const struct i2c_device_id kx022a_i2c_id[] = {
> { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
> + { .name = "kx132", .driver_data = (kernel_ulong_t)&kx_chip_info[KX132] },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
>
> static const struct of_device_id kx022a_of_match[] = {
> { .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] },
> + { .compatible = "kionix,kx132", .data = &kx_chip_info[KX132] },
> { }
> };
> MODULE_DEVICE_TABLE(of, kx022a_of_match);
> diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c
> index ec076af0f261..86a10d6d33ff 100644
> --- a/drivers/iio/accel/kionix-kx022a-spi.c
> +++ b/drivers/iio/accel/kionix-kx022a-spi.c
> @@ -38,12 +38,14 @@ static int kx022a_spi_probe(struct spi_device *spi)
>
> static const struct spi_device_id kx022a_spi_id[] = {
> { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
> + { .name = "kx132", .driver_data = (kernel_ulong_t)&kx_chip_info[KX132] },
> { }
> };
> MODULE_DEVICE_TABLE(spi, kx022a_spi_id);
>
> static const struct of_device_id kx022a_of_match[] = {
> { .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] },
> + { .compatible = "kionix,kx132", .data = &kx_chip_info[KX132] },
> { }
> };
> MODULE_DEVICE_TABLE(of, kx022a_of_match);
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index 27e8642aa8f5..3cacec99f792 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -128,6 +128,101 @@ static const struct regmap_config kx022a_regmap_config = {
> .cache_type = REGCACHE_RBTREE,
> };
>
> +/* Regmap configs kx132 */
> +static const struct regmap_range kx132_volatile_ranges[] = {
> + {
> + .range_min = KX132_REG_XADP_L,
> + .range_max = KX132_REG_COTR,
> + }, {
> + .range_min = KX132_REG_TSCP,
> + .range_max = KX132_REG_INT_REL,
> + }, {
> + /* The reset bit will be cleared by sensor */
> + .range_min = KX132_REG_CNTL2,
> + .range_max = KX132_REG_CNTL2,
> + }, {
> + .range_min = KX132_REG_BUF_STATUS_1,
> + .range_max = KX132_REG_BUF_READ,
> + },
> +};
> +
> +static const struct regmap_access_table kx132_volatile_regs = {
> + .yes_ranges = &kx132_volatile_ranges[0],
> + .n_yes_ranges = ARRAY_SIZE(kx132_volatile_ranges),
> +};
> +
> +static const struct regmap_range kx132_precious_ranges[] = {
> + {
> + .range_min = KX132_REG_INT_REL,
> + .range_max = KX132_REG_INT_REL,
> + },
> +};
> +
> +static const struct regmap_access_table kx132_precious_regs = {
> + .yes_ranges = &kx132_precious_ranges[0],
> + .n_yes_ranges = ARRAY_SIZE(kx132_precious_ranges),
> +};
> +
> +static const struct regmap_range kx132_read_only_ranges[] = {
> + {
> + .range_min = KX132_REG_XADP_L,
> + .range_max = KX132_REG_INT_REL,
> + }, {
> + .range_min = KX132_REG_BUF_STATUS_1,
> + .range_max = KX132_REG_BUF_STATUS_2,
> + }, {
> + .range_min = KX132_REG_BUF_READ,
> + .range_max = KX132_REG_BUF_READ,
> + },
> +};
> +
> +static const struct regmap_access_table kx132_ro_regs = {
> + .no_ranges = &kx132_read_only_ranges[0],
> + .n_no_ranges = ARRAY_SIZE(kx132_read_only_ranges),
> +};
> +
> +static const struct regmap_range kx132_write_only_ranges[] = {
> + {
> + .range_min = KX132_REG_MAN_WAKE,
> + .range_max = KX132_REG_MAN_WAKE,
> + }, {
> + .range_min = KX132_REG_SELF_TEST,
> + .range_max = KX132_REG_SELF_TEST,
> + }, {
> + .range_min = KX132_REG_BUF_CLEAR,
> + .range_max = KX132_REG_BUF_CLEAR,
> + },
> +};
> +
> +static const struct regmap_access_table kx132_wo_regs = {
> + .no_ranges = &kx132_write_only_ranges[0],
> + .n_no_ranges = ARRAY_SIZE(kx132_write_only_ranges),
> +};
> +
> +static const struct regmap_range kx132_noinc_read_ranges[] = {
> + {
> + .range_min = KX132_REG_BUF_READ,
> + .range_max = KX132_REG_BUF_READ,
> + },
> +};
> +
> +static const struct regmap_access_table kx132_nir_regs = {
> + .yes_ranges = &kx132_noinc_read_ranges[0],
> + .n_yes_ranges = ARRAY_SIZE(kx132_noinc_read_ranges),
> +};
> +
> +static const struct regmap_config kx132_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .volatile_table = &kx132_volatile_regs,
> + .rd_table = &kx132_wo_regs,
> + .wr_table = &kx132_ro_regs,
> + .rd_noinc_table = &kx132_nir_regs,
> + .precious_table = &kx132_precious_regs,
> + .max_register = KX132_MAX_REGISTER,
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> static const struct iio_mount_matrix *
> kx022a_get_mount_matrix(const struct iio_dev *idev,
> const struct iio_chan_spec *chan)
> @@ -175,6 +270,13 @@ static const struct iio_chan_spec kx022a_channels[] = {
> IIO_CHAN_SOFT_TIMESTAMP(3),
> };
>
> +static const struct iio_chan_spec kx132_channels[] = {
> + KX022A_ACCEL_CHAN(X, 0, KX132),
> + KX022A_ACCEL_CHAN(Y, 1, KX132),
> + KX022A_ACCEL_CHAN(Z, 2, KX132),
> + IIO_CHAN_SOFT_TIMESTAMP(3),
> +};
> +
> /*
> * The sensor HW can support ODR up to 1600 Hz, which is beyond what most of the
> * Linux CPUs can handle without dropping samples. Also, the low power mode is
> @@ -249,6 +351,30 @@ const struct kx022a_chip_info kx_chip_info[] = {
> .inc6 = KX022A_REG_INC6,
> .xout_l = KX022A_REG_XOUT_L,
> },
> + [KX132] = {
> + .name = "kx132",
> + .type = KX132,
> + .regmap_config = &kx132_regmap_config,
> + .channels = kx132_channels,
> + .num_channels = ARRAY_SIZE(kx132_channels),
> + .fifo_length = KX132_FIFO_LENGTH,
> + .who = KX132_REG_WHO,
> + .id = KX132_ID,
> + .cntl = KX132_REG_CNTL,
> + .cntl2 = KX132_REG_CNTL2,
> + .odcntl = KX132_REG_ODCNTL,
> + .buf_cntl1 = KX132_REG_BUF_CNTL1,
> + .buf_cntl2 = KX132_REG_BUF_CNTL2,
> + .buf_clear = KX132_REG_BUF_CLEAR,
> + .buf_status1 = KX132_REG_BUF_STATUS_1,
> + .buf_smp_lvl_mask = KX132_MASK_BUF_SMP_LVL,
> + .buf_read = KX132_REG_BUF_READ,
> + .inc1 = KX132_REG_INC1,
> + .inc4 = KX132_REG_INC4,
> + .inc5 = KX132_REG_INC5,
> + .inc6 = KX132_REG_INC6,
> + .xout_l = KX132_REG_XOUT_L,
> + },
> };
> EXPORT_SYMBOL_NS_GPL(kx_chip_info, IIO_KX022A);
>
> diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
> index 3bb40e9f5613..7e43bdb37156 100644
> --- a/drivers/iio/accel/kionix-kx022a.h
> +++ b/drivers/iio/accel/kionix-kx022a.h
> @@ -90,8 +90,61 @@
> #define KX022A_REG_SELF_TEST 0x60
> #define KX022A_MAX_REGISTER 0x60
>
> +
Push these down into the c file.
> +#define KX132_REG_WHO 0x13
> +#define KX132_ID 0x3d
> +
> +#define KX132_FIFO_LENGTH 86
> +
> +#define KX132_REG_CNTL2 0x1c
> +#define KX132_REG_CNTL 0x1b
> +#define KX132_MASK_RES BIT(6)
> +#define KX132_GSEL_2 0x0
> +#define KX132_GSEL_4 BIT(3)
> +#define KX132_GSEL_8 BIT(4)
> +#define KX132_GSEL_16 GENMASK(4, 3)
> +
> +#define KX132_REG_INS2 0x17
> +#define KX132_MASK_INS2_WMI BIT(5)
> +
> +#define KX132_REG_XADP_L 0x02
> +#define KX132_REG_XOUT_L 0x08
> +#define KX132_REG_YOUT_L 0x0a
> +#define KX132_REG_ZOUT_L 0x0c
> +#define KX132_REG_COTR 0x12
> +#define KX132_REG_TSCP 0x14
> +#define KX132_REG_INT_REL 0x1a
> +
> +#define KX132_REG_ODCNTL 0x21
> +
> +#define KX132_REG_BTS_WUF_TH 0x4a
> +#define KX132_REG_MAN_WAKE 0x4d
> +
> +#define KX132_REG_BUF_CNTL1 0x5e
> +#define KX132_REG_BUF_CNTL2 0x5f
> +#define KX132_REG_BUF_STATUS_1 0x60
> +#define KX132_REG_BUF_STATUS_2 0x61
> +#define KX132_MASK_BUF_SMP_LVL GENMASK(9, 0)
> +#define KX132_REG_BUF_CLEAR 0x62
> +#define KX132_REG_BUF_READ 0x63
> +#define KX132_ODR_SHIFT 3
> +#define KX132_FIFO_MAX_WMI_TH 86
> +
> +#define KX132_REG_INC1 0x22
> +#define KX132_REG_INC5 0x26
> +#define KX132_REG_INC6 0x27
> +#define KX132_IPOL_LOW 0
> +#define KX132_IPOL_HIGH KX_MASK_IPOL
> +#define KX132_ITYP_PULSE KX_MASK_ITYP
> +
> +#define KX132_REG_INC4 0x25
> +
> +#define KX132_REG_SELF_TEST 0x5d
> +#define KX132_MAX_REGISTER 0x76
> +
> enum kx022a_device_type {
> KX022A,
> + KX132,
As mentioned in previous review, I think this would be neater
done by just exporting the chip_info structures directly rather than
putting them in an array.
> };
>
> enum {
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-19 16:20 ` Jonathan Cameron
@ 2023-03-20 7:17 ` Matti Vaittinen
2023-03-20 12:24 ` Jonathan Cameron
0 siblings, 1 reply; 30+ messages in thread
From: Matti Vaittinen @ 2023-03-20 7:17 UTC (permalink / raw)
To: Jonathan Cameron, Mehdi Djait
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, linux-iio,
linux-kernel, devicetree
Hi Mehdi and Jonathan,
Just my take on couple of comments from Jonathan :) I still have my own
review to do though...
On 3/19/23 18:20, Jonathan Cameron wrote:
> On Fri, 17 Mar 2023 00:48:36 +0100
> Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
>
>> Refactor the kx022a driver implementation to make it more
>> generic and extensible.
>> Add the chip_info structure will to the driver's private
>> data to hold all the device specific infos.
>> Move the enum, struct and constants definitions to the header
>> file.
>
> You also introduce an i2c_device_id table
>
> Without that I think module autoloading will be broken anyway so that's
> definitely a good thing to add.
I am pretty sure the autoloading worked for OF-systems. But yes, adding
the i2c_device_id is probably a good idea. Thanks.
> A few comments inline. Mostly around reducing the name changes.
> Wild cards (or simply shorted 'generic' prefixes like KX_)
> almost always bite back in the long run. Hence we generally just try
> to name things after the first device that they were relevant to.
I must say I disagree on this with you Jonathan. I know wildcards tend
to get confusing - but I still like the idea of showing which of the
definitions are IC specific and which ones are generic or at least used
by more than one variant - especially as long as we only have two
supported ICs. I definitely like the macro naming added by Mehdi. This
approach has been very helpful for me for example in the BD718x7
(BD71837/BD71847/BD71850) PMIC driver. My take on this is:
1) I like the generic KX_define.
2) I would not try adding wildcards like KX_X22 - to denote support for
122 and 022 - while not supporting 132 - in my experience - that won't
scale.
3) I definitely like the idea of using exact model number prefix for
'stuff' which is intended to work only on one exact model.
Regarding the 3) - I am not so strict on how the register/mask defines
are handled - I _like_ the 1) 2) 3) approach above - but mask/register
defines tend to get set (correctly) once and not required to be looked
up after this. But. When the 'stuff' is functions - this gets very
useful as one is very often required to see which functions are executed
on which IC variant. Same goes to structs.
So, if we manage to convince Jonathan about the naming, then I like what
yoo had here! I would hovever do it in two steps. I would at first do
renaming patch where the generic defines were renamed - without any
functional changes - and only then add the kx132 stuff in a subsequent
patch. That would simplify seeing which changes are just renaming and
which are functional ones.
But here, I must go with the wind - if subsystem maintainer says the
code should not have naming like this - then I have no say over it... :/
>>
>> +static const struct i2c_device_id kx022a_i2c_id[] = {
>> + { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
> If there are a small set and we aren't ever going to index the chip_info structure
> we might be better off not bothering with the enum and instead using a separate
> instance of the structure for each chip.
>
I kind of like also the table added by Mehdi. I admit I was at first
just thinking that we should have a pointer to the struct here without
any tables - but... After I took a peek in the kionix-kx022a.c - I kind
of liked the table and not exporting the struct names. So, I don't have
a strong opinion on this.
I think it's worth noting that this driver could (maybe easily enough)
be extended to support also a few other kionix accelerometers. Maybe, if
we don't scare Mehdi away, we will see a few other variants supported as
well ;)
>> data->regmap = regmap;
>> data->dev = dev;
>> data->irq = irq;
>> - data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
>> + data->odr_ns = KX_DEFAULT_PERIOD_NS;
>> mutex_init(&data->mutex);
>>
>> - idev->channels = kx022a_channels;
>> - idev->num_channels = ARRAY_SIZE(kx022a_channels);
>> - idev->name = "kx022-accel";
>
> Ah. Missed this naming in original driver review. We only normally
> postfix with accel in devices that have multiple sensors with separate
> drivers. Don't think that is true of the kx022a.
Ouch. I am not 100% sure but may be you didn't miss it. It may be I just
missed fixing this because your comment here sounds somewhat familiar to
me! (Or then you commented on suffix in driver-name).
> It's ABI so we are stuck with it, but avoid repeating that issue
> for new devices. >
>>
>> +enum kx022a_device_type {
>> + KX022A,
>> +};
>
> As below. I'd avoid using the enum unless needed.
> That can make sense where a driver supports lots of devices but I don't think
> it does here.
Well, I know it is usually not too clever to be prepared for the future
stuff too well. But - I don't think the enum and table are adding much
of complexity? I am saying this as I think this driver could be extended
to support also kx022 (without the A), kx023, kx122. I've also seen some
references to model kx022A-120B (but I have no idea what's the story
there or if that IC is publicly available). Maybe Mehdi would like to
extend this driver further after the KX132 is done ;)
>> -int kx022a_probe_internal(struct device *dev);
>> -extern const struct regmap_config kx022a_regmap;
>> +struct kx022a_chip_info {
>> + const char *name;
>> + enum kx022a_device_type type;
>> + const struct regmap_config *regmap_config;
>> + const struct iio_chan_spec *channels;
>> + unsigned int num_channels;
>> + unsigned int fifo_length;
>> + u8 who;
> Some of these are no immediately obvious so either rename the
> field so it is more obvious what it is, or add comments.
I would vote for adding a comment :) I like the who. Both the band and
this member here :) Data-sheet has register named as "who_am_i" - so I
don't think this name is too obfuscating - and what matters to me - it
is short yet meaningful.
>> + u8 id;
>> + u8 cntl;
>> + u8 cntl2;
>> + u8 odcntl;
>> + u8 buf_cntl1;
>> + u8 buf_cntl2;
>> + u8 buf_clear;
>> + u8 buf_status1;
>> + u16 buf_smp_lvl_mask;
>> + u8 buf_read;
>> + u8 inc1;
>> + u8 inc4;
>> + u8 inc5;
>> + u8 inc6;
>> + u8 xout_l;
>> +};
>> +
>> +struct kx022a_data {
>
> Why move this to the header? Unless there is a strong reason
> I'd prefer this to stay down in the .c file.
So would I. It's definitely nice to be able to see the struct in the
same file where the code referencing it is.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-16 23:48 ` [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure Mehdi Djait
` (3 preceding siblings ...)
2023-03-19 16:20 ` Jonathan Cameron
@ 2023-03-20 9:35 ` Matti Vaittinen
2023-03-20 12:02 ` Andy Shevchenko
` (2 more replies)
2023-03-21 1:05 ` kernel test robot
5 siblings, 3 replies; 30+ messages in thread
From: Matti Vaittinen @ 2023-03-20 9:35 UTC (permalink / raw)
To: Mehdi Djait, jic23
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, linux-iio,
linux-kernel, devicetree
On 3/17/23 01:48, Mehdi Djait wrote:
> Refactor the kx022a driver implementation to make it more
> generic and extensible.
> Add the chip_info structure will to the driver's private
> data to hold all the device specific infos.
> Move the enum, struct and constants definitions to the header
> file.
>
> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
> ---
> drivers/iio/accel/kionix-kx022a-i2c.c | 19 +-
> drivers/iio/accel/kionix-kx022a-spi.c | 22 +-
> drivers/iio/accel/kionix-kx022a.c | 289 ++++++++++++--------------
> drivers/iio/accel/kionix-kx022a.h | 128 ++++++++++--
> 4 files changed, 274 insertions(+), 184 deletions(-)
>
> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> index e6fd02d931b6..21c4c0ae1a68 100644
> --- a/drivers/iio/accel/kionix-kx022a-i2c.c
> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> @@ -15,23 +15,35 @@
> static int kx022a_i2c_probe(struct i2c_client *i2c)
> {
> struct device *dev = &i2c->dev;
> + struct kx022a_chip_info *chip_info;
> struct regmap *regmap;
> + const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
>
> if (!i2c->irq) {
> dev_err(dev, "No IRQ configured\n");
> return -EINVAL;
> }
>
> - regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
> + chip_info = device_get_match_data(&i2c->dev);
> + if (!chip_info)
> + chip_info = (const struct kx022a_chip_info *) id->driver_data;
> +
> + regmap = devm_regmap_init_i2c(i2c, chip_info->regmap_config);
> if (IS_ERR(regmap))
> return dev_err_probe(dev, PTR_ERR(regmap),
> "Failed to initialize Regmap\n");
Hm. I would like to pull the regmap_config out of the chip_info struct.
As far as I see, the regmap_config is only needed in these bus specific
files. On the other hand, the chip-info is only needed in the
kionix-kx022a.c file, right?
So, maybe you could here just get the regmap_config based on the chip-id
(enum value you added - the data pointer in match tables could be just
the enum value indicating the IC type). Then, you could pass this enum
value to kx022a_probe_internal() - and the chip-info struct could be
selected in the kionix-kx022a.c based on it. That way you would not need
the struct chip-info here or regmap_config in kionix-kx022a.c. Same in
the *-spi.c
Something like:
enum {
KIONIX_IC_KX022A,
KIONIX_IC_KX132_xxx, /* xxx denotes accurate model suffix */
};
static const struct of_device_id kx022a_of_match[] = {
{ .compatible = "kionix,kx022a", .data = KIONIX_IC_KX022A },
...
chip_id = device_get_match_data(&i2c->dev);
regmap_cfg = kx022a_kx_regmap_cfg[chip_id];
regmap = devm_regmap_init_i2c(i2c, regmap_cfg);
...
return kx022a_probe_internal(dev, chip_id);
Do you think that would work?
OTOH, to really benefit from this we should probably pull out the
regmap-configs from the kionix-kx022a.c. I am not really sure where we
should put it then though. Hence, if there is no good ideas how to split
the config and chip-info so they are only available/used where needed -
then I am also Ok with the current approach.
> -
> -struct kx022a_data {
> - struct regmap *regmap;
> - struct iio_trigger *trig;
> - struct device *dev;
> - struct iio_mount_matrix orientation;
> - int64_t timestamp, old_timestamp;
> -
> - int irq;
> - int inc_reg;
> - int ien_reg;
> -
> - unsigned int state;
> - unsigned int odr_ns;
> -
> - bool trigger_enabled;
> - /*
> - * Prevent toggling the sensor stby/active state (PC1 bit) in the
> - * middle of a configuration, or when the fifo is enabled. Also,
> - * protect the data stored/retrieved from this structure from
> - * concurrent accesses.
> - */
> - struct mutex mutex;
> - u8 watermark;
> -
> - /* 3 x 16bit accel data + timestamp */
> - __le16 buffer[8] __aligned(IIO_DMA_MINALIGN);
> - struct {
> - __le16 channels[3];
> - s64 ts __aligned(8);
> - } scan;
> -};
As mentioned by Jonathan - It'd be better to keep this struct in C-file.
>
> +const struct kx022a_chip_info kx_chip_info[] = {
> + [KX022A] = {
> + .name = "kx022a",
> + .type = KX022A,
> + .regmap_config = &kx022a_regmap_config,
As mentioned above, the regmap config is not really needed after the
regmap is initialized. Id prefer this not being part of the chip info.
> + .channels = kx022a_channels,
> + .num_channels = ARRAY_SIZE(kx022a_channels),
> + .fifo_length = KX022A_FIFO_LENGTH,
> + .who = KX022A_REG_WHO,
> + .id = KX022A_ID,
> + .cntl = KX022A_REG_CNTL,
> + .cntl2 = KX022A_REG_CNTL2,
> + .odcntl = KX022A_REG_ODCNTL,
> + .buf_cntl1 = KX022A_REG_BUF_CNTL1,
> + .buf_cntl2 = KX022A_REG_BUF_CNTL2,
> + .buf_clear = KX022A_REG_BUF_CLEAR,
> + .buf_status1 = KX022A_REG_BUF_STATUS_1,
> + .buf_smp_lvl_mask = KX022A_MASK_BUF_SMP_LVL,
> + .buf_read = KX022A_REG_BUF_READ,
> + .inc1 = KX022A_REG_INC1,
> + .inc4 = KX022A_REG_INC4,
> + .inc5 = KX022A_REG_INC5,
> + .inc6 = KX022A_REG_INC6,
> + .xout_l = KX022A_REG_XOUT_L,
> + },
> +};
> +EXPORT_SYMBOL_NS_GPL(kx_chip_info, IIO_KX022A);
> +
> static int kx022a_read_avail(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> const int **vals, int *type, int *length,
> @@ -309,19 +275,17 @@ static int kx022a_read_avail(struct iio_dev *indio_dev,
> }
> }
>
> -#define KX022A_DEFAULT_PERIOD_NS (20 * NSEC_PER_MSEC)
> -
> static void kx022a_reg2freq(unsigned int val, int *val1, int *val2)
> {
> - *val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][0];
> - *val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][1];
> + *val1 = kx022a_accel_samp_freq_table[val & KX_MASK_ODR][0];
> + *val2 = kx022a_accel_samp_freq_table[val & KX_MASK_ODR][1];
> }
>
As mentioned elsewhere, doing the renaming separately from the
functional changes will ease the reviewing.
>
> +static int kx022a_get_fifo_bytes(struct kx022a_data *data)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + __le16 buf_status;
> + int ret, fifo_bytes;
> +
> + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
> + if (ret) {
> + dev_err(dev, "Error reading buffer status\n");
> + return ret;
> + }
> +
> + buf_status &= data->chip_info->buf_smp_lvl_mask;
> + fifo_bytes = le16_to_cpu(buf_status);
> +
> + /*
> + * The KX022A has FIFO which can store 43 samples of HiRes data from 2
> + * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
> + * 258 bytes of sample data. The quirk to know is that the amount of bytes in
> + * the FIFO is advertised via 8 bit register (max value 255). The thing to note
> + * is that full 258 bytes of data is indicated using the max value 255.
> + */
> + if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
> + fifo_bytes = KX022A_FIFO_MAX_BYTES;
> +
> + if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)
> + dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
> +
> + return fifo_bytes;
> +}
I like adding this function. Here I agree with Jonathan - having a
device specific functions would clarify this a bit. The KX022A "quirk"
is a bit confusing. You could then get rid of the buf_smp_lvl_mask.
> +
> static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> {
> /*
> @@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> */
> data->timestamp = 0;
>
> - return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
> + return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
> }
>
> static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> bool irq)
> {
> struct kx022a_data *data = iio_priv(idev);
> - struct device *dev = regmap_get_device(data->regmap);
> - __le16 buffer[KX022A_FIFO_LENGTH * 3];
> + __le16 buffer[data->chip_info->fifo_length * 3];
I don't like this. Having the length of an array decided at run-time is
not something I appreciate. Maybe you could just always reserve the
memory so that the largest FIFO gets supported. I am just wondering how
large arrays we can safely allocate from the stack?
> @@ -812,14 +792,14 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
> goto unlock_out;
>
> /* Enable buffer */
> - ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
> - KX022A_MASK_BUF_EN);
> + ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
> + KX_MASK_BUF_EN);
> if (ret)
> goto unlock_out;
>
> - data->state |= KX022A_STATE_FIFO;
> + data->state |= KX_STATE_FIFO;
> ret = regmap_set_bits(data->regmap, data->ien_reg,
> - KX022A_MASK_WMI);
> + KX_MASK_WMI);
I think this fits to one line now. (even on my screen)
> if (ret)
> goto unlock_out;
>
> -int kx022a_probe_internal(struct device *dev)
> +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
As mentioned elsewhere, this might also work if the chip-type enum was
passed here as parameter. That way the bus specific part would not need
to know about the struct chip_info...
> {
> static const char * const regulator_names[] = {"io-vdd", "vdd"};
> struct iio_trigger *indio_trig;
> @@ -1023,6 +1003,7 @@ int kx022a_probe_internal(struct device *dev)
> return -ENOMEM;
>
> data = iio_priv(idev);
> + data->chip_info = chip_info;
...Here you could then pick the correct chip_info based on the chip-type
enum. In that case I'd like to get the regmap_config(s) in own file. Not
sure how that would look like though.
All in all, I like how this looks like. Nice job!
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer
2023-03-16 23:48 ` [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
2023-03-19 16:22 ` Jonathan Cameron
@ 2023-03-20 9:57 ` Matti Vaittinen
1 sibling, 0 replies; 30+ messages in thread
From: Matti Vaittinen @ 2023-03-20 9:57 UTC (permalink / raw)
To: Mehdi Djait, jic23
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, linux-iio,
linux-kernel, devicetree
On 3/17/23 01:48, Mehdi Djait wrote:
> Add support for the basic accelerometer features such as getting the
> acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
> using the WMI IRQ).
>
Hi Mehdi,
I have nothing to say to this patch yet. Let's see how it looks like
after the comments from Jonathan/Andy have been discussed/reworked :)
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-20 9:35 ` Matti Vaittinen
@ 2023-03-20 12:02 ` Andy Shevchenko
2023-03-20 12:34 ` Jonathan Cameron
2023-03-21 15:56 ` Mehdi Djait
2 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2023-03-20 12:02 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Mehdi Djait, jic23, krzysztof.kozlowski+dt, linux-iio,
linux-kernel, devicetree
On Mon, Mar 20, 2023 at 11:35:06AM +0200, Matti Vaittinen wrote:
> On 3/17/23 01:48, Mehdi Djait wrote:
> > Refactor the kx022a driver implementation to make it more
> > generic and extensible.
> > Add the chip_info structure will to the driver's private
> > data to hold all the device specific infos.
> > Move the enum, struct and constants definitions to the header
> > file.
...
> Something like:
>
> enum {
> KIONIX_IC_KX022A,
> KIONIX_IC_KX132_xxx, /* xxx denotes accurate model suffix */
> };
>
> static const struct of_device_id kx022a_of_match[] = {
> { .compatible = "kionix,kx022a", .data = KIONIX_IC_KX022A },
> ...
>
> chip_id = device_get_match_data(&i2c->dev);
No, please avoid putting plain integers as pointers of driver_data.
The problem you introduced with your suggestion is impossibility
to distinguish 0 and NULL, beyond other not good things (like missing
castings which are ugly).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-20 7:17 ` Matti Vaittinen
@ 2023-03-20 12:24 ` Jonathan Cameron
2023-03-21 15:39 ` Mehdi Djait
0 siblings, 1 reply; 30+ messages in thread
From: Jonathan Cameron @ 2023-03-20 12:24 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Jonathan Cameron, Mehdi Djait, krzysztof.kozlowski+dt,
andriy.shevchenko, linux-iio, linux-kernel, devicetree
On Mon, 20 Mar 2023 09:17:54 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> Hi Mehdi and Jonathan,
>
> Just my take on couple of comments from Jonathan :) I still have my own
> review to do though...
>
> On 3/19/23 18:20, Jonathan Cameron wrote:
> > On Fri, 17 Mar 2023 00:48:36 +0100
> > Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> >
> >> Refactor the kx022a driver implementation to make it more
> >> generic and extensible.
> >> Add the chip_info structure will to the driver's private
> >> data to hold all the device specific infos.
> >> Move the enum, struct and constants definitions to the header
> >> file.
> >
> > You also introduce an i2c_device_id table
> >
> > Without that I think module autoloading will be broken anyway so that's
> > definitely a good thing to add.
>
> I am pretty sure the autoloading worked for OF-systems. But yes, adding
> the i2c_device_id is probably a good idea. Thanks.
Ah. Maybe that issue only occurred for SPI - I'd assumed it was more general.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/?id=5fa6863ba692
>
> > A few comments inline. Mostly around reducing the name changes.
> > Wild cards (or simply shorted 'generic' prefixes like KX_)
> > almost always bite back in the long run. Hence we generally just try
> > to name things after the first device that they were relevant to.
>
> I must say I disagree on this with you Jonathan. I know wildcards tend
> to get confusing - but I still like the idea of showing which of the
> definitions are IC specific and which ones are generic or at least used
> by more than one variant - especially as long as we only have two
> supported ICs. I definitely like the macro naming added by Mehdi. This
> approach has been very helpful for me for example in the BD718x7
> (BD71837/BD71847/BD71850) PMIC driver. My take on this is:
>
> 1) I like the generic KX_define.
We already have other kionix drivers that don't use these defines.
This is less of an issue if they are very local - so pushed down to the C file,
but I still don't like the implication that they extend to a broad range of
devices.
> 2) I would not try adding wildcards like KX_X22 - to denote support for
> 122 and 022 - while not supporting 132 - in my experience - that won't
> scale.
I think this already runs into this problem or at least sets the driver
up to hit it very soon. The reality is that these definitions are shared
by the 2 parts supported so far. 3rd part comes along and I'd be willing
to place a bet that at least one of these definitions doesn't apply.
So we end up with a mess converting it back to a specific name.
I've gone down this path many times before and it very rarely works out.
> 3) I definitely like the idea of using exact model number prefix for
> 'stuff' which is intended to work only on one exact .
When you have 2 devices it is easy to separate the 'generic' from the
'specific'. That breaks when you have 3. If we are sure there won't be
a 3rd device supported by this driver then fair enough...
>
> Regarding the 3) - I am not so strict on how the register/mask defines
> are handled - I _like_ the 1) 2) 3) approach above - but mask/register
> defines tend to get set (correctly) once and not required to be looked
> up after this. But. When the 'stuff' is functions - this gets very
> useful as one is very often required to see which functions are executed
> on which IC variant. Same goes to structs.
Given they tend to be accessed via a function pointer, even functions
are only set up the once. For these I'm fine with a nasty listing
type approach with multiple part names in the function defintion.
That doesn't scale great either as lots of parts get added but it at least
calls out which function covers which parts.
>
> So, if we manage to convince Jonathan about the naming, then I like what
> yoo had here! I would hovever do it in two steps. I would at first do
> renaming patch where the generic defines were renamed - without any
> functional changes - and only then add the kx132 stuff in a subsequent
> patch. That would simplify seeing which changes are just renaming and
> which are functional ones.
>
> But here, I must go with the wind - if subsystem maintainer says the
> code should not have naming like this - then I have no say over it... :/
If we have truely universal defines - sometimes this happens for WHO AM I
registers for example as they are the same over all devices from a manufacturer
(more or less anyway) then the broad forms are fine. Otherwise it just tends
to end up as a mess if lots of parts added.
>
> >>
> >> +static const struct i2c_device_id kx022a_i2c_id[] = {
> >> + { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
> > If there are a small set and we aren't ever going to index the chip_info structure
> > we might be better off not bothering with the enum and instead using a separate
> > instance of the structure for each chip.
> >
>
> I kind of like also the table added by Mehdi. I admit I was at first
> just thinking that we should have a pointer to the struct here without
> any tables - but... After I took a peek in the kionix-kx022a.c - I kind
> of liked the table and not exporting the struct names. So, I don't have
> a strong opinion on this.
>
> I think it's worth noting that this driver could (maybe easily enough)
> be extended to support also a few other kionix accelerometers. Maybe, if
> we don't scare Mehdi away, we will see a few other variants supported as
> well ;)
This one wasn't a particularly important bit of feedback. I'm fine with the
table, though seems slightly less readable to my eyes.
>
> >> data->regmap = regmap;
> >> data->dev = dev;
> >> data->irq = irq;
> >> - data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
> >> + data->odr_ns = KX_DEFAULT_PERIOD_NS;
> >> mutex_init(&data->mutex);
> >>
> >> - idev->channels = kx022a_channels;
> >> - idev->num_channels = ARRAY_SIZE(kx022a_channels);
> >> - idev->name = "kx022-accel";
> >
> > Ah. Missed this naming in original driver review. We only normally
> > postfix with accel in devices that have multiple sensors with separate
> > drivers. Don't think that is true of the kx022a.
>
> Ouch. I am not 100% sure but may be you didn't miss it. It may be I just
> missed fixing this because your comment here sounds somewhat familiar to
> me! (Or then you commented on suffix in driver-name).
Meh. This stuff happens and at the end of the day it's a magic string
that userspace can match against. No userspace knows all of them anyway
so most likely it's just provided in a 'selection' box for a user or encoded
in a custom script / config file. So not hugely important for it to have
the simplest possible form.
>
> > It's ABI so we are stuck with it, but avoid repeating that issue
> > for new devices. >
>
> >>
> >> +enum kx022a_device_type {
> >> + KX022A,
> >> +};
> >
> > As below. I'd avoid using the enum unless needed.
> > That can make sense where a driver supports lots of devices but I don't think
> > it does here.
>
> Well, I know it is usually not too clever to be prepared for the future
> stuff too well. But - I don't think the enum and table are adding much
> of complexity? I am saying this as I think this driver could be extended
> to support also kx022 (without the A), kx023, kx122. I've also seen some
> references to model kx022A-120B (but I have no idea what's the story
> there or if that IC is publicly available). Maybe Mehdi would like to
> extend this driver further after the KX132 is done ;)
Not adding a lot, but you are going to end up with adding one line
to an enum in the header for each new device, vs one
extern line. So I'm not sure it saves anything either.
>
> >> -int kx022a_probe_internal(struct device *dev);
> >> -extern const struct regmap_config kx022a_regmap;
> >> +struct kx022a_chip_info {
> >> + const char *name;
> >> + enum kx022a_device_type type;
> >> + const struct regmap_config *regmap_config;
> >> + const struct iio_chan_spec *channels;
> >> + unsigned int num_channels;
> >> + unsigned int fifo_length;
> >> + u8 who;
> > Some of these are no immediately obvious so either rename the
> > field so it is more obvious what it is, or add comments.
>
> I would vote for adding a comment :) I like the who. Both the band and
> this member here :) Data-sheet has register named as "who_am_i" - so I
> don't think this name is too obfuscating - and what matters to me - it
> is short yet meaningful.
>
> >> + u8 id;
> >> + u8 cntl;
> >> + u8 cntl2;
> >> + u8 odcntl;
> >> + u8 buf_cntl1;
> >> + u8 buf_cntl2;
> >> + u8 buf_clear;
> >> + u8 buf_status1;
> >> + u16 buf_smp_lvl_mask;
> >> + u8 buf_read;
> >> + u8 inc1;
> >> + u8 inc4;
> >> + u8 inc5;
> >> + u8 inc6;
> >> + u8 xout_l;
> >> +};
> >> +
> >> +struct kx022a_data {
> >
> > Why move this to the header? Unless there is a strong reason
> > I'd prefer this to stay down in the .c file.
>
> So would I. It's definitely nice to be able to see the struct in the
> same file where the code referencing it is.
>
>
> Yours,
> -- Matti
Thanks,
Jonathan
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-20 9:35 ` Matti Vaittinen
2023-03-20 12:02 ` Andy Shevchenko
@ 2023-03-20 12:34 ` Jonathan Cameron
2023-03-20 12:52 ` Matti Vaittinen
2023-03-21 15:56 ` Mehdi Djait
2 siblings, 1 reply; 30+ messages in thread
From: Jonathan Cameron @ 2023-03-20 12:34 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Mehdi Djait, jic23, krzysztof.kozlowski+dt, andriy.shevchenko,
linux-iio, linux-kernel, devicetree
On Mon, 20 Mar 2023 11:35:06 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> On 3/17/23 01:48, Mehdi Djait wrote:
> > Refactor the kx022a driver implementation to make it more
> > generic and extensible.
> > Add the chip_info structure will to the driver's private
> > data to hold all the device specific infos.
> > Move the enum, struct and constants definitions to the header
> > file.
> >
> > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
> > ---
> > drivers/iio/accel/kionix-kx022a-i2c.c | 19 +-
> > drivers/iio/accel/kionix-kx022a-spi.c | 22 +-
> > drivers/iio/accel/kionix-kx022a.c | 289 ++++++++++++--------------
> > drivers/iio/accel/kionix-kx022a.h | 128 ++++++++++--
> > 4 files changed, 274 insertions(+), 184 deletions(-)
> >
> > diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> > index e6fd02d931b6..21c4c0ae1a68 100644
> > --- a/drivers/iio/accel/kionix-kx022a-i2c.c
> > +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> > @@ -15,23 +15,35 @@
> > static int kx022a_i2c_probe(struct i2c_client *i2c)
> > {
> > struct device *dev = &i2c->dev;
> > + struct kx022a_chip_info *chip_info;
> > struct regmap *regmap;
> > + const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
> >
> > if (!i2c->irq) {
> > dev_err(dev, "No IRQ configured\n");
> > return -EINVAL;
> > }
> >
> > - regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
> > + chip_info = device_get_match_data(&i2c->dev);
> > + if (!chip_info)
> > + chip_info = (const struct kx022a_chip_info *) id->driver_data;
> > +
> > + regmap = devm_regmap_init_i2c(i2c, chip_info->regmap_config);
> > if (IS_ERR(regmap))
> > return dev_err_probe(dev, PTR_ERR(regmap),
> > "Failed to initialize Regmap\n");
>
> Hm. I would like to pull the regmap_config out of the chip_info struct.
> As far as I see, the regmap_config is only needed in these bus specific
> files. On the other hand, the chip-info is only needed in the
> kionix-kx022a.c file, right?
>
I disagree. We've moved quite a few drivers away from the enum route
because the indirection doesn't add anything useful and leads to
nasty casting to enums. In particular, we have to avoid using enum
value of 0 if we want to check if there is a match. For a pointer that's
an easy check against NULL.
The regmap is product specific so makes sense as part of the chip_info
structure.
> So, maybe you could here just get the regmap_config based on the chip-id
> (enum value you added - the data pointer in match tables could be just
> the enum value indicating the IC type). Then, you could pass this enum
> value to kx022a_probe_internal() - and the chip-info struct could be
> selected in the kionix-kx022a.c based on it. That way you would not need
> the struct chip-info here or regmap_config in kionix-kx022a.c. Same in
> the *-spi.c
>
> Something like:
>
> enum {
> KIONIX_IC_KX022A,
> KIONIX_IC_KX132_xxx, /* xxx denotes accurate model suffix */
> };
>
> static const struct of_device_id kx022a_of_match[] = {
> { .compatible = "kionix,kx022a", .data = KIONIX_IC_KX022A },
> ...
>
> chip_id = device_get_match_data(&i2c->dev);
This fails for probes using the i2c_device_id table entries.
So you need to check for invalid entry. Unfortunately that is
a NULL return which you can't detect if your enum has a value of 0.
>
> regmap_cfg = kx022a_kx_regmap_cfg[chip_id];
> regmap = devm_regmap_init_i2c(i2c, regmap_cfg);
> ...
> return kx022a_probe_internal(dev, chip_id);
>
> Do you think that would work?
It would work with the enum starting at 1, and it's a pattern that
used to be common. Less so now because with multiple firmware types
we want to be able to check trivially if we have a match.
>
> OTOH, to really benefit from this we should probably pull out the
> regmap-configs from the kionix-kx022a.c. I am not really sure where we
> should put it then though. Hence, if there is no good ideas how to split
> the config and chip-info so they are only available/used where needed -
> then I am also Ok with the current approach.
Definitely stick to current approach. If I had the time I'd
rip out all the code useing enums in match tables. It's bitten us
a few times with nasty to track down bugs that only affect more obscure
ways of binding the driver.
...
>
> >
> > +static int kx022a_get_fifo_bytes(struct kx022a_data *data)
> > +{
> > + struct device *dev = regmap_get_device(data->regmap);
> > + __le16 buf_status;
> > + int ret, fifo_bytes;
> > +
> > + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
> > + if (ret) {
> > + dev_err(dev, "Error reading buffer status\n");
> > + return ret;
> > + }
> > +
> > + buf_status &= data->chip_info->buf_smp_lvl_mask;
> > + fifo_bytes = le16_to_cpu(buf_status);
> > +
> > + /*
> > + * The KX022A has FIFO which can store 43 samples of HiRes data from 2
> > + * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
> > + * 258 bytes of sample data. The quirk to know is that the amount of bytes in
> > + * the FIFO is advertised via 8 bit register (max value 255). The thing to note
> > + * is that full 258 bytes of data is indicated using the max value 255.
> > + */
> > + if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
> > + fifo_bytes = KX022A_FIFO_MAX_BYTES;
> > +
> > + if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)
> > + dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
> > +
> > + return fifo_bytes;
> > +}
>
> I like adding this function. Here I agree with Jonathan - having a
> device specific functions would clarify this a bit. The KX022A "quirk"
> is a bit confusing. You could then get rid of the buf_smp_lvl_mask.
I'd missed the type quirk. Good point, definitely have a callback.
Get rid of that 'type' element of the chip_info.
That is a bad design pattern as it doesn't scale to lots of devices
as you end up with big switch statements.
>
> > +
> > static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> > {
> > /*
> > @@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> > */
> > data->timestamp = 0;
> >
> > - return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
> > + return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
> > }
> >
> > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> > bool irq)
> > {
> > struct kx022a_data *data = iio_priv(idev);
> > - struct device *dev = regmap_get_device(data->regmap);
> > - __le16 buffer[KX022A_FIFO_LENGTH * 3];
> > + __le16 buffer[data->chip_info->fifo_length * 3];
>
> I don't like this. Having the length of an array decided at run-time is
> not something I appreciate. Maybe you could just always reserve the
> memory so that the largest FIFO gets supported. I am just wondering how
> large arrays we can safely allocate from the stack?
I'd missed this as well. Definitely don't have a variable length array.
Allocate it as a buffer accessed via a pointer in kx022a_data
>
>
> > if (ret)
> > goto unlock_out;
> >
>
> > -int kx022a_probe_internal(struct device *dev)
> > +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
>
> As mentioned elsewhere, this might also work if the chip-type enum was
> passed here as parameter. That way the bus specific part would not need
> to know about the struct chip_info...
It only knows there is a pointer. Doesn't need to know more than that.
+ argument against as above.
Jonathan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-20 12:34 ` Jonathan Cameron
@ 2023-03-20 12:52 ` Matti Vaittinen
0 siblings, 0 replies; 30+ messages in thread
From: Matti Vaittinen @ 2023-03-20 12:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Mehdi Djait, jic23, krzysztof.kozlowski+dt, andriy.shevchenko,
linux-iio, linux-kernel, devicetree
On 3/20/23 14:34, Jonathan Cameron wrote:
> On Mon, 20 Mar 2023 11:35:06 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
>> On 3/17/23 01:48, Mehdi Djait wrote:
>>
>> OTOH, to really benefit from this we should probably pull out the
>> regmap-configs from the kionix-kx022a.c. I am not really sure where we
>> should put it then though. Hence, if there is no good ideas how to split
>> the config and chip-info so they are only available/used where needed -
>> then I am also Ok with the current approach.
>
> Definitely stick to current approach. If I had the time I'd
> rip out all the code useing enums in match tables. It's bitten us
> a few times with nasty to track down bugs that only affect more obscure
> ways of binding the driver.
>
Seems like Jonathan has a strong opinion on this. Please follow his and
Andy's guidance on this one and forget my comment.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-16 23:48 ` [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure Mehdi Djait
` (4 preceding siblings ...)
2023-03-20 9:35 ` Matti Vaittinen
@ 2023-03-21 1:05 ` kernel test robot
5 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2023-03-21 1:05 UTC (permalink / raw)
To: Mehdi Djait, jic23, mazziesaccount
Cc: oe-kbuild-all, krzysztof.kozlowski+dt, andriy.shevchenko,
linux-iio, linux-kernel, devicetree, Mehdi Djait
Hi Mehdi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on next-20230320]
[cannot apply to linus/master v6.3-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mehdi-Djait/dt-bindings-iio-Add-KX132-accelerometer/20230317-075056
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/3ddca10a4c03c3a64afb831cc9dd1e01fe89d305.1679009443.git.mehdi.djait.k%40gmail.com
patch subject: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
config: loongarch-randconfig-s032-20230319 (https://download.01.org/0day-ci/archive/20230321/202303210809.RAQ7nfl7-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/40c75341c42d0e5bea5d73961202978a4be41cd2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mehdi-Djait/dt-bindings-iio-Add-KX132-accelerometer/20230317-075056
git checkout 40c75341c42d0e5bea5d73961202978a4be41cd2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=loongarch olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=loongarch SHELL=/bin/bash drivers/iio/accel/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303210809.RAQ7nfl7-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/iio/accel/kionix-kx022a-spi.c:27:19: sparse: sparse: incorrect type in assignment (different modifiers) @@ expected struct kx022a_chip_info *chip_info @@ got void const * @@
drivers/iio/accel/kionix-kx022a-spi.c:27:19: sparse: expected struct kx022a_chip_info *chip_info
drivers/iio/accel/kionix-kx022a-spi.c:27:19: sparse: got void const *
>> drivers/iio/accel/kionix-kx022a-spi.c:29:27: sparse: sparse: incorrect type in assignment (different modifiers) @@ expected struct kx022a_chip_info *chip_info @@ got struct kx022a_chip_info const * @@
drivers/iio/accel/kionix-kx022a-spi.c:29:27: sparse: expected struct kx022a_chip_info *chip_info
drivers/iio/accel/kionix-kx022a-spi.c:29:27: sparse: got struct kx022a_chip_info const *
vim +27 drivers/iio/accel/kionix-kx022a-spi.c
14
15 static int kx022a_spi_probe(struct spi_device *spi)
16 {
17 struct device *dev = &spi->dev;
18 struct kx022a_chip_info *chip_info;
19 struct regmap *regmap;
20 const struct spi_device_id *id = spi_get_device_id(spi);
21
22 if (!spi->irq) {
23 dev_err(dev, "No IRQ configured\n");
24 return -EINVAL;
25 }
26
> 27 chip_info = device_get_match_data(&spi->dev);
28 if (!chip_info)
> 29 chip_info = (const struct kx022a_chip_info *) id->driver_data;
30
31 regmap = devm_regmap_init_spi(spi, chip_info->regmap_config);
32 if (IS_ERR(regmap))
33 return dev_err_probe(dev, PTR_ERR(regmap),
34 "Failed to initialize Regmap\n");
35
36 return kx022a_probe_internal(dev, chip_info);
37 }
38
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer
2023-03-19 7:43 ` Matti Vaittinen
@ 2023-03-21 13:16 ` Mehdi Djait
2023-03-22 7:47 ` Matti Vaittinen
0 siblings, 1 reply; 30+ messages in thread
From: Mehdi Djait @ 2023-03-21 13:16 UTC (permalink / raw)
To: Matti Vaittinen
Cc: jic23, krzysztof.kozlowski+dt, andriy.shevchenko, linux-iio,
linux-kernel, devicetree
Hi Matti,
On Sun, Mar 19, 2023 at 09:43:19AM +0200, Matti Vaittinen wrote:
> Hi Mehdi,
>
> Things have been piling up for me during last two weeks... I will do proper
> review during next week.
>
> On 3/17/23 01:48, Mehdi Djait wrote:
> > KX132 accelerometer is a sensor which:
> > - supports G-ranges of (+/-) 2, 4, 8, and 16G
> > - can be connected to I2C or SPI
> > - has internal HW FIFO buffer
> > - supports various ODRs (output data rates)
> >
> > The KX132 accelerometer is very similair to the KX022A.
> > One key difference is number of bits to report the number of data bytes that
> > have been stored in the sample buffer: 8 bits for KX022A vs 10 bits for KX132.
>
> The KX022A has 16bits of data in HiRes mode. This is the default for kx022a
> driver.
I am talking here about "Buffer Sample Level number of bits": kx022a uses
8 bits: just BUF_STATUS_1 to report the status of the buffer. kx132 uses
BUF_STATUS_1 and the Bit0, Bit1 of BUF_STATUS_2.
That's the reason for adding the kx022a_get_fifo_bytes function.
>
> > A complete list of differences is listed in [1]
> >
> >
> > [1] https://kionixfs.azureedge.net/en/document/AN112-Transitioning-to-KX132-1211-Accelerometer.pdf1
>
> This document is somewhat misleading. It does not contain KX022A but the
> older KX022. Kionix has the somewhat confusing habit of having very similar
> names for models with - occasionally significant - differences. (My own
> opinion).
Yes, I am aware that it does not contain KX022A, but from my
understanding of your code, the document can be used to highlight the
difference I mentioned
>
> I the "Technical referene manual" is more interesting document than the
> data-sheet:
>
> https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
>
> I have heard that there have been a few very different versions of KX132 as
> well. Not sure if they have "leaked" out to public though. In any case, for
> the kx132 it might be safest to use the full model name - especially in the
> DT compatibles.
>
I will change it to kx132-1211
> Finally, AFAIK the key "thing" in KX132 is the "ADP" (Advanced Data Path)
> feature which allows filtering the data "in sensor". Unfortunately, I am not
> really familiar with this feature. Do you think this is something that might
> get configured only once at start-up depending on the purpose of the board?
> If yes, this might be something that will end-up having properties in
> device-tree. If yes, then it might be a good idea to have own binding doc
> for KX132. Currently it seems Ok to have them in the same binding doc
> though.
>
Correct me if I am wrong: the Devicetree is a description of the
hardware and the transitioning document states:
"From a hardware perspective, all these sensors
have an identical pad/pin layouts and identical pin functionality"
I was thinking about adding an advanced_data_path boolean to the chip_info
struct and providing different driver callbacks depending on the value.
Something like in the bmc150-accel-core: iio_info for bmc150_accel_info
and iio_info for bmc150_accel_info_fifo
--
Kind Regards
Mehdi Djait
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: Add KX132 accelerometer
2023-03-19 15:54 ` Jonathan Cameron
@ 2023-03-21 13:22 ` Mehdi Djait
0 siblings, 0 replies; 30+ messages in thread
From: Mehdi Djait @ 2023-03-21 13:22 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mazziesaccount, krzysztof.kozlowski+dt, andriy.shevchenko,
linux-iio, linux-kernel, devicetree
Hello Jonathan,
On Sun, Mar 19, 2023 at 03:54:51PM +0000, Jonathan Cameron wrote:
> On Fri, 17 Mar 2023 00:48:35 +0100
> Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
>
> > Extend the kionix,kx022a.yaml file to support the
> > kx132 device
> >
> > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
>
> Pins and power supplies etc all look the same to me so indeed seems that
> you have covered all that is needed. One small comment inline
> and I think Matti's point about more specific compatibles probably
> needs to be taken into account if there are known variants.
>
> Kionix has done this for a long time. I remember that fun with the
> kxsd9 lots of years back - that had lots of subtle variants.
>
> > ---
> > .../bindings/iio/accel/kionix,kx022a.yaml | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml b/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
> > index 986df1a6ff0a..ac1e27402d5e 100644
> > --- a/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
> > +++ b/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
> > @@ -4,19 +4,22 @@
> > $id: http://devicetree.org/schemas/iio/accel/kionix,kx022a.yaml#
> > $schema: http://devicetree.org/meta-schemas/core.yaml#
> >
> > -title: ROHM/Kionix KX022A Accelerometer
> > +title: ROHM/Kionix KX022A and KX132 Accelerometers
> >
> > maintainers:
> > - Matti Vaittinen <mazziesaccount@gmail.com>
> >
> > description: |
> > - KX022A is a 3-axis accelerometer supporting +/- 2G, 4G, 8G and 16G ranges,
> > - output data-rates from 0.78Hz to 1600Hz and a hardware-fifo buffering.
> > - KX022A can be accessed either via I2C or SPI.
> > + KX022A and KX132 are 3-axis accelerometers supporting +/- 2G, 4G, 8G and
> > + 16G ranges, output data-rates from 0.78Hz to 1600Hz and a hardware-fifo
>
> This may be one of those 'there are many versions' of the chip issues, but
> the random datasheet I got via digikey (kionix website was slow and I'm
> impatient) has max as 25600Hz for the KX132-1211.
>
> No particular reason the sampling rates need to be in this description so
> if they are different I'd just remove the mention or just say
> "variable output data-rates"
Yes indeed, the max frequency is different and the max frequency
supported in the driver is 200Hz anyway.
Removing the values of samling rates is best here. I will change it in v2.
--
Kind Regards
Mehdi Djait
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-20 12:24 ` Jonathan Cameron
@ 2023-03-21 15:39 ` Mehdi Djait
0 siblings, 0 replies; 30+ messages in thread
From: Mehdi Djait @ 2023-03-21 15:39 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Matti Vaittinen, Jonathan Cameron, krzysztof.kozlowski+dt,
andriy.shevchenko, linux-iio, linux-kernel, devicetree
Hello everyone,
On Mon, Mar 20, 2023 at 12:24:47PM +0000, Jonathan Cameron wrote:
> On Mon, 20 Mar 2023 09:17:54 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> > Hi Mehdi and Jonathan,
> >
> > Just my take on couple of comments from Jonathan :) I still have my own
> > review to do though...
> >
> > On 3/19/23 18:20, Jonathan Cameron wrote:
> > > On Fri, 17 Mar 2023 00:48:36 +0100
> > > Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> > >
> > >> Refactor the kx022a driver implementation to make it more
> > >> generic and extensible.
> > >> Add the chip_info structure will to the driver's private
> > >> data to hold all the device specific infos.
> > >> Move the enum, struct and constants definitions to the header
> > >> file.
> > >
> > > You also introduce an i2c_device_id table
> > >
> > > Without that I think module autoloading will be broken anyway so that's
> > > definitely a good thing to add.
> >
> > I am pretty sure the autoloading worked for OF-systems. But yes, adding
> > the i2c_device_id is probably a good idea. Thanks.
>
> Ah. Maybe that issue only occurred for SPI - I'd assumed it was more general.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/?id=5fa6863ba692
>
> >
> > > A few comments inline. Mostly around reducing the name changes.
> > > Wild cards (or simply shorted 'generic' prefixes like KX_)
> > > almost always bite back in the long run. Hence we generally just try
> > > to name things after the first device that they were relevant to.
> >
> > I must say I disagree on this with you Jonathan. I know wildcards tend
> > to get confusing - but I still like the idea of showing which of the
> > definitions are IC specific and which ones are generic or at least used
> > by more than one variant - especially as long as we only have two
> > supported ICs. I definitely like the macro naming added by Mehdi. This
> > approach has been very helpful for me for example in the BD718x7
> > (BD71837/BD71847/BD71850) PMIC driver. My take on this is:
> >
> > 1) I like the generic KX_define.
>
> We already have other kionix drivers that don't use these defines.
> This is less of an issue if they are very local - so pushed down to the C file,
> but I still don't like the implication that they extend to a broad range of
> devices.
>
> > 2) I would not try adding wildcards like KX_X22 - to denote support for
> > 122 and 022 - while not supporting 132 - in my experience - that won't
> > scale.
>
> I think this already runs into this problem or at least sets the driver
> up to hit it very soon. The reality is that these definitions are shared
> by the 2 parts supported so far. 3rd part comes along and I'd be willing
> to place a bet that at least one of these definitions doesn't apply.
> So we end up with a mess converting it back to a specific name.
>
> I've gone down this path many times before and it very rarely works out.
>
> > 3) I definitely like the idea of using exact model number prefix for
> > 'stuff' which is intended to work only on one exact .
>
> When you have 2 devices it is easy to separate the 'generic' from the
> 'specific'. That breaks when you have 3. If we are sure there won't be
> a 3rd device supported by this driver then fair enough...
>
> >
> > Regarding the 3) - I am not so strict on how the register/mask defines
> > are handled - I _like_ the 1) 2) 3) approach above - but mask/register
> > defines tend to get set (correctly) once and not required to be looked
> > up after this. But. When the 'stuff' is functions - this gets very
> > useful as one is very often required to see which functions are executed
> > on which IC variant. Same goes to structs.
>
> Given they tend to be accessed via a function pointer, even functions
> are only set up the once. For these I'm fine with a nasty listing
> type approach with multiple part names in the function defintion.
> That doesn't scale great either as lots of parts get added but it at least
> calls out which function covers which parts.
>
> >
> > So, if we manage to convince Jonathan about the naming, then I like what
> > yoo had here! I would hovever do it in two steps. I would at first do
> > renaming patch where the generic defines were renamed - without any
> > functional changes - and only then add the kx132 stuff in a subsequent
> > patch. That would simplify seeing which changes are just renaming and
> > which are functional ones.
> >
> > But here, I must go with the wind - if subsystem maintainer says the
> > code should not have naming like this - then I have no say over it... :/
>
> If we have truely universal defines - sometimes this happens for WHO AM I
> registers for example as they are the same over all devices from a manufacturer
> (more or less anyway) then the broad forms are fine. Otherwise it just tends
> to end up as a mess if lots of parts added.
I will remove the generic defines.
I also really liked the idea of seperating the IC specific stuff from the
generic ones. Better play it safe here, I can also see it becoming a mess in
the long run.
> >
> > >>
> > >> +static const struct i2c_device_id kx022a_i2c_id[] = {
> > >> + { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
> > > If there are a small set and we aren't ever going to index the chip_info structure
> > > we might be better off not bothering with the enum and instead using a separate
> > > instance of the structure for each chip.
> > >
> >
> > I kind of like also the table added by Mehdi. I admit I was at first
> > just thinking that we should have a pointer to the struct here without
> > any tables - but... After I took a peek in the kionix-kx022a.c - I kind
> > of liked the table and not exporting the struct names. So, I don't have
> > a strong opinion on this.
> >
> > I think it's worth noting that this driver could (maybe easily enough)
> > be extended to support also a few other kionix accelerometers. Maybe, if
> > we don't scare Mehdi away, we will see a few other variants supported as
> > well ;)
>
> This one wasn't a particularly important bit of feedback. I'm fine with the
> table, though seems slightly less readable to my eyes.
My reasoning behind it: when supporting multiple devices, having a single
array of chip_info and one single EXPORT_SYMBOL is more concise and
readable.
>
> >
> > >> data->regmap = regmap;
> > >> data->dev = dev;
> > >> data->irq = irq;
> > >> - data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
> > >> + data->odr_ns = KX_DEFAULT_PERIOD_NS;
> > >> mutex_init(&data->mutex);
> > >>
> > >> - idev->channels = kx022a_channels;
> > >> - idev->num_channels = ARRAY_SIZE(kx022a_channels);
> > >> - idev->name = "kx022-accel";
> > >
> > > Ah. Missed this naming in original driver review. We only normally
> > > postfix with accel in devices that have multiple sensors with separate
> > > drivers. Don't think that is true of the kx022a.
> >
> > Ouch. I am not 100% sure but may be you didn't miss it. It may be I just
> > missed fixing this because your comment here sounds somewhat familiar to
> > me! (Or then you commented on suffix in driver-name).
>
> Meh. This stuff happens and at the end of the day it's a magic string
> that userspace can match against. No userspace knows all of them anyway
> so most likely it's just provided in a 'selection' box for a user or encoded
> in a custom script / config file. So not hugely important for it to have
> the simplest possible form.
>
> >
> > > It's ABI so we are stuck with it, but avoid repeating that issue
> > > for new devices. >
I will change the name in the chip_info of kx022a back to "kx022-accel"
I am already using "kx132" as name for the newly supported device
> >
> > >>
> > >> +enum kx022a_device_type {
> > >> + KX022A,
> > >> +};
> > >
> > > As below. I'd avoid using the enum unless needed.
> > > That can make sense where a driver supports lots of devices but I don't think
> > > it does here.
> >
> > Well, I know it is usually not too clever to be prepared for the future
> > stuff too well. But - I don't think the enum and table are adding much
> > of complexity? I am saying this as I think this driver could be extended
> > to support also kx022 (without the A), kx023, kx122. I've also seen some
> > references to model kx022A-120B (but I have no idea what's the story
> > there or if that IC is publicly available). Maybe Mehdi would like to
> > extend this driver further after the KX132 is done ;)
Yes, my goal is to support more devices and I want to make as easy as
possible to extend this driver :)
>
> Not adding a lot, but you are going to end up with adding one line
> to an enum in the header for each new device, vs one
> extern line. So I'm not sure it saves anything either.
>
Using a separate instance of the chip_info structure for each chip means
also an extra symbol export
> >
> > >> -int kx022a_probe_internal(struct device *dev);
> > >> -extern const struct regmap_config kx022a_regmap;
> > >> +struct kx022a_chip_info {
> > >> + const char *name;
> > >> + enum kx022a_device_type type;
> > >> + const struct regmap_config *regmap_config;
> > >> + const struct iio_chan_spec *channels;
> > >> + unsigned int num_channels;
> > >> + unsigned int fifo_length;
> > >> + u8 who;
> > > Some of these are no immediately obvious so either rename the
> > > field so it is more obvious what it is, or add comments.
> >
> > I would vote for adding a comment :) I like the who. Both the band and
> > this member here :) Data-sheet has register named as "who_am_i" - so I
> > don't think this name is too obfuscating - and what matters to me - it
> > is short yet meaningful.
> >
> > >> + u8 id;
> > >> + u8 cntl;
> > >> + u8 cntl2;
> > >> + u8 odcntl;
> > >> + u8 buf_cntl1;
> > >> + u8 buf_cntl2;
> > >> + u8 buf_clear;
> > >> + u8 buf_status1;
> > >> + u16 buf_smp_lvl_mask;
> > >> + u8 buf_read;
> > >> + u8 inc1;
> > >> + u8 inc4;
> > >> + u8 inc5;
> > >> + u8 inc6;
> > >> + u8 xout_l;
> > >> +};
> > >> +
> > >> +struct kx022a_data {
> > >
> > > Why move this to the header? Unless there is a strong reason
> > > I'd prefer this to stay down in the .c file.
> >
> > So would I. It's definitely nice to be able to see the struct in the
> > same file where the code referencing it is.
no strong reason, I will move it back to the .c file
--
Kind Regards
Mehdi Djait
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-20 9:35 ` Matti Vaittinen
2023-03-20 12:02 ` Andy Shevchenko
2023-03-20 12:34 ` Jonathan Cameron
@ 2023-03-21 15:56 ` Mehdi Djait
2023-03-22 6:37 ` Matti Vaittinen
2 siblings, 1 reply; 30+ messages in thread
From: Mehdi Djait @ 2023-03-21 15:56 UTC (permalink / raw)
To: Matti Vaittinen
Cc: jic23, krzysztof.kozlowski+dt, andriy.shevchenko, linux-iio,
linux-kernel, devicetree
Hello Matti,
> > +static int kx022a_get_fifo_bytes(struct kx022a_data *data)
> > +{
> > + struct device *dev = regmap_get_device(data->regmap);
> > + __le16 buf_status;
> > + int ret, fifo_bytes;
> > +
> > + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
> > + if (ret) {
> > + dev_err(dev, "Error reading buffer status\n");
> > + return ret;
> > + }
> > +
> > + buf_status &= data->chip_info->buf_smp_lvl_mask;
> > + fifo_bytes = le16_to_cpu(buf_status);
> > +
> > + /*
> > + * The KX022A has FIFO which can store 43 samples of HiRes data from 2
> > + * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
> > + * 258 bytes of sample data. The quirk to know is that the amount of bytes in
> > + * the FIFO is advertised via 8 bit register (max value 255). The thing to note
> > + * is that full 258 bytes of data is indicated using the max value 255.
> > + */
> > + if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
> > + fifo_bytes = KX022A_FIFO_MAX_BYTES;
> > +
> > + if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)
> > + dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
> > +
> > + return fifo_bytes;
> > +}
>
> I like adding this function. Here I agree with Jonathan - having a device
> specific functions would clarify this a bit. The KX022A "quirk" is a bit
> confusing. You could then get rid of the buf_smp_lvl_mask.
my bad here, I should have made a separate patch and explained more ...
buf_smp_lvl_mask is essential because kionix products use different
number of bits to report "the number of data bytes that have been stored in the
sample buffer" using the registers BUF_STATUS_1 and BUF_STATUS_2
kx022a: 8bits
kx132: 10bits
kx12x: 11bits
kx126: 12bits
I think this function is quite generic and can be used for different
kionix devices:
- It reads BUF_STATUS_1 and BUF_STATUS_2 and then uses a chip specific
mask
- It takes care of the quirk of kx022a which is just a simple if statement
>
> > +
> > static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> > {
> > /*
> > @@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> > */
> > data->timestamp = 0;
> > - return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
> > + return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
> > }
> > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> > bool irq)
> > {
> > struct kx022a_data *data = iio_priv(idev);
> > - struct device *dev = regmap_get_device(data->regmap);
> > - __le16 buffer[KX022A_FIFO_LENGTH * 3];
> > + __le16 buffer[data->chip_info->fifo_length * 3];
>
> I don't like this. Having the length of an array decided at run-time is not
> something I appreciate. Maybe you could just always reserve the memory so
> that the largest FIFO gets supported. I am just wondering how large arrays
> we can safely allocate from the stack?
I was stupid enough to ignore the warnings...
I will take care of it in the v2
>
>
> > @@ -812,14 +792,14 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
> > goto unlock_out;
> > /* Enable buffer */
> > - ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
> > - KX022A_MASK_BUF_EN);
> > + ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
> > + KX_MASK_BUF_EN);
> > if (ret)
> > goto unlock_out;
> > - data->state |= KX022A_STATE_FIFO;
> > + data->state |= KX_STATE_FIFO;
> > ret = regmap_set_bits(data->regmap, data->ien_reg,
> > - KX022A_MASK_WMI);
> > + KX_MASK_WMI);
>
> I think this fits to one line now. (even on my screen)
>
> > if (ret)
> > goto unlock_out;
>
> > -int kx022a_probe_internal(struct device *dev)
> > +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
>
> As mentioned elsewhere, this might also work if the chip-type enum was
> passed here as parameter. That way the bus specific part would not need to
> know about the struct chip_info...
>
> > {
> > static const char * const regulator_names[] = {"io-vdd", "vdd"};
> > struct iio_trigger *indio_trig;
> > @@ -1023,6 +1003,7 @@ int kx022a_probe_internal(struct device *dev)
> > return -ENOMEM;
> > data = iio_priv(idev);
> > + data->chip_info = chip_info;
>
> ...Here you could then pick the correct chip_info based on the chip-type
> enum. In that case I'd like to get the regmap_config(s) in own file. Not
> sure how that would look like though.
>
> All in all, I like how this looks like. Nice job!
Thank you for the feedback :)
--
Kind Regards
Mehdi Djait
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer
2023-03-19 16:22 ` Jonathan Cameron
@ 2023-03-21 16:34 ` Mehdi Djait
2023-03-25 18:12 ` Jonathan Cameron
0 siblings, 1 reply; 30+ messages in thread
From: Mehdi Djait @ 2023-03-21 16:34 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mazziesaccount, krzysztof.kozlowski+dt, andriy.shevchenko,
linux-iio, linux-kernel, devicetree
Hello Jonathan,
On Sun, Mar 19, 2023 at 04:22:07PM +0000, Jonathan Cameron wrote:
> On Fri, 17 Mar 2023 00:48:37 +0100
> Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
>
> > Add support for the basic accelerometer features such as getting the
> > acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
> > using the WMI IRQ).
> >
> > Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
>
> Nothing much specific to this patch, most changes will be as a result
> of bringing this inline with the changes suggested for patch 2.
>
> thanks,
>
> Jonathan
> >
> > diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
> > index 3bb40e9f5613..7e43bdb37156 100644
> > --- a/drivers/iio/accel/kionix-kx022a.h
> > +++ b/drivers/iio/accel/kionix-kx022a.h
> > @@ -90,8 +90,61 @@
> > #define KX022A_REG_SELF_TEST 0x60
> > #define KX022A_MAX_REGISTER 0x60
> >
> > +
>
> Push these down into the c file.
Do you mean all REG and MASK defines ?
Even kx022a defines them in the h file, or am I misunderstanding your
comment ?
>
> > +#define KX132_REG_WHO 0x13
> > +#define KX132_ID 0x3d
> > +
> > +#define KX132_FIFO_LENGTH 86
> > +
> > +#define KX132_REG_CNTL2 0x1c
> > +#define KX132_REG_CNTL 0x1b
> > +#define KX132_MASK_RES BIT(6)
> > +#define KX132_GSEL_2 0x0
> > +#define KX132_GSEL_4 BIT(3)
> > +#define KX132_GSEL_8 BIT(4)
> > +#define KX132_GSEL_16 GENMASK(4, 3)
> > +
> > +#define KX132_REG_INS2 0x17
> > +#define KX132_MASK_INS2_WMI BIT(5)
> > +
> > +#define KX132_REG_XADP_L 0x02
> > +#define KX132_REG_XOUT_L 0x08
> > +#define KX132_REG_YOUT_L 0x0a
> > +#define KX132_REG_ZOUT_L 0x0c
> > +#define KX132_REG_COTR 0x12
> > +#define KX132_REG_TSCP 0x14
> > +#define KX132_REG_INT_REL 0x1a
> > +
> > +#define KX132_REG_ODCNTL 0x21
> > +
> > +#define KX132_REG_BTS_WUF_TH 0x4a
> > +#define KX132_REG_MAN_WAKE 0x4d
> > +
> > +#define KX132_REG_BUF_CNTL1 0x5e
> > +#define KX132_REG_BUF_CNTL2 0x5f
> > +#define KX132_REG_BUF_STATUS_1 0x60
> > +#define KX132_REG_BUF_STATUS_2 0x61
> > +#define KX132_MASK_BUF_SMP_LVL GENMASK(9, 0)
> > +#define KX132_REG_BUF_CLEAR 0x62
> > +#define KX132_REG_BUF_READ 0x63
> > +#define KX132_ODR_SHIFT 3
> > +#define KX132_FIFO_MAX_WMI_TH 86
> > +
> > +#define KX132_REG_INC1 0x22
> > +#define KX132_REG_INC5 0x26
> > +#define KX132_REG_INC6 0x27
> > +#define KX132_IPOL_LOW 0
> > +#define KX132_IPOL_HIGH KX_MASK_IPOL
> > +#define KX132_ITYP_PULSE KX_MASK_ITYP
> > +
> > +#define KX132_REG_INC4 0x25
> > +
> > +#define KX132_REG_SELF_TEST 0x5d
> > +#define KX132_MAX_REGISTER 0x76
> > +
> > enum kx022a_device_type {
> > KX022A,
> > + KX132,
> As mentioned in previous review, I think this would be neater
> done by just exporting the chip_info structures directly rather than
> putting them in an array.
I gave the reason in a response to the previous review.
--
Kind Regards
Mehdi Djait
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
2023-03-21 15:56 ` Mehdi Djait
@ 2023-03-22 6:37 ` Matti Vaittinen
0 siblings, 0 replies; 30+ messages in thread
From: Matti Vaittinen @ 2023-03-22 6:37 UTC (permalink / raw)
To: Mehdi Djait
Cc: jic23, krzysztof.kozlowski+dt, andriy.shevchenko, linux-iio,
linux-kernel, devicetree
On 3/21/23 17:56, Mehdi Djait wrote:
> Hello Matti,
>
>>> +static int kx022a_get_fifo_bytes(struct kx022a_data *data)
>>> +{
>>> + struct device *dev = regmap_get_device(data->regmap);
>>> + __le16 buf_status;
>>> + int ret, fifo_bytes;
>>> +
>>> + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
>>> + if (ret) {
>>> + dev_err(dev, "Error reading buffer status\n");
>>> + return ret;
>>> + }
>>> +
>>> + buf_status &= data->chip_info->buf_smp_lvl_mask;
>>> + fifo_bytes = le16_to_cpu(buf_status);
>>> +
>>> + /*
>>> + * The KX022A has FIFO which can store 43 samples of HiRes data from 2
>>> + * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
>>> + * 258 bytes of sample data. The quirk to know is that the amount of bytes in
>>> + * the FIFO is advertised via 8 bit register (max value 255). The thing to note
>>> + * is that full 258 bytes of data is indicated using the max value 255.
>>> + */
>>> + if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
>>> + fifo_bytes = KX022A_FIFO_MAX_BYTES;
>>> +
>>> + if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)
>>> + dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
>>> +
>>> + return fifo_bytes;
>>> +}
>>
>> I like adding this function. Here I agree with Jonathan - having a device
>> specific functions would clarify this a bit. The KX022A "quirk" is a bit
>> confusing. You could then get rid of the buf_smp_lvl_mask.
>
> my bad here, I should have made a separate patch and explained more ...
> buf_smp_lvl_mask is essential because kionix products use different
> number of bits to report "the number of data bytes that have been stored in the
> sample buffer" using the registers BUF_STATUS_1 and BUF_STATUS_2
Yes, they have different size of FIFO, and the KX022A does also have the
nasty "FIFO FULL" quirk. Due to this quirk and other differences I was
suggesting you created own functions for kx022a and kx132. Eg something
along the lines:
static int kx022a_get_fifo_bytes(struct kx022a_data *data)
{
...
}
static int kx132_get_fifo_bytes(struct kx022a_data *data)
{
...
}
struct chip_info {
...
int (*fifo_bytes)(struct kx022a_data *);
};
and do the:
fifo_bytes = kx022a_get_fifo_bytes;
or
fifo_bytes = kx132_get_fifo_bytes;
in probe. That will also remove the need to check the IC variant for
each FIFO read.
If you did that you could remove the buf_smp_lvl_mask and maybe also the
buf_statusX members from the chip_info struct (at least for now). You
could also do regular read for KX022A and drop the endianess conversion
for it. Bulk read is only needed for ICs with more than 8bits of FIFO
status. Furthermore, the IC-type check could then go away and the above
mentioned KX022A-specific handling would not be obfuscating the kx132 code.
>
> kx022a: 8bits
> kx132: 10bits
> kx12x: 11bits
> kx126: 12bits
>
> I think this function is quite generic and can be used for different
> kionix devices:
>
> - It reads BUF_STATUS_1 and BUF_STATUS_2 and then uses a chip specific
> mask
> - It takes care of the quirk of kx022a which is just a simple if statement
Yes. Your function definitely works. And I do like the fact that you did
own function for the "amount of data in fifo"-check. Still, the code
would be little simpler and perform a tiny bit better if you did two
functions instead of one.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer
2023-03-21 13:16 ` Mehdi Djait
@ 2023-03-22 7:47 ` Matti Vaittinen
0 siblings, 0 replies; 30+ messages in thread
From: Matti Vaittinen @ 2023-03-22 7:47 UTC (permalink / raw)
To: Mehdi Djait
Cc: jic23, krzysztof.kozlowski+dt, andriy.shevchenko, linux-iio,
linux-kernel, devicetree
On 3/21/23 15:16, Mehdi Djait wrote:
> Hi Matti,
>
> On Sun, Mar 19, 2023 at 09:43:19AM +0200, Matti Vaittinen wrote:
>> Hi Mehdi,
>>
>> Things have been piling up for me during last two weeks... I will do proper
>> review during next week.
>>
>> On 3/17/23 01:48, Mehdi Djait wrote:
>>> KX132 accelerometer is a sensor which:
>>> - supports G-ranges of (+/-) 2, 4, 8, and 16G
>>> - can be connected to I2C or SPI
>>> - has internal HW FIFO buffer
>>> - supports various ODRs (output data rates)
>>>
>>> The KX132 accelerometer is very similair to the KX022A.
>>> One key difference is number of bits to report the number of data bytes that
>>> have been stored in the sample buffer: 8 bits for KX022A vs 10 bits for KX132.
>>
>> The KX022A has 16bits of data in HiRes mode. This is the default for kx022a
>> driver.
>
> I am talking here about "Buffer Sample Level number of bits":
Ah. My bad. I misunderstood. Mentioning the number of bits made me to
instantly think of the format of measured data not the size of the FIFO
and how many bits are needed to represent the full FIFO.
kx022a uses
> 8 bits: just BUF_STATUS_1 to report the status of the buffer. kx132 uses
> BUF_STATUS_1 and the Bit0, Bit1 of BUF_STATUS_2.
>
> That's the reason for adding the kx022a_get_fifo_bytes function.
>
>>
>>> A complete list of differences is listed in [1]
>>>
>>>
>>> [1] https://kionixfs.azureedge.net/en/document/AN112-Transitioning-to-KX132-1211-Accelerometer.pdf1
>>
>> This document is somewhat misleading. It does not contain KX022A but the
>> older KX022. Kionix has the somewhat confusing habit of having very similar
>> names for models with - occasionally significant - differences. (My own
>> opinion).
>
> Yes, I am aware that it does not contain KX022A, but from my
> understanding of your code, the document can be used to highlight the
> difference I mentioned
I don't remember all the differences between the KX022 and KX022A - but
I believe at least the supported G-ranges were different. I think there
probably were also some other things.
>> Finally, AFAIK the key "thing" in KX132 is the "ADP" (Advanced Data Path)
>> feature which allows filtering the data "in sensor". Unfortunately, I am not
>> really familiar with this feature. Do you think this is something that might
>> get configured only once at start-up depending on the purpose of the board?
>> If yes, this might be something that will end-up having properties in
>> device-tree. If yes, then it might be a good idea to have own binding doc
>> for KX132. Currently it seems Ok to have them in the same binding doc
>> though.
>>
>
> Correct me if I am wrong: the Devicetree is a description of the
> hardware
Yes.
> and the transitioning document states:
>
> "From a hardware perspective, all these sensors
> have an identical pad/pin layouts and identical pin functionality"
Sometimes (fixed) hardware _configuration_ can be visible in
device-tree. Eg, device-tree can be used to tell: "On this board this
configurable piece of hardware shall look like this". My understanding
of ADP is very limited, but I thought it may be used to adjust the
hardware by for example adding some filters.
As I said, I however don't know if the ADP will be used with 'static
configurations' which could be seen as hardware properties. Hence I
asked if you had insight to this.
> I was thinking about adding an advanced_data_path boolean to the chip_info
> struct and providing different driver callbacks depending on the value.
I can't really comment on this much as I lack of knowledge. I have
understood the ADP can be configured to do very different type of
filtering(?) I wonder how the boolean flag suits these needs but I guess
we can go into those details when ADP support is being implemented.
Right now I am happy if you say you have analyzed the ADP to the point
you don't expect many kx132 specific dt-bindings to be needed. And. even
if this analysis was wrong, we can later separate the kx132 bindings to
own doc if that is needed. So, I am happy with the bindings being in
same file now.
> Something like in the bmc150-accel-core: iio_info for bmc150_accel_info
> and iio_info for bmc150_accel_info_fifo
I will see those later, thanks.
Yours
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer
2023-03-21 16:34 ` Mehdi Djait
@ 2023-03-25 18:12 ` Jonathan Cameron
0 siblings, 0 replies; 30+ messages in thread
From: Jonathan Cameron @ 2023-03-25 18:12 UTC (permalink / raw)
To: Mehdi Djait
Cc: mazziesaccount, krzysztof.kozlowski+dt, andriy.shevchenko,
linux-iio, linux-kernel, devicetree
On Tue, 21 Mar 2023 17:34:15 +0100
Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> Hello Jonathan,
>
> On Sun, Mar 19, 2023 at 04:22:07PM +0000, Jonathan Cameron wrote:
> > On Fri, 17 Mar 2023 00:48:37 +0100
> > Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> >
> > > Add support for the basic accelerometer features such as getting the
> > > acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
> > > using the WMI IRQ).
> > >
> > > Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
> >
> > Nothing much specific to this patch, most changes will be as a result
> > of bringing this inline with the changes suggested for patch 2.
> >
> > thanks,
> >
> > Jonathan
> > >
> > > diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
> > > index 3bb40e9f5613..7e43bdb37156 100644
> > > --- a/drivers/iio/accel/kionix-kx022a.h
> > > +++ b/drivers/iio/accel/kionix-kx022a.h
> > > @@ -90,8 +90,61 @@
> > > #define KX022A_REG_SELF_TEST 0x60
> > > #define KX022A_MAX_REGISTER 0x60
> > >
> > > +
> >
> > Push these down into the c file.
>
> Do you mean all REG and MASK defines ?
> Even kx022a defines them in the h file, or am I misunderstanding your
> comment ?
Hmm. Generally we only put reg defines in a header if they
are accessed from multiple c files. Otherwise it's both noise and more
code that has to be parsed when compiling (even if it's all unused / ignored).
I'm fine with this patch set just continuing with local style given they
are already there, but if you fancy moving the existing ones down to the C file
as a precursor patch, then even better!
>
> >
> > > +#define KX132_REG_WHO 0x13
> > > +#define KX132_ID 0x3d
> > > +
> > > +#define KX132_FIFO_LENGTH 86
> > > +
> > > +#define KX132_REG_CNTL2 0x1c
> > > +#define KX132_REG_CNTL 0x1b
> > > +#define KX132_MASK_RES BIT(6)
> > > +#define KX132_GSEL_2 0x0
> > > +#define KX132_GSEL_4 BIT(3)
> > > +#define KX132_GSEL_8 BIT(4)
> > > +#define KX132_GSEL_16 GENMASK(4, 3)
> > > +
> > > +#define KX132_REG_INS2 0x17
> > > +#define KX132_MASK_INS2_WMI BIT(5)
> > > +
> > > +#define KX132_REG_XADP_L 0x02
> > > +#define KX132_REG_XOUT_L 0x08
> > > +#define KX132_REG_YOUT_L 0x0a
> > > +#define KX132_REG_ZOUT_L 0x0c
> > > +#define KX132_REG_COTR 0x12
> > > +#define KX132_REG_TSCP 0x14
> > > +#define KX132_REG_INT_REL 0x1a
> > > +
> > > +#define KX132_REG_ODCNTL 0x21
> > > +
> > > +#define KX132_REG_BTS_WUF_TH 0x4a
> > > +#define KX132_REG_MAN_WAKE 0x4d
> > > +
> > > +#define KX132_REG_BUF_CNTL1 0x5e
> > > +#define KX132_REG_BUF_CNTL2 0x5f
> > > +#define KX132_REG_BUF_STATUS_1 0x60
> > > +#define KX132_REG_BUF_STATUS_2 0x61
> > > +#define KX132_MASK_BUF_SMP_LVL GENMASK(9, 0)
> > > +#define KX132_REG_BUF_CLEAR 0x62
> > > +#define KX132_REG_BUF_READ 0x63
> > > +#define KX132_ODR_SHIFT 3
> > > +#define KX132_FIFO_MAX_WMI_TH 86
> > > +
> > > +#define KX132_REG_INC1 0x22
> > > +#define KX132_REG_INC5 0x26
> > > +#define KX132_REG_INC6 0x27
> > > +#define KX132_IPOL_LOW 0
> > > +#define KX132_IPOL_HIGH KX_MASK_IPOL
> > > +#define KX132_ITYP_PULSE KX_MASK_ITYP
> > > +
> > > +#define KX132_REG_INC4 0x25
> > > +
> > > +#define KX132_REG_SELF_TEST 0x5d
> > > +#define KX132_MAX_REGISTER 0x76
> > > +
> > > enum kx022a_device_type {
> > > KX022A,
> > > + KX132,
> > As mentioned in previous review, I think this would be neater
> > done by just exporting the chip_info structures directly rather than
> > putting them in an array.
>
> I gave the reason in a response to the previous review.
If you strongly prefer the enum indexing array that's fine, but
definitely don't use the enum for the data in the tables - that should
be the pointer to the particular element of the array.
>
> --
> Kind Regards
> Mehdi Djait
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2023-03-25 17:57 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-16 23:48 [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
2023-03-16 23:48 ` [PATCH 1/3] dt-bindings: iio: Add " Mehdi Djait
2023-03-19 15:54 ` Jonathan Cameron
2023-03-21 13:22 ` Mehdi Djait
2023-03-16 23:48 ` [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure Mehdi Djait
2023-03-17 1:01 ` kernel test robot
2023-03-17 4:57 ` kernel test robot
2023-03-17 12:06 ` Andy Shevchenko
2023-03-18 16:12 ` Mehdi Djait
2023-03-19 16:20 ` Jonathan Cameron
2023-03-20 7:17 ` Matti Vaittinen
2023-03-20 12:24 ` Jonathan Cameron
2023-03-21 15:39 ` Mehdi Djait
2023-03-20 9:35 ` Matti Vaittinen
2023-03-20 12:02 ` Andy Shevchenko
2023-03-20 12:34 ` Jonathan Cameron
2023-03-20 12:52 ` Matti Vaittinen
2023-03-21 15:56 ` Mehdi Djait
2023-03-22 6:37 ` Matti Vaittinen
2023-03-21 1:05 ` kernel test robot
2023-03-16 23:48 ` [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
2023-03-19 16:22 ` Jonathan Cameron
2023-03-21 16:34 ` Mehdi Djait
2023-03-25 18:12 ` Jonathan Cameron
2023-03-20 9:57 ` Matti Vaittinen
2023-03-17 12:07 ` [PATCH 0/3] " Andy Shevchenko
2023-03-18 15:55 ` Mehdi Djait
2023-03-19 7:43 ` Matti Vaittinen
2023-03-21 13:16 ` Mehdi Djait
2023-03-22 7:47 ` Matti Vaittinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).