Hello everyone,
Version 3 for adding support for the kx132-1211 accelerometer
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 buffer: 8 bits for KX022A vs 10 bits for
KX132-1211.
Changes in v3:
- added two new patches by separating the addition of the
i2c_device_id table and the removal of blank lines from other
unrelated changes
- fixes a warning detected by the kernel test robot
- made all the changes related the chip_info in one patch
Changes in v2:
- added a new patch for warning when the device_id match fails in the
probe function
- added a new patch for the function that retrieves the number of bytes
in the buffer
- added a change to the Kconfig file in the patch adding the support
for the kx132-1211
- various fixes and modifications listed under each patch
Mehdi Djait (7):
dt-bindings: iio: Add KX132-1211 accelerometer
iio: accel: kionix-kx022a: Remove blank lines
iio: accel: kionix-kx022a: Warn on failed matches and assume
compatibility
iio: accel: kionix-kx022a: Add an i2c_device_id table
iio: accel: kionix-kx022a: Refactor driver and add chip_info structure
iio: accel: kionix-kx022a: Add a function to retrieve number of bytes
in buffer
iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer
.../bindings/iio/accel/kionix,kx022a.yaml | 12 +-
drivers/iio/accel/Kconfig | 8 +-
drivers/iio/accel/kionix-kx022a-i2c.c | 22 +-
drivers/iio/accel/kionix-kx022a-spi.c | 17 +-
drivers/iio/accel/kionix-kx022a.c | 295 ++++++++++++++----
drivers/iio/accel/kionix-kx022a.h | 110 ++++++-
6 files changed, 391 insertions(+), 73 deletions(-)
--
2.30.2
Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support
ranges from ±2G to ±16G, digital output through I²C/SPI.
Add support for basic accelerometer features such as reading acceleration
via IIO using raw reads, triggered buffer (data-ready), or the WMI IRQ.
Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
Signed-off-by: Mehdi Djait <[email protected]>
---
drivers/iio/accel/Kconfig | 8 +-
drivers/iio/accel/kionix-kx022a-i2c.c | 2 +
drivers/iio/accel/kionix-kx022a-spi.c | 2 +
drivers/iio/accel/kionix-kx022a.c | 147 ++++++++++++++++++++++++++
drivers/iio/accel/kionix-kx022a.h | 52 +++++++++
5 files changed, 207 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index b6b45d359f28..d8cc6e6f2bb9 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -418,8 +418,8 @@ config IIO_KX022A_SPI
select IIO_KX022A
select REGMAP_SPI
help
- Enable support for the Kionix KX022A digital tri-axis
- accelerometer connected to I2C interface.
+ Enable support for the Kionix KX022A, KX132-1211 digital tri-axis
+ accelerometers connected to SPI interface.
config IIO_KX022A_I2C
tristate "Kionix KX022A tri-axis digital accelerometer I2C interface"
@@ -427,8 +427,8 @@ config IIO_KX022A_I2C
select IIO_KX022A
select REGMAP_I2C
help
- Enable support for the Kionix KX022A digital tri-axis
- accelerometer connected to I2C interface.
+ Enable support for the Kionix KX022A, KX132-1211 digital tri-axis
+ accelerometers connected to I2C interface.
config KXSD9
tristate "Kionix KXSD9 Accelerometer Driver"
diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
index ce299d0446f7..4ea28d2482ec 100644
--- a/drivers/iio/accel/kionix-kx022a-i2c.c
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -39,12 +39,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)&kx022a_chip_info },
+ { .name = "kx132-1211", .driver_data = (kernel_ulong_t)&kx132_chip_info },
{ }
};
MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
static const struct of_device_id kx022a_of_match[] = {
{ .compatible = "kionix,kx022a", .data = &kx022a_chip_info },
+ { .compatible = "kionix,kx132-1211", .data = &kx132_chip_info },
{ }
};
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 b84503e24510..b755b2b395ed 100644
--- a/drivers/iio/accel/kionix-kx022a-spi.c
+++ b/drivers/iio/accel/kionix-kx022a-spi.c
@@ -39,12 +39,14 @@ static int kx022a_spi_probe(struct spi_device *spi)
static const struct spi_device_id kx022a_id[] = {
{ .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info },
+ { .name = "kx132-1211", .driver_data = (kernel_ulong_t)&kx132_chip_info },
{ }
};
MODULE_DEVICE_TABLE(spi, kx022a_id);
static const struct of_device_id kx022a_of_match[] = {
{ .compatible = "kionix,kx022a", .data = &kx022a_chip_info },
+ { .compatible = "kionix,kx132-1211", .data = &kx132_chip_info },
{ }
};
MODULE_DEVICE_TABLE(of, kx022a_of_match);
diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index 4a31d17c1f22..a6808ab12162 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -150,6 +150,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,
+};
+
struct kx022a_data {
const struct kx022a_chip_info *chip_info;
struct regmap *regmap;
@@ -237,6 +332,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, KX132_REG_XOUT_L, 0),
+ KX022A_ACCEL_CHAN(Y, KX132_REG_YOUT_L, 1),
+ KX022A_ACCEL_CHAN(Z, KX132_REG_ZOUT_L, 2),
+ 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
@@ -613,6 +715,25 @@ static int kx022a_get_fifo_bytes(struct kx022a_data *data)
return fifo_bytes;
}
+static int kx132_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;
+ }
+
+ fifo_bytes = le16_to_cpu(buf_status);
+ fifo_bytes &= data->chip_info->buf_smp_lvl_mask;
+
+ return fifo_bytes;
+}
+
static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
bool irq)
{
@@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
};
EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
+const struct kx022a_chip_info kx132_chip_info = {
+ .name = "kx132-1211",
+ .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,
+ .get_fifo_bytes = kx132_get_fifo_bytes,
+};
+EXPORT_SYMBOL_NS_GPL(kx132_chip_info, IIO_KX022A);
+
int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
{
static const char * const regulator_names[] = {"io-vdd", "vdd"};
diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
index f043767067b7..1f4135cf20eb 100644
--- a/drivers/iio/accel/kionix-kx022a.h
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -74,6 +74,57 @@
#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_CNTL 0x1b
+#define KX132_REG_CNTL2 0x1c
+#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 KX022A_MASK_IPOL
+#define KX132_ITYP_PULSE KX022A_MASK_ITYP
+
+#define KX132_REG_INC4 0x25
+
+#define KX132_REG_SELF_TEST 0x5d
+#define KX132_MAX_REGISTER 0x76
+
struct device;
struct kx022a_data;
@@ -132,5 +183,6 @@ struct kx022a_chip_info {
int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);
extern const struct kx022a_chip_info kx022a_chip_info;
+extern const struct kx022a_chip_info kx132_chip_info;
#endif
--
2.30.2
Extend the kionix,kx022a.yaml file to support the kx132-1211 device
Acked-by: Krzysztof Kozlowski <[email protected]>
Acked-by: Matti Vaittinen <[email protected]>
Signed-off-by: Mehdi Djait <[email protected]>
---
v3:
- no changes
v2:
- made the device name more specific from "kx132" to "kx132-1211"
- removed the output data-rates mentioned and replaced them with "variable
output data-rates"
.../devicetree/bindings/iio/accel/kionix,kx022a.yaml | 12 +++++++-----
1 file changed, 7 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..034b69614416 100644
--- a/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
@@ -4,19 +4,21 @@
$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-1211 Accelerometers
maintainers:
- Matti Vaittinen <[email protected]>
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-1211 are 3-axis accelerometers supporting +/- 2G, 4G, 8G and
+ 16G ranges, variable output data-rates and a hardware-fifo buffering.
+ KX022A and KX132-1211 can be accessed either via I2C or SPI.
properties:
compatible:
- const: kionix,kx022a
+ enum:
+ - kionix,kx022a
+ - kionix,kx132-1211
reg:
maxItems: 1
--
2.30.2
Avoid error returns on a failure to match and instead just warn with
assumption that we have a correct dt-binding telling us that
some new device with a different ID is backwards compatible.
Acked-by: Matti Vaittinen <[email protected]>
Signed-off-by: Mehdi Djait <[email protected]>
---
v3:
- changed from 'unsupported' to 'unknown'
- removed the opening bracket
v2:
- no changes, this patch is introduced in the v2
drivers/iio/accel/kionix-kx022a.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index ff8aa7b9568e..494e81ba1da9 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -1036,10 +1036,8 @@ int kx022a_probe_internal(struct device *dev)
if (ret)
return dev_err_probe(dev, ret, "Failed to access sensor\n");
- if (chip_id != KX022A_ID) {
- dev_err(dev, "unsupported device 0x%x\n", chip_id);
- return -EINVAL;
- }
+ if (chip_id != KX022A_ID)
+ dev_warn(dev, "unknown device 0x%x\n", chip_id);
irq = fwnode_irq_get_byname(fwnode, "INT1");
if (irq > 0) {
--
2.30.2
Remove blank lines pointed out by the checkpatch script
Signed-off-by: Mehdi Djait <[email protected]>
---
v3:
- no changes, this patch is introduced in the v2
drivers/iio/accel/kionix-kx022a.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index f98393d74666..ff8aa7b9568e 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -341,7 +341,6 @@ static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
dev_err(data->dev, "Turn %s fail %d\n", str_on_off(on), ret);
return ret;
-
}
static int kx022a_turn_off_lock(struct kx022a_data *data)
@@ -1121,7 +1120,6 @@ int kx022a_probe_internal(struct device *dev)
if (ret)
return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
-
ret = devm_iio_trigger_register(dev, indio_trig);
if (ret)
return dev_err_probe(data->dev, ret,
--
2.30.2
On 4/25/23 01:22, Mehdi Djait wrote:
> Remove blank lines pointed out by the checkpatch script
>
> Signed-off-by: Mehdi Djait <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
> ---
> v3:
> - no changes, this patch is introduced in the v2
>
> drivers/iio/accel/kionix-kx022a.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index f98393d74666..ff8aa7b9568e 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -341,7 +341,6 @@ static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
> dev_err(data->dev, "Turn %s fail %d\n", str_on_off(on), ret);
>
> return ret;
> -
> }
>
> static int kx022a_turn_off_lock(struct kx022a_data *data)
> @@ -1121,7 +1120,6 @@ int kx022a_probe_internal(struct device *dev)
> if (ret)
> return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
>
> -
> ret = devm_iio_trigger_register(dev, indio_trig);
> if (ret)
> return dev_err_probe(data->dev, ret,
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 4/25/23 01:22, Mehdi Djait wrote:
> Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support
> ranges from ±2G to ±16G, digital output through I²C/SPI.
> Add support for basic accelerometer features such as reading acceleration
> via IIO using raw reads, triggered buffer (data-ready), or the WMI IRQ.
>
> Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> Signed-off-by: Mehdi Djait <[email protected]>
> ---
> drivers/iio/accel/Kconfig | 8 +-
> drivers/iio/accel/kionix-kx022a-i2c.c | 2 +
> drivers/iio/accel/kionix-kx022a-spi.c | 2 +
> drivers/iio/accel/kionix-kx022a.c | 147 ++++++++++++++++++++++++++
> drivers/iio/accel/kionix-kx022a.h | 52 +++++++++
> 5 files changed, 207 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index b6b45d359f28..d8cc6e6f2bb9 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -418,8 +418,8 @@ config IIO_KX022A_SPI
> select IIO_KX022A
> select REGMAP_SPI
> help
> - Enable support for the Kionix KX022A digital tri-axis
> - accelerometer connected to I2C interface.
> + Enable support for the Kionix KX022A, KX132-1211 digital tri-axis
> + accelerometers connected to SPI interface.
>
> config IIO_KX022A_I2C
> tristate "Kionix KX022A tri-axis digital accelerometer I2C interface"
> @@ -427,8 +427,8 @@ config IIO_KX022A_I2C
> select IIO_KX022A
> select REGMAP_I2C
> help
> - Enable support for the Kionix KX022A digital tri-axis
> - accelerometer connected to I2C interface.
> + Enable support for the Kionix KX022A, KX132-1211 digital tri-axis
> + accelerometers connected to I2C interface.
>
> config KXSD9
> tristate "Kionix KXSD9 Accelerometer Driver"
> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> index ce299d0446f7..4ea28d2482ec 100644
> --- a/drivers/iio/accel/kionix-kx022a-i2c.c
> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> @@ -39,12 +39,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)&kx022a_chip_info },
> + { .name = "kx132-1211", .driver_data = (kernel_ulong_t)&kx132_chip_info },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
>
> static const struct of_device_id kx022a_of_match[] = {
> { .compatible = "kionix,kx022a", .data = &kx022a_chip_info },
> + { .compatible = "kionix,kx132-1211", .data = &kx132_chip_info },
> { }
> };
> 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 b84503e24510..b755b2b395ed 100644
> --- a/drivers/iio/accel/kionix-kx022a-spi.c
> +++ b/drivers/iio/accel/kionix-kx022a-spi.c
> @@ -39,12 +39,14 @@ static int kx022a_spi_probe(struct spi_device *spi)
>
> static const struct spi_device_id kx022a_id[] = {
> { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info },
> + { .name = "kx132-1211", .driver_data = (kernel_ulong_t)&kx132_chip_info },
> { }
> };
> MODULE_DEVICE_TABLE(spi, kx022a_id);
>
> static const struct of_device_id kx022a_of_match[] = {
> { .compatible = "kionix,kx022a", .data = &kx022a_chip_info },
> + { .compatible = "kionix,kx132-1211", .data = &kx132_chip_info },
> { }
> };
> MODULE_DEVICE_TABLE(of, kx022a_of_match);
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index 4a31d17c1f22..a6808ab12162 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -150,6 +150,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,
> + },
> +};
Maybe the CNTL5 should also be added to volatile table as it has "self
clearing" bits? I didn't go through all the registers to see if there
are more.
> +
> +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,
> + },
> +};
Do you think adding the "Kionix reserved" registers to read-only ranges
would make things safer or is there a reason to keep them writable? I
think the data-sheet states these should not be written to.
> +
> +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,
Why the WUFC is write-only? Also, I don't think the KX022A_REG_MAN_WAKE
and KX132_REG_MAN_WAKE reflect same functionality. The naming of the
define may be slightly misleading.
> + }, {
> + .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,
> +};
> +
> struct kx022a_data {
> const struct kx022a_chip_info *chip_info;
> struct regmap *regmap;
> @@ -237,6 +332,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, KX132_REG_XOUT_L, 0),
> + KX022A_ACCEL_CHAN(Y, KX132_REG_YOUT_L, 1),
> + KX022A_ACCEL_CHAN(Z, KX132_REG_ZOUT_L, 2),
> + 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
> @@ -613,6 +715,25 @@ static int kx022a_get_fifo_bytes(struct kx022a_data *data)
> return fifo_bytes;
> }
>
> +static int kx132_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;
> + }
> +
> + fifo_bytes = le16_to_cpu(buf_status);
> + fifo_bytes &= data->chip_info->buf_smp_lvl_mask;
This is probably just my limitation but I've hard time thinking how this
works out on BE machines. It'd be much easier for me to understand this
if the data was handled as two u8 values and mask was applied before
endianes conversion. (Eg - untested pseudo code follows;
__le16 buf_status;
u8 *reg_data;
...
ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1,
&buf_status, sizeof(buf_status));
...
reg_data = (u8 *)&buf_status;
/* Clear the unused bits form 2.nd reg */
reg_data[1] = reg_data[i] & MASK_SMP_LVL_REG_HIGH_BITS;
/* Convert to CPU endianess */
fifo_bytes = le16_to_cpu(buf_status);
Well, others may have different view on this :)
> +
> + return fifo_bytes;
> +}
> +
> static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> bool irq)
> {
> @@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
> };
> EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
>
> +const struct kx022a_chip_info kx132_chip_info = {
> + .name = "kx132-1211",
> + .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,
> + .get_fifo_bytes = kx132_get_fifo_bytes,
> +};
> +EXPORT_SYMBOL_NS_GPL(kx132_chip_info, IIO_KX022A);
> +
> int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
> {
> static const char * const regulator_names[] = {"io-vdd", "vdd"};
> diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
> index f043767067b7..1f4135cf20eb 100644
> --- a/drivers/iio/accel/kionix-kx022a.h
> +++ b/drivers/iio/accel/kionix-kx022a.h
> @@ -74,6 +74,57 @@
> #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_CNTL 0x1b
> +#define KX132_REG_CNTL2 0x1c
> +#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
As mentioned elsewhere, I don't think this is functionally same as
KX022A_REG_MAN_WAKE.
> +
> +#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 KX022A_MASK_IPOL
> +#define KX132_ITYP_PULSE KX022A_MASK_ITYP
> +
> +#define KX132_REG_INC4 0x25
> +
> +#define KX132_REG_SELF_TEST 0x5d
> +#define KX132_MAX_REGISTER 0x76
> +
> struct device;
>
> struct kx022a_data;
> @@ -132,5 +183,6 @@ struct kx022a_chip_info {
> int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);
>
> extern const struct kx022a_chip_info kx022a_chip_info;
> +extern const struct kx022a_chip_info kx132_chip_info;
>
> #endif
Thanks for adding this support!
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Tue, 25 Apr 2023 00:22:27 +0200
Mehdi Djait <[email protected]> wrote:
> Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support
> ranges from ±2G to ±16G, digital output through I²C/SPI.
> Add support for basic accelerometer features such as reading acceleration
> via IIO using raw reads, triggered buffer (data-ready), or the WMI IRQ.
>
> Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> Signed-off-by: Mehdi Djait <[email protected]>
Two tiny things inline.
> +static int kx132_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;
> + }
> +
> + fifo_bytes = le16_to_cpu(buf_status);
> + fifo_bytes &= data->chip_info->buf_smp_lvl_mask;
Slight preference for FIELD_GET() as it saves me checking the mask includes
lowest bits.
> +
> + return fifo_bytes;
> +}
> +
> static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> bool irq)
> {
> @@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
> };
> EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
>
> +const struct kx022a_chip_info kx132_chip_info = {
> + .name = "kx132-1211",
> + .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,
There are some things in here (typically where the define isn't used
anywhere else) where I think it would be easier to follow if the
value was listed here. Masks and IDs for example.
> + .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,
> + .get_fifo_bytes = kx132_get_fifo_bytes,
> +};
> +EXPORT_SYMBOL_NS_GPL(kx132_chip_info, IIO_KX022A);
> +
> int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
> {
> static const char * const regulator_names[] = {"io-vdd", "vdd"};
> > +static int kx132_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;
> > + }
> > +
> > + fifo_bytes = le16_to_cpu(buf_status);
> > + fifo_bytes &= data->chip_info->buf_smp_lvl_mask;
>
> This is probably just my limitation but I've hard time thinking how this
> works out on BE machines. It'd be much easier for me to understand this
> if the data was handled as two u8 values and mask was applied before
> endianes conversion. (Eg - untested pseudo code follows;
>
> __le16 buf_status;
> u8 *reg_data;
>
> ...
>
> ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1,
> &buf_status, sizeof(buf_status));
> ...
>
> reg_data = (u8 *)&buf_status;
>
> /* Clear the unused bits form 2.nd reg */
> reg_data[1] = reg_data[i] & MASK_SMP_LVL_REG_HIGH_BITS;
>
> /* Convert to CPU endianess */
> fifo_bytes = le16_to_cpu(buf_status);
>
> Well, others may have different view on this :)
:)
I go the other way. It's less obvious to me that it is appropriate
to apply le16_to_cpu(buf_status) after applying a mask to some
bits. The moment that is appropriate, then we certainly hope a single
mask application is as well.
I think treating it as a 16 bit register is appropriate, in particular
as the field is described as SMP_LEV[9:0] on the datasheet
(of course there are datasheets that do that for unconnected sets of
bits so this doesn't always work ;)
Jonathan
Hello Jonathan,
On Mon, May 01, 2023 at 03:56:45PM +0100, Jonathan Cameron wrote:
> On Tue, 25 Apr 2023 00:22:27 +0200
> Mehdi Djait <[email protected]> wrote:
>
> > Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support
> > ranges from ?2G to ?16G, digital output through I?C/SPI.
> > Add support for basic accelerometer features such as reading acceleration
> > via IIO using raw reads, triggered buffer (data-ready), or the WMI IRQ.
> >
> > Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> > Signed-off-by: Mehdi Djait <[email protected]>
>
> Two tiny things inline.
>
> > +static int kx132_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;
> > + }
> > +
> > + fifo_bytes = le16_to_cpu(buf_status);
> > + fifo_bytes &= data->chip_info->buf_smp_lvl_mask;
>
> Slight preference for FIELD_GET() as it saves me checking the mask includes
> lowest bits.
This will mean I have the remove the chip_info member buf_smp_lvl_mask
and use KX132_MASK_BUF_SMP_LVL directly because otherwise the
__builtin_constant_p function will cause an error when building.
Check: https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h#L65
I can change it to FIELD_GET() if you want to.
>
>
> > +
> > + return fifo_bytes;
> > +}
> > +
> > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> > bool irq)
> > {
> > @@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
> > };
> > EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
> >
> > +const struct kx022a_chip_info kx132_chip_info = {
> > + .name = "kx132-1211",
> > + .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,
>
> There are some things in here (typically where the define isn't used
> anywhere else) where I think it would be easier to follow if the
> value was listed here. Masks and IDs for example.
>
After removing buf_smp_lvl_mask, which members will be easier to understand (besides id) ?
--
Kind Regards
Mehdi Djait
On Fri, 5 May 2023 20:11:33 +0200
Mehdi Djait <[email protected]> wrote:
> Hello Jonathan,
>
> On Mon, May 01, 2023 at 03:56:45PM +0100, Jonathan Cameron wrote:
> > On Tue, 25 Apr 2023 00:22:27 +0200
> > Mehdi Djait <[email protected]> wrote:
> >
> > > Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support
> > > ranges from ±2G to ±16G, digital output through I²C/SPI.
> > > Add support for basic accelerometer features such as reading acceleration
> > > via IIO using raw reads, triggered buffer (data-ready), or the WMI IRQ.
> > >
> > > Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> > > Signed-off-by: Mehdi Djait <[email protected]>
> >
> > Two tiny things inline.
> >
> > > +static int kx132_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;
> > > + }
> > > +
> > > + fifo_bytes = le16_to_cpu(buf_status);
> > > + fifo_bytes &= data->chip_info->buf_smp_lvl_mask;
> >
> > Slight preference for FIELD_GET() as it saves me checking the mask includes
> > lowest bits.
>
> This will mean I have the remove the chip_info member buf_smp_lvl_mask
> and use KX132_MASK_BUF_SMP_LVL directly because otherwise the
> __builtin_constant_p function will cause an error when building.
> Check: https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h#L65
>
> I can change it to FIELD_GET() if you want to.
Good point. You could use le16_get_bits() though which I'm fairly sure will take
a variable just fine.
>
> >
> >
> > > +
> > > + return fifo_bytes;
> > > +}
> > > +
> > > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> > > bool irq)
> > > {
> > > @@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
> > > };
> > > EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
> > >
> > > +const struct kx022a_chip_info kx132_chip_info = {
> > > + .name = "kx132-1211",
> > > + .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,
> >
> > There are some things in here (typically where the define isn't used
> > anywhere else) where I think it would be easier to follow if the
> > value was listed here. Masks and IDs for example.
> >
>
> After removing buf_smp_lvl_mask, which members will be easier to understand (besides id) ?
I haven't gone through them. Length seems an obvious one. It's a number not a magic value.
Register addresses might also be simpler if they aren't used elsewhere.
Not really about understanding just about a define that adds nothing if all we do is
assign it to a variable of the same name.
>
> --
> Kind Regards
> Mehdi Djait
Hello Jonathan,
On Sat, May 06, 2023 at 05:46:51PM +0100, Jonathan Cameron wrote:
> On Fri, 5 May 2023 20:11:33 +0200
> Mehdi Djait <[email protected]> wrote:
>
> > Hello Jonathan,
> >
> > On Mon, May 01, 2023 at 03:56:45PM +0100, Jonathan Cameron wrote:
> > > On Tue, 25 Apr 2023 00:22:27 +0200
> > > Mehdi Djait <[email protected]> wrote:
> > >
> > > > Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support
> > > > ranges from ?2G to ?16G, digital output through I?C/SPI.
> > > > Add support for basic accelerometer features such as reading acceleration
> > > > via IIO using raw reads, triggered buffer (data-ready), or the WMI IRQ.
> > > >
> > > > Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> > > > Signed-off-by: Mehdi Djait <[email protected]>
> > >
> > > Two tiny things inline.
> > >
> > > > +static int kx132_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;
> > > > + }
> > > > +
> > > > + fifo_bytes = le16_to_cpu(buf_status);
> > > > + fifo_bytes &= data->chip_info->buf_smp_lvl_mask;
> > >
> > > Slight preference for FIELD_GET() as it saves me checking the mask includes
> > > lowest bits.
> >
> > This will mean I have the remove the chip_info member buf_smp_lvl_mask
> > and use KX132_MASK_BUF_SMP_LVL directly because otherwise the
> > __builtin_constant_p function will cause an error when building.
> > Check: https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h#L65
> >
> > I can change it to FIELD_GET() if you want to.
>
> Good point. You could use le16_get_bits() though which I'm fairly sure will take
> a variable just fine.
>
I don't think it will work.
From the commit log introducing the {type}_get_bits to <linux/bitfield.h>
" Field specification must be a constant; __builtin_constant_p() doesn't
have to be true for it, but compiler must be able to evaluate it at
build time. If it cannot or if the value does not encode any bitfield,
the build will fail. "
Actually Geert Uytterhoeven tried to solve excatly this issue, but it
seems that the patch was not accepted.
Check: https://lore.kernel.org/linux-iio/3a54a6703879d10f08cf0275a2a69297ebd2b1d4.1637592133.git.geert+renesas@glider.be/
So which solution would be the best:
1. Use directly KX132_MASK_BUF_SMP_LVL, the only reason I was trying to
avoid this was to make this function generic enough for other chip
variants
2. Copy the field_get() definition from drivers/clk/at91 or from the commit
of Geert and use it in this driver
3. leave it as it is ?
4. another solution ?
> >
> > >
> > >
> > > > +
> > > > + return fifo_bytes;
> > > > +}
> > > > +
> > > > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> > > > bool irq)
> > > > {
> > > > @@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
> > > > };
> > > > EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
> > > >
> > > > +const struct kx022a_chip_info kx132_chip_info = {
> > > > + .name = "kx132-1211",
> > > > + .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,
> > >
> > > There are some things in here (typically where the define isn't used
> > > anywhere else) where I think it would be easier to follow if the
> > > value was listed here. Masks and IDs for example.
> > >
> >
> > After removing buf_smp_lvl_mask, which members will be easier to understand (besides id) ?
>
> I haven't gone through them. Length seems an obvious one. It's a number not a magic value.
> Register addresses might also be simpler if they aren't used elsewhere.
>
> Not really about understanding just about a define that adds nothing if all we do is
> assign it to a variable of the same name.
Do you have a strong opinion about this ?
I would really prefer to leave it like this, for the following reasons:
1. If I directly use values here, I have to do it also in the previous
patch where I introduce the chip_info for the kx022a -> this means I
will have defines in the h file which are not used at all -> the defines should
be deleted -> the patch will get unnecessarily bigger. I received
multiple comments about removing unnecessary changes and reducing of the
patch size when possible.
2. Consistency: having all the defines in one place really seems to be
better for understanding IMO. I find the mix of values and defines in
the chip-info a bit confusing, e.g., I will use the direct value for
KX132_REG_CNTL but not for KX132_REG_CNTL2 because KX132_REG_CNTL2 is
referenced in a regmap_range.
--
Kind Regards
Mehdi Djait
On Sun, 7 May 2023 16:56:55 +0200
Mehdi Djait <[email protected]> wrote:
> Hello Jonathan,
>
> On Sat, May 06, 2023 at 05:46:51PM +0100, Jonathan Cameron wrote:
> > On Fri, 5 May 2023 20:11:33 +0200
> > Mehdi Djait <[email protected]> wrote:
> >
> > > Hello Jonathan,
> > >
> > > On Mon, May 01, 2023 at 03:56:45PM +0100, Jonathan Cameron wrote:
> > > > On Tue, 25 Apr 2023 00:22:27 +0200
> > > > Mehdi Djait <[email protected]> wrote:
> > > >
> > > > > Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support
> > > > > ranges from ±2G to ±16G, digital output through I²C/SPI.
> > > > > Add support for basic accelerometer features such as reading acceleration
> > > > > via IIO using raw reads, triggered buffer (data-ready), or the WMI IRQ.
> > > > >
> > > > > Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> > > > > Signed-off-by: Mehdi Djait <[email protected]>
> > > >
> > > > Two tiny things inline.
> > > >
> > > > > +static int kx132_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;
> > > > > + }
> > > > > +
> > > > > + fifo_bytes = le16_to_cpu(buf_status);
> > > > > + fifo_bytes &= data->chip_info->buf_smp_lvl_mask;
> > > >
> > > > Slight preference for FIELD_GET() as it saves me checking the mask includes
> > > > lowest bits.
> > >
> > > This will mean I have the remove the chip_info member buf_smp_lvl_mask
> > > and use KX132_MASK_BUF_SMP_LVL directly because otherwise the
> > > __builtin_constant_p function will cause an error when building.
> > > Check: https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h#L65
> > >
> > > I can change it to FIELD_GET() if you want to.
> >
> > Good point. You could use le16_get_bits() though which I'm fairly sure will take
> > a variable just fine.
> >
>
> I don't think it will work.
>
> From the commit log introducing the {type}_get_bits to <linux/bitfield.h>
> " Field specification must be a constant; __builtin_constant_p() doesn't
> have to be true for it, but compiler must be able to evaluate it at
> build time. If it cannot or if the value does not encode any bitfield,
> the build will fail. "
>
> Actually Geert Uytterhoeven tried to solve excatly this issue, but it
> seems that the patch was not accepted.
> Check: https://lore.kernel.org/linux-iio/3a54a6703879d10f08cf0275a2a69297ebd2b1d4.1637592133.git.geert+renesas@glider.be/
>
>
> So which solution would be the best:
>
> 1. Use directly KX132_MASK_BUF_SMP_LVL, the only reason I was trying to
> avoid this was to make this function generic enough for other chip
> variants
>
> 2. Copy the field_get() definition from drivers/clk/at91 or from the commit
> of Geert and use it in this driver
>
> 3. leave it as it is ?
This fine. Sorry for the diversion to nowhere!
Jonathan
>
> 4. another solution ?
>
> > >
> > > >
> > > >
> > > > > +
> > > > > + return fifo_bytes;
> > > > > +}
> > > > > +
> > > > > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> > > > > bool irq)
> > > > > {
> > > > > @@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
> > > > > };
> > > > > EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
> > > > >
> > > > > +const struct kx022a_chip_info kx132_chip_info = {
> > > > > + .name = "kx132-1211",
> > > > > + .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,
> > > >
> > > > There are some things in here (typically where the define isn't used
> > > > anywhere else) where I think it would be easier to follow if the
> > > > value was listed here. Masks and IDs for example.
> > > >
> > >
> > > After removing buf_smp_lvl_mask, which members will be easier to understand (besides id) ?
> >
> > I haven't gone through them. Length seems an obvious one. It's a number not a magic value.
> > Register addresses might also be simpler if they aren't used elsewhere.
> >
> > Not really about understanding just about a define that adds nothing if all we do is
> > assign it to a variable of the same name.
>
> Do you have a strong opinion about this ?
>
> I would really prefer to leave it like this, for the following reasons:
>
> 1. If I directly use values here, I have to do it also in the previous
> patch where I introduce the chip_info for the kx022a -> this means I
> will have defines in the h file which are not used at all -> the defines should
> be deleted -> the patch will get unnecessarily bigger. I received
> multiple comments about removing unnecessary changes and reducing of the
> patch size when possible.
>
> 2. Consistency: having all the defines in one place really seems to be
> better for understanding IMO. I find the mix of values and defines in
> the chip-info a bit confusing, e.g., I will use the direct value for
> KX132_REG_CNTL but not for KX132_REG_CNTL2 because KX132_REG_CNTL2 is
> referenced in a regmap_range.
>
> --
> Kind Regards
> Mehdi Djait