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: <20210329174928.18816-1-henning.schild@siemens.com>
 <20210329174928.18816-4-henning.schild@siemens.com> <ffdfe9a9-ab17-18af-300e-062b79d132f3@metux.net>
 <20210406165247.78791bf7@md1za8fc.ad001.siemens.net>
In-Reply-To: <20210406165247.78791bf7@md1za8fc.ad001.siemens.net>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 7 Apr 2021 11:53:51 +0300
Message-ID: <CAHp75VcOw0WD8s9ZP=-N5Gesmfx0UEkV5s7SSwDwCzYDYoVuPg@mail.gmail.com>
Subject: Re: [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for
 Siemens Industrial PCs
To:     Henning Schild <henning.schild@siemens.com>
Cc:     "Enrico Weigelt, metux IT consult" <lkml@metux.net>,
        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>,
        Hans de Goede <hdegoede@redhat.com>,
        Pavel Machek <pavel@ucw.cz>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Apr 6, 2021 at 5:56 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> Am Thu, 1 Apr 2021 18:15:41 +0200
> schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:
>
> > On 29.03.21 19:49, Henning Schild wrote:
> >
> > Hi,
> >
> > > This driver adds initial support for several devices from Siemens.
> > > It is based on a platform driver introduced in an earlier commit.
> >
> > Where does the wdt actually come from ?
> >
> > Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty
> > usual case.
> >
> > Or some external chip ?
>
> I guess external chip, but again we are talking about multiple
> machines. And the manuals i read so far do not go into that sort of
> detail. In fact on some of the machines you will have two watchdogs,
> one from the SoC and that "special" one.
> That has several reasons, probably not too important here. The HW guys
> are adding another wd not just for fun, and it would be nice to have a
> driver.
>
> > The code smells a bit like two entirely different wdt's that just have
> > some similarities. If that's the case, I'd rather split it into two
> > separate drivers and let the parent driver (board file) instantiate
> > the correct one.
>
> Yes, it is two. Just like for the LEDs. One version PIO-based another
> version gpio/p2sb/mmio based.

I tend to agree with Enrico that this should be two separate drivers.

> In fact the latter should very likely be based on that gpio pinctl,
> whether it really needs to be a separate driver will have to be seen.
> There are probably pros and cons for both options.


-- 
With Best Regards,
Andy Shevchenko