Re: [PATCH v2] net: dsa: ksz8795: adjust CPU link to host interface

[Vladimir Oltean <olteanv@gmail.com>]

Hi Jean,

On Tue, Dec 01, 2020 at 07:58:01PM +0100, Jean Pihet wrote:
> Hi Andrew,
>
> On Tue, Dec 1, 2020 at 7:41 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Tue, Dec 01, 2020 at 09:34:08AM +0100, Jean Pihet wrote:
> > > Add support for RGMII in 100 and 1000 Mbps.
> > >
> > > Adjust the CPU port settings from the host interface settings: interface
> > > MII type, speed, duplex.
> >
> > Hi Jean
> >
> > You have still not explained why this is needed. Why? is always the
> > important question to answer in the commit message. The What? is
> > obvious from reading the patch. Why does you board need this, when no
> > over board does?
>
> I reworked the commit description about the What and thought it was
> enough. Do you need a cover letter to describe it more?
>
> The Why is:
> "
> Configure the host port of the switch to match the host interface
> settings. This is useful when the switch is directly connected to the
> host MAC interface.
> "
> Thank you for reviewing the patch.

First of all, I am not clear if you want the patch merged or not. If you
do, then I don't understand why you did not use the ./scripts/get_maintainer.pl
tool to get the email addresses of the people who can help you with
that. No one from Microchip, not the DSA maintainers, not the networking
maintainer.

Secondly, don't you get an annoying warning that you should not use
.adjust_link and should migrate to .phylink_mac_link_up? Why do you
ignore it? Did you even see it?

Thirdly, your patch is opaque and has three changes folded into one. You
refactor some code from ksz8795_port_setup into a separate function, you
add logic for the speeds of 100 and 10 for RGMII, and you call this
function from .adjust_link. You must justify why you need all of this,
and cannot just add 3 lines to ksz8795_port_setup. You must explain that
the ksz8795_port_setup function does not use information from device
tree. Then you must explain why the patch is correct.
The code refactored out of ksz8795_port_setup, plus the changes you've
added to it, looks now super weird. Half of ksz8795_mii_config treats
p->phydev.speed as an output variable, and half of it as an input
variable. To the untrained eye this looks like a hack. I'm sure you can
clarify. This is what Andrew wants to see.

Fourth, seriously now, could you just copy Microchip people to your
patches? The phylink conversion was done this summer, I'm sure they can
help with some suggestions.