Hi, Stu:
On Wed, 2018-05-23 at 17:28 +0800, Stu Hsieh wrote:
> On Wed, 2018-05-23 at 13:23 +0800, CK Hu wrote:
> > Hi, Stu:
> >
> > I've some inline comment.
> >
> > On Wed, 2018-05-23 at 10:25 +0800, Stu Hsieh wrote:
> > > This patch add support for the Mediatek MT2712 DISP subsystem.
> > > There are two OVL engine and three disp output in MT2712.
> > >
> > > Signed-off-by: Stu Hsieh <stu.hs...@mediatek.com>
> > > ---
> > > drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 50
> > > +++++++++++++++++++++++++++--
> > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 8 +++--
> > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 7 ++--
> > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 47
> > > +++++++++++++++++++++++++--
> > > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 7 ++--
> > > 5 files changed, 108 insertions(+), 11 deletions(-)
> > >
> > > +#define MT2712_MUTEX_MOD_DISP_AAL0 BIT(20)
> > > +#define MT2712_MUTEX_MOD_DISP_UFOE BIT(22)
> > > +#define MT2712_MUTEX_MOD_DISP_PWM0 BIT(23)
> > > +#define MT2712_MUTEX_MOD_DISP_PWM1 BIT(24)
> > > +#define MT2712_MUTEX_MOD_DISP_PWM2 BIT(10)
> > > +#define MT2712_MUTEX_MOD_DISP_OD0 BIT(25)
> > > +/* modules more than 32, add BIT(31) when using DISP_REG_MUTEX_MOD2 bit
> > > */
> > > +#define MT2712_MUTEX_MOD2_DISP_AAL1 (BIT(1) | BIT(31))
> >
> > I think a better definition is
> >
> > #define MT2712_MUTEX_MOD2_DISP_AAL1 BIT(33)
> >
> > when you need to access this register,
> >
> > if (ddp->mutex_mod[id] < BIT(32)) {
> > offset = DISP_REG_MUTEX_MOD(mutex->id);
> > reg = readl_relaxed(ddp->regs + offset);
> > reg |= ddp->mutex_mod[id];
> > writel_relaxed(reg, ddp->regs + offset);
> > } else {
> > offset = DISP_REG_MUTEX_MOD2(mutex->id);
> > reg = readl_relaxed(ddp->regs + offset);
> > reg |= (ddp->mutex_mod[id] >> 32);
> > writel_relaxed(reg, ddp->regs + offset);
> > }
> >
> > because DISP_REG_MUTEX_MOD BIT(31) could be used for some module.
>
> This modification is workable, but result some build warning like
> following:
> 1. #define BIT(nr) (1UL << (nr)) in include/linux/bitops.h
> 2. [DDP_COMPONENT_AAL1] = MT2712_MUTEX_MOD2_DISP_AAL1,
> => we need to modify the definition about "static const unsigned int
> mt2712_mutex_mod"
>
Currently, mutex_mod is a bitwise definition. I think it could be
changed to index definition such as
#define MT2712_MUTEX_MOD_DISP_PWM2 10
#define MT2712_MUTEX_MOD_DISP_OD0 25
#define MT2712_MUTEX_MOD2_DISP_AAL1 33
when you need to access this register,
if (ddp->mutex_mod[id] < 32) {
offset = DISP_REG_MUTEX_MOD(mutex->id);
reg = readl_relaxed(ddp->regs + offset);
reg |= 1 << ddp->mutex_mod[id];
writel_relaxed(reg, ddp->regs + offset);
} else {
offset = DISP_REG_MUTEX_MOD2(mutex->id);
reg = readl_relaxed(ddp->regs + offset);
reg |= 1 << (ddp->mutex_mod[id] - 32);
writel_relaxed(reg, ddp->regs + offset);
}
Regards,
CK
> > > +#define MT2712_MUTEX_MOD2_DISP_OD1 (BIT(2) | BIT(31))
> > > +
> > > #define MT2701_MUTEX_MOD_DISP_OVL BIT(3)
> > > #define MT2701_MUTEX_MOD_DISP_WDMA BIT(6)
> > > #define MT2701_MUTEX_MOD_DISP_COLOR BIT(7)
> > > @@ -74,6 +96,7 @@
> > >
> > >
> >
> >
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
