[<prev] [next>] [day] [month] [year] [list]
Message-ID: <CA+Px+wW-L__+mGpvA3P0C7KQh=mJrQ2g0ciPVO=rKnueONqo0A@mail.gmail.com>
Date: Tue, 6 Jul 2021 19:00:38 +0800
From: Tzung-Bi Shih <tzungbi@...gle.com>
To: "kyrie.wu" <kyrie.wu@...iatek.com>
Cc: Hans Verkuil <hverkuil-cisco@...all.nl>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Bin Liu <bin.liu@...iatek.com>,
Matthias Brugger <matthias.bgg@...il.com>,
Tzung-Bi Shih <tzungbi@...omium.org>,
Project_Global_Chrome_Upstream_Group@...iatek.com,
linux-media@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org,
Tomasz Figa <tfiga@...omium.org>, xia.jiang@...iatek.com,
maoguang.meng@...iatek.com, srv_heupstream@...iatek.com
Subject: Re: [PATCH v2,4/9] media: mtk-jpegenc: Refactor jpeg clock interface
On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu <kyrie.wu@...iatek.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.
Powered by blists - more mailing lists