Hello Wolfgang
>> I'm not sure that plx_pci is a right name for this driver. But I think
>> this driver have certain potentialities to support any other similar
>> cards (based on PLX bridges).
>>
>
> The name sounds reasonable. In principle, also the ixxat_pci cards could
> be supported by this driver. IIRC, there was just some trick with
> probing the number of CAN channels. You may want to check/compare? In
> general it's *good* to share code.
>
I'm not fully satisfied with such name. plx_pci is too general name and
it may interfere with other (non-CAN) plx-based devices. Currently, as I
understand it, all drivers named by the following scheme: [vendor of
card]_interface. But PLX is not a vendor of some card. Maybe is it
better to name it as plx_can_pci or like this?
> - Use the proper style for comments. Please check:
>
> http://lxr.linux.no/#linux+v2.6.32/Documentation/CodingStyle#L425
>
Oh, ok! I've read it and fixed all comments!
> - Consider using (dev)/(!dev) instead of ((dev != NULL)/(dev == NULL).
>
And this I have fixed too.
>> +
>> +#define ADLINK_PCI_VENDOR_ID 0x144A
>> +#define ADLINK_PCI_DEVICE_ID 0x7841
>> +
>> +#define MARATHON_PCI_DEVICE_ID 0x2715
>> +
>> +#define TEWS_PCI_VENDOR_ID 0x1498
>> +#define TEWS_PCI_DEVICE_ID_TMPC810 0x032A
>>
>
> Please check if the vendors are already defined in the kernel's
> "include/linux/pci_ids.h".
>
There are no such vendors in pci_ids.h
>> +
>> +struct channel_map {
>> + u8 bar;
>>
>
> As it does not save any space, u32 is fine for "bar" as well.
>
Ok.
>> +static struct plx_card_info {
>> + const char *name;
>> + u8 channel_count;
>>
>
> unsigned int? See above.
>
It also have changed to u32.
> Please use just one tab per indention level, put "{" on a separate line
> and concatenate some lines, e.g.:
>
> } plx_card_info_tbl[] __devinitdata = {
> {
> "Marathon CAN-bus-PCI", 2, PLX_PCI_CAN_CLOCK,
> { {0, 0x00, 0x00}, {2, 0x00, 0x00}, {4, 0x00, 0x00} }
> },
>
Ok.
>> + {"TEWS TECHNOLOGIES TPMC810",
>> + 2, PLX_PCI_CAN_CLOCK, { {0, 0x00, 0x00},
>> + {2, 0x000, 0x80},
>> + {2, 0x100, 0x80} }
>> + },
>> + { NULL }
>>
>
> Do you need the zero termination? Use ARRAY_SIZE() instead.
>
removed
>> + { /* TEWS TECHNOLOGIES TPMC810 card */
>> + TEWS_PCI_VENDOR_ID, TEWS_PCI_DEVICE_ID_TMPC810,
>> + PCI_ANY_ID, PCI_ANY_ID,
>> + 0, 0,
>> + TEWS_TPMC810
>> + },
>> + { 0,}
>> +};
>> +
>>
>
> Please remove empty line above.
>
removed
Why this empty line should be removed?
>
>> +MODULE_DEVICE_TABLE(pci, plx_pci_tbl);
>> +
>> +static u8 plx_pci_read_reg(const struct sja1000_priv *priv, int port)
>> +{
>> + return readb(priv->reg_base + port);
>> +}
>> +
>> +static void plx_pci_write_reg(const struct sja1000_priv *priv, int port, u8
>> val)
>> +{
>> + writeb(val, priv->reg_base + port);
>> +}
>> +
>> +/*
>> + * Check if a CAN controller is present at the specified location
>> + * by trying to set 'em into the PeliCAN mode
>> + */
>> +static inline int plx_pci_check_sja1000(const struct sja1000_priv *priv)
>> +{
>> + u8 res;
>> +
>> + /* Make sure SJA1000 is in reset mode */
>> + priv->write_reg(priv, REG_MOD, 1);
>> +
>> + priv->write_reg(priv, REG_CDR, CDR_PELICAN);
>> +
>> + /* read reset-values */
>> + res = priv->read_reg(priv, REG_CDR);
>> +
>> + if (res == CDR_PELICAN)
>> + return 1;
>>
>
> The ixxat pci driver uses a more sophisticated probing mechanism, which
> might fit here as well.
>
I'll review ixxat driver once again. But I don't understand why
sophisticated probing mechanism is be better?
>> +
>> + /* Allocate card structures to hold addresses, ... */
>> + card = kzalloc(sizeof(struct plx_pci_card), GFP_KERNEL);
>>
>
> Please use
>
> card = kzalloc(*card, ...
>
Maybe
card = kzalloc(sizeof(*card), ... ? :)
>> + if (card->conf_addr == NULL) {
>>
>
> Hm, this test is wrong if "ci->ch_map_tbl[0].offset != 0", right?
>
Yeah, it's my mistake! I've corrected it.
Thanks
>> + /* Check if channel is present */
>> + if (plx_pci_check_sja1000(priv)) {
>> + priv->can.clock.freq = ci->can_clock;
>> + priv->ocr = PLX_PCI_OCR;
>> + priv->cdr = PLX_PCI_CDR;
>>
>
> BTW: do all cards work with the same OCR/CDR settings?
>
I'm not sure at all, but all cards work well. Where can I find an exact
values of these registers? As I understand these registers doesn't play
a role if can transceiver is used. Is it right?
If it's necessary I can introduce an additional fields in
plx_card_info_tbl[] array for these registers.
> > + dev_info(&pdev->dev, "Channel #%d at 0x%x, irq %d "
> > + "registered as %s\n",
> > + i + 1, (u32)dev->base_addr,
> > + dev->irq, dev->name);
>
>
> Use "0x%p" to avoid the cast.
>
dev->base_addr has long unsigned int type. I've used "0x%lx".
I have considered all other minor remarks.
> Do you intend to push the driver to mainlain as well? That would be nice.
>
Yes, I do. What should I do to do it? :)
> Thanks for your contribution.
>
> Wolfgang.
>
Happy New Year! :)
Pavel
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
