Re: [PATCH net v3] net: dsa: mt7530: fix impossible MDIO address and issue warning

[Daniel Golle <daniel@makrotopia.org>]

On Wed, Jul 03, 2024 at 07:13:08PM -0700, Jakub Kicinski wrote:
> On Wed, 3 Jul 2024 00:44:28 +0100 Daniel Golle wrote:
> > +	/* The corrected address is calculated as stated below:
> > +	 * 0~6   -> 31
> > +	 * 8~14  -> 7
> > +	 * 16~22 -> 15
> > +	 * 24~30 -> 23
> > +	 */
> > +return ((((phy_addr - MT7530_NUM_PORTS) & ~MT7530_NUM_PORTS) % PHY_MAX_ADDR) +
> > +	MT7530_NUM_PORTS) & (PHY_MAX_ADDR - 1);
> 
> nit: the return statement lacks indentation

Yes, lacks an additional space to match the level of the first open parentheses.
I'll fix that in the next round.

> 
> but also based on the comment, isn't it:
> 
> 	return (round_down(phy_addr, MT7530_NUM_PORTS + 1) - 1)	& (PHY_MAX_ADDR - 1);

The original, more complicated statement covers also the correct addresses,
ie. 31 -> 31, 7 -> 7, 15 -> 15, 23 -> 23. However, the function is never
called if the address is deemed correct, so that doesn't actually matter.

It's kinda difficult to decide whether it is more important to return
correct results also for values never used with the current code, or
have a slightly more readable and shorter function but with expectations
regarding the input values given by the caller.

Opinions?