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

[Lee Jones <lee.jones@linaro.org>]

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@fi.rohmeurope.com>
> > > + * 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?

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