diff mbox series

of: property: special #nvmem-cell-cells handling

Message ID	20221118214036.1269005-1-michael@walle.cc
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Michael Walle <michael@walle.cc> To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, Miquel Raynal <miquel.raynal@bootlin.com>, Rob Herring <robh+dt@kernel.org>, Frank Rowand <frowand.list@gmail.com> Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, Michael Walle <michael@walle.cc> Subject: [PATCH] of: property: special #nvmem-cell-cells handling Date: Fri, 18 Nov 2022 22:40:36 +0100 Message-Id: <20221118214036.1269005-1-michael@walle.cc> In-Reply-To: <8b976cf546bad3aa159a6f05cd3c15d1@walle.cc> References: <8b976cf546bad3aa159a6f05cd3c15d1@walle.cc> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	of: property: special #nvmem-cell-cells handling \| of: property: special #nvmem-cell-cells handling

Commit Message

Michael Walle Nov. 18, 2022, 9:40 p.m. UTC

  Since recently, there is a new #nvmem-cell-cells. To be backwards
compatible this is optional. Therefore, we need special handling and
cannot use DEFINE_SIMPLE_PROP() anymore.

Signed-off-by: Michael Walle <michael@walle.cc>
---
This patch will be part of the following series:
https://lore.kernel.org/linux-arm-kernel/20221118185118.1190044-1-michael@walle.cc/

 drivers/of/property.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Rob Herring Nov. 18, 2022, 9:52 p.m. UTC | #1

On Fri, Nov 18, 2022 at 3:40 PM Michael Walle <michael@walle.cc> wrote:
>
> Since recently, there is a new #nvmem-cell-cells. To be backwards
> compatible this is optional. Therefore, we need special handling and
> cannot use DEFINE_SIMPLE_PROP() anymore.
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> This patch will be part of the following series:
> https://lore.kernel.org/linux-arm-kernel/20221118185118.1190044-1-michael@walle.cc/
>
>  drivers/of/property.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 967f79b59016..93c0ea662336 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1305,7 +1305,6 @@ DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
>  DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
>  DEFINE_SIMPLE_PROP(hwlocks, "hwlocks", "#hwlock-cells")
>  DEFINE_SIMPLE_PROP(extcon, "extcon", NULL)
> -DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", NULL)
>  DEFINE_SIMPLE_PROP(phys, "phys", "#phy-cells")
>  DEFINE_SIMPLE_PROP(wakeup_parent, "wakeup-parent", NULL)
>  DEFINE_SIMPLE_PROP(pinctrl0, "pinctrl-0", NULL)
> @@ -1381,6 +1380,22 @@ static struct device_node *parse_interrupts(struct device_node *np,
>         return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
>  }
>
> +static struct device_node *parse_nvmem_cells(struct device_node *np,
> +                                            const char *prop_name, int index)
> +{
> +       struct of_phandle_args sup_args;
> +
> +       if (strcmp(prop_name, "nvmem-cells"))
> +               return NULL;
> +
> +       if (of_parse_phandle_with_optional_args(np, prop_name,
> +                                               "#nvmem-cell-cells", index,
> +                                               &sup_args))
> +               return NULL;
> +
> +       return sup_args.np;
> +}

There's a couple of other cases like that (MSI IIRC), so can we
generalize this to work in more than 1 case?

Rob

Michael Walle Nov. 18, 2022, 10:03 p.m. UTC | #2

Am 2022-11-18 22:52, schrieb Rob Herring:
> On Fri, Nov 18, 2022 at 3:40 PM Michael Walle <michael@walle.cc> wrote:
>> 
>> Since recently, there is a new #nvmem-cell-cells. To be backwards
>> compatible this is optional. Therefore, we need special handling and
>> cannot use DEFINE_SIMPLE_PROP() anymore.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> This patch will be part of the following series:
>> https://lore.kernel.org/linux-arm-kernel/20221118185118.1190044-1-michael@walle.cc/
>> 
>>  drivers/of/property.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/of/property.c b/drivers/of/property.c
>> index 967f79b59016..93c0ea662336 100644
>> --- a/drivers/of/property.c
>> +++ b/drivers/of/property.c
>> @@ -1305,7 +1305,6 @@ DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
>>  DEFINE_SIMPLE_PROP(power_domains, "power-domains", 
>> "#power-domain-cells")
>>  DEFINE_SIMPLE_PROP(hwlocks, "hwlocks", "#hwlock-cells")
>>  DEFINE_SIMPLE_PROP(extcon, "extcon", NULL)
>> -DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", NULL)
>>  DEFINE_SIMPLE_PROP(phys, "phys", "#phy-cells")
>>  DEFINE_SIMPLE_PROP(wakeup_parent, "wakeup-parent", NULL)
>>  DEFINE_SIMPLE_PROP(pinctrl0, "pinctrl-0", NULL)
>> @@ -1381,6 +1380,22 @@ static struct device_node 
>> *parse_interrupts(struct device_node *np,
>>         return of_irq_parse_one(np, index, &sup_args) ? NULL : 
>> sup_args.np;
>>  }
>> 
>> +static struct device_node *parse_nvmem_cells(struct device_node *np,
>> +                                            const char *prop_name, 
>> int index)
>> +{
>> +       struct of_phandle_args sup_args;
>> +
>> +       if (strcmp(prop_name, "nvmem-cells"))
>> +               return NULL;
>> +
>> +       if (of_parse_phandle_with_optional_args(np, prop_name,
>> +                                               "#nvmem-cell-cells", 
>> index,
>> +                                               &sup_args))
>> +               return NULL;
>> +
>> +       return sup_args.np;
>> +}
> 
> There's a couple of other cases like that (MSI IIRC), so can we
> generalize this to work in more than 1 case?

You mean addding a new DEFINE_SIMPLE_PROP_OPTIONAL_ARGS()?

-michael

Rob Herring Nov. 22, 2022, 11:44 p.m. UTC | #3

On Fri, Nov 18, 2022 at 4:03 PM Michael Walle <michael@walle.cc> wrote:
>
> Am 2022-11-18 22:52, schrieb Rob Herring:
> > On Fri, Nov 18, 2022 at 3:40 PM Michael Walle <michael@walle.cc> wrote:
> >>
> >> Since recently, there is a new #nvmem-cell-cells. To be backwards
> >> compatible this is optional. Therefore, we need special handling and
> >> cannot use DEFINE_SIMPLE_PROP() anymore.
> >>
> >> Signed-off-by: Michael Walle <michael@walle.cc>
> >> ---
> >> This patch will be part of the following series:
> >> https://lore.kernel.org/linux-arm-kernel/20221118185118.1190044-1-michael@walle.cc/
> >>
> >>  drivers/of/property.c | 17 ++++++++++++++++-
> >>  1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/of/property.c b/drivers/of/property.c
> >> index 967f79b59016..93c0ea662336 100644
> >> --- a/drivers/of/property.c
> >> +++ b/drivers/of/property.c
> >> @@ -1305,7 +1305,6 @@ DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
> >>  DEFINE_SIMPLE_PROP(power_domains, "power-domains",
> >> "#power-domain-cells")
> >>  DEFINE_SIMPLE_PROP(hwlocks, "hwlocks", "#hwlock-cells")
> >>  DEFINE_SIMPLE_PROP(extcon, "extcon", NULL)
> >> -DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", NULL)
> >>  DEFINE_SIMPLE_PROP(phys, "phys", "#phy-cells")
> >>  DEFINE_SIMPLE_PROP(wakeup_parent, "wakeup-parent", NULL)
> >>  DEFINE_SIMPLE_PROP(pinctrl0, "pinctrl-0", NULL)
> >> @@ -1381,6 +1380,22 @@ static struct device_node
> >> *parse_interrupts(struct device_node *np,
> >>         return of_irq_parse_one(np, index, &sup_args) ? NULL :
> >> sup_args.np;
> >>  }
> >>
> >> +static struct device_node *parse_nvmem_cells(struct device_node *np,
> >> +                                            const char *prop_name,
> >> int index)
> >> +{
> >> +       struct of_phandle_args sup_args;
> >> +
> >> +       if (strcmp(prop_name, "nvmem-cells"))
> >> +               return NULL;
> >> +
> >> +       if (of_parse_phandle_with_optional_args(np, prop_name,
> >> +                                               "#nvmem-cell-cells",
> >> index,
> >> +                                               &sup_args))
> >> +               return NULL;
> >> +
> >> +       return sup_args.np;
> >> +}
> >
> > There's a couple of other cases like that (MSI IIRC), so can we
> > generalize this to work in more than 1 case?
>
> You mean addding a new DEFINE_SIMPLE_PROP_OPTIONAL_ARGS()?

Actually, I think you can just do something like the below. I don't
think we need to separately handle an optional #.*-cells and a
required one. It's really just validation which we do already both
with the tools and when the subsystems parse these bindings. Of
course, if we need to handle cases other than 0 default cells, we'll
have to restructure the define some to pass the default cells.

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 967f79b59016..198f56633eb0 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1254,7 +1254,7 @@ static struct device_node
*parse_suffix_prop_cells(struct device_node *np,
        if (strcmp_suffix(prop_name, suffix))
                return NULL;

-       if (of_parse_phandle_with_args(np, prop_name, cells_name, index,
+       if (__of_parse_phandle_with_args(np, prop_name, cells_name, 0, index,
                                       &sup_args))
                return NULL;

Miquel Raynal Nov. 23, 2022, 12:24 p.m. UTC | #4

Hi Rob, Michael,

robh+dt@kernel.org wrote on Tue, 22 Nov 2022 17:44:26 -0600:

> On Fri, Nov 18, 2022 at 4:03 PM Michael Walle <michael@walle.cc> wrote:
> >
> > Am 2022-11-18 22:52, schrieb Rob Herring:  
> > > On Fri, Nov 18, 2022 at 3:40 PM Michael Walle <michael@walle.cc> wrote:  
> > >>
> > >> Since recently, there is a new #nvmem-cell-cells. To be backwards
> > >> compatible this is optional. Therefore, we need special handling and
> > >> cannot use DEFINE_SIMPLE_PROP() anymore.
> > >>
> > >> Signed-off-by: Michael Walle <michael@walle.cc>
> > >> ---
> > >> This patch will be part of the following series:
> > >> https://lore.kernel.org/linux-arm-kernel/20221118185118.1190044-1-michael@walle.cc/
> > >>
> > >>  drivers/of/property.c | 17 ++++++++++++++++-
> > >>  1 file changed, 16 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/of/property.c b/drivers/of/property.c
> > >> index 967f79b59016..93c0ea662336 100644
> > >> --- a/drivers/of/property.c
> > >> +++ b/drivers/of/property.c
> > >> @@ -1305,7 +1305,6 @@ DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
> > >>  DEFINE_SIMPLE_PROP(power_domains, "power-domains",
> > >> "#power-domain-cells")
> > >>  DEFINE_SIMPLE_PROP(hwlocks, "hwlocks", "#hwlock-cells")
> > >>  DEFINE_SIMPLE_PROP(extcon, "extcon", NULL)
> > >> -DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", NULL)
> > >>  DEFINE_SIMPLE_PROP(phys, "phys", "#phy-cells")
> > >>  DEFINE_SIMPLE_PROP(wakeup_parent, "wakeup-parent", NULL)
> > >>  DEFINE_SIMPLE_PROP(pinctrl0, "pinctrl-0", NULL)
> > >> @@ -1381,6 +1380,22 @@ static struct device_node
> > >> *parse_interrupts(struct device_node *np,
> > >>         return of_irq_parse_one(np, index, &sup_args) ? NULL :
> > >> sup_args.np;
> > >>  }
> > >>
> > >> +static struct device_node *parse_nvmem_cells(struct device_node *np,
> > >> +                                            const char *prop_name,
> > >> int index)
> > >> +{
> > >> +       struct of_phandle_args sup_args;
> > >> +
> > >> +       if (strcmp(prop_name, "nvmem-cells"))
> > >> +               return NULL;
> > >> +
> > >> +       if (of_parse_phandle_with_optional_args(np, prop_name,
> > >> +                                               "#nvmem-cell-cells",
> > >> index,
> > >> +                                               &sup_args))
> > >> +               return NULL;
> > >> +
> > >> +       return sup_args.np;
> > >> +}  
> > >
> > > There's a couple of other cases like that (MSI IIRC), so can we
> > > generalize this to work in more than 1 case?  
> >
> > You mean addding a new DEFINE_SIMPLE_PROP_OPTIONAL_ARGS()?  
> 
> Actually, I think you can just do something like the below. I don't
> think we need to separately handle an optional #.*-cells and a
> required one. It's really just validation which we do already both
> with the tools and when the subsystems parse these bindings. Of
> course, if we need to handle cases other than 0 default cells, we'll
> have to restructure the define some to pass the default cells.
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 967f79b59016..198f56633eb0 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1254,7 +1254,7 @@ static struct device_node
> *parse_suffix_prop_cells(struct device_node *np,
>         if (strcmp_suffix(prop_name, suffix))
>                 return NULL;
> 
> -       if (of_parse_phandle_with_args(np, prop_name, cells_name, index,
> +       if (__of_parse_phandle_with_args(np, prop_name, cells_name, 0, index,
>                                        &sup_args))
>                 return NULL;

Excellent, this small change aside with

	-DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", NULL)
	+DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", "#nvmem-cell-cells")

look much simpler and work perfectly.

Tested-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

diff mbox series

Patch

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 967f79b59016..93c0ea662336 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1305,7 +1305,6 @@  DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
 DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
 DEFINE_SIMPLE_PROP(hwlocks, "hwlocks", "#hwlock-cells")
 DEFINE_SIMPLE_PROP(extcon, "extcon", NULL)
-DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", NULL)
 DEFINE_SIMPLE_PROP(phys, "phys", "#phy-cells")
 DEFINE_SIMPLE_PROP(wakeup_parent, "wakeup-parent", NULL)
 DEFINE_SIMPLE_PROP(pinctrl0, "pinctrl-0", NULL)
@@ -1381,6 +1380,22 @@  static struct device_node *parse_interrupts(struct device_node *np,
 	return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }
 
+static struct device_node *parse_nvmem_cells(struct device_node *np,
+					     const char *prop_name, int index)
+{
+	struct of_phandle_args sup_args;
+
+	if (strcmp(prop_name, "nvmem-cells"))
+		return NULL;
+
+	if (of_parse_phandle_with_optional_args(np, prop_name,
+						"#nvmem-cell-cells", index,
+						&sup_args))
+		return NULL;
+
+	return sup_args.np;
+}
+
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
 	{ .parse_prop = parse_interconnects, },