Re: [PATCH v6 1/3] drivers/mfd: Add realtek pcie card reader driver

2012-10-06 Thread wwang
于 2012年10月01日 06:49, Samuel Ortiz 写道:
 Hi Wei,

 On Tue, Sep 11, 2012 at 12:54:22PM +0800, wei_w...@realsil.com.cn wrote:
 Although  pretty big, the patch looks mostly good to me.
 I only have a few comments:

 diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
 index d1facef..4c07a34 100644
 --- a/drivers/mfd/Kconfig
 +++ b/drivers/mfd/Kconfig
 @@ -63,6 +63,15 @@ config MFD_SM501_GPIO
   lines on the SM501. The platform data is used to supply the
   base number for the first GPIO line to register.
  
 +config MFD_RTSX_PCI
 +tristate Support for Realtek PCI-E driver-based card reader
 +depends on PCI
 +help
 +  This supports for Realtek PCI-Express driver-based card reader 
 including
 +  rts5209, rts5229, etc. Realtek driver-based card reader supports 
 access
 driver-based ?

Sorry, this is our internal naming convention. We have two groups of
card reader, one group has embedded MCU and uses in-box driver, the
other hasn't MCU and uses vendor driver. So we call the first group as
MCU-based reader, and the second group as driver-based reader. This
naming convention may confuse others, I will modify the description.

 +
 +static u8 rts5209_get_ic_version(struct rtsx_pcr *pcr)
 +{
 +u8 val;
 +
 +val = rtsx_pci_readb(pcr, 0x1C);
 +return val  0x0F;
 For your IO accesses, it would e worth looking at the regmap MMIO routines. It
 would also simplify your main probe routine.

regmap MMIO is great. But I haven't used it before, and it seems that we
haven't enough time to test it in this short merge window. I will take
your advice and use these routines in the near future.

 +static int rts5209_turn_on_led(struct rtsx_pcr *pcr)
 +{
 +return rtsx_pci_write_register(pcr, 0xFD58, 0x01, 0x00);
 You have many hardcoded register adresses around this code. Defining them in
 your header file wouldn't hurt.

I will modify it soon.


 +static struct platform_device *rtsx_pci_create_subdev(struct rtsx_pcr *pcr,
 +const char *name, int slot_id)
 +{
 +struct platform_device *p_dev;
 +int err;
 +
 +p_dev = platform_device_alloc(name, pcr-id);
 +if (p_dev == NULL) {
 +dev_dbg((pcr-pci-dev), alloc platform device fail!\n);
 +return NULL;
 +}
 +
 +p_dev-dev.parent = (pcr-pci-dev);
 +platform_set_drvdata(p_dev, pcr);
 +pcr-slots[slot_id].p_dev = p_dev;
 +
 +err = platform_device_add(p_dev);
 +if (err) {
 +dev_dbg((pcr-pci-dev), add platform device fail!\n);
 +pcr-slots[slot_id].p_dev = NULL;
 +platform_device_put(p_dev);
 +return NULL;
 +}
 +
 +return p_dev;
 +}
 Please use the MFD device addition API for that.


You suggest using mfd_cell ? I have considered it, but it seems not very
flexible in my situation. I will create two sub devices in the driver,
one for SD/MMC card, the other for Memstick. I need to create the sub
device when the card inserted, and destroy it when the card removed. The
timing to create and destroy is totally up to the user.
But mfd_cell will create and destroy all of the sub devices at the same
time, using API mfd_add_devices and mfd_remove_devices, which can not
meet my desire.

 +static irqreturn_t rtsx_pci_isr(int irq, void *dev_id)
 +{
 +struct rtsx_pcr *pcr = dev_id;
 +u32 int_reg;
 +
 +if (!pcr)
 +return IRQ_NONE;
 +
 +spin_lock(pcr-lock);
 +
 +int_reg = rtsx_pci_readl(pcr, RTSX_BIPR);
 +/* Clear interrupt flag */
 +rtsx_pci_writel(pcr, RTSX_BIPR, int_reg);
 +if ((int_reg  pcr-bier) == 0) {
 +spin_unlock(pcr-lock);
 +return IRQ_NONE;
 +}
 +if (int_reg == 0x) {
 +spin_unlock(pcr-lock);
 +return IRQ_HANDLED;
 +}
 +
 +int_reg = (pcr-bier | 0x7F);
 +
 +if (int_reg  SD_INT) {
 +if (int_reg  SD_EXIST) {
 +pcr-need_reset |= SD_EXIST;
 +} else {
 +pcr-need_release |= SD_EXIST;
 +pcr-need_reset = ~SD_EXIST;
 +}
 +}
 +
 +if (int_reg  MS_INT) {
 +if (int_reg  MS_EXIST) {
 +pcr-need_reset |= MS_EXIST;
 +} else {
 +pcr-need_release |= MS_EXIST;
 +pcr-need_reset = ~MS_EXIST;
 +}
 +}
 +
 +if (pcr-need_reset || pcr-need_release)
 +queue_delayed_work(workqueue, pcr-carddet_work,
 +msecs_to_jiffies(200));
 Why do we need a delayed work here ?

We need a period of time to debounce the CD glitch, so I use a delayed
work here. After 200 milliseconds, I will check the CD signal again to
make sure this is a valid signal.


 +static void rtsx_pci_enter_idle(unsigned long __data)
 +{
 +struct rtsx_pcr *pcr = (struct rtsx_pcr *)__data;
 +
 +queue_work(workqueue, pcr-idle_work);
 +}
 OT: We need threaded timers...


Sorry, I can't understand this...

 +static int __init 

Re: [PATCH] mmc: mxs-mmc: Fix merge issue causing build error

2012-10-06 Thread Shawn Guo
Copy Chris who should be able to help send this fix for 3.7-rc.

Shawn

On Mon, Oct 01, 2012 at 10:52:43PM +0200, Marek Vasut wrote:
 The following error appeared due to trivial merge problem:
 
 drivers/mmc/host/mxs-mmc.c: In function 'mxs_mmc_enable_sdio_irq':
 drivers/mmc/host/mxs-mmc.c:527:3: error: 'struct mxs_mmc_host' has no member 
 named 'base'
 drivers/mmc/host/mxs-mmc.c:527:3: error: 'struct mxs_mmc_host' has no member 
 named 'devid'
 make[3]: *** [drivers/mmc/host/mxs-mmc.o] Error 1
 
 This patches corrects the issue.
 
 Signed-off-by: Marek Vasut ma...@denx.de
 Cc: Fabio Estevam fabio.este...@freescale.com
 Cc: Shawn Guo shawn@linaro.org
 ---
  drivers/mmc/host/mxs-mmc.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
 index bb4c2bf..80d1e6d 100644
 --- a/drivers/mmc/host/mxs-mmc.c
 +++ b/drivers/mmc/host/mxs-mmc.c
 @@ -525,7 +525,7 @@ static void mxs_mmc_enable_sdio_irq(struct mmc_host *mmc, 
 int enable)
   writel(BM_SSP_CTRL0_SDIO_IRQ_CHECK,
  ssp-base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
   writel(BM_SSP_CTRL1_SDIO_IRQ_EN,
 -host-base + HW_SSP_CTRL1(host) + STMP_OFFSET_REG_SET);
 +ssp-base + HW_SSP_CTRL1(ssp) + STMP_OFFSET_REG_SET);
   } else {
   writel(BM_SSP_CTRL0_SDIO_IRQ_CHECK,
  ssp-base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 -- 
 1.7.10.4
 
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] mmc: mxs-mmc: Fix merge issue causing build error

2012-10-06 Thread Marek Vasut
Dear Shawn Guo,

 Copy Chris who should be able to help send this fix for 3.7-rc.
 
 Shawn

Ok, thanks.

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html