RE: [PATCH v5 1/4] [media] s5p-mfc: Update MFCv5 driver for callback based architecture

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Arun,

Please find my comments inline.

Best wishes,
--
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center


> From: Arun Kumar K [mailto:arun.kk@xxxxxxxxxxx]
> Sent: 27 August 2012 04:58

[...]

> diff --git a/drivers/media/video/s5p-mfc/s5p_mfc.c
b/drivers/media/video/s5p-
> mfc/s5p_mfc.c
> index 9bb68e7..ab66680 100644
> --- a/drivers/media/video/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/video/s5p-mfc/s5p_mfc.c
> @@ -21,15 +21,15 @@

[...]

> @@ -552,22 +546,23 @@ static irqreturn_t s5p_mfc_irq(int irq, void *priv)
>  	atomic_set(&dev->watchdog_cnt, 0);
>  	ctx = dev->ctx[dev->curr_ctx];
>  	/* Get the reason of interrupt and the error code */
> -	reason = s5p_mfc_get_int_reason();
> -	err = s5p_mfc_get_int_err();
> +	reason = s5p_mfc_get_int_reason(dev);
> +	err = s5p_mfc_get_int_err(dev);
>  	mfc_debug(1, "Int reason: %d (err: %08x)\n", reason, err);
>  	switch (reason) {
> -	case S5P_FIMV_R2H_CMD_ERR_RET:
> +	case S5P_MFC_R2H_CMD_ERR_RET:
>  		/* An error has occured */
>  		if (ctx->state == MFCINST_RUNNING &&
> -			s5p_mfc_err_dec(err) >= S5P_FIMV_ERR_WARNINGS_START)
> +			s5p_mfc_err_dec(err) >= s5p_mfc_get_warn_start(dev))

It's still a function call. I have meant that it could an argument of the
dev structure that is set in probe. It's much better to use a value directly
than call a function.

>  			s5p_mfc_handle_frame(ctx, reason, err);
>  		else
>  			s5p_mfc_handle_error(ctx, reason, err);
>  		clear_bit(0, &dev->enter_suspend);
>  		break;
> 
> -	case S5P_FIMV_R2H_CMD_SLICE_DONE_RET:
> -	case S5P_FIMV_R2H_CMD_FRAME_DONE_RET:
> +	case S5P_MFC_R2H_CMD_SLICE_DONE_RET:
> +	case S5P_MFC_R2H_CMD_FIELD_DONE_RET:
> +	case S5P_MFC_R2H_CMD_FRAME_DONE_RET:
>  		if (ctx->c_ops->post_frame_start) {
>  			if (ctx->c_ops->post_frame_start(ctx))
>  				mfc_err("post_frame_start() failed\n");

[...]

> +/* This function is used to send a command to the MFC */
> +int s5p_mfc_cmd_host2risc(struct s5p_mfc_dev *dev, int cmd,
> +				struct s5p_mfc_cmd_args *args)
> +{
> +	return s5p_mfc_hw_call(s5p_mfc_cmds, cmd_host2risc, dev, cmd, args);
>  }
> 	

Arun, also I think that we misunderstood each other. I suggested that
for example s5p_mfc_cmd_host2risc could be changed to
s5p_mfc_hw_call(s5p_mfc_cmds, cmd_host2risc, dev, cmd, args);

It would be much better to use s5p_mfc_hw_call directly in the code.
The idea was to completely remove function such as the above, the ones
that have nothing more than a call to the ops.

[...]

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux