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

[Beleswar Prasad Padhi <b-padhi@ti.com>]

On 28/04/26 00:53, Shenwei Wang wrote:
[...]
>>> +
>>> +struct rpmsg_gpio_packet {
>>> +     u8 type;        /* Message type */
>>> +     u8 cmd;         /* Command code */
>>> +     u8 port_idx;
>>> +     u8 line;
>>> +     u8 val1;
>>> +     u8 val2;
>>> +};
>>
>> Could you please document the fields in these structs (and the below ones too)?
>> From the code, it looks like while sending a message from Linux to Firmware; val1
>> and val2 are used to describe the values to set. Whereas while receiving a
>> response, val1 represents a possible error code, and val2 represents the actual
>> message of get type queries. If that is so, you might want to change the variable
>> names to be more descriptive and also use a union.
>>
> The fields in the two structs are fairly self-explanatory. Do we really need the additional comments?


val1 and val2 sounded arbitrary, that's all. If we are moving away from
that, then there is no need :)

[...]

>
>>> +     void *channel_devices[MAX_PORT_PER_CHANNEL];
>>
>> So this is technically a rpmsg endpoint (struct rpmsg_endpoint) without naming it
>> "endpoint". Every rpmsg endpoint has a reference to its parent rpmsg channel
>> (struct rpmsg_device) which represents the same information here. So we should
>> use the framework standard here.
>>
> Yes, agree to use "endpoint_devices".


I did not mean to say to just change the variable name from
"channel_devices" to "endpoint_devices". Infact you would not need to
have this field & struct anymore.

Pseudo-code:
1. Add a 'struct rpmsg_endpoint *ept' field to struct rpmsg_gpio_port
    to maintain the ept to port idx map.

2. Call port->ept = rpmsg_create_ept(rpdev,
                                                            rpmsg_gpio_channel_callback,
                                                            port, {rpdev.id.name,
                                                            RPMSG_ADDR_ANY,
                                                            RPMSG_ADDR_ANY})
    from rpmsg_gpiochip_register().

3. Send msgs from local ept in rpmsg_gpio_send_message() by:
    rpmsg_send(port->ept, msg, sizeof(*msg));

4. Get the port info in rpmsg_gpio_channel_callback() by:
    struct rpmsg_gpio_port *port = priv;

Which also eliminates the need for struct rpdev_drvdata as you can just
do rpmsg_get_rproc_node_name(rpdev) from rpmsg_gpiochip_register().


>
>> This also allows for dynamic creation and deletion of ports too! (if/when the
>> firmware supports it)
>>
>> Which means at port init time, we should make a call to
>> rpmsg_create_ept() for each port tying the same callback
>> rpmsg_gpio_channel_callback(). And based on the 'u32 src', we could identify the
>> appropriate gpio port in the callback.
>>

[...]
>>
>>> +
>>> +     girq = &gc->irq;
>>> +     gpio_irq_chip_set_chip(girq, &gpio_rpmsg_irq_chip);
>>> +     girq->parent_handler = NULL;
>>> +     girq->num_parents = 0;
>>> +     girq->parents = NULL;
>>> +     girq->chip->name = devm_kasprintf(&rpdev->dev, GFP_KERNEL, "%s-
>> gpio%d",
>>> +                                       drvdata->rproc_name,
>>> + port->idx);
>>
>> We could just re-use gc->label here...
> We also want to include the remoteproc name (for example, remoteproc-cm33-gpio0), rather than just gpio0.


Isn't it also included in the gc->label field?

gc->label = devm_kasprintf(&rpdev->dev, GFP_KERNEL, "%s-gpio%d",
+                   drvdata->rproc_name, port->idx);

[...]

>>> +}
>>> +
>>> +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 = -ENODEV;
>>> +
>>> +     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;
>>
>> I know this was asked in the v10 version also. But I don't think the answer is
>> sufficient. Should we not continue the intialization of drvdata etc if np != 0? Why
>> return a deferred probe, and let the kernel come back to it again to do the same
>> stuff with extra latency?
>>
>> We could just do:
>> if (!np) return -EPROBE_DEFER;
>> else {everything_else};
>>
> After configuring dev->of_node, it would be better to restart the driver probe process. 
> This ensures that all required resources, such as pinctrl, clocks, and power domains, are 
> properly set up if they are specified in the device node, before the probe function is invoked.


Hmm that makes sense to me... Thanks!

Thanks,
Beleswar