On Sun, 7 May 2023 13:16:57 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
> On 5/6/23 21:07, Jonathan Cameron wrote:
> > On Thu, 4 May 2023 07:59:00 +0300
> > Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxx>
> >> 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 :)
I think that's only true if you provide various things you haven't in the
regmap config.
>
> 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.
True, but that's rather heavy weight when we know we only touched one register.
>
> > 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 :)
>
regcache_cache_bypass(map, true / false);
> > 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).
OK. I'm fine with just reinitializing it and paying the penalty of that being
overkill given current code.
Combine this with the other patch into one clean fix / tidy up though.
Jonathan
>
> Yours,
> -- Matti
>
