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

[Xingyu Chen]

Hi,Evan

On 2020/4/22 9:39, Evan Benn wrote:
> On Wed, Apr 22, 2020 at 6:31 AM Julius Werner <jwerner@xxxxxxxxxxxx> 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
>
> .