On Monday 13 August 2012, wei_wang@xxxxxxxxxxxxxx wrote: > From: Wei WANG <wei_wang@xxxxxxxxxxxxxx> > > Realtek PCI-E card reader driver adapts requests from upper-level > sdmmc/memstick layer to the real physical card reader. > > Signed-off-by: Wei WANG <wei_wang@xxxxxxxxxxxxxx> Hi, This looks pretty good overall, I'm generally happy with how you have changed the structure. Looking at some of the details now: > + > +static const struct pcr_ops rts5209_pcr_ops = { > + .extra_init_hw = rts5209_extra_init_hw, > + .optimize_phy = rts5209_optimize_phy, > + .turn_on_led = rts5209_turn_on_led, > + .turn_off_led = rts5209_turn_off_led, > + .enable_auto_blink = rts5209_enable_auto_blink, > + .disable_auto_blink = rts5209_disable_auto_blink, > +}; > ... > + > +static const struct pcr_ops rts5229_pcr_ops = { > + .extra_init_hw = rts5229_extra_init_hw, > + .optimize_phy = rts5229_optimize_phy, > + .turn_on_led = rts5229_turn_on_led, > + .turn_off_led = rts5229_turn_off_led, > + .enable_auto_blink = rts5229_enable_auto_blink, > + .disable_auto_blink = rts5229_disable_auto_blink, > +}; Doing this separation is fine for two or three variants, especially since the code behind them is not very big. If it becomes likely that there will be many more variants, you should consider moving them into separate files. > +void rtsx_pci_start_run(struct rtsx_pcr *pcr) > +{ > + /* If pci device removed, don't queue idle work any more */ > + if (pcr->remove_pci) > + return; > + > + if (pcr->state != PDEV_STAT_RUN) { > + pcr->state = PDEV_STAT_RUN; > + pcr->ops->enable_auto_blink(pcr); > + } > + > + cancel_delayed_work(&pcr->idle_work); > + queue_delayed_work(workqueue, &pcr->idle_work, msecs_to_jiffies(200)); > +} > +EXPORT_SYMBOL_GPL(rtsx_pci_start_run); Let me make sure I understand the logic here: When you send a command to the driver, you turn on the blinking LED and then set a timer to turn it off again after 200 ms, unless another command comes in, right? This is slightly more overhead than I think you should have here. Doing mod_timer() on a timer function might be better if you have a lot of commands calling into rtsx_pci_start_run. Since you currently require a mutex to be held when disabling the blinking, you might need to schedule a work queue without timer from that timer function though, or alternatively change the code to not require the mutex. Arnd _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel