Re: [PATCH 0/5] RTL8231 GPIO expander support

From: Sander Vanheule <sander@svanheule.net>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>,
	Lee Jones <lee.jones@linaro.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	"linux-leds@vger.kernel.org" <linux-leds@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 0/5] RTL8231 GPIO expander support
Date: Mon, 17 May 2021 10:50:57 +0200	[thread overview]
Message-ID: <a717bd96509b38bdfd8883847b8d085f9bdd919f.camel@svanheule.net> (raw)
In-Reply-To: <CAHp75VdS63n55b3RG7ZL1=uVgB=7ZiBtTOmV0t+5hw6xKoLFSg@mail.gmail.com>

On Mon, 2021-05-17 at 11:13 +0300, Andy Shevchenko wrote:
> On Mon, May 17, 2021 at 12:40 AM Sander Vanheule <sander@svanheule.net> wrote:
> > On Wed, 2021-05-12 at 18:29 +0300, Andy Shevchenko wrote:
> > > On Tuesday, May 11, 2021, Sander Vanheule <sander@svanheule.net> wrote:
> 
> ...
> 
> > >  * > > > I have several comments to the series, but I may give them next
> > >    week.
> > > 
> > > Just couple here:
> > > 1. If subsystem provides a regmap API I would suggest to use it, I.o.w.
> > > try
> > > again to understand what is wrong with MDIO case.
> > 
> > Are you referring to the MDIO regmap interface, or the GPIO regmap
> > interface?
> 
> MDIO
> 
> > For the MDIO regmap interface, I have been able to resolve the Kconfig
> > dependency issue. So I can reintroduce that, if that's preferred over the
> > solution in this v1.
> > 
> > With an extra patch, I was able to use the gpio-regmap interface, dropping
> > most
> > of the GPIO code. The current gpio-regmap implementation makes the
> > assumption
> > that an output value can be set while a pin is configured as an input. That
> > assumption is invalid for this chip, so I had to provide an extra flag for
> > gpio_regmap_config, similar to how this is handled in gpio-mmio.
> > 
> > 
> > > 2. Please, switch to fwnode API in LED driver
> > 
> > Since you had the same comment on my previous patch set, I had already tried
> > to
> > this this into account as much as possible.
> > 
> > There's a few things I couldn't find the fwnode-equivalent for:
> >  * I use of_node_name_prefix to enforce the naming required by the binding.
> > I
> >    could just walk over all (available) child nodes, which would be mostly
> >    equivalent.
> 
> AFAIU the LED traditional bindings is that you define LED compatible
> nodes and all child nodes of it are the one-per-LED ones, there
> shouldn't be others.

OK, then I can just iterate over all child fwnodes.

> >  * To get the address of an LED child node, I use of_get_address, since this
> >    appeared to provide what I want to do: get the address of the node. I
> > know
> >    next to nothing about ACPI. Does the equivalent exist there? Or am I
> > taking
> >    the wrong approach?
> 
> What are the means of an address in this case?

The chip appears to be intended for use with ethernet switches. The registers
are organised to into a few groups, to provide 2 or 3 status LEDs per switch
port:

 * "LED0" group for 32 ports,
 * "LED1" group for 32 ports,
 * "LED2" group for 24 ports

The number of LEDs that can be used depends on the output mode, so I use a two-
part <#PORT #LED> address, resembling how this is defined by Realtek.

A single linear LED address space would get awkward gaps in bi-color mode (where
only the lower 24 ports can be used), but would still require addresses to be
able to specify which LED is where. For example in case the user want to link
them to a phy trigger for a specific switch port.

Best,
Sander