Re: [PATCH v2,4/9] media: mtk-jpegenc: Refactor jpeg clock interface

[Tomasz Figa]

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@xxxxxxxxxxxx>
> ---
>  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