Messages in this thread | | | Subject | Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver | From | Guenter Roeck <> | Date | Tue, 21 Apr 2020 20:23:22 -0700 |
| |
On 4/21/20 1:31 PM, Julius Werner 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. > >> +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? >
Actually, the current code is what is hacky. I'd either do what you suggest, or allocate a structure such as
struct local_data { u32 smc_func_id; struct watchdog_device wdd; };
and use it accordingly.
Guenter
>> +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.) >
| |