Message-ID: <2cad533d-32d1-5ca1-74e6-e2debcbdad81@gmail.com>
Date:   Fri, 21 Oct 2022 10:10:08 +0300
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Nikita Yushchenko <nikita.yoush@...entembedded.com>,
        Dmitry Rokosov <DDRokosov@...rdevices.ru>,
        Jagath Jog J <jagathjog1996@...il.com>,
        Cosmin Tanislav <demonsingur@...il.com>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] iio: accel: Support Kionix/ROHM KX022A
 accelerometer

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! ~~

Powered by blists - more mailing lists