Re: [PATCH v5 2/5] remoteproc: imx_rproc: Populate devices under "rpmsg" subnode

[Peng Fan <peng.fan@oss.nxp.com>]

Hi Shenwei,

It looks good to me, only a few minor comments.

On Tue, Nov 04, 2025 at 02:33:12PM -0600, Shenwei Wang wrote:
>Register the RPMsg channel driver and populate remote devices defined
>under the "rpmsg" subnode upon receiving their notification messages.
>
>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>;
>				};
>
>				gpio@1 {
>					compatible = "fsl,imx-rpmsg-gpio";
>					reg = <1>;
>				};

I think better drop "imx" 

>
>				...
>			};
>
>			rpmsg-i2c-channel {
>				i2c@0 {
>					compatible = "fsl,imx-rpmsg-i2c";
>					reg = <0>;

i2c channel could be removed, it is not included in this patchset.

>				};
>			};
>
>			...
>+

....

>+static char *channel_device_map[][2] = {
>+	{"rpmsg-io-channel", "fsl,imx-rpmsg-gpio"},
>+	{"rpmsg-i2c-channel", "fsl,imx-rpmsg-i2c"},

only keep gpio and drop imx.

>+};
>+
>+static int imx_rpmsg_endpoint_cb(struct rpmsg_device *rpdev, void *data,
>+				 int len, void *priv, u32 src)
>+{
>+	struct imx_rpmsg_driver_data *drvdata;
>+
>+	drvdata = dev_get_drvdata(&rpdev->dev);
>+	if (drvdata && drvdata->rx_callback)
>+		return drvdata->rx_callback(rpdev, data, len, priv, src);
>+
>+	return 0;
>+}
>+
>+static void imx_rpmsg_endpoint_remove(struct rpmsg_device *rpdev)
>+{
>+	of_platform_depopulate(&rpdev->dev);
>+}
>+
>+static int imx_rpmsg_endpoint_probe(struct rpmsg_device *rpdev)
>+{
>+	struct imx_rpmsg_driver_data *drvdata;
>+	struct imx_rpmsg_driver *imx_rpdrv;
>+	struct device *dev = &rpdev->dev;
>+	struct of_dev_auxdata *auxdata;
>+	struct rpmsg_driver *rpdrv;
>+	int i;
>+
>+	rpdrv = container_of(dev->driver, struct rpmsg_driver, drv);
>+	imx_rpdrv = container_of(rpdrv, struct imx_rpmsg_driver, rpdrv);
>+
>+	if (!imx_rpdrv->driver_data)
>+		return -EINVAL;
>+
>+	drvdata = devm_kmemdup(dev, imx_rpdrv->driver_data, sizeof(*drvdata), GFP_KERNEL);
>+	if (!drvdata)
>+		return -ENOMEM;
>+
>+	i = drvdata->map_idx;
>+	if (i >= ARRAY_SIZE(channel_device_map))
>+		return -ENODEV;
>+
>+	auxdata = devm_kzalloc(dev, sizeof(*auxdata) * 2, GFP_KERNEL);
>+	if (!auxdata)
>+		return -ENOMEM;
>+
>+	drvdata->rpdev = rpdev;
>+	auxdata[0].compatible = channel_device_map[i][1];
>+	auxdata[0].platform_data = drvdata;
>+	dev_set_drvdata(dev, drvdata);
>+
>+	of_platform_populate(drvdata->channel_node, NULL, auxdata, dev);
>+	of_node_put(drvdata->channel_node);
>+
>+	return 0;
>+}
>+
>+static int imx_of_rpmsg_is_in_map(const char *name)
>+{
>+	int i;
>+
>+	for (i = 0; i < ARRAY_SIZE(channel_device_map); i++) {
>+		if (strcmp(name, channel_device_map[i][0]) == 0)
>+			return i;
>+	}
>+
>+	return -1;
>+}
>+
>+static int imx_of_rpmsg_register_rpdriver(struct device_node *channel,
>+					  struct device *dev, int idx)
>+{
>+	struct imx_rpmsg_driver_data *driver_data;
>+	struct imx_rpmsg_driver *rp_driver;
>+	struct rpmsg_device_id *rpdev_id;
>+
>+	rpdev_id = devm_kzalloc(dev, sizeof(*rpdev_id) * 2, GFP_KERNEL);
>+	if (!rpdev_id)
>+		return -ENOMEM;
>+
>+	strscpy(rpdev_id[0].name, channel_device_map[idx][0], RPMSG_NAME_SIZE);
>+
>+	rp_driver = devm_kzalloc(dev, sizeof(*rp_driver), GFP_KERNEL);
>+	if (!rp_driver)
>+		return -ENOMEM;
>+
>+	driver_data = devm_kzalloc(dev, sizeof(*driver_data), GFP_KERNEL);
>+	if (!driver_data)
>+		return -ENOMEM;
>+
>+	driver_data->rproc_name = dev->of_node->name;
>+	driver_data->channel_node = channel;
>+	driver_data->map_idx = idx;
>+
>+	rp_driver->rpdrv.drv.name = channel_device_map[idx][0];
>+	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;
>+
>+	register_rpmsg_driver(&rp_driver->rpdrv);
>+
>+	return 0;
>+}
>+
>+static int imx_of_rpmsg_node_init(struct platform_device *pdev)
>+{
>+	struct device_node *np __free(device_node);
>+	struct device *dev = &pdev->dev;
>+	int idx, ret;
>+
>+	np = of_get_child_by_name(dev->of_node, "rpmsg");
>+	if (!np)
>+		return 0;
>+
>+	for_each_child_of_node_scoped(np, child) {
>+		idx = imx_of_rpmsg_is_in_map(child->name);
>+		if (idx < 0)
>+			ret = of_platform_default_populate(child, NULL, dev);
>+		else
>+			ret = imx_of_rpmsg_register_rpdriver(child, dev, idx);
>+
>+		if (ret < 0)
>+			return ret;
>+	}
>+
>+	return 0;
>+}
>+
> static int imx_rproc_probe(struct platform_device *pdev)
> {
> 	struct device *dev = &pdev->dev;
>@@ -1177,6 +1319,10 @@ static int imx_rproc_probe(struct platform_device *pdev)
> 		goto err_put_clk;
> 	}
> 
>+	ret = imx_of_rpmsg_node_init(pdev);
>+	if (ret < 0)
>+		dev_info(dev, "populating 'rpmsg' node failed\n");

In best case to make this generic enough, an generic function
could be provided saying rproc_of_rpmsg_node_init() in remoteproc core
or rpmsg core. But for now, it should be ok to keep it in imx_rproc.c.

Regards
Peng