Hi Tzung-Bi,
Thanks for your suggestion. On Wed, 2021-11-10 at 18:30 +0800, Tzung-Bi Shih wrote:
On Tue, Nov 09, 2021 at 08:50:17PM +0800, Yunfei Dong wrote:
Manage each hardware information which includes irq/power/clk. The hardware includes LAT0, LAT1 and CORE.
The commit message doesn't explain the code. Could you provide some explanations about how the async mechanism works? (e.g. A bitmap for all sub-devices' readiness ...)
add more detail description for commit message.
Reported-by: kernel test robot lkp@intel.com
Apparently wrong tag.
Remove
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c index b7a51e96d4ba..eb2af42aa102 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c @@ -18,19 +18,49 @@
#include "mtk_vcodec_drv.h" #include "mtk_vcodec_dec.h" +#include "mtk_vcodec_dec_hw.h" #include "mtk_vcodec_dec_pm.h" #include "mtk_vcodec_intr.h" -#include "mtk_vcodec_util.h"
Why does mtk_vcodec_util.h need to remove?
Put #include "mtk_vcodec_util.h" in mtk_vcodec_dec_hw.h.
+static int mtk_vcodec_subdev_device_check(struct mtk_vcodec_ctx *ctx)
- {
Remove the extra space.
Fix
struct mtk_vcodec_dev *vdec_dev = ctx->dev;struct platform_device *pdev = vdec_dev->plat_dev;struct device_node *subdev_node;enum mtk_vdec_hw_id hw_idx;const struct of_device_id *of_id;int i;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;if (!of_device_is_available(subdev_node)) {of_node_put(subdev_node);dev_err(&pdev->dev, "Fail to get MMSYSnode\n");
I am not sure if the error message makes sense about mentioning MMSYS here.
Fix with subdev node.
continue;}hw_idx = (enum mtk_vdec_hw_id)(uintptr_t)of_id->data;Does it really need to cast twice?
yes, or will build warning.
vdec_dev->subdev_node[hw_idx] = subdev_node;if (!test_bit(hw_idx, vdec_dev->hardware_bitmap)) {dev_err(&pdev->dev, "Vdec hw_idx is not ready%d.",
hw_idx);I would prefer "Vdec %d is not ready\n".
Fix
return -EINVAL;-EAGAIN makes more sense. Fix
}}return 0;+}
Would it need to call of_node_put() in the error handling path?
yes, fix it.
+static int mtk_vcodec_init_dec_params(struct mtk_vcodec_dev *dev) +{
I would rather not call them "params". They are more like "resources".
Fix it with resources.
struct platform_device *pdev = dev->plat_dev;int ret;ret = mtk_vcodec_get_reg_bases(dev);if (ret)return ret;if (!dev->vdec_pdata->is_subdev_supported) {dev->dec_irq = platform_get_irq(pdev, 0);if (dev->dec_irq < 0) {dev_err(&pdev->dev, "failed to get irqnumber");
return dev->dec_irq;}irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);ret = devm_request_irq(&pdev->dev, dev->dec_irq,mtk_vcodec_dec_irq_handler, 0, pdev->name,dev);
if (ret) {dev_err(&pdev->dev, "failed to install dev-dec_irq %d (%d)",
dev->dec_irq, ret);return ret;}ret = mtk_vcodec_init_dec_pm(pdev, &dev->pm);if (ret < 0) {dev_err(&pdev->dev, "failed to get mt vcodecclock source");
return ret;}}I would prefer:
if (dev->vdec_pdata->is_subdev_supported) return 0;
Fix
And decrease the indent level by 1 for the following blocks.
@@ -329,6 +377,13 @@ static int mtk_vcodec_probe(struct platform_device *pdev) goto err_event_workq; }
ret = of_platform_populate(pdev->dev.of_node, NULL, NULL,&pdev->dev);if (ret) {mtk_v4l2_err("Master device of_platform_populatefailed.");
s/Master/Main/
Doesn't it need to reference `is_subdev_supported` before populating?
Fix
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c new file mode 100644 index 000000000000..745be12548ef
[...]
+const struct of_device_id mtk_vdec_hw_match[] = {
{.compatible = "mediatek,mtk-vcodec-lat",.data = (void *)MTK_VDEC_LAT0,},{.compatible = "mediatek,mtk-vcodec-core",.data = (void *)MTK_VDEC_CORE,},{},+}; +EXPORT_SYMBOL_GPL(mtk_vdec_hw_match);
Introducing new compatible strings. Need a dt-bindings patch for them.
Add in patch 15.
+static int mtk_vdec_hw_probe(struct platform_device *pdev) +{
[...]
subdev_dev->reg_base[VDEC_HW_MISC] =devm_platform_ioremap_resource_byname(pdev, "misc");if (IS_ERR((__force void *)subdev_dev-reg_base[VDEC_HW_MISC])) {
ret = PTR_ERR((__force void *)subdev_dev-reg_base[VDEC_HW_MISC]);
goto err;}Resource "misc" needs a dt-bindings patch to explain how to use/specify it.
Remove.
hw_idx = (enum mtk_vdec_hw_id)(uintptr_t)of_id->data;Does it really need to cast twice?
yes, or will build warning.
if (hw_idx < MTK_VDEC_HW_MAX) {main_dev->subdev_dev[hw_idx] = subdev_dev;subdev_dev->hw_idx = hw_idx;subdev_dev->main_dev = main_dev;subdev_dev->reg_base[VDEC_HW_SYS] =main_dev->reg_base[VDEC_HW_SYS];set_bit(subdev_dev->hw_idx, main_dev-hardware_bitmap);
}mtk_vcodec_subdev_device_check() doesn't check the value of of_id->data. Does it make more sense to align the implementation? If hw_idx is equal to or bigger than MTK_VDEC_HW_MAX, shall it print warning messages for example.
bitmap is used to record whether the hardware index is init or not. Set the hardware index to 1 in bitmap when init done.
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
[...]
+#define VDEC_HW_ACTIVE 0x10 +#define VDEC_IRQ_CFG 0x11 +#define VDEC_IRQ_CLR 0x10 +#define VDEC_IRQ_CFG_REG 0xa4
If only mtk_vcodec_dec_hw.c uses them, could they move to the file?
Also used in mtk_vcodec_dec_drv.c
@@ -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
*/
- @hardware_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];DECLARE_BITMAP(hardware_bitmap, MTK_VDEC_HW_MAX);I would prefer to use name `subdev_bitmap`.
Fix
Thanks, Yunfei Dong