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.
