Re: [PATCH V6, 2/5] media: mtk-jpegenc: manage jpegenc multi-hardware

From: kyrie.wu <kyrie.wu@mediatek.com>
To: AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Tomasz Figa <tfiga@chromium.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	"Tzung-Bi Shih" <tzungbi@chromium.org>
Cc: <Project_Global_Chrome_Upstream_Group@mediatek.com>,
	<linux-media@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-mediatek@lists.infradead.org>, <xia.jiang@mediatek.com>,
	<maoguang.meng@mediatek.com>, <srv_heupstream@mediatek.com>,
	<irui.wang@mediatek.com>
Subject: Re: [PATCH V6, 2/5] media: mtk-jpegenc: manage jpegenc multi-hardware
Date: Mon, 21 Feb 2022 09:48:11 +0800	[thread overview]
Message-ID: <af2f8e675baedbe3c6ece234dc5ff296133f65b1.camel@mediatek.com> (raw)
In-Reply-To: <4e1b549b-748e-94f3-befb-59ceab950297@collabora.com>

On Mon, 2022-02-07 at 15:50 +0100, AngeloGioacchino Del Regno wrote:
> Il 03/12/21 04:13, kyrie.wu ha scritto:
> > manage each hardware information, including irq/clk/power.
> > the hardware includes HW0 and HW1.
> > 
> > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> 
> Hello Kyrie,
> thanks for the patch!
> 
> However, there are a few things to improve...
> 
> > ---
> > V6: use of_platform_populate to replace component framework
> > to manage multi-hardware
> > ---
> >   drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 103 +++++++
> > ---
> >   drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  55 +++++
> >   drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 232
> > +++++++++++++++++++++-
> >   3 files changed, 362 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > index a89c7b2..da7eb84 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > @@ -1353,33 +1353,40 @@ static int mtk_jpeg_probe(struct
> > platform_device *pdev)
> >   	spin_lock_init(&jpeg->hw_lock);
> >   	jpeg->dev = &pdev->dev;
> >   	jpeg->variant = of_device_get_match_data(jpeg->dev);
> > -	INIT_DELAYED_WORK(&jpeg->job_timeout_work,
> > mtk_jpeg_job_timeout_work);
> >   
> > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -	jpeg->reg_base = devm_ioremap_resource(&pdev->dev, res);
> 
> This has to be rebased... new versions are using
> devm_platform_ioremap_resource(),
> which is shorter and cleaner...
Hi AngeloGioacchino,
Thanks for your kindly reply, I will fix this problem in the next
version.

> 
> > -	if (IS_ERR(jpeg->reg_base)) {
> > -		ret = PTR_ERR(jpeg->reg_base);
> > -		return ret;
> > -	}
> > +	if (!jpeg->variant->is_encoder) {
> 
> What about mediatek,mtk-jpgenc? That's also an encoder... this
> "is_encoder"
> variable is a bit confusing... Is that a newer version of the IP?
> Please, let's find a better name for this variable to avoid
> confusion.
The confusion would modify in the next version.
Thanks.

> 
> 
> > +		INIT_DELAYED_WORK(&jpeg->job_timeout_work,
> > +				mtk_jpeg_job_timeout_work);
> >   
> > -	jpeg_irq = platform_get_irq(pdev, 0);
> > -	if (jpeg_irq < 0) {
> > -		dev_err(&pdev->dev, "Failed to get jpeg_irq %d.\n",
> > jpeg_irq);
> > -		return jpeg_irq;
> > -	}
> > +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +		jpeg->reg_base = devm_ioremap_resource(&pdev->dev,
> > res);
> > +		if (IS_ERR(jpeg->reg_base)) {
> > +			ret = PTR_ERR(jpeg->reg_base);
> > +			return ret;
> > +		}
> >   
> > -	ret = devm_request_irq(&pdev->dev, jpeg_irq,
> > -			       jpeg->variant->irq_handler, 0, pdev-
> > >name, jpeg);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "Failed to request jpeg_irq %d
> > (%d)\n",
> > -			jpeg_irq, ret);
> > -		goto err_req_irq;
> > -	}
> > +		jpeg_irq = platform_get_irq(pdev, 0);
> > +		if (jpeg_irq < 0) {
> > +			dev_err(&pdev->dev, "Failed to get
> > jpeg_irq.\n");
> > +			return jpeg_irq;
> > +		}
> >   
> > -	ret = mtk_jpeg_clk_init(jpeg);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "Failed to init clk, err %d\n",
> > ret);
> > -		goto err_clk_init;
> > +		ret = devm_request_irq(&pdev->dev, jpeg_irq,
> > +				       jpeg->variant->irq_handler,
> > +					   0, pdev->name, jpeg);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "Failed to request jpeg_irq
> > %d (%d)\n",
> > +				jpeg_irq, ret);
> > +			goto err_req_irq;
> > +		}
> > +
> > +		ret = mtk_jpeg_clk_init(jpeg);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "Failed to init clk(%d)\n",
> > ret);
> > +			goto err_clk_init;
> > +		}
> > +
> > +		pm_runtime_enable(&pdev->dev);
> >   	}
> >   
> >   	ret = v4l2_device_register(&pdev->dev, &jpeg->v4l2_dev);
> 
> ...snip...
> 
> /
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > index 1cf037b..4ccda1d 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > @@ -4,12 +4,27 @@
> >    * Author: Xia Jiang <xia.jiang@mediatek.com>
> >    *
> >    */
> > -
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> >   #include <linux/io.h>
> >   #include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> > +#include <media/media-device.h>
> >   #include <media/videobuf2-core.h>
> >   #include <media/videobuf2-dma-contig.h>
> > +#include <media/videobuf2-v4l2.h>
> > +#include <media/v4l2-mem2mem.h>
> > +#include <media/v4l2-dev.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-fh.h>
> > +#include <media/v4l2-event.h>
> >   
> > +#include "mtk_jpeg_core.h"
> >   #include "mtk_jpeg_enc_hw.h"
> >   
> >   static const struct mtk_jpeg_enc_qlt mtk_jpeg_enc_quality[] = {
> > @@ -30,6 +45,21 @@ static const struct mtk_jpeg_enc_qlt
> > mtk_jpeg_enc_quality[] = {
> >   	{.quality_param = 97, .hardware_value = JPEG_ENC_QUALITY_Q97},
> >   };
> >   
> > +#if defined(CONFIG_OF)
> > +static const struct of_device_id mtk_jpegenc_drv_ids[] = {
> 
> There's nothing special in jpgenc1 or, at least, nothing that really
> needs
> us to differentiate between jpgenc0 and jpgenc1, apart knowing which
> core is
> the "main" one, hence, you don't need a special compatible string for
> each core.
> 
> Here's my proposal:
> - Use one compatible "mediatek,mt8195-jpgenc-hw"
> - Add either of:
>    - A bool "mediatek,hw-leader" on core 0; or
>    - A u8 "mediatek,instance" (0, 1, 2 ... for core number)
> 
> A comment is required on "mediatek,instance" though... this way
> should only be
> chosen if it is expected, in the future, to have the following
> situation on
> newer SoCs:
> - More than two cores, and
> - non-interchangeable cores (meaning that, for example, frame 1
> *shall* go to
>    core 1, frame 2 shall go to core 2, frame 3 to core 3, BUT core
> 2/3 are not
>    interchangeable, as in there are reasons to never process frame 2
> on core 3),
>    so this means that it's not important if Linux labels core 3 as
> core 2.
> 
> Otherwise, if it's not expected to have non-interchangeable "hw-
> follower" cores,
> or if more than two cores are not ever expected, "mediatek,hw-leader" 
> is the best
> choice for this.
> 
> Example:
> 
> jpegenc@address {
> 	compatible = "mediatek,mt8195-jpgenc";
> 	reg = < .... >;
> 	ranges = < ....... >;
> 	.... other properties here ....
> 
> 	jpegenc-hw0@relative-address {
> 		compatible = "mediatek,mt8195-jpgenc-hw",
> "mediatek,jpgenc-hw";
> 		iommus, interrupts, other properties here ...
> 		mediatek,hw-leader;
> 	};
> 
> 	jpegenc-hw1@relative-address {
> 		compatible = "mediatek,mt8195-jpgenc-hw",
> "mediatek,jpgenc-hw";
> 		iommus, interrupts, etc.....
> 	};
> };
I will take your suggestion in the next version.
Thanks.
> 
> > +	{
> > +		.compatible = "mediatek,mt8195-jpgenc0",
> > +		.data = (void *)MTK_JPEGENC_HW0,
> > +	},
> > +	{
> > +		.compatible = "mediatek,mt8195-jpgenc1",
> > +		.data = (void *)MTK_JPEGENC_HW1,
> > +	},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_jpegenc_drv_ids);
> > +#endif
> > +
> >   void mtk_jpeg_enc_reset(void __iomem *base)
> >   {
> >   	writel(0, base + JPEG_ENC_RSTB);
> 
> Thanks,
> Angelo
> 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek