Hi Qingfang, thanks for your patch! Overall it looks good and I will not nitpick details since this is RFC.
On Wed, Feb 17, 2021 at 7:21 AM DENG Qingfang <dqf...@gmail.com> wrote: > > Support Realtek RTL8366S/SR switch > > Signed-off-by: DENG Qingfang <dqf...@gmail.com> I would mention that the DT bindings for the switch are already in Documentation/devicetree/bindings/net/dsa/realtek-smi.txt (Hmmm we should convert these bindings to YAML too.) > select NET_DSA_TAG_RTL4_A > + select NET_DSA_TAG_RTL8366S Hopefully we can use NET_DSA_TAG_RTL4_A for this switch as mentioned. > +/* bits 0..7 = port 0, bits 8..15 = port 1 */ > +#define RTL8366S_PAACR0 0x0011 > +/* bits 0..7 = port 2, bits 8..15 = port 3 */ > +#define RTL8366S_PAACR1 0x0012 > +/* bits 0..7 = port 4, bits 8..15 = port 5 */ > +#define RTL8366S_PAACR2 0x0013 Is this all? I was under the impression that RTL8366S supports up to 8 ports (7+1). > +#define RTL8366S_CHIP_ID_REG 0x0105 > +#define RTL8366S_CHIP_ID_8366 0x8366 Curiously RTL8366RB presents ID 0x5937. Oh well. Interesting engineering process I suppose. > +/* LED control registers */ > +#define RTL8366S_LED_BLINKRATE_REG 0x0420 > +#define RTL8366S_LED_BLINKRATE_BIT 0 > +#define RTL8366S_LED_BLINKRATE_MASK 0x0007 > + > +#define RTL8366S_LED_CTRL_REG 0x0421 > +#define RTL8366S_LED_0_1_CTRL_REG 0x0422 > +#define RTL8366S_LED_2_3_CTRL_REG 0x0423 Again I wonder if there are more registers here for 8 ports? > +/* PHY registers control */ > +#define RTL8366S_PHY_ACCESS_CTRL_REG 0x8028 > +#define RTL8366S_PHY_ACCESS_DATA_REG 0x8029 > + > +#define RTL8366S_PHY_CTRL_READ 1 > +#define RTL8366S_PHY_CTRL_WRITE 0 > + > +#define RTL8366S_PORT_NUM_CPU 5 > +#define RTL8366S_NUM_PORTS 6 Hm 5+1? Isn't RTL8366S 7+1? > +#define RTL8366S_PORT_1 BIT(0) /* In userspace port 0 > */ > +#define RTL8366S_PORT_2 BIT(1) /* In userspace port 1 > */ > +#define RTL8366S_PORT_3 BIT(2) /* In userspace port 2 > */ > +#define RTL8366S_PORT_4 BIT(3) /* In userspace port 3 > */ > +#define RTL8366S_PORT_5 BIT(4) /* In userspace port 4 > */ > +#define RTL8366S_PORT_CPU BIT(5) /* CPU port */ Same here. Overall the question about whether the switch is 5+1 or 7+1 is my big design remark. Maybe it is only 5+1 who knows... Yours, Linus Walleij
