MIME-Version: 1.0
References: <20211213120502.20661-1-henning.schild@siemens.com> <20211213132113.09104069@md1za8fc.ad001.siemens.net>
In-Reply-To: <20211213132113.09104069@md1za8fc.ad001.siemens.net>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 13 Dec 2021 16:37:11 +0200
Message-ID: <CAHp75VfHcQup26Z0h5v=Z7tkBRo0VhyTZBsyztxX5hx-LV55Ng@mail.gmail.com>
Subject: Re: [PATCH v5 0/4] add device drivers for Siemens Industrial PCs
To:     Henning Schild <henning.schild@siemens.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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Dec 13, 2021 at 2:21 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> This is now the outcome of the phone call i had with Andy concerning
> the use of pinctrl. I could lay out why it is not "that easy" and why
> we do access GPIO memory not in a "general purpose" way.
>
> Yes we have thousands of devices out there with missing ACPI entries
> so pinctrl does not come up. But our firmware guys will not add them,
> because they strictly only address security fixes and not functional
> changes. (stable ...)
> Adding new ACPI entries could have unforeseeable side-effects on
> devices already at the end-user, where many (also proprietary low-level
> SW components (OS/hypervisor)) could be in use.
>
> I hope in coming generations we will not forget such entries again, in
> fact it is not even clear who forgot to put them in. Might even have
> been Intel to begin with.

Yep and on top of that we agreed that whenever the better solution
comes out (meaning P2SB generic way of handling whichever it will be),
Siemens would help in testing and will move their driver forward to
support that solution. With all that said,

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Am Mon, 13 Dec 2021 13:04:58 +0100
> schrieb Henning Schild <henning.schild@siemens.com>:
>
> > changes since v4:
> > - make everything around GPIO memory usage more verbose
> >   - commit messages, FIXME in p1, cover-letter
> >
> > changes since v3:
> >
> > - fix io access width and region reservations
> > - fix style in p1
> >
> > changes since v2:
> >
> > - remove "simatic-ipc" prefix from LED names
> > - fix style issues found in v2, mainly LED driver
> > - fix OEM specific dmi code, and remove magic numbers
> > - more "simatic_ipc" name prefixing
> > - improved pmc quirk code using callbacks
> >
> > changes since v1:
> >
> > - fixed lots of style issues found in v1
> >   - (debug) printing
> >   - header ordering
> > - fixed license issues GPLv2 and SPDX in all files
> > - module_platform_driver instead of __init __exit
> > - wdt simplifications cleanup
> > - lots of fixes in wdt driver, all that was found in v1
> > - fixed dmi length in dmi helper
> > - changed LED names to allowed ones
> > - move led driver to simple/
> > - switched pmc_atom to dmi callback with global variable
> >
> >
> > This series adds support for watchdogs and leds of several x86 devices
> > from Siemens.
> >
> > It is structured with a platform driver that mainly does
> > identification of the machines. It might trigger loading of the
> > actual device drivers by attaching devices to the platform bus.
> >
> > The identification is vendor specific, parsing a special binary DMI
> > entry. The implementation of that platform identification is applied
> > on pmc_atom clock quirks in the final patch.
> >
> > It is all structured in a way that we can easily add more devices and
> > more platform drivers later. Internally we have some more code for
> > hardware monitoring, more leds, watchdogs etc. This will follow some
> > day.
> >
> > The LED as well as the watchdog drivers access GPIO memory directly.
> > Using pinctrl is not possible because the machines lack ACPI entries
> > for the pinctrl drivers. Updates to the ACPI tables are not expected.
> > So we can rule out a conflict where two drivers would try and access
> > that GPIO memory.
> > So we do not use those pins as "general purpose" but as "Siemens
> > purpose", after having identified the devices very clearly.
> >
> > Henning Schild (4):
> >   platform/x86: simatic-ipc: add main driver for Siemens devices
> >   leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
> >   watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
> >   platform/x86: pmc_atom: improve critclk_systems matching for Siemens
> >     PCs
> >
> >  drivers/leds/Kconfig                          |   3 +
> >  drivers/leds/Makefile                         |   3 +
> >  drivers/leds/simple/Kconfig                   |  11 +
> >  drivers/leds/simple/Makefile                  |   2 +
> >  drivers/leds/simple/simatic-ipc-leds.c        | 202 ++++++++++++++++
> >  drivers/platform/x86/Kconfig                  |  12 +
> >  drivers/platform/x86/Makefile                 |   3 +
> >  drivers/platform/x86/pmc_atom.c               |  54 +++--
> >  drivers/platform/x86/simatic-ipc.c            | 176 ++++++++++++++
> >  drivers/watchdog/Kconfig                      |  11 +
> >  drivers/watchdog/Makefile                     |   1 +
> >  drivers/watchdog/simatic-ipc-wdt.c            | 228
> > ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h      |
> > 29 +++ include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++
> >  14 files changed, 786 insertions(+), 21 deletions(-)
> >  create mode 100644 drivers/leds/simple/Kconfig
> >  create mode 100644 drivers/leds/simple/Makefile
> >  create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
> >  create mode 100644 drivers/platform/x86/simatic-ipc.c
> >  create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
> >  create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
> >  create mode 100644 include/linux/platform_data/x86/simatic-ipc.h
> >
>


-- 
With Best Regards,
Andy Shevchenko