[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 5 May 2023 10:29:58 +0200
From: Mårten Lindahl <martenli@...s.com>
To: Mark Brown <broonie@...nel.org>
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
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.
Powered by blists - more mailing lists