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
