| Message ID | 20201019225903.14276-8-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows | expand |
On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: > fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices > only; that status being determined through the .device_is_available() op > of the device's fwnode. As software_nodes don't have that operation and > adding it is meaningless, we instead need to check if the device's fwnode > is a software_node and if so pass the appropriate flag to disable that > check Period. I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id().
Hi Andy, On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: > On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: > > fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices > > only; that status being determined through the .device_is_available() op > > of the device's fwnode. As software_nodes don't have that operation and > > adding it is meaningless, we instead need to check if the device's fwnode > > is a software_node and if so pass the appropriate flag to disable that > > check > > Period. > > I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). The device availability test is actually there for a reason. Some firmware implementations put all the potential devices in the tables and only one (of some) of them are available. Could this be implemented so that if the node is a software node, then get its parent and then see if that is available? I guess that could be implemented in software node ops. Any opinions?
Hi Sakari On 20/10/2020 13:06, Sakari Ailus wrote: > Hi Andy, > > On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: >> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: >>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices >>> only; that status being determined through the .device_is_available() op >>> of the device's fwnode. As software_nodes don't have that operation and >>> adding it is meaningless, we instead need to check if the device's fwnode >>> is a software_node and if so pass the appropriate flag to disable that >>> check >> Period. >> >> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). > The device availability test is actually there for a reason. Some firmware > implementations put all the potential devices in the tables and only one > (of some) of them are available. > > Could this be implemented so that if the node is a software node, then get > its parent and then see if that is available? > > I guess that could be implemented in software node ops. Any opinions? Actually when considering the cio2 device, it seems that set_secondary_fwnode() actually overwrites the _primary_, given fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, this wouldn't work.
Hi Dan, On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote: > Hi Sakari > > On 20/10/2020 13:06, Sakari Ailus wrote: > > Hi Andy, > > > > On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: > >> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: > >>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices > >>> only; that status being determined through the .device_is_available() op > >>> of the device's fwnode. As software_nodes don't have that operation and > >>> adding it is meaningless, we instead need to check if the device's fwnode > >>> is a software_node and if so pass the appropriate flag to disable that > >>> check > >> Period. > >> > >> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). > > The device availability test is actually there for a reason. Some firmware > > implementations put all the potential devices in the tables and only one > > (of some) of them are available. > > > > Could this be implemented so that if the node is a software node, then get > > its parent and then see if that is available? > > > > I guess that could be implemented in software node ops. Any opinions? > Actually when considering the cio2 device, it seems that > set_secondary_fwnode() actually overwrites the _primary_, given > fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, > this wouldn't work. Ouch. I wonder when this happens --- have you checked what's the primary there? I guess it might be if it's a PCI device without the corresponding ACPI device node? I remember you had an is_available implementation that just returned true for software nodes in an early version of the set? I think it would still be a lesser bad in this case.
Hi Sakari On 20/10/2020 23:49, Sakari Ailus wrote: > Hi Dan, > > On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote: >> Hi Sakari >> >> On 20/10/2020 13:06, Sakari Ailus wrote: >>> Hi Andy, >>> >>> On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: >>>> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: >>>>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices >>>>> only; that status being determined through the .device_is_available() op >>>>> of the device's fwnode. As software_nodes don't have that operation and >>>>> adding it is meaningless, we instead need to check if the device's fwnode >>>>> is a software_node and if so pass the appropriate flag to disable that >>>>> check >>>> Period. >>>> >>>> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). >>> The device availability test is actually there for a reason. Some firmware >>> implementations put all the potential devices in the tables and only one >>> (of some) of them are available. >>> >>> Could this be implemented so that if the node is a software node, then get >>> its parent and then see if that is available? >>> >>> I guess that could be implemented in software node ops. Any opinions? >> Actually when considering the cio2 device, it seems that >> set_secondary_fwnode() actually overwrites the _primary_, given >> fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, >> this wouldn't work. > Ouch. I wonder when this happens --- have you checked what's the primary > there? I guess it might be if it's a PCI device without the corresponding > ACPI device node? Yes; it's null, and I think that diagnosis is correct. > I remember you had an is_available implementation that just returned true > for software nodes in an early version of the set? I think it would still > be a lesser bad in this case. Yep - I can put that back in and just drop this patch then; fine for me.
Hi Sakari On Wed, Oct 21, 2020 at 01:49:10AM +0300, Sakari Ailus wrote: > On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote: > > On 20/10/2020 13:06, Sakari Ailus wrote: > > > On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: > > >> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: > > >>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices > > >>> only; that status being determined through the .device_is_available() op > > >>> of the device's fwnode. As software_nodes don't have that operation and > > >>> adding it is meaningless, we instead need to check if the device's fwnode > > >>> is a software_node and if so pass the appropriate flag to disable that > > >>> check > > >> Period. > > >> > > >> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). > > > The device availability test is actually there for a reason. Some firmware > > > implementations put all the potential devices in the tables and only one > > > (of some) of them are available. > > > > > > Could this be implemented so that if the node is a software node, then get > > > its parent and then see if that is available? > > > > > > I guess that could be implemented in software node ops. Any opinions? > > Actually when considering the cio2 device, it seems that > > set_secondary_fwnode() actually overwrites the _primary_, given > > fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, > > this wouldn't work. > > Ouch. I wonder when this happens --- have you checked what's the primary > there? I guess it might be if it's a PCI device without the corresponding > ACPI device node? > > I remember you had an is_available implementation that just returned true > for software nodes in an early version of the set? I think it would still > be a lesser bad in this case. How about the following ? diff --git a/drivers/base/property.c b/drivers/base/property.c index 81bd01ed4042..ea44ba846299 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -706,9 +706,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); /** * fwnode_device_is_available - check if a device is available for use * @fwnode: Pointer to the fwnode of the device. + * + * For fwnode node types that don't implement the .device_is_available() + * operation, such as software nodes, this function returns true. */ bool fwnode_device_is_available(const struct fwnode_handle *fwnode) { + if (!fwnode_has_op(fwnode, device_is_available)) + return true; return fwnode_call_bool_op(fwnode, device_is_available); } EXPORT_SYMBOL_GPL(fwnode_device_is_available);
On Sat, Oct 24, 2020 at 03:39:55AM +0300, Laurent Pinchart wrote: > Hi Sakari > > On Wed, Oct 21, 2020 at 01:49:10AM +0300, Sakari Ailus wrote: > > On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote: > > > On 20/10/2020 13:06, Sakari Ailus wrote: > > > > On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: > > > >> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: > > > >>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices > > > >>> only; that status being determined through the .device_is_available() op > > > >>> of the device's fwnode. As software_nodes don't have that operation and > > > >>> adding it is meaningless, we instead need to check if the device's fwnode > > > >>> is a software_node and if so pass the appropriate flag to disable that > > > >>> check > > > >> Period. > > > >> > > > >> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). > > > > The device availability test is actually there for a reason. Some firmware > > > > implementations put all the potential devices in the tables and only one > > > > (of some) of them are available. > > > > > > > > Could this be implemented so that if the node is a software node, then get > > > > its parent and then see if that is available? > > > > > > > > I guess that could be implemented in software node ops. Any opinions? > > > Actually when considering the cio2 device, it seems that > > > set_secondary_fwnode() actually overwrites the _primary_, given > > > fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, > > > this wouldn't work. > > > > Ouch. I wonder when this happens --- have you checked what's the primary > > there? I guess it might be if it's a PCI device without the corresponding > > ACPI device node? > > > > I remember you had an is_available implementation that just returned true > > for software nodes in an early version of the set? I think it would still > > be a lesser bad in this case. > > How about the following ? Looks good to me. > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 81bd01ed4042..ea44ba846299 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -706,9 +706,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); > /** > * fwnode_device_is_available - check if a device is available for use > * @fwnode: Pointer to the fwnode of the device. > + * > + * For fwnode node types that don't implement the .device_is_available() > + * operation, such as software nodes, this function returns true. > */ > bool fwnode_device_is_available(const struct fwnode_handle *fwnode) > { > + if (!fwnode_has_op(fwnode, device_is_available)) > + return true; > return fwnode_call_bool_op(fwnode, device_is_available); > } > EXPORT_SYMBOL_GPL(fwnode_device_is_available); >
On 24/10/2020 15:29, Sakari Ailus wrote: > On Sat, Oct 24, 2020 at 03:39:55AM +0300, Laurent Pinchart wrote: >> Hi Sakari >> >> On Wed, Oct 21, 2020 at 01:49:10AM +0300, Sakari Ailus wrote: >>> On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote: >>>> On 20/10/2020 13:06, Sakari Ailus wrote: >>>>> On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: >>>>>> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: >>>>>>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices >>>>>>> only; that status being determined through the .device_is_available() op >>>>>>> of the device's fwnode. As software_nodes don't have that operation and >>>>>>> adding it is meaningless, we instead need to check if the device's fwnode >>>>>>> is a software_node and if so pass the appropriate flag to disable that >>>>>>> check >>>>>> Period. >>>>>> >>>>>> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). >>>>> The device availability test is actually there for a reason. Some firmware >>>>> implementations put all the potential devices in the tables and only one >>>>> (of some) of them are available. >>>>> >>>>> Could this be implemented so that if the node is a software node, then get >>>>> its parent and then see if that is available? >>>>> >>>>> I guess that could be implemented in software node ops. Any opinions? >>>> Actually when considering the cio2 device, it seems that >>>> set_secondary_fwnode() actually overwrites the _primary_, given >>>> fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, >>>> this wouldn't work. >>> Ouch. I wonder when this happens --- have you checked what's the primary >>> there? I guess it might be if it's a PCI device without the corresponding >>> ACPI device node? >>> >>> I remember you had an is_available implementation that just returned true >>> for software nodes in an early version of the set? I think it would still >>> be a lesser bad in this case. >> How about the following ? > Looks good to me. If we're agreed on this (and it's fine by me too), do you want me to include it in the next set, or are you going to do it separately Laurent? >> diff --git a/drivers/base/property.c b/drivers/base/property.c >> index 81bd01ed4042..ea44ba846299 100644 >> --- a/drivers/base/property.c >> +++ b/drivers/base/property.c >> @@ -706,9 +706,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); >> /** >> * fwnode_device_is_available - check if a device is available for use >> * @fwnode: Pointer to the fwnode of the device. >> + * >> + * For fwnode node types that don't implement the .device_is_available() >> + * operation, such as software nodes, this function returns true. >> */ >> bool fwnode_device_is_available(const struct fwnode_handle *fwnode) >> { >> + if (!fwnode_has_op(fwnode, device_is_available)) >> + return true; >> return fwnode_call_bool_op(fwnode, device_is_available); >> } >> EXPORT_SYMBOL_GPL(fwnode_device_is_available); >>
Hi Dan, On Sat, Oct 24, 2020 at 05:33:32PM +0100, Dan Scally wrote: > On 24/10/2020 15:29, Sakari Ailus wrote: > > On Sat, Oct 24, 2020 at 03:39:55AM +0300, Laurent Pinchart wrote: > >> On Wed, Oct 21, 2020 at 01:49:10AM +0300, Sakari Ailus wrote: > >>> On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote: > >>>> On 20/10/2020 13:06, Sakari Ailus wrote: > >>>>> On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: > >>>>>> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: > >>>>>>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices > >>>>>>> only; that status being determined through the .device_is_available() op > >>>>>>> of the device's fwnode. As software_nodes don't have that operation and > >>>>>>> adding it is meaningless, we instead need to check if the device's fwnode > >>>>>>> is a software_node and if so pass the appropriate flag to disable that > >>>>>>> check > >>>>>> > >>>>>> Period. > >>>>>> > >>>>>> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). > >>>>> > >>>>> The device availability test is actually there for a reason. Some firmware > >>>>> implementations put all the potential devices in the tables and only one > >>>>> (of some) of them are available. > >>>>> > >>>>> Could this be implemented so that if the node is a software node, then get > >>>>> its parent and then see if that is available? > >>>>> > >>>>> I guess that could be implemented in software node ops. Any opinions? > >>>> > >>>> Actually when considering the cio2 device, it seems that > >>>> set_secondary_fwnode() actually overwrites the _primary_, given > >>>> fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, > >>>> this wouldn't work. > >>> > >>> Ouch. I wonder when this happens --- have you checked what's the primary > >>> there? I guess it might be if it's a PCI device without the corresponding > >>> ACPI device node? > >>> > >>> I remember you had an is_available implementation that just returned true > >>> for software nodes in an early version of the set? I think it would still > >>> be a lesser bad in this case. > >> > >> How about the following ? > > > > Looks good to me. > > If we're agreed on this (and it's fine by me too), do you want me to > include it in the next set, or are you going to do it separately Laurent? Feel free to include it in the next version, but I can send a patch if you prefer. > >> diff --git a/drivers/base/property.c b/drivers/base/property.c > >> index 81bd01ed4042..ea44ba846299 100644 > >> --- a/drivers/base/property.c > >> +++ b/drivers/base/property.c > >> @@ -706,9 +706,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); > >> /** > >> * fwnode_device_is_available - check if a device is available for use > >> * @fwnode: Pointer to the fwnode of the device. > >> + * > >> + * For fwnode node types that don't implement the .device_is_available() > >> + * operation, such as software nodes, this function returns true. > >> */ > >> bool fwnode_device_is_available(const struct fwnode_handle *fwnode) > >> { > >> + if (!fwnode_has_op(fwnode, device_is_available)) > >> + return true; > >> return fwnode_call_bool_op(fwnode, device_is_available); > >> } > >> EXPORT_SYMBOL_GPL(fwnode_device_is_available);
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c index 4e598e937..f68ef0f6b 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c @@ -1466,6 +1466,7 @@ static const struct v4l2_async_notifier_operations cio2_async_ops = { static int cio2_parse_firmware(struct cio2_device *cio2) { + unsigned long allow_disabled; unsigned int i; int ret; @@ -1474,11 +1475,15 @@ static int cio2_parse_firmware(struct cio2_device *cio2) .bus_type = V4L2_MBUS_CSI2_DPHY }; struct sensor_async_subdev *s_asd = NULL; + struct fwnode_handle *fwnode; struct fwnode_handle *ep; + fwnode = dev_fwnode(&cio2->pci_dev->dev); + allow_disabled = is_software_node(fwnode) ? FWNODE_GRAPH_DEVICE_DISABLED : 0; + ep = fwnode_graph_get_endpoint_by_id( - dev_fwnode(&cio2->pci_dev->dev), i, 0, - FWNODE_GRAPH_ENDPOINT_NEXT); + fwnode, i, 0, + FWNODE_GRAPH_ENDPOINT_NEXT | allow_disabled); if (!ep) continue;
fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices only; that status being determined through the .device_is_available() op of the device's fwnode. As software_nodes don't have that operation and adding it is meaningless, we instead need to check if the device's fwnode is a software_node and if so pass the appropriate flag to disable that check Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes in v3: - patch introduced drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)