[PATCH 2/2] Subject: Update PWM driver imported from BBBIO

[Martin Galvan]

Thanks for the patch. FYI we usually tag patches as [PATCH v2], [PATCH
v3] and so on for patch versions greater than 1.

One thing I've noticed (and that I missed for v1) is that in many
places you have things like if((pwmid<3) & (pwmid >=0)), using bitwise
& instead of logical &&. Your tests must've worked because you only
tried valid PWM IDs; do some negative testing as well to cover all the
cases.

A few more comments below.

On Thu, Jun 30, 2016 at 8:58 AM, Punit Vara <punitvara at gmail.com> wrote:
> +
> +       default:
> +               printf("PWM output is not available on this pin\n");
> +               break;
> +}
> +return true;

Looks like you're always returning true, even if an error occurred.

>  /* PWMSS setting
> - *     set pluse rgument of epwm module
> + *      set pulse argument of epwm module
>   *
> - *      @param PWMID    : EPWMSS number , 0~2
> - *      @param HZ      : pluse HZ
> + *      @param pwm_id    : EPWMSS number , 0~2
> + *      @param pwm_freq : frequency to be generated
>   *      @param dutyA    : Duty Cycle in ePWM A
>   *      @param dutyB    : Duty Cycle in ePWM B
>   *
>   *      @return         : 1 for success , 0 for failed
>   *
> - *      @example        :  BBBIO_PWMSS_Setting(0 , 50.0f , 50.0f , 25.0f);     // Generate 50HZ pwm in PWM0 ,
> - *                                                                             // duty cycle is 50% for ePWM0A , 25% for ePWM0B
> + *      @example        :  PWMSS_Setting(0 , 50.0f , 50.0f , 25.0f);      // Generate 50HZ pwm in PWM0 ,
> + *                                                                              // duty cycle is 50% for ePWM0A , 25% for ePWM0B
>   *
> - *     @Note :
> - *             find an number nearst 65535 for TBPRD , to improve duty precision,
> + *      @Note :
> + *              find an number nearst 65535 for TBPRD , to improve duty precision,
>   *
> - *             Using big TBPRD can increase the range of CMPA and CMPB ,
> - *             and it means we can get better precision on duty cycle.
> + *              Using big TBPRD can increase the range of CMPA and CMPB ,
> + *              and it means we can get better precision on duty cycle.
>   *
> - *             EX : 20.25% duty cycle
> + *              EX : 20.25% duty cycle
>   *                  on TBPRD = 62500 , CMPA = 12656.25 ( .25 rejection) , real duty : 20.2496% (12656 /62500)
>   *                  on TBPRD = 6250  , CMPA = 1265.625 ( .625 rejection), real duty : 20.24%   (1265 6250)
>   *                  on TBPRD = 500   , CMPA = 101.25   ( .25 rejection) , real duty : 20.2%    (101/500)
>   *
> - *             Divisor = CLKDIV * HSPCLKDIV
> - *                             1 TBPRD : 10 ns (default)
> - *                     65535 TBPRD : 655350 ns
> - *                     65535 TBPRD : 655350 * Divisor ns  = X TBPRD : Cyclens
> + *              Divisor = CLKDIV * HSPCLKDIV
> + *                      1 TBPRD : 10 ns (default)
> + *                      65535 TBPRD : 655350 ns
> + *                      65535 TBPRD : 655350 * Divisor ns  = X TBPRD : Cyclens
>   *
> - *             accrooding to that , we must find a Divisor value , let X nearest 65535 .
> - *             so , Divisor must  Nearest Cyclens/655350
> -*/

No need to repeat this comment here if you already placed it above the
function declaration.

> +int beagle_pwmss_setting(uint32_t pwm_id, float pwm_freq, float dutyA, float dutyB)
> +{
> +  uint32_t baseAddr;
> +  int param_error =1;
> +  if(pwm_freq < 0)
> +       param_error =0;
> +  if(dutyA < 0.0f || dutyA > 100.0f || dutyB < 0.0f || dutyB > 100.0f)
> +       param_error = 0;
> +  if(param_error == 0) {
> +       printf("ERROR in parameter \n");
> +  }

You could use this param_error to actually return a status code, which
the function already does at a couple of places.