Re: [PATCH] can: c_can: use regmap_update_bits() to modify RAMINIT register

[Roger Quadros <rogerq@ti.com>]

+Mark,

On 09/01/15 15:50, Tomi Valkeinen wrote:
> On 07/01/15 16:32, Roger Quadros wrote:
>> use of regmap_read() and regmap_write() in c_can_hw_raminit_syscon()
>> is not safe as the RAMINIT register can be shared between different drivers
>> at least for TI SoCs.
>>
>> To make the modification atomic we switch to using regmap_update_bits().
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/net/can/c_can/c_can_platform.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
>> index f363972..364209a 100644
>> --- a/drivers/net/can/c_can/c_can_platform.c
>> +++ b/drivers/net/can/c_can/c_can_platform.c
>> @@ -110,6 +110,10 @@ static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
>>  	 */
>>  	ctrl &= ~(1 << raminit->bits.start);
>>  	ctrl |= 1 << raminit->bits.done;
>> +
>> +	/* we can't use regmap_update_bits here as it will bypass the write
>> +	 * if START is already 0 and DONE is already 1.
>> +	 */
>>  	regmap_write(raminit->syscon, raminit->reg, ctrl);
> 
> Can you first use update_bits to change either bit, so that this update
> will be done?
> 

Initial condition is START=0, DONE=1.

Now setting and clearing START bit initiates the start process and will cause the
DONE bit to be set after a while.
So unfortunately this doesn't work for us.

Mark,

The problem here is we can't use regmap_update_bits() because we need to write a 1 to the DONE bit to clear it and _regmap_update_bits() doesn't allow us to do that because of commit
d91e8db2c3bb regmap: Suppress noop writes in regmap_update_bits()

Is reverting it going to cause other issues? If yes then can we have a flag to specify forced update?

cheers,
-roger
[1] - http://lxr.free-electrons.com/source/drivers/base/regmap/regmap.c#L2332