On Fri, Sep 19, 2014 at 10:00:36PM +0800, Chen-Yu Tsai wrote: > Hi, > > On Fri, Sep 19, 2014 at 12:54 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > > On 09/16/2014 07:42 AM, Chen-Yu Tsai wrote: > >> > >> This patch adds support for hardware parameters tied to compatible > >> strings, so similar hardware can reuse the driver. > >> > >> This will be used to support the newer watchdog found in A31 and > >> later SoCs. Differences in the new hardware include separate > >> interrupt lines for each watchdog, and corresponding interrupt > >> control/status registers. Watchdog control registers were also > >> slightly rearranged. > >> > > You might also mention that you replace some iowrite32 with writel. > > Ah.... Had that in mind, but forgot about it when finishing the series :( > > >> Signed-off-by: Chen-Yu Tsai <wens@xxxxxxxx> > > > > > > Have you considered using regmap ? Seems to be an ideal candidate. > > I don't follow. Do you mean using regmap to share the interrupt registers > with the timer on sun4i? Otherwise I don't really see a difference. > Use regmap instead of direct writel. Nice thing about it is that regmap offers bit manipulation functions, so you could replace the read / mask / modify /write in a single regmap function call. See imx2_wdt.c for an example how it is used. > Am I missing something? > Not really. Just a suggestion. > >> > >> - /* Enable timer and set reset bit in the watchdog */ > >> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE); > >> + /* Set lowest timeout and enable watchdog */ > >> + val = readl(wdt_base + regs->wdt_mode); > >> + val &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift); > >> + val |= WDT_MODE_EN; > > > > > > I think it would make sense to also define WDT_MODE_EN and > > WDT_TIMEOUT_MASK as configurable, even if not currently needed. > > It is odd to have the shift configurable but not the mask, > > and to have the mode register configurable but not the mode value. > > I think keeping these values constants clearly shows that they are > the same and shared among the hardware. Making them configurable > shouldn't impose much penalty, but I'd prefer to keep them as is > until we need to change them. > Ok, guess we have a different philosophy ;-). I'll leave it up to you. Guenter -- 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
