I have some opinion about the usage of wait_event_interruptible_timeout()
[email protected] wrote: [snip] > + > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_intr.c > b/drivers/media/video/s5p-mfc/s5p_mfc_intr.c > new file mode 100644 > index 0000000..543f3fb > --- /dev/null > +++ b/drivers/media/video/s5p-mfc/s5p_mfc_intr.c > @@ -0,0 +1,77 @@ > +/* > + * drivers/media/video/samsung/mfc5/s5p_mfc_intr.c > + * > + * C file for Samsung MFC (Multi Function Codec - FIMV) driver > + * This file contains functions used to wait for command completion. > + * > + * Kamil Debski, Copyright (c) 2010 Samsung Electronics > + * http://www.samsung.com/ > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/delay.h> > +#include <linux/errno.h> > +#include <linux/wait.h> > +#include <linux/sched.h> > +#include <linux/io.h> > +#include "regs-mfc5.h" > +#include "s5p_mfc_intr.h" > +#include "s5p_mfc_logmsg.h" > +#include "s5p_mfc_common.h" > + > +int s5p_mfc_wait_for_done_dev(struct s5p_mfc_dev *dev, int command) > +{ > + if (wait_event_interruptible_timeout(dev->queue, > + (dev->int_cond && (dev->int_type == command > + || dev->int_type == S5P_FIMV_R2H_CMD_DECODE_ERR_RET)), > + msecs_to_jiffies(MFC_INT_TIMEOUT)) == 0) { > + mfc_err("Interrupt (%d dev) timed out.\n", dev->int_type); > + return 1; > + } > + mfc_debug("Finished waiting (dev->queue, %d).\n", dev->int_type); > + if (dev->int_type == S5P_FIMV_R2H_CMD_ERROR_RET) > + return 1; > + return 0; > +} You used wait_event_interruptible_timeout() in the driver, but this function is considered not only int. by MFC but also int. by some signal. I'm wondering whether we have to consider interrupt by signal in the middle of hw operation. and also I cannot see some operation(handler) in case of wake-up by signal. So, why don’t you remove the interruptible function or add some operation in case of wake-up by signal. (refer to the other driver in the kernel) > + > +void s5p_mfc_clean_dev_int_flags(struct s5p_mfc_dev *dev) > +{ > + dev->int_cond = 0; > + dev->int_type = 0; > + dev->int_err = 0; > +} > + > +int s5p_mfc_wait_for_done_ctx(struct s5p_mfc_ctx *ctx, > + int command, int interrupt) > +{ > + int ret; > + if (interrupt) { > + ret = wait_event_interruptible_timeout(ctx->queue, > + (ctx->int_cond && (ctx->int_type == command > + || ctx->int_type == S5P_FIMV_R2H_CMD_DECODE_ERR_RET)), > + msecs_to_jiffies(MFC_INT_TIMEOUT)); > + } else { > + ret = wait_event_timeout(ctx->queue, > + (ctx->int_cond && (ctx->int_type == command > + || ctx->int_type == S5P_FIMV_R2H_CMD_DECODE_ERR_RET)), > + msecs_to_jiffies(MFC_INT_TIMEOUT)); > + } > + if (ret == 0) { > + mfc_err("Interrupt (%d ctx) timed out.\n", ctx->int_type); > + return 1; > + } > + mfc_debug("Finished waiting (ctx->queue, %d).\n", ctx->int_type); > + if (ctx->int_type == S5P_FIMV_R2H_CMD_ERROR_RET) > + return 1; > + return 0; > +} > + > +void s5p_mfc_clean_ctx_int_flags(struct s5p_mfc_ctx *ctx) > +{ > + ctx->int_cond = 0; > + ctx->int_type = 0; > + ctx->int_err = 0; > +} [snip] > -- > 1.6.3.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
