Christoph Hellwig wrote:
On Wed, May 02, 2007 at 02:15:42PM +0200, Stefan Richter wrote:
+/* -*- c-basic-offset: 8 -*-
Please don't pollute the code with annotation for some editors.
OK.
+ * fw-card.c - card level functions
Please don't put the filename into a comment inside the filename.
It's not actually useful and it gets out of date very easily.
Yeah, true.
+static DECLARE_RWSEM(card_rwsem);
this one is only ever used with down_read/up_read so a normal
mutex would probably better suited.
down_write/up_write, but yes, I'll change that to a mutex. The card_rwsem was
previously subsystem.rwsem, but that went away.
+static LIST_HEAD(card_list);
+#define bib_pmc ((1) << 27)
+#define bib_bmc ((1) << 28)
+#define bib_isc ((1) << 29)
+#define bib_cmc ((1) << 30)
+#define bib_imc ((1) << 31)
These are rather odd names for constants. They should at least
start with an uppercase letter if not be all uppercase. Also
an enum might be right here.
Sure, I'll do that... all those uppercase letters hurt my eyes, though.
+static u32 *
+generate_config_rom (struct fw_card *card, size_t *config_rom_length)
Please don't put white spaces after the function identifier.
Ugh, yeah, I don't know why I keep doing that...
+ /* Initialize contents of config rom buffer. On the OHCI
+ * controller, block reads to the config rom accesses the host
+ * memory, but quadlet read access the hardware bus info block
+ * registers. That's just crack, but it means we should make
+ * sure the contents of bus info block in host memory mathces
+ * the version stored in the OHCI registers. */
Normal style for block comments is:
/*
* Initialize contents of config rom buffer. On the OHCI
* controller, block reads to the config rom accesses the host
* memory, but quadlet read access the hardware bus info block
* registers. That's just crack, but it means we should make
* sure the contents of bus info block in host memory mathces
* the version stored in the OHCI registers.
*/
Oh, ok, I can clean that up.
+struct fw_node {
+ u16 node_id;
+ u8 color;
+ u8 port_count;
+ unsigned link_on : 1;
+ unsigned initiated_reset : 1;
+ unsigned b_path : 1;
+ u8 phy_speed : 3; /* As in the self ID packet. */
+ u8 max_speed : 5; /* Minimum of all phy-speeds and port speeds on
+ * the path from the local node to this node. */
+ u8 max_depth : 4; /* Maximum depth to any leaf node */
+ u8 max_hops : 4; /* Max hops in this sub tree */
I don't think we need to save the few bits here and can just use
u8 instead of bitfields.
These bitfields aren't used on the wire or anywhere outside the driver... what
the problem? Perfomance?
+ /* Pop the child nodes off the stack and push the new node. */
+ __list_del(h->prev, &stack);
Please don't ever use __list_del directly, it's an interna implementation
detail.
Do you have an alternative suggestion here? I need to take a sub-list of the
list, which is exactly what __list_del does. I could loop through the
sublist, but it's pretty pointless since I have both ends of the sublist.
I also notices that close to none of the export functions have kerneldoc
comment blocks. I think we really should have them on something that
is a driver API.
Yeah... I'll sit down and try to document it better.
thanks
Kristian
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
