Messages in this thread | | | Date | Fri, 24 Feb 2023 13:42:25 +0000 | From | Mark Brown <> | Subject | Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators |
| |
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.
> @@ -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.
> +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. [unhandled content-type:application/pgp-signature] | |