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

[Shree Ramamoorthy <s-ramamoorthy@ti.com>]

Hi,

On 1/6/25 4:57 PM, Andrew Davis wrote:
> On 1/6/25 4:02 PM, Shree Ramamoorthy wrote:
>> Hi,
>>
>> On 1/4/2025 12:45 PM, Roger Quadros wrote:
>>>
>>> On 26/12/2024 23:54, Shree Ramamoorthy wrote:
>>>> 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@ti.com>
>>>> ---
>>>>   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;
>>> reverse xmas tree?
>>
>> Applied reverse xmas tree style to this file & will review other 
>> files as well for this.
>>
>>>> +
>>>> +    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);
>>>> +
>>>> +            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;
>>> here too.
>>>
>>>> +
>>>> +    for (i = 0; i < arr_size; ++i) {
>>>> +        irq_type = &irq_types[i];
>>>> +
>>> unnecessary new line.
>>>
>>>> +        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;
>>>> +
>>> here too
>>
>> Removed both new lines.
>>
>>>> +        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);
>>>> +            return error;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static int tps65219_regulator_probe(struct platform_device *pdev)
>>>>   {
>>>>       struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
>>> This patch by itself will complain during build as there are no 
>>> users for
>>> these functions.
>>> Could you please squash patches 6 and 7?
>>
>> I kept patch 6 and 7 separate as the diff was hard to read &
>> the git diff options did not resolve this. Is there a way to keep 
>> these 2 patches
>> separate for user readability and avoid the build error? Or just 
>> squash them to
>> prevent build errors knowing the diff will be hard to read? Thank you 
>> for your help!
>>
>>
>
> Instead of splitting the adding and the using of the functions, could you
> split tps65219_register_regulators() and tps65219_request_irqs() into 
> their
> own patches? Each patch should add and also make use of the added 
> function.
>
> Andrew

I was able to split up the 2 helper functions & usage into their own patches. The diff is clean
except for a mistaken new function, but it's easy to read compared to squashing this patch with 7/7.


-- 
Best,
Shree Ramamoorthy
PMIC Software Engineer