The corresponding CPP-block can never be selected since there are
conflicting Kconfig constraints:
- CONFIG_S3C24XX_DMAC has a dependency on ARCH_S3C24XX
- The surrounding CPP-block needs CONFIG_S3C64XX_DEV_SPI0 to be defined.
- CONFIG_S3C64XX_DEV_SPI0 is only selected by MACH_WLF_CRAGG_6410
- MACH_WLF_CRAGG_6410 however has a dependency on ARCH_S3C64XX
(through a surrounding if-statement in Kconfig)
- ARCH_S3C64XX and ARCH_S3C24XX are mutually exclusive since they are
declared in the same choice and cannot be enabled at the same time.
Hence, the innner block
"#elif defined(CONFIG_S3C24XX_DMAC)"
cannot be enabled at the same time with the surrounding block
"#ifdef CONFIG_S3C64XX_DEV_SPI0"
and therefore is dead.
This (logical) defect has been found with the undertaker tool
(https://undertaker.cs.fau.de)
Signed-off-by: Stefan Hengelein <[email protected]>
---
arch/arm/plat-samsung/devs.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/arm/plat-samsung/devs.c b/arch/arm/plat-samsung/devs.c
index 83c7d15..b38b601 100644
--- a/arch/arm/plat-samsung/devs.c
+++ b/arch/arm/plat-samsung/devs.c
@@ -1134,8 +1134,6 @@ void __init s3c64xx_spi0_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
pd.filter = pl330_filter;
#elif defined(CONFIG_S3C64XX_PL080)
pd.filter = pl08x_filter_id;
-#elif defined(CONFIG_S3C24XX_DMAC)
- pd.filter = s3c24xx_dma_filter;
#endif
s3c_set_platdata(&pd, sizeof(pd), &s3c64xx_device_spi0);
--
1.9.1
On Wednesday 17 December 2014 16:40:37 Stefan Hengelein wrote:
> The corresponding CPP-block can never be selected since there are
> conflicting Kconfig constraints:
> - CONFIG_S3C24XX_DMAC has a dependency on ARCH_S3C24XX
> - The surrounding CPP-block needs CONFIG_S3C64XX_DEV_SPI0 to be defined.
> - CONFIG_S3C64XX_DEV_SPI0 is only selected by MACH_WLF_CRAGG_6410
> - MACH_WLF_CRAGG_6410 however has a dependency on ARCH_S3C64XX
> (through a surrounding if-statement in Kconfig)
> - ARCH_S3C64XX and ARCH_S3C24XX are mutually exclusive since they are
> declared in the same choice and cannot be enabled at the same time.
>
> Hence, the innner block
> "#elif defined(CONFIG_S3C24XX_DMAC)"
> cannot be enabled at the same time with the surrounding block
> "#ifdef CONFIG_S3C64XX_DEV_SPI0"
> and therefore is dead.
>
> This (logical) defect has been found with the undertaker tool
> (https://undertaker.cs.fau.de)
Nice catch!
> Signed-off-by: Stefan Hengelein <[email protected]>
> ---
> arch/arm/plat-samsung/devs.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/arch/arm/plat-samsung/devs.c b/arch/arm/plat-samsung/devs.c
> index 83c7d15..b38b601 100644
> --- a/arch/arm/plat-samsung/devs.c
> +++ b/arch/arm/plat-samsung/devs.c
> @@ -1134,8 +1134,6 @@ void __init s3c64xx_spi0_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
> pd.filter = pl330_filter;
> #elif defined(CONFIG_S3C64XX_PL080)
> pd.filter = pl08x_filter_id;
> -#elif defined(CONFIG_S3C24XX_DMAC)
> - pd.filter = s3c24xx_dma_filter;
> #endif
>
> s3c_set_platdata(&pd, sizeof(pd), &s3c64xx_device_spi0);
>
This was introduced in 7f99ef2284b46f ("ARM: SAMSUNG: set
s3c24xx_dma_filter for s3c64xx-spi0 device"), but never used on s3c24xx as
far as I can tell. Heiko, can you comment on the patch? Did this
simply get obsoleted by the DT conversion of s3c2416 and s3c2443?
Arnd
Am Mittwoch, 17. Dezember 2014, 16:52:40 schrieb Arnd Bergmann:
> On Wednesday 17 December 2014 16:40:37 Stefan Hengelein wrote:
> > The corresponding CPP-block can never be selected since there are
> > conflicting Kconfig constraints:
> > - CONFIG_S3C24XX_DMAC has a dependency on ARCH_S3C24XX
> > - The surrounding CPP-block needs CONFIG_S3C64XX_DEV_SPI0 to be defined.
> >
> > - CONFIG_S3C64XX_DEV_SPI0 is only selected by MACH_WLF_CRAGG_6410
> > - MACH_WLF_CRAGG_6410 however has a dependency on ARCH_S3C64XX
> >
> > (through a surrounding if-statement in Kconfig)
> >
> > - ARCH_S3C64XX and ARCH_S3C24XX are mutually exclusive since they are
> >
> > declared in the same choice and cannot be enabled at the same time.
> >
> > Hence, the innner block
> >
> > "#elif defined(CONFIG_S3C24XX_DMAC)"
> >
> > cannot be enabled at the same time with the surrounding block
> >
> > "#ifdef CONFIG_S3C64XX_DEV_SPI0"
> >
> > and therefore is dead.
> >
> > This (logical) defect has been found with the undertaker tool
> > (https://undertaker.cs.fau.de)
>
> Nice catch!
>
> > Signed-off-by: Stefan Hengelein <[email protected]>
> > ---
> >
> > arch/arm/plat-samsung/devs.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/arch/arm/plat-samsung/devs.c b/arch/arm/plat-samsung/devs.c
> > index 83c7d15..b38b601 100644
> > --- a/arch/arm/plat-samsung/devs.c
> > +++ b/arch/arm/plat-samsung/devs.c
> > @@ -1134,8 +1134,6 @@ void __init s3c64xx_spi0_set_platdata(int
> > (*cfg_gpio)(void), int src_clk_nr,>
> > pd.filter = pl330_filter;
> >
> > #elif defined(CONFIG_S3C64XX_PL080)
> >
> > pd.filter = pl08x_filter_id;
> >
> > -#elif defined(CONFIG_S3C24XX_DMAC)
> > - pd.filter = s3c24xx_dma_filter;
> >
> > #endif
> >
> > s3c_set_platdata(&pd, sizeof(pd), &s3c64xx_device_spi0);
>
> This was introduced in 7f99ef2284b46f ("ARM: SAMSUNG: set
> s3c24xx_dma_filter for s3c64xx-spi0 device"), but never used on s3c24xx as
> far as I can tell. Heiko, can you comment on the patch? Did this
> simply get obsoleted by the DT conversion of s3c2416 and s3c2443?
We just have no in-tree users currently.
The S3C2416 and S3C2450 use the same type of spi controllers as the s3c64xx.
When writing the s3c24xx dma driver I also used this to test the driver. The
change was necessary to make the driver talk to my s3c2416 device, so it made
sense at the time.
As the s3c24xx-dma driver currently is still lacking dt support [burried
somewhere on my todo list], board files are also currently the only way to do
fast spi on those at all.
So removing this is dependent on how hard we want to make it for downstream
users [there seem to be a small number of those]. If this were part of
removing all non-dt cruft from the driver after s3c64xx migrated to be dt-only
I wouldn't object, but as it only affects the 2 lines of s3c24xx support,
personally I'd like to keep it around :-) .
Heiko
So you actually tested the code I removed in the patch? can you
provide a configuration that compiles that piece of code?
2014-12-17 17:16 GMT+01:00 Heiko Stübner <[email protected]>:
> Am Mittwoch, 17. Dezember 2014, 16:52:40 schrieb Arnd Bergmann:
>> On Wednesday 17 December 2014 16:40:37 Stefan Hengelein wrote:
>> > The corresponding CPP-block can never be selected since there are
>> > conflicting Kconfig constraints:
>> > - CONFIG_S3C24XX_DMAC has a dependency on ARCH_S3C24XX
>> > - The surrounding CPP-block needs CONFIG_S3C64XX_DEV_SPI0 to be defined.
>> >
>> > - CONFIG_S3C64XX_DEV_SPI0 is only selected by MACH_WLF_CRAGG_6410
>> > - MACH_WLF_CRAGG_6410 however has a dependency on ARCH_S3C64XX
>> >
>> > (through a surrounding if-statement in Kconfig)
>> >
>> > - ARCH_S3C64XX and ARCH_S3C24XX are mutually exclusive since they are
>> >
>> > declared in the same choice and cannot be enabled at the same time.
>> >
>> > Hence, the innner block
>> >
>> > "#elif defined(CONFIG_S3C24XX_DMAC)"
>> >
>> > cannot be enabled at the same time with the surrounding block
>> >
>> > "#ifdef CONFIG_S3C64XX_DEV_SPI0"
>> >
>> > and therefore is dead.
>> >
>> > This (logical) defect has been found with the undertaker tool
>> > (https://undertaker.cs.fau.de)
>>
>> Nice catch!
>>
>> > Signed-off-by: Stefan Hengelein <[email protected]>
>> > ---
>> >
>> > arch/arm/plat-samsung/devs.c | 2 --
>> > 1 file changed, 2 deletions(-)
>> >
>> > diff --git a/arch/arm/plat-samsung/devs.c b/arch/arm/plat-samsung/devs.c
>> > index 83c7d15..b38b601 100644
>> > --- a/arch/arm/plat-samsung/devs.c
>> > +++ b/arch/arm/plat-samsung/devs.c
>> > @@ -1134,8 +1134,6 @@ void __init s3c64xx_spi0_set_platdata(int
>> > (*cfg_gpio)(void), int src_clk_nr,>
>> > pd.filter = pl330_filter;
>> >
>> > #elif defined(CONFIG_S3C64XX_PL080)
>> >
>> > pd.filter = pl08x_filter_id;
>> >
>> > -#elif defined(CONFIG_S3C24XX_DMAC)
>> > - pd.filter = s3c24xx_dma_filter;
>> >
>> > #endif
>> >
>> > s3c_set_platdata(&pd, sizeof(pd), &s3c64xx_device_spi0);
>>
>> This was introduced in 7f99ef2284b46f ("ARM: SAMSUNG: set
>> s3c24xx_dma_filter for s3c64xx-spi0 device"), but never used on s3c24xx as
>> far as I can tell. Heiko, can you comment on the patch? Did this
>> simply get obsoleted by the DT conversion of s3c2416 and s3c2443?
>
> We just have no in-tree users currently.
>
> The S3C2416 and S3C2450 use the same type of spi controllers as the s3c64xx.
> When writing the s3c24xx dma driver I also used this to test the driver. The
> change was necessary to make the driver talk to my s3c2416 device, so it made
> sense at the time.
>
> As the s3c24xx-dma driver currently is still lacking dt support [burried
> somewhere on my todo list], board files are also currently the only way to do
> fast spi on those at all.
>
> So removing this is dependent on how hard we want to make it for downstream
> users [there seem to be a small number of those]. If this were part of
> removing all non-dt cruft from the driver after s3c64xx migrated to be dt-only
> I wouldn't object, but as it only affects the 2 lines of s3c24xx support,
> personally I'd like to keep it around :-) .
>
>
> Heiko
Hi Stefan,
Am Donnerstag, 18. Dezember 2014, 14:43:01 schrieb Stefan Hengelein:
> So you actually tested the code I removed in the patch? can you
> provide a configuration that compiles that piece of code?
Yep, one of my boards (Asus eeeReader DR-900) was actually able to transmit
stuff via the libertas spi wifi driver using the s3c64xx-spi driver.
The smdk2416 is currently the only board for the s3c2416 in the kernel. I don't
know if it had actual spi devices connected, but if it had, a suitable diff would
look something like the diff below. Of course hooking up spi devices would
work the same in any out-of-tree board for the supported socs.
For people reading along, the s3c24xx spi distribution is as follows:
s3c2410, s3c2412, s3c2440: (I think) two s3c24xx-spi
s3c2443: one s3c64xx-spi and one s3c24xx-spi
s3c2416: one s3c64xx-spi
s3c2450: two s3c64xx-spi
Heiko
------------ 8< ------------
diff --git a/arch/arm/mach-s3c24xx/Kconfig b/arch/arm/mach-s3c24xx/Kconfig
index 9eb2229..5210e5d 100644
--- a/arch/arm/mach-s3c24xx/Kconfig
+++ b/arch/arm/mach-s3c24xx/Kconfig
@@ -419,6 +419,8 @@ config MACH_SMDK2416
select S3C_DEV_HSMMC1
select S3C_DEV_NAND
select S3C_DEV_USB_HOST
+ select S3C64XX_DEV_SPI0
+ select S3C2443_SETUP_SPI
help
Say Y here if you are using an SMDK2416
diff --git a/arch/arm/mach-s3c24xx/mach-smdk2416.c b/arch/arm/mach-s3c24xx/mach-smdk2416.c
index 86394f7..b6c6ff4 100644
--- a/arch/arm/mach-s3c24xx/mach-smdk2416.c
+++ b/arch/arm/mach-s3c24xx/mach-smdk2416.c
@@ -52,6 +52,9 @@
#include <linux/platform_data/s3c-hsudc.h>
#include <plat/samsung-time.h>
+#include <linux/spi/spi.h>
+#include <linux/platform_data/spi-s3c64xx.h>
+
#include <plat/fb.h>
#include "common.h"
@@ -207,6 +210,26 @@ static struct s3c_sdhci_platdata smdk2416_hsmmc1_pdata __initdata = {
.cd_type = S3C_SDHCI_CD_NONE,
};
+struct s3c64xx_spi_csinfo libertas_cs_info = {
+ .fb_delay = 0,
+ .line = 128, /* gpio cs line */
+};
+
+
+static struct spi_board_info spi_board_info[] = {
+ {
+ .modalias = "libertas_spi",
+ .max_speed_hz = 1200000,
+ .bus_num = 0,
+ .irq = 12, /* some interrupt number */
+ .chip_select = 0,
+ .mode = SPI_MODE_3,
+ .controller_data = &libertas_cs_info,
+/* .platform_data = &foo1_pdata, */
+ },
+};
+
+
static struct platform_device *smdk2416_devices[] __initdata = {
&s3c_device_fb,
&s3c_device_wdt,
@@ -216,6 +239,7 @@ static struct platform_device *smdk2416_devices[] __initdata = {
&s3c_device_hsmmc1,
&s3c_device_usb_hsudc,
&s3c2443_device_dma,
+ &s3c64xx_device_spi0,
};
static void __init smdk2416_init_time(void)
@@ -250,6 +274,9 @@ static void __init smdk2416_machine_init(void)
gpio_request(S3C2410_GPB(1), "Display Reset");
gpio_direction_output(S3C2410_GPB(1), 1);
+ s3c64xx_spi0_set_platdata(NULL, 0, ARRAY_SIZE(spi_board_info));
+ spi_register_board_info(spi_board_info, ARRAY_SIZE(spi_board_info));
+
platform_add_devices(smdk2416_devices, ARRAY_SIZE(smdk2416_devices));
smdk_machine_init();
}
diff --git a/arch/arm/mach-s3c24xx/setup-spi.c b/arch/arm/mach-s3c24xx/setup-spi.c
index 3d47e02..d66c4e0 100644
--- a/arch/arm/mach-s3c24xx/setup-spi.c
+++ b/arch/arm/mach-s3c24xx/setup-spi.c
@@ -16,6 +16,7 @@
#include <mach/hardware.h>
#include <mach/regs-gpio.h>
+#include <mach/gpio-samsung.h>
#ifdef CONFIG_S3C64XX_DEV_SPI0
int s3c64xx_spi0_cfg_gpio(void)
>From what i can see, the block was already dead when it was introduced.
d2193ce2 changed the "if ARCH_S3C64XX" into the Kconfig file itself,
before it was around the source statement in arch/arm/Kconfig
if there are really just downstream users that explicitly have to add
a statement to select S3C64XX_DEV_SPI0 and therefore add the
possibility to enable the block i want to remove, i'd argue that these
downstream users could also add the block itself. I'm not sure how
intuitive it might be for downstream users to add a select in Kconfig
to enable their machine to communicate with a device, but i'm also not
familiar with the hardware we're talking about.
However, i'd prefer to have a consistent upstream state and leave
these problems to downstream users, but that's for the Maintainer to
decide :)
2014-12-18 20:03 GMT+01:00 Heiko Stübner <[email protected]>:
> Hi Stefan,
>
> Am Donnerstag, 18. Dezember 2014, 14:43:01 schrieb Stefan Hengelein:
>> So you actually tested the code I removed in the patch? can you
>> provide a configuration that compiles that piece of code?
>
> Yep, one of my boards (Asus eeeReader DR-900) was actually able to transmit
> stuff via the libertas spi wifi driver using the s3c64xx-spi driver.
>
> The smdk2416 is currently the only board for the s3c2416 in the kernel. I don't
> know if it had actual spi devices connected, but if it had, a suitable diff would
> look something like the diff below. Of course hooking up spi devices would
> work the same in any out-of-tree board for the supported socs.
>
>
> For people reading along, the s3c24xx spi distribution is as follows:
> s3c2410, s3c2412, s3c2440: (I think) two s3c24xx-spi
> s3c2443: one s3c64xx-spi and one s3c24xx-spi
> s3c2416: one s3c64xx-spi
> s3c2450: two s3c64xx-spi
>
>
> Heiko
>
>
> ------------ 8< ------------
> diff --git a/arch/arm/mach-s3c24xx/Kconfig b/arch/arm/mach-s3c24xx/Kconfig
> index 9eb2229..5210e5d 100644
> --- a/arch/arm/mach-s3c24xx/Kconfig
> +++ b/arch/arm/mach-s3c24xx/Kconfig
> @@ -419,6 +419,8 @@ config MACH_SMDK2416
> select S3C_DEV_HSMMC1
> select S3C_DEV_NAND
> select S3C_DEV_USB_HOST
> + select S3C64XX_DEV_SPI0
> + select S3C2443_SETUP_SPI
> help
> Say Y here if you are using an SMDK2416
>
> diff --git a/arch/arm/mach-s3c24xx/mach-smdk2416.c b/arch/arm/mach-s3c24xx/mach-smdk2416.c
> index 86394f7..b6c6ff4 100644
> --- a/arch/arm/mach-s3c24xx/mach-smdk2416.c
> +++ b/arch/arm/mach-s3c24xx/mach-smdk2416.c
> @@ -52,6 +52,9 @@
> #include <linux/platform_data/s3c-hsudc.h>
> #include <plat/samsung-time.h>
>
> +#include <linux/spi/spi.h>
> +#include <linux/platform_data/spi-s3c64xx.h>
> +
> #include <plat/fb.h>
>
> #include "common.h"
> @@ -207,6 +210,26 @@ static struct s3c_sdhci_platdata smdk2416_hsmmc1_pdata __initdata = {
> .cd_type = S3C_SDHCI_CD_NONE,
> };
>
> +struct s3c64xx_spi_csinfo libertas_cs_info = {
> + .fb_delay = 0,
> + .line = 128, /* gpio cs line */
> +};
> +
> +
> +static struct spi_board_info spi_board_info[] = {
> + {
> + .modalias = "libertas_spi",
> + .max_speed_hz = 1200000,
> + .bus_num = 0,
> + .irq = 12, /* some interrupt number */
> + .chip_select = 0,
> + .mode = SPI_MODE_3,
> + .controller_data = &libertas_cs_info,
> +/* .platform_data = &foo1_pdata, */
> + },
> +};
> +
> +
> static struct platform_device *smdk2416_devices[] __initdata = {
> &s3c_device_fb,
> &s3c_device_wdt,
> @@ -216,6 +239,7 @@ static struct platform_device *smdk2416_devices[] __initdata = {
> &s3c_device_hsmmc1,
> &s3c_device_usb_hsudc,
> &s3c2443_device_dma,
> + &s3c64xx_device_spi0,
> };
>
> static void __init smdk2416_init_time(void)
> @@ -250,6 +274,9 @@ static void __init smdk2416_machine_init(void)
> gpio_request(S3C2410_GPB(1), "Display Reset");
> gpio_direction_output(S3C2410_GPB(1), 1);
>
> + s3c64xx_spi0_set_platdata(NULL, 0, ARRAY_SIZE(spi_board_info));
> + spi_register_board_info(spi_board_info, ARRAY_SIZE(spi_board_info));
> +
> platform_add_devices(smdk2416_devices, ARRAY_SIZE(smdk2416_devices));
> smdk_machine_init();
> }
> diff --git a/arch/arm/mach-s3c24xx/setup-spi.c b/arch/arm/mach-s3c24xx/setup-spi.c
> index 3d47e02..d66c4e0 100644
> --- a/arch/arm/mach-s3c24xx/setup-spi.c
> +++ b/arch/arm/mach-s3c24xx/setup-spi.c
> @@ -16,6 +16,7 @@
>
> #include <mach/hardware.h>
> #include <mach/regs-gpio.h>
> +#include <mach/gpio-samsung.h>
>
> #ifdef CONFIG_S3C64XX_DEV_SPI0
> int s3c64xx_spi0_cfg_gpio(void)
>
On Friday 19 December 2014 15:15:09 Stefan Hengelein wrote:
> From what i can see, the block was already dead when it was introduced.
> d2193ce2 changed the "if ARCH_S3C64XX" into the Kconfig file itself,
> before it was around the source statement in arch/arm/Kconfig
>
> if there are really just downstream users that explicitly have to add
> a statement to select S3C64XX_DEV_SPI0 and therefore add the
> possibility to enable the block i want to remove, i'd argue that these
> downstream users could also add the block itself. I'm not sure how
> intuitive it might be for downstream users to add a select in Kconfig
> to enable their machine to communicate with a device, but i'm also not
> familiar with the hardware we're talking about.
>
> However, i'd prefer to have a consistent upstream state and leave
> these problems to downstream users, but that's for the Maintainer to
> decide
In general, I totally agree: dead code should be eliminated and out of
tree users of dead code can add it back as a patch.
However, in this case, I'd lean more towards leaving the code in there,
basically because the current code correctly documents what the hardware
requirements are, and that is helpful even for reading the code when you
work on DT based support for the same hardware. Eventually we will be
able to remove the entire function.
Arnd