Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
From: Dan Murphy <dmurphy@ti.com>
To: Thomas Bogendoerfer <tbogendoerfer@suse.de>
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 09:21:59 -0600
Message-ID: <4a6dd4c6-4719-6662-a5ac-eef9a4112c2e@ti.com> (raw)
In-Reply-To: <20200116120545.2f4a907b9ddda84e76e445e1@suse.de>

Thomas

On 1/16/20 5:05 AM, Thomas Bogendoerfer wrote:
> 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.
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



      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
2020-01-16 15:21     ` Dan Murphy [this message]

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=4a6dd4c6-4719-6662-a5ac-eef9a4112c2e@ti.com \
    --to=dmurphy@ti.com \
    --cc=jacek.anaszewski@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --cc=tbogendoerfer@suse.de \
    /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