Re: [RFC PATCH 2/4] drivers/bus: Freescale Management Complex (fsl-mc) bus driver

[German Rivera <German.Rivera@freescale.com>]

Hi Arnd,

I have posted respin v3 of this patch series to address your lastest  of 
comments. Please see below the resolutions.

Thanks,

German

On 08/21/2014 06:30 AM, Arnd Bergmann wrote:
> On Tuesday 19 August 2014, German Rivera wrote:
>>>> + * @dev_node: Node in the container's child list
>>>
>>> Same here: just use the device model's list management instead if you can,
>>> or explain why this is needed.
>>>
>> We still need to keep a per-bus list of child devices (devices contained
>> in a given DPRC object). Unless I'm missing something,
>> I think the device model's list management links together all the
>> devices of the same bus type. We are trying to follow a similar approach
>> to the pci_dev/pci_bus structs.
>
> There are multiple lists in the device handling. device_for_each_child()
> should iterate over the children of a particular device using the
> klist_children member.
>
Removed per-bus list of children, and instead use 
device_for_each_child() as you suggested.

>>>> +/**
>>>> + * struct fsl_mc_dprc - Data Path Resource Container (DPRC) object
>>>> + * @magic: marker to verify identity of this structure
>>>> + * @mc_dev: pointer to MC object device object for this DPRC
>>>> + * @mutex: mutex to serialize access to the container.
>>>> + * @child_device_count: have the count of devices in this DPRC
>>>> + * @child_list:     anchor node of list of child devices on this DPRC
>>>> + */
>>>> +struct fsl_mc_dprc {
>>>> +#   define FSL_MC_DPRC_MAGIC   FSL_MC_MAGIC('D', 'P', 'R', 'C')
>>>> +    uint32_t magic;
>>>> +    struct fsl_mc_device *mc_dev;
>>>> +    struct mutex mutex;     /* serializes access to fields below */
>>>> +    uint16_t child_device_count;    /* Count of devices in this DPRC */
>>>> +    struct list_head child_list;
>>>> +};
>>>
>>> It's not clear what this represents to me. mc_dev presumably already
>>> has a list of children, so why not just use a pointer to mc_dev
>>> and remove this structure entirely?
>>>
>> This structure represents the per-bus (per DPRC object) information.
>> It is kind of the equivalent to 'struct pci_bus' in the PCI world.
>> I have renamed this struct to 'struct fsl_mc_bus'.
>
> Ok, I'll look at the new version when I get back to Germany. I still think
> that can remove all members of the current structure and just use the
> same structure for fsl_mc_bus and fsl_mc_device. If you really need
> a small number of extra members beyond what is in the device, you have
> two other choices:
>
By removing the child list from the fsl_mc_bus structure as you 
suggested, the fsl_mc_bus structure does not need to exist for this 
patch series. As you rightfully suggested, we can use just one
structure (fsl_mc_device) to represent both regular devices (children)
and bus devices.

> a) put the members into the device structure as well but not use them
>     for a device that is not a bus
>
> b) embed the device structure within the bus structure like
>
> 	struct fsl_mc_bus {
> 		int something;
> 		struct fsl_mc_device;
> 	};
>
> and then use container_of() to go from the device to the bus where needed
> rather than having two objects that are allocated separately. This is
> what a lot of other subsystems (not PCI) do. See for instance
> platform_device, which often has child devices as well.
>
In other functionality to be delivered as a follow-on patch series
(after this patch series), we will need to track some per-bus 
information, and we will do so using your "b)" recommendation.

> 	Arnd
>