From: Xingyu Chen <xingyu.chen@amlogic.com>
To: Evan Benn <evanbenn@chromium.org>, Julius Werner <jwerner@chromium.org>
Cc: LKML <linux-kernel@vger.kernel.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>,
Jianxin Pan <jianxin.pan@amlogic.com>,
Yonghui Yu <yonghui.yu@amlogic.com>,
Xingyu Chen <xingyu.chen@amlogic.com>
Subject: Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver
Date: Wed, 22 Apr 2020 14:02:21 +0800 [thread overview]
Message-ID: <bbd8f97b-8479-c38e-011f-b52e2cf34403@amlogic.com> (raw)
In-Reply-To: <CAKz_xw3-tvYoCDs07xEUBBMf024aezGMoZ35LsTN+_dXT9nC2Q@mail.gmail.com>
Hi,Evan
On 2020/4/22 9:39, Evan Benn wrote:
> On Wed, Apr 22, 2020 at 6:31 AM Julius Werner <jwerner@chromium.org> wrote:
>>
>>> +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.
>
> That makes sense, I avoided it because smcwd_call's are made during
> 'probe', ~while
> we are 'constructing' the wdd. But this is C, so I think I have
> permission to do this!
>
>>> +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?
>
> I am not yet used to what hacks are allowed in the kernel.
> Where I learned C that would not be allowed.
> I assumed the kernel allocator has fast paths for tiny sizes though.
>
>>> +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.)
>
> Yes I think you are correct. I got some reviews about the compatible name,
> but I think I misinterpreted those, and arm,smc-wdt would work. I did wonder
> if Xingyu from amlogic needed to modify the driver more, EG with different
> SMCWD_enum values for the amlogic chip, he could then just add an
> amlogic compatible
> and keep our devices running with the other compatible. Although of
> course it would be nicer if
> the amlogic firmware could copy the values here.
Using DTS property(arm,smc-id) or vendor's compatible to specify the
Function ID is available for the meson-A1.The generic "arm, smc-wdt"
looks nicer for MTK and Amlogic sec wdt, but the driver may not cover
the other vendor's platform-specific differences in the future, so the
platform-specific compatible string may be introduced eventually.
But again, I can accept the two methods above.
Thanks
>
> Thanks
>
> Evan
>
> .
>
next prev parent reply other threads:[~2020-04-22 6:01 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
2020-04-22 1:39 ` Evan Benn
2020-04-22 6:02 ` Xingyu Chen [this message]
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=bbd8f97b-8479-c38e-011f-b52e2cf34403@amlogic.com \
--to=xingyu.chen@amlogic.com \
--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=jianxin.pan@amlogic.com \
--cc=jwerner@chromium.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=yonghui.yu@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).