Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers

From: Vladimir Oltean <olteanv@gmail.com>
To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	netdev <netdev@vger.kernel.org>
Subject: Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers
Date: Thu, 12 Mar 2020 14:54:55 +0200	[thread overview]
Message-ID: <CA+h21ho9wkWC5E+PwAshjtvKEaFuftewYOauG8yPzf_6F8oVFQ@mail.gmail.com> (raw)
In-Reply-To: <20200311203245.GS25745@shell.armlinux.org.uk>

On Wed, 11 Mar 2020 at 22:32, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Mar 11, 2020 at 09:59:18PM +0200, Vladimir Oltean wrote:
> > On Wed, 11 Mar 2020 at 21:32, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > > So, why abuse some other subsystem's datastructure for something that
> > > is entirely separate, potentially making the maintanence of that
> > > subsystem more difficult for the maintainers?  I don't get why one
> > > would think this is an acceptable approach.
> > >
> > > What you've said is that you want to use struct phy_device, but you
> > > don't want to publish it into the device model, you don't want to
> > > use mdio accesses, you don't want to use phylib helpers.  So, what's
> > > the point of using struct phy_device?  I don't see _any_ reason to
> > > do that and make things unnecessarily more difficult for the phylib
> > > maintainers.
> > >
> >
> > So if it's such a big mistake...
> >
> > > > > Sorry, but you need to explain better what you would like to see here.
> > > > > The additions I'm adding are to the SGMII specification; I find your
> > > > > existing definitions to be obscure because they conflate two different
> > > > > bit fields together to produce something for the ethtool linkmodes
> > > > > (which I think is a big mistake.)
> > > >
> > > > I'm saying that there were already LPA_SGMII definitions in there.
> > > > There are 2 "generic" solutions proposed now and yet they cannot agree
> > > > on config_reg definitions. Omitting the fact that you did have a
> > > > chance to point out that big mistake before it got merged, I'm
> > > > wondering why you didn't remove them and add your new ones instead.
> > > > The code rework is minimal. Is it because the definitions are in UAPI?
> > > > If so, isn't it an even bigger mistake to put more stuff in UAPI? Why
> > > > would user space care about the SGMII config_reg? There's no user even
> > > > of the previous SGMII definitions as far as I can tell.
> > >
> > > I don't see it as a big deal - certainly not the kind of fuss you're
> > > making over it.
> > >
> >
> > ...why keep it?
> > I'm all for creating a common interface for configuring this. It just
> > makes me wonder how common it is going to be, if there's already a
> > driver in-tree, from the same PCS hardware vendor, which after the
> > patchset you're proposing is still going to use a different
> > infrastructure.
>
> Do you see any reason why felix_vsc9959 couldn't make use of the code
> I'm proposing?
>

No. But the intentions just from reading the cover letter and the
patches seemed a bit unclear. The fact that there are no proposed
users in this series, and that in your private cex7 branch only dpaa2
uses it, it seemed to me that at least some clarification was due.
I have no further comments. The patches themselves are fairly trivial.

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Thanks
-Vladimir