Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver

From: Julius Werner <jwerner@chromium.org>
To: Evan Benn <evanbenn@chromium.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Xingyu Chen <xingyu.chen@amlogic.com>,
	Julius Werner <jwerner@chromium.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Anson Huang <Anson.Huang@nxp.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	"David S. Miller" <davem@davemloft.net>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Leonard Crestez <leonard.crestez@nxp.com>,
	Li Yang <leoyang.li@nxp.com>,
	Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
	Olof Johansson <olof@lixom.net>, Rob Herring <robh@kernel.org>,
	Shawn Guo <shawnguo@kernel.org>,
	Valentin Schneider <valentin.schneider@arm.com>,
	Will Deacon <will@kernel.org>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
	<linux-arm-kernel@lists.infradead.org>,
	"moderated list:ARM/Mediatek SoC support" 
	<linux-mediatek@lists.infradead.org>,
	LINUX-WATCHDOG <linux-watchdog@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver
Date: Tue, 21 Apr 2020 13:31:25 -0700	[thread overview]
Message-ID: <CAODwPW9MtDLSL_up9W0TO1PcjyA_9cUtNo3No7XXusiwqKBLDw@mail.gmail.com> (raw)
In-Reply-To: <20200421210403.v2.2.Ia92bb4d4ce84bcefeba1d00aaa1c1e919b6164ef@changeid>

> +static int smcwd_call(unsigned long smc_func_id, enum smcwd_call call,
> +                     unsigned long arg, struct arm_smccc_res *res)

I think you should just take a struct watchdog_device* here and do the
drvdata unpacking inside the function.

> +static int smcwd_probe(struct platform_device *pdev)
> +{
> +       struct watchdog_device *wdd;
> +       int err;
> +       struct arm_smccc_res res;
> +       u32 *smc_func_id;
> +
> +       smc_func_id =
> +               devm_kzalloc(&pdev->dev, sizeof(*smc_func_id), GFP_KERNEL);
> +       if (!smc_func_id)
> +               return -ENOMEM;

nit: Could save the allocation by just casting the value itself to a
pointer? Or is that considered too hacky?

> +static const struct of_device_id smcwd_dt_ids[] = {
> +       { .compatible = "mediatek,mt8173-smc-wdt" },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, smcwd_dt_ids);

So I'm a bit confused about this... I thought the plan was to either
use arm,smc-id and then there'll be no reason to put platform-specific
quirks into the driver, so we can just use a generic "arm,smc-wdt"
compatible string on all platforms; or we put individual compatible
strings for each platform and use them to hardcode platform-specific
differences (like the SMC ID) in the driver. But now you're kinda
doing both by making the driver code platform-independent but still
using a platform-specific compatible string, that doesn't seem to fit
together. (If the driver can be platform independent, I think it's
nicer to have a generic compatible string so that future platforms
which support the same interface don't have to land code changes in
order to just use the driver.)