On 06/16/2011 07:23 PM, Dan Carpenter wrote: > This patch means we poll 20 times a second and test to see if we > have gone for 1000 polls without doing anything if so then we sleep. > Is there a way to get the same effect without the polling? Like > maybe set a timer for 50 seconds in the future and every time we do > something then push the timer further into the future? >
Dear Dan, Thanks for your suggestions, which have been applied to new patches, and patches have been sent out. Please help review. BRs Edwin > On Thu, Jun 16, 2011 at 03:10:35PM +0800, [email protected] wrote: >> +config REALTEK_AUTOPM >> + bool "Realtek Card Reader autosuspend support" >> + depends on USB_STORAGE_REALTEK >> + default y >> > This should probably depend on CONFIG_PM_RUNTIME as well. > >> struct rts51x_chip { >> - u16 vendor_id; >> - u16 product_id; >> - char max_lun; >> + u16 vendor_id; >> + u16 product_id; >> + char max_lun; > There are about 50 lines of whitespace changes in this patch, but > those should be separated out and sent as a second patch. I'm not > going to go through the patch and point out all 50 lines > individually. > >> /* flag definition */ >> @@ -97,9 +139,28 @@ struct rts51x_chip { >> #define RTS51X_GET_VID(chip) ((chip)->vendor_id) >> #define RTS51X_GET_PID(chip) ((chip)->product_id) >> >> +#define VENDOR_ID(chip) ((chip)->status->vid) >> +#define PRODUCT_ID(chip) ((chip)->status->pid) >> +#ifdef CONFIG_REALTEK_AUTOPM >> +#define FW_VERSION(chip) ((chip)->status->fw_ver) >> +#else >> #define FW_VERSION(chip) ((chip)->status[0].fw_ver) > These definitions of FW_VERSION() produce the same thing, so we only > need one. The original definition is better because chip->status > is an array, it's allocated in init_realtek_cr() > > size = (chip->max_lun + 1) * sizeof(struct rts51x_status); > chip->status = kzalloc(size, GFP_KERNEL); > > VENDOR_ID() and PRODUCT_ID() should be defined the same way. > >> -#define wait_timeout_x(task_state, msecs) \ >> -do { \ >> - set_current_state((task_state)); \ >> - schedule_timeout((msecs) * HZ / 1000); \ >> -} while (0) >> - >> -#define wait_timeout(msecs) \ >> - wait_timeout_x(TASK_INTERRUPTIBLE, (msecs)) >> - > Removing these unused macros is a nice cleanup, but it should go in > a separate patch. > >> +static int fw5895_init(struct us_data *us) >> +{ >> + struct rts51x_chip *chip = (struct rts51x_chip *)(us->extra); >> + int retval; >> + u8 val; >> + >> + US_DEBUGP("-- %s --\n", __func__); >> + >> + if ((PRODUCT_ID(chip) != 0x0158) || (FW_VERSION(chip) != 0x5895)) { >> + US_DEBUGP("Not the specified device, return immediately!\n"); >> + return STATUS_SUCCESS; > Should we really return SUCCESS here? No one actually checks the > return value of this function so I suppose it doesn't matter either > way. > >> + } >> + >> + retval = rts51x_read_mem(us, 0xFD6F, &val, 1); >> + if (retval != STATUS_SUCCESS) { >> + US_DEBUGP("Read memory fail\n"); >> + RETURN(STATUS_FAIL); > There are two debug output lines in a row here. One is enough. > >> +void rts51x_polling(struct work_struct *work) >> +{ >> + struct rts51x_chip *chip = container_of(work, struct rts51x_chip, >> + rts51x_delayed_work.work); >> + struct us_data *us = chip->us; >> + >> + /* lock the device pointers */ >> + mutex_lock(&(us->dev_mutex)); > ^ ^ > These parens are not needed. (My static checker sucks and > it complains that the mutex_lock() and mutex_unlock() don't > match because one has parens and one doesn't). > >> + >> + US_DEBUGP("%s: <---\n", __func__); >> + >> + switch (rts51x_get_stat(chip)) { >> + case RTS51X_STAT_INIT: >> + break; >> + case RTS51X_STAT_RUN: >> + chip->idle_counter = 0; >> + break; >> + case RTS51X_STAT_IDLE:{ >> + if (chip->idle_counter < >> + (ss_delay * 1000 / POLLING_INTERVAL)) { >> + US_DEBUGP("%s: idle_counter ++\n", __func__); >> + chip->idle_counter++; >> + break; >> + } > ^ > Should be back one indent level. > >> + } > ^ > This pair of braces is not needed. > >> + case RTS51X_STAT_SS:{ >> + US_DEBUGP("%s: RTS51X_STAT_SS, intf->pm_usage_cnt:%d," >> + "power.usage:%d\n", __func__, >> + atomic_read(&us->pusb_intf->pm_usage_cnt), >> + atomic_read(&us->pusb_intf->dev.power.usage_count)); >> + >> + if (atomic_read(&us->pusb_intf->pm_usage_cnt) > 0) { >> + US_DEBUGP("%s: Ready to enter SS state.\n", >> + __func__); >> + rts51x_set_stat(chip, RTS51X_STAT_SS); >> + /* ignore mass storage interface's children */ >> + pm_suspend_ignore_children(&us->pusb_intf->dev, true); >> + usb_autopm_put_interface(us->pusb_intf); >> + US_DEBUGP("%s: RTS51X_STAT_SS 01," >> + "intf->pm_usage_cnt:%d, power.usage:%d\n", >> + __func__, >> + atomic_read(&us->pusb_intf->pm_usage_cnt), >> + atomic_read( >> + &us->pusb_intf->dev.power.usage_count)); >> + } >> + break; >> + } > ^ > Same situation with these braces. > > >> + default: >> + US_DEBUGP("%s: Unknonwn state !!!\n", __func__); >> + break; >> + } >> + >> + if (rts51x_get_stat(chip) != RTS51X_STAT_SS) >> + queue_delayed_work(chip->rts51x_wq, >> + &(chip->rts51x_delayed_work), > ^ ^ > These parens are not needed. > >> + POLLING_INTERVAL * HZ / 1000); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Use msecs_to_jiffies(POLLING_INTERVAL) > >> + /* unlock the device pointers */ >> + mutex_unlock(&us->dev_mutex); >> + >> + US_DEBUGP("%s: --->\n", __func__); >> +} >> + > regards, > dan carpenter _______________________________________________ devel mailing list [email protected] http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
