| Message ID | 20200723030451.5616-23-xia.jiang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for mt2701 JPEG ENC support | expand |
Hi Xia, On Thu, Jul 23, 2020 at 11:04:45AM +0800, Xia Jiang wrote: > Use the generic of_property_* helpers to get the clock_nums and clocks > from device tree. > Use the generic clk_bulk_* helpers to enable and disable clocks. > > Signed-off-by: Xia Jiang <xia.jiang@mediatek.com> > --- > v10: new add patch > --- > .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 47 +++++++++++++++---- > .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 8 ++-- > 2 files changed, 42 insertions(+), 13 deletions(-) > Thank you for the patch. Please see my comments inline. > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > index 7881e9c93df7..921ed21f7db3 100644 > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > @@ -783,14 +783,15 @@ static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg) > ret = mtk_smi_larb_get(jpeg->larb); > if (ret) > dev_err(jpeg->dev, "mtk_smi_larb_get larbvdec fail %d\n", ret); > - clk_prepare_enable(jpeg->clk_jdec_smi); > - clk_prepare_enable(jpeg->clk_jdec); > + > + ret = clk_bulk_prepare_enable(jpeg->num_clks, jpeg->clks); > + if (ret) > + dev_err(jpeg->dev, "Failed to open jpeg clk: %d\n", ret); > } > > static void mtk_jpeg_clk_off(struct mtk_jpeg_dev *jpeg) > { > - clk_disable_unprepare(jpeg->clk_jdec); > - clk_disable_unprepare(jpeg->clk_jdec_smi); > + clk_bulk_disable_unprepare(jpeg->num_clks, jpeg->clks); > mtk_smi_larb_put(jpeg->larb); > } > > @@ -939,6 +940,7 @@ static int mtk_jpeg_clk_init(struct mtk_jpeg_dev *jpeg) > { > struct device_node *node; > struct platform_device *pdev; > + int ret, i; > > node = of_parse_phandle(jpeg->dev->of_node, "mediatek,larb", 0); > if (!node) > @@ -952,12 +954,39 @@ static int mtk_jpeg_clk_init(struct mtk_jpeg_dev *jpeg) > > jpeg->larb = &pdev->dev; > > - jpeg->clk_jdec = devm_clk_get(jpeg->dev, "jpgdec"); > - if (IS_ERR(jpeg->clk_jdec)) > - return PTR_ERR(jpeg->clk_jdec); > + jpeg->num_clks = > + of_property_count_strings(jpeg->dev->of_node, "clock-names"); > + > + if (jpeg->num_clks > 0) { > + jpeg->clks = devm_kcalloc(jpeg->dev, jpeg->num_clks, > + sizeof(struct clk_bulk_data), > + GFP_KERNEL); > + if (!jpeg->clks) > + return -ENOMEM; > + } else { > + dev_err(&pdev->dev, "Failed to get jpeg clock count\n"); > + return -EINVAL; > + } > + > + for (i = 0; i < jpeg->num_clks; i++) { > + ret = of_property_read_string_index(jpeg->dev->of_node, > + "clock-names", i, > + &jpeg->clks->id); The names of the clocks must be explicitly specified in the driver, as per the DT bindings. Best regards, Tomasz
On Thu, 2020-07-30 at 16:34 +0000, Tomasz Figa wrote: > Hi Xia, > > On Thu, Jul 23, 2020 at 11:04:45AM +0800, Xia Jiang wrote: > > Use the generic of_property_* helpers to get the clock_nums and clocks > > from device tree. > > Use the generic clk_bulk_* helpers to enable and disable clocks. > > > > Signed-off-by: Xia Jiang <xia.jiang@mediatek.com> > > --- > > v10: new add patch > > --- > > .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 47 +++++++++++++++---- > > .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 8 ++-- > > 2 files changed, 42 insertions(+), 13 deletions(-) > > > > Thank you for the patch. Please see my comments inline. > > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > index 7881e9c93df7..921ed21f7db3 100644 > > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > @@ -783,14 +783,15 @@ static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg) > > ret = mtk_smi_larb_get(jpeg->larb); > > if (ret) > > dev_err(jpeg->dev, "mtk_smi_larb_get larbvdec fail %d\n", ret); > > - clk_prepare_enable(jpeg->clk_jdec_smi); > > - clk_prepare_enable(jpeg->clk_jdec); > > + > > + ret = clk_bulk_prepare_enable(jpeg->num_clks, jpeg->clks); > > + if (ret) > > + dev_err(jpeg->dev, "Failed to open jpeg clk: %d\n", ret); > > } > > > > static void mtk_jpeg_clk_off(struct mtk_jpeg_dev *jpeg) > > { > > - clk_disable_unprepare(jpeg->clk_jdec); > > - clk_disable_unprepare(jpeg->clk_jdec_smi); > > + clk_bulk_disable_unprepare(jpeg->num_clks, jpeg->clks); > > mtk_smi_larb_put(jpeg->larb); > > } > > > > @@ -939,6 +940,7 @@ static int mtk_jpeg_clk_init(struct mtk_jpeg_dev *jpeg) > > { > > struct device_node *node; > > struct platform_device *pdev; > > + int ret, i; > > > > node = of_parse_phandle(jpeg->dev->of_node, "mediatek,larb", 0); > > if (!node) > > @@ -952,12 +954,39 @@ static int mtk_jpeg_clk_init(struct mtk_jpeg_dev *jpeg) > > > > jpeg->larb = &pdev->dev; > > > > - jpeg->clk_jdec = devm_clk_get(jpeg->dev, "jpgdec"); > > - if (IS_ERR(jpeg->clk_jdec)) > > - return PTR_ERR(jpeg->clk_jdec); > > + jpeg->num_clks = > > + of_property_count_strings(jpeg->dev->of_node, "clock-names"); > > + > > + if (jpeg->num_clks > 0) { > > + jpeg->clks = devm_kcalloc(jpeg->dev, jpeg->num_clks, > > + sizeof(struct clk_bulk_data), > > + GFP_KERNEL); > > + if (!jpeg->clks) > > + return -ENOMEM; > > + } else { > > + dev_err(&pdev->dev, "Failed to get jpeg clock count\n"); > > + return -EINVAL; > > + } > > + > > + for (i = 0; i < jpeg->num_clks; i++) { > > + ret = of_property_read_string_index(jpeg->dev->of_node, > > + "clock-names", i, > > + &jpeg->clks->id); > > The names of the clocks must be explicitly specified in the driver, as per > the DT bindings. Dear Tomasz, Thank you for your reply. You mean that I should keep the v9 version about names of the clocks in the match data. The v10 version about the names of the clocks follows the upstreamed mtk_venc/vdec.I think that this method is more generic. For example,when other project has more clocks, we can get the names of clocks from dtsi without changing the driver code. What about your further opinion? Best Regards, Xia Jiang > > Best regards, > Tomasz
On Fri, Jul 31, 2020 at 5:20 AM Xia Jiang <xia.jiang@mediatek.com> wrote: > > On Thu, 2020-07-30 at 16:34 +0000, Tomasz Figa wrote: > > Hi Xia, > > > > On Thu, Jul 23, 2020 at 11:04:45AM +0800, Xia Jiang wrote: > > > Use the generic of_property_* helpers to get the clock_nums and clocks > > > from device tree. > > > Use the generic clk_bulk_* helpers to enable and disable clocks. > > > > > > Signed-off-by: Xia Jiang <xia.jiang@mediatek.com> > > > --- > > > v10: new add patch > > > --- > > > .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 47 +++++++++++++++---- > > > .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 8 ++-- > > > 2 files changed, 42 insertions(+), 13 deletions(-) > > > > > > > Thank you for the patch. Please see my comments inline. > > > > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > > index 7881e9c93df7..921ed21f7db3 100644 > > > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > > @@ -783,14 +783,15 @@ static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg) > > > ret = mtk_smi_larb_get(jpeg->larb); > > > if (ret) > > > dev_err(jpeg->dev, "mtk_smi_larb_get larbvdec fail %d\n", ret); > > > - clk_prepare_enable(jpeg->clk_jdec_smi); > > > - clk_prepare_enable(jpeg->clk_jdec); > > > + > > > + ret = clk_bulk_prepare_enable(jpeg->num_clks, jpeg->clks); > > > + if (ret) > > > + dev_err(jpeg->dev, "Failed to open jpeg clk: %d\n", ret); > > > } > > > > > > static void mtk_jpeg_clk_off(struct mtk_jpeg_dev *jpeg) > > > { > > > - clk_disable_unprepare(jpeg->clk_jdec); > > > - clk_disable_unprepare(jpeg->clk_jdec_smi); > > > + clk_bulk_disable_unprepare(jpeg->num_clks, jpeg->clks); > > > mtk_smi_larb_put(jpeg->larb); > > > } > > > > > > @@ -939,6 +940,7 @@ static int mtk_jpeg_clk_init(struct mtk_jpeg_dev *jpeg) > > > { > > > struct device_node *node; > > > struct platform_device *pdev; > > > + int ret, i; > > > > > > node = of_parse_phandle(jpeg->dev->of_node, "mediatek,larb", 0); > > > if (!node) > > > @@ -952,12 +954,39 @@ static int mtk_jpeg_clk_init(struct mtk_jpeg_dev *jpeg) > > > > > > jpeg->larb = &pdev->dev; > > > > > > - jpeg->clk_jdec = devm_clk_get(jpeg->dev, "jpgdec"); > > > - if (IS_ERR(jpeg->clk_jdec)) > > > - return PTR_ERR(jpeg->clk_jdec); > > > + jpeg->num_clks = > > > + of_property_count_strings(jpeg->dev->of_node, "clock-names"); > > > + > > > + if (jpeg->num_clks > 0) { > > > + jpeg->clks = devm_kcalloc(jpeg->dev, jpeg->num_clks, > > > + sizeof(struct clk_bulk_data), > > > + GFP_KERNEL); > > > + if (!jpeg->clks) > > > + return -ENOMEM; > > > + } else { > > > + dev_err(&pdev->dev, "Failed to get jpeg clock count\n"); > > > + return -EINVAL; > > > + } > > > + > > > + for (i = 0; i < jpeg->num_clks; i++) { > > > + ret = of_property_read_string_index(jpeg->dev->of_node, > > > + "clock-names", i, > > > + &jpeg->clks->id); > > > > The names of the clocks must be explicitly specified in the driver, as per > > the DT bindings. > Dear Tomasz, > > Thank you for your reply. > You mean that I should keep the v9 version about names of the clocks in > the match data. > The v10 version about the names of the clocks follows the upstreamed > mtk_venc/vdec.I think that this method is more generic. For example,when > other project has more clocks, we can get the names of clocks from dtsi > without changing the driver code. > What about your further opinion? The problem with that method is that one can put any random names in the DT and the driver will happily accept them, without any correctness checking. Moreover, if the other project has more clocks, it already requires a different compatible string in the DT bindings, so the kernel needs to be changed anyway. Actually this is something that needs to be fixed in the mtk_venc/vdec driver as well. I believe someone just overlooked it when the driver was being reviewed. Best regards, Tomasz
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c index 7881e9c93df7..921ed21f7db3 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c @@ -783,14 +783,15 @@ static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg) ret = mtk_smi_larb_get(jpeg->larb); if (ret) dev_err(jpeg->dev, "mtk_smi_larb_get larbvdec fail %d\n", ret); - clk_prepare_enable(jpeg->clk_jdec_smi); - clk_prepare_enable(jpeg->clk_jdec); + + ret = clk_bulk_prepare_enable(jpeg->num_clks, jpeg->clks); + if (ret) + dev_err(jpeg->dev, "Failed to open jpeg clk: %d\n", ret); } static void mtk_jpeg_clk_off(struct mtk_jpeg_dev *jpeg) { - clk_disable_unprepare(jpeg->clk_jdec); - clk_disable_unprepare(jpeg->clk_jdec_smi); + clk_bulk_disable_unprepare(jpeg->num_clks, jpeg->clks); mtk_smi_larb_put(jpeg->larb); } @@ -939,6 +940,7 @@ static int mtk_jpeg_clk_init(struct mtk_jpeg_dev *jpeg) { struct device_node *node; struct platform_device *pdev; + int ret, i; node = of_parse_phandle(jpeg->dev->of_node, "mediatek,larb", 0); if (!node) @@ -952,12 +954,39 @@ static int mtk_jpeg_clk_init(struct mtk_jpeg_dev *jpeg) jpeg->larb = &pdev->dev; - jpeg->clk_jdec = devm_clk_get(jpeg->dev, "jpgdec"); - if (IS_ERR(jpeg->clk_jdec)) - return PTR_ERR(jpeg->clk_jdec); + jpeg->num_clks = + of_property_count_strings(jpeg->dev->of_node, "clock-names"); + + if (jpeg->num_clks > 0) { + jpeg->clks = devm_kcalloc(jpeg->dev, jpeg->num_clks, + sizeof(struct clk_bulk_data), + GFP_KERNEL); + if (!jpeg->clks) + return -ENOMEM; + } else { + dev_err(&pdev->dev, "Failed to get jpeg clock count\n"); + return -EINVAL; + } + + for (i = 0; i < jpeg->num_clks; i++) { + ret = of_property_read_string_index(jpeg->dev->of_node, + "clock-names", i, + &jpeg->clks->id); + if (ret) { + dev_err(&pdev->dev, + "Failed to get clock name id = %d", i); + return ret; + } + } - jpeg->clk_jdec_smi = devm_clk_get(jpeg->dev, "jpgdec-smi"); - return PTR_ERR_OR_ZERO(jpeg->clk_jdec_smi); + + ret = devm_clk_bulk_get(jpeg->dev, jpeg->num_clks, jpeg->clks); + if (ret) { + dev_err(&pdev->dev, "Failed to get jpeg clock:%d\n", ret); + return ret; + } + + return 0; } static void mtk_jpeg_job_timeout_work(struct work_struct *work) diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h index 4c76a9dcc4b7..a54e1e82e655 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h @@ -52,8 +52,8 @@ enum mtk_jpeg_ctx_state { * @alloc_ctx: videobuf2 memory allocator's context * @dec_vdev: video device node for decoder mem2mem mode * @dec_reg_base: JPEG registers mapping - * @clk_jdec: JPEG hw working clock - * @clk_jdec_smi: JPEG SMI bus clock + * @clks: clock names + * @num_clks: numbers of clock * @larb: SMI device * @job_timeout_work: IRQ timeout structure */ @@ -67,8 +67,8 @@ struct mtk_jpeg_dev { void *alloc_ctx; struct video_device *dec_vdev; void __iomem *dec_reg_base; - struct clk *clk_jdec; - struct clk *clk_jdec_smi; + struct clk_bulk_data *clks; + int num_clks; struct device *larb; struct delayed_work job_timeout_work; };
Use the generic of_property_* helpers to get the clock_nums and clocks from device tree. Use the generic clk_bulk_* helpers to enable and disable clocks. Signed-off-by: Xia Jiang <xia.jiang@mediatek.com> --- v10: new add patch --- .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 47 +++++++++++++++---- .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 8 ++-- 2 files changed, 42 insertions(+), 13 deletions(-)