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

["Mårten Lindahl" <martenli@axis.com>]

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.