Re: [PATCH 1/2] watchdog: sunxi: support parameterized compatible strings

[Guenter Roeck <linux@roeck-us.net>]

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@roeck-us.net> 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@csie.org>
> >
> >
> > 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