* [PATCH v8 1/7] dt-bindings: iio: Add KX132-1211 accelerometer
2023-08-23 21:16 [PATCH v8 0/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer Mehdi Djait
@ 2023-08-23 21:16 ` Mehdi Djait
2023-08-23 21:16 ` [PATCH v8 2/7] iio: accel: kionix-kx022a: Remove blank lines Mehdi Djait
` (5 subsequent siblings)
6 siblings, 0 replies; 30+ messages in thread
From: Mehdi Djait @ 2023-08-23 21:16 UTC (permalink / raw)
To: jic23, mazziesaccount
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, robh+dt, lars,
linux-iio, linux-kernel, devicetree, Mehdi Djait,
Krzysztof Kozlowski
Extend the kionix,kx022a.yaml file to support the kx132-1211 device
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
v8:
v7:
v6:
v5:
v4:
v3:
- no changes
.../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 <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-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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v8 2/7] iio: accel: kionix-kx022a: Remove blank lines
2023-08-23 21:16 [PATCH v8 0/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer Mehdi Djait
2023-08-23 21:16 ` [PATCH v8 1/7] dt-bindings: iio: Add " Mehdi Djait
@ 2023-08-23 21:16 ` Mehdi Djait
2023-08-24 11:49 ` Andy Shevchenko
2023-08-23 21:16 ` [PATCH v8 3/7] iio: accel: kionix-kx022a: Warn on failed matches and assume compatibility Mehdi Djait
` (4 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Mehdi Djait @ 2023-08-23 21:16 UTC (permalink / raw)
To: jic23, mazziesaccount
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, robh+dt, lars,
linux-iio, linux-kernel, devicetree, Mehdi Djait
Remove blank lines pointed out by the checkpatch script
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
v8:
v7:
v6:
v5:
v4:
- no changes
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v8 2/7] iio: accel: kionix-kx022a: Remove blank lines
2023-08-23 21:16 ` [PATCH v8 2/7] iio: accel: kionix-kx022a: Remove blank lines Mehdi Djait
@ 2023-08-24 11:49 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2023-08-24 11:49 UTC (permalink / raw)
To: Mehdi Djait
Cc: jic23, mazziesaccount, krzysztof.kozlowski+dt, robh+dt, lars,
linux-iio, linux-kernel, devicetree
On Wed, Aug 23, 2023 at 11:16:36PM +0200, Mehdi Djait wrote:
> Remove blank lines pointed out by the checkpatch script
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v8 3/7] iio: accel: kionix-kx022a: Warn on failed matches and assume compatibility
2023-08-23 21:16 [PATCH v8 0/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer Mehdi Djait
2023-08-23 21:16 ` [PATCH v8 1/7] dt-bindings: iio: Add " Mehdi Djait
2023-08-23 21:16 ` [PATCH v8 2/7] iio: accel: kionix-kx022a: Remove blank lines Mehdi Djait
@ 2023-08-23 21:16 ` Mehdi Djait
2023-08-24 11:51 ` Andy Shevchenko
2023-08-23 21:16 ` [PATCH v8 4/7] iio: accel: kionix-kx022a: Add an i2c_device_id table Mehdi Djait
` (3 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Mehdi Djait @ 2023-08-23 21:16 UTC (permalink / raw)
To: jic23, mazziesaccount
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, robh+dt, lars,
linux-iio, linux-kernel, devicetree, Mehdi Djait
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 <mazziesaccount@gmail.com>
Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
v8:
v7:
v6:
v5:
v4:
- no changes
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v8 3/7] iio: accel: kionix-kx022a: Warn on failed matches and assume compatibility
2023-08-23 21:16 ` [PATCH v8 3/7] iio: accel: kionix-kx022a: Warn on failed matches and assume compatibility Mehdi Djait
@ 2023-08-24 11:51 ` Andy Shevchenko
2023-08-27 17:57 ` Jonathan Cameron
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-08-24 11:51 UTC (permalink / raw)
To: Mehdi Djait
Cc: jic23, mazziesaccount, krzysztof.kozlowski+dt, robh+dt, lars,
linux-iio, linux-kernel, devicetree
On Wed, Aug 23, 2023 at 11:16:37PM +0200, Mehdi Djait wrote:
> 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.
As we already discussed in the past I think this patch might be
not okay in case a hardware vendor decides to make incompatible
device with non-supported yet ID. It might be even harmful to
the hardware to program register without knowing that this is safe.
That said, I'm neither NAKing nor reviewing this and leave to
maintainers to decide.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v8 3/7] iio: accel: kionix-kx022a: Warn on failed matches and assume compatibility
2023-08-24 11:51 ` Andy Shevchenko
@ 2023-08-27 17:57 ` Jonathan Cameron
0 siblings, 0 replies; 30+ messages in thread
From: Jonathan Cameron @ 2023-08-27 17:57 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mehdi Djait, mazziesaccount, krzysztof.kozlowski+dt, robh+dt,
lars, linux-iio, linux-kernel, devicetree
On Thu, 24 Aug 2023 14:51:49 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Aug 23, 2023 at 11:16:37PM +0200, Mehdi Djait wrote:
> > 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.
>
> As we already discussed in the past I think this patch might be
> not okay in case a hardware vendor decides to make incompatible
> device with non-supported yet ID. It might be even harmful to
> the hardware to program register without knowing that this is safe.
That only matters if they 'claim' it is compatible. Otherwise the
driver never probes and we are fine.
DT maintainers view was that the job of the kernel was not to defend
against bugs in device tree firmwares or someone binding the driver
to the wrong device by hand.
>
> That said, I'm neither NAKing nor reviewing this and leave to
> maintainers to decide.
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v8 4/7] iio: accel: kionix-kx022a: Add an i2c_device_id table
2023-08-23 21:16 [PATCH v8 0/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer Mehdi Djait
` (2 preceding siblings ...)
2023-08-23 21:16 ` [PATCH v8 3/7] iio: accel: kionix-kx022a: Warn on failed matches and assume compatibility Mehdi Djait
@ 2023-08-23 21:16 ` Mehdi Djait
2023-08-24 11:52 ` Andy Shevchenko
2023-08-23 21:16 ` [PATCH v8 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure Mehdi Djait
` (2 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Mehdi Djait @ 2023-08-23 21:16 UTC (permalink / raw)
To: jic23, mazziesaccount
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, robh+dt, lars,
linux-iio, linux-kernel, devicetree, Mehdi Djait
Add the missing i2c device id.
Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
v8:
v7:
v6:
v5:
v4:
- no changes
v3:
- no changes, this patch is introduced in the v2
drivers/iio/accel/kionix-kx022a-i2c.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
index e6fd02d931b6..b5a85ce3a891 100644
--- a/drivers/iio/accel/kionix-kx022a-i2c.c
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -30,6 +30,12 @@ static int kx022a_i2c_probe(struct i2c_client *i2c)
return kx022a_probe_internal(dev);
}
+static const struct i2c_device_id kx022a_i2c_id[] = {
+ { .name = "kx022a" },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
+
static const struct of_device_id kx022a_of_match[] = {
{ .compatible = "kionix,kx022a", },
{ }
@@ -42,6 +48,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);
--
2.30.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v8 4/7] iio: accel: kionix-kx022a: Add an i2c_device_id table
2023-08-23 21:16 ` [PATCH v8 4/7] iio: accel: kionix-kx022a: Add an i2c_device_id table Mehdi Djait
@ 2023-08-24 11:52 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2023-08-24 11:52 UTC (permalink / raw)
To: Mehdi Djait
Cc: jic23, mazziesaccount, krzysztof.kozlowski+dt, robh+dt, lars,
linux-iio, linux-kernel, devicetree
On Wed, Aug 23, 2023 at 11:16:38PM +0200, Mehdi Djait wrote:
> Add the missing i2c device id.
This needs an explanation. Why do we need legacy table, what for?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v8 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure
2023-08-23 21:16 [PATCH v8 0/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer Mehdi Djait
` (3 preceding siblings ...)
2023-08-23 21:16 ` [PATCH v8 4/7] iio: accel: kionix-kx022a: Add an i2c_device_id table Mehdi Djait
@ 2023-08-23 21:16 ` Mehdi Djait
2023-08-24 11:55 ` Andy Shevchenko
2023-08-23 21:16 ` [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer Mehdi Djait
2023-08-23 21:16 ` [PATCH v8 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer Mehdi Djait
6 siblings, 1 reply; 30+ messages in thread
From: Mehdi Djait @ 2023-08-23 21:16 UTC (permalink / raw)
To: jic23, mazziesaccount
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, robh+dt, lars,
linux-iio, linux-kernel, devicetree, Mehdi Djait
Add the chip_info structure to the driver's private data to hold all
the device specific infos.
Refactor the kx022a driver implementation to make it more generic and
extensible.
Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
v8:
- replaced min_t() by min() and kmalloc() by kmalloc_array()
v7:
- no changes
v6:
- placed chip_info after the regmap elemnt in kx022a_data struct to save
memory as suggested by Andy
- added a check for the availability of chip_info for the SPI case as
suggested by Andy
v5:
- moved the "kfree" call to match the reverse of what happens in
kx022a_fifo_enable() as suggested by Matti and Jonathan
- used min_t, checked for availability of chip_info as suggested by Andy
v4:
- allocating and freeing the buffer moved to the kx022a_fifo{enable,
disable} functions
- used the spi_get_device_match_data helper function
v3:
- added the change of the buffer's allocation in the __kx022a_fifo_flush
to this patch
- added the chip_info to the struct kx022a_data
drivers/iio/accel/kionix-kx022a-i2c.c | 18 +++-
drivers/iio/accel/kionix-kx022a-spi.c | 13 ++-
drivers/iio/accel/kionix-kx022a.c | 117 +++++++++++++++++---------
drivers/iio/accel/kionix-kx022a.h | 52 +++++++++++-
4 files changed, 148 insertions(+), 52 deletions(-)
diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
index b5a85ce3a891..006ffb51d3e6 100644
--- a/drivers/iio/accel/kionix-kx022a-i2c.c
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -15,6 +15,7 @@
static int kx022a_i2c_probe(struct i2c_client *i2c)
{
struct device *dev = &i2c->dev;
+ const struct kx022a_chip_info *chip_info;
struct regmap *regmap;
if (!i2c->irq) {
@@ -22,22 +23,31 @@ static int kx022a_i2c_probe(struct i2c_client *i2c)
return -EINVAL;
}
- regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
+ chip_info = device_get_match_data(&i2c->dev);
+ if (!chip_info) {
+ const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
+
+ chip_info = (const struct kx022a_chip_info *)id->driver_data;
+ if (!chip_info)
+ return -EINVAL;
+ }
+
+ 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" },
+ { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info },
{ }
};
MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
static const struct of_device_id kx022a_of_match[] = {
- { .compatible = "kionix,kx022a", },
+ { .compatible = "kionix,kx022a", .data = &kx022a_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 9cd047f7b346..896b57866fc9 100644
--- a/drivers/iio/accel/kionix-kx022a-spi.c
+++ b/drivers/iio/accel/kionix-kx022a-spi.c
@@ -15,6 +15,7 @@
static int kx022a_spi_probe(struct spi_device *spi)
{
struct device *dev = &spi->dev;
+ const struct kx022a_chip_info *chip_info;
struct regmap *regmap;
if (!spi->irq) {
@@ -22,22 +23,26 @@ static int kx022a_spi_probe(struct spi_device *spi)
return -EINVAL;
}
- regmap = devm_regmap_init_spi(spi, &kx022a_regmap);
+ chip_info = spi_get_device_match_data(spi);
+ if (!chip_info)
+ return -EINVAL;
+
+ 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" },
+ { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info },
{ }
};
MODULE_DEVICE_TABLE(spi, kx022a_id);
static const struct of_device_id kx022a_of_match[] = {
- { .compatible = "kionix,kx022a", },
+ { .compatible = "kionix,kx022a", .data = &kx022a_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 494e81ba1da9..6bac618c63b4 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -48,7 +48,7 @@ enum {
KX022A_STATE_FIFO,
};
-/* Regmap configs */
+/* kx022a Regmap configs */
static const struct regmap_range kx022a_volatile_ranges[] = {
{
.range_min = KX022A_REG_XHP_L,
@@ -138,7 +138,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,10 +149,10 @@ 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;
+ const struct kx022a_chip_info *chip_info;
struct iio_trigger *trig;
struct device *dev;
struct iio_mount_matrix orientation;
@@ -175,6 +175,8 @@ struct kx022a_data {
struct mutex mutex;
u8 watermark;
+ __le16 *fifo_buffer;
+
/* 3 x 16bit accel data + timestamp */
__le16 buffer[8] __aligned(IIO_DMA_MINALIGN);
struct {
@@ -208,7 +210,7 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
{ }
};
-#define KX022A_ACCEL_CHAN(axis, index) \
+#define KX022A_ACCEL_CHAN(axis, reg, index) \
{ \
.type = IIO_ACCEL, \
.modified = 1, \
@@ -220,7 +222,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 = reg, \
.scan_index = index, \
.scan_type = { \
.sign = 's', \
@@ -231,9 +233,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, KX022A_REG_XOUT_L, 0),
+ KX022A_ACCEL_CHAN(Y, KX022A_REG_YOUT_L, 1),
+ KX022A_ACCEL_CHAN(Z, KX022A_REG_ZOUT_L, 2),
IIO_CHAN_SOFT_TIMESTAMP(3),
};
@@ -332,10 +334,10 @@ 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,
+ ret = regmap_set_bits(data->regmap, data->chip_info->cntl,
KX022A_MASK_PC1);
else
- ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
+ ret = regmap_clear_bits(data->regmap, data->chip_info->cntl,
KX022A_MASK_PC1);
if (ret)
dev_err(data->dev, "Turn %s fail %d\n", str_on_off(on), ret);
@@ -402,7 +404,7 @@ static int kx022a_write_raw(struct iio_dev *idev,
break;
ret = regmap_update_bits(data->regmap,
- KX022A_REG_ODCNTL,
+ data->chip_info->odcntl,
KX022A_MASK_ODR, n);
data->odr_ns = kx022a_odrs[n];
kx022a_turn_on_unlock(data);
@@ -423,7 +425,7 @@ static int kx022a_write_raw(struct iio_dev *idev,
if (ret)
break;
- ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
+ ret = regmap_update_bits(data->regmap, data->chip_info->cntl,
KX022A_MASK_GSEL,
n << KX022A_GSEL_SHIFT);
kx022a_turn_on_unlock(data);
@@ -445,7 +447,7 @@ static int kx022a_fifo_set_wmi(struct kx022a_data *data)
threshold = data->watermark;
- return regmap_update_bits(data->regmap, KX022A_REG_BUF_CNTL1,
+ return regmap_update_bits(data->regmap, data->chip_info->buf_cntl1,
KX022A_MASK_WM_TH, threshold);
}
@@ -488,7 +490,7 @@ 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;
@@ -503,7 +505,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;
@@ -530,8 +532,7 @@ 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;
+ val = min(data->chip_info->fifo_length, val);
mutex_lock(&data->mutex);
data->watermark = val;
@@ -592,7 +593,7 @@ 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,
@@ -600,7 +601,6 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
{
struct kx022a_data *data = iio_priv(idev);
struct device *dev = regmap_get_device(data->regmap);
- __le16 buffer[KX022A_FIFO_LENGTH * 3];
uint64_t sample_period;
int count, fifo_bytes;
bool renable = false;
@@ -679,13 +679,13 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
}
fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES;
- ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
- &buffer[0], fifo_bytes);
+ ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read,
+ data->fifo_buffer, fifo_bytes);
if (ret)
goto renable_out;
for (i = 0; i < count; i++) {
- __le16 *sam = &buffer[i * 3];
+ __le16 *sam = &data->fifo_buffer[i * 3];
__le16 *chs;
int bit;
@@ -732,10 +732,10 @@ 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,
+ return regmap_set_bits(data->regmap, data->chip_info->cntl,
KX022A_MASK_DRDY);
- return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
+ return regmap_clear_bits(data->regmap, data->chip_info->cntl,
KX022A_MASK_DRDY);
}
@@ -770,7 +770,7 @@ static int kx022a_fifo_disable(struct kx022a_data *data)
if (ret)
goto unlock_out;
- ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+ ret = regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
KX022A_MASK_BUF_EN);
if (ret)
goto unlock_out;
@@ -779,6 +779,8 @@ static int kx022a_fifo_disable(struct kx022a_data *data)
kx022a_drop_fifo_contents(data);
+ kfree(data->fifo_buffer);
+
return kx022a_turn_on_unlock(data);
unlock_out:
@@ -801,6 +803,12 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
{
int ret;
+ data->fifo_buffer = kmalloc_array(data->chip_info->fifo_length,
+ KX022A_FIFO_SAMPLES_SIZE_BYTES,
+ GFP_KERNEL);
+ if (!data->fifo_buffer)
+ return -ENOMEM;
+
ret = kx022a_turn_off_lock(data);
if (ret)
return ret;
@@ -811,7 +819,7 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
goto unlock_out;
/* Enable buffer */
- ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+ ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
KX022A_MASK_BUF_EN);
if (ret)
goto unlock_out;
@@ -857,7 +865,7 @@ 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,
+ ret = regmap_bulk_read(data->regmap, data->chip_info->xout_l, data->buffer,
KX022A_FIFO_SAMPLES_SIZE_BYTES);
if (ret < 0)
goto err_read;
@@ -905,7 +913,7 @@ static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
if (data->state & KX022A_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;
}
@@ -958,7 +966,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, KX022A_MASK_SRST);
if (ret)
return ret;
@@ -968,7 +976,7 @@ static int kx022a_chip_init(struct kx022a_data *data)
*/
msleep(1);
- ret = regmap_read_poll_timeout(data->regmap, KX022A_REG_CNTL2, val,
+ ret = regmap_read_poll_timeout(data->regmap, data->chip_info->cntl2, val,
!(val & KX022A_MASK_SRST),
KX022A_SOFT_RESET_WAIT_TIME_US,
KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US);
@@ -978,14 +986,14 @@ static int kx022a_chip_init(struct kx022a_data *data)
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,
+ ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
KX022A_MASK_BRES16);
if (ret) {
dev_err(data->dev, "Failed to set data resolution\n");
@@ -995,7 +1003,31 @@ static int kx022a_chip_init(struct kx022a_data *data)
return kx022a_prepare_irq_pin(data);
}
-int kx022a_probe_internal(struct device *dev)
+const struct kx022a_chip_info kx022a_chip_info = {
+ .name = "kx022-accel",
+ .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_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(kx022a_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"};
struct iio_trigger *indio_trig;
@@ -1022,6 +1054,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
@@ -1032,24 +1065,24 @@ 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, 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 != chip_info->id)
dev_warn(dev, "unknown device 0x%x\n", chip_id);
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 = chip_info->inc1;
+ data->ien_reg = 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 = chip_info->inc5;
+ data->ien_reg = chip_info->inc6;
}
data->regmap = regmap;
@@ -1058,9 +1091,9 @@ int kx022a_probe_internal(struct device *dev)
data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
mutex_init(&data->mutex);
- idev->channels = kx022a_channels;
- idev->num_channels = ARRAY_SIZE(kx022a_channels);
- idev->name = "kx022-accel";
+ idev->channels = chip_info->channels;
+ idev->num_channels = chip_info->num_channels;
+ idev->name = chip_info->name;
idev->info = &kx022a_info;
idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
idev->available_scan_masks = kx022a_scan_masks;
diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
index 12424649d438..0e5026019213 100644
--- a/drivers/iio/accel/kionix-kx022a.h
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -76,7 +76,55 @@
struct device;
-int kx022a_probe_internal(struct device *dev);
-extern const struct regmap_config kx022a_regmap;
+/**
+ * struct kx022a_chip_info - Kionix accelerometer chip specific information
+ *
+ * @name: name of the device
+ * @regmap_config: pointer to register map configuration
+ * @channels: pointer to iio_chan_spec array
+ * @num_channels: number of iio_chan_spec channels
+ * @fifo_length: number of 16-bit samples in a full buffer
+ * @who: WHO_AM_I register
+ * @id: WHO_AM_I register value
+ * @cntl: control register 1
+ * @cntl2: control register 2
+ * @odcntl: output data control register
+ * @buf_cntl1: buffer control register 1
+ * @buf_cntl2: buffer control register 2
+ * @buf_clear: buffer clear register
+ * @buf_status1: buffer status register 1
+ * @buf_read: buffer read register
+ * @inc1: interrupt control register 1
+ * @inc4: interrupt control register 4
+ * @inc5: interrupt control register 5
+ * @inc6: interrupt control register 6
+ * @xout_l: x-axis output least significant byte
+ */
+struct kx022a_chip_info {
+ const char *name;
+ 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;
+ u8 buf_read;
+ u8 inc1;
+ u8 inc4;
+ u8 inc5;
+ u8 inc6;
+ u8 xout_l;
+};
+
+int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);
+
+extern const struct kx022a_chip_info kx022a_chip_info;
#endif
--
2.30.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v8 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure
2023-08-23 21:16 ` [PATCH v8 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure Mehdi Djait
@ 2023-08-24 11:55 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2023-08-24 11:55 UTC (permalink / raw)
To: Mehdi Djait
Cc: jic23, mazziesaccount, krzysztof.kozlowski+dt, robh+dt, lars,
linux-iio, linux-kernel, devicetree
On Wed, Aug 23, 2023 at 11:16:39PM +0200, Mehdi Djait wrote:
> Add the chip_info structure to the driver's private data to hold all
> the device specific infos.
> Refactor the kx022a driver implementation to make it more generic and
> extensible.
...
> + chip_info = device_get_match_data(&i2c->dev);
> + if (!chip_info) {
> + const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
> +
> + chip_info = (const struct kx022a_chip_info *)id->driver_data;
> + if (!chip_info)
> + return -EINVAL;
> + }
There is a new API for that, please use it.
...
> +/* kx022a Regmap configs */
regmap
...
> irq = fwnode_irq_get_byname(fwnode, "INT2");
> if (irq <= 0)
= 0 case is fixed now, means should never appear here,
but it's a material for another, separate change.
> return dev_err_probe(dev, irq, "No suitable IRQ\n");
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-23 21:16 [PATCH v8 0/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer Mehdi Djait
` (4 preceding siblings ...)
2023-08-23 21:16 ` [PATCH v8 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure Mehdi Djait
@ 2023-08-23 21:16 ` Mehdi Djait
2023-08-24 11:58 ` Andy Shevchenko
2023-08-23 21:16 ` [PATCH v8 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer Mehdi Djait
6 siblings, 1 reply; 30+ messages in thread
From: Mehdi Djait @ 2023-08-23 21:16 UTC (permalink / raw)
To: jic23, mazziesaccount
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, robh+dt, lars,
linux-iio, linux-kernel, devicetree, Mehdi Djait
Since Kionix accelerometers use various numbers of bits to report data, a
device-specific function is required.
Implement the function as a callback in the device-specific chip_info structure
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
v8:
v7:
- no changes
v6:
- directly return KX022A_FIFO_MAX_BYTES as suggested by Andy
v5:
- no changes
v4:
- removed the comment about "bogus value from i2c"
- removed regmap_get_device(data->regmap); dev is present in the
driver's private data
v3:
- no changes
v2:
- separated this change from the chip_info introduction and made it a patch in v2
- changed the function from generic implementation for to device-specific one
- removed blank lines pointed out by checkpatch
- changed the allocation of the "buffer" array in __kx022a_fifo_flush
drivers/iio/accel/kionix-kx022a.c | 28 ++++++++++++++++++----------
drivers/iio/accel/kionix-kx022a.h | 4 ++++
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index 6bac618c63b4..458859ebc645 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -596,26 +596,33 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
}
+static int kx022a_get_fifo_bytes(struct kx022a_data *data)
+{
+ int ret, fifo_bytes;
+
+ ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
+ if (ret) {
+ dev_err(data->dev, "Error reading buffer status\n");
+ return ret;
+ }
+
+ if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
+ return KX022A_FIFO_MAX_BYTES;
+
+ return fifo_bytes;
+}
+
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);
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;
+ fifo_bytes = data->chip_info->get_fifo_bytes(data);
if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES)
dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
@@ -1024,6 +1031,7 @@ const struct kx022a_chip_info kx022a_chip_info = {
.inc5 = KX022A_REG_INC5,
.inc6 = KX022A_REG_INC6,
.xout_l = KX022A_REG_XOUT_L,
+ .get_fifo_bytes = kx022a_get_fifo_bytes,
};
EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
index 0e5026019213..c9f9aee7e597 100644
--- a/drivers/iio/accel/kionix-kx022a.h
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -76,6 +76,8 @@
struct device;
+struct kx022a_data;
+
/**
* struct kx022a_chip_info - Kionix accelerometer chip specific information
*
@@ -99,6 +101,7 @@ struct device;
* @inc5: interrupt control register 5
* @inc6: interrupt control register 6
* @xout_l: x-axis output least significant byte
+ * @get_fifo_bytes: function pointer to get number of bytes in the FIFO buffer
*/
struct kx022a_chip_info {
const char *name;
@@ -121,6 +124,7 @@ struct kx022a_chip_info {
u8 inc5;
u8 inc6;
u8 xout_l;
+ int (*get_fifo_bytes)(struct kx022a_data *);
};
int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);
--
2.30.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-23 21:16 ` [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer Mehdi Djait
@ 2023-08-24 11:58 ` Andy Shevchenko
2023-08-24 12:52 ` Matti Vaittinen
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-08-24 11:58 UTC (permalink / raw)
To: Mehdi Djait
Cc: jic23, mazziesaccount, krzysztof.kozlowski+dt, robh+dt, lars,
linux-iio, linux-kernel, devicetree
On Wed, Aug 23, 2023 at 11:16:40PM +0200, Mehdi Djait wrote:
> Since Kionix accelerometers use various numbers of bits to report data, a
> device-specific function is required.
> Implement the function as a callback in the device-specific chip_info structure
...
> + int ret, fifo_bytes;
> +
> + ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
> + if (ret) {
> + dev_err(data->dev, "Error reading buffer status\n");
> + return ret;
> + }
> +
> + if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
> + return KX022A_FIFO_MAX_BYTES;
> +
> + return fifo_bytes;
This will be called each time ->get_fifo_bytes() called.
Do you expect the fifo_bytes to be changed over times?
Shouldn't we simply cache the value?
...
> + fifo_bytes = data->chip_info->get_fifo_bytes(data);
>
Now this blank line becomes redundant.
> if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES)
> dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-24 11:58 ` Andy Shevchenko
@ 2023-08-24 12:52 ` Matti Vaittinen
2023-08-24 13:39 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Matti Vaittinen @ 2023-08-24 12:52 UTC (permalink / raw)
To: Andy Shevchenko, Mehdi Djait
Cc: jic23, krzysztof.kozlowski+dt, robh+dt, lars, linux-iio,
linux-kernel, devicetree
On 8/24/23 14:58, Andy Shevchenko wrote:
> On Wed, Aug 23, 2023 at 11:16:40PM +0200, Mehdi Djait wrote:
>> Since Kionix accelerometers use various numbers of bits to report data, a
>> device-specific function is required.
>> Implement the function as a callback in the device-specific chip_info structure
>
> ...
>
>> + int ret, fifo_bytes;
>> +
>> + ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
>> + if (ret) {
>> + dev_err(data->dev, "Error reading buffer status\n");
>> + return ret;
>> + }
>> +
>> + if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
>> + return KX022A_FIFO_MAX_BYTES;
>> +
>> + return fifo_bytes;
>
> This will be called each time ->get_fifo_bytes() called.
> Do you expect the fifo_bytes to be changed over times?
> Shouldn't we simply cache the value?
I think this value tells how many samples there currently is in the
FIFO. Caching it does not sound meaningful unless I am missing something.
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 v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-24 12:52 ` Matti Vaittinen
@ 2023-08-24 13:39 ` Andy Shevchenko
2023-08-24 13:44 ` Mehdi Djait
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-08-24 13:39 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Mehdi Djait, jic23, krzysztof.kozlowski+dt, robh+dt, lars,
linux-iio, linux-kernel, devicetree
On Thu, Aug 24, 2023 at 03:52:56PM +0300, Matti Vaittinen wrote:
> On 8/24/23 14:58, Andy Shevchenko wrote:
> > On Wed, Aug 23, 2023 at 11:16:40PM +0200, Mehdi Djait wrote:
...
> > > + int ret, fifo_bytes;
> > > +
> > > + ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
> > > + if (ret) {
> > > + dev_err(data->dev, "Error reading buffer status\n");
> > > + return ret;
> > > + }
> > > +
> > > + if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
> > > + return KX022A_FIFO_MAX_BYTES;
> > > +
> > > + return fifo_bytes;
> >
> > This will be called each time ->get_fifo_bytes() called.
> > Do you expect the fifo_bytes to be changed over times?
> > Shouldn't we simply cache the value?
>
> I think this value tells how many samples there currently is in the FIFO.
> Caching it does not sound meaningful unless I am missing something.
I see. I think my confusion can be easily cured by renaming the callback to
get_amount_bytes_in_fifo()
or
get_bytes_in_fifo()
or alike.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-24 13:39 ` Andy Shevchenko
@ 2023-08-24 13:44 ` Mehdi Djait
2023-08-24 13:47 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Mehdi Djait @ 2023-08-24 13:44 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Matti Vaittinen, jic23, krzysztof.kozlowski+dt, robh+dt, lars,
linux-iio, linux-kernel, devicetree
Hello Andy,
Thank you for the review.
On Thu, Aug 24, 2023 at 3:39 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Aug 24, 2023 at 03:52:56PM +0300, Matti Vaittinen wrote:
> > On 8/24/23 14:58, Andy Shevchenko wrote:
> > > On Wed, Aug 23, 2023 at 11:16:40PM +0200, Mehdi Djait wrote:
>
> ...
>
> > > > + int ret, fifo_bytes;
> > > > +
> > > > + ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
> > > > + if (ret) {
> > > > + dev_err(data->dev, "Error reading buffer status\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
> > > > + return KX022A_FIFO_MAX_BYTES;
> > > > +
> > > > + return fifo_bytes;
> > >
> > > This will be called each time ->get_fifo_bytes() called.
> > > Do you expect the fifo_bytes to be changed over times?
> > > Shouldn't we simply cache the value?
> >
> > I think this value tells how many samples there currently is in the FIFO.
> > Caching it does not sound meaningful unless I am missing something.
>
> I see. I think my confusion can be easily cured by renaming the callback to
>
> get_amount_bytes_in_fifo()
>
> or
>
> get_bytes_in_fifo()
>
> or alike.
or leave it as is. The function is documented:
@@ -99,6 +101,7 @@ struct device;
* @inc5: interrupt control register 5
* @inc6: interrupt control register 6
* @xout_l: x-axis output least significant byte
+ * @get_fifo_bytes: function pointer to get number of bytes in the FIFO buffer
*/
struct kx022a_chip_info {
const char *name;
@@ -121,6 +124,7 @@ struct kx022a_chip_info {
u8 inc5;
u8 inc6;
u8 xout_l;
+ int (*get_fifo_bytes)(struct kx022a_data *);
};
--
Kind Regards
Mehdi Djait
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-24 13:44 ` Mehdi Djait
@ 2023-08-24 13:47 ` Andy Shevchenko
2023-08-24 14:23 ` Mehdi Djait
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-08-24 13:47 UTC (permalink / raw)
To: Mehdi Djait
Cc: Matti Vaittinen, jic23, krzysztof.kozlowski+dt, robh+dt, lars,
linux-iio, linux-kernel, devicetree
On Thu, Aug 24, 2023 at 03:44:29PM +0200, Mehdi Djait wrote:
> On Thu, Aug 24, 2023 at 3:39 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Aug 24, 2023 at 03:52:56PM +0300, Matti Vaittinen wrote:
> > > On 8/24/23 14:58, Andy Shevchenko wrote:
> > > > On Wed, Aug 23, 2023 at 11:16:40PM +0200, Mehdi Djait wrote:
...
> > I see. I think my confusion can be easily cured by renaming the callback to
> >
> > get_amount_bytes_in_fifo()
> >
> > or
> >
> > get_bytes_in_fifo()
> >
> > or alike.
>
> or leave it as is. The function is documented:
> + * @get_fifo_bytes: function pointer to get number of bytes in the FIFO buffer
Do you find it unambiguous? I do not.
Still needs more words to explain if it's a capacity of FIFO or is it amount of
valid bytes for the current transfer or what?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-24 13:47 ` Andy Shevchenko
@ 2023-08-24 14:23 ` Mehdi Djait
2023-08-24 14:39 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Mehdi Djait @ 2023-08-24 14:23 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Matti Vaittinen, jic23, krzysztof.kozlowski+dt, robh+dt, lars,
linux-iio, linux-kernel, devicetree
On Thu, Aug 24, 2023 at 4:06 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Aug 24, 2023 at 03:44:29PM +0200, Mehdi Djait wrote:
> > On Thu, Aug 24, 2023 at 3:39 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Aug 24, 2023 at 03:52:56PM +0300, Matti Vaittinen wrote:
> > > > On 8/24/23 14:58, Andy Shevchenko wrote:
> > > > > On Wed, Aug 23, 2023 at 11:16:40PM +0200, Mehdi Djait wrote:
>
> ...
>
> > > I see. I think my confusion can be easily cured by renaming the callback to
> > >
> > > get_amount_bytes_in_fifo()
> > >
> > > or
> > >
> > > get_bytes_in_fifo()
> > >
> > > or alike.
> >
> > or leave it as is. The function is documented:
>
> > + * @get_fifo_bytes: function pointer to get number of bytes in the FIFO buffer
>
> Do you find it unambiguous? I do not.
>
> Still needs more words to explain if it's a capacity of FIFO or is it amount of
> valid bytes for the current transfer or what?
how about change the description to:
function pointer to get amount of acceleration data bytes currently
stored in the sensor's FIFO buffer
and change the function to "get_amount_bytes_in_fifo()"
--
Kind Regards
Mehdi Djait
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-24 14:23 ` Mehdi Djait
@ 2023-08-24 14:39 ` Andy Shevchenko
2023-08-27 18:09 ` Jonathan Cameron
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-08-24 14:39 UTC (permalink / raw)
To: Mehdi Djait
Cc: Matti Vaittinen, jic23, krzysztof.kozlowski+dt, robh+dt, lars,
linux-iio, linux-kernel, devicetree
On Thu, Aug 24, 2023 at 04:23:09PM +0200, Mehdi Djait wrote:
> On Thu, Aug 24, 2023 at 4:06 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Aug 24, 2023 at 03:44:29PM +0200, Mehdi Djait wrote:
> > > On Thu, Aug 24, 2023 at 3:39 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Thu, Aug 24, 2023 at 03:52:56PM +0300, Matti Vaittinen wrote:
> > > > > On 8/24/23 14:58, Andy Shevchenko wrote:
> > > > > > On Wed, Aug 23, 2023 at 11:16:40PM +0200, Mehdi Djait wrote:
...
> > > > I see. I think my confusion can be easily cured by renaming the callback to
> > > >
> > > > get_amount_bytes_in_fifo()
> > > >
> > > > or
> > > >
> > > > get_bytes_in_fifo()
> > > >
> > > > or alike.
> > >
> > > or leave it as is. The function is documented:
> >
> > > + * @get_fifo_bytes: function pointer to get number of bytes in the FIFO buffer
> >
> > Do you find it unambiguous? I do not.
> >
> > Still needs more words to explain if it's a capacity of FIFO or is it amount of
> > valid bytes for the current transfer or what?
>
> how about change the description to:
> function pointer to get amount of acceleration data bytes currently
> stored in the sensor's FIFO buffer
>
> and change the function to "get_amount_bytes_in_fifo()"
Sounds good to me, thank you!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-24 14:39 ` Andy Shevchenko
@ 2023-08-27 18:09 ` Jonathan Cameron
2023-08-28 6:24 ` Matti Vaittinen
0 siblings, 1 reply; 30+ messages in thread
From: Jonathan Cameron @ 2023-08-27 18:09 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mehdi Djait, Matti Vaittinen, krzysztof.kozlowski+dt, robh+dt,
lars, linux-iio, linux-kernel, devicetree
> > > > > I see. I think my confusion can be easily cured by renaming the callback to
> > > > >
> > > > > get_amount_bytes_in_fifo()
> > > > >
> > > > > or
> > > > >
> > > > > get_bytes_in_fifo()
> > > > >
> > > > > or alike.
> > > >
> > > > or leave it as is. The function is documented:
> > >
> > > > + * @get_fifo_bytes: function pointer to get number of bytes in the FIFO buffer
> > >
> > > Do you find it unambiguous? I do not.
> > >
> > > Still needs more words to explain if it's a capacity of FIFO or is it amount of
> > > valid bytes for the current transfer or what?
> >
> > how about change the description to:
> > function pointer to get amount of acceleration data bytes currently
> > stored in the sensor's FIFO buffer
> >
> > and change the function to "get_amount_bytes_in_fifo()"
>
> Sounds good to me, thank you!
>
Bikeshedding time ;)
I don't like "amount" in this - it ends up adding little meaning
and to me it is ugly English. It's making it clear that we are dealing
with some sort of count but that is already true of get_bytes_in_fifo()
So to my reading it adds nothing wrt to removing ambiguity.
get_number_of_bytes_in_fifo() flows better but also adds nothing over
get_bytes_in_fifo()
You could make it clear it is something that changes over time.
get_current_bytes_in_fifo()
Which at least implies it changes - though it doesn't rule out a weird
variable max size fifo.
get_fifo_bytes_available() might be the clearest option and is the one
I would prefer. It's still a little messy as it could mean
'number of bytes of data that haven't been used yet in the fifo and
are available for samples in the future'.
Sigh. Maybe least ambiguous is something longer like.
get_fifo_bytes_available_to_read()
get_fifo_bytes_available_out()
Honestly I don't care that much what you go with :)
Jonathan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-27 18:09 ` Jonathan Cameron
@ 2023-08-28 6:24 ` Matti Vaittinen
2023-08-28 10:53 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Matti Vaittinen @ 2023-08-28 6:24 UTC (permalink / raw)
To: Jonathan Cameron, Andy Shevchenko
Cc: Mehdi Djait, krzysztof.kozlowski+dt, robh+dt, lars, linux-iio,
linux-kernel, devicetree
On 8/27/23 21:09, Jonathan Cameron wrote:
>
>>>>>> I see. I think my confusion can be easily cured by renaming the callback to
>>>>>>
>>>>>> get_amount_bytes_in_fifo()
>>>>>>
>>>>>> or
>>>>>>
>>>>>> get_bytes_in_fifo()
>>>>>>
>>>>>> or alike.
>>>>>
>>>>> or leave it as is. The function is documented:
>>>>
>>>>> + * @get_fifo_bytes: function pointer to get number of bytes in the FIFO buffer
>>>>
>>>> Do you find it unambiguous? I do not.
>>>>
>>>> Still needs more words to explain if it's a capacity of FIFO or is it amount of
>>>> valid bytes for the current transfer or what?
>>>
>>> how about change the description to:
>>> function pointer to get amount of acceleration data bytes currently
>>> stored in the sensor's FIFO buffer
>>>
>>> and change the function to "get_amount_bytes_in_fifo()"
>>
>> Sounds good to me, thank you!
>>
> Bikeshedding time ;)
>
> I don't like "amount" in this - it ends up adding little meaning
> and to me it is ugly English. It's making it clear that we are dealing
> with some sort of count but that is already true of get_bytes_in_fifo()
> So to my reading it adds nothing wrt to removing ambiguity.
>
> get_number_of_bytes_in_fifo() flows better but also adds nothing over
> get_bytes_in_fifo()
>
> You could make it clear it is something that changes over time.
>
> get_current_bytes_in_fifo()
>
> Which at least implies it changes - though it doesn't rule out a weird
> variable max size fifo.
>
> get_fifo_bytes_available() might be the clearest option and is the one
> I would prefer. It's still a little messy as it could mean
> 'number of bytes of data that haven't been used yet in the fifo and
> are available for samples in the future'.
>
> Sigh. Maybe least ambiguous is something longer like.
>
> get_fifo_bytes_available_to_read()
> get_fifo_bytes_available_out()
>
> Honestly I don't care that much what you go with :)
If this was a democracy (which it isn't) - my vote would go for "leave
as it is" because the concept of a data collecting fifo where amount of
data acquired in FIFO is readable from a register is common enough. I
think that people who work on a driver like this should guess what this
is for. Besides, if anything more than looking at the code is needed,
then the plain guessing won't do and one needs anyway to open the
data-sheet.
From my perspective this series adds a nice value and is good to go.
Just my 10 cents though :)
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 v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-28 6:24 ` Matti Vaittinen
@ 2023-08-28 10:53 ` Andy Shevchenko
2023-08-29 6:33 ` Matti Vaittinen
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-08-28 10:53 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Jonathan Cameron, Mehdi Djait, krzysztof.kozlowski+dt, robh+dt,
lars, linux-iio, linux-kernel, devicetree
On Mon, Aug 28, 2023 at 09:24:25AM +0300, Matti Vaittinen wrote:
> On 8/27/23 21:09, Jonathan Cameron wrote:
...
> I think that people who work on a driver like this should guess what this is
> for.
_This_ is the result of what people always forgot to think about, i.e. newcomers.
What _if_ the newcomer starts with this code and already being puzzled enough on
what the heck the function does. With all ambiguity we rise the threshold for the
newcomers and make the kernel project not attractive to start with (besides the
C language which is already considered as mastodon among youngsters).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-28 10:53 ` Andy Shevchenko
@ 2023-08-29 6:33 ` Matti Vaittinen
2023-09-06 16:03 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Matti Vaittinen @ 2023-08-29 6:33 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Mehdi Djait, krzysztof.kozlowski+dt, robh+dt,
lars, linux-iio, linux-kernel, devicetree
On 8/28/23 13:53, Andy Shevchenko wrote:
> On Mon, Aug 28, 2023 at 09:24:25AM +0300, Matti Vaittinen wrote:
>> On 8/27/23 21:09, Jonathan Cameron wrote:
>
> ...
>
>> I think that people who work on a driver like this should guess what this is
>> for.
>
> _This_ is the result of what people always forgot to think about, i.e. newcomers.
Thanks Andy. This was a good heads-up for me. I do also see the need for
fresh blood here - we aren't getting any younger.
> What _if_ the newcomer starts with this code and already being puzzled enough on
> what the heck the function does. With all ambiguity we rise the threshold for the
> newcomers and make the kernel project not attractive to start with
I really appreciate you making a point about attracting newcomers (and
there is no sarcasm in this statement). I however don't think we're
rising the bar here. If a newcomer wants to work on a device-driver, the
_first_ thing to do is to be familiar with the device. Without prior
experience of this kind of devices it is really a must to get the
data-sheet and see how the device operates before jumping into reading
the code. I would say that after reading the fifo lvl description from
data-sheet this should be obvious - and no, I don't think we should
replicate the data-sheet documentation in the drivers for parts that
aren't very peculiar.
But the question how to attract newcomers to kernel is very valid and I
guess that not too many of us is thinking of it. Actually, I think we
should ask from the newcomers we have that what has been the most
repulsive part of the work when they have contributed.
(besides the
> C language which is already considered as mastodon among youngsters).
I think this is at least partially the truth. However, I think that in
many cases one of the issues goes beyond the language - many younger
generation people I know aren't really interested in _why_ things work,
they just want to get things working in any way they can - and nowadays
when you can find a tutorial for pretty much anything - one really can
just look up instruction about how a "foobar can be made to buzz"
instead of trying to figure out what makes a "foobar to buzz" in order
to make it to buzz. So, I don't blame people getting used to take a
different approach. (Not sure this makes sense - don't really know how
to express my thoughts about this in a clear way - besides, it may not
even matter).
Anyways, I am pretty sure that - as with any community - the way people
are treated and how their contribution is appreciated is the key to make
them feel good and like the work. I think that in some cases it may
include allowing new contributors to get their code merged when it has
reached "good enough" state - even if it was not perfect. (Sure, when
things are good enough is subject to greater minds than me to ponder) ;)
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 v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-08-29 6:33 ` Matti Vaittinen
@ 2023-09-06 16:03 ` Andy Shevchenko
2023-09-07 6:33 ` Matti Vaittinen
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-09-06 16:03 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Jonathan Cameron, Mehdi Djait, krzysztof.kozlowski+dt, robh+dt,
lars, linux-iio, linux-kernel, devicetree
On Tue, Aug 29, 2023 at 09:33:27AM +0300, Matti Vaittinen wrote:
> On 8/28/23 13:53, Andy Shevchenko wrote:
> > On Mon, Aug 28, 2023 at 09:24:25AM +0300, Matti Vaittinen wrote:
> > > On 8/27/23 21:09, Jonathan Cameron wrote:
Sorry it took a bit of time to reply on this.
...
> > > I think that people who work on a driver like this should guess what this is
> > > for.
> >
> > _This_ is the result of what people always forgot to think about, i.e. newcomers.
>
> Thanks Andy. This was a good heads-up for me. I do also see the need for
> fresh blood here - we aren't getting any younger.
>
> > What _if_ the newcomer starts with this code and already being puzzled enough on
> > what the heck the function does. With all ambiguity we rise the threshold for the
> > newcomers and make the kernel project not attractive to start with
>
> I really appreciate you making a point about attracting newcomers (and there
> is no sarcasm in this statement). I however don't think we're rising the bar
> here. If a newcomer wants to work on a device-driver, the _first_ thing to
> do is to be familiar with the device. Without prior experience of this kind
> of devices it is really a must to get the data-sheet and see how the device
> operates before jumping into reading the code. I would say that after
> reading the fifo lvl description from data-sheet this should be obvious -
> and no, I don't think we should replicate the data-sheet documentation in
> the drivers for parts that aren't very peculiar.
There are (at least?) two approaches on the contribution:
1) generic / library wise;
2) specific hardware wise.
You are talking about 2), while my remark is about both. I can imagine a newcomer
who possess a hardware that looks similar to what this driver is for. Now, they
would like to write a new driver (note, that compatibility can be checked by
reading the RTL definitions, so no need to dive into the code) and use this as
a (nice) reference. With that in mind, they can read a function named
get_fifo_bytes() with not so extensive documentation nor fully self-explanatory
name. One may mistakenly though about this as a function for something that
returns FIFO capacity, but in the reality it is current amount of valid / data
bytes in the FIFO for the ongoing communication with the device.
> But the question how to attract newcomers to kernel is very valid and I
> guess that not too many of us is thinking of it. Actually, I think we should
> ask from the newcomers we have that what has been the most repulsive part of
> the work when they have contributed.
> (besides the
> > C language which is already considered as mastodon among youngsters).
>
> I think this is at least partially the truth. However, I think that in many
> cases one of the issues goes beyond the language - many younger generation
> people I know aren't really interested in _why_ things work, they just want
> to get things working in any way they can - and nowadays when you can find a
> tutorial for pretty much anything - one really can just look up instruction
> about how a "foobar can be made to buzz" instead of trying to figure out
> what makes a "foobar to buzz" in order to make it to buzz. So, I don't blame
> people getting used to take a different approach. (Not sure this makes sense
> - don't really know how to express my thoughts about this in a clear way -
> besides, it may not even matter).
Yeah, I share your frustration and agree that people are loosing the feel of
curiosity. Brave New World in front of us...
> Anyways, I am pretty sure that - as with any community - the way people are
> treated and how their contribution is appreciated is the key to make them
> feel good and like the work. I think that in some cases it may include
> allowing new contributors to get their code merged when it has reached "good
> enough" state - even if it was not perfect. (Sure, when things are good
> enough is subject to greater minds than me to ponder) ;)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-09-06 16:03 ` Andy Shevchenko
@ 2023-09-07 6:33 ` Matti Vaittinen
2023-09-11 13:03 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Matti Vaittinen @ 2023-09-07 6:33 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Mehdi Djait, krzysztof.kozlowski+dt, robh+dt,
lars, linux-iio, linux-kernel, devicetree
On 9/6/23 19:03, Andy Shevchenko wrote:
> On Tue, Aug 29, 2023 at 09:33:27AM +0300, Matti Vaittinen wrote:
>> On 8/28/23 13:53, Andy Shevchenko wrote:
>>> On Mon, Aug 28, 2023 at 09:24:25AM +0300, Matti Vaittinen wrote:
>>>> On 8/27/23 21:09, Jonathan Cameron wrote:
>
> Sorry it took a bit of time to reply on this.
No problem. Autumn is approaching and darkness is falling in Finland...
So, at least I am really slowing down with everything... :|
> ...
>
>>>> I think that people who work on a driver like this should guess what this is
>>>> for.
>>>
>>> _This_ is the result of what people always forgot to think about, i.e. newcomers.
>>
>> Thanks Andy. This was a good heads-up for me. I do also see the need for
>> fresh blood here - we aren't getting any younger.
>>
>>> What _if_ the newcomer starts with this code and already being puzzled enough on
>>> what the heck the function does. With all ambiguity we rise the threshold for the
>>> newcomers and make the kernel project not attractive to start with
>>
>> I really appreciate you making a point about attracting newcomers (and there
>> is no sarcasm in this statement). I however don't think we're rising the bar
>> here. If a newcomer wants to work on a device-driver, the _first_ thing to
>> do is to be familiar with the device. Without prior experience of this kind
>> of devices it is really a must to get the data-sheet and see how the device
>> operates before jumping into reading the code. I would say that after
>> reading the fifo lvl description from data-sheet this should be obvious -
>> and no, I don't think we should replicate the data-sheet documentation in
>> the drivers for parts that aren't very peculiar.
>
> There are (at least?) two approaches on the contribution:
> 1) generic / library wise;
> 2) specific hardware wise.
>
> You are talking about 2), while my remark is about both. I can imagine a newcomer
> who possess a hardware that looks similar to what this driver is for.
Yes. I am talking about 2). And my stance is that device drivers belong
to category 2). If one works with a device driver for some HW, then
he/she needs to be willing to understand the hardware.
Now, they
> would like to write a new driver (note, that compatibility can be checked by
> reading the RTL definitions, so no need to dive into the code) and use this as
> a (nice) reference. With that in mind, they can read a function named
> get_fifo_bytes() with not so extensive documentation nor fully self-explanatory
> name. One may mistakenly though about this as a function for something that
> returns FIFO capacity, but in the reality it is current amount of valid / data
> bytes in the FIFO for the ongoing communication with the device.
I can't avoid having a feeling that this is a very unlikely scenario. I
am afraid that by requesting this type of improvements at patch series
which is at v8 and has been running for half an year (and which was of a
good quality to start with, especially knowing this was the author's
first driver) is going to be more repulsive to the newcomers than the
potential obfuscation.
I don't try claiming that no-one could ever hit this trap (even if I
don't see it likely). I still believe that if one does so, he/she will
also get such a bug fixed without being totally discouraged - it's
business as usual.
I hope this does not come out as rude. I do appreciate your reviews,
it's comforting to know someone looks my code with sharp eyes and points
out things like the dead code in BM1390 driver! I just like the words
Jonathan once spilled out:
"Don't let the perfect be enemy of good" (or something along those lines).
>> But the question how to attract newcomers to kernel is very valid and I
>> guess that not too many of us is thinking of it. Actually, I think we should
>> ask from the newcomers we have that what has been the most repulsive part of
>> the work when they have contributed.
>
>> (besides the
>>> C language which is already considered as mastodon among youngsters).
>>
>> I think this is at least partially the truth. However, I think that in many
>> cases one of the issues goes beyond the language - many younger generation
>> people I know aren't really interested in _why_ things work, they just want
>> to get things working in any way they can - and nowadays when you can find a
>> tutorial for pretty much anything - one really can just look up instruction
>> about how a "foobar can be made to buzz" instead of trying to figure out
>> what makes a "foobar to buzz" in order to make it to buzz. So, I don't blame
>> people getting used to take a different approach. (Not sure this makes sense
>> - don't really know how to express my thoughts about this in a clear way -
>> besides, it may not even matter).
>
> Yeah, I share your frustration and agree that people are loosing the feel of
> curiosity. Brave New World in front of us...
Well, who knows how things will be working out for the new generations?
Maybe they won't need the kernel in the future? Yes, I am stubbornly
hanging in the past practices and values. Direction things seem to head
do not always appeal to me - but perhaps it's just me? Who can say my
values and practices are the right ones for new generations :) My oldest
son just moved to his own home and I need to accept that young do build
their own lives on different values I had. And who knows, maybe the
approach of just doing things without knowing what exactly happens under
the hood makes this world very good for them?
But yes - I don't think it suits the kernel project at all :) This is a
project of dinosaurs like us XD
(DISCLAIMER: I don't know quite all young people in the world. Frankly
to tell, not even 90% XD So, I am not trying to say "all young people
are like this or that". I just have a feeling that certain way of
thinking is more common amongst certain generations - but maybe it's
just my misjudgement. Please, don't be offended).
>
>> Anyways, I am pretty sure that - as with any community - the way people are
>> treated and how their contribution is appreciated is the key to make them
>> feel good and like the work. I think that in some cases it may include
>> allowing new contributors to get their code merged when it has reached "good
>> enough" state - even if it was not perfect. (Sure, when things are good
>> enough is subject to greater minds than me to ponder) ;)
>
--
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 v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer
2023-09-07 6:33 ` Matti Vaittinen
@ 2023-09-11 13:03 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2023-09-11 13:03 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Jonathan Cameron, Mehdi Djait, krzysztof.kozlowski+dt, robh+dt,
lars, linux-iio, linux-kernel, devicetree
On Thu, Sep 07, 2023 at 09:33:47AM +0300, Matti Vaittinen wrote:
> On 9/6/23 19:03, Andy Shevchenko wrote:
> > On Tue, Aug 29, 2023 at 09:33:27AM +0300, Matti Vaittinen wrote:
> > > On 8/28/23 13:53, Andy Shevchenko wrote:
> > > > On Mon, Aug 28, 2023 at 09:24:25AM +0300, Matti Vaittinen wrote:
> > > > > On 8/27/23 21:09, Jonathan Cameron wrote:
...
> > > > > I think that people who work on a driver like this should guess what this is
> > > > > for.
> > > >
> > > > _This_ is the result of what people always forgot to think about, i.e.
> > > > newcomers.
> > >
> > > Thanks Andy. This was a good heads-up for me. I do also see the need for
> > > fresh blood here - we aren't getting any younger.
> > >
> > > > What _if_ the newcomer starts with this code and already being puzzled
> > > > enough on what the heck the function does. With all ambiguity we rise
> > > > the threshold for the newcomers and make the kernel project not
> > > > attractive to start with
> > >
> > > I really appreciate you making a point about attracting newcomers (and there
> > > is no sarcasm in this statement). I however don't think we're rising the bar
> > > here. If a newcomer wants to work on a device-driver, the _first_ thing to
> > > do is to be familiar with the device. Without prior experience of this kind
> > > of devices it is really a must to get the data-sheet and see how the device
> > > operates before jumping into reading the code. I would say that after
> > > reading the fifo lvl description from data-sheet this should be obvious -
> > > and no, I don't think we should replicate the data-sheet documentation in
> > > the drivers for parts that aren't very peculiar.
> >
> > There are (at least?) two approaches on the contribution:
> > 1) generic / library wise;
> > 2) specific hardware wise.
> >
> > You are talking about 2), while my remark is about both. I can imagine a
> > newcomer who possess a hardware that looks similar to what this driver is
> > for.
>
> Yes. I am talking about 2). And my stance is that device drivers belong to
> category 2). If one works with a device driver for some HW, then he/she
> needs to be willing to understand the hardware.
>
> > Now, they would like to write a new driver (note, that compatibility can be
> > checked by reading the RTL definitions, so no need to dive into the code)
> > and use this as a (nice) reference. With that in mind, they can read a
> > function named get_fifo_bytes() with not so extensive documentation nor
> > fully self-explanatory name. One may mistakenly though about this as a
> > function for something that returns FIFO capacity, but in the reality it is
> > current amount of valid / data bytes in the FIFO for the ongoing
> > communication with the device.
>
> I can't avoid having a feeling that this is a very unlikely scenario. I am
> afraid that by requesting this type of improvements at patch series which is
> at v8 and has been running for half an year (and which was of a good quality
> to start with, especially knowing this was the author's first driver) is
> going to be more repulsive to the newcomers than the potential obfuscation.
I agree and this is a side talk to the topic.
> I don't try claiming that no-one could ever hit this trap (even if I don't
> see it likely). I still believe that if one does so, he/she will also get
> such a bug fixed without being totally discouraged - it's business as usual.
>
> I hope this does not come out as rude. I do appreciate your reviews, it's
> comforting to know someone looks my code with sharp eyes and points out
> things like the dead code in BM1390 driver! I just like the words Jonathan
> once spilled out:
>
> "Don't let the perfect be enemy of good" (or something along those lines).
True.
> > > But the question how to attract newcomers to kernel is very valid and I
> > > guess that not too many of us is thinking of it. Actually, I think we should
> > > ask from the newcomers we have that what has been the most repulsive part of
> > > the work when they have contributed.
> >
> > > > (besides the C language which is already considered as mastodon among
> > > > youngsters).
> > >
> > > I think this is at least partially the truth. However, I think that in many
> > > cases one of the issues goes beyond the language - many younger generation
> > > people I know aren't really interested in _why_ things work, they just want
> > > to get things working in any way they can - and nowadays when you can find a
> > > tutorial for pretty much anything - one really can just look up instruction
> > > about how a "foobar can be made to buzz" instead of trying to figure out
> > > what makes a "foobar to buzz" in order to make it to buzz. So, I don't blame
> > > people getting used to take a different approach. (Not sure this makes sense
> > > - don't really know how to express my thoughts about this in a clear way -
> > > besides, it may not even matter).
> >
> > Yeah, I share your frustration and agree that people are loosing the feel of
> > curiosity. Brave New World in front of us...
>
> Well, who knows how things will be working out for the new generations?
> Maybe they won't need the kernel in the future? Yes, I am stubbornly hanging
> in the past practices and values. Direction things seem to head do not
> always appeal to me - but perhaps it's just me? Who can say my values and
> practices are the right ones for new generations :) My oldest son just moved
> to his own home and I need to accept that young do build their own lives on
> different values I had. And who knows, maybe the approach of just doing
> things without knowing what exactly happens under the hood makes this world
> very good for them?
>
> But yes - I don't think it suits the kernel project at all :) This is a
> project of dinosaurs like us XD
>
> (DISCLAIMER: I don't know quite all young people in the world. Frankly to
> tell, not even 90% XD So, I am not trying to say "all young people are like
> this or that".
Operating in terms of universal quantifier is always wrong (pun intended).
> I just have a feeling that certain way of thinking is more
> common amongst certain generations - but maybe it's just my misjudgement.
> Please, don't be offended).
>
> > > Anyways, I am pretty sure that - as with any community - the way people
> > > are treated and how their contribution is appreciated is the key to make
> > > them feel good and like the work. I think that in some cases it may
> > > include allowing new contributors to get their code merged when it has
> > > reached "good enough" state - even if it was not perfect. (Sure, when
> > > things are good enough is subject to greater minds than me to ponder) ;)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v8 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer
2023-08-23 21:16 [PATCH v8 0/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer Mehdi Djait
` (5 preceding siblings ...)
2023-08-23 21:16 ` [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer Mehdi Djait
@ 2023-08-23 21:16 ` Mehdi Djait
2023-08-24 12:02 ` Andy Shevchenko
6 siblings, 1 reply; 30+ messages in thread
From: Mehdi Djait @ 2023-08-23 21:16 UTC (permalink / raw)
To: jic23, mazziesaccount
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, robh+dt, lars,
linux-iio, linux-kernel, devicetree, Mehdi Djait
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
Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
v8:
- replaced min_t() by min()
v7:
- added a min_t in kx132_get_fifo_bytes to ensure that that the fifo_bytes is
never bigger than the fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES no matter
what we read from I2C as suggested by Matti
v6:
- no changes
v5:
- moved the position of u16 buf_smp_lvl_mask as suggested by Andy
- introduced buf_smp_lvl_mask in Patch 7 as suggested by Jonathan
v4:
- added KX132_REG_CNTL5 to the volatile ranges
- added the kionix reserved regs to the read_only ranges
- removed KX132_REG_MAN_WAKEUP from the write_only ranges
v3:
- fixed the warning of the kernel test robot in kx132_get_fifo_bytes
(invalid assignment: &=, left side has type restricted __le16
right side has type unsigned short)
v2:
- mentioned the kx132-1211 in the Kconfig
- added a kx132-specific get_fifo_bytes function
- changed the device name from "kx132" to "kx132-1211
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 | 164 ++++++++++++++++++++++++++
drivers/iio/accel/kionix-kx022a.h | 54 +++++++++
5 files changed, 226 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 006ffb51d3e6..286754dc5e97 100644
--- a/drivers/iio/accel/kionix-kx022a-i2c.c
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -42,12 +42,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 896b57866fc9..c7766492bcd9 100644
--- a/drivers/iio/accel/kionix-kx022a-spi.c
+++ b/drivers/iio/accel/kionix-kx022a-spi.c
@@ -37,12 +37,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 458859ebc645..b61a97325052 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -150,6 +150,117 @@ 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_CNTL5,
+ .range_max = KX132_REG_CNTL5,
+ }, {
+ .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,
+ }, {
+ /* Kionix reserved registers: should not be written */
+ .range_min = 0x28,
+ .range_max = 0x28,
+ }, {
+ .range_min = 0x35,
+ .range_max = 0x36,
+ }, {
+ .range_min = 0x3c,
+ .range_max = 0x48,
+ }, {
+ .range_min = 0x4e,
+ .range_max = 0x5c,
+ }, {
+ .range_min = 0x77,
+ .range_max = 0x7f,
+ },
+};
+
+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_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 {
struct regmap *regmap;
const struct kx022a_chip_info *chip_info;
@@ -239,6 +350,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
@@ -612,6 +730,26 @@ static int kx022a_get_fifo_bytes(struct kx022a_data *data)
return fifo_bytes;
}
+static int kx132_get_fifo_bytes(struct kx022a_data *data)
+{
+ __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(data->dev, "Error reading buffer status\n");
+ return ret;
+ }
+
+ fifo_bytes = le16_to_cpu(buf_status);
+ fifo_bytes &= data->chip_info->buf_smp_lvl_mask;
+ fifo_bytes = min((unsigned int)fifo_bytes, data->chip_info->fifo_length *
+ KX022A_FIFO_SAMPLES_SIZE_BYTES);
+
+ return fifo_bytes;
+}
+
static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
bool irq)
{
@@ -1035,6 +1173,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 c9f9aee7e597..ea6202d29303 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_REG_CNTL5 0x1f
+#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_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;
@@ -86,6 +137,7 @@ struct kx022a_data;
* @channels: pointer to iio_chan_spec array
* @num_channels: number of iio_chan_spec channels
* @fifo_length: number of 16-bit samples in a full buffer
+ * @buf_smp_lvl_mask: buffer sample level mask
* @who: WHO_AM_I register
* @id: WHO_AM_I register value
* @cntl: control register 1
@@ -109,6 +161,7 @@ struct kx022a_chip_info {
const struct iio_chan_spec *channels;
unsigned int num_channels;
unsigned int fifo_length;
+ u16 buf_smp_lvl_mask;
u8 who;
u8 id;
u8 cntl;
@@ -130,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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v8 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer
2023-08-23 21:16 ` [PATCH v8 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer Mehdi Djait
@ 2023-08-24 12:02 ` Andy Shevchenko
2023-08-24 12:51 ` Matti Vaittinen
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2023-08-24 12:02 UTC (permalink / raw)
To: Mehdi Djait
Cc: jic23, mazziesaccount, krzysztof.kozlowski+dt, robh+dt, lars,
linux-iio, linux-kernel, devicetree
On Wed, Aug 23, 2023 at 11:16:41PM +0200, 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.
...
> 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.
I know I have given a tag, but since it most likely require a new version,
this can be amended for the better maintenance as
Enable support for the Kionix digital tri-axis accelerometers
connected to SPI interface. Supported devices are:
KX022A, KX132-1211
...
> 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.
Ditto.
...
> +static const struct regmap_access_table kx132_volatile_regs = {
> + .yes_ranges = &kx132_volatile_ranges[0],
This should be the same as
.yes_ranges = kx132_volatile_ranges,
> + .n_yes_ranges = ARRAY_SIZE(kx132_volatile_ranges),
> +};
Ditto for the following.
> +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_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_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_access_table kx132_nir_regs = {
> + .yes_ranges = &kx132_noinc_read_ranges[0],
> + .n_yes_ranges = ARRAY_SIZE(kx132_noinc_read_ranges),
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v8 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer
2023-08-24 12:02 ` Andy Shevchenko
@ 2023-08-24 12:51 ` Matti Vaittinen
2023-08-24 13:39 ` Andy Shevchenko
0 siblings, 1 reply; 30+ messages in thread
From: Matti Vaittinen @ 2023-08-24 12:51 UTC (permalink / raw)
To: Andy Shevchenko, Mehdi Djait
Cc: jic23, krzysztof.kozlowski+dt, robh+dt, lars, linux-iio,
linux-kernel, devicetree
On 8/24/23 15:02, Andy Shevchenko wrote:
> On Wed, Aug 23, 2023 at 11:16:41PM +0200, 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.
>
> ...
>
>> 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.
>
> I know I have given a tag, but since it most likely require a new version,
> this can be amended for the better maintenance as
>
> Enable support for the Kionix digital tri-axis accelerometers
> connected to SPI interface. Supported devices are:
> KX022A, KX132-1211
>
> ...
>
>> 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.
>
> Ditto.
>
> ...
>
>> +static const struct regmap_access_table kx132_volatile_regs = {
>> + .yes_ranges = &kx132_volatile_ranges[0],
>
>
> This should be the same as
>
> .yes_ranges = kx132_volatile_ranges,
>
The driver uses &kx132_volatile_ranges[0] in a few places (for kx022a)
so I believe this is okay. Well, I know I am biased as I do personally
find &kx132_volatile_ranges[0] clearer. Here we point to the first
element in an array - and yes, it may be I am minority here - but at
least I wouldn't ask for changing this.
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 v8 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer
2023-08-24 12:51 ` Matti Vaittinen
@ 2023-08-24 13:39 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2023-08-24 13:39 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Mehdi Djait, jic23, krzysztof.kozlowski+dt, robh+dt, lars,
linux-iio, linux-kernel, devicetree
On Thu, Aug 24, 2023 at 03:51:18PM +0300, Matti Vaittinen wrote:
> On 8/24/23 15:02, Andy Shevchenko wrote:
> > On Wed, Aug 23, 2023 at 11:16:41PM +0200, Mehdi Djait wrote:
...
> > > + .yes_ranges = &kx132_volatile_ranges[0],
> >
> >
> > This should be the same as
> >
> > .yes_ranges = kx132_volatile_ranges,
> >
>
> The driver uses &kx132_volatile_ranges[0] in a few places (for kx022a) so I
> believe this is okay. Well, I know I am biased as I do personally find
> &kx132_volatile_ranges[0] clearer. Here we point to the first element in an
> array - and yes, it may be I am minority here - but at least I wouldn't ask
> for changing this.
It's a minor thingy, can be ignored.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread