From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christopher Heiny Subject: Re: [RFC PATCH 06/06] input/rmi4: F11 - 2D touch interface Date: Thu, 25 Oct 2012 14:39:01 -0700 Message-ID: <5089B175.2080705@synaptics.com> References: <1349496603-20775-1-git-send-email-cheiny@synaptics.com> <1349496603-20775-7-git-send-email-cheiny@synaptics.com> <20121010182130.GA436@polaris.bitmath.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from us-mx3.synaptics.com ([12.239.217.85]:9283 "EHLO us-mx3.synaptics.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750912Ab2JYVjF (ORCPT ); Thu, 25 Oct 2012 17:39:05 -0400 In-Reply-To: <20121010182130.GA436@polaris.bitmath.org> Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Henrik Rydberg Cc: Dmitry Torokhov , Jean Delvare , Linux Kernel , Linux Input , Allie Xiong , Vivian Ly , Daniel Rosenberg , Alexandra Chin , Joerie de Gram , Wolfram Sang , Mathieu Poirier , Linus Walleij , Naveen Kumar Gaddipati On 10/10/2012 11:21 AM, Henrik Rydberg wrote: > Hi Christopher, > >> rmi_f11.c is a driver for 2D touch sensors. It has been updated to = support >> the MT-B specification, partition control attributes between debugfs= andsysfs, >> and to use the standard bus model for loading/unloading. > > Please find comments inline. Thanks very much. I though I'd replied to this along with the other=20 replies sent a couple of weeks ago, but don't see it in the archives. M= y=20 apologies if this is a duplicate message. > > Generally, if you want this merged as an input device sometime in the > future, you need to reduce the size and complexity of the whole > patchset. Given that you only need to feed the input subsystem with > raw data, you can make do without most of the sysfs nodes, debug > output, and internal gesture state. > > The 2D sensor data, currently living in debugfs, might eventually fin= d > a home in the input subsystem, based on the growing interest from > several directions. However, if you are just looking for a way to > transport the rmi data to userland, please consider implementing this > under a different subsystem. I must disagree with some of this. Feeding raw data to the input=20 subsystem is not the only thing that needs to be done - there needs to=20 be a way to configure and control the operation of the touch sensor,=20 both during product prototyping/development (we've partitioned that=20 stuff into debugfs) and during normal operation (we've put that stuff i= n=20 sysfs). If we remove the related code, that would not be possible. If= =20 we move it to a different subsystem, where do you suggest it be moved t= o? The gesture data is a different matter. Although the most popular user= =20 space implementations may contain gesture recognition engines, not all=20 of them do. In that case, it is desirable to have the touch sensor=20 firmware perform basic gesture recognition. However, there is at this=20 time no way standard way to transfer that via the input subsystem. We=20 chose to use sysfs for this, but alternatively we could investigate=20 using a custom event stream in the input subsystem. We'd also be quite= =20 happy to work with you to define a standard set of gesture events for=20 basic gestures. > >> diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f= 11.c [snip] >> + >> +#define FINGER_STATE_MASK 0x03 >> +#define GET_FINGER_STATE(f_states, i) \ >> + ((f_states[i / 4] >> (2 * (i % 4))) & FINGER_STATE_MASK) > > These could be open-coded or put in a function instead. We'll put that in a function. >> + >> +#define F11_CTRL_SENSOR_MAX_X_POS_OFFSET 6 >> +#define F11_CTRL_SENSOR_MAX_Y_POS_OFFSET 8 >> + >> +#define F11_CEIL(x, y) (((x) + ((y)-1)) / (y)) >> +#define INBOX(x, y, box) (x >=3D box.x && x < (box.x + box.width) \ >> + && y >=3D box.y && y < (box.y + box.height)) >> + >> +#define DEFAULT_XY_MAX 9999 >> +#define DEFAULT_MAX_ABS_MT_PRESSURE 255 >> +#define DEFAULT_MAX_ABS_MT_TOUCH 15 >> +#define DEFAULT_MAX_ABS_MT_ORIENTATION 1 >> +#define DEFAULT_MIN_ABS_MT_TRACKING_ID 1 >> +#define DEFAULT_MAX_ABS_MT_TRACKING_ID 10 >> +#define MAX_NAME_LENGTH 256 >> + >> +static ssize_t f11_relreport_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf); >> + >> +static ssize_t f11_relreport_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count); >> + >> +static ssize_t f11_maxPos_show(struct device *dev, >> + struct device_attribute *attr, char *buf); >> + >> +static ssize_t f11_rezero_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count); >> + >> +static void rmi_f11_free_memory(struct rmi_function_container *fc); >> + >> +static int rmi_f11_initialize(struct rmi_function_container *fc); >> + >> +static int rmi_f11_create_sysfs(struct rmi_function_container *fc); >> + >> +static int rmi_f11_config(struct rmi_function_container *fc); >> + >> +static int rmi_f11_register_devices(struct rmi_function_container *= fc); >> + >> +static void rmi_f11_free_devices(struct rmi_function_container *fc)= ; >> + >> +static void f11_set_abs_params(struct rmi_function_container *fc, i= nt index); > > Please try to get rid of these. OK [snip] >> + >> +/** >> + * @rezero - writing 1 to this will cause the sensor to calibrate t= o the >> + * current capacitive state. >> + */ >> +union f11_2d_commands { >> + struct { >> + bool rezero:1; >> + u8 reserved:7; >> + } __attribute__((__packed__)); >> + u8 reg; >> +}; > > Maybe a constant would be more suitable here? We're trying to use a single idiom for accessing registers (the=20 union/struct mechanism you see here). In a couple of cases, that means= =20 we use a struct to control a single bit, instead of a constant. The en= d=20 result is the same, and we find that not mixing our idioms makes the=20 code a lot easier to maintain. > >> + >> +/** >> + * @nbr_of_sensors - the number of 2D sensors on the touch device. >> + * @has_query9 - indicates the F11_2D_Query9 register exists. >> + * @has_query11 - indicates the F11_2D_Query11 register exists. >> + * @has_z_tuning - if set, the sensor supports Z tuning and registe= rs >> + * F11_2D_Ctrl29 through F11_2D_Ctrl33 exist. >> + * @has_pos_interpolation_tuning - TBD >> + * @has_w_tuning - the sensor supports Wx and Wy scaling and regist= ers >> + * F11_2D_Ctrl36 through F11_2D_Ctrl39 exist. >> + * @has_pitch_info - the X and Y pitches of the sensor electrodes c= an be >> + * configured and registers F11_2D_Ctrl40 and F11_2D_Ctrl41 exist. >> + * @has_default_finger_width - the default finger width settings f= or the >> + * sensor can be configured and registers F11_2D_Ctrl42 through F11= _2D_Ctrl44 >> + * exist. >> + * @has_segmentation_aggressiveness - the sensor=E2=80=99s ability = to distinguish >> + * multiple objects close together can be configured and register F= 11_2D_Ctrl45 >> + * exists. >> + * @has_tx_rw_clip - the inactive outside borders of the sensor ca= n be >> + * configured and registers F11_2D_Ctrl46 through F11_2D_Ctrl49 exi= st. >> + * @has_drumming_correction - the sensor can be configured to disti= nguish >> + * between a fast flick and a quick drumming movement and registers >> + * F11_2D_Ctrl50 and F11_2D_Ctrl51 exist. >> + */ >> +struct f11_2d_device_query { >> + union { >> + struct { >> + u8 nbr_of_sensors:3; >> + bool has_query9:1; >> + bool has_query11:1; >> + u8 reserved:3; >> + } __attribute__((__packed__)); >> + u8 f11_2d_query0; >> + }; > > Why union here? It seems all you need is a packed struct named query0= =2E If Dmitry's suggestion (discussed in other emails) to use void * instea= d=20 of u8 * for data buffers works out, then this union will become just a=20 struct. [snip dittos of the above topic] > >> + >> +/** >> + * @number_of_fingers - describes the maximum number of fingers the= 2-Dsensor >> + * supports. >> + * @has_rel - the sensor supports relative motion reporting. >> + * @has_abs - the sensor supports absolute poition reporting. >> + * @has_gestures - the sensor supports gesture reporting. >> + * @has_sensitivity_adjust - the sensor supports a global sensitivi= ty >> + * adjustment. >> + * @configurable - the sensor supports various configuration option= s. >> + * @num_of_x_electrodes - the maximum number of electrodes the 2-D= sensor >> + * supports on the X axis. >> + * @num_of_y_electrodes - the maximum number of electrodes the 2-D= sensor >> + * supports on the Y axis. >> + * @max_electrodes - the total number of X and Y electrodes that ma= y be >> + * configured. >> + * @abs_data_size - describes the format of data reported by the ab= solute >> + * data source. Only one format (the kind used here) is supported = at this >> + * time. >> + * @has_anchored_finger - then the sensor supports the high-precisi= on second >> + * finger tracking provided by the manual tracking and motion sensi= tivity >> + * options. >> + * @has_adjust_hyst - the difference between the finger release thr= eshold and >> + * the touch threshold. >> + * @has_dribble - the sensor supports the generation of dribble int= errupts, >> + * which may be enabled or disabled with the dribble control bit. >> + * @f11_2d_query6 - reserved. >> + * @has_single_tap - a basic single-tap gesture is supported. >> + * @has_tap_n_hold - tap-and-hold gesture is supported. >> + * @has_double_tap - double-tap gesture is supported. >> + * @has_early_tap - early tap is supported and reported as soon as = the finger >> + * lifts for any tap event that could be interpreted as either a si= ngletap >> + * or as the first tap of a double-tap or tap-and-hold gesture. >> + * @has_flick - flick detection is supported. >> + * @has_press - press gesture reporting is supported. >> + * @has_pinch - pinch gesture detection is supported. >> + * @has_palm_det - the 2-D sensor notifies the host whenever a larg= e conductive >> + * object such as a palm or a cheek touches the 2-D sensor. >> + * @has_rotate - rotation gesture detection is supported. >> + * @has_touch_shapes - TouchShapes are supported. A TouchShape is = a fixed >> + * rectangular area on the sensor that behaves like a capacitive bu= tton. >> + * @has_scroll_zones - scrolling areas near the sensor edges are su= pported. >> + * @has_individual_scroll_zones - if 1, then 4 scroll zones are sup= ported; >> + * if 0, then only two are supported. >> + * @has_multi_finger_scroll - the multifinger_scrolling bit will be= setwhen >> + * more than one finger is involved in a scrolling action. >> + * @nbr_touch_shapes - the total number of touch shapes supported. >> + */ >> +struct f11_2d_sensor_query { >> + union { >> + struct { >> + /* query1 */ >> + u8 number_of_fingers:3; >> + bool has_rel:1; >> + bool has_abs:1; >> + bool has_gestures:1; >> + bool has_sensitivity_adjust:1; >> + bool configurable:1; >> + /* query2 */ >> + u8 num_of_x_electrodes:7; >> + u8 reserved_1:1; >> + /* query3 */ >> + u8 num_of_y_electrodes:7; >> + u8 reserved_2:1; >> + /* query4 */ >> + u8 max_electrodes:7; >> + u8 reserved_3:1; >> + } __attribute__((__packed__)); >> + u8 f11_2d_query1__4[4]; >> + }; >> + >> + union { >> + struct { >> + u8 abs_data_size:3; >> + bool has_anchored_finger:1; >> + bool has_adj_hyst:1; >> + bool has_dribble:1; >> + u8 reserved_4:2; >> + } __attribute__((__packed__)); >> + u8 f11_2d_query5; >> + }; >> + >> + u8 f11_2d_query6; >> + >> + union { >> + struct { >> + bool has_single_tap:1; >> + bool has_tap_n_hold:1; >> + bool has_double_tap:1; >> + bool has_early_tap:1; >> + bool has_flick:1; >> + bool has_press:1; >> + bool has_pinch:1; >> + bool padding:1; >> + >> + bool has_palm_det:1; >> + bool has_rotate:1; >> + bool has_touch_shapes:1; >> + bool has_scroll_zones:1; >> + bool has_individual_scroll_zones:1; >> + bool has_multi_finger_scroll:1; >> + } __attribute__((__packed__)); >> + u8 f11_2d_query7__8[2]; >> + }; >> + >> + union f11_2d_query9 query9; >> + >> + union { >> + struct { >> + u8 nbr_touch_shapes:5; >> + } __attribute__((__packed__)); >> + u8 f11_2d_query10; >> + }; >> +}; > > Ditto. It is nice with some documentation, but in this case, it might > make sense to add it on the same line in the struct. Also, since an M= T > device outputs the raw position data and leaves the gestures to > userspace, this whole construct is more complex than it needs to be. See remarks on gestures at the top of this email. > >> + >> +/** >> + * @reporting_mode - controls how often finger position data is rep= orted. >> + * @abs_pos_filt - when set, enables various noise and jitter filte= ring >> + * algorithms for absolute reports. >> + * @rel_pos_filt - when set, enables various noise and jitter filte= ring >> + * algorithms for relative reports. >> + * @rel_ballistics - enables ballistics processing for the relative= finger >> + * motion on the 2-D sensor. >> + * @dribble - enables the dribbling feature. >> + * @report_beyond_clip - when this is set, fingers outside the acti= ve area >> + * specified by the x_clip and y_clip registers will be reported, b= ut with >> + * reported finger position clipped to the edge of the active area. >> + * @palm_detect_thresh - the threshold at which a wide finger is co= nsidered a >> + * palm. A value of 0 inhibits palm detection. >> + * @motion_sensitivity - specifies the threshold an anchored finger= must move >> + * before it is considered no longer anchored. High values mean mo= re >> + * sensitivity. >> + * @man_track_en - for anchored finger tracking, whether the host (= 1) or the >> + * device (0) determines which finger is the tracked finger. >> + * @man_tracked_finger - when man_track_en is 1, specifies whether = finger 0 or >> + * finger 1 is the tracked finger. >> + * @delta_x_threshold - 2-D position update interrupts are inhibite= d unless >> + * the finger moves more than a certain threshold distance along th= e X axis. >> + * @delta_y_threshold - 2-D position update interrupts are inhibite= d unless >> + * the finger moves more than a certain threshold distance along th= e Y axis. >> + * @velocity - When rel_ballistics is set, this register defines th= e >> + * velocity ballistic parameter applied to all relative motion even= ts. >> + * @acceleration - When rel_ballistics is set, this register define= s the >> + * acceleration ballistic parameter applied to all relative motion = events. >> + * @sensor_max_x_pos - the maximum X coordinate reported by the sen= sor. >> + * @sensor_max_y_pos - the maximum Y coordinate reported by the sen= sor. >> + */ >> +union f11_2d_ctrl0_9 { >> + struct { >> + /* F11_2D_Ctrl0 */ >> + u8 reporting_mode:3; >> + bool abs_pos_filt:1; >> + bool rel_pos_filt:1; >> + bool rel_ballistics:1; >> + bool dribble:1; >> + bool report_beyond_clip:1; >> + /* F11_2D_Ctrl1 */ >> + u8 palm_detect_thres:4; >> + u8 motion_sensitivity:2; >> + bool man_track_en:1; >> + bool 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; >> + } __attribute__((__packed__)); >> + struct { >> + u8 regs[10]; >> + u16 address; >> + } __attribute__((__packed__)); >> +}; >> + >> +/** >> + * @single_tap_int_enable - enable tap gesture recognition. >> + * @tap_n_hold_int_enable - enable tap-and-hold gesture recognition= =2E >> + * @double_tap_int_enable - enable double-tap gesture recognition. >> + * @early_tap_int_enable - enable early tap notification. >> + * @flick_int_enable - enable flick detection. >> + * @press_int_enable - enable press gesture recognition. >> + * @pinch_int_enable - enable pinch detection. >> + */ >> +union f11_2d_ctrl10 { >> + struct { >> + bool single_tap_int_enable:1; >> + bool tap_n_hold_int_enable:1; >> + bool double_tap_int_enable:1; >> + bool early_tap_int_enable:1; >> + bool flick_int_enable:1; >> + bool press_int_enable:1; >> + bool pinch_int_enable:1; >> + } __attribute__((__packed__)); >> + u8 reg; >> +}; >> + >> +/** >> + * @palm_detect_int_enable - enable palm detection feature. >> + * @rotate_int_enable - enable rotate gesture detection. >> + * @touch_shape_int_enable - enable the TouchShape feature. >> + * @scroll_zone_int_enable - enable scroll zone reporting. >> + * @multi_finger_scroll_int_enable - enable the multfinger scroll f= eature. >> + */ >> +union f11_2d_ctrl11 { >> + struct { >> + bool palm_detect_int_enable:1; >> + bool rotate_int_enable:1; >> + bool touch_shape_int_enable:1; >> + bool scroll_zone_int_enable:1; >> + bool multi_finger_scroll_int_enable:1; >> + } __attribute__((__packed__)); >> + u8 reg; >> +}; >> + >> +union f11_2d_ctrl12 { >> + struct { >> + u8 sensor_map:7; >> + bool xy_sel:1; >> + } __attribute__((__packed__)); >> + u8 reg; >> +}; >> + >> +/** >> + * @sens_adjustment - allows a host to alter the overall sensitivit= y ofa >> + * 2-D sensor. A positive value in this register will make the sens= or more >> + * sensitive than the factory defaults, and a negative value will m= ake it >> + * less sensitive. >> + * @hyst_adjustment - increase the touch/no-touch hysteresis by 2 Z= -units for >> + * each one unit increment in this setting. >> + */ >> +union f11_2d_ctrl14 { >> + struct { >> + s8 sens_adjustment:5; >> + u8 hyst_adjustment:3; >> + } __attribute__((__packed__)); >> + u8 reg; >> +}; >> + >> +/** >> + * @max_tap_time - the maximum duration of a tap, in 10-millisecond= units. >> + */ >> +union f11_2d_ctrl15 { >> + struct { >> + u8 max_tap_time:8; >> + } __attribute__((__packed__)); >> + u8 reg; >> +}; >> + >> +/** >> + * @min_press_time - The minimum duration required for stationary f= inger(s) to >> + * generate a press gesture, in 10-millisecond units. >> + */ >> +union f11_2d_ctrl16 { >> + struct { >> + u8 min_press_time:8; >> + } __attribute__((__packed__)); >> + u8 reg; >> +}; >> + >> +/** >> + * @max_tap_distance - Determines the maximum finger movement allow= ed during >> + * a tap, in 0.1-millimeter units. >> + */ >> +union f11_2d_ctrl17 { >> + struct { >> + u8 max_tap_distance:8; >> + } __attribute__((__packed__)); >> + u8 reg; >> +}; >> + >> +/** >> + * @min_flick_distance - the minimum finger movement for a flick ge= sture, >> + * in 1-millimeter units. >> + * @min_flick_speed - the minimum finger speed for a flick gesture,= in >> + * 10-millimeter/second units. >> + */ >> +union f11_2d_ctrl18_19 { >> + struct { >> + u8 min_flick_distance:8; >> + u8 min_flick_speed:8; >> + } __attribute__((__packed__)); >> + u8 reg[2]; >> +}; >> + >> +/** >> + * @pen_detect_enable - enable reporting of stylus activity. >> + * @pen_jitter_filter_enable - Setting this enables the stylus anti= -jitter >> + * filter. >> + * @pen_z_threshold - This is the stylus-detection lower threshold.= Smaller >> + * values result in higher sensitivity. >> + */ >> +union f11_2d_ctrl20_21 { >> + struct { >> + bool pen_detect_enable:1; >> + bool pen_jitter_filter_enable:1; >> + u8 ctrl20_reserved:6; >> + u8 pen_z_threshold:8; >> + } __attribute__((__packed__)); >> + u8 reg[2]; >> +}; > > Given that most of the above will not be used in this driver, it can > probably be compressed quite a bit. I'm not sure why you say these will not be used. They are currently or= =20 soon will be used by userspace control and configuration programs. > >> + >> +/** >> + * These are not accessible through sysfs yet. >> + * >> + * @proximity_detect_int_en - enable proximity detection feature. >> + * @proximity_jitter_filter_en - enables an anti-jitter filter on p= roximity >> + * data. >> + * @proximity_detection_z_threshold - the threshold for finger-prox= imity >> + * detection. >> + * @proximity_delta_x_threshold - In reduced-reporting modes, this = is the >> + * threshold for proximate-finger movement in the direction paralle= l tothe >> + * X-axis. >> + * @proximity_delta_y_threshold - In reduced-reporting modes, this = is the >> + * threshold for proximate-finger movement in the direction paralle= l tothe >> + * Y-axis. >> + * * @proximity_delta_Z_threshold - In reduced-reporting modes, thi= s isthe >> + * threshold for proximate-finger movement in the direction paralle= l tothe >> + * Z-axis. >> + */ >> +union f11_2d_ctrl22_26 { >> + struct { >> + /* control 22 */ >> + bool proximity_detect_int_en:1; >> + bool 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; >> + } __attribute__((__packed__)); >> + u8 regs[5]; >> +}; >> + >> +/** >> + * @palm_detecy_sensitivity - When this value is small, smaller obj= ectswill >> + * be identified as palms; when this value is large, only larger ob= jects will >> + * be identified as palms. 0 represents the factory default. >> + * @suppress_on_palm_detect - when set, all F11 interrupts except p= alm_detect >> + * are suppressed while a palm is detected. >> + */ >> +union f11_2d_ctrl27 { >> + struct { >> + s8 palm_detect_sensitivity:4; >> + bool suppress_on_palm_detect:1; >> + u8 f11_2d_ctrl27_b5__7:3; >> + } __attribute__((__packed__)); >> + u8 regs[1]; >> +}; >> + >> +/** >> + * @multi_finger_scroll_mode - allows choice of multi-finger scroll= mode and >> + * determines whether and how X or Y displacements are reported. >> + * @edge_motion_en - enables the edge_motion feature. >> + * @multi_finger_scroll_momentum - controls the length of time that= scrolling >> + * continues after fingers have been lifted. >> + */ >> +union f11_2d_ctrl28 { >> + struct { >> + u8 multi_finger_scroll_mode:2; >> + bool edge_motion_en:1; >> + bool f11_2d_ctrl28b_3:1; >> + u8 multi_finger_scroll_momentum:4; >> + } __attribute__((__packed__)); >> + u8 regs[1]; >> +}; >> + >> +/** >> + * @z_touch_threshold - Specifies the finger-arrival Z threshold. L= argevalues >> + * may cause smaller fingers to be rejected. >> + * @z_touch_hysteresis - Specifies the difference between the finge= r-arrival >> + * Z threshold and the finger-departure Z threshold. >> + */ >> +union f11_2d_ctrl29_30 { >> + struct { >> + u8 z_touch_threshold; >> + u8 z_touch_hysteresis; >> + } __attribute__((__packed__)); >> + 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; >> +}; > > Will any of this data be used at all? Yes. > >> + >> +/** >> + * @x_msb - top 8 bits of X finger position. >> + * @y_msb - top 8 bits of Y finger position. >> + * @x_lsb - bottom 4 bits of X finger position. >> + * @y_lsb - bottom 4 bits of Y finger position. >> + * @w_y - contact patch width along Y axis. >> + * @w_x - contact patch width along X axis. >> + * @z - finger Z value (proxy for pressure). >> + */ >> +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; >> +}; >> + >> +/** >> + * @delta_x - relative motion along X axis. >> + * @delta_y - relative motion along Y axis. >> + */ >> +struct f11_2d_data_6_7 { >> + s8 delta_x; >> + s8 delta_y; >> +}; >> + >> +/** >> + * @single_tap - a single tap was recognized. >> + * @tap_and_hold - a tap-and-hold gesture was recognized. >> + * @double_tap - a double tap gesture was recognized. >> + * @early_tap - a tap gesture might be happening. >> + * @flick - a flick gesture was detected. >> + * @press - a press gesture was recognized. >> + * @pinch - a pinch gesture was detected. >> + */ >> +struct f11_2d_data_8 { >> + bool single_tap:1; >> + bool tap_and_hold:1; >> + bool double_tap:1; >> + bool early_tap:1; >> + bool flick:1; >> + bool press:1; >> + bool pinch:1; >> +}; >> + >> +/** >> + * @palm_detect - a palm or other large object is in contact with t= he sensor. >> + * @rotate - a rotate gesture was detected. >> + * @shape - a TouchShape has been activated. >> + * @scrollzone - scrolling data is available. >> + * @finger_count - number of fingers involved in the reported gestu= re. >> + */ >> +struct f11_2d_data_9 { >> + bool palm_detect:1; >> + bool rotate:1; >> + bool shape:1; >> + bool scrollzone:1; >> + u8 finger_count:3; >> +}; >> + >> +/** >> + * @pinch_motion - when a pinch gesture is detected, this is the ch= angein >> + * distance between the two fingers since this register was last re= ad. >> + */ >> +struct f11_2d_data_10 { >> + s8 pinch_motion; >> +}; >> + >> +/** >> + * @x_flick_dist - when a flick gesture is detected, the distance = of flick >> + * gesture in X direction. >> + * @y_flick_dist - when a flick gesture is detected, the distance = of flick >> + * gesture in Y direction. >> + * @flick_time - the total time of the flick gesture, in 10ms units= =2E >> + */ >> +struct f11_2d_data_10_12 { >> + s8 x_flick_dist; >> + s8 y_flick_dist; >> + u8 flick_time; >> +}; >> + >> +/** >> + * @motion - when a rotate gesture is detected, the accumulated dis= tance >> + * of the rotate motion. Clockwise motion is positive and countercl= ockwise >> + * motion is negative. >> + * @finger_separation - when a rotate gesture is detected, the dist= ance >> + * between the fingers. >> + */ >> +struct f11_2d_data_11_12 { >> + s8 motion; >> + u8 finger_separation; >> +}; >> + >> +/** >> + * @shape_n - a bitmask of the currently activate TouchShapes (if a= ny). >> + */ >> +struct f11_2d_data_13 { >> + u8 shape_n; >> +}; >> + >> +/** >> + * @horizontal - chiral scrolling distance in the X direction. >> + * @vertical - chiral scrolling distance in the Y direction. >> + */ >> +struct f11_2d_data_14_15 { >> + s8 horizontal; >> + s8 vertical; >> +}; >> + >> +/** >> + * @x_low - scroll zone motion along the lower edge of the sensor. >> + * @y_right - scroll zone motion along the right edge of the sensor= =2E >> + * @x_upper - scroll zone motion along the upper edge of the sensor= =2E >> + * @y_left - scroll zone motion along the left edge of the sensor. >> + */ >> +struct f11_2d_data_14_17 { >> + s8 x_low; >> + s8 y_right; >> + s8 x_upper; >> + s8 y_left; >> +}; >> + >> +struct f11_2d_data { >> + u8 *f_state; >> + const struct f11_2d_data_1_5 *abs_pos; >> + const struct f11_2d_data_6_7 *rel_pos; >> + const struct f11_2d_data_8 *gest_1; >> + const struct f11_2d_data_9 *gest_2; >> + const struct f11_2d_data_10 *pinch; >> + const struct f11_2d_data_10_12 *flick; >> + const struct f11_2d_data_11_12 *rotate; >> + const struct f11_2d_data_13 *shapes; >> + const struct f11_2d_data_14_15 *multi_scroll; >> + const struct f11_2d_data_14_17 *scroll_zones; >> +}; >> + >> +struct f11_2d_sensor { >> + struct rmi_f11_2d_axis_alignment axis_align; >> + struct f11_2d_sensor_query sens_query; >> + struct f11_2d_data data; >> + int prev_x[F11_MAX_NUM_OF_FINGERS]; >> + int prev_y[F11_MAX_NUM_OF_FINGERS]; >> + u16 max_x; >> + u16 max_y; >> + u8 nbr_fingers; >> + u8 finger_tracker[F11_MAX_NUM_OF_FINGERS]; >> + u8 *data_pkt; >> + int pkt_size; >> + u8 sensor_index; >> + u8 *button_map; >> + struct rmi_f11_virtualbutton_map virtual_buttons; >> + bool type_a; >> + char input_name[MAX_NAME_LENGTH]; >> + char input_phys[MAX_NAME_LENGTH]; >> + struct input_dev *input; >> + struct input_dev *mouse_input; >> + struct rmi_function_container *fc; >> + >> +#ifdef CONFIG_RMI4_DEBUG >> + struct dentry *debugfs_flip; >> + struct dentry *debugfs_clip; >> + struct dentry *debugfs_delta_threshold; >> + struct dentry *debugfs_offset; >> + struct dentry *debugfs_swap; >> + struct dentry *debugfs_type_a; >> +#endif >> +}; > > Up to this point in the file, very little is essential to the input d= eivce. I really don't understand what you're saying here. If we remove the=20 things corresponding to the data reported by the sensor, how can that=20 data be read and reported to user space (whether by input subsystem or=20 via sysfs)? If we remove the configuration parameters, how should the=20 sensor be configured to operate correctly? > >> + >> +struct f11_data { >> + struct f11_2d_device_query dev_query; >> + struct f11_2d_ctrl dev_controls; >> + struct mutex dev_controls_mutex; >> + u16 rezero_wait_ms; >> + struct f11_2d_sensor sensors[F11_MAX_NUM_OF_SENSORS]; >> + >> +#ifdef CONFIG_RMI4_DEBUG >> + struct dentry *debugfs_rezero_wait; >> +#endif >> +}; >> + >> +enum finger_state_values { >> + F11_NO_FINGER =3D 0x00, >> + F11_PRESENT =3D 0x01, >> + F11_INACCURATE =3D 0x02, >> + F11_RESERVED =3D 0x03 >> +}; >> + >> +/* ctrl sysfs files */ >> +show_store_union_struct_prototype(abs_pos_filt) >> +show_store_union_struct_prototype(z_touch_threshold) >> +show_store_union_struct_prototype(z_touch_hysteresis) >> + >> +#ifdef CONFIG_RMI4_DEBUG >> + >> +struct sensor_debugfs_data { >> + bool done; >> + struct f11_2d_sensor *sensor; >> +}; > > And this is really needed? Yes - please see notes on configuration at the top of this email. > > [...] > >> +static void rmi_f11_abs_pos_report(struct f11_data *f11, >> + struct f11_2d_sensor *sensor, >> + u8 finger_state, u8 n_finger) >> +{ >> + struct f11_2d_data *data =3D &sensor->data; >> + struct rmi_f11_2d_axis_alignment *axis_align =3D &sensor->axis_ali= gn; >> + u8 prev_state =3D sensor->finger_tracker[n_finger]; > > No need to keep track of the old state. We'll look into that. In previous versions of this code, some of the=20 user space implementations didn't behave correctly if we didn't do that= =2E=20 However, since that time we've dropped support for older user spaces,= =20 so maybe we don't need that any more. >> + int x, y, z; >> + int w_x, w_y, w_max, w_min, orient; >> + int temp; >> + >> + if (prev_state && !finger_state) { >> + /* this is a release */ >> + x =3D y =3D z =3D w_max =3D w_min =3D orient =3D 0; > > This data is not sent during a release. OK. > >> + } else if (!prev_state && !finger_state) { >> + /* nothing to report */ >> + return; >> + } else { [snip] >> + x =3D max(axis_align->clip_X_low, x); >> + y =3D max(axis_align->clip_Y_low, y); >> + if (axis_align->clip_X_high) >> + x =3D min(axis_align->clip_X_high, x); >> + if (axis_align->clip_Y_high) >> + y =3D min(axis_align->clip_Y_high, y); > > Why is the clipping configurable? Because during prototyping of new products, engineers sometimes use a=20 sensor that is bigger than the display and not necessarily aligned to=20 the origin. This is frequent enough a use case that we include it in=20 the driver as a convenience feature in debugfs. > >> + >> + } >> + >> + /* Some UIs ignore W of zero, so we fudge it to 1 for pens. */ >> + if (IS_ENABLED(CONFIG_RMI4_F11_PEN) && >> + get_tool_type(sensor, finger_state) =3D=3D MT_TOOL_PEN) { >> + w_max =3D max(1, w_max); >> + w_min =3D max(1, w_min); >> + } > > Is this not true for all tool types? In our experience, no. It ought to be true in an ideal world, but=20 unfortunately we aren't living in one :-(. >> + >> + if (sensor->type_a) { >> + input_report_abs(sensor->input, ABS_MT_TRACKING_ID, n_finger); >> + input_report_abs(sensor->input, ABS_MT_TOOL_TYPE, >> + get_tool_type(sensor, finger_state)); >> + } else { >> + input_mt_slot(sensor->input, n_finger); >> + input_mt_report_slot_state(sensor->input, >> + get_tool_type(sensor, finger_state), finger_state); >> + } > > The driver should only report MT-B, please. Is that valid even if we have a type A sensor? >> + input_report_abs(sensor->input, ABS_MT_PRESSURE, z); >> + input_report_abs(sensor->input, ABS_MT_TOUCH_MAJOR, w_max); >> + input_report_abs(sensor->input, ABS_MT_TOUCH_MINOR, w_min); >> + input_report_abs(sensor->input, ABS_MT_ORIENTATION, orient); >> + input_report_abs(sensor->input, ABS_MT_POSITION_X, x); >> + input_report_abs(sensor->input, ABS_MT_POSITION_Y, y); > > Only if finger_state is true. OK. > >> + dev_dbg(&sensor->fc->dev, >> + "finger[%d]:%d - x:%d y:%d z:%d w_max:%d w_min:%d\n", >> + n_finger, finger_state, x, y, z, w_max, w_min); >> + /* MT sync between fingers */ >> + if (sensor->type_a) >> + input_mt_sync(sensor->input); >> + >> + sensor->finger_tracker[n_finger] =3D finger_state; >> +} >> + >> +#ifdef CONFIG_RMI4_VIRTUAL_BUTTON >> +static int rmi_f11_virtual_button_handler(struct f11_2d_sensor *sen= sor) >> +{ >> + int i; >> + int x; >> + int y; >> + struct rmi_button_map *virtualbutton_map; >> + >> + if (sensor->sens_query.has_gestures && >> + sensor->data.gest_1->single_tap) { >> + virtualbutton_map =3D &sensor->virtualbutton_map; >> + x =3D ((sensor->data.abs_pos[0].x_msb << 4) | >> + sensor->data.abs_pos[0].x_lsb); >> + y =3D ((sensor->data.abs_pos[0].y_msb << 4) | >> + sensor->data.abs_pos[0].y_lsb); >> + for (i =3D 0; i < virtualbutton_map->buttons; i++) { >> + if (INBOX(x, y, virtualbutton_map->map[i])) { >> + input_report_key(sensor->input, >> + virtualbutton_map->map[i].code, 1); >> + input_report_key(sensor->input, >> + virtualbutton_map->map[i].code, 0); >> + input_sync(sensor->input); >> + return 0; >> + } >> + } >> + } >> + return 0; >> +} >> +#else >> +#define rmi_f11_virtual_button_handler(sensor) >> +#endif > > No virtual buttons, please, this can easily be mapped in userspace. Not for all user spaces. But we can remove this from the next submissi= on. > >> +static void rmi_f11_finger_handler(struct f11_data *f11, >> + struct f11_2d_sensor *sensor) >> +{ >> + const u8 *f_state =3D sensor->data.f_state; >> + u8 finger_state; >> + u8 finger_pressed_count; >> + u8 i; >> + >> + for (i =3D 0, finger_pressed_count =3D 0; i < sensor->nbr_fingers;= i++) { >> + /* Possible of having 4 fingers per f_statet register */ >> + finger_state =3D GET_FINGER_STATE(f_state, i); >> + >> + if (finger_state =3D=3D F11_RESERVED) { >> + pr_err("%s: Invalid finger state[%d]:0x%02x.", __func__, >> + i, finger_state); >> + continue; >> + } else if ((finger_state =3D=3D F11_PRESENT) || >> + (finger_state =3D=3D F11_INACCURATE)) { >> + finger_pressed_count++; >> + } >> + >> + if (sensor->data.abs_pos) >> + rmi_f11_abs_pos_report(f11, sensor, finger_state, i); >> + >> + if (sensor->data.rel_pos) >> + rmi_f11_rel_pos_report(sensor, i); >> + } >> + input_report_key(sensor->input, BTN_TOUCH, finger_pressed_count); > > Please use input_mt_sync_frame() here instead. OK > >> + input_sync(sensor->input); >> +} > > Stopping here. -- To unsubscribe from this list: send the line "unsubscribe linux-input" = in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html