[PATCH V6, 2/5] media: mtk-jpegenc: manage jpegenc multi-hardware
kyrie.wu
kyrie.wu at mediatek.com
Sun Feb 20 17:48:11 PST 2022
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 at 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 at 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 at address {
> compatible = "mediatek,mt8195-jpgenc";
> reg = < .... >;
> ranges = < ....... >;
> .... other properties here ....
>
> jpegenc-hw0 at relative-address {
> compatible = "mediatek,mt8195-jpgenc-hw",
> "mediatek,jpgenc-hw";
> iommus, interrupts, other properties here ...
> mediatek,hw-leader;
> };
>
> jpegenc-hw1 at 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
>
More information about the linux-arm-kernel
mailing list