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: <bd4aa2ad-4535-94ca-7630-846546ae3d82@baylibre.com>
Date:   Thu, 23 Mar 2023 10:12:21 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver
 for TI TPS6594 regulators
Content-Language: en-US
To:     Mark Brown <broonie@kernel.org>,
        Esteban Blanc <eblanc@baylibre.com>
Cc:     linus.walleij@linaro.org, lgirdwood@gmail.com,
        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
References: <20230224133129.887203-1-eblanc@baylibre.com>
 <20230224133129.887203-4-eblanc@baylibre.com>
 <Y/i+wVSy+eQxDFJ3@sirena.org.uk>
From:   jerome Neanne <jneanne@baylibre.com>
In-Reply-To: <Y/i+wVSy+eQxDFJ3@sirena.org.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


>> +static int tps6594_get_rdev_by_name(const char *regulator_name,
>> +				    struct regulator_dev *rdevbucktbl[BUCK_NB],
>> +				    struct regulator_dev *rdevldotbl[LDO_NB],
>> +				    struct regulator_dev *dev)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i <= BUCK_NB; i++) {
>> +		if (strcmp(regulator_name, buck_regs[i].name) == 0) {
>> +			dev = rdevbucktbl[i];
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ldo_regs); i++) {
>> +		if (strcmp(regulator_name, ldo_regs[i].name) == 0) {
>> +			dev = rdevldotbl[i];
>> +			return 0;
>> +		}
>> +	}
>> +	return -EINVAL;
>> +}
> 
>> +	for (i = 0; i < ARRAY_SIZE(tps6594_regulator_irq_types); ++i) {
>> +		irq_type = &tps6594_regulator_irq_types[i];
>> +
>> +		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>> +		if (irq < 0)
>> +			return -EINVAL;
>> +
>> +		irq_data[i].dev = tps->dev;
>> +		irq_data[i].type = irq_type;
>> +
>> +		tps6594_get_rdev_by_name(irq_type->regulator_name, rdevbucktbl,
>> +					 rdevldotbl, rdev);
> 
> This would be simpler and you wouldn't need this lookup function if the
> regulator descriptions included their IRQ names, then you could just
> request the interrupts while registering the regulators.
I changed the code to follow your recommendations then now in case of a 
multiphase buck, only one set of interrupt is requested.
ex: multiphase buck1234 single buck5
cat /proc/interrupts:
563:          0          0  tps6594-0-0x4c   0 Edge      buck1_ov
564:          0          0  tps6594-0-0x4c   1 Edge      buck1_uv
565:          0          0  tps6594-0-0x4c   2 Edge      buck1_sc
566:          0          0  tps6594-0-0x4c   3 Edge      buck1_ilim
579:          0          0  tps6594-0-0x4c  16 Edge      buck5_ov
580:          0          0  tps6594-0-0x4c  17 Edge      buck5_uv
581:          0          0  tps6594-0-0x4c  18 Edge      buck5_sc
582:          0          0  tps6594-0-0x4c  19 Edge      buck5_ilim

buck2, buck3, buck4 are not associated to a regulator device because 
buck1 registers control all the multiphase bucks (only one logic 
regulator). Consequently the mapping for the associated interrupts does 
not occur.
I'm not sure it's the right option.
Do you suggest to keep it like that for multiphase?
Is it better to request all the interrupts anyway and map it to the same 
rdev?

> 
>> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
>> +						  tps6594_regulator_irq_handler,
>> +						  IRQF_ONESHOT,
>> +						  irq_type->irq_name,
>> +						  &irq_data[i]);
>> +		if (error) {
>> +			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
>> +				irq_type->irq_name, irq, error);
>> +			return error;
>> +		}
> 
> This leaks all previously requested interrupts.
I'm not sure to understand this sentence correctly. You mean all the 
interrupts already requested are still allocated after the error occurs?