Skip to contentSkip to conversation

spi: s3c drivers shouldn't care about spi_board_info

PATCH In review David Brownell 3 participants 5 messages Jan 22, 2008 – Feb 21, 2008 3 messages not in the archive

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 spi: s3c drivers shouldn't care about spi_board_info

The two S3C SPI master drivers got merged without much review, so I just noticed that they're doing something that the SPI core code is responsible for, rather than any adapter driver: they try to register SPI devices.

This removes that support from those drivers so they act normally. Interestingly, none of the current boards are affected. So it's a net code shrink with no loss of functionality.

---
I did a test build with the s3c defconfig. The only problem was with a missing #define breaking the a.out support.

 drivers/spi/spi_s3c24xx.c      |   12 ------------
 drivers/spi/spi_s3c24xx_gpio.c |   12 ------------
 include/asm/arch/spi-gpio.h    |    6 ------
 include/asm/arch/spi.h         |    6 ------
 4 files changed, 36 deletions(-)
drivers/spi/spi_s3c24xx.c +0 -12
@@ -237,10 +237,8 @@ static int __init s3c24xx_spi_probe(stru
 {
 	struct s3c24xx_spi *hw;
 	struct spi_master *master;
-	struct spi_board_info *bi;
 	struct resource *res;
 	int err = 0;
-	int i;
 
 	master = spi_alloc_master(&pdev->dev, sizeof(struct s3c24xx_spi));
 	if (master == NULL) {
@@ -348,16 +346,6 @@ static int __init s3c24xx_spi_probe(stru
 		goto err_register;
 	}
 
-	/* register all the devices associated */
-
-	bi = &hw->pdata->board_info[0];
-	for (i = 0; i < hw->pdata->board_size; i++, bi++) {
-		dev_info(hw->dev, "registering %s\n", bi->modalias);
-
-		bi->controller_data = hw;
-		spi_new_device(master, bi);
-	}
-
 	return 0;
 
  err_register:
drivers/spi/spi_s3c24xx_gpio.c +0 -12
@@ -100,7 +100,6 @@ static int s3c2410_spigpio_probe(struct 
 	struct spi_master	*master;
 	struct s3c2410_spigpio  *sp;
 	int ret;
-	int i;
 
 	master = spi_alloc_master(&dev->dev, sizeof(struct s3c2410_spigpio));
 	if (master == NULL) {
@@ -143,17 +142,6 @@ static int s3c2410_spigpio_probe(struct 
 	if (ret)
 		goto err_no_bitbang;
 
-	/* register the chips to go with the board */
-
-	for (i = 0; i < sp->info->board_size; i++) {
-		dev_info(&dev->dev, "registering %p: %s\n",
-			 &sp->info->board_info[i],
-			 sp->info->board_info[i].modalias);
-
-		sp->info->board_info[i].controller_data = sp;
-		spi_new_device(master, sp->info->board_info + i);
-	}
-
 	return 0;
 
  err_no_bitbang:
include/asm/arch/spi-gpio.h +0 -6
@@ -13,9 +13,6 @@
 #ifndef __ASM_ARCH_SPIGPIO_H
 #define __ASM_ARCH_SPIGPIO_H __FILE__
 
-struct s3c2410_spigpio_info;
-struct spi_board_info;
-
 struct s3c2410_spigpio_info {
 	unsigned long		 pin_clk;
 	unsigned long		 pin_mosi;
@@ -23,9 +20,6 @@ struct s3c2410_spigpio_info {
 
 	int			 bus_num;
 
-	unsigned long		 board_size;
-	struct spi_board_info	*board_info;
-
 	void (*chip_select)(struct s3c2410_spigpio_info *spi, int cs);
 };
 
include/asm/arch/spi.h +0 -6
@@ -13,15 +13,9 @@
 #ifndef __ASM_ARCH_SPI_H
 #define __ASM_ARCH_SPI_H __FILE__
 
-struct s3c2410_spi_info;
-struct spi_board_info;
-
 struct s3c2410_spi_info {
 	unsigned long		 pin_cs;	/* simple gpio cs */
 
-	unsigned long		 board_size;
-	struct spi_board_info	*board_info;
-
 	void (*set_cs)(struct s3c2410_spi_info *spi, int cs, int pol);
 };
 

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

A message that is not in the archive (sent off-list or not yet received): 200801212341.07086.david-b-yBeKhBN/[email protected]

#2 lore raw
Quoted 4 lines · On Mon, 21 Jan 2008 23:41:06 -0800 David Brownell…

The two S3C SPI master drivers got merged without much review, so I just noticed that they're doing something that the SPI core code is responsible for, rather than any adapter driver: they try to register SPI devices.

err, but these driver predate the generic SPI core, I think?

Quoted 4 lines
drivers/spi/spi_s3c24xx.c      |   12 ------------
drivers/spi/spi_s3c24xx_gpio.c |   12 ------------
include/asm/arch/spi-gpio.h    |    6 ------
include/asm/arch/spi.h         |    6 ------

argh, please don't do that.

<searches>

It looks like this is referring to include/asm-arm/arch-s3c2410/

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

A message that is not in the archive (sent off-list or not yet received): 20080122000911.4b61af5c.akpm-de/[email protected]

#4 lore raw
Quoted 8 lines · Andrew Morton
Quoted 4 lines · On Mon, 21 Jan 2008 23:41:06 -0800 David Brownell…

The two S3C SPI master drivers got merged without much review, so I just noticed that they're doing something that the SPI core code is responsible for, rather than any adapter driver: they try to register SPI devices.

err, but these driver predate the generic SPI core, I think?

Nope. Such predated drivers don't land in drivers/spi ... and if they did predate it, they wouldn't have been able to try using those interfaces!

Quoted 10 lines
Quoted 4 lines
drivers/spi/spi_s3c24xx.c      |   12 ------------
drivers/spi/spi_s3c24xx_gpio.c |   12 ------------
include/asm/arch/spi-gpio.h    |    6 ------
include/asm/arch/spi.h         |    6 ------

argh, please don't do that.

<searches>

It looks like this is referring to include/asm-arm/arch-s3c2410/

Whoops, yes -- you're right. Sorry.

Quilt is doing a disservice there, by not using /bin/pwd to figure out directories. It's *way* too easy for that sort of mistake to sneak in when you just "quilt add spi*h". And then sometimes slip through on upstream pushes.

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

#5 lore raw
In reply to a message not in the archive
Quoted 8 lines · David Brownell

The two S3C SPI master drivers got merged without much review, so I just noticed that they're doing something that the SPI core code is responsible for, rather than any adapter driver: they try to register SPI devices.

This removes that support from those drivers so they act normally. Interestingly, none of the current boards are affected. So it's a net code shrink with no loss of functionality.

For you, for me you've just broken my current in-development patch-set.

Numbering busses makes life more difficult when registering the things, as on a design using a pluggable cpu module, you do not know at compile time what bus numbers are going to be assigned, especially if your adapters are not all of the same hardware.

Quoted 118 lines

---
I did a test build with the s3c defconfig. The only problem was with a missing #define breaking the a.out support.

 drivers/spi/spi_s3c24xx.c      |   12 ------------
 drivers/spi/spi_s3c24xx_gpio.c |   12 ------------
 include/asm/arch/spi-gpio.h    |    6 ------
 include/asm/arch/spi.h         |    6 ------
 4 files changed, 36 deletions(-)
drivers/spi/spi_s3c24xx.c +0 -12
@@ -237,10 +237,8 @@ static int __init s3c24xx_spi_probe(stru
 {
 	struct s3c24xx_spi *hw;
 	struct spi_master *master;
-	struct spi_board_info *bi;
 	struct resource *res;
 	int err = 0;
-	int i;
 
 	master = spi_alloc_master(&pdev->dev, sizeof(struct s3c24xx_spi));
 	if (master == NULL) {
@@ -348,16 +346,6 @@ static int __init s3c24xx_spi_probe(stru
 		goto err_register;
 	}
 
-	/* register all the devices associated */
-
-	bi = &hw->pdata->board_info[0];
-	for (i = 0; i < hw->pdata->board_size; i++, bi++) {
-		dev_info(hw->dev, "registering %s\n", bi->modalias);
-
-		bi->controller_data = hw;
-		spi_new_device(master, bi);
-	}
-
 	return 0;
 
  err_register:
drivers/spi/spi_s3c24xx_gpio.c +0 -12
@@ -100,7 +100,6 @@ static int s3c2410_spigpio_probe(struct 
 	struct spi_master	*master;
 	struct s3c2410_spigpio  *sp;
 	int ret;
-	int i;
 
 	master = spi_alloc_master(&dev->dev, sizeof(struct s3c2410_spigpio));
 	if (master == NULL) {
@@ -143,17 +142,6 @@ static int s3c2410_spigpio_probe(struct 
 	if (ret)
 		goto err_no_bitbang;
 
-	/* register the chips to go with the board */
-
-	for (i = 0; i < sp->info->board_size; i++) {
-		dev_info(&dev->dev, "registering %p: %s\n",
-			 &sp->info->board_info[i],
-			 sp->info->board_info[i].modalias);
-
-		sp->info->board_info[i].controller_data = sp;
-		spi_new_device(master, sp->info->board_info + i);
-	}
-
 	return 0;
 
  err_no_bitbang:
include/asm/arch/spi-gpio.h +0 -6
@@ -13,9 +13,6 @@
 #ifndef __ASM_ARCH_SPIGPIO_H
 #define __ASM_ARCH_SPIGPIO_H __FILE__
 
-struct s3c2410_spigpio_info;
-struct spi_board_info;
-
 struct s3c2410_spigpio_info {
 	unsigned long		 pin_clk;
 	unsigned long		 pin_mosi;
@@ -23,9 +20,6 @@ struct s3c2410_spigpio_info {
 
 	int			 bus_num;
 
-	unsigned long		 board_size;
-	struct spi_board_info	*board_info;
-
 	void (*chip_select)(struct s3c2410_spigpio_info *spi, int cs);
 };
 
include/asm/arch/spi.h +0 -7
@@ -13,15 +13,9 @@
 #ifndef __ASM_ARCH_SPI_H
 #define __ASM_ARCH_SPI_H __FILE__
 
-struct s3c2410_spi_info;
-struct spi_board_info;
-
 struct s3c2410_spi_info {
 	unsigned long		 pin_cs;	/* simple gpio cs */
 
-	unsigned long		 board_size;
-	struct spi_board_info	*board_info;
-
 	void (*set_cs)(struct s3c2410_spi_info *spi, int cs, int pol);
 };
 
 
-------------------------------------------------------------------------

This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
spi-devel-general mailing list spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Signature · 11 lines
-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

A message that is not in the archive (sent off-list or not yet received): 20080219101048.GA24550-SMNkleLxa3Z6Wcw2j4pizdi2O/[email protected]

#7 lore raw
Quoted 11 lines · Ben Dooks
Quoted 8 lines · David Brownell

The two S3C SPI master drivers got merged without much review, so I just noticed that they're doing something that the SPI core code is responsible for, rather than any adapter driver: they try to register SPI devices.

This removes that support from those drivers so they act normally. Interestingly, none of the current boards are affected. So it's a net code shrink with no loss of functionality.

For you, for me you've just broken my current in-development patch-set.

So those patches have problems that won't get a chance to go upstream. :)

Quoted 4 lines

Numbering busses makes life more difficult when registering the things, as on a design using a pluggable cpu module, you do not know at compile time what bus numbers are going to be assigned, especially if your adapters are not all of the same hardware.

Those problems are workable. There's clearly *something* which knows which physical bus has which devices attached, and can connect them. It can call spi_new_device() for example -- with no driver-specific mechanisms required.

If this is a general problem, it deserves a general solution, not one that's specific to one controller driver. There are such mechanisms already, which you're not using. If you want some different generic mechanisms, then feel free to propose them.

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/