[PATCH] Support TrackStick of Thinkpad L570

[PATCH] Support TrackStick of Thinkpad L570

[Masaki Ota]

From: Masaki Ota <masaki.ota@jp.alps.com>
- The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
- Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
- If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
- Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.

Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
---
 drivers/input/mouse/alps.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 850b00e3ad8e..dd2c9e9f4830 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -2541,13 +2541,31 @@ static int alps_update_btn_info_ss4_v2(unsigned char otp[][4],
 }
 
 static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
-				       struct alps_data *priv)
+				       struct alps_data *priv,
+					struct psmouse *psmouse)
 {
 	bool is_dual = false;
+	int reg_val = 0;
+	struct ps2dev *ps2dev = &psmouse->ps2dev;
 
-	if (IS_SS4PLUS_DEV(priv->dev_id))
+	if (IS_SS4PLUS_DEV(priv->dev_id)) {
 		is_dual = (otp[0][0] >> 4) & 0x01;
 
+		if (!is_dual) {
+			/* For support TrackStick of Thinkpad L570 device */
+			if (alps_exit_command_mode(psmouse) == 0 &&
+				alps_enter_command_mode(psmouse) == 0) {
+				reg_val = alps_command_mode_read_reg(psmouse,
+									0xD7);
+			}
+			alps_exit_command_mode(psmouse);
+			ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
+
+			if (reg_val == 0x0C || reg_val == 0x1D)
+				is_dual = true;
+		}
+	}
+
 	if (is_dual)
 		priv->flags |= ALPS_DUALPOINT |
 					ALPS_DUALPOINT_WITH_PRESSURE;
@@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
 
 	alps_update_btn_info_ss4_v2(otp, priv);
 
-	alps_update_dual_info_ss4_v2(otp, priv);
+	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
 
 	return 0;
 }
-- 
2.14.1

Re: [PATCH] Support TrackStick of Thinkpad L570

[Pali Rohár]

On Wednesday 29 November 2017 16:01:05 Masaki Ota wrote:
> From: Masaki Ota <masaki.ota@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> 
> Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> ---
>  drivers/input/mouse/alps.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..dd2c9e9f4830 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,31 @@ static int alps_update_btn_info_ss4_v2(unsigned char otp[][4],
>  }
>  
>  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -				       struct alps_data *priv)
> +				       struct alps_data *priv,
> +					struct psmouse *psmouse)
>  {
>  	bool is_dual = false;
> +	int reg_val = 0;
> +	struct ps2dev *ps2dev = &psmouse->ps2dev;
>  
> -	if (IS_SS4PLUS_DEV(priv->dev_id))
> +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
>  		is_dual = (otp[0][0] >> 4) & 0x01;
>  
> +		if (!is_dual) {
> +			/* For support TrackStick of Thinkpad L570 device */

Hi! Jonathan Liu wrote that this patch is needed also for ThinkPad E570p
laptop. So it would be better to add comment that Thinkpad L570 is not
the only one affected and fixing is_dual is needed also for others.
(Probably all Thinkpads which has same generation with this touchpad.)
Such information could prevent in future to wrongly refactor this hook
code.

> +			if (alps_exit_command_mode(psmouse) == 0 &&
> +				alps_enter_command_mode(psmouse) == 0) {
> +				reg_val = alps_command_mode_read_reg(psmouse,
> +									0xD7);
> +			}
> +			alps_exit_command_mode(psmouse);
> +			ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> +			if (reg_val == 0x0C || reg_val == 0x1D)
> +				is_dual = true;
> +		}
> +	}
> +
>  	if (is_dual)
>  		priv->flags |= ALPS_DUALPOINT |
>  					ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
>  
>  	alps_update_btn_info_ss4_v2(otp, priv);
>  
> -	alps_update_dual_info_ss4_v2(otp, priv);
> +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>  
>  	return 0;
>  }

-- 
Pali Rohár
pali.rohar@gmail.com

[PATCH] Support TrackStick of Thinkpad L570

[Masaki Ota]

From: Masaki Ota <masaki.ota@jp.alps.com>
- The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
- Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
- If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
- Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.

Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
---
 drivers/input/mouse/alps.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 850b00e3ad8e..6f092bdd9fc5 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -2541,13 +2541,31 @@ static int alps_update_btn_info_ss4_v2(unsigned char otp[][4],
 }
 
 static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
-				       struct alps_data *priv)
+				       struct alps_data *priv,
+					struct psmouse *psmouse)
 {
 	bool is_dual = false;
+	int reg_val = 0;
+	struct ps2dev *ps2dev = &psmouse->ps2dev;
 
-	if (IS_SS4PLUS_DEV(priv->dev_id))
+	if (IS_SS4PLUS_DEV(priv->dev_id)) {
 		is_dual = (otp[0][0] >> 4) & 0x01;
 
+		if (!is_dual) {
+			/* For support TrackStick of Thinkpad L/E series */
+			if (alps_exit_command_mode(psmouse) == 0 &&
+				alps_enter_command_mode(psmouse) == 0) {
+				reg_val = alps_command_mode_read_reg(psmouse,
+									0xD7);
+			}
+			alps_exit_command_mode(psmouse);
+			ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
+
+			if (reg_val == 0x0C || reg_val == 0x1D)
+				is_dual = true;
+		}
+	}
+
 	if (is_dual)
 		priv->flags |= ALPS_DUALPOINT |
 					ALPS_DUALPOINT_WITH_PRESSURE;
@@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
 
 	alps_update_btn_info_ss4_v2(otp, priv);
 
-	alps_update_dual_info_ss4_v2(otp, priv);
+	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
 
 	return 0;
 }
-- 
2.14.1

Re: [PATCH] Support TrackStick of Thinkpad L570

[Aaron Ma]

Please add the patch version next time.

The patch make trackstick work on L570.

Tested-by: Aaron Ma <aaron.ma@canonical.com>

On 11/29/2017 04:33 PM, Masaki Ota wrote:
> From: Masaki Ota <masaki.ota@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> 
> Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> ---
>  drivers/input/mouse/alps.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..6f092bdd9fc5 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,31 @@ static int alps_update_btn_info_ss4_v2(unsigned char otp[][4],
>  }
>  
>  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -				       struct alps_data *priv)
> +				       struct alps_data *priv,
> +					struct psmouse *psmouse)
>  {
>  	bool is_dual = false;
> +	int reg_val = 0;
> +	struct ps2dev *ps2dev = &psmouse->ps2dev;
>  
> -	if (IS_SS4PLUS_DEV(priv->dev_id))
> +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
>  		is_dual = (otp[0][0] >> 4) & 0x01;
>  
> +		if (!is_dual) {
> +			/* For support TrackStick of Thinkpad L/E series */
> +			if (alps_exit_command_mode(psmouse) == 0 &&
> +				alps_enter_command_mode(psmouse) == 0) {
> +				reg_val = alps_command_mode_read_reg(psmouse,
> +									0xD7);
> +			}
> +			alps_exit_command_mode(psmouse);
> +			ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> +			if (reg_val == 0x0C || reg_val == 0x1D)
> +				is_dual = true;
> +		}
> +	}
> +
>  	if (is_dual)
>  		priv->flags |= ALPS_DUALPOINT |
>  					ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
>  
>  	alps_update_btn_info_ss4_v2(otp, priv);
>  
> -	alps_update_dual_info_ss4_v2(otp, priv);
> +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>  
>  	return 0;
>  }
> 

Re: [PATCH] Support TrackStick of Thinkpad L570

[Pali Rohár]

On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
> From: Masaki Ota <masaki.ota@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> 
> Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> ---
>  drivers/input/mouse/alps.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..6f092bdd9fc5 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,31 @@ static int alps_update_btn_info_ss4_v2(unsigned char otp[][4],
>  }
>  
>  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -				       struct alps_data *priv)
> +				       struct alps_data *priv,
> +					struct psmouse *psmouse)
>  {
>  	bool is_dual = false;
> +	int reg_val = 0;
> +	struct ps2dev *ps2dev = &psmouse->ps2dev;
>  
> -	if (IS_SS4PLUS_DEV(priv->dev_id))
> +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
>  		is_dual = (otp[0][0] >> 4) & 0x01;
>  
> +		if (!is_dual) {
> +			/* For support TrackStick of Thinkpad L/E series */
> +			if (alps_exit_command_mode(psmouse) == 0 &&
> +				alps_enter_command_mode(psmouse) == 0) {
> +				reg_val = alps_command_mode_read_reg(psmouse,
> +									0xD7);
> +			}
> +			alps_exit_command_mode(psmouse);
> +			ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> +			if (reg_val == 0x0C || reg_val == 0x1D)
> +				is_dual = true;
> +		}
> +	}
> +
>  	if (is_dual)
>  		priv->flags |= ALPS_DUALPOINT |
>  					ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
>  
>  	alps_update_btn_info_ss4_v2(otp, priv);
>  
> -	alps_update_dual_info_ss4_v2(otp, priv);
> +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);

Now looking at this change... Is there reason why you are passing
psmouse parameter there? Because struct alps_data contains psmouse
member.

>  
>  	return 0;
>  }

-- 
Pali Rohár
pali.rohar@gmail.com

RE: [PATCH] Support TrackStick of Thinkpad L570

[Masaki Ota]

Hi, Pali,

I don't get your point.
Please modify the code if you have an idea.

Best Regards,
Masaki Ota
-----Original Message-----
From: Pali Rohár [mailto:pali.rohar@gmail.com] 
Sent: Saturday, December 02, 2017 6:08 AM
To: Masaki Ota <012nexus@gmail.com>
Cc: dmitry.torokhov@gmail.com; benjamin.tissoires@redhat.com; aaron.ma@canonical.com; jaak@ristioja.ee; 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Support TrackStick of Thinkpad L570

On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
> From: Masaki Ota <masaki.ota@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> 
> Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> ---
>  drivers/input/mouse/alps.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c 
> index 850b00e3ad8e..6f092bdd9fc5 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,31 @@ static int 
> alps_update_btn_info_ss4_v2(unsigned char otp[][4],  }
>  
>  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -				       struct alps_data *priv)
> +				       struct alps_data *priv,
> +					struct psmouse *psmouse)
>  {
>  	bool is_dual = false;
> +	int reg_val = 0;
> +	struct ps2dev *ps2dev = &psmouse->ps2dev;
>  
> -	if (IS_SS4PLUS_DEV(priv->dev_id))
> +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
>  		is_dual = (otp[0][0] >> 4) & 0x01;
>  
> +		if (!is_dual) {
> +			/* For support TrackStick of Thinkpad L/E series */
> +			if (alps_exit_command_mode(psmouse) == 0 &&
> +				alps_enter_command_mode(psmouse) == 0) {
> +				reg_val = alps_command_mode_read_reg(psmouse,
> +									0xD7);
> +			}
> +			alps_exit_command_mode(psmouse);
> +			ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> +			if (reg_val == 0x0C || reg_val == 0x1D)
> +				is_dual = true;
> +		}
> +	}
> +
>  	if (is_dual)
>  		priv->flags |= ALPS_DUALPOINT |
>  					ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct 
> psmouse *psmouse,
>  
>  	alps_update_btn_info_ss4_v2(otp, priv);
>  
> -	alps_update_dual_info_ss4_v2(otp, priv);
> +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);

Now looking at this change... Is there reason why you are passing psmouse parameter there? Because struct alps_data contains psmouse member.

>  
>  	return 0;
>  }

--
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] Support TrackStick of Thinkpad L570

[Pali Rohár]

On Monday 04 December 2017 04:48:43 Masaki Ota wrote:
> Hi, Pali,
> 
> I don't get your point.
> Please modify the code if you have an idea.

See below

> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com] 
> Sent: Saturday, December 02, 2017 6:08 AM
> To: Masaki Ota <012nexus@gmail.com>
> Cc: dmitry.torokhov@gmail.com; benjamin.tissoires@redhat.com; aaron.ma@canonical.com; jaak@ristioja.ee; 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
> 
> On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
> > From: Masaki Ota <masaki.ota@jp.alps.com>
> > - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> > - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> > - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> > - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> > 
> > Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> > ---
> >  drivers/input/mouse/alps.c | 24 +++++++++++++++++++++---
> >  1 file changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c 
> > index 850b00e3ad8e..6f092bdd9fc5 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -2541,13 +2541,31 @@ static int 
> > alps_update_btn_info_ss4_v2(unsigned char otp[][4],  }
> >  
> >  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> > -				       struct alps_data *priv)
> > +				       struct alps_data *priv,
> > +					struct psmouse *psmouse)
> >  {

You can access psmouse from the priv structure as:

  struct psmouse *psmouse = priv->psmouse;

Therefore you do not have to extend function parameters with psmouse
pointer as that is already present int alps_data.

struct alps_data is defined as:

struct alps_data {
	struct psmouse *psmouse;
	...
}

> >  	bool is_dual = false;
> > +	int reg_val = 0;
> > +	struct ps2dev *ps2dev = &psmouse->ps2dev;
> >  
> > -	if (IS_SS4PLUS_DEV(priv->dev_id))
> > +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> >  		is_dual = (otp[0][0] >> 4) & 0x01;
> >  
> > +		if (!is_dual) {
> > +			/* For support TrackStick of Thinkpad L/E series */
> > +			if (alps_exit_command_mode(psmouse) == 0 &&
> > +				alps_enter_command_mode(psmouse) == 0) {
> > +				reg_val = alps_command_mode_read_reg(psmouse,
> > +									0xD7);
> > +			}
> > +			alps_exit_command_mode(psmouse);
> > +			ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> > +
> > +			if (reg_val == 0x0C || reg_val == 0x1D)
> > +				is_dual = true;
> > +		}
> > +	}
> > +
> >  	if (is_dual)
> >  		priv->flags |= ALPS_DUALPOINT |
> >  					ALPS_DUALPOINT_WITH_PRESSURE;
> > @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct 
> > psmouse *psmouse,
> >  
> >  	alps_update_btn_info_ss4_v2(otp, priv);
> >  
> > -	alps_update_dual_info_ss4_v2(otp, priv);
> > +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
> 
> Now looking at this change... Is there reason why you are passing psmouse parameter there? Because struct alps_data contains psmouse member.
> 
> >  
> >  	return 0;
> >  }
> 
> --
> Pali Rohár
> pali.rohar@gmail.com

-- 
Pali Rohár
pali.rohar@gmail.com

RE: [PATCH] Support TrackStick of Thinkpad L570

[Masaki Ota]

Hi, Pali,

It does not work in my test result.

BTW, other some functions also use both of "struct psmouse" and "struct alps_data" argument.
I just followed it.

Best Regards,
Masaki Ota
-----Original Message-----
From: Pali Rohár [mailto:pali.rohar@gmail.com] 
Sent: Monday, December 04, 2017 6:12 PM
To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
Cc: Masaki Ota <012nexus@gmail.com>; dmitry.torokhov@gmail.com; benjamin.tissoires@redhat.com; aaron.ma@canonical.com; jaak@ristioja.ee; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Support TrackStick of Thinkpad L570

On Monday 04 December 2017 04:48:43 Masaki Ota wrote:
> Hi, Pali,
> 
> I don't get your point.
> Please modify the code if you have an idea.

See below

> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Saturday, December 02, 2017 6:08 AM
> To: Masaki Ota <012nexus@gmail.com>
> Cc: dmitry.torokhov@gmail.com; benjamin.tissoires@redhat.com; 
> aaron.ma@canonical.com; jaak@ristioja.ee; 太田 真喜 Masaki Ota 
> <masaki.ota@jp.alps.com>; linux-input@vger.kernel.org; 
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
> 
> On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
> > From: Masaki Ota <masaki.ota@jp.alps.com>
> > - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> > - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> > - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> > - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> > 
> > Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> > ---
> >  drivers/input/mouse/alps.c | 24 +++++++++++++++++++++---
> >  1 file changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c 
> > index 850b00e3ad8e..6f092bdd9fc5 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -2541,13 +2541,31 @@ static int
> > alps_update_btn_info_ss4_v2(unsigned char otp[][4],  }
> >  
> >  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> > -				       struct alps_data *priv)
> > +				       struct alps_data *priv,
> > +					struct psmouse *psmouse)
> >  {

You can access psmouse from the priv structure as:

  struct psmouse *psmouse = priv->psmouse;

Therefore you do not have to extend function parameters with psmouse pointer as that is already present int alps_data.

struct alps_data is defined as:

struct alps_data {
	struct psmouse *psmouse;
	...
}

> >  	bool is_dual = false;
> > +	int reg_val = 0;
> > +	struct ps2dev *ps2dev = &psmouse->ps2dev;
> >  
> > -	if (IS_SS4PLUS_DEV(priv->dev_id))
> > +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> >  		is_dual = (otp[0][0] >> 4) & 0x01;
> >  
> > +		if (!is_dual) {
> > +			/* For support TrackStick of Thinkpad L/E series */
> > +			if (alps_exit_command_mode(psmouse) == 0 &&
> > +				alps_enter_command_mode(psmouse) == 0) {
> > +				reg_val = alps_command_mode_read_reg(psmouse,
> > +									0xD7);
> > +			}
> > +			alps_exit_command_mode(psmouse);
> > +			ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> > +
> > +			if (reg_val == 0x0C || reg_val == 0x1D)
> > +				is_dual = true;
> > +		}
> > +	}
> > +
> >  	if (is_dual)
> >  		priv->flags |= ALPS_DUALPOINT |
> >  					ALPS_DUALPOINT_WITH_PRESSURE;
> > @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct 
> > psmouse *psmouse,
> >  
> >  	alps_update_btn_info_ss4_v2(otp, priv);
> >  
> > -	alps_update_dual_info_ss4_v2(otp, priv);
> > +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
> 
> Now looking at this change... Is there reason why you are passing psmouse parameter there? Because struct alps_data contains psmouse member.
> 
> >  
> >  	return 0;
> >  }
> 
> --
> Pali Rohár
> pali.rohar@gmail.com

--
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] Support TrackStick of Thinkpad L570

[Pali Rohár]

On Monday 04 December 2017 09:40:04 Masaki Ota wrote:
> Hi, Pali,
> 
> It does not work in my test result.

Hm.. that is strange, we have dangling pointers in struct alps_data?
Otherwise I have no idea why does not work.

> BTW, other some functions also use both of "struct psmouse" and "struct alps_data" argument.

It does not make sense to pass one structure (via pointers) two times.
And if this "pattern" is already used in code and reason is that one
pointer "does not work" because it is dangling, then it is really wrong.

I know it is irrelevant to your patch, but this problem with dangling
pointer should be fixed, e.g. in next patches (not in this one).

Problems (with memory allocation/pointers) should not be camouflaged.
Memory corruption in kernel can lead to fatal problems.

> I just followed it.

Blindly following bad code is a bad idea. When we see something like
this, we should at least stop and ask question "why is this code pattern
used?".

> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com] 
> Sent: Monday, December 04, 2017 6:12 PM
> To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
> Cc: Masaki Ota <012nexus@gmail.com>; dmitry.torokhov@gmail.com; benjamin.tissoires@redhat.com; aaron.ma@canonical.com; jaak@ristioja.ee; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
> 
> On Monday 04 December 2017 04:48:43 Masaki Ota wrote:
> > Hi, Pali,
> > 
> > I don't get your point.
> > Please modify the code if you have an idea.
> 
> See below
> 
> > Best Regards,
> > Masaki Ota
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Saturday, December 02, 2017 6:08 AM
> > To: Masaki Ota <012nexus@gmail.com>
> > Cc: dmitry.torokhov@gmail.com; benjamin.tissoires@redhat.com; 
> > aaron.ma@canonical.com; jaak@ristioja.ee; 太田 真喜 Masaki Ota 
> > <masaki.ota@jp.alps.com>; linux-input@vger.kernel.org; 
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
> > 
> > On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
> > > From: Masaki Ota <masaki.ota@jp.alps.com>
> > > - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> > > - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> > > - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> > > - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> > > 
> > > Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> > > ---
> > >  drivers/input/mouse/alps.c | 24 +++++++++++++++++++++---
> > >  1 file changed, 21 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c 
> > > index 850b00e3ad8e..6f092bdd9fc5 100644
> > > --- a/drivers/input/mouse/alps.c
> > > +++ b/drivers/input/mouse/alps.c
> > > @@ -2541,13 +2541,31 @@ static int
> > > alps_update_btn_info_ss4_v2(unsigned char otp[][4],  }
> > >  
> > >  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> > > -				       struct alps_data *priv)
> > > +				       struct alps_data *priv,
> > > +					struct psmouse *psmouse)
> > >  {
> 
> You can access psmouse from the priv structure as:
> 
>   struct psmouse *psmouse = priv->psmouse;
> 
> Therefore you do not have to extend function parameters with psmouse pointer as that is already present int alps_data.
> 
> struct alps_data is defined as:
> 
> struct alps_data {
> 	struct psmouse *psmouse;
> 	...
> }
> 
> > >  	bool is_dual = false;
> > > +	int reg_val = 0;
> > > +	struct ps2dev *ps2dev = &psmouse->ps2dev;
> > >  
> > > -	if (IS_SS4PLUS_DEV(priv->dev_id))
> > > +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > >  		is_dual = (otp[0][0] >> 4) & 0x01;
> > >  
> > > +		if (!is_dual) {
> > > +			/* For support TrackStick of Thinkpad L/E series */
> > > +			if (alps_exit_command_mode(psmouse) == 0 &&
> > > +				alps_enter_command_mode(psmouse) == 0) {
> > > +				reg_val = alps_command_mode_read_reg(psmouse,
> > > +									0xD7);
> > > +			}
> > > +			alps_exit_command_mode(psmouse);
> > > +			ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> > > +
> > > +			if (reg_val == 0x0C || reg_val == 0x1D)
> > > +				is_dual = true;
> > > +		}
> > > +	}
> > > +
> > >  	if (is_dual)
> > >  		priv->flags |= ALPS_DUALPOINT |
> > >  					ALPS_DUALPOINT_WITH_PRESSURE;
> > > @@ -2570,7 +2588,7 @@ static int alps_set_defaults_ss4_v2(struct 
> > > psmouse *psmouse,
> > >  
> > >  	alps_update_btn_info_ss4_v2(otp, priv);
> > >  
> > > -	alps_update_dual_info_ss4_v2(otp, priv);
> > > +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
> > 
> > Now looking at this change... Is there reason why you are passing psmouse parameter there? Because struct alps_data contains psmouse member.
> > 
> > >  
> > >  	return 0;
> > >  }
> > 
> > --
> > Pali Rohár
> > pali.rohar@gmail.com
> 
> --
> Pali Rohár
> pali.rohar@gmail.com

-- 
Pali Rohár
pali.rohar@gmail.com

RE: [PATCH] Support TrackStick of Thinkpad L570

[Masaki Ota]

Hi, Pali,

OK, I got it.
So, the problem of structure will be solved next chance.

Best Regards,
Masaki Ota
-----Original Message-----
From: Pali Rohár [mailto:pali.rohar@gmail.com] 
Sent: Monday, December 04, 2017 6:51 PM
To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
Cc: Masaki Ota <012nexus@gmail.com>; dmitry.torokhov@gmail.com; benjamin.tissoires@redhat.com; aaron.ma@canonical.com; jaak@ristioja.ee; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Support TrackStick of Thinkpad L570

On Monday 04 December 2017 09:40:04 Masaki Ota wrote:
> Hi, Pali,
> 
> It does not work in my test result.

Hm.. that is strange, we have dangling pointers in struct alps_data?
Otherwise I have no idea why does not work.

> BTW, other some functions also use both of "struct psmouse" and "struct alps_data" argument.

It does not make sense to pass one structure (via pointers) two times.
And if this "pattern" is already used in code and reason is that one pointer "does not work" because it is dangling, then it is really wrong.

I know it is irrelevant to your patch, but this problem with dangling pointer should be fixed, e.g. in next patches (not in this one).

Problems (with memory allocation/pointers) should not be camouflaged.
Memory corruption in kernel can lead to fatal problems.

> I just followed it.

Blindly following bad code is a bad idea. When we see something like this, we should at least stop and ask question "why is this code pattern used?".

> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Monday, December 04, 2017 6:12 PM
> To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
> Cc: Masaki Ota <012nexus@gmail.com>; dmitry.torokhov@gmail.com; 
> benjamin.tissoires@redhat.com; aaron.ma@canonical.com; 
> jaak@ristioja.ee; linux-input@vger.kernel.org; 
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
> 
> On Monday 04 December 2017 04:48:43 Masaki Ota wrote:
> > Hi, Pali,
> > 
> > I don't get your point.
> > Please modify the code if you have an idea.
> 
> See below
> 
> > Best Regards,
> > Masaki Ota
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Saturday, December 02, 2017 6:08 AM
> > To: Masaki Ota <012nexus@gmail.com>
> > Cc: dmitry.torokhov@gmail.com; benjamin.tissoires@redhat.com; 
> > aaron.ma@canonical.com; jaak@ristioja.ee; 太田 真喜 Masaki Ota 
> > <masaki.ota@jp.alps.com>; linux-input@vger.kernel.org; 
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
> > 
> > On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
> > > From: Masaki Ota <masaki.ota@jp.alps.com>
> > > - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> > > - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> > > - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> > > - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> > > 
> > > Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> > > ---
> > >  drivers/input/mouse/alps.c | 24 +++++++++++++++++++++---
> > >  1 file changed, 21 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/input/mouse/alps.c 
> > > b/drivers/input/mouse/alps.c index 850b00e3ad8e..6f092bdd9fc5 
> > > 100644
> > > --- a/drivers/input/mouse/alps.c
> > > +++ b/drivers/input/mouse/alps.c
> > > @@ -2541,13 +2541,31 @@ static int 
> > > alps_update_btn_info_ss4_v2(unsigned char otp[][4],  }
> > >  
> > >  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> > > -				       struct alps_data *priv)
> > > +				       struct alps_data *priv,
> > > +					struct psmouse *psmouse)
> > >  {
> 
> You can access psmouse from the priv structure as:
> 
>   struct psmouse *psmouse = priv->psmouse;
> 
> Therefore you do not have to extend function parameters with psmouse pointer as that is already present int alps_data.
> 
> struct alps_data is defined as:
> 
> struct alps_data {
> 	struct psmouse *psmouse;
> 	...
> }
> 
> > >  	bool is_dual = false;
> > > +	int reg_val = 0;
> > > +	struct ps2dev *ps2dev = &psmouse->ps2dev;
> > >  
> > > -	if (IS_SS4PLUS_DEV(priv->dev_id))
> > > +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > >  		is_dual = (otp[0][0] >> 4) & 0x01;
> > >  
> > > +		if (!is_dual) {
> > > +			/* For support TrackStick of Thinkpad L/E series */
> > > +			if (alps_exit_command_mode(psmouse) == 0 &&
> > > +				alps_enter_command_mode(psmouse) == 0) {
> > > +				reg_val = alps_command_mode_read_reg(psmouse,
> > > +									0xD7);
> > > +			}
> > > +			alps_exit_command_mode(psmouse);
> > > +			ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> > > +
> > > +			if (reg_val == 0x0C || reg_val == 0x1D)
> > > +				is_dual = true;
> > > +		}
> > > +	}
> > > +
> > >  	if (is_dual)
> > >  		priv->flags |= ALPS_DUALPOINT |
> > >  					ALPS_DUALPOINT_WITH_PRESSURE; @@ -2570,7 +2588,7 @@ static 
> > > int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
> > >  
> > >  	alps_update_btn_info_ss4_v2(otp, priv);
> > >  
> > > -	alps_update_dual_info_ss4_v2(otp, priv);
> > > +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
> > 
> > Now looking at this change... Is there reason why you are passing psmouse parameter there? Because struct alps_data contains psmouse member.
> > 
> > >  
> > >  	return 0;
> > >  }
> > 
> > --
> > Pali Rohár
> > pali.rohar@gmail.com
> 
> --
> Pali Rohár
> pali.rohar@gmail.com

--
Pali Rohár
pali.rohar@gmail.com

[PATCH] Support TrackStick of Thinkpad L570

[Masaki Ota]

From: Masaki Ota <masaki.ota@jp.alps.com>
- The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
- Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
- If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
- Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.

Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
---
 drivers/input/mouse/alps.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 850b00e3ad8e..cce52104ed5a 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -2541,13 +2541,28 @@ static int alps_update_btn_info_ss4_v2(unsigned char otp[][4],
 }
 
 static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
-				       struct alps_data *priv)
+				       struct alps_data *priv,
+					struct psmouse *psmouse)
 {
 	bool is_dual = false;
+	int reg_val = 0;
+	struct ps2dev *ps2dev = &psmouse->ps2dev;
 
-	if (IS_SS4PLUS_DEV(priv->dev_id))
+	if (IS_SS4PLUS_DEV(priv->dev_id)) {
 		is_dual = (otp[0][0] >> 4) & 0x01;
 
+		/* For support TrackStick of Thinkpad L570 device */
+		if (alps_exit_command_mode(psmouse) == 0 &&
+			alps_enter_command_mode(psmouse) == 0) {
+			reg_val = alps_command_mode_read_reg(psmouse, 0xD7);
+		}
+		alps_exit_command_mode(psmouse);
+		ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
+
+		if (reg_val == 0x0C || reg_val == 0x1D)
+			is_dual = true;
+	}
+
 	if (is_dual)
 		priv->flags |= ALPS_DUALPOINT |
 					ALPS_DUALPOINT_WITH_PRESSURE;
@@ -2570,7 +2585,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
 
 	alps_update_btn_info_ss4_v2(otp, priv);
 
-	alps_update_dual_info_ss4_v2(otp, priv);
+	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
 
 	return 0;
 }
-- 
2.14.1

Re: [PATCH] Support TrackStick of Thinkpad L570

[Pali Rohár]

On Monday 20 November 2017 16:55:30 Masaki Ota wrote:
> From: Masaki Ota <masaki.ota@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.

So, the root of this problem is in ALPS firmware which provides wrong
information to kernel?

Masaki, I have two questions:

1) Can ALPS or Lenovo release a new firmware update for this Thinkpad to
   fix this issue?

2) Have all Thinkpad L570 machines trackpoint?

Dmitry, as a workaround for firmware bug on particular notebook, would
not be better to check DMI information and DMI based hook?

> Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> ---
>  drivers/input/mouse/alps.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..cce52104ed5a 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,28 @@ static int alps_update_btn_info_ss4_v2(unsigned char otp[][4],
>  }
>  
>  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -				       struct alps_data *priv)
> +				       struct alps_data *priv,
> +					struct psmouse *psmouse)
>  {
>  	bool is_dual = false;
> +	int reg_val = 0;
> +	struct ps2dev *ps2dev = &psmouse->ps2dev;
>  
> -	if (IS_SS4PLUS_DEV(priv->dev_id))
> +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
>  		is_dual = (otp[0][0] >> 4) & 0x01;
>  
> +		/* For support TrackStick of Thinkpad L570 device */
> +		if (alps_exit_command_mode(psmouse) == 0 &&
> +			alps_enter_command_mode(psmouse) == 0) {
> +			reg_val = alps_command_mode_read_reg(psmouse, 0xD7);
> +		}
> +		alps_exit_command_mode(psmouse);
> +		ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> +		if (reg_val == 0x0C || reg_val == 0x1D)
> +			is_dual = true;
> +	}
> +
>  	if (is_dual)
>  		priv->flags |= ALPS_DUALPOINT |
>  					ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2585,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
>  
>  	alps_update_btn_info_ss4_v2(otp, priv);
>  
> -	alps_update_dual_info_ss4_v2(otp, priv);
> +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>  
>  	return 0;
>  }

-- 
Pali Rohár
pali.rohar@gmail.com

RE: [PATCH] Support TrackStick of Thinkpad L570

[Masaki Ota]

Hi, Pali,

1) Yes, but we don't have FW update plan.
 And this device works as SMBus in Windows, so this issue happens on only Linux.

2) Yes.

Best Regards,
Masaki Ota
-----Original Message-----
From: Pali Rohár [mailto:pali.rohar@gmail.com] 
Sent: Monday, November 20, 2017 5:30 PM
To: Masaki Ota <012nexus@gmail.com>; dmitry.torokhov@gmail.com
Cc: benjamin.tissoires@redhat.com; aaron.ma@canonical.com; jaak@ristioja.ee; 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Support TrackStick of Thinkpad L570

On Monday 20 November 2017 16:55:30 Masaki Ota wrote:
> From: Masaki Ota <masaki.ota@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.

So, the root of this problem is in ALPS firmware which provides wrong information to kernel?

Masaki, I have two questions:

1) Can ALPS or Lenovo release a new firmware update for this Thinkpad to
   fix this issue?

2) Have all Thinkpad L570 machines trackpoint?

Dmitry, as a workaround for firmware bug on particular notebook, would not be better to check DMI information and DMI based hook?

> Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> ---
>  drivers/input/mouse/alps.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c 
> index 850b00e3ad8e..cce52104ed5a 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,28 @@ static int 
> alps_update_btn_info_ss4_v2(unsigned char otp[][4],  }
>  
>  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -				       struct alps_data *priv)
> +				       struct alps_data *priv,
> +					struct psmouse *psmouse)
>  {
>  	bool is_dual = false;
> +	int reg_val = 0;
> +	struct ps2dev *ps2dev = &psmouse->ps2dev;
>  
> -	if (IS_SS4PLUS_DEV(priv->dev_id))
> +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
>  		is_dual = (otp[0][0] >> 4) & 0x01;
>  
> +		/* For support TrackStick of Thinkpad L570 device */
> +		if (alps_exit_command_mode(psmouse) == 0 &&
> +			alps_enter_command_mode(psmouse) == 0) {
> +			reg_val = alps_command_mode_read_reg(psmouse, 0xD7);
> +		}
> +		alps_exit_command_mode(psmouse);
> +		ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> +		if (reg_val == 0x0C || reg_val == 0x1D)
> +			is_dual = true;
> +	}
> +
>  	if (is_dual)
>  		priv->flags |= ALPS_DUALPOINT |
>  					ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2585,7 @@ static int alps_set_defaults_ss4_v2(struct 
> psmouse *psmouse,
>  
>  	alps_update_btn_info_ss4_v2(otp, priv);
>  
> -	alps_update_dual_info_ss4_v2(otp, priv);
> +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>  
>  	return 0;
>  }

--
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] Support TrackStick of Thinkpad L570

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 4051 bytes --]

Masaki: About firmware update, there is Linux Vendor Firmware Service
project at https://fwupd.org/ exactly for purpose to update firmwares
from Linux distributions. Maintainer of this project, Richard Hughes
(CCed), is more times willing to write support for new devices. So
maybe this can be a solution for future.

On Monday 20 November 2017 08:43:45 Masaki Ota wrote:
> Hi, Pali,
> 
> 1) Yes, but we don't have FW update plan.
>  And this device works as SMBus in Windows, so this issue happens on only Linux.
> 
> 2) Yes.
> 
> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com] 
> Sent: Monday, November 20, 2017 5:30 PM
> To: Masaki Ota <012nexus@gmail.com>; dmitry.torokhov@gmail.com
> Cc: benjamin.tissoires@redhat.com; aaron.ma@canonical.com; jaak@ristioja.ee; 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Support TrackStick of Thinkpad L570
> 
> On Monday 20 November 2017 16:55:30 Masaki Ota wrote:
> > From: Masaki Ota <masaki.ota@jp.alps.com>
> > - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> > - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> > - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> > - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> 
> So, the root of this problem is in ALPS firmware which provides wrong information to kernel?
> 
> Masaki, I have two questions:
> 
> 1) Can ALPS or Lenovo release a new firmware update for this Thinkpad to
>    fix this issue?
> 
> 2) Have all Thinkpad L570 machines trackpoint?
> 
> Dmitry, as a workaround for firmware bug on particular notebook, would not be better to check DMI information and DMI based hook?
> 
> > Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> > ---
> >  drivers/input/mouse/alps.c | 21 ++++++++++++++++++---
> >  1 file changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c 
> > index 850b00e3ad8e..cce52104ed5a 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -2541,13 +2541,28 @@ static int 
> > alps_update_btn_info_ss4_v2(unsigned char otp[][4],  }
> >  
> >  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> > -				       struct alps_data *priv)
> > +				       struct alps_data *priv,
> > +					struct psmouse *psmouse)
> >  {
> >  	bool is_dual = false;
> > +	int reg_val = 0;
> > +	struct ps2dev *ps2dev = &psmouse->ps2dev;
> >  
> > -	if (IS_SS4PLUS_DEV(priv->dev_id))
> > +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> >  		is_dual = (otp[0][0] >> 4) & 0x01;
> >  
> > +		/* For support TrackStick of Thinkpad L570 device */
> > +		if (alps_exit_command_mode(psmouse) == 0 &&
> > +			alps_enter_command_mode(psmouse) == 0) {
> > +			reg_val = alps_command_mode_read_reg(psmouse, 0xD7);
> > +		}
> > +		alps_exit_command_mode(psmouse);
> > +		ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> > +
> > +		if (reg_val == 0x0C || reg_val == 0x1D)
> > +			is_dual = true;
> > +	}
> > +
> >  	if (is_dual)
> >  		priv->flags |= ALPS_DUALPOINT |
> >  					ALPS_DUALPOINT_WITH_PRESSURE;
> > @@ -2570,7 +2585,7 @@ static int alps_set_defaults_ss4_v2(struct 
> > psmouse *psmouse,
> >  
> >  	alps_update_btn_info_ss4_v2(otp, priv);
> >  
> > -	alps_update_dual_info_ss4_v2(otp, priv);
> > +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
> >  
> >  	return 0;
> >  }
> 
> --
> Pali Rohár
> pali.rohar@gmail.com

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] Support TrackStick of Thinkpad L570

[Aaron Ma]

Tested-by: Aaron Ma <aaron.ma@canonical.com>

On 11/20/2017 03:55 PM, Masaki Ota wrote:
> From: Masaki Ota <masaki.ota@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> 
> Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> ---
>  drivers/input/mouse/alps.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..cce52104ed5a 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,28 @@ static int alps_update_btn_info_ss4_v2(unsigned char otp[][4],
>  }
>  
>  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -				       struct alps_data *priv)
> +				       struct alps_data *priv,
> +					struct psmouse *psmouse)
>  {
>  	bool is_dual = false;
> +	int reg_val = 0;
> +	struct ps2dev *ps2dev = &psmouse->ps2dev;
>  
> -	if (IS_SS4PLUS_DEV(priv->dev_id))
> +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
>  		is_dual = (otp[0][0] >> 4) & 0x01;
>  
> +		/* For support TrackStick of Thinkpad L570 device */
> +		if (alps_exit_command_mode(psmouse) == 0 &&
> +			alps_enter_command_mode(psmouse) == 0) {
> +			reg_val = alps_command_mode_read_reg(psmouse, 0xD7);
> +		}
> +		alps_exit_command_mode(psmouse);
> +		ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> +		if (reg_val == 0x0C || reg_val == 0x1D)
> +			is_dual = true;
> +	}
> +
>  	if (is_dual)
>  		priv->flags |= ALPS_DUALPOINT |
>  					ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2585,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
>  
>  	alps_update_btn_info_ss4_v2(otp, priv);
>  
> -	alps_update_dual_info_ss4_v2(otp, priv);
> +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>  
>  	return 0;
>  }
> 

Re: [PATCH] Support TrackStick of Thinkpad L570

[Dmitry Torokhov]

Hi,

On Mon, Nov 20, 2017 at 04:55:30PM +0900, Masaki Ota wrote:
> From: Masaki Ota <masaki.ota@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.

Is it custom protocol over SMBus/I2C or HID (via i2c-hid)? If it is
custom protocol, can we add the proper driver to the kernel for it so we
can get away from the PS/2 emulation in firmware?

Thanks!

> - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> 
> Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> ---
>  drivers/input/mouse/alps.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..cce52104ed5a 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,28 @@ static int alps_update_btn_info_ss4_v2(unsigned char otp[][4],
>  }
>  
>  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -				       struct alps_data *priv)
> +				       struct alps_data *priv,
> +					struct psmouse *psmouse)
>  {
>  	bool is_dual = false;
> +	int reg_val = 0;
> +	struct ps2dev *ps2dev = &psmouse->ps2dev;
>  
> -	if (IS_SS4PLUS_DEV(priv->dev_id))
> +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
>  		is_dual = (otp[0][0] >> 4) & 0x01;
>  
> +		/* For support TrackStick of Thinkpad L570 device */
> +		if (alps_exit_command_mode(psmouse) == 0 &&
> +			alps_enter_command_mode(psmouse) == 0) {
> +			reg_val = alps_command_mode_read_reg(psmouse, 0xD7);
> +		}
> +		alps_exit_command_mode(psmouse);
> +		ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> +		if (reg_val == 0x0C || reg_val == 0x1D)
> +			is_dual = true;
> +	}
> +
>  	if (is_dual)
>  		priv->flags |= ALPS_DUALPOINT |
>  					ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2585,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
>  
>  	alps_update_btn_info_ss4_v2(otp, priv);
>  
> -	alps_update_dual_info_ss4_v2(otp, priv);
> +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>  
>  	return 0;
>  }
> -- 
> 2.14.1
> 

-- 
Dmitry

RE: [PATCH] Support TrackStick of Thinkpad L570

[Masaki Ota]

Hi, Domitry,

I think we can add the code to HID for this device, but we will not use SMBus interface for Touchpad in the future.
We should not do it.

Best Regards,
Masaki Ota
-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] 
Sent: Sunday, November 26, 2017 9:16 AM
To: Masaki Ota <012nexus@gmail.com>
Cc: benjamin.tissoires@redhat.com; pali.rohar@gmail.com; aaron.ma@canonical.com; jaak@ristioja.ee; 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Support TrackStick of Thinkpad L570

Hi,

On Mon, Nov 20, 2017 at 04:55:30PM +0900, Masaki Ota wrote:
> From: Masaki Ota <masaki.ota@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.

Is it custom protocol over SMBus/I2C or HID (via i2c-hid)? If it is custom protocol, can we add the proper driver to the kernel for it so we can get away from the PS/2 emulation in firmware?

Thanks!

> - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> 
> Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> ---
>  drivers/input/mouse/alps.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c 
> index 850b00e3ad8e..cce52104ed5a 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,28 @@ static int 
> alps_update_btn_info_ss4_v2(unsigned char otp[][4],  }
>  
>  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -				       struct alps_data *priv)
> +				       struct alps_data *priv,
> +					struct psmouse *psmouse)
>  {
>  	bool is_dual = false;
> +	int reg_val = 0;
> +	struct ps2dev *ps2dev = &psmouse->ps2dev;
>  
> -	if (IS_SS4PLUS_DEV(priv->dev_id))
> +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
>  		is_dual = (otp[0][0] >> 4) & 0x01;
>  
> +		/* For support TrackStick of Thinkpad L570 device */
> +		if (alps_exit_command_mode(psmouse) == 0 &&
> +			alps_enter_command_mode(psmouse) == 0) {
> +			reg_val = alps_command_mode_read_reg(psmouse, 0xD7);
> +		}
> +		alps_exit_command_mode(psmouse);
> +		ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> +		if (reg_val == 0x0C || reg_val == 0x1D)
> +			is_dual = true;
> +	}
> +
>  	if (is_dual)
>  		priv->flags |= ALPS_DUALPOINT |
>  					ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2585,7 @@ static int alps_set_defaults_ss4_v2(struct 
> psmouse *psmouse,
>  
>  	alps_update_btn_info_ss4_v2(otp, priv);
>  
> -	alps_update_dual_info_ss4_v2(otp, priv);
> +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>  
>  	return 0;
>  }
> --
> 2.14.1
> 

-- 
Dmitry

Re: [PATCH] Support TrackStick of Thinkpad L570

[Dmitry Torokhov]

On Mon, Nov 20, 2017 at 04:55:30PM +0900, Masaki Ota wrote:
> From: Masaki Ota <masaki.ota@jp.alps.com>
> - The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
> - Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
> - If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
> - Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
> 
> Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> ---
>  drivers/input/mouse/alps.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 850b00e3ad8e..cce52104ed5a 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2541,13 +2541,28 @@ static int alps_update_btn_info_ss4_v2(unsigned char otp[][4],
>  }
>  
>  static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> -				       struct alps_data *priv)
> +				       struct alps_data *priv,
> +					struct psmouse *psmouse)
>  {
>  	bool is_dual = false;
> +	int reg_val = 0;
> +	struct ps2dev *ps2dev = &psmouse->ps2dev;
>  
> -	if (IS_SS4PLUS_DEV(priv->dev_id))
> +	if (IS_SS4PLUS_DEV(priv->dev_id)) {
>  		is_dual = (otp[0][0] >> 4) & 0x01;
>  
> +		/* For support TrackStick of Thinkpad L570 device */
> +		if (alps_exit_command_mode(psmouse) == 0 &&
> +			alps_enter_command_mode(psmouse) == 0) {
> +			reg_val = alps_command_mode_read_reg(psmouse, 0xD7);
> +		}
> +		alps_exit_command_mode(psmouse);
> +		ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);

Should we execute this sequence only if is_dual is false above?

> +
> +		if (reg_val == 0x0C || reg_val == 0x1D)
> +			is_dual = true;
> +	}
> +
>  	if (is_dual)
>  		priv->flags |= ALPS_DUALPOINT |
>  					ALPS_DUALPOINT_WITH_PRESSURE;
> @@ -2570,7 +2585,7 @@ static int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
>  
>  	alps_update_btn_info_ss4_v2(otp, priv);
>  
> -	alps_update_dual_info_ss4_v2(otp, priv);
> +	alps_update_dual_info_ss4_v2(otp, priv, psmouse);
>  
>  	return 0;
>  }
> -- 
> 2.14.1
> 

Thanks.

-- 
Dmitry