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

[punit vara]

Thank you very much for detailed review of patch. I have done most of
the changes as you said here.

On Sat, Jul 2, 2016 at 2:42 AM, Martin Galvan
<martin.galvan at tallertechnologies.com> wrote:
> Hi Punit, thanks for the patch. As we spoke in the group chat with the other mentors, this seems to
> be good for a first version. I'll be pointing out the required changes for the next version, but
> unless somebody sees anything blocking this should be good to merge.
>
> Besides improving the code itself, my intention here is for you to learn from having mistakes pointed out.

Yes It is great learning experience for me Martin :)

> On Fri, Jul 1, 2016 at 12:28 PM, Punit Vara <punitvara at gmail.com> wrote:
>  c/src/lib/libbsp/arm/beagle/Makefile.am       |   4 +
>  c/src/lib/libbsp/arm/beagle/include/bbb-pwm.h | 162 +++++-
>
> I think we should follow a naming convention for pwm.c/h. I suggested renaming bbb-pwm.h to just
> pwm.h, but I think someone else said to keep it this way?

Once I asked Ben and he told me to rename it bbb-pwm.h

>  libbsp_a_SOURCES += gpio/bbb-gpio.c
> +#pwm
>
> Whenever possible, try to follow the style of the file you're editing. For example,
> here you should add a blank line between the comment and the previous line. Also, "#pwm" -> "# PWM".

I renamed it and I already have black line between comment and the
previous line I don't know why it does not appear in patch.

> +#define BBB_CONTROL_CONF_GPMC_AD(n)   (0x800 + (n * 4))
> +#define BBB_CONTROL_CONF_LCD_DATA(n)   (0x8a0 + (n * 4))
>
> As Ketul suggested, it would be good if you did a quick check for which macros are processor-specific,
> and named them AM335X_* instead of BBB_*.

I have renamed when Ketul suggested so this patch is according to
that. Above Macros are pinmux setting according to BBB that's why it
is BBB specific IMHO.

> As we said before, there should be a prominent comment here explaining how this works,
> and why the TI StarterWare code isn't as accurate as this. It would also be nice if
> we referred to the manual and the Google groups link explaining where the value for
> SYSCLKOUT comes from, as we discussed in the previous patch. You could even go further
> and write a README in the pwm directory with all we've learned. That would allow for
> a shorter comment here. The README could also include a sample code of how to use this API.

I will add this in README as we don't directly use SYSCLKOUT in BBBIO code.

> Additionally, I insist in that we should have a naming convention for the PWM functions.
> Sometimes we use "pwm", others "pwmss", and others "ehrpwm". You should decide on which
> one is better and follow that convention on the rest of the functions.


> +static uint32_t select_pwmss(uint32_t pwm_id)
>  {
> +uint32_t baseAddr=0;
> +   if (pwm_id == BBB_PWMSS0)
> +   {
> +       baseAddr = AM335X_EPWM_0_REGS;
> +       return baseAddr;
> +   }
> +   else if (pwm_id == BBB_PWMSS1)
> +   {
> +       baseAddr = AM335X_EPWM_1_REGS;
> +       return baseAddr;
> +   }
> +   else if (pwm_id == BBB_PWMSS2)
> +   {
> +       baseAddr = AM335X_EPWM_2_REGS;
> +       return baseAddr;
> +   }
> +   else
> +   {
> +       printf("Invalid PWM Id\n");
>
> As we said before, you should look into some way to mark these printfs as debug-only, or remove them
> altogether.

I will remove printfs as for now and return false instead of just description.

> +static void module_clk_config(uint32_t pwmss_id)
>  {
> +        if(pwmss_id == 0)
> +        {
> +                REG(AM335X_CM_PER_ADDR + AM335X_CM_PER_EPWMSS0_CLKCTRL) |=
> +                        AM335X_CM_PER_EPWMSS0_CLKCTRL_MODULEMODE_ENABLE;
> +
> +        while(AM335X_CM_PER_EPWMSS0_CLKCTRL_MODULEMODE_ENABLE !=
> +              (REG(AM335X_CM_PER_ADDR + AM335X_CM_PER_EPWMSS0_CLKCTRL) &
> +               AM335X_CM_PER_EPWMSS0_CLKCTRL_MODULEMODE));
> +
> +        while((AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST_FUNC <<
> +               AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST_SHIFT) !=
> +              (REG(AM335X_CM_PER_ADDR + AM335X_CM_PER_EPWMSS0_CLKCTRL) &
> +               AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST));
>
> This is all a bit hard to read. Perhaps you could use variables to ease reading? E.g.
>
> const uint32_t is_idle = AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST_FUNC <<
>                          AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST_SHIFT;
> const uint32_t clkctrl = AM335X_CM_PER_ADDR + AM335X_CM_PER_EPWMSS0_CLKCTRL;
> const uint32_t idle_bits = AM335X_CM_PER_EPWMSS0_CLKCTRL_IDLEST;
>
> while ((REG(clkctrl) & idle_bits) != is_idle) {
>   /* Keep waiting */
> }
>
> and so on.
>
> (Quiz: why did I need to wrap the bitwise & in parens? The answer will surprise you.
> It certainly surprised me and a coworker back when we were working on the BBB interrupt
> handler and the damn thing would keep hanging).

I would like to know this solution as everything looks fine to me :)

> +int beagle_pwmss_setting(uint32_t pwm_id, float pwm_freq, float dutyA, float dutyB)
>
> This function needs a bit more work than the others. I'd start by specifying what dutyA and dutyB mean;
> and perhaps renaming them to something more descriptive (if possible).

dutyA represents duty cycle for channel A and dutyB represents duty
cycle for channel B. Any suggestions here ?

> +  /*Compute necessary TBPRD*/
> +  float Cyclens = 0.0f;
>
> "Cyclens" isn't a good name; consider renaming this.
This is actually cycles. It will be renamed as cycles.

> +  float Divisor =0;
> +  int i,j;
>
> These can be unsigned.
>
> +  const float CLKDIV_div[] = {1.0,2.0,4.0,8.0,16.0,32.0,64.0,128.0};
> +  const float HSPCLKDIV_div[] = {1.0, 2.0, 4.0, 6.0, 8.0, 10.0,12.0, 14.0};
> There really needs to be a description of how these work, either in a comment or the README.

I will add in README as this function is already large adding long
comments in between will not be easy reading of function.

> +  /*setting clock divider and freeze time base*/
> +  REG16(baseAddr + AM335X_EPWM_CMPB) = (unsigned short)((float)(NearTBPRD) * dutyB);
> +  REG16(baseAddr + AM335X_EPWM_CMPA) = (unsigned short)((float)(NearTBPRD) * dutyA);
> +  REG16(baseAddr + AM335X_EPWM_TBPRD) = (unsigned short)NearTBPRD;
> +  REG16(baseAddr + AM335X_EPWM_TBCNT) = 0;

TBPRD  is 32 bit value and CMPB is 16 bit so We have to use typecasting here.