Wolfgang Grandegger wrote:
>
> here finally comes my review, sorry for the delay.
>
Thanks, I'll make all the changes you suggested and resubmit.
I have however made some notes ...
>> +
>> +#define PEAK_PCI_MAP_SIZE (4 << 10)
>
> I find "0x4000" more readable.
>
Maybe, but it'd be wrong, it's 0x1000 :). I write it that way to
indicate it's 4K.
>
>> +#define PEAK_PCI_CAN_CLOCK (8 * 1000 * 1000)
>
> Why not just "8000000"? Or even better (16000000 / 2) to make clear that
> it's half of the oscillator frequency.
>
It's obviously 8 million. 8000000 is more difficult to read, is it
80000000 or 8000000 or 800000?
>> +
>> +static u8 peak_pci_can_read(struct sja1000_priv const *ctlr, int reg)
>
> "const struct sja1000_priv" please.
>
It's easier to see where the "const" applies this way, you just read
from right to left. It makes the difference between "struct x * const"
and "struct x const *" more obvious.
>> +
>> +static unsigned int peak_pci_intr_mask(struct sja1000_priv const *ctlr)
>> +{
>> + static unsigned int const mask[] =
>> + {
>> + 1 << 1,
>> + 1 << 0,
>> + 1 << 6,
>> + 1 << 7,
>> + };
>> +
>> + struct peak_pci_card *card;
>> + unsigned int chan;
>> +
>> + card = ctlr->priv;
>> +
>> + chan = (ctlr->reg_base - card->base_addr) / PEAK_PCI_CHAN_SIZE;
>> +
>> + BUG_ON(chan >= ARRAY_SIZE(mask));
>> +
>> + return mask[chan];
>> +}
>
> Hm, calculating the channel from the base address frequently seems not
> to be a good idea. Also please put the interrupt mask declaration into
> the header, e.g.:
>
> static const unsigned int peak_pci_intr_mask[PEAK_PCI_CHAN_MAX] = ...
>
I did think about the calculation but it's very fast, it's just a
subtraction and a shift. The other way to do it is a per channel
structure (as you mentioned below) which seems wasteful, it adds an
extra indirection for each access of the card structure. Maybe we'd be
better adding a "channel" member to "sja1000_priv" for use by the
drivers, in addition to the "priv" member. This would benefit all
multichannel cards.
>> +
>> + mask |= peak_pci_intr_mask(ctlr);
>
> mask |= peak_pci_intr_mask[card->channels]; ?
>
> I also would prefer reading the ICR first and just setting/clearing the
> bit relevant for the channel.
>
Why generate an extra PCI access for no gain?
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
