Re: [PATCH 1/4] rtc: stmp3xxx: add wdt-accessor function

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

Hi Andrew,

> > Andrew, Wim: Alessandro was not around in the last weeks, so I'd suggest this
> > patch goes via the watchdog-tree together with the rest of this series if it
> > passes the review of the watchdog-list? All changes here are in preparation for
> > the watchdog driver and do not affect the general RTC. Is that okay?
> 
> OK by me.  Be careful about the Kconfig setup when doing
> cross-subsystem dependencies like this.  Otherwise you get sad emails
> from Randy when his build breaks.

Yes, the watchdog driver depends on the RTC.

> > +	.name = "stmp3xxx_rtc_wdt",
> > +	.id = -1,
> > +};
> > +
> > +void stmp3xxx_wdt_setup(struct device *dev, u32 timeout)
> 
> I'd suggest documenting `timeout' at least.  Why would one want to set
> it to zero and what effect does that have?  What are its units?

See below.

> The patch adds a global symbol but fails to declare that symbol in a
> header file.  This is always wrong.

I admit the aproach was quite wrong. I exported the function but wanted the
watchdog driver to be the only user (and so the parameters of the function were
quite specific to the watchdog driver). Thus, I kind of tried to hide its
interface for other users. I will try to come up with something better,
probably passing the function via the platform_device created at runtime. I
also played with sharing resources, yet that would require a top-level driver
managing the resources and two sub-level drivers (RTC and watchdog) which feels
unneccessary complex to me.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]