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

Thu Nov 4 09:18:31 CDT 2010

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/977/#review2882
-----------------------------------------------------------

Besides the comments below I'll ask that you combine the fxo_si3052.h into the fxo_si3052.c file.  Most modern editors make splitting your window (or opening up multiple copies of the file) possible if you want to see the definitions along with where they are used.  Keeping them all in the same file makes it *explicit* that the definitions aren't used outside of that file which eases any future maintenance.

/linux/trunk/drivers/dahdi/fxo_si3052.h
<https://reviewboard.asterisk.org/r/977/#comment6087>

    dev_dbg will give you compile time debug print options....

/linux/trunk/drivers/dahdi/fxo_si3052.c
<https://reviewboard.asterisk.org/r/977/#comment6086>

    I recommend using something like "companding" here instead.  I'm trying to migrate the drivers to that form so it's consistent as oppposed to alawoverride/t1e1override/use_ulaw, etc..  companding and a string makes it explicit.

/linux/trunk/drivers/dahdi/fxo_si3052.c
<https://reviewboard.asterisk.org/r/977/#comment6081>

    There are mixed tabs / spaces for the indentation style here.  while is indented with 4 spaces, "unsigned int" is intended with a tab.

    I would recommend just making sure that checkpatch.pl reports 0 errors or warnings.  It will just eliminate any debate unless you have a compelling reason for deviating from those recommendations.

/linux/trunk/drivers/dahdi/fxo_si3052.c
<https://reviewboard.asterisk.org/r/977/#comment6088>

    dead code?  if si3052_open and si3052_close need to be here in a commented out form, perhaps a comment why, or just remove them if they don't need to be here.

/linux/trunk/drivers/dahdi/fxo_si3052.c
<https://reviewboard.asterisk.org/r/977/#comment6089>

    Using num_devices like this is not safe for device numbering.  1) nothing says that si3052_probe can't be called in parallel for multiple devies (that I'm aware of) and 2) say you have two cards and the first one is unbound at runtime and then rebound.  You would in this case have two cards with devno == 1.

/linux/trunk/drivers/dahdi/fxo_si3052.c
<https://reviewboard.asterisk.org/r/977/#comment6085>

    Consider removing dead code here, or put a comment/conditional about why you want it to remain in.

/linux/trunk/drivers/dahdi/fxo_si3052.c
<https://reviewboard.asterisk.org/r/977/#comment6084>

    ARRAY_SIZE(fxo_modes) is preferred...

/linux/trunk/drivers/dahdi/fxo_si3052.c
<https://reviewboard.asterisk.org/r/977/#comment6083>

    ARRAY_SIZE(fxo_modes) here too.

/linux/trunk/include/dahdi/kernel.h
<https://reviewboard.asterisk.org/r/977/#comment6082>

    Looks like a typo here:  flush_scheduled_work doesn't have x passed to it.

- Shaun

On 2010-10-22 14:23:18, mhej wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/977/
> -----------------------------------------------------------
> 
> (Updated 2010-10-22 14:23:18)
> 
> 
> 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 9455 
>   /linux/trunk/drivers/dahdi/fxo_modes.h 9455 
>   /linux/trunk/drivers/dahdi/fxo_si3052.h PRE-CREATION 
>   /linux/trunk/drivers/dahdi/fxo_si3052.c PRE-CREATION 
>   /linux/trunk/include/dahdi/kernel.h 9455 
> 
> 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
> 
>