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

From: Mark Brown <broonie@kernel.org>
To: Jerome Neanne <jneanne@baylibre.com>
Cc: lgirdwood@gmail.com, robh+dt@kernel.org, nm@ti.com,
	kristo@kernel.org, khilman@baylibre.com, narmstrong@baylibre.com,
	msp@baylibre.com, j-keerthy@ti.c, lee.jones@linaro.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v1 08/14] regulator: drivers: Add TI TPS65219 PMIC regulators support
Date: Tue, 19 Jul 2022 14:32:58 +0100	[thread overview]
Message-ID: <YtayikFdidxXXubS@sirena.org.uk> (raw)
In-Reply-To: <20220719091742.3221-9-jneanne@baylibre.com>

[-- Attachment #1.1: Type: text/plain, Size: 2350 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel