linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Henning Schild <henning.schild@siemens.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: "Enrico Weigelt, metux IT consult" <lkml@metux.net>,
	Andy Shevchenko <andy.shevchenko@gmail.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>
Subject: Re: [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
Date: Fri, 26 Mar 2021 10:55:42 +0100	[thread overview]
Message-ID: <20210326105542.10122edd@md1za8fc.ad001.siemens.net> (raw)
In-Reply-To: <420f0e08-bec8-f85a-d9af-b9900072df66@redhat.com>

Am Thu, 18 Mar 2021 12:45:01 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi,
> 
> On 3/18/21 12:30 PM, Enrico Weigelt, metux IT consult wrote:
> > On 17.03.21 21:03, Hans de Goede wrote:
> > 
> > Hi,
> >   
> >>> It just identifies the box and tells subsequent drivers which one
> >>> it is, which watchdog and LED path to take. Moving the knowledge
> >>> of which box has which LED/watchdog into the respective drivers
> >>> seems to be the better way to go.
> >>>
> >>> So we would end up with a LED and a watchdog driver both
> >>> MODULE_ALIAS("dmi:*:svnSIEMENSAG:*");  
> > 
> > Uh, isn't that a bit too broad ? This basically implies that Siemens
> > will never produce boards with different configurations.  
> 
> There is a further check done in probe() based on some Siemens
> specific DMI table entries.
> 
> >>> and doing the identification with the inline dmi from that header,
> >>> doing p2sb with the support to come ... possibly a
> >>> "//TODO\ninline" in the meantime.
> >>>
> >>> So no "main platform" driver anymore, but still central platform
> >>> headers.
> >>>
> >>> Not sure how this sounds, but i think making that change should be
> >>> possible. And that is what i will try and go for in v3.  
> >>
> >> Dropping the main drivers/platform/x86 driver sounds good to me,
> >> I was already wondering a bit about its function since it just
> >> instantiates devs to which the other ones bind to then instantiate
> >> more devs (in the LED case).  
> > 
> > hmm, IMHO that depends on whether the individual sub-devices can be
> > more generic than just that specific machine. (@Hanning: could you
> > tell us more about that ?).
> > 
> > Another question is how they're actually probed .. only dmi or maybe
> > also pci dev ? (i've seen some refs to pci stuff in the led driver,
> > but missed the other code thats called here).
> > 
> > IMHO, if the whole thing lives on some PCI device (which can be
> > probed via pci ID), and that device has the knowledge, where the
> > LED registers actually are (eg. based on device ID, pci mmio
> > mapping, ...) then there should be some parent driver that
> > instantiates the led devices (and possibly other board specific
> > stuff). That would be a clear separation, modularization. In that
> > case, maybe this LED driver could even be replaced by some really
> > generic "register-based-LED" driver, which just needs to be fed
> > with some parameters like register ranges, bitmasks, etc.
> > 
> > OTOH, if everything can be derived entirely from DMI match, w/o
> > things like pci mappings involved (IOW: behaves like directly wired
> > to the cpu's mem/io bus, no other "intelligent" bus involved), and
> > it's all really board specific logic (no generic led or gpio
> > controllers involved), then it might be better to have entirely
> > separate drivers.  

In fact it does dmi and not "common" but unfortunately vendor-specific.
On top it does pci, so it might be fair to call it "intelligent" and
keep it.

> FWIW I'm fine with either solution, and if we go the "parent driver"
> route I'm happy to have that driver sit in drivers/platform/x86
> (once all the discussions surrounding this are resolved).
> 
> My reply was because I noticed that the Led driver seemed to sort of
> also act as a parent driver (last time I looked) and instantiated
> a bunch of stuff, so then we have 2 parent(ish) drivers. If things
> stay that way then having 2 levels of parent drivers seems a bit too
> much to me, esp. if it can all be done cleanly in e.g. the LED driver.

One "leds" driver doing multiple leds seems to be a common pattern. So
that "1 parent N children" maybe does not count as parentish.

> But as said I'm fine either way as long as the code is reasonably
> clean and dealing with this sort of platform specific warts happens
> a lot in drivers/platform/x86 .

I thought about it again and also prefer the "parent driver" idea as it
is. That parent identifies the machine and depending on it, causes
device drivers to be loaded. At the moment LED and watchdog, but with
nvram, hwmon to come.

I will stick with "platform" instead of "mfd" because it is really a
machine having multiple devices. Not a device having multiple functions.

regards,
Henning

> Regards,
> 
> Hans
> 


  reply	other threads:[~2021-03-26 10:11 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-15  9:57 [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Henning Schild
2021-03-15  9:57 ` [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
2021-03-15 10:31   ` Andy Shevchenko
2021-03-15 16:30     ` Henning Schild
2021-03-17 19:13     ` Henning Schild
2021-03-17 20:03       ` Hans de Goede
2021-03-18 11:30         ` Enrico Weigelt, metux IT consult
2021-03-18 11:45           ` Hans de Goede
2021-03-26  9:55             ` Henning Schild [this message]
2021-03-26 12:21               ` Hans de Goede
2021-03-15  9:57 ` [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
2021-03-15 10:48   ` Andy Shevchenko
2021-03-15 11:19     ` Pavel Machek
2021-03-15 11:26       ` Andy Shevchenko
2021-03-15 12:40       ` Henning Schild
2021-03-18 11:38       ` Enrico Weigelt, metux IT consult
2021-03-27  9:46         ` Henning Schild
2021-04-01 16:20           ` Enrico Weigelt, metux IT consult
2021-03-27  9:56       ` Henning Schild
2021-03-18 10:27     ` Enrico Weigelt, metux IT consult
2021-03-18 12:40       ` Alexander Dahl
2021-03-23 17:45         ` Henning Schild
2021-03-26 16:33     ` Henning Schild
2021-03-18 10:25   ` Enrico Weigelt, metux IT consult
2021-03-27 11:16     ` Henning Schild
2021-03-27 11:41       ` Henning Schild
2021-03-15  9:57 ` [PATCH v2 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
2021-03-15 15:10   ` Guenter Roeck
2021-03-29 16:28     ` Henning Schild
2021-03-15  9:57 ` [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
2021-03-15 10:14   ` Henning Schild
2021-03-15 10:19     ` Hans de Goede
2021-03-15 12:09       ` Henning Schild
2021-03-15 14:58       ` [PATCH] platform/x86: pmc_atom: use callback for all dmi quirk entries Henning Schild
2021-03-15 16:31         ` Hans de Goede
2021-03-15 17:00           ` Henning Schild
2021-03-15 18:01             ` Hans de Goede
2021-03-16  5:47               ` Henning Schild
2021-03-16  9:43                 ` Hans de Goede
2021-03-15 13:25   ` [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs kernel test robot
2021-03-15 10:55 ` [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Andy Shevchenko
2021-03-15 16:08   ` Henning Schild
2021-08-02  9:21   ` Henning Schild

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210326105542.10122edd@md1za8fc.ad001.siemens.net \
    --to=henning.schild@siemens.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=gerd.haeussler.ext@siemens.com \
    --cc=hdegoede@redhat.com \
    --cc=jan.kiszka@siemens.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=lkml@metux.net \
    --cc=mgross@linux.intel.com \
    --cc=pavel@ucw.cz \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=skrishnakar@gmail.com \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).