P.B.Cheblakov wrote:
> 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?
I see, as we use "driver name" == "file name". This is more or less also
true for the other drivers, e.g. there could be a cc770 based PCI card
driver named "<vendor>_pci" as well. For the time being, I suggest to
keep the file name and use:
#define DRV_NAME "sja1000_plx_pci"
It might make sense to adapt the other PCI drivers as well.
[snip]
>>> + { /* 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?
Please ignore, it seems just to be my personal preference.
>>> +MODULE_DEVICE_TABLE(pci, plx_pci_tbl);
>>> +
[snip]
>>> +/*
>>> + * 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?
The method above will work with any normal memory location. The method
from the IXXAT driver will check if the device did come out of the reset
properly (by taking into account the reset values of the SJA1000). It's
the more reliable probing method, I believe.
>>> + /* 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), ... ? :)
It's just less error prune, but a minor issue, of course.
>>> + 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?
Right, some cards need other OCR/CDR settings, sometimes also depending
on the channel. The bits are documented in the SJA1000 data sheet.
> If it's necessary I can introduce an additional fields in
> plx_card_info_tbl[] array for these registers.
Yes, like you have already done for the clock (which is also the same
for all cards). Fine for now.
>>> + 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".
OK, missed that.
> 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? :)
OK, more infos on that later...
> Happy New Year! :)
Same to you.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
