[PATCH 1/2] drivers/mfd: Add realtek pcie card reader driver
Arnd Bergmann
arnd at arndb.de
Mon Aug 13 13:06:21 UTC 2012
On Monday 13 August 2012, wei_wang at realsil.com.cn wrote:
> From: Wei WANG <wei_wang at realsil.com.cn>
>
> 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 at realsil.com.cn>
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
More information about the devel
mailing list