Re: [PATCH v11 2/8] mfd: bd70528: Support ROHM bd70528 PMIC - core

[Vaittinen, Matti]

Hello Alexandre,

On Thu, 2019-04-04 at 09:56 +0200, Alexandre Belloni wrote:
> On 04/04/2019 10:24:49+0300, Matti Vaittinen wrote:
> > On Thu, Apr 04, 2019 at 07:54:52AM +0100, Lee Jones wrote:
> > > On Thu, 04 Apr 2019, Vaittinen, Matti wrote:
> > > 
> > > > Actually, now that I thik of it the right way to do this would
> > > > have
> > > > been the function pointer in parent data as was done in
> > > > original patch
> > > > set. HW-colleagues tend to re-use HW blocks, and we like to re-
> > > > use our
> > > > drivers. If the next PMIC from ROHM uses same RTC block but
> > > > does not
> > > > provide watchdog - then it is cleanest solution to fall back to
> > > > function pointer and leave it to NULL when there is no WDT or
> > > > when WDT
> > > > is unused. Another option is to export dummy function - which
> > > > is not so
> > > > nice.
> > > 
> > > I think the converse is true.
> > > 
> > > Pointers to functions outside of a subsystem API context are
> > > generally
> > > horrible.  It's much nicer to call a function which can be easily
> > > stubbed out in a header file based on a Kconfig option.  It's how
> > > most
> > > kernel APIs work.
> > 
> > I hate to admit but I see your point. This nicely solves any issues
> > in
> > syncronizing the startup for driver providing function pointer and
> > for
> > driver using it.
> > 
> 
> Wouldn't it be easier to register the watchdog driver as part of the
> RTC
> driver?
> 
> As I see it, the wdt is just a glorified RTC alarm.

Do you suggest me to put all the stuff now placed in
drivers/watchdog/bd70528_wdt.c into rtc driver? It would be doable -
but I'd rather kept the WDT independent module so that it can be left
out of config if WDT needs not to be used. And same with RTC. Also, re-
use of RTC driver in HW which does not include WDT is easier when WDT
is a separate module. To me it looks much cleaner to have the WDT as
own module than polluting the RTC driver with config ifdefs.

But from HW perspective you are correct. The WDT in BD70528 seems to be
kind of RTC alarm which shuts of the PMIC if triggered.

Br,
	Matti Vaittinen