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

Jeffrey Hugo
2020-11-24T17:38:01Z

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.