Re: [PATCH v1 5/6] soc: mediatek: mutex: Add mtk_mutex_set_mod support to set MOD1

From: AngeloGioacchino Del Regno  <angelogioacchino.delregno@collabora.com>
To: Moudy Ho <moudy.ho@mediatek.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Chun-Kuang Hu <chunkuang.hu@kernel.org>
Cc: linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	Project_Global_Chrome_Upstream_Group@mediatek.com,
	"Roy-CW.Yeh" <roy-cw.yeh@mediatek.com>
Subject: Re: [PATCH v1 5/6] soc: mediatek: mutex: Add mtk_mutex_set_mod support to set MOD1
Date: Tue, 4 Oct 2022 14:38:02 +0200	[thread overview]
Message-ID: <42ab09ef-427d-f2dd-c480-f3e11f4aaa79@collabora.com> (raw)
In-Reply-To: <20221004093319.5069-6-moudy.ho@mediatek.com>

Il 04/10/22 11:33, Moudy Ho ha scritto:
> From: "Roy-CW.Yeh" <roy-cw.yeh@mediatek.com>
> 
> Add mtk_mutex_set_mod support to set MOD1
> 
> Signed-off-by: Roy-CW.Yeh <roy-cw.yeh@mediatek.com>
> ---
>   drivers/soc/mediatek/mtk-mutex.c | 27 +++++++++++++++++++--------
>   1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-mutex.c b/drivers/soc/mediatek/mtk-mutex.c
> index c1a33d52038e..5dcbd61fe42c 100644
> --- a/drivers/soc/mediatek/mtk-mutex.c
> +++ b/drivers/soc/mediatek/mtk-mutex.c
> @@ -23,6 +23,7 @@
>   #define DISP_REG_MUTEX(n)			(0x24 + 0x20 * (n))
>   #define DISP_REG_MUTEX_RST(n)			(0x28 + 0x20 * (n))
>   #define DISP_REG_MUTEX_MOD(mutex_mod_reg, n)	(mutex_mod_reg + 0x20 * (n))
> +#define DISP_REG_MUTEX_MOD1(mutex_mod_reg, n)	((mutex_mod_reg) + 0x20 * (n) + 0x4)
>   #define DISP_REG_MUTEX_SOF(mutex_sof_reg, n)	(mutex_sof_reg + 0x20 * (n))
>   #define DISP_REG_MUTEX_MOD2(n)			(0x34 + 0x20 * (n))
>   
> @@ -750,14 +751,24 @@ int mtk_mutex_write_mod(struct mtk_mutex *mutex,
>   		return -EINVAL;
>   	}
>   
> -	offset = DISP_REG_MUTEX_MOD(mtx->data->mutex_mod_reg,
> -				    mutex->id);
> -	reg = readl_relaxed(mtx->regs + offset);
> -
> -	if (clear)
> -		reg &= ~BIT(mtx->data->mutex_table_mod[idx]);
> -	else
> -		reg |= BIT(mtx->data->mutex_table_mod[idx]);
> +	if (mtx->data->mutex_table_mod[idx] < 32) {

What if we do...

	u8 id_offset = 0;

	/*
	 * Some SoCs may have multiple MUTEX_MOD registers as more than 32 mods
	 * are present, hence requiring multiple 32-bits registers.
	 *
	 * The mutex_table_mod fully represents that by defining the number of
	 * the mod sequentially, later used as a bit number, which can be more
	 * than 0..31.
	 *
	 * In order to retain compatibility with older SoCs, we perform R/W on
	 * the single 32 bits registers, but this requires us to translate the
	 * mutex ID bit accordingly.
	 */
	if (mtx->data->mutex_table_mod[idx] < 32) {
		reg_offset = DISP_REG_MUTEX_MOD0(mtx->data->mutex_mod_reg,
						 mutex->id);
	} else {
		reg_offset = DISP_REG_MUTEX_MOD1(mtx->data->mutex_mod_reg,
						 mutex->id);
		id_offset = 32;
	}

	reg = readl_relaxed(mtx->regs + offset);
	if (clear)
		reg &= ~BIT(mtx->data->mutex_table_mod[idx] - id_offset);
	else
		reg |= BIT(mtx->data->mutex_table_mod[idx] - id_offset);

	writel_relaxed(reg, mtx->regs + offset);

...like this, we give good documentation and also keep the code "short" :-)

Regards,
Angelo

> +		offset = DISP_REG_MUTEX_MOD(mtx->data->mutex_mod_reg,
> +					    mutex->id);
> +		reg = readl_relaxed(mtx->regs + offset);
> +		if (clear)
> +			reg &= ~BIT(mtx->data->mutex_table_mod[idx]);
> +		else
> +			reg |= BIT(mtx->data->mutex_table_mod[idx]);
> +
> +	} else {
> +		offset = DISP_REG_MUTEX_MOD1(mtx->data->mutex_mod_reg,
> +					     mutex->id);
> +		reg = readl_relaxed(mtx->regs + offset);
> +		if (clear)
> +			reg &= ~BIT(mtx->data->mutex_table_mod[idx] - 32);
> +		else
> +			reg |= BIT(mtx->data->mutex_table_mod[idx] - 32);
> +	}
>   
>   	writel_relaxed(reg, mtx->regs + offset);
>   