Re: [PATCH v5 02/14] net: pch_gbe: Pull PHY GPIO handling out of Minnow code

Hi Andrew,

On Sat, Feb 17, 2018 at 11:29:33PM +0100, Andrew Lunn wrote:

On Sat, Feb 17, 2018 at 12:10:25PM -0800, Paul Burton wrote:

quoted

The MIPS Boston development board uses the Intel EG20T Platform
Controller Hub, including its gigabit ethernet controller, and requires
that its RTL8211E PHY be reset much like the Minnow platform. Pull the
PHY reset GPIO handling out of Minnow-specific code such that it can be
shared by later patches.

Hi Paul

I'm i right in saying the driver currently supports the Atheros AT8031
PHY? The same phy which is supported in drivers/net/phy/at803x.c?

It looks like the driver does contain some code relating to that PHY,
but it's not the one I'm using with the MIPS Boston board - there we
have a Realtek RTL8211E (as mentioned in the commit message) which is
working fine alongside this pch_gbe driver too.

If so, i think you are doing this all wrong.

Note that this is a driver which is already in mainline, and I didn't
write it. Claiming that *I* am doing this all wrong is a bit of a
stretch - all this patch does is make small changes to some existing
code, which only tangentially relates to a PHY driver, such that it
ceases to be specific to a single platform.

You would be much better off throwing away pch_gbe_phy.c and write a
proper MDIO driver. You then get the PHY driver for free, and the MDIO
code could will handle your GPIO for you, in the standardised way.

Even if that is true, rewriting the driver's PHY handling would be a
very separate change to the changes this series make which allow this
driver to work on a platform besides the Minnowboard. The *only* thing
this series does relating to the PHY is allow the reset GPIO to be
handled properly - rewriting the existing PHY handling is beyond it's
scope.

Note that I do have various cleanups to the driver beyond this series
which I intend to submit after it is functional for my system[1], so I
am not saying that I don't care about improving the driver. But please,
let's do one thing at a time.

Thanks,
    Paul

[1] https://git.linux-mips.org/cgit/paul/linux.git/log/?h=up417-boston-eth-cleanup