diff mbox

synaptics: fix rmi4 bus dependencies

Message ID 20180109221933.nexohjl75g4uu2nl@dtor-ws (mailing list archive)
State New, archived
Headers show

Commit Message

Dmitry Torokhov Jan. 9, 2018, 10:19 p.m. UTC 
On Tue, Jan 02, 2018 at 11:37:07AM -0800, Andrew Duggan wrote:
> On 01/02/2018 11:21 AM, Dmitry Torokhov wrote:
> > On Tue, Jan 02, 2018 at 08:16:09PM +0100, Matteo Croce wrote:
> > > Make MOUSE_PS2_SYNAPTICS_SMBUS select MOUSE_PS2_SYNAPTICS because if
> > > the latter is disabled the detection and switching code isn't being run.
> > Hmm, it should. In synaptics.c:
> > 
> > #if defined(CONFIG_MOUSE_PS2_SYNAPTICS) || \
> >      defined(CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS)
> > 
> > int synaptics_init(struct psmouse *psmouse)
> > {
> > 	struct synaptics_device_info info;
> > 	int error;
> > 	int retval;
> > 
> > 	psmouse_reset(psmouse);
> > ...
> > }
> > 
> > The intent was to allow disabling the PS/2 portion of Synaptics driver
> > while retaining the switching to SMbus capability.
> 
> Oh, I suggested to Matteo that this patch might be needed off list. I
> clearly didn't look closely enough to see that MOUSE_PS2_SYNAPTICS_SMBUS is
> intended to be able to be used independently of MOUSE_PS2_SYNAPTICS.
> 
> However, he did seem to see behavior where the mode switch did not occur
> when MOUSE_PS2_SYNAPTICS was disabled, but MOUSE_PS2_SYNAPTICS_SMBUS was
> enabled. That will need to be investigated further.

OK, I see what the problem is. Could you please try the version below?

Thanks.

Comments

Matteo Croce Jan. 16, 2018, 6:08 p.m. UTC | #1 
On Tue, Jan 9, 2018 at 11:19 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Jan 02, 2018 at 11:37:07AM -0800, Andrew Duggan wrote:
>> On 01/02/2018 11:21 AM, Dmitry Torokhov wrote:
>> > On Tue, Jan 02, 2018 at 08:16:09PM +0100, Matteo Croce wrote:
>> > > Make MOUSE_PS2_SYNAPTICS_SMBUS select MOUSE_PS2_SYNAPTICS because if
>> > > the latter is disabled the detection and switching code isn't being run.
>> > Hmm, it should. In synaptics.c:
>> >
>> > #if defined(CONFIG_MOUSE_PS2_SYNAPTICS) || \
>> >      defined(CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS)
>> >
>> > int synaptics_init(struct psmouse *psmouse)
>> > {
>> >     struct synaptics_device_info info;
>> >     int error;
>> >     int retval;
>> >
>> >     psmouse_reset(psmouse);
>> > ...
>> > }
>> >
>> > The intent was to allow disabling the PS/2 portion of Synaptics driver
>> > while retaining the switching to SMbus capability.
>>
>> Oh, I suggested to Matteo that this patch might be needed off list. I
>> clearly didn't look closely enough to see that MOUSE_PS2_SYNAPTICS_SMBUS is
>> intended to be able to be used independently of MOUSE_PS2_SYNAPTICS.
>>
>> However, he did seem to see behavior where the mode switch did not occur
>> when MOUSE_PS2_SYNAPTICS was disabled, but MOUSE_PS2_SYNAPTICS_SMBUS was
>> enabled. That will need to be investigated further.
>
> OK, I see what the problem is. Could you please try the version below?
>
> Thanks.
>
> --
> Dmitry
>
>
> Input: psmouse - fix Synaptics detection when protocol is disabled
>
> When Synaptics protocol is disabled, we still need to ytry and detect the
> hardware, so we can switch to SMBus device if SMbus is detected, or we know
> that it is Synaptics device and reset it properly for the bare PS/2
> protocol.
>
> Fixes: c378b5119eb0 ("Input: psmouse - factor out common protocol probing code")
> Reported-by: Matteo Croce <mcroce@redhat.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/mouse/psmouse-base.c |   34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> index f3eb407a3b7d..cbe12671f784 100644
> --- a/drivers/input/mouse/psmouse-base.c
> +++ b/drivers/input/mouse/psmouse-base.c
> @@ -984,6 +984,21 @@ static void psmouse_apply_defaults(struct psmouse *psmouse)
>         psmouse->pt_deactivate = NULL;
>  }
>
> +static bool psmouse_do_detect(int (*detect)(struct psmouse *, bool),
> +                             struct psmouse *psmouse, bool allow_passthrough,
> +                             bool set_properties)
> +{
> +       if (psmouse->ps2dev.serio->id.type == SERIO_PS_PSTHRU &&
> +           !allow_passthrough) {
> +               return false;
> +       }
> +
> +       if (set_properties)
> +               psmouse_apply_defaults(psmouse);
> +
> +       return detect(psmouse, set_properties) == 0;
> +}
> +
>  static bool psmouse_try_protocol(struct psmouse *psmouse,
>                                  enum psmouse_type type,
>                                  unsigned int *max_proto,
> @@ -995,15 +1010,8 @@ static bool psmouse_try_protocol(struct psmouse *psmouse,
>         if (!proto)
>                 return false;
>
> -       if (psmouse->ps2dev.serio->id.type == SERIO_PS_PSTHRU &&
> -           !proto->try_passthru) {
> -               return false;
> -       }
> -
> -       if (set_properties)
> -               psmouse_apply_defaults(psmouse);
> -
> -       if (proto->detect(psmouse, set_properties) != 0)
> +       if (!psmouse_do_detect(proto->detect, psmouse, proto->try_passthru,
> +                              set_properties))
>                 return false;
>
>         if (set_properties && proto->init && init_allowed) {
> @@ -1036,8 +1044,8 @@ static int psmouse_extensions(struct psmouse *psmouse,
>          * Always check for focaltech, this is safe as it uses pnp-id
>          * matching.
>          */
> -       if (psmouse_try_protocol(psmouse, PSMOUSE_FOCALTECH,
> -                                &max_proto, set_properties, false)) {
> +       if (psmouse_do_detect(focaltech_detect,
> +                             psmouse, false, set_properties)) {
>                 if (max_proto > PSMOUSE_IMEX &&
>                     IS_ENABLED(CONFIG_MOUSE_PS2_FOCALTECH) &&
>                     (!set_properties || focaltech_init(psmouse) == 0)) {
> @@ -1083,8 +1091,8 @@ static int psmouse_extensions(struct psmouse *psmouse,
>          * probing for IntelliMouse.
>          */
>         if (max_proto > PSMOUSE_PS2 &&
> -           psmouse_try_protocol(psmouse, PSMOUSE_SYNAPTICS, &max_proto,
> -                                set_properties, false)) {
> +           psmouse_do_detect(synaptics_detect,
> +                             psmouse, false, set_properties)) {
>                 synaptics_hardware = true;
>
>                 if (max_proto > PSMOUSE_IMEX) {

Hi Dmitry,

I've compiled the kernel with this config:

$ zgrep CONFIG_MOUSE_PS2 /proc/config.gz
CONFIG_MOUSE_PS2=m
# CONFIG_MOUSE_PS2_ALPS is not set
# CONFIG_MOUSE_PS2_BYD is not set
# CONFIG_MOUSE_PS2_LOGIPS2PP is not set
# CONFIG_MOUSE_PS2_SYNAPTICS is not set
CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS=y
# CONFIG_MOUSE_PS2_CYPRESS is not set
# CONFIG_MOUSE_PS2_LIFEBOOK is not set
CONFIG_MOUSE_PS2_TRACKPOINT=y
# CONFIG_MOUSE_PS2_ELANTECH is not set
# CONFIG_MOUSE_PS2_SENTELIC is not set
# CONFIG_MOUSE_PS2_TOUCHKIT is not set
# CONFIG_MOUSE_PS2_FOCALTECH is not set
CONFIG_MOUSE_PS2_SMBUS=y

and the mouse is correctly detected as rmi4:

$ dmesg |grep rmi4
[   20.750242] rmi4_smbus 7-002c: registering SMbus-connected sensor
[   20.808625] rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer:
Synaptics, product: TM3145-003, fw id: 2073050
[   20.905347] input: Synaptics TM3145-003 as /devices/rmi4-00/input/input16
[   21.299073] input: TPPS/2 IBM TrackPoint as
/devices/rmi4-00/rmi4-00.fn03/serio2/input/input17

Anyway, is CONFIG_MOUSE_PS2_TRACKPOINT still needed for the trackpoint
or CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS will handle both?

Regards,
Dmitry Torokhov Jan. 16, 2018, 11:44 p.m. UTC | #2 
On Tue, Jan 16, 2018 at 07:08:04PM +0100, Matteo Croce wrote:
> On Tue, Jan 9, 2018 at 11:19 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Tue, Jan 02, 2018 at 11:37:07AM -0800, Andrew Duggan wrote:
> >> On 01/02/2018 11:21 AM, Dmitry Torokhov wrote:
> >> > On Tue, Jan 02, 2018 at 08:16:09PM +0100, Matteo Croce wrote:
> >> > > Make MOUSE_PS2_SYNAPTICS_SMBUS select MOUSE_PS2_SYNAPTICS because if
> >> > > the latter is disabled the detection and switching code isn't being run.
> >> > Hmm, it should. In synaptics.c:
> >> >
> >> > #if defined(CONFIG_MOUSE_PS2_SYNAPTICS) || \
> >> >      defined(CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS)
> >> >
> >> > int synaptics_init(struct psmouse *psmouse)
> >> > {
> >> >     struct synaptics_device_info info;
> >> >     int error;
> >> >     int retval;
> >> >
> >> >     psmouse_reset(psmouse);
> >> > ...
> >> > }
> >> >
> >> > The intent was to allow disabling the PS/2 portion of Synaptics driver
> >> > while retaining the switching to SMbus capability.
> >>
> >> Oh, I suggested to Matteo that this patch might be needed off list. I
> >> clearly didn't look closely enough to see that MOUSE_PS2_SYNAPTICS_SMBUS is
> >> intended to be able to be used independently of MOUSE_PS2_SYNAPTICS.
> >>
> >> However, he did seem to see behavior where the mode switch did not occur
> >> when MOUSE_PS2_SYNAPTICS was disabled, but MOUSE_PS2_SYNAPTICS_SMBUS was
> >> enabled. That will need to be investigated further.
> >
> > OK, I see what the problem is. Could you please try the version below?
> >
> > Thanks.
> >
> > --
> > Dmitry
> >
> >
> > Input: psmouse - fix Synaptics detection when protocol is disabled
> >
> > When Synaptics protocol is disabled, we still need to ytry and detect the
> > hardware, so we can switch to SMBus device if SMbus is detected, or we know
> > that it is Synaptics device and reset it properly for the bare PS/2
> > protocol.
> >
> > Fixes: c378b5119eb0 ("Input: psmouse - factor out common protocol probing code")
> > Reported-by: Matteo Croce <mcroce@redhat.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/mouse/psmouse-base.c |   34 +++++++++++++++++++++-------------
> >  1 file changed, 21 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> > index f3eb407a3b7d..cbe12671f784 100644
> > --- a/drivers/input/mouse/psmouse-base.c
> > +++ b/drivers/input/mouse/psmouse-base.c
> > @@ -984,6 +984,21 @@ static void psmouse_apply_defaults(struct psmouse *psmouse)
> >         psmouse->pt_deactivate = NULL;
> >  }
> >
> > +static bool psmouse_do_detect(int (*detect)(struct psmouse *, bool),
> > +                             struct psmouse *psmouse, bool allow_passthrough,
> > +                             bool set_properties)
> > +{
> > +       if (psmouse->ps2dev.serio->id.type == SERIO_PS_PSTHRU &&
> > +           !allow_passthrough) {
> > +               return false;
> > +       }
> > +
> > +       if (set_properties)
> > +               psmouse_apply_defaults(psmouse);
> > +
> > +       return detect(psmouse, set_properties) == 0;
> > +}
> > +
> >  static bool psmouse_try_protocol(struct psmouse *psmouse,
> >                                  enum psmouse_type type,
> >                                  unsigned int *max_proto,
> > @@ -995,15 +1010,8 @@ static bool psmouse_try_protocol(struct psmouse *psmouse,
> >         if (!proto)
> >                 return false;
> >
> > -       if (psmouse->ps2dev.serio->id.type == SERIO_PS_PSTHRU &&
> > -           !proto->try_passthru) {
> > -               return false;
> > -       }
> > -
> > -       if (set_properties)
> > -               psmouse_apply_defaults(psmouse);
> > -
> > -       if (proto->detect(psmouse, set_properties) != 0)
> > +       if (!psmouse_do_detect(proto->detect, psmouse, proto->try_passthru,
> > +                              set_properties))
> >                 return false;
> >
> >         if (set_properties && proto->init && init_allowed) {
> > @@ -1036,8 +1044,8 @@ static int psmouse_extensions(struct psmouse *psmouse,
> >          * Always check for focaltech, this is safe as it uses pnp-id
> >          * matching.
> >          */
> > -       if (psmouse_try_protocol(psmouse, PSMOUSE_FOCALTECH,
> > -                                &max_proto, set_properties, false)) {
> > +       if (psmouse_do_detect(focaltech_detect,
> > +                             psmouse, false, set_properties)) {
> >                 if (max_proto > PSMOUSE_IMEX &&
> >                     IS_ENABLED(CONFIG_MOUSE_PS2_FOCALTECH) &&
> >                     (!set_properties || focaltech_init(psmouse) == 0)) {
> > @@ -1083,8 +1091,8 @@ static int psmouse_extensions(struct psmouse *psmouse,
> >          * probing for IntelliMouse.
> >          */
> >         if (max_proto > PSMOUSE_PS2 &&
> > -           psmouse_try_protocol(psmouse, PSMOUSE_SYNAPTICS, &max_proto,
> > -                                set_properties, false)) {
> > +           psmouse_do_detect(synaptics_detect,
> > +                             psmouse, false, set_properties)) {
> >                 synaptics_hardware = true;
> >
> >                 if (max_proto > PSMOUSE_IMEX) {
> 
> Hi Dmitry,
> 
> I've compiled the kernel with this config:
> 
> $ zgrep CONFIG_MOUSE_PS2 /proc/config.gz
> CONFIG_MOUSE_PS2=m
> # CONFIG_MOUSE_PS2_ALPS is not set
> # CONFIG_MOUSE_PS2_BYD is not set
> # CONFIG_MOUSE_PS2_LOGIPS2PP is not set
> # CONFIG_MOUSE_PS2_SYNAPTICS is not set
> CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS=y
> # CONFIG_MOUSE_PS2_CYPRESS is not set
> # CONFIG_MOUSE_PS2_LIFEBOOK is not set
> CONFIG_MOUSE_PS2_TRACKPOINT=y
> # CONFIG_MOUSE_PS2_ELANTECH is not set
> # CONFIG_MOUSE_PS2_SENTELIC is not set
> # CONFIG_MOUSE_PS2_TOUCHKIT is not set
> # CONFIG_MOUSE_PS2_FOCALTECH is not set
> CONFIG_MOUSE_PS2_SMBUS=y
> 
> and the mouse is correctly detected as rmi4:
> 
> $ dmesg |grep rmi4
> [   20.750242] rmi4_smbus 7-002c: registering SMbus-connected sensor
> [   20.808625] rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer:
> Synaptics, product: TM3145-003, fw id: 2073050
> [   20.905347] input: Synaptics TM3145-003 as /devices/rmi4-00/input/input16
> [   21.299073] input: TPPS/2 IBM TrackPoint as
> /devices/rmi4-00/rmi4-00.fn03/serio2/input/input17

Great, putting you as tested-by then.

> 
> Anyway, is CONFIG_MOUSE_PS2_TRACKPOINT still needed for the trackpoint
> or CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS will handle both?

No, the trackpoint is still a PS/2 device, it is simply attached as
a "guest" or pass-through port of an RMI device. You still need
CONFIG_MOUSE_PS2_TRACKPOINT if you want its advanced features.

Thanks.
diff mbox

Patch

diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
index f3eb407a3b7d..cbe12671f784 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -984,6 +984,21 @@  static void psmouse_apply_defaults(struct psmouse *psmouse)
 	psmouse->pt_deactivate = NULL;
 }
 
+static bool psmouse_do_detect(int (*detect)(struct psmouse *, bool),
+			      struct psmouse *psmouse, bool allow_passthrough,
+			      bool set_properties)
+{
+	if (psmouse->ps2dev.serio->id.type == SERIO_PS_PSTHRU &&
+	    !allow_passthrough) {
+		return false;
+	}
+
+	if (set_properties)
+		psmouse_apply_defaults(psmouse);
+
+	return detect(psmouse, set_properties) == 0;
+}
+
 static bool psmouse_try_protocol(struct psmouse *psmouse,
 				 enum psmouse_type type,
 				 unsigned int *max_proto,
@@ -995,15 +1010,8 @@  static bool psmouse_try_protocol(struct psmouse *psmouse,
 	if (!proto)
 		return false;
 
-	if (psmouse->ps2dev.serio->id.type == SERIO_PS_PSTHRU &&
-	    !proto->try_passthru) {
-		return false;
-	}
-
-	if (set_properties)
-		psmouse_apply_defaults(psmouse);
-
-	if (proto->detect(psmouse, set_properties) != 0)
+	if (!psmouse_do_detect(proto->detect, psmouse, proto->try_passthru,
+			       set_properties))
 		return false;
 
 	if (set_properties && proto->init && init_allowed) {
@@ -1036,8 +1044,8 @@  static int psmouse_extensions(struct psmouse *psmouse,
 	 * Always check for focaltech, this is safe as it uses pnp-id
 	 * matching.
 	 */
-	if (psmouse_try_protocol(psmouse, PSMOUSE_FOCALTECH,
-				 &max_proto, set_properties, false)) {
+	if (psmouse_do_detect(focaltech_detect,
+			      psmouse, false, set_properties)) {
 		if (max_proto > PSMOUSE_IMEX &&
 		    IS_ENABLED(CONFIG_MOUSE_PS2_FOCALTECH) &&
 		    (!set_properties || focaltech_init(psmouse) == 0)) {
@@ -1083,8 +1091,8 @@  static int psmouse_extensions(struct psmouse *psmouse,
 	 * probing for IntelliMouse.
 	 */
 	if (max_proto > PSMOUSE_PS2 &&
-	    psmouse_try_protocol(psmouse, PSMOUSE_SYNAPTICS, &max_proto,
-				 set_properties, false)) {
+	    psmouse_do_detect(synaptics_detect,
+			      psmouse, false, set_properties)) {
 		synaptics_hardware = true;
 
 		if (max_proto > PSMOUSE_IMEX) {