Peter Horton wrote:
> 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.
That just shows that it's an unusual representation for me.
>>> +#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?
OK, then please use ((16 * 1000 * 1000) / 2) to make everybody happy.
>>> +
>>> +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.
I have never seen that writing in the kernel. Please use the common way
of adding "const".
>>> +
>>> +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
There will be no real performance benefit, I agree.
> 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.
Normally, "priv" from "struct sja1000_priv" is for per-channel (device)
data. See:
http://lxr.linux.no/#linux+v2.6.31/drivers/net/can/sja1000/sja1000.c#L565
You can request to allocate extra space for the device and "priv" will
point to it afterwards. Maybe you can find a more clever solution.
>>> +
>>> + 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?
You touch data of other channels which might not be transparent to the
reader.
Thanks,
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
