lkml.org 
[lkml]   [2013]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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




\
 
 \ /
  Last update: 2013-05-21 10:41    [W:0.082 / U:0.888 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site