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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YtayikFdidxXXubS@sirena.org.uk>
Date:   Tue, 19 Jul 2022 14:32:58 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Jerome Neanne <jneanne@...libre.com>
Cc:     lgirdwood@...il.com, robh+dt@...nel.org, nm@...com,
        kristo@...nel.org, khilman@...libre.com, narmstrong@...libre.com,
        msp@...libre.com, j-keerthy@...c, lee.jones@...aro.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v1 08/14] regulator: drivers: Add TI TPS65219 PMIC
 regulators support

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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)