Date:   Mon, 10 Oct 2022 13:20:23 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Matti Vaittinen <mazziesaccount@...il.com>
CC:     Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jagath Jog J <jagathjog1996@...il.com>,
        Nikita Yushchenko <nikita.yoush@...entembedded.com>,
        Cosmin Tanislav <demonsingur@...il.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM KX022A
 accelerometer

On 10/10/22 14:58, Andy Shevchenko wrote:
> On Mon, Oct 10, 2022 at 12:12:34PM +0300, Matti Vaittinen wrote:
>> Hello Andy,
>>
>> Thanks for taking the time and doing the review. Quite a few comments I do
>> agree with. I didn't respond to those unless you asked a question :) Thanks
>> for the help! OTOH, as usual, I do not fully agree with everything - and at
>> some places I would like to get some further information :)
>>
>> On 10/6/22 21:32, Andy Shevchenko wrote:
>>> On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:
> 
> ...
> 
>>>> +config IIO_KX022A_SPI
>>>> +	tristate "Kionix KX022A tri-axis digital accelerometer"
>>>> +	depends on I2C
>>>> +	select IIO_KX022A
>>>> +	select REGMAP_SPI
>>>> +	help
>>>> +	  Say Y here to enable support for the Kionix KX022A digital tri-axis
>>>> +	  accelerometer connected to I2C interface.
>>>

> Oh, I didn't know that the rest is also using this pattern. My main objection
> here is Y vs. M and maybe less insisting tone of the language

Ah. I didn't notice how this emphasizes Y over M. Now I think I 
understand your note. Thanks for the explanation.

>>> Also what will be the module name (see other Kconfig for the pattern)?
>>
>> Here the pattern is IMHO improved by cutting the useless text. Even the
>> cross-reference from SPI to I2C and I2C to SPI driver which was in v1 is
>> more useful. Module autoloading for SPI should just work. Adding the module
>> name here seems just a silly habit - which I see many people are breaking.
>> If module name is to be needed, then it should be checked from the Makefile
>> and not trusted that Kconfig text stays up-to-date...
> 
> I think the module name is a good thing to have for a user who might be
> not knowing about Makefile:s and how to derive the module name from that
> (sometimes it's not so obvious).

My point is that average users do not really even need to know the 
module names. For most simple drivers the module name is just the base 
of the filename. I really doubt there are that many people who

a) do need to know module name
b) read config menu entries but don't know how to find a module name 
from Makefile
c) want to know module name and can't pair the name from lsmod to module
d) rather look up the config entry than browse the contents of 
lib/modules/...

It'd be nice to know if anyone reading this mail has actually ever used 
the module name information placed in Kconfig entry for _anything else_ 
except for making the Kconfig entry to look a bit more fatty...


>>
>> The blank prior function return? I think I have been told to have blanks
>> preceding returns to make returns more obvious - which I kind of understand.
> 
> Yes, _adding_ blank line (see what I referred to? To SPI driver as a good
> example).

Ah. Thanks. I misunderstood your comment.

> ...
> 
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/sysfs.h>
>>>> +#include <linux/iio/trigger.h>
>>>> +#include <linux/iio/trigger_consumer.h>
>>>> +#include <linux/iio/triggered_buffer.h>
>>>
>>> Can this group be placed after units.h (with blank line in between)?
>>
>> Why? What's wrong with alphabetical order?
> 
> Nothing wrong with the order and order is orthogonal to the grouping.
> My comment is for the latter. The rationale is to easily see generic header
> group and subsystem related group.

I can for sure change this but It'd be nice to know why should the 
subsystem headers be treated differently from the rest of the headers?


> But I see that it is there, I simply missed it.
> 
Ok.

> ...
> 
>>>> +struct kx022a_data {
>>>> +	int irq;
>>>> +	int inc_reg;
>>>> +	int ien_reg;
>>>
>>>> +	struct regmap *regmap;
>>>
>>> Putting this as a first member might improve code by making pointer arithmetics
>>> better. Have you checked possibilities with bloat-o-meter?
>>
>> No. Amount of combinations is just too huge for me to try randomly
>> reordering structs and running bloat-o-meter. Can you please explain me why
>> you think reordering this would have visible impact and if this is something
>> that is universally true or if such an optimization is architecture
>> dependent?
> 
> Usually regmap pointer is used more often than, for instance, the 'irq' member,
> and I believe that moving 'irq' member deeper will be harmless, while moving
> regmap might be winning.

So you suggest us to try optimizing the code by placing a pointer to 
more frequently used member(s) at the top of the struct? I didn't know 
it would have any impact. I understand arranging the members of 
frequently copied structs based on sizes so that we can avoid eating 
space by padding - but I had never heard of things being improved by 
ordering based on frequency of use. It'd be great to understand the 
mechanism this optimization is based on?

> 
>>>> +	struct iio_trigger *trig;
>>>> +	bool trigger_enabled;
>>>> +
>>>> +	struct device *dev;
>>>> +	unsigned int g_range;
>>>
>>>> +	struct mutex mutex;
>>>
>>> No warning from checkpatch? Every lock should be commented what it is for.
>>
>> No. I didn't see a warn from checkpatch with v6.0-rc7. And I am glad I
>> didn't. I am not a fan of statements like "always document a lock" or "never
>> use a goto" - because blindly following such "rules" tend to lead silly
>> corner cases. In lock commenting case it is likely to lead copy-pasting
>> comments for locks which are used for trivial protections. And I am
>> definitely not a fan of boilerplate.
> 
> So, tell then, what is this lock for? How as a code reader I have to know that?

I can add comment to this lock as it does protect more than one thing 
(and may not be trivial). But I am not at all sure requiring all locks 
to be documented will have optimal end-result... And no, I don't think 
checkpatch in v6.0-rc7 did warn about this lock. Do you think checkpatch 
has been modified to do that?

> ...
> 
>>>> +		int n = ARRAY_SIZE(kx022a_accel_samp_freq_table);
>>>
>>> You may drop {} in each case and have n defined once for all cases.
>>
>> Could you please explain how? We use different table depending on the case.
> 
> 	{
> 		int n;
> 
> 		switch (x) {
> 		case Y:
> 			n = ARRAY_SIZE(foo);
> 			...
> 			break
> 		case Z:
> 			n = ARRAY_SIZE(bar);
> 			...
> 		}
> 	}

Ah, sure. I feel a bit stupid :) But thanks for the example!

> 
>>>> +
>>>> +		while (n-- > 0)
>>>
>>> while (n--) ? Or why the 0 is ignored?
>>
>> What do you mean by 0 being ignored? I am sorry but I do have difficulties
>> following some of your comments.
> 
> Your while loop stops when n == 0. The 0 is not going inside while-loop body...

I think you are making a mistake here.

>>>> +			if (val == kx022a_accel_samp_freq_table[n][0] &&
>>>> +			    kx022a_accel_samp_freq_table[n][1] == val2)
>>>> +				break;
>>>
>>>> +		if (n < 0) {
>>>
>>> How is it even possible with your conditional?
>>
>> Eh? Why it would not be possible? I don't see any problem with the code.
>> Could you explain me what you mean?
> 
> ...and as a side effect, this condition won't be ever true. Did I miss
> something?
> 

Please try running:
#include <stdio.h>

int main()
{
	int n = 2;

	while (n-- > 0)
		printf("in loop n = %d\n", n);

	printf("exit loop n = %d\n", n);

	return 0;
}

>>>
>>>> +			ret = -EINVAL;
>>>> +			goto unlock_out;
>>>> +		}
> 
> ...
> 
>>>> +	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
>>>> +			       sizeof(s16));
> 
>>> No endianess awareness (sizeof __le16 / __be16)
> 
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	*val = data->buffer[0];
>>>
>>> Ditto (get_unaligned_be16/le16 / le16/be16_to_cpup()).
>>
>> I have probably misunderstood something but I don't see why we should use
>> 'endianess awareness' in drivers? I thought the IIO framework code takes
>> care of the endianes conversions based on scan_type so each individual
>> driver does not need to do that. That however has been just my assumption. I
>> will need to check this. Thanks for pointing it out.
> 
> The IIO core uses endianness field only once in iio_show_fixed_type() AFAICS.
> And it does nothing with it. Maybe Jonathan can shed a light what is it for
> (I mean the field)?
> 
> ...
> 
>>>> +	if (val > KX022A_FIFO_LENGTH)
>>>> +		val = KX022A_FIFO_LENGTH;
>>>
>>> max() from minmax.h?
>>
>> Definitely not. We could use min() to squeeze one line but for my personal
>> preference the open-coded limit check is clearer. Can switch to min() if
>> this is a subsystem standard though.
> 
> Not sure how min() here would check for >. Are you sure you haven't forgotten
> to tell me something more?
>

Please bear with me if I am wrong here. I have spent the weekend 
wandering in the forest and I am tired as ....

Anyways, my pretty dizzy brains are telling me that we want to use the 
val if it is smaller than the KX022A_FIFO_LENGTH. If val is greater, 
then we use the KX022A_FIFO_LENGTH. This should match with the

 >>>> +	if (val > KX022A_FIFO_LENGTH)
 >>>> +		val = KX022A_FIFO_LENGTH;

Eg, we always want to use the smaller of the two, right? Hence:

val = min(val, KX022A_FIFO_LENGTH);

would do the same job, right? But for me the open-coded check if val 
exceeds the FIFO_LENGTH looks more obvious.

> ...
> 
>>>> +	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0xff);
>>>
>>> GENMASK() ?
>>
>> I don't really see a need for GENMASK here. It does not matter what is
>> written to this register.
> 
> Why not 0? At least it will be aligned to the _CLEAR part without need of
> any comment. Otherwise it sounds like 0xff is special here.

I can change it to 0 if it pleases the audience :) 0xff was just first 
thing that came to my mind when I read the spec that something should be 
written to this register.

>>>> +	if (en)
>>>> +		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
>>>> +				       KX022A_MASK_DRDY);
>>>
>>> I would put redundant 'else' here to have them on the same column, but
>>> it's pity we don't have regmap_assign_bits() API (or whatever name you
>>> can come up with) to hide this kind of code.
>>
>> Eh, you mean you would put else here even though we return from the if? And
>> then put another return in else, and no return outside the if/else?
>>
>> I definitely don't like the idea.
>>
>> We could probably use regmap_update_bits and ternary but in my opinion that
>> would be just much less obvious. I do like the use of set/clear bits which
>> also makes the meaning / "polarity" of bits really clear.
> 
> The idea behind is simple (and note that I'm usually on the side of _removing_
> redundancy)
> 
> 	if (en)
> 		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> 				       KX022A_MASK_DRDY);
> 	else
> 		return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> 					 ...
> 
> Because the branches are semantically tighten to each other. But it's not
> a big deal to me.

What I do not really like in above example is that we never reach the 
end of function body. It is against the expectation - and adding the 
else has no real meaning when if returns. I actually think that 
checkpatch could even warn about that construct.

> 
>>>> +	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
>>>> +				 KX022A_MASK_DRDY);
> 
> 
>>>> +	irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
>>>> +	if (irq > 0) {
>>>> +		data->inc_reg = KX022A_REG_INC1;
>>>> +		data->ien_reg = KX022A_REG_INC4;
>>>> +	} else {
>>>> +		irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
>>>> +		if (irq < 0)
>>>> +			return dev_err_probe(dev, irq, "No suitable IRQ\n");
>>>
>>> Missed check for 0.
>>
>> Why check for 0? fwnode_irq_get_byname() doc states:
>>   * Return:
>>   * Linux IRQ number on success, or negative errno otherwise.
>>
>> I don't see 0 indicating a failure?
> 
> It's a mislead by documentation. You need to read the fwnode_irq_get()
> to see what's going on. Feel free to submit a fix for the
> fwnode_irq_get_byname() kernel doc.

Thanks. I had missed the mapping error.

> ...
> 
>>>> +module_param(g_kx022a_use_buffer, bool, 0);
>>>> +MODULE_PARM_DESC(g_kx022a_use_buffer,
>>>> +		 "Buffer samples. Use at own risk. Fifo must not overflow");
>>>
>>> Why?! We usually do not allow module parameters in the new code.
>>
>> There are still quite a few parameters that have been added during previous
>> cycle. And many of those are to allow behaviour which is undesirable in many
>> cases for those who can benefit from it.
>>
>> 3dbec44d9c94 ("KVM: VMX: Reject kvm_intel if an inconsistent VMCS config is
>> detected")
>>
>> can serve as a random example.
> 
> KVM is quite different by nature to an arbitrary driver of an arbitrary sensor
> that might appear in the real hardware at hand on 1:100 ratio basis.
> 
>> As explained by the comment:
>> /*
>>   * Filling up the HW-FIFO can cause nasty problems. Thus we do not
>>   * enable the fifo unless it is explicitly requested by a module param.
>>   * If you are _sure_ your system can serve the interrupts in time you
>>   * can enable the HW fifo. I do not recommend it for sample frequencies
>>   * higher than 2 Hz - and even in that case I would set the watermark
>>   * somewhere near 20 samples (HI-RES) to have magnitude of 10 sec
>>   * safety-margin.
>>   */
>>
>> this is also the use-case here.
>>
>> And yes, using the FIFO regardless of the problems can be beneficial so just
>> dropping the support completely would be counter-productive. I agree with
>> Jonathan that disabling the FIFO by default does probably help avoiding bug
>> reports - but allowing enabling the FIFO with own risk is just giving a hand
>> to those who need it. Thus I'd really like to see the support upstream. We
>> all know that code which is sitting in some downstream git is not likely to
>> help people - and it is the reason why I do upstream code in first place.
> 
> I would suggest to split the part that adds parameter (with whatever it does
> elsewhere) from the basic configuration. In this case the commit message should
> at least clarify the needs of the parameter.

I think adding the parameter in a separate patch is a good idea - even 
if patching something that was added by earlier patch in the series is 
often frowned upon.

> JFYI: It's Greg KH strong opinion about parameters and I agree with him on
> that. Note that this subsystem goes via on of the Greg's trees.

Sure. And if this is not accepted by a maintainer in the chain - then it 
needs to be re-evaluated, changed or dropped. But at least I can try 
explaining why I think this is needed / useful.

Thanks for the help once again.

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