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
