Received-SPF: Pass (protection.outlook.com: domain of siemens.com designates
 194.138.21.70 as permitted sender) receiver=protection.outlook.com;
 client-ip=194.138.21.70; helo=hybrid.siemens.com;
Date:   Mon, 20 Dec 2021 08:53:55 +0100
From:   Henning Schild <henning.schild@siemens.com>
To:     Pavel Machek <pavel@ucw.cz>
CC:     Hans de Goede <hdegoede@redhat.com>,
        <linux-kernel@vger.kernel.org>, <linux-leds@vger.kernel.org>,
        <platform-driver-x86@vger.kernel.org>,
        <linux-watchdog@vger.kernel.org>,
        Srikanth Krishnakar <skrishnakar@gmail.com>,
        Jan Kiszka <jan.kiszka@siemens.com>,
        "Gerd Haeussler" <gerd.haeussler.ext@siemens.com>,
        Guenter Roeck <linux@roeck-us.net>,
        Wim Van Sebroeck <wim@linux-watchdog.org>,
        Mark Gross <mgross@linux.intel.com>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        Enrico Weigelt <lkml@metux.net>
Subject: Re: [PATCH v5 2/4] leds: simatic-ipc-leds: add new driver for
 Siemens Industial PCs
Message-ID: <20211220085220.23753298@md1za8fc.ad001.siemens.net>
In-Reply-To: <20211219164903.GB28481@duo.ucw.cz>
References: <20211213120502.20661-1-henning.schild@siemens.com>
        <20211213120502.20661-3-henning.schild@siemens.com>
        <20211215201800.GA28336@duo.ucw.cz>
        <61983e62-bc3d-a711-c197-7436a2e33531@redhat.com>
        <20211219164903.GB28481@duo.ucw.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Dec 2021 07:54:01.8753
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: a569fc84-f2cb-49a7-f15b-08d9c38de337
X-MS-Exchange-CrossTenant-Id: 38ae3bcd-9579-4fd4-adda-b42e1495d55a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=38ae3bcd-9579-4fd4-adda-b42e1495d55a;Ip=[194.138.21.70];Helo=[hybrid.siemens.com]
X-MS-Exchange-CrossTenant-AuthSource: HE1EUR01FT059.eop-EUR01.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM9PR10MB4385
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am Sun, 19 Dec 2021 17:49:03 +0100
schrieb Pavel Machek <pavel@ucw.cz>:

> On Wed 2021-12-15 21:53:56, Hans de Goede wrote:
> > Hi,
> > 
> > On 12/15/21 21:18, Pavel Machek wrote:  
> > > On Mon 2021-12-13 13:05:00, Henning Schild wrote:  
> > >> This driver adds initial support for several devices from
> > >> Siemens. It is based on a platform driver introduced in an
> > >> earlier commit.
> > >>
> > >> One of the supported machines has GPIO connected LEDs, here we
> > >> poke GPIO memory directly because pinctrl does not come up.
> > >>
> > >> Signed-off-by: Henning Schild <henning.schild@siemens.com>  
> > > 
> > > Acked-by: Pavel Machek <pavel@ucw.cz>  
> > 
> > I see that this patch #includes
> > linux/platform_data/x86/simatic-ipc-base.h which gets added by
> > patch 1/4.
> > 
> > Pavel, can I take this patch upstream through the pdx86 tree (with
> > you Ack added)? Or shall I prepare an immutable branch with patch 1
> > for you to merge ?  
> 
> Yes, you can.
> 
> 
> > >> +
> > >> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > >> +	{1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
> > >> +	{1 << 7,  "yellow:" LED_FUNCTION_STATUS "-1" },
> > >> +	{1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
> > >> +	{1 << 6,  "yellow:" LED_FUNCTION_STATUS "-2" },
> > >> +	{1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
> > >> +	{1 << 5,  "yellow:" LED_FUNCTION_STATUS "-3" },
> > >> +	{ }
> > >> +};  
> 
> But I'd still like better naming than red:status-2.

We had the name discussion already several times, and i have to admit i
am not too happy either.

But my impression was that this is an acceptable compromise. I am not
happy because the names lack scope, which i had in the first round with
"simatic-ipc:red:...".

Function is also a bit unclear, but with the numbers and the user
manual, or looking at the chassis it kind of adds up and should be
clear to users which is which.

But i agree with Hans that we should sort this out before merge. So
please say what makes you unhappy, maybe that can be fixed ... might
even make me happier about the names i feel i had to choose.

The LEDs are per definition of the manuals meant for users/applications
to signal whatever the use-case might want to signal. There are 3 of
them numbered 1-3 on the chassis, and next to the number can often (not
always) be found a string like "error", "maint", "run-stop"
So a function suggestion i would say.

I could envision to use "fault" or "alarm" instead of "status" for the
one labeled "error". And maybe "standby" for the one called "maint" but
i would really like to keep the numbers.

Which would look like

status-1
alarm-2
standby-3

But still i have to clue what those names stand for and choosing
and of those "undefined" names could just suggest things and break
expectations. Calling them all "status" is neutral ... 

Or can you explain the difference between "fault", "panic" and "alarm".
Ask 5 people and get at least 3 different expectations ... i guess.

Henning


> 								Pavel