Re: [V13,3/7] mtk-jpegenc: support jpegenc multi-hardware

[Christophe JAILLET]

Le 26/08/2022 à 11:29, Irui Wang a écrit :
> From: kyrie wu <kyrie.wu-NuS5LvNUpcJWk0Htik3J/w@xxxxxxxxxxxxxxxx>
>
> support jpeg encode multi-hardware includes HW0 and HW1.
>
> Signed-off-by: kyrie wu <kyrie.wu-NuS5LvNUpcJWk0Htik3J/w@xxxxxxxxxxxxxxxx>
> Signed-off-by: irui wang <irui.wang-NuS5LvNUpcJWk0Htik3J/w@xxxxxxxxxxxxxxxx>
> ---
>   drivers/media/platform/mediatek/jpeg/Makefile |  11 +-
>   .../platform/mediatek/jpeg/mtk_jpeg_core.c    |  67 +++----
>   .../platform/mediatek/jpeg/mtk_jpeg_core.h    |  37 ++++
>   .../platform/mediatek/jpeg/mtk_jpeg_enc_hw.c  | 163 ++++++++++++++++++
>   4 files changed, 245 insertions(+), 33 deletions(-)

[...]

> static int mtk_jpegenc_hw_init_irq(struct mtk_jpegenc_comp_dev *dev)
> +{
> +	struct platform_device *pdev = dev->plat_dev;
> +	int ret;
> +
> +	dev->jpegenc_irq = platform_get_irq(pdev, 0);
> +	if (dev->jpegenc_irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get irq resource");

Hi,

Should there be a v14, this dev_err() is useless.
platform_get_irq() already prints a more informative message.

> +		return dev->jpegenc_irq;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev,
> +			       dev->jpegenc_irq,
> +			       mtk_jpegenc_hw_irq_handler,
> +			       0,
> +			       pdev->name, dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to devm_request_irq %d (%d)",
> +			dev->jpegenc_irq, ret);
> +		return -ENOENT;

Why not propagate 'ret' here?

> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
> +{
> +	struct mtk_jpegenc_clk *jpegenc_clk;
> +	struct mtk_jpeg_dev *master_dev;
> +	struct mtk_jpegenc_comp_dev *dev;
> +	int ret, i;
> +
> +	struct device *decs = &pdev->dev;
> +
> +	if (!decs->parent)
> +		return -EPROBE_DEFER;
> +
> +	master_dev = dev_get_drvdata(decs->parent);
> +	if (!master_dev)
> +		return -EPROBE_DEFER;
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	dev->plat_dev = pdev;
> +	dev->dev = &pdev->dev;
> +
> +	if (!master_dev->is_jpgenc_multihw) {
> +		master_dev->is_jpgenc_multihw = true;
> +		for (i = 0; i < MTK_JPEGENC_HW_MAX; i++)
> +			master_dev->enc_hw_dev[i] = NULL;
> +	}
> +
> +	jpegenc_clk = &dev->venc_clk;
> +
> +	jpegenc_clk->clk_num = devm_clk_bulk_get_all(&pdev->dev,
> +						     &jpegenc_clk->clks);
> +	if (jpegenc_clk->clk_num < 0)
> +		return dev_err_probe(&pdev->dev, jpegenc_clk->clk_num,
> +				     "Failed to get jpegenc clock count\n");
> +
> +	dev->reg_base =
> +		devm_platform_ioremap_resource(pdev, 0);

This can be written on 1 line only.

just my 2c,

CJ

> +	if (IS_ERR(dev->reg_base))
> +		return PTR_ERR(dev->reg_base);
> +
> +	ret = mtk_jpegenc_hw_init_irq(dev);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
> +		if (master_dev->enc_hw_dev[i])
> +			continue;
> +
> +		master_dev->enc_hw_dev[i] = dev;
> +		master_dev->reg_encbase[i] = dev->reg_base;
> +		dev->master_dev = master_dev;
> +	}
> +
> +	platform_set_drvdata(pdev, dev);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +struct platform_driver mtk_jpegenc_hw_driver = {
> +	.probe = mtk_jpegenc_hw_probe,
> +	.driver = {
> +		.name = "mtk-jpegenc-hw",
> +		.of_match_table = of_match_ptr(mtk_jpegenc_drv_ids),
> +	},
> +};
> +
> +module_platform_driver(mtk_jpegenc_hw_driver);
> +
> +MODULE_DESCRIPTION("MediaTek JPEG encode HW driver");
> +MODULE_LICENSE("GPL");