| Message ID | 20230802150018.327079-3-apatel@ventanamicro.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Linux RISC-V AIA Support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
On Wed, Aug 02, 2023 at 08:30:05PM +0530, Anup Patel wrote: > This allows fw_devlink to create device links between consumers of > a MSI and the supplier of the MSI. > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > --- > drivers/of/property.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index ddc75cd50825..bc20535deed7 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1325,6 +1325,37 @@ 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_msi_parent(struct device_node *np, > + const char *prop_name, int index) > +{ > + struct of_phandle_args sup_args; > + struct device_node *msi_np; > + > + if (IS_ENABLED(CONFIG_SPARC)) > + return NULL; > + > + if (strcmp(prop_name, "msi-parent")) > + return NULL; > + > + msi_np = of_parse_phandle(np, prop_name, 0); > + if (msi_np) { > + if (!of_property_read_bool(msi_np, "#msi-cells")) { Use of_property_present() to check presence. However, this check is wrong. #msi-cells is optional and assumed to be 0 if not present. There's another flavor of of_parse_phandle_with_args() that allows specifying a default cell count, so I think you can get rid of all this checking. > + if (index) { > + of_node_put(msi_np); > + return NULL; > + } > + return msi_np; > + } > + of_node_put(msi_np); > + } > + > + if (of_parse_phandle_with_args(np, prop_name, "#msi-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, }, > @@ -1359,6 +1390,7 @@ static const struct supplier_bindings of_supplier_bindings[] = { > { .parse_prop = parse_regulators, }, > { .parse_prop = parse_gpio, }, > { .parse_prop = parse_gpios, }, > + { .parse_prop = parse_msi_parent, }, > {} > }; > > -- > 2.34.1 >
On Fri, Aug 11, 2023 at 12:39 PM Rob Herring <robh@kernel.org> wrote: > > On Wed, Aug 02, 2023 at 08:30:05PM +0530, Anup Patel wrote: > > This allows fw_devlink to create device links between consumers of > > a MSI and the supplier of the MSI. > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > --- > > drivers/of/property.c | 32 ++++++++++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index ddc75cd50825..bc20535deed7 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -1325,6 +1325,37 @@ 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_msi_parent(struct device_node *np, > > + const char *prop_name, int index) > > +{ > > + struct of_phandle_args sup_args; > > + struct device_node *msi_np; > > + > > + if (IS_ENABLED(CONFIG_SPARC)) > > + return NULL; > > + > > + if (strcmp(prop_name, "msi-parent")) > > + return NULL; > > + > > + msi_np = of_parse_phandle(np, prop_name, 0); > > + if (msi_np) { > > + if (!of_property_read_bool(msi_np, "#msi-cells")) { > > Use of_property_present() to check presence. > > However, this check is wrong. #msi-cells is optional and assumed to be 0 > if not present. There's another flavor of of_parse_phandle_with_args() > that allows specifying a default cell count, so I think you can get rid > of all this checking. Rob, The existing DEFINE_SIMPLE_PROP() already does this btw. See commit ff24fed10ba41. Anup just ignores review comments. Anup, Saw your reply here: https://lore.kernel.org/lkml/CAK9=C2W+3pOWL36ZPme1LpHz5PEBVdKWv+kR7DYKmRyw1AymMA@mail.gmail.com/ Did you even try the macro? Why are you so hell bent on wasting our time by ignoring code review comments and sending the same useless patch? -Saravana > > > + if (index) { > > + of_node_put(msi_np); > > + return NULL; > > + } > > + return msi_np; > > + } > > + of_node_put(msi_np); > > + } > > + > > + if (of_parse_phandle_with_args(np, prop_name, "#msi-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, }, > > @@ -1359,6 +1390,7 @@ static const struct supplier_bindings of_supplier_bindings[] = { > > { .parse_prop = parse_regulators, }, > > { .parse_prop = parse_gpio, }, > > { .parse_prop = parse_gpios, }, > > + { .parse_prop = parse_msi_parent, }, > > {} > > }; > > > > -- > > 2.34.1 > >
On Sat, Aug 12, 2023 at 1:09 AM Rob Herring <robh@kernel.org> wrote: > > On Wed, Aug 02, 2023 at 08:30:05PM +0530, Anup Patel wrote: > > This allows fw_devlink to create device links between consumers of > > a MSI and the supplier of the MSI. > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > --- > > drivers/of/property.c | 32 ++++++++++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index ddc75cd50825..bc20535deed7 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -1325,6 +1325,37 @@ 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_msi_parent(struct device_node *np, > > + const char *prop_name, int index) > > +{ > > + struct of_phandle_args sup_args; > > + struct device_node *msi_np; > > + > > + if (IS_ENABLED(CONFIG_SPARC)) > > + return NULL; > > + > > + if (strcmp(prop_name, "msi-parent")) > > + return NULL; > > + > > + msi_np = of_parse_phandle(np, prop_name, 0); > > + if (msi_np) { > > + if (!of_property_read_bool(msi_np, "#msi-cells")) { > > Use of_property_present() to check presence. > > However, this check is wrong. #msi-cells is optional and assumed to be 0 > if not present. There's another flavor of of_parse_phandle_with_args() > that allows specifying a default cell count, so I think you can get rid > of all this checking. Okay, I will update in the next revision. Thanks, Anup > > > + if (index) { > > + of_node_put(msi_np); > > + return NULL; > > + } > > + return msi_np; > > + } > > + of_node_put(msi_np); > > + } > > + > > + if (of_parse_phandle_with_args(np, prop_name, "#msi-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, }, > > @@ -1359,6 +1390,7 @@ static const struct supplier_bindings of_supplier_bindings[] = { > > { .parse_prop = parse_regulators, }, > > { .parse_prop = parse_gpio, }, > > { .parse_prop = parse_gpios, }, > > + { .parse_prop = parse_msi_parent, }, > > {} > > }; > > > > -- > > 2.34.1 > >
diff --git a/drivers/of/property.c b/drivers/of/property.c index ddc75cd50825..bc20535deed7 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1325,6 +1325,37 @@ 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_msi_parent(struct device_node *np, + const char *prop_name, int index) +{ + struct of_phandle_args sup_args; + struct device_node *msi_np; + + if (IS_ENABLED(CONFIG_SPARC)) + return NULL; + + if (strcmp(prop_name, "msi-parent")) + return NULL; + + msi_np = of_parse_phandle(np, prop_name, 0); + if (msi_np) { + if (!of_property_read_bool(msi_np, "#msi-cells")) { + if (index) { + of_node_put(msi_np); + return NULL; + } + return msi_np; + } + of_node_put(msi_np); + } + + if (of_parse_phandle_with_args(np, prop_name, "#msi-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, }, @@ -1359,6 +1390,7 @@ static const struct supplier_bindings of_supplier_bindings[] = { { .parse_prop = parse_regulators, }, { .parse_prop = parse_gpio, }, { .parse_prop = parse_gpios, }, + { .parse_prop = parse_msi_parent, }, {} };
This allows fw_devlink to create device links between consumers of a MSI and the supplier of the MSI. Signed-off-by: Anup Patel <apatel@ventanamicro.com> --- drivers/of/property.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)