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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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

> -	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@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.....
	};
};

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