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: <0f7f8b5d-728b-4f97-9100-5879eacb8c93@ti.com>
Date: Mon, 6 Jan 2025 16:57:55 -0600
From: Andrew Davis <afd@...com>
To: Shree Ramamoorthy <s-ramamoorthy@...com>,
        Roger Quadros
	<rogerq@...nel.org>, <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>, <tony@...mide.com>,
        <jerome.neanne@...libre.com>, <linux-omap@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>
CC: <m-leonard@...com>, <praneeth@...com>
Subject: Re: [PATCH v1 6/7] regulator: tps65215: Define probe() helper
 functions

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