Hi Ohad, >>> It might be safer though to invoke ->setup() in probe/remove, instead >>> of in start/stop (just like you essentially did before). >>> >>> This way we don't assume that stop is always called before remove (as >>> assumption that might be implicitly valid today, but may break in the >>> future). >>> >>> If you want, I can just change that before applying. >> >> Sounds good, please just go ahead and change this before applying. >> Thank you. > > I'm going to squash the below into the patch - please ack before I > apply and push - thanks!
Looks good, thanks. Acked-by: Sjur Brændeland <sjur.brandel...@stericsson.om> > > diff --git a/drivers/remoteproc/ste_modem_rproc.c > b/drivers/remoteproc/ste_modem_rproc.c > index fcc8677..a7743c0 100644 > --- a/drivers/remoteproc/ste_modem_rproc.c > +++ b/drivers/remoteproc/ste_modem_rproc.c > @@ -190,9 +190,6 @@ static int sproc_start(struct rproc *rproc) > > sproc_dbg(sproc, "start ste-modem\n"); > > - /* Provide callback functions to modem device */ > - sproc->mdev->ops.setup(sproc->mdev, &sproc_dev_cb); > - > /* Sanity test the max_notifyid */ > if (rproc->max_notifyid > SPROC_MAX_NOTIFY_ID) { > sproc_err(sproc, "Notification IDs too high:%d\n", > @@ -220,9 +217,6 @@ static int sproc_stop(struct rproc *rproc) > struct sproc *sproc = rproc->priv; > sproc_dbg(sproc, "stop ste-modem\n"); > > - /* Reset device callback functions */ > - sproc->mdev->ops.setup(sproc->mdev, NULL); > - > return sproc->mdev->ops.power(sproc->mdev, false); > } > > @@ -241,6 +235,9 @@ static int sproc_drv_remove(struct platform_device *pdev) > > sproc_dbg(sproc, "remove ste-modem\n"); > > + /* Reset device callback functions */ > + sproc->mdev->ops.setup(sproc->mdev, NULL); > + > /* Unregister as remoteproc device */ > rproc_del(sproc->rproc); > rproc_put(sproc->rproc); > @@ -260,6 +257,13 @@ static int sproc_probe(struct platform_device *pdev) > int err; > > dev_dbg(&mdev->pdev.dev, "probe ste-modem\n"); > + > + if (!mdev->ops.setup || !mdev->ops.kick || !mdev->ops.kick_subscribe > || > + !mdev->ops.power) { > + dev_err(&mdev->pdev.dev, "invalid mdev ops\n"); > + return -EINVAL; > + } > + > rproc = rproc_alloc(&mdev->pdev.dev, mdev->pdev.name, &sproc_ops, > SPROC_MODEM_FIRMWARE, sizeof(*sproc)); > if (!rproc) > @@ -270,6 +274,9 @@ static int sproc_probe(struct platform_device *pdev) > sproc->rproc = rproc; > mdev->drv_data = sproc; > > + /* Provide callback functions to modem device */ > + sproc->mdev->ops.setup(sproc->mdev, &sproc_dev_cb); > + > /* Set the STE-modem specific firmware handler */ > rproc->fw_ops = &sproc_fw_ops; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/