Am Fri, 26 Nov 2021 08:02:11 -0800 schrieb Guenter Roeck <linux@xxxxxxxxxxxx>: > On 11/26/21 7:34 AM, Henning Schild wrote: > > Hi all, > > > > in p3 not too much was left open so i hope this might be the last > > and might be quick. > > > > The two points that have been open where: > > 1 wish to split wdt driver into two > > 2 wish to use pinctrl for LEDs/WDT > > > > 1 was ignored for now. We can split later when we add more devices. > > It remains unclear if splitting is the way to go when more devices > > come. > > The code is already quite messy, in part because memory regions are > declared locally and not passed through the parent device as they > should. Oh is it, can you please be more precise when being so "harsh". The parent driver is just a detector which devices in which variant are available on a given box. It does not drive and should not claim resources i guess. > I don't see how splitting the driver into multiple drivers > would improve the situation. In fact that already is a split i would say. > The platform code claims to be inspired by the lpc_ich driver. Only the P2SB bit, getting the memory base of the pinctrl memory. > Maybe it should take a real example from that > and pass version or variant specific details through platform data > instead of maintaining it in the watchdog driver. It does that ... or i misunderstand you. The driver from p1 can find these three watchdog types SIMATIC_IPC_DEVICE_NONE SIMATIC_IPC_DEVICE_227E SIMATIC_IPC_DEVICE_427E Which one to use is determined in the platform driver that will platform_device_register_data(NULL, KBUILD_MODNAME "_wdt" ... &platform_data) So we tell the watchdog driver about a watchdog and tell it which one ... If we ever split we could register(KBUILD_MODNAME "_wdt_227e" or register(KBUILD_MODNAME "_wdt_427e" with no platform data needed anymore. But as i said, i do not want to split just yet. Just want to defend that code when someone calls it "pretty messy" so late in the process. Henning > Guenter
