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, 30 Aug 2022 19:35:19 +0300
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     Linus Walleij <linus.walleij@linaro.org>
Cc:     Mark Brown <broonie@kernel.org>, linux-kernel@vger.kernel.org,
        Brian Cain <bcain@quicinc.com>, linux-hexagon@vger.kernel.org,
        Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH 2/2 v5] regmap: mmio: Support accelerared noinc operations
Message-ID: <Yw48R+EBmmZYl9x+@smile.fi.intel.com>
References: <20220816204832.265837-1-linus.walleij@linaro.org>
 <20220816204832.265837-2-linus.walleij@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220816204832.265837-2-linus.walleij@linaro.org>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Tue, Aug 16, 2022 at 10:48:32PM +0200, Linus Walleij wrote:
> Use the newly added callback for accelerated noinc MMIO
> to provide writesb, writesw, writesl, writesq, readsb, readsw,
> readsl and readsq.
> 
> A special quirk is needed to deal with big endian regmaps: there
> are no accelerated operations defined for big endian, so fall
> back to calling the big endian operations itereatively for this
> case.
> 
> The Hexagon architecture turns out to have an incomplete
> <asm/io.h>: writesb() is not implemented. Fix this by doing
> what other architectures do: include <asm-generic/io.h> into
> the <asm/io.h> file.

Wonderful!

So, I have seen a recent blow up by kernel bot due to Alpha issues on these
accessors.

Also see below for the further followups.

...

> +	if (!IS_ERR(ctx->clk)) {
> +		ret = clk_enable(ctx->clk);
> +		if (ret < 0)
> +			return ret;
> +	}

It's a new place of the duplicating check, can we have a helper for that?

...

> +	/*
> +	 * There are no native, assembly-optimized write single register
> +	 * operations for big endian, so fall back to emulation if this
> +	 * is needed. (Single bytes are fine, they are not affected by
> +	 * endianness.)
> +	 */

Wouldn't be faster to memdup() / swap / use corresponding IO accessor?

...

> +	/*
> +	 * There are no native, assembly-optimized write single register
> +	 * operations for big endian, so fall back to emulation if this
> +	 * is needed. (Single bytes are fine, they are not affected by
> +	 * endianness.)
> +	 */
> +	if (ctx->big_endian && (ctx->val_bytes > 1)) {
> +		switch (ctx->val_bytes) {
> +		case 2:
> +		{
> +			u16 *valp = (u16 *)val;
> +			for (i = 0; i < val_count; i++)
> +				valp[i] = swab16(valp[i]);
> +			break;
> +		}
> +		case 4:
> +		{
> +			u32 *valp = (u32 *)val;
> +			for (i = 0; i < val_count; i++)
> +				valp[i] = swab32(valp[i]);
> +			break;
> +		}
> +#ifdef CONFIG_64BIT
> +		case 8:
> +		{
> +			u64 *valp = (u64 *)val;
> +			for (i = 0; i < val_count; i++)
> +				valp[i] = swab64(valp[i]);
> +			break;
> +		}
> +#endif
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +	}

So, two questions here:

1) can we use helpers from include/linux/byteorder/generic.h, such as
   cpu_to_be32_array()/be32_to_cpu_array()?

2) have you considered using memcpy_toio() / memcpy_fromio() and why
   it's not okay to use them?

...

> +
> +

Single blank line is enough.

> +out_clk:
> +	if (!IS_ERR(ctx->clk))
> +		clk_disable(ctx->clk);
> +
> +	return ret;
> +
> +	return 0;

Seems like misrebase? I believe this has to be fixed.

-- 
With Best Regards,
Andy Shevchenko