Date:   Thu, 2 Mar 2023 15:55:54 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Michael Walle <michael@walle.cc>
Cc:     Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC net-next] net: phy: intel-xway: remove LED
 configuration
Message-ID: <ZAC4+kjSxLtxMZDR@lunn.ch>
References: <20230302141651.377261-1-michael@walle.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230302141651.377261-1-michael@walle.cc>
Precedence: bulk

On Thu, Mar 02, 2023 at 03:16:51PM +0100, Michael Walle wrote:
> For this PHY, the LEDs can either be configured through an attached
> EEPROM or if not available, the PHY offers sane default modes. Right now,
> the driver will configure to a mode just suitable for one configuration
> (although there is a bold claim that "most boards have just one LED").
> I'd argue, that as long as there is no configuration through other means
> (like device tree), the driver shouldn't configure anything LED related
> so that the PHY is using either the modes configured by the EEPROM or
> the power-on defaults.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> I know there is ongoing work on the device tree, but even then, my
> argument holds, if there is no config in the device tree, the driver
> shouldn't just use "any" configuration when there are means by the
> hardware to configure the LEDs.
> 
> Not just as an RFC because netdev is closed, but also to get other
> opinions. Not to be applied.

I would suggest you CC: the two people responsible for adding this
code:

    Signed-off-by: John Crispin <john@phrozen.org>
    Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

So i guess this came from OpenWRT? Maybe they can tell us if this
change is going to cause regressions.

I would say there is no right defaults for LEDs, whatever you do will
be wrong for somebody. And the way this driver sets the LEDs has been
there since the first commit. This is its decision of what the default
should be. So i'm leaning towards rejecting your definition of what
the default should be.

There is progress on controlling PHY LEDs via /sysfs. So i think you
should wait until Christian and I get the API between the core and the
PHY driver stable, and then help us by implementing support in the
intel-xway.

	Andrew