Re: [PATCH v2 07/15] soc: mediatek: mutex: refactor SOF settings for output components
From: Paul-pl Chen (陳柏霖)
Date: Tue Apr 01 2025 - 23:31:10 EST
On Mon, 2025-03-24 at 18:12 +0100, AngeloGioacchino Del Regno wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> Il 21/03/25 10:33, paul-pl.chen ha scritto:
> > From: Nancy Lin <nancy.lin@xxxxxxxxxxxx>
> >
> > Refactor SOF settings by adding mtk_mutex_get_output_comp_sof()
> > and extracting SOF logic from mtk_mutex_add_comp()
> > and mtk_mutex_remove_comp().
> >
> > - Added mtk_mutex_add_comp_sof() and mtk_mutex_remove_comp_sof()
> > for SOF settings.
> > - Reused the switch case for SOF IDs.
> > - Separated MOD and SOF logic.
> >
>
> This also needs more than one commit.
>
> The cleanups go in a commit doing only cleanups.
>
Hi AngeloGioacchino,
Based on your advice, I'm considering dividing the changes as follows:
Commit 1:refactor SOF settings for output components
- Added mtk_mutex_add_comp_sof() and mtk_mutex_remove_comp_sof()
for SOF settings.
- Separated MOD and SOF logic.
Commit 2:
- Reused the switch case for SOF IDs.
> > Signed-off-by: Nancy Lin <nancy.lin@xxxxxxxxxxxx>
> > Signed-off-by: Paul-pl Chen <paul-pl.chen@xxxxxxxxxxxx>
> > ---
> > drivers/soc/mediatek/mtk-mutex.c | 121 +++++++++++++++-----
> > -----
> > include/linux/soc/mediatek/mtk-mutex.h | 4 +
> > 2 files changed, 79 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/soc/mediatek/mtk-mutex.c
> > b/drivers/soc/mediatek/mtk-mutex.c
> > index aaa965d4b050..c026ac0e6969 100644
> > --- a/drivers/soc/mediatek/mtk-mutex.c
> > +++ b/drivers/soc/mediatek/mtk-mutex.c
> > @@ -853,43 +853,84 @@ void mtk_mutex_unprepare(struct mtk_mutex
> > *mutex)
> > }
> > EXPORT_SYMBOL_GPL(mtk_mutex_unprepare);
> >
> > -void mtk_mutex_add_comp(struct mtk_mutex *mutex,
> > - enum mtk_ddp_comp_id id)
> > +static int mtk_mutex_get_output_comp_sof(enum mtk_ddp_comp_id id)
> > {
> > - struct mtk_mutex_ctx *mtx = container_of(mutex, struct
> > mtk_mutex_ctx,
> > - mutex[mutex->id]);
> > - unsigned int reg;
> > - unsigned int sof_id;
> > - unsigned int offset;
> > -
> > - WARN_ON(&mtx->mutex[mutex->id] != mutex);
> > -
> > switch (id) {
> > case DDP_COMPONENT_DSI0:
> > - sof_id = MUTEX_SOF_DSI0;
> > - break;
> > + return MUTEX_SOF_DSI0;
> > case DDP_COMPONENT_DSI1:
> > - sof_id = MUTEX_SOF_DSI0;
> > - break;
> > + return MUTEX_SOF_DSI1;
> > case DDP_COMPONENT_DSI2:
> > - sof_id = MUTEX_SOF_DSI2;
> > - break;
> > + return MUTEX_SOF_DSI2;
> > case DDP_COMPONENT_DSI3:
> > - sof_id = MUTEX_SOF_DSI3;
> > - break;
> > + return MUTEX_SOF_DSI3;
> > case DDP_COMPONENT_DPI0:
> > - sof_id = MUTEX_SOF_DPI0;
> > - break;
> > + return MUTEX_SOF_DPI0;
> > case DDP_COMPONENT_DPI1:
> > - sof_id = MUTEX_SOF_DPI1;
> > - break;
> > + return MUTEX_SOF_DPI1;
> > case DDP_COMPONENT_DP_INTF0:
> > - sof_id = MUTEX_SOF_DP_INTF0;
> > - break;
> > + return MUTEX_SOF_DP_INTF0;
> > case DDP_COMPONENT_DP_INTF1:
> > - sof_id = MUTEX_SOF_DP_INTF1;
> > - break;
> > + return MUTEX_SOF_DP_INTF1;
> > default:
> > + break;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +void mtk_mutex_add_comp_sof(struct mtk_mutex *mutex, enum
> > mtk_ddp_comp_id id)
> > +{
> > + struct mtk_mutex_ctx *mtx = container_of(mutex, struct
> > mtk_mutex_ctx,
> > + mutex[mutex->id]);
> > + int sof_id = mtk_mutex_get_output_comp_sof(id);
> > + unsigned int offset;
> > +
> > + if (sof_id < 0 || sof_id >= DDP_MUTEX_SOF_MAX)
> > + return;
> > +
> > + WARN_ON(&mtx->mutex[mutex->id] != mutex);
> > +
> > + offset = DISP_REG_MUTEX_SOF(mtx->data->mutex_sof_reg, mutex-
> > >id);
> > +
> > + writel_relaxed(mtx->data->mutex_sof[sof_id],
> > + mtx->regs + offset);
>
> fits in one line
>
> OK, I will fix it in one line.
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mutex_add_comp_sof);
> > +
> > +void mtk_mutex_remove_comp_sof(struct mtk_mutex *mutex, enum
> > mtk_ddp_comp_id id)
> > +{
> > + struct mtk_mutex_ctx *mtx = container_of(mutex, struct
> > mtk_mutex_ctx,
> > + mutex[mutex->id]);
> > + unsigned int reg;
> > + int sof_id = mtk_mutex_get_output_comp_sof(id);
> > + unsigned int offset;
> > +
> > + if (sof_id < 0 || sof_id >= DDP_MUTEX_SOF_MAX)
> > + return;
> > +
> > + WARN_ON(&mtx->mutex[mutex->id] != mutex);
> > +
> > + offset = DISP_REG_MUTEX_SOF(mtx->data->mutex_sof_reg, mutex-
> > >id);
> > + reg = readl_relaxed(mtx->regs + offset);
> > + reg &= ~(1 << mtx->data->mutex_sof[id]);
> > +
> > + writel_relaxed(reg, mtx->regs + offset);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mutex_remove_comp_sof);
> > +
> > +void mtk_mutex_add_comp(struct mtk_mutex *mutex,
> > + enum mtk_ddp_comp_id id)
> > +{
> > + struct mtk_mutex_ctx *mtx = container_of(mutex, struct
> > mtk_mutex_ctx,
> > + mutex[mutex->id]);
> > + unsigned int reg;
> > + unsigned int offset;
> > + bool is_output_comp = !!mtk_mutex_get_output_comp_sof(id);
> > +
> > + WARN_ON(&mtx->mutex[mutex->id] != mutex);
> > +
>
> Looks like you can just do
>
> if (is_output_comp) {
> mtk_mutex_add_comp_sof(mutex, id);
> return;
> }
>
> if (mtx->data->mutex_mod[id] < 32) { .....etc
>
OK,
This is because the next patch [PATCH v2 08/15],
we have to consider using the condition of "mtx->data->need_sof_mod"
to set the register setting of DISP_REG_MUTEX_MOD.
> > + if (!is_output_comp) {
> > if (mtx->data->mutex_mod[id] < 32) {
> > offset = DISP_REG_MUTEX_MOD(mtx->data-
> > >mutex_mod_reg,
> > mutex->id);
> > @@ -902,12 +943,10 @@ void mtk_mutex_add_comp(struct mtk_mutex
> > *mutex,
> > reg |= 1 << (mtx->data->mutex_mod[id] - 32);
> > writel_relaxed(reg, mtx->regs + offset);
> > }
> > - return;
> > }
> >
> > - writel_relaxed(mtx->data->mutex_sof[sof_id],
> > - mtx->regs +
> > - DISP_REG_MUTEX_SOF(mtx->data->mutex_sof_reg,
> > mutex->id));
> > + if (is_output_comp)
> > + mtk_mutex_add_comp_sof(mutex, id);
> > }
> > EXPORT_SYMBOL_GPL(mtk_mutex_add_comp);
> >
> > @@ -918,24 +957,11 @@ void mtk_mutex_remove_comp(struct mtk_mutex
> > *mutex,
> > mutex[mutex->id]);
> > unsigned int reg;
> > unsigned int offset;
> > + bool is_output_comp = !!mtk_mutex_get_output_comp_sof(id);
> >
> > WARN_ON(&mtx->mutex[mutex->id] != mutex);
> >
> > - switch (id) {
> > - case DDP_COMPONENT_DSI0:
> > - case DDP_COMPONENT_DSI1:
> > - case DDP_COMPONENT_DSI2:
> > - case DDP_COMPONENT_DSI3:
> > - case DDP_COMPONENT_DPI0:
> > - case DDP_COMPONENT_DPI1:
> > - case DDP_COMPONENT_DP_INTF0:
> > - case DDP_COMPONENT_DP_INTF1:
> > - writel_relaxed(MUTEX_SOF_SINGLE_MODE,
> > - mtx->regs +
> > - DISP_REG_MUTEX_SOF(mtx->data-
> > >mutex_sof_reg,
> > - mutex->id));
> > - break;
> > - default:
> > + if (!is_output_comp) {
>
> same comment as before.
>
OK,
This is because the next patch [PATCH v2 08/15],
we have to consider using the condition of "mtx->data->need_sof_mod"
to set the register setting of DISP_REG_MUTEX_MOD.
Best regards,
Paul
> > if (mtx->data->mutex_mod[id] < 32) {
> > offset = DISP_REG_MUTEX_MOD(mtx->data-
> > >mutex_mod_reg,
> > mutex->id);
> > @@ -948,8 +974,11 @@ void mtk_mutex_remove_comp(struct mtk_mutex
> > *mutex,
> > reg &= ~(1 << (mtx->data->mutex_mod[id] -
> > 32));
> > writel_relaxed(reg, mtx->regs + offset);
> > }
> > - break;
> > }
> > +
> > + if (is_output_comp)
> > + mtk_mutex_remove_comp_sof(mutex, id);
> > +
>
> Regards,
> Angelo
>
>