This patch should definately be reviewed by Henrik Rydberg so include him on
the next iteration.
On Fri, Aug 17, 2012 at 3:17 PM, Christopher Heiny <cheiny@xxxxxxxxxxxxx> wrote:
Verbose description of what the patch does please.
(...)
> +++ b/drivers/input/rmi4/rmi_f11.c
(...)
> +#ifdef CONFIG_RMI4_F11_TYPEB
> +#include <linux/mt.h>
> +#endif
It doesn't *HURT* if you include one file to many so
just skip the #ifdef.
> +#include <linux/rmi.h>
> +#include "rmi_driver.h"
> +
> +#ifdef CONFIG_RMI4_DEBUG
Same here, just leave all headers in, skip the #ifdef
> +#include <linux/debugfs.h>
> +#include <linux/fs.h>
> +/*#include <asm/uaccess.h> */
Delete that line
> +#include <linux/uaccess.h>
> +#endif
> +
> +#define RESUME_REZERO (1 && defined(CONFIG_PM))
> +#if RESUME_REZERO
> +#include <linux/delay.h>
> +#define DEFAULT_REZERO_WAIT_MS 40
> +#endif
Dito.
> +#define GET_FINGER_STATE(f_states, i) \
> + ((f_states[i / 4] >> (2 * (i % 4))) & FINGER_STATE_MASK)
Convert to a static inline function.
> +#define INBOX(x, y, box) (x >= box.x && x < (box.x + box.width) \
> + && y >= box.y && y < (box.y + box.height))
Dito.
> +/* Adding debugfs for flip, clip, offset and swap */
> +#ifdef CONFIG_RMI4_DEBUG
> +
> +static int setup_debugfs(struct rmi_device *rmi_dev);
> +static void teardown_debugfs(struct rmi_device *rmi_dev);
> +#endif
> +/* End adding debugfs */
Better to depend on CONFIG_DEBUG_FS
(...)
> +#if RESUME_REZERO
This RESUME_REZERO business scares me off. First it should
be a Kconfig thing, and why is it optional? Also put in a comment
explaining what it's all about.
> +static struct device_attribute attrs[] = {
> + __ATTR(relreport, RMI_RW_ATTR, f11_relreport_show, f11_relreport_store),
> + __ATTR(maxPos, RMI_RO_ATTR, f11_maxPos_show, rmi_store_error),
> +#if RESUME_REZERO
> + __ATTR(rezeroOnResume, RMI_RW_ATTR, f11_rezeroOnResume_show,
> + f11_rezeroOnResume_store),
> + __ATTR(rezeroWait, RMI_RW_ATTR, f11_rezeroWait_show,
> + f11_rezeroWait_store),
> +#endif
> + __ATTR(rezero, RMI_WO_ATTR, rmi_show_error, f11_rezero_store)
> +};
Documentation/ABI/testing/* needs these, plus consider moving some
of these to debugfs.
> +union f11_2d_commands {
> + struct {
> + u8 rezero:1;
> + };
> + u8 reg;
Now I think I can see what you're trying to do. The .reg
is just there to make sure this thing is padded to 8 bits right?
__attribute__((packed)); ?
> +struct f11_2d_device_query {
> + union {
> + struct {
> + u8 nbr_of_sensors:3;
> + u8 has_query9:1;
> + u8 has_query11:1;
> + };
> + u8 f11_2d_query0;
> + };
> +
> + union {
> + struct {
> + u8 has_z_tuning:1;
> + u8 has_pos_interpolation_tuning:1;
> + u8 has_w_tuning:1;
> + u8 has_pitch_info:1;
> + u8 has_default_finger_width:1;
> + u8 has_segmentation_aggressiveness:1;
> + u8 has_tx_rw_clip:1;
> + u8 has_drumming_correction:1;
> + };
> + u8 f11_2d_query11;
> + };
> +};
__attribute__((packed)); ?
> +union f11_2d_query9 {
> + struct {
> + u8 has_pen:1;
> + u8 has_proximity:1;
> + u8 has_palm_det_sensitivity:1;
> + u8 has_suppress_on_palm_detect:1;
> + u8 has_two_pen_thresholds:1;
> + u8 has_contact_geometry:1;
> + };
> + u8 reg;
> +};
__attribute__((packed)); ?
> +struct f11_2d_sensor_query {
> + union {
> + struct {
> + /* query1 */
> + u8 number_of_fingers:3;
> + u8 has_rel:1;
> + u8 has_abs:1;
> + u8 has_gestures:1;
> + u8 has_sensitivity_adjust:1;
> + u8 configurable:1;
> + /* query2 */
> + u8 num_of_x_electrodes:7;
> + /* query3 */
> + u8 num_of_y_electrodes:7;
> + /* query4 */
> + u8 max_electrodes:7;
> + };
> + u8 f11_2d_query1__4[4];
> + };
> +
> + union {
> + struct {
> + u8 abs_data_size:3;
> + u8 has_anchored_finger:1;
> + u8 has_adj_hyst:1;
> + u8 has_dribble:1;
> + };
> + u8 f11_2d_query5;
> + };
> +
> + u8 f11_2d_query6;
> +
> + union {
> + struct {
> + u8 has_single_tap:1;
> + u8 has_tap_n_hold:1;
> + u8 has_double_tap:1;
> + u8 has_early_tap:1;
> + u8 has_flick:1;
> + u8 has_press:1;
> + u8 has_pinch:1;
> + u8 padding:1;
> +
> + u8 has_palm_det:1;
> + u8 has_rotate:1;
> + u8 has_touch_shapes:1;
> + u8 has_scroll_zones:1;
> + u8 has_individual_scroll_zones:1;
> + u8 has_multi_finger_scroll:1;
> + };
> + u8 f11_2d_query7__8[2];
> + };
> +
> + union f11_2d_query9 query9;
> +
> + union {
> + struct {
> + u8 nbr_touch_shapes:5;
> + };
> + u8 f11_2d_query10;
> + };
> +};
__attribute__((packed)); ?
> +union f11_2d_ctrl0_9 {
> + struct {
> + /* F11_2D_Ctrl0 */
> + u8 reporting_mode:3;
> + u8 abs_pos_filt:1;
> + u8 rel_pos_filt:1;
> + u8 rel_ballistics:1;
> + u8 dribble:1;
> + u8 report_beyond_clip:1;
> + /* F11_2D_Ctrl1 */
> + u8 palm_detect_thres:4;
> + u8 motion_sensitivity:2;
> + u8 man_track_en:1;
> + u8 man_tracked_finger:1;
> + /* F11_2D_Ctrl2 and 3 */
> + u8 delta_x_threshold:8;
> + u8 delta_y_threshold:8;
> + /* F11_2D_Ctrl4 and 5 */
> + u8 velocity:8;
> + u8 acceleration:8;
> + /* F11_2D_Ctrl6 thru 9 */
> + u16 sensor_max_x_pos:12;
> + u8 ctrl7_reserved:4;
> + u16 sensor_max_y_pos:12;
> + u8 ctrl9_reserved:4;
> + };
> + struct {
> + u8 regs[10];
> + u16 address;
> + };
> +};
__attribute__((packed)); ?
> +union f11_2d_ctrl10 {
> + struct {
> + u8 single_tap_int_enable:1;
> + u8 tap_n_hold_int_enable:1;
> + u8 double_tap_int_enable:1;
> + u8 early_tap_int_enable:1;
> + u8 flick_int_enable:1;
> + u8 press_int_enable:1;
> + u8 pinch_int_enable:1;
> + };
> + u8 reg;
> +};
__attribute__((packed)); ?
> +union f11_2d_ctrl11 {
> + struct {
> + u8 palm_detect_int_enable:1;
> + u8 rotate_int_enable:1;
> + u8 touch_shape_int_enable:1;
> + u8 scroll_zone_int_enable:1;
> + u8 multi_finger_scroll_int_enable:1;
> + };
> + u8 reg;
> +};
__attribute__((packed)); ?
> +union f11_2d_ctrl12 {
> + struct {
> + u8 sensor_map:7;
> + u8 xy_sel:1;
> + };
> + u8 reg;
> +};
__attribute__((packed)); ?
> +union f11_2d_ctrl14 {
> + struct {
> + u8 sens_adjustment:5;
> + u8 hyst_adjustment:3;
> + };
> + u8 reg;
> +};
__attribute__((packed)); ?
> +union f11_2d_ctrl15 {
> + struct {
> + u8 max_tap_time:8;
> + };
> + u8 reg;
> +};
__attribute__((packed)); ?
> +union f11_2d_ctrl16 {
> + struct {
> + u8 min_press_time:8;
> + };
> + u8 reg;
> +};
__attribute__((packed)); ?
> +union f11_2d_ctrl17 {
> + struct {
> + u8 max_tap_distance:8;
> + };
> + u8 reg;
> +};
__attribute__((packed)); ?
> +union f11_2d_ctrl18_19 {
> + struct {
> + u8 min_flick_distance:8;
> + u8 min_flick_speed:8;
> + };
> + u8 reg[2];
> +};
__attribute__((packed)); ?
> +union f11_2d_ctrl20_21 {
> + struct {
> + u8 pen_detect_enable:1;
> + u8 pen_jitter_filter_enable:1;
> + u8 ctrl20_reserved:6;
> + u8 pen_z_threshold:8;
> + };
> + u8 reg[2];
> +};
__attribute__((packed)); ?
> +/* These are not accessible through sysfs yet. */
> +union f11_2d_ctrl22_26 {
> + struct {
> + /* control 22 */
> + u8 proximity_detect_int_en:1;
> + u8 proximity_jitter_filter_en:1;
> + u8 f11_2d_ctrl6_b3__7:6;
> +
> + /* control 23 */
> + u8 proximity_detection_z_threshold;
> +
> + /* control 24 */
> + u8 proximity_delta_x_threshold;
> +
> + /* control 25 */
> + u8 proximity_delta_y_threshold;
> +
> + /* control 26 */
> + u8 proximity_delta_z_threshold;
> + };
> + u8 regs[5];
> +};
__attribute__((packed)); ?
> +/* control 27 - haspalmdetectsensitivity or has suppressonpalmdetect */
> +union f11_2d_ctrl27 {
> + struct {
> + u8 palm_detecy_sensitivity:4;
> + u8 suppress_on_palm_detect:1;
> + u8 f11_2d_ctrl27_b5__7:3;
> + };
> + u8 regs[1];
> +};
__attribute__((packed)); ?
> +/* control 28 - has_multifingerscroll */
> +union f11_2d_ctrl28 {
> + struct {
> + u8 multi_finger_scroll_mode:2;
> + u8 edge_motion_en:1;
> + u8 f11_2d_ctrl28b_3:1;
> + u8 multi_finger_scroll_momentum:4;
> + };
> + u8 regs[1];
> +};
__attribute__((packed)); ?
> +/* control 29 & 30 - hasztuning */
> +union f11_2d_ctrl29_30 {
> + struct {
> + u8 z_touch_threshold;
> + u8 z_touch_hysteresis;
> + };
> + struct {
> + u8 regs[2];
> + u16 address;
> + };
> +};
__attribute__((packed)); ?
> +struct f11_2d_ctrl {
> + union f11_2d_ctrl0_9 *ctrl0_9;
> + union f11_2d_ctrl10 *ctrl10;
> + union f11_2d_ctrl11 *ctrl11;
> + union f11_2d_ctrl12 *ctrl12;
> + u8 ctrl12_size;
> + union f11_2d_ctrl14 *ctrl14;
> + union f11_2d_ctrl15 *ctrl15;
> + union f11_2d_ctrl16 *ctrl16;
> + union f11_2d_ctrl17 *ctrl17;
> + union f11_2d_ctrl18_19 *ctrl18_19;
> + union f11_2d_ctrl20_21 *ctrl20_21;
> + union f11_2d_ctrl22_26 *ctrl22_26;
> + union f11_2d_ctrl27 *ctrl27;
> + union f11_2d_ctrl28 *ctrl28;
> + union f11_2d_ctrl29_30 *ctrl29_30;
> +};
__attribute__((packed)); ?
> +struct f11_2d_data_1_5 {
> + u8 x_msb;
> + u8 y_msb;
> + u8 x_lsb:4;
> + u8 y_lsb:4;
> + u8 w_y:4;
> + u8 w_x:4;
> + u8 z;
> +};
__attribute__((packed)); ?
> +struct f11_2d_data_6_7 {
> + s8 delta_x;
> + s8 delta_y;
> +};
__attribute__((packed)); ?
> +struct f11_2d_data_8 {
> + u8 single_tap:1;
> + u8 tap_and_hold:1;
> + u8 double_tap:1;
> + u8 early_tap:1;
> + u8 flick:1;
> + u8 press:1;
> + u8 pinch:1;
> +};
__attribute__((packed)); ?
> +struct f11_2d_data_9 {
> + u8 palm_detect:1;
> + u8 rotate:1;
> + u8 shape:1;
> + u8 scrollzone:1;
> + u8 finger_count:3;
> +};
__attribute__((packed)); ?
(...)
> +/* Adding debugfs for flip, clip, offset and swap */
> +#ifdef CONFIG_RMI4_DEBUG
I think this should be just switched on #CONFIG_DEBUG_FS
(...)
> +static int get_tool_type(struct f11_2d_sensor *sensor, u8 finger_state)
> +{
> +#ifdef CONFIG_RMI4_F11_PEN
> + if (sensor->sens_query.query9.has_pen && finger_state == F11_PEN)
> + return MT_TOOL_PEN;
> +#endif
> + return MT_TOOL_FINGER;
> +}
Consider the case where we build a single kernel used on two devices:
one has a pen input and another one has a finger input.
What do you config in your kernel build?
I'm uncertain about the above code, but I hope you can just
define that you want pen support and have both work just
as well.
(...)
> +#ifdef ABS_MT_PRESSURE
Isn't it CONFIG_ABS_MT_PRESSURE?
> + input_report_abs(sensor->input, ABS_MT_PRESSURE, z);
> +#endif
Henrik will have to answer but why is this optional?
Can't your driver just select ABS_MT_PRESSURE and just always
report this?
> +static int f11_allocate_control_regs(struct rmi_device *rmi_dev,
> + struct f11_2d_device_query *device_query,
> + struct f11_2d_sensor_query *sensor_query,
> + struct f11_2d_ctrl *ctrl,
> + u16 ctrl_base_addr) {
> +
> + int error = 0;
> + ctrl->ctrl0_9 = kzalloc(sizeof(union f11_2d_ctrl0_9),
> + GFP_KERNEL);
> + if (!ctrl->ctrl0_9) {
> + error = -ENOMEM;
> + goto error_exit;
> + }
> + if (sensor_query->f11_2d_query7__8[0]) {
> + ctrl->ctrl10 = kzalloc(sizeof(union f11_2d_ctrl10),
> + GFP_KERNEL);
> + if (!ctrl->ctrl10) {
> + error = -ENOMEM;
> + goto error_exit;
> + }
> + }
> +
> + if (sensor_query->f11_2d_query7__8[1]) {
> + ctrl->ctrl11 = kzalloc(sizeof(union f11_2d_ctrl11),
> + GFP_KERNEL);
> + if (!ctrl->ctrl11) {
> + error = -ENOMEM;
> + goto error_exit;
> + }
> + }
> +
> + if (device_query->has_query9 && sensor_query->query9.has_pen) {
> + ctrl->ctrl20_21 = kzalloc(sizeof(union f11_2d_ctrl20_21),
> + GFP_KERNEL);
> + if (!ctrl->ctrl20_21) {
> + error = -ENOMEM;
> + goto error_exit;
> + }
> + }
> +
> + if (device_query->has_query9 && sensor_query->query9.has_proximity) {
> + ctrl->ctrl22_26 = kzalloc(sizeof(union f11_2d_ctrl22_26),
> + GFP_KERNEL);
> + if (!ctrl->ctrl22_26) {
> + error = -ENOMEM;
> + goto error_exit;
> + }
> + }
> +
> + if (device_query->has_query9 &&
> + (sensor_query->query9.has_palm_det_sensitivity ||
> + sensor_query->query9.has_suppress_on_palm_detect)) {
> + ctrl->ctrl27 = kzalloc(sizeof(union f11_2d_ctrl27),
> + GFP_KERNEL);
> + if (!ctrl->ctrl27) {
> + error = -ENOMEM;
> + goto error_exit;
> + }
> + }
> +
> + if (sensor_query->has_multi_finger_scroll) {
> + ctrl->ctrl28 = kzalloc(sizeof(union f11_2d_ctrl28),
> + GFP_KERNEL);
> + if (!ctrl->ctrl28) {
> + error = -ENOMEM;
> + goto error_exit;
> + }
> + }
> +
> + if (device_query->has_query11 && device_query->has_z_tuning) {
> + ctrl->ctrl29_30 = kzalloc(sizeof(union f11_2d_ctrl29_30),
> + GFP_KERNEL);
> + if (!ctrl->ctrl29_30) {
> + error = -ENOMEM;
> + goto error_exit;
> + }
> + }
> +
> + return f11_read_control_regs(rmi_dev, ctrl, ctrl_base_addr);
> +
> +error_exit:
> + f11_free_control_regs(ctrl);
> + return error;
> +}
Can't you use devm_kzalloc() for all of these and move them to the call
site?
(...)
> +static int rmi_f11_get_query_parameters(struct rmi_device *rmi_dev,
> + struct f11_2d_sensor_query *query, u16 query_base_addr)
> +{
> + if (query->has_abs) {
(...)
> + if (query->has_rel) {
(...)
> + if (query->has_gestures) {
(...)
> + if (query->has_touch_shapes) {
(...)
This runtime construct is really nice.
> +static void rmi_f11_free_memory(struct rmi_function_container *fc)
> +{
> + struct f11_data *f11 = fc->data;
> + int i;
> +
> + if (f11) {
> + f11_free_control_regs(&f11->dev_controls);
> + for (i = 0; i < f11->dev_query.nbr_of_sensors + 1; i++)
> + kfree(f11->sensors[i].virtualbutton_map.map);
> + kfree(f11);
> + fc->data = NULL;
> + }
> +}
So with devm* allocators you can cut down on this.
(...)
> + /* set bits for each button... */
> + for (j = 0; j < vm_pdata->buttons; j++) {
> + memcpy(&vm_sensor->map[j], &vm_pdata->map[j],
> + sizeof(struct virtualbutton_map));
> + set_bit(vm_sensor->map[j].code,
> + f11->sensors[i].input->keybit);
Hm here you are using the nice bitops set_bit whereas other code isn't...
> + f11->sensors[i].mouse_input = input_dev_mouse;
> + input_dev_mouse->name = "rmi_mouse";
> + input_dev_mouse->phys = "rmi_f11/input0";
> +
> + input_dev_mouse->id.vendor = 0x18d1;
Describe magic numbers.
18d1 is particularly strange since in usb.ids this is Google Inc.
I think you would want to use 0x06cb (Synaptics)
I always was under the impression that USB, PCI (etc) IDs were
in the same number registry since they are often the same.
> + input_dev_mouse->id.product = 0x0210;
Usually some company-assigned person managed these numbers for e.g.
USB and PCI. Please check with her/him what to use here if possible.
(---)
> +#if RESUME_REZERO
#ifdef?
> +#if defined(CONFIG_HAS_EARLYSUSPEND) && \
> + !defined(CONFIG_RMI4_SPECIAL_EARLYSUSPEND)
> + .late_resume = rmi_f11_resume
Funny Android stuff... not supported.
Yours,
Linus Walleij
--
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
