Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts

[Pavel Machek <pavel@suse.cz>]

Hi!

> The current hdaps driver queries the hardware on (almost) any sysfs
> read, reading just the information it needs and discarding the rest
> This is inefficient, because every hardware query actually gives all 
> information. It also means we're losing data, because readouts are
> offered by the hardware at a constant rate and each query "eats up"
> a readout. It also results in unnecessarily complex code.
> 
> This patch moves all hardware value reading+parsing to a single 
> function, __hdaps_update(). All values are cached, and easily 
> referenced afterwards. This function is still invoked on every sysfs 
> read. This will be fixed in a later patch.
> 
> Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>

> +/* __hdaps_update - read current state and update global state variables.
> + * Also prefetches the next read, to reduce udelay busy-waiting.
> + * If fast!=0, do one quick attempt without retries.
> + * Caller must hold controller lock. 
>   */

Linuxdoc, please.

> +	/* Parse position data: */
> +	pos_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1) * (hdaps_invert?-1:1);
> +	pos_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1) * (hdaps_invert?-1:1);
> +
> +	/* Parse so-called "variance" data: */
> +	var_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS2) * (hdaps_invert?-1:1);
> +	var_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS2) * (hdaps_invert?-1:1);

Perhaps hdaps_invert should already have 1/-1 values.

>  {
> -	int ret = thinkpad_ec_lock();
> +	int ret;
> +	ret = thinkpad_ec_lock();

I actually liked the previous version more, and this change does not
really belong here.

						Pavel
-- 
Thanks for all the (sleeping) penguins.