Subject: Add PWM driver for beagle bone black

[Martin Galvan]

On Wed, Jun 22, 2016 at 12:32 AM, punit vara <punitvara at gmail.com> wrote:
> Thank you very much for detailed review. I will do each changes you
> suggested. Though I have some specific doubt I would like to ask as
> following.

Sure. We were discussing it with Ben and agreed that not all these
changes are *required*, e.g. we won't be blocking the merge for
something like whitespace. The changes that you should definitely work
on are:

- Improving the API (I think you discussed that with Ben previously?).
- Removing unneeded files (tests and so on).
- Moving changes away from shared/include/gpio.h.
- Reworking EPWMPinMuxSetup so that it will reconfigure only a
requested pin, thus not breaking any existing user configurations.

The rest we can leave for after the midterm evaluation.

> On Wed, Jun 22, 2016 at 1:07 AM, Martin Galvan
> <martin.galvan at tallertechnologies.com> wrote:
>> Hi Punit, thanks for sending this. If I understood correctly this is the BBBIO code
>> plus some changes of your own, right? If so, I think it would be best to send a patch
>> with the BBBIO code as is, and then another with your changes on top of it. I think
>> that was what we were going for with StarterWare, too.
>>
>
> I would like to confirm it once again. Should I send a patch even if
> it breaks the build right ?

Yeah. We'll be applying both patches as separate commits and pushing
them at the same time, so users shouldn't see their builds broken
unless they're intentionally backtracking to the first commit. It'd be
great if you could do that neat git thing where you send many patches
in a single thread, like [PATCH 1/2], [PATCH 2/2] and so on.

> Right now I am not using this BASE_CLOCK but IMHO we should keep it
> here so that in future maintainer can know which system clock should
> be used for BBB. Sure I will link it to TRM as well.

I see. In that case, IMHO a comment (or README) is better than an
unused macro. You don't have to do it now, but please document
SYSCLKOUT when you start refactoring PWMSS_Setting.

>> Is this function changing all the muxed pins to PWM, or something like that?
>> That's wrong; we should only touch the pins that the user requests, otherwise we'll
>> be breaking any configurations he may have. Also, the arguments to BBB_MUXMODE shouldn't
>> be magic numbers.
>
> Yes you are absolutely right. Yet I have a doubt. Should we provide
> access to individual pins or should we divide pins into groups like
> pins for PWM0 , PWM1, PWM2 ?

That's a good question. Does a given channel have multiple pins that
need to be configured? If so, then yeah, I think we should definitely
group them somehow.

Perhaps we could have a struct representing a PWM channel (I think you
discussed that with Ben before?), and it could have some info on which
pins correspond to it. That would also help with improving pwm_init.
If you don't feel like doing that right now, though, you could just
use an if-else chain that would take a channel number (possibly an
enum) and configure the appropriate pins.

> Yes I will document everything in detailed. All documentation should I
> do in RTEMS wiki, or README ?

I personally like documentation as either a README or at least as a
comment. A README in the pwm directory would be nice.

> Does RTEMS has any debug mode like
>
> https://github.com/VegetableAvenger/BBBIOlib/blob/master/BBBio_lib/BBBiolib_PWMSS.c#L112
>
> so that printf can be only enabled in debug mode. IMHO printf can be
> handy while debugging if there is any kind of debugging mode is
> available.

Good question. I honestly don't know :) maybe the other guys do?

>>> +       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};
>
> Commenting dividers as it is in TRM would be beneficial. What do you suggest ?

Sure thing. You could point out what those values mean, and which
section of the TRM describes them.

> This is to check for which module clock is enabled. This helped me a
> lot during debugging. Debugging functions should not be included right
> ?

Yeah, it's a pretty simple function, so a maintainer could reasonably
be expected to come up with something similar on their own if they
needed to.

>> Please avoid adding BSP-specific code to shared headers.
> Where should I add declarations of functions that I defined in bbb-pwm.c ?

Do we really need to export all of those functions? The ones you use
internally could be marked as static. The ones you want to expose to
the user should go in a pwm.h file within beagle/include. If you look
at that directory you'll notice similar files for other modules.