Re: [RFC PATCH v3 7/9] ipu3-cio2: Check if pci_dev->dev's fwnode is a software_node in cio2_parse_firmware() and set FWNODE_GRAPH_DEVICE_DISABLED if so

[Dan Scally]

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);
>>