Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

On 10/3/25 20:41, Shenwei Wang wrote:
> Hi Arnaud,
>
>> -----Original Message-----
>> From: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
>> Sent: Friday, October 3, 2025 2:40 AM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; Bjorn Andersson
>> <andersson@kernel.org>; Mathieu Poirier <mathieu.poirier@linaro.org>; Rob
>> Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor
>> Dooley <conor+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Sascha
>> Hauer <s.hauer@pengutronix.de>; Linus Walleij <linus.walleij@linaro.org>;
>> Bartosz Golaszewski <brgl@bgdev.pl>
>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
>> <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; linux-
>> remoteproc@vger.kernel.org; devicetree@vger.kernel.org; imx@lists.linux.dev;
>>> These processors communicate via the RPMSG protocol.
>>> The driver implements the standard GPIO interface, allowing the Linux
>>> side to control GPIO controllers which reside in the remote processor
>>> via RPMSG protocol.
>> What about my request in previous version to make this driver generic?
>>
> The only platform-dependent part of this driver is the layout of the message packet, which defines the
> communication protocol between the host and the remote processor. It would be challenging to require
> other vendors to follow our protocol and conform to the expected behavior.
>
>> In ST we have similar driver in downstream, we would be interested in reusing it if
>> generic. Indeed we need some rpmsg mechanism for gpio-interrupt. Today we
>> have a downstream rpmsg driver [1][2] that could migrate on a generic rpmsg-
>> gpio driver.
>>
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/gpio/driver.h>
>>> +#include <linux/rpmsg/imx_rpmsg.h>
>>> +#include <linux/init.h>
>>> +#include <linux/irqdomain.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/rpmsg.h>
>>> +
>>> +#define IMX_RPMSG_GPIO_PER_PORT      32
>>> +#define RPMSG_TIMEOUT        1000
>>> +
>>> +enum gpio_input_trigger_type {
>>> +     GPIO_RPMSG_TRI_IGNORE,
>>> +     GPIO_RPMSG_TRI_RISING,
>>> +     GPIO_RPMSG_TRI_FALLING,
>>> +     GPIO_RPMSG_TRI_BOTH_EDGE,
>>> +     GPIO_RPMSG_TRI_LOW_LEVEL,
>>> +     GPIO_RPMSG_TRI_HIGH_LEVEL,
>>> +};
>> What about taking inspiration from the|IRQ_TYPE|bitfield defined in|irq.h|?
>> For instance:
>> GPIO_RPMSG_TRI_BOTH_EDGE = GPIO_RPMSG_TRI_FALLING |
>> GPIO_RPMSG_TRI_RISING,
> Yes, the suggestion is better.
>
>>> +
>>> +enum gpio_rpmsg_header_type {
>>> +     GPIO_RPMSG_SETUP,
>>> +     GPIO_RPMSG_REPLY,
>>> +     GPIO_RPMSG_NOTIFY,
>>> +};
>>> +
>>> +enum gpio_rpmsg_header_cmd {
>>> +     GPIO_RPMSG_INPUT_INIT,
>>> +     GPIO_RPMSG_OUTPUT_INIT,
>>> +     GPIO_RPMSG_INPUT_GET,
>>> +};
>>> +
>>> +struct gpio_rpmsg_data {
>>> +     struct imx_rpmsg_head header;
>>> +     u8 pin_idx;
>>> +     u8 port_idx;
>>> +     union {
>>> +             u8 event;
>>> +             u8 retcode;
>>> +             u8 value;
>>> +     } out;
>>> +     union {
>>> +             u8 wakeup;
>>> +             u8 value;
>> nitpicking put "value" field out of union as common
> I'm afraid we can't make it common, as the two 'value' fields serve different purposes-one is used for outgoing messages and
> the other for incoming messages-and they are located in different parts of the packet.
>
>>> +     } in;
>>> +} __packed __aligned(8);
>> Any reason to pack it an align it?
>> This structure will be copied in the RPMSg payload, right?
>>
> Yes. The message will then be transmitted via the MU hardware to the remote processor, so proper alignment is required.
>
>> I also wonder if that definition should not be in a header file with double licensing
>> that the DT. Indeed this need to be common with the remote side
>> implementation  that can be non GPL implementation.
>>
>>> +
>>> +struct imx_rpmsg_gpio_pin {
>>> +     u8 irq_shutdown;
>>> +     u8 irq_unmask;
>>> +     u8 irq_mask;
>>> +     u32 irq_wake_enable;
>>> +     u32 irq_type;
>>> +     struct gpio_rpmsg_data msg;
>>> +};
>>> +
>>> +struct imx_gpio_rpmsg_info {
>>> +     struct rpmsg_device *rpdev;
>>> +     struct gpio_rpmsg_data *notify_msg;
>>> +     struct gpio_rpmsg_data *reply_msg;
>>> +     struct completion cmd_complete;
>>> +     struct mutex lock;
>>> +     msg->pin_idx = gpio;
>>> +     msg->port_idx = port->idx;
>>> +
>>> +     ret = gpio_send_message(port, msg, true);
>>> +     if (!ret)
>>> +             ret = !!port->gpio_pins[gpio].msg.in.value;
>> Does this code is save?  !! return a boolean right?
>> why not force to 1 if  greater that 1?
>>
> This approach is intended to simplify the implementation. Forcing  to 1 would introduce an additional condition check.
> I'm open to changing it if that's considered standard practice.
>
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int imx_rpmsg_gpio_direction_input(struct gpio_chip *gc,
>>> +                                       unsigned int gpio) {
>>> +     struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
>>> +     struct gpio_rpmsg_data *msg = NULL;
>>> +
>>> +     guard(mutex)(&port->info.lock);
>>> +
>>> +     msg = gpio_get_pin_msg(port, gpio);
>>> +     msg->header.cate = IMX_RPMSG_GPIO;
>> Do you use a single rpmsg channel for several feature?
>> Any reason to not use one rpmsg channel per feature category?
>>
> The current implementation on the remote side uses a single channel to handle all GPIO controllers.
> However, this driver itself does not have that limitation.
>
>>> +     msg->header.major = IMX_RMPSG_MAJOR;
>>> +     msg->header.minor = IMX_RMPSG_MINOR;
>>> +     msg->header.type = GPIO_RPMSG_SETUP;
>>> +     msg->header.cmd = GPIO_RPMSG_INPUT_INIT;
>>> +     msg->pin_idx = gpio;
>>> +     msg->port_idx = port->idx;
>>> +
>>> +     msg->out.event = GPIO_RPMSG_TRI_IGNORE;
>>> +     msg->in.wakeup = 0;
>>> +
>>> +     return gpio_send_message(port, msg, true); }
>>> +
>>> +static inline void imx_rpmsg_gpio_direction_output_init(struct gpio_chip *gc,
>>> +             unsigned int gpio, int val, struct gpio_rpmsg_data *msg)
>>> +
>>> +static struct platform_driver imx_rpmsg_gpio_driver = {
>>> +     .driver = {
>>> +             .name = "gpio-imx-rpmsg",
>>> +             .of_match_table = imx_rpmsg_gpio_dt_ids,
>>> +     },
>>> +     .probe = imx_rpmsg_gpio_probe,
>>> +};
>>> +
>>> +module_platform_driver(imx_rpmsg_gpio_driver);
>> This implementation seems strange to me.
>> You have a platform driver, but your RPMsg driver appears split between this
>> driver and the remoteproc driver, especially regarding the
>> imx_rpmsg_endpoint_probe() function.
>>
> See my reply below.
>
>>   From my point of view, this driver should declare both a platform_driver and an
>> rpmsg_driver structures.
>>
>> Your imx_rpmsg_gpio_driver platform driver should be probed by your
>> remoteproc platform.
>> This platform driver would be responsible for:
>> - Parsing the device tree node
>> - Registering the RPMsg driver
>>
>> Then, the RPMsg device should be probed either by the remote processor using
>> the name service announcement mechanism or if not possible by your
>> remoteproc driver.
>>
> The idea is to probe the GPIO driver successfully only after the remote processor is online and has sent the name service announcement.
> Until then, the GPIO driver will remain in a deferred state, ensuring that all consumers of the associated GPIOs are also deferred.
> The implementation you provided below does not guarantee that the related consumers will be properly deferred. This is the most
> important behavior for a GPIO/I2C controllers.


As long as you keep the GPIO/I2C device as a child of the remote 
processor node,
you should not have deferred probe issues. 
The|of_platform_populate()|function ensures
that the I2C/GPIO devices are probed when the remote processor is started.
Calling|devm_gpiochip_add_data|in the RPMsg driver probe should also 
prevent such issues.

Regards,
Arnaud

>
> Thanks,
> Shenwei
>
>> To better understand my proposal you can have a look to [1]and [2].
>> Here is another example for an rpmsg_i2c( ST downstream implementation):
>> https://github.co/
>> m%2FSTMicroelectronics%2Flinux%2Fblob%2Fv6.6-
>> stm32mp%2Fdrivers%2Fi2c%2Fbusses%2Fi2c-
>> rpmsg.c&data=05%7C02%7Cshenwei.wang%40nxp.com%7C22a9c88be60b474e
>> 391008de02502ec7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
>> 8950740622597592%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRyd
>> WUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%
>> 3D%7C0%7C%7C%7C&sdata=6lCk20Qhb%2F0MTw0NFtto7tj7EFYwZ%2BlOR1F3
>> Qk7kQn8%3D&reserved=0
>> https://github.co/
>> m%2FSTMicroelectronics%2Flinux%2Fblob%2Fv6.6-
>> stm32mp%2FDocumentation%2Fdevicetree%2Fbindings%2Fi2c%2Fi2c-
>> rpmsg.yaml&data=05%7C02%7Cshenwei.wang%40nxp.com%7C22a9c88be60b4
>> 74e391008de02502ec7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
>> C638950740622612512%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnR
>> ydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D
>> %3D%7C0%7C%7C%7C&sdata=4Gva%2FpqP2u8T57XDxSDaoHhvDeJ%2Fo5HtAB
>> L9TY5gbDI%3D&reserved=0
>>
>> Thanks and Regards,
>> Arnaud
>>
>>> +
>>> +MODULE_AUTHOR("NXP Semiconductor");
>>> +MODULE_DESCRIPTION("NXP i.MX SoC rpmsg gpio driver");
>>> +MODULE_LICENSE("GPL");