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

[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?

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.


-- 
With Best Regards,
Andy Shevchenko