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 <diand...@chromium.org>
> > ---
> > 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;
...
}
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
