Messages in this thread | | | Date | Mon, 9 May 2022 21:20:30 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next v4 04/12] net: pcs: add Renesas MII converter driver |
| |
Hi,
On Mon, May 09, 2022 at 03:18:52PM +0200, Clément Léger wrote: > +#define MIIC_PRCMD 0x0 > +#define MIIC_ESID_CODE 0x4 > + > +#define MIIC_MODCTRL 0x20 > +#define MIIC_MODCTRL_SW_MODE GENMASK(4, 0) > + > +#define MIIC_CONVCTRL(port) (0x100 + (port) * 4) > + > +#define MIIC_CONVCTRL_CONV_SPEED GENMASK(1, 0) > +#define CONV_MODE_10MBPS 0 > +#define CONV_MODE_100MBPS BIT(0) > +#define CONV_MODE_1000MBPS BIT(1)
I think this is an inappropriate use of the BIT() macro. BIT() should be used for single bit rather than for field values.
You seem to have a two bit field in bits 1 and 0 of a register, which has the values of: 0 - 10MBPS 1 - 100MBPS 2 - 1GBPS
I'd guess 3 is listed as "undefined", "do not use" or something similar?
> + > +#define MIIC_CONVCTRL_CONV_MODE GENMASK(3, 2) > +#define CONV_MODE_MII 0 > +#define CONV_MODE_RMII BIT(0) > +#define CONV_MODE_RGMII BIT(1)
This looks similar. a 2-bit field in bits 3 and 2 taking values: 0 - MII 1 - RMII 2 - RGMII
...
> +static int miic_config(struct phylink_pcs *pcs, unsigned int mode, > + phy_interface_t interface, > + const unsigned long *advertising, bool permit) > +{ > + u32 speed = CONV_MODE_10MBPS, conv_mode = CONV_MODE_MII, val; > + struct miic_port *miic_port = phylink_pcs_to_miic_port(pcs); > + struct miic *miic = miic_port->miic; > + int port = miic_port->port; > + > + switch (interface) { > + case PHY_INTERFACE_MODE_RMII: > + conv_mode = CONV_MODE_RMII; > + speed = CONV_MODE_100MBPS; > + break; > + case PHY_INTERFACE_MODE_RGMII: > + conv_mode = CONV_MODE_RGMII; > + speed = CONV_MODE_1000MBPS; > + break; > + case PHY_INTERFACE_MODE_MII:
I'm not sure why you need to initialise "speed" and "conv_mode" above when you could set them here.
Thanks.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |