Messages in this thread | | | Date | Tue, 21 May 2013 01:22:05 -0700 | From | "Christian Daudt" <> | Subject | Re: [PATCH V2 1/2] ARM: mmc: bcm281xx SDHCI driver |
| |
Hi Arnd, Thanks for the review. See below for comments.
On 13-05-16 03:09 PM, Arnd Bergmann wrote: > On Friday 10 May 2013, Christian Daudt wrote: >> + >> +struct sdhci_bcm_kona_cfg { >> + unsigned int max_freq; >> + int is_8bit; >> + int irq; >> + int cd_gpio; >> + int wp_gpio; >> + int non_removable; >> +}; > I see no use for this structure to be separate: a lot of the fields are > duplicated in the sdhci_host, or should just get merged into > sdhci_bcm_kona_dev. ok. Will do >> +struct sdhci_bcm_kona_dev { >> + struct sdhci_bcm_kona_cfg *cfg; >> + struct device *dev; >> + struct sdhci_host *host; >> + struct clk *peri_clk; >> + struct clk *sleep_clk; >> +}; > The *dev and *host members in this structure are redundant, just > allocate it together with sdhci_host and use use container_of() > to get from the sdhci_host back it it. ok. >> +static void sdhci_bcm_kona_sd_init(struct sdhci_host *host) >> +{ >> + unsigned int val; >> + >> + /* enable the interrupt from the IP core */ >> + val = sdhci_readl(host, KONA_SDHOST_COREIMR); >> + val |= KONA_SDHOST_IP; >> + sdhci_writel(host, val, KONA_SDHOST_COREIMR); >> + >> + /* Enable the AHB clock gating module to the host */ >> + val = sdhci_readl(host, KONA_SDHOST_CORECTRL); >> + val |= KONA_SDHOST_EN; >> + >> + /* >> + * Back-to-Back register write needs a delay of 1ms at bootup (min 10uS) >> + * Back-to-Back writes to same register needs delay when SD bus clock >> + * is very low w.r.t AHB clock, mainly during boot-time and during card >> + * insert-removal. >> + */ >> + mdelay(1); >> + sdhci_writel(host, val, KONA_SDHOST_CORECTRL); >> +} > Why not use msleep() instead of mdelay() here? I don't think that there's any reason. will replace. > >> +static int sdhci_bcm_kona_sd_card_emulate(struct sdhci_host *host, int insert) >> +{ >> + struct sdhci_pltfm_host *pltfm_priv = sdhci_priv(host); >> + struct sdhci_bcm_kona_dev *kona_dev = pltfm_priv->priv; >> + u32 val; >> + unsigned long flags; >> + >> + /* this function can be called from various contexts including ISR */ >> + spin_lock_irqsave(&host->lock, flags); >> + /* Ensure SD bus scanning to detect media change */ >> + host->mmc->rescan_disable = 0; >> + >> + /* >> + * Back-to-Back register write needs a delay of min 10uS. >> + * Back-to-Back writes to same register needs delay when SD bus clock >> + * is very low w.r.t AHB clock, mainly during boot-time and during card >> + * insert-removal. >> + * We keep 20uS >> + */ >> + udelay(20); >> + val = sdhci_readl(host, KONA_SDHOST_CORESTAT); > Does the delay have to be done with interrupts disabled? That is not particularly > nice. > > I hope the hardware designers have been appropriately punished for the creating > such crap. I had some internal discussions on this one, and the code was originally written for non-threaded irqs. Now that it is only called as threaded_irq thread_fn, it is safe to replace the spinlock that includes the delay with a mutex instead.
>> +static void sdhci_bcm_kona_init_74_clocks(struct sdhci_host *host, >> + u8 power_mode) >> +{ >> + if (power_mode == MMC_POWER_OFF) >> + return; >> + else >> + mdelay(10); >> +} > This requires at the minimum a comment about why the mdelay is needed. > Maybe we can change the set_ios function so we never need to call it > in atomic context. I'll look into this one. >> +static struct sdhci_bcm_kona_cfg * __init sdhci_bcm_kona_parse_dt( >> + struct platform_device *pdev) >> +{ >> + struct sdhci_bcm_kona_cfg *cfg; >> + struct device_node *np = pdev->dev.of_node; >> + u32 temp; > fold this function into probe() ok. >> + if (!np) >> + return NULL; > impossible ok >> + cfg = devm_kzalloc(&pdev->dev, sizeof(*cfg), GFP_KERNEL); >> + if (!cfg) { >> + dev_err(&pdev->dev, "Can't allocate platform cfg\n"); >> + return NULL; >> + } > Not needed what is not needed ? >> +static int __init sdhci_bcm_kona_probe(struct platform_device *pdev) >> +{ >> + const struct of_device_id *match; > constant, so not needed. I'll remove it. >> + struct sdhci_bcm_kona_cfg *kona_cfg = NULL; > No need to initialize this. ok >> + const struct sdhci_pltfm_data *plat_data; > make it global. why make this global ? > >> + struct sdhci_bcm_kona_dev *kona_dev = NULL; > No need to initialize this. ok. >> + kona_dev = devm_kzalloc(dev, sizeof(*kona_dev), GFP_KERNEL); >> + if (!kona_dev) { >> + dev_err(dev, "Can't allocate kona_dev\n"); >> + ret = -ENOMEM; >> + goto err_pltfm_free; >> + } > It is rather silly to have the base sdhci code allocate extra > memory for the platform drivers but then require an extra allocation. > Better change the sdhci_pltfm_init function to let you pass the extra > allocation size. ok, I'll look into this. >> +MODULE_AUTHOR("Broadcom"); > No person? > A collective effort :)
Thanks, csd
| |