Re: [PATCH 2/3] nvmem: mtk-efuse: add support for 32-bit aligned reads

[AngeloGioacchino Del Regno]

On 7/15/26 16:27, Roman Vivchar via B4 Relay wrote:
> From: Roman Vivchar <rva333@xxxxxxxxxxxxxx>
>
> Some MediaTek SoCs, such as mt6572, don't support 8-bit reads, leading
> to zeroes or garbage data. 32-bit aligned reads must be used instead.
>
> Introduce an 'needs_aligned_read' field to the platform data to enforce
> 32-bit aligned register access. All reads will be performed by reading
> 4-byte words and masking them.
>
> Signed-off-by: Roman Vivchar <rva333@xxxxxxxxxxxxxx>

You're effectively overriding the entire mtk_reg_read function when
needs_aligned_read==true... so.... (check below)

> ---
>   drivers/nvmem/mtk-efuse.c | 31 ++++++++++++++++++++++++++++---
>   1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c
> index af953e1d9230..1462f2760818 100644
> --- a/drivers/nvmem/mtk-efuse.c
> +++ b/drivers/nvmem/mtk-efuse.c
> @@ -4,6 +4,7 @@
>    * Author: Andrew-CT Chen <andrew-ct.chen@xxxxxxxxxxxx>
>    */
>   
> +#include <linux/align.h>
>   #include <linux/device.h>
>   #include <linux/module.h>
>   #include <linux/mod_devicetable.h>
> @@ -14,10 +15,12 @@
>   
>   struct mtk_efuse_pdata {
>   	bool uses_post_processing;
> +	bool needs_aligned_read;
>   };
>   
>   struct mtk_efuse_priv {
>   	void __iomem *base;
> +	const struct mtk_efuse_pdata *data;

...instead of adding pdata here...

>   };
>   
>   static int mtk_reg_read(void *context,
> @@ -28,6 +31,26 @@ static int mtk_reg_read(void *context,
>   	u8 *val = _val;
>   	int i;
>   
> +	if (priv->data->needs_aligned_read) {
> +		u32 pos, shift, val32;
> +
> +		for (i = 0; i < bytes; i++, val++) {
> +			pos = reg + i;
> +
> +			/*
> +			 * Read on 32-bit word boundary or if it's the first
> +			 * iteration
> +			 */
> +			if (i == 0 || IS_ALIGNED(pos, 4))
> +				val32 = readl(priv->base + (pos & ~3));
> +
> +			shift = (pos & 3) * 8;
> +			*val = (val32 >> shift) & 0xff;
> +		}
> +
> +		return 0;
> +	}
> +
>   	for (i = 0; i < bytes; i++, val++)
>   		*val = readb(addr + i);
>   
> @@ -67,7 +90,6 @@ static int mtk_efuse_probe(struct platform_device *pdev)
>   	struct nvmem_device *nvmem;
>   	struct nvmem_config econfig = {};
>   	struct mtk_efuse_priv *priv;
> -	const struct mtk_efuse_pdata *pdata;
>   	struct platform_device *socinfo;
>   
>   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -78,7 +100,8 @@ static int mtk_efuse_probe(struct platform_device *pdev)
>   	if (IS_ERR(priv->base))
>   		return PTR_ERR(priv->base);
>   
> -	pdata = device_get_match_data(dev);
> +	priv->data = device_get_match_data(dev);
> +

...and instead of changing a bunch of lines here...

>   	econfig.add_legacy_fixed_of_cells = true;
>   	econfig.stride = 1;
>   	econfig.word_size = 1;

it's easier at this point if you simply do

	if (pdata->needs_aligned_read) {
		econfig.stride = 4;
		econfig.reg_read = mtk_reg_read_aligned;
	} else {
		econfig.stride = 1;
		econfig.reg_read = mtk_reg_read;
	}

and you avoid touching mtk_reg_read entirely.

Though it's not going to be used in any performance path, that's also faster as
there's one less branch per execution.

Cheers,
Angelo