Re: [PATCH v3 02/14] media: mediatek: vcodec: add decoder compatible to support mt8196

[Nicolas Dufresne]

Hi,

Le jeudi 12 février 2026 à 17:48 +0100, Nicolas Frattaroli a écrit :
> On Wednesday, 11 February 2026 06:41:29 Central European Standard Time Yunfei
> Dong wrote:
> > MT8196 is lat single core architecture. Support its compatible and
> > use `mtk_lat_sig_core_pdata` to initialize platform data.
> > 
> > Signed-off-by: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> > ---
> >  .../platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c   | 6 ++++++
> >  .../platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h   | 1 +
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> > index 3b81fae9f913..d9f722698198 100644
> > --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> > @@ -347,6 +347,8 @@ static void mtk_vcodec_dec_get_chip_name(struct
> > mtk_vcodec_dec_dev *vdec_dev)
> >  		vdec_dev->chip_name = MTK_VDEC_MT8186;
> >  	else if (of_device_is_compatible(dev->of_node, "mediatek,mt8188-
> > vcodec-dec"))
> >  		vdec_dev->chip_name = MTK_VDEC_MT8188;
> > +	else if (of_device_is_compatible(dev->of_node, "mediatek,mt8196-
> > vcodec-dec"))
> > +		vdec_dev->chip_name = MTK_VDEC_MT8196;
> 
> Why is this entire function written like this, and why does it exist
> at all? You can store the chip name in the platform data for a compatible,
> thereby avoiding a massive of_device_is_compatible if block because that's
> what platform data is for.
> 
> The only place where this function is even used is in probe.
> 
> Just store it in your mtk_vcodec_match's .data struct and pull it from
> dev->vdec_pdata. No need for the function.

I have already asked this to the dev working on some other MTK codec patchset,
and I think he already posted some proper refactoring (but had more changes to
finish it up). Mind, Yunfei, coordinating the effort on removing all the if/else
please ? We are doing the same code review again and again with every single
individual working on this driver.

regards,
Nicolas

> 
> >  	else
> >  		vdec_dev->chip_name = MTK_VDEC_INVAL;
> >  }
> > @@ -560,6 +562,10 @@ static const struct of_device_id mtk_vcodec_match[] = {
> >  		.compatible = "mediatek,mt8188-vcodec-dec",
> >  		.data = &mtk_lat_sig_core_pdata,
> >  	},
> > +	{
> > +		.compatible = "mediatek,mt8196-vcodec-dec",
> > +		.data = &mtk_lat_sig_core_pdata,
> > +	},
> >  	{},
> >  };
> >  
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> > index c9d27534c63e..f06dfc1a3455 100644
> > --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> > +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> > @@ -29,6 +29,7 @@ enum mtk_vcodec_dec_chip_name {
> >  	MTK_VDEC_MT8188 = 8188,
> >  	MTK_VDEC_MT8192 = 8192,
> >  	MTK_VDEC_MT8195 = 8195,
> > +	MTK_VDEC_MT8196 = 8196,
> >  };
> >  
> >  /*
> > 
> 
> 
>

Attachment: signature.asc
Description: This is a digitally signed message part