Re: [PATCH v2 07/15] soc: mediatek: mutex: refactor SOF settings for output components

["Paul-pl Chen (陳柏霖)" <Paul-pl.Chen@mediatek.com>]

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@mediatek.com>
> > 
> > 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@mediatek.com>
> > Signed-off-by: Paul-pl Chen <paul-pl.chen@mediatek.com>
> > ---
> >   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
> 
>