From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.1 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 32A4AC10DCE for ; Thu, 12 Mar 2020 13:13:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id CF21B20663 for ; Thu, 12 Mar 2020 13:13:36 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=armlinux.org.uk header.i=@armlinux.org.uk header.b="gF1G7v7E" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727175AbgCLNNg (ORCPT ); Thu, 12 Mar 2020 09:13:36 -0400 Received: from pandora.armlinux.org.uk ([78.32.30.218]:42676 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725978AbgCLNNf (ORCPT ); Thu, 12 Mar 2020 09:13:35 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=lTvYZWgPWaTaFg1b+bfuiBFIRb/R+MEnYUklQ05qCKc=; b=gF1G7v7EqqFfLm/mk26lPKUNr 47xui8/a6p3o8pXXS6evu0pgeNuiMhKRAyCACaxnprktmgz/DvKOlUPNnZADpLYeXcGgEwjG+zJwH 6bX6KZR4R7dDLjrPX6CK04KbjQ9PlKE5BNTwzrgASbpC5SVO5QPDv3WYRNM5YuN8pEDznFIwfwiy0 f9zn/DR6x6rzi+hd5HeeUpqKBT4I+ZTKOQ/SoAu0IVJ1nuu2NxYvPpq14mLtCgRYzlQiI9b9qYsGt gCJmZp2V2LlJ/lsbOrjvmTb4tmxYKSekrKsEza4dwIzWFox9Ir+aXAsRY1/WTRPML3g7WQm0Fmv4o pjrngGwoA==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:35466) by pandora.armlinux.org.uk with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jCNeT-0001NB-7Z; Thu, 12 Mar 2020 13:13:29 +0000 Received: from linux by shell.armlinux.org.uk with local (Exim 4.92) (envelope-from ) id 1jCNeQ-0006Mn-U3; Thu, 12 Mar 2020 13:13:26 +0000 Date: Thu, 12 Mar 2020 13:13:26 +0000 From: Russell King - ARM Linux admin To: Vladimir Oltean Cc: Andrew Lunn , Florian Fainelli , Heiner Kallweit , "David S. Miller" , netdev Subject: Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers Message-ID: <20200312131326.GA25745@shell.armlinux.org.uk> References: <20200311120643.GN25745@shell.armlinux.org.uk> <20200311170918.GQ25745@shell.armlinux.org.uk> <20200311193223.GR25745@shell.armlinux.org.uk> <20200311203245.GS25745@shell.armlinux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, Mar 12, 2020 at 02:54:55PM +0200, Vladimir Oltean wrote: > On Wed, 11 Mar 2020 at 22:32, Russell King - ARM Linux admin > 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 > > > 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. I have been told by Andrew to send small series, so that's what I do. I have not included the DPAA2 changes in this series because it was not ready for submission - I had to initially hard-code the physical addresses of the MDIO blocks, but I've later moved to describing them in the DTS, which now brings with it additional complexities since (a) existing DTS need to continue working and (b) working out how to submit those changes since the DTS changes and the net changes should go via different paths, and ensuring that no breakage will occur should they become separated. If you look at the branches that I publish, you will notice that they are based on v5.5, and so do not contain your changes to felix_vsc9959 that you have been talking about, so felix_vsc9959 is not yet on my radar. However, it seems we take different approaches to contributing code to the kernel; I look to see whether there is value to providing common infrastructure and then provide it, whereas you seem to take the approach of writing specific drivers and hope that someone else spots the code in your driver and converts it to something generic. I disagree with your approach because it's been well proven over the years that the kernel has been around that relying on others to spot code that could be refactored into common helpers just doesn't happen. Yes, it happens but only occasionally, and not always when common helpers get introduced. You have already proven the worth of having a set of common helpers - it seems that felix_vsc9959 and DPAA2 can both make use of these, which does not surprise me one bit, since these helpers are only implementing what is published in industry standards or defacto industry standards - and as such are likely to be implemented by a lot of vendors. Sure, there will be exceptions and augmentations, which is something I considered when creating these common helpers. That's why they are helpers rather than being mandatory implementations. -- 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