Guenter, On Thu, Dec 5, 2013 at 8:40 AM, Guenter Roeck <linux@xxxxxxxxxxxx> 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; > ... > } Done in my proposed fixup for Leela Krishna's patch. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
