linux-kernel - Re: [PATCH v2 2/2] regulator: Add support for TI TPS6287x regulators

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: <ZFOfX+PTsmA35TsC@finisterre.sirena.org.uk>
Date:   Thu, 4 May 2023 21:04:47 +0900
From:   Mark Brown <broonie@...nel.org>
To:     Mårten Lindahl <marten.lindahl@...s.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        kernel@...s.com
Subject: Re: [PATCH v2 2/2] regulator: Add support for TI TPS6287x regulators

On Thu, May 04, 2023 at 10:30:27AM +0200, Mårten Lindahl wrote:

> +static int tps6287x_get_voltage(struct regulator_dev *rdev)
> +{
> +	struct device *dev = rdev_get_dev(rdev);
> +	struct tps6287x_chip *chip =
> +	    i2c_get_clientdata(to_i2c_client(dev->parent));
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(rdev->regmap, TPS6287X_VSET, &val);
> +	if (ret != 0)
> +		return -ENOTRECOVERABLE;
> +
> +	return (val * chip->uv_step) + rdev->constraints->min_uV;
> +}

Don't open code the voltage conversion, just use selectors - in which
case you can simply describe the bitfield that the device has and use
the generic regmap helpers.

The driver should also never be referring to constraints to figure out
what the register values mean, this is just not going to work - boards
will typically be able to use far fewer voltages than the regulator
supports.

Also try to avoid squashing error codes, just pass the result back.

> +static int tps6287x_set_voltage(struct regulator_dev *rdev, int min_uv,
> +				int max_uv, unsigned int *selector)

Similarly here, describe the bitfield and use the generic helpers.

> +static int tps6287x_setup_vrange(struct tps6287x_chip *chip)
> +{
> +	struct regulator_dev *rdev = chip->rdev;
> +	unsigned int val, r;
> +	bool found = false;
> +	int ret;
> +
> +	/*
> +	 * Match DT voltage range to one of the predefined ranges,
> +	 * and configure the regulator with the selected range.
> +	 */
> +	for (r = 0; r < ARRAY_SIZE(tps6287x_voltage_table); r++) {
> +		if (tps6287x_voltage_table[r][0] == rdev->constraints->min_uV &&
> +		    tps6287x_voltage_table[r][1] == rdev->constraints->max_uV) {
> +			found = true;
> +			break;
> +		}
> +	}

No, as I said above the driver should just know what the device
supports based on the device ID.  In general if a regulator driver is
looking at the constraints that indicates that it's doing something
wrong, the purpose of constraints is to grant permission for the
features of the regulator to be used on the board.

> +static const struct of_device_id tps6287x_dt_ids[] = {
> +	{ .compatible = "ti,tps62870", },
> +	{ .compatible = "ti,tps62871", },
> +	{ .compatible = "ti,tps62872", },
> +	{ .compatible = "ti,tps62873", },
> +	{ }
> +};

Use the .data field here...

> +static const struct i2c_device_id tps6287x_i2c_id[] = {
> +	{ "tps62870", 0 },
> +	{ "tps62871", 0 },
> +	{ "tps62872", 0 },
> +	{ "tps62873", 0 },
> +	{},
> +};

...and here to enumerate which of the variants is being used and hence
which voltage range is required.

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