Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset thesystem

[Tomasz Figa]

On Thursday 05 of December 2013 08:40:38 Guenter Roeck wrote:
> On Thu, Dec 05, 2013 at 05:21:47PM +0100, Tomasz Figa wrote:
> > Hi Doug,
> > 
> > Please see my comments inline.
> > 
> > On Monday 02 of December 2013 10:14:41 Doug Anderson wrote:
> > > A good watchdog driver is supposed to report when it was responsible
> > > for resetting the system.  Implement this for the s3c2410, at least on
> > > exynos5250 and exynos5420 where we already have a pointer to the PMU
> > > registers to read the information.
> > > 
> > > Signed-off-by: Doug Anderson <dianders@xxxxxxxxxxxx>
> > > ---
> > > This patch is based atop Leela Krishna's recent series that ends with
> > > (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
> > > AKA <https://patchwork.kernel.org/patch/3251861/>.
> > > 
> > >  drivers/watchdog/s3c2410_wdt.c | 42 +++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 39 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > > index 47f4dcf..2c87d37 100644
> > > --- a/drivers/watchdog/s3c2410_wdt.c
> > > +++ b/drivers/watchdog/s3c2410_wdt.c
> > > @@ -62,9 +62,13 @@
> > >  #define CONFIG_S3C2410_WATCHDOG_ATBOOT		(0)
> > >  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME	(15)
> > >  
> > > +#define RST_STAT_REG_OFFSET		0x0404
> > 
> > IMHO this should be namespaced, at least with EXYNOS5 prefix. The two
> > registers below should be as well, but I missed this in the patch adding
> > them.
> > 
> > >  #define WDT_DISABLE_REG_OFFSET		0x0408
> > >  #define WDT_MASK_RESET_REG_OFFSET	0x040c
> > >  #define QUIRK_NEEDS_PMU_CONFIG		(1 << 0)
> > > +#define QUIRK_HAS_RST_STAT		(1 << 1)
> > > +#define QUIRKS_NEED_PMUREG		(QUIRK_NEEDS_PMU_CONFIG | \
> > > +					 QUIRK_HAS_RST_STAT)
> > >  
> > >  static bool nowayout	= WATCHDOG_NOWAYOUT;
> > >  static int tmr_margin;
> > > @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for debug (default 0)");
> > >   * timer reset functionality.
> > >   * @mask_bit: Bit number for the watchdog timer in the disable register and the
> > >   * mask reset register.
> > > + * @rst_stat_reg: Offset in pmureg for the register that has the reset status.
> > > + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
> > > + * reset.
> > >   * @quirks: A bitfield of quirks.
> > >   */
> > >  
> > > @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
> > >  	int disable_reg;
> > >  	int mask_reset_reg;
> > >  	int mask_bit;
> > > +	int rst_stat_reg;
> > > +	int rst_stat_bit;
> > >  	u32 quirks;
> > >  };
> > >  
> > > @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant drv_data_exynos5250  = {
> > >  	.disable_reg = WDT_DISABLE_REG_OFFSET,
> > >  	.mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
> > >  	.mask_bit = 20,
> > > -	.quirks = QUIRK_NEEDS_PMU_CONFIG
> > > +	.rst_stat_reg = RST_STAT_REG_OFFSET,
> > > +	.rst_stat_bit = 20,
> > > +	.quirks = QUIRK_NEEDS_PMU_CONFIG |
> > > +		QUIRK_HAS_RST_STAT,
> > >  };
> > >  
> > >  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
> > >  	.disable_reg = WDT_DISABLE_REG_OFFSET,
> > >  	.mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
> > >  	.mask_bit = 0,
> > > -	.quirks = QUIRK_NEEDS_PMU_CONFIG
> > > +	.rst_stat_reg = RST_STAT_REG_OFFSET,
> > > +	.rst_stat_bit = 9,
> > > +	.quirks = QUIRK_NEEDS_PMU_CONFIG |
> > > +		QUIRK_HAS_RST_STAT,
> > >  };
> > >  
> > >  static const struct of_device_id s3c2410_wdt_match[] = {
> > > @@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
> > >  }
> > >  #endif
> > >  
> > > +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
> > > +{
> > > +	unsigned int bootstatus = 0;
> > > +	int ret;
> > > +
> > > +	if (wdt->drv_data->quirks & QUIRK_HAS_RST_STAT) {
> > 
> > nit: I guess it's just a matter of taste, but to reduce code indentation
> > you could inverse the check and simply return 0 here.
> > 
> 
> nit: If so, it would make sense to to the same for the other 'quirk'
> for consistency.
> 
> static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
> {
> 	...
> 	if (!(wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG))
> 		return 0;
> 	...
> }

That's quite a bit different story, as currently the check is outside of
this function, but I agree, it would look better.

Best regards,
Tomasz

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