[PATCH] mmc: Kconfig: Add dependency on GPIOLIB for MMC_SDHCI

[PATCH] mmc: Kconfig: Add dependency on GPIOLIB for MMC_SDHCI

[Michal Simek]

Add GPIOLIB dependency for MMC_SDHCI.

Problem was observed after adding the patch
"mmc: sdhci-of-arasan: Call OF parsing for MMC"
(sha1: 16b23787fc709fe60c5d2bd05927b1a3da33d4e9) which calls
mmc_of_parse() -> mmc_gpiod_request_cd() (slot-gpio.c) which
calls devm_gpiod_get_index() which returns -ENOSYS.

Error log:
sdhci-arasan ff160000.sdhci: parsing dt failed (4294967258)
sdhci-arasan: probe of ff160000.sdhci failed with error -38

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

After RFC here https://lkml.org/lkml/2015/7/24/371
adding dependency on MMC_SDHCI.

---
 drivers/mmc/host/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index fd9a58e216a5..5519803c124b 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -38,6 +38,7 @@ config MMC_PXA
 config MMC_SDHCI
 	tristate "Secure Digital Host Controller Interface support"
 	depends on HAS_DMA
+	depends on GPIOLIB
 	help
 	  This selects the generic Secure Digital Host Controller Interface.
 	  It is used by manufacturers such as Texas Instruments(R), Ricoh(R)
-- 
2.3.5

Re: [PATCH] mmc: Kconfig: Add dependency on GPIOLIB for MMC_SDHCI

[Ulf Hansson]

On 6 August 2015 at 07:39, Michal Simek <michal.simek@xilinx.com> wrote:
> Add GPIOLIB dependency for MMC_SDHCI.
>
> Problem was observed after adding the patch
> "mmc: sdhci-of-arasan: Call OF parsing for MMC"
> (sha1: 16b23787fc709fe60c5d2bd05927b1a3da33d4e9) which calls
> mmc_of_parse() -> mmc_gpiod_request_cd() (slot-gpio.c) which
> calls devm_gpiod_get_index() which returns -ENOSYS.
>
> Error log:
> sdhci-arasan ff160000.sdhci: parsing dt failed (4294967258)
> sdhci-arasan: probe of ff160000.sdhci failed with error -38
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

Thanks, applied for next!

Kind regards
Uffe

> ---
>
> After RFC here https://lkml.org/lkml/2015/7/24/371
> adding dependency on MMC_SDHCI.
>
> ---
>  drivers/mmc/host/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index fd9a58e216a5..5519803c124b 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -38,6 +38,7 @@ config MMC_PXA
>  config MMC_SDHCI
>         tristate "Secure Digital Host Controller Interface support"
>         depends on HAS_DMA
> +       depends on GPIOLIB
>         help
>           This selects the generic Secure Digital Host Controller Interface.
>           It is used by manufacturers such as Texas Instruments(R), Ricoh(R)
> --
> 2.3.5
>

Re: [PATCH] mmc: Kconfig: Add dependency on GPIOLIB for MMC_SDHCI

[Ulf Hansson]

On 25 August 2015 at 14:04, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 6 August 2015 at 07:39, Michal Simek <michal.simek@xilinx.com> wrote:
>> Add GPIOLIB dependency for MMC_SDHCI.
>>
>> Problem was observed after adding the patch
>> "mmc: sdhci-of-arasan: Call OF parsing for MMC"
>> (sha1: 16b23787fc709fe60c5d2bd05927b1a3da33d4e9) which calls
>> mmc_of_parse() -> mmc_gpiod_request_cd() (slot-gpio.c) which
>> calls devm_gpiod_get_index() which returns -ENOSYS.
>>
>> Error log:
>> sdhci-arasan ff160000.sdhci: parsing dt failed (4294967258)
>> sdhci-arasan: probe of ff160000.sdhci failed with error -38
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>
> Thanks, applied for next!

kbuild test robot reports a warning for this one, so I am dropping it
from my next branch.

-----
warning: (AKEBONO) selects MMC_SDHCI which has unmet direct
dependencies (MMC && HAS_DMA && GPIOLIB)
https://lists.01.org/pipermail/kbuild-all/2015-August/011824.html
-----

Perhaps we should just live with this kind of configuration problems
as this patch is trying to fix?
Looking into drivers in general I don't think we normally specify all
the Kconfig dependencies to be able to meet a successfully probe().
The GPIOLIB is just one thing out of many.

Or you have a better idea? :-)

Kind regards
Uffe

Re: [PATCH] mmc: Kconfig: Add dependency on GPIOLIB for MMC_SDHCI

[Michal Simek]

Hi Ulf,

On 08/27/2015 01:32 PM, Ulf Hansson wrote:
> On 25 August 2015 at 14:04, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 6 August 2015 at 07:39, Michal Simek <michal.simek@xilinx.com> wrote:
>>> Add GPIOLIB dependency for MMC_SDHCI.
>>>
>>> Problem was observed after adding the patch
>>> "mmc: sdhci-of-arasan: Call OF parsing for MMC"
>>> (sha1: 16b23787fc709fe60c5d2bd05927b1a3da33d4e9) which calls
>>> mmc_of_parse() -> mmc_gpiod_request_cd() (slot-gpio.c) which
>>> calls devm_gpiod_get_index() which returns -ENOSYS.
>>>
>>> Error log:
>>> sdhci-arasan ff160000.sdhci: parsing dt failed (4294967258)
>>> sdhci-arasan: probe of ff160000.sdhci failed with error -38
>>>
>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>
>> Thanks, applied for next!
> 
> kbuild test robot reports a warning for this one, so I am dropping it
> from my next branch.

I think is just better to fix the problem there instead of dropping this
patch which fix GPIO dependency.

Fix is quite easy.
diff --git a/arch/powerpc/platforms/44x/Kconfig
b/arch/powerpc/platforms/44x/Kconfig
index 5538e57c36c1..874f07c7d0b8 100644
--- a/arch/powerpc/platforms/44x/Kconfig
+++ b/arch/powerpc/platforms/44x/Kconfig
@@ -219,6 +219,7 @@ config AKEBONO
        select USB_EHCI_HCD_PLATFORM if USB_EHCI_HCD
        select MMC_SDHCI
        select MMC_SDHCI_PLTFM
+       select GPIOLIB
        select ATA
        select SATA_AHCI_PLATFORM
        help

But the question is if we should keep these ancient targets in the tree.

I am happy to send this patch but it should go via PPC tree. Or are you
happy to apply it to your tree?

Thanks,
Michal

Re: [PATCH] mmc: Kconfig: Add dependency on GPIOLIB for MMC_SDHCI

[Ulf Hansson]

On 27 August 2015 at 13:43, Michal Simek <michal.simek@xilinx.com> wrote:
> Hi Ulf,
>
> On 08/27/2015 01:32 PM, Ulf Hansson wrote:
>> On 25 August 2015 at 14:04, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 6 August 2015 at 07:39, Michal Simek <michal.simek@xilinx.com> wrote:
>>>> Add GPIOLIB dependency for MMC_SDHCI.
>>>>
>>>> Problem was observed after adding the patch
>>>> "mmc: sdhci-of-arasan: Call OF parsing for MMC"
>>>> (sha1: 16b23787fc709fe60c5d2bd05927b1a3da33d4e9) which calls
>>>> mmc_of_parse() -> mmc_gpiod_request_cd() (slot-gpio.c) which
>>>> calls devm_gpiod_get_index() which returns -ENOSYS.
>>>>
>>>> Error log:
>>>> sdhci-arasan ff160000.sdhci: parsing dt failed (4294967258)
>>>> sdhci-arasan: probe of ff160000.sdhci failed with error -38
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>
>>> Thanks, applied for next!
>>
>> kbuild test robot reports a warning for this one, so I am dropping it
>> from my next branch.
>
> I think is just better to fix the problem there instead of dropping this
> patch which fix GPIO dependency.
>
> Fix is quite easy.
> diff --git a/arch/powerpc/platforms/44x/Kconfig
> b/arch/powerpc/platforms/44x/Kconfig
> index 5538e57c36c1..874f07c7d0b8 100644
> --- a/arch/powerpc/platforms/44x/Kconfig
> +++ b/arch/powerpc/platforms/44x/Kconfig
> @@ -219,6 +219,7 @@ config AKEBONO
>         select USB_EHCI_HCD_PLATFORM if USB_EHCI_HCD
>         select MMC_SDHCI
>         select MMC_SDHCI_PLTFM
> +       select GPIOLIB
>         select ATA
>         select SATA_AHCI_PLATFORM
>         help
>
> But the question is if we should keep these ancient targets in the tree.
>
> I am happy to send this patch but it should go via PPC tree. Or are you
> happy to apply it to your tree?

It's getting really late for 4.3 so I would rather postpone this to
the next release cycle.

As I stated in my earlier reply, do we really want to add the GPIOLIB
dependency to the Kconfig file for SDHCI?
I assume we have lots of other Kconfig dependencies, then these should
also to be added for the same reasons. I doubt this is the right thing
to do.

How about if the mmc core instead treat GPIOs as optional from an API
point of view and thus it won't cause ->probe() to fail. Is that a way
forward for you?

Kind regards
Uffe

Re: [PATCH] mmc: Kconfig: Add dependency on GPIOLIB for MMC_SDHCI

[Michal Simek]

On 08/27/2015 02:30 PM, Ulf Hansson wrote:
> On 27 August 2015 at 13:43, Michal Simek <michal.simek@xilinx.com> wrote:
>> Hi Ulf,
>>
>> On 08/27/2015 01:32 PM, Ulf Hansson wrote:
>>> On 25 August 2015 at 14:04, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 6 August 2015 at 07:39, Michal Simek <michal.simek@xilinx.com> wrote:
>>>>> Add GPIOLIB dependency for MMC_SDHCI.
>>>>>
>>>>> Problem was observed after adding the patch
>>>>> "mmc: sdhci-of-arasan: Call OF parsing for MMC"
>>>>> (sha1: 16b23787fc709fe60c5d2bd05927b1a3da33d4e9) which calls
>>>>> mmc_of_parse() -> mmc_gpiod_request_cd() (slot-gpio.c) which
>>>>> calls devm_gpiod_get_index() which returns -ENOSYS.
>>>>>
>>>>> Error log:
>>>>> sdhci-arasan ff160000.sdhci: parsing dt failed (4294967258)
>>>>> sdhci-arasan: probe of ff160000.sdhci failed with error -38
>>>>>
>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>
>>>> Thanks, applied for next!
>>>
>>> kbuild test robot reports a warning for this one, so I am dropping it
>>> from my next branch.
>>
>> I think is just better to fix the problem there instead of dropping this
>> patch which fix GPIO dependency.
>>
>> Fix is quite easy.
>> diff --git a/arch/powerpc/platforms/44x/Kconfig
>> b/arch/powerpc/platforms/44x/Kconfig
>> index 5538e57c36c1..874f07c7d0b8 100644
>> --- a/arch/powerpc/platforms/44x/Kconfig
>> +++ b/arch/powerpc/platforms/44x/Kconfig
>> @@ -219,6 +219,7 @@ config AKEBONO
>>         select USB_EHCI_HCD_PLATFORM if USB_EHCI_HCD
>>         select MMC_SDHCI
>>         select MMC_SDHCI_PLTFM
>> +       select GPIOLIB
>>         select ATA
>>         select SATA_AHCI_PLATFORM
>>         help
>>
>> But the question is if we should keep these ancient targets in the tree.
>>
>> I am happy to send this patch but it should go via PPC tree. Or are you
>> happy to apply it to your tree?
> 
> It's getting really late for 4.3 so I would rather postpone this to
> the next release cycle.

No problem at all. :-)

> 
> As I stated in my earlier reply, do we really want to add the GPIOLIB
> dependency to the Kconfig file for SDHCI?
> I assume we have lots of other Kconfig dependencies, then these should
> also to be added for the same reasons. I doubt this is the right thing
> to do.

Is it the right solution not to list them if they are there?

> How about if the mmc core instead treat GPIOs as optional from an API
> point of view and thus it won't cause ->probe() to fail. Is that a way
> forward for you?

In my test case I am not using GPIO at all and probe is failing. If this
is fixed because it is probably common setting for others we don't need
to list this dependency.
But really for my case and I am not using gpio at all probe just failed
which is incorrect and should be fixed.

Thanks,
Michal

Re: [PATCH] mmc: Kconfig: Add dependency on GPIOLIB for MMC_SDHCI

[Ulf Hansson]

On 27 August 2015 at 15:43, Michal Simek <michal.simek@xilinx.com> wrote:
> On 08/27/2015 02:30 PM, Ulf Hansson wrote:
>> On 27 August 2015 at 13:43, Michal Simek <michal.simek@xilinx.com> wrote:
>>> Hi Ulf,
>>>
>>> On 08/27/2015 01:32 PM, Ulf Hansson wrote:
>>>> On 25 August 2015 at 14:04, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> On 6 August 2015 at 07:39, Michal Simek <michal.simek@xilinx.com> wrote:
>>>>>> Add GPIOLIB dependency for MMC_SDHCI.
>>>>>>
>>>>>> Problem was observed after adding the patch
>>>>>> "mmc: sdhci-of-arasan: Call OF parsing for MMC"
>>>>>> (sha1: 16b23787fc709fe60c5d2bd05927b1a3da33d4e9) which calls
>>>>>> mmc_of_parse() -> mmc_gpiod_request_cd() (slot-gpio.c) which
>>>>>> calls devm_gpiod_get_index() which returns -ENOSYS.
>>>>>>
>>>>>> Error log:
>>>>>> sdhci-arasan ff160000.sdhci: parsing dt failed (4294967258)
>>>>>> sdhci-arasan: probe of ff160000.sdhci failed with error -38
>>>>>>
>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>
>>>>> Thanks, applied for next!
>>>>
>>>> kbuild test robot reports a warning for this one, so I am dropping it
>>>> from my next branch.
>>>
>>> I think is just better to fix the problem there instead of dropping this
>>> patch which fix GPIO dependency.
>>>
>>> Fix is quite easy.
>>> diff --git a/arch/powerpc/platforms/44x/Kconfig
>>> b/arch/powerpc/platforms/44x/Kconfig
>>> index 5538e57c36c1..874f07c7d0b8 100644
>>> --- a/arch/powerpc/platforms/44x/Kconfig
>>> +++ b/arch/powerpc/platforms/44x/Kconfig
>>> @@ -219,6 +219,7 @@ config AKEBONO
>>>         select USB_EHCI_HCD_PLATFORM if USB_EHCI_HCD
>>>         select MMC_SDHCI
>>>         select MMC_SDHCI_PLTFM
>>> +       select GPIOLIB
>>>         select ATA
>>>         select SATA_AHCI_PLATFORM
>>>         help
>>>
>>> But the question is if we should keep these ancient targets in the tree.
>>>
>>> I am happy to send this patch but it should go via PPC tree. Or are you
>>> happy to apply it to your tree?
>>
>> It's getting really late for 4.3 so I would rather postpone this to
>> the next release cycle.
>
> No problem at all. :-)
>
>>
>> As I stated in my earlier reply, do we really want to add the GPIOLIB
>> dependency to the Kconfig file for SDHCI?
>> I assume we have lots of other Kconfig dependencies, then these should
>> also to be added for the same reasons. I doubt this is the right thing
>> to do.
>
> Is it the right solution not to list them if they are there?

Well, I don't think there are *one* answer to this.

Still, the reason to why we have API implementing stubs when used is
to manage these cases.

>
>> How about if the mmc core instead treat GPIOs as optional from an API
>> point of view and thus it won't cause ->probe() to fail. Is that a way
>> forward for you?
>
> In my test case I am not using GPIO at all and probe is failing. If this
> is fixed because it is probably common setting for others we don't need
> to list this dependency.
> But really for my case and I am not using gpio at all probe just failed
> which is incorrect and should be fixed.
>

Thanks for clarifying. In this regard I think it's pretty obvious that
we need to make GPIO optional.
Else we will force the footprint to increase for the kernel image,
even when not needed.

Typically checking for -ENOSYS from the response from the GPIOLIB
would do the trick. That's actually what we do already for GPIOs in
mmc pwrseq simple case.

Do you want to send another patch?

Kind regards
Uffe

Re: [PATCH] mmc: Kconfig: Add dependency on GPIOLIB for MMC_SDHCI

[Michal Simek]

On 08/27/2015 04:12 PM, Ulf Hansson wrote:
> On 27 August 2015 at 15:43, Michal Simek <michal.simek@xilinx.com> wrote:
>> On 08/27/2015 02:30 PM, Ulf Hansson wrote:
>>> On 27 August 2015 at 13:43, Michal Simek <michal.simek@xilinx.com> wrote:
>>>> Hi Ulf,
>>>>
>>>> On 08/27/2015 01:32 PM, Ulf Hansson wrote:
>>>>> On 25 August 2015 at 14:04, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>> On 6 August 2015 at 07:39, Michal Simek <michal.simek@xilinx.com> wrote:
>>>>>>> Add GPIOLIB dependency for MMC_SDHCI.
>>>>>>>
>>>>>>> Problem was observed after adding the patch
>>>>>>> "mmc: sdhci-of-arasan: Call OF parsing for MMC"
>>>>>>> (sha1: 16b23787fc709fe60c5d2bd05927b1a3da33d4e9) which calls
>>>>>>> mmc_of_parse() -> mmc_gpiod_request_cd() (slot-gpio.c) which
>>>>>>> calls devm_gpiod_get_index() which returns -ENOSYS.
>>>>>>>
>>>>>>> Error log:
>>>>>>> sdhci-arasan ff160000.sdhci: parsing dt failed (4294967258)
>>>>>>> sdhci-arasan: probe of ff160000.sdhci failed with error -38
>>>>>>>
>>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>>
>>>>>> Thanks, applied for next!
>>>>>
>>>>> kbuild test robot reports a warning for this one, so I am dropping it
>>>>> from my next branch.
>>>>
>>>> I think is just better to fix the problem there instead of dropping this
>>>> patch which fix GPIO dependency.
>>>>
>>>> Fix is quite easy.
>>>> diff --git a/arch/powerpc/platforms/44x/Kconfig
>>>> b/arch/powerpc/platforms/44x/Kconfig
>>>> index 5538e57c36c1..874f07c7d0b8 100644
>>>> --- a/arch/powerpc/platforms/44x/Kconfig
>>>> +++ b/arch/powerpc/platforms/44x/Kconfig
>>>> @@ -219,6 +219,7 @@ config AKEBONO
>>>>         select USB_EHCI_HCD_PLATFORM if USB_EHCI_HCD
>>>>         select MMC_SDHCI
>>>>         select MMC_SDHCI_PLTFM
>>>> +       select GPIOLIB
>>>>         select ATA
>>>>         select SATA_AHCI_PLATFORM
>>>>         help
>>>>
>>>> But the question is if we should keep these ancient targets in the tree.
>>>>
>>>> I am happy to send this patch but it should go via PPC tree. Or are you
>>>> happy to apply it to your tree?
>>>
>>> It's getting really late for 4.3 so I would rather postpone this to
>>> the next release cycle.
>>
>> No problem at all. :-)
>>
>>>
>>> As I stated in my earlier reply, do we really want to add the GPIOLIB
>>> dependency to the Kconfig file for SDHCI?
>>> I assume we have lots of other Kconfig dependencies, then these should
>>> also to be added for the same reasons. I doubt this is the right thing
>>> to do.
>>
>> Is it the right solution not to list them if they are there?
> 
> Well, I don't think there are *one* answer to this.
> 
> Still, the reason to why we have API implementing stubs when used is
> to manage these cases.
> 
>>
>>> How about if the mmc core instead treat GPIOs as optional from an API
>>> point of view and thus it won't cause ->probe() to fail. Is that a way
>>> forward for you?
>>
>> In my test case I am not using GPIO at all and probe is failing. If this
>> is fixed because it is probably common setting for others we don't need
>> to list this dependency.
>> But really for my case and I am not using gpio at all probe just failed
>> which is incorrect and should be fixed.
>>
> 
> Thanks for clarifying. In this regard I think it's pretty obvious that
> we need to make GPIO optional.
> Else we will force the footprint to increase for the kernel image,
> even when not needed.

right. Footprint is valid argument.

> 
> Typically checking for -ENOSYS from the response from the GPIOLIB
> would do the trick. That's actually what we do already for GPIOs in
> mmc pwrseq simple case.

I didn't parse the gpio code to be 100% sure that this is enough. Also
if this will cover all cases which we can have. Depends on gpio error
code consistency.

> Do you want to send another patch?

I would love to but I am quite occupied right now but I am happy to test
on our SoC when we have right fix for it.

Thanks,
Michal