| Message ID | 20201026080127.40499-1-biwen.li@oss.nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external interrupt | expand |
On 2020-10-26 08:01, Biwen Li wrote: > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > Add an new IRQ chip declaration for LS1043A and LS1088A > - compatible "fsl,ls1043a-extirq" for LS1043A, LS1046A > - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA Three things: - This commit message doesn't describe the bit_reverse change - Please add a cover letter - Sending the same series again after 4 days is not OK, specially when the initial one was during the merge window. Thanks, M. > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > Signed-off-by: Biwen Li <biwen.li@nxp.com> > --- > drivers/irqchip/irq-ls-extirq.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/irqchip/irq-ls-extirq.c > b/drivers/irqchip/irq-ls-extirq.c > index 4d1179fed77c..564e6de0bd8e 100644 > --- a/drivers/irqchip/irq-ls-extirq.c > +++ b/drivers/irqchip/irq-ls-extirq.c > @@ -1,4 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0 > +// Copyright 2019-2020 NXP > > #define pr_fmt(fmt) "irq-ls-extirq: " fmt > > @@ -183,6 +184,9 @@ ls_extirq_of_init(struct device_node *node, struct > device_node *parent) > priv->bit_reverse = (revcr != 0); > } > > + if (of_device_is_compatible(node, "fsl,ls1043a-extirq")) > + priv->bit_reverse = true; > + > domain = irq_domain_add_hierarchy(parent_domain, 0, priv->nirq, node, > &extirq_domain_ops, priv); > if (!domain) > @@ -195,3 +199,5 @@ ls_extirq_of_init(struct device_node *node, struct > device_node *parent) > } > > IRQCHIP_DECLARE(ls1021a_extirq, "fsl,ls1021a-extirq", > ls_extirq_of_init); > +IRQCHIP_DECLARE(ls1043a_extirq, "fsl,ls1043a-extirq", > ls_extirq_of_init); > +IRQCHIP_DECLARE(ls1088a_extirq, "fsl,ls1088a-extirq", > ls_extirq_of_init);
On 26/10/2020 09.44, Marc Zyngier wrote: > On 2020-10-26 08:01, Biwen Li wrote: >> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> >> >> Add an new IRQ chip declaration for LS1043A and LS1088A >> - compatible "fsl,ls1043a-extirq" for LS1043A, LS1046A >> - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA > > Three things: > - This commit message doesn't describe the bit_reverse change Yeah, please elaborate on that, as the RM for 1043 or 1046 doesn't mention anything about bit reversal for the scfg registers - they don't seem to have the utter nonsense that is SCFG_SCFGREVCR, but perhaps, instead of removing it, that has just become a hard-coded part of the IP. Also, IANAL etc., but >> +// Copyright 2019-2020 NXP really? Seems to be a bit of a stretch. At the very least, cc'ing the original author and only person to ever touch that file would have been appreciated. Rasmus
On 2020-10-26 09:06, Rasmus Villemoes wrote: > On 26/10/2020 09.44, Marc Zyngier wrote: >> On 2020-10-26 08:01, Biwen Li wrote: >>> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> >>> >>> Add an new IRQ chip declaration for LS1043A and LS1088A >>> - compatible "fsl,ls1043a-extirq" for LS1043A, LS1046A >>> - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA >> >> Three things: >> - This commit message doesn't describe the bit_reverse change > > Yeah, please elaborate on that, as the RM for 1043 or 1046 doesn't > mention anything about bit reversal for the scfg registers - they don't > seem to have the utter nonsense that is SCFG_SCFGREVCR, but perhaps, > instead of removing it, that has just become a hard-coded part of the > IP. > > Also, IANAL etc., but > >>> +// Copyright 2019-2020 NXP > > really? Seems to be a bit of a stretch. > > At the very least, cc'ing the original author and only person to ever > touch that file would have been appreciated. Huh. Well spotted. That's definitely not on. NXP people, please talk to your legal department. M.
> -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: Monday, October 26, 2020 4:23 AM > To: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Cc: Biwen Li (OSS) <biwen.li@oss.nxp.com>; shawnguo@kernel.org; > robh+dt@kernel.org; mark.rutland@arm.com; Leo Li <leoyang.li@nxp.com>; > Z.q. Hou <zhiqiang.hou@nxp.com>; tglx@linutronix.de; > jason@lakedaemon.net; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; Jiafei Pan <jiafei.pan@nxp.com>; Xiaobo Xie > <xiaobo.xie@nxp.com>; linux-arm-kernel@lists.infradead.org; Biwen Li > <biwen.li@nxp.com> > Subject: Re: [RESEND 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external > interrupt > > On 2020-10-26 09:06, Rasmus Villemoes wrote: > > On 26/10/2020 09.44, Marc Zyngier wrote: > >> On 2020-10-26 08:01, Biwen Li wrote: > >>> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > >>> > >>> Add an new IRQ chip declaration for LS1043A and LS1088A > >>> - compatible "fsl,ls1043a-extirq" for LS1043A, LS1046A > >>> - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA > >> > >> Three things: > >> - This commit message doesn't describe the bit_reverse change > > > > Yeah, please elaborate on that, as the RM for 1043 or 1046 doesn't > > mention anything about bit reversal for the scfg registers - they > > don't seem to have the utter nonsense that is SCFG_SCFGREVCR, but > > perhaps, instead of removing it, that has just become a hard-coded > > part of the IP. > > > > Also, IANAL etc., but > > > >>> +// Copyright 2019-2020 NXP > > > > really? Seems to be a bit of a stretch. > > > > At the very least, cc'ing the original author and only person to ever > > touch that file would have been appreciated. > > Huh. Well spotted. That's definitely not on. > NXP people, please talk to your legal department. We do have an internal policy to require developer adding/updating NXP copyright on non-trivial changes. I'm not sure if this change should be considered trivial, but adding copyright claim on a file without prior copyright claims could causing confusion like in this case. One potential solution is to add a more specific description on the NXP change together with the copyright claim. But maybe an easier solution is to add Rasmus your Copyright claim first if you are ok with it. Regards, Leo
On 2020-10-26 15:06, Leo Li wrote: >> -----Original Message----- >> From: Marc Zyngier <maz@kernel.org> >> Sent: Monday, October 26, 2020 4:23 AM >> To: Rasmus Villemoes <linux@rasmusvillemoes.dk> >> Cc: Biwen Li (OSS) <biwen.li@oss.nxp.com>; shawnguo@kernel.org; >> robh+dt@kernel.org; mark.rutland@arm.com; Leo Li <leoyang.li@nxp.com>; >> Z.q. Hou <zhiqiang.hou@nxp.com>; tglx@linutronix.de; >> jason@lakedaemon.net; devicetree@vger.kernel.org; linux- >> kernel@vger.kernel.org; Jiafei Pan <jiafei.pan@nxp.com>; Xiaobo Xie >> <xiaobo.xie@nxp.com>; linux-arm-kernel@lists.infradead.org; Biwen Li >> <biwen.li@nxp.com> >> Subject: Re: [RESEND 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A >> external >> interrupt >> >> On 2020-10-26 09:06, Rasmus Villemoes wrote: >> > On 26/10/2020 09.44, Marc Zyngier wrote: >> >> On 2020-10-26 08:01, Biwen Li wrote: >> >>> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> >> >>> >> >>> Add an new IRQ chip declaration for LS1043A and LS1088A >> >>> - compatible "fsl,ls1043a-extirq" for LS1043A, LS1046A >> >>> - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA >> >> >> >> Three things: >> >> - This commit message doesn't describe the bit_reverse change >> > >> > Yeah, please elaborate on that, as the RM for 1043 or 1046 doesn't >> > mention anything about bit reversal for the scfg registers - they >> > don't seem to have the utter nonsense that is SCFG_SCFGREVCR, but >> > perhaps, instead of removing it, that has just become a hard-coded >> > part of the IP. >> > >> > Also, IANAL etc., but >> > >> >>> +// Copyright 2019-2020 NXP >> > >> > really? Seems to be a bit of a stretch. >> > >> > At the very least, cc'ing the original author and only person to ever >> > touch that file would have been appreciated. >> >> Huh. Well spotted. That's definitely not on. >> NXP people, please talk to your legal department. > > We do have an internal policy to require developer adding/updating NXP > copyright on non-trivial changes. I'm not sure if this change should > be considered trivial, but adding copyright claim on a file without > prior copyright claims could causing confusion like in this case. The copyright exists implicitly, and doesn't require a copyright claim in the file itself. Please talk to your legal department. > One > potential solution is to add a more specific description on the NXP > change together with the copyright claim. But maybe an easier > solution is to add Rasmus your Copyright claim first if you are ok > with it. That's for Rasmus to decide whether he wants to add such a claim, given that it exists implicitly. Adding copyright claims for any odd change you make isn't acceptable either (your changes are already unambiguously identified in git). For the time being, I'm not taking any NXP patch carrying additional copyright update until this is clarified. M.
> > -----Original Message----- > > From: Marc Zyngier <maz@kernel.org> > > Sent: Monday, October 26, 2020 4:23 AM > > To: Rasmus Villemoes <linux@rasmusvillemoes.dk> > > Cc: Biwen Li (OSS) <biwen.li@oss.nxp.com>; shawnguo@kernel.org; > > robh+dt@kernel.org; mark.rutland@arm.com; Leo Li <leoyang.li@nxp.com>; > > Z.q. Hou <zhiqiang.hou@nxp.com>; tglx@linutronix.de; > > jason@lakedaemon.net; devicetree@vger.kernel.org; linux- > > kernel@vger.kernel.org; Jiafei Pan <jiafei.pan@nxp.com>; Xiaobo Xie > > <xiaobo.xie@nxp.com>; linux-arm-kernel@lists.infradead.org; Biwen Li > > <biwen.li@nxp.com> > > Subject: Re: [RESEND 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A > > external interrupt > > > > On 2020-10-26 09:06, Rasmus Villemoes wrote: > > > On 26/10/2020 09.44, Marc Zyngier wrote: > > >> On 2020-10-26 08:01, Biwen Li wrote: > > >>> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > >>> > > >>> Add an new IRQ chip declaration for LS1043A and LS1088A > > >>> - compatible "fsl,ls1043a-extirq" for LS1043A, LS1046A > > >>> - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA > > >> > > >> Three things: > > >> - This commit message doesn't describe the bit_reverse change > > > > > > Yeah, please elaborate on that, as the RM for 1043 or 1046 doesn't > > > mention anything about bit reversal for the scfg registers - they > > > don't seem to have the utter nonsense that is SCFG_SCFGREVCR, but > > > perhaps, instead of removing it, that has just become a hard-coded > > > part of the IP. > > > > > > Also, IANAL etc., but > > > > > >>> +// Copyright 2019-2020 NXP > > > > > > really? Seems to be a bit of a stretch. > > > > > > At the very least, cc'ing the original author and only person to > > > ever touch that file would have been appreciated. > > > > Huh. Well spotted. That's definitely not on. > > NXP people, please talk to your legal department. > > We do have an internal policy to require developer adding/updating NXP > copyright on non-trivial changes. I'm not sure if this change should be > considered trivial, but adding copyright claim on a file without prior copyright > claims could causing confusion like in this case. One potential solution is to > add a more specific description on the NXP change together with the copyright > claim. But maybe an easier solution is to add Rasmus your Copyright claim > first if you are ok with it. Yes, added a wrong Copyright. > > Regards, > Leo
> Subject: Re: [RESEND 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external > interrupt > > On 26/10/2020 09.44, Marc Zyngier wrote: > > On 2020-10-26 08:01, Biwen Li wrote: > >> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > >> > >> Add an new IRQ chip declaration for LS1043A and LS1088A > >> - compatible "fsl,ls1043a-extirq" for LS1043A, LS1046A > >> - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA > > > > Three things: > > - This commit message doesn't describe the bit_reverse change > > Yeah, please elaborate on that, as the RM for 1043 or 1046 doesn't mention > anything about bit reversal for the scfg registers - they don't seem to have the > utter nonsense that is SCFG_SCFGREVCR, but perhaps, instead of removing it, > that has just become a hard-coded part of the IP. Yeah, you are right, I will update it in v2. > > Also, IANAL etc., but > > >> +// Copyright 2019-2020 NXP > > really? Seems to be a bit of a stretch. > > At the very least, cc'ing the original author and only person to ever touch that > file would have been appreciated. Okay, it's my fault, I will update it, thanks. > > Rasmus
diff --git a/drivers/irqchip/irq-ls-extirq.c b/drivers/irqchip/irq-ls-extirq.c index 4d1179fed77c..564e6de0bd8e 100644 --- a/drivers/irqchip/irq-ls-extirq.c +++ b/drivers/irqchip/irq-ls-extirq.c @@ -1,4 +1,5 @@ // SPDX-License-Identifier: GPL-2.0 +// Copyright 2019-2020 NXP #define pr_fmt(fmt) "irq-ls-extirq: " fmt @@ -183,6 +184,9 @@ ls_extirq_of_init(struct device_node *node, struct device_node *parent) priv->bit_reverse = (revcr != 0); } + if (of_device_is_compatible(node, "fsl,ls1043a-extirq")) + priv->bit_reverse = true; + domain = irq_domain_add_hierarchy(parent_domain, 0, priv->nirq, node, &extirq_domain_ops, priv); if (!domain) @@ -195,3 +199,5 @@ ls_extirq_of_init(struct device_node *node, struct device_node *parent) } IRQCHIP_DECLARE(ls1021a_extirq, "fsl,ls1021a-extirq", ls_extirq_of_init); +IRQCHIP_DECLARE(ls1043a_extirq, "fsl,ls1043a-extirq", ls_extirq_of_init); +IRQCHIP_DECLARE(ls1088a_extirq, "fsl,ls1088a-extirq", ls_extirq_of_init);