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...@samsung.com] > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
