Re: [PATCH v5 2/2] OMAP4 HSMMC: Adding card detect support for MMC1 Controller

[Adrian Hunter <adrian.hunter@nokia.com>]

kishore kadiyala wrote:
> On Tue, Jun 29, 2010 at 1:00 PM, Adrian Hunter <adrian.hunter@nokia.com> wrote:
>> ext kishore kadiyala wrote:
>>> Adrian ,
>>>
>>> Sorry for the late response
>>>
>>> <snip>
>>>> As per my email 5/5/10, I would suggest the only change to omap_hsmmc is:
>>> Agreed  and followed the changes mostly but made some more changes on top
>>> of it.
>>>
>>> <snip>
>>>> And that the late init function is used to do the rest e.g.
>>>> find a home for these 3 functions:
>>> I agree just having the 3 functions makes it work.
>>>
>>>> static int omap4_twl6030_hsmmc_late_init(struct device *dev)
>>>> {
>>>>       int ret = 0;
>>>>       struct platform_device *pdev = container_of(dev,
>>>>                                       struct platform_device, dev);
>>>>       struct omap_mmc_platform_data *pdata = dev->platform_data;
>>>>
>>>>       /* MMC1 Card detect Configuration */
>>>>       if (pdev->id == 0) {
>>>>               ret = omap4_hsmmc1_card_detect_config();
>>>>               if (ret < 0)
>>>>                       pr_err("Unable to configure Card detect for
>>>> MMC1\n");
>>>>               pdata->slots[0].card_detect = twl6030_mmc_card_detect;
>>>>               pdata->slots[0].card_detect_irq = TWL6030_IRQ_BASE +
>>>>                                                 MMCDETECT_INTR_OFFSET;
>>>>       }
>>>>       return ret;
>>> <snip>
>>>
>>> Few Comments below:
>>>
>>> 1) In the above function, initializing "card_detect"  in the driver as
>>> done in omap_hsmmc_gpio_init might be more readable and this has been
>>> done in nongpio_init instead.
>>> Even having initialization of  "card_detect_irq" inside nongpio_init is
>>> fine.
>> The problem is that referencing twl6030 from omap_hsmmc.c is not ok.
>> The driver must work with any platform and that is the reason that
>> platform data provides callbacks.
> 
> ok, in that case how about having handler initialized in
> mach-omap2/hsmmc.c  for both gpio and non-gpio case.

Unless twl6030 is part of OMAP4 then it doesn't belong in hsmmc.c either

> 
> -Kishore
>>> 2)Also calling omap_hsmmc_gpio_init in case of a card detect line
>>> which is not GPIO
>>> doesn't make sense though it assigns -EINVAL to switch_pin in case of
>>> invalid GPIO
>>> which is intended for a non-removable card .
>>>
>>> 3) And also having some thing like GPIO and NON_GPIO flag to
>>> distinguish might make sense.
>>>
>>> Regards,
>>> Kishore
>>>
>>
>