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 [off-list ref] wrote:

Verbose description of what the patch does please.

(...)

(...)

+#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