lkml.org 
[lkml]   [2020]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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.)

\
 
 \ /
  Last update: 2020-04-21 22:33    [W:0.210 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site