netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

  reply	other threads:[~2020-03-12 12:55 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-11 12:06 [PATCH net-next 0/5] add phylink support for PCS Russell King - ARM Linux admin
2020-03-11 12:07 ` [PATCH net-next 1/5] net: mii: convert mii_lpa_to_ethtool_lpa_x() to linkmode variant Russell King
2020-03-11 12:07 ` [PATCH net-next 2/5] net: mii: add linkmode_adv_to_mii_adv_x() Russell King
2020-03-11 12:07 ` [PATCH net-next 3/5] net: mdiobus: add APIs for modifying a MDIO device register Russell King
2020-03-11 12:07 ` [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers Russell King
2020-03-11 14:06   ` Vladimir Oltean
2020-03-11 17:09     ` Russell King - ARM Linux admin
2020-03-11 18:44       ` Vladimir Oltean
2020-03-11 19:32         ` Russell King - ARM Linux admin
2020-03-11 19:59           ` Vladimir Oltean
2020-03-11 20:32             ` Russell King - ARM Linux admin
2020-03-12 12:54               ` Vladimir Oltean [this message]
2020-03-12 13:13                 ` Russell King - ARM Linux admin
2020-03-12 13:35                   ` Vladimir Oltean
2020-03-11 12:07 ` [PATCH net-next 5/5] net: phylink: pcs: add 802.3 clause 45 helpers Russell King
2020-03-11 12:46 ` [PATCH net-next 0/5] add phylink support for PCS Vladimir Oltean
2020-03-11 12:54   ` Russell King - ARM Linux admin
2020-03-11 13:57     ` Vladimir Oltean
2020-03-11 17:05       ` Russell King - ARM Linux admin
2020-03-11 18:16         ` Vladimir Oltean

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CA+h21ho9wkWC5E+PwAshjtvKEaFuftewYOauG8yPzf_6F8oVFQ@mail.gmail.com \
    --to=olteanv@gmail.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=linux@armlinux.org.uk \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).