On 03/10/2014 03:57 PM, Courtney Cavin wrote:
On Mon, Mar 10, 2014 at 11:45:50PM +0100, Courtney Cavin wrote:
On Mon, Mar 10, 2014 at 11:33:06PM +0100, Christopher Heiny wrote:
On 03/10/2014 07:46 AM, Courtney Cavin wrote:
On Sat, Mar 08, 2014 at 03:29:51AM +0100, Christopher Heiny wrote:
Signed-off-by: Christopher Heiny <cheiny@xxxxxxxxxxxxx>
Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
Cc: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
Cc: Linux Walleij <linus.walleij@xxxxxxxxxx>
Cc: David Herrmann <dh.herrmann@xxxxxxxxx>
Cc: Jiri Kosina <jkosina@xxxxxxx>
---
drivers/input/rmi4/rmi_f01.c | 96 ++-----------------------------------
drivers/input/rmi4/rmi_f01.h | 110 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 114 insertions(+), 92 deletions(-)
[...]
I might be missing something, but these seem like the only defines used
in the flash code. Why not keep these in the f01 driver, and export
a couple more functions, like rmi_f01_reset() and rmi_f01_set_sleep_mode() ?
It seems better to me to have the information defined in a single place,
rather than scattered hither and yon through the source files.
Uh. Exactly? This is why I'm suggesting that you keep this information
isolated in the driver to which is directly related.
Perhaps what you mean is that the regs/bits for the entire chip
functionality should be exposed in header files, so one can read/write
it from anywhere? That seems backwards to the idea of separating these
'functions' out into drivers.
Ah. Wait. I think there was some mis-communication on my part. What I
should have said: Why not keep all of the defines in the driver, and
export a couple more functions?
My point is exactly yours. Keep the defines with the code. Expose
what's needed.
I don't see any win to that approach. For example, you can hide the
sleep mode mask and the nosleep bit #defines in rmi_f01.c, but you've
got to add a new function with either a tri-state field for the nosleep
bit (set it, clear it, don't change it) which will require an additional
3 #defines and the new sleep mode which still needs the sleep mode
#defines (plus an additional one to indicate that you want to leave the
sleep mode as it was), or 3 new functions, once of which changes the
sleep mode, one of which sets/clears the nosleep bit, and one of which
does both. In either case, you still need the sleep mode #defines, so
some of the related stuff is in the header and some of it is in the .c
file. Now you wind up with more stuff in the .h file (at least twice as
much as you've removed) and you no longer have one stop shopping for the
F01_Ctrl0 #defines.
BTW - thanks very much for all of today's input. Even though we don't
necessarily agree with all of it, it's been helpful to go back and ask
"did we make the right decision?".
Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
