Re: [PATCH v5] peak_pci: add support for PEAK-System PCIe/PCIeC/miniPCI cards

[Wolfgang Grandegger <wg@grandegger.com>]

On 02/03/2012 05:03 PM, Oliver Hartkopp wrote:
> Hallo Stephane,
> 
> looks really nice now.
> 
> Just a single question:
> 
> On 03.02.2012 15:53, Stephane Grosjean wrote:
> 
>> - the PCANExpressCard device id is finally let into the device_id table even
>>   if it is not explicitly selected in Kconfig; however the device probing fails
>>   and logs an explicit error message
> 
> 
> What is the reason for that?

We discussed that. Either we use the #ifdef in the id table or we return
"-ENODEV" in the placebo function. The later allows for a meaningful
error message (which is still missing).

> If we generally have the possibility to check for PCIEC, i would make it in
> the device id table and the module description too:
> 
>>  #include <linux/kernel.h>
>> @@ -26,22 +23,26 @@
>>  #include <linux/delay.h>
>>  #include <linux/pci.h>
>>  #include <linux/io.h>
> 
> #ifdef CONFIG_CAN_PEAK_PCIEC
> 
>> +#include <linux/i2c.h>
>> +#include <linux/i2c-algo-bit.h>
> 
> #endif

No, please not! #ifdef's around includes should never be necessary.

>>  #include <linux/can.h>
>>  #include <linux/can/dev.h>
>>  
>>  #include "sja1000.h"
>>  
>>  MODULE_AUTHOR("Wolfgang Grandegger <wg@grandegger.com>");
>> -MODULE_DESCRIPTION("Socket-CAN driver for PEAK PCAN PCI/PCIe cards");
>> -MODULE_SUPPORTED_DEVICE("PEAK PCAN PCI/PCIe CAN card");
>> +MODULE_DESCRIPTION("Socket-CAN driver for PEAK PCAN PCI family cards");
> #ifdef CONFIG_CAN_PEAK_PCIEC
>> +MODULE_SUPPORTED_DEVICE("PEAK PCAN PCI/PCIe/PCIeC miniPCI CAN cards");
> 
> #else
> 
>> +MODULE_SUPPORTED_DEVICE("PEAK PCAN PCI/PCIe miniPCI CAN cards");
> 
> #endif

Well, the driver is for all devices. Don't like that extra #ifdef. And
MODULE_SUPPORTED_DEVICE() is not even implemented yet.

>>  MODULE_LICENSE("GPL v2");
>>  
>>  #define DRV_NAME  "peak_pci"
>>  
>> +struct peak_pciec_card;
>>  struct peak_pci_chan {
>>  	void __iomem *cfg_base;		/* Common for all channels */
>>  	struct net_device *prev_dev;	/* Chain of network devices */
>>  	u16 icr_mask;			/* Interrupt mask for fast ack */
>> +	struct peak_pciec_card *pciec_card;	/* only for PCIeC LEDs */
>>  };
>>  
>>  #define PEAK_PCI_CAN_CLOCK	(16000000 / 2)
>> @@ -61,16 +62,469 @@ struct peak_pci_chan {
>>  
>>  #define PEAK_PCI_VENDOR_ID	0x001C	/* The PCI device and vendor IDs */
>>  #define PEAK_PCI_DEVICE_ID	0x0001	/* for PCI/PCIe slot cards */
>> +#define PEAK_PCIEC_DEVICE_ID	0x0002	/* for ExpressCard slot cards */
>> +#define PEAK_PCIE_DEVICE_ID	0x0003	/* for nextgen PCIe slot cards */
>> +#define PEAK_MPCI_DEVICE_ID	0x0008	/* The miniPCI slot cards */
>> +
>> +#define PEAK_PCI_CHAN_MAX	4
>>  
>> -static const u16 peak_pci_icr_masks[] = {0x02, 0x01, 0x40, 0x80};
>> +static const u16 peak_pci_icr_masks[PEAK_PCI_CHAN_MAX] = {
>> +	0x02, 0x01, 0x40, 0x80
>> +};
>>  
>>  static DEFINE_PCI_DEVICE_TABLE(peak_pci_tbl) = {
>>  	{PEAK_PCI_VENDOR_ID, PEAK_PCI_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,},
>> +	{PEAK_PCI_VENDOR_ID, PEAK_PCIE_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,},
>> +	{PEAK_PCI_VENDOR_ID, PEAK_MPCI_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,},
> #ifdef CONFIG_CAN_PEAK_PCIEC
>> +	{PEAK_PCI_VENDOR_ID, PEAK_PCIEC_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,},
> 
> #endif

See above.

Wolfgang.