[PATCH] regulator: core: add of_match_full_name boolean flag

[PATCH] regulator: core: add of_match_full_name boolean flag

[Cristian Marussi]

When an .of_match non-null string is defined in struct regulator_desc, the
regulator core searches for regulators trying to match, at first, such
string against the 'regulator-compatible' property and then falls back to
use the name of the node itself.

Property 'regulator-compatible' is now deprecated (even if still widely
used in the code base), and the node-name fallback works fine only as long
as the nodes are named in an unique way; if it makes sense to use a common
name and identifying them using an index through a 'reg' property the
standard advices to use a naming in the form <common-name>@<unit>.

In this case the above matching mechanism based on the simple (common) name
will fail and the only viable alternative would be to properly define the
deprecrated 'regulator-compatible' property equal to the full name
<common-name>@<unit>.

In order to address this case without using such deprecated property,
define a new boolean flag .of_match_full_name in struct regulator_desc to
force the core to match against the node full-name instead.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/regulator/of_regulator.c | 3 ++-
 include/linux/regulator/driver.h | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 06c0b15fe4c0..f60cb0093b40 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -414,7 +414,8 @@ device_node *regulator_of_get_init_node(struct device *dev,
 	for_each_available_child_of_node(search, child) {
 		name = of_get_property(child, "regulator-compatible", NULL);
 		if (!name)
-			name = child->name;
+			name = !desc->of_match_full_name ?
+				child->name : child->full_name;
 
 		if (!strcmp(desc->of_match, name)) {
 			of_node_put(search);
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 8539f34ae42b..5d9b011fcef6 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -223,6 +223,8 @@ enum regulator_type {
  * @name: Identifying name for the regulator.
  * @supply_name: Identifying the regulator supply
  * @of_match: Name used to identify regulator in DT.
+ * @of_match_full_name: A flag to indicate that the of_match string, if
+ *			present, should be matched against the node full_name.
  * @regulators_node: Name of node containing regulator definitions in DT.
  * @of_parse_cb: Optional callback called only if of_match is present.
  *               Will be called for each regulator parsed from DT, during
@@ -314,6 +316,7 @@ struct regulator_desc {
 	const char *name;
 	const char *supply_name;
 	const char *of_match;
+	bool of_match_full_name;
 	const char *regulators_node;
 	int (*of_parse_cb)(struct device_node *,
 			    const struct regulator_desc *,
-- 
2.17.1

Re: [PATCH] regulator: core: add of_match_full_name boolean flag

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]

On Wed, Aug 19, 2020 at 03:04:48PM +0100, Cristian Marussi wrote:

> Property 'regulator-compatible' is now deprecated (even if still widely
> used in the code base), and the node-name fallback works fine only as long

I'm seeing a very small number of DTs using it, the majority of which
are pretty old - the arm64 ones are just mistakes on the part of
reviewers.

> as the nodes are named in an unique way; if it makes sense to use a common
> name and identifying them using an index through a 'reg' property the
> standard advices to use a naming in the form <common-name>@<unit>.

> In this case the above matching mechanism based on the simple (common) name
> will fail and the only viable alternative would be to properly define the
> deprecrated 'regulator-compatible' property equal to the full name
> <common-name>@<unit>.

This seems like a massive jump.  You appear to be saying that the reg
property is unusable which doesn't seem right to me?

> In order to address this case without using such deprecated property,
> define a new boolean flag .of_match_full_name in struct regulator_desc to
> force the core to match against the node full-name instead.

I can't tell from this description what this change is intended to do,
and I suspect it'd be difficult for anyone trying to figure out if they
should use this or not.  What is a full name and what should people put
in there?  What would one look like for example?  I have to look at the
code to see that this is changing to compare against the full_name field
in the node and there's no kerneldoc for struct device_node.

I'm also wondering why we can't just add this to the list of fallbacks
rather than requiring some custom per driver thing?

> -			name = child->name;
> +			name = !desc->of_match_full_name ?
> +				child->name : child->full_name;

Please write normal conditional statements for the benefits of people
who have to read this code, the extra ! in there isn't adding anything
here either.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: core: add of_match_full_name boolean flag

[Cristian Marussi]

Hi Mark 

thanks for the review.

On Wed, Aug 19, 2020 at 07:22:45PM +0100, Mark Brown wrote:
> On Wed, Aug 19, 2020 at 03:04:48PM +0100, Cristian Marussi wrote:
> 
> > Property 'regulator-compatible' is now deprecated (even if still widely
> > used in the code base), and the node-name fallback works fine only as long
> 
> I'm seeing a very small number of DTs using it, the majority of which
> are pretty old - the arm64 ones are just mistakes on the part of
> reviewers.
> 

Yes indeed 'widely' was a bit of an exaggeration, what I (poorly) meant to point
out was that beside still used is deprecated and so not a viable option to solve
my issue.

> > as the nodes are named in an unique way; if it makes sense to use a common
> > name and identifying them using an index through a 'reg' property the
> > standard advices to use a naming in the form <common-name>@<unit>.
> 
> > In this case the above matching mechanism based on the simple (common) name
> > will fail and the only viable alternative would be to properly define the
> > deprecrated 'regulator-compatible' property equal to the full name
> > <common-name>@<unit>.
> 
> This seems like a massive jump.  You appear to be saying that the reg
> property is unusable which doesn't seem right to me?
> 

The 'issue' I observed while working on another series was that with the
following example DT:

firmware {
	scmi {
		...
		scmi_voltage: scmi_protocol@17 {
			reg = <0x17>;
			
			regulators {
				#address-cells = <1>;
				#size-cells = <0>;

				regulator_scmi_discrete: regulator_scmi_discrete@0 {
					reg = <0>;
				};
				
				regulator_scmi_range: regulator_scmi_range@2 {
					reg = <2>;
				};
				
				regulator_scmi_vd3: regulator_scmi@3 {
					reg = <3>;
				};

				regulator_scmi_vd4: regulator_scmi@4 {
					reg = <4>;
				};
			};
		};
};

and the struct regulator_desc configured roughly as:

	sreg->desc.regulators_node = "regulators";
	sreg->desc.of_match = sreg->of_node->name;    <<< This being the regulator_* nodes

the regulator framework standard initialization routines were able to match univocally the
first two regulators above (and parse autonomously the constraints without me explicitly
calling of_get_regulator_init_data() as in a previous version of the series), but got fooled
by the last two since the node name is the same and they differ only by the index, which in
turn anyway seemed to me a sensible thing to be able to do when a node uses reg indexing.

Note that with these SCMI regulators this .of_match configuration happens dynamically at
run-time (as above) since it is defined by the DT and the SCMI fw platform which regulators are
visible and defined and the common SCMI regulator driver handles them all if defined in the DT
and known to the SCMI fw, while with the normal regulators the .of_match setup happens statically
at compile time with macros driver by driver, since the DT defines what is present and the driver
declares what can support.

With this patch you could support both the above naming instead configuring like:

	sreg->desc.regulators_node = "regulators";
	sreg->desc.of_match_full_name = true;
	sreg->desc.of_match = sreg->of_node->full_name;

but I'm not sure that this is needed and worth the effort sincerely at this point,
and probably makes more sense to look at this possible naming issue after I post
the whole original series (without this patch) just to be sure I'm not getting
something wrong somewhere else instead.

> > In order to address this case without using such deprecated property,
> > define a new boolean flag .of_match_full_name in struct regulator_desc to
> > force the core to match against the node full-name instead.
> 
> I can't tell from this description what this change is intended to do,
> and I suspect it'd be difficult for anyone trying to figure out if they
> should use this or not.  What is a full name and what should people put
> in there?  What would one look like for example?  I have to look at the
> code to see that this is changing to compare against the full_name field
> in the node and there's no kerneldoc for struct device_node.
> 

Yes I agree it is hard to understand how to use this from the commit log and if
it is useful or not in a specific use case.

> I'm also wondering why we can't just add this to the list of fallbacks
> rather than requiring some custom per driver thing?
> 

It's an option but it would have doubled the following strncmp() if we just started
checking against full_name every time we fail to find against name so I thought
to make it configurable with the new of_match_full_name....but, given it is far from
clear with this proposed patch, maybe it's better to use a default fallback as you said.

> > -			name = child->name;
> > +			name = !desc->of_match_full_name ?
> > +				child->name : child->full_name;
> 
> Please write normal conditional statements for the benefits of people
> who have to read this code, the extra ! in there isn't adding anything
> here either.

At this point I could just hold this patch and post the original series ignoring
the above issue at first and discuss around that code if this is needed at all
(so to have more context to discuss this), or simplify the patch as above if you
think this fix is still worth.

Regards

Cristian

Re: [PATCH] regulator: core: add of_match_full_name boolean flag

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

On Thu, Aug 20, 2020 at 05:38:44PM +0100, Cristian Marussi wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> On Wed, Aug 19, 2020 at 07:22:45PM +0100, Mark Brown wrote:
> > On Wed, Aug 19, 2020 at 03:04:48PM +0100, Cristian Marussi wrote:

> > > In this case the above matching mechanism based on the simple (common) name
> > > will fail and the only viable alternative would be to properly define the
> > > deprecrated 'regulator-compatible' property equal to the full name
> > > <common-name>@<unit>.

> > This seems like a massive jump.  You appear to be saying that the reg
> > property is unusable which doesn't seem right to me?

> The 'issue' I observed while working on another series was that with the
> following example DT:

...

> the regulator framework standard initialization routines were able to match univocally the
> first two regulators above (and parse autonomously the constraints without me explicitly
> calling of_get_regulator_init_data() as in a previous version of the series), but got fooled

My point is that your jump to "this is the only possible approach" seems
to suggest we can't involve the reg property in the matching which like
I say doesn't seem right.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]