Messages in this thread | | | Date | Wed, 30 Jun 2010 11:28:29 -0700 | From | Andy Isaacson <> | 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
| |