On 12/11/23 12:54 AM, Ayush Singh wrote:
Make gb-beagleplay greybus spec compliant by moving cport information to
transport layer instead of using `header->pad` bytes.
Greybus HDLC frame now has the following payload:
1. le16 cport
2. gb_operation_msg_hdr msg_header
3. u8 *msg_payload
Fixes: ec558bbfea67 ("greybus: Add BeaglePlay Linux Driver")
Signed-off-by: Ayush Singh <ayushdevel1325@xxxxxxxxx>
I would say that this is an improvement, but I wish I
had a better picture in mind of how this works. The
initial commit provided some explanation, but even
there it talks about the "CC1352 (running SVC Zephyr
application)" and that leads me to wonder even how
the hardware is structured. (I'm not really asking
you for this right now, but you have a reference to
something that provides some background, you should
provide it for context.)
Another general comment is that the use of HDLC seems
like it could be a more clearly separated layer that
could be used by other Greybus protocols or applications.
Maybe that's overkill, but it is a distinct layer, right?
I had a comment or two about using (void *) instead of
(u8 *), to reduce the need for explicit type casts. But
I found that (u8 *) is used elsewhere in the Greybus code.
One comment I *will* share is that the serdev RX callback
has a const receive buffer. I recommend you preserve that
"constness" in your code.
-Alex