iio: bu27034: Ensure reset is written

PATCH Superseded Matti Vaittinen 2 participants 4 messages May 3, 2023 – May 7, 2023

Subsystems (2)

IIO ROHM BU270xx light sensor DRIVERs

lore

Share & tools

lore

Matti Vaittinen

author maintainer

2023-05-03 10:01 UTC #0 lore raw

PATCH v2 1/1 iio: bu27034: Ensure reset is written

The reset bit must be always written to the hardware no matter what value is in a cache or register. Ensure this by using regmap_write_bits() instead of the regmap_update_bits(). Furthermore, the RESET bit may be self-clearing, so mark the SYSTEM_CONTROL register volatile to guarantee we do also read the right state - should we ever need to read it.

Signed-off-by Matti Vaittinen <[email protected]>

Fixes e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")

---
Changelog:
v1 => v2:
- Fix SoB tag

I haven't verified if the reset bit is self-clearin as I did temporarily give away the HW.

In worst case the bit is not self clearing - but we don't really get performance penalty even if we set the register volatile because the SYSTEM_CONTROL register only has the part-ID and the reset fields. The part-id is only read once at probe.

---
 drivers/iio/light/rohm-bu27034.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/iio/light/rohm-bu27034.c +4 -1

index 25c9b79574a5..740ebd86b6e5 100644
@@ -231,6 +231,9 @@ struct bu27034_result {
 
 static const struct regmap_range bu27034_volatile_ranges[] = {
 	{
+		.range_min = BU27034_REG_SYSTEM_CONTROL,
+		.range_max = BU27034_REG_SYSTEM_CONTROL,
+	}, {
 		.range_min = BU27034_REG_MODE_CONTROL4,
 		.range_max = BU27034_REG_MODE_CONTROL4,
 	}, {

@@ -1272,7 +1275,7 @@ static int bu27034_chip_init(struct bu27034_data *data)
 	int ret, sel;
 
 	/* Reset */
-	ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL,
+	ret = regmap_write_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL,
 			   BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET);
 	if (ret)
 		return dev_err_probe(data->dev, ret, "Sensor reset failed\n");

base-commit: 7fcbd72176076c44b47e8f68f0223c02c411f420

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

maintainer

2023-05-06 18:11 UTC #1 lore raw

Quoted 8 lines · On Wed, 3 May 2023 13:01:32 +0300 Matti Vaittinen <[email protected]>…

The reset bit must be always written to the hardware no matter what value is in a cache or register. Ensure this by using regmap_write_bits() instead of the regmap_update_bits(). Furthermore, the RESET bit may be self-clearing, so mark the SYSTEM_CONTROL register volatile to guarantee we do also read the right state - should we ever need to read it.
Signed-off-by Matti Vaittinen <[email protected]>
Fixes e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")

This obviously interacts with the regcache update.

Fun question is whether a register is volatile if it results in all registers (including itself) resetting. In my view, no it isn't volatile. So fixing the regcache stuff as in your other patch is more appropriate.

Jonathan

Quoted 44 lines

---
Changelog:
v1 => v2:
- Fix SoB tag
I haven't verified if the reset bit is self-clearin as I did temporarily give away the HW.
In worst case the bit is not self clearing - but we don't really get performance penalty even if we set the register volatile because the SYSTEM_CONTROL register only has the part-ID and the reset fields. The part-id is only read once at probe.
---
 drivers/iio/light/rohm-bu27034.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/iio/light/rohm-bu27034.c +4 -1
index 25c9b79574a5..740ebd86b6e5 100644
@@ -231,6 +231,9 @@ struct bu27034_result {
 
 static const struct regmap_range bu27034_volatile_ranges[] = {
 	{
+		.range_min = BU27034_REG_SYSTEM_CONTROL,
+		.range_max = BU27034_REG_SYSTEM_CONTROL,
+	}, {
 		.range_min = BU27034_REG_MODE_CONTROL4,
 		.range_max = BU27034_REG_MODE_CONTROL4,
 	}, {
@@ -1272,7 +1275,7 @@ static int bu27034_chip_init(struct bu27034_data *data)
 	int ret, sel;
 
 	/* Reset */
-	ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL,
+	ret = regmap_write_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL,
 			   BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET);
 	if (ret)
 		return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
base-commit: 7fcbd72176076c44b47e8f68f0223c02c411f420

Matti Vaittinen

author maintainer

2023-05-07 09:59 UTC #2 lore raw

Quoted 9 lines · Jonathan Cameron

Quoted 6 lines · On Wed, 3 May 2023 13:01:32 +0300 Matti Vaittinen <[email protected]>…
The reset bit must be always written to the hardware no matter what value is in a cache or register. Ensure this by using regmap_write_bits() instead of the regmap_update_bits(). Furthermore, the RESET bit may be self-clearing, so mark the SYSTEM_CONTROL register volatile to guarantee we do also read the right state - should we ever need to read it.

Quoted 8 lines

Quoted 2 lines
Signed-off-by Matti Vaittinen <[email protected]>
Fixes e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")
This obviously interacts with the regcache update.
Fun question is whether a register is volatile if it results in all registers (including itself) resetting. In my view, no it isn't volatile. So fixing the regcache stuff as in your other patch is more appropriate.

Hi Jonathan,

I think the key thing here is to ensure writing the reset-bit will always be performed no matter what value is found from cache/hardware. I guess marking the register as volatile is indeed unnecessary, although I don't think it is wrong though, as it underlines we have something special in this register. However, using the write_bits() instead of update_bits() is in my opinion very much "the right thing" to do :)

Yours¸
-- Matti

Quoted 48 lines

Jonathan
Quoted 44 lines
---
Changelog:
v1 => v2:
- Fix SoB tag
I haven't verified if the reset bit is self-clearin as I did temporarily give away the HW.
In worst case the bit is not self clearing - but we don't really get performance penalty even if we set the register volatile because the SYSTEM_CONTROL register only has the part-ID and the reset fields. The part-id is only read once at probe.
---
  drivers/iio/light/rohm-bu27034.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)
drivers/iio/light/rohm-bu27034.c +4 -1
index 25c9b79574a5..740ebd86b6e5 100644
@@ -231,6 +231,9 @@ struct bu27034_result {
 
  static const struct regmap_range bu27034_volatile_ranges[] = {
  	{
+		.range_min = BU27034_REG_SYSTEM_CONTROL,
+		.range_max = BU27034_REG_SYSTEM_CONTROL,
+	}, {
  		.range_min = BU27034_REG_MODE_CONTROL4,
  		.range_max = BU27034_REG_MODE_CONTROL4,
  	}, {
@@ -1272,7 +1275,7 @@ static int bu27034_chip_init(struct bu27034_data *data)
  	int ret, sel;
 
  	/* Reset */
-	ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL,
+	ret = regmap_write_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL,
  			   BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET);
  	if (ret)
  		return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
base-commit: 7fcbd72176076c44b47e8f68f0223c02c411f420

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

maintainer

2023-05-07 13:15 UTC #3 lore raw

Quoted 19 lines · On Sun, 7 May 2023 12:58:52 +0300 Matti Vaittinen <[email protected]>…

Quoted 9 lines · Jonathan Cameron
Quoted 6 lines · On Wed, 3 May 2023 13:01:32 +0300 Matti Vaittinen <[email protected]>…
The reset bit must be always written to the hardware no matter what value is in a cache or register. Ensure this by using regmap_write_bits() instead of the regmap_update_bits(). Furthermore, the RESET bit may be self-clearing, so mark the SYSTEM_CONTROL register volatile to guarantee we do also read the right state - should we ever need to read it.
Quoted 8 lines
Quoted 2 lines
Signed-off-by Matti Vaittinen <[email protected]>
Fixes e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")
This obviously interacts with the regcache update.
Fun question is whether a register is volatile if it results in all registers (including itself) resetting. In my view, no it isn't volatile. So fixing the regcache stuff as in your other patch is more appropriate.

Quoted 8 lines

Hi Jonathan,
I think the key thing here is to ensure writing the reset-bit will always be performed no matter what value is found from cache/hardware. I guess marking the register as volatile is indeed unnecessary, although I don't think it is wrong though, as it underlines we have something special in this register. However, using the write_bits() instead of update_bits() is in my opinion very much "the right thing" to do :)

It's a reasonable change, but whether it's fixing a bug is more complex. If we handle the cache correctly so it always says the bits need writing then there will be no difference between update_bits() and write_bits().

Meh, better safe than sorry.

Jonathan

Quoted 53 lines

Yours¸
-- Matti
Quoted 48 lines
Jonathan
Quoted 44 lines
---
Changelog:
v1 => v2:
- Fix SoB tag
I haven't verified if the reset bit is self-clearin as I did temporarily give away the HW.
In worst case the bit is not self clearing - but we don't really get performance penalty even if we set the register volatile because the SYSTEM_CONTROL register only has the part-ID and the reset fields. The part-id is only read once at probe.
---
  drivers/iio/light/rohm-bu27034.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)
drivers/iio/light/rohm-bu27034.c +4 -1
index 25c9b79574a5..740ebd86b6e5 100644
@@ -231,6 +231,9 @@ struct bu27034_result {
 
  static const struct regmap_range bu27034_volatile_ranges[] = {
  	{
+		.range_min = BU27034_REG_SYSTEM_CONTROL,
+		.range_max = BU27034_REG_SYSTEM_CONTROL,
+	}, {
  		.range_min = BU27034_REG_MODE_CONTROL4,
  		.range_max = BU27034_REG_MODE_CONTROL4,
  	}, {
@@ -1272,7 +1275,7 @@ static int bu27034_chip_init(struct bu27034_data *data)
  	int ret, sel;
 
  	/* Reset */
-	ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL,
+	ret = regmap_write_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL,
  			   BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET);
  	if (ret)
  		return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
base-commit: 7fcbd72176076c44b47e8f68f0223c02c411f420