From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Duggan Subject: Re: [PATCH 2/3] Input: synaptics-rmi4 - ability disable abs or rel reporting Date: Tue, 25 Mar 2014 13:45:25 -0700 Message-ID: <5331EAE5.40807@synaptics.com> References: <1395191031-3144-1-git-send-email-cheiny@synaptics.com> <1395191031-3144-2-git-send-email-cheiny@synaptics.com> <5329B17C.8080202@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from us-mx2.synaptics.com ([192.147.44.131]:44887 "EHLO us-mx2.synaptics.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755016AbaCYUuo (ORCPT ); Tue, 25 Mar 2014 16:50:44 -0400 In-Reply-To: <5329B17C.8080202@redhat.com> Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Benjamin Tissoires , Christopher Heiny , Dmitry Torokhov Cc: Linux Input , Vincent Huang , Vivian Ly , Daniel Rosenberg , Linus Walleij , David Herrmann , Jiri Kosina Hi Benjamin, Thanks for reviewing our patches. We are putting together a v2 set based on your comments. Everything looks straight forward, I just have one comment in line. On 03/19/2014 08:02 AM, Benjamin Tissoires wrote: > > > On 03/18/2014 09:03 PM, Christopher Heiny wrote: >> Even if the RMI4 touchscreen/touchpad provides reporting both >> relative and absolute coordinates, reporting both to userspace >> could be confusing. Allow the platform data to disable either >> absolute or relative coordinates. > > General comments on the patch: > Is there really a need to export the rel axis when there is already an > abs collection? > I mean, with the RMI4 over HID over I2C found on the XPS Haswell > series, RMI4 will be bound automatically, and the sensor may (will) > pretend that it can do both abs and rel. However, we are not using a > platform_data for them (I think we should not), and we will get the > two collections. > > I would personally be in favor of having a priority mechanism: if abs > is here, skip rel, otherwise use rel. But I have no clue if you will > ship devices which will require both. So you make the call. > >> >> Signed-off-by: Andrew Duggan >> Acked-by: Christopher Heiny >> Cc: Dmitry Torokhov >> Cc: Benjamin Tissoires >> Cc: Linux Walleij >> Cc: David Herrmann >> Cc: Jiri Kosina >> >> --- >> drivers/input/rmi4/rmi_f11.c | 78 >> +++++++++++++++++++++++++++++++++++++------- >> include/linux/rmi.h | 6 ++++ >> 2 files changed, 73 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.c >> index 07044d79..c87c6cc3 100644 >> --- a/drivers/input/rmi4/rmi_f11.c >> +++ b/drivers/input/rmi4/rmi_f11.c >> @@ -520,6 +520,8 @@ struct f11_2d_sensor { >> struct rmi_function *fn; >> char input_phys[NAME_BUFFER_SIZE]; >> char input_phys_mouse[NAME_BUFFER_SIZE]; >> + u8 report_abs; >> + u8 report_rel; >> }; >> >> /** Data pertaining to F11 in general. For per-sensor data, see >> struct >> @@ -544,6 +546,10 @@ struct f11_data { >> struct mutex dev_controls_mutex; >> u16 rezero_wait_ms; >> struct f11_2d_sensor sensor; >> + unsigned long *abs_mask; >> + unsigned long *rel_mask; >> + unsigned long *result_bits; >> + unsigned long mask_memory[]; >> }; >> >> enum finger_state_values { >> @@ -591,10 +597,14 @@ static void rmi_f11_rel_pos_report(struct >> f11_2d_sensor *sensor, u8 n_finger) >> if (x || y) { >> input_report_rel(sensor->input, REL_X, x); >> input_report_rel(sensor->input, REL_Y, y); >> - input_report_rel(sensor->mouse_input, REL_X, x); >> - input_report_rel(sensor->mouse_input, REL_Y, y); >> + >> + if (sensor->mouse_input) { >> + input_report_rel(sensor->mouse_input, REL_X, x); >> + input_report_rel(sensor->mouse_input, REL_Y, y); >> + } >> } >> - input_sync(sensor->mouse_input); >> + if (sensor->mouse_input) >> + input_sync(sensor->mouse_input); >> } >> >> static void rmi_f11_abs_pos_report(struct f11_data *f11, >> @@ -694,13 +704,17 @@ static void rmi_f11_abs_pos_report(struct >> f11_data *f11, >> } >> >> static void rmi_f11_finger_handler(struct f11_data *f11, >> - struct f11_2d_sensor *sensor) >> + struct f11_2d_sensor *sensor, >> + unsigned long *irq_bits, int num_irq_regs) >> { >> const u8 *f_state = sensor->data.f_state; >> u8 finger_state; >> u8 finger_pressed_count; >> u8 i; >> >> + int rel_bits; >> + int abs_bits; >> + >> for (i = 0, finger_pressed_count = 0; i < sensor->nbr_fingers; >> i++) { >> /* Possible of having 4 fingers per f_statet register */ >> finger_state = (f_state[i / 4] >> (2 * (i % 4))) & >> @@ -714,13 +728,19 @@ static void rmi_f11_finger_handler(struct >> f11_data *f11, >> finger_pressed_count++; >> } >> >> - if (sensor->data.abs_pos) >> + abs_bits = bitmap_and(f11->result_bits, irq_bits, >> f11->abs_mask, >> + num_irq_regs); >> + if (abs_bits) >> rmi_f11_abs_pos_report(f11, sensor, finger_state, i); > > rmi_driver.c uses bitmap_and this way: > bitmap_and(data->fn_irq_bits, data->irq_status, fn->irq_mask, > data->irq_count); > if (!bitmap_empty(data->fn_irq_bits, data->irq_count)) > fh->attention(fn, data->fn_irq_bits); > > Not sure which way is the best. Looks like based on commit f4b0373b26567cafd421d91101852ed7a34e9e94 the call to bitmap_empty is redundant so I am going to leave it the way it is. We should clean up rmi_driver.c in a future patch. > >> >> - if (sensor->data.rel_pos) >> + rel_bits = bitmap_and(f11->result_bits, irq_bits, >> f11->rel_mask, >> + num_irq_regs); >> + if (rel_bits) >> rmi_f11_rel_pos_report(sensor, i); >> } >> + >> input_report_key(sensor->input, BTN_TOUCH, finger_pressed_count); >> + > > those two blank lines are unrelated to the commit. > >> input_sync(sensor->input); >> } >> >> @@ -1180,21 +1200,33 @@ static int rmi_f11_initialize(struct >> rmi_function *fn) >> u16 max_x_pos, max_y_pos, temp; >> int rc; >> const struct rmi_device_platform_data *pdata = >> rmi_get_platform_data(rmi_dev); >> + struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev); >> struct f11_2d_sensor *sensor; >> u8 buf; >> + int mask_size; >> >> dev_dbg(&fn->dev, "Initializing F11 values for %s.\n", >> pdata->sensor_name); >> >> + mask_size = BITS_TO_LONGS(drvdata->irq_count) * sizeof(unsigned >> long); >> + >> /* >> ** init instance data, fill in values and create any sysfs files >> */ >> - f11 = devm_kzalloc(&fn->dev, sizeof(struct f11_data), GFP_KERNEL); >> + f11 = devm_kzalloc(&fn->dev, sizeof(struct f11_data) + mask_size >> * 3, >> + GFP_KERNEL); >> if (!f11) >> return -ENOMEM; >> >> f11->rezero_wait_ms = pdata->f11_rezero_wait; >> >> + f11->abs_mask = f11->mask_memory + mask_size * 0; > > I personally don't like the " + mask_size * 0" > > Can't you just also remove the mask_memory field and use sizeof(struct > f11_data)? > >> + f11->rel_mask = f11->mask_memory + mask_size * 1; >> + f11->result_bits = f11->mask_memory + mask_size * 2; >> + >> + set_bit(fn->irq_pos, f11->abs_mask); >> + set_bit(fn->irq_pos + 1, f11->rel_mask); >> + >> query_base_addr = fn->fd.query_base_addr; >> control_base_addr = fn->fd.control_base_addr; >> >> @@ -1226,12 +1258,25 @@ static int rmi_f11_initialize(struct >> rmi_function *fn) >> return rc; >> } >> >> + sensor->report_rel = sensor->sens_query.has_rel; >> + sensor->report_abs = sensor->sens_query.has_abs; >> + >> if (pdata->f11_sensor_data) { >> sensor->axis_align = >> pdata->f11_sensor_data->axis_align; >> sensor->type_a = pdata->f11_sensor_data->type_a; >> sensor->sensor_type = >> pdata->f11_sensor_data->sensor_type; >> + >> + if (sensor->sens_query.has_abs) >> + sensor->report_abs = sensor->report_abs >> + && !(pdata->f11_sensor_data->disable_report_mask >> + & RMI_F11_DISABLE_ABS_REPORT); > > sensor->report_abs already contains sensor->sens_query.has_abs (set > few lines above)... > > so you can just skip the if test here. > > >> + >> + if (sensor->sens_query.has_rel) >> + sensor->report_rel = sensor->report_rel >> + && !(pdata->f11_sensor_data->disable_report_mask >> + & RMI_F11_DISABLE_REL_REPORT); > > same here. > >> } >> >> rc = rmi_read_block(rmi_dev, >> @@ -1324,9 +1369,10 @@ static int rmi_f11_register_devices(struct >> rmi_function *fn) >> set_bit(EV_ABS, input_dev->evbit); >> input_set_capability(input_dev, EV_KEY, BTN_TOUCH); >> >> - f11_set_abs_params(fn, f11); >> + if (sensor->report_abs) >> + f11_set_abs_params(fn, f11); >> >> - if (sensor->sens_query.has_rel) { >> + if (sensor->report_rel) { >> set_bit(EV_REL, input_dev->evbit); >> set_bit(REL_X, input_dev->relbit); >> set_bit(REL_Y, input_dev->relbit); >> @@ -1338,7 +1384,7 @@ static int rmi_f11_register_devices(struct >> rmi_function *fn) >> goto error_unregister; >> } >> >> - if (sensor->sens_query.has_rel) { >> + if (sensor->report_rel) { >> /*create input device for mouse events */ >> input_dev_mouse = input_allocate_device(); >> if (!input_dev_mouse) { >> @@ -1407,8 +1453,16 @@ error_unregister: >> static int rmi_f11_config(struct rmi_function *fn) >> { >> struct f11_data *f11 = dev_get_drvdata(&fn->dev); >> + struct rmi_driver *drv = fn->rmi_dev->driver; >> + struct f11_2d_sensor *sensor = &f11->sensor; >> int rc; >> >> + if (!sensor->report_abs) >> + drv->clear_irq_bits(fn->rmi_dev, f11->abs_mask); >> + >> + if (!sensor->report_rel) >> + drv->clear_irq_bits(fn->rmi_dev, f11->rel_mask); >> + >> rc = f11_write_control_regs(fn, &f11->sensor.sens_query, >> &f11->dev_controls, fn->fd.query_base_addr); >> if (rc < 0) >> @@ -1420,6 +1474,7 @@ static int rmi_f11_config(struct rmi_function *fn) >> static int rmi_f11_attention(struct rmi_function *fn, unsigned long >> *irq_bits) >> { >> struct rmi_device *rmi_dev = fn->rmi_dev; >> + struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev); >> struct f11_data *f11 = dev_get_drvdata(&fn->dev); >> u16 data_base_addr = fn->fd.data_base_addr; >> u16 data_base_addr_offset = 0; >> @@ -1432,7 +1487,8 @@ static int rmi_f11_attention(struct >> rmi_function *fn, unsigned long *irq_bits) >> if (error) >> return error; >> >> - rmi_f11_finger_handler(f11, &f11->sensor); >> + rmi_f11_finger_handler(f11, &f11->sensor, irq_bits, >> + drvdata->num_of_irq_regs); >> data_base_addr_offset += f11->sensor.pkt_size; >> >> return 0; >> diff --git a/include/linux/rmi.h b/include/linux/rmi.h >> index 735e978..a0d0187 100644 >> --- a/include/linux/rmi.h >> +++ b/include/linux/rmi.h >> @@ -76,6 +76,9 @@ enum rmi_f11_sensor_type { >> rmi_f11_sensor_touchpad >> }; >> >> +#define RMI_F11_DISABLE_ABS_REPORT (1 << 0) >> +#define RMI_F11_DISABLE_REL_REPORT (1 << 1) > > We have BIT() macro in the kernel for this (I know, I do not use it > that much either... :-P ) > > Cheers, > Benjamin > >> + >> /** >> * struct rmi_f11_sensor_data - overrides defaults for a single F11 >> 2D sensor. >> * @axis_align - provides axis alignment overrides (see above). >> @@ -86,11 +89,14 @@ enum rmi_f11_sensor_type { >> * pointing device (touchpad) rather than a direct pointing device >> * (touchscreen). This is useful when F11_2D_QUERY14 register is not >> * available. >> + * @disable_report_mask - Force data to not be reported even if it >> is supported >> + * by the firware. >> */ >> struct rmi_f11_sensor_data { >> struct rmi_f11_2d_axis_alignment axis_align; >> bool type_a; >> enum rmi_f11_sensor_type sensor_type; >> + int disable_report_mask; >> }; >> >> /** >> Andrew