On Thu, Nov 11, 2021 at 12:14:47PM +0800, Yunfei Dong wrote:
> Register each hardware(subdev) as platform device used to manage each
> hardware information which includes irq/power/clk. The hardware includes
> LAT0, LAT1 and CORE. And call of_platform_populate in main device.
>
> Using subdev_bitmap to record whether each device is register done. Then check
> whether all subdev are register done before open main device.
I can somehow understand what the patch is trying to do but the commit message needs to be rephrased for people to understand the patch.
> --- a/drivers/media/platform/mtk-vcodec/Makefile
> +++ b/drivers/media/platform/mtk-vcodec/Makefile
> @@ -2,7 +2,8 @@
>
> obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk-vcodec-dec.o \
> mtk-vcodec-enc.o \
> - mtk-vcodec-common.o
> + mtk-vcodec-common.o \
> + mtk-vcodec-dec-hw.o
Looks better to align to previous lines.
> +static int mtk_vcodec_subdev_device_check(struct mtk_vcodec_ctx *ctx)
> +{
> + struct mtk_vcodec_dev *vdec_dev = ctx->dev;
ctx isn't used in other places. Looks like the function can accept "struct mtk_vcodec_dev *vdec_dev" as the only argument.
> + for (i = 0; i < ARRAY_SIZE(mtk_vdec_hw_match); i++) {
> + of_id = &mtk_vdec_hw_match[i];
> + subdev_node = of_find_compatible_node(NULL, NULL,
> + of_id->compatible);
> + if (!subdev_node)
> + continue;
Does it really need to continue if one of the sub-devices is not ready?
It depends on whether mtk_vdec_hw_match is a must list or not. For example, if mtk_vdec_hw_match has 4 entries but the DT only has 2 entries, shall it return an error about the entry count mismatch?
> + if (!of_device_is_available(subdev_node)) {
> + of_node_put(subdev_node);
> + dev_err(&pdev->dev, "Fail to get subdev node\n");
> + continue;
The error message shouldn't be "Fail to get ...". Also the same question: does it really need to continue?
> + hw_idx = (enum mtk_vdec_hw_id)(uintptr_t)of_id->data;
> + vdec_dev->subdev_node[hw_idx] = subdev_node;
I am wondering if it wouldn't need subdev_node. Isn't vdec_dev->subdev_dev sufficient to clue all the things?
> + if (!test_bit(hw_idx, vdec_dev->subdev_bitmap)) {
> + of_node_put(subdev_node);
> + dev_err(&pdev->dev, "Vdec %d is not ready", hw_idx);
> + return -EAGAIN;
> + }
> + of_node_put(subdev_node);
> + }
> +
> + return 0;
> +}
In addition to the question for subdev_node. The function calls of_node_put() for every paths. I am not sure if the function should call of_node_put() in non-error-handling paths (i.e. I thought it needs someone to hold the reference count).
By reading [v10,11/19] media: mtk-vcodec: Generalize power and clock on/off interfaces[1], the mtk_vcodec_get_hw_dev() calls of_node_put() after it gets the hw_pdev. Looks like the code is meant to borrow the reference count to mtk_vcodec_get_hw_dev().
In short, if the subdev_node is designed to borrow reference count to others, mtk_vcodec_subdev_device_check() shouldn't call of_node_put() in non-error-handling paths.
[1]: https://patchwork.linuxtv.org/project/linux-media/patch/20211111041500.17363-12-yunfei.dong@xxxxxxxxxxxx/
> @@ -249,32 +322,10 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
[...]
> + ret = mtk_vcodec_init_dec_resources(dev);
> if (ret) {
> - dev_err(&pdev->dev, "Failed to install dev->dec_irq %d (%d)",
> - dev->dec_irq,
> - ret);
> - goto err_res;
> + dev_err(&pdev->dev, "Failed to init pm and registers");
The error message makes less sense about mentioning pm and registers.
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
[...]
> +static int mtk_vdec_hw_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mtk_vdec_hw_dev *subdev_dev;
> + struct mtk_vcodec_dev *main_dev;
> + const struct of_device_id *of_id;
> + int hw_idx;
> + int ret;
> +
> + if (!dev->parent)
> + return -EPROBE_DEFER;
IIUC, it shouldn't happen because the deivce is populated from main device. Moreover, would it help to defer the probe if dev->parent is NULL?
> + main_dev = dev_get_drvdata(dev->parent);
> + if (!main_dev)
> + return -EPROBE_DEFER;
Share the same concern with comment above.
> +static struct platform_driver mtk_vdec_driver = {
> + .probe = mtk_vdec_hw_probe,
> + .driver = {
> + .name = "mtk-vdec-comp",
> + .of_match_table = mtk_vdec_hw_match,
> + },
> +};
> +
> +module_platform_driver(mtk_vdec_driver);
I prefer to remove the blank line in between mtk_vdec_driver and module_platform_driver.
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
[...]
> @@ -423,6 +436,11 @@ struct mtk_vcodec_enc_pdata {
> * @pm: power management control
> * @dec_capability: used to identify decode capability, ex: 4k
> * @enc_capability: used to identify encode capability
> + *
> + * @subdev_dev: subdev hardware device
> + * @subdev_node: subdev node
> + *
> + * @subdev_bitmap: used to record hardware is ready or not
> */
> struct mtk_vcodec_dev {
> struct v4l2_device v4l2_dev;
> @@ -460,6 +478,11 @@ struct mtk_vcodec_dev {
> struct mtk_vcodec_pm pm;
> unsigned int dec_capability;
> unsigned int enc_capability;
> +
> + void *subdev_dev[MTK_VDEC_HW_MAX];
> + struct device_node *subdev_node[MTK_VDEC_HW_MAX];
The same question: if it already has subdev_dev, does it still need subdev_node?
