On 2 June 2014 21:31, Kevin Hilman khilman@linaro.org wrote:
You missed some previous comments.
That's very bad :(
Note that the early 'return' changes the *functional* behavior of this code. e.g. it used to do 2 writes to this register, but now it does one. Assuming this is well-behaved hardware, it's probably fine but we just don't know.
I had the impression that you meant that only for the currently available modes and not for 'default case' in general and so kept it that way..
As I mentioned in an earlier review, to preserve the *behavior* of the current code used to disable timers, you should not mess with any of the register accesses.
Instead, add an 'int ret = 0' at the beginning, 'ret = -ENOSYS' in the default case and 'return ret' at the end.
This not only makes the code easier to review for those who are not intimately familiar with each of these clockevent devices, but it's much more obvious on inspection that you are not changing the *behavior* of the code.
The same problem exists on several of the following changes, so I stopped reviewing after I saw 3-4 of them.
Will keep behavior of 'default case' as is .. Will try to send a V5 soon, if that's the only complaint you have.