Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210302163309.25528-1-henning.schild@siemens.com>
 <20210302163309.25528-2-henning.schild@siemens.com> <CAHp75VfDDGxdhP0-yKOCJyJ_+Y2Zu3TmOdvUJmEZ0AvQnceV6A@mail.gmail.com>
 <2fad304a-9e1e-c83d-7a9e-02b35ed22418@redhat.com>
In-Reply-To: <2fad304a-9e1e-c83d-7a9e-02b35ed22418@redhat.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 5 Mar 2021 17:42:42 +0200
Message-ID: <CAHp75VfB8v1n3Hav_oMqG0k4C31NBEUe082i8NrrOGUbSgoESw@mail.gmail.com>
Subject: Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for
 Siemens devices
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     Henning Schild <henning.schild@siemens.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>,
        Pavel Machek <pavel@ucw.cz>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 3/4/21 11:11 AM, Andy Shevchenko wrote:
> > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> > <henning.schild@siemens.com> wrote:

...

> >> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
> >> +{
> >> +       u32 bar0 = 0;
> >
> >> +#ifdef CONFIG_PCI
> >
> > It's ugly besides the fact that you have a dependency.
> >
> >> +       struct pci_bus *bus;
> >
> > Missed blank line.
> >
> >> +       /*
> >> +        * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device
> >> +        * to have a quick look at it, before we hide it again.
> >> +        * Also grab the pci rescan lock so that device does not get discovered
> >> +        * and remapped while it is visible.
> >> +        * This code is inspired by drivers/mfd/lpc_ich.c
> >> +        */
> >> +       bus = pci_find_bus(0, 0);
> >> +       pci_lock_rescan_remove();
> >> +       pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
> >> +       pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
> >> +
> >> +       bar0 &= ~0xf;
> >> +       pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
> >> +       pci_unlock_rescan_remove();
> >> +#endif /* CONFIG_PCI */
> >> +       return bar0;
> >> +}
> >> +EXPORT_SYMBOL(simatic_ipc_get_membase0);
> >
> > Oy vey! I know what this is and let's do it differently. I have some
> > (relatively old) patch series I can send you privately for testing.
>
> This bit stood out the most to me too, it would be good if we can this fixed
> in some cleaner work. So I'm curious how things will look with Andy's work
> integrated.
>
> Also I don't think this should be exported. Instead this (or its replacement)
> should be used to get the address for an IOMEM resource to add the platform
> devices when they are instantiated. Then the platform-dev drivers can just
> use the regular functions to get their resources instead of relying on this
> module.

I have published a WIP branch [1]. I have no means to test (I don't
know what hardware at hand I can use right now), but I made it compile
after 4 years of gathering dust...
Feel free to give any kind of comments or share your ideas on how it
can be improved (the above idea on IOMEM resource is interesting, but
devices are PCI, not sure how this can be done).

[1]: https://gitlab.com/andy-shev/next/-/tree/p2sb

-- 
With Best Regards,
Andy Shevchenko