I have several issues with this commit. I pointed out at least one of them already very long ago!
repository service wrote:
+++ trunk/src/drivers/Kconfig Fri Jun 4 21:53:55 2010 (r5609) @@ -23,3 +23,21 @@ help It sets PCI class to IDE compatible native mode, allowing SeaBIOS, FILO etc... to boot from it.
+config DRIVERS_TI
- bool
+config DRIVERS_TI_PCI1225
- select DRIVERS_TI
- bool
+config DRIVERS_TI_PCI1420
- select DRIVERS_TI
- bool
+config DRIVERS_TI_PCI1520
- select DRIVERS_TI
- bool
DRIVERS_TI is not a great name! And is the code for these three devices really so different that it warrants three different config variables? I doubt that.
+++ trunk/src/drivers/Makefile.inc Fri Jun 4 21:53:55 2010 (r5609) @@ -1,3 +1,5 @@ subdirs-y += generic/debug subdirs-y += ati/ragexl subdirs-y += sil/3114 +subdirs-y += ti/pcmcia-cardbus
The way this is done there is now a big disconnect between those CONFIG_TI_* and the filenames that they relate to. That really hurts understandability of our code base and I don't like that at all.
Added: trunk/src/drivers/dec/21143/21143pd.c
This path is really redundant. Is it correct? I am not sure. "Others are like that" is not a good reason. And why is there both 21143 and 21143pd? Does the code apply to ALL 21143 or strictly to 21143PD? The CONFIG_ name implies strictly 21143PD, but the directory name implies the opposite. This ambiguity is not good!
+++ trunk/src/drivers/dec/21143/21143pd.c Fri Jun 4 21:53:55 2010 (r5609)
..
+/**
- The following should be set in the mainboard-specific Kconfig file.
- */
+#if (!defined(CONFIG_DEC21143_CACHE_LINE_SIZE) || \
!defined(CONFIG_DEC21143_EXPANSION_ROM_BASE_ADDRESS) || \
!defined(CONFIG_DEC21143_COMMAND_AND_STATUS_CONFIGURATION))
+#error "you must supply these values in your mainboard-specific Kconfig file" +#endif
+/* CONFIG_DEC21143_CACHE_LINE_SIZE try 0x00000000 if unsure */ +/* CONFIG_DEC21143_EXPANSION_ROM_BASE_ADDRESS try 0x00000000 if unsure */ +/* CONFIG_DEC21143_COMMAND_AND_STATUS_CONFIGURATION try 0x02800107 or 0x02800007 if unsure */
Please explain why they should be set by the mainboard if there are default values?
I very strongly dislike adding any configuration whatsoever to mainboards, I think it is important to try to always avoid it. Can you explain why this is absolutely neccessary?
+++ trunk/src/drivers/ti/pcmcia-cardbus/ti-pcmcia-cardbus.c Fri Jun 4 21:53:55 2010 (r5609)
..
+#if ( !defined( CONFIG_TI_PCMCIA_CARDBUS_CMDR ) || \
- !defined( CONFIG_TI_PCMCIA_CARDBUS_CLSR ) || \
- !defined( CONFIG_TI_PCMCIA_CARDBUS_CLTR ) || \
- !defined( CONFIG_TI_PCMCIA_CARDBUS_BCR ) || \
- !defined( CONFIG_TI_PCMCIA_CARDBUS_SCR ) || \
- !defined( CONFIG_TI_PCMCIA_CARDBUS_MRR ) )
+#error "you must supply these values in your mainboard-specific Kconfig file" +#endif
Same thing here. Please explain more about how these values are mainboard-specific?
+static void ti_pci1x2y_init(struct device *dev) +{
- printk(BIOS_INFO, "Init of Texas Instruments PCI1x2x PCMCIA/CardBus controller\n");
Here is confirmation that the CONFIG_ names are not very good for this piece of code. The function name and printk suggests that the exact same code is used for all controllers.
+#ifdef CONFIG_DRIVERS_TI_PCI1225 +static const struct pci_driver ti_pci1225_driver __pci_driver = {
.ops = &ti_pci1x2y_ops,
.vendor = PCI_VENDOR_ID_TI,
.device = PCI_DEVICE_ID_TI_1225,
+};
+#endif +#ifdef CONFIG_DRIVERS_TI_PCI1420 +static const struct pci_driver ti_pci1420_driver __pci_driver = {
.ops = &ti_pci1x2y_ops,
.vendor = PCI_VENDOR_ID_TI,
.device = PCI_DEVICE_ID_TI_1420,
+}; +#endif +#ifdef CONFIG_DRIVERS_TI_PCI1520 +static const struct pci_driver ti_pci1520_driver __pci_driver = {
.ops = &ti_pci1x2y_ops,
.vendor = PCI_VENDOR_ID_TI,
.device = PCI_DEVICE_ID_TI_1420,
+}; +#endif
And yes indeed - the exact same code is used for all devices. Then I think there must also only be a single CONFIG_ variable.
+++ trunk/src/include/device/pci_ids.h Fri Jun 4 21:53:55 2010 (r5609) @@ -696,6 +696,7 @@ #define PCI_DEVICE_ID_TI_4410 0xac41 #define PCI_DEVICE_ID_TI_4451 0xac42 #define PCI_DEVICE_ID_TI_1420 0xac51 +#define PCI_DEVICE_ID_TI_1520 0xAC55
Somewhat minor, but please use lower case for a-f to look like other values in the file.
@@ -1741,6 +1742,10 @@ #define PCI_DEVICE_ID_CCD_B00C 0xb00c #define PCI_DEVICE_ID_CCD_B100 0xb100
+#define PCI_VENDOR_ID_NOKIA 0x13B8 // Nokia Telecommunications oy +#define PCI_VENDOR_ID_NOKIA_WIRELESS 0x1603 // Nokia Wireless Communications +#define PCI_VENDOR_ID_NOKIA_HOME 0x1622 // Nokia Home Communications
And please don't add such comments. The name of the define should be good enough to provide sufficiently meaningful information about what this number represents. If a comment is neccessary then the name is bad. I think the names are great, so there should be no comments. (Also, please avoid // comments, they're allowed in C99 but not in older C standards.)
+++ trunk/src/mainboard/nokia/ip530/Kconfig Fri Jun 4 21:53:55 2010 (r5609) @@ -24,10 +24,12 @@ select NORTHBRIDGE_INTEL_I440BX select SOUTHBRIDGE_INTEL_I82371EB select SUPERIO_SMSC_SMSCSUPERIO
- select DRIVERS_TI_PCI1225
- select DRIVERS_DEC_21143PD select ROMCC
- select PIRQ_ROUTE select HAVE_PIRQ_TABLE select UDELAY_TSC
- select BOARD_ROMSIZE_KB_256
I already asked many times why the romsize was removed, but Myles has already fixed it.
+## Configuration items for the ethernet adaptors +config DEC21143_CACHE_LINE_SIZE
If these are absolutely neccessary then I think they should go into devicetree.cb rather than Kconfig.
+## Configuration for the PCMCIA-Cardbus controller. +config TI_PCMCIA_CARDBUS_CMDR
Same for these.
//Peter