Message-Id: <20060731233536.92b39035.khali@linux-fr.org>
Date:	Mon, 31 Jul 2006 23:35:36 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	"Shem Multinymous" <multinymous@...il.com>
Cc:	"Vojtech Pavlik" <vojtech@...e.cz>,
	"Brown, Len" <len.brown@...el.com>, "Pavel Machek" <pavel@...e.cz>,
	"Matthew Garrett" <mjg59@...f.ucam.org>,
	"kernel list" <linux-kernel@...r.kernel.org>,
	linux-thinkpad@...ux-thinkpad.org, linux-acpi@...r.kernel.org,
	"Henrique de Moraes Holschuh" <hmh@...ian.org>,
	"Mark Underwood" <basicmark@...oo.com>, "Greg KH" <greg@...ah.com>
Subject: Re: Generic battery interface

Hi Shem,

Disclaimer: I have no idea how the input interface works currently. And
I don't know what problem you are trying to solve. I just thought my
hwmon-oriented comments might help.

> Here's an updated rough proto-spec for the userspace side of the
> continuous-parameter polling interface, intended for hwmon, batteries
> and their ilk, and maybe even the input infrastructure.
> 
> Compared to the parent post, this version adds a second parameter to
> the ioctl (see discussion elsewhere in this thread), and corrects a
> few inaccuracies.
> 
> With an event-based data source, this interface allows polling with no
> time interrupts on tickless kernels. With a polling-based data source
> and a bit of luck, the frequency of polling = frequency of induced
> timer interrupts will be the minimum that satisfies the greediest
> client (even if there are many). In general, complicated data sources
> can be handled optimally by custom driver+infrastructure code. All of
> this is completely transparent  to userspace, which just states its
> needs and has its every desire fulfilled.
> 
> Hardware readouts are obtained from a dedicated file - a sysfs
> attribute (as in hwmon and tp_smapi) or a device file (as in the input
> infrastructure). The file has the following properties:
> 
> 1. A new ioctl DELAYED_UPDATE, with parameters min_wait and
>    min_fresh, meaning: "I want an a fresh readout. If I poll() this FD
>    with POLLIN then send an input-ready event at time is T+min_wait, or
>    when you have a readout that was received from the hardware at  time
>    T+min_fresh, whichever is *later*. Likewise if I select()".
>    Here T is the time of the ioctl call and min_wait>=min_fresh.

"A or B, whichever is later", effectively means "A and B". Or am I
missing something? I fail to see the difference between min_wait and
min_fresh.

> 2. When the file is opened in O_NONBLOCKing mode, read() always return
>    the latest cached readout rather than querying the hardware
>    (unless the latter has negligible cost).
> 3. When the file is opened in normal (blocking) mode, read() blocks
>    until a fresh readout is available and returns this readout; see
>    below.

The hwmon interface (sysfs now, procfs before) has been returning
cached values by defaut for years. Changing this at this point might be
confusing. I don't see much benefit in waiting for updated values
compared to reading them from the cache. The driver knows better how
frequently it can read from the chip. And no hardware monitoring chip I
know of can tell when the monitored value has changed - you have to read
the chip registers to know.

> To illustrate, here's an example of a proper polling loop (sans error
> checking). This app wants to refresh its display when the data has
> changed, but not more often than once per second. It wants the
> readouts to be reasonly spaced: they should be obtained at least 700ms
> apart. And it needs to update its GUI every 3 seconds regardless of
> readouts.

I don't see the point in the 700ms rule. If you don't want new data
more often than once per second, the readouts will be spaced by one
second, which implies > 700ms already.

> Application code:
> --------------------------
> /* Open attribute file with O_NONBLOCK so that all reads will
> * return cached values instead of blocking:
> */
> int fd = open("/whatever/voltage", O_NONBLOCK|O_RDONLY);
> 
> /* Read and process latest cached attribute value: */
> read(fd, ...);
> ...
> 
> while (1) {
> 	const struct delayed_update_req dureq =
> 		{ .min_wait=1000, min_fresh=700 };
> 	struct pollfd ufds = { .fd=fd, events=POLLIN };
> 
> 	/* Tell the driver we want a fresh readout, but no sooner than
> 	 * 1sec from now, and we want the readout to reflect reality no
> 	 * sooner than 700ms now:
> 	 */
> 	ioctl(fd, DELAYED_UPDATE, &dureq);
> 
> 	/* Wait for an update for at most 3 seconds. Nominally this will
> 	* block for at least 1 second, because of the above ioct. If we
> 	* were reading multiple attributes, we could poll them all
> 	* simultaneously.
> 	*/
> 	poll(&ufds, 1, 3000);
> 
> 	/* Read latest cached attribute value: */

No seek(fd, ..., 0) here? sysfs files are supposed to be simple text
files, aren't they?

> 	read(fd, ...);
> 
> 	... handle readout, update GUI ...
> }

-- 
Jean Delvare
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists