Hi Kamil, Thank you for the comments. Please find my response inline. I will provide the updated patches rebased to the new media tree. On Wed, Aug 29, 2012 at 3:29 AM, Kamil Debski <k.debski@xxxxxxxxxxx> wrote: > 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. > Ok got it. Will change it. >> 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. > Ok Kamil now I understood. You wanted the macro to replace the original function calls from other common files of the driver. Will modify it accordingly. > [...] > > -- Regards Arunÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ¥Šwÿº{.nÇ+‰·¥Š{±þg?‰¯âžØ^n‡r¡ö¦zË?ëh™¨èÚ&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~††Ûiÿÿï?êÿ‘êçz_è®æj:+v‰¨þ)ߣøm
