Re: [PATCH 1/2] regmap: Support half writes and padding between registerand value.

[Jonathan Cameron]

On 09/08/11 17:27, Mark Brown wrote:
> On Thu, Sep 08, 2011 at 03:09:23PM +0100, Jonathan Cameron wrote:
> 
>> Note half writes currently assume address numbers are even only.
>> That's a pain for caching so other suggestions welcome.  I could set
>> it as a 7 bit address and increase the padding to 9 bits.  That makes
>> the write bit a little strange though as it will be going into the
>> padding.
> 
> This needs more documentation somewhere explaining what a half write is
> and/or a better name for half write but doesn't look too invasive so I
> think this approach can work.  If we come up with something better later
> then we can always change things, there shouldn't be too many users to
> update if that happens.
Agreed. I'll document it thoroughly and explain the restricted use case.
> 
>> -	map->format.reg_bytes = config->reg_bits / 8;
>> +	map->format.buf_size = (config->reg_bits +
>> +				config->reg_pad_bits +
>> +				config->val_bits) / 8;
>> +	map->format.reg_bytes = (config->reg_bits + config->reg_pad_bits)/ 8;
> 
> Please do a patch adding padding separately - that does seem like a
> useful thing to have in general, with the option of having it both
> before and after the register.
Happy to do it separately, but not sure the padding before the register
address is a real use case.  You pad after to give the device time to
respond, before doesn't make much sense to me on a register based device...
I'll change the naming to make it explicit that it is after the register though
so as to leave room for this to be added in future.

> 
>> -	if (val == map->work_buf + map->format.reg_bytes)
>> -		ret = map->bus->write(map->dev, map->work_buf,
>> -				      map->format.reg_bytes + val_len);
>> -	else if (map->bus->gather_write)
>> +	if (val == map->work_buf + map->format.reg_bytes) {
>> +		if (map->format.half_write) {
>> +			ret = map->bus->write(map->dev, map->work_buf,
>> +					      (map->format.reg_bytes +
>> +					       val_len) >> 1);
>> +			if (ret >= 0)
>> +				ret = map->bus->write(map->dev,
>> +						      map->work_buf +
>> +						      ((map->format.reg_bytes +
>> +							val_len) >> 1),
>> +						      (map->format.reg_bytes +
>> +						       val_len) >> 1);
>> +		} else {
>> +			ret = map->bus->write(map->dev, map->work_buf,
>> +					      map->format.reg_bytes + val_len);
>> +		}
> 
> This code is getting very complicated...  I think it'd be clearer to
> have a special case at the head of the function that does the half write
> stuff.  It also feels like the half bit needs parameterisation, but I
> can't immediately think of how to do that sensibly.
Likewise. It's a very specific case. In theory there are two parameters that
I can think of.

write_length (bits per write)
address_mangler (one for each write needed?)

I'll clean it up and repost.
> 

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