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