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

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1: Type: text/plain, Size: 2284 bytes --]

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.

> 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?

> +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).

> +    /* 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.

> +#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.

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]