Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 8/20/22 19:21, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 1:05 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>> On 8/20/22 10:18, Andy Shevchenko wrote:
>>> On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
>>> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
>>>> On 8/20/22 09:25, Andy Shevchenko wrote:
>>>>> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
>>>>> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
>>>>>> On 8/20/22 02:30, Andy Shevchenko wrote:
>>>>>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
>>>>>>> <mazziesaccount@gmail.com> wrote:
>>>>>
>>>>> What did I miss?
>>>>
>>>>    >>>>           struct bmg160_data *data;
>>>>    >>>>           struct iio_dev *indio_dev;
>>>>
>>>> This does already violate the rule.
>>>
>>> Indeed, I am reading this with an MTA that has True Type fonts, and I
>>> can't see it at the first glance. But this breaks that rule slightly
>>> while your added line breaks it significantly.
>>
>> Yes. As I said, I think the reverse xmas tree rule is not too well
>> justified. Bunch of the subsystems do not really follow it, nor did this
>> function. Yet, as I said, I can move the array to the first line in the
>> function when I respin the series..
> 
> You still can do better in _your_ series, right?

I don't see the benefit of the reverse xmas tree. We have discussed this 
already in the past :) I definitely have no need to start using reverse 
xmas tree thingee somewhere it has not been previously used. It may be 
better in _your_ opinion.

>>>>>>> this case you even can move it out of the function, so we will see
>>>>>>> clearly that this is (not a hidden) global variable.
>>>>>>
>>>>>> Here I do disagree with you. Moving the array out of the function makes
>>>>>> it _much_ less obvious it is not used outside this function. Reason for
>>>>>> making is "static const" is to allow the data be placed in read-only
>>>>>> area (thanks to Guenter who originally gave me this tip).
>>
>> Just wanted to correct - it was Sebastian Reichel, not Guenter who
>> explained me why doing local static const arrays is better than plain const.
> 
> Did he suggest putting it inside the function?

He asked me to convert a local array to static const. I though like you 
do now that the local array should not be static but just const - and he 
corrected me in his reply. This can be seen in the discussion I linked 
below.

>>>>> "static" in C language means two things (that's what come to my mind):
>>>>> - for functions this tells that a function is not used outside of the module;
>>>>> - for variables that it is a _global_ variable.
>>>>>
>>>>> Hiding static inside functions is not a good coding practice since it
>>>>> hides scope of the variable.
>>>>
>>>> For const arrays the static in function does make sense. Being able to
>>>> place the data in read-only areas do help with the memory on limited
>>>> systems.
>>>
>>> I'm not sure we are on the same page. I do not object to the "const"
>>> part and we are _not_ talking about that.
>>
>> Maybe the explanation by Sebastian here can put us on the same page:
>> https://lore.kernel.org/all/20190502073539.GB7864@localhost.localdomain/
>> https://lore.kernel.org/all/322fa765ddd72972aba931c706657661ca685afa.camel@fi.rohmeurope.com/
> 
> Again, you are too focused on "const", I'm talking about "static". The
> above doesn't clear a bit regarding why you hide the global variable
> inside a function. I don't see either Sebastian's clear point on this.

I don't really see why you talk about "hiding a global variable in a 
function"? A static variable which is declared in function is not 
global. It is local. It causes no more name collisions than a regular 
local variable does so I really don't understand your reasoning.

>>>>> And if you look into the kernel code, I
>>>>> believe the use you are proposing is in minority.
>>>>
>>>> I don't know about the statistics. What I know is that we do have a
>>>> technical benefits when we use static const arrays instead of non static
>>>> ones in the functions. I do also believe placing the variables in blocks
>>>> is a good practice.
>>>
>>> Yes, and global variables are better to be seen as global variables.
>>>
>>>> I tend to agree with you that using local, non const statics has
>>>> pitfalls - but the pitfalls do not really apply with const ones. You
>>>> know the value and have no races. Benefit is that just by seeing that no
>>>> pointer is returned you can be sure that no "sane code" uses the data
>>>> outside the function it resides.
>>>
>>> Putting a global variable (const or non-const) to the function will
>>> hide its scope and it is prone to getting two variables with the same
>>> or very similar names with quite different semantics.
>>
>> I don't see how moving something from a local block to a global scope
>> does make conflicts less of an issue?
> 
> You may add a static variable inside each function in the same module
> and name it "foo" and there will be no conflict, but when you read the
> code your brain will be spoiled.

And how is it different from reading functions where the regular 
variables have identical names? I _really_ can't follow your reasoning.

> This is simply _bad code practice_. I
> don't know how else I can explain this to you.
> 
>> On the contrary, it makes things
>> worse as then the moved variable will collide with any other variable in
>> any of the functions in the whole file. Having the array as function
>> local static makes the naming collisions to be issue only if another
>> global variable has the same name.
> 
> Again, you missed my point. I'm talking about reading and analysing
> the code.

I _definitely_ miss your point here. I have zero problems reading code 
where static const variables are used in a function. I think it is 
pretty much as hard as seeing a #defined value - difference being that 
one can point to the variable.

I admit that static variables whose value is changed can be more of a 
problem especially when access to function is not serialized.

> Otherwise (good) compiler should spill a lot of warnings in
> case you have global vs. local naming collision.
> 
>> And if that happened - the chances
>> are code would still be correct as the function here is clearly intended
>> to use the local one. If someone really later adds a global with the
>> same name - and uses the global in this function - then he should have
>> noted we have local variable with same name. Additionally - such user
>> would be using terribly bad name for a global variable.
>>
>> Please note that scope of the function local static variable is limited
>> to function even if the life-time is not just the life-time of a function.
> 
> Nope. The RO section might be very well flashed into ROM, so...

...so?

>>> That's why it's
>>> really not good practice. I would rather see it outside of the
>>> function _esp_ because it's static const.
>>
>> Sorry, I really don't agree with your reasoning here. :(
> 
> So, whom should we listen to here? Because bad code is bad code. And
> this is code above.

Bad is a subjective concept. I'd say the code gets much worse if we make 
the local variable a global one.

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

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