Messages in this thread | | | Date | Sun, 7 May 2023 12:58:52 +0300 | Subject | Re: [PATCH v2] iio: bu27034: Ensure reset is written | From | Matti Vaittinen <> |
| |
On 5/6/23 21:27, Jonathan Cameron wrote: > On Wed, 3 May 2023 13:01:32 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> 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 <mazziesaccount@gmail.com> >> 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
> > Jonathan > >> >> --- >> 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(-) >> >> diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c >> index 25c9b79574a5..740ebd86b6e5 100644 >> --- a/drivers/iio/light/rohm-bu27034.c >> +++ b/drivers/iio/light/rohm-bu27034.c >> @@ -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 Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |