Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
From: Thomas Bogendoerfer <tbogendoerfer@suse.de>
To: Dan Murphy <dmurphy@ti.com>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	Pavel Machek <pavel@ucw.cz>, <linux-kernel@vger.kernel.org>,
	<linux-leds@vger.kernel.org>
Subject: Re: [PATCH] leds: add SGI IP30 led support
Date: Thu, 16 Jan 2020 12:05:45 +0100
Message-ID: <20200116120545.2f4a907b9ddda84e76e445e1@suse.de> (raw)
In-Reply-To: <b5bf7941-3fc1-641d-5482-509eeae34eac@ti.com>

On Wed, 15 Jan 2020 07:46:13 -0600
Dan Murphy <dmurphy@ti.com> 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@suse.de>
> > ---
> >   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

  reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-15 13:05 Thomas Bogendoerfer
2020-01-15 13:46 ` Dan Murphy
2020-01-16 11:05   ` Thomas Bogendoerfer [this message]
2020-01-16 15:21     ` Dan Murphy

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200116120545.2f4a907b9ddda84e76e445e1@suse.de \
    --to=tbogendoerfer@suse.de \
    --cc=dmurphy@ti.com \
    --cc=jacek.anaszewski@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=pavel@ucw.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org
	public-inbox-index linux-leds

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git