On 2/17/22 4:20 AM, Manivannan Sadhasivam wrote:
On Tue, Feb 15, 2022 at 02:02:50PM -0600, Alex Elder wrote:
[...]
+static int mhi_ep_driver_remove(struct device *dev)
+{
+ struct mhi_ep_device *mhi_dev = to_mhi_ep_device(dev);
+ struct mhi_ep_driver *mhi_drv = to_mhi_ep_driver(dev->driver);
+ struct mhi_result result = {};
+ struct mhi_ep_chan *mhi_chan;
+ int dir;
+
+ /* Skip if it is a controller device */
+ if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+ return 0;
+
It would be my preference to encapsulate the body of the
following loop into a called function, then call that once
for the UL channel and once for the DL channel.
This follows the host stack, so I'd like to keep it the same.
I think you should change both, but I'll leave that up to you.
+ /* Disconnect the channels associated with the driver */
+ for (dir = 0; dir < 2; dir++) {
+ mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
+
+ if (!mhi_chan)
+ continue;
+
+ mutex_lock(&mhi_chan->lock);
+ /* Send channel disconnect status to the client driver */
+ if (mhi_chan->xfer_cb) {
+ result.transaction_status = -ENOTCONN;
+ result.bytes_xferd = 0;
+ mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result);
It appears the result is ignored here. If so, can we
define the xfer_cb() function so that a NULL pointer may
be supplied by the caller in cases like this?
result is not ignored, only the bytes_xfered. "transaction_status" will
be used by the client drivers for error handling.
Sorry, I was looking at the code *after* the call, and was
ignoring that it was information being passed in... My
mistake.
+ }
+
+ /* Set channel state to DISABLED */
That comment is a little tautological. Just omit it.
+ mhi_chan->state = MHI_CH_STATE_DISABLED;
+ mhi_chan->xfer_cb = NULL;
+ mutex_unlock(&mhi_chan->lock);
+ }
+
+ /* Remove the client driver now */
+ mhi_drv->remove(mhi_dev);
+
+ return 0;
+}
[...]
+struct mhi_ep_driver {
+ const struct mhi_device_id *id_table;
+ struct device_driver driver;
+ int (*probe)(struct mhi_ep_device *mhi_ep,
+ const struct mhi_device_id *id);
+ void (*remove)(struct mhi_ep_device *mhi_ep);
I get confused by the "ul" versus "dl" naming scheme here.
Is "ul" from the perspective of the host, meaning upload
is from the host toward the WWAN network (and therefore
toward the SDX AP), and download is from the WWAN toward
the host? Somewhere this should be stated clearly in
comments; maybe I just missed it.
Yes UL and DL are as per host context. I didn't state this explicitly
since this is the MHI host stack behaviour but I'll add a comment for
clarity
Sounds good, thanks.
-Alex
Thanks,
Mani
