Messages in this thread | | | Date | Tue, 12 Dec 2023 20:00:52 +0530 | Subject | Re: [PATCH V2 1/1] greybus: gb-beagleplay: Remove use of pad bytes | From | Ayush Singh <> |
| |
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@gmail.com> > > 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
| |