LinuxLists.cc - [PATCH v3 00/14] Use devm helpers for regulator get and enable

[permalink] [raw]

Subject: [PATCH v3 13/14] iio: max1363: simplify using devm_regulator_get_enable()

Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
add_action_or_reset(regulator_disable)' and use the
devm_regulator_get_enable() and drop the pointer to the regulator.
This simplifies code and makes it less tempting to add manual control
for the regulator which is also controlled by devm.

Signed-off-by: Matti Vaittinen <[email protected]>

---
v2 => v3:
New patch
---
drivers/iio/adc/max1363.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index eef55ed4814a..d900e29d6cb7 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -169,7 +169,6 @@ struct max1363_state {
const struct max1363_chip_info *chip_info;
const struct max1363_mode *current_mode;
u32 requestedmask;
- struct regulator *reg;
struct mutex lock;

/* Using monitor modes and buffer at the same time is
@@ -1603,15 +1602,7 @@ static int max1363_probe(struct i2c_client *client,
st = iio_priv(indio_dev);

mutex_init(&st->lock);
- st->reg = devm_regulator_get(&client->dev, "vcc");
- if (IS_ERR(st->reg))
- return PTR_ERR(st->reg);
-
- ret = regulator_enable(st->reg);
- if (ret)
- return ret;
-
- ret = devm_add_action_or_reset(&client->dev, max1363_reg_disable, st->reg);
+ ret = devm_regulator_get_enable(&client->dev, "vcc");
if (ret)
return ret;

--
2.37.1

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]

Attachments:

(No filename) (1.74 kB)
signature.asc (499.00 B)
Download all attachments

2022-08-19 20:14:25

[permalink] [raw]

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

Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
bulk-enable, add-action-to-disable-at-detach - pattern.

Signed-off-by: Matti Vaittinen <[email protected]>

---
v2 => v3
Split to own patch.
---
drivers/iio/gyro/bmg160_core.c | 24 +++---------------------
1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
index cedd9f02ea21..baa80980c99f 100644
--- a/drivers/iio/gyro/bmg160_core.c
+++ b/drivers/iio/gyro/bmg160_core.c
@@ -93,7 +93,6 @@

struct bmg160_data {
struct regmap *regmap;
- struct regulator_bulk_data regulators[2];
struct iio_trigger *dready_trig;
struct iio_trigger *motion_trig;
struct iio_mount_matrix orientation;
@@ -1067,19 +1066,13 @@ static const char *bmg160_match_acpi_device(struct device *dev)
return dev_name(dev);
}

-static void bmg160_disable_regulators(void *d)
-{
- struct bmg160_data *data = d;
-
- regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
-}
-
int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
const char *name)
{
struct bmg160_data *data;
struct iio_dev *indio_dev;
int ret;
+ static const char * const regulators[] = {"vdd", "vddio"};

indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
if (!indio_dev)
@@ -1090,22 +1083,11 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
data->irq = irq;
data->regmap = regmap;

- data->regulators[0].supply = "vdd";
- data->regulators[1].supply = "vddio";
- ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
- data->regulators);
+ ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
+ regulators);
if (ret)
return dev_err_probe(dev, ret, "Failed to get regulators\n");

- ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
- data->regulators);
- if (ret)
- return ret;
-
- ret = devm_add_action_or_reset(dev, bmg160_disable_regulators, data);
- if (ret)
- return ret;
-
ret = iio_read_mount_matrix(dev, &data->orientation);
if (ret)
return ret;
--
2.37.1

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]

Attachments:

(No filename) (2.49 kB)
signature.asc (499.00 B)
Download all attachments

2022-08-19 20:14:34

[permalink] [raw]

Subject: [PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
add_action_or_reset(regulator_disable)' and use the
devm_regulator_get_enable() and drop the pointer to the regulator.
This simplifies code and makes it less tempting to add manual control
for the regulator which is also controlled by devm.

Signed-off-by: Matti Vaittinen <[email protected]>

---
v2 => v3:
New patch
---
drivers/iio/amplifiers/hmc425a.c | 17 +----------------
1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
index ce80e0c916f4..108f0f1685ef 100644
--- a/drivers/iio/amplifiers/hmc425a.c
+++ b/drivers/iio/amplifiers/hmc425a.c
@@ -34,7 +34,6 @@ struct hmc425a_chip_info {
};

struct hmc425a_state {
- struct regulator *reg;
struct mutex lock; /* protect sensor state */
struct hmc425a_chip_info *chip_info;
struct gpio_descs *gpios;
@@ -162,13 +161,6 @@ static const struct of_device_id hmc425a_of_match[] = {
};
MODULE_DEVICE_TABLE(of, hmc425a_of_match);

-static void hmc425a_reg_disable(void *data)
-{
- struct hmc425a_state *st = data;
-
- regulator_disable(st->reg);
-}
-
static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
[ID_HMC425A] = {
.name = "hmc425a",
@@ -211,14 +203,7 @@ static int hmc425a_probe(struct platform_device *pdev)
return -ENODEV;
}

- st->reg = devm_regulator_get(&pdev->dev, "vcc-supply");
- if (IS_ERR(st->reg))
- return PTR_ERR(st->reg);
-
- ret = regulator_enable(st->reg);
- if (ret)
- return ret;
- ret = devm_add_action_or_reset(&pdev->dev, hmc425a_reg_disable, st);
+ ret = devm_regulator_get_enable(&pdev->dev, "vcc-supply");
if (ret)
return ret;

--
2.37.1

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]

Attachments:

(No filename) (2.06 kB)
signature.asc (499.00 B)
Download all attachments

2022-08-19 23:38:44

[permalink] [raw]

Subject: Re: [PATCH v3 00/14] Use devm helpers for regulator get and enable

On Fri, Aug 19, 2022 at 10:20 PM Matti Vaittinen
<[email protected]> wrote:
>
> Use devm helpers for regulator get and enable
>
> NOTE: The series depends on commit
> ee94aff2628b ("Devm helpers for regulator get and enable")
> which currently sits in Mark's regulator/for-next
>
> A few* drivers seem to pattern demonstrated by pseudocode:
>
> - devm_regulator_get()
> - regulator_enable()
> - devm_add_action_or_reset(regulator_disable())
>
> devm helpers for this pattern were added to remove bunch of code from

remove a bunch

> drivers. Typically following:
>
> - replace 3 calls (devm_regulator_get[_optional](), regulator_enable(),
> devm_add_action_or_reset()) with just one
> (devm_regulator_get_enable[_optional]()).
> - drop disable callback.
>
> I believe this simplifies things by removing some dublicated code.

duplicated

> This series reowrks a few drivers. There is still plenty of fish in the

reworks

> sea for people who like to improve the code (or count the beans ;]).
>
> Finally - most of the converted drivers have not been tested (other than
> compile-tested) due to lack of HW. All reviews and testing is _highly_
> appreciated (as always!).

...

> docs: devres: regulator: Add new get_enable functions to devres.rst
> clk: cdce925: simplify using devm_regulator_get_enable()
> gpu: drm: simplify drivers using devm_regulator_*get_enable*()
> hwmon: lm90: simplify using devm_regulator_get_enable()
> hwmon: adm1177: simplify using devm_regulator_get_enable()

hwmon uses a different pattern for the Subject line.

--
With Best Regards,
Andy Shevchenko

2022-08-19 23:52:52

[permalink] [raw]

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

On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
<[email protected]> wrote:
>
> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
> bulk-enable, add-action-to-disable-at-detach - pattern.

...

> int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
> const char *name)
> {
> struct bmg160_data *data;
> struct iio_dev *indio_dev;
> int ret;
> + static const char * const regulators[] = {"vdd", "vddio"};

Please, keep this following the "longest line first" rule. Note, in
this case you even can move it out of the function, so we will see
clearly that this is (not a hidden) global variable.

P.S. Same applies for the rest of the similar places in your series.

--
With Best Regards,
Andy Shevchenko

2022-08-20 06:28:30

[permalink] [raw]

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

Thanks for the review Andy

On 8/20/22 02:30, Andy Shevchenko wrote:
> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> <[email protected]> wrote:
>>
>> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
>> bulk-enable, add-action-to-disable-at-detach - pattern.
>
> ...
>
>> int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
>> const char *name)
>> {
>> struct bmg160_data *data;
>> struct iio_dev *indio_dev;
>> int ret;
>> + static const char * const regulators[] = {"vdd", "vddio"};
>
> Please, keep this following the "longest line first" rule. Note, in

This was not following the (IMO slightly silly) rule even prior my
patch. I can for sure move my line up - but that won't give you the
"reverse X-mas tree".

I don't have any real objections on changing the styling though - I
don't expect this to be merged before the dependency is in rc1 - so I
guess I will anyways need to respin this for next cycle. I can do the
styling then.

> 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).

> P.S. Same applies for the rest of the similar places in your series.
>

Br,
-- Matti

--
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

2022-08-20 06:30:34

[permalink] [raw]

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

On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
<[email protected]> wrote:
> On 8/20/22 02:30, Andy Shevchenko wrote:
> > On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> > <[email protected]> wrote:

...

> >> struct bmg160_data *data;
> >> struct iio_dev *indio_dev;
> >> int ret;
> >> + static const char * const regulators[] = {"vdd", "vddio"};
> >
> > Please, keep this following the "longest line first" rule. Note, in
>
> This was not following the (IMO slightly silly) rule even prior my
> patch. I can for sure move my line up - but that won't give you the
> "reverse X-mas tree".

What do you mean by this? In the above case the rule does exactly give
you "reversed xmas tree order". What did I miss?

> I don't have any real objections on changing the styling though - I
> don't expect this to be merged before the dependency is in rc1 - so I
> guess I will anyways need to respin this for next cycle. I can do the
> styling then.

Fine with me.

> > 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).

"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. And if you look into the kernel code, I
believe the use you are proposing is in minority.

--
With Best Regards,
Andy Shevchenko

2022-08-20 06:56:54

[permalink] [raw]

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

On 8/20/22 09:25, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
> <[email protected]> wrote:
>> On 8/20/22 02:30, Andy Shevchenko wrote:
>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
>>> <[email protected]> wrote:
>
> What did I miss?

>>>> struct bmg160_data *data;
>>>> struct iio_dev *indio_dev;

This does already violate the rule.

>
>>> 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).
>
> "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.

> 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.

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.

Best Regards
-- Matti

--
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

2022-08-20 07:21:35

[permalink] [raw]

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

On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
<[email protected]> wrote:
> On 8/20/22 09:25, Andy Shevchenko wrote:
> > On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
> > <[email protected]> wrote:
> >> On 8/20/22 02:30, Andy Shevchenko wrote:
> >>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> >>> <[email protected]> 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.

> >>> 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).
> >
> > "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.

> > 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. That's why it's
really not good practice. I would rather see it outside of the
function _esp_ because it's static const.

--
With Best Regards,
Andy Shevchenko

2022-08-20 10:25:06

[permalink] [raw]

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

On 8/20/22 10:18, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
> <[email protected]> wrote:
>> On 8/20/22 09:25, Andy Shevchenko wrote:
>>> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
>>> <[email protected]> wrote:
>>>> On 8/20/22 02:30, Andy Shevchenko wrote:
>>>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
>>>>> <[email protected]> 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..

>>>>> 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.

>>>
>>> "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/[email protected]/
https://lore.kernel.org/all/[email protected]/

>>> 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? 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. 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.

> 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. :(

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

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

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));

2022-08-20 11:20:05

[permalink] [raw]

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

On Fri, 19 Aug 2022 22:19:31 +0300
Matti Vaittinen <[email protected]> wrote:

> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
> bulk-enable, add-action-to-disable-at-detach - pattern.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
>
> ---
> v2 => v3
> Split to own patch.
> ---
> drivers/iio/gyro/bmg160_core.c | 24 +++---------------------
> 1 file changed, 3 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> index cedd9f02ea21..baa80980c99f 100644
> --- a/drivers/iio/gyro/bmg160_core.c
> +++ b/drivers/iio/gyro/bmg160_core.c
> @@ -93,7 +93,6 @@
>
> struct bmg160_data {
> struct regmap *regmap;
> - struct regulator_bulk_data regulators[2];
> struct iio_trigger *dready_trig;
> struct iio_trigger *motion_trig;
> struct iio_mount_matrix orientation;
> @@ -1067,19 +1066,13 @@ static const char *bmg160_match_acpi_device(struct device *dev)
> return dev_name(dev);
> }
>
> -static void bmg160_disable_regulators(void *d)
> -{
> - struct bmg160_data *data = d;
> -
> - regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> -}
> -
> int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
> const char *name)
> {
> struct bmg160_data *data;
> struct iio_dev *indio_dev;
> int ret;
> + static const char * const regulators[] = {"vdd", "vddio"};

As in previous, small preference for spaces after { and before }

>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> @@ -1090,22 +1083,11 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
> data->irq = irq;
> data->regmap = regmap;
>
> - data->regulators[0].supply = "vdd";
> - data->regulators[1].supply = "vddio";
> - ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
> - data->regulators);
> + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
> + regulators);
> if (ret)
> return dev_err_probe(dev, ret, "Failed to get regulators\n");
>
> - ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> - data->regulators);
> - if (ret)
> - return ret;
> -
> - ret = devm_add_action_or_reset(dev, bmg160_disable_regulators, data);
> - if (ret)
> - return ret;
> -
> ret = iio_read_mount_matrix(dev, &data->orientation);
> if (ret)
> return ret;

2022-08-20 11:30:13

[permalink] [raw]

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

On Sat, 20 Aug 2022 06:19:00 +0000
"Vaittinen, Matti" <[email protected]> wrote:

> Thanks for the review Andy
>
> On 8/20/22 02:30, Andy Shevchenko wrote:
> > On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> > <[email protected]> wrote:
> >>
> >> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
> >> bulk-enable, add-action-to-disable-at-detach - pattern.
> >
> > ...
> >
> >> int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
> >> const char *name)
> >> {
> >> struct bmg160_data *data;
> >> struct iio_dev *indio_dev;
> >> int ret;
> >> + static const char * const regulators[] = {"vdd", "vddio"};
> >
> > Please, keep this following the "longest line first" rule. Note, in
>
> This was not following the (IMO slightly silly) rule even prior my
> patch. I can for sure move my line up - but that won't give you the
> "reverse X-mas tree".
>
> I don't have any real objections on changing the styling though - I
> don't expect this to be merged before the dependency is in rc1 - so I
> guess I will anyways need to respin this for next cycle. I can do the
> styling then.
I was a bit surprised Mark didn't do an immutable branch for this, but
indeed looks like it's going to be a multiple cycle thing - so we'll
probably have a bunch of new cases introduced in the meantime that
we need to tidy up. Ah well.

>
> > 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).
>
> > P.S. Same applies for the rest of the similar places in your series.
> >
>
> Br,
> -- Matti
>

2022-08-20 13:46:19

[permalink] [raw]

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

On 8/20/22 14:38, Jonathan Cameron wrote:
> On Sat, 20 Aug 2022 06:19:00 +0000
> "Vaittinen, Matti" <[email protected]> wrote:
>
>> Thanks for the review Andy
>>
>> On 8/20/22 02:30, Andy Shevchenko wrote:
>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
>>> <[email protected]> wrote:
>> >>I
>> don't expect this to be merged before the dependency is in rc1 - so I
>> guess I will anyways need to respin this for next cycle. I can do the
>> styling then.
> I was a bit surprised Mark didn't do an immutable branch for this, but
> indeed looks like it's going to be a multiple cycle thing - so we'll
> probably have a bunch of new cases introduced in the meantime that
> we need to tidy up. Ah well.
>

I guess we can ask Mark what he thinks of the immutable branch ;) I
admit I am not too keen on rebasing this - especially if I find the time
to go through more drivers.

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

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

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));

2022-08-20 13:46:21

[permalink] [raw]

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

On 8/20/22 14:22, Jonathan Cameron wrote:
> On Fri, 19 Aug 2022 22:19:31 +0300
> Matti Vaittinen <[email protected]> wrote:
>
>> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
>> bulk-enable, add-action-to-disable-at-detach - pattern.
>>
>> Signed-off-by: Matti Vaittinen <[email protected]>
>>
>> ---
>> v2 => v3
>> Split to own patch.
>> ---
>> drivers/iio/gyro/bmg160_core.c | 24 +++---------------------
>> 1 file changed, 3 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
>> index cedd9f02ea21..baa80980c99f 100644
>> --- a/drivers/iio/gyro/bmg160_core.c
>> +++ b/drivers/iio/gyro/bmg160_core.c
>> @@ -93,7 +93,6 @@
>>
>> struct bmg160_data {
>> struct regmap *regmap;
>> - struct regulator_bulk_data regulators[2];
>> struct iio_trigger *dready_trig;
>> struct iio_trigger *motion_trig;
>> struct iio_mount_matrix orientation;
>> @@ -1067,19 +1066,13 @@ static const char *bmg160_match_acpi_device(struct device *dev)
>> return dev_name(dev);
>> }
>>
>> -static void bmg160_disable_regulators(void *d)
>> -{
>> - struct bmg160_data *data = d;
>> -
>> - regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
>> -}
>> -
>> int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
>> const char *name)
>> {
>> struct bmg160_data *data;
>> struct iio_dev *indio_dev;
>> int ret;
>> + static const char * const regulators[] = {"vdd", "vddio"};
>
> As in previous, small preference for spaces after { and before }

Thanks. I'll fix it when I respin.

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

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

2022-08-20 16:26:00

[permalink] [raw]

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

On Sat, Aug 20, 2022 at 1:05 PM Matti Vaittinen
<[email protected]> wrote:
> On 8/20/22 10:18, Andy Shevchenko wrote:
> > On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
> > <[email protected]> wrote:
> >> On 8/20/22 09:25, Andy Shevchenko wrote:
> >>> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
> >>> <[email protected]> wrote:
> >>>> On 8/20/22 02:30, Andy Shevchenko wrote:
> >>>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> >>>>> <[email protected]> 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?

> >>>>> 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?

> >>> "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/[email protected]/
> https://lore.kernel.org/all/[email protected]/

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.

> >>> 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. 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. 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...

> > 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.

--
With Best Regards,
Andy Shevchenko

2022-08-20 17:52:15

[permalink] [raw]

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

On 8/20/22 19:21, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 1:05 PM Matti Vaittinen
> <[email protected]> wrote:
>> On 8/20/22 10:18, Andy Shevchenko wrote:
>>> On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
>>> <[email protected]> wrote:
>>>> On 8/20/22 09:25, Andy Shevchenko wrote:
>>>>> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
>>>>> <[email protected]> wrote:
>>>>>> On 8/20/22 02:30, Andy Shevchenko wrote:
>>>>>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
>>>>>>> <[email protected]> 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/[email protected]/
>> https://lore.kernel.org/all/[email protected]/
>
> 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! ~~

2022-08-21 13:26:32

[permalink] [raw]

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

On Sat, Aug 20, 2022 at 8:27 PM Matti Vaittinen
<[email protected]> wrote:
> On 8/20/22 19:21, Andy Shevchenko wrote:
> > On Sat, Aug 20, 2022 at 1:05 PM Matti Vaittinen
> > <[email protected]> wrote:
> >> On 8/20/22 10:18, Andy Shevchenko wrote:
> >>> On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
> >>> <[email protected]> wrote:
> >>>> On 8/20/22 09:25, Andy Shevchenko wrote:
> >>>>> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
> >>>>> <[email protected]> wrote:
> >>>>>> On 8/20/22 02:30, Andy Shevchenko wrote:
> >>>>>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> >>>>>>> <[email protected]> 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.

Yes, this is a style issue and not a real coding problem. As a
reviewer with some of a background I feel that reversed xmas tree
ordering is better to read, maintain, and review. So, as a reviewer I
have an opinion, as a maintainer I can speak for the IIO subsystem,
since it's not on my watch.

> >>>>>>> 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.

Yes, and it's irrelevant to what I'm trying to tell you.

> >>>>> "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/[email protected]/
> >> https://lore.kernel.org/all/[email protected]/
> >
> > 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.

SInce it's static it's global by nature, but local by namespace.

> It causes no more name collisions than a regular
> local variable does so I really don't understand your reasoning.

And I have no other words to explain it to you. You are using a global
variable in the scope of function. This is not good for the
maintenance and development as it's prone to get an issue in the
future.

> >>>>> 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.

Because they are on the stack and not one per module. When you read
the code it's very easy to miss that the variable is static if you
have a lot of other variables defined.

> > 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.

Good for you.

> 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?

It won't be created by function, it's created by the compiler /
linker. It won't gone if function gone. e.g.

__init foo()
{
static bar ...;
}

is nonsense. And it takes a ROM space.

> >>> 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.

...

To summarize, we have a huge disagreement on the placement of the
static variables. Not sure we ever get into compromize here, so I
leave it up to maintainers, but my opinion that it is simply a bad
code practice.

--
With Best Regards,
Andy Shevchenko

2022-08-22 06:23:11

[permalink] [raw]

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

On 8/21/22 16:08, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 8:27 PM Matti Vaittinen
> <[email protected]> wrote:
>> On 8/20/22 19:21, Andy Shevchenko wrote:
>>> On Sat, Aug 20, 2022 at 1:05 PM Matti Vaittinen
>>> <[email protected]> wrote:
>>>> On 8/20/22 10:18, Andy Shevchenko wrote:
>>>>> On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
>>>>> <[email protected]> wrote:
>>>>>> On 8/20/22 09:25, Andy Shevchenko wrote:
>>>>>>> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
>>>>>>> <[email protected]> wrote:
>>>>>>>> On 8/20/22 02:30, Andy Shevchenko wrote:
>>>>>>>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
>>>>>>>>> <[email protected]> wrote:

//snip

> SInce it's static it's global by nature, but local by namespace.

Which is an _improvement_ over having it in global namespace?

>> It causes no more name collisions than a regular
>> local variable does so I really don't understand your reasoning.
>
> And I have no other words to explain it to you. You are using a global
> variable in the scope of function. This is not good for the
> maintenance and development as it's prone to get an issue in the
> future.

If you foresee some issues, please describe them as I don't see one
single problem with a local static const data. I have seen you telling
me that "static const" variables are _harder_ for you to review. Could
you please explain what are the potential mistake(s) a reviewer can do,
and what is the 'issue' that mistake can cause?

>>> 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.
>
> ...
>
>
> To summarize, we have a huge disagreement on the placement of the
> static variables. Not sure we ever get into compromize here, so I
> leave it up to maintainers, but my opinion that it is simply a bad
> code practice.

Bad and good are labels we can place on things. We however need to have
the reason for those labels to be meaningful. I am sorry but I don't
want to label the local _const_ static variables bad without reason.
This discussion starts to remind me on statements like "using goto is
always bad" or "one must never use macros in C".

Yeah - ultimately it is a maintainer decision.

Best Regards
-- Matti

--
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

2022-08-30 12:00:02

by Nuno Sa

[permalink] [raw]

Subject: RE: [PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

> From: Matti Vaittinen <[email protected]>
> Sent: Friday, August 19, 2022 9:21 PM
> To: Matti Vaittinen <[email protected]>; Matti Vaittinen
> <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael
> <[email protected]>; Jonathan Cameron
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: [PATCH v3 14/14] iio: hmc425a: simplify using
> devm_regulator_get_enable()
>
> [External]
>
> Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
> add_action_or_reset(regulator_disable)' and use the
> devm_regulator_get_enable() and drop the pointer to the regulator.
> This simplifies code and makes it less tempting to add manual control
> for the regulator which is also controlled by devm.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
>
> ---

Acked-by: Nuno S? <[email protected]>

(I see that in this patch you are not using dev_err_probe(). Maybe some
consistency in the series and where applicable would be appropriate :))

2022-08-30 12:02:46

by Nuno Sa

[permalink] [raw]

Subject: RE: [PATCH v3 13/14] iio: max1363: simplify using devm_regulator_get_enable()

> -----Original Message-----
> From: Matti Vaittinen <[email protected]>
> Sent: Friday, August 19, 2022 9:21 PM
> To: Matti Vaittinen <[email protected]>; Matti Vaittinen
> <[email protected]>
> Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>; Matti Vaittinen <[email protected]>;
> Alexandru Ardelean <[email protected]>; linux-
> [email protected]; [email protected]
> Subject: [PATCH v3 13/14] iio: max1363: simplify using
> devm_regulator_get_enable()
>
> [External]
>
> Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
> add_action_or_reset(regulator_disable)' and use the
> devm_regulator_get_enable() and drop the pointer to the regulator.
> This simplifies code and makes it less tempting to add manual control
> for the regulator which is also controlled by devm.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
>
> ---

Acked-by: Nuno S? <[email protected]>

2022-08-30 13:21:28

[permalink] [raw]

Subject: Re: [PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

On 8/30/22 14:49, Sa, Nuno wrote:
>
>> From: Matti Vaittinen <[email protected]>
>> Sent: Friday, August 19, 2022 9:21 PM
>> To: Matti Vaittinen <[email protected]>; Matti Vaittinen
>> <[email protected]>
>> Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael
>> <[email protected]>; Jonathan Cameron
>> <[email protected]>; [email protected]; linux-
>> [email protected]
>> Subject: [PATCH v3 14/14] iio: hmc425a: simplify using
>> devm_regulator_get_enable()
>>
>> [External]
>>
>> Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
>> add_action_or_reset(regulator_disable)' and use the
>> devm_regulator_get_enable() and drop the pointer to the regulator.
>> This simplifies code and makes it less tempting to add manual control
>> for the regulator which is also controlled by devm.
>>
>> Signed-off-by: Matti Vaittinen <[email protected]>
>>
>> ---
>
> Acked-by: Nuno Sá <[email protected]>
>
> (I see that in this patch you are not using dev_err_probe(). Maybe some
> consistency in the series and where applicable would be appropriate :))

I don't think the driver did originally print an error if regulator get
or enable failed. I didn't want to add any new prints - just converted
the existing ones to use dev_err_probe(). I believe adding new prints
would've been somewhat unrelated change :)

Yours,
-- Matti

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

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

2022-08-30 14:43:10

by Nuno Sa

[permalink] [raw]

Subject: RE: [PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

> -----Original Message-----
> From: Matti Vaittinen <[email protected]>
> Sent: Tuesday, August 30, 2022 3:00 PM
> To: Sa, Nuno <[email protected]>; Matti Vaittinen
> <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael
> <[email protected]>; Jonathan Cameron
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH v3 14/14] iio: hmc425a: simplify using
> devm_regulator_get_enable()
>
> [External]
>
> On 8/30/22 14:49, Sa, Nuno wrote:
> >
> >> From: Matti Vaittinen <[email protected]>
> >> Sent: Friday, August 19, 2022 9:21 PM
> >> To: Matti Vaittinen <[email protected]>; Matti Vaittinen
> >> <[email protected]>
> >> Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael
> >> <[email protected]>; Jonathan Cameron
> >> <[email protected]>; [email protected]; linux-
> >> [email protected]
> >> Subject: [PATCH v3 14/14] iio: hmc425a: simplify using
> >> devm_regulator_get_enable()
> >>
> >> [External]
> >>
> >> Drop open-coded pattern: 'devm_regulator_get(),
> regulator_enable(),
> >> add_action_or_reset(regulator_disable)' and use the
> >> devm_regulator_get_enable() and drop the pointer to the
> regulator.
> >> This simplifies code and makes it less tempting to add manual
> control
> >> for the regulator which is also controlled by devm.
> >>
> >> Signed-off-by: Matti Vaittinen <[email protected]>
> >>
> >> ---
> >
> > Acked-by: Nuno Sá <[email protected]>
> >
> > (I see that in this patch you are not using dev_err_probe(). Maybe
> some
> > consistency in the series and where applicable would be appropriate
> :))
>
> I don't think the driver did originally print an error if regulator get
> or enable failed. I didn't want to add any new prints - just converted
> the existing ones to use dev_err_probe(). I believe adding new prints
> would've been somewhat unrelated change :)
>

Ahh that makes sense. I failed to see the print that you were replacing
in the ad7606 patch...

- Nuno Sá

2022-10-16 16:22:17

[permalink] [raw]

Subject: Re: [PATCH v3 13/14] iio: max1363: simplify using devm_regulator_get_enable()

On Tue, 30 Aug 2022 11:50:05 +0000
"Sa, Nuno" <[email protected]> wrote:

> > -----Original Message-----
> > From: Matti Vaittinen <[email protected]>
> > Sent: Friday, August 19, 2022 9:21 PM
> > To: Matti Vaittinen <[email protected]>; Matti Vaittinen
> > <[email protected]>
> > Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > <[email protected]>; Matti Vaittinen <[email protected]>;
> > Alexandru Ardelean <[email protected]>; linux-
> > [email protected]; [email protected]
> > Subject: [PATCH v3 13/14] iio: max1363: simplify using
> > devm_regulator_get_enable()
> >
> > [External]
> >
> > Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
> > add_action_or_reset(regulator_disable)' and use the
> > devm_regulator_get_enable() and drop the pointer to the regulator.
> > This simplifies code and makes it less tempting to add manual control
> > for the regulator which is also controlled by devm.
> >
> > Signed-off-by: Matti Vaittinen <[email protected]>
> >
> > ---
>
> Acked-by: Nuno Sá <[email protected]>

Applied.

2022-10-16 16:24:17

[permalink] [raw]

Subject: Re: [PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

On Tue, 30 Aug 2022 13:55:38 +0000
"Sa, Nuno" <[email protected]> wrote:

> > -----Original Message-----
> > From: Matti Vaittinen <[email protected]>
> > Sent: Tuesday, August 30, 2022 3:00 PM
> > To: Sa, Nuno <[email protected]>; Matti Vaittinen
> > <[email protected]>
> > Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael
> > <[email protected]>; Jonathan Cameron
> > <[email protected]>; [email protected]; linux-
> > [email protected]
> > Subject: Re: [PATCH v3 14/14] iio: hmc425a: simplify using
> > devm_regulator_get_enable()
> >
> > [External]
> >
> > On 8/30/22 14:49, Sa, Nuno wrote:
> > >
> > >> From: Matti Vaittinen <[email protected]>
> > >> Sent: Friday, August 19, 2022 9:21 PM
> > >> To: Matti Vaittinen <[email protected]>; Matti Vaittinen
> > >> <[email protected]>
> > >> Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael
> > >> <[email protected]>; Jonathan Cameron
> > >> <[email protected]>; [email protected]; linux-
> > >> [email protected]
> > >> Subject: [PATCH v3 14/14] iio: hmc425a: simplify using
> > >> devm_regulator_get_enable()
> > >>
> > >> [External]
> > >>
> > >> Drop open-coded pattern: 'devm_regulator_get(),
> > regulator_enable(),
> > >> add_action_or_reset(regulator_disable)' and use the
> > >> devm_regulator_get_enable() and drop the pointer to the
> > regulator.
> > >> This simplifies code and makes it less tempting to add manual
> > control
> > >> for the regulator which is also controlled by devm.
> > >>
> > >> Signed-off-by: Matti Vaittinen <[email protected]>
> > >>
> > >> ---
> > >
> > > Acked-by: Nuno Sá <[email protected]>
> > >
> > > (I see that in this patch you are not using dev_err_probe(). Maybe
> > some
> > > consistency in the series and where applicable would be appropriate
> > :))
> >
> > I don't think the driver did originally print an error if regulator get
> > or enable failed. I didn't want to add any new prints - just converted
> > the existing ones to use dev_err_probe(). I believe adding new prints
> > would've been somewhat unrelated change :)
> >
>
> Ahh that makes sense. I failed to see the print that you were replacing
> in the ad7606 patch...
>
Applied to the togreg branch of iio.git though for now that's just pushed
out as testing.

As I mentioned on an earlier patch I forgot these existed and was about
to duplicate some of the work.. Anyhow, we only overlapped on a few
patches so I'll send the rest of my set out shortly.

There are probably other cases still in IIO but they are fiddly to grep
for!

Jonathan
> - Nuno Sá

2022-10-16 16:34:41

[permalink] [raw]

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

On Fri, 19 Aug 2022 22:19:31 +0300
Matti Vaittinen <[email protected]> wrote:

> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
> bulk-enable, add-action-to-disable-at-detach - pattern.
>
> Signed-off-by: Matti Vaittinen <[email protected]>

Applied with tweaks:
- patch title includes gyro:
- ordering as Andy suggested
- spaces after { and before }
>
> ---
> v2 => v3
> Split to own patch.
> ---
> drivers/iio/gyro/bmg160_core.c | 24 +++---------------------
> 1 file changed, 3 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> index cedd9f02ea21..baa80980c99f 100644
> --- a/drivers/iio/gyro/bmg160_core.c
> +++ b/drivers/iio/gyro/bmg160_core.c
> @@ -93,7 +93,6 @@
>
> struct bmg160_data {
> struct regmap *regmap;
> - struct regulator_bulk_data regulators[2];
> struct iio_trigger *dready_trig;
> struct iio_trigger *motion_trig;
> struct iio_mount_matrix orientation;
> @@ -1067,19 +1066,13 @@ static const char *bmg160_match_acpi_device(struct device *dev)
> return dev_name(dev);
> }
>
> -static void bmg160_disable_regulators(void *d)
> -{
> - struct bmg160_data *data = d;
> -
> - regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> -}
> -
> int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
> const char *name)
> {
> struct bmg160_data *data;
> struct iio_dev *indio_dev;
> int ret;
> + static const char * const regulators[] = {"vdd", "vddio"};
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> @@ -1090,22 +1083,11 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
> data->irq = irq;
> data->regmap = regmap;
>
> - data->regulators[0].supply = "vdd";
> - data->regulators[1].supply = "vddio";
> - ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
> - data->regulators);
> + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
> + regulators);
> if (ret)
> return dev_err_probe(dev, ret, "Failed to get regulators\n");
>
> - ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> - data->regulators);
> - if (ret)
> - return ret;
> -
> - ret = devm_add_action_or_reset(dev, bmg160_disable_regulators, data);
> - if (ret)
> - return ret;
> -
> ret = iio_read_mount_matrix(dev, &data->orientation);
> if (ret)
> return ret;

2022-10-16 16:53:33

[permalink] [raw]

Subject: Re: [PATCH v3 13/14] iio: max1363: simplify using devm_regulator_get_enable()

On Sun, 16 Oct 2022 17:18:38 +0100
Jonathan Cameron <[email protected]> wrote:

> On Tue, 30 Aug 2022 11:50:05 +0000
> "Sa, Nuno" <[email protected]> wrote:
>
> > > -----Original Message-----
> > > From: Matti Vaittinen <[email protected]>
> > > Sent: Friday, August 19, 2022 9:21 PM
> > > To: Matti Vaittinen <[email protected]>; Matti Vaittinen
> > > <[email protected]>
> > > Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > <[email protected]>; Matti Vaittinen <[email protected]>;
> > > Alexandru Ardelean <[email protected]>; linux-
> > > [email protected]; [email protected]
> > > Subject: [PATCH v3 13/14] iio: max1363: simplify using
> > > devm_regulator_get_enable()
> > >
> > > [External]
> > >
> > > Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
> > > add_action_or_reset(regulator_disable)' and use the
> > > devm_regulator_get_enable() and drop the pointer to the regulator.
> > > This simplifies code and makes it less tempting to add manual control
> > > for the regulator which is also controlled by devm.
> > >
> > > Signed-off-by: Matti Vaittinen <[email protected]>
> > >
> > > ---
> >
> > Acked-by: Nuno Sá <[email protected]>
>
> Applied.

This one was missing cleaning up the docs as well. fixed up.

2022-10-17 04:52:55