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;
Date:   Sat, 13 May 2023 19:23:13 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Matti Vaittinen <mazziesaccount@gmail.com>
Cc:     Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] iio: bu27034: Ensure reset is written
Message-ID: <20230513192313.57e1dd21@jic23-huawei>
In-Reply-To: <ZFjWhbfuN5XcKty+@fedora>
References: <ZFjWhbfuN5XcKty+@fedora>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 8 May 2023 14:01:25 +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 SWRESET 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.
> 
> Finally, writing the SWRESET bit will restore the default register
> values. This can cause register cache to be outdated if there are any
> register values cached.
> 
> Rebuild register cache after SWRESET and use regmap_update_bits() when
> performing the reset.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Fixes: e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")

Still unclear to me if we should treat the register as volatile as hitting
relevant bits causes a reset of the whole device anyway, but meh, it doesn't
cause any problems and it's your driver.

Applied to the fixes-togreg branch of iio.git

Thanks,

Jonathan

> 
> ---
> v3: Combined patches:
> ("iio: bu27034: Ensure reset is written")
> https://lore.kernel.org/lkml/ZFIw%2FKdApZe1euN8@fedora/
> ("iio: bu27034: Reinit regmap cache after reset")
> https://lore.kernel.org/lkml/ZFM7lE4ZuDrUTspH@fedora/
> ---
>  drivers/iio/light/rohm-bu27034.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
> index 25c9b79574a5..f85194fda6b0 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,12 +1275,19 @@ 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");
>  
>  	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