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.)
next prev parent reply other threads:[~2020-04-21 20:31 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-21 11:05 [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Evan Benn
2020-04-21 11:05 ` [PATCH v2 1/2] dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog Evan Benn
2020-04-21 11:05 ` [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn
2020-04-21 14:34 ` Guenter Roeck
2020-04-21 20:31 ` Julius Werner [this message]
2020-04-22 1:39 ` Evan Benn
2020-04-22 6:02 ` Xingyu Chen
2020-04-22 22:22 ` Julius Werner
2020-04-22 3:23 ` Guenter Roeck
2020-04-22 4:02 ` Xingyu Chen
2020-04-21 14:32 ` [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Guenter Roeck
2020-04-22 0:40 ` Evan Benn
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=CAODwPW9MtDLSL_up9W0TO1PcjyA_9cUtNo3No7XXusiwqKBLDw@mail.gmail.com \
--to=jwerner@chromium.org \
--cc=Anson.Huang@nxp.com \
--cc=bjorn.andersson@linaro.org \
--cc=catalin.marinas@arm.com \
--cc=davem@davemloft.net \
--cc=evanbenn@chromium.org \
--cc=geert+renesas@glider.be \
--cc=gregkh@linuxfoundation.org \
--cc=leonard.crestez@nxp.com \
--cc=leoyang.li@nxp.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=marcin.juszkiewicz@linaro.org \
--cc=matthias.bgg@gmail.com \
--cc=mchehab+huawei@kernel.org \
--cc=olof@lixom.net \
--cc=robh@kernel.org \
--cc=shawnguo@kernel.org \
--cc=valentin.schneider@arm.com \
--cc=will@kernel.org \
--cc=wim@linux-watchdog.org \
--cc=xingyu.chen@amlogic.com \
/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).