Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Bjorn Andersson]

On Thu, Jan 29, 2015 at 5:51 PM, Bjorn Andersson
<bjorn.andersson-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> wrote:
> Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> ---
> After discussing this back and forth we've concluded that we should be
> future compatible with these bindings, so let's make an attempt to make
> it possible to use the Qualcomm family A regulators.
>

ping?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Lee Jones]

On Mon, 09 Feb 2015, Bjorn Andersson wrote:

> On Thu, Jan 29, 2015 at 5:51 PM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
> > Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > ---
> > After discussing this back and forth we've concluded that we should be
> > future compatible with these bindings, so let's make an attempt to make
> > it possible to use the Qualcomm family A regulators.
> >
> 
> ping?

Pings don't work.  If I haven't replied, there is a reason.

I've been hectic at 'proper work' lately -- I'll get to you.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Lee Jones]

On Tue, 10 Feb 2015, Lee Jones wrote:

> On Mon, 09 Feb 2015, Bjorn Andersson wrote:
> 
> > On Thu, Jan 29, 2015 at 5:51 PM, Bjorn Andersson
> > <bjorn.andersson@sonymobile.com> wrote:
> > > Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > > ---
> > > After discussing this back and forth we've concluded that we should be
> > > future compatible with these bindings, so let's make an attempt to make
> > > it possible to use the Qualcomm family A regulators.
> > >
> > 
> > ping?
> 
> Pings don't work.  If I haven't replied, there is a reason.
> 
> I've been hectic at 'proper work' lately -- I'll get to you.

In fact, this needs a DT Ack.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Stephen Boyd]

On 02/18/15 13:08, Bjorn Andersson wrote:
> On Wed, Feb 18, 2015 at 12:28 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>
>> MFD name matching isn't required. All we need to do is have a regulators
>> node and put a compatible = "qcom,rpm-msmXXXX-regulators" in there. Then
>> of_platform_populate() does most of the work and we rework the RPM
>> driver to match on this compatible. Thus the regulator stuff is
>> encapsulated in the drivers/regulator/ directory.
>>
> Right, this would only affect the mfd children...
>
>
>> e.g.
>>
>> rpm@108000 {
>>         compatible = "qcom,rpm-msm8960";
>>         reg = <0x108000 0x1000>;
>>         qcom,ipc = <&apcs 0x8 2>;
>>
>>         interrupts = <0 19 0>, <0 21 0>, <0 22 0>;
>>         interrupt-names = "ack", "err", "wakeup";
>>
>>         regulators {
>>                 compatible = "qcom,rpm-msm8960-regulators";
>>
>>                 s1: s1 {
>>                         regulator-min-microvolt = <1225000>;
>>                         regulator-max-microvolt = <1225000>;
>>                         bias-pull-down;
>>                         qcom,switch-mode-frequency = <3200000>;
>>                 };
> This means that the regulator driver needs to have a list of
> regulators per platform supported.
>
> If we keep the compatible in the regulator nodes, we can use that for
> matching with an implementation - still without using the
> platform_device model for instantiating them. We would not need said
> list in that case.

Agreed. It was intentional because the type of regulator is another
static implementation detail.

>
>>                 ...
>>         };
>>
>>         rpmcc: clock-controller (or clocks?) {
>>                 compatible = "qcom,rpm-msm8960-clocks";
>>                 #clock-cells = <1>;
>>         };
>> };
>>
>> This is probably missing a size-cells somewhere, but you get the idea. I
>> intentionally named the node "s1" in the hopes of the compiler
>> consolidating the multiple "s1" strings for all the different pmic match
>> tables into one string in some literal pool somewhere. Also, I removed
>> reg from the regulator nodes to stay flexible in case we want to change
>> the rpm write API in the future (it would go into the match table as
>> driver data).
> If we fall back to define the regulators in the code and map the
> property nodes by name, then we can just add the content of the reg
> property in those tables as well.

Yep.

>
>> (Goes to look at the RPM write API...)
>>
>> BTW, some of those rpm tables are going to be huge when you consider
>> that the flat number space of QCOM_RPM_<RESOURCE> is monotonically
>> increasing but the actual resources used by a particular PMIC is only a
>> subset of that space. For example, some arrays might only have resources
>> that start at 100, so the first 100 entries in the array are wasted
>> space. Maybe the rpm write API shouldn't be doing this fake resource
>> numbering thing. Instead it should rely on the client drivers to pack up
>> a structure that the write API can interpret, i.e. push the resource
>> tables out to the drivers.
>>
> Correct, but David Collins comment on the subject was clear; we don't
> want these tables in the code. And after playing around with various
> options I came to the same conclusion - maintaining the tables will be
> a mess.

We're already maintaining these tables in qcom-rpm.c. I'm advocating for
removing those tables from the rpm driver and putting the data into the
regulator driver. Then we don't have to index into a sparse table to
figure out what values the RPM wants to use. Instead we have the data at
hand for a particular regulator based on the of_regulator_match.

I don't understand the maintenance argument either. The data has to go
somewhere so the maintenance is always there.

>From what I can tell, that data is split in two places. The regulator
type knows how big the data we want to send is (1 or 2) and that is
checked in the RPM driver to make sure that the two agree (this part
looks like over-defensive coding). Then the DT has a made up number that
maps to 3 different numbers in the RPM driver. The same could be done
for b-family, where we have two numbers that just so happen to be user
friendly enough to make sense on their own. Instead of being 165, 68,
and 48, they're a #define SMPS and 1. We could make a #define SMPS1 and
map it to a tuple with #define SMPS and 1. It looks like that was how
you had b-family prototyped at one point.

>
> So for family B, where this is not necessary for the rpm driver, I
> have revised this to instead be:
>
> #address-cells = <2>;
> smps1 {
>    reg = <QCOM_SMD_RPM_SMPA 1>;
> }
>
> With #define QCOM_SMD_RPM_SMPA 0x61706d73, this information goes
> straight into the smd packet and we don't have any tables at all.

How does the rpm write API look there? Does it take two numbers
(resource type and resource id) instead of 1 (enum)? Is the plan to keep
the write API the same as what's in qcom-rpm.c today?

>
>
> Josh made an attempt to encode the data needed for family A in the DT
> in a similar fasion, but we never found a reasonable way to do so.

With Josh's attempt would this be?

#address-cells = <3>;
smps1 {
    reg = <165 68 48>;
};

What's different here from b-family besides another number? If the
b-family way is reasonable I'm lost how this is not reasonable. Honestly
it doesn't make much sense to me to have the reg property done like this
if the value is always the same. If the RPM was moving these offsets
around within the same compatible string it would make sense to put that
in DT and figure out dynamically what the offsets are because they
really can be different. But given that the values are always the same
for a given compatible it just means that the reg properties have to be
a certain value as long as the compatible is qcom,rpm-msmXXXX-regulators.

>
>>>
>>> A drawback of both these solutions is that supplies are specified on
>>> the device's of_node, and hence it is no longer possible to describe
>>> the supply dependency between our regulators - something that have
>>> shown to be needed. Maybe we can open code the supply logic in the
>>> regulator driver or we have to convince Mark about the necessity of
>>> this.
>> The supply dependencies are a detail of the PMIC implementation and it
>> isn't configurable on a board-by-board basis. If we did the individual
>> nodes and had the container regulators "device" we could specify the
>> dependencies in C and then vin-supply is not necessary. That sounds like
>> a win to me because we sidestep adding a new property.
>>
> Correct, it's not configurable on a board basis, but if I read the
> pmic documentation correctly it's handled by external routing, hence
> is not entirely static per pmic?

Hmm, yes you're right. It's not entirely static, in the sense that some
supplies could be configured differently when the pmic is the same.

>
> Non the less, we must provide this information to the regulator
> framework in some way if we want its help.
> If we define all regulators in code (and just bring their properties
> from DT) then we could encapsulate the relationship there as well.
> But with the current suggested solution of having all this data coming
> from DT I simply rely on the existing infrastructure for describing
> supply-dependencies in DT.
>
>

Yes I don't see how putting the data in C or in DT or having a
regulators encapsulating node makes it impossible to specify the supply.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Bjorn Andersson]

On Wed 18 Feb 18:29 PST 2015, Stephen Boyd wrote:

> On 02/18/15 13:08, Bjorn Andersson wrote:
> > On Wed, Feb 18, 2015 at 12:28 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:

After taking a deeper look at this I've come to the following
conclusion:

We can save 2100 bytes of data by spreading out the magic numbers from
the rpm resource tables into the regulator, clock and bus drivers. At
the cost of having this rpm-specific information spread out.

Separate of that we could rewrite the entire regulator implementation to
define all regulators in platform specific tables in the regulators.
This would save about 12-15k of ram.

This can however be done in two ways:
1) We modify the mfd driver to instatiate 3 mfd_cells; one of them
being "qcom,rpm-regulators". We modify the regulator driver to have
tables of the regulators for each platform and matching regulator
parameters by of_node name and registering these.

2) We stick with this binding, we then go ahead and do the same
modification to the mfd as above - passing the rpm of_node to the
regulator driver, that walks the children and match that to the current
compatible list. (in other words, we replace of_platform_populate with
our own mechamism).


The benefit of 2 is that we can use the code that was posted 10 months
ago and merged 3 months ago until someone prioritize those 12kb.


To take either of these paths the issue at the bottom has to be
resolved first.


More answers follows inline:

> 
> We're already maintaining these tables in qcom-rpm.c. I'm advocating for
> removing those tables from the rpm driver and putting the data into the
> regulator driver. Then we don't have to index into a sparse table to
> figure out what values the RPM wants to use. Instead we have the data at
> hand for a particular regulator based on the of_regulator_match.
> 

I do like the "separation of concerns" between the rpm driver and the
children, but you're right in that we're wasting almost 3kb in doing so:

(gdb) p sizeof(apq8064_rpm_resource_table) + sizeof(msm8660_rpm_resource_table) + sizeof(msm8960_rpm_resource_table)
$2 = 6384

vs

(gdb) p sizeof(struct qcom_rpm_resource) * (77 + 74 + 73)
$3 = 3584

The alternative would be to spread those magic constants out in the
three client drivers.

Looking at this from a software design perspective I stand by the
argument that the register layout of the rpm is not a regulator driver
implementation detail and is better kept separate.

As we decided to define the regulators in code but the actual
composition in dt it was not possible to put the individual numbers in
DT. Having reg = <165 68 48> does not make any sense, unless we perhaps
maintain said table in the dt binding documentation.

> I don't understand the maintenance argument either. The data has to go
> somewhere so the maintenance is always there.
> 

Well, you used exactly that argument when you questioned the way I did
pinctrl-msm, with a table per platform.

> From what I can tell, that data is split in two places. The regulator
> type knows how big the data we want to send is (1 or 2) and that is
> checked in the RPM driver to make sure that the two agree (this part
> looks like over-defensive coding).

Yeah, after debugging production code for years I like to be slightly on
the defensive side. But the size could of course be dropped from the
resource-tables; reducing the savings of your suggestion by another 800
bytes...

> Then the DT has a made up number that
> maps to 3 different numbers in the RPM driver.

Reading the following snippet in my dts file makes imidiate sense:

reg = <QCOM_RPM_PM8921_SMPS1>

the following doesn't make any sense:

reg = <116 31 30>;

Maybe it's write-once in a dts so it doesn't matter that much - as long
as the node is descriptive. But I like the properties to be human
understandable.

> The same could be done
> for b-family, where we have two numbers that just so happen to be user
> friendly enough to make sense on their own. Instead of being 165, 68,
> and 48, they're a #define SMPS and 1. We could make a #define SMPS1 and
> map it to a tuple with #define SMPS and 1. It looks like that was how
> you had b-family prototyped at one point.
> 

In family B things are completely different. The reason why I proposed
the similar setup was simply because I wanted to keep the api similar
(or even the same). But the mechanism is completely different;

The regulator driver builds up a key-value-pair list and encapsulte this
in a packet with a header that includes 'type' and 'id' of the resource
to be modified. This is then put in the packet-channel (smd) that goes
to the rpm.

The types are magic numbers but the id correlates to the resource id of
that type - so it doesn't give anything to maintain any tables for
family B.

Family A and B are so different that it doesn't make sense to share any
code, it was however my indention to have the dt bindings follow the
same structure!

> >
> > So for family B, where this is not necessary for the rpm driver, I
> > have revised this to instead be:
> >
> > #address-cells = <2>;
> > smps1 {
> >    reg = <QCOM_SMD_RPM_SMPA 1>;
> > }
> >
> > With #define QCOM_SMD_RPM_SMPA 0x61706d73, this information goes
> > straight into the smd packet and we don't have any tables at all.
> 
> How does the rpm write API look there? Does it take two numbers
> (resource type and resource id) instead of 1 (enum)? Is the plan to keep
> the write API the same as what's in qcom-rpm.c today?
> 

The prototype I have right now is:

int qcom_rpm_smd_write(struct qcom_smd_rpm *rpm,
		int state,
		u32 resource_type,
		u32 resource_id,
		void *buf,
		size_t count)

The function builds a packet - with the payload of "buf" - sends that
off and waits for an ack. There's not the extra steps that needs to be
taken in family A and both type and id are human readable.

> >
> >
> > Josh made an attempt to encode the data needed for family A in the DT
> > in a similar fasion, but we never found a reasonable way to do so.
> 
> With Josh's attempt would this be?
> 
> #address-cells = <3>;
> smps1 {
>     reg = <165 68 48>;
> };
> 

Yes, and with the approach of having this information in DT this would
have been a write-once never understand.

> What's different here from b-family besides another number? If the
> b-family way is reasonable I'm lost how this is not reasonable. Honestly
> it doesn't make much sense to me to have the reg property done like this
> if the value is always the same.

Right, from a RPM perspective the word "smps1" carries this information
with it. But the binding defines the "smps", which needs that additional
information (reg) to know what resource to bind to.

> If the RPM was moving these offsets
> around within the same compatible string it would make sense to put that
> in DT and figure out dynamically what the offsets are because they
> really can be different.

The proposed binding states the following:

- compatible:
        Usage: required
        Value type: <string>
        Definition: must be one of:
                    "qcom,rpm-pm8058-smps"
                    "qcom,rpm-pm8901-ftsmps"
                    "qcom,rpm-pm8921-smps"
                    "qcom,rpm-pm8921-ftsmps"

Doesn't this map to the case you say make sense?

> But given that the values are always the same
> for a given compatible it just means that the reg properties have to be
> a certain value as long as the compatible is qcom,rpm-msmXXXX-regulators.
> 

Yes, if the compatible covers the entire set of regulators. But if we're
going to align this with the other mfds (as you propose) then I don't
think there should be a compatible; mfd already have a way to
instantiate children and the rpm binding would describe every part of
the children as well.

> >
> >>>
> >>> A drawback of both these solutions is that supplies are specified on
> >>> the device's of_node, and hence it is no longer possible to describe
> >>> the supply dependency between our regulators - something that have
> >>> shown to be needed. Maybe we can open code the supply logic in the
> >>> regulator driver or we have to convince Mark about the necessity of
> >>> this.
> >> The supply dependencies are a detail of the PMIC implementation and it
> >> isn't configurable on a board-by-board basis. If we did the individual
> >> nodes and had the container regulators "device" we could specify the
> >> dependencies in C and then vin-supply is not necessary. That sounds like
> >> a win to me because we sidestep adding a new property.
> >>
> > Correct, it's not configurable on a board basis, but if I read the
> > pmic documentation correctly it's handled by external routing, hence
> > is not entirely static per pmic?
> 
> Hmm, yes you're right. It's not entirely static, in the sense that some
> supplies could be configured differently when the pmic is the same.
> 

Static or not, within rpm/pmic regulators there are dependencies that we
have to inform the regulator framework about - or handle ourselves.
Neither the rpm nor pmic will handle these for us.

> >
> > Non the less, we must provide this information to the regulator
> > framework in some way if we want its help.
> > If we define all regulators in code (and just bring their properties
> > from DT) then we could encapsulate the relationship there as well.
> > But with the current suggested solution of having all this data coming
> > from DT I simply rely on the existing infrastructure for describing
> > supply-dependencies in DT.
> >
> >
> 
> Yes I don't see how putting the data in C or in DT or having a
> regulators encapsulating node makes it impossible to specify the supply.
> 

Me neither, a month ago...

Here's the discussion we had with Mark regarding having the regulator
core pick up -supply properties from the individual child of_nodes of a
regulator driver:

https://lkml.org/lkml/2015/2/4/759

As far as I can see this has to be fixed for us to move over to having
one driver registering all our regulators, independently of how we
structure this binding.

Regards,
Bjorn

[PATCH] regulator: qcom-rpm: Rework for single device

[Stephen Boyd]

The RPM regulators are not individual devices. Creating platform
devices for each regulator bloats the kernel's runtime memory
footprint by ~12k. Let's rework this driver to be a single
platform device for all the RPM regulators. This makes the
DT match the schematic/datasheet closer too because now the
regulators node contains a list of supplies at the package level
for a particular PMIC model.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---

On 02/24, Bjorn Andersson wrote:
> On Wed 18 Feb 18:29 PST 2015, Stephen Boyd wrote:
> 
> > On 02/18/15 13:08, Bjorn Andersson wrote:
> > > On Wed, Feb 18, 2015 at 12:28 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> After taking a deeper look at this I've come to the following
> conclusion:
> 
> We can save 2100 bytes of data by spreading out the magic numbers from
> the rpm resource tables into the regulator, clock and bus drivers. At
> the cost of having this rpm-specific information spread out.
> 
> Separate of that we could rewrite the entire regulator implementation to
> define all regulators in platform specific tables in the regulators.
> This would save about 12-15k of ram.

Cool I started doing that.

> 
> This can however be done in two ways:
> 1) We modify the mfd driver to instatiate 3 mfd_cells; one of them
> being "qcom,rpm-regulators". We modify the regulator driver to have
> tables of the regulators for each platform and matching regulator
> parameters by of_node name and registering these.
> 
> 2) We stick with this binding, we then go ahead and do the same
> modification to the mfd as above - passing the rpm of_node to the
> regulator driver, that walks the children and match that to the current
> compatible list. (in other words, we replace of_platform_populate with
> our own mechamism).
> 
> 
> The benefit of 2 is that we can use the code that was posted 10 months
> ago and merged 3 months ago until someone prioritize those 12kb.

I did (1) without modifying the MFD driver.

> 
> 
> To take either of these paths the issue at the bottom has to be
> resolved first.

Right. I think that's resolved now.

> 
> 
> More answers follows inline:
> 
> > 
> > We're already maintaining these tables in qcom-rpm.c. I'm advocating for
> > removing those tables from the rpm driver and putting the data into the
> > regulator driver. Then we don't have to index into a sparse table to
> > figure out what values the RPM wants to use. Instead we have the data at
> > hand for a particular regulator based on the of_regulator_match.
> > 
> 
> I do like the "separation of concerns" between the rpm driver and the
> children, but you're right in that we're wasting almost 3kb in doing so:
> 
> (gdb) p sizeof(apq8064_rpm_resource_table) + sizeof(msm8660_rpm_resource_table) + sizeof(msm8960_rpm_resource_table)
> $2 = 6384
> 
> vs
> 
> (gdb) p sizeof(struct qcom_rpm_resource) * (77 + 74 + 73)
> $3 = 3584
> 
> The alternative would be to spread those magic constants out in the
> three client drivers.
> 
> Looking at this from a software design perspective I stand by the
> argument that the register layout of the rpm is not a regulator driver
> implementation detail and is better kept separate.
> 
> As we decided to define the regulators in code but the actual
> composition in dt it was not possible to put the individual numbers in
> DT. Having reg = <165 68 48> does not make any sense, unless we perhaps
> maintain said table in the dt binding documentation.

For now I've left it so that the #define is used in C code.

> 
> > From what I can tell, that data is split in two places. The regulator
> > type knows how big the data we want to send is (1 or 2) and that is
> > checked in the RPM driver to make sure that the two agree (this part
> > looks like over-defensive coding).
> 
> Yeah, after debugging production code for years I like to be slightly on
> the defensive side. But the size could of course be dropped from the
> resource-tables; reducing the savings of your suggestion by another 800
> bytes...

Sounds good. We should probably do it.

> 
> > Then the DT has a made up number that
> > maps to 3 different numbers in the RPM driver.
> 
> Reading the following snippet in my dts file makes imidiate sense:
> 
> reg = <QCOM_RPM_PM8921_SMPS1>
> 
> the following doesn't make any sense:
> 
> reg = <116 31 30>;
> 
> Maybe it's write-once in a dts so it doesn't matter that much - as long
> as the node is descriptive. But I like the properties to be human
> understandable.

Wouldn't a

 #define QCOM_RPM_PM8921_SMPS1 116 31 30

suffice? That looks to be the same readability.

> 
> > If the RPM was moving these offsets
> > around within the same compatible string it would make sense to put that
> > in DT and figure out dynamically what the offsets are because they
> > really can be different.
> 
> The proposed binding states the following:
> 
> - compatible:
>         Usage: required
>         Value type: <string>
>         Definition: must be one of:
>                     "qcom,rpm-pm8058-smps"
>                     "qcom,rpm-pm8901-ftsmps"
>                     "qcom,rpm-pm8921-smps"
>                     "qcom,rpm-pm8921-ftsmps"
> 
> Doesn't this map to the case you say make sense?

I mean the compatible for the entire RPM/regulators node (i.e. qcom,rpm-msm8960
or qcom,rpm-apq8064). Not each individual regulator, which IMO shouldn't need a compatible at all.

> 
> > >
> > > Non the less, we must provide this information to the regulator
> > > framework in some way if we want its help.
> > > If we define all regulators in code (and just bring their properties
> > > from DT) then we could encapsulate the relationship there as well.
> > > But with the current suggested solution of having all this data coming
> > > from DT I simply rely on the existing infrastructure for describing
> > > supply-dependencies in DT.
> > >
> > >
> > 
> > Yes I don't see how putting the data in C or in DT or having a
> > regulators encapsulating node makes it impossible to specify the supply.
> > 
> 
> Me neither, a month ago...
> 
> Here's the discussion we had with Mark regarding having the regulator
> core pick up -supply properties from the individual child of_nodes of a
> regulator driver:
> 
> https://lkml.org/lkml/2015/2/4/759
> 
> As far as I can see this has to be fixed for us to move over to having
> one driver registering all our regulators, independently of how we
> structure this binding.
> 

Mark is saying that the entire PMIC (e.g. PM8921) is the package. On the
package there are pins like VIN_L27, VIN_L24, etc (look at a schematic). When
you consider the regulators node (compatible = "qcom,rpm-pm8921-regulators")
is the package then you can see that it should have a handful of vin_*-supply
properties that correspond to what you see in the schematic and the datasheet.
This way if a board designer has decided to run a particular supply to
VIN_L1_L2_L12_L18 they just wire it up in DT and everything works. They don't
have to look at the binding for the L1, L2, L12, and L18 regulators and figure
out that it uses vin-supply for the supply. Plus everything matches what
they see in the schematic and datasheets.

Note: This patch is not complete. We still need to fill out the other pmics
and standalone regulators (smb208) that this driver is for.

 drivers/regulator/qcom_rpm-regulator.c | 484 ++++++++++++++++++++++++++++-----
 1 file changed, 416 insertions(+), 68 deletions(-)

diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
index 00c5cc3d9546..538733bb7e8b 100644
--- a/drivers/regulator/qcom_rpm-regulator.c
+++ b/drivers/regulator/qcom_rpm-regulator.c
@@ -581,27 +581,347 @@ static const struct qcom_rpm_reg smb208_smps = {
 	.supports_force_mode_bypass = false,
 };
 
+struct qcom_rpm_reg_desc {
+	const struct qcom_rpm_reg *template;
+	int resource;
+	const char *supply;
+};
+
+struct qcom_rpm_of_match {
+	struct of_regulator_match *of_match;
+	unsigned int size;
+};
+
+#define DEFINE_QCOM_RPM_OF_MATCH(t)		\
+	struct qcom_rpm_of_match t##_m = {	\
+		.of_match = (t), 		\
+		.size = ARRAY_SIZE(t), 		\
+	}
+
+static struct of_regulator_match pm8921_regs[] = {
+	{
+		.name = "s1",
+		.driver_data = &(struct qcom_rpm_reg_desc){
+			.template = &pm8921_smps,
+			.resource = QCOM_RPM_PM8921_SMPS1,
+		},
+	},
+	{
+		.name = "s2",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_smps,
+			.resource = QCOM_RPM_PM8921_SMPS2,
+		},
+	},
+	{
+		.name = "s3",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_smps,
+			.resource = QCOM_RPM_PM8921_SMPS3,
+		},
+	},
+	{
+		.name = "s4",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_smps,
+			.resource = QCOM_RPM_PM8921_SMPS4,
+		},
+	},
+	{
+		.name = "s7",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_smps,
+			.resource = QCOM_RPM_PM8921_SMPS7,
+		},
+	},
+	{
+		.name = "s8",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_smps,
+			.resource = QCOM_RPM_PM8921_SMPS8,
+		},
+	},
+	{
+		.name = "l1",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_nldo,
+			.resource = QCOM_RPM_PM8921_LDO1,
+			.supply = "vin_l1_l2_l12_l18",
+		},
+	},
+	{
+		.name = "l2",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_nldo,
+			.resource = QCOM_RPM_PM8921_LDO2,
+			.supply = "vin_l1_l2_l12_l18",
+		},
+	},
+	{
+		.name = "l3",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO3,
+		},
+	},
+	{
+		.name = "l4",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO4,
+		},
+	},
+	{
+		.name = "l5",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO5,
+		},
+	},
+	{
+		.name = "l6",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO6,
+		},
+	},
+	{
+		.name = "l7",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO7,
+		},
+	},
+	{
+		.name = "l8",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO8,
+		},
+	},
+	{
+		.name = "l9",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO9,
+		},
+	},
+	{
+		.name = "l10",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO10,
+		},
+	},
+	{
+		.name = "l11",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO11,
+		},
+	},
+	{
+		.name = "l12",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_nldo,
+			.resource = QCOM_RPM_PM8921_LDO12,
+			.supply = "vin_l1_l2_l12_l18",
+		},
+	},
+	{
+		.name = "l13",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO13,
+		},
+	},
+	{
+		.name = "l14",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO14,
+		},
+	},
+	{
+		.name = "l15",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO15,
+		},
+	},
+	{
+		.name = "l16",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO16,
+		},
+	},
+	{
+		.name = "l17",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO17,
+		},
+	},
+	{
+		.name = "l18",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_nldo,
+			.resource = QCOM_RPM_PM8921_LDO18,
+			.supply = "vin_l1_l2_l12_l18",
+		},
+	},
+	{
+		.name = "l21",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO21,
+		},
+	},
+	{
+		.name = "l22",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO22,
+		},
+	},
+	{
+		.name = "l23",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO23,
+		},
+	},
+	{
+		.name = "l24",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_nldo1200,
+			.resource = QCOM_RPM_PM8921_LDO24,
+			.supply = "vin_l24",
+		},
+	},
+	{
+		.name = "l25",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_nldo1200,
+			.resource = QCOM_RPM_PM8921_LDO25,
+			.supply = "vin_l25",
+		},
+	},
+	{
+		.name = "l26",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_nldo1200,
+			.resource = QCOM_RPM_PM8921_LDO26,
+		},
+	},
+	{
+		.name = "l27",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_nldo1200,
+			.resource = QCOM_RPM_PM8921_LDO27,
+			.supply = "vin_l27",
+		},
+	},
+	{
+		.name = "l28",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_nldo1200,
+			.resource = QCOM_RPM_PM8921_LDO28,
+			.supply = "vin_l28",
+		},
+	},
+	{
+		.name = "l29",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_pldo,
+			.resource = QCOM_RPM_PM8921_LDO29
+		},
+	},
+	{
+		.name = "lvs1",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_switch,
+			.resource = QCOM_RPM_PM8921_LVS1,
+			.supply = "vin_lvs_1_3_6",
+		},
+	},
+	{
+		.name = "lvs2",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_switch,
+			.resource = QCOM_RPM_PM8921_LVS2,
+			.supply = "vin_lvs2",
+		},
+	},
+	{
+		.name = "lvs3",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_switch,
+			.resource = QCOM_RPM_PM8921_LVS3,
+			.supply = "vin_lvs_1_3_6",
+		},
+	},
+	{
+		.name = "lvs4",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_switch,
+			.resource = QCOM_RPM_PM8921_LVS4,
+			.supply = "vin_lvs_4_5_7",
+		},
+	},
+	{
+		.name = "lvs5",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_switch,
+			.resource = QCOM_RPM_PM8921_LVS5,
+			.supply = "vin_lvs_4_5_7",
+		},
+	},
+	{
+		.name = "lvs6",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_switch,
+			.resource = QCOM_RPM_PM8921_LVS6,
+			.supply = "vin_lvs_1_3_6",
+		},
+	},
+	{
+		.name = "lvs7",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_switch,
+			.resource = QCOM_RPM_PM8921_LVS7,
+			.supply = "vin_lvs_4_5_7",
+		},
+	},
+	{
+		.name = "usb-switch",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_switch,
+			.resource = QCOM_RPM_USB_OTG_SWITCH,
+		},
+	},
+	{
+		.name = "hdmi-switch",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_switch,
+			.resource = QCOM_RPM_HDMI_SWITCH,
+		},
+	},
+	{
+		.name = "ncp",
+		.driver_data = &(struct qcom_rpm_reg_desc) {
+			.template = &pm8921_ncp,
+			.resource = QCOM_RPM_PM8921_NCP,
+			.supply = "vin_ncp",
+		},
+	},
+};
+
+static DEFINE_QCOM_RPM_OF_MATCH(pm8921_regs);
+
 static const struct of_device_id rpm_of_match[] = {
-	{ .compatible = "qcom,rpm-pm8058-pldo",     .data = &pm8058_pldo },
-	{ .compatible = "qcom,rpm-pm8058-nldo",     .data = &pm8058_nldo },
-	{ .compatible = "qcom,rpm-pm8058-smps",     .data = &pm8058_smps },
-	{ .compatible = "qcom,rpm-pm8058-ncp",      .data = &pm8058_ncp },
-	{ .compatible = "qcom,rpm-pm8058-switch",   .data = &pm8058_switch },
-
-	{ .compatible = "qcom,rpm-pm8901-pldo",     .data = &pm8901_pldo },
-	{ .compatible = "qcom,rpm-pm8901-nldo",     .data = &pm8901_nldo },
-	{ .compatible = "qcom,rpm-pm8901-ftsmps",   .data = &pm8901_ftsmps },
-	{ .compatible = "qcom,rpm-pm8901-switch",   .data = &pm8901_switch },
-
-	{ .compatible = "qcom,rpm-pm8921-pldo",     .data = &pm8921_pldo },
-	{ .compatible = "qcom,rpm-pm8921-nldo",     .data = &pm8921_nldo },
-	{ .compatible = "qcom,rpm-pm8921-nldo1200", .data = &pm8921_nldo1200 },
-	{ .compatible = "qcom,rpm-pm8921-smps",     .data = &pm8921_smps },
-	{ .compatible = "qcom,rpm-pm8921-ftsmps",   .data = &pm8921_ftsmps },
-	{ .compatible = "qcom,rpm-pm8921-ncp",      .data = &pm8921_ncp },
-	{ .compatible = "qcom,rpm-pm8921-switch",   .data = &pm8921_switch },
-
-	{ .compatible = "qcom,rpm-smb208", .data = &smb208_smps },
+	{ .compatible = "qcom,rpm-pm8921-regulators", .data = &pm8921_regs_m },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, rpm_of_match);
@@ -619,7 +939,8 @@ static int rpm_reg_set(struct qcom_rpm_reg *vreg,
 	return 0;
 }
 
-static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg)
+static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg,
+		struct device_node *of_node)
 {
 	static const int freq_table[] = {
 		19200000, 9600000, 6400000, 4800000, 3840000, 3200000, 2740000,
@@ -633,9 +954,10 @@ static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg)
 	int i;
 
 	key = "qcom,switch-mode-frequency";
-	ret = of_property_read_u32(dev->of_node, key, &freq);
+	ret = of_property_read_u32(of_node, key, &freq);
 	if (ret) {
-		dev_err(dev, "regulator requires %s property\n", key);
+		dev_err(dev, "regulator %s requires %s property\n",
+				vreg->desc.name, key);
 		return -EINVAL;
 	}
 
@@ -646,88 +968,74 @@ static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg)
 		}
 	}
 
-	dev_err(dev, "invalid frequency %d\n", freq);
+	dev_err(dev, "regulator %s has invalid frequency %d\n", vreg->desc.name,
+			freq);
 	return -EINVAL;
 }
 
-static int rpm_reg_probe(struct platform_device *pdev)
+static int rpm_regulator_register(struct platform_device *pdev,
+				  struct of_regulator_match *match,
+				  struct qcom_rpm *rpm)
 {
+	struct qcom_rpm_reg_desc *rpm_desc = match->driver_data;
 	struct regulator_init_data *initdata;
-	const struct qcom_rpm_reg *template;
-	const struct of_device_id *match;
 	struct regulator_config config = { };
 	struct regulator_dev *rdev;
 	struct qcom_rpm_reg *vreg;
+	struct device_node *of_node = match->of_node;
 	const char *key;
 	u32 force_mode;
 	bool pwm;
 	u32 val;
 	int ret;
 
-	match = of_match_device(rpm_of_match, &pdev->dev);
-	template = match->data;
-
 	vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
-	if (!vreg) {
-		dev_err(&pdev->dev, "failed to allocate vreg\n");
+	if (!vreg)
 		return -ENOMEM;
-	}
-	memcpy(vreg, template, sizeof(*vreg));
+
+	memcpy(vreg, rpm_desc->template, sizeof(*vreg));
 	mutex_init(&vreg->lock);
 	vreg->dev = &pdev->dev;
 	vreg->desc.id = -1;
 	vreg->desc.owner = THIS_MODULE;
 	vreg->desc.type = REGULATOR_VOLTAGE;
-	vreg->desc.name = pdev->dev.of_node->name;
-	vreg->desc.supply_name = "vin";
-
+	vreg->desc.name = of_node->name;
+	vreg->desc.supply_name = rpm_desc->supply;
 	vreg->rpm = dev_get_drvdata(pdev->dev.parent);
-	if (!vreg->rpm) {
-		dev_err(&pdev->dev, "unable to retrieve handle to rpm\n");
-		return -ENODEV;
-	}
-
-	initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
-					      &vreg->desc);
-	if (!initdata)
-		return -EINVAL;
-
-	key = "reg";
-	ret = of_property_read_u32(pdev->dev.of_node, key, &val);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to read %s\n", key);
-		return ret;
-	}
-	vreg->resource = val;
+	vreg->resource = rpm_desc->resource;
+	initdata = match->init_data;
 
 	if ((vreg->parts->uV.mask || vreg->parts->mV.mask) &&
 	    (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) {
-		dev_err(&pdev->dev, "no voltage specified for regulator\n");
+		dev_err(&pdev->dev, "no voltage specified for regulator %s\n",
+				vreg->desc.name);
 		return -EINVAL;
 	}
 
 	key = "bias-pull-down";
-	if (of_property_read_bool(pdev->dev.of_node, key)) {
+	if (of_property_read_bool(of_node, key)) {
 		ret = rpm_reg_set(vreg, &vreg->parts->pd, 1);
 		if (ret) {
-			dev_err(&pdev->dev, "%s is invalid", key);
+			dev_err(&pdev->dev, "%s is invalid (%s)", key,
+					vreg->desc.name);
 			return ret;
 		}
 	}
 
 	if (vreg->parts->freq.mask) {
-		ret = rpm_reg_of_parse_freq(&pdev->dev, vreg);
+		ret = rpm_reg_of_parse_freq(&pdev->dev, vreg, of_node);
 		if (ret < 0)
 			return ret;
 	}
 
 	if (vreg->parts->pm.mask) {
 		key = "qcom,power-mode-hysteretic";
-		pwm = !of_property_read_bool(pdev->dev.of_node, key);
+		pwm = !of_property_read_bool(of_node, key);
 
 		ret = rpm_reg_set(vreg, &vreg->parts->pm, pwm);
 		if (ret) {
-			dev_err(&pdev->dev, "failed to set power mode\n");
+			dev_err(&pdev->dev, "failed to set power mode (%s)\n",
+					vreg->desc.name);
 			return ret;
 		}
 	}
@@ -736,11 +1044,12 @@ static int rpm_reg_probe(struct platform_device *pdev)
 		force_mode = -1;
 
 		key = "qcom,force-mode";
-		ret = of_property_read_u32(pdev->dev.of_node, key, &val);
+		ret = of_property_read_u32(of_node, key, &val);
 		if (ret == -EINVAL) {
 			val = QCOM_RPM_FORCE_MODE_NONE;
 		} else if (ret < 0) {
-			dev_err(&pdev->dev, "failed to read %s\n", key);
+			dev_err(&pdev->dev, "failed to read %s (%s)\n", key,
+					vreg->desc.name);
 			return ret;
 		}
 
@@ -775,13 +1084,15 @@ static int rpm_reg_probe(struct platform_device *pdev)
 		}
 
 		if (force_mode == -1) {
-			dev_err(&pdev->dev, "invalid force mode\n");
+			dev_err(&pdev->dev, "invalid force mode (%s)\n",
+					vreg->desc.name);
 			return -EINVAL;
 		}
 
 		ret = rpm_reg_set(vreg, &vreg->parts->fm, force_mode);
 		if (ret) {
-			dev_err(&pdev->dev, "failed to set force mode\n");
+			dev_err(&pdev->dev, "failed to set force mode (%s)\n",
+					vreg->desc.name);
 			return ret;
 		}
 	}
@@ -789,11 +1100,48 @@ static int rpm_reg_probe(struct platform_device *pdev)
 	config.dev = &pdev->dev;
 	config.init_data = initdata;
 	config.driver_data = vreg;
-	config.of_node = pdev->dev.of_node;
+	config.of_node = of_node;
+
 	rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config);
-	if (IS_ERR(rdev)) {
-		dev_err(&pdev->dev, "can't register regulator\n");
-		return PTR_ERR(rdev);
+
+	return PTR_ERR_OR_ZERO(rdev);
+}
+
+static int rpm_reg_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	const struct qcom_rpm_of_match *rpm_match;
+	struct of_regulator_match *of_match;
+	struct qcom_rpm *rpm;
+	int ret;
+
+	rpm = dev_get_drvdata(pdev->dev.parent);
+	if (!rpm) {
+		dev_err(&pdev->dev, "unable to retrieve handle to rpm\n");
+		return -ENODEV;
+	}
+
+	match = of_match_device(rpm_of_match, &pdev->dev);
+	rpm_match = match->data;
+	of_match = rpm_match->of_match;
+
+	ret = of_regulator_match(&pdev->dev, pdev->dev.of_node,
+				 of_match,
+				 rpm_match->size);
+	if (ret < 0) {
+		dev_err(&pdev->dev,
+			"Error parsing regulator init data: %d\n", ret);
+		return ret;
+	}
+
+	for (; of_match < rpm_match->of_match + rpm_match->size; of_match++) {
+		if (!of_match->of_node)
+			continue;
+		ret = rpm_regulator_register(pdev, of_match, rpm);
+		if (ret) {
+			dev_err(&pdev->dev, "can't register regulator\n");
+			return ret;
+		}
 	}
 
 	return 0;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] regulator: qcom-rpm: Rework for single device

[Stephen Boyd]

On 02/26/15 16:00, Stephen Boyd wrote:
> The RPM regulators are not individual devices. Creating platform
> devices for each regulator bloats the kernel's runtime memory
> footprint by ~12k. Let's rework this driver to be a single
> platform device for all the RPM regulators. This makes the
> DT match the schematic/datasheet closer too because now the
> regulators node contains a list of supplies at the package level
> for a particular PMIC model.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>


This is the RPM node I used for testing so you can see the resulting DT.

		rpm@108000 {
			compatible	= "qcom,rpm-apq8064";
			reg		= <0x108000 0x1000>;
			qcom,ipc	= <&l2cc 0x8 2>;

			interrupts	= <0 19 0>, <0 21 0>, <0 22 0>;
			interrupt-names	= "ack", "err", "wakeup";

			regulators {
				compatible = "qcom,rpm-pm8921-regulators";
				vin_l1_l2_l12_l18-supply = <&pm8921_s4>;
				vin_lvs_1_3_6-supply = <&pm8921_s4>;
				vin_lvs_4_5_7-supply = <&pm8921_s4>;
				vin_ncp-supply = <&pm8921_l6>;
				vin_lvs2-supply = <&pm8921_s4>;
				vin_l24-supply = <&pm8921_s1>;
				vin_l25-supply = <&pm8921_s1>;
				vin_l27-supply = <&pm8921_s7>;
				vin_l28-supply = <&pm8921_s7>;

				/* Buck SMPS */
				pm8921_s1: s1 {
					regulator-always-on;
					regulator-min-microvolt = <1225000>;
					regulator-max-microvolt = <1225000>;
					qcom,switch-mode-frequency = <3200000>;
					bias-pull-down;
				};

				pm8921_s2: s2 {
					regulator-min-microvolt = <1300000>;
					regulator-max-microvolt = <1300000>;
					qcom,switch-mode-frequency = <1600000>;
					bias-pull-down;
				};

				pm8921_s3: s3 {
					regulator-min-microvolt = <500000>;
					regulator-max-microvolt = <1150000>;
					qcom,switch-mode-frequency = <4800000>;
					bias-pull-down;
				};

				pm8921_s4: s4 {
					regulator-always-on;
					regulator-min-microvolt = <1800000>;
					regulator-max-microvolt = <1800000>;
					qcom,switch-mode-frequency = <1600000>;
					bias-pull-down;
					qcom,force-mode = <QCOM_RPM_FORCE_MODE_AUTO>;
				};

				pm8921_s7: s7 {
					regulator-min-microvolt = <1300000>;
					regulator-max-microvolt = <1300000>;
					qcom,switch-mode-frequency = <3200000>;
				};

				pm8921_s8: s8 {
					regulator-min-microvolt = <2200000>;
					regulator-max-microvolt = <2200000>;
					qcom,switch-mode-frequency = <1600000>;
				};

				/* PMOS LDO */
				pm8921_l1: l1 {
					regulator-always-on;
					regulator-min-microvolt = <1100000>;
					regulator-max-microvolt = <1100000>;
					bias-pull-down;
				};

				pm8921_l2: l2 {
					regulator-min-microvolt = <1200000>;
					regulator-max-microvolt = <1200000>;
					bias-pull-down;
				};

				pm8921_l3: l3 {
					regulator-min-microvolt = <3075000>;
					regulator-max-microvolt = <3075000>;
					bias-pull-down;
				};

				pm8921_l4: l4 {
					regulator-always-on;
					regulator-min-microvolt = <1800000>;
					regulator-max-microvolt = <1800000>;
					bias-pull-down;
				};

				pm8921_l5: l5 {
					regulator-min-microvolt = <2950000>;
					regulator-max-microvolt = <2950000>;
					bias-pull-down;
				};

				pm8921_l6: l6 {
					regulator-min-microvolt = <2950000>;
					regulator-max-microvolt = <2950000>;
					bias-pull-down;
				};

				pm8921_l7: l7 {
					regulator-min-microvolt = <1850000>;
					regulator-max-microvolt = <2950000>;
					bias-pull-down;
				};

				pm8921_l8: l8 {
					regulator-min-microvolt = <2800000>;
					regulator-max-microvolt = <2800000>;
					bias-pull-down;
				};

				pm8921_l9: l9 {
					regulator-min-microvolt = <3000000>;
					regulator-max-microvolt = <3000000>;
					bias-pull-down;
				};

				pm8921_l10: l10 {
					regulator-min-microvolt = <2900000>;
					regulator-max-microvolt = <2900000>;
					bias-pull-down;
				};

				pm8921_l11: l11 {
					regulator-min-microvolt = <3000000>;
					regulator-max-microvolt = <3000000>;
					bias-pull-down;
				};

				pm8921_l12: l12 {
					regulator-min-microvolt = <1200000>;
					regulator-max-microvolt = <1200000>;
					bias-pull-down;
				};

					    /*
				pm8921_l13: l13 {
					regulator-min-microvolt = <2220000>;
					regulator-max-microvolt = <2220000>;
				};
				*/

				pm8921_l14: l14 {
					regulator-min-microvolt = <1800000>;
					regulator-max-microvolt = <1800000>;
					bias-pull-down;
				};

				pm8921_l15: l15 {
					regulator-min-microvolt = <1800000>;
					regulator-max-microvolt = <2950000>;
					bias-pull-down;
				};

				pm8921_l16: l16 {
					regulator-min-microvolt = <2800000>;
					regulator-max-microvolt = <2800000>;
					bias-pull-down;
				};

				pm8921_l17: l17 {
					regulator-min-microvolt = <2000000>;
					regulator-max-microvolt = <2000000>;
					bias-pull-down;
				};

				pm8921_l18: l18 {
					regulator-min-microvolt = <1300000>;
					regulator-max-microvolt = <1800000>;
					bias-pull-down;
				};

				pm8921_l21: l21 {
					regulator-min-microvolt = <1050000>;
					regulator-max-microvolt = <1050000>;
					bias-pull-down;
				};

				pm8921_l22: l22 {
					regulator-min-microvolt = <2600000>;
					regulator-max-microvolt = <2600000>;
					bias-pull-down;
				};

				pm8921_l23: l23 {
					regulator-min-microvolt = <1800000>;
					regulator-max-microvolt = <1800000>;
					bias-pull-down;
				};

				pm8921_l24: l24 {
					regulator-min-microvolt = <750000>;
					regulator-max-microvolt = <1150000>;
					bias-pull-down;
				};

				pm8921_l25: l25 {
					regulator-always-on;
					regulator-min-microvolt = <1250000>;
					regulator-max-microvolt = <1250000>;
					bias-pull-down;
				};

				pm8921_l27: l27 {
					regulator-min-microvolt = <1100000>;
					regulator-max-microvolt = <1100000>;
				};

				pm8921_l28: l28 {
					regulator-min-microvolt = <1050000>;
					regulator-max-microvolt = <1050000>;
					bias-pull-down;
				};

				pm8921_l29: l29 {
					regulator-min-microvolt = <2000000>;
					regulator-max-microvolt = <2000000>;
					bias-pull-down;
				};

				/* Low Voltage Switch */
				pm8921_lvs1: lvs1 {
					bias-pull-down;
				};

				pm8921_lvs2: lvs2 {
					bias-pull-down;
				};

				pm8921_lvs3: lvs3 {
					bias-pull-down;
				};

				pm8921_lvs4: lvs4 {
					bias-pull-down;
				};

				pm8921_lvs5: lvs5 {
					bias-pull-down;
				};

				pm8921_lvs6: lvs6 {
					bias-pull-down;
				};

				pm8921_lvs7: lvs7 {
					bias-pull-down;
				};

				pm8921_ncp: ncp {
					regulator-min-microvolt = <2000000>;
					regulator-max-microvolt = <2000000>;
					qcom,switch-mode-frequency = <1600000>;
				};
			};
		};

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] regulator: qcom-rpm: Rework for single device

[Srinivas Kandagatla]

Thanks for the patch.

On 27/02/15 00:00, Stephen Boyd wrote:
> The RPM regulators are not individual devices. Creating platform
> devices for each regulator bloats the kernel's runtime memory
> footprint by ~12k. Let's rework this driver to be a single
> platform device for all the RPM regulators. This makes the
> DT match the schematic/datasheet closer too because now the
> regulators node contains a list of supplies at the package level
> for a particular PMIC model.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---

Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Tested SATA, USB with the dt patches on top of mainline.

Mark/Stephen, Are you going to take this patch as fix into rc release? 
Depending on which I could rebase and send the DT patches for peripheral 
support on APQ8064.

--srini

>
> On 02/24, Bjorn Andersson wrote:
>> On Wed 18 Feb 18:29 PST 2015, Stephen Boyd wrote:
>>
>>> On 02/18/15 13:08, Bjorn Andersson wrote:
>>>> On Wed, Feb 18, 2015 at 12:28 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>
>> After taking a deeper look at this I've come to the following
>> conclusion:
>>
>> We can save 2100 bytes of data by spreading out the magic numbers from
>> the rpm resource tables into the regulator, clock and bus drivers. At
>> the cost of having this rpm-specific information spread out.
>>
>> Separate of that we could rewrite the entire regulator implementation to
>> define all regulators in platform specific tables in the regulators.
>> This would save about 12-15k of ram.
>
> Cool I started doing that.
>
>>
>> This can however be done in two ways:
>> 1) We modify the mfd driver to instatiate 3 mfd_cells; one of them
>> being "qcom,rpm-regulators". We modify the regulator driver to have
>> tables of the regulators for each platform and matching regulator
>> parameters by of_node name and registering these.
>>
>> 2) We stick with this binding, we then go ahead and do the same
>> modification to the mfd as above - passing the rpm of_node to the
>> regulator driver, that walks the children and match that to the current
>> compatible list. (in other words, we replace of_platform_populate with
>> our own mechamism).
>>
>>
>> The benefit of 2 is that we can use the code that was posted 10 months
>> ago and merged 3 months ago until someone prioritize those 12kb.
>
> I did (1) without modifying the MFD driver.
>
>>
>>
>> To take either of these paths the issue at the bottom has to be
>> resolved first.
>
> Right. I think that's resolved now.
>
>>
>>
>> More answers follows inline:
>>
>>>
>>> We're already maintaining these tables in qcom-rpm.c. I'm advocating for
>>> removing those tables from the rpm driver and putting the data into the
>>> regulator driver. Then we don't have to index into a sparse table to
>>> figure out what values the RPM wants to use. Instead we have the data at
>>> hand for a particular regulator based on the of_regulator_match.
>>>
>>
>> I do like the "separation of concerns" between the rpm driver and the
>> children, but you're right in that we're wasting almost 3kb in doing so:
>>
>> (gdb) p sizeof(apq8064_rpm_resource_table) + sizeof(msm8660_rpm_resource_table) + sizeof(msm8960_rpm_resource_table)
>> $2 = 6384
>>
>> vs
>>
>> (gdb) p sizeof(struct qcom_rpm_resource) * (77 + 74 + 73)
>> $3 = 3584
>>
>> The alternative would be to spread those magic constants out in the
>> three client drivers.
>>
>> Looking at this from a software design perspective I stand by the
>> argument that the register layout of the rpm is not a regulator driver
>> implementation detail and is better kept separate.
>>
>> As we decided to define the regulators in code but the actual
>> composition in dt it was not possible to put the individual numbers in
>> DT. Having reg = <165 68 48> does not make any sense, unless we perhaps
>> maintain said table in the dt binding documentation.
>
> For now I've left it so that the #define is used in C code.
>
>>
>>>  From what I can tell, that data is split in two places. The regulator
>>> type knows how big the data we want to send is (1 or 2) and that is
>>> checked in the RPM driver to make sure that the two agree (this part
>>> looks like over-defensive coding).
>>
>> Yeah, after debugging production code for years I like to be slightly on
>> the defensive side. But the size could of course be dropped from the
>> resource-tables; reducing the savings of your suggestion by another 800
>> bytes...
>
> Sounds good. We should probably do it.
>
>>
>>> Then the DT has a made up number that
>>> maps to 3 different numbers in the RPM driver.
>>
>> Reading the following snippet in my dts file makes imidiate sense:
>>
>> reg = <QCOM_RPM_PM8921_SMPS1>
>>
>> the following doesn't make any sense:
>>
>> reg = <116 31 30>;
>>
>> Maybe it's write-once in a dts so it doesn't matter that much - as long
>> as the node is descriptive. But I like the properties to be human
>> understandable.
>
> Wouldn't a
>
>   #define QCOM_RPM_PM8921_SMPS1 116 31 30
>
> suffice? That looks to be the same readability.
>
>>
>>> If the RPM was moving these offsets
>>> around within the same compatible string it would make sense to put that
>>> in DT and figure out dynamically what the offsets are because they
>>> really can be different.
>>
>> The proposed binding states the following:
>>
>> - compatible:
>>          Usage: required
>>          Value type: <string>
>>          Definition: must be one of:
>>                      "qcom,rpm-pm8058-smps"
>>                      "qcom,rpm-pm8901-ftsmps"
>>                      "qcom,rpm-pm8921-smps"
>>                      "qcom,rpm-pm8921-ftsmps"
>>
>> Doesn't this map to the case you say make sense?
>
> I mean the compatible for the entire RPM/regulators node (i.e. qcom,rpm-msm8960
> or qcom,rpm-apq8064). Not each individual regulator, which IMO shouldn't need a compatible at all.
>
>>
>>>>
>>>> Non the less, we must provide this information to the regulator
>>>> framework in some way if we want its help.
>>>> If we define all regulators in code (and just bring their properties
>>>> from DT) then we could encapsulate the relationship there as well.
>>>> But with the current suggested solution of having all this data coming
>>>> from DT I simply rely on the existing infrastructure for describing
>>>> supply-dependencies in DT.
>>>>
>>>>
>>>
>>> Yes I don't see how putting the data in C or in DT or having a
>>> regulators encapsulating node makes it impossible to specify the supply.
>>>
>>
>> Me neither, a month ago...
>>
>> Here's the discussion we had with Mark regarding having the regulator
>> core pick up -supply properties from the individual child of_nodes of a
>> regulator driver:
>>
>> https://lkml.org/lkml/2015/2/4/759
>>
>> As far as I can see this has to be fixed for us to move over to having
>> one driver registering all our regulators, independently of how we
>> structure this binding.
>>
>
> Mark is saying that the entire PMIC (e.g. PM8921) is the package. On the
> package there are pins like VIN_L27, VIN_L24, etc (look at a schematic). When
> you consider the regulators node (compatible = "qcom,rpm-pm8921-regulators")
> is the package then you can see that it should have a handful of vin_*-supply
> properties that correspond to what you see in the schematic and the datasheet.
> This way if a board designer has decided to run a particular supply to
> VIN_L1_L2_L12_L18 they just wire it up in DT and everything works. They don't
> have to look at the binding for the L1, L2, L12, and L18 regulators and figure
> out that it uses vin-supply for the supply. Plus everything matches what
> they see in the schematic and datasheets.
>
> Note: This patch is not complete. We still need to fill out the other pmics
> and standalone regulators (smb208) that this driver is for.
>
>   drivers/regulator/qcom_rpm-regulator.c | 484 ++++++++++++++++++++++++++++-----
>   1 file changed, 416 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
> index 00c5cc3d9546..538733bb7e8b 100644
> --- a/drivers/regulator/qcom_rpm-regulator.c
> +++ b/drivers/regulator/qcom_rpm-regulator.c
> @@ -581,27 +581,347 @@ static const struct qcom_rpm_reg smb208_smps = {
>   	.supports_force_mode_bypass = false,
>   };
>
> +struct qcom_rpm_reg_desc {
> +	const struct qcom_rpm_reg *template;
> +	int resource;
> +	const char *supply;
> +};
> +
> +struct qcom_rpm_of_match {
> +	struct of_regulator_match *of_match;
> +	unsigned int size;
> +};
> +
> +#define DEFINE_QCOM_RPM_OF_MATCH(t)		\
> +	struct qcom_rpm_of_match t##_m = {	\
> +		.of_match = (t), 		\
> +		.size = ARRAY_SIZE(t), 		\
> +	}
> +
> +static struct of_regulator_match pm8921_regs[] = {
> +	{
> +		.name = "s1",
> +		.driver_data = &(struct qcom_rpm_reg_desc){
> +			.template = &pm8921_smps,
> +			.resource = QCOM_RPM_PM8921_SMPS1,
> +		},
> +	},
> +	{
> +		.name = "s2",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_smps,
> +			.resource = QCOM_RPM_PM8921_SMPS2,
> +		},
> +	},
> +	{
> +		.name = "s3",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_smps,
> +			.resource = QCOM_RPM_PM8921_SMPS3,
> +		},
> +	},
> +	{
> +		.name = "s4",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_smps,
> +			.resource = QCOM_RPM_PM8921_SMPS4,
> +		},
> +	},
> +	{
> +		.name = "s7",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_smps,
> +			.resource = QCOM_RPM_PM8921_SMPS7,
> +		},
> +	},
> +	{
> +		.name = "s8",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_smps,
> +			.resource = QCOM_RPM_PM8921_SMPS8,
> +		},
> +	},
> +	{
> +		.name = "l1",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_nldo,
> +			.resource = QCOM_RPM_PM8921_LDO1,
> +			.supply = "vin_l1_l2_l12_l18",
> +		},
> +	},
> +	{
> +		.name = "l2",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_nldo,
> +			.resource = QCOM_RPM_PM8921_LDO2,
> +			.supply = "vin_l1_l2_l12_l18",
> +		},
> +	},
> +	{
> +		.name = "l3",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO3,
> +		},
> +	},
> +	{
> +		.name = "l4",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO4,
> +		},
> +	},
> +	{
> +		.name = "l5",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO5,
> +		},
> +	},
> +	{
> +		.name = "l6",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO6,
> +		},
> +	},
> +	{
> +		.name = "l7",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO7,
> +		},
> +	},
> +	{
> +		.name = "l8",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO8,
> +		},
> +	},
> +	{
> +		.name = "l9",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO9,
> +		},
> +	},
> +	{
> +		.name = "l10",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO10,
> +		},
> +	},
> +	{
> +		.name = "l11",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO11,
> +		},
> +	},
> +	{
> +		.name = "l12",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_nldo,
> +			.resource = QCOM_RPM_PM8921_LDO12,
> +			.supply = "vin_l1_l2_l12_l18",
> +		},
> +	},
> +	{
> +		.name = "l13",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO13,
> +		},
> +	},
> +	{
> +		.name = "l14",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO14,
> +		},
> +	},
> +	{
> +		.name = "l15",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO15,
> +		},
> +	},
> +	{
> +		.name = "l16",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO16,
> +		},
> +	},
> +	{
> +		.name = "l17",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO17,
> +		},
> +	},
> +	{
> +		.name = "l18",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_nldo,
> +			.resource = QCOM_RPM_PM8921_LDO18,
> +			.supply = "vin_l1_l2_l12_l18",
> +		},
> +	},
> +	{
> +		.name = "l21",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO21,
> +		},
> +	},
> +	{
> +		.name = "l22",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO22,
> +		},
> +	},
> +	{
> +		.name = "l23",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO23,
> +		},
> +	},
> +	{
> +		.name = "l24",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_nldo1200,
> +			.resource = QCOM_RPM_PM8921_LDO24,
> +			.supply = "vin_l24",
> +		},
> +	},
> +	{
> +		.name = "l25",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_nldo1200,
> +			.resource = QCOM_RPM_PM8921_LDO25,
> +			.supply = "vin_l25",
> +		},
> +	},
> +	{
> +		.name = "l26",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_nldo1200,
> +			.resource = QCOM_RPM_PM8921_LDO26,
> +		},
> +	},
> +	{
> +		.name = "l27",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_nldo1200,
> +			.resource = QCOM_RPM_PM8921_LDO27,
> +			.supply = "vin_l27",
> +		},
> +	},
> +	{
> +		.name = "l28",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_nldo1200,
> +			.resource = QCOM_RPM_PM8921_LDO28,
> +			.supply = "vin_l28",
> +		},
> +	},
> +	{
> +		.name = "l29",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_pldo,
> +			.resource = QCOM_RPM_PM8921_LDO29
> +		},
> +	},
> +	{
> +		.name = "lvs1",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_switch,
> +			.resource = QCOM_RPM_PM8921_LVS1,
> +			.supply = "vin_lvs_1_3_6",
> +		},
> +	},
> +	{
> +		.name = "lvs2",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_switch,
> +			.resource = QCOM_RPM_PM8921_LVS2,
> +			.supply = "vin_lvs2",
> +		},
> +	},
> +	{
> +		.name = "lvs3",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_switch,
> +			.resource = QCOM_RPM_PM8921_LVS3,
> +			.supply = "vin_lvs_1_3_6",
> +		},
> +	},
> +	{
> +		.name = "lvs4",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_switch,
> +			.resource = QCOM_RPM_PM8921_LVS4,
> +			.supply = "vin_lvs_4_5_7",
> +		},
> +	},
> +	{
> +		.name = "lvs5",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_switch,
> +			.resource = QCOM_RPM_PM8921_LVS5,
> +			.supply = "vin_lvs_4_5_7",
> +		},
> +	},
> +	{
> +		.name = "lvs6",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_switch,
> +			.resource = QCOM_RPM_PM8921_LVS6,
> +			.supply = "vin_lvs_1_3_6",
> +		},
> +	},
> +	{
> +		.name = "lvs7",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_switch,
> +			.resource = QCOM_RPM_PM8921_LVS7,
> +			.supply = "vin_lvs_4_5_7",
> +		},
> +	},
> +	{
> +		.name = "usb-switch",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_switch,
> +			.resource = QCOM_RPM_USB_OTG_SWITCH,
> +		},
> +	},
> +	{
> +		.name = "hdmi-switch",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_switch,
> +			.resource = QCOM_RPM_HDMI_SWITCH,
> +		},
> +	},
> +	{
> +		.name = "ncp",
> +		.driver_data = &(struct qcom_rpm_reg_desc) {
> +			.template = &pm8921_ncp,
> +			.resource = QCOM_RPM_PM8921_NCP,
> +			.supply = "vin_ncp",
> +		},
> +	},
> +};
> +
> +static DEFINE_QCOM_RPM_OF_MATCH(pm8921_regs);
> +
>   static const struct of_device_id rpm_of_match[] = {
> -	{ .compatible = "qcom,rpm-pm8058-pldo",     .data = &pm8058_pldo },
> -	{ .compatible = "qcom,rpm-pm8058-nldo",     .data = &pm8058_nldo },
> -	{ .compatible = "qcom,rpm-pm8058-smps",     .data = &pm8058_smps },
> -	{ .compatible = "qcom,rpm-pm8058-ncp",      .data = &pm8058_ncp },
> -	{ .compatible = "qcom,rpm-pm8058-switch",   .data = &pm8058_switch },
> -
> -	{ .compatible = "qcom,rpm-pm8901-pldo",     .data = &pm8901_pldo },
> -	{ .compatible = "qcom,rpm-pm8901-nldo",     .data = &pm8901_nldo },
> -	{ .compatible = "qcom,rpm-pm8901-ftsmps",   .data = &pm8901_ftsmps },
> -	{ .compatible = "qcom,rpm-pm8901-switch",   .data = &pm8901_switch },
> -
> -	{ .compatible = "qcom,rpm-pm8921-pldo",     .data = &pm8921_pldo },
> -	{ .compatible = "qcom,rpm-pm8921-nldo",     .data = &pm8921_nldo },
> -	{ .compatible = "qcom,rpm-pm8921-nldo1200", .data = &pm8921_nldo1200 },
> -	{ .compatible = "qcom,rpm-pm8921-smps",     .data = &pm8921_smps },
> -	{ .compatible = "qcom,rpm-pm8921-ftsmps",   .data = &pm8921_ftsmps },
> -	{ .compatible = "qcom,rpm-pm8921-ncp",      .data = &pm8921_ncp },
> -	{ .compatible = "qcom,rpm-pm8921-switch",   .data = &pm8921_switch },
> -
> -	{ .compatible = "qcom,rpm-smb208", .data = &smb208_smps },
> +	{ .compatible = "qcom,rpm-pm8921-regulators", .data = &pm8921_regs_m },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(of, rpm_of_match);
> @@ -619,7 +939,8 @@ static int rpm_reg_set(struct qcom_rpm_reg *vreg,
>   	return 0;
>   }
>
> -static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg)
> +static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg,
> +		struct device_node *of_node)
>   {
>   	static const int freq_table[] = {
>   		19200000, 9600000, 6400000, 4800000, 3840000, 3200000, 2740000,
> @@ -633,9 +954,10 @@ static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg)
>   	int i;
>
>   	key = "qcom,switch-mode-frequency";
> -	ret = of_property_read_u32(dev->of_node, key, &freq);
> +	ret = of_property_read_u32(of_node, key, &freq);
>   	if (ret) {
> -		dev_err(dev, "regulator requires %s property\n", key);
> +		dev_err(dev, "regulator %s requires %s property\n",
> +				vreg->desc.name, key);
>   		return -EINVAL;
>   	}
>
> @@ -646,88 +968,74 @@ static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg)
>   		}
>   	}
>
> -	dev_err(dev, "invalid frequency %d\n", freq);
> +	dev_err(dev, "regulator %s has invalid frequency %d\n", vreg->desc.name,
> +			freq);
>   	return -EINVAL;
>   }
>
> -static int rpm_reg_probe(struct platform_device *pdev)
> +static int rpm_regulator_register(struct platform_device *pdev,
> +				  struct of_regulator_match *match,
> +				  struct qcom_rpm *rpm)
>   {
> +	struct qcom_rpm_reg_desc *rpm_desc = match->driver_data;
>   	struct regulator_init_data *initdata;
> -	const struct qcom_rpm_reg *template;
> -	const struct of_device_id *match;
>   	struct regulator_config config = { };
>   	struct regulator_dev *rdev;
>   	struct qcom_rpm_reg *vreg;
> +	struct device_node *of_node = match->of_node;
>   	const char *key;
>   	u32 force_mode;
>   	bool pwm;
>   	u32 val;
>   	int ret;
>
> -	match = of_match_device(rpm_of_match, &pdev->dev);
> -	template = match->data;
> -
>   	vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
> -	if (!vreg) {
> -		dev_err(&pdev->dev, "failed to allocate vreg\n");
> +	if (!vreg)
>   		return -ENOMEM;
> -	}
> -	memcpy(vreg, template, sizeof(*vreg));
> +
> +	memcpy(vreg, rpm_desc->template, sizeof(*vreg));
>   	mutex_init(&vreg->lock);
>   	vreg->dev = &pdev->dev;
>   	vreg->desc.id = -1;
>   	vreg->desc.owner = THIS_MODULE;
>   	vreg->desc.type = REGULATOR_VOLTAGE;
> -	vreg->desc.name = pdev->dev.of_node->name;
> -	vreg->desc.supply_name = "vin";
> -
> +	vreg->desc.name = of_node->name;
> +	vreg->desc.supply_name = rpm_desc->supply;
>   	vreg->rpm = dev_get_drvdata(pdev->dev.parent);
> -	if (!vreg->rpm) {
> -		dev_err(&pdev->dev, "unable to retrieve handle to rpm\n");
> -		return -ENODEV;
> -	}
> -
> -	initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
> -					      &vreg->desc);
> -	if (!initdata)
> -		return -EINVAL;
> -
> -	key = "reg";
> -	ret = of_property_read_u32(pdev->dev.of_node, key, &val);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to read %s\n", key);
> -		return ret;
> -	}
> -	vreg->resource = val;
> +	vreg->resource = rpm_desc->resource;
> +	initdata = match->init_data;
>
>   	if ((vreg->parts->uV.mask || vreg->parts->mV.mask) &&
>   	    (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) {
> -		dev_err(&pdev->dev, "no voltage specified for regulator\n");
> +		dev_err(&pdev->dev, "no voltage specified for regulator %s\n",
> +				vreg->desc.name);
>   		return -EINVAL;
>   	}
>
>   	key = "bias-pull-down";
> -	if (of_property_read_bool(pdev->dev.of_node, key)) {
> +	if (of_property_read_bool(of_node, key)) {
>   		ret = rpm_reg_set(vreg, &vreg->parts->pd, 1);
>   		if (ret) {
> -			dev_err(&pdev->dev, "%s is invalid", key);
> +			dev_err(&pdev->dev, "%s is invalid (%s)", key,
> +					vreg->desc.name);
>   			return ret;
>   		}
>   	}
>
>   	if (vreg->parts->freq.mask) {
> -		ret = rpm_reg_of_parse_freq(&pdev->dev, vreg);
> +		ret = rpm_reg_of_parse_freq(&pdev->dev, vreg, of_node);
>   		if (ret < 0)
>   			return ret;
>   	}
>
>   	if (vreg->parts->pm.mask) {
>   		key = "qcom,power-mode-hysteretic";
> -		pwm = !of_property_read_bool(pdev->dev.of_node, key);
> +		pwm = !of_property_read_bool(of_node, key);
>
>   		ret = rpm_reg_set(vreg, &vreg->parts->pm, pwm);
>   		if (ret) {
> -			dev_err(&pdev->dev, "failed to set power mode\n");
> +			dev_err(&pdev->dev, "failed to set power mode (%s)\n",
> +					vreg->desc.name);
>   			return ret;
>   		}
>   	}
> @@ -736,11 +1044,12 @@ static int rpm_reg_probe(struct platform_device *pdev)
>   		force_mode = -1;
>
>   		key = "qcom,force-mode";
> -		ret = of_property_read_u32(pdev->dev.of_node, key, &val);
> +		ret = of_property_read_u32(of_node, key, &val);
>   		if (ret == -EINVAL) {
>   			val = QCOM_RPM_FORCE_MODE_NONE;
>   		} else if (ret < 0) {
> -			dev_err(&pdev->dev, "failed to read %s\n", key);
> +			dev_err(&pdev->dev, "failed to read %s (%s)\n", key,
> +					vreg->desc.name);
>   			return ret;
>   		}
>
> @@ -775,13 +1084,15 @@ static int rpm_reg_probe(struct platform_device *pdev)
>   		}
>
>   		if (force_mode == -1) {
> -			dev_err(&pdev->dev, "invalid force mode\n");
> +			dev_err(&pdev->dev, "invalid force mode (%s)\n",
> +					vreg->desc.name);
>   			return -EINVAL;
>   		}
>
>   		ret = rpm_reg_set(vreg, &vreg->parts->fm, force_mode);
>   		if (ret) {
> -			dev_err(&pdev->dev, "failed to set force mode\n");
> +			dev_err(&pdev->dev, "failed to set force mode (%s)\n",
> +					vreg->desc.name);
>   			return ret;
>   		}
>   	}
> @@ -789,11 +1100,48 @@ static int rpm_reg_probe(struct platform_device *pdev)
>   	config.dev = &pdev->dev;
>   	config.init_data = initdata;
>   	config.driver_data = vreg;
> -	config.of_node = pdev->dev.of_node;
> +	config.of_node = of_node;
> +
>   	rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config);
> -	if (IS_ERR(rdev)) {
> -		dev_err(&pdev->dev, "can't register regulator\n");
> -		return PTR_ERR(rdev);
> +
> +	return PTR_ERR_OR_ZERO(rdev);
> +}
> +
> +static int rpm_reg_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	const struct qcom_rpm_of_match *rpm_match;
> +	struct of_regulator_match *of_match;
> +	struct qcom_rpm *rpm;
> +	int ret;
> +
> +	rpm = dev_get_drvdata(pdev->dev.parent);
> +	if (!rpm) {
> +		dev_err(&pdev->dev, "unable to retrieve handle to rpm\n");
> +		return -ENODEV;
> +	}
> +
> +	match = of_match_device(rpm_of_match, &pdev->dev);
> +	rpm_match = match->data;
> +	of_match = rpm_match->of_match;
> +
> +	ret = of_regulator_match(&pdev->dev, pdev->dev.of_node,
> +				 of_match,
> +				 rpm_match->size);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"Error parsing regulator init data: %d\n", ret);
> +		return ret;
> +	}
> +
> +	for (; of_match < rpm_match->of_match + rpm_match->size; of_match++) {
> +		if (!of_match->of_node)
> +			continue;
> +		ret = rpm_regulator_register(pdev, of_match, rpm);
> +		if (ret) {
> +			dev_err(&pdev->dev, "can't register regulator\n");
> +			return ret;
> +		}
>   	}
>
>   	return 0;
>

Re: [PATCH] regulator: qcom-rpm: Rework for single device

[Bjorn Andersson]

On Thu 26 Feb 16:00 PST 2015, Stephen Boyd wrote:

> The RPM regulators are not individual devices. Creating platform
> devices for each regulator bloats the kernel's runtime memory
> footprint by ~12k. Let's rework this driver to be a single
> platform device for all the RPM regulators. This makes the
> DT match the schematic/datasheet closer too because now the
> regulators node contains a list of supplies at the package level
> for a particular PMIC model.
> 

As we discussed on IRC I agree with the benefits of redesigning this,
especially making the supplies matching the data sheet rather than the
code...

As you write below your patch needs to be updated with regards to the
missing platforms, but it also needs to be rebased upon Mark's tree.


By moving to a dt binding structure that better seem to resemble what's
expected by the regulator framework we can drop some of the code from
this driver and let the framework take care of matching of child nodes
etc.

I took the liberty of sending out a patch series with a slightly
different solution, please let me know what you think. If nothing else
it has an updated proposal for the dt binding document.

[..]
> >
> > This can however be done in two ways:
> > 1) We modify the mfd driver to instatiate 3 mfd_cells; one of them
> > being "qcom,rpm-regulators". We modify the regulator driver to have
> > tables of the regulators for each platform and matching regulator
> > parameters by of_node name and registering these.
> >
> > 2) We stick with this binding, we then go ahead and do the same
> > modification to the mfd as above - passing the rpm of_node to the
> > regulator driver, that walks the children and match that to the current
> > compatible list. (in other words, we replace of_platform_populate with
> > our own mechamism).
> >
> >
> > The benefit of 2 is that we can use the code that was posted 10 months
> > ago and merged 3 months ago until someone prioritize those 12kb.
> 
> I did (1) without modifying the MFD driver.
> 

I tried moving the table from the rpm into the regulator driver, but the
problem is that the regulator driver can no long only map to a PMIC.

The "address" depends on both pmic and rpm/platform, so we would need to
either have a more specific compatible or double check parent compatible
and map that to one table per platform.

So I've left this as well for now and after looking at the options I
think it's better to let it be (but perhaps dropping the 800 bytes of
"size").

[..]

> > As we decided to define the regulators in code but the actual
> > composition in dt it was not possible to put the individual numbers in
> > DT. Having reg = <165 68 48> does not make any sense, unless we perhaps
> > maintain said table in the dt binding documentation.
> 
> For now I've left it so that the #define is used in C code.
> 

As we've dropped it from dt we're free to change it later.

> >
> > > From what I can tell, that data is split in two places. The regulator
> > > type knows how big the data we want to send is (1 or 2) and that is
> > > checked in the RPM driver to make sure that the two agree (this part
> > > looks like over-defensive coding).
> >
> > Yeah, after debugging production code for years I like to be slightly on
> > the defensive side. But the size could of course be dropped from the
> > resource-tables; reducing the savings of your suggestion by another 800
> > bytes...
> 
> Sounds good. We should probably do it.
> 

I looked at making it depend on DEBUG or something, but I don't have any
good patch at this point. I just imagine that it would be mighty useful
to have when implementing the msm_rpm driver - handling entries that are
of very strange sizes.

Maybe I'm just pessimistic...

[..]

> >
> > Me neither, a month ago...
> >
> > Here's the discussion we had with Mark regarding having the regulator
> > core pick up -supply properties from the individual child of_nodes of a
> > regulator driver:
> >
> > https://lkml.org/lkml/2015/2/4/759
> >
> > As far as I can see this has to be fixed for us to move over to having
> > one driver registering all our regulators, independently of how we
> > structure this binding.
> >
> 
> Mark is saying that the entire PMIC (e.g. PM8921) is the package. On the
> package there are pins like VIN_L27, VIN_L24, etc (look at a schematic). When
> you consider the regulators node (compatible = "qcom,rpm-pm8921-regulators")
> is the package then you can see that it should have a handful of vin_*-supply
> properties that correspond to what you see in the schematic and the datasheet.
> This way if a board designer has decided to run a particular supply to
> VIN_L1_L2_L12_L18 they just wire it up in DT and everything works. They don't
> have to look at the binding for the L1, L2, L12, and L18 regulators and figure
> out that it uses vin-supply for the supply. Plus everything matches what
> they see in the schematic and datasheets.
> 

This makes sense and it can't be represented in the previously suggested
binding. Splitting the regulators in PMIC subnodes turned out to make
this quite clear as well. So I like the end result.

> Note: This patch is not complete. We still need to fill out the other pmics
> and standalone regulators (smb208) that this driver is for.
> 

I unfortunately don't have documentation for the ipq8064 platform, so I
was not able to add this to my tables either. But I added the needed
PMICs for 8660 and did some quick prototyping on 8974 as well.

>  drivers/regulator/qcom_rpm-regulator.c | 484 ++++++++++++++++++++++++++++-----
>  1 file changed, 416 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c

[..]

> +
> +static int rpm_reg_probe(struct platform_device *pdev)
> +{
> +       const struct of_device_id *match;
> +       const struct qcom_rpm_of_match *rpm_match;
> +       struct of_regulator_match *of_match;
> +       struct qcom_rpm *rpm;
> +       int ret;
> +
> +       rpm = dev_get_drvdata(pdev->dev.parent);
> +       if (!rpm) {
> +               dev_err(&pdev->dev, "unable to retrieve handle to rpm\n");
> +               return -ENODEV;
> +       }
> +
> +       match = of_match_device(rpm_of_match, &pdev->dev);
> +       rpm_match = match->data;
> +       of_match = rpm_match->of_match;
> +
> +       ret = of_regulator_match(&pdev->dev, pdev->dev.of_node,
> +                                of_match,
> +                                rpm_match->size);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev,
> +                       "Error parsing regulator init data: %d\n", ret);
> +               return ret;
> +       }
> +
> +       for (; of_match < rpm_match->of_match + rpm_match->size; of_match++) {
> +               if (!of_match->of_node)
> +                       continue;
> +               ret = rpm_regulator_register(pdev, of_match, rpm);
> +               if (ret) {
> +                       dev_err(&pdev->dev, "can't register regulator\n");
> +                       return ret;
> +               }

I believe this can be done more elegantly by setting desc->of_match when
calling regulator_register() and have that handle the of-matching
internally.

>         }
> 
>         return 0;

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html