On Tue, Oct 06, 2020 at 07:15:49PM +0000, Ben Levinsky wrote: > > Hi Michael, > > Thanks for the review > < ... snip ... > > > > + z_rproc = rproc->priv; > > > + z_rproc->dev.release = zynqmp_r5_release; > > > > This is the only field of z_rproc->dev that's actually initialized, and > > this device is not registered with the core at all, so zynqmp_r5_release > > will never be called. > > > > Since it doesn't look like there's a need to create this additional > > device, I'd suggest: > > - Dropping the struct device from struct zynqmp_r5_rproc > > - Performing the necessary cleanup in the driver remove > > callback instead of trying to tie it to device release > > For the most part I agree. I believe the device is still needed for > the mailbox client setup. > > As the call to mbox_request_channel_byname() requires its own device > that has the corresponding child node with the corresponding > mbox-related properties. > > With that in mind, is it still ok to keep the device node? Ah, I see. Thanks for the clarification! Instead of manually dealing with the device node creation for the individual processors, perhaps it makes more sense to use devm_of_platform_populate() to create them. This is also consistent with the way the TI K3 R5F remoteproc driver does things. Cheers, Michael
