Re: [PATCH v2,4/9] media: mtk-jpegenc: Refactor jpeg clock interface
From: Tzung-Bi Shih <hidden>
Date: 2021-07-06 11:00:54
Also in:
linux-devicetree, linux-media, linux-mediatek, lkml
On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu [off-list ref] 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().
quoted hunk ↗ jump to hunk
+/** * 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel