From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [PATCH v3 09/17] irqchip/irq-mvebu-icu: support ICU subnodes Date: Wed, 4 Jul 2018 13:43:58 +0100 Message-ID: References: <20180622151432.1566-1-miquel.raynal@bootlin.com> <20180622151432.1566-10-miquel.raynal@bootlin.com> <61ff2e4f-55b6-cf70-5f04-32cd197c35f8@arm.com> <20180704110917.65557d0a@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20180704110917.65557d0a@xps13> Content-Language: en-GB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Miquel Raynal Cc: Mark Rutland , Andrew Lunn , Jason Cooper , devicetree@vger.kernel.org, Antoine Tenart , Catalin Marinas , Gregory Clement , Haim Boot , Will Deacon , Maxime Chevallier , Nadav Haklai , Rob Herring , Thomas Petazzoni , Thomas Gleixner , Hanna Hawa , linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth List-Id: devicetree@vger.kernel.org On 04/07/18 10:09, Miquel Raynal wrote: > Hi Marc, > > Marc Zyngier wrote on Thu, 28 Jun 2018 13:45:09 > +0100: > >> On 22/06/18 16:14, Miquel Raynal wrote: >>> The ICU can handle several type of interrupt, each of them being handled >>> differently on AP side. On CP side, the ICU should be able to make the >>> distinction between each interrupt group by pointing to the right parent. >>> >>> This is done through the introduction of new bindings, presenting the ICU >>> node as the parent of multiple ICU sub-nodes, each of them being an >>> interrupt type with a different interrupt parent. ICU interrupt 'clients' >>> now directly point to the right sub-node, avoiding the need for the extra >>> ICU_GRP_* parameter. >>> >>> ICU subnodes are probed automatically with devm_platform_populate(). If >>> the node as no child, the probe function for NSRs will still be called >>> 'manually' in order to preserve backward compatibility with DT using the >>> old binding. >>> >>> Signed-off-by: Miquel Raynal >>> --- >>> drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++------- >>> 1 file changed, 74 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c >>> index 24d45186eb6b..f7c2ede9c222 100644 >>> --- a/drivers/irqchip/irq-mvebu-icu.c >>> +++ b/drivers/irqchip/irq-mvebu-icu.c >>> @@ -43,6 +43,7 @@ struct mvebu_icu { >>> struct regmap *regmap; >>> struct device *dev; >>> atomic_t initialized; >>> + bool legacy_bindings; >>> }; >>> >>> struct mvebu_icu_irq_data { >>> @@ -51,6 +52,30 @@ struct mvebu_icu_irq_data { >>> unsigned int type; >>> }; >>> >>> +static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev) >>> +{ >>> + struct mvebu_icu *icu; >>> + >>> + /* >>> + * Device data being populated means we should be using legacy bindings. >>> + * Using the _parent_ device data means we should be using new bindings. >>> + */ >>> + icu = dev_get_drvdata(&pdev->dev); >>> + if (icu) { >>> + if (!icu->legacy_bindings) >>> + return ERR_PTR(-EINVAL); >>> + } else { >>> + icu = dev_get_drvdata(pdev->dev.parent); >>> + if (!icu) >>> + return ERR_PTR(-ENODEV); >>> + >>> + if (icu->legacy_bindings) >>> + return ERR_PTR(-EINVAL); >>> + } >> >> Doesn't this make legacy_bindings completely redundant? Either the >> pointer is !NULL in the device, and this is using a legacy binging, or >> it is stored in the parent, and this is a new binding. You could even >> have a helper for that: >> >> static bool is_legacy(struct platform_device *pdev) >> { >> return !dev_get_drvdata(&pdev->dev); >> } >> >> The driver really doesn't need to be defending against itself, if >> anything, and it would save you quite a bit of error handling in the >> callers of this function. > > I simplified the sanity checks but I had to keep an icu->is_legacy > boolean because the above function would not have worked, for instance, > in the *_translate() hook. As this hook does not receive a struct > device * (or platform_device) as parameter, I tried to use icu->dev > instead. This cannot work as it always points to the device having the > driver data attached. You could still have the pdev as part of the domain host_data, right? Isn't that just a matter of having a pointer to the pdev as part of the icu data structure? Thanks, M. -- Jazz is not dead. It just smells funny... From mboxrd@z Thu Jan 1 00:00:00 1970 From: marc.zyngier@arm.com (Marc Zyngier) Date: Wed, 4 Jul 2018 13:43:58 +0100 Subject: [PATCH v3 09/17] irqchip/irq-mvebu-icu: support ICU subnodes In-Reply-To: <20180704110917.65557d0a@xps13> References: <20180622151432.1566-1-miquel.raynal@bootlin.com> <20180622151432.1566-10-miquel.raynal@bootlin.com> <61ff2e4f-55b6-cf70-5f04-32cd197c35f8@arm.com> <20180704110917.65557d0a@xps13> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 04/07/18 10:09, Miquel Raynal wrote: > Hi Marc, > > Marc Zyngier wrote on Thu, 28 Jun 2018 13:45:09 > +0100: > >> On 22/06/18 16:14, Miquel Raynal wrote: >>> The ICU can handle several type of interrupt, each of them being handled >>> differently on AP side. On CP side, the ICU should be able to make the >>> distinction between each interrupt group by pointing to the right parent. >>> >>> This is done through the introduction of new bindings, presenting the ICU >>> node as the parent of multiple ICU sub-nodes, each of them being an >>> interrupt type with a different interrupt parent. ICU interrupt 'clients' >>> now directly point to the right sub-node, avoiding the need for the extra >>> ICU_GRP_* parameter. >>> >>> ICU subnodes are probed automatically with devm_platform_populate(). If >>> the node as no child, the probe function for NSRs will still be called >>> 'manually' in order to preserve backward compatibility with DT using the >>> old binding. >>> >>> Signed-off-by: Miquel Raynal >>> --- >>> drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++------- >>> 1 file changed, 74 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c >>> index 24d45186eb6b..f7c2ede9c222 100644 >>> --- a/drivers/irqchip/irq-mvebu-icu.c >>> +++ b/drivers/irqchip/irq-mvebu-icu.c >>> @@ -43,6 +43,7 @@ struct mvebu_icu { >>> struct regmap *regmap; >>> struct device *dev; >>> atomic_t initialized; >>> + bool legacy_bindings; >>> }; >>> >>> struct mvebu_icu_irq_data { >>> @@ -51,6 +52,30 @@ struct mvebu_icu_irq_data { >>> unsigned int type; >>> }; >>> >>> +static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev) >>> +{ >>> + struct mvebu_icu *icu; >>> + >>> + /* >>> + * Device data being populated means we should be using legacy bindings. >>> + * Using the _parent_ device data means we should be using new bindings. >>> + */ >>> + icu = dev_get_drvdata(&pdev->dev); >>> + if (icu) { >>> + if (!icu->legacy_bindings) >>> + return ERR_PTR(-EINVAL); >>> + } else { >>> + icu = dev_get_drvdata(pdev->dev.parent); >>> + if (!icu) >>> + return ERR_PTR(-ENODEV); >>> + >>> + if (icu->legacy_bindings) >>> + return ERR_PTR(-EINVAL); >>> + } >> >> Doesn't this make legacy_bindings completely redundant? Either the >> pointer is !NULL in the device, and this is using a legacy binging, or >> it is stored in the parent, and this is a new binding. You could even >> have a helper for that: >> >> static bool is_legacy(struct platform_device *pdev) >> { >> return !dev_get_drvdata(&pdev->dev); >> } >> >> The driver really doesn't need to be defending against itself, if >> anything, and it would save you quite a bit of error handling in the >> callers of this function. > > I simplified the sanity checks but I had to keep an icu->is_legacy > boolean because the above function would not have worked, for instance, > in the *_translate() hook. As this hook does not receive a struct > device * (or platform_device) as parameter, I tried to use icu->dev > instead. This cannot work as it always points to the device having the > driver data attached. You could still have the pdev as part of the domain host_data, right? Isn't that just a matter of having a pointer to the pdev as part of the icu data structure? Thanks, M. -- Jazz is not dead. It just smells funny...