[asterisk-dev] [Code Review] a driver for single-port FXO cards based on Si3052 chip + Si3011/17/18 DAA (Motorola 52-6116-2A, PM560MS etc.)
Shaun Ruffell
sruffell at digium.com
Sun Oct 17 18:40:03 CDT 2010
> On 2010-10-17 12:21:42, Tzafrir Cohen wrote:
> > Hi,
> >
> > First-off, it's good to see an extra driver.
> >
> > Before I look into the code, I figure I'd start with the obvious checks:
> >
> > $ make
> >
> > drivers/dahdi/fxo_si3052.c: In function ‘si3052_irq’:
> > drivers/dahdi/fxo_si3052.c:660: warning: passing argument 1 of ‘si3052_dma_buffers’ makes integer from pointer without a cast
> > drivers/dahdi/fxo_si3052.c:671: warning: passing argument 1 of ‘si3052_dma_buffers’ makes integer from pointer without a cast
> > drivers/dahdi/fxo_si3052.c:681: warning: passing argument 1 of ‘si3052_dma_buffers’ makes integer from pointer without a cast
> > drivers/dahdi/fxo_si3052.c: In function ‘si3052_probe’:
> > drivers/dahdi/fxo_si3052.c:1063: warning: passing argument 3 of ‘tasklet_init’ makes integer from pointer without a cast
> > drivers/dahdi/fxo_si3052.c:1072: warning: assignment makes integer from pointer without a cast
> > drivers/dahdi/fxo_si3052.c:1075: warning: assignment makes integer from pointer without a cast
> > drivers/dahdi/fxo_si3052.c:1078: warning: assignment makes integer from pointer without a cast
> >
> > If that cast is really needed (looks so, at first glance), make it explicit.
> >
> > Then there are the petty styling issues. I checked your patch with the checkpatch.pl script (see build_tools/kernel-cp). There are tons of errors. Almost all of them are "over 80 characters in a line". The rest are "suspect code indent for conditional statements" (8,12 instead of 8,16).
> >
> > Next I tried sparse (make C=1, with the package sparse installed. In Debian it is in non-free).
> >
> > drivers/dahdi/fxo_si3052.c:55:35: warning: incorrect type in argument 1 (different address spaces)
> > drivers/dahdi/fxo_si3052.c:55:35: expected void [noderef] <asn:2>*<noident>
> > drivers/dahdi/fxo_si3052.c:55:35: got void *
> > drivers/dahdi/fxo_si3052.c:60:36: warning: incorrect type in argument 2 (different address spaces)
> > drivers/dahdi/fxo_si3052.c:60:36: expected void [noderef] <asn:2>*<noident>
> > drivers/dahdi/fxo_si3052.c:60:36: got void *
> > drivers/dahdi/fxo_si3052.c:76:34: warning: incorrect type in argument 1 (different address spaces)
> > drivers/dahdi/fxo_si3052.c:681:22: got struct si3052_dev [usertype] *dev
> > drivers/dahdi/fxo_si3052.c:961:19: warning: incorrect type in assignment (different address spaces)
> > drivers/dahdi/fxo_si3052.c:961:19: expected void *[usertype] mmio_ptr
> > drivers/dahdi/fxo_si3052.c:961:19: got void [noderef] <asn:2>*
> > drivers/dahdi/fxo_si3052.c:1015:16: warning: incorrect type in argument 1 (different address spaces)
> > drivers/dahdi/fxo_si3052.c:1015:16: expected void volatile [noderef] <asn:2>*addr
> > drivers/dahdi/fxo_si3052.c:1015:16: got void *[usertype] mmio_ptr
> > drivers/dahdi/fxo_si3052.c:1063:64: warning: incorrect type in argument 3 (different base types)
> > drivers/dahdi/fxo_si3052.c:1063:64: expected unsigned long [unsigned] data
> > drivers/dahdi/fxo_si3052.c:1063:64: got struct si3052_dev [usertype] *[assigned] dev
> > drivers/dahdi/fxo_si3052.c:1072:28: warning: incorrect type in assignment (different base types)
> > drivers/dahdi/fxo_si3052.c:1072:28: expected unsigned long [unsigned] [usertype] data
> > drivers/dahdi/fxo_si3052.c:1072:28: got struct si3052_dev [usertype] *[assigned] dev
> > drivers/dahdi/fxo_si3052.c:1075:28: warning: incorrect type in assignment (different base types)
> > drivers/dahdi/fxo_si3052.c:1075:28: expected unsigned long [unsigned] [usertype] data
> > drivers/dahdi/fxo_si3052.c:1075:28: got struct si3052_dev [usertype] *[assigned] dev
> > drivers/dahdi/fxo_si3052.c:1078:29: warning: incorrect type in assignment (different base types)
> > drivers/dahdi/fxo_si3052.c:1078:29: expected unsigned long [unsigned] [usertype] data
> > drivers/dahdi/fxo_si3052.c:1078:29: got struct si3052_dev [usertype] *[assigned] dev
> > drivers/dahdi/fxo_si3052.c:1183:16: warning: incorrect type in argument 1 (different address spaces)
> > drivers/dahdi/fxo_si3052.c:1183:16: expected void volatile [noderef] <asn:2>*addr
> > drivers/dahdi/fxo_si3052.c:1183:16: got void *[usertype] mmio_ptr
> > drivers/dahdi/fxo_si3052.c:726:12: warning: context imbalance in 'si3052_irq': wrong count at exit
> > drivers/dahdi/fxo_si3052.c:726:12: context 'lock': wanted 0, got 1
> >
> >
> > Though honestly, regarding sparse, wcfxo and wctdm are not much better.
> >
> >
> > Oh, and it fails to build with kernel 2.6.18 (note: this is with stock 2.6.18, not with any later distro variant)
> >
> > drivers/dahdi/fxo_si3052.c: In function ‘si3052_irq’:
> > drivers/dahdi/fxo_si3052.c:660: warning: passing argument 1 of ‘si3052_dma_buffers’ makes integer from pointer without a cast
> > drivers/dahdi/fxo_si3052.c:290: note: expected ‘long unsigned int’ but argument is of type ‘struct si3052_dev *’
> > drivers/dahdi/fxo_si3052.c:671: warning: passing argument 1 of ‘si3052_dma_buffers’ makes integer from pointer without a cast
> > drivers/dahdi/fxo_si3052.c:290: note: expected ‘long unsigned int’ but argument is of type ‘struct si3052_dev *’
> > drivers/dahdi/fxo_si3052.c:681: warning: passing argument 1 of ‘si3052_dma_buffers’ makes integer from pointer without a cast
> > drivers/dahdi/fxo_si3052.c:290: note: expected ‘long unsigned int’ but argument is of type ‘struct si3052_dev *’
> > drivers/dahdi/fxo_si3052.c: In function ‘si3052_init’:
> > drivers/dahdi/fxo_si3052.c:1003: warning: passing argument 2 of ‘request_irq’ from incompatible pointer type
> > ‘irqreturn_t (*)(int, void *, struct pt_regs *)’ but argument is of type ‘irqreturn_t (*)(int, void *)’
> > drivers/dahdi/fxo_si3052.c: In function ‘si3052_probe’:
> > drivers/dahdi/fxo_si3052.c:1063: warning: passing argument 3 of ‘tasklet_init’ makes integer from pointer without a cast
> > d ‘long unsigned int’ but argument is of type ‘struct si3052_dev *’
> > drivers/dahdi/fxo_si3052.c:1068:51: error: macro "INIT_WORK" requires 3 arguments, but only 2 given
> > drivers/dahdi/fxo_si3052.c:1068: error: ‘INIT_WORK’ undeclared (first use in this function)
> > drivers/dahdi/fxo_si3052.c:1068: error: (Each undeclared identifier is reported only once
> > drivers/dahdi/fxo_si3052.c:1068: error: for each function it appears in.)
> > drivers/dahdi/fxo_si3052.c:1072: warning: assignment makes integer from pointer without a cast
> > drivers/dahdi/fxo_si3052.c:1075: warning: assignment makes integer from pointer without a cast
> > drivers/dahdi/fxo_si3052.c:1078: warning: assignment makes integer from pointer without a cast
> > drivers/dahdi/fxo_si3052.c:1131: warning: implicit declaration of function ‘cancel_work_sync’
> >
> > Note that this is stock 2.6.18 . I haven't yet tested it with distro kernels.
> >
>
> mhej wrote:
> > I checked your patch with the checkpatch.pl script (see build_tools/kernel-cp).
> > There are tons of errors. Almost all of them are "over 80 characters in a line".
> > The rest are "suspect code indent for conditional statements" (8,12 instead of 8,16).
>
> They are warnings not errors. I checked the code before with checkpatch.pl and fixed all issues reported as errors.
> Of course if that "over 80 characters in a line" are show-stoppers I can fix them.
>
> > drivers/dahdi/fxo_si3052.c:1068:51: error: macro "INIT_WORK" requires 3 arguments, but only 2 given
>
> INIT_WORK requires 2 arguments since 2.6.20 (see here: http://lxr.linux.no/linux+v2.6.20/include/linux/workqueue.h#L98 ).
> 2.6.18 is preety an ancient kernel. Is it a some kind of reference one for DAHDI?
>
> Will check sparse warnings later.
> Anyway, thank you for reviewing code!
>
Just to add to tzafrir's comments: I would go ahead and fix most of the warnings as well. While the value of editing code that is already part of the code base to "fix" checkpatch.pl warnings is arguably suspect, I think making it all compliant just makes things easier. Many of the warnings I see appear to be lines that aren't indented with tabs and the the warning about not defining new types (for struct si3052_dev) definitely appears warranted.
Also, DAHDI still ships packages for RHEL 4 (2.6.9 based) so yeah...2.6.18 support is required. If you just get it to build on 2.6.18, I'll sign up for any 2.6.9 work that may be needed.
So just a general comments (I'll wait for any updates before looking at things more in depth):
* I recommend removing the header file. Nothing is defined there that is used anywhere outside the *.c file.
* I recommend adding a comment about what act and act2 is in fxo_modes (I know there are other fields in that structure that could use a comment or two...).
* I recommend trying to make the user interface (in this case, the module_parameters) as consistent as possible. I.e. you have "region_param" instead of "opermode" in the wctdm.c, "min_ringer_time" instead of "ringdebounce" (or ring_debounce), and use_ulaw instead of "companding". Actually, on that last one http://svn.asterisk.org/view/dahdi?view=revision&revision=9101 started changing the idea of "ulaw/alawoverride" into just specifying a companding. It's a little more straightforward. Also, it would seem that all the FXO modules should be able to make a good guess as to the default companding it should use based on the current oper_mode...but I digress....
- Shaun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/977/#review2844
-----------------------------------------------------------
On 2010-10-14 16:30:27, mhej wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/977/
> -----------------------------------------------------------
>
> (Updated 2010-10-14 16:30:27)
>
>
> Review request for Asterisk Developers.
>
>
> Summary
> -------
>
> Attached here is a driver for single-port FXO cards based on Si3052 chip + Si3011/17/18 DAA. They are marketed as Motorola 52-6116-2A, PM560MS and many others.
> Driver is written from scratch to take advantage of international features some of the DAAs have. There is also patch for wcfxo module to support these cards circulating on Net, but it runs the card in backwards-compatible mode and doesn't allow most of the POTS settings to be tweaked.
>
> Usage:
> * DAHDI-tools need a simple patch from bug https://issues.asterisk.org/view.php?id=18138 (dahdi-gencfg.patch) for dahdi_genconf to recognize new driver.
> * You have to set the region name to one of those listed in fxo_modes.h file (this file is also used for wctdm driver, so region names are the same). For example: "modprobe fxo_si3052 region_param=ARGENTINA" (w/o quotes) would load the module with proper settings for Argentina. Also add "use_ulaw=1" if you plan to use ULAW instead of ALAW (default setting is ALAW).
> * Generate config file for the card with dahdi_genconf, configure with dahdi_cfg, write config files for Asterisk, all just like with the wcfxo driver.
>
> If CallerID detection doesn't work the problem might be with too long minimum ringer time. Default is 512ms, but sometimes first ring signal is shorter and is not detected as ring, so CallerID detection is not started by Asterisk. In this case check ringer length and add appropriate "min_ringer_time" parameter (for example "min_ringer_time=150" for 150ms). Valid values are 100, 150, 200, 256, 384, 512 and 1024 ms.
>
> Comments and suggestions are welcome.
>
>
> This addresses bug 18138.
> http://issues.asterisk.org/view.php?id=18138
>
>
> Diffs
> -----
>
> /linux/trunk/drivers/dahdi/Kbuild 9425
> /linux/trunk/drivers/dahdi/fxo_modes.h 9425
> /linux/trunk/drivers/dahdi/fxo_si3052.h PRE-CREATION
> /linux/trunk/drivers/dahdi/fxo_si3052.c PRE-CREATION
>
> Diff: https://reviewboard.asterisk.org/r/977/diff
>
>
> Testing
> -------
>
> Tested features:
> * Sending and receiving voice,
> * Ringer detection and validation,
> * DTMF detection,
> * Caller ID reception (only ETSI FSK mode tested, but different ones should work too in principle),
> * ULAW/ALAW mode,
> * Busy pattern hangup detecion,
> * Line disconnection detection (only when offhook due to hardware limitation).
> * No errors were indicated by checkpatch.pl.
>
>
> Thanks,
>
> mhej
>
>
More information about the asterisk-dev
mailing list