Re: [PATCH 1/6] firewire: handling of cards, buses, nodes

Kristian Høgsberg
2007-05-07T23:44:19Z

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