On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > On 3/5/21 4:42 PM, Andy Shevchenko wrote: > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > >> On 3/4/21 11:11 AM, Andy Shevchenko wrote: > >>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > >>> <henning.schild@xxxxxxxxxxx> wrote: ... > >>> 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... > > So I took a quick look at the following 2 commits: (One of the latter commits moves the code to drivers/pci/pci-p2sb.c, do you think it's better like that? The idea is to deduplicate __pci_bus_read_base() call) > "platform/x86: p2sb: New Primary to Sideband bridge support library" > "mfd: lpc_ich: Switch to generic p2sb_bar()" > > And this looks good to me, although compared to the code from this > patch-set you are missing the pci_lock_rescan_remove(); and > pci_unlock_rescan_remove(); calls. Oh, indeed. > > 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). > > The code added by this patch introduces a register_platform_devices() > function which creates a bunch of platform-devices; and then the > device-drivers for those call simatic_ipc_get_membase0() to get their > base-address. Sounds like an MFD approach... > My suggestion was to instead put the simatic_ipc_get_membase0() call > inside the code instantiating the platform devices and to add the > base-address for that pdev as IOMEM resource to the instantiated > platform-devices. > > I hope this helps to clarify what I was trying to say. Yes, thanks! -- With Best Regards, Andy Shevchenko
