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.gal...@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 <punitv...@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.
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
