diff mbox

[v3,4/5] regulators: tps65912: Add regulator driver for the TPS65912 PMIC

Message ID 1443106374-4126-5-git-send-email-afd@ti.com
State New
Headers show

Commit Message

Andrew Davis Sept. 24, 2015, 2:52 p.m. UTC
This patch adds support for TPS65912 PMIC regulators.

The regulators set consists of 4 DCDCs and 10 LDOs. The output
voltages are configurable and are meant to supply power to the
main processor and other components.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/regulator/Kconfig              |   6 +
 drivers/regulator/Makefile             |   1 +
 drivers/regulator/tps65912-regulator.c | 240 +++++++++++++++++++++++++++++++++
 3 files changed, 247 insertions(+)
 create mode 100644 drivers/regulator/tps65912-regulator.c

Comments

Mark Brown Sept. 25, 2015, 6:05 p.m. UTC | #1
On Thu, Sep 24, 2015 at 09:52:53AM -0500, Andrew F. Davis wrote:

> +static int tps65912_regulator_probe(struct platform_device *pdev)
> +{
> +	struct tps65912 *tps = dev_get_drvdata(pdev->dev.parent);
> +	struct regulator_init_data *init_data;
> +	const struct tps_info *template;
> +	struct regulator_dev *rdev;
> +	const struct of_device_id *match;
> +	struct regulator_config config = { };
> +	int id;
> +
> +	match = of_match_device(tps65912_regulator_of_match_table, &pdev->dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	template = match->data;
> +	id = template->id;
> +	init_data = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
> +					       &regulators[id]);

Don't open code this stuff, use the core DT matching in the
regulator_desc instead.

Please also use subject lines mathcing the style for the subsystem.
Andrew Davis Sept. 25, 2015, 8:10 p.m. UTC | #2
On 09/25/2015 01:05 PM, Mark Brown wrote:
> On Thu, Sep 24, 2015 at 09:52:53AM -0500, Andrew F. Davis wrote:
>
>> +static int tps65912_regulator_probe(struct platform_device *pdev)
>> +{
>> +	struct tps65912 *tps = dev_get_drvdata(pdev->dev.parent);
>> +	struct regulator_init_data *init_data;
>> +	const struct tps_info *template;
>> +	struct regulator_dev *rdev;
>> +	const struct of_device_id *match;
>> +	struct regulator_config config = { };
>> +	int id;
>> +
>> +	match = of_match_device(tps65912_regulator_of_match_table, &pdev->dev);
>> +	if (!match)
>> +		return -ENODEV;
>> +
>> +	template = match->data;
>> +	id = template->id;
>> +	init_data = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
>> +					       &regulators[id]);
>
> Don't open code this stuff, use the core DT matching in the
> regulator_desc instead.
>

I assume you are referring to your additions in a0c7b164ad11? If so I'm not
sure that will save me anything as my probe function is called with a DT
match already, so no searching is needed.

> Please also use subject lines mathcing the style for the subsystem.
>

I'm not sure I know what you mean? What is wrong with my subject line, it looks
like the others I've looked at?
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Sept. 29, 2015, 3:13 p.m. UTC | #3
On Fri, Sep 25, 2015 at 03:10:04PM -0500, Andrew F. Davis wrote:
> On 09/25/2015 01:05 PM, Mark Brown wrote:
> >On Thu, Sep 24, 2015 at 09:52:53AM -0500, Andrew F. Davis wrote:

> >>+	match = of_match_device(tps65912_regulator_of_match_table, &pdev->dev);
> >>+	if (!match)
> >>+		return -ENODEV;

> >>+	template = match->data;
> >>+	id = template->id;
> >>+	init_data = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
> >>+					       &regulators[id]);

> >Don't open code this stuff, use the core DT matching in the
> >regulator_desc instead.

> I assume you are referring to your additions in a0c7b164ad11? If so I'm not

Please don't refer to commits by ID only, include a human readable
description of the commit for the benefit of any humans who might read
your mail.

> sure that will save me anything as my probe function is called with a DT
> match already, so no searching is needed.

You've not understood what that change is replacing, the code I'm
quoting above is exactly that code.  Check out some of the existing
drivers using this API.

> >Please also use subject lines mathcing the style for the subsystem.

> I'm not sure I know what you mean? What is wrong with my subject line, it looks
> like the others I've looked at?

You are using "regulators:", the regulator API uses "regulator:" - this
should be really clear if you do something like a git shortlog.
Andrew Davis Sept. 29, 2015, 6:08 p.m. UTC | #4
On 09/29/2015 10:13 AM, Mark Brown wrote:
> On Fri, Sep 25, 2015 at 03:10:04PM -0500, Andrew F. Davis wrote:
>> On 09/25/2015 01:05 PM, Mark Brown wrote:
>>> On Thu, Sep 24, 2015 at 09:52:53AM -0500, Andrew F. Davis wrote:
>
>>>> +	match = of_match_device(tps65912_regulator_of_match_table, &pdev->dev);
>>>> +	if (!match)
>>>> +		return -ENODEV;
>
>>>> +	template = match->data;
>>>> +	id = template->id;
>>>> +	init_data = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
>>>> +					       &regulators[id]);
>
>>> Don't open code this stuff, use the core DT matching in the
>>> regulator_desc instead.
>
>> I assume you are referring to your additions in a0c7b164ad11? If so I'm not
>
> Please don't refer to commits by ID only, include a human readable
> description of the commit for the benefit of any humans who might read
> your mail.
>

Forgot about those pesky humans :), OK, will do from now on.

>> sure that will save me anything as my probe function is called with a DT
>> match already, so no searching is needed.
>
> You've not understood what that change is replacing, the code I'm
> quoting above is exactly that code.  Check out some of the existing
> drivers using this API.
>

Looking at other drivers that use this API they all call regulator_register
in a loop in their probe, once for each possible regulator, in this case
letting the API do the DT node search makes sense. My probe on the other-hand
is only called when we already have a DT match, therefor searching is not
necessary and all I have to do is call of_get_regulator_init_data myself on
the already found DT node. No need to add node names to my regulator_desc
and make the API re-search for the node.

I have further cleaned up this code to show this.

If this is acceptable, I'll push the re-spin so you can review further.

>>> Please also use subject lines mathcing the style for the subsystem.
>
>> I'm not sure I know what you mean? What is wrong with my subject line, it looks
>> like the others I've looked at?
>
> You are using "regulators:", the regulator API uses "regulator:" - this
> should be really clear if you do something like a git shortlog.
>

Ah, slipped right past me, thanks for clarifying, fixed.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Sept. 29, 2015, 6:38 p.m. UTC | #5
On Tue, Sep 29, 2015 at 01:08:50PM -0500, Andrew F. Davis wrote:
> On 09/29/2015 10:13 AM, Mark Brown wrote:

> >>sure that will save me anything as my probe function is called with a DT
> >>match already, so no searching is needed.

> >You've not understood what that change is replacing, the code I'm
> >quoting above is exactly that code.  Check out some of the existing
> >drivers using this API.

> Looking at other drivers that use this API they all call regulator_register
> in a loop in their probe, once for each possible regulator, in this case
> letting the API do the DT node search makes sense. My probe on the other-hand
> is only called when we already have a DT match, therefor searching is not
> necessary and all I have to do is call of_get_regulator_init_data myself on
> the already found DT node. No need to add node names to my regulator_desc
> and make the API re-search for the node.

Oh, ick.  The binding has a compatible string in the individual
regulator bindings which is broken unless there really are lots of
variants being configured via DT (which is just not the case here).
It's not only more typing in the DT, it also means that we can't read
back the configuration of the device unless the user goes and creates a
DT which explicitly lists each regulator on the device which is
unhelpful.  We should be able to read back the configurations of all the
regulators by simply listing the device in DT.

The fact that this is different to the bindings for other regulator
drivers and requires more code ought to have been a big warning sign
here :(
Andrew Davis Sept. 29, 2015, 6:58 p.m. UTC | #6
On 09/29/2015 01:38 PM, Mark Brown wrote:
> On Tue, Sep 29, 2015 at 01:08:50PM -0500, Andrew F. Davis wrote:
>> On 09/29/2015 10:13 AM, Mark Brown wrote:
>
>>>> sure that will save me anything as my probe function is called with a DT
>>>> match already, so no searching is needed.
>
>>> You've not understood what that change is replacing, the code I'm
>>> quoting above is exactly that code.  Check out some of the existing
>>> drivers using this API.
>
>> Looking at other drivers that use this API they all call regulator_register
>> in a loop in their probe, once for each possible regulator, in this case
>> letting the API do the DT node search makes sense. My probe on the other-hand
>> is only called when we already have a DT match, therefor searching is not
>> necessary and all I have to do is call of_get_regulator_init_data myself on
>> the already found DT node. No need to add node names to my regulator_desc
>> and make the API re-search for the node.
>
> Oh, ick.  The binding has a compatible string in the individual
> regulator bindings which is broken unless there really are lots of
> variants being configured via DT (which is just not the case here).
> It's not only more typing in the DT,

I don't see this, the alternative is matching to this "regulator-compatible",
why not just use the existing compatible.

> it also means that we can't read
> back the configuration of the device unless the user goes and creates a
> DT which explicitly lists each regulator on the device which is
> unhelpful.  We should be able to read back the configurations of all the
> regulators by simply listing the device in DT.
>

Could you expand this? I'm not sure I understand why we still cant do this
using this new way.

Bindings should have compatible strings when they describe hardware like this,
we can then do stuff like put the LDO and DCDC drivers in separate modules for
instance, letting DT only load what we need. There are other benefits like
not having to search our own DT binding for data, and we only get probed for
devices in the DT.

This also eliminates the need for MFD_CORE, we just call
of_platform_populate on ourself and DT helpers do the rest. Why hard code
mfd_cell's and do matching when DT does the same thing.

> The fact that this is different to the bindings for other regulator
> drivers and requires more code ought to have been a big warning sign
> here :(
>

The binding is the same as the new tps65218 driver, different isn't always
a warning sign. And what do you mean "requires more code"? This regulator
driver is smaller than almost any other. DT takes care of everything for
us relating to hardware instantiation like it should.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Sept. 30, 2015, 5:28 p.m. UTC | #7
On Tue, Sep 29, 2015 at 01:58:41PM -0500, Andrew F. Davis wrote:
> On 09/29/2015 01:38 PM, Mark Brown wrote:

> >Oh, ick.  The binding has a compatible string in the individual
> >regulator bindings which is broken unless there really are lots of
> >variants being configured via DT (which is just not the case here).
> >It's not only more typing in the DT,

> I don't see this, the alternative is matching to this "regulator-compatible",
> why not just use the existing compatible.

No, you don't need to use regulator-compatible - that's deprecated.
Just use the node names.

> >it also means that we can't read
> >back the configuration of the device unless the user goes and creates a
> >DT which explicitly lists each regulator on the device which is
> >unhelpful.  We should be able to read back the configurations of all the
> >regulators by simply listing the device in DT.

> Could you expand this? I'm not sure I understand why we still cant do this
> using this new way.

I'm not sure what there is to add...  if the regulator is only
instantiated when it features in the device tree then obviously it must
be included in the device to be instantiated.

> Bindings should have compatible strings when they describe hardware like this,
> we can then do stuff like put the LDO and DCDC drivers in separate modules for
> instance, letting DT only load what we need. There are other benefits like
> not having to search our own DT binding for data, and we only get probed for
> devices in the DT.

Only getting probed for device is in DT is exactly the problem here, and
nothing prevents us having separate modules for things without
enumerating everything in DT.

> This also eliminates the need for MFD_CORE, we just call
> of_platform_populate on ourself and DT helpers do the rest. Why hard code
> mfd_cell's and do matching when DT does the same thing.

Putting everything in DT means more work for people integrating the
device and means that we have to have a full and complete understanding
of the device at the time we write the DT, including decions about how
we split the functionality of the device between subsystems.

> >The fact that this is different to the bindings for other regulator
> >drivers and requires more code ought to have been a big warning sign
> >here :(

> The binding is the same as the new tps65218 driver, different isn't always
> a warning sign. And what do you mean "requires more code"? This regulator
> driver is smaller than almost any other. DT takes care of everything for
> us relating to hardware instantiation like it should.

That's not a new driver, it's from more than a year ago (before or about
the same time the helpers got added IIRC).
Andrew Davis Sept. 30, 2015, 8:29 p.m. UTC | #8
On 09/30/2015 12:28 PM, Mark Brown wrote:
> On Tue, Sep 29, 2015 at 01:58:41PM -0500, Andrew F. Davis wrote:
>> On 09/29/2015 01:38 PM, Mark Brown wrote:
>
>>> Oh, ick.  The binding has a compatible string in the individual
>>> regulator bindings which is broken unless there really are lots of
>>> variants being configured via DT (which is just not the case here).
>>> It's not only more typing in the DT,
>
>> I don't see this, the alternative is matching to this "regulator-compatible",
>> why not just use the existing compatible.
>
> No, you don't need to use regulator-compatible - that's deprecated.
> Just use the node names.
>

Are we sure matching on node names is a good idea? Most are just arbitrary
names meant to be human readable and reference-able, giving them function
may lead to confusion. This seems to be why we have "compatible", for specific
identification of node function. But I'm new so maybe I'm wrong?

>>> it also means that we can't read
>>> back the configuration of the device unless the user goes and creates a
>>> DT which explicitly lists each regulator on the device which is
>>> unhelpful.  We should be able to read back the configurations of all the
>>> regulators by simply listing the device in DT.
>
>> Could you expand this? I'm not sure I understand why we still cant do this
>> using this new way.
>
> I'm not sure what there is to add...  if the regulator is only
> instantiated when it features in the device tree then obviously it must
> be included in the device to be instantiated.
>

This is already the case then, missing regulator nodes in old drivers will not
get instantiated ether. And old drivers don't always store any more info about
available regulators than mine does.

>> Bindings should have compatible strings when they describe hardware like this,
>> we can then do stuff like put the LDO and DCDC drivers in separate modules for
>> instance, letting DT only load what we need. There are other benefits like
>> not having to search our own DT binding for data, and we only get probed for
>> devices in the DT.
>
> Only getting probed for device is in DT is exactly the problem here, and
> nothing prevents us having separate modules for things without
> enumerating everything in DT.
>

Sure, but then we have to do some fiddling with MFD_CORE to do that work,
why not remove the dependency and let DT do that for DT only drivers?

>> This also eliminates the need for MFD_CORE, we just call
>> of_platform_populate on ourself and DT helpers do the rest. Why hard code
>> mfd_cell's and do matching when DT does the same thing.
>
> Putting everything in DT means more work for people integrating the
> device and means that we have to have a full and complete understanding
> of the device at the time we write the DT, including decions about how
> we split the functionality of the device between subsystems.
>

We are not adding anything extra to the DT node, we just use the "compatible"
string to identify and match the node vs. "regulator-name", or the nodes name,
or whatever else has been used. The node is then just filled with the standard
optional properties just like every other driver's node.

>>> The fact that this is different to the bindings for other regulator
>>> drivers and requires more code ought to have been a big warning sign
>>> here :(
>
>> The binding is the same as the new tps65218 driver, different isn't always
>> a warning sign. And what do you mean "requires more code"? This regulator
>> driver is smaller than almost any other. DT takes care of everything for
>> us relating to hardware instantiation like it should.
>
> That's not a new driver, it's from more than a year ago (before or about
> the same time the helpers got added IIRC).
>

Newer than a lot, I chose to base my driver off of that not just because
it is a similar TI part, but because it was the cleanest, simplest looking
one IMHO. The helpers would require more code (you need to know how many
regulators you have and call the helpers in a loop).

I have another PMIC I'm about to push a driver for when this gets figured out
that does the same thing, and it's more important I think to do it this way for
this new part. Some of the new regulators are designed without a dedicated
SOC or board to power in mind, so they will have a whole bunch for different
regulator types on one chip and it will be up to the designer to pick which ones
to turn on and use. With this DT approach you can just list the ones you want,
and we may even be able to split different types into different modules, then
we can use the same regulator driver in different spins of the PMIC with more
or less of that type of regulator, we just add that same node under a different
parent PMIC driver.

I could use the helper with this style and save a couple lines of code if we
could make regulator_of_get_init_data to also match on "compatible", it
currently only matches on "regulator-compatible" or the node name. I could make
this addition and send the patch if you would like to see what I have in mind.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Sept. 30, 2015, 10:20 p.m. UTC | #9
On Wed, Sep 30, 2015 at 03:29:30PM -0500, Andrew F. Davis wrote:
> On 09/30/2015 12:28 PM, Mark Brown wrote:

> >No, you don't need to use regulator-compatible - that's deprecated.
> >Just use the node names.

> Are we sure matching on node names is a good idea? Most are just arbitrary
> names meant to be human readable and reference-able, giving them function
> may lead to confusion. This seems to be why we have "compatible", for specific
> identification of node function. But I'm new so maybe I'm wrong?

Yes, I'm sure.  The regulators are just properties on the device, they
are not devices themselves.

> >I'm not sure what there is to add...  if the regulator is only
> >instantiated when it features in the device tree then obviously it must
> >be included in the device to be instantiated.

> This is already the case then, missing regulator nodes in old drivers will not
> get instantiated ether. And old drivers don't always store any more info about
> available regulators than mine does.

No, well implemented older drivers will still unconditionally register
everything.

> >Only getting probed for device is in DT is exactly the problem here, and
> >nothing prevents us having separate modules for things without
> >enumerating everything in DT.

> Sure, but then we have to do some fiddling with MFD_CORE to do that work,
> why not remove the dependency and let DT do that for DT only drivers?

For the reasons I've outlined repeatedly.

> >Putting everything in DT means more work for people integrating the
> >device and means that we have to have a full and complete understanding
> >of the device at the time we write the DT, including decions about how
> >we split the functionality of the device between subsystems.

> We are not adding anything extra to the DT node, we just use the "compatible"
> string to identify and match the node vs. "regulator-name", or the nodes name,
> or whatever else has been used. The node is then just filled with the standard
> optional properties just like every other driver's node.

No, you are missing the point.  The point is that only regulators
explicitly listed in the DT will be instantiated.

> Newer than a lot, I chose to base my driver off of that not just because
> it is a similar TI part, but because it was the cleanest, simplest looking
> one IMHO. The helpers would require more code (you need to know how many
> regulators you have and call the helpers in a loop).

You're talking about a trivial loop that takes perhaps a couple of lines
to open and close the for loop and another line to declare an iterator
variable.

Look, please stop arguing about this.  There appears to be nothing
special about this device that makes it different to other devices.

> I have another PMIC I'm about to push a driver for when this gets figured out
> that does the same thing, and it's more important I think to do it this way for
> this new part. Some of the new regulators are designed without a dedicated
> SOC or board to power in mind, so they will have a whole bunch for different
> regulator types on one chip and it will be up to the designer to pick which ones
> to turn on and use. With this DT approach you can just list the ones you want,
> and we may even be able to split different types into different modules, then
> we can use the same regulator driver in different spins of the PMIC with more
> or less of that type of regulator, we just add that same node under a different
> parent PMIC driver.

This sounds exactly the same as the majority of PMIC drivers, there is
nothing remarkable here.

> I could use the helper with this style and save a couple lines of code if we
> could make regulator_of_get_init_data to also match on "compatible", it
> currently only matches on "regulator-compatible" or the node name. I could make
> this addition and send the patch if you would like to see what I have in mind.

No, this would be broken - devices are instantiated via the driver
model.
Andrew Davis Sept. 30, 2015, 11:32 p.m. UTC | #10
On 09/30/2015 05:20 PM, Mark Brown wrote:
> On Wed, Sep 30, 2015 at 03:29:30PM -0500, Andrew F. Davis wrote:
>> On 09/30/2015 12:28 PM, Mark Brown wrote:
>
>>> No, you don't need to use regulator-compatible - that's deprecated.
>>> Just use the node names.
>
>> Are we sure matching on node names is a good idea? Most are just arbitrary
>> names meant to be human readable and reference-able, giving them function
>> may lead to confusion. This seems to be why we have "compatible", for specific
>> identification of node function. But I'm new so maybe I'm wrong?
>
> Yes, I'm sure.  The regulators are just properties on the device, they
> are not devices themselves.
>

OK

>>> I'm not sure what there is to add...  if the regulator is only
>>> instantiated when it features in the device tree then obviously it must
>>> be included in the device to be instantiated.
>
>> This is already the case then, missing regulator nodes in old drivers will not
>> get instantiated ether. And old drivers don't always store any more info about
>> available regulators than mine does.
>
> No, well implemented older drivers will still unconditionally register
> everything.
>

Is this desired? If they are not in the DT could this be used to signal
we don't want to register this regulator?

>>> Only getting probed for device is in DT is exactly the problem here, and
>>> nothing prevents us having separate modules for things without
>>> enumerating everything in DT.
>
>> Sure, but then we have to do some fiddling with MFD_CORE to do that work,
>> why not remove the dependency and let DT do that for DT only drivers?
>
> For the reasons I've outlined repeatedly.
>
>>> Putting everything in DT means more work for people integrating the
>>> device and means that we have to have a full and complete understanding
>>> of the device at the time we write the DT, including decions about how
>>> we split the functionality of the device between subsystems.
>
>> We are not adding anything extra to the DT node, we just use the "compatible"
>> string to identify and match the node vs. "regulator-name", or the nodes name,
>> or whatever else has been used. The node is then just filled with the standard
>> optional properties just like every other driver's node.
>
> No, you are missing the point.  The point is that only regulators
> explicitly listed in the DT will be instantiated.
>

Same questions as above. ^^^

>> Newer than a lot, I chose to base my driver off of that not just because
>> it is a similar TI part, but because it was the cleanest, simplest looking
>> one IMHO. The helpers would require more code (you need to know how many
>> regulators you have and call the helpers in a loop).
>
> You're talking about a trivial loop that takes perhaps a couple of lines
> to open and close the for loop and another line to declare an iterator
> variable.
>

Adding any extra code and complexity to use a helper that does something
that already works with less doesn't make much sense to me, but if that's
the API you want I'll use it.

> Look, please stop arguing about this.  There appears to be nothing
> special about this device that makes it different to other devices.
>

I'm sorry if I sounded overly argumentative about this, I'm just trying to make
points in favor of me not having to re-write my driver.

The new framework helpers do not help my driver as it does things differently.
You signed off on this different way of doing things just last year with the
TPS65218.

If you no longer want it done this way then I'll go and change my driver.

>> I have another PMIC I'm about to push a driver for when this gets figured out
>> that does the same thing, and it's more important I think to do it this way for
>> this new part. Some of the new regulators are designed without a dedicated
>> SOC or board to power in mind, so they will have a whole bunch for different
>> regulator types on one chip and it will be up to the designer to pick which ones
>> to turn on and use. With this DT approach you can just list the ones you want,
>> and we may even be able to split different types into different modules, then
>> we can use the same regulator driver in different spins of the PMIC with more
>> or less of that type of regulator, we just add that same node under a different
>> parent PMIC driver.
>
> This sounds exactly the same as the majority of PMIC drivers, there is
> nothing remarkable here.
>

Nothing meant to be remarkable, I was pointing out uses.

>> I could use the helper with this style and save a couple lines of code if we
>> could make regulator_of_get_init_data to also match on "compatible", it
>> currently only matches on "regulator-compatible" or the node name. I could make
>> this addition and send the patch if you would like to see what I have in mind.
>
> No, this would be broken - devices are instantiated via the driver
> model.
>

OK
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Oct. 1, 2015, 10:53 a.m. UTC | #11
On Wed, Sep 30, 2015 at 06:32:14PM -0500, Andrew F. Davis wrote:
> On 09/30/2015 05:20 PM, Mark Brown wrote:

> >>This is already the case then, missing regulator nodes in old drivers will not
> >>get instantiated ether. And old drivers don't always store any more info about
> >>available regulators than mine does.

> >No, well implemented older drivers will still unconditionally register
> >everything.

> Is this desired? If they are not in the DT could this be used to signal
> we don't want to register this regulator?

Yes, this is desired.  The point is that there is no situation in which
we do not want to register all the regulators physically present on the
device.

> >You're talking about a trivial loop that takes perhaps a couple of lines
> >to open and close the for loop and another line to declare an iterator
> >variable.

> Adding any extra code and complexity to use a helper that does something
> that already works with less doesn't make much sense to me, but if that's
> the API you want I'll use it.

It would take a lot less effort to implement the requested changes
rather than go on and on about this.  There is no meaningful complexity
here, we're talking about removing code and moving some data tables from
DT (where they are an ABI) to code (where they are not an ABI).

> >Look, please stop arguing about this.  There appears to be nothing
> >special about this device that makes it different to other devices.

> I'm sorry if I sounded overly argumentative about this, I'm just trying to make
> points in favor of me not having to re-write my driver.

It's hardly a rewrite.

> The new framework helpers do not help my driver as it does things differently.
> You signed off on this different way of doing things just last year with the
> TPS65218.

> If you no longer want it done this way then I'll go and change my driver.

It should already be apparent that this is not a desired configuration,
old drivers are never a good reason not to do the right thing on new
code.
Grygorii Strashko Oct. 1, 2015, 3:33 p.m. UTC | #12
Hi Andrew,
 
On 09/30/2015 03:29 PM, Andrew F. Davis wrote:
> On 09/30/2015 12:28 PM, Mark Brown wrote:
>> On Tue, Sep 29, 2015 at 01:58:41PM -0500, Andrew F. Davis wrote:
>>> On 09/29/2015 01:38 PM, Mark Brown wrote:
>>
>>>> Oh, ick.  The binding has a compatible string in the individual
>>>> regulator bindings which is broken unless there really are lots of
>>>> variants being configured via DT (which is just not the case here).
>>>> It's not only more typing in the DT,
>>
>>> I don't see this, the alternative is matching to this 
>>> "regulator-compatible",
>>> why not just use the existing compatible.
>>
>> No, you don't need to use regulator-compatible - that's deprecated.
>> Just use the node names.
>>
> 
> Are we sure matching on node names is a good idea? Most are just arbitrary
> names meant to be human readable and reference-able, giving them function
> may lead to confusion. This seems to be why we have "compatible", for 
> specific
> identification of node function. But I'm new so maybe I'm wrong?
> 
>>>> it also means that we can't read
>>>> back the configuration of the device unless the user goes and creates a
>>>> DT which explicitly lists each regulator on the device which is
>>>> unhelpful.  We should be able to read back the configurations of all 
>>>> the
>>>> regulators by simply listing the device in DT.
>>
>>> Could you expand this? I'm not sure I understand why we still cant do 
>>> this
>>> using this new way.
>>
>> I'm not sure what there is to add...  if the regulator is only
>> instantiated when it features in the device tree then obviously it must
>> be included in the device to be instantiated.
>>
> 
> This is already the case then, missing regulator nodes in old drivers 
> will not
> get instantiated ether. And old drivers don't always store any more info 
> about
> available regulators than mine does.
> 
>>> Bindings should have compatible strings when they describe hardware 
>>> like this,
>>> we can then do stuff like put the LDO and DCDC drivers in separate 
>>> modules for
>>> instance, letting DT only load what we need. There are other benefits 
>>> like
>>> not having to search our own DT binding for data, and we only get 
>>> probed for
>>> devices in the DT.
>>
>> Only getting probed for device is in DT is exactly the problem here, and
>> nothing prevents us having separate modules for things without
>> enumerating everything in DT.
>>
> 
> Sure, but then we have to do some fiddling with MFD_CORE to do that work,
> why not remove the dependency and let DT do that for DT only drivers?
> 
>>> This also eliminates the need for MFD_CORE, we just call
>>> of_platform_populate on ourself and DT helpers do the rest. Why hard 
>>> code
>>> mfd_cell's and do matching when DT does the same thing.
>>
>> Putting everything in DT means more work for people integrating the
>> device and means that we have to have a full and complete understanding
>> of the device at the time we write the DT, including decions about how
>> we split the functionality of the device between subsystems.
>>
> 
> We are not adding anything extra to the DT node, we just use the 
> "compatible"
> string to identify and match the node vs. "regulator-name", or the nodes 
> name,
> or whatever else has been used. The node is then just filled with the 
> standard
> optional properties just like every other driver's node.
> 
>>>> The fact that this is different to the bindings for other regulator
>>>> drivers and requires more code ought to have been a big warning sign
>>>> here :(
>>
>>> The binding is the same as the new tps65218 driver, different isn't 
>>> always
>>> a warning sign. And what do you mean "requires more code"? This 
>>> regulator
>>> driver is smaller than almost any other. DT takes care of everything for
>>> us relating to hardware instantiation like it should.
>>
>> That's not a new driver, it's from more than a year ago (before or about
>> the same time the helpers got added IIRC).
>>
> 
> Newer than a lot, I chose to base my driver off of that not just because
> it is a similar TI part, but because it was the cleanest, simplest looking
> one IMHO. The helpers would require more code (you need to know how many
> regulators you have and call the helpers in a loop).
> 
> I have another PMIC I'm about to push a driver for when this gets 
> figured out
> that does the same thing, and it's more important I think to do it this 
> way for
> this new part. Some of the new regulators are designed without a dedicated
> SOC or board to power in mind, so they will have a whole bunch for 
> different
> regulator types on one chip and it will be up to the designer to pick 
> which ones
> to turn on and use. With this DT approach you can just list the ones you 
> want,
> and we may even be able to split different types into different modules, 
> then
> we can use the same regulator driver in different spins of the PMIC with 
> more
> or less of that type of regulator, we just add that same node under a 
> different
> parent PMIC driver.
> 

That's all make sense only if you have all information required for driver probing in DT.
But you don't, because  a lot of information are still hard-coded in driver:
- regulator IDs,
- registers required to control regulator
- ranges information and etc.

So, you will not be be able to easily separate some regulator and reuse it with another PMIC.

Actually, both approaches have right to live and which one to select depends on 
functionality which has to be implemented by regulator driver. 
For example, with this (your) approach the separate platform device will be created 
for each regulator, so, if needed, corresponding driver's callback (.remove/shutdown and
dev_pm_ops)  can be implemented individually for each regulator. Do you need this?
As for me, such approach is reasonable if you have devices which represents one/two regulators max.

The disadvantage of this approach is that you need separate compatible string fore each regulator
- and this, actually, is usually banned by DT maintainers ;)

For devices, like this one (or twl, or plamas) which has dozens of regulators it simply make no sense :)
Just imaging that tomorrow tps65912-1 will be released and it will be fully compatible with  tps65912, but
with one exception - it will have dcdc5 and all followed registers offsets will be shifted.
What will you do? All this code, based on compatible strings, will not work any more.
And you will need to introduce another bunch of compatible strings.. 

By the way, this implementation is not optimal any way :)
- you are using compatible string to get tps65912_pmic_regs structure
- then tps65912_pmic_regs is used to get regulator ID
- and then, finally, regulator ID is used to get regulator_desc structure   

And, finally, pay attention pls, that regulator_of_get_init_data() is called from
from regulator_register().

So, in my opinion, the tps65217-regulator.c driver is really good example of how it could be done.
Andrew Davis Oct. 1, 2015, 4:08 p.m. UTC | #13
On 10/01/2015 10:33 AM, Grygorii Strashko wrote:
> Hi Andrew,
>
> On 09/30/2015 03:29 PM, Andrew F. Davis wrote:
>> On 09/30/2015 12:28 PM, Mark Brown wrote:
>>> On Tue, Sep 29, 2015 at 01:58:41PM -0500, Andrew F. Davis wrote:
>>>> On 09/29/2015 01:38 PM, Mark Brown wrote:
>>>
>>>>> Oh, ick.  The binding has a compatible string in the individual
>>>>> regulator bindings which is broken unless there really are lots of
>>>>> variants being configured via DT (which is just not the case here).
>>>>> It's not only more typing in the DT,
>>>
>>>> I don't see this, the alternative is matching to this
>>>> "regulator-compatible",
>>>> why not just use the existing compatible.
>>>
>>> No, you don't need to use regulator-compatible - that's deprecated.
>>> Just use the node names.
>>>
>>
>> Are we sure matching on node names is a good idea? Most are just arbitrary
>> names meant to be human readable and reference-able, giving them function
>> may lead to confusion. This seems to be why we have "compatible", for
>> specific
>> identification of node function. But I'm new so maybe I'm wrong?
>>
>>>>> it also means that we can't read
>>>>> back the configuration of the device unless the user goes and creates a
>>>>> DT which explicitly lists each regulator on the device which is
>>>>> unhelpful.  We should be able to read back the configurations of all
>>>>> the
>>>>> regulators by simply listing the device in DT.
>>>
>>>> Could you expand this? I'm not sure I understand why we still cant do
>>>> this
>>>> using this new way.
>>>
>>> I'm not sure what there is to add...  if the regulator is only
>>> instantiated when it features in the device tree then obviously it must
>>> be included in the device to be instantiated.
>>>
>>
>> This is already the case then, missing regulator nodes in old drivers
>> will not
>> get instantiated ether. And old drivers don't always store any more info
>> about
>> available regulators than mine does.
>>
>>>> Bindings should have compatible strings when they describe hardware
>>>> like this,
>>>> we can then do stuff like put the LDO and DCDC drivers in separate
>>>> modules for
>>>> instance, letting DT only load what we need. There are other benefits
>>>> like
>>>> not having to search our own DT binding for data, and we only get
>>>> probed for
>>>> devices in the DT.
>>>
>>> Only getting probed for device is in DT is exactly the problem here, and
>>> nothing prevents us having separate modules for things without
>>> enumerating everything in DT.
>>>
>>
>> Sure, but then we have to do some fiddling with MFD_CORE to do that work,
>> why not remove the dependency and let DT do that for DT only drivers?
>>
>>>> This also eliminates the need for MFD_CORE, we just call
>>>> of_platform_populate on ourself and DT helpers do the rest. Why hard
>>>> code
>>>> mfd_cell's and do matching when DT does the same thing.
>>>
>>> Putting everything in DT means more work for people integrating the
>>> device and means that we have to have a full and complete understanding
>>> of the device at the time we write the DT, including decions about how
>>> we split the functionality of the device between subsystems.
>>>
>>
>> We are not adding anything extra to the DT node, we just use the
>> "compatible"
>> string to identify and match the node vs. "regulator-name", or the nodes
>> name,
>> or whatever else has been used. The node is then just filled with the
>> standard
>> optional properties just like every other driver's node.
>>
>>>>> The fact that this is different to the bindings for other regulator
>>>>> drivers and requires more code ought to have been a big warning sign
>>>>> here :(
>>>
>>>> The binding is the same as the new tps65218 driver, different isn't
>>>> always
>>>> a warning sign. And what do you mean "requires more code"? This
>>>> regulator
>>>> driver is smaller than almost any other. DT takes care of everything for
>>>> us relating to hardware instantiation like it should.
>>>
>>> That's not a new driver, it's from more than a year ago (before or about
>>> the same time the helpers got added IIRC).
>>>
>>
>> Newer than a lot, I chose to base my driver off of that not just because
>> it is a similar TI part, but because it was the cleanest, simplest looking
>> one IMHO. The helpers would require more code (you need to know how many
>> regulators you have and call the helpers in a loop).
>>
>> I have another PMIC I'm about to push a driver for when this gets
>> figured out
>> that does the same thing, and it's more important I think to do it this
>> way for
>> this new part. Some of the new regulators are designed without a dedicated
>> SOC or board to power in mind, so they will have a whole bunch for
>> different
>> regulator types on one chip and it will be up to the designer to pick
>> which ones
>> to turn on and use. With this DT approach you can just list the ones you
>> want,
>> and we may even be able to split different types into different modules,
>> then
>> we can use the same regulator driver in different spins of the PMIC with
>> more
>> or less of that type of regulator, we just add that same node under a
>> different
>> parent PMIC driver.
>>
>
> That's all make sense only if you have all information required for driver probing in DT.
> But you don't, because  a lot of information are still hard-coded in driver:
> - regulator IDs,
> - registers required to control regulator
> - ranges information and etc.
>
> So, you will not be be able to easily separate some regulator and reuse it with another PMIC.
>

Unless we encode the needed registers/info into the DT, that info
could be considered a device description if you consider each regulator
to be a separate device burned onto the same silicon, much like we
do with the different IPs on SOCs. It would be nice to have 100% generic
regulator drivers, but the DT maintainers might not go for that :)

> Actually, both approaches have right to live and which one to select depends on
> functionality which has to be implemented by regulator driver.
> For example, with this (your) approach the separate platform device will be created
> for each regulator, so, if needed, corresponding driver's callback (.remove/shutdown and
> dev_pm_ops)  can be implemented individually for each regulator. Do you need this?
> As for me, such approach is reasonable if you have devices which represents one/two regulators max.
>
> The disadvantage of this approach is that you need separate compatible string fore each regulator
> - and this, actually, is usually banned by DT maintainers ;)
>
> For devices, like this one (or twl, or plamas) which has dozens of regulators it simply make no sense :)
> Just imaging that tomorrow tps65912-1 will be released and it will be fully compatible with  tps65912, but
> with one exception - it will have dcdc5 and all followed registers offsets will be shifted.
> What will you do? All this code, based on compatible strings, will not work any more.
> And you will need to introduce another bunch of compatible strings..
>

Agree, unless we go with my above idea! :)

> By the way, this implementation is not optimal any way :)
> - you are using compatible string to get tps65912_pmic_regs structure
> - then tps65912_pmic_regs is used to get regulator ID
> - and then, finally, regulator ID is used to get regulator_desc structure
>

Yeah, I saw that after I posted this, my new version just puts a pointer
to the right regulator_desc struct in the of_device_id.data, then we can
use it directly with no middle man steps, much cleaner.

> And, finally, pay attention pls, that regulator_of_get_init_data() is called from
> from regulator_register().
>

Right, but only if you supply .of_match in your regulator_desc, and even then it
only matches on node name, not "compatible" string, if it did I could use it to
eliminate my call to of_get_regulator_init_data, the whole probe function would
become about 10 lines of code :)

> So, in my opinion, the tps65217-regulator.c driver is really good example of how it could be done.
>
>

Thanks, I was looking for good examples, that and rt5033 seem to be the requested
style for new regulator drivers.
Mark Brown Oct. 1, 2015, 4:25 p.m. UTC | #14
On Thu, Oct 01, 2015 at 10:33:49AM -0500, Grygorii Strashko wrote:

> So, in my opinion, the tps65217-regulator.c driver is really good
> example of how it could be done.

Yes, this should be a good example to refer to.
diff mbox

Patch

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 3cb2de9..1dec96a 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -727,6 +727,12 @@  config REGULATOR_TPS65910
 	help
 	  This driver supports TPS65910/TPS65911 voltage regulator chips.
 
+config REGULATOR_TPS65912
+	tristate "TI TPS65912 Power regulator"
+	depends on MFD_TPS65912
+	help
+	    This driver supports TPS65912 voltage regulator chip.
+
 config REGULATOR_TPS80031
 	tristate "TI TPS80031/TPS80032 power regualtor driver"
 	depends on MFD_TPS80031
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 222ff5f..0f81749 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -91,6 +91,7 @@  obj-$(CONFIG_REGULATOR_TPS65218) += tps65218-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6524X) += tps6524x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6586X) += tps6586x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
+obj-$(CONFIG_REGULATOR_TPS65912) += tps65912-regulator.o
 obj-$(CONFIG_REGULATOR_TPS80031) += tps80031-regulator.o
 obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o
 obj-$(CONFIG_REGULATOR_VEXPRESS) += vexpress.o
diff --git a/drivers/regulator/tps65912-regulator.c b/drivers/regulator/tps65912-regulator.c
new file mode 100644
index 0000000..343dae1
--- /dev/null
+++ b/drivers/regulator/tps65912-regulator.c
@@ -0,0 +1,240 @@ 
+/*
+ * Regulator driver for TPS65912x PMIC
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Author: Andrew F. Davis <afd@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether expressed or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License version 2 for more details.
+ *
+ * Based on the TPS65218 driver and the previous TPS65912 driver by
+ * Margarita Olaya Cabrera <magi@slimlogic.co.uk>
+ */
+
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+#include <linux/mfd/tps65912.h>
+
+enum tps65912_regulators { DCDC1, DCDC2, DCDC3, DCDC4, LDO1, LDO2, LDO3,
+	LDO4, LDO5, LDO6, LDO7, LDO8, LDO9, LDO10 };
+
+#define TPS65912_REGULATOR(_name, _id, _ops, _vr, _er, _lr, _nlr)	\
+	{								\
+		.name			= _name,			\
+		.id			= _id,				\
+		.ops			= &_ops,			\
+		.n_voltages		= 64,				\
+		.type			= REGULATOR_VOLTAGE,		\
+		.owner			= THIS_MODULE,			\
+		.vsel_reg		= _vr,				\
+		.vsel_mask		= 0x3f,				\
+		.enable_reg		= _er,				\
+		.enable_mask		= BIT(7),			\
+		.volt_table		= NULL,				\
+		.linear_ranges		= _lr,				\
+		.n_linear_ranges	= _nlr,				\
+	}								\
+
+#define TPS65912_INFO(_id, _nm, _min, _max)	\
+	[_id] = {				\
+		.id		= _id,		\
+		.name		= _nm,		\
+		.min_uV		= _min,		\
+		.max_uV		= _max,		\
+	}
+
+static const struct regulator_linear_range tps65912_dcdc_ranges[] = {
+	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x3f, 50000),
+};
+
+static const struct regulator_linear_range tps65912_ldo_ranges[] = {
+	REGULATOR_LINEAR_RANGE(800000, 0x0, 0x20, 25000),
+	REGULATOR_LINEAR_RANGE(1650000, 0x21, 0x3c, 50000),
+	REGULATOR_LINEAR_RANGE(3100000, 0x3d, 0x3f, 100000),
+};
+
+static struct tps_info tps65912_pmic_regs[] = {
+	TPS65912_INFO(DCDC1, "DCDC1", 500000, 3800000),
+	TPS65912_INFO(DCDC2, "DCDC2", 500000, 3800000),
+	TPS65912_INFO(DCDC3, "DCDC3", 500000, 3800000),
+	TPS65912_INFO(DCDC4, "DCDC4", 500000, 3800000),
+	TPS65912_INFO(LDO1, "LDO1", 800000, 3300000),
+	TPS65912_INFO(LDO2, "LDO2", 800000, 3300000),
+	TPS65912_INFO(LDO3, "LDO3", 800000, 3300000),
+	TPS65912_INFO(LDO4, "LDO4", 1600000, 3300000),
+	TPS65912_INFO(LDO5, "LDO5", 1600000, 3300000),
+	TPS65912_INFO(LDO6, "LDO6", 800000, 3300000),
+	TPS65912_INFO(LDO7, "LDO7", 800000, 3300000),
+	TPS65912_INFO(LDO8, "LDO8", 800000, 3300000),
+	TPS65912_INFO(LDO9, "LDO9", 800000, 3300000),
+	TPS65912_INFO(LDO10, "LDO10", 800000, 3300000),
+};
+
+#define TPS65912_OF_MATCH(comp, label)	\
+	{				\
+		.compatible = comp,	\
+		.data = &label,		\
+	}
+
+static const struct of_device_id tps65912_regulator_of_match_table[] = {
+	TPS65912_OF_MATCH("ti,tps65912-dcdc1", tps65912_pmic_regs[DCDC1]),
+	TPS65912_OF_MATCH("ti,tps65912-dcdc2", tps65912_pmic_regs[DCDC2]),
+	TPS65912_OF_MATCH("ti,tps65912-dcdc3", tps65912_pmic_regs[DCDC3]),
+	TPS65912_OF_MATCH("ti,tps65912-dcdc4", tps65912_pmic_regs[DCDC4]),
+	TPS65912_OF_MATCH("ti,tps65912-ldo1", tps65912_pmic_regs[LDO1]),
+	TPS65912_OF_MATCH("ti,tps65912-ldo2", tps65912_pmic_regs[LDO2]),
+	TPS65912_OF_MATCH("ti,tps65912-ldo3", tps65912_pmic_regs[LDO3]),
+	TPS65912_OF_MATCH("ti,tps65912-ldo4", tps65912_pmic_regs[LDO4]),
+	TPS65912_OF_MATCH("ti,tps65912-ldo5", tps65912_pmic_regs[LDO5]),
+	TPS65912_OF_MATCH("ti,tps65912-ldo6", tps65912_pmic_regs[LDO6]),
+	TPS65912_OF_MATCH("ti,tps65912-ldo7", tps65912_pmic_regs[LDO7]),
+	TPS65912_OF_MATCH("ti,tps65912-ldo8", tps65912_pmic_regs[LDO8]),
+	TPS65912_OF_MATCH("ti,tps65912-ldo9", tps65912_pmic_regs[LDO9]),
+	TPS65912_OF_MATCH("ti,tps65912-ldo10", tps65912_pmic_regs[LDO10]),
+	{ /*sentinel*/ },
+};
+MODULE_DEVICE_TABLE(of, tps65912_regulator_of_match_table);
+
+/* Operations permitted on DCDCx */
+static struct regulator_ops tps65912_ops_dcdc = {
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.list_voltage		= regulator_list_voltage_linear_range,
+};
+
+/* Operations permitted on LDOx */
+static struct regulator_ops tps65912_ops_ldo = {
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.map_voltage		= regulator_map_voltage_linear_range,
+};
+
+static const struct regulator_desc regulators[] = {
+	TPS65912_REGULATOR("DCDC1", TPS65912_DCDC_1, tps65912_ops_dcdc,
+			   TPS65912_DCDC1_OP, TPS65912_DCDC1_CTRL,
+			   tps65912_dcdc_ranges,
+			   ARRAY_SIZE(tps65912_dcdc_ranges)),
+	TPS65912_REGULATOR("DCDC2", TPS65912_DCDC_2, tps65912_ops_dcdc,
+			   TPS65912_DCDC2_OP, TPS65912_DCDC2_CTRL,
+			   tps65912_dcdc_ranges,
+			   ARRAY_SIZE(tps65912_dcdc_ranges)),
+	TPS65912_REGULATOR("DCDC3", TPS65912_DCDC_3, tps65912_ops_dcdc,
+			   TPS65912_DCDC3_OP, TPS65912_DCDC3_CTRL,
+			   tps65912_dcdc_ranges,
+			   ARRAY_SIZE(tps65912_dcdc_ranges)),
+	TPS65912_REGULATOR("DCDC4", TPS65912_DCDC_4, tps65912_ops_dcdc,
+			   TPS65912_DCDC4_OP, TPS65912_DCDC4_CTRL,
+			   tps65912_dcdc_ranges,
+			   ARRAY_SIZE(tps65912_dcdc_ranges)),
+	TPS65912_REGULATOR("LDO1", TPS65912_LDO_1, tps65912_ops_ldo,
+			   TPS65912_LDO1_OP, TPS65912_LDO1_AVS,
+			   tps65912_ldo_ranges,
+			   ARRAY_SIZE(tps65912_ldo_ranges)),
+	TPS65912_REGULATOR("LDO2", TPS65912_LDO_2, tps65912_ops_ldo,
+			   TPS65912_LDO2_OP, TPS65912_LDO2_AVS,
+			   tps65912_ldo_ranges,
+			   ARRAY_SIZE(tps65912_ldo_ranges)),
+	TPS65912_REGULATOR("LDO3", TPS65912_LDO_3, tps65912_ops_ldo,
+			   TPS65912_LDO3_OP, TPS65912_LDO3_AVS,
+			   tps65912_ldo_ranges,
+			   ARRAY_SIZE(tps65912_ldo_ranges)),
+	TPS65912_REGULATOR("LDO4", TPS65912_LDO_4, tps65912_ops_ldo,
+			   TPS65912_LDO4_OP, TPS65912_LDO4_AVS,
+			   tps65912_ldo_ranges,
+			   ARRAY_SIZE(tps65912_ldo_ranges)),
+	TPS65912_REGULATOR("LDO5", TPS65912_LDO_5, tps65912_ops_ldo,
+			   TPS65912_LDO5, TPS65912_LDO5,
+			   tps65912_ldo_ranges,
+			   ARRAY_SIZE(tps65912_ldo_ranges)),
+	TPS65912_REGULATOR("LDO6", TPS65912_LDO_6, tps65912_ops_ldo,
+			   TPS65912_LDO6, TPS65912_LDO6,
+			   tps65912_ldo_ranges,
+			   ARRAY_SIZE(tps65912_ldo_ranges)),
+	TPS65912_REGULATOR("LDO7", TPS65912_LDO_7, tps65912_ops_ldo,
+			   TPS65912_LDO7, TPS65912_LDO7,
+			   tps65912_ldo_ranges,
+			   ARRAY_SIZE(tps65912_ldo_ranges)),
+	TPS65912_REGULATOR("LDO8", TPS65912_LDO_8, tps65912_ops_ldo,
+			   TPS65912_LDO8, TPS65912_LDO8,
+			   tps65912_ldo_ranges,
+			   ARRAY_SIZE(tps65912_ldo_ranges)),
+	TPS65912_REGULATOR("LDO9", TPS65912_LDO_9, tps65912_ops_ldo,
+			   TPS65912_LDO9, TPS65912_LDO9,
+			   tps65912_ldo_ranges,
+			   ARRAY_SIZE(tps65912_ldo_ranges)),
+	TPS65912_REGULATOR("LDO10", TPS65912_LDO_10, tps65912_ops_ldo,
+			   TPS65912_LDO10, TPS65912_LDO10,
+			   tps65912_ldo_ranges,
+			   ARRAY_SIZE(tps65912_ldo_ranges)),
+};
+
+static int tps65912_regulator_probe(struct platform_device *pdev)
+{
+	struct tps65912 *tps = dev_get_drvdata(pdev->dev.parent);
+	struct regulator_init_data *init_data;
+	const struct tps_info *template;
+	struct regulator_dev *rdev;
+	const struct of_device_id *match;
+	struct regulator_config config = { };
+	int id;
+
+	match = of_match_device(tps65912_regulator_of_match_table, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	template = match->data;
+	id = template->id;
+	init_data = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
+					       &regulators[id]);
+
+	platform_set_drvdata(pdev, tps);
+
+	tps->info[id] = &tps65912_pmic_regs[id];
+	config.dev = &pdev->dev;
+	config.init_data = init_data;
+	config.driver_data = tps;
+	config.regmap = tps->regmap;
+	config.of_node = pdev->dev.of_node;
+
+	rdev = devm_regulator_register(&pdev->dev, &regulators[id], &config);
+	if (IS_ERR(rdev)) {
+		dev_err(tps->dev, "failed to register %s regulator\n",
+			pdev->name);
+		return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+
+static struct platform_driver tps65912_regulator_driver = {
+	.driver = {
+		.name = "tps65912-pmic",
+		.of_match_table = tps65912_regulator_of_match_table,
+	},
+	.probe = tps65912_regulator_probe,
+};
+
+module_platform_driver(tps65912_regulator_driver);
+
+MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
+MODULE_DESCRIPTION("TPS65912 voltage regulator driver");
+MODULE_ALIAS("platform:tps65912-pmic");
+MODULE_LICENSE("GPL v2");