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:   Tue, 19 Jul 2022 12:54:02 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
CC:     Mark Brown <broonie@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        <linux-kernel@vger.kernel.org>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>
Subject: Re: [PATCH] regmap: support regmap_field_write() on non-readable
 fields
Message-ID: <20220719125401.GA92394@ediswmail.ad.cirrus.com>
References: <20220719121446.375095-1-krzysztof.kozlowski@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220719121446.375095-1-krzysztof.kozlowski@linaro.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Tue, Jul 19, 2022 at 02:14:46PM +0200, Krzysztof Kozlowski wrote:
> Current implementation of regmap_field_write() performs an update of
> register (read+write), therefore it ignores regmap read-restrictions and
> is not suitable for write-only registers (e.g. interrupt clearing).
> 
> Extend regmap_field_write() and regmap_field_force_write() to check if
> register is readable and only then perform an update.  In the other
> case, it is expected that mask of field covers entire register thus a
> full write is allowed.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ---
> 
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/base/regmap/regmap.c | 50 ++++++++++++++++++++++++++++++++++++
>  include/linux/regmap.h       | 15 ++---------
>  2 files changed, 52 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 0caa5690c560..4d18a34f7b2c 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -2192,6 +2192,56 @@ int regmap_noinc_write(struct regmap *map, unsigned int reg,
>  }
>  EXPORT_SYMBOL_GPL(regmap_noinc_write);
>  
> +static int _regmap_field_write_or_update(struct regmap_field *field,
> +					 unsigned int val, bool *change,
> +					 bool async, bool force)
> +{
> +	unsigned int mask = (~0 << field->shift) & field->mask;
> +	unsigned int map_val_mask, map_val_mask_h;
> +	int ret;
> +
> +	if (regmap_readable(field->regmap, field->reg))
> +		return regmap_update_bits_base(field->regmap, field->reg,
> +					       mask, val << field->shift,
> +					       change, async, force);
> +

I think this will break other valid use-cases, regmap_readable (I
believe) returns if the register is physically readable, however
it should still be possible to use update bits if the register is
in the cache even if it can't physically be read. So really you
need to fall into this path if it is readable or in the cache.

Which does I guess also raise the question if your problem would
be better solved with caching the register?

Thanks,
Charles