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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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: signature.asc
Description: Digital signature


[Site Home]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Tools]     [DDR & Rambus]     [Asterisk Internet PBX]     [Linux API]     [Monitors]

Add to Google