From: Dan Scally <djrscally@gmail.com> To: Heikki Krogerus <heikki.krogerus@linux.intel.com>, Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Sakari Ailus <sakari.ailus@linux.intel.com>, Linux Media Mailing List <linux-media@vger.kernel.org>, Bingbu Cao <bingbu.cao@intel.com>, Tian Shu Qiu <tian.shu.qiu@intel.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "open list:STAGING SUBSYSTEM" <devel@driverdev.osuosl.org>, Jordan Hand <jorhand@linux.microsoft.com>, Kieran Bingham <kieran.bingham@ideasonboard.com>, Tsuchiya Yuto <kitakar@gmail.com> Subject: Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI Date: Wed, 9 Sep 2020 00:41:36 +0100 [thread overview] Message-ID: <2d4f1abb-c617-476a-1005-0ed91906a5f5@gmail.com> (raw) In-Reply-To: <20200526143110.GC3284396@kuha.fi.intel.com> Hi Heikki On 26/05/2020 15:31, Heikki Krogerus wrote: > On Fri, May 22, 2020 at 11:57:36AM +0200, Mauro Carvalho Chehab wrote: >> Em Thu, 21 May 2020 11:00:19 +0300 >> Andy Shevchenko <andy.shevchenko@gmail.com> escreveu: >> >>> +Cc: Heikki (swnode expert) >>> >>> On Wed, May 20, 2020 at 2:19 PM Mauro Carvalho Chehab >>> <mchehab+huawei@kernel.org> wrote: >>>> Em Wed, 20 May 2020 11:26:08 +0300 >>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: >>> >>> ... >>> >>>> As I said, the problem is not probing the sensor via ACPI, but, instead, >>>> to be able receive platform-specific data. >>> >>> There is no problem with swnodes, except missing parts (*). >>> I have Skylake laptop with IPU3 and with half-baked ACPI tables, but >>> since we have drivers in place with fwnode support, we only need to >>> recreate fwnode graph in some board file to compensate the gap in >>> ACPI. >>> >>> *) Missing part is graph support for swnodes. With that done it will >>> be feasible to achieve the rest. >>> I forgot if we have anything for this already done. Heikki? >> >> Hmm... I guess I should try this approach. I never heard about swnodes >> before. Do you have already some patch with the needed swnodes setup, >> and the missing parts to recreate the fwnode graph? > > Here you go. I tested it with this code: > > static const struct software_node nodes[]; > > static const struct property_entry ep0_props[] = { > PROPERTY_ENTRY_REF("remote-endpoint", &nodes[5]), > { } > }; > > static const struct property_entry ep1_props[] = { > PROPERTY_ENTRY_REF("remote-endpoint", &nodes[2]), > { } > }; > > static const struct software_node nodes[] = { > { "dev0" }, > { "port0", &nodes[0] }, > { "endpoint", &nodes[1], ep0_props }, > { "dev1" }, > { "port0", &nodes[3] }, > { "endpoint", &nodes[4], ep1_props }, > { } > }; > > void test(void) > { > const struct software_node *swnode; > struct fwnode_handle *fwnode; > > software_node_register_nodes(nodes); > > fwnode = fwnode_graph_get_remote_port_parent(software_node_fwnode(&nodes[5])); > swnode = to_software_node(fwnode); > printk("first parent: %s\n", swnode->name); > > fwnode = fwnode_graph_get_remote_port_parent(software_node_fwnode(&nodes[2])); > swnode = to_software_node(fwnode); > printk("second parent: %s\n", swnode->name); > > software_node_unregister_nodes(nodes); > } > > thanks, > One of the problems we're having trying to build (using the changes you attached here) a module to connect sensors to the cio2 infrastructure is that we can't unload it cleanly. There seems to be a couple of reasons for that; but one of them is that cio2_parse_firmware() in ipu3-cio2.c ticks up the refcount for fwnode_handles of the ports for the CIO2 device by calling software_node_graph_get_next_endpoint() once per _possible_ cio2 port; each time that happens it gets a reference to the port's fwnode_handle but doesn't release it. This isn't really a patch as such, since I don't think the changes you attached are actually applied either upstream or in the media_tree git (what are the plans in that regard, by the way? Will that patch be sent upstream at some point?) so there's nowhere to apply it to, but I think something like the below fixes it. What do you think? Regards, Dan --- diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 3667467196f0..62a1e3de8cb3 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -584,7 +584,9 @@ software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode, endpoint = software_node_get_next_child(port, old); fwnode_handle_put(old); if (endpoint) - break; + break; + else + fwnode_handle_put(port); } fwnode_handle_put(port);
WARNING: multiple messages have this Message-ID (diff)
From: Dan Scally <djrscally@gmail.com> To: Heikki Krogerus <heikki.krogerus@linux.intel.com>, Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Cc: "open list:STAGING SUBSYSTEM" <devel@driverdev.osuosl.org>, Jordan Hand <jorhand@linux.microsoft.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Kieran Bingham <kieran.bingham@ideasonboard.com>, Tsuchiya Yuto <kitakar@gmail.com>, Sakari Ailus <sakari.ailus@linux.intel.com>, Bingbu Cao <bingbu.cao@intel.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Tian Shu Qiu <tian.shu.qiu@intel.com>, Linux Media Mailing List <linux-media@vger.kernel.org> Subject: Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI Date: Wed, 9 Sep 2020 00:41:36 +0100 [thread overview] Message-ID: <2d4f1abb-c617-476a-1005-0ed91906a5f5@gmail.com> (raw) In-Reply-To: <20200526143110.GC3284396@kuha.fi.intel.com> Hi Heikki On 26/05/2020 15:31, Heikki Krogerus wrote: > On Fri, May 22, 2020 at 11:57:36AM +0200, Mauro Carvalho Chehab wrote: >> Em Thu, 21 May 2020 11:00:19 +0300 >> Andy Shevchenko <andy.shevchenko@gmail.com> escreveu: >> >>> +Cc: Heikki (swnode expert) >>> >>> On Wed, May 20, 2020 at 2:19 PM Mauro Carvalho Chehab >>> <mchehab+huawei@kernel.org> wrote: >>>> Em Wed, 20 May 2020 11:26:08 +0300 >>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: >>> >>> ... >>> >>>> As I said, the problem is not probing the sensor via ACPI, but, instead, >>>> to be able receive platform-specific data. >>> >>> There is no problem with swnodes, except missing parts (*). >>> I have Skylake laptop with IPU3 and with half-baked ACPI tables, but >>> since we have drivers in place with fwnode support, we only need to >>> recreate fwnode graph in some board file to compensate the gap in >>> ACPI. >>> >>> *) Missing part is graph support for swnodes. With that done it will >>> be feasible to achieve the rest. >>> I forgot if we have anything for this already done. Heikki? >> >> Hmm... I guess I should try this approach. I never heard about swnodes >> before. Do you have already some patch with the needed swnodes setup, >> and the missing parts to recreate the fwnode graph? > > Here you go. I tested it with this code: > > static const struct software_node nodes[]; > > static const struct property_entry ep0_props[] = { > PROPERTY_ENTRY_REF("remote-endpoint", &nodes[5]), > { } > }; > > static const struct property_entry ep1_props[] = { > PROPERTY_ENTRY_REF("remote-endpoint", &nodes[2]), > { } > }; > > static const struct software_node nodes[] = { > { "dev0" }, > { "port0", &nodes[0] }, > { "endpoint", &nodes[1], ep0_props }, > { "dev1" }, > { "port0", &nodes[3] }, > { "endpoint", &nodes[4], ep1_props }, > { } > }; > > void test(void) > { > const struct software_node *swnode; > struct fwnode_handle *fwnode; > > software_node_register_nodes(nodes); > > fwnode = fwnode_graph_get_remote_port_parent(software_node_fwnode(&nodes[5])); > swnode = to_software_node(fwnode); > printk("first parent: %s\n", swnode->name); > > fwnode = fwnode_graph_get_remote_port_parent(software_node_fwnode(&nodes[2])); > swnode = to_software_node(fwnode); > printk("second parent: %s\n", swnode->name); > > software_node_unregister_nodes(nodes); > } > > thanks, > One of the problems we're having trying to build (using the changes you attached here) a module to connect sensors to the cio2 infrastructure is that we can't unload it cleanly. There seems to be a couple of reasons for that; but one of them is that cio2_parse_firmware() in ipu3-cio2.c ticks up the refcount for fwnode_handles of the ports for the CIO2 device by calling software_node_graph_get_next_endpoint() once per _possible_ cio2 port; each time that happens it gets a reference to the port's fwnode_handle but doesn't release it. This isn't really a patch as such, since I don't think the changes you attached are actually applied either upstream or in the media_tree git (what are the plans in that regard, by the way? Will that patch be sent upstream at some point?) so there's nowhere to apply it to, but I think something like the below fixes it. What do you think? Regards, Dan --- diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 3667467196f0..62a1e3de8cb3 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -584,7 +584,9 @@ software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode, endpoint = software_node_get_next_child(port, old); fwnode_handle_put(old); if (endpoint) - break; + break; + else + fwnode_handle_put(port); } fwnode_handle_put(port); _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2020-09-08 23:41 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-16 10:43 [PATCH] media: ipu3: add a module to probe sensors via ACPI Mauro Carvalho Chehab 2020-05-16 10:43 ` Mauro Carvalho Chehab 2020-05-17 10:36 ` Sakari Ailus 2020-05-17 10:36 ` Sakari Ailus 2020-05-20 7:44 ` Mauro Carvalho Chehab 2020-05-20 7:44 ` Mauro Carvalho Chehab 2020-05-20 8:26 ` Sakari Ailus 2020-05-20 8:26 ` Sakari Ailus 2020-05-20 11:18 ` Mauro Carvalho Chehab 2020-05-20 11:18 ` Mauro Carvalho Chehab 2020-05-21 8:00 ` Andy Shevchenko 2020-05-21 8:00 ` Andy Shevchenko 2020-05-22 9:57 ` Mauro Carvalho Chehab 2020-05-22 9:57 ` Mauro Carvalho Chehab 2020-05-26 14:31 ` Heikki Krogerus 2020-05-26 14:31 ` Heikki Krogerus 2020-07-01 1:16 ` Jordan Hand 2020-07-01 1:16 ` Jordan Hand 2020-09-07 13:17 ` Dan Scally 2020-09-07 13:17 ` Dan Scally 2020-09-08 23:41 ` Dan Scally [this message] 2020-09-08 23:41 ` Dan Scally 2020-05-25 7:59 ` Heikki Krogerus 2020-05-25 7:59 ` Heikki Krogerus
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=2d4f1abb-c617-476a-1005-0ed91906a5f5@gmail.com \ --to=djrscally@gmail.com \ --cc=andriy.shevchenko@linux.intel.com \ --cc=bingbu.cao@intel.com \ --cc=devel@driverdev.osuosl.org \ --cc=gregkh@linuxfoundation.org \ --cc=heikki.krogerus@linux.intel.com \ --cc=jorhand@linux.microsoft.com \ --cc=kieran.bingham@ideasonboard.com \ --cc=kitakar@gmail.com \ --cc=linux-media@vger.kernel.org \ --cc=mchehab+huawei@kernel.org \ --cc=sakari.ailus@linux.intel.com \ --cc=tian.shu.qiu@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.