Re: [Xenomai-core] [PATCH] RT-Socket-CAN driver for EMS CPC PCI card

[Wolfgang Grandegger <wg@domain.hid>]

Jan Kiszka wrote:
> Wolfgang Grandegger wrote:
>> Hello,
>>
>> the following patch changes:
>>
>> 2007-08-22  Wolfgang Grandegger  <wg@domain.hid>
>>
>>     * ksrc/drivers/can/sja1000: Add the RT-Socket-CAN SJA1000 driver
>>     rtcan_ems_pci.c for the EMS CPC PCI card from EMS Dr. Thomas
>>     Wuensche (http://www.ems-wuensche.de).
>>
>> I would like to apply it to Xenomai's trunk and v2.3.x branch.
>>
> 
> I few quick comments from my side:
> 
>  - Could you submit new code already Lindent'ified? This keeps the
>    effort bounded to convert the rest one day.

I was already thinking about it.

>> Index: ksrc/drivers/can/sja1000/rtcan_ems_pci.c
>> ===================================================================
>> --- ksrc/drivers/can/sja1000/rtcan_ems_pci.c	(revision 0)
>> +++ ksrc/drivers/can/sja1000/rtcan_ems_pci.c	(revision 0)
> ...
>> +#define EMS_PCI_SINGLE 0 /* this is a single channel device */
> 
> This define is unused - is this intended?

Well, there are more. I'm going to remove them.

>> +static int rtcan_ems_pci_add_chan(struct pci_dev *pdev, int channel,
>> +				   struct rtcan_device **master_dev)
>> +{
>> +    struct rtcan_device *dev;
>> +    struct rtcan_sja1000 *chip;
>> +    struct rtcan_ems_pci *board;
>> +    unsigned long addr;
>> +    int ret, init_step = 1;
> 
> ret!=0 always means error here? In that case I find "err" a more telling
> variable name ("if (err) ..." -- personal flavour).

Agreed.

>> +    /* Register and setup interrupt handling */
>> +#ifdef CONFIG_XENO_OPT_SHIRQ_LEVEL
>> +    chip->irq_flags = RTDM_IRQTYPE_SHARED;
>> +#else
>> +    chip->irq_flags = 0;
>> +#endif
> 
> That #ifdef is redundant, just use RTDM_IRQTYPE_SHARED unconditionally.

OK.

>> +#ifdef CONFIG_XENO_OPT_SHIRQ_LEVEL
>> +    if ((ret = rtcan_ems_pci_add_chan(pdev, EMS_PCI_MASTER,
>> +				      &master_dev)))
>> +	    goto failure_cleanup;
>> +    if ((ret = rtcan_ems_pci_add_chan(pdev, EMS_PCI_SLAVE,
>> +				      &master_dev)))
>> +	    goto failure_cleanup;
>> +#else
>> +    printk("Shared interrupts not enabled, using single channel!\n");
>> +    if ((ret = rtcan_ems_pci_add_chan(pdev, EMS_PCI_MASTER,
>> +				      &master_dev)))
>> +	    goto failure_cleanup;
>> +#endif
> 
> The master registration should be moved out off the #ifdef - it's identical.

Yes, right.

> Besides this, I have no concerns merging it, also into 2.3. Thanks for
> your effort!

OK, thanks for reviewing the code.

Wolfgang.