>
> > + req.func_cb = NULL;
>
> maybe below is a better patch:
>
> diff --git a/drivers/i2c/chips/twl4030-madc.c
> b/drivers/i2c/chips/twl4030-madc.c
> index 72b126b..6d8915e 100644
> --- a/drivers/i2c/chips/twl4030-madc.c
> +++ b/drivers/i2c/chips/twl4030-madc.c
> @@ -360,7 +360,7 @@ static int twl4030_madc_ioctl(struct inode *inode,
> struct file *filp,
>
> switch (cmd) {
> case TWL4030_MADC_IOCX_ADC_RAW_READ: {
> - struct twl4030_madc_request req;
> + static struct twl4030_madc_request req;
> if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
> return -EINVAL;
I don't like this idea because:
- It's fragile. This struct, which is not a const, gets initialized only
once but we are still passing a pointer to it, expecting that a fairly
complex function will not modify it. This assertion is probably true
today but makes it easier for somebody to create a bug in the future.
- You introduce another shared datum and it is only protected by the BKL
in fs/ioctl.c:vfs_ioctl().
- I didn't see any argument why this variable should be static. Making
local variables static just to get cheap zero initialization is a weird
thing to do IMO.
best regards,
Viktor
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
