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

[jerome Neanne <jneanne@baylibre.com>]

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