Re: [PATCH v2] bus: mhi: core: Fix device hierarchy issue

[Jeffrey Hugo <jhugo@codeaurora.org>]

On 11/24/2020 9:57 AM, Loic Poulain wrote:
> On Tue, 24 Nov 2020 at 17:36, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>>
>> On 11/24/2020 9:18 AM, Loic Poulain wrote:
>>> A MHI client device should be child of the MHI controller device.
>>> Today both MHI controller and its MHI clients are direct children
>>> of the same bus device. This patch fixes the hierarchy.
>>
>> Why?
>>
>> I'm not particularly arguing for or against this change (I think it
>> affects me slightly, but not in a breaking way), but this commit text
>> seems pretty generic.  It doesn't really help me understand the
>> relevance of this change.  It seems to be only describing what you are
>> doing, but not the why.  How did you find this?  How does this affect
>> the client drivers?  Does it make something the client drivers care
>> about better?
>>
>> To put this another way, "should" is an opinion, and you've provided no
>> facts to assert why your opinion is superior to others.
> 
> That's right I've not elaborate too much, but it's mainly to respect
> the hierarchy of devices, as it is done for other busses. The
> hierarchy is especially important for things like power management
> ordering (PM core must suspend devices before their controller, wakeup
> the controller before its devices...). Moreover it will also be useful
> for userspace (thanks to sysfs) to determine which devices are behind
> which controllers (and so determine if e.g. QMI and IP channels are
> part of the same device).

This sounds like two relevant usecases which should be mentioned in the 
commit text.

> 
> Regards,
> Loic
> 
> 
> 
>>
>>>
>>> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>>> ---
>>>    v2: fix commit message
>>>
>>>    drivers/bus/mhi/core/init.c | 10 +++++++++-
>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>>> index 436221c..c7a7354 100644
>>> --- a/drivers/bus/mhi/core/init.c
>>> +++ b/drivers/bus/mhi/core/init.c
>>> @@ -1137,7 +1137,15 @@ struct mhi_device *mhi_alloc_device(struct mhi_controller *mhi_cntrl)
>>>        device_initialize(dev);
>>>        dev->bus = &mhi_bus_type;
>>>        dev->release = mhi_release_device;
>>> -     dev->parent = mhi_cntrl->cntrl_dev;
>>> +
>>> +     if (mhi_cntrl->mhi_dev) {
>>> +             /* for MHI client devices, parent is the MHI controller device */
>>> +             dev->parent = &mhi_cntrl->mhi_dev->dev;
>>> +     } else {
>>> +             /* for MHI controller device, parent is the bus device (e.g. pci device) */
>>> +             dev->parent = mhi_cntrl->cntrl_dev;
>>> +     }
>>> +
>>>        mhi_dev->mhi_cntrl = mhi_cntrl;
>>>        mhi_dev->dev_wake = 0;
>>>
>>>
>>
>>
>> --
>> Jeffrey Hugo
>> Qualcomm Technologies, Inc. is a member of the
>> Code Aurora Forum, a Linux Foundation Collaborative Project.


-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.