On 12/12/23 19:31, Alex Elder wrote:
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.)
Yes, I am thinking of revamping the Beagle connect docs to reflect the new architecture with some charts and provide a better overall picture. It is sorely needed at this point.
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?
Initial commits of gb-beagleplay did separate all the HDLC parts from the driver. However, it was decided to keep it together and maybe extract it in the future if other drivers need it.
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
The constness of the receive buffer is actually preserved. The `gb_tty_receive` function calls `hdlc_rx` (which takes const u8 *). This function copies the data to a separate buffer (`gb_beagleplay->rx_buffer`) for further processing. So the const data is not modified.
Ayush Singh _______________________________________________ greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx
|