Message ID | 20200504092611.9798-31-laurent.pinchart@ideasonboard.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Delegated to: | Hans Verkuil |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1jVXKU-00AuBY-3Q; Mon, 04 May 2020 09:24:02 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728439AbgEDJ0x (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Mon, 4 May 2020 05:26:53 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:56776 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728401AbgEDJ0v (ORCPT <rfc822;linux-media@vger.kernel.org>); Mon, 4 May 2020 05:26:51 -0400 Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 4947F304; Mon, 4 May 2020 11:26:47 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1588584407; bh=kA6YRPSNeFZEAYiNKAMVdXCXB8iBJh1FSsBXbLabxA8=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=YF1cIwjY386T5teD5tz/eZo/fhmr0WIwlSLS22a9kQ2ANKWax3ASPFkk/KjCtKtTs 5oYR7xbpS42NtMRshv3wQ50xnFzVixudHsh3BxOBZgF69aTIOMdVxQxcc9zR1JwfHa r2ObMFn2U7bEEpdy0bbaVh4jxYM3Z66U/FxPvkAA= From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: linux-media@vger.kernel.org Cc: Kieran Bingham <kieran.bingham@ideasonboard.com>, Jacopo Mondi <jacopo@jmondi.org>, =?utf-8?q?Niklas_S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>, Naushir Patuck <naush@raspberrypi.com>, Dave Stevenson <dave.stevenson@raspberrypi.com>, Phil Elwell <phil@raspberrypi.com> Subject: [PATCH v2 30/34] staging: vchiq_arm: Give vchiq children DT nodes Date: Mon, 4 May 2020 12:26:07 +0300 Message-Id: <20200504092611.9798-31-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20200504092611.9798-1-laurent.pinchart@ideasonboard.com> References: <20200504092611.9798-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
Drivers for the BCM283x CSI-2/CCP2 receiver and ISP
|
|
Commit Message
Laurent Pinchart
May 4, 2020, 9:26 a.m. UTC
From: Phil Elwell <phil@raspberrypi.com> vchiq kernel clients are now instantiated as platform drivers rather than using DT, but the children of the vchiq interface may still benefit from access to DT properties. Give them the option of a a sub-node of the vchiq parent for configuration and to allow them to be disabled. Signed-off-by: Phil Elwell <phil@raspberrypi.com> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++++++ 1 file changed, 8 insertions(+)
Comments
Hi Phil, Laurent, On Mon, 2020-05-04 at 12:26 +0300, Laurent Pinchart wrote: > From: Phil Elwell <phil@raspberrypi.com> > > vchiq kernel clients are now instantiated as platform drivers rather > than using DT, but the children of the vchiq interface may still > benefit from access to DT properties. Give them the option of a > a sub-node of the vchiq parent for configuration and to allow > them to be disabled. > > Signed-off-by: Phil Elwell <phil@raspberrypi.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > index dd3c8f829daa..2325ab825941 100644 > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > @@ -2734,12 +2734,20 @@ vchiq_register_child(struct platform_device *pdev, > const char *name) > pdevinfo.id = PLATFORM_DEVID_NONE; > pdevinfo.dma_mask = DMA_BIT_MASK(32); > > + np = of_get_child_by_name(pdev->dev.of_node, name); > + > + /* Skip the child if it is explicitly disabled */ > + if (np && !of_device_is_available(np)) > + return NULL; I think this is alright, although I'd reshufle the code a little so it looks nicer: + /* Skip the child if it is explicitly disabled */ + np = of_get_child_by_name(pdev->dev.of_node, name); + if (np && !of_device_is_available(np)) + return NULL; > child = platform_device_register_full(&pdevinfo); > if (IS_ERR(child)) { > dev_warn(&pdev->dev, "%s not registered\n", name); > child = NULL; > } > > + child->dev.of_node = np; Is this really needed? I'd rather have the parent's np (as commented in patch 26) as long as this is not a real device-tree defined platform device. Regards, Nicolas
Hi Nicolas, On 04/05/2020 18:12, Nicolas Saenz Julienne wrote: > Hi Phil, Laurent, > > On Mon, 2020-05-04 at 12:26 +0300, Laurent Pinchart wrote: >> From: Phil Elwell <phil@raspberrypi.com> >> >> vchiq kernel clients are now instantiated as platform drivers rather >> than using DT, but the children of the vchiq interface may still >> benefit from access to DT properties. Give them the option of a >> a sub-node of the vchiq parent for configuration and to allow >> them to be disabled. >> >> Signed-off-by: Phil Elwell <phil@raspberrypi.com> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >> --- >> .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >> index dd3c8f829daa..2325ab825941 100644 >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >> @@ -2734,12 +2734,20 @@ vchiq_register_child(struct platform_device *pdev, >> const char *name) >> pdevinfo.id = PLATFORM_DEVID_NONE; >> pdevinfo.dma_mask = DMA_BIT_MASK(32); >> >> + np = of_get_child_by_name(pdev->dev.of_node, name); >> + >> + /* Skip the child if it is explicitly disabled */ >> + if (np && !of_device_is_available(np)) >> + return NULL; > > I think this is alright, although I'd reshufle the code a little so it looks > nicer: > > + /* Skip the child if it is explicitly disabled */ > + np = of_get_child_by_name(pdev->dev.of_node, name); > + if (np && !of_device_is_available(np)) > + return NULL; I prefer the original. >> child = platform_device_register_full(&pdevinfo); >> if (IS_ERR(child)) { >> dev_warn(&pdev->dev, "%s not registered\n", name); >> child = NULL; >> } >> >> + child->dev.of_node = np; > > Is this really needed? I'd rather have the parent's np (as commented in patch > 26) as long as this is not a real device-tree defined platform device. Unless the of_node pointer refers to the sub-node for the child, all children would have to share a common set of properties, rather defeating the point of the change. Phil
On Mon, 2020-05-04 at 20:42 +0100, Phil Elwell wrote: > Hi Nicolas, > > On 04/05/2020 18:12, Nicolas Saenz Julienne wrote: > > Hi Phil, Laurent, > > > > On Mon, 2020-05-04 at 12:26 +0300, Laurent Pinchart wrote: > > > From: Phil Elwell <phil@raspberrypi.com> > > > > > > vchiq kernel clients are now instantiated as platform drivers rather > > > than using DT, but the children of the vchiq interface may still > > > benefit from access to DT properties. Give them the option of a > > > a sub-node of the vchiq parent for configuration and to allow > > > them to be disabled. > > > > > > Signed-off-by: Phil Elwell <phil@raspberrypi.com> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > > index dd3c8f829daa..2325ab825941 100644 > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > > @@ -2734,12 +2734,20 @@ vchiq_register_child(struct platform_device *pdev, > > > const char *name) > > > pdevinfo.id = PLATFORM_DEVID_NONE; > > > pdevinfo.dma_mask = DMA_BIT_MASK(32); > > > > > > + np = of_get_child_by_name(pdev->dev.of_node, name); > > > + > > > + /* Skip the child if it is explicitly disabled */ > > > + if (np && !of_device_is_available(np)) > > > + return NULL; > > > > I think this is alright, although I'd reshufle the code a little so it looks > > nicer: > > > > + /* Skip the child if it is explicitly disabled */ > > + np = of_get_child_by_name(pdev->dev.of_node, name); > > + if (np && !of_device_is_available(np)) > > + return NULL; > > I prefer the original. Fair enough > > > child = platform_device_register_full(&pdevinfo); > > > if (IS_ERR(child)) { > > > dev_warn(&pdev->dev, "%s not registered\n", name); > > > child = NULL; > > > } > > > > > > + child->dev.of_node = np; > > > > Is this really needed? I'd rather have the parent's np (as commented in > > patch > > 26) as long as this is not a real device-tree defined platform device. > > Unless the of_node pointer refers to the sub-node for the child, all children > would have to share a common set of properties, rather defeating the point of > the > change. Sorry I wasn't clear, my main point is that, since manually editing device internals is bad a practice, specially after they have been registered (there are potential races with dma_configure()/probe()). I want to make sure we need it in the first place (i.e. I don't see any further usage of that device node). If we can get rid of this line, we're better-off. If we actually need the device node further down I propose two things: - Use dev.of_node_reused, and do an children lookup anytime you need to get a property. It's a one-liner in the end. - Move device registration to DT. There has been some push-back of this in the past, but IMO things like arm's SCMI already set a standard on what firmware devices can do trough DT and it fits this situation. Regards, Nicolas
Hi Nicolas, On Tue, 5 May 2020 at 11:37, Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > On Mon, 2020-05-04 at 20:42 +0100, Phil Elwell wrote: > > Hi Nicolas, > > > > On 04/05/2020 18:12, Nicolas Saenz Julienne wrote: > > > Hi Phil, Laurent, > > > > > > On Mon, 2020-05-04 at 12:26 +0300, Laurent Pinchart wrote: > > > > From: Phil Elwell <phil@raspberrypi.com> > > > > > > > > vchiq kernel clients are now instantiated as platform drivers rather > > > > than using DT, but the children of the vchiq interface may still > > > > benefit from access to DT properties. Give them the option of a > > > > a sub-node of the vchiq parent for configuration and to allow > > > > them to be disabled. > > > > > > > > Signed-off-by: Phil Elwell <phil@raspberrypi.com> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > > > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > > > index dd3c8f829daa..2325ab825941 100644 > > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > > > @@ -2734,12 +2734,20 @@ vchiq_register_child(struct platform_device *pdev, > > > > const char *name) > > > > pdevinfo.id = PLATFORM_DEVID_NONE; > > > > pdevinfo.dma_mask = DMA_BIT_MASK(32); > > > > > > > > + np = of_get_child_by_name(pdev->dev.of_node, name); > > > > + > > > > + /* Skip the child if it is explicitly disabled */ > > > > + if (np && !of_device_is_available(np)) > > > > + return NULL; > > > > > > I think this is alright, although I'd reshufle the code a little so it looks > > > nicer: > > > > > > + /* Skip the child if it is explicitly disabled */ > > > + np = of_get_child_by_name(pdev->dev.of_node, name); > > > + if (np && !of_device_is_available(np)) > > > + return NULL; > > > > I prefer the original. > > Fair enough > > > > > child = platform_device_register_full(&pdevinfo); > > > > if (IS_ERR(child)) { > > > > dev_warn(&pdev->dev, "%s not registered\n", name); > > > > child = NULL; > > > > } > > > > > > > > + child->dev.of_node = np; > > > > > > Is this really needed? I'd rather have the parent's np (as commented in > > > patch > > > 26) as long as this is not a real device-tree defined platform device. > > > > Unless the of_node pointer refers to the sub-node for the child, all children > > would have to share a common set of properties, rather defeating the point of > > the > > change. > > Sorry I wasn't clear, my main point is that, since manually editing device > internals is bad a practice, specially after they have been registered (there > are potential races with dma_configure()/probe()). I want to make sure we need > it in the first place (i.e. I don't see any further usage of that device node). > > If we can get rid of this line, we're better-off. Thanks - that is much clearer. > If we actually need the device node further down I propose two things: > - Use dev.of_node_reused, and do an children lookup anytime you need to get a > property. It's a one-liner in the end. > - Move device registration to DT. There has been some push-back of this in the > past, but IMO things like arm's SCMI already set a standard on what firmware > devices can do trough DT and it fits this situation. I much prefer registration via DT - enumerating the children in code rather than data always felt like a baffling step backwards. Phil
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c index dd3c8f829daa..2325ab825941 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c @@ -2734,12 +2734,20 @@ vchiq_register_child(struct platform_device *pdev, const char *name) pdevinfo.id = PLATFORM_DEVID_NONE; pdevinfo.dma_mask = DMA_BIT_MASK(32); + np = of_get_child_by_name(pdev->dev.of_node, name); + + /* Skip the child if it is explicitly disabled */ + if (np && !of_device_is_available(np)) + return NULL; + child = platform_device_register_full(&pdevinfo); if (IS_ERR(child)) { dev_warn(&pdev->dev, "%s not registered\n", name); child = NULL; } + child->dev.of_node = np; + /* * We want the dma-ranges etc to be copied from a device with the * correct dma-ranges for the VPU.