On 9/23/2020 8:35 AM, Loic Poulain wrote:
Hi Jeffrey,
On Wed, 23 Sep 2020 at 16:04, Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx> wrote:
On 9/23/2020 7:40 AM, Loic Poulain wrote:
This is a generic MHI-over-PCI controller driver for MHI only devices
such as QCOM modems. For now it supports registering of Qualcomm SDX55
based PCIe modules. The MHI channels have been extracted from mhi
downstream driver.
This driver is easily extendable to support other MHI PCI devices like
different modem hw or OEM superset.
Maybe I'm being a bit dense, but what does this "driver" even do?
Ok, it'll bind to SDX55 and "power up" the MHI. I don't see any
communication with the device, so I'm not really seeing the value here.
Feels like this should be patch 1 of a series, but patches 2 - X are
"missing".
Well, yes and no. On MHI controller side point-of-view, the driver is
quite complete, since its only goal is to implement the MHI transport
layer and to register the available channels. The PCI driver is really
no expected to do more (contrary to ath11k), everything else will be
implemented by MHI device drivers at upper level. I agree most of them
are not yet implemented (only qrtr-mhi for IPCR channel), but I'm
currently working on this.
Hmm. I guess I wonder why the functionality provided by this patch
isn't incorporated into some other driver. I see why its not really a
great idea to pick a random client driver (like ipc router) and push the
responsibility into them. However, do we hook up these external modems
to remoteproc? If we are managing the devices somewhere else (for FW
loading, SSR, etc), then it would seem like a good place for this
functionality.
In summary, this feels like a shim driver that is a driver for driver's
sake, which doesn't feel like the right design. I don't think I have a
better suggestion through. Since I don't have a concrete "this should
be done some other way" I won't be offended if you ignore this "complaint".
However, assuming you continue with this approach, I think this change
needs more documentation. At a minimum I think what you just explained
to me needs to be in the commit text.
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
