Re: [PATCH] leds: add SGI IP30 led support

[Dan Murphy]

Thomas

On 1/16/20 5:05 AM, Thomas Bogendoerfer wrote:
> On Wed, 15 Jan 2020 07:46:13 -0600
> Dan Murphy <dmurphy@xxxxxx> wrote:
>
> > Thomas
> >
> > On 1/15/20 7:05 AM, Thomas Bogendoerfer wrote:
> > > This patch implemenets a driver to support the front panel LEDs of
> > > SGI Octane (IP30) workstations.
> > Thanks for the patch
> >
> > Some nitpicks below
> >
> >
> > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@xxxxxxx>
> > > ---
> > >    drivers/leds/Kconfig     | 11 ++++++
> > >    drivers/leds/Makefile    |  1 +
> > >    drivers/leds/leds-ip30.c | 82 ++++++++++++++++++++++++++++++++++++++++
> > >    3 files changed, 94 insertions(+)
> > >    create mode 100644 drivers/leds/leds-ip30.c
> > >
> > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > index 4b68520ac251..8ef0fe900928 100644
> > > --- a/drivers/leds/Kconfig
> > > +++ b/drivers/leds/Kconfig
> > > @@ -836,6 +836,17 @@ config LEDS_LM36274
> > >    	  Say Y to enable the LM36274 LED driver for TI LMU devices.
> > >    	  This supports the LED device LM36274.
> > >    
> > > +config LEDS_IP30
> > > +	tristate "LED support for SGI Octane machines"
> > > +	depends on LEDS_CLASS
> > > +	depends on SGI_MFD_IOC3
> > What is the dependency on the MFD?
> the gpio lines where the leds are connected are managed by the mfd ioc3 driver.
> So without that driver this led driver will not be started.
Ack
> > > +
> > > +
> > > +	if (num == 0) {
> > > +		data->cdev.name = "ip30:white";
> > This also needs a function as defined in dt-bindings/common.h
> > > +		data->cdev.default_trigger = "default-on";
> > The name, color, function and trigger can be pulled from the DT or Firmware.
> >
> > The firmware should contain a node for each LED to be configured.
> SGI Octanes don't have DT and the firmware just toggles some of the leds,
> but doesn't provide informations about it. That's why this is hardcoded
> in the driver. The MFD driver detects the ioc3 chip and knows it's a
> SGI Octane mainboard and creates the led platform device.
>
> How is the correct way to this without DT ?

Please refer to the leds-class document (1) under LED device naming 
convention

If you don't have a DT then set the name accordingly. The functions are 
defined in the common.h and the colors are exported in the leds-class driver


1 https://www.kernel.org/doc/Documentation/leds/leds-class.rst

> > > +	} else {
> > > +		data->cdev.name = "ip30:red";
> > Same as above
> > > +		data->cdev.default_trigger = "panic";
> > > +		writel(0, data->reg);
> > Is the LED on by default?
> Depends on the hardware state. If PROM firmware detects some hardware issues,
> it turns on the red LED. Otherwise it's off.

So why would we turn it off if the PROM FW turned it on?

Dan