Received-SPF: pass (google.com: domain of linux-kernel+bounces-152321-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sat, 20 Apr 2024 18:04:08 +0200
From: Andrew Lunn <andrew@lunn.ch>
To: Josua Mayer <josua@solid-run.com>
Cc: Michael Hennerich <michael.hennerich@analog.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Alexandru Tachici <alexandru.tachici@analog.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	Jon Nettleton <jon@solid-run.com>,
	Yazan Shhady <yazan.shhady@solid-run.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next 2/2] net: phy: adin: add support for setting
 led-, link-status-pin polarity
Message-ID: <6d843f6f-01f9-4ac9-a661-af452f5ab623@lunn.ch>
References: <20240419-adin-pin-polarity-v1-0-eaae8708db8d@solid-run.com>
 <20240419-adin-pin-polarity-v1-2-eaae8708db8d@solid-run.com>
 <65411c68-c76a-499d-88c7-e80ca59a3027@lunn.ch>
 <fb13743f-a1f6-44b7-9659-882976f0bc7d@solid-run.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <fb13743f-a1f6-44b7-9659-882976f0bc7d@solid-run.com>

> Hi Andrew,
> 
> That looks very much related!
> 
> I was already planning to investigate adding led support ... .
> 
> 1. for the? LINK_ST pin I believe we still need a non-led-framework
> device property for setting polarity, as it is a fixed function signal
> that we can't even turn on or off from software.

We should separate the device tree binding from the implementation of
the binding. The binding describes the hardware. The hardware is
active low. And the binding can describe that. So i don't see a need
for anything vendor specific.

I think the real question is, can the current generic LED code be made
to handle this LED, or should you have code in the PHY driver to
handle this binding in a specific way for this LED?

Given the restrictions on this LED, i don't think it makes sense to
expose it in /sys/class/leds. But is it possible to leverage the
framework to parse the binding and call the polarity function?

> 2. LED_0 control not currently supported by adin driver.
> The phy supports what data-sheet calls extended configuration
> (disabled by default) for controlling led state (on, off, patterns).
> 
> Since it is not default, I see the polarity setting separate from leds.
> However I do believe the led_polarity_set callback is an acceptable
> solution.

Again, you should use the LED binding, even if you don't use the LED
framework. I just wounder how much code you will duplicate if you do
decide to implement the binding in the driver. And when you fully
implement the control of the LED using the framework, are you going to
throw the code away again?

       Andrew