linux-kernel - Re: [PATCH V1] new API regmap_multi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 10 Oct 2013 13:20:48 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Anthony Olech <anthony.olech.opensource@...semi.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	David Dajun Chen <david.chen@...semi.com>
Subject: Re: [PATCH V1] new API regmap_multi_write()

On Thu, Oct 10, 2013 at 11:50:23AM +0100, Anthony Olech wrote:

> +/*
> + * regmap_multi_write(): Write multiple non sequential registers to the device
> + *
> + * @map: Register map to write to
> + * @reg: Array of registers to be written, all on the same page

Why all on the same page?  That doesn't seem helpful for things trying
to build on top of this.  We currently manage to split even block writes
up over page boundaries.

> + * @val: Block of data to be written, in native register size for device
> + * @reg_count: Number of registers to write

I'm not seeing anything here which says how the registers and values are
related to each other.  I assume that the idea is that the same number
of registers are provided as values...

This really doesn't feel like an idiomatic abstraction - it's a bit
cumbersome to have the pair of arrays and try to line them up especially
in native register format, normally we do this with an array
reg_default.  This would also mean that generic code like patches and
cache syncing could pick up on the same functionality.

> +	/*
> +	 * Some devices do not support multi write, for
> +	 * them we have a series of single write operations.
> +	 */
> +	if (map->use_single_rw) {

single_rw is somewhat relevant but this needs a separate flag since...

> +	} else {
> +		ret = _regmap_raw_multi_write(map,
> +					      reg_count,
> +					      reg,
> +					      wval);
> +	}

...this will try to use the new functionality even if the device doesn't
support it.  Indeed what this looks like is support for devices that can
only do single register writes but in the I2C case allow it to be done
without releasing the bus (it looks a lot like someone optimised things
to look like a bunch of SPI register writes, with SPI bouncing /CS is
much quicker than starting a new I2C transfer is).

I can see it being nice to have something like this but it needs more
thought on the API and implementation.  I'd suggest splitting the API
addition from the underlying implementation to make things easier to
review (the API should work for any user even if it just ends up as a
series of separate register writes).  Something like DAPM in ASoC could
use it for example, as well as the patch and cache sync code.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)