lkml.org 
[lkml]   [2021]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v10, 06/19] media: mtk-vcodec: Manage multi hardware information
    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@mediatek.com/

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

    \
     
     \ /
      Last update: 2021-11-16 10:24    [W:2.906 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site