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

[Lee Jones <lee.jones@xxxxxxxxxx>]

On Thu, 04 Apr 2019, Vaittinen, Matti wrote:

> 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.

Great.

> 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.

Have a look for yourself how many there are:

 git grep -A5 inline -- include/linux/

> 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.

Why would anything else try to use it?

There are 1000's of exported functions in the kernel.  If it's
properly namespaced a consumer would have to purposely call it, which
if they really wanted to, they could do anyway.  I don't really see
your point.

> 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'.

No, a call-back function would be a back-step.

Ego or no ego, you're wrong. =:-D

> 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)

Please keep the WDT code in the WDT driver.  Create a little stub for
the cases where the WDT driver is not enabled - job done.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog