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