Hi Suman,
On Mon, Mar 30, 2020 at 03:38:14PM -0500, Suman Anna wrote:
> Hi Mathieu,
>
> On 3/27/20 6:10 AM, Loic PALLARDY wrote:
> > Hi Mathieu,
> >
> >>
> >> In preparation to allocate the synchronisation operation and state
> >> machine, spin off a new function in order to keep rproc_alloc() as
> >> clean as possible.
> >>
> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> >> ---
> >> drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++---
> >> -
> >> 1 file changed, 22 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_core.c
> >> b/drivers/remoteproc/remoteproc_core.c
> >> index ee277bc5556c..9da245734db6 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -2018,6 +2018,26 @@ static int rproc_alloc_ops(struct rproc *rproc,
> >> const struct rproc_ops *ops)
> >> return 0;
> >> }
> >>
> >> +static int rproc_alloc_internals(struct rproc *rproc, const char *name,
> >> + const struct rproc_ops *boot_ops,
> >> + const char *firmware, int len)
> >
> > len argument is not used in the patch nor in the following, maybe removed from my pov.
>
> Indeed.
>
> >
> > Regards,
> > Loic
>
> >> +{
> >> + int ret;
> >> +
> >> + /* We have a boot_ops so allocate firmware name and operations */
> >> + if (boot_ops) {
> >> + ret = rproc_alloc_firmware(rproc, name, firmware);
> >> + if (ret)
> >> + return ret;
>
> So, can you explain why firmware allocation now becomes conditional on
> this boot_ops?
There is no point in allocating a firmware name in a scenario where the
remoteproc core is only synchronising with the MCU. As soon as a boot_ops (to
be renamed ops as per your comment below) is present I assume firmware loading
will be involved at some point. Do you see a scenario where that wouldn't be
be case?
>
> Perhaps, continue to call this as ops following the field name in struct
> rproc.
Ok
>
> regards
> Suman
>
> >> +
> >> + ret = rproc_alloc_ops(rproc, boot_ops);
> >> + if (ret)
> >> + return ret;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> /**
> >> * rproc_alloc() - allocate a remote processor handle
> >> * @dev: the underlying device
> >> @@ -2064,10 +2084,8 @@ struct rproc *rproc_alloc(struct device *dev, const
> >> char *name,
> >> rproc->dev.class = &rproc_class;
> >> rproc->dev.driver_data = rproc;
> >>
> >> - if (rproc_alloc_firmware(rproc, name, firmware))
> >> - goto out;
> >> -
> >> - if (rproc_alloc_ops(rproc, ops))
> >> + if (rproc_alloc_internals(rproc, name, ops,
> >> + firmware, len))
> >> goto out;
> >>
> >> /* Assign a unique device index and name */
> >> --
> >> 2.20.1
> >
>
