Re: [PATCH 1/3] net: phy: mscc: Add possibilty to disable combined LED mode

[Frieder Schrempf <frieder@fris.de>]

Hi Marek,

On 01.10.21 12:09, Marek Behún wrote:
> On Fri, 1 Oct 2021 11:20:36 +0200
> Frieder Schrempf <frieder.schrempf@kontron.de> wrote:
> 
>> On 01.10.21 02:05, Andrew Lunn wrote:
>>> On Thu, Sep 30, 2021 at 02:57:43PM +0200, Frieder Schrempf wrote:
>>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>
>>>> By default the LED modes offer to combine two indicators like speed/link
>>>> and activity in one LED. In order to use a LED only for the first of the
>>>> two modes, the combined feature needs to be disabled.
>>>>
>>>> In order to do this we introduce a boolean devicetree property
>>>> 'vsc8531,led-[N]-combine-disable' and wire it up to the matching
>>>> bits in the LED behavior register.
>>>
>>> Sorry, but no DT property. Each PHY has its own magic combination of
>>> DT properties, nothing shared, nothing common. This does not scale.
>>>
>>> Please look at the work being done to control PHY LEDs using the Linux
>>> LED infrastructure. That should give us one uniform interface for all
>>> PHY LEDs.
>>
>> +Cc: Marek
>>
>> I guess you are referring to this: [1]?
>>
>> If so, the last version I could find is a year old now. Is anyone still
>> working on this?
> 
> Yes, I am still working on this.
> 
> Anyway the last version is not one year old, the last version to add
> this support is 4 months old:
> https://lore.kernel.org/netdev/20210602144439.4d20b295@dellmb/T/

Thanks for pointing out the latest patches. Good to know that you are 
still working on this.

> 
> This version does not add the code for ethernet PHYs, instead it just
> tries to touch only the LED subsystem by adding the API for offloading
> LED triggers and an example implementation for Turris Omnia LED
> controller.
> 
> I will probably send another version this weekend. Sorry this takes
> this long.

No worries, and thanks for the work!

> 
> 
>> I understand, that the generic approach is the one we want to have, but
>> does this really mean adding PHY led configuration via DT to existing
>> drivers (that already use DT properties for LED modes) is not accepted
>> anymore, even if the new API is not yet in place?
> 
> I don't know about Rob, but I would be against it.
> 
> But if you need to have your PHY LED configured with via devicetree
> ASAP, instead of proposing the vendor specific property, you can
> propose LED subnodes and properties that will be generic and compatible
> with the LED subsystem API, i.e. something like:
> 
>    ethernet-phy@1 {
>      .... eth phy properties;
> 
>      leds {
>        led@0 {
>          reg = <0>;
>          color = <LED_COLOR_ID_GREEN>;
>          /* this LED should indicate link/speed */
>          function = LED_FUNCTION_LINK;
>        };
>      };
>    }
> 
> Then make your PHY driver parse this, and make it so that if
> function is LED_FUNCTION_LINK or LED_FUNCTION_ACTIVITY, the driver will
> disable combined mode.
> 
> Afterwards, when LED subsystem has support for trigger offloading, you
> can update mscc driver so that instead of just disabling combined mode,
> it will register the LEDs via LED subsystem...

Good idea, but I'm not really in a hurry. Now knowing that work on the 
trigger offloading is still active, I guess I will just wait a bit until 
the dust has settled and maybe the bindings have been defined. Then I 
can try to implement this in the PHY driver.

Thanks
Frieder