[PATCH] iio: adc: Nothing in ADC should be a bool CONFIG

[PATCH] iio: adc: Nothing in ADC should be a bool CONFIG

[Doug Anderson]

The whole IIO subsystem can be moved to a module.  If you make it a
module then stuff marked as "Y" in the adc directory simply won't be
linked in properly.

The two configs that were wrong were EXYNOS_ADC and LP8788_ADC.  I
know for a fact that EXYNOS_ADC will work as a module (though it
appears to crash when you unload it--that needs to be addressed
separately).  I assume LP8788_ADC will also be fine..

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/iio/adc/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index d86196c..24c28e3 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -106,7 +106,7 @@ config AT91_ADC
 	  Say yes here to build support for Atmel AT91 ADC.
 
 config EXYNOS_ADC
-	bool "Exynos ADC driver support"
+	tristate "Exynos ADC driver support"
 	depends on OF
 	help
 	  Core support for the ADC block found in the Samsung EXYNOS series
@@ -114,7 +114,7 @@ config EXYNOS_ADC
 	  this resource.
 
 config LP8788_ADC
-	bool "LP8788 ADC driver"
+	tristate "LP8788 ADC driver"
 	depends on MFD_LP8788
 	help
 	  Say yes here to build support for TI LP8788 ADC.
-- 
1.9.1.423.g4596e3a

Re: [PATCH] iio: adc: Nothing in ADC should be a bool CONFIG

[Jonathan Cameron]

On 22/04/14 01:03, Doug Anderson wrote:
> The whole IIO subsystem can be moved to a module.  If you make it a
> module then stuff marked as "Y" in the adc directory simply won't be
> linked in properly.
>
> The two configs that were wrong were EXYNOS_ADC and LP8788_ADC.  I
> know for a fact that EXYNOS_ADC will work as a module (though it
> appears to crash when you unload it--that needs to be addressed
> separately).
I'd really like to see this pinned down before taking this patch.
I can see you argument that the current approach is clearly wrong,
but swapping one issue for another is not an approach I'd particularly
like to take...

I can't immediately spot the cause of the crash, but there are certainly
some interesting order issues in this driver.  Not enabling the vdd
regulator until after the userspace interfaces are exposed (by the
iio_device_register call) is interesting for a start.

The remove doesn't run in the reverse of the probe order (see clocks
vs regulators for example.)

Gah, my reviewing for one clearly missed some things in this driver.

> I assume LP8788_ADC will also be fine..
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>   drivers/iio/adc/Kconfig | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index d86196c..24c28e3 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -106,7 +106,7 @@ config AT91_ADC
>   	  Say yes here to build support for Atmel AT91 ADC.
>
>   config EXYNOS_ADC
> -	bool "Exynos ADC driver support"
> +	tristate "Exynos ADC driver support"
>   	depends on OF
>   	help
>   	  Core support for the ADC block found in the Samsung EXYNOS series
> @@ -114,7 +114,7 @@ config EXYNOS_ADC
>   	  this resource.
>
>   config LP8788_ADC
> -	bool "LP8788 ADC driver"
> +	tristate "LP8788 ADC driver"
>   	depends on MFD_LP8788
>   	help
>   	  Say yes here to build support for TI LP8788 ADC.
>

Re: [PATCH] iio: adc: Nothing in ADC should be a bool CONFIG

[Doug Anderson]

Jonathan,

On Wed, Apr 23, 2014 at 1:54 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 22/04/14 01:03, Doug Anderson wrote:
>>
>> The whole IIO subsystem can be moved to a module.  If you make it a
>> module then stuff marked as "Y" in the adc directory simply won't be
>> linked in properly.
>>
>> The two configs that were wrong were EXYNOS_ADC and LP8788_ADC.  I
>> know for a fact that EXYNOS_ADC will work as a module (though it
>> appears to crash when you unload it--that needs to be addressed
>> separately).
>
> I'd really like to see this pinned down before taking this patch.
> I can see you argument that the current approach is clearly wrong,
> but swapping one issue for another is not an approach I'd particularly
> like to take...
>
> I can't immediately spot the cause of the crash, but there are certainly
> some interesting order issues in this driver.  Not enabling the vdd
> regulator until after the userspace interfaces are exposed (by the
> iio_device_register call) is interesting for a start.
>
> The remove doesn't run in the reverse of the probe order (see clocks
> vs regulators for example.)
>
> Gah, my reviewing for one clearly missed some things in this driver.

OK, fair enough.  I took a quick look and couldn't spot anything
either.  I've requested that Samsung dig into these problems.  If they
are unable to I will take a crack at it as time permits.  ;)

Thanks!

-Doug

Re: [PATCH] iio: adc: Nothing in ADC should be a bool CONFIG

[Doug Anderson]

Jonathan / Naveen,

On Wed, Apr 23, 2014 at 9:37 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 23/04/14 22:54, Doug Anderson wrote:
>>
>> Jonathan,
>>
>> On Wed, Apr 23, 2014 at 1:54 PM, Jonathan Cameron <jic23@kernel.org>
>> wrote:
>>>
>>> On 22/04/14 01:03, Doug Anderson wrote:
>>>>
>>>>
>>>> The whole IIO subsystem can be moved to a module.  If you make it a
>>>> module then stuff marked as "Y" in the adc directory simply won't be
>>>> linked in properly.
>>>>
>>>> The two configs that were wrong were EXYNOS_ADC and LP8788_ADC.  I
>>>> know for a fact that EXYNOS_ADC will work as a module (though it
>>>> appears to crash when you unload it--that needs to be addressed
>>>> separately).
>>>
>>>
>>> I'd really like to see this pinned down before taking this patch.
>>> I can see you argument that the current approach is clearly wrong,
>>> but swapping one issue for another is not an approach I'd particularly
>>> like to take...
>>>
>>> I can't immediately spot the cause of the crash, but there are certainly
>>> some interesting order issues in this driver.  Not enabling the vdd
>>> regulator until after the userspace interfaces are exposed (by the
>>> iio_device_register call) is interesting for a start.
>>>
>>> The remove doesn't run in the reverse of the probe order (see clocks
>>> vs regulators for example.)
>>>
>>> Gah, my reviewing for one clearly missed some things in this driver.
>>
>>
>> OK, fair enough.  I took a quick look and couldn't spot anything
>> either.  I've requested that Samsung dig into these problems.  If they
>> are unable to I will take a crack at it as time permits.  ;)
>>
> Cool and good luck (either way ;)

Ah ha!  Naveen pointed out that it was the "device_for_each_child"
that was the source of the crash.  ...and here's the problem...  I
think this is what we want:

-       ret = of_platform_populate(np, exynos_adc_match, NULL, &pdev->dev);
+       ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);

-       device_for_each_child(&pdev->dev, NULL,
+       device_for_each_child(&indio_dev->dev, NULL,

Naveen has been volunteered to send the patch up.  ;)  Naveen:
hopefully you can address Jonathan's other concerns as well.  ;)

-Doug

Re: [PATCH] iio: adc: Nothing in ADC should be a bool CONFIG

[Jonathan Cameron]

On 22/04/14 01:03, Doug Anderson wrote:
> The whole IIO subsystem can be moved to a module.  If you make it a
> module then stuff marked as "Y" in the adc directory simply won't be
> linked in properly.
>
> The two configs that were wrong were EXYNOS_ADC and LP8788_ADC.  I
> know for a fact that EXYNOS_ADC will work as a module (though it
> appears to crash when you unload it--that needs to be addressed
> separately).  I assume LP8788_ADC will also be fine..
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
Now the fix is in for the Exynos crash, I've applied this (with
some description editing to reflect that the crash isn't there
any more) to the fixes-togreg branch of iio.git.

We may get some functional breakage on Lp8788 if someone did
build this as a module as it provides some services to the
charger driver.  The right dependencies are there though so
may be fine and as you point out, it is definitely bad right now.

Thanks,

J
> ---
>   drivers/iio/adc/Kconfig | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index d86196c..24c28e3 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -106,7 +106,7 @@ config AT91_ADC
>   	  Say yes here to build support for Atmel AT91 ADC.
>
>   config EXYNOS_ADC
> -	bool "Exynos ADC driver support"
> +	tristate "Exynos ADC driver support"
>   	depends on OF
>   	help
>   	  Core support for the ADC block found in the Samsung EXYNOS series
> @@ -114,7 +114,7 @@ config EXYNOS_ADC
>   	  this resource.
>
>   config LP8788_ADC
> -	bool "LP8788 ADC driver"
> +	tristate "LP8788 ADC driver"
>   	depends on MFD_LP8788
>   	help
>   	  Say yes here to build support for TI LP8788 ADC.
>