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: <b8c19793-8650-b997-e868-3efd1e119b9a@gmail.com>
Date:   Sun, 7 May 2023 12:58:52 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v2] iio: bu27034: Ensure reset is written
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: <ZFIw/KdApZe1euN8@fedora> <20230506192702.57ebf533@jic23-huawei>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
In-Reply-To: <20230506192702.57ebf533@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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! ~~