| Message ID | 1606234711-20125-1-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] bus: mhi: core: Fix device hierarchy issue | expand |
On Tue 24 Nov 10:18 CST 2020, 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. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> I like this, but have a nagging feeling that we discussed this during the upstreaming of MHI. Mani, do you remember? Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > --- > 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; > > -- > 2.7.4 >
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. > > 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.
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). 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.
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.
On Tue, Nov 24, 2020 at 05:18:31PM +0100, 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. > Change looks good to me but as Jeff said, please elaborate the commit message to justify the reasoning behind this. Thanks, Mani > 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; > > -- > 2.7.4 >
On Tue, 24 Nov 2020 at 18:37, Jeffrey Hugo <jhugo@codeaurora.org> wrote: > > 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. Yes, thanks, going to reword the commit message. Loic
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;
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. 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(-) -- 2.7.4