On Wed, 23 Sep 2020 at 16:55, Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx> wrote: > > 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. I understand what you mean, but here this driver is a (logical) controller driver for the MHI bus, in the same way as e.g. I2C adapter driver is a controller for the I2C bus, then MHI device drivers (e.g. ipc router) implement the device function/class like e.g. I2C client driver (though you could argue that MHI channels are not fully independant devices). The device is not managed from somewhere else and the firmware loading is a generic piece of the MHI subsystem/bus. Moreover there is currently no modem/wwan subsystem in Linux, so there is not really something else to register with... at least for now. > 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. I will. Thanks, Loic
