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

[Frieder Schrempf]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > >
> > > > 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