| Message ID | fc9b3afaf8826fd437ba91397eb7fa231db2c05c.1700449792.git.zhoubinbin@loongson.cn (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | dt-bindings: interrupt-controller: Fix some loongson,liointc warnings | expand |
On Mon, Nov 20 2023 at 17:06, Binbin Zhou wrote: $Subject: s/parse/parsing/ > In keeping with naming standards, 'loongson,parent_int_map' is renamed > to 'loongson,parent-int-map'. But for the driver, we need to make sure > that both forms can be parsed. Please keep changelogs in neutral or imperative tone: For backwards compatibility it is required to parse the original string too. Makes it entirely clear what this is about without 'we'. See also: https://www.kernel.org/doc/html/latest/process/submitting-patches.rst > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> > Acked-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn> > --- > drivers/irqchip/irq-loongson-liointc.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-loongson-liointc.c b/drivers/irqchip/irq-loongson-liointc.c > index e4b33aed1c97..add2e0a955b8 100644 > --- a/drivers/irqchip/irq-loongson-liointc.c > +++ b/drivers/irqchip/irq-loongson-liointc.c > @@ -330,6 +330,7 @@ static int __init liointc_of_init(struct device_node *node, > bool have_parent = FALSE; > int sz, i, index, revision, err = 0; > struct resource res; > + const char *prop_name = "loongson,parent-int-map"; Please don't glue variables randomly into the declaration section: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations > if (!of_device_is_compatible(node, "loongson,liointc-2.0")) { > index = 0; > @@ -350,8 +351,12 @@ static int __init liointc_of_init(struct device_node *node, > if (!have_parent) > return -ENODEV; > > + if (!of_find_property(node, prop_name, &i)) > + /* Fallback to 'loongson,parent_int_map'. */ > + prop_name = "loongson,parent_int_map"; This lacks curly brackets: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#bracket-rules > sz = of_property_read_variable_u32_array(node, > - "loongson,parent_int_map", > + prop_name, > &parent_int_map[0], > LIOINTC_NUM_PARENT, > LIOINTC_NUM_PARENT); Thanks, tglx
On Fri, Dec 8, 2023 at 10:20 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > On Mon, Nov 20 2023 at 17:06, Binbin Zhou wrote: > > $Subject: s/parse/parsing/ > > > In keeping with naming standards, 'loongson,parent_int_map' is renamed > > to 'loongson,parent-int-map'. But for the driver, we need to make sure > > that both forms can be parsed. > > Please keep changelogs in neutral or imperative tone: > > For backwards compatibility it is required to parse the original > string too. > > Makes it entirely clear what this is about without 'we'. See also: > > https://www.kernel.org/doc/html/latest/process/submitting-patches.rst > > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> > > Acked-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn> > > --- > > drivers/irqchip/irq-loongson-liointc.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/irqchip/irq-loongson-liointc.c b/drivers/irqchip/irq-loongson-liointc.c > > index e4b33aed1c97..add2e0a955b8 100644 > > --- a/drivers/irqchip/irq-loongson-liointc.c > > +++ b/drivers/irqchip/irq-loongson-liointc.c > > @@ -330,6 +330,7 @@ static int __init liointc_of_init(struct device_node *node, > > bool have_parent = FALSE; > > int sz, i, index, revision, err = 0; > > struct resource res; > > + const char *prop_name = "loongson,parent-int-map"; > > Please don't glue variables randomly into the declaration section: > > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations > > > if (!of_device_is_compatible(node, "loongson,liointc-2.0")) { > > index = 0; > > @@ -350,8 +351,12 @@ static int __init liointc_of_init(struct device_node *node, > > if (!have_parent) > > return -ENODEV; > > > > + if (!of_find_property(node, prop_name, &i)) > > + /* Fallback to 'loongson,parent_int_map'. */ > > + prop_name = "loongson,parent_int_map"; > > This lacks curly brackets: > > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#bracket-rules Hi Thomas: Thanks for the detailed review. Rob suggested in the V5 patchset to remove the 'loongson,parent-int-map' renaming operation as an ABI breakage[1]. I had tried to explain that the driver would be compatible with the parsing of both naming styles[2], but unfortunately did not get a response from Rob. As a result, I removed the renaming-related patches in the V6 patchset, including this one[3]. However, how the 'loongson,parent-int-map' renaming operation is finally going to be handled needs to be decided together, and if it remains needed, I will fix the above issue and submit it as part of the V7 patchset. [1]: https://lore.kernel.org/all/20231127182836.GA2150516-robh@kernel.org/ [2]: https://lore.kernel.org/all/CAMpQs4LSTV6PgZSuyQx2Nq+87OHxSa=-Wz5nbhFVsmmvHubQFQ@mail.gmail.com/ [3]: https://lore.kernel.org/all/cover.1701933946.git.zhoubinbin@loongson.cn/ Thanks. Binbin > > > sz = of_property_read_variable_u32_array(node, > > - "loongson,parent_int_map", > > + prop_name, > > &parent_int_map[0], > > LIOINTC_NUM_PARENT, > > LIOINTC_NUM_PARENT); > > Thanks, > > tglx
diff --git a/drivers/irqchip/irq-loongson-liointc.c b/drivers/irqchip/irq-loongson-liointc.c index e4b33aed1c97..add2e0a955b8 100644 --- a/drivers/irqchip/irq-loongson-liointc.c +++ b/drivers/irqchip/irq-loongson-liointc.c @@ -330,6 +330,7 @@ static int __init liointc_of_init(struct device_node *node, bool have_parent = FALSE; int sz, i, index, revision, err = 0; struct resource res; + const char *prop_name = "loongson,parent-int-map"; if (!of_device_is_compatible(node, "loongson,liointc-2.0")) { index = 0; @@ -350,8 +351,12 @@ static int __init liointc_of_init(struct device_node *node, if (!have_parent) return -ENODEV; + if (!of_find_property(node, prop_name, &i)) + /* Fallback to 'loongson,parent_int_map'. */ + prop_name = "loongson,parent_int_map"; + sz = of_property_read_variable_u32_array(node, - "loongson,parent_int_map", + prop_name, &parent_int_map[0], LIOINTC_NUM_PARENT, LIOINTC_NUM_PARENT);