Re: [PATCH net-next 03/30] net: dsa: mt7530: properly support MT7531AE and MT7531BE

[Arınç ÜNAL]

On 24.05.2023 17:48, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:05PM +0300, arinc9.unal@xxxxxxxxx wrote:
> > From: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
> >
> > Introduce the p5_sgmii field to store the information for whether port 5
> > has got SGMII or not.
> >
> > Move the comment about MT7531AE and MT7531BE to mt7531_setup(), where the
> > switch is identified.
> >
> > Get rid of mt7531_dual_sgmii_supported() now that priv->p5_sgmii stores the
> > information. Address the code where mt7531_dual_sgmii_supported() is used.
> >
> > Get rid of mt7531_is_rgmii_port() which just prints the opposite of
> > priv->p5_sgmii.
> >
> > Remove P5_INTF_SEL_GMAC5_SGMII. The p5_interface_select enum is supposed to
> > represent the mode that port 5 is being used in, not the hardware
> > information of port 5. Set p5_intf_sel to P5_INTF_SEL_GMAC5 instead, if
> > port 5 is not dsa_is_unused_port().
> >
> > Signed-off-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
> > Acked-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>
> > ---
>
> Pretty busy patch, and after reading it, I'm not sure:
>
> - why? (this seems to be absent from the commit message)
> - how are MT7531AE and MT7531BE supported any more properly after this
>    change, as the commit title claims
> - what is the overall effect, other than just refactoring. If that's
>    all, what's written on the tin needs to be a better representation of
>    what's inside.
>
> Pure refactoring is not a bad goal in itself, as long as we're on the
> same page that there is a justification which makes the new code better
> than the old one.

With this patch, instead of reading the MT7531_TOP_SIG_SR register 
multiple times, the register will be read once and the value will be 
stored on the p5_sgmii field. This saves unnecessary reads of the 
register. I will mention this on the patch log.

This is already being done on the U-Boot driver.

https://github.com/u-boot/u-boot/blob/master/drivers/net/mtk_eth.c#L859

I think "improve MT7531AE and MT7531BE support" will be a better subject.

Arınç