Re: [PATCH v1 08/14] regulator: drivers: Add TI TPS65219 PMIC regulators support

[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