| Message ID | 1625038079-25815-5-git-send-email-kyrie.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support jpeg encode for MT8195 | expand |
On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu <kyrie.wu@mediatek.com> wrote: > Using the needed param for lock on/off function. The description makes less sense. The patch is more than a "refactor". Please change the title accordingly. > static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg) > { > - int ret; > + struct mtk_jpeg_dev *comp_dev; > + struct mtk_jpegenc_pm *pm; > + struct mtk_jpegenc_clk *jpegclk; > + struct mtk_jpegenc_clk_info *clk_info; > + int ret, i; > + > + if (jpeg->variant->is_encoder) { > + for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) { > + comp_dev = jpeg->hw_dev[i]; > + if (!comp_dev) { > + dev_err(jpeg->dev, "Failed to get hw dev\n"); > + return; > + } > + > + pm = &comp_dev->pm; > + jpegclk = &pm->venc_clk; > + clk_info = jpegclk->clk_info; > + ret = clk_prepare_enable(clk_info->jpegenc_clk); > + if (ret) { > + dev_err(jpeg->dev, "jpegenc clk enable %d %s fail\n", > + i, jpegclk->clk_info->clk_name); > + return; > + } > + } > + return; > + } Doesn't it need to call clk_disable_unprepare() in the error paths? > + pm = &comp_dev->pm; > + jpegclk = &pm->venc_clk; > + clk_disable_unprepare(jpegclk->clk_info->jpegenc_clk); Consistency issue: mtk_jpeg_clk_on() has struct mtk_jpegenc_clk_info *clk_info. Why not also have the local variable here? Is it a good idea to just separate functions for ->is_encoder for mtk_jpeg_clk_on() and mtk_jpeg_clk_off()? For example, mtk_jpegenc_clk_on() and mtk_jpegdec_clk_on(). > +/** * struct mtk_jpegenc_clk_info - Structure used to store clock name */ > +struct mtk_jpegenc_clk_info { > + const char *clk_name; > + struct clk *jpegenc_clk; > +}; > + > +/* struct mtk_vcodec_clk - Structure used to store vcodec clock information */ > +struct mtk_jpegenc_clk { > + struct mtk_jpegenc_clk_info *clk_info; > + int clk_num; > +}; > + > +/** * struct mtk_vcodec_pm - Power management data structure */ > +struct mtk_jpegenc_pm { > + struct mtk_jpegenc_clk venc_clk; > + struct device *dev; > + struct mtk_jpeg_dev *mtkdev; > +}; > + > /** > * struct mtk_jpeg_dev - JPEG IP abstraction > * @lock: the mutex protecting this structure > @@ -103,6 +128,9 @@ struct mtk_jpeg_dev { > struct device *larb; > struct delayed_work job_timeout_work; > const struct mtk_jpeg_variant *variant; > + > + struct mtk_jpeg_dev *hw_dev[MTK_JPEGENC_HW_MAX]; > + struct mtk_jpegenc_pm pm; > }; If the declaration cannot align, using 1-space is sufficient.
Hi Kyrie, On Wed, Jun 30, 2021 at 03:27:54PM +0800, kyrie.wu wrote: > Using the needed param for lock on/off function. > > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com> > --- > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 46 ++++++++++++++++++++++++- > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h | 28 +++++++++++++++ > 2 files changed, 73 insertions(+), 1 deletion(-) > Thanks for the patch. Please see my comments inline. Also, how does this patch refactor anything? I only see new code being added. Does the subject and/or commit message need some adjustment? > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > index 24edd87..7c053e3 100644 > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > @@ -1053,7 +1053,32 @@ static int mtk_jpeg_queue_init(void *priv, struct vb2_queue *src_vq, > > static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg) > { > - int ret; > + struct mtk_jpeg_dev *comp_dev; > + struct mtk_jpegenc_pm *pm; > + struct mtk_jpegenc_clk *jpegclk; > + struct mtk_jpegenc_clk_info *clk_info; > + int ret, i; > + > + if (jpeg->variant->is_encoder) { > + for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) { Why do we need to enable clocks for all hardware instances? Wouldn't it make more sense to only enable the clock for the instance that is selected for given encode job? > + comp_dev = jpeg->hw_dev[i]; > + if (!comp_dev) { > + dev_err(jpeg->dev, "Failed to get hw dev\n"); > + return; > + } > + > + pm = &comp_dev->pm; > + jpegclk = &pm->venc_clk; > + clk_info = jpegclk->clk_info; > + ret = clk_prepare_enable(clk_info->jpegenc_clk); > + if (ret) { > + dev_err(jpeg->dev, "jpegenc clk enable %d %s fail\n", > + i, jpegclk->clk_info->clk_name); Missing undo. (But the suggestion below would take care of it.) > + return; > + } > + } How about using the clk_bulk_ API instead of the open coded loop? > + return; > + } Rather than multiple if/else variants in one function, it's a common practice to have two separate functions and then a function pointer in a hardware variant descriptor struct pointing to the right function. It makes the code more readable. > > ret = mtk_smi_larb_get(jpeg->larb); > if (ret) > @@ -1067,6 +1092,25 @@ static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg) > > static void mtk_jpeg_clk_off(struct mtk_jpeg_dev *jpeg) > { > + struct mtk_jpeg_dev *comp_dev; > + struct mtk_jpegenc_pm *pm; > + struct mtk_jpegenc_clk *jpegclk; > + int i; > + > + if (jpeg->variant->is_encoder) { > + for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) { > + comp_dev = jpeg->hw_dev[i]; > + if (!comp_dev) { > + dev_err(jpeg->dev, "Failed to get hw dev\n"); > + return; > + } > + > + pm = &comp_dev->pm; > + jpegclk = &pm->venc_clk; > + clk_disable_unprepare(jpegclk->clk_info->jpegenc_clk); > + } > + return; > + } Same comments here as for the clk_on function. > clk_bulk_disable_unprepare(jpeg->variant->num_clks, > jpeg->variant->clks); > mtk_smi_larb_put(jpeg->larb); > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > index bdbd768..93ea71c 100644 > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > @@ -75,6 +75,31 @@ struct mtk_jpeg_variant { > u32 cap_q_default_fourcc; > }; > > +enum mtk_jpegenc_hw_id { > + MTK_JPEGENC_HW0, > + MTK_JPEGENC_HW1, > + MTK_JPEGENC_HW_MAX, > +}; There is no added value from the enum above. Just use integer index, > + > +/** * struct mtk_jpegenc_clk_info - Structure used to store clock name */ > +struct mtk_jpegenc_clk_info { > + const char *clk_name; > + struct clk *jpegenc_clk; > +}; > + > +/* struct mtk_vcodec_clk - Structure used to store vcodec clock information */ > +struct mtk_jpegenc_clk { > + struct mtk_jpegenc_clk_info *clk_info; > + int clk_num; > +}; This looks like the generic clk_bulk_data struct. > + > +/** * struct mtk_vcodec_pm - Power management data structure */ vcodec? > +struct mtk_jpegenc_pm { > + struct mtk_jpegenc_clk venc_clk; venc? > + struct device *dev; > + struct mtk_jpeg_dev *mtkdev; > +}; > + > /** > * struct mtk_jpeg_dev - JPEG IP abstraction > * @lock: the mutex protecting this structure > @@ -103,6 +128,9 @@ struct mtk_jpeg_dev { > struct device *larb; > struct delayed_work job_timeout_work; > const struct mtk_jpeg_variant *variant; > + > + struct mtk_jpeg_dev *hw_dev[MTK_JPEGENC_HW_MAX]; Why is this recursively having the same struct as its children? Should we have a separate struct that describes a hardware instance (core?)? 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 24edd87..7c053e3 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c @@ -1053,7 +1053,32 @@ static int mtk_jpeg_queue_init(void *priv, struct vb2_queue *src_vq, static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg) { - int ret; + struct mtk_jpeg_dev *comp_dev; + struct mtk_jpegenc_pm *pm; + struct mtk_jpegenc_clk *jpegclk; + struct mtk_jpegenc_clk_info *clk_info; + int ret, i; + + if (jpeg->variant->is_encoder) { + for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) { + comp_dev = jpeg->hw_dev[i]; + if (!comp_dev) { + dev_err(jpeg->dev, "Failed to get hw dev\n"); + return; + } + + pm = &comp_dev->pm; + jpegclk = &pm->venc_clk; + clk_info = jpegclk->clk_info; + ret = clk_prepare_enable(clk_info->jpegenc_clk); + if (ret) { + dev_err(jpeg->dev, "jpegenc clk enable %d %s fail\n", + i, jpegclk->clk_info->clk_name); + return; + } + } + return; + } ret = mtk_smi_larb_get(jpeg->larb); if (ret) @@ -1067,6 +1092,25 @@ static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg) static void mtk_jpeg_clk_off(struct mtk_jpeg_dev *jpeg) { + struct mtk_jpeg_dev *comp_dev; + struct mtk_jpegenc_pm *pm; + struct mtk_jpegenc_clk *jpegclk; + int i; + + if (jpeg->variant->is_encoder) { + for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) { + comp_dev = jpeg->hw_dev[i]; + if (!comp_dev) { + dev_err(jpeg->dev, "Failed to get hw dev\n"); + return; + } + + pm = &comp_dev->pm; + jpegclk = &pm->venc_clk; + clk_disable_unprepare(jpegclk->clk_info->jpegenc_clk); + } + return; + } clk_bulk_disable_unprepare(jpeg->variant->num_clks, jpeg->variant->clks); mtk_smi_larb_put(jpeg->larb); diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h index bdbd768..93ea71c 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h @@ -75,6 +75,31 @@ struct mtk_jpeg_variant { u32 cap_q_default_fourcc; }; +enum mtk_jpegenc_hw_id { + MTK_JPEGENC_HW0, + MTK_JPEGENC_HW1, + MTK_JPEGENC_HW_MAX, +}; + +/** * struct mtk_jpegenc_clk_info - Structure used to store clock name */ +struct mtk_jpegenc_clk_info { + const char *clk_name; + struct clk *jpegenc_clk; +}; + +/* struct mtk_vcodec_clk - Structure used to store vcodec clock information */ +struct mtk_jpegenc_clk { + struct mtk_jpegenc_clk_info *clk_info; + int clk_num; +}; + +/** * struct mtk_vcodec_pm - Power management data structure */ +struct mtk_jpegenc_pm { + struct mtk_jpegenc_clk venc_clk; + struct device *dev; + struct mtk_jpeg_dev *mtkdev; +}; + /** * struct mtk_jpeg_dev - JPEG IP abstraction * @lock: the mutex protecting this structure @@ -103,6 +128,9 @@ struct mtk_jpeg_dev { struct device *larb; struct delayed_work job_timeout_work; const struct mtk_jpeg_variant *variant; + + struct mtk_jpeg_dev *hw_dev[MTK_JPEGENC_HW_MAX]; + struct mtk_jpegenc_pm pm; }; /**
Using the needed param for lock on/off function. Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com> --- drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 46 ++++++++++++++++++++++++- drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h | 28 +++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-)