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

[Guenter Roeck]

On 11/13/2015 08:53 AM, Måns Rullgård wrote:
> Guenter Roeck <linux@xxxxxxxxxxxx> writes:
>
> > On 11/13/2015 05:14 AM, Mans Rullgard wrote:
> > > This adds support for the Sigma Designs SMP86xx family built-in
> > > watchdog.
> > >
> > > Signed-off-by: Mans Rullgard <mans@xxxxxxxxx>
> > > ---
> > >    drivers/watchdog/Kconfig      |   7 ++
> > >    drivers/watchdog/Makefile     |   1 +
> > >    drivers/watchdog/tangox_wdt.c | 185 ++++++++++++++++++++++++++++++++++++++++++
> >
> > Why tangox_wdt instead of smp86xx_wdt.c ?
> >
> > tangox also implies that this would (should) work for SMP87xx as well,
> > about which no statement is made. So why not tango3_wdt ?
> >
> > [ ok, I see all drivers are named tangox, so if the other maintainers
> >    are ok with that, so am I. ]
> >
> > Is it known if the driver will work for any of the other chips of the
> > series (SMP86XX/SMP87XX) ?
>
> It does work on SMP87xx (tango4) as well.  I wrote the driver before I
> had any such hardware, then forgot to update the help text and commit
> message.
>
> > I think it would be helpful to describe in more detail which chips
> > are supported, or at least which chips should work but are untested.
> >
> > >    3 files changed, 193 insertions(+)
> > >    create mode 100644 drivers/watchdog/tangox_wdt.c
> > >
> > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > index 79e1aa1..0ed5ee8 100644
> > > --- a/drivers/watchdog/Kconfig
> > > +++ b/drivers/watchdog/Kconfig
> > > @@ -1337,6 +1337,13 @@ config RALINK_WDT
> > >    	help
> > >    	  Hardware driver for the Ralink SoC Watchdog Timer.
> > >
> > > +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.

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

> > > +static int tangox_wdt_restart(struct notifier_block *nb, unsigned long action,
> > > +			      void *data)
> > > +{
> > > +	struct tangox_wdt_device *dev =
> > > +		container_of(nb, struct tangox_wdt_device, restart);
> > > +
> > > +	writel(1, dev->base + WD_COUNTER);
> > > +
> >
> > A comment might be useful here, explaining what this does (reset after minimum timeout ?).
> > Also, the code should wait a bit to ensure that the reset 'catches'
> > before the function returns.
>
> Writing 1 to the counter asserts the reset immediately.
>
> > > +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.

As for which chips to list, the easy answer would be to only list the IDs
for chips known to work.

Thanks,
Guenter

--
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/