Hi Punit! Sorry for the delay; I finally got to review your code. First and foremost, it'd be great if you could tell us which StarterWare version/git commit are you using, so that we can keep it handy when reviewing your code. Sorry if you already mentioned it, my memory is a bit sketchy these days :)
As a general comment I'd say you should keep a consistent coding style throughout the code you write yourself. While the core RTEMS style (spaces after parentheses, etc) isn't required in BSP code, at least indentations should be correct. Indentations in RTEMS are usually two spaces. More on coding style later. On Fri, Apr 15, 2016 at 4:48 PM, punit vara <punitv...@gmail.com> wrote: > This is my first patch I already sent you when I successfully merged TI SW > code. > > https://github.com/punitvara/rtems_gsoc16/tree/master/pwm/patch I take it you're referring to 0001-add-new-pwm-driver.patch. A few comments: 1) Whenever possible, try to keep the original coding style for imported code. This makes it easier to track changes and such. Same goes for e.g. the order in which functions are declared (e.g. EHRPWMConfigureAQActionOnA and B). 2) I think you should add the TI license to bbb-pwm.c as well. It would also be really nice if you added a comment atop the imported files saying which SW version/git commit they come from. 3) Are those REG* macros in the original SW code? I recall having seen them defined in RTEMS, while SW used HWREG*. If they're from RTEMS, please generate a patch that adds the vanilla SW code, then a second one with your changes to adapt it to RTEMS like Gedare suggested. I saw you already did that with SOC_CONTROL_REGS -> AM335X_PADCONF_BASE. 4) I saw that on bbb-pwm.c you placed functions from different SW files (e.g. PWMSSModuleClkConfig comes from platform/evmAM335x/pwmss.c while EHRPWMTimebaseClkConfig comes from drivers/ehrpwm.c). While I agree that it's not necessary to maintain the exact same file structure from SW, it'd be nice if you grouped each set of functions from a SW file together and added a comment atop saying something like /* These functions come from drivers/ehrpwm.c in TI StarterWare */. However that's just my opinion; perhaps there's a better way to do this (Gedare?). 5) I'm seeing your patch includes changes to a couple of M4 files. This probably comes from a bootstrap, and should be kept off your final patch. > 2. Second patch which shows I added useful registers from TI SW header files > > https://github.com/punitvara/rtems_gsoc16/blob/master/pwm/patch/0002-add-register-for-pwm-driver.patch > > After above two patches I created testsuite > > https://github.com/punitvara/rtems_gsoc16/blob/master/pwm/fail.c >From what I'm seeing, your example comes from examples/evmAM335x/ehrpwm_haptics/ehrpwm_haptics.c, which drives a Pico-Haptics 304-100 motor that's connected to the B-Channel of the PWMSS2 of the AM335x EVM. I've taken a look at your version vs the SW example, and while I can't immediately point out what's wrong I suspect there may be some register magic your code is missing, or perhaps some of the values aren't being set correctly. In any case, double-check the SW example with the AM335x Technical Reference Manual; don't rule out bugs in SW itself. > In this testsuite, LED is constantly glowing as I told. No matter what > counter I set or frequency set I am not able to get expected output. > > 3. Then I added three new function that can be seen in below patch.TI > Copy right is also added as Gedare Bloom suggested me. > > https://github.com/punitvara/rtems_gsoc16/blob/master/pwm/patch/0001-PWM-successful-code-added.patch Don't forget to mark the code changes with #if defined(__rtems__) like Gedare suggested. > You should look these definition in patch. I wrote this after first > failed testsuite. > > - PWMSS_Setting > - ehrPWM_Enable > - ehrPWM_Disable > - EPWMPinMuxSetup This last function I also used in first test case. If you are going to keep those functions I think you should improve the coding style a bit (e.g. name them using CamelCase as the rest of SW code, avoid magic numbers, etc). I think for now we should focus on making your PWM example work using the SW APIs. If you realize it definitely won't work because the API has a bug, you should patch the API. All in all, good work! _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel