Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <453da7af-8a83-f302-eea6-159e6222f430@gmail.com>
Date:   Sun, 7 May 2023 13:16:57 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Content-Language: en-US, en-GB
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
References: <ZFM7lE4ZuDrUTspH@fedora> <20230506190738.0b6e4b45@jic23-huawei>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH] iio: bu27034: Reinit regmap cache after reset
In-Reply-To: <20230506190738.0b6e4b45@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/6/23 21:07, Jonathan Cameron wrote:
> On Thu, 4 May 2023 07:59:00 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> When BU27034 restores the default register values when SWRESET is
>> issued. This can cause register cache to be outdated.
>>
>> Rebuild register cache after SWRESET.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> Fixes: e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")
>>
>> ---
>> I noticed this was missing while writing driver for another light
>> sensor. The change is not tested in hardware as I don't have the BU27034
>> at my hands right now. Careful review would be highly appreciated.
>>
>> This change is built on top of the
>> https://lore.kernel.org/all/ZFIw%2FKdApZe1euN8@fedora/
>> and could probably be squashed with it. Unfortunately I spotted the
>> missing cache re-init only after sending the fix linked above.
>>
> 
> I'm not sure I follow what would be happening here or if this makes sense.
> 
> Partly the following is based on my mental image of how regmap caching works
> and could be completely wrong :)
> 
> I don't think it goes off an reads registers until they are actually accessed
> the first time.

I think this is not the absolute truth. AFAIR the regmap_init may lead 
to regcache_hw_init() - which can read the non volatile registers to the 
cache. I can't say if this is the case with current bu27034 
regmap-config without taking a good look at this with some thought :)

Nevertheless, we know that _if_ there is anything in cache when we do 
reset, the cache will most likely be invalid as HW will reset the 
registers. My thinking was that it is just safest to reinit the cache 
when this happens, then we do not need to care if regcache was populated 
when this is called.

>  In this case nothing has been accessed before this point
> other than the SYSTEM_CONTROL register and that happens to the one that
> is set to trigger the reset.
> 
> So at worst I think the only cache element that will potentially be wrong
> is the SYSTEM_CONTROL register as the cache will contain the reset bits as set.
> 
> That would be a problem if you read it again anywhere in the driver after that
> point, but you don't so not a 'bug' but perhaps prevention of potential future
> bugs as this behaviour is odd.  If you were to try setting some other bits
> then you'd probably accidentally reset the device :)
> 
> So, what's the ideal solution.  You could just bypass the regcache initially
> and turn it on later.

I think I've never temporarily bypassed the cache when I've used one :) 
I need to check how this is done :)

>  Thus it would never become wrong due to the reset (as nothing
> would be cached until after that).
> 
> Or just leave it as you have it here, but explain why it matters - as prevention
> of potential issues due to wrong value in that single register.

Hm. I'd not limit the potential problems to single register as probe may 
get changed - or error handling could be added and reset performed after 
probe. (I was actually thinking of this as the spec states that if VCC 
drops the sensor may go to undefined state and won't recover unless VCC 
is turned off for > 1mS. Didn't add this for now as it is not at all 
obvious the regulator would support detecting under-voltage - or that 
the sensor could really turn-off the regulator as it might be also 
supplying something else - so I didn't want to implement some overkill 
error handling when we might not have hardware which actually benefits 
from this).

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~