[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <613601c2-cc98-526b-a9e3-2ad2abc68e1d@baylibre.com>
Date: Wed, 7 Jun 2023 13:44:46 +0200
From: jerome Neanne <jneanne@...libre.com>
To: andy.shevchenko@...il.com, Esteban Blanc <eblanc@...libre.com>
Cc: linus.walleij@...aro.org, lgirdwood@...il.com, broonie@...nel.org,
a.zummo@...ertech.it, alexandre.belloni@...tlin.com,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-rtc@...r.kernel.org, jpanis@...libre.com,
aseketeli@...libre.com, u-kumar1@...com
Subject: Re: [PATCH v5 3/3] regulator: tps6594-regulator: Add driver for TI
TPS6594 regulators
>
>> + enum {
>> + MULTI_BUCK12,
>> + MULTI_BUCK123,
>> + MULTI_BUCK1234,
>> + MULTI_BUCK12_34,
>
>> + MULTI_FIRST = MULTI_BUCK12,
>> + MULTI_LAST = MULTI_BUCK12_34,
>> + MULTI_NUM = MULTI_LAST - MULTI_FIRST + 1
>
> MULT_NUM
>
> will suffice instead all this.
>
>> + };
>
> But why enum at all? See below.
Just for the switch case readability.
I have to iterate across the multiphases array for look up name into
device tree and evaluate in that order.
This can be reduced to:
enum {
MULTI_BUCK12,
MULTI_BUCK123,
MULTI_BUCK1234,
MULTI_BUCK12_34,
MULTI_NUM = MULTI_BUCK12_34 - MULTI_BUCK12 + 1
};
>
> ...
>
>> + /*
>> + * Switch case defines different possible multi phase config
>> + * This is based on dts buck node name.
>> + * Buck node name must be chosen accordingly.
>> + * Default case is no Multiphase buck.
>> + * In case of Multiphase configuration, value should be defined for
>> + * buck_configured to avoid creating bucks for every buck in multiphase
>> + */
>> + for (multi = MULTI_FIRST; multi < MULTI_NUM; multi++) {
>> + np = of_find_node_by_name(tps->dev->of_node, multiphases[multi]);
>> + npname = of_node_full_name(np);
>> + np_pmic_parent = of_get_parent(of_get_parent(np));
>> + if (of_node_cmp(of_node_full_name(np_pmic_parent), tps->dev->of_node->full_name))
>
> Why not of_node_full_name() in the second case?
Sure.
>
>
>> + continue;
>> + delta = strcmp(npname, multiphases[multi]);
>> + if (!delta) {
>> + switch (multi) {
>> + case MULTI_BUCK12:
>
> This all looks like match_string() reinvention.
I can go with match_string but this is not significantly changing the game:
index = match_string(multiphases, ARRAY_SIZE(multiphases), npname);
if (index >= 0) {
switch (index) {
No question on all your other feedback. Just wondering if I missed
something with match_string use. Looks like a good idea indeed but this
is not drastically changing the code as you seem to expect... Let me
know if you think I'm doing it in a wrong way.
Regards,
Jerome.
Powered by blists - more mailing lists