Re: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Alexandre Belloni <alexandre.belloni@free-electrons.com>]

Hi,

On 01/10/2015 at 19:20:32 +0200, harald@ccbib.org wrote :
> Since the rtc and watchdog drivers need different bits of the same control
> register, we would need to use regmap_fields to pass them around.
> Unfortunately this means we can't use the _SET and _CLR registers, which
> means we need to use regmap everywhere to get proper locking. A lot of
> overhead for no benefit.
> 
> After reading lots of messages around the genesis and usage of
> regmap_mmio,
> it seems to me the intended usecases are to fix some endianness issues
> and to have a register cache available during suspend - both of which
> don't
> apply in this case.
> 

I'd say one of the main advantage of regmap is that it does proper
locking of shared registers and I think that is the main reason of using
it when with platform drivers. Obviously, because you have _SET and _CLR
register on that platform you don't really care about locking.

> If you want to get rid of the callbacks we should just pass the pointer
> to the register block to the child device, I think. Which way is
> preferable
> is probably only a matter of taste so I won't override the driver authors
> decision unless there is some clear statement that this is the preferred
> style in the rtc subsystem.
> 

Ok. Like I said, this is not blocking, it was just a cleanup I was
suggesting.
However, this device is clearly an MFD and that callback feels a bit
hackish. Also registering the watchdog would probably better be done
from the mfd subsystem (this is the only RTC driver doing so).
If you have a look at the history, this was a patch from 2011 taken in
2013 so the kernel had plenty of time to evolve in between ;)

We are dealing with legacy and I'm fine with that driver staying that
way until it is absolutely necessary to change it.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com