Re: [PATCH v3 2/3] iio: accel: Support Kionix/ROHM KX022A accelerometer

From: Matti Vaittinen
Date: Fri Oct 21 2022 - 03:10:22 EST

Hi Andy & All,

Thanks again for the review.

On 10/20/22 17:34, Andy Shevchenko wrote:
On Thu, Oct 20, 2022 at 02:37:15PM +0300, Matti Vaittinen wrote:
KX022A is a 3-axis accelerometer from ROHM/Kionix. The sensor features
include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
tap/motion detection, wake-up & back-to-sleep events, four acceleration
ranges (2, 4, 8 and 16g) and probably some other cool features.

Add support for the basic accelerometer features such as getting the
acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
using the WMI IRQ).

Important things to be added include the double-tap, motion
detection and wake-up as well as the runtime power management.

...

+ if (!i2c->irq) {
+ dev_err(dev, "No IRQ configured\n");
+ return -EINVAL;

At least

return dev_err_probe(...);

for know error codes (or when we know that there won't be EPROBE_DEFER), takes
less LoCs in the source file.

I think we discussed this already (and disagreed). To me it looks plain ugly to have hard-coded return value in dev_err_probe(). That's why I prefer the

>> + dev_err(dev, "No IRQ configured\n");
>> + return -EINVAL;

when -EINVAL is hard-coded. On the other hand, your comment below is very valid...


+ }

...

+ regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
+ if (IS_ERR(regmap)) {
+ dev_err(dev, "Failed to initialize Regmap\n");
+ return PTR_ERR(regmap);
> Ditto here and anywhere else for the similar cases.

...Yes. This is different from the case above, and I agree the dev_err_probe() should be used here. Thanks for pointing it out. I'll change this and the one in SPI driver as well.


+ }

...

+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *vals = (const int *)kx022a_accel_samp_freq_table;
+ *length = ARRAY_SIZE(kx022a_accel_samp_freq_table) * 2;
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ return IIO_AVAIL_LIST;
+ case IIO_CHAN_INFO_SCALE:
+ *vals = (const int *)kx022a_scale_table;
+ *length = ARRAY_SIZE(kx022a_scale_table) * 2;
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ return IIO_AVAIL_LIST;

These ' * 2' can be replaced with respective ARRAY_SIZE() of nested element

To me this sounds good. I'll see how it would look like.


...

+static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
+{
+ int ret;
+
+ if (on)
+ ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL,
+ KX022A_MASK_PC1);
+ else
+ ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
+ KX022A_MASK_PC1);
+
+ if (ret)
+ dev_err(data->dev, "Turn %s fail %d\n", (on) ? "ON" : "OFF",
+ ret);

str_on_off() ?

Never heard of that before. Seems we have all kinds of gadgets in kernel :) I deeply dislike how ternary looks like so I will gladly hide it in str_on_off() - thanks!

+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ n = ARRAY_SIZE(kx022a_accel_samp_freq_table);
+
+ while (n--)
+ if (val == kx022a_accel_samp_freq_table[n][0] &&
+ kx022a_accel_samp_freq_table[n][1] == val2)

Why not to use the same kind of l and r arguments in == lines?
In current form it's a bit harder to see what the catch here.

As to why I didn't - because for me the order does not matter regarding how hard it is to catch the meaning here. However, there is no problem changing this because the order does not matter for me :)

...

+static int kx022a_get_axis(struct kx022a_data *data,
+ struct iio_chan_spec const *chan,
+ int *val)
+{
+ int ret;
+
+ ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
+ sizeof(__le16));
+ if (ret)
+ return ret;
+
+ *val = le16_to_cpu(data->buffer[0]);

'p'-variant of the above would look better

*val = le16_to_cpup(data->buffer);

since it will be the same as above address without any additional arithmetics.


I guess there is no significant performance difference? To my eye the le16_to_cpu(data->buffer[0]) is much more clear. I see right from the call that we have an array here and use the first member. If there is no obvious technical merit for using le16_to_cpup(data->buffer) over le16_to_cpu(data->buffer[0]), then I do really prefer the latter for clarity.

+ return IIO_VAL_INT;
+}


...

+ return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);

Would simple '0' suffice?

Technically, yes. I however prefer having the register values in hex - unless they directly map to some meaningful physical world entity that is easier to understand when in decimal. (For example, a register which content would directly represent millivolts or divider or any such meaningful physical entity).

...

+ for (i = 0; i < count; i++) {
+ int bit;
+ u16 *samples = &buffer[i * 3];

I would put it as

u16 *samples = &buffer[i * 3];
int bit;

Well, you know my opinion but Ok ;)

Now I also noticed that the name of the block scoped variable 'samples' collides with the function scoped variable of same name. I'll rename the block scoped 'samples' just to avoid confusion.


+ for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
+ memcpy(&data->scan.channels[bit], &samples[bit],
+ sizeof(data->scan.channels[0]));

Why not use bit instead of 0 for the sake of consistency?

Because, again, using 0 is clearer to me. It leaves zero room for wondering :)


Also might be good to have a temporary for channels:

... *chs = data->scan.channels;

I think we have discussed this too previously somewhere. I do dislike hiding things in temporary variables. I like seeing that we are really using the driver private data and not some stack variable and only use temporary variables when they significantly reduce the line count.

However, in this particular case I can scope the temporary variable in this smallish block of code - which makes it pretty easy to spot we are using the data->scan.channel underneath (as the chs is assigned just a row above). And it helps us avoid line split so ... Ok.



for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
memcpy(&chs[bit], &samples[bit], sizeof(chs[bit]));

+ iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
+
+ tstamp += sample_period;
+ }

...

+ ret = regmap_clear_bits(data->regmap, data->ien_reg,
+ KX022A_MASK_WMI);

I don't see why it's not on a single line. Even if you are a conservative
adept of 80.

Good catch. Thanks.

...

+ int ret = IRQ_NONE;
+
+ mutex_lock(&data->mutex);
+
+ if (data->trigger_enabled) {
+ iio_trigger_poll_chained(data->trig);
+ ret = IRQ_HANDLED;
+ }
+
+ if (data->state & KX022A_STATE_FIFO) {

+ ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+ if (ret > 0)
+ ret = IRQ_HANDLED;

I don't like it. Perhaps

bool handled = false;
int ret;

...
ret = ...
if (ret > 0)
handled = true;
...

return IRQ_RETVAL(handled);

I don't see the benefit of adding another variable 'handled'.
If I understand correctly, it just introduces one extra 'if' in IRQ thread handling while hiding the return value in IRQ_RETVAL() - macro.

I do like seeing the IRQ_NONE being returned by default and IRQ_HANDLED only when "handlers" are successfully executed. Adding extra variable just obfuscates this (from my eyes) while adding also the additional 'if'.


+ }
+
+ mutex_unlock(&data->mutex);
+
+ return ret;

...

+ if (!dev)
+ return -ENODEV;

Do you really need this check?

Good question. In principle I do like checking the parameters of exported calls. OTOH, this export is now done using the driver namespace so yes, I think we can drop the check.


...

+ fw = dev_fwnode(dev);
+ if (!fw)
+ return -ENODEV;

You may combine these two in one.

struct fwnode_handle *fwnode;


fwnode = dev ? dev_fwnode(dev) : NULL;
if (!fwnode)
return -ENODEV;

I just drop the check for !dev. But even if I didn't, I wouldn't use ternary here - to me it is _much_ harder to read compared to two separate ifs while giving no obvious benefits.

And please, call it fwnode.

Ok. I personally like fw more - probably because I'm used to that - but I guess the 'fwnode' is used in number of other places. Thanks.

...

+ irq = fwnode_irq_get_byname(fw, "INT1");
+ if (irq > 0) {
+ data->inc_reg = KX022A_REG_INC1;
+ data->ien_reg = KX022A_REG_INC4;
+
+ if (fwnode_irq_get_byname(dev_fwnode(dev), "INT2") > 0)

Why not use fwnode again

I think I've beeen distracted while writing this part :) I guess I have added the temporary variable 'fw' just for the purpose of being able to call the dev_fwnode() just once. So, Thanks!


...

+ if (ret)
+ return dev_err_probe(data->dev, ret,
+ "iio_triggered_buffer_setup_ext FAIL %d\n",
+ ret);

Drop dup ret at the end, dev_err_probe() has been adding it to each message.

Thanks!


...

+ /*
+ * No need to check for NULL. request_threadedI_irq() defaults to
+ * dev_name() should the alloc fail.
+ */
+ name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a",
+ dev_name(data->dev));

It's not clear why do you need a suffix here.


Because for example just "spi0,0" is much less informative compared to "spi0.0-kx022a". As an user I like seeing the device generating the IRQ. I don't wan't to dig out details like which bus/chipselect my device is connected to - especially if I have only one accelerometer connected. The dev_name() is used just to make this unique for cases where we could have multiple similar devices connected to the system (as you suggested in previous review).


Once again, thanks for the review! I appreciate your help/suggestions.

Yours
-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~