Re: [RFC RESEND v1 pinctrl-next 0/1] add blink and activity functions to SGPIO

[Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>]

Hi Andrew and Christian,

On Wed, Jul 12, 2023 at 04:55:44PM +0200, Andrew Lunn wrote:
> On Wed, Jul 12, 2023 at 03:59:10AM +0200, Christian Marangi wrote:
> > On Tue, Jul 11, 2023 at 07:22:49PM -0700, Colin Foster wrote:
> > > Preface (new for resend):
> > > 
> > > This is a resend of a patch I'd sent a couple years back. At that time,
> > > I was told to wait for hardware-offloaded LEDS. It looks like that time
> > > has finally come, so I've changed this from PATCH down to an RFC to make
> > > sure this is the right approach for the framework.
> > > 
> > > Ocelot chips (VSC7511, VSC7512, VSC7513, VSC7514) have support for
> > > hardware-offloaded LEDs based on network activity. This is currenty
> > > managed by way of pinctrl-microchip-sgpio (and this current patch).
> > > 
> > > The purpose of this resend is two-fold. First, to come up with an idea
> > > of how this pinctrl-microchip-sgpio module can fit in with the new
> > > hardware-offloaded netdev triggers Christian Marangi recently added. Is
> > > this something that should be in the pinctrl module itself? Or should
> > > there be a drivers/net/ethernet/mscc/ocelot_leds.c module that I should
> > > add?
> > >
> > 
> > I'm a bit out of the loop on what magic OEM did to make LED work on
> > ocelot but I feel an ocelot_leds submodule is needed.

The configuration is basically SPI to an GPIO expander. The ocelot chip
fully manages the SPI bus.

> > 
> > To correctly supports the hw many API needs to be defined and for switch
> > I would stick with how things are done with qca8k, codewise and DT wise
> > (with how LEDs are defined in DT)
> > 
> > Ideally the feature for MAC will be generilized and added to the DSA ops
> > struct, so having things in the DSA driver would make the migration
> > easier.
> 
> `ocelot` is a bit of an odd device, since it is both a DSA device for
> felix and seville and a pure switchdev device for ocelot.

Now you tell me :-)

> 
> You need some integration with the switch driver, because i expect
> only the switch driver has the knowledge of how LEDs are mapped to
> struct netdev and ports. And in order to offload blinking you need
> that mapping.
> 
> I have some WIP patches to add a generalized DSA interface for LEDs,
> and support for mv88e6xxx. I would also like to move qca8k over to
> that. So it could be that felix and seville would use that. Ocelot
> would need to do it slightly different, but i expect it is just a
> layer on top of some shared code, much like the rest of ocelot.

Based on this comment, I might sit on the sidelines for a couple more
cycles.

> 
> Having pinmux in the middle is interesting. I've no idea how that will
> work, but i've not looked at it.

Yeah, I'm not exactly sure either. And integration with the switch
driver will be interesting to say the least. I don't know if Felix /
Seville have this capability (probably not).


I'll keep all this on the back of my mind. It seems like there will
definitely be some subtleties in any way. I figure it is better to bring
up the concept earlier than later, so thanks for the feedback!

> 
>       Andrew