Re: [PATCH v4 0/4] add device drivers for Siemens Industrial PCs

[Henning Schild]

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