ARM: s3c24xx: get rid of custom <mach/gpio.h>

This isolates the custom S3C24xx GPIO definition table to <linux/platform_data/gpio-samsung-s3x24xx.h> as this is used in a few different places in the kernel, removing the need to depend on the implicit inclusion of <mach/gpio.h> from <linux/gpio.h> and thus getting rid of a few nasty cross-dependencies.

We also delete the nifty CONFIG_S3C24XX_GPIO_EXTRA stuff. The biggest this can ever be for the S3C24XX is CONFIG_S3C24XX_GPIO_EXTRA = 128, and then for CPU_S3C2443 or CPU_S3C2416 32*12 GPIOs are added, so 32*12+128 = 512 is the absolute roof value on this platform. So we set the size of ARCH_NR_GPIO to this and the GPIOs array will fit any S3C24XX platform, as per pattern from other archs.

Cc Tomasz Figa <[email protected]>

Cc Sylwester Nawrocki <[email protected]>

Cc Ben Dooks <[email protected]>

Cc linux-samsung-soc at vger.kernel.org

Acked-by Kukjin Kim <[email protected]>

Acked-by Heiko Stuebner <[email protected]>

Tested-by Heiko Stuebner <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

Quoted 15 lines · Linus Walleij

This isolates the custom S3C24xx GPIO definition table to <linux/platform_data/gpio-samsung-s3x24xx.h> as this is used in a few different places in the kernel, removing the need to depend on the implicit inclusion of <mach/gpio.h> from <linux/gpio.h> and thus getting rid of a few nasty cross-dependencies.

We also delete the nifty CONFIG_S3C24XX_GPIO_EXTRA stuff. The biggest this can ever be for the S3C24XX is CONFIG_S3C24XX_GPIO_EXTRA = 128, and then for CPU_S3C2443 or CPU_S3C2416 32*12 GPIOs are added, so 32*12+128 = 512 is the absolute roof value on this platform. So we set the size of ARCH_NR_GPIO to this and the GPIOs array will fit any S3C24XX platform, as per pattern from other archs.

Quoted 8 lines

Cc Tomasz Figa <[email protected]>

Cc Sylwester Nawrocki <[email protected]>

Cc Ben Dooks <[email protected]>

Cc linux-samsung-soc at vger.kernel.org

Acked-by Kukjin Kim <[email protected]>

Acked-by Heiko Stuebner <[email protected]>

Tested-by Heiko Stuebner <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

I'm getting build errors in linux-next for s3c2410_defconfig now because of this patch :(

Two problems:

* Some files that use functions or macros defined in this file fail to include it. I see drivers/leds/leds-s3c24xx.c

and
mach-osiris-dvs.c,

but there might be more that don't get built
by default.

* The file includes <plat/gpio-cfg.h>, which is not a bug yet, but will be once we move s3c24xx to multiplatform, which would make it impossible to include this file from outside of arch/arm.

Note that on Exynos, the solution for the gpio driver dependencies was to scrap the driver and use pinctrl-exynos instead.

Quoted 5 lines · Linus Walleij

* The file includes <plat/gpio-cfg.h>, which is not a bug yet, but

will be once we move s3c24xx to multiplatform, which would make it impossible to include this file from outside of arch/arm.

Yes that needs to be a step for multiplatform enablement. My series only tries to make the problem smaller and remove the dependence on <mach/gpio.h>. All the <mach/*> and <plat/*> stuff needs to go away eventually ...

Quoted 2 lines

Note that on Exynos, the solution for the gpio driver dependencies was to scrap the driver and use pinctrl-exynos instead.

I think the S3C driver is a different piece of hardware unfortunately.

Quoted 1 line · Arnd Bergmann

On Friday 13 December 2013, Linus Walleij wrote:

Hm. I just compiled this defconfig on linux-next:

  AS      arch/arm/boot/compressed/lib1funcs.o
  AS      arch/arm/boot/compressed/ashldi3.o
  AS      arch/arm/boot/compressed/bswapsdi2.o
  CC      arch/arm/boot/compressed/decompress.o
  CC      arch/arm/boot/compressed/misc.o
  GZIP    arch/arm/boot/compressed/piggy.gzip
  AS      arch/arm/boot/compressed/piggy.gzip.o
  LD      arch/arm/boot/compressed/vmlinux

OBJCOPY arch/arm/boot/zImage
Kernel: arch/arm/boot/zImage is ready

It appears that these problems appear if you explicitly enable the DT board support, can't we just put that into the defconfig then, so we don't miss such things?

Quoted 4 lines

Two problems:

* Some files that use functions or macros defined in this file fail to include it. I see drivers/leds/leds-s3c24xx.c

I merged a patch fixing this by having that file explicitly include <plat/gpip-cfg.h> (it was already including it implicitly).

Quoted 2 lines

and
mach-osiris-dvs.c,

It needs this:
#include <linux/platform_data/gpio-samsung-s3c24xx.h>

(I have sent a patch.)

Quoted 2 lines

but there might be more that don't get built
by default.

Yeah implicit includes are a nightmare for refactoring... I'm trying to grep around for some symbols to see if I can find some lost cases.

Quoted 3 lines

* The file includes <plat/gpio-cfg.h>, which is not a bug yet, but will be once we move s3c24xx to multiplatform, which would make it impossible to include this file from outside of arch/arm.

Yes that needs to be a step for multiplatform enablement. My series only tries to make the problem smaller and remove the dependence on <mach/gpio.h>. All the <mach/*> and <plat/*> stuff needs to go away eventually ...

Quoted 2 lines

Note that on Exynos, the solution for the gpio driver dependencies was to scrap the driver and use pinctrl-exynos instead.

I think the S3C driver is a different piece of hardware unfortunately.

Quoted 13 lines · Linus Walleij

Quoted 1 line · Arnd Bergmann

On Friday 13 December 2013, Linus Walleij wrote:

Hm. I just compiled this defconfig on linux-next:

  AS      arch/arm/boot/compressed/lib1funcs.o
  AS      arch/arm/boot/compressed/ashldi3.o
  AS      arch/arm/boot/compressed/bswapsdi2.o
  CC      arch/arm/boot/compressed/decompress.o
  CC      arch/arm/boot/compressed/misc.o
  GZIP    arch/arm/boot/compressed/piggy.gzip
  AS      arch/arm/boot/compressed/piggy.gzip.o
  LD      arch/arm/boot/compressed/vmlinux

Quoted 6 lines

OBJCOPY arch/arm/boot/zImage
Kernel: arch/arm/boot/zImage is ready

It appears that these problems appear if you explicitly enable the DT board support, can't we just put that into the defconfig then, so we don't miss such things?

I don't understand. I didn't enable it manually and I still get it on linux-next-20130107. Maybe you were still on the older linux-next that had not been updated for some time?

Quoted 8 lines

Quoted 4 lines

Two problems:

* Some files that use functions or macros defined in this file fail to include it. I see drivers/leds/leds-s3c24xx.c

I merged a patch fixing this by having that file explicitly include <plat/gpip-cfg.h> (it was already including it implicitly).

Quoted 7 lines

Quoted 2 lines

and
mach-osiris-dvs.c,

It needs this:
#include <linux/platform_data/gpio-samsung-s3c24xx.h>

(I have sent a patch.)

Ok, thanks!

Quoted 6 lines

Quoted 2 lines

but there might be more that don't get built
by default.

Yeah implicit includes are a nightmare for refactoring... I'm trying to grep around for some symbols to see if I can find some lost cases.

I just tried building with "make allmodconfig KCONFIG_ALLCONFIG=allconfig", with the allconfig file containing a CONFIG_MACH_S3C2410=y statement. This caught a number of additionl problems, some related and some not.

Quoted 8 lines

Quoted 3 lines

* The file includes <plat/gpio-cfg.h>, which is not a bug yet, but will be once we move s3c24xx to multiplatform, which would make it impossible to include this file from outside of arch/arm.

Yes that needs to be a step for multiplatform enablement. My series only tries to make the problem smaller and remove the dependence on <mach/gpio.h>. All the <mach/*> and <plat/*> stuff needs to go away eventually ...

True. What is actually the bigger worry here is that the contents of the new file, while correctly moved out of mach/gpio.h also don't belong into include/linux/platform_data, because they don't define a pdata structure but rather the contents that are supposed to be passed from the platform code. I would much prefer if you could move the file back into mach-s3c24xx/ under a new name and keep it out of platform_data. I suspect that the only thing actually needed by the gpio driver is the number of GPIO lines per platform, and we can find another way to communicate that. Most of the contents should be private to the mach-s3c code and not be visible to either the gpio driver or any drivers using the plat/gpio-cfg.h interface.

Quoted 4 lines

Quoted 2 lines

Note that on Exynos, the solution for the gpio driver dependencies was to scrap the driver and use pinctrl-exynos instead.

I think the S3C driver is a different piece of hardware unfortunately.

But exynos was using this driver before it moved to the new pinctrl driver.

Quoted 3 lines · Linus Walleij

It appears that these problems appear if you explicitly enable the DT board support, can't we just put that into the defconfig then, so we don't miss such things?

I don't understand. I didn't enable it manually and I still get it on linux-next-20130107. Maybe you were still on the older linux-next that had not been updated for some time?

I just tried building with "make allmodconfig KCONFIG_ALLCONFIG=allconfig", with the allconfig file containing a CONFIG_MACH_S3C2410=y statement. This caught a number of additionl problems, some related and some not.

What is actually the bigger worry here is that the contents of the new file, while correctly moved out of mach/gpio.h also don't belong into include/linux/platform_data, because they don't define a pdata structure but rather the contents that are supposed to be passed from the platform code. I would much prefer if you could move the file back into mach-s3c24xx/ under a new name and keep it out of platform_data.

I suspect that the only thing actually needed by the gpio driver is the number of GPIO lines per platform, and we can find another way to communicate that.

Most of the contents should
be private to the mach-s3c code and not be visible to either the gpio driver or any drivers using the plat/gpio-cfg.h interface.

Quoted 4 lines

Quoted 2 lines

Note that on Exynos, the solution for the gpio driver dependencies was to scrap the driver and use pinctrl-exynos instead.

I think the S3C driver is a different piece of hardware unfortunately.

But exynos was using this driver before it moved to the new pinctrl driver.

Quoted 7 lines · Arnd Bergmann

What is actually the bigger worry here is that the contents of the new file, while correctly moved out of mach/gpio.h also don't belong into include/linux/platform_data, because they don't define a pdata structure but rather the contents that are supposed to be passed from the platform code. I would much prefer if you could move the file back into mach-s3c24xx/ under a new name and keep it out of platform_data.

Unfortunately it cannot live right under mach-s3c24xx because there are drivers here and there referring directly to the contents of this file.

The only quick-fix option would be to move it back to <mach/gpio-samsung-s3c24xx.h>
but the real solution is of course to augment all drivers to use gpio descriptors and add descriptor tables to the boardfiles.

I'm a bit reluctant to do that as I'd prefer to be able to test such modifications on HW ... plus time may be better invested in DT migration (as I think is the conclusion of this thread eventually).

Quoted 3 lines

I suspect that the only thing actually needed by the gpio driver is the number of GPIO lines per platform, and we can find another way to communicate that.

A bit more: if you look in drivers/gpio/gpio-samsung.c you see bank base GPIO offset for each bank into the global scope *and* the number of GPIOs for each bank propagated from machine headers instead of using platform data.

Again the proper solution (if the boardfiles are kept) is to switch to using a GPIO descriptor table. Or using DT.

Quoted 3 lines

Most of the contents should
be private to the mach-s3c code and not be visible to either the gpio driver or any drivers using the plat/gpio-cfg.h interface.

Samsungs <plat/gpio-cfg.h> is basically a custom legacy pin control implementation.

The real solution to getting rid of that file is to switch over to using pinctrl-[samsung|s3c24xx] which as Heiko describes mandates also using DT, and thus blocks attempts
at using this path for fixing the legacy boardfiles.

It's one of these situations where we end up with an all-or-nothing conversion path: either you change everything over to device tree or everything stays in two copies ... I'm trying to refactor the existing board files here maybe that is in vain :-/ as GPIO maintainer I want to get rid of <mach/gpio.h>.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Note that on Exynos, the solution for the gpio driver dependencies was to scrap the driver and use pinctrl-exynos instead.

I think the S3C driver is a different piece of hardware unfortunately.

But exynos was using this driver before it moved to the new pinctrl driver.

I was wrong about this, too much in my head. As Tomasz says, pinctrl-samsung can be used, but mandates that everything is moved over to device tree.

Probably the best thing now that I have one problem less is to leave it to the S3C maintainers to complete their DT migration?

Quoted 1 line · Linus Walleij

On Tue, Jan 7, 2014 at 8:52 PM, Arnd Bergmann <[email protected]> wrote:

Quoted 6 lines

I was wrong about this, too much in my head. As Tomasz says, pinctrl-samsung can be used, but mandates that everything is moved over to device tree.

Probably the best thing now that I have one problem less is to leave it to the S3C maintainers to complete their DT migration?

Let me have another look first, maybe I can find an intermediate step that helps you on your conquest to kill mach/gpio.h.

I've asked the mighty grep and it said that these are the files using the contents you moved around:

drivers/gpio/gpio-samsung.c
drivers/leds/leds-s3c24xx.c
drivers/mmc/host/s3cmci.c
sound/soc/samsung/h1940_uda1380.c
sound/soc/samsung/neo1973_wm8753.c
sound/soc/samsung/rx1950_uda1380.c
sound/soc/samsung/s3c2412-i2s.c
sound/soc/samsung/s3c24xx-i2s.c
sound/soc/samsung/smartq_wm8987.c

The symbols used in these drivers are:

S3C....GP.()
S3C_GPIO_END
S3C_GPIO_PULL_.*
S3C_GPIO_SFN()
s3c_gpio_cfgall_range()
s3c_gpio_setpull()

I think it would be worthwhile to go ahead and make all inclusions of the plat/gpio-cfg.h file explicit, and remove it from mach/gpio.h, that part should be simple enough and helpful at the same time.

Quoted 13 lines · Arnd Bergmann

I've asked the mighty grep and it said that these are the files using the contents you moved around:

drivers/gpio/gpio-samsung.c
drivers/leds/leds-s3c24xx.c
drivers/mmc/host/s3cmci.c
sound/soc/samsung/h1940_uda1380.c
sound/soc/samsung/neo1973_wm8753.c
sound/soc/samsung/rx1950_uda1380.c
sound/soc/samsung/s3c2412-i2s.c
sound/soc/samsung/s3c24xx-i2s.c
sound/soc/samsung/smartq_wm8987.c

Quoted 8 lines

The symbols used in these drivers are:

S3C....GP.()
S3C_GPIO_END
S3C_GPIO_PULL_.*
S3C_GPIO_SFN()
s3c_gpio_cfgall_range()
s3c_gpio_setpull()

Hm I will look closer at these. Maybe I can even fix some to just pass GPIO thru platform data and end-of-story them. Targeting v3.15.

But I guess you want me to move the <linux/platform_data/gpio-s3c*> back to <mach/*> before v3.14 so focusing on that now. (These patches cannot be just reverted as people have fixed other Samsung GPIO issues on top of them, so it needs to be done carefully.)

Quoted 3 lines

I think it would be worthwhile to go ahead and make all inclusions of the plat/gpio-cfg.h file explicit, and remove it from mach/gpio.h, that part should be simple enough and helpful at the same time.

<plat/gpio-cfg.h> is used by S5P as well, but I guess you've checked that.

If I take it driver-by-driver it should be possible to get rid of it or most of it I think, reducing it to the arch/arm/mach-* folder.

Certainly I should be attacking S3C and S5P simultaneously.

I think the S3C driver is a different piece of hardware unfortunately.