linux-kernel - Re: [RFC PATCH 3/5] 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: <Yqc10QuvjQ1e/ivw@sirena.org.uk>
Date:   Mon, 13 Jun 2022 14:04:17 +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, will@...nel.org, lee.jones@...aro.org,
        khilman@...libre.com, narmstrong@...libre.com, msp@...libre.com,
        j-keerthy@...com, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH 3/5] regulator: drivers: Add TI TPS65219 PMIC
 regulators support

On Mon, Jun 13, 2022 at 11:06:02AM +0200, Jerome NEANNE wrote:

> +static int tps65219_pmic_enable(struct regulator_dev *dev)
> +{
> +	struct tps65219 *tps = rdev_get_drvdata(dev);
> +	int rid = rdev_get_id(dev);
> +	int ret;
> +
> +
> +

Lots of random blank space here.

> +	if (rid < TPS65219_BUCK_1 || rid > TPS65219_LDO_4)
> +		return -EINVAL;
> +

If a regulator doesn't support an operation it shouldn't be providing
it, no need to check here.

> +	ret = tps65219_set_bits(tps, dev->desc->enable_reg,
> +				 dev->desc->enable_mask, dev->desc->enable_mask);

If you provide the regmap to the child devices then this can use the
standard regmap helpers for most of the operations, meaning most of
the operations in the driver can be removed.

> +/* generic regulator_set_bypass_regmap does not match requirements use custom instead */

In what way?  Please also try to keep within 80 columns normally.

> +	if (dev->desc->ops->is_enabled) {
> +		dev_err(tps->dev, "%s LDO%d is enabled, should be shut down to set bypass ",
> +					 __func__, rid);
> +		return -EBUSY;
> +	}

This is testing if there is an enable operation, not if the regulator is
enabled.

> +	if (enable) {
> +		dev_dbg(tps->dev, "%s, LDO%d already in bypass mode", __func__, rid);
> +		return ret;
> +	}

This is not a problem, just silently succeed.

> +	/* Allocate memory for strobes */
> +	tps->strobes = devm_kcalloc(&pdev->dev,
> +				    TPS65219_NUM_REGULATOR, sizeof(u8),
> +				    GFP_KERNEL);
> +	if (!tps->strobes)
> +		return -ENOMEM;

These are never referenced anywhere else in the driver, no need to
allocate them.

> +		ret = regmap_read(tps->regmap, regulators[i].bypass_reg, &val);
> +		if (ret) {
> +			dev_err(tps->dev, "dev_err failed to map register for %s regulator\n",
> +				pdev->name);
> +			return ret;
> +		}

I'm not clear what this check is intended to do, it at least needs to be
better documented.

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