subdevice config into pointer (was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times)
Message ID | 20170302090727.GC27818@amd (mailing list archive) |
---|---|
State | RFC, archived |
Delegated to: | Laurent Pinchart |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1cjMjo-0003Qp-CN; Thu, 02 Mar 2017 09:09:28 +0000 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.84_2/mailfrontend-8) with esmtp id 1cjMjm-0004MD-ji; Thu, 02 Mar 2017 10:09:28 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754536AbdCBJJQ (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 2 Mar 2017 04:09:16 -0500 Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]:36076 "EHLO atrey.karlin.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754430AbdCBJH5 (ORCPT <rfc822; linux-media@vger.kernel.org>); Thu, 2 Mar 2017 04:07:57 -0500 Received: by atrey.karlin.mff.cuni.cz (Postfix, from userid 512) id 575D681998; Thu, 2 Mar 2017 10:07:28 +0100 (CET) Date: Thu, 2 Mar 2017 10:07:27 +0100 From: Pavel Machek <pavel@ucw.cz> To: Sakari Ailus <sakari.ailus@iki.fi> Cc: sre@kernel.org, pali.rohar@gmail.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, laurent.pinchart@ideasonboard.com, mchehab@kernel.org, ivo.g.dimitrov.75@gmail.com Subject: subdevice config into pointer (was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times) Message-ID: <20170302090727.GC27818@amd> References: <d315073f004ce46e0198fd614398e046ffe649e7.1487111824.git.pavel@ucw.cz> <20170220103114.GA9800@amd> <20170220130912.GT16975@valkosipuli.retiisi.org.uk> <20170220135636.GU16975@valkosipuli.retiisi.org.uk> <20170221110721.GD5021@amd> <20170221111104.GD16975@valkosipuli.retiisi.org.uk> <20170225000918.GB23662@amd> <20170225134444.6qzumpvasaow5qoj@ihha.localdomain> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="wxDdMuZNg1r63Hyj" Content-Disposition: inline In-Reply-To: <20170225134444.6qzumpvasaow5qoj@ihha.localdomain> User-Agent: Mutt/1.5.23 (2014-03-12) 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-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2017.3.2.90319 X-PMX-Spam: Gauge=IIIIIIIII, Probability=9%, Report=' BODY_PARA_IS_SENTENCE_URL 0.1, MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, KNOWN_FREEWEB_URI 0.05, MSGID_ADDED_BY_MTA 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_3000_3999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, INVALID_MSGID_NO_FQDN 0, IN_REP_TO 0, LEGITIMATE_SIGNS 0, MSG_THREAD 0, MULTIPLE_REAL_RCPTS 0, NO_URI_HTTPS 0, REFERENCES 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __ATTACHMENT_SIZE_0_10K 0, __CD 0, __CP_URI_IN_BODY 0, __CT 0, __CTYPE_HAS_BOUNDARY 0, __CTYPE_MULTIPART 0, __FORWARDED_MSG 0, __HAS_ATTACHMENT 0, __HAS_ATTACHMENT1 0, __HAS_ATTACHMENT2 0, __HAS_CC_HDR 0, __HAS_FROM 0, __HAS_LIST_ID 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __KNOWN_FREEWEB_URI2 0, __MIME_TEXT_P 0, __MIME_TEXT_P1 0, __MIME_TEXT_P2 0, __MIME_VERSION 0, __MULTIPLE_RCPTS_CC_X2 0, __MULTIPLE_URI_TEXT 0, __NO_HTML_TAG_RAW 0, __REFERENCES 0, __SANE_MSGID 0, __SUBJ_ALPHA_START 0, __TO_MALFORMED_2 0, __TO_NAME 0, __TO_NAME_DIFF_FROM_ACC 0, __TO_REAL_NAMES 0, __URI_IN_BODY 0, __URI_NS , __URI_WITH_PATH 0, __USER_AGENT 0' |
Commit Message
Pavel Machek
March 2, 2017, 9:07 a.m. UTC
Hi! > Making the sub-device bus configuration a pointer should be in a separate > patch. It makes sense since the entire configuration is not valid for all > sub-devices attached to the ISP anymore. I think it originally was a > separate patch, but they probably have been merged at some point. I can't > find it right now anyway. Something like this? Pavel commit df9141c66678b549fac9d143bd55ed0b242cf36e Author: Pavel <pavel@ucw.cz> Date: Wed Mar 1 13:27:56 2017 +0100 Turn bus in struct isp_async_subdev into pointer; some of our subdevs (flash, focus) will not need bus configuration. Signed-off-by: Pavel Machek <pavel@ucw.cz>
Comments
Hi Pavel, On Thu, Mar 02, 2017 at 10:07:27AM +0100, Pavel Machek wrote: > Hi! > > > Making the sub-device bus configuration a pointer should be in a separate > > patch. It makes sense since the entire configuration is not valid for all > > sub-devices attached to the ISP anymore. I think it originally was a > > separate patch, but they probably have been merged at some point. I can't > > find it right now anyway. > > Something like this? > Pavel > > commit df9141c66678b549fac9d143bd55ed0b242cf36e > Author: Pavel <pavel@ucw.cz> > Date: Wed Mar 1 13:27:56 2017 +0100 > > Turn bus in struct isp_async_subdev into pointer; some of our subdevs > (flash, focus) will not need bus configuration. > > Signed-off-by: Pavel Machek <pavel@ucw.cz> I applied this to the ccp2 branch with an improved patch description. > > diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c > index 8a456d4..36bd359 100644 > --- a/drivers/media/platform/omap3isp/isp.c > +++ b/drivers/media/platform/omap3isp/isp.c > @@ -2030,12 +2030,18 @@ enum isp_of_phy { > static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn, > struct isp_async_subdev *isd) > { > - struct isp_bus_cfg *buscfg = &isd->bus; > + struct isp_bus_cfg *buscfg; > struct v4l2_fwnode_endpoint vfwn; > unsigned int i; > int ret; > bool csi1 = false; > > + buscfg = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL); > + if (!buscfg) > + return -ENOMEM; > + > + isd->bus = buscfg; > + > ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn); > if (ret) > return ret; > @@ -2246,7 +2252,7 @@ static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async, > container_of(asd, struct isp_async_subdev, asd); > > isd->sd = subdev; > - isd->sd->host_priv = &isd->bus; > + isd->sd->host_priv = isd->bus; > > return 0; > } > diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h > index 7e6f663..c0b9d1d 100644 > --- a/drivers/media/platform/omap3isp/isp.h > +++ b/drivers/media/platform/omap3isp/isp.h > @@ -228,7 +228,7 @@ struct isp_device { > > struct isp_async_subdev { > struct v4l2_subdev *sd; > - struct isp_bus_cfg bus; > + struct isp_bus_cfg *bus; > struct v4l2_async_subdev asd; > }; > > diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c > index f20abe8..be23408 100644 > --- a/drivers/media/platform/omap3isp/ispcsiphy.c > +++ b/drivers/media/platform/omap3isp/ispcsiphy.c > @@ -202,7 +202,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy) > struct isp_async_subdev *isd = > container_of(pipe->external->asd, > struct isp_async_subdev, asd); > - buscfg = &isd->bus; > + buscfg = isd->bus; > } > > if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1 > >
Hi! > > > Making the sub-device bus configuration a pointer should be in a separate > > > patch. It makes sense since the entire configuration is not valid for all > > > sub-devices attached to the ISP anymore. I think it originally was a > > > separate patch, but they probably have been merged at some point. I can't > > > find it right now anyway. > > > > Something like this? > > > > commit df9141c66678b549fac9d143bd55ed0b242cf36e > > Author: Pavel <pavel@ucw.cz> > > Date: Wed Mar 1 13:27:56 2017 +0100 > > > > Turn bus in struct isp_async_subdev into pointer; some of our subdevs > > (flash, focus) will not need bus configuration. > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > I applied this to the ccp2 branch with an improved patch > description. Thanks! [But the important part is to get subdevices to work on ccp2 based branch, and it still fails to work at all if I attempt to enable them. I'd like to understand why...] Pavel
Hi Pavel, On Thu, Mar 02, 2017 at 03:58:08PM +0100, Pavel Machek wrote: > Hi! > > > > > Making the sub-device bus configuration a pointer should be in a separate > > > > patch. It makes sense since the entire configuration is not valid for all > > > > sub-devices attached to the ISP anymore. I think it originally was a > > > > separate patch, but they probably have been merged at some point. I can't > > > > find it right now anyway. > > > > > > Something like this? > > > > > > commit df9141c66678b549fac9d143bd55ed0b242cf36e > > > Author: Pavel <pavel@ucw.cz> > > > Date: Wed Mar 1 13:27:56 2017 +0100 > > > > > > Turn bus in struct isp_async_subdev into pointer; some of our subdevs > > > (flash, focus) will not need bus configuration. > > > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > > > I applied this to the ccp2 branch with an improved patch > > description. > > Thanks! > > [But the important part is to get subdevices to work on ccp2 based > branch, and it still fails to work at all if I attempt to enable > them. I'd like to understand why...] Did you add the flash / lens to the async list? The patches currently in the ccp branch do not include that --- it should be in parsing the flash / lens-focus properties in omap3isp device's node.
Hi Sakari, On Thursday 02 Mar 2017 16:16:17 Sakari Ailus wrote: > On Thu, Mar 02, 2017 at 10:07:27AM +0100, Pavel Machek wrote: > > Hi! > > > > > Making the sub-device bus configuration a pointer should be in a > > > separate patch. It makes sense since the entire configuration is not > > > valid for all sub-devices attached to the ISP anymore. I think it > > > originally was a separate patch, but they probably have been merged at > > > some point. I can'tfind it right now anyway. > > > > Something like this? > > > > Pavel > > > > commit df9141c66678b549fac9d143bd55ed0b242cf36e > > Author: Pavel <pavel@ucw.cz> > > Date: Wed Mar 1 13:27:56 2017 +0100 > > > > Turn bus in struct isp_async_subdev into pointer; some of our subdevs > > (flash, focus) will not need bus configuration. > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > I applied this to the ccp2 branch with an improved patch description. > > > diff --git a/drivers/media/platform/omap3isp/isp.c > > b/drivers/media/platform/omap3isp/isp.c index 8a456d4..36bd359 100644 > > --- a/drivers/media/platform/omap3isp/isp.c > > +++ b/drivers/media/platform/omap3isp/isp.c > > @@ -2030,12 +2030,18 @@ enum isp_of_phy { > > > > static int isp_fwnode_parse(struct device *dev, struct fwnode_handle > > *fwn, > > > > struct isp_async_subdev *isd) > > > > { > > > > - struct isp_bus_cfg *buscfg = &isd->bus; > > + struct isp_bus_cfg *buscfg; > > > > struct v4l2_fwnode_endpoint vfwn; > > unsigned int i; > > int ret; > > bool csi1 = false; > > > > + buscfg = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL); Given that you recently get rid of devm_kzalloc() in the driver, let's not introduce a new one here. > > + if (!buscfg) > > + return -ENOMEM; > > + > > + isd->bus = buscfg; > > + > > ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn); > > if (ret) > > > > return ret; > > [snip]
Hi! > > > static int isp_fwnode_parse(struct device *dev, struct fwnode_handle > > > *fwn, > > > > > > struct isp_async_subdev *isd) > > > > > > { > > > > > > - struct isp_bus_cfg *buscfg = &isd->bus; > > > + struct isp_bus_cfg *buscfg; > > > > > > struct v4l2_fwnode_endpoint vfwn; > > > unsigned int i; > > > int ret; > > > bool csi1 = false; > > > > > > + buscfg = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL); > > Given that you recently get rid of devm_kzalloc() in the driver, let's not > introduce a new one here. What is wrong with devm_kzalloc()? Pavel
Hi Laurent, On Thu, Mar 02, 2017 at 08:39:51PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Thursday 02 Mar 2017 16:16:17 Sakari Ailus wrote: > > On Thu, Mar 02, 2017 at 10:07:27AM +0100, Pavel Machek wrote: > > > Hi! > > > > > > > Making the sub-device bus configuration a pointer should be in a > > > > separate patch. It makes sense since the entire configuration is not > > > > valid for all sub-devices attached to the ISP anymore. I think it > > > > originally was a separate patch, but they probably have been merged at > > > > some point. I can'tfind it right now anyway. > > > > > > Something like this? > > > > > > Pavel > > > > > > commit df9141c66678b549fac9d143bd55ed0b242cf36e > > > Author: Pavel <pavel@ucw.cz> > > > Date: Wed Mar 1 13:27:56 2017 +0100 > > > > > > Turn bus in struct isp_async_subdev into pointer; some of our subdevs > > > (flash, focus) will not need bus configuration. > > > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > > > I applied this to the ccp2 branch with an improved patch description. > > > > > diff --git a/drivers/media/platform/omap3isp/isp.c > > > b/drivers/media/platform/omap3isp/isp.c index 8a456d4..36bd359 100644 > > > --- a/drivers/media/platform/omap3isp/isp.c > > > +++ b/drivers/media/platform/omap3isp/isp.c > > > @@ -2030,12 +2030,18 @@ enum isp_of_phy { > > > > > > static int isp_fwnode_parse(struct device *dev, struct fwnode_handle > > > *fwn, > > > > > > struct isp_async_subdev *isd) > > > > > > { > > > > > > - struct isp_bus_cfg *buscfg = &isd->bus; > > > + struct isp_bus_cfg *buscfg; > > > > > > struct v4l2_fwnode_endpoint vfwn; > > > unsigned int i; > > > int ret; > > > bool csi1 = false; > > > > > > + buscfg = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL); > > Given that you recently get rid of devm_kzalloc() in the driver, let's not > introduce a new one here. That's certainly a valid point. Still, the entire async sub-devices array is allocated with devm_() allocation functions still; that part wasn't addressed by the patchset mostly removing devm_() memory allocation, so this patch does actually not change how the memory is allocated. Beyond that, I'm not entirely sure whether this is a problem to begin with: devm resources are released after remove() callback and access to this data structure should only happen as a direct result of user IOCTL. IOCTLs may only be in progress as long as there are open file handles on a device --- and such file handles must be closed until the remove() callback may finish. (Referring to the Oslo meeting notes.) Some of the above must be still verified; either way, but the options are clear: either devm must be removed here as well (with the rest) or that it's fine to use it here: from this point of view this patch makes no difference. > > > > + if (!buscfg) > > > + return -ENOMEM; > > > + > > > + isd->bus = buscfg; > > > + > > > ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn); > > > if (ret) > > > > > > return ret; > > > > > [snip] >
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c index 8a456d4..36bd359 100644 --- a/drivers/media/platform/omap3isp/isp.c +++ b/drivers/media/platform/omap3isp/isp.c @@ -2030,12 +2030,18 @@ enum isp_of_phy { static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn, struct isp_async_subdev *isd) { - struct isp_bus_cfg *buscfg = &isd->bus; + struct isp_bus_cfg *buscfg; struct v4l2_fwnode_endpoint vfwn; unsigned int i; int ret; bool csi1 = false; + buscfg = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL); + if (!buscfg) + return -ENOMEM; + + isd->bus = buscfg; + ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn); if (ret) return ret; @@ -2246,7 +2252,7 @@ static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async, container_of(asd, struct isp_async_subdev, asd); isd->sd = subdev; - isd->sd->host_priv = &isd->bus; + isd->sd->host_priv = isd->bus; return 0; } diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h index 7e6f663..c0b9d1d 100644 --- a/drivers/media/platform/omap3isp/isp.h +++ b/drivers/media/platform/omap3isp/isp.h @@ -228,7 +228,7 @@ struct isp_device { struct isp_async_subdev { struct v4l2_subdev *sd; - struct isp_bus_cfg bus; + struct isp_bus_cfg *bus; struct v4l2_async_subdev asd; }; diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c index f20abe8..be23408 100644 --- a/drivers/media/platform/omap3isp/ispcsiphy.c +++ b/drivers/media/platform/omap3isp/ispcsiphy.c @@ -202,7 +202,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy) struct isp_async_subdev *isd = container_of(pipe->external->asd, struct isp_async_subdev, asd); - buscfg = &isd->bus; + buscfg = isd->bus; } if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1