Re: [PATCH] bus: mhi: Add MHI PCI support

[Jeffrey Hugo <jhugo@codeaurora.org>]

On 9/23/2020 8:35 AM, Loic Poulain wrote:
> Hi Jeffrey,
> 
> On Wed, 23 Sep 2020 at 16:04, Jeffrey Hugo <jhugo@codeaurora.org> 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.