Re: [RESEND][PATCH] watchdog: add support for Sigma Designs SMP86xx

[Måns Rullgård]

Guenter Roeck <linux@xxxxxxxxxxxx> writes:

>>>> +config TANGOX_WDT
>>>> +	tristate "SMP86xx watchdog"
>>>> +	select WATCHDOG_CORE
>>>> +	depends on ARCH_TANGOX
>>>> +	help
>>>> +	  Watchdog driver for Sigma Designs SMP86xx.
>>>
>>> Not really; it is for SMP8642, and we don't know if other (later ?) chips
>>> will be supported by the same driver. You should be explicit here. More chips
>>> can be added later (that would be needed for the devicetree bindings anyway)
>>> as they are tested.
>>
>> I have tested it on SMP8642 and SMP8759.  The documentation for SMP8654
>> agrees.
>>
>
> We should have that information somewhere - maybe in the driver header.
> It is very useful to know which hardware this was tested with and which
> hardware is supposed to work.

OK, I'll add that info somewhere.

>>>> +static int tangox_wdt_set_timeout(struct watchdog_device *wdt,
>>>> +				  unsigned int new_timeout)
>>>> +{
>>>> +	struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
>>>> +
>>>> +	wdt->timeout = new_timeout;
>>>> +	dev->timeout = 1 + new_timeout * clk_get_rate(dev->clk);
>>>
>>> Why "1 +" ?
>>
>> The counter counts down from the loaded value and asserts the reset pin
>> when it reaches 1.  Setting it to zero disables the watchdog.
>
> You might want to explain that somewhere. Maybe use a define, explain
> it there, and use the define here and below.

Will do.

>>>> +static const struct of_device_id tangox_wdt_dt_ids[] = {
>>>> +	{ .compatible = "sigma,smp8642-wdt" },
>>>
>>> So this is really for smp8642 only, not for any other chips in the series ?
>>
>> It's for about a dozen SMP86xx, SMP87xx, and SMP89xx chips.  Should I
>> list them all?  I don't even know where to find a comprehensive list of
>> device numbers.
>>
> I thought so, but I am not a devicetree expert, and I see some "xx" in
> existing devicetree bindings. Something to ask when you submit the
> bindings to the devicetree mailing list. Either case, I think it would
> be either something like "sigma,smp86xx-wdt" or a list of all of them,
> but not "sigma,smp8642-wdt" to be used for all chips.

The general advice is to not use wildcards in DT bindings since the next
chip matching the pattern might not be compatible at all.  New chips
known to be compatible with an old one can specify both the exact chip
and the older one such that existing drivers will work automatically.
If at some point they are found not to be compatible after all (hardware
bugs, perhaps) a fixed driver will work with existing device trees.

Thanks for the review.

-- 
Måns Rullgård
mans@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/