Re: [RFC V1] drivers/base/regmap: Implementation for regmap_multi_reg_write

[Mark Brown]

On Thu, Feb 27, 2014 at 11:28:56AM +0000, Opensource [Anthony Olech] wrote:
> This is the implementation of regmap_multi_reg_write()
> 
> It replaces the first definition, which just defined the API.

Aside from any review comments this won't apply with the recent patches
that Charles did to provide a bypassed version of the API, it needs to
be rebased.

> a) should an async operation be allowed? easy in the case where
>    all the changes are in the same page - but if the operation
>    is broken due to changes over several pages not so easy.

It's fine to support only the simple cases, async operation is just an
optimisation so we can always just serialise in cases where it gets
complicated and someone can optimise later if they care.  It'd be fine
to just decay to a series of regmap_reg_write()s if there's paging
involved.

> b) the user supplied set (array of struct reg_default) of changes
>    has the register address modified when the target page alters.
>    Would it be better not to do an in-situ change, but rather to
>    alloc a new array of struct reg_default?

Yes, the user should be able to pass in a const pointer (indeed Charles
changed the API to do that).

> +++ b/drivers/base/regmap/regmap.c
> @@ -1442,6 +1442,7 @@ int regmap_field_write(struct regmap_field *field, unsigned int val)
>  }
>  EXPORT_SYMBOL_GPL(regmap_field_write);
>  
> +
>  /**
>   * regmap_field_update_bits():	Perform a read/modify/write cycle

Random whitespace change here.

> +static int _switch_register_page(struct regmap *map, unsigned int  win_page,
> +					struct regmap_range_node *range)
> +{
> +	int ret;
> +	bool page_chg;
> +	void *orig_work_buf = map->work_buf;
> +	unsigned int swp;
> +
> +	map->work_buf = map->selector_work_buf;
> +
> +	swp = win_page << range->selector_shift;
> +	ret = _regmap_update_bits(map,
> +				range->selector_reg,
> +				range->selector_mask,
> +				swp, &page_chg);
> +
> +	map->work_buf = orig_work_buf;
> +

I'd expect this to be using _regmap_select_page()?  In general there
seems like quite a bit of duplication to handle paging.

> +	return ret;
> +}
> +/*

You need a blank here.

> +	buf = kzalloc(len , GFP_KERNEL);
> +
> +	if (!buf)
> +		return -ENOMEM;

Coding style - extra blank between the kzalloc and the check and an
extra space before the comma.

> +	/*
> +	 * the set of registers are not neccessarily in order, but
> +	 * since the order of write must be preserved this algorithm
> +	 * chops the set each time the page changes
> +	 */
> +	for (i = 0, n = 0, switched = false, base = regs; i < num_regs;
> +								i++, n++) {

Don't put all this stuff in the for (), just put the iteration in the
for ().

> +	/*
> +	 * Some devices do not support multi write, for
> +	 * them we have a series of single write operations.
> +	 */
> +	if (map->use_single_rw) {
> +		for (i = 0; i < num_regs; i++) {
> +			ret = _regmap_write(map, regs[i].reg, regs[i].def);
> +			if (ret != 0)
> +				goto out;
> +		}
> +	} else {
> +		ret = _regmap_multi_reg_write(map, regs, num_regs);

I'd expect to see something that devices do to specifically advertise
this capability, it doesn't follow that a device that a device that only
supports single writes will support the multi write operation and
frameworks may try to use the multi write API to help optimise things.

Attachment: signature.asc
Description: Digital signature