iio: bu27034: Reinit regmap cache after reset
Subsystems (2)
Matti Vaittinen
maintainerPATCH 1/1 iio: bu27034: Reinit regmap cache after reset
When BU27034 restores the default register values when SWRESET is issued. This can cause register cache to be outdated.
Rebuild register cache after SWRESET.
---
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.
Please, let me know if you want me to squash and respin.
--- drivers/iio/light/rohm-bu27034.c | 7 +++++++ 1 file changed, 7 insertions(+)
drivers/iio/light/rohm-bu27034.c +7 -0
@@ -1281,6 +1281,13 @@ static int bu27034_chip_init(struct bu27034_data *data)return dev_err_probe (data->dev, ret,"Sensor reset failed\n" );msleep (1 ); + + ret =regmap_reinit_cache (data->regmap, &bu27034_regmap); +if (ret) { +dev_err (data->dev,"Failed to reinit reg cache\n" ); +return ret; + } +/* * Read integration time here to ensure it is in regmap cache. We do * this to speed-up the int-time acquisition in the start of the buffer
Signature · 14 lines
2.40.0 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =]
Jonathan Cameron
maintainerQuoted 13 lines · On Thu, 4 May 2023 07:59:00 +0300 Matti Vaittinen <[email protected]>…
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 <[email protected]>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.
Quoted 5 lines
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. 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. 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.
Jonathan
Quoted 23 lines
Please, let me know if you want me to squash and respin.
--- drivers/iio/light/rohm-bu27034.c | 7 +++++++ 1 file changed, 7 insertions(+)drivers/iio/light/rohm-bu27034.c +7
-0@@ -1281,6 +1281,13 @@ static int bu27034_chip_init(struct bu27034_data *data)return dev_err_probe (data->dev, ret,"Sensor reset failed\n" );msleep (1 ); + + ret =regmap_reinit_cache (data->regmap, &bu27034_regmap); +if (ret) { +dev_err (data->dev,"Failed to reinit reg cache\n" ); +return ret; + } +/* * Read integration time here to ensure it is in regmap cache. We do * this to speed-up the int-time acquisition in the start of the buffer
Matti Vaittinen
maintainerQuoted 22 lines · Jonathan Cameron
Quoted 13 lines · On Thu, 4 May 2023 07:59:00 +0300 Matti Vaittinen <[email protected]>…
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 <[email protected]>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.Quoted 5 lines
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.
Quoted 7 lines
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.
Quoted 7 lines
In this case nothing has been accessed before this point
other than theSYSTEM_CONTROLregister 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_CONTROLregister as the cache will contain the reset bits as set.
Quoted 7 lines
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 :)
Quoted 5 lines
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
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
Jonathan Cameron
maintainerQuoted 28 lines · On Sun, 7 May 2023 13:16:57 +0300 Matti Vaittinen <[email protected]>…
Quoted 27 lines · Jonathan Cameron
Quoted 13 lines · On Thu, 4 May 2023 07:59:00 +0300 Matti Vaittinen <[email protected]>…
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 <[email protected]>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.Quoted 5 lines
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 :)
Quoted 7 lines
Quoted 2 lines
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_initmay lead toregcache_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.
Quoted 6 lines
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.
Quoted 13 lines
Quoted 13 lines
In this case nothing has been accessed before this point
other than theSYSTEM_CONTROLregister 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_CONTROLregister 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 :)
Quoted 6 lines
Quoted 2 lines
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);
Quoted 6 lines
Quoted 5 lines
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.
Quoted 10 lines
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
Quoted 4 lines
Yours,
-- Matti
Matti Vaittinen
maintainerQuoted 4 lines · Jonathan Cameron
Quoted 1 line · On Sun, 7 May 2023 13:16:57 +0300 Matti Vaittinen <[email protected]>…
On 5/6/23 21:07, Jonathan Cameron wrote:
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
On Thu, 4 May 2023 07:59:00 +0300
Matti Vaittinen <[email protected]> wrote: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);
Thanks! This is a nice tool to have in my toolbox ;)
Quoted 2 lines
Combine this with the other patch into one clean fix / tidy up though.
Sure, thanks!
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~