PWM driver tested in RTEMS with RGB

[Martin Galvan]

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 <punitvara at 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!