Messages in this thread | | | From | Linus Walleij <> | Date | Wed, 13 Oct 2021 13:02:02 +0200 | Subject | Re: [PATCH net-next 4/6] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag |
| |
On Tue, Oct 12, 2021 at 2:37 PM Alvin Šipraga <alvin@pqrs.dk> wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk> > > This commit implements a basic version of the 8 byte tag protocol used > in the Realtek RTL8365MB-VC unmanaged switch, which carries with it a > protocol version of 0x04. > > The implementation itself only handles the parsing of the EtherType > value and Realtek protocol version, together with the source or > destination port fields. The rest is left unimplemented for now. > > The tag format is described in a confidential document provided to my > company by Realtek Semiconductor Corp. Permission has been granted by > the vendor to publish this driver based on that material, together with > an extract from the document describing the tag format and its fields. > It is hoped that this will help future implementors who do not have > access to the material but who wish to extend the functionality of > drivers for chips which use this protocol. > > In addition, two possible values of the REASON field are specified, > based on experiments on my end. Realtek does not specify what value this > field can take. > > Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
The code definately looks good: Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Some nitpicky personal preferences below:
> +#define RTL8_4_TAG_LEN 8 > +/* 0x04 = RTL8365MB DSA protocol > + */ > +#define RTL8_4_PROTOCOL_RTL8365MB 0x04
This is #defined
> + /* Zero FID_EN, FID, PRI_EN, PRI, KEEP; set LEARN_DIS */ > + tag[2] = htons(1 << 5);
I would create defines for the flags like this: #define RTL8365MB_LEARN_DIS BIT(5)
> + /* Parse Protocol */ > + proto = ntohs(tag[1]) >> 8;
In the 4byte header code we have something like this: #define RTL8_4_PROTOCOL_SHIFT 8
> + /* Parse TX (switch->CPU) */ > + port = ntohs(tag[3]) & 0xf; /* Port number is the LSB 4 bits */
This I think is fair enough. No need to define that mask.
Yours, Linus Walleij
| |