> -----Original Message-----
> From: Bjorn Andersson <[email protected]>
> Sent: Thursday, December 18, 2025 8:24 PM
> To: Shenwei Wang <[email protected]>
> Cc: Linus Walleij <[email protected]>; Bartosz Golaszewski <[email protected]>;
> Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>;
> Conor Dooley <[email protected]>; Mathieu Poirier
> <[email protected]>; Shawn Guo <[email protected]>; Sascha
> Hauer <[email protected]>; Jonathan Corbet <[email protected]>;
> Pengutronix Kernel Team <[email protected]>; Fabio Estevam
> <[email protected]>; Peng Fan <[email protected]>; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>
> Subject: [EXT] Re: [PATCH v6 2/5] remoteproc: imx_rproc: Populate devices
> under "rpmsg" subnode
>
> Caution: This is an external email. Please take care when clicking links or
> opening
> attachments. When in doubt, report the message using the 'Report this email'
> button
>
>
> On Fri, Dec 12, 2025 at 01:43:38PM -0600, Shenwei Wang wrote:
> > Register the RPMsg channel driver and populate remote devices defined
> > under the "rpmsg" subnode upon receiving their notification messages.
>
> Please provide a proper description of what "problem" this patch solves.
>
> >
> > The following illustrates the expected DTS layout structure:
> >
> > cm33: remoteproc-cm33 {
> > compatible = "fsl,imx8ulp-cm33";
> >
> > rpmsg {
> > rpmsg-io-channel {
> > gpio@0 {
> > compatible = "fsl,imx-rpmsg-gpio";
> > reg = <0>;
>
> Surely there needs to be some "gpio-controller" and "#gpio-cells" here?
> Would be useful if the example is somewhat complete, to give a picture of
> what's
> actually going on.
>
Okay. Will add those in next version.
> > };
> >
> > gpio@1 {
> > compatible = "fsl,imx-rpmsg-gpio";
> > reg = <1>;
> > };
> >
> > ...
> > };
> >
> > ...
> > };
> > };
> >
> > + drvdata->rpdev = rpdev;
> > + auxdata[0].compatible = devm_kstrdup(dev, imx_rpdrv->compat,
> GFP_KERNEL);
> > + auxdata[0].platform_data = drvdata;
> > + dev_set_drvdata(dev, drvdata);
> > +
> > + of_platform_populate(drvdata->channel_node, NULL, auxdata, dev);
>
> auxiliary_bus would be a better choice, but I don't understand why you probe a
> rpmsg_device for each "gpio channel" and then from that create a
> platform_device.
>
> Why don't you just make the rpmsg_device register the gpio controller
> directly?
>
The "GPIO channel" is just one example-there are also "PWM channel", "I2C
channel", and other channels.
The goal is to manage all these channels under a common logic, which helps
avoid redundant code and keeps
the implementation consistent.
> > +
> > + return 0;
> > +}
> > +
> > + rp_driver->rpdrv.drv.name = name;
> > + rp_driver->rpdrv.id_table = rpdev_id;
> > + rp_driver->rpdrv.probe = imx_rpmsg_endpoint_probe;
> > + rp_driver->rpdrv.remove = imx_rpmsg_endpoint_remove;
> > + rp_driver->rpdrv.callback = imx_rpmsg_endpoint_cb;
> > + rp_driver->driver_data = driver_data;
> > + rp_driver->compat = compat;
> > +
> > + register_rpmsg_driver(&rp_driver->rpdrv);
>
> This would then also imply that it's the gpio driver that registers the
> rpmsg_driver.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int rproc_of_rpmsg_node_init(struct platform_device *pdev) {
> > + struct device *dev = &pdev->dev;
> > + const char *compat;
> > + int ret;
> > +
> > + struct device_node *np __free(device_node) = of_get_child_by_name(dev-
> >of_node, "rpmsg");
> > + if (!np)
> > + return 0;
> > +
> > + for_each_child_of_node_scoped(np, child) {
> > + compat = imx_of_rpmsg_is_in_map(child->name);
> > + if (!compat)
> > + ret = of_platform_default_populate(child, NULL,
> > + dev);
>
> So if you don't recognize the child device node name you just register
> platform_devices for each of the children?
>
Yes. That would register platform_devices without the platform_data.
> > + else
> > + ret = imx_of_rpmsg_register_rpdriver(child, dev,
> > + child->name, compat);
> > +
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int imx_rproc_probe(struct platform_device *pdev) {
> > struct device *dev = &pdev->dev; @@ -1114,6 +1253,10 @@ static
> > int imx_rproc_probe(struct platform_device *pdev)
> > goto err_put_pm;
> > }
> >
> > + ret = rproc_of_rpmsg_node_init(pdev);
> > + if (ret < 0)
> > + dev_info(dev, "populating 'rpmsg' node failed\n");
> > +
> > return 0;
> >
> > err_put_pm:
> > diff --git a/include/linux/rpmsg/rpdev_info.h
> > b/include/linux/rpmsg/rpdev_info.h
> > new file mode 100644
> > index 000000000000..13e020cd028b
> > --- /dev/null
> > +++ b/include/linux/rpmsg/rpdev_info.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright 2025 NXP */
> > +
> > +/*
> > + * @file linux/rpdev_info.h
> > + *
> > + * @brief Global header file for RPDEV Info
> > + *
> > + * @ingroup RPMSG
> > + */
> > +#ifndef __LINUX_RPDEV_INFO_H__
> > +#define __LINUX_RPDEV_INFO_H__
> > +
> > +#define MAX_DEV_PER_CHANNEL 10
> > +
> > +/**
> > + * rpdev_platform_info - store the platform information of rpdev
> > + * @rproc_name: the name of the remote proc.
> > + * @rpdev: rpmsg channel device
> > + * @device_node: pointer to the device node of the rpdev.
> > + * @rx_callback: rx callback handler of the rpdev.
> > + * @channel_devices: an array of the devices related to the rpdev.
> > + */
> > +struct rpdev_platform_info {
>
> I don't understand what this structure represents. Why is this glue between
> the
> rpmsg_device and a made up platform_device needed?
>
The purpose is to have a shared array that can be accessed by all devices
within
the same channel.
> > + const char *rproc_name;
>
> You don't need this, because you can rproc_get_by_child(&self) and then get
> the
> remoteproc name from that.
>
Good to know. Will try it in the next version.
> > + struct rpmsg_device *rpdev;
> > + struct device_node *channel_node;
> > + int (*rx_callback)(struct rpmsg_device *rpdev, void *data,
> > + int len, void *priv, u32 src);
> > + void *channel_devices[MAX_DEV_PER_CHANNEL];
>
> Why 10? What does it mean?
>
This is based on practical experience. For example, on the i.MX platform, we
typically don't have
more than eight same devices on the remote system.
> I think this becomes the list of the 10 grandchildren of the remoteproc (per
> child
> node). So what happens if those matches against two different drivers, what
> will
> rx_callback point to?
>
This is the limitation. That's why I used the map to populate the known child
device for one specific channel.
Thanks,
Shenwei
> > +};
>
> Regards,
> Bjorn
>
> > +
> > +#endif /* __LINUX_RPDEV_INFO_H__ */
> > --
> > 2.43.0
> >
