Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <613601c2-cc98-526b-a9e3-2ad2abc68e1d@baylibre.com>
Date:   Wed, 7 Jun 2023 13:44:46 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v5 3/3] regulator: tps6594-regulator: Add driver for TI
 TPS6594 regulators
Content-Language: en-US
To:     andy.shevchenko@gmail.com, Esteban Blanc <eblanc@baylibre.com>
Cc:     linus.walleij@linaro.org, lgirdwood@gmail.com, broonie@kernel.org,
        a.zummo@towertech.it, alexandre.belloni@bootlin.com,
        linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-rtc@vger.kernel.org, jpanis@baylibre.com,
        aseketeli@baylibre.com, u-kumar1@ti.com
References: <20230522163115.2592883-1-eblanc@baylibre.com>
 <20230522163115.2592883-4-eblanc@baylibre.com> <ZG0VHnEByyMW9i4a@surfacebook>
From:   jerome Neanne <jneanne@baylibre.com>
In-Reply-To: <ZG0VHnEByyMW9i4a@surfacebook>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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