On Tue, Oct 06, 2020 at 07:15:49PM +0000, Ben Levinsky wrote:

Hi Michael,

Thanks for the review

< ... snip ... >

quoted

quoted

+	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