linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Henning Schild <henning.schild@siemens.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: 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 v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
Date: Mon, 12 Apr 2021 17:15:40 +0200	[thread overview]
Message-ID: <20210412171540.19154dea@md1za8fc.ad001.siemens.net> (raw)
In-Reply-To: <CAHp75VcU-7-BVum4xuuQcG7NZZc9xXOoXYpfSBUwwPr6iZLWGg@mail.gmail.com>

Am Thu, 1 Apr 2021 14:04:51 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thu, Apr 1, 2021 at 1:44 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > Am Wed, 31 Mar 2021 18:40:23 +0300
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> >  
> > > On Tue, Mar 30, 2021 at 6:33 PM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> > > > Am Tue, 30 Mar 2021 15:41:53 +0300
> > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> > > > > <henning.schild@siemens.com> wrote:  
> > > > > > Am Tue, 30 Mar 2021 15:15:16 +0300
> > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > > > > > <henning.schild@siemens.com> wrote:  
> > > > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > > > > > <henning.schild@siemens.com> wrote:  
> > > > >  
> > > > > > > > > > +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"},
> > > > > > > > > > +       { }
> > > > > > > > > > +};  
> > > > > > > > >
> > > > > > > > > It seems to me like poking GPIO controller registers
> > > > > > > > > directly. This is not good. The question still
> > > > > > > > > remains: Can we simply register a GPIO (pin control)
> > > > > > > > > driver and use an LED GPIO driver with an additional
> > > > > > > > > board file that instantiates it?  
> > > > > > > >
> > > > > > > > I wrote about that in reply to the cover letter. My
> > > > > > > > view is still that it would be an abstraction with only
> > > > > > > > one user, just causing work and likely not ending up as
> > > > > > > > generic as it might eventually have to be.
> > > > > > > >
> > > > > > > > The region is reserved, not sure what the problem with
> > > > > > > > the "poking" is.  
> > > > > > >
> > > > > > >  
> > > > > > > > Maybe i do not understand all the benefits of such a
> > > > > > > > split at this point in time. At the moment i only see
> > > > > > > > work with hardly any benefit, not just work for me but
> > > > > > > > also for maintainers. I sure do not mean to be
> > > > > > > > ignorant. Maybe you go into details and convince me or
> > > > > > > > we wait for other peoples opinions on how to proceed,
> > > > > > > > maybe there is a second user that i am not aware of?
> > > > > > > > Until i am convinced otherwise i will try to argue that
> > > > > > > > a single-user-abstraction is needless work/code, and
> > > > > > > > should be done only when actually needed.  
> > > > > > >
> > > > > > > I have just read your messages (there is a cover letter
> > > > > > > and additional email which was sent lately).
> > > > > > >
> > > > > > > I would like to know what the CPU model number on that
> > > > > > > board is. Than we can continue to see what possibilities
> > > > > > > we have here.  
> > > > > >
> > > > > > I guess we are talking about the one that uses memory
> > > > > > mapped, that is called an "IPC127E" and seems to have
> > > > > > either Intel Atom E3940 or E3930 which seems to be Apollo
> > > > > > Lake.  
> > > > >
> > > > > Yep. And now the question, in my patch series you should have
> > > > > got the apollolake-pinctrl driver loaded (if not, we have to
> > > > > investigate why it's not being instantiated). This will give
> > > > > you a read GPIO driver.  
> > > >
> > > > Ok, so there is the existing driver i asked about several times.
> > > > Thanks for pointing it out.  
> > >
> > > If you remember, I asked you about the chip twice :-)
> > > I assumed that we were talking about Apollo Lake and that's why I
> > > insisted that the driver is in the kernel source tree.  
> >
> > Sorry, maybe i did not get the context of your question and which of
> > the machines you asked about. Now it is clear i guess.
> >  
> > >  
> > > > > So, you may use regular LED GPIO on top of it
> > > > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> > > > > I would like to understand why it can't be achieved.  
> > > >
> > > > Will have a look. Unfortunately this one box is missing in my
> > > > personal collection, but let us assume that one can be
> > > > converted to that existing driver.  
> > >
> > > OK!
> > >  
> > > > I guess that will still mean the PIO-based part of the LED
> > > > driver will have to stay as is.  
> > >
> > > Probably yes. I haven't looked into that part and I have no idea
> > > what's going on on that platform(s).
> > >  
> >
> > Which i guess means the series can be reviewed as if the mmio bits
> > for that apollo lake would not be in it, maybe i will even send a
> > version without that one box. We have others in the "backlog" might
> > as well delay that one if it helps sorting out a base-line.  
> 
> It depends on the role of P2SB in this case.
> Shouldn't you drop that completely out from this series?

Unfortunately the WDT uses one P2SB-GPIO pin as well (for 1 of the two
machine types it supports). Dropping would mean loosing 1/5 machines in
LED and 2/4 in WDT. So i rather let this series sit until the P2SB
stuff is sorted out.
But that would just be my personal "preference". We could go "divide
and conquer", shrink the number of supported devices and drop all that
needs P2SB ... also a valid way, we have the platform-abstraction to
build upon ... and we would get p4 out of the way. 

Henning

> Otherwise we have to understand what to do with it.
> It seems the best approach can be to expose the P2SB device to Linux,
> but we have to answer to Bjorn's request about region reservations.
> 
> 


  parent reply	other threads:[~2021-04-12 15:16 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-29 17:49 [PATCH v3 0/4] add device drivers for Siemens Industrial PCs Henning Schild
2021-03-29 17:49 ` [PATCH v3 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
2021-11-26 12:39   ` Henning Schild
2021-03-29 17:49 ` [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
2021-03-30 11:04   ` Andy Shevchenko
2021-03-30 11:58     ` Henning Schild
2021-03-30 12:15       ` Andy Shevchenko
2021-03-30 12:30         ` Henning Schild
2021-03-30 12:41           ` Andy Shevchenko
2021-03-30 15:23             ` Henning Schild
2021-03-31 15:40               ` Andy Shevchenko
2021-04-01 10:44                 ` Henning Schild
2021-04-01 11:04                   ` Andy Shevchenko
2021-04-12 11:56                     ` Henning Schild
2021-05-05 14:58                       ` Enrico Weigelt, metux IT consult
2021-04-12 15:15                     ` Henning Schild [this message]
2021-11-26 13:28     ` Henning Schild
2021-11-26 14:02       ` Andy Shevchenko
2021-11-26 14:44         ` Henning Schild
2021-11-26 14:59           ` Andy Shevchenko
2021-11-26 19:54             ` Henning Schild
2021-11-25 17:11   ` Henning Schild
2021-03-29 17:49 ` [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
2021-04-01 16:15   ` Enrico Weigelt, metux IT consult
2021-04-06 14:52     ` Henning Schild
2021-04-07  8:53       ` Andy Shevchenko
2021-04-07 12:17         ` Guenter Roeck
2021-11-26 13:18           ` Henning Schild
2021-04-12 15:35     ` Henning Schild
2021-04-12 16:06       ` Guenter Roeck
2021-04-12 16:17         ` Henning Schild
2021-11-25 17:08   ` Henning Schild
2021-11-25 17:10   ` Henning Schild
2021-03-29 17:49 ` [PATCH v3 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
2021-03-29 18:00 ` [PATCH v3 0/4] add device drivers for Siemens Industrial PCs Henning Schild
2021-04-07 11:36 ` Hans de Goede
2021-04-12 11:27   ` Henning Schild
2021-07-12 11:35   ` Henning Schild
2021-07-12 12:09     ` Andy Shevchenko
2021-07-12 16:11       ` Henning Schild

Reply instructions:

You may reply publicly 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=20210412171540.19154dea@md1za8fc.ad001.siemens.net \
    --to=henning.schild@siemens.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=gerd.haeussler.ext@siemens.com \
    --cc=hdegoede@redhat.com \
    --cc=jan.kiszka@siemens.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=lkml@metux.net \
    --cc=mgross@linux.intel.com \
    --cc=pavel@ucw.cz \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=skrishnakar@gmail.com \
    --cc=wim@linux-watchdog.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).