Kevin, Ack on both suggestions. My next patch should take care of the redundant comments. For the tick I was thinking of replacing the whole EMAC MDIO code with linux mdio infrastructure. With that done the timer in the driver will not be required. If I take too much time to get the MDIO removal done then I will reduce the timer tick to about 10 ms or so and send a patch.
Regards, Anant -----Original Message----- From: Kevin Hilman [mailto:[EMAIL PROTECTED] Sent: Tuesday, June 24, 2008 3:04 AM To: Gole, Anant Cc: [email protected]; [EMAIL PROTECTED] Subject: Re: [Patch 2/2] Net:DaVinci: EMAC driver recoded - cleaned up unused code, coded as per linux conventions Kevin Hilman <[EMAIL PROTECTED]> writes: > "Gole, Anant" <[EMAIL PROTECTED]> writes: > >> This patch does the following: >> - cleanup of EMAC driver code - followed linux coding guidelines >> - removed unused code >> - removed register access, using davinci register access macros >> >> This is a large patch but I preferred to cleanup the whole driver >> rather than bits and pieces of it. Attached a compressed >> patch as I could not send it inline or as plain attachment due >> to size limitations on this mailing list. >> >> Signed-off-by: Anant Gole <[EMAIL PROTECTED]> > > Thanks, pushing today. > > One other minor thing for the next round of cleanups... > > Most of the comments for the fields in the various structs are > redundant. IOW, the struct field name is as descriptive as the > comment. I think you can get rid of most of the comments there as > they don't add much value. Hi Anant, Another thing that I had forgotten about, but just remembered... There is a timer in this driver configured to go off every 1ms. On a normal tick-based system, this will only go off every jiffies, but on a dynamic tick system (which is now the default .config) this timer indeed fires every 1ms. Using CONFIG_TIMER_STATS=y and a simple test[1], you can see that the emac_timer_cb timer is waking up very often. This timer needs to be reworked to be much less often. I'm not sure exactly what this timer is for, so it's up to you to decide how often. Remeber that any timer events can take the system out of idle, thus taking away any power savings that can be had in idle. Kevin [1] On an idle system, simply run the following: # echo 1 > /proc/timer_stats # sleep 10 # echo 0 > /proc/timer_stats # cat /proc/timer_stats _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
