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

[moudy ho <moudy.ho@mediatek.com>]

On Tue, 2022-10-04 at 14:38 +0200, AngeloGioacchino Del Regno wrote:
> 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
> 
> 

Hi Angelo,

Thanks for helping with outstanding comments and cleaning up the code,
I'll follow this guide to improve accordingly.

Regards,
Moudy

> > +		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);
> >   
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel