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

[Matti Vaittinen]

On 8/20/22 19:21, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 1:05 PM Matti Vaittinen
> <mazziesaccount@xxxxxxxxx> wrote:
> > On 8/20/22 10:18, Andy Shevchenko wrote:
> > > On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
> > > <Matti.Vaittinen@xxxxxxxxxxxxxxxxx> wrote:
> > > > On 8/20/22 09:25, Andy Shevchenko wrote:
> > > > > On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
> > > > > <Matti.Vaittinen@xxxxxxxxxxxxxxxxx> wrote:
> > > > > > On 8/20/22 02:30, Andy Shevchenko wrote:
> > > > > > > On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> > > > > > > <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxx/
>
> 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! ~~