Messages in this thread | | | From | Julius Werner <> | Date | Tue, 21 Apr 2020 13:31:25 -0700 | Subject | Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver |
| |
> +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.)
| |