by Christopher Heiny

[permalink] [raw]

Subject: Re: [RFC PATCH 02/06] input/rmi4: Core files

On 10/23/2012 05:11 PM, Dmitry Torokhov wrote:
> On Tuesday, October 23, 2012 04:46:28 PM Christopher Heiny wrote:
>> On 10/11/2012 01:13 AM, Dmitry Torokhov wrote:
>>> On Thu, Oct 11, 2012 at 04:15:56AM +0000, Christopher Heiny wrote:
>>>> On Thursday, October 11, 2012 02:21:53 AM you wrote:
>>>>> On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny <[email protected]>
> wrote:

[snip]

>>>>>> +static int process_interrupt_requests(struct rmi_device *rmi_dev)
>>>>>> +{
>>>>>> + struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
>>>>>> + struct device *dev = &rmi_dev->dev;
>>>>>> + struct rmi_function_container *entry;
>>>>>> + u8 irq_status[data->num_of_irq_regs];
>>>>>
>>>>> Looking at this...
>>>>>
>>>>> What does the data->num_of_irq_regs actually contain?
>>>>>
>>>>> I just fear that it is something constant like always 2 or always 4,
>>>>> so there is actually, in reality, a 16 or 32 bit register hiding in
>>>>> there.
>>>>>
>>>>> In that case what you should do is to represent it as a u16 or u32 here,
>>>>> just or the bits into a status word, and then walk over that status
>>>>> word with something like ffs(bitword); ...
>>>>
>>>> Nope, it's not constant. In theory, and RMI4 based sensor can have up
>>>> to 128 functions (in practice, it's far fewer), and each function can
>>>> have as many as 7 interrupts. So the number of IRQ registers can vary
>>>> from RMI4 sensor to RMI4 sensor, and needs to be computed during the
>>>> scan of the product descriptor table.
>>>
>>> Is it a good idea to have it on stack then? Should it be part of
>>> rmi_device instead?
>>
>> It's not coming off the stack. We're allocating it via devm_kzalloc()
>> in rmi_driver_probe().
>
> No, look at the part of the code that was quoted. "u8 irq_status[data-
> num_of_irq_regs];" is on stack.

Sorry - I thought you were referring to data->num_of_irq_regs rather
than irq_status. We'll move that.

2012-10-24 17:49:15

by Mark Brown

[permalink] [raw]

Subject: Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation

On Tue, Oct 23, 2012 at 03:10:20PM -0700, Christopher Heiny wrote:
> On 10/11/2012 10:16 PM, Mark Brown wrote:
> >On Thu, Oct 11, 2012 at 03:56:22AM +0000, Christopher Heiny wrote:

> >Fix your mailer to word wrap within paragraphs.

> Sorry - I was on the road and had to use a web interface. It looked
> OK during composition. Is this better?

Yes, that's great - thanks.

> However, a general capability in SPI core is a fine idea for future
> work, once we get the RMI4 driver ironed out. Can we agree to defer
> that for now?

Obviously there's no pressure to implement the framework feature,
however it shouldn't be implemented at the driver level.

Attachments:

(No filename) (658.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-10-25 21:39:08

by Christopher Heiny

[permalink] [raw]

Subject: Re: [RFC PATCH 06/06] input/rmi4: F11 - 2D touch interface

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
replies sent a couple of weeks ago, but don't see it in the archives. My
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 find
> 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
subsystem is not the only thing that needs to be done - there needs to
be a way to configure and control the operation of the touch sensor,
both during product prototyping/development (we've partitioned that
stuff into debugfs) and during normal operation (we've put that stuff in
sysfs). If we remove the related code, that would not be possible. If
we move it to a different subsystem, where do you suggest it be moved to?

The gesture data is a different matter. Although the most popular user
space implementations may contain gesture recognition engines, not all
of them do. In that case, it is desirable to have the touch sensor
firmware perform basic gesture recognition. However, there is at this
time no way standard way to transfer that via the input subsystem. We
chose to use sysfs for this, but alternatively we could investigate
using a custom event stream in the input subsystem. We'd also be quite
happy to work with you to define a standard set of gesture events for
basic gestures.

>
>> diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.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 >= box.x && x < (box.x + box.width) \
>> + && y >= 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, int index);
>
> Please try to get rid of these.

OK

[snip]

>> +
>> +/**
>> + * @rezero - writing 1 to this will cause the sensor to calibrate to 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
union/struct mechanism you see here). In a couple of cases, that means
we use a struct to control a single bit, instead of a constant. The end
result is the same, and we find that not mixing our idioms makes the
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 registers
>> + * F11_2D_Ctrl29 through F11_2D_Ctrl33 exist.
>> + * @has_pos_interpolation_tuning - TBD
>> + * @has_w_tuning - the sensor supports Wx and Wy scaling and registers
>> + * F11_2D_Ctrl36 through F11_2D_Ctrl39 exist.
>> + * @has_pitch_info - the X and Y pitches of the sensor electrodes can be
>> + * configured and registers F11_2D_Ctrl40 and F11_2D_Ctrl41 exist.
>> + * @has_default_finger_width - the default finger width settings for the
>> + * sensor can be configured and registers F11_2D_Ctrl42 through F11_2D_Ctrl44
>> + * exist.
>> + * @has_segmentation_aggressiveness - the sensor’s ability to distinguish
>> + * multiple objects close together can be configured and register F11_2D_Ctrl45
>> + * exists.
>> + * @has_tx_rw_clip - the inactive outside borders of the sensor can be
>> + * configured and registers F11_2D_Ctrl46 through F11_2D_Ctrl49 exist.
>> + * @has_drumming_correction - the sensor can be configured to distinguish
>> + * 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.

If Dmitry's suggestion (discussed in other emails) to use void * instead
of u8 * for data buffers works out, then this union will become just a
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 sensitivity
>> + * adjustment.
>> + * @configurable - the sensor supports various configuration options.
>> + * @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 may be
>> + * configured.
>> + * @abs_data_size - describes the format of data reported by the absolute
>> + * data source. Only one format (the kind used here) is supported at this
>> + * time.
>> + * @has_anchored_finger - then the sensor supports the high-precision second
>> + * finger tracking provided by the manual tracking and motion sensitivity
>> + * options.
>> + * @has_adjust_hyst - the difference between the finger release threshold and
>> + * the touch threshold.
>> + * @has_dribble - the sensor supports the generation of dribble interrupts,
>> + * 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 singletap
>> + * 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 large 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 button.
>> + * @has_scroll_zones - scrolling areas near the sensor edges are supported.
>> + * @has_individual_scroll_zones - if 1, then 4 scroll zones are supported;
>> + * 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 MT
> 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 reported.
>> + * @abs_pos_filt - when set, enables various noise and jitter filtering
>> + * algorithms for absolute reports.
>> + * @rel_pos_filt - when set, enables various noise and jitter filtering
>> + * 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 active area
>> + * specified by the x_clip and y_clip registers will be reported, but with
>> + * reported finger position clipped to the edge of the active area.
>> + * @palm_detect_thresh - the threshold at which a wide finger is considered 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 more
>> + * 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 inhibited unless
>> + * the finger moves more than a certain threshold distance along the X axis.
>> + * @delta_y_threshold - 2-D position update interrupts are inhibited unless
>> + * the finger moves more than a certain threshold distance along the Y axis.
>> + * @velocity - When rel_ballistics is set, this register defines the
>> + * velocity ballistic parameter applied to all relative motion events.
>> + * @acceleration - When rel_ballistics is set, this register defines the
>> + * acceleration ballistic parameter applied to all relative motion events.
>> + * @sensor_max_x_pos - the maximum X coordinate reported by the sensor.
>> + * @sensor_max_y_pos - the maximum Y coordinate reported by the sensor.
>> + */
>> +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.
>> + * @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 feature.
>> + */
>> +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 sensitivity ofa
>> + * 2-D sensor. A positive value in this register will make the sensor more
>> + * sensitive than the factory defaults, and a negative value will make 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 finger(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 allowed 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 gesture,
>> + * 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
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 proximity
>> + * data.
>> + * @proximity_detection_z_threshold - the threshold for finger-proximity
>> + * detection.
>> + * @proximity_delta_x_threshold - In reduced-reporting modes, this is the
>> + * threshold for proximate-finger movement in the direction parallel tothe
>> + * X-axis.
>> + * @proximity_delta_y_threshold - In reduced-reporting modes, this is the
>> + * threshold for proximate-finger movement in the direction parallel tothe
>> + * Y-axis.
>> + * * @proximity_delta_Z_threshold - In reduced-reporting modes, this isthe
>> + * threshold for proximate-finger movement in the direction parallel 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 objectswill
>> + * be identified as palms; when this value is large, only larger objects will
>> + * be identified as palms. 0 represents the factory default.
>> + * @suppress_on_palm_detect - when set, all F11 interrupts except palm_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. Largevalues
>> + * may cause smaller fingers to be rejected.
>> + * @z_touch_hysteresis - Specifies the difference between the finger-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 the 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 gesture.
>> + */
>> +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 changein
>> + * distance between the two fingers since this register was last read.
>> + */
>> +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.
>> + */
>> +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 distance
>> + * of the rotate motion. Clockwise motion is positive and counterclockwise
>> + * motion is negative.
>> + * @finger_separation - when a rotate gesture is detected, the distance
>> + * between the fingers.
>> + */
>> +struct f11_2d_data_11_12 {
>> + s8 motion;
>> + u8 finger_separation;
>> +};
>> +
>> +/**
>> + * @shape_n - a bitmask of the currently activate TouchShapes (if any).
>> + */
>> +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.
>> + * @x_upper - scroll zone motion along the upper edge of the sensor.
>> + * @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 deivce.

I really don't understand what you're saying here. If we remove the
things corresponding to the data reported by the sensor, how can that
data be read and reported to user space (whether by input subsystem or
via sysfs)? If we remove the configuration parameters, how should the
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 = 0x00,
>> + F11_PRESENT = 0x01,
>> + F11_INACCURATE = 0x02,
>> + F11_RESERVED = 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 = &sensor->data;
>> + struct rmi_f11_2d_axis_alignment *axis_align = &sensor->axis_align;
>> + u8 prev_state = 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
user space implementations didn't behave correctly if we didn't do that.
However, since that time we've dropped support for older user spaces,
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 = y = z = w_max = w_min = orient = 0;
>
> This data is not sent during a release.

OK.

>
>> + } else if (!prev_state && !finger_state) {
>> + /* nothing to report */
>> + return;
>> + } else {

[snip]

>> + x = max(axis_align->clip_X_low, x);
>> + y = max(axis_align->clip_Y_low, y);
>> + if (axis_align->clip_X_high)
>> + x = min(axis_align->clip_X_high, x);
>> + if (axis_align->clip_Y_high)
>> + y = min(axis_align->clip_Y_high, y);
>
> Why is the clipping configurable?

Because during prototyping of new products, engineers sometimes use a
sensor that is bigger than the display and not necessarily aligned to
the origin. This is frequent enough a use case that we include it in
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) == MT_TOOL_PEN) {
>> + w_max = max(1, w_max);
>> + w_min = 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
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] = finger_state;
>> +}
>> +
>> +#ifdef CONFIG_RMI4_VIRTUAL_BUTTON
>> +static int rmi_f11_virtual_button_handler(struct f11_2d_sensor *sensor)
>> +{
>> + 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 = &sensor->virtualbutton_map;
>> + x = ((sensor->data.abs_pos[0].x_msb << 4) |
>> + sensor->data.abs_pos[0].x_lsb);
>> + y = ((sensor->data.abs_pos[0].y_msb << 4) |
>> + sensor->data.abs_pos[0].y_lsb);
>> + for (i = 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 submission.

>
>> +static void rmi_f11_finger_handler(struct f11_data *f11,
>> + struct f11_2d_sensor *sensor)
>> +{
>> + const u8 *f_state = sensor->data.f_state;
>> + u8 finger_state;
>> + u8 finger_pressed_count;
>> + u8 i;
>> +
>> + for (i = 0, finger_pressed_count = 0; i < sensor->nbr_fingers; i++) {
>> + /* Possible of having 4 fingers per f_statet register */
>> + finger_state = GET_FINGER_STATE(f_state, i);
>> +
>> + if (finger_state == F11_RESERVED) {
>> + pr_err("%s: Invalid finger state[%d]:0x%02x.", __func__,
>> + i, finger_state);
>> + continue;
>> + } else if ((finger_state == F11_PRESENT) ||
>> + (finger_state == 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.