Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[jerome Neanne <jneanne@baylibre.com>]

On 24/02/2023 14:42, Mark Brown wrote:
> On Fri, Feb 24, 2023 at 02:31:29PM +0100, Esteban Blanc wrote:
>> From: Jerome Neanne <jneanne@baylibre.com>
>>
>> This patch adds support for TPS6594 regulators (bucks and LDOs).
>> The output voltages are configurable and are meant to supply power
>> to the main processor and other components.
>> Bucks can be used in single or multiphase mode, depending on PMIC
>> part number.
>>
>> Signed-off-by: Jerome Neanne <jneanne@baylibre.com>
>> ---
> 
> You've not provided a Signed-off-by for this so I can't do anything with
> it, please see Documentation/process/submitting-patches.rst for details
> on what this is and why it's important.
> 
I did this patch but Esteban sent the whole patch-list. The sign-off has 
not been updated accordingly. Sorry for disturbance. We'll fix that.
>> @@ -0,0 +1,559 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Regulator driver for tps6594 PMIC
>> + *
>> + * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
> 
> Please make the entire comment block a C++ one so things look more
> intentional.
> 
>> +static unsigned int tps6594_get_mode(struct regulator_dev *dev)
>> +{
>> +	return REGULATOR_MODE_NORMAL;
>> +}
> 
> If configuring modes isn't supported just omit all mode operations.
> 
>> +	}
>> +
>> +	regulator_notifier_call_chain(irq_data->rdev,
>> +				      irq_data->type->event, NULL);
>> +
>> +	dev_err(irq_data->dev, "Error IRQ trap %s for %s\n",
>> +		irq_data->type->event_name, irq_data->type->regulator_name);
> 
> I suspect it might avoid future confusion to log the error before
> notifying so that any consequences of the error more clearly happen in
> response to the error.
> 
I'll rework all that section for v2 following your recommendations
>> +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.
> 
>> +		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.
Thanks for your time and precious feedback.