Re: [PATCH v2] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Lubomir Rintel]

On Sun, 2013-03-24 at 09:12 -0700, Guenter Roeck wrote:
> On Sun, Mar 24, 2013 at 03:22:53PM +0100, Lubomir Rintel wrote:
> > This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> > used in Raspberry Pi and Roku 2 devices.
> > 
> > Interface to the Broadcom BCM2835 watchdog timer hardware is based on
> > "bcm2708_wdog" driver written by Luke Diamand that was obtained from branch
> > "rpi-3.6.y" of git://github.com/raspberrypi/linux.git
> > 
> I would suggest to put that into the coments at the top of the driver.
> Also, if the original code has a copyright statement, you might want
> to copy that.

Okay.

> > +#include <linux/miscdevice.h>
> 
> Just noticed - you should not need that include.

Well, I think the bottommost line of the driver uses that and it's a
good practice to provide a device number alias. Is that one completely
useless?

  MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

> > +#define PM_RSTS				0x20
> 
> Not used

Dropped.

> > +#define PM_RSTC_WRCFG_MASK		0x00000030
> 
> Not used

Dropped.

> > +#define PM_RSTS_HADWRH_SET		0x00000040
> 
> Not used

Dropped.

> > +#define PM_RSTC_WRCFG_SET		0x00000030
> 
> Not used 

Dropped.

> > +#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
> 
> Defined twice

Extra occurrence dropped.

> 
> > +#define PM_RSTC_RESET			0x00000102
> > +
> > +#define SECS_TO_WDOG_TICKS(x) ((x) << 16)
> > +#define WDOG_TICKS_TO_SECS(x) ((x) >> 16)
> > +
> > +struct bcm2835_wdt {
> > +	void __iomem		*base;
> > +	spinlock_t		lock;
> > +};
> > +
> > +static int heartbeat = -1;
> 
> The parameter passed to watchdog_init_timeout is an unsigned int, so you should
> really use an unsigned int here. Depending on -1 being converted to maxuint is a
> bit of side effect programming. Easier to just set it to 0, which the watchdog
> core interprets as invalid as well.

Done.

> > +	dev_info(dev, "Broadcom BCM2835 watchdog timer");
> > +
> I think this should come later, after you are sure that there will be
> no failure.

Moved.

> > +	platform_set_drvdata(pdev, NULL);
> 
> It is unnecessary to set this to NULL (the infrastructure does it for you).

Okay.

I've seen this being done on way too many places. I've really should
have checked that though.

Thank you for your response, I'll submit an updated revision shortly.
-- 
Lubomir Rintel <lkundrak@xxxxx>

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