All, I built & tested the change based on current master. Looks great, ragunath! I agree it's worth addressing Joel's comments. I have two more:
- please check in the device read/write functions that you're getting the 'minor' argument that you expect (0), and not return valid data otherwise. - git reports some spurious white space can you address these & resubmit? I'll be happy to merge then. Thanks for your contribution, very good companion to your GSOC proposal! On Wed, Apr 1, 2015 at 7:48 PM, Joel Sherrill <joel.sherr...@oarcorp.com> wrote: > I noticed this wasn't merged. > > Ben.. ? > > some comments interspersed. > > On 3/10/2015 4:21 PM, ragu nath wrote: >> Hi All, >> >> I was exploring the beagle code base to find a task to get hands on >> experience with RTEMS internals. I found that "Real time Clock (RTC)" >> support is >> missing for beaglebone. I made the effort to add the driver for the device. >> Included the patch for the driver. RTEMS already have a RTC framework. >> I defined the necessary structures to hook the driver to the framework. >> >> Enabled the driver by defining the macro >> "CONFIGURE_APPLICATION_NEEDS_RTC_DRIVER" and tested the following. >> Testing is done from the shell using "date" and "rtc" commands. >> 1. Able to read the time from rtc. >> 2. Able to set the time >> 3. Time values are preserved between reboots >> >> Pls review the patch and let me know if you have any comments. >> >> I have following doubts >> >> 1. The support is applicable only to beaglebone black. It is not valid >> for beagleboard. Both of them seem to have same codebase. Is there any >> compile >> time flags available to not compile the driver for beagleboard? > > Look in the configure.ac in the BSP. There is already a cpp define generated > based on the board variant for DM3730 or AM335x. Do you need more than > that? I am not an expert on the differences between the various models so > this is just a question. > > If the RTC has to be ignored completely for some variants, the configure.ac > logic can be enhanced to generate a conditional the Makefile.am can use > to conditionally build some files. > > >> 2. What is procedure to commit the change (In case the patch is >> accepted)? Do we need to open a bug to track the changes? > This normally would have been more than enough. But pinging if you don't > get a response in a reasonable length of time. > > The patch didn't get included in my response so here are some odd comments. > > + "disabl" check spelling. > + "Steps to enable RTC" should have "*" at front of each line in comment > block > + spaces around equal signs > + Don't print from driver > + I see an unneeded blank line before closing } in some methods > + Put ";" on busy spin loops on a separate line so it is obvious > - if there is a known number of maximum spins, it should not > lock up. > - the spins do look the same so moving them to a subroutine > and adding a maximum spin count would be a nice way to > make the code more robust and clearer. > + Rather than RTEMS_DEBUG, may want to use a BB_DEBUG > or similar for debug code. > + Watch columns lining up. RTC_Table comments don't line up > and value column in register macros in .h don't either. > + >> Thanks, >> Ragunath > > -- > Joel Sherrill, Ph.D. Director of Research & Development > joel.sherr...@oarcorp.com On-Line Applications Research > Ask me about RTEMS: a free RTOS Huntsville AL 35805 > Support Available (256) 722-9985 > > _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel