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>;
};
...
};
rpmsg-i2c-channel {
i2c@0 {
compatible = "fsl,imx-rpmsg-i2c";
reg = <0>;
};
};
...
};
};
Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
drivers/remoteproc/imx_rproc.c | 146 ++++++++++++++++++++++++++++++++
include/linux/rpmsg/imx_rpmsg.h | 48 +++++++++++
2 files changed, 194 insertions(+)
create mode 100644 include/linux/rpmsg/imx_rpmsg.h
diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index a6eef0080ca9..e21a7980c490 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -8,6 +8,7 @@
#include <linux/clk.h>
#include <linux/err.h>
#include <linux/firmware/imx/sci.h>
+#include <linux/rpmsg/imx_rpmsg.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/mailbox_client.h>
@@ -15,6 +16,8 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
#include <linux/of_reserved_mem.h>
#include <linux/platform_device.h>
#include <linux/pm_domain.h>
@@ -22,6 +25,7 @@
#include <linux/reboot.h>
#include <linux/regmap.h>
#include <linux/remoteproc.h>
+#include <linux/rpmsg.h>
#include <linux/workqueue.h>
#include "imx_rproc.h"
@@ -1084,6 +1088,144 @@ static int imx_rproc_sys_off_handler(struct sys_off_data *data)
return NOTIFY_DONE;
}
+struct imx_rpmsg_driver {
+ struct rpmsg_driver rpdrv;
+ void *driver_data;
+};
+
+static char *channel_device_map[][2] = {
+ {"rpmsg-io-channel", "fsl,imx-rpmsg-gpio"},
+ {"rpmsg-i2c-channel", "fsl,imx-rpmsg-i2c"},
+};
+
+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");
+
return 0;
err_put_clk:
diff --git a/include/linux/rpmsg/imx_rpmsg.h b/include/linux/rpmsg/imx_rpmsg.h
new file mode 100644
index 000000000000..04a5ad2d4a1d
--- /dev/null
+++ b/include/linux/rpmsg/imx_rpmsg.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright 2025 NXP */
+
+/*
+ * @file linux/imx_rpmsg.h
+ *
+ * @brief Global header file for iMX RPMSG
+ *
+ * @ingroup RPMSG
+ */
+#ifndef __LINUX_IMX_RPMSG_H__
+#define __LINUX_IMX_RPMSG_H__
+
+/* Category define */
+#define IMX_RMPSG_LIFECYCLE 1
+#define IMX_RPMSG_PMIC 2
+#define IMX_RPMSG_AUDIO 3
+#define IMX_RPMSG_KEY 4
+#define IMX_RPMSG_GPIO 5
+#define IMX_RPMSG_RTC 6
+#define IMX_RPMSG_SENSOR 7
+
+/* rpmsg version */
+#define IMX_RMPSG_MAJOR 1
+#define IMX_RMPSG_MINOR 0
+
+#define MAX_DEV_PER_CHANNEL 10
+
+struct imx_rpmsg_head {
+ u8 cate; /* Category */
+ u8 major; /* Major version */
+ u8 minor; /* Minor version */
+ u8 type; /* Message type */
+ u8 cmd; /* Command code */
+ u8 reserved[5];
+} __packed;
+
+struct imx_rpmsg_driver_data {
+ int map_idx;
+ const char *rproc_name;
+ 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];
+};
+
+#endif /* __LINUX_IMX_RPMSG_H__ */
--
2.43.0On 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>;
> };
>
> ...
> };
>
> rpmsg-i2c-channel {
> i2c@0 {
> compatible = "fsl,imx-rpmsg-i2c";
> reg = <0>;
> };
> };
>
> ...
> };
> };
>
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
> drivers/remoteproc/imx_rproc.c | 146 ++++++++++++++++++++++++++++++++
> include/linux/rpmsg/imx_rpmsg.h | 48 +++++++++++
> 2 files changed, 194 insertions(+)
> create mode 100644 include/linux/rpmsg/imx_rpmsg.h
>
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index a6eef0080ca9..e21a7980c490 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -8,6 +8,7 @@
> #include <linux/clk.h>
> #include <linux/err.h>
> #include <linux/firmware/imx/sci.h>
> +#include <linux/rpmsg/imx_rpmsg.h>
> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> #include <linux/mailbox_client.h>
> @@ -15,6 +16,8 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> #include <linux/of_reserved_mem.h>
> #include <linux/platform_device.h>
> #include <linux/pm_domain.h>
> @@ -22,6 +25,7 @@
> #include <linux/reboot.h>
> #include <linux/regmap.h>
> #include <linux/remoteproc.h>
> +#include <linux/rpmsg.h>
> #include <linux/workqueue.h>
>
> #include "imx_rproc.h"
> @@ -1084,6 +1088,144 @@ static int imx_rproc_sys_off_handler(struct sys_off_data *data)
> return NOTIFY_DONE;
> }
>
> +struct imx_rpmsg_driver {
> + struct rpmsg_driver rpdrv;
> + void *driver_data;
> +};
> +
> +static char *channel_device_map[][2] = {
> + {"rpmsg-io-channel", "fsl,imx-rpmsg-gpio"},
> + {"rpmsg-i2c-channel", "fsl,imx-rpmsg-i2c"},
> +};
> +
> +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)
> +{
Where does the rpmsg_device come from? Usually there is a call to
rpmsg_register_device() and I don't see it anywhere in this patchset. I also
don't see a link to the remote processor. I can't continue with this set for as
long as I don't have this information.
> + 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");
> +
> return 0;
>
> err_put_clk:
> diff --git a/include/linux/rpmsg/imx_rpmsg.h b/include/linux/rpmsg/imx_rpmsg.h
> new file mode 100644
> index 000000000000..04a5ad2d4a1d
> --- /dev/null
> +++ b/include/linux/rpmsg/imx_rpmsg.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright 2025 NXP */
> +
> +/*
> + * @file linux/imx_rpmsg.h
> + *
> + * @brief Global header file for iMX RPMSG
> + *
> + * @ingroup RPMSG
> + */
> +#ifndef __LINUX_IMX_RPMSG_H__
> +#define __LINUX_IMX_RPMSG_H__
> +
> +/* Category define */
> +#define IMX_RMPSG_LIFECYCLE 1
> +#define IMX_RPMSG_PMIC 2
> +#define IMX_RPMSG_AUDIO 3
> +#define IMX_RPMSG_KEY 4
> +#define IMX_RPMSG_GPIO 5
> +#define IMX_RPMSG_RTC 6
> +#define IMX_RPMSG_SENSOR 7
> +
> +/* rpmsg version */
> +#define IMX_RMPSG_MAJOR 1
> +#define IMX_RMPSG_MINOR 0
> +
> +#define MAX_DEV_PER_CHANNEL 10
> +
> +struct imx_rpmsg_head {
> + u8 cate; /* Category */
> + u8 major; /* Major version */
> + u8 minor; /* Minor version */
> + u8 type; /* Message type */
> + u8 cmd; /* Command code */
> + u8 reserved[5];
> +} __packed;
> +
> +struct imx_rpmsg_driver_data {
> + int map_idx;
> + const char *rproc_name;
> + 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];
> +};
> +
> +#endif /* __LINUX_IMX_RPMSG_H__ */
> --
> 2.43.0
>
Good morning,
Here are some first-pass comments - more to come later.
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>;
> };
>
> ...
> };
>
> rpmsg-i2c-channel {
> i2c@0 {
> compatible = "fsl,imx-rpmsg-i2c";
> reg = <0>;
> };
> };
>
> ...
> };
> };
2 observations here:
(1) Why are you trying to introduce both i2c and GPIO in the same patchset?
(2) My first impression is that i2c, GPIO and others should be bundled with the
platform definitions for i2c and GPIO, with a phandle to the remote processor
they hang off from. My opinion may change once I have reviewed the other
patches in this set.
>
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
> drivers/remoteproc/imx_rproc.c | 146 ++++++++++++++++++++++++++++++++
> include/linux/rpmsg/imx_rpmsg.h | 48 +++++++++++
> 2 files changed, 194 insertions(+)
> create mode 100644 include/linux/rpmsg/imx_rpmsg.h
>
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index a6eef0080ca9..e21a7980c490 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -8,6 +8,7 @@
> #include <linux/clk.h>
> #include <linux/err.h>
> #include <linux/firmware/imx/sci.h>
> +#include <linux/rpmsg/imx_rpmsg.h>
> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> #include <linux/mailbox_client.h>
> @@ -15,6 +16,8 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> #include <linux/of_reserved_mem.h>
> #include <linux/platform_device.h>
> #include <linux/pm_domain.h>
> @@ -22,6 +25,7 @@
> #include <linux/reboot.h>
> #include <linux/regmap.h>
> #include <linux/remoteproc.h>
> +#include <linux/rpmsg.h>
> #include <linux/workqueue.h>
>
> #include "imx_rproc.h"
> @@ -1084,6 +1088,144 @@ static int imx_rproc_sys_off_handler(struct sys_off_data *data)
> return NOTIFY_DONE;
> }
>
> +struct imx_rpmsg_driver {
> + struct rpmsg_driver rpdrv;
> + void *driver_data;
> +};
> +
> +static char *channel_device_map[][2] = {
> + {"rpmsg-io-channel", "fsl,imx-rpmsg-gpio"},
> + {"rpmsg-i2c-channel", "fsl,imx-rpmsg-i2c"},
> +};
> +
> +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);
Something with the overall design of this feature doesn't work if you need to do
this.
> + 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);
Why is there a need for of_node_put() when imx_of_rprmsg_node_init() is using
for_each_child_of_node_scoped()?
> +
> + 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;
Please return a real error code.
> +}
> +
> +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);
I had a really good time figuring out why 2 instances were needed. Reviewing
this work without a single line of comment isn't motivating.
> + 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;
struct rpmsg_device_id already contains a @driver_data. As with my other
comment above, something in the design of this feature is not working if you
need to do something like this.
> +
> + 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);
Why is this called?
> + else
> + ret = imx_of_rpmsg_register_rpdriver(child, dev, idx);
> +
> + if (ret < 0)
> + return ret;
In case of an error, the reference to @np is not released.
> + }
> +
> + return 0;
> +}
> +
If we were to keep the current design, all of the above should be in
drivers/rpmsg/imx_rpmsg.c.
> 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");
> +
> return 0;
>
> err_put_clk:
> diff --git a/include/linux/rpmsg/imx_rpmsg.h b/include/linux/rpmsg/imx_rpmsg.h
> new file mode 100644
> index 000000000000..04a5ad2d4a1d
> --- /dev/null
> +++ b/include/linux/rpmsg/imx_rpmsg.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright 2025 NXP */
> +
> +/*
> + * @file linux/imx_rpmsg.h
> + *
> + * @brief Global header file for iMX RPMSG
> + *
> + * @ingroup RPMSG
> + */
> +#ifndef __LINUX_IMX_RPMSG_H__
> +#define __LINUX_IMX_RPMSG_H__
> +
> +/* Category define */
> +#define IMX_RMPSG_LIFECYCLE 1
> +#define IMX_RPMSG_PMIC 2
> +#define IMX_RPMSG_AUDIO 3
> +#define IMX_RPMSG_KEY 4
> +#define IMX_RPMSG_GPIO 5
> +#define IMX_RPMSG_RTC 6
> +#define IMX_RPMSG_SENSOR 7
> +
> +/* rpmsg version */
> +#define IMX_RMPSG_MAJOR 1
> +#define IMX_RMPSG_MINOR 0
> +
> +#define MAX_DEV_PER_CHANNEL 10
> +
> +struct imx_rpmsg_head {
> + u8 cate; /* Category */
> + u8 major; /* Major version */
> + u8 minor; /* Minor version */
> + u8 type; /* Message type */
> + u8 cmd; /* Command code */
> + u8 reserved[5];
> +} __packed;
This structure is not used in this file, please remove.
> +
> +struct imx_rpmsg_driver_data {
> + int map_idx;
> + const char *rproc_name;
> + 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];
> +};
For the above 2 struct, proper documentation is needed.
Thanks,
Mathieu
> +
> +#endif /* __LINUX_IMX_RPMSG_H__ */
> --
> 2.43.0
>
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
> > rpmsg-i2c-channel {
> > i2c@0 {
> > compatible = "fsl,imx-rpmsg-i2c";
> > reg = <0>;
>
> i2c channel could be removed, it is not included in this patchset.
And i also expect a similar sort of generic driver for i2c. There
should not be anything IMX specific needed.
Andrew
© 2016 - 2025 Red Hat, Inc.