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

[Evan Benn]

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.

Thanks

Evan