Re: [RFC PATCH 10/11] input: RMI4 F19 - capacitive buttons
From: Christopher Heiny
Date: Tue Jan 10 2012 - 15:13:19 EST
On 01/05/2012 06:50 PM, Dmitry Torokhov wrote:
On Thu, Jan 05, 2012 at 04:05:43PM -0800, Christopher Heiny wrote:
On 01/04/2012 11:53 PM, Dmitry Torokhov wrote:
Hi Christopher,
On Wed, Dec 21, 2011 at 06:10:01PM -0800, Christopher Heiny wrote:
+
+struct f19_0d_control {
+ struct f19_0d_control_0 *general_control;
Single instance, does not need to be allocated separately.
+ struct f19_0d_control_1 *button_int_enable;
+ struct f19_0d_control_2 *single_button_participation;
+ struct f19_0d_control_3_4 *sensor_map;
This should probably be an array of
struct f19_button_ctrl {
struct f19_0d_control_1 int_enable;
struct f19_0d_control_2 participation;
struct f19_0d_control_3_4 sensor_map;
};
located at the end of f19_0d_control structure so it can be all
allocated in one shot.
We organized it this way because of how the controls are organized
in the register map: first the interrupt enables for all buttons,
then the participation for all buttons, and then the sensor map for
all buttons. Typical client interactions are to adjust these in an
"all at once" approach - that is, change as a single group all the
interrupt enables, all the participation settings, or all the sensor
map. By organizing them the way we did, it makes it very easy to
read/write the data to the RMI4 sensor's register map. Using an
array of structs would require a building buffers (on write) or
tedious extraction from buffers (on read).
However, the first two of these are bitmasks, and don't really need
their own structs - they can conveniently be u8 * instead.
I'll try coding something to show that it is not as bad as it seems...
OK, we'll look forward to that.
[snip]
+
+static struct device_attribute attrs[] = {
+ __ATTR(button_count, RMI_RO_ATTR,
+ rmi_f19_button_count_show, rmi_store_error),
Why not NULL instead of rmi_store_error?
We found that customers picking up our driver would try to write RO
sysfs attributes (or read WO attrs) by invoking echo from the
command line. The operation would fail silently (I'm looking at
you, Android shell), leaving the engineer baffled as to why the
sensor behavior didn't change. So we adopted this approach so as to
give some clue as to the fact that the operation failed.
But this ends up in various logs (dmesg, /var/log/messages, etc) making
it potentially DOS scenario. Please help fixing tools instead.
I see your point about the DOS scenario. How about we do this: make the
rmi_store/show_error routines a #define, that normally is set to NULL,
but have an optional debug flag (RMI4_SYSFS_DEBUG) that uses these
verbose functions.
+
+ f19->button_control->single_button_participation =
+ kzalloc(f19->button_data_buffer_size *
+ sizeof(struct f19_0d_control_2), GFP_KERNEL);
+ if (!f19->button_control->single_button_participation) {
+ dev_err(&fc->dev, "Failed to allocate"
+ " f19_0d_control_2.\n");
Do not split error messages; it's OK if they exceed 80 column limit.
We have one customer who refuses to accept any code if any line
exceeds 80 columns, so we wind up with ugliness like this.
This is contrary to current kernel policy though
(Documentation/CodingStyle, ch 2):
"However, never break user-visible strings such as
printk messages, because that breaks the ability to grep for them."
Yes, but then checkpatch.pl whines about them, which is also
unacceptable to that customer. However, at this point I think
usefulness takes priority, so we'll unbreak those strings.
[snip]
+
+ /* Generate events for buttons that change state. */
+ for (button = 0; button< f19->button_count;
+ button++) {
+ int button_reg;
+ int button_shift;
+ bool button_status;
+
+ /* determine which data byte the button status is in */
+ button_reg = button / 7;
+ /* bit shift to get button's status */
+ button_shift = button % 8;
+ button_status =
+ ((f19->button_data_buffer[button_reg]>> button_shift)
+ & 0x01) != 0;
+
+ /* if the button state changed from the last time report it
+ * and store the new state */
+ if (button_status != f19->button_down[button]) {
+ dev_dbg(&fc->dev, "%s: Button %d (code %d) -> %d.\n",
+ __func__, button, f19->button_map[button],
+ button_status);
+ /* Generate an event here. */
+ input_report_key(f19->input, f19->button_map[button],
+ button_status);
+ f19->button_down[button] = button_status;
+ }
+ }
All of the above could be reduced to:
for (button = 0; button< f19->button_count; button++)
input_report_key(f19->input, f19->button_map[button],
test_bit(button, f19->button_data_buffer);
I'd like to, but I'm not sure - can we count on the endian-ness of
the host processor being the same as the RMI4 register endian-ness?
Hmm, I'd expect RMI transport functions take care of converting into
proper endianness.
If I can convince myself that it will never do something surprising,
we'll implement that.
[snip]
+
+static ssize_t rmi_f19_sensor_map_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct rmi_function_container *fc;
+ struct f19_data *data;
+ int sensor_map;
+ int i;
+ int retval = count;
+ int button_count = 0;
+ int ctrl_bass_addr;
+ int button_reg;
+ fc = to_rmi_function_container(dev);
+ data = fc->data;
+
+ if (data->button_query.configurable == 0) {
+ dev_err(dev,
+ "%s: Error - sensor map is not configuralbe at"
+ " run-time", __func__);
This is not driver error, return error silently.
I don't like failing silently, for reasons outlined above.
And for the reason also outlined above I disagree. Driver errors
(especially KERNEL_ERR level) should be used to signal conditions when
driver either can't continue or its functionality is seriously impaired.
Bad user input does not qualify here.
I think switching to the attribute groups will eliminate this particular
case, along with a few others. We'll switch others to warning, which is
a more appropriate level, and look at making them conditional (default
to silent).
[snip]
+static ssize_t rmi_f19_button_map_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t count)
+{
+ struct rmi_function_container *fc;
+ struct f19_data *data;
+ unsigned int button;
+ int i;
+ int retval = count;
+ int button_count = 0;
+ unsigned char temp_button_map[KEY_MAX];
+
+ fc = to_rmi_function_container(dev);
+ data = fc->data;
+
+ /* Do validation on the button map data passed in. Store button
+ * mappings into a temp buffer and then verify button count and
+ * data prior to clearing out old button mappings and storing the
+ * new ones. */
+ for (i = 0; i< data->button_count&& *buf != 0;
+ i++) {
+ /* get next button mapping value and store and bump up to
+ * point to next item in buf */
+ sscanf(buf, "%u",&button);
+
+ /* Make sure the key is a valid key */
+ if (button> KEY_MAX) {
+ dev_err(dev,
+ "%s: Error - button map for button %d is not a"
+ " valid value 0x%x.\n", __func__, i, button);
+ retval = -EINVAL;
+ goto err_ret;
+ }
+
+ temp_button_map[i] = button;
+ button_count++;
+
+ /* bump up buf to point to next item to read */
+ while (*buf != 0) {
+ buf++;
+ if (*(buf - 1) == ' ')
+ break;
+ }
+ }
+
+ /* Make sure the button count matches */
+ if (button_count != data->button_count) {
+ dev_err(dev,
+ "%s: Error - button map count of %d doesn't match device "
+ "button count of %d.\n", __func__, button_count,
+ data->button_count);
+ retval = -EINVAL;
+ goto err_ret;
+ }
+
+ /* Clear the key bits for the old button map. */
+ for (i = 0; i< button_count; i++)
+ clear_bit(data->button_map[i], data->input->keybit);
+
+ /* Switch to the new map. */
+ memcpy(data->button_map, temp_button_map,
+ data->button_count);
+
+ /* Loop through the key map and set the key bit for the new mapping. */
+ for (i = 0; i< button_count; i++)
+ set_bit(data->button_map[i], data->input->keybit);
+
+err_ret:
+ return retval;
+}
Button map (keymap) should be manipulated with EVIOCGKEYCODE and
EVIOCSKEYCODE ioctls, no need to invent driver-specific way of doing
this.
Makes sense, but... we had a customer request to specify the
boot-time keymap through the RMI4 driver's platform data. Is it
legal to call setkeycode() to do that instead?
No, the input device should be fully registered for that... But you can
supply initial keymap in platform data, almost all drivers do that. It
is new sysfs interface I object to here.
Thanks - we'll study this more closely. I agree with not having another
sysfs interface if the functionality is already there. We just don't
always realize that there is already an existing interface.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/