Messages in this thread | | | Date | Fri, 22 Jul 2022 12:12:05 +0200 | Subject | Re: [PATCH v1 08/14] regulator: drivers: Add TI TPS65219 PMIC regulators support | From | jerome Neanne <> |
| |
On 19/07/2022 15:32, Mark Brown wrote: > On Tue, Jul 19, 2022 at 11:17:36AM +0200, Jerome Neanne wrote: > >> @@ -0,0 +1,414 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * tps65219-regulator.c >> + * > Please make the entire comment a C++ one so things look more > intentional. > >> +static int tps65219_pmic_set_voltage_sel(struct regulator_dev *dev, >> + unsigned int selector) >> +{ >> + int ret; >> + struct tps65219 *tps = rdev_get_drvdata(dev); >> + >> + /* Set the voltage based on vsel value */ >> + ret = regmap_update_bits(tps->regmap, dev->desc->vsel_reg, >> + dev->desc->vsel_mask, selector); >> + if (ret) { >> + dev_dbg(tps->dev, "%s failed for regulator %s: %d ", >> + __func__, dev->desc->name, ret); >> + } >> + return ret; >> +} > This should just be able to use the standard regmap helper, as should > the enable and disable operations? > >> +static int tps65219_set_mode(struct regulator_dev *dev, unsigned int mode) >> +{ >> + struct tps65219 *tps = rdev_get_drvdata(dev); >> + >> + switch (mode) { >> + case REGULATOR_MODE_NORMAL: >> + return regmap_set_bits(tps->regmap, TPS65219_REG_STBY_1_CONFIG, >> + dev->desc->enable_mask); >> + >> + case REGULATOR_MODE_STANDBY: >> + return regmap_clear_bits(tps->regmap, >> + TPS65219_REG_STBY_1_CONFIG, >> + dev->desc->enable_mask); >> + } >> + >> + return -EINVAL; > It'd be a little clearer to have that -EINVAL in a default statement. > >> +static irqreturn_t tps65219_regulator_irq_handler(int irq, void *data) >> +{ >> + struct tps65219_regulator_irq_data *irq_data = data; >> + >> + if (irq_data->type->event_name[0] == '\0') { >> + /* This is the timeout interrupt */ >> + dev_err(irq_data->dev, "System was put in shutdown during an active or standby transition.\n"); >> + return IRQ_HANDLED; >> + } >> + >> + dev_err(irq_data->dev, "Registered %s for %s\n", >> + irq_data->type->event_name, irq_data->type->regulator_name); > This should be reporting the events through the notification API, see > regulator_notifier_call_chain(). That will require a bit of refactoring > of the way the driver is registering interrupts unfortunately, at the > minute it doesn't have data joining them up with the > > I'd also reword that log message to be something more like "Error %s > reported for %s" - at the minute it looks more like a probe message. > > Otherwise this looks good. Thanks for your review.
Refactoring the code with regulator_notifier_call_chain, I realized that some of the events in TPS65219 are not listed as standard REGULATOR_EVENT in consumer.h
This is the case for below event list: REGULATOR_EVENT_SCG (ShortCut to Gnd)
REGULATOR_EVENT_RV (Residual Voltage)
REGULATOR_EVENT_RV_SD (Residual Voltage ShutDown)
Should I add those events to the list of standard regulator events and assign a code? (if yes, any rule for the values?) Would it fit with some other predefined standard macro defined elsewhere? (if yes, could you point me to the right location?)
> @@ -0,0 +1,414 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * tps65219-regulator.c > + *
Please make the entire comment a C++ one so things look more intentional.
checkpatch is complaining about that:
--------------------------------------------------------------------- v5.19-rc6-PB-MSP1/0005-mfd-drivers-Add-TI-TPS65219-PMIC-support.patch --------------------------------------------------------------------- WARNING: Improper SPDX comment style for 'drivers/mfd/tps65219.c', please use '//' instead #91: FILE: drivers/mfd/tps65219.c:1: +/* SPDX-License-Identifier: GPL-2.0
Let me know if I should ignore checkpatch recommendations here.
Regards, Jerome
| |