linux-kernel - Re: [PATCH v1 6/7] regulator: tps65215: Define probe() helper functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fb684d79-ca6b-4071-9f5c-d89218dd8f81@wanadoo.fr>
Date: Fri, 3 Jan 2025 14:10:12 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Shree Ramamoorthy <s-ramamoorthy@...com>
Cc: m-leonard@...com, praneeth@...com, lgirdwood@...il.com,
 broonie@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, aaro.koskinen@....fi, andreas@...nade.info,
 khilman@...libre.com, rogerq@...nel.org, tony@...mide.com,
 jerome.neanne@...libre.com, linux-omap@...r.kernel.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v1 6/7] regulator: tps65215: Define probe() helper
 functions

Le 03/01/2025 à 00:41, Shree Ramamoorthy a écrit :
> Hi,
> 
> On 1/1/25 5:01 AM, Christophe JAILLET wrote:
>> Le 26/12/2024 à 22:54, Shree Ramamoorthy a écrit :
>>> Factor register_regulators() and request_irqs() out into smaller 
>>> functions.
>>> These 2 helper functions are used in the next restructure probe() 
>>> patch to
>>> go through the common (overlapping) regulators and irqs first, then the
>>> device-specific structs identifed in the chip_data struct.
>>>
>>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy- 
>>> l0cyMroinI0@...lic.gmane.org>
>>> ---
>>>   drivers/regulator/tps65219-regulator.c | 64 ++++++++++++++++++++++++++
>>>   1 file changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/ 
>>> regulator/tps65219-regulator.c
>>> index 13f0e68d8e85..8469ee89802c 100644
>>> --- a/drivers/regulator/tps65219-regulator.c
>>> +++ b/drivers/regulator/tps65219-regulator.c
>>> @@ -346,6 +346,70 @@ static struct chip_data chip_info_table[] = {
>>>       },
>>>   };
>>>   +static int tps65219_register_regulators(const struct 
>>> regulator_desc *regulators,
>>> +                    struct tps65219 *tps,
>>> +                    struct device *dev,
>>> +                    struct regulator_config config,
>>> +                    unsigned int arr_size)
>>> +{
>>> +    int i;
>>> +    struct regulator_dev *rdev;
>>> +
>>> +    config.driver_data = tps;
>>> +    config.dev = tps->dev;
>>> +    config.regmap = tps->regmap;
>>> +
>>> +    for (i = 0; i < arr_size; i++) {
>>> +        rdev = devm_regulator_register(dev, &regulators[i],
>>> +                        &config);
>>> +        if (IS_ERR(rdev)) {
>>> +            dev_err(tps->dev,
>>> +                "Failed to register %s regulator\n",
>>> +                regulators[i].name);
>>
>> This will be called from probe in 7/7.
>> So this could be return dev_err_probe()
>>
> I left these as dev_err(), since dev_err_probe() is used when there is a 
> chance
> -EPROBE_DEFER is returned. For both functions using dev_err() here, - 
> ENOMEM is returned.
> Should I still switch these 2 instances to dev_err_probe()?
> 
> Thank you for your help!
> 
>>> +
>>> +            return PTR_ERR(rdev);
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int tps65219_request_irqs(struct tps65219_regulator_irq_type 
>>> *irq_types,
>>> +                 struct tps65219 *tps, struct platform_device *pdev,
>>> +                 struct tps65219_regulator_irq_data *irq_data,
>>> +                 unsigned int arr_size)
>>> +{
>>> +    int i;
>>> +    int irq;
>>> +    int error;
>>> +    struct tps65219_regulator_irq_type *irq_type;
>>> +
>>> +    for (i = 0; i < arr_size; ++i) {
>>> +        irq_type = &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;
>>> +
>>> +        error = devm_request_threaded_irq(tps->dev, irq, NULL,
>>> +                          tps65219_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);
>>
>> This will be called from probe in 7/7.
>> So this could be return dev_err_probe()

Up to you to choose one or the other.

The other advantages of using dev_err_probe() are:
   - log the error code in a human readable format
   - combined with return, it usually saves a few LoC, because some { } 
can be removed most of the time.

CJ

>>
>>> +            return error;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int tps65219_regulator_probe(struct platform_device *pdev)
>>>   {
>>>       struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
>>
>