Topcliff PHUB: Generate PacketHub driver
in [Kernel]
|
Prev: [PATCH] vfs: remove unused MNT_STRICTATIME
Next: wait_even_interruptible_timeout(), signal, spin_lock() = system hang
From: Andy Isaacson on 29 Jun 2010 19:40 On Tue, Jun 22, 2010 at 02:33:01PM +0900, Masayuki Ohtak wrote: > +config PCH_PHUB > + tristate "PCH PHUB" > + depends on PCI > + help > + If you say yes to this option, support will be included for the > + PCH Packet Hub Host controller. > + This driver is for PCH Packet hub driver for Topcliff. "Topcliff" is probably some kind of internal codename; please use a name that will be visible to customers of this product. References to what kind of device (IEEE standards it implements, what systems it might be present on, etc) are also appropriate. > + This driver is integrated as built-in. This sentence doesn't parse to me. What are you trying to say? > + This driver can access GbE MAC address. > + This driver can access HW register. I think you mean something more like "This driver allows access to the GbE MAC address and HW registers of the PCH Packet Hub." If this is a driver for an Ethernet MAC, what is it doing in drivers/char/ ? > + You can also be integrated as module. Please look at any other tristate and copy the standard wording. > +/*! > + * @file pch_phub.c > + * @brief Provides all the implementation of the interfaces pertaining to > + * the Packet Hub module. We don't normally merge comment markup languages other than kernel-doc. Please read Documentation/kernel-doc-nano-HOWTO.txt and follow it. (Or, provide a pointer to documentation and tools for this mysterious markup language.) > +/* > + * History: > + * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD. > + * > + * created: > + * OKI SEMICONDUCTOR 04/14/2010 > + * modified: No changelogs in comments, that's what git history is for. > +/* includes */ Useless comment. > +/*-------------------------------------------- > + macros > +--------------------------------------------*/ More useless comments. > +/* Status Register offset */ > +#define PHUB_STATUS 0x00 > +/* Control Register offset */ > +#define PHUB_CONTROL 0x04 I would format this as: #define PHUB_STATUS 0x00 /* Status Register offset */ #define PHUB_CONTROL 0x04 /* Control Register offset */ but it's up to you. > +struct pch_phub_reg { > + unsigned int phub_id_reg; Since these are used to hold HW-defined data, you should use fixed-size types such as u32. Also, you should consider whether they should be marked little endian / big endian. > +/* ToDo: major number allocation via module parameter */ > +static dev_t pch_phub_dev_no; > +static int pch_phub_major_no; > +static struct cdev pch_phub_dev; If this is to remain a chardev, use register_chrdev(). You shouldn't need a module parameter to configure your device numbers. > + void __iomem *reg_addr = pch_phub_reg.pch_phub_base_address +\ > + reg_addr_offset; That \ is superfluous. There are several of these in this file. The indentation on the second line is too large, and the fact that "a = b + c" spills onto a second line is a clue that your struct names are too long. > + iowrite32(((ioread32(reg_addr) & ~mask)) | data, reg_addr); > + return; The return is unneeded if this remains a void function. (many more occurrences.) > +/** pch_phub_restore_reg_conf - restore register configuration > + */ Don't use /** unless you're using kernel-doc. > + unsigned int i; > + void __iomem *p = pch_phub_reg.pch_phub_base_address; > + > + dev_dbg(&pdev->dev, "pch_phub_restore_reg_conf ENTRY\n"); > + /* to store contents of PHUB_ID register */ > + iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_PHUB_ID_REG); Don't include comments that just duplicate the code. Also, rename your constants from PCH_PHUB_PHUB_ to, I dunno, PHUB_ or something. > +/** pch_phub_read_serial_rom - Implements the functionality of reading Serial > + * ROM. > + * @offset_address: Contains the Serial ROM address offset value > + * @data: Contains the Serial ROM value > + */ This comment is almost correctly formatted, but there are extra words and some whitespace problems, and it doesn't document what actually happens. +/** + * pch_phub_read_serial_rom - read PHUB Serial ROM. + * @offset_address: serial ROM offset to read. + * @data: pointer at which to store value that was read. + */ is more correct. Similar problems in several function comments below. > +static int pch_phub_read_serial_rom(unsigned int offset_address, > + unsigned char *data) The second line is indented too far. We use 8-space tabstops, so the "u" of unsigned is all the way over under the "t" of offset_address. It should either be two tabstops indented, or lined up with the previous argument. (This applies to several functions below too.) It should be "u8 *data". > + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +\ > + offset_address; > + > + *data = ioread8(mem_addr); > + > + return 0; If this can't fail, why not have it just return the value rather than forcing the caller to provide a pointer? (If you want to be futureproof and support errorhandling in the future, that's OK.) > +static int pch_phub_write_serial_rom(unsigned int offset_address, > + unsigned char data) > +{ > + int retval = 0; > + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +\ > + (offset_address & 0xFFFFFFFC); > + int i = 0; > + unsigned int word_data = 0; > + > + iowrite32(PCH_PHUB_ROM_WRITE_ENABLE, > + pch_phub_reg.pch_phub_extrom_base_address +\ > + PHUB_CONTROL); > + > + word_data = ioread32(mem_addr); > + > + switch (offset_address % 4) { > + case 0: > + word_data &= 0xFFFFFF00; > + iowrite32((word_data | (unsigned int)data), > + mem_addr); > + break; This function is much larger than need be. Remove the 0xFFFFFFFC magic number -- something like #define PCH_WORD_ADDR_MASK (~((1 << 4) - 1)) and implement the byte masking as something like pos = (offset_address % 4) * 8; mask = ~(0xFF << pos); iowrite32((word_data & mask) | (u32)data << pos, mem_addr); (You can keep the switch if there's a significant performance benefit to doing so, but please provide measurements.) This is a read-modify-write cycle -- where is the locking if two users try to update the serial ROM simultaneously? Any required locking should be documented in the kernel-doc comment. > + while (0x00 != > + ioread8(pch_phub_reg.pch_phub_extrom_base_address +\ > + PHUB_STATUS)) { > + msleep(1); > + if (PHUB_TIMEOUT == i) { > + retval = -EPERM; > + break; > + } > + i++; Rewrite this as a for loop (and remove the initialization from the declaration of i at the top of this function). I am not sure if msleep() is safe here -- what context will this be called from? > +/** pch_phub_read_serial_rom_val - Implements the functionality of reading > + * Serial ROM value. > + * @offset_address: Contains the Serial ROM address offset value > + * @data: Contains the Serial ROM value > + */ > +static int pch_phub_read_serial_rom_val(unsigned int offset_address, > + unsigned char *data) > +{ > + int retval = 0; > + unsigned int mem_addr; > + > + mem_addr = (offset_address / 4 * 8) + 3 - > + (offset_address % 4) + PCH_PHUB_ROM_START_ADDR; Is that formula correct? It is very suprising. If it is correct, the formula is bizarre enough to warrant a long comment explaining the theory of operation and/or pointing to hardware docs. > +static ssize_t pch_phub_read(struct file *file, char __user *buf, size_t size, > + loff_t *ppos) > +{ > + unsigned int rom_signature = 0; > + unsigned char rom_length; > + int ret_value; > + unsigned int tmp; > + unsigned char data; > + unsigned int addr_offset = 0; > + unsigned int orom_size; > + loff_t pos = *ppos; > + > + if (pos < 0) > + return -EINVAL; > + > + /*Get Rom signature*/ > + pch_phub_read_serial_rom(0x80, (unsigned char *)&rom_signature); > + pch_phub_read_serial_rom(0x81, (unsigned char *)&tmp); I seriously doubt that your device is special enough to warrant a custom /dev node with proprietary semantics. If this is just part of an Ethernet driver, please implement it in drivers/net/; if this is a generic PROM accessor, there must be some semi-standardized EPROM access interface but I don't know what it is offhand. -andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Masayuki Ohtake on 30 Jun 2010 02:00 Hi Andy Thank you for your comments. Please refer to the following inline. Thanks, Ohtake. ----- Original Message ----- From: "Andy Isaacson" <adi(a)hexapodia.org> To: "Masayuki Ohtak" <masa-korg(a)dsn.okisemi.com> Cc: "Arnd Bergmann" <arnd(a)arndb.de>; "Wang, Yong Y" <yong.y.wang(a)intel.com>; "Wang Qi" <qi.wang(a)intel.com>; "Intel OTC" <joel.clark(a)intel.com>; "Andrew" <andrew.chih.howe.khor(a)intel.com>; "Alan Cox" <alan(a)lxorguk.ukuu.org.uk>; "LKML" <linux-kernel(a)vger.kernel.org> Sent: Wednesday, June 30, 2010 8:31 AM Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver > On Tue, Jun 22, 2010 at 02:33:01PM +0900, Masayuki Ohtak wrote: > > +config PCH_PHUB > > + tristate "PCH PHUB" > > + depends on PCI > > + help > > + If you say yes to this option, support will be included for the > > + PCH Packet Hub Host controller. > > + This driver is for PCH Packet hub driver for Topcliff. > > "Topcliff" is probably some kind of internal codename; please use a name Topcliff is not internal codename but external product name. > that will be visible to customers of this product. References to what > kind of device (IEEE standards it implements, what systems it might be > present on, etc) are also appropriate. I will add what kinds of device. > > > + This driver is integrated as built-in. > > This sentence doesn't parse to me. What are you trying to say? > I will modify above. > > + This driver can access GbE MAC address. > > + This driver can access HW register. > > I think you mean something more like "This driver allows access to the > GbE MAC address and HW registers of the PCH Packet Hub." > > If this is a driver for an Ethernet MAC, what is it doing in > drivers/char/ ? In Topcliff, MAC address is in PHUB HW not in GbE HW. > > > + You can also be integrated as module. > > Please look at any other tristate and copy the standard wording. I will modify above. > > > +/*! > > + * @file pch_phub.c > > + * @brief Provides all the implementation of the interfaces pertaining to > > + * the Packet Hub module. > > We don't normally merge comment markup languages other than kernel-doc. > Please read Documentation/kernel-doc-nano-HOWTO.txt and follow it. (Or, > provide a pointer to documentation and tools for this mysterious markup > language.) I will modify above. > > > +/* > > + * History: > > + * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD. > > + * > > + * created: > > + * OKI SEMICONDUCTOR 04/14/2010 > > + * modified: > > No changelogs in comments, that's what git history is for. I will modify above. > > > +/* includes */ > > Useless comment. I will delete above. > > > +/*-------------------------------------------- > > + macros > > +--------------------------------------------*/ > > More useless comments. I will delete above. > > > +/* Status Register offset */ > > +#define PHUB_STATUS 0x00 > > +/* Control Register offset */ > > +#define PHUB_CONTROL 0x04 > > I would format this as: > > #define PHUB_STATUS 0x00 /* Status Register offset */ > #define PHUB_CONTROL 0x04 /* Control Register offset */ > > but it's up to you. I will modify above. > > > +struct pch_phub_reg { > > + unsigned int phub_id_reg; > > Since these are used to hold HW-defined data, you should use fixed-size > types such as u32. Also, you should consider whether they should be > marked little endian / big endian. I will modify above. > > > +/* ToDo: major number allocation via module parameter */ > > +static dev_t pch_phub_dev_no; > > +static int pch_phub_major_no; > > +static struct cdev pch_phub_dev; > > If this is to remain a chardev, use register_chrdev(). You shouldn't > need a module parameter to configure your device numbers. I will delete module paramter. > > > + void __iomem *reg_addr = pch_phub_reg.pch_phub_base_address +\ > > + reg_addr_offset; > > That \ is superfluous. There are several of these in this file. I will delete uncecessary '\'. > > The indentation on the second line is too large, and the fact that > "a = b + c" spills onto a second line is a clue that your struct names > are too long. We don't modify structure name this time. > > > + iowrite32(((ioread32(reg_addr) & ~mask)) | data, reg_addr); > > + return; > > The return is unneeded if this remains a void function. (many more > occurrences.) I will delete 'return' of void function. > > > +/** pch_phub_restore_reg_conf - restore register configuration > > + */ > > Don't use /** unless you're using kernel-doc. I will modify above. > > > + unsigned int i; > > + void __iomem *p = pch_phub_reg.pch_phub_base_address; > > + > > + dev_dbg(&pdev->dev, "pch_phub_restore_reg_conf ENTRY\n"); > > + /* to store contents of PHUB_ID register */ > > + iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_PHUB_ID_REG); > > Don't include comments that just duplicate the code. Also, rename your > constants from PCH_PHUB_PHUB_ to, I dunno, PHUB_ or something. Sorry, I can't understand your intention. Please give us more information. > > > +/** pch_phub_read_serial_rom - Implements the functionality of reading Serial > > + * ROM. > > + * @offset_address: Contains the Serial ROM address offset value > > + * @data: Contains the Serial ROM value > > + */ > > This comment is almost correctly formatted, but there are extra words > and some whitespace problems, and it doesn't document what actually > happens. I will modify above. > > +/** > + * pch_phub_read_serial_rom - read PHUB Serial ROM. > + * @offset_address: serial ROM offset to read. > + * @data: pointer at which to store value that was read. > + */ > > is more correct. I will modify above. > > Similar problems in several function comments below. > > > +static int pch_phub_read_serial_rom(unsigned int offset_address, > > + unsigned char *data) > > The second line is indented too far. We use 8-space tabstops, so the > "u" of unsigned is all the way over under the "t" of offset_address. It > should either be two tabstops indented, or lined up with the previous > argument. (This applies to several functions below too.) I will modify above. > > It should be "u8 *data". I will modify above. > > > + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +\ > > + offset_address; > > + > > + *data = ioread8(mem_addr); > > + > > + return 0; > > If this can't fail, why not have it just return the value rather than > forcing the caller to provide a pointer? (If you want to be futureproof > and support errorhandling in the future, that's OK.) I will modify to void. > > > +static int pch_phub_write_serial_rom(unsigned int offset_address, > > + unsigned char data) > > +{ > > + int retval = 0; > > + void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +\ > > + (offset_address & 0xFFFFFFFC); > > + int i = 0; > > + unsigned int word_data = 0; > > + > > + iowrite32(PCH_PHUB_ROM_WRITE_ENABLE, > > + pch_phub_reg.pch_phub_extrom_base_address +\ > > + PHUB_CONTROL); > > + > > + word_data = ioread32(mem_addr); > > + > > + switch (offset_address % 4) { > > + case 0: > > + word_data &= 0xFFFFFF00; > > + iowrite32((word_data | (unsigned int)data), > > + mem_addr); > > + break; > > This function is much larger than need be. Remove the 0xFFFFFFFC magic > number -- something like > > #define PCH_WORD_ADDR_MASK (~((1 << 4) - 1)) > > and implement the byte masking as something like > > pos = (offset_address % 4) * 8; > mask = ~(0xFF << pos); > iowrite32((word_data & mask) | (u32)data << pos, mem_addr); > > (You can keep the switch if there's a significant performance benefit to > doing so, but please provide measurements.) I will modify.(This processing is not performance critical.) > > This is a read-modify-write cycle -- where is the locking if two users > try to update the serial ROM simultaneously? Any required locking > should be documented in the kernel-doc comment. I will modify using MUTEX like ioctl. > > > + while (0x00 != > > + ioread8(pch_phub_reg.pch_phub_extrom_base_address +\ > > + PHUB_STATUS)) { > > + msleep(1); > > + if (PHUB_TIMEOUT == i) { > > + retval = -EPERM; > > + break; > > + } > > + i++; > > Rewrite this as a for loop (and remove the initialization from the > declaration of i at the top of this function). I am not sure if > msleep() is safe here -- what context will this be called from? This function is called from ioctl context(not interrput). Thus, I think safe. > > > +/** pch_phub_read_serial_rom_val - Implements the functionality of reading > > + * Serial ROM value. > > + * @offset_address: Contains the Serial ROM address offset value > > + * @data: Contains the Serial ROM value > > + */ > > +static int pch_phub_read_serial_rom_val(unsigned int offset_address, > > + unsigned char *data) > > +{ > > + int retval = 0; > > + unsigned int mem_addr; > > + > > + mem_addr = (offset_address / 4 * 8) + 3 - > > + (offset_address % 4) + PCH_PHUB_ROM_START_ADDR; > > Is that formula correct? It is very suprising. > > If it is correct, the formula is bizarre enough to warrant a long > comment explaining the theory of operation and/or pointing to hardware > docs. I will modify the above and add comments. > > > +static ssize_t pch_phub_read(struct file *file, char __user *buf, size_t size, > > + loff_t *ppos) > > +{ > > + unsigned int rom_signature = 0; > > + unsigned char rom_length; > > + int ret_value; > > + unsigned int tmp; > > + unsigned char data; > > + unsigned int addr_offset = 0; > > + unsigned int orom_size; > > + loff_t pos = *ppos; > > + > > + if (pos < 0) > > + return -EINVAL; > > + > > + /*Get Rom signature*/ > > + pch_phub_read_serial_rom(0x80, (unsigned char *)&rom_signature); > > + pch_phub_read_serial_rom(0x81, (unsigned char *)&tmp); > > I seriously doubt that your device is special enough to warrant a custom > /dev node with proprietary semantics. If this is just part of an > Ethernet driver, please implement it in drivers/net/; if this is a > generic PROM accessor, there must be some semi-standardized EPROM access > interface but I don't know what it is offhand. Since SROM is not in GbE HW but Phub HW, Phub is not part of Ethernet driver. Packet hub is not generic driver but special device. > > -andy > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andy Isaacson on 30 Jun 2010 14:30 On Wed, Jun 30, 2010 at 02:58:25PM +0900, Masayuki Ohtake wrote: > > > + unsigned int i; > > > + void __iomem *p = pch_phub_reg.pch_phub_base_address; > > > + > > > + dev_dbg(&pdev->dev, "pch_phub_restore_reg_conf ENTRY\n"); > > > + /* to store contents of PHUB_ID register */ > > > + iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_PHUB_ID_REG); > > > > Don't include comments that just duplicate the code. Also, rename your > > constants from PCH_PHUB_PHUB_ to, I dunno, PHUB_ or something. > > Sorry, I can't understand your intention. > Please give us more information. My mistake, I merged two comments into one paragraph, let me clarify. 1. When writing comments, do not write comments that duplicate the code. Instead of writing /* store PHUB_ID */ iowrite32(..._PHUB_ID_REG); /* store PHUB_FOO */ iowrite32(..._PHUB_FOO_REG); you should delete the line-by-line comments and just write iowrite32(..._PHUB_ID_REG); iowrite32(..._PHUB_FOO_REG); 2. your register names are very long. Since the #define names are private to this driver, there's no need to make them extremely descriptive. Instead of naming your registers PCH_PHUB_PHUB_ID_REG, you should change the names to be shorter, like PHUB_ID_REG or PCH_ID_REG. This will make your source code much more readable by reducing linewrapping. > > I seriously doubt that your device is special enough to warrant a custom > > /dev node with proprietary semantics. If this is just part of an > > Ethernet driver, please implement it in drivers/net/; if this is a > > generic PROM accessor, there must be some semi-standardized EPROM access > > interface but I don't know what it is offhand. > > Since SROM is not in GbE HW but Phub HW, Phub is not part of Ethernet driver. > Packet hub is not generic driver but special device. It sounds like PHUB is a system-level device which provides access to a SROM which contains GbE configuration data. If that is correct, then I have two comments: 1. There are many other systems with similar configurations -- MIPS SiByte, Alpha SRM, SPARC OpenFirmware, and some ARM systems, just to name a few. None of them expose the SROM as a custom /dev node AFAIK. Is there a shared infrastructure that you can implement? 2. How does your GbE driver get the MAC address from the SPROM? If there is an in-kernel user of the PHUB interface, it might be much easier to understand the design. Thanks for responding to my review so quickly! -andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Masayuki Ohtake on 1 Jul 2010 00:10
Hi Andy, > 1. When writing comments, do not write comments that duplicate the code. > Instead of writing Our Phub patch I have resubmitted yesterday have been already modified above. > 2. your register names are very long. Since the #define names are > private to this driver, there's no need to make them extremely > descriptive. Instead of naming your registers > PCH_PHUB_PHUB_ID_REG, you should change the names to be shorter, like > PHUB_ID_REG or PCH_ID_REG. This will make your source code much more > readable by reducing linewrapping. This was our mistake. I have modified PCH_PHUB_PHUB_ID_REG to PCH_PHUB_ID_REG. Our Phub patch I have resubmitted yesterday have been already modified above. > It sounds like PHUB is a system-level device which provides access to a > SROM which contains GbE configuration data. If that is correct, then I > have two comments: Yes, SROM has GbE configuration data (GbE mac address) . > > 1. There are many other systems with similar configurations -- MIPS > SiByte, Alpha SRM, SPARC OpenFirmware, and some ARM systems, just to > name a few. None of them expose the SROM as a custom /dev node AFAIK. > Is there a shared infrastructure that you can implement? Sorry, I can't understand your intension. Please give me more detail. > > 2. How does your GbE driver get the MAC address from the SPROM? If > there is an in-kernel user of the PHUB interface, it might be much > easier to understand the design. PHUB HW transfers MAC address(in SROM) data to GbE register to set MAC address when boot processing. Thanks, Ohtake ----- Original Message ----- From: "Andy Isaacson" <adi(a)hexapodia.org> To: "Masayuki Ohtake" <masa-korg(a)dsn.okisemi.com> Cc: "Arnd Bergmann" <arnd(a)arndb.de>; "Wang, Yong Y" <yong.y.wang(a)intel.com>; "Wang Qi" <qi.wang(a)intel.com>; "Intel OTC" <joel.clark(a)intel.com>; "Andrew" <andrew.chih.howe.khor(a)intel.com>; "Alan Cox" <alan(a)lxorguk.ukuu.org.uk>; "LKML" <linux-kernel(a)vger.kernel.org> Sent: Thursday, July 01, 2010 3:28 AM Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver > On Wed, Jun 30, 2010 at 02:58:25PM +0900, Masayuki Ohtake wrote: > > > > + unsigned int i; > > > > + void __iomem *p = pch_phub_reg.pch_phub_base_address; > > > > + > > > > + dev_dbg(&pdev->dev, "pch_phub_restore_reg_conf ENTRY\n"); > > > > + /* to store contents of PHUB_ID register */ > > > > + iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_PHUB_ID_REG); > > > > > > Don't include comments that just duplicate the code. Also, rename your > > > constants from PCH_PHUB_PHUB_ to, I dunno, PHUB_ or something. > > > > Sorry, I can't understand your intention. > > Please give us more information. > > My mistake, I merged two comments into one paragraph, let me clarify. > > 1. When writing comments, do not write comments that duplicate the code. > Instead of writing > /* store PHUB_ID */ > iowrite32(..._PHUB_ID_REG); > /* store PHUB_FOO */ > iowrite32(..._PHUB_FOO_REG); > you should delete the line-by-line comments and just write > iowrite32(..._PHUB_ID_REG); > iowrite32(..._PHUB_FOO_REG); > > 2. your register names are very long. Since the #define names are > private to this driver, there's no need to make them extremely > descriptive. Instead of naming your registers > PCH_PHUB_PHUB_ID_REG, you should change the names to be shorter, like > PHUB_ID_REG or PCH_ID_REG. This will make your source code much more > readable by reducing linewrapping. > > > > I seriously doubt that your device is special enough to warrant a custom > > > /dev node with proprietary semantics. If this is just part of an > > > Ethernet driver, please implement it in drivers/net/; if this is a > > > generic PROM accessor, there must be some semi-standardized EPROM access > > > interface but I don't know what it is offhand. > > > > Since SROM is not in GbE HW but Phub HW, Phub is not part of Ethernet driver. > > Packet hub is not generic driver but special device. > > It sounds like PHUB is a system-level device which provides access to a > SROM which contains GbE configuration data. If that is correct, then I > have two comments: > > 1. There are many other systems with similar configurations -- MIPS > SiByte, Alpha SRM, SPARC OpenFirmware, and some ARM systems, just to > name a few. None of them expose the SROM as a custom /dev node AFAIK. > Is there a shared infrastructure that you can implement? > > 2. How does your GbE driver get the MAC address from the SPROM? If > there is an in-kernel user of the PHUB interface, it might be much > easier to understand the design. > > Thanks for responding to my review so quickly! > -andy > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |