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

2007-08-24 Thread Jan Kiszka 
Wolfgang Grandegger wrote:
 Hello,
 
 the following patch changes:
 
 2007-08-22  Wolfgang Grandegger  [EMAIL PROTECTED]
 
 * 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



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

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

2007-08-24 Thread Wolfgang Grandegger 
Jan Kiszka wrote:
 Wolfgang Grandegger wrote:
 Hello,

 the following patch changes:

 2007-08-22  Wolfgang Grandegger  [EMAIL PROTECTED]

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


___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

[Xenomai-core] [Website] About reported problems

2007-08-24 Thread Bruno Rouchouse 
Hi Xenomai users,

I've been reported some problems regarding Xenomai website (access problems
as well as skin and look and feel problems).

I'm currently investigating these issues, sorry for the inconvenience.

--
Bruno Rouchouse
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core