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.)
>
