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

[Henning Schild]

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

> On Thu, Apr 1, 2021 at 1:44 PM Henning Schild
> <henning.schild@xxxxxxxxxxx> wrote:
> >
> > Am Wed, 31 Mar 2021 18:40:23 +0300
> > schrieb Andy Shevchenko <andy.shevchenko@xxxxxxxxx>:
> >  
> > > On Tue, Mar 30, 2021 at 6:33 PM Henning Schild
> > > <henning.schild@xxxxxxxxxxx> wrote:  
> > > > Am Tue, 30 Mar 2021 15:41:53 +0300
> > > > schrieb Andy Shevchenko <andy.shevchenko@xxxxxxxxx>:  
> > > > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> > > > > <henning.schild@xxxxxxxxxxx> wrote:  
> > > > > > Am Tue, 30 Mar 2021 15:15:16 +0300
> > > > > > schrieb Andy Shevchenko <andy.shevchenko@xxxxxxxxx>:  
> > > > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > > > > > <henning.schild@xxxxxxxxxxx> wrote:  
> > > > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > > > > > schrieb Andy Shevchenko <andy.shevchenko@xxxxxxxxx>:  
> > > > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > > > > > <henning.schild@xxxxxxxxxxx> 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?

We still have one such GPIO bit in one wdt, might be "sunrisepoint".
Still have to clarify if that can maybe be dropped for a first step.

> 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.

I now have such an apollolake to play with. Having your series applied
my LEDs driver fails to alloc that mmio memory, so far so correct.
Still have to figure out how to use those GPIOs.

I was hoping to find a gpiochip in sysfs and be able to export gpios to
userland.

Enrico, does "gpio_amd_fch" show up under /sys/class/gpio as a
gpiochip? Or how to interact with that driver before basing another one
on top?

I am afraid that pinctrl-broxton might be loaded but not working as
expected.

Henning