On Thu, Jul 25, 2013 at 5:32 PM, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > Tomasz, > > On Thu, Jul 25, 2013 at 3:27 AM, Tomasz Figa <t.figa@xxxxxxxxxxx> wrote: >> Hi Leela, >> >> On Wednesday 24 of July 2013 15:08:17 Leela Krishna Amudala wrote: >>> This patch parses the watchdog node to read pmu wdt sys registers >>> addresses and do mask/unmask enable/disable of WDT in probe and s2r >>> scenarios. >>> >>> Reviewed-by: Doug Anderson <dianders@xxxxxxxxxxxx> >>> Signed-off-by: Leela Krishna Amudala <l.krishna@xxxxxxxxxxx> >>> --- >>> .../devicetree/bindings/watchdog/samsung-wdt.txt | 14 ++++- >>> drivers/watchdog/s3c2410_wdt.c | 56 >>> ++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>> b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt index >>> 2aa486c..4c798e3 100644 >>> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>> @@ -7,8 +7,20 @@ occurred. >>> Required properties: >>> - compatible : should be "samsung,s3c2410-wdt" >>> - reg : base physical address of the controller and length of memory >>> mapped - region. >>> + region and the optional (addresses and length of memory mapped regions >>> + of) PMU registers for masking/unmasking WDT. >>> - interrupts : interrupt number to the cpu. >> >> I don't think PMU registers should be passed like this, even if USB PHY >> driver currently does it - it needs to be fixed. Every time you are mapping >> a single 4-byte register, you are creating new mapping for the whole page >> (4K) containing it. In addition, this makes the WDT driver map IO memory >> that does not belong to the IP block it handles, which is not nice. >> >> I would suggest looking into regmap helpers to implement a driver for the >> PMU that can share PMU registers with interested drivers. > > We've done this in some other drivers as well. I remember someone > suggested that this was fine when I posted the change to the ADC > driver for something similar. > > Do you really think it's worth adding a level of abstraction here? > The argument I remember in the past was that it was fine as long as > the register itself was used only by this driver. > > If we want to do as you say, we'll have to figure out whether you want > to create a new generic interface for this or reuse an existing one. > You could kinda think of these bits as enabling power, so you could > use the regulator framework. ...but that framework often gets abused. > > Olof: I think I got your advice when I did the ADC work. Do you have > any advice here? These tradeoffs are always tricky since it's hard to decide what corners are OK to cut a bit off of, and when you have to be a stickler for really following hard strict rules and stick to abstraction models. I think the more reasonable approach is the one you've taken here, Doug. I think we risk over-abstracting this otherwise, to little actual gain. The register is not shared so there's no conflict in that area. The argument that it wastes a 4K page isn't a big deal either, since we don't do this everywhere all the time. I'd rather have the register described in the reg range like this, than having the driver go out and find the PMU node and find a magic offset into that, with or without an abstraction layer for the PMU register access. -Olof -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
