Re: [PATCH V6, 2/5] media: mtk-jpegenc: manage jpegenc multi-hardware
From: AngeloGioacchino Del Regno
Date: Mon Feb 07 2022 - 10:11:18 EST
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@xxxxxxxxxxxx>
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...
- 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.
+ 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@xxxxxxxxxxxx>
*
*/
-
+#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.....
};
};
+ {
+ .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