On 5/20/2010 2:40 AM, Andrew Morton wrote:
> On Wed, 5 May 2010 15:05:18 +0530
> Viresh KUMAR <viresh.kumar@xxxxxx> wrote:
>
>> This patch adds glue layer for support of sdhci driver on ST SPEAr platform.
>>
>
> Looks OK to my untrained eye. A few very minor things:
>
>>
>> ...
>>
>> +static irqreturn_t sdhci_gpio_irq(int irq, void *dev_id)
>> +{
>> + struct platform_device *pdev = dev_id;
>> + struct sdhci_host *host = platform_get_drvdata(pdev);
>> + struct spear_sdhci *sdhci =
>> + (struct spear_sdhci *)(pdev->dev.platform_data);
>
> Seems wrong to open-code all of this. There should be a
>
> void *platform_device_get_platform_data(struct platform_device *);
>
> but I can't find it. The code can use dev_get_platdata(), however.
OK.
>
>
> Also, you don't _have_ to initialise things at the definition site. If
> you were to do:
>
> struct platform_device *pdev;
> struct sdhci_host *host;
> struct spear_sdhci *sdhci;
>
> pdev = dev_id;
> host = platform_get_drvdata(pdev);
> sdhci = (struct spear_sdhci *)(pdev->dev.platform_data);
>
> then you wouldn't need to do weird things to fit the code in 80-cols.
>
> Also, platform_data has type void*, so the typecast here is unneeded
> and, because it defeats typechecking it is undesirable.
OK.
>
>> + unsigned long gpio_irq_type;
>> + int val;
>> +
>> + val = gpio_get_value(sdhci->data->card_int_gpio);
>> +
>> + /* val == 1 -> card removed, val == 0 -> card inserted */
>> + /* if card removed - set irq for low level, else vice versa */
>> + gpio_irq_type = val ? IRQF_TRIGGER_LOW : IRQF_TRIGGER_HIGH;
>> + set_irq_type(irq, gpio_irq_type);
>> +
>> + if (sdhci->data->card_power_gpio >= 0) {
>> + if (!sdhci->data->power_always_enb) {
>> + /* if card inserted, give power, otherwise remove it */
>> + val = sdhci->data->power_active_high ? !val : val ;
>> + gpio_set_value(sdhci->data->card_power_gpio, val);
>> + }
>> + }
>> +
>> + /* inform sdhci driver about card insertion/removal */
>> + tasklet_schedule(&host->card_tasklet);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int __devinit sdhci_probe(struct platform_device *pdev)
>> +{
>> + struct sdhci_host *host;
>> + struct resource *iomem;
>> + struct spear_sdhci *sdhci;
>> + int ret;
>> +
>> + BUG_ON(pdev == NULL);
>> +
>> + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!iomem) {
>> + ret = -ENOMEM;
>> + dev_dbg(&pdev->dev, "memory resource not defined\n");
>> + goto err;
>> + }
>> +
>> + if (!request_mem_region(iomem->start, resource_size(iomem),
>> + "spear-sdhci")) {
>> + ret = -EBUSY;
>> + dev_dbg(&pdev->dev, "cannot request region\n");
>> + goto err;
>> + }
>> +
>> + sdhci = kzalloc(sizeof(*sdhci), GFP_KERNEL);
>> + if (!sdhci) {
>> + ret = -ENOMEM;
>> + dev_dbg(&pdev->dev, "cannot allocate memory for sdhci\n");
>> + goto err_kzalloc;
>> + }
>> +
>> + /* clk enable */
>> + sdhci->clk = clk_get(&pdev->dev, NULL);
>> + if (IS_ERR(sdhci->clk)) {
>> + ret = PTR_ERR(sdhci->clk);
>> + dev_dbg(&pdev->dev, "Error getting clock\n");
>> + goto err_clk_get;
>> + }
>> +
>> + ret = clk_enable(sdhci->clk);
>> + if (ret) {
>> + dev_dbg(&pdev->dev, "Error enabling clock\n");
>> + goto err_clk_enb;
>> + }
>> +
>> + /* overwrite platform_data */
>> + sdhci->data = (struct sdhci_plat_data *)(pdev->dev.platform_data);
>
> Similarly.
OK.
>
>> + pdev->dev.platform_data = sdhci;
>> +
>> + if (pdev->dev.parent)
>> + host = sdhci_alloc_host(pdev->dev.parent, 0);
>> + else
>> + host = sdhci_alloc_host(&pdev->dev, 0);
>> +
>> + if (IS_ERR(host)) {
>> + ret = PTR_ERR(host);
>> + dev_dbg(&pdev->dev, "error allocating host\n");
>> + goto err_alloc_host;
>> + }
>> +
>> + host->hw_name = "sdhci";
>> + host->ops = &sdhci_pltfm_ops;
>> + host->irq = platform_get_irq(pdev, 0);
>> + host->quirks = SDHCI_QUIRK_BROKEN_ADMA;
>> +
>> + host->ioaddr = ioremap(iomem->start, resource_size(iomem));
>> + if (!host->ioaddr) {
>> + ret = -ENOMEM;
>> + dev_dbg(&pdev->dev, "failed to remap registers\n");
>> + goto err_ioremap;
>> + }
>> +
>> + ret = sdhci_add_host(host);
>> + if (ret) {
>> + dev_dbg(&pdev->dev, "error adding host\n");
>> + goto err_add_host;
>> + }
>> +
>> + platform_set_drvdata(pdev, host);
>> +
>>
>> ...
>>
>> +static int __devexit sdhci_remove(struct platform_device *pdev)
>> +{
>> + struct sdhci_host *host = platform_get_drvdata(pdev);
>> + struct resource *iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + struct spear_sdhci *sdhci =
>> + (struct spear_sdhci *)(pdev->dev.platform_data);
>
> Again.
OK. Will be corrected everywhere.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
