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

[Mark Brown]

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.

Attachment: signature.asc
Description: PGP signature