From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 96228C433E4 for ; Fri, 17 Jul 2020 10:49:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 71CA920734 for ; Fri, 17 Jul 2020 10:49:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594982989; bh=m4E2Qlwl0PzfDZp6G5x1yaTQ8T2DRdddavKw5MRwr2Q=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=AaKV1dI1WkCfc9DbPiEUjsZm7+HunJxs+wcSGR97gvvGMqzj/WphETFIeo4y1RIno 584DO4uovajXFLbnzhdVLsAkOlNckApy6nD8yQnd2Fgxop1ngdmEiyoEy86KW3APEB /Ab8G26YlO5vhxQ2VmwXb/ED63TM/CufSzWn6wZc= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726422AbgGQKts (ORCPT ); Fri, 17 Jul 2020 06:49:48 -0400 Received: from mail.kernel.org ([198.145.29.99]:51808 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725912AbgGQKto (ORCPT ); Fri, 17 Jul 2020 06:49:44 -0400 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 8D44520734; Fri, 17 Jul 2020 10:49:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594982983; bh=m4E2Qlwl0PzfDZp6G5x1yaTQ8T2DRdddavKw5MRwr2Q=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=FikxkxVbC7w8+8HsO1EqgoFIw2oPjWZzC9Zyy7PuyS9KyJ3mGvNiIwUabspzX8kpW bDocfwoAb67WQk9J9YncgdrgB4ApFeU2dOWUScUj1heeTAOiDckHNnf6tTHOGWFWWp Oi6aIvZ8BRzTUWnX59nyuIpZI4CEZapa5UEvjDwk= Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jwNvy-00CbMK-3Z; Fri, 17 Jul 2020 11:49:42 +0100 Date: Fri, 17 Jul 2020 11:49:41 +0100 Message-ID: <87k0z2xvp6.wl-maz@kernel.org> From: Marc Zyngier To: Saravana Kannan Cc: Thomas Gleixner , Jason Cooper , Matthias Brugger , John Stultz , CC Hwang , Loda Chou , Hanks Chen , kernel-team@android.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org Subject: Re: [PATCH v2] irqchip: Add IRQCHIP_PLATFORM_DRIVER_BEGIN/END and IRQCHIP_MATCH helper macros In-Reply-To: <20200717024447.3128361-1-saravanak@google.com> References: <20200717024447.3128361-1-saravanak@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26.3 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: saravanak@google.com, tglx@linutronix.de, jason@lakedaemon.net, matthias.bgg@gmail.com, john.stultz@linaro.org, cc.hwang@mediatek.com, loda.chou@mediatek.com, hanks.chen@mediatek.com, kernel-team@android.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Saravana, Thanks for re-spinning this one. On Fri, 17 Jul 2020 03:44:47 +0100, Saravana Kannan wrote: > > Compiling an irqchip driver as a platform driver needs to bunch of > things to be done right: > - Making sure the parent domain is initialized first > - Making sure the device can't be unbound from sysfs > - Disallowing module unload if it's built as a module > - Finding the parent node > - Etc. > > Instead of trying to make sure all future irqchip platform drivers get > this right, provide boilerplate macros that take care of all of this. > > An example use would look something like this. Where acme_foo_init and > acme_bar_init are similar to what would be passed to IRQCHIP_DECLARE. > > IRQCHIP_PLATFORM_DRIVER_BEGIN I think there is some value in having the BEGIN statement containing the driver name, see below. > IRQCHIP_MATCH(foo, "acme,foo", acme_foo_init) > IRQCHIP_MATCH(bar, "acme,bar", acme_bar_init) > IRQCHIP_PLATFORM_DRIVER_END(acme_irq) > > Signed-off-by: Saravana Kannan > --- > drivers/irqchip/irqchip.c | 22 ++++++++++++++++++++++ > include/linux/irqchip.h | 23 +++++++++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c > index 2b35e68bea82..236ea793f01c 100644 > --- a/drivers/irqchip/irqchip.c > +++ b/drivers/irqchip/irqchip.c > @@ -10,8 +10,10 @@ > > #include > #include > +#include > #include > #include > +#include > > /* > * This special of_device_id is the sentinel at the end of the > @@ -29,3 +31,23 @@ void __init irqchip_init(void) > of_irq_init(__irqchip_of_table); > acpi_probe_device_table(irqchip); > } > + > +int platform_irqchip_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct device_node *par_np = of_irq_find_parent(np); > + of_irq_init_cb_t irq_init_cb = of_device_get_match_data(&pdev->dev); > + > + if (!irq_init_cb) > + return -EINVAL; > + > + if (par_np == np) > + par_np = NULL; > + > + /* If there's a parent irqchip, make sure it has been initialized. */ > + if (par_np && !irq_find_matching_host(np, DOMAIN_BUS_ANY)) There is no guarantee that the calling driver wants BUS_ANY as a token for its parent. It may work for now, if you only have dependencies to drivers that only expose a single domain, but that's not a general purpose check.. At least, please add a comment saying that the new driver may want to check that the irqdomain it depends on may not be available. > + return -EPROBE_DEFER; > + > + return irq_init_cb(np, par_np); > +} > +EXPORT_SYMBOL_GPL(platform_irqchip_probe); > diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h > index 950e4b2458f0..6d5eba7cbbb7 100644 > --- a/include/linux/irqchip.h > +++ b/include/linux/irqchip.h > @@ -13,6 +13,7 @@ > > #include > #include > +#include > > /* > * This macro must be used by the different irqchip drivers to declare > @@ -26,6 +27,28 @@ > */ > #define IRQCHIP_DECLARE(name, compat, fn) OF_DECLARE_2(irqchip, name, compat, fn) > > +extern int platform_irqchip_probe(struct platform_device *pdev); > + > +#define IRQCHIP_PLATFORM_DRIVER_BEGIN \ > +static const struct of_device_id __irqchip_match_table[] = { How about: #define IRQCHIP_PLATFORM_DRIVER_BEGIN(drv_name) \ static const struct of_device_id __irqchip_match_table_##drv_name = { which makes it easier to debug when you want to identify specific structures in an object (otherwise, they all have the same name...). it is also much more pleasing aesthetically ;-). > + > +#define IRQCHIP_MATCH(compat, fn) { .compatible = compat, .data = fn }, > + > +#define IRQCHIP_PLATFORM_DRIVER_END(drv_name) \ > + {}, \ > +}; \ > +MODULE_DEVICE_TABLE(of, __irqchip_match_table); \ > +static struct platform_driver drv_name##_driver = { \ const? > + .probe = platform_irqchip_probe, \ > + .driver = { \ > + .name = #drv_name, \ > + .owner = THIS_MODULE, \ > + .of_match_table = __irqchip_match_table,\ > + .suppress_bind_attrs = true, \ > + }, \ > +}; \ > +builtin_platform_driver(drv_name##_driver) > + > /* > * This macro must be used by the different irqchip drivers to declare > * the association between their version and their initialization function. > -- > 2.28.0.rc0.105.gf9edc3c819-goog > > Otherwise looks good. When you respin it, it would be good to also submit one user of this API by converting an existing driver, as I'd hate to merge something that has no user. Thanks, M. -- Without deviation from the norm, progress is not possible. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5B9F5C433E8 for ; Fri, 17 Jul 2020 10:50:02 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2558120734 for ; Fri, 17 Jul 2020 10:50:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="TXT3ti7f"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="FikxkxVb" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2558120734 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Subject:To:From: Message-ID:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Gb8Vj/X7yS3pLVXAnbnQRyZdWXCZtMoqaBeJHeWirwQ=; b=TXT3ti7ftpWYla9CWkDZc4cX9 ns/Vu3R9oK2450i50byWzTbWFYav1dtSMhwm+5L/WHbZRIA5bC5Rle0rKwRpAI/R1O/5DrlDV5HRk birfHw+xyu2ZG4rUoFBrJ0ncyEUhmrUF9EKX6r5EyJpnF2ziqFhi7bSz79f6cZSqECX0Bt0hEuYga tWfqLlI5wvSqKbb25EHUWUlsQcsOSSoc0x0Ik+AVstYsC7G1lL0ZJE1qU35S8kpddl9w5O3EWxk/D wuiTFQWxuUE1k/SVEM6H15bPRgnyfHkfCJtndpTdLgVr41LnKwTtwwiUpGDRZJbxvzi4OhwHcBnNJ k2ARpw+eg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jwNw4-0004mY-OO; Fri, 17 Jul 2020 10:49:48 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jwNw0-0004kw-K0; Fri, 17 Jul 2020 10:49:45 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 8D44520734; Fri, 17 Jul 2020 10:49:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594982983; bh=m4E2Qlwl0PzfDZp6G5x1yaTQ8T2DRdddavKw5MRwr2Q=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=FikxkxVbC7w8+8HsO1EqgoFIw2oPjWZzC9Zyy7PuyS9KyJ3mGvNiIwUabspzX8kpW bDocfwoAb67WQk9J9YncgdrgB4ApFeU2dOWUScUj1heeTAOiDckHNnf6tTHOGWFWWp Oi6aIvZ8BRzTUWnX59nyuIpZI4CEZapa5UEvjDwk= Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jwNvy-00CbMK-3Z; Fri, 17 Jul 2020 11:49:42 +0100 Date: Fri, 17 Jul 2020 11:49:41 +0100 Message-ID: <87k0z2xvp6.wl-maz@kernel.org> From: Marc Zyngier To: Saravana Kannan Subject: Re: [PATCH v2] irqchip: Add IRQCHIP_PLATFORM_DRIVER_BEGIN/END and IRQCHIP_MATCH helper macros In-Reply-To: <20200717024447.3128361-1-saravanak@google.com> References: <20200717024447.3128361-1-saravanak@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26.3 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: saravanak@google.com, tglx@linutronix.de, jason@lakedaemon.net, matthias.bgg@gmail.com, john.stultz@linaro.org, cc.hwang@mediatek.com, loda.chou@mediatek.com, hanks.chen@mediatek.com, kernel-team@android.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200717_064944_798192_99CF6434 X-CRM114-Status: GOOD ( 36.07 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: CC Hwang , Jason Cooper , Hanks Chen , Loda Chou , linux-kernel@vger.kernel.org, John Stultz , Matthias Brugger , linux-mediatek@lists.infradead.org, Thomas Gleixner , kernel-team@android.com, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org Hi Saravana, Thanks for re-spinning this one. On Fri, 17 Jul 2020 03:44:47 +0100, Saravana Kannan wrote: > > Compiling an irqchip driver as a platform driver needs to bunch of > things to be done right: > - Making sure the parent domain is initialized first > - Making sure the device can't be unbound from sysfs > - Disallowing module unload if it's built as a module > - Finding the parent node > - Etc. > > Instead of trying to make sure all future irqchip platform drivers get > this right, provide boilerplate macros that take care of all of this. > > An example use would look something like this. Where acme_foo_init and > acme_bar_init are similar to what would be passed to IRQCHIP_DECLARE. > > IRQCHIP_PLATFORM_DRIVER_BEGIN I think there is some value in having the BEGIN statement containing the driver name, see below. > IRQCHIP_MATCH(foo, "acme,foo", acme_foo_init) > IRQCHIP_MATCH(bar, "acme,bar", acme_bar_init) > IRQCHIP_PLATFORM_DRIVER_END(acme_irq) > > Signed-off-by: Saravana Kannan > --- > drivers/irqchip/irqchip.c | 22 ++++++++++++++++++++++ > include/linux/irqchip.h | 23 +++++++++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c > index 2b35e68bea82..236ea793f01c 100644 > --- a/drivers/irqchip/irqchip.c > +++ b/drivers/irqchip/irqchip.c > @@ -10,8 +10,10 @@ > > #include > #include > +#include > #include > #include > +#include > > /* > * This special of_device_id is the sentinel at the end of the > @@ -29,3 +31,23 @@ void __init irqchip_init(void) > of_irq_init(__irqchip_of_table); > acpi_probe_device_table(irqchip); > } > + > +int platform_irqchip_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct device_node *par_np = of_irq_find_parent(np); > + of_irq_init_cb_t irq_init_cb = of_device_get_match_data(&pdev->dev); > + > + if (!irq_init_cb) > + return -EINVAL; > + > + if (par_np == np) > + par_np = NULL; > + > + /* If there's a parent irqchip, make sure it has been initialized. */ > + if (par_np && !irq_find_matching_host(np, DOMAIN_BUS_ANY)) There is no guarantee that the calling driver wants BUS_ANY as a token for its parent. It may work for now, if you only have dependencies to drivers that only expose a single domain, but that's not a general purpose check.. At least, please add a comment saying that the new driver may want to check that the irqdomain it depends on may not be available. > + return -EPROBE_DEFER; > + > + return irq_init_cb(np, par_np); > +} > +EXPORT_SYMBOL_GPL(platform_irqchip_probe); > diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h > index 950e4b2458f0..6d5eba7cbbb7 100644 > --- a/include/linux/irqchip.h > +++ b/include/linux/irqchip.h > @@ -13,6 +13,7 @@ > > #include > #include > +#include > > /* > * This macro must be used by the different irqchip drivers to declare > @@ -26,6 +27,28 @@ > */ > #define IRQCHIP_DECLARE(name, compat, fn) OF_DECLARE_2(irqchip, name, compat, fn) > > +extern int platform_irqchip_probe(struct platform_device *pdev); > + > +#define IRQCHIP_PLATFORM_DRIVER_BEGIN \ > +static const struct of_device_id __irqchip_match_table[] = { How about: #define IRQCHIP_PLATFORM_DRIVER_BEGIN(drv_name) \ static const struct of_device_id __irqchip_match_table_##drv_name = { which makes it easier to debug when you want to identify specific structures in an object (otherwise, they all have the same name...). it is also much more pleasing aesthetically ;-). > + > +#define IRQCHIP_MATCH(compat, fn) { .compatible = compat, .data = fn }, > + > +#define IRQCHIP_PLATFORM_DRIVER_END(drv_name) \ > + {}, \ > +}; \ > +MODULE_DEVICE_TABLE(of, __irqchip_match_table); \ > +static struct platform_driver drv_name##_driver = { \ const? > + .probe = platform_irqchip_probe, \ > + .driver = { \ > + .name = #drv_name, \ > + .owner = THIS_MODULE, \ > + .of_match_table = __irqchip_match_table,\ > + .suppress_bind_attrs = true, \ > + }, \ > +}; \ > +builtin_platform_driver(drv_name##_driver) > + > /* > * This macro must be used by the different irqchip drivers to declare > * the association between their version and their initialization function. > -- > 2.28.0.rc0.105.gf9edc3c819-goog > > Otherwise looks good. When you respin it, it would be good to also submit one user of this API by converting an existing driver, as I'd hate to merge something that has no user. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 83DE0C433E2 for ; Fri, 17 Jul 2020 10:51:17 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4D950206BE for ; Fri, 17 Jul 2020 10:51:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="kjuWkqVf"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="FikxkxVb" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4D950206BE Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Subject:To:From: Message-ID:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Cu/esBK1l1+2wY5RXN5YbNsmBszDrAAQRykRrtqqQ/o=; b=kjuWkqVfLwpMcYkL3NLDQCSAp ycAmtV9xkpg9lIJmhUtNaqTE5fdElXOTsOtto6bgDEAlUPOtJLt0n5VWWRyjC71z4Hm5eF5gE3xFh BT8Rx6MDez2/ftGg8oWWPg5bvXBmcJkkgcabrM8plBDy08i8f2+O2btzJWPXjgTmPsx7SCWSf7G/q yvTVAnPEY2Ap24CwWqEXdeAO2+mamcFSyAGyHFVj5bdDGhtFgFZf05xqXP7Ukfhb2txEzTShFasSz z/hWcZ6YKwkyW44AWG3Osc63URWhnQ7Cpa8UKaoU/faIssqdMO4/5X5H+u/9xtQM0843CxwV9+0ZZ kUTDdIM4w==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jwNw3-0004m7-B8; Fri, 17 Jul 2020 10:49:47 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jwNw0-0004kw-K0; Fri, 17 Jul 2020 10:49:45 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 8D44520734; Fri, 17 Jul 2020 10:49:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594982983; bh=m4E2Qlwl0PzfDZp6G5x1yaTQ8T2DRdddavKw5MRwr2Q=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=FikxkxVbC7w8+8HsO1EqgoFIw2oPjWZzC9Zyy7PuyS9KyJ3mGvNiIwUabspzX8kpW bDocfwoAb67WQk9J9YncgdrgB4ApFeU2dOWUScUj1heeTAOiDckHNnf6tTHOGWFWWp Oi6aIvZ8BRzTUWnX59nyuIpZI4CEZapa5UEvjDwk= Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jwNvy-00CbMK-3Z; Fri, 17 Jul 2020 11:49:42 +0100 Date: Fri, 17 Jul 2020 11:49:41 +0100 Message-ID: <87k0z2xvp6.wl-maz@kernel.org> From: Marc Zyngier To: Saravana Kannan Subject: Re: [PATCH v2] irqchip: Add IRQCHIP_PLATFORM_DRIVER_BEGIN/END and IRQCHIP_MATCH helper macros In-Reply-To: <20200717024447.3128361-1-saravanak@google.com> References: <20200717024447.3128361-1-saravanak@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26.3 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: saravanak@google.com, tglx@linutronix.de, jason@lakedaemon.net, matthias.bgg@gmail.com, john.stultz@linaro.org, cc.hwang@mediatek.com, loda.chou@mediatek.com, hanks.chen@mediatek.com, kernel-team@android.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200717_064944_798192_99CF6434 X-CRM114-Status: GOOD ( 36.07 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: CC Hwang , Jason Cooper , Hanks Chen , Loda Chou , linux-kernel@vger.kernel.org, John Stultz , Matthias Brugger , linux-mediatek@lists.infradead.org, Thomas Gleixner , kernel-team@android.com, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Saravana, Thanks for re-spinning this one. On Fri, 17 Jul 2020 03:44:47 +0100, Saravana Kannan wrote: > > Compiling an irqchip driver as a platform driver needs to bunch of > things to be done right: > - Making sure the parent domain is initialized first > - Making sure the device can't be unbound from sysfs > - Disallowing module unload if it's built as a module > - Finding the parent node > - Etc. > > Instead of trying to make sure all future irqchip platform drivers get > this right, provide boilerplate macros that take care of all of this. > > An example use would look something like this. Where acme_foo_init and > acme_bar_init are similar to what would be passed to IRQCHIP_DECLARE. > > IRQCHIP_PLATFORM_DRIVER_BEGIN I think there is some value in having the BEGIN statement containing the driver name, see below. > IRQCHIP_MATCH(foo, "acme,foo", acme_foo_init) > IRQCHIP_MATCH(bar, "acme,bar", acme_bar_init) > IRQCHIP_PLATFORM_DRIVER_END(acme_irq) > > Signed-off-by: Saravana Kannan > --- > drivers/irqchip/irqchip.c | 22 ++++++++++++++++++++++ > include/linux/irqchip.h | 23 +++++++++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c > index 2b35e68bea82..236ea793f01c 100644 > --- a/drivers/irqchip/irqchip.c > +++ b/drivers/irqchip/irqchip.c > @@ -10,8 +10,10 @@ > > #include > #include > +#include > #include > #include > +#include > > /* > * This special of_device_id is the sentinel at the end of the > @@ -29,3 +31,23 @@ void __init irqchip_init(void) > of_irq_init(__irqchip_of_table); > acpi_probe_device_table(irqchip); > } > + > +int platform_irqchip_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct device_node *par_np = of_irq_find_parent(np); > + of_irq_init_cb_t irq_init_cb = of_device_get_match_data(&pdev->dev); > + > + if (!irq_init_cb) > + return -EINVAL; > + > + if (par_np == np) > + par_np = NULL; > + > + /* If there's a parent irqchip, make sure it has been initialized. */ > + if (par_np && !irq_find_matching_host(np, DOMAIN_BUS_ANY)) There is no guarantee that the calling driver wants BUS_ANY as a token for its parent. It may work for now, if you only have dependencies to drivers that only expose a single domain, but that's not a general purpose check.. At least, please add a comment saying that the new driver may want to check that the irqdomain it depends on may not be available. > + return -EPROBE_DEFER; > + > + return irq_init_cb(np, par_np); > +} > +EXPORT_SYMBOL_GPL(platform_irqchip_probe); > diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h > index 950e4b2458f0..6d5eba7cbbb7 100644 > --- a/include/linux/irqchip.h > +++ b/include/linux/irqchip.h > @@ -13,6 +13,7 @@ > > #include > #include > +#include > > /* > * This macro must be used by the different irqchip drivers to declare > @@ -26,6 +27,28 @@ > */ > #define IRQCHIP_DECLARE(name, compat, fn) OF_DECLARE_2(irqchip, name, compat, fn) > > +extern int platform_irqchip_probe(struct platform_device *pdev); > + > +#define IRQCHIP_PLATFORM_DRIVER_BEGIN \ > +static const struct of_device_id __irqchip_match_table[] = { How about: #define IRQCHIP_PLATFORM_DRIVER_BEGIN(drv_name) \ static const struct of_device_id __irqchip_match_table_##drv_name = { which makes it easier to debug when you want to identify specific structures in an object (otherwise, they all have the same name...). it is also much more pleasing aesthetically ;-). > + > +#define IRQCHIP_MATCH(compat, fn) { .compatible = compat, .data = fn }, > + > +#define IRQCHIP_PLATFORM_DRIVER_END(drv_name) \ > + {}, \ > +}; \ > +MODULE_DEVICE_TABLE(of, __irqchip_match_table); \ > +static struct platform_driver drv_name##_driver = { \ const? > + .probe = platform_irqchip_probe, \ > + .driver = { \ > + .name = #drv_name, \ > + .owner = THIS_MODULE, \ > + .of_match_table = __irqchip_match_table,\ > + .suppress_bind_attrs = true, \ > + }, \ > +}; \ > +builtin_platform_driver(drv_name##_driver) > + > /* > * This macro must be used by the different irqchip drivers to declare > * the association between their version and their initialization function. > -- > 2.28.0.rc0.105.gf9edc3c819-goog > > Otherwise looks good. When you respin it, it would be good to also submit one user of this API by converting an existing driver, as I'd hate to merge something that has no user. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel