iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

Remove blank lines pointed out by the checkpatch script

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.

Quoted 3 lines · 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.

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.

Add the missing i2c device id.

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.

+/* kx022a Regmap configs */

return dev_err_probe(dev, irq, "No suitable IRQ\n");

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

drivers/iio/accel/kionix-kx022a.c +0 -0

 	if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES)
 		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");

Quoted 3 lines · 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

...

drivers/iio/accel/kionix-kx022a.c static int kx022a_drop_fifo_contents(struct kx022a_data *data) · line 612

Quoted 12 lines

drivers/iio/accel/kionix-kx022a.c +12 -0

+	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?

Quoted 1 line · Andy Shevchenko

On Wed, Aug 23, 2023 at 11:16:40PM +0200, Mehdi Djait wrote:

Quoted 8 lines

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c +8 -0

+	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;
+	}
+

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c static int kx022a_drop_fifo_contents(struct kx022a_data *data) · line 612

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c +4 -0

+	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.

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Wed, Aug 23, 2023 at 11:16:40PM +0200, Mehdi Djait wrote:

...

Quoted 17 lines

Quoted 8 lines

Quoted 8 lines

+ int ret, fifo_bytes;

drivers/iio/accel/kionix-kx022a.c +7 -0

+
+ 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;
+ }
+

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c static int kx022a_drop_fifo_contents(struct kx022a_data *data) · line 612

Quoted 4 lines

+ if (fifo_bytes == KX022A_FIFO_FULL_VALUE)

drivers/iio/accel/kionix-kx022a.c +3 -0

+         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?

Quoted 2 lines

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.

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Wed, Aug 23, 2023 at 11:16:40PM +0200, Mehdi Djait wrote:

Quoted 6 lines

I see. I think my confusion can be easily cured by renaming the callback to

get_amount_bytes_in_fifo()

or

Quoted 3 lines

        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

Quoted 5 lines · Mehdi Djait

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Wed, Aug 23, 2023 at 11:16:40PM +0200, Mehdi Djait wrote:

...

Quoted 10 lines

Quoted 9 lines

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.

Quoted 3 lines

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?

Quoted 6 lines · Andy Shevchenko

Quoted 5 lines · Mehdi Djait

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Wed, Aug 23, 2023 at 11:16:40PM +0200, Mehdi Djait wrote:

Quoted 16 lines

Quoted 6 lines

Quoted 6 lines

I see. I think my confusion can be easily cured by renaming the callback to

get_amount_bytes_in_fifo()

or

Quoted 7 lines

Quoted 3 lines

        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.

Quoted 2 lines

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()"

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

I see. I think my confusion can be easily cured by renaming the callback to

get_amount_bytes_in_fifo()

or

Quoted 7 lines

Quoted 3 lines

        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?

Quoted 5 lines

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!

Quoted 20 lines

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

I see. I think my confusion can be easily cured by renaming the callback to

get_amount_bytes_in_fifo()

or

Quoted 7 lines

Quoted 3 lines

         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?

Quoted 8 lines

Quoted 5 lines

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 :)

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.

Quoted 1 line · Matti Vaittinen

On 8/27/23 21:09, Jonathan Cameron wrote:

...

Quoted 2 lines

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

C language which is already considered as mastodon among youngsters).

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Matti Vaittinen

On 8/27/23 21:09, Jonathan Cameron wrote:

Quoted 4 lines

Quoted 2 lines

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.

Quoted 3 lines

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

Quoted 1 line

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) ;)

Quoted 3 lines · Matti Vaittinen

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Matti Vaittinen

On 8/27/23 21:09, Jonathan Cameron wrote:

Sorry it took a bit of time to reply on this.

...

Quoted 12 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 3 lines

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

Quoted 9 lines

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.

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.

Quoted 8 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

Quoted 1 line

C language which is already considered as mastodon among youngsters).

Quoted 10 lines

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...

Quoted 7 lines

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) ;)

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Matti Vaittinen

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Matti Vaittinen

On 8/27/23 21:09, Jonathan Cameron wrote:

Quoted 24 lines

Quoted 14 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 9 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

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).

Quoted 19 lines

Quoted 8 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.

Quoted 2 lines

(besides the C language which is already considered as mastodon among youngsters).

Quoted 10 lines

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).

Quoted 2 lines

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).

Quoted 6 lines

Quoted 6 lines

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) ;)

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.

drivers/iio/accel/kionix-kx022a.c +16 -0

+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),
+};

Quoted 4 lines · 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.

...

drivers/iio/accel/Kconfig config IIO_KX022A_SPI · line 422

Quoted 5 lines

help

drivers/iio/accel/Kconfig +2 -2

-	  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

...

drivers/iio/accel/Kconfig config IIO_KX022A_I2C · line 431

Quoted 5 lines

help

drivers/iio/accel/Kconfig +2 -2

-	  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.

...

drivers/iio/accel/kionix-kx022a.c static const struct regmap_config kx022a_regmap_config = { · line 175

Quoted 2 lines

drivers/iio/accel/kionix-kx022a.c +2 -0

+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,

Quoted 1 line · Andy Shevchenko

On Wed, Aug 23, 2023 at 11:16:41PM +0200, Mehdi Djait wrote:

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.c static const struct regmap_config kx022a_regmap_config = { · line 175

Quoted 1 line

drivers/iio/accel/kionix-kx022a.c +1 -0

+	.yes_ranges = &kx132_volatile_ranges[0],

This should be the same as

Quoted 2 lines

.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.