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.
+ * 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.
+static DECLARE_RWSEM(card_rwsem);
this one is only ever used with down_read/up_read so a normal
mutex would probably better suited.
+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.
+static u32 *
+generate_config_rom (struct fw_card *card, size_t *config_rom_length)
Please don't put white spaces after the function identifier.
+ /* 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.
*/
+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.
+ /* 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.
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.