regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

drivers/regulator/twl-regulator.c static struct regulator_ops twl4030smps_ops = { · line 573

Quoted 10 lines · Tero Kristo

drivers/regulator/twl-regulator.c +10 -0

+static int twl6030coresmps_set_voltage(struct regulator_dev *rdev, int min_uV,
+	int max_uV, unsigned *selector)
+{
+	struct twlreg_info *info = rdev_get_drvdata(rdev);
+
+	if (info->set_voltage)
+		return info->set_voltage(info->data, min_uV);
+
+	return 0;
+}

This should be returning an error if it failed to set the voltage.

Since you're using min_uV as the "register value" you probably ought to be returning that as the selector too and supplying a list_voltage() which just passes the selector back in case something tries to use it and gets confused.

drivers/regulator/twl-regulator.c static struct regulator_ops twl4030smps_ops = { · line 582

Quoted 9 lines

drivers/regulator/twl-regulator.c +8 -0

+static int twl6030coresmps_get_voltage(struct regulator_dev *rdev)
+{
+	struct twlreg_info *info = rdev_get_drvdata(rdev);
+
+	if (info->get_voltage)
+		return info->get_voltage(info->data);
+
+	return 0;

Similarly here.

Quoted 4 lines · Mark Brown

Since you're using min_uV as the "register value" you probably ought to be returning that as the selector too and supplying a list_voltage() which just passes the selector back in case something tries to use it and gets confused.

I was thinking at some point about adding a list_voltage for these regulators, however I dropped that idea, because the regulators can support a range of voltages (from min to max) with some stepping value. But... if you propose that the list_voltage would just return the current voltage back, wouldn't that also potentially confuse the user more, as it can only see the single voltage and nothing else, maybe making it to think that the regulator can only support one voltage level?

Quoted 7 lines · Tero Kristo

Quoted 4 lines · Mark Brown

Since you're using min_uV as the "register value" you probably ought to be returning that as the selector too and supplying a list_voltage() which just passes the selector back in case something tries to use it and gets confused.

Quoted 8 lines

I was thinking at some point about adding a list_voltage for these regulators, however I dropped that idea, because the regulators can support a range of voltages (from min to max) with some stepping value. But... if you propose that the list_voltage would just return the current voltage back, wouldn't that also potentially confuse the user more, as it can only see the single voltage and nothing else, maybe making it to think that the regulator can only support one voltage level?

Yes, that'd be completely broken. You'd need to just return the selector back which would tell them that they had voltage control in microvolt steps.

I believe this would fail still. I took a look at a few drivers that use regulator_list_voltage(), but all of these seem to numerate voltages based on regulator_count_voltages(), which will return -EINVAL for the SMPS ones as the num_voltages is zero. Also, even if I defined num_voltages here, I would be attempting to list_voltage for zero index, returning zero, but this would be invalid voltage for the cpu obviously (and is also out of range for the regulator min_voltage, and also according to docs invalid return value for the function in the first place.)

Review

Quoted 8 lines · Tero Kristo

I still ain't quite sure how this would work, do you mean adding something like this:

(quoted diff) +5 -0

+static int twl6030smps_list_voltage(struct regulator_dev *rdev,
+                                   unsigned int selector)
+{
+       return selector;
+}

Yes.

Quoted 9 lines

I believe this would fail still. I took a look at a few drivers that use regulator_list_voltage(), but all of these seem to numerate voltages based on regulator_count_voltages(), which will return -EINVAL for the SMPS ones as the num_voltages is zero. Also, even if I defined num_voltages here, I would be attempting to list_voltage for zero index, returning zero, but this would be invalid voltage for the cpu obviously (and is also out of range for the regulator min_voltage, and also according to docs invalid return value for the function in the first place.)

Well, clearly some of the values won't actually be useful and you should feel free to return error values for those or apply an offset or something but the basic principle applies.

So, do you want me to also change the num_voltages value for the regulator from zero to be the same as max_uV, as we have this check within regulator/core:

       if (!ops->list_voltage || selector >= rdev->desc->n_voltages)
                return -EINVAL;

This will also potentially make some code to iterate over regulator voltages for ~1.5M times. I still don't think adding list_voltage for the SMPS regulators makes any sense, unless either the API for regulator_list_voltage is changed, or we change the control for these regulators completely from set_voltage() based to set_voltage_sel() based implementation.

Quoted 7 lines · Tero Kristo

So, do you want me to also change the num_voltages value for the regulator from zero to be the same as max_uV, as we have this check within regulator/core:

       if (!ops->list_voltage || selector >= rdev->desc->n_voltages)
                return -EINVAL;

Quoted 6 lines

This will also potentially make some code to iterate over regulator voltages for ~1.5M times. I still don't think adding list_voltage for the SMPS regulators makes any sense, unless either the API for regulator_list_voltage is changed, or we change the control for these regulators completely from set_voltage() based to set_voltage_sel() based implementation.

Well, the important thing here is to fill in something useful for the returned selector rather than just leaving it undefined. Providing a list_voltage() would be nice and make things more robust.

Still, setting selector in this case does nothing, as it is immediately overwritten by the regulator core by -1. This looks like a perfectly acceptable way to implement a regulator, as everything checks for the presence of list_voltage anyway.

Quoted 4 lines · Tero Kristo

Still, setting selector in this case does nothing, as it is immediately overwritten by the regulator core by -1. This looks like a perfectly acceptable way to implement a regulator, as everything checks for the presence of list_voltage anyway.

Ah, so it is - we're fixing things up in the core. I'd forgotten we did that.

Quoted 2 lines · Mark Brown

Ah, so it is - we're fixing things up in the core. I'd forgotten we did that.

So, no need to add list_voltage to this, I just fix the return values for the get / set and send the new version, am I right?

Quoted 7 lines · Tero Kristo

Quoted 2 lines · Mark Brown

Ah, so it is - we're fixing things up in the core. I'd forgotten we did that.

So, no need to add list_voltage to this, I just fix the return values for the get / set and send the new version, am I right?

I guess. Though actually given that there's no interaction with the chip itself at all I'm not sure I understand why this is part of the twl6030 driver rather than being a separate driver for whatever implements the operations - it's not like the case with the other regulators where there was some functionality implemented through the twl6030 driver beyond straight passthrough.