> -----Original Message-----
> From: Arnaud POULIQUEN <[email protected]>
> Sent: Friday, February 20, 2026 3:33 AM
> To: Shenwei Wang <[email protected]>; Linus Walleij
> <[email protected]>; Bartosz Golaszewski <[email protected]>; Jonathan Corbet
> <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>; Bjorn Andersson
> <[email protected]>; Mathieu Poirier <[email protected]>; Frank Li
> <[email protected]>; Sascha Hauer <[email protected]>
> Cc: Shuah Khan <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; Pengutronix Kernel Team
> <[email protected]>; Fabio Estevam <[email protected]>; Peng Fan
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; linux-arm-
> [email protected]; dl-linux-imx <[email protected]>; Bartosz
> Golaszewski <[email protected]>; Andrew Lunn <[email protected]>
> Subject: [EXT] Re: [PATCH v8 3/4] gpio: rpmsg: add generic rpmsg GPIO driver
> >> Subject: [EXT] Re: [PATCH v8 3/4] gpio: rpmsg: add generic rpmsg GPIO
> >> driver
> >>> + rproc = rproc_get_by_child(&rpdev->dev);
> >>> + if (!rproc)
> >>> + return NULL;
> >>> +
> >>> + np = of_node_get(rproc->dev.of_node);
> >>> + if (!np && rproc->dev.parent)
> >>> + np = of_node_get(rproc->dev.parent->of_node);
> >>
> >> Is a topology where they is no rproc->dev node but a parent node exist?
> >>
> >
> > If no rproc->dev, it should return NULL in the above check.
>
> Regarding rproc_alloc, seems that rproc->dev.of_node is always NULL.
> so probably test on it is useless.
>
Even if rproc->dev.of_node happens to be NULL in current rproc_alloc() usage,
the logic here is
written to handle both cases correctly. It’s safer not to rely on that
assumption, since making
decisions based on an expected NULL value could break if the allocation path
changes in the future.
Keeping the check avoids this unnecessary constraint.
> >
> >>> +
> >>> + if (np) {
> >>> + /* Balance the of_node_put() performed by
> of_find_node_by_name().
> >> */
> >>> + of_node_get(np);
> >>> + np_chan = of_find_node_by_name(np, chan_name);
> >>> + of_node_put(np);
> >>> + }
> >>> +
> >>> + return np_chan;
> >>> +}
> >>> +
> >>> +static int
> >>> +rpmsg_gpio_channel_callback(struct rpmsg_device *rpdev, void *data,
> >>> + int len, void *priv, u32 src) {
> >>> + struct gpio_rpmsg_packet *msg = data;
> >>> + struct rpmsg_gpio_port *port = NULL;
> >>> + struct rpdev_drvdata *drvdata;
> >>> +
> >>> + drvdata = dev_get_drvdata(&rpdev->dev);
> >>> + if (drvdata && msg && msg->port_idx < MAX_PORT_PER_CHANNEL)
> >>> + port = drvdata->channel_devices[msg->port_idx];
> >>> +
> >>> + if (!port)
> >>> + return -ENODEV;
> >>> +
> >>> + if (msg->header.type == GPIO_RPMSG_REPLY) {
> >>> + *port->info.reply_msg = *msg;
> >>> + complete(&port->info.cmd_complete);
> >>
> >> What happen if the remoteprocessor answer after the completion timeout?
> >> Could it result in desynchronization between the request and the answer?
> >
> > If the remote processor responds after the timeout, that late reply
> > will be ignored. The current transfer should fail with TIMEOUT, and
> > the state won’t be carried over because cmd_complete is reinitialized
> > before each new request, so a stale completion won’t desynchronize the
> > next transaction. Each command–reply cycle is isolated, so a delayed reply
> cannot corrupt or mix with a subsequent request.
>
> I missed the reinit_completion. Indeed, that prevents issue if reply arrive
> after
> the time out.
>
> That said a second request can be sent before the remote processor responds to
> the first one:
> - resquest 1 sent to remoteprocessor.
> - timeout occurs
> - request 2 sent to remote processor
> - reply of request 1 received
>
> Wouldn't this lead to a desynchronization between requests and replies?
> I do not see a mechanism that would prevent this
>
There’s a lock in place that sequences the requests, so we won’t end up with
two requests
running in parallel. If request 1 hasn’t completed yet, request 2 won’t start.
> >
> >>
> >> Having a cmd_counter in gpio_rpmsg_head could help to identify
> >> current request and answer
> >>
> >> the use of reinit_completion could be also needed
> >>
> >>> + } else if (msg->header.type == GPIO_RPMSG_NOTIFY) {
> >>> + generic_handle_domain_irq_safe(port->gc.irq.domain, msg-
> >pin_idx);
> >>> + } else
> >>> + dev_err(&rpdev->dev, "wrong command type!\n");
> >>
> >> Could you print the msg->header.type value to help for debug?
> >>
> >
> > Sure. Will add it in next version.
> >
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int rpmsg_gpio_channel_probe(struct rpmsg_device *rpdev) {
> >>> + struct device *dev = &rpdev->dev;
> >>> + struct rpdev_drvdata *drvdata;
> >>> + struct device_node *np;
> >>> + int ret;
> >>> +
> >>> + if (!dev->of_node) {
> >>> + np = rpmsg_get_channel_ofnode(rpdev, rpdev->id.name);
> >>> + if (np) {
> >>> + dev->of_node = np;
> >>> + set_primary_fwnode(dev, of_fwnode_handle(np));
> >>> + }
> >>> + return -EPROBE_DEFER;
> >>> + }
> >>> +
> >>> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> >>> + if (!drvdata)
> >>> + return -ENOMEM;
> >>> +
> >>> + drvdata->rproc_name = rpmsg_get_rproc_node_name(rpdev);
> >>> + dev_set_drvdata(dev, drvdata);
> >>> +
> >>> + for_each_child_of_node_scoped(dev->of_node, child) {
> >>> + if (!of_device_is_available(child))
> >>> + continue;
> >>> +
> >>> + if (!of_match_node(dev->driver->of_match_table, child))
> >>> + continue;
> >>> +
> >>> + ret = rpmsg_gpiochip_register(rpdev, child);
> >>> + if (ret < 0)
> >>> + dev_err(dev, "Failed to register: %pOF\n", child);
> >>> + }
> >>> +
> >>> + return 0;
> >>
> >> return ret
> >> or indicate why the return of rpmsg_gpiochip_register is not taken
> >> into account
> >>
> >
> > rpmsg_gpiochip_register() failing only affects whether the GPIO
> > instance gets created. The rpmsg channel driver itself can still probe
> successfully and continue to operate for other features.
>
> This is not safe, by default you have to exist with error if something fails,
> ensuring
> that all resources allocated during the probe are released.
> If there is a strong reason to not do this you have to explain the exception
> in a
> comment.
>
I wouldn’t say it is not safe. The behavior here really depends on the policy
we want for this
driver: either fail fast (one failure causes the entire probe to fail) or
best‑effort (continue even
if some parts fail). Both approaches can be valid as long as the logic is
consistent.
> >
> >>
> >>> +}
> >>> +
> >>> +static void rpmsg_gpio_channel_remove(struct rpmsg_device *rpdev) {
> >>> + dev_info(&rpdev->dev, "rpmsg gpio channel driver is
> >>> +removed\n"); }
> >>> +
> >>> +static const struct of_device_id rpmsg_gpio_dt_ids[] = {
> >>> + { .compatible = "rpmsg-gpio" },
> >>> + { /* sentinel */ }
> >>> +};
> >>> +
> >>> +static struct rpmsg_device_id rpmsg_gpio_channel_id_table[] = {
> >>> + { .name = "rpmsg-io-channel" },
> >>
> >> I would remove the "-channel" suffix to have similar naming than
> >> "rpmsg-tty" and "rpmsg-raw"
> >>
> >
> > The channel name comes from the remote firmware, so we can’t freely
> > rename it on the Linux side. On i.MX platforms the firmware follows
> > its own naming conventions, and the *-channel suffix is part of that scheme.
>
> As Andrew mentioned, in other words, you cannot expect to impose upstream
> constraints based on your downstream legacy. Your legacy firmware will
> continue to be supported by your legacy NXP rpmsg GPIO driver.
>
> Moreover, changing the name of this rpmsg channel will help you have both
> drivers coexist in your downstream kernel.
>
The id_table can support multiple channel names, so we don’t have to choose
only one.
Our driver depends on the company’s IP, which has been widely deployed by our
customers,
so we still need to keep supporting the existing channel name used on i.MX
platforms.
At the same time, we can add the suggested "rpmsg-io" entry to the table so the
driver works for
both the generic use case and i.MX platforms.
Thanks,
Shenwei
> Regards
> Arnaud
>
>
> >
> > Thanks,
> > Shenwei
> >
> >> Regards,
> >> Arnaud
> >>
> >>> + { },
> >>> +};
> >>> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_gpio_channel_id_table);
> >>> +
> >>> +static struct rpmsg_driver rpmsg_gpio_channel_client = {
> >>> + .drv.name = KBUILD_MODNAME,
> >>> + .drv.of_match_table = rpmsg_gpio_dt_ids,
> >>> + .id_table = rpmsg_gpio_channel_id_table,
> >>> + .probe = rpmsg_gpio_channel_probe,
> >>> + .callback = rpmsg_gpio_channel_callback,
> >>> + .remove = rpmsg_gpio_channel_remove,
> >>> +};
> >>> +module_rpmsg_driver(rpmsg_gpio_channel_client);
> >>> +
> >>> +MODULE_AUTHOR("Shenwei Wang <[email protected]>");
> >>> +MODULE_DESCRIPTION("generic rpmsg gpio driver");
> >>> +MODULE_LICENSE("GPL");
> >
