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

On Mon, 2022-02-07 at 15:50 +0100, AngeloGioacchino Del Regno wrote:

Il 03/12/21 04:13, kyrie.wu ha scritto:

quoted

manage each hardware information, including irq/clk/power.
the hardware includes HW0 and HW1.

Signed-off-by: kyrie.wu <redacted>

Hello Kyrie,
thanks for the patch!

However, there are a few things to improve...

quoted

---
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.

quoted

-	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.

quoted

+		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-

quoted

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...

/

quoted

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.

quoted

+	{
+		.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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel