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

[Vaittinen, Matti]

On Thu, 2019-04-04 at 03:52 +0100, Lee Jones wrote:
> On Wed, 03 Apr 2019, Vaittinen, Matti wrote:
> 
> > On Wed, 2019-04-03 at 12:25 +0100, Lee Jones wrote:
> > > On Wed, 03 Apr 2019, Matti Vaittinen wrote:
> > > 
> > > > On Wed, Apr 03, 2019 at 10:30:15AM +0100, Lee Jones wrote:
> > > > > On Wed, 03 Apr 2019, Matti Vaittinen wrote:
> > > > > 
> > > > > > Hello Lee,
> > > > > > 
> > > > > > Thanks for taking a look on this again =) I agree with most
> > > > > > of
> > > > > > the
> > > > > > comments and correct them at next version.
> > > > > > 
> > > > > > On Wed, Apr 03, 2019 at 08:31:52AM +0100, Lee Jones wrote:
> > > > > > > On Mon, 25 Mar 2019, Matti Vaittinen wrote:
> > > > > > > 
> > > > > > > > ROHM BD70528MWV is an ultra-low quiescent current
> > > > > > > > general
> > > > > > > > purpose single-chip power management IC for battery-
> > > > > > > > powered
> > > > > > > > portable devices.
> > > > > > > > 
> > > > > > > > Add MFD core which enables chip access for following
> > > > > > > > subdevices:
> > > > > > > > 	- regulators/LED drivers
> > > > > > > > 	- battery-charger
> > > > > > > > 	- gpios
> > > > > > > > 	- 32.768kHz clk
> > > > > > > > 	- RTC
> > > > > > > > 	- watchdog
> > > > > > > > 
> > > > > > > > Signed-off-by: Matti Vaittinen <
> > > > > > > > matti.vaittinen@xxxxxxxxxxxxxxxxx>
> > > > > > > > + * Mapping of main IRQ register bits to sub irq
> > > > > > > > register
> > > > > > > > offsets so
> > > > > > > 
> > > > > > > "sub-IRQ"
> > > > > > > 
> > > > > > > > + * that we can access corect sub IRQ registers based
> > > > > > > > on
> > > > > > > > bits that
> > > > > > > 
> > > > > > > "sub IRQ" is also fine, but please standardise.
> > > > > > > 
> > > > > > > I do prefer "sub-IRQ" though.
> > > > > > 
> > > > > > I'll go with "sub-IRQ" then
> > > > > > 
> > > > > > > > +
> > > > > > > > +#define WD_CTRL_MAGIC1 0x55
> > > > > > > > +#define WD_CTRL_MAGIC2 0xAA
> > > > > > > > +/**
> > > > > > > > + * bd70528_wdt_set - arm or disarm watchdog timer
> > > > > > > > + *
> > > > > > > > + * @data:	device data for the PMIC instance we
> > > > > > > > want to
> > > > > > > > operate on
> > > > > > > > + * @enable:	new state of WDT. zero to disable, non
> > > > > > > > zero to enable
> > > > > > > > + * @old_state:	previous state of WDT will be filled
> > > > > > > > here
> > > > > > > > + *
> > > > > > > > + * Arm or disarm WDT on BD70528 PMIC. Expected to be
> > > > > > > > called only by
> > > > > > > > + * BD70528 RTC and BD70528 WDT drivers. The
> > > > > > > > rtc_timer_lock
> > > > > > > > must be taken
> > > > > > > > + * by calling bd70528_wdt_lock before calling
> > > > > > > > bd70528_wdt_set.
> > > > > > > > + */
> > > > > > > > +int bd70528_wdt_set(struct rohm_regmap_dev *data, int
> > > > > > > > enable, int *old_state)
> > > > > > > 
> > > > > > > Why doesn't this reside in the watchdog driver?
> > > > > > 
> > > > > > If my memory serves me right we shortly discussed this
> > > > > > already
> > > > > > during v8
> > > > > > review ;) Cant blame you though as I have seen some of the
> > > > > > mail
> > > > > > traffic
> > > > > > going through your inbox :D
> > > > > > 
> > > > > > The motivation to have the functions exported from MFD is
> > > > > > to
> > > > > > not create
> > > > > > sirect dependency between RTC and WDT. There may be cases
> > > > > > where
> > > > > > we want
> > > > > > to leave either RTC or WDT out of compilation. MFD is
> > > > > > always
> > > > > > needed so
> > > > > > the dependency from MFD to RTC/WDT does not harm.
> > > > > > 
> > > > > > (Here's some discussion necromancy if you are interested in
> > > > > > re-
> > > > > > reading
> > > > > > how we did end up with this implementation:
> > > > > > https://lore.kernel.org/lkml/20190212091723.GZ20638@dell/)
> > > > > > 
> > > > > > I hope you are still Ok with having the WDT control
> > > > > > functions
> > > > > > in MFD.
> > > > > 
> > > > > OOI, why does the RTC need to control the WDT?
> > > > 
> > > > I thought I had a comment about this somewhere in code... O_o
> > > > Must
> > > > have
> > > > been in some development branch I had :/
> > > > 
> > > > Anyways, setting the RTC counter may cause watchdog to trigger.
> > > > It
> > > > is not
> > > > further explained why but I would guess watchdog uses RTC
> > > > counter
> > > > to check
> > > > if it should've been pinged already. So RTC needs to disable
> > > > watch
> > > > dog for
> > > > the duration of hwclock setting and enable it again after the
> > > > new
> > > > time is
> > > > set. I can add a comment about this to MFD driver if it helps
> > > > :)
> > > 
> > > How does the user select between using the RTC and the WDT?
> > > 
> > > Or are the generally both enabled at the same time?
> > > 
> > 
> > Both RTC and WDT can be enabled at the same time. But they are not
> > required to be used. When WDT is enabled, it uses current RTC time
> > as
> > 'base' (and RTC time is running no matter if we have the RTC driver
> > here or not) - and time-out gets scheduled to specified amount of
> > time
> > into future. (Same setting timeout into the future happens when WDT
> > is
> > pinged).
> > 
> > When we set RTC, we disable WDT (if it was enabled), set clock and
> > re-
> > enable WDT. This causes the previously used time-out value to be
> > set to
> > WDT again. This works Ok because BD70528 does not support 'short
> > ping
> > detection'. Only side-effect will be one 'prolonged' WDT feeding
> > period
> > when RTC is set. (absolute time when RTC was set minus absolute
> > time
> > when previous WD ping or enable was done) longer than reqular
> > period.
> > 
> > So user should not need to care about this 'dependency'. Basically
> > the
> > only possible problem I see is that someone could accidentally hang
> > the
> > system with something that keeps setting the RTC time - this would
> > then
> > prevent watch dog from doing the reset. This, I believe, is a
> > corner
> > case which I don't consider now - and if this is considered to be
> > an
> > issue then such a system may disable the RTC driver and do RTC
> > setting
> > in a what-ever-manner sees practical.
> > 
> > I'm not sure if I answered to question you asked though =)
> 
> I think you answered it just fine.
> 
> So my suggestion is to have the RTC depend on the WRT via Kconfig,
> and
> place this WRT code in the WRT driver where it belongs.  These
> functions can be exported from the WRT driver and the RTC can call
> into them in the same way it calls into the MFD driver currently.

Yes, we could. But then we need to always compile the watch dog driver
when we want to use RTC. It is not a huge driver though so it probably
won't matter. So, I don't like this but I can do it so if you insist :]

> Avoiding a dependency on the WRT would seem strange, because there is
> one.

The dependency is artificial. It's caused by the current driver design.
If watchdog is not used, then RTC has no reason to touch the watchdog
block. RTC works just fine without watchdog. So from HW point there is
no dependency.

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.

Additional benefit from function pointer would have been that the
function pointer can be only used by drivers which have acces to it.
This exported function is globally visible. The WDT disable/enable is
very specific procedure and it actually would be nicer design to not
have it visible globally. It is not intended to be used by anything
else besides the WDT and RTC here.

But I can't say there will be PMIC with same RTC and no WDT coming from
ROHM. Also, I am not terribly excited about the option of changing this
back to function-pointer as I already removed the pointer from parent
data and this changed parent data is already adapted to all sub drivers
- so this is all just babbling. Maybe it is just my huge ego shouting
there - 'I was right, I must have the final say'.

As a side note, I already did submit v12 with other styling fixes but
which left the WDT function in MFD. If you still see the WDT functions
should be exported from WDT - then please ignore the v12. I'll do v13
at the afternoon (my time, which is only a bit after noon your time I
guess) which will export these functions from WDT. (Well, I had to try
once more :D)

Br,
    Matti Vaittinen