Re: [PATCH v4 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

From: Henning Schild <henning.schild@siemens.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: <kai.svahn@intel.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux LED Subsystem <linux-leds@vger.kernel.org>,
	Platform Driver <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>,
	"Hans de Goede" <hdegoede@redhat.com>,
	Pavel Machek <pavel@ucw.cz>, Enrico Weigelt <lkml@metux.net>
Subject: Re: [PATCH v4 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
Date: Fri, 3 Dec 2021 20:35:54 +0100	[thread overview]
Message-ID: <20211203203554.0b43f14f@md1za8fc.ad001.siemens.net> (raw)
In-Reply-To: <YaZAAgDPquDMpvIn@smile.fi.intel.com>

Am Tue, 30 Nov 2021 17:15:14 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Fri, Nov 26, 2021 at 07:12:03PM +0100, Henning Schild wrote:
> > Am Fri, 26 Nov 2021 17:02:00 +0200
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > On Fri, Nov 26, 2021 at 4:10 PM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> > > >
> > > > This driver adds initial support for several devices from
> > > > Siemens. It is based on a platform driver introduced in an
> > > > earlier commit.    
> > > 
> > > ...
> > >   
> > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > > > +       {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > > > +       {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > > > +       {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > > > +       {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > > > +       {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > > > +       {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > > > +       { }
> > > > +};    
> > > 
> > > Like I said, this is not okay.
> > > 
> > > Why can't you simply enable the pinctrl driver and use it?  
> 
> I have talked to my boss today and I have got an approval to
> prioritize the task, so I'm all yours starting from tomorrow.

We had a long and fruitful conversation today. In very short the story
will be that i will send a v5. It will make clear in the cover letter,
in the FIXME, and in commit messages that the P2SB bits are hacky, same
for poking on GPIO memory.
And also say again why it is like that and why it currently can
probably not be done much better.
With these documentation changes Andy said he would be willing to ack.

On top Andy will work on P2SB improvements, and maybe also pinctrl
infrastructure to make the existing drivers actually probe without a
need for ACPI fixes in firmware.

When these patches are ready i will change the Siemens drivers to use
that and take out hackery where possible.

Andy please follow up in case i summarized things wrong, but i bet i do
not have to tell you.

> Let's finish it once for all!

I sure hope we get there!

regards,
Henning

> > I propose we set up a call, that might help clearing up the
> > situation. If you agree please send me an email and possibly
> > propose a time-slot. I would take it from there and send you a
> > meeting link.  
> 
> Sure!
> 