Re: [PATCH 4/8 v3] Input: synaptics - add image sensor support

["Henrik Rydberg" <rydberg@euromail.se>]

Hi Daniel,

looks good, some details below.

> +static void synaptics_report_slot_empty(struct input_dev *dev, int slot)
> +{
> +	input_mt_slot(dev, slot);
> +	input_mt_report_slot_state(dev, MT_TOOL_FINGER, false);
> +}
> +
> +static void synaptics_report_slot_sgm(struct input_dev *dev, int slot,
> +				      const struct synaptics_hw_state *sgm)
> +{
> +	input_mt_slot(dev, slot);
> +	input_mt_report_slot_state(dev, MT_TOOL_FINGER, true);
> +	input_report_abs(dev, ABS_MT_POSITION_X, sgm->x);
> +	input_report_abs(dev, ABS_MT_POSITION_Y, invert_y(sgm->y));
> +	input_report_abs(dev, ABS_MT_PRESSURE, sgm->z);
> +	/* SGM can sometimes contain valid width */
> +	if (sgm->w >= 4)
> +		input_report_abs(dev, ABS_MT_TOUCH_MAJOR, sgm->w);
> +}

The ABS_MT_TOUCH_MAJOR is supposed to have zero intercept, to remain
compatible with user space handling of type A devices. Also, the scale
should match the screen/pad size, such that the actual size of the
touch area can be deduced. In addition, based on the current sensor
technologies, ABS_MT_PRESSURE and ABS_MT_TOUCH_MAJOR are normally
mutually exclusive.

All in all, I would prefer to only report width via (the single-finger
axis) ABS_TOOL_WIDTH, and only for compatibility reasons.

> +
> +static void synaptics_report_slot_agm(struct input_dev *dev, int slot,
> +				      const struct synaptics_hw_state *agm)
> +{
> +	input_mt_slot(dev, slot);
> +	input_mt_report_slot_state(dev, MT_TOOL_FINGER, true);
> +	input_report_abs(dev, ABS_MT_POSITION_X, agm->x);
> +	input_report_abs(dev, ABS_MT_POSITION_Y, invert_y(agm->y));
> +	input_report_abs(dev, ABS_MT_PRESSURE, agm->z);
> +}

With ABS_MT_TOUCH_MAJOR dropped, sgm and agm seems to coincide...

> +
> +static void synaptics_report_mt(struct psmouse *psmouse,
> +				int count,
> +				const struct synaptics_hw_state *sgm)
> +{
> +	struct input_dev *dev = psmouse->dev;
> +	struct synaptics_data *priv = psmouse->private;
> +	struct synaptics_hw_state *agm = &priv->agm;
> +
> +	switch (count) {
> +	case 0:
> +		synaptics_report_slot_empty(dev, 0);
> +		synaptics_report_slot_empty(dev, 1);
> +		break;
> +	case 1:
> +		synaptics_report_slot_sgm(dev, 0, sgm);
> +		synaptics_report_slot_empty(dev, 1);
> +		break;
> +	case 2:
> +	case 3: /* Fall-through case */
> +		synaptics_report_slot_sgm(dev, 0, sgm);
> +		synaptics_report_slot_agm(dev, 1, agm);
> +		break;
> +	}
> +
> +	/* Don't use active slot count to generate BTN_TOOL events. */
> +	input_mt_report_pointer_emulation(dev, false);
> +
> +	/* Send the number of fingers reported by touchpad itself. */
> +	input_mt_report_finger_count(dev, count);
> +
> +	input_report_key(dev, BTN_LEFT, sgm->left);
> +	input_sync(dev);
> +}
> +
> +static void synaptics_image_sensor_process(struct psmouse *psmouse,
> +					   struct synaptics_hw_state *sgm)
> +{
> +	int count;
> +
> +	if (sgm->z == 0)
> +		count = 0;
> +	else if (sgm->w >= 4)
> +		count = 1;
> +	else if (sgm->w == 0)
> +		count = 2;
> +	else
> +		count = 3;
> +
> +	/* Send resulting input events to user space */
> +	synaptics_report_mt(psmouse, count, sgm);
> +}
> +
>  /*
>   *  called for each full received packet from the touchpad
>   */
> @@ -558,6 +642,11 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>  	if (synaptics_parse_hw_state(psmouse->packet, priv, &hw))
>  		return;
>  
> +	if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) {
> +		synaptics_image_sensor_process(psmouse, &hw);
> +		return;
> +	}
> +
>  	if (hw.scroll) {
>  		priv->scroll += hw.scroll;
>  
> @@ -739,7 +828,16 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>  	set_abs_position_params(dev, priv, ABS_X, ABS_Y);
>  	input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
>  
> -	if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
> +	if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) {
> +		input_mt_init_slots(dev, 2);
> +		set_abs_position_params(dev, priv, ABS_MT_POSITION_X,
> +					ABS_MT_POSITION_Y);
> +		/* Image sensors can report per-contact pressure */
> +		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +		/* Image sensors can sometimes report per-contact width */
> +		input_set_abs_params(dev, ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0);
> +	} else if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
> +		/* Non-image sensors with AGM use semi-mt */
>  		__set_bit(INPUT_PROP_SEMI_MT, dev->propbit);
>  		input_mt_init_slots(dev, 2);
>  		set_abs_position_params(dev, priv, ABS_MT_POSITION_X,
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index a9efbf3..0ea7616 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -74,6 +74,8 @@
>   * 2	0x04	reduced filtering	firmware does less filtering on
>   *					position data, driver should watch
>   *					for noise.
> + * 2	0x08	image sensor		image sensor tracks 5 fingers, but only
> + *					reports 2.
>   * 2	0x20	report min		query 0x0f gives min coord reported
>   */
>  #define SYN_CAP_CLICKPAD(ex0c)		((ex0c) & 0x100000) /* 1-button ClickPad */
> @@ -82,6 +84,7 @@
>  #define SYN_CAP_MIN_DIMENSIONS(ex0c)	((ex0c) & 0x002000)
>  #define SYN_CAP_ADV_GESTURE(ex0c)	((ex0c) & 0x080000)
>  #define SYN_CAP_REDUCED_FILTERING(ex0c)	((ex0c) & 0x000400)
> +#define SYN_CAP_IMAGE_SENSOR(ex0c)	((ex0c) & 0x000800)
>  
>  /* synaptics modes query bits */
>  #define SYN_MODE_ABSOLUTE(m)		((m) & (1 << 7))
> -- 
> 1.7.3.1
> 

Looks good otherwise.

Thanks,
Henrik