Re: [PATCH v13 3/4] gpio: rpmsg: add generic rpmsg GPIO driver

Arnaud POULIQUEN
2026-05-05T08:46:18Z

Hi Beleswar

On 5/5/26 07:25, Beleswar Prasad Padhi wrote:
> Hi Arnaud,
>
> On 04/05/26 22:34, Arnaud POULIQUEN wrote:
>> Hi Beleswar,
>>
>> On 5/4/26 10:17, Beleswar Prasad Padhi wrote:
>>
>
> [...]
>
>>>
>>>>
>>>> I may have misunderstood your solution. Could you please help me
>>>> understand your proposal by explaining how you would handle three
>>>> GPIO ports defined in the DT, considering that the endpoint
>>>> addresses on the Linux side can be random?
>>>> If I assume there is a unique endpoint on the remote side,
>>>> I do not understand how you can match, on the firmware side,
>>>> the Linux endpoint address to the GPIO port.
>>>
>>>
>>> Sure, let me take an example:
>>> Assumptions: 3 GPIO ports in DT, 3 endpoints in Linux (one per port),
>>> 1 endpoint in remote (0xd) and 1 rpmsg channel (rpmsg-io)
>>>
>>>         rpmsg {
>>>           rpmsg-io {
>>>             #address-cells = <1>;
>>>             #size-cells = <0>;
>>>
>>>             gpio@25 {
>>>               compatible = "rpmsg-gpio";
>>>               reg = <25>;
>>>               gpio-controller;
>>>               #gpio-cells = <2>;
>>>               #interrupt-cells = <2>;
>>>               interrupt-controller;
>>>             };
>>>
>>>             gpio@32 {
>>>               compatible = "rpmsg-gpio";
>>>               reg = <32>;
>>>               gpio-controller;
>>>               #gpio-cells = <2>;
>>>               #interrupt-cells = <2>;
>>>               interrupt-controller;
>>>             };
>>>
>>>             gpio@35 {
>>>               compatible = "rpmsg-gpio";
>>>               reg = <35>;
>>>               gpio-controller;
>>>               #gpio-cells = <2>;
>>>               #interrupt-cells = <2>;
>>>               interrupt-controller;
>>>             };
>>>           };
>>>         };
>>>
>>> Code Flow:
>>> 1. "rpmsg-io" channel is announced from remote firmware with unique dst
>>>      ept = 0xd.
>>>
>>> 2. rpmsg_core.c creates the default dynamic local ept for the channel
>>>      ept = 0x405.
>>>
>>> 3. rpmsg_core.c assigns the allocated addr to rpdev device:
>>>      rpdev->src = 0x405 and rpdev->dst = 0xd.
>>>
>>> 4. rpmsg_gpio_channel_probe() is triggered. For *each* of the GPIO ports
>>>      in DT, it will trigger rpmsg_gpiochip_register() which will now:
>>>         a. Call port->ept = rpmsg_create_ept(rpdev,
>>>                                                                     rpmsg_gpio_channel_callback,
>>>                                                                     port,
>>>                                                                    {rpdev.id.name,
>>>                                                                     RPMSG_ADDR_ANY,
>>>                                                                     RPMSG_ADDR_ANY});
>>>             Ex- port->ept->addr = 0x408
>>>
>>>         b. Prepare a 8-byte message having 2 fields:
>>>             port->ept->addr (0x408) and port->idx (25)
>>>
>>>         c. Send this message to remote firmware on default channel ept
>>>             (0x405 -> 0xd) by:
>>>             rpmsg_send(rpdev->ept, &message, sizeof(message));
>>>
>>>         d. Remote side receives this message and creates a map of the
>>>             linux_ept_addr to gpio_port. (0x408 <-> 25)
>>>
>>> 5. After this point, any gpio messages sent from Linux from gpio port
>>>      endpoints (Ex- 0x408) can be decoded at remote side by looking up
>>>      its map (Ex- map[0x408] = 25).
>>>
>>> 6. Any messages sent from remote to Linux for a particular gpio port can
>>>      also be decoded at Linux by simply fetching the priv pointer to get
>>>      the per-port device:
>>>      struct rpmsg_gpio_port *port = priv;
>>>
>>
>> Thanks for the details!
>>
>> To sum up:
>> - the default endpoint acts as the GPIO controller (0x405),
>> - one extra Linux endpoint is created per port defined in DT.
>>
>> This should work, but my concerns remain the same:
>>
>>   1) This implementation forces the remote processor to handle a single
>>      endpoint instead of one endpoint per port. This may add complexity to
>>      the remote firmware if each port is managed in a separate thread.
>
>
> A. Not really, I just chose 1 remote endpoint for this example as you
>     suggested to. We can scale it for two-way communication via the
>     get_config message like you suggested below.
>
> B. Isn't it a bad design of the firmware if it is handling 10 gpio ports
>     in 10 threads? The logic to handle all the ports is the same, only
>     the parameters (e.g. line number, msg) is different.
>
>>
>>   2) Linux, as a consumer, should not expose its capabilities to the remote
>>      side (in your proposal it enumerates the ports defined in the DT).     In my view, the remote processor should expose its capabilities as the
>>      provider.
>
>
> Agreed on this.
>
>>
>> From my perspective, based on your proposal:
>>  1) Linux should send a get_config message to the remote proc (0x405 -> 0xD). 2) The remote processor would respond with the list of ports, associated
>>     with an remote endpoint addresses.
>
>
> Agreed, we can scale it for multiple remote endpoints like this.
>
>>  3) Linux would parse the response, compare it with the DT, enable the GPIO
>>     ports accordingly, creating it local endpoint and associating it with
>>     the remote endpoint.
>> Using name service to identify the ports should avoid step 1 & 2 ...
>
>
> Yes, but won't that make a lot of hard-codings in the driver?
>
> +static struct rpmsg_device_id rpmsg_gpio_channel_id_table[] = {
> +    { .name = "rpmsg-io-25" },
> +    { .name = "rpmsg-io-32" },
> +    { .name = "rpmsg-io-35" },
> +    { },
> +};
>
> What if tomorrow another vendor decides to add more remoteproc
> controlled GPIO ports to Linux, they would have to update this struct in
> the driver everytime. And the port indexes (25/32/35) could also differ
> between vendors. We should make the driver dynamic i.e. vendor
> agnostic.
>
> I think querying the remote firmware at runtime (step 1 & 2 above) is a
> common design pattern and makes the driver vendor agnostic. But feel
> free to correct me.
>

You are right. My proposal would require a patch in rpmsg-core. The idea of
allowing a postfix in the compatible string has been discussed before, but,
if I remember correctly, it was not concluded.

/* rpmsg devices and drivers are matched using the service name */
static inline int rpmsg_id_match(const struct rpmsg_device *rpdev,
const struct rpmsg_device_id *id)
{
size_t len;

+ len = strnlen(id->name, RPMSG_NAME_SIZE);
+ if (len && id->name[len - 1] == '*')
+ return !strncmp(id->name, rpdev->id.name, len - 1);

return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0;
}

Then, in rpmsg-gpio, and possibly in other drivers such as rpmsg-tty and
a future rpmsg-i2c, we could use:
static struct rpmsg_device_id rpmsg_gpio_channel_id_table[] = {
{ .name = "rpmsg-io" },
{ .name = "rpmsg-io-*" },
{ },
};

If exact name matching is strongly required, then this proposal would
not be suitable.

A third option would be a combination of both approaches: instantiate
the device using the same name service from the remote side, as done in
rpmsg-tty. In that case, a get_config message, or a similar mechanism,
would also be needed to retrieve the port information from the remote side.

Tanmaya also proposed another alternative based on reserved addresses.

At this point, I suggest letting Mathieu review the discussion and
recommend the most suitable approach.

Thanks,
Arnaud

>>
>> At the end, whatever solution is implemented, my main concern is that the
>> Linux driver design should, if possible, avoid adding unnecessary complexity
>> or limitations on the remote side (for instance in openAMP project).
>
>
> Yes definitely, I want the same. Feel free to let me know if this does
> not suit with the OpenAMP project.
>
> Thanks,
> Beleswar
>
>>
>> Thanks,
>> Arnaud
>>
>>
>>> So Linux does not need to send the port idx everytime while sending a
>>> gpio message anymore.
>>>
>>> Thanks,
>>> Beleswar
>>>
>>> [...]
>>>
>>