Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1625038079-25815-1-git-send-email-kyrie.wu@mediatek.com> <1625038079-25815-5-git-send-email-kyrie.wu@mediatek.com>
In-Reply-To: <1625038079-25815-5-git-send-email-kyrie.wu@mediatek.com>
From:   Tzung-Bi Shih <tzungbi@google.com>
Date:   Tue, 6 Jul 2021 19:00:38 +0800
Message-ID: <CA+Px+wW-L__+mGpvA3P0C7KQh=mJrQ2g0ciPVO=rKnueONqo0A@mail.gmail.com>
Subject: Re: [PATCH v2,4/9] media: mtk-jpegenc: Refactor jpeg clock interface
To:     "kyrie.wu" <kyrie.wu@mediatek.com>
Cc:     Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Bin Liu <bin.liu@mediatek.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Tzung-Bi Shih <tzungbi@chromium.org>,
        Project_Global_Chrome_Upstream_Group@mediatek.com,
        linux-media@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org,
        Tomasz Figa <tfiga@chromium.org>, xia.jiang@mediatek.com,
        maoguang.meng@mediatek.com, srv_heupstream@mediatek.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.