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

[Mårten Lindahl]

Hi Mark!

On 5/4/23 14:04, Mark Brown wrote:
> 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.
I understand. I'll change it. Explanation below why I did it like this.
> > +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.

The reason for doing like this is that all 4 device IDs support all 4 
voltage ranges:

  0.4-V to 0.71875-V in 1.25-mV steps
  0.4-V to 1.0375-V in 2.5-mV steps
  0.4-V to 1.675-V in 5-mV steps
  0.8-V to 3.35-V in 10-mV steps

Of which the third is default for all devices. The range is solely 
selected by a register
field (no hardware pin connection), so I can't associate a specific 
device ID with a
specific voltage range, which is why I let the DT properties min/max 
decide the range.

But I see now it should be done in another way. I can think of 2 ways to 
implement it:

1. Introduce a DT property for this driver, like "ti,vrange-selector" 
and select one of
    the 4 voltage ranges by desc->of_parse_cb. This way allows a new 
voltage only to be set
    if it is within the selected range.

2. Dynamically set the range when a new voltage is set. This way any 
voltage from
    0.4V to 3.35V could be set if the DT node has:
        regulator-min-microvolt = <400000>;
        regulator-max-microvolt = <3350000>;

I hope I was clear enough with my reasoning. Maybe there are better ways 
to do it?

Kind regards

Mårten

> > +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.