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

[Thomas Bogendoerfer]

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.

> > +
> > +
> > +	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 ?

> 
> > +	} 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.

Thomas.

-- 
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Felix Imendörffer