Re: [PATCH 07/13] Input: synaptics-rmi4 - add support for F03

From: Benjamin Tissoires
Date: Thu Dec 01 2016 - 09:54:37 EST

Next message: Mauro Carvalho Chehab: "Re: [PATCH 29/39] Annotate hardware config module parameters in drivers/staging/media/"
Previous message: Philipp Zabel: "Re: [i.MX6 DRM IPUv3] Regression 4.9-rc5: greenish screen with YUV420 video"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Dmitry,

On Nov 30 2016 or thereabouts, Dmitry Torokhov wrote:
> Hi Benjamin,
>
> On Tue, Nov 29, 2016 at 11:08:18AM +0100, Benjamin Tissoires wrote:
> > From: Lyude Paul <thatslyude@xxxxxxxxx>
> >
> > This adds basic functionality for PS/2 passthrough on Synaptics
> > Touchpads using RMI4 through smbus.
> >
> > Reviewed-by: Andrew Duggan <aduggan@xxxxxxxxxxxxx>
> > Signed-off-by: Lyude Paul <thatslyude@xxxxxxxxx>
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> >
> > ---
> > drivers/input/mouse/psmouse-base.c | 6 +
> > drivers/input/rmi4/Kconfig | 9 ++
> > drivers/input/rmi4/Makefile | 1 +
> > drivers/input/rmi4/rmi_bus.c | 3 +
> > drivers/input/rmi4/rmi_driver.h | 1 +
> > drivers/input/rmi4/rmi_f03.c | 227 +++++++++++++++++++++++++++++++++++++
> > include/uapi/linux/serio.h | 1 +
> > 7 files changed, 248 insertions(+)
> > create mode 100644 drivers/input/rmi4/rmi_f03.c
> >
> > diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> > index fb4b185..691dd74 100644
> > --- a/drivers/input/mouse/psmouse-base.c
> > +++ b/drivers/input/mouse/psmouse-base.c
> > @@ -1663,6 +1663,12 @@ static struct serio_device_id psmouse_serio_ids[] = {
> > .id = SERIO_ANY,
> > .extra = SERIO_ANY,
> > },
> > + {
> > + .type = SERIO_RMI_PSTHRU,
>
> Why do we need new serio type here? We had SERIO_PS_PSTHRU because we
> needed some quirks in how it interacted with the parent PS/2 port, but
> here it seems SERIO_I8042 (which could be called SERIO_STANDARD_PS2)
> would suffice?

I guess this must be some kind of left over from the time we were trying
to fix the suspend/resume interactions between this driver and the PS/2
connection of the rmi-smbus devices.

Given the rest of the code which seems to be indeed not dependent of
SERIO_RMI_PSTHRU, I'll switch over to SERIO_I8042.

>
> > + .proto = SERIO_ANY,
> > + .id = SERIO_ANY,
> > + .extra = SERIO_ANY,
> > + },
> > { 0 }
> > };
> >
> > diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
> > index a9c36a5..b8189a3 100644
> > --- a/drivers/input/rmi4/Kconfig
> > +++ b/drivers/input/rmi4/Kconfig
> > @@ -39,6 +39,15 @@ config RMI4_SMB
> > To compile this driver as a module, choose M here: the module will be
> > called rmi_smbus.
> >
> > +config RMI4_F03
> > + bool "RMI4 Function 03 (PS2 Guest)"
> > + depends on RMI4_CORE
> > + help
> > + Say Y here if you want to add support for RMI4 function 03.
> > +
> > + Function 03 provides PS2 guest support for RMI4 devices. This
> > + includes support for TrackPoints on TouchPads.
> > +
> > config RMI4_2D_SENSOR
> > bool
> > depends on RMI4_CORE
> > diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
> > index e7f4ca6..a199cbe 100644
> > --- a/drivers/input/rmi4/Makefile
> > +++ b/drivers/input/rmi4/Makefile
> > @@ -4,6 +4,7 @@ rmi_core-y := rmi_bus.o rmi_driver.o rmi_f01.o
> > rmi_core-$(CONFIG_RMI4_2D_SENSOR) += rmi_2d_sensor.o
> >
> > # Function drivers
> > +rmi_core-$(CONFIG_RMI4_F03) += rmi_f03.o
> > rmi_core-$(CONFIG_RMI4_F11) += rmi_f11.o
> > rmi_core-$(CONFIG_RMI4_F12) += rmi_f12.o
> > rmi_core-$(CONFIG_RMI4_F30) += rmi_f30.o
> > diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> > index 81be9c1..1c40d94 100644
> > --- a/drivers/input/rmi4/rmi_bus.c
> > +++ b/drivers/input/rmi4/rmi_bus.c
> > @@ -305,6 +305,9 @@ struct bus_type rmi_bus_type = {
> >
> > static struct rmi_function_handler *fn_handlers[] = {
> > &rmi_f01_handler,
> > +#ifdef CONFIG_RMI4_F03
> > + &rmi_f03_handler,
> > +#endif
> > #ifdef CONFIG_RMI4_F11
> > &rmi_f11_handler,
> > #endif
> > diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> > index cc94585..24f8f76 100644
> > --- a/drivers/input/rmi4/rmi_driver.h
> > +++ b/drivers/input/rmi4/rmi_driver.h
> > @@ -121,6 +121,7 @@ static inline void rmi_f34_remove_sysfs(struct rmi_device *rmi_dev)
> > #endif /* CONFIG_RMI_F34 */
> >
> > extern struct rmi_function_handler rmi_f01_handler;
> > +extern struct rmi_function_handler rmi_f03_handler;
> > extern struct rmi_function_handler rmi_f11_handler;
> > extern struct rmi_function_handler rmi_f12_handler;
> > extern struct rmi_function_handler rmi_f30_handler;
> > diff --git a/drivers/input/rmi4/rmi_f03.c b/drivers/input/rmi4/rmi_f03.c
> > new file mode 100644
> > index 0000000..a7e1b98
> > --- /dev/null
> > +++ b/drivers/input/rmi4/rmi_f03.c
> > @@ -0,0 +1,227 @@
> > +/*
> > + * Copyright (C) 2015-2016 Red Hat
> > + * Copyright (C) 2015 Lyude Paul <thatslyude@xxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as published by
> > + * the Free Software Foundation.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/serio.h>
> > +#include <linux/notifier.h>
> > +#include "rmi_driver.h"
> > +
> > +#define RMI_F03_RX_DATA_OFB 0x01
> > +#define RMI_F03_OB_SIZE 2
> > +
> > +#define RMI_F03_OB_OFFSET 2
> > +#define RMI_F03_OB_DATA_OFFSET 1
> > +#define RMI_F03_OB_FLAG_TIMEOUT (1 << 6)
> > +#define RMI_F03_OB_FLAG_PARITY (1 << 7)
>
> BIT()?
>
> > +
> > +#define RMI_F03_DEVICE_COUNT 0x07
> > +#define RMI_F03_BYTES_PER_DEVICE_MASK 0x70
> > +#define RMI_F03_BYTES_PER_DEVICE_SHIFT 4
> > +#define RMI_F03_QUEUE_LENGTH 0x0F
> > +
> > +struct f03_data {
> > + struct rmi_function *fn;
> > +
> > + struct serio *serio;
> > +
> > + unsigned int overwrite_buttons;
>
> Unused?

Looks like it's a bad split between the F03 core implementation and the
tweaks we have to do for the trackstick "buttons-that-are-wired-into-the-
touchpad-instead-of-the-trackstick".

>
> > +
> > + u8 device_count;
> > + u8 rx_queue_length;
> > +};
> > +
> > +static int rmi_f03_pt_write(struct serio *id, unsigned char val)
> > +{
> > + struct f03_data *f03 = id->port_data;
> > + int rc;
> > +
> > + rmi_dbg(RMI_DEBUG_FN, &f03->fn->dev,
> > + "%s: Wrote %.2hhx to PS/2 passthrough address",
> > + __func__, val);
> > +
> > + rc = rmi_write(f03->fn->rmi_dev, f03->fn->fd.data_base_addr, val);
>
> error = rmi_write()?
>
> > + if (rc) {
> > + dev_err(&f03->fn->dev,
> > + "%s: Failed to write to F03 TX register.\n", __func__);
>
> Please report error code as well.
>
> > + return rc;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static inline int rmi_f03_initialize(struct rmi_function *fn)
>
> Why inline?
>
> > +{
> > + struct f03_data *f03;
> > + struct device *dev = &fn->dev;
> > + int rc;
> > + u8 bytes_per_device;
> > + u8 query1;
> > + size_t query2_len;
> > +
> > + rc = rmi_read(fn->rmi_dev, fn->fd.query_base_addr, &query1);
>
> error = rmi_read()?
>
> > + if (rc) {
> > + dev_err(dev, "Failed to read query register.\n");
> > + return rc;
> > + }
> > +
> > + f03 = devm_kzalloc(dev, sizeof(struct f03_data), GFP_KERNEL);
> > + if (!f03)
> > + return -ENOMEM;
> > +
> > + f03->device_count = query1 & RMI_F03_DEVICE_COUNT;
> > + bytes_per_device = (query1 & RMI_F03_BYTES_PER_DEVICE_MASK) >>
> > + RMI_F03_BYTES_PER_DEVICE_SHIFT;
> > +
> > + query2_len = f03->device_count * bytes_per_device;
> > +
> > + /*
> > + * The first generation of image sensors don't have a second part to
> > + * their f03 query, as such we have to set some of these values manually
> > + */
> > + if (query2_len < 1) {
> > + f03->device_count = 1;
> > + f03->rx_queue_length = 7;
> > + } else {
> > + u8 query2[query2_len];
> > +
> > + rc = rmi_read_block(fn->rmi_dev, fn->fd.query_base_addr + 1,
> > + query2, query2_len);
> > + if (rc) {
> > + dev_err(dev, "Failed to read second set of query registers.\n");
> > + return rc;
> > + }
> > +
> > + f03->rx_queue_length = query2[0] & RMI_F03_QUEUE_LENGTH;
> > + }
> > +
> > + f03->fn = fn;
> > +
> > + dev_set_drvdata(dev, f03);
> > +
> > + return f03->device_count;
>
> I'd rather we returned customary 0 here.
>
> > +}
> > +
> > +static inline int rmi_f03_register_pt(struct rmi_function *fn)
> > +{
> > + struct f03_data *f03 = dev_get_drvdata(&fn->dev);
> > + struct serio *serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
>
> Please do not do allocations at declaration; limit to declarations with
> initialization to operations without side effect. So:
>
> struct serio *serio;
>
> serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> if (!serio)
> return -ENOMEM;
>
> > +
> > + if (!serio)
> > + return -ENOMEM;
> > +
> > + serio->id.type = SERIO_RMI_PSTHRU;
> > + serio->write = rmi_f03_pt_write;
> > + serio->port_data = f03;
> > +
> > + strlcpy(serio->name, "Synaptics RMI4 PS2 pass-through",
> > + sizeof(serio->name));
> > + strlcpy(serio->phys, "synaptics-rmi4-pt/serio1",
> > + sizeof(serio->phys));
> > + serio->dev.parent = &fn->dev;
>
> Extra space after tab in indentation.
>
> > +
> > + f03->serio = serio;
> > +
> > + serio_register_port(serio);
> > +
> > + return 0;
> > +}
> > +
> > +static int rmi_f03_probe(struct rmi_function *fn)
> > +{
> > + int rc;
>
> int error;
>
> Maybe allocate the memory here...
>
> > +
> > + rc = rmi_f03_initialize(fn);
> > + if (rc < 0)
> > + return rc;
> > +
> > + rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%d devices on PS/2 passthrough", rc);
>
> so you can use f03->device_count here.
>
> Do we need to warn if we see device count greater than 1?

Good quesstion. I can't recall the exact protocol, and we certainly
never seen anything with more than one device. So probably yes, we
should shout something if it's not what we expect.

>
> > +
> > + rc = rmi_f03_register_pt(fn);
> > + if (rc)
> > + return rc;
> > +
> > + return 0;
> > +}
> > +
> > +static int rmi_f03_config(struct rmi_function *fn)
> > +{
> > + fn->rmi_dev->driver->set_irq_bits(fn->rmi_dev, fn->irq_mask);
> > +
> > + return 0;
> > +}
> > +
> > +static int rmi_f03_attention(struct rmi_function *fn, unsigned long *irq_bits)
> > +{
> > + struct f03_data *f03 = dev_get_drvdata(&fn->dev);
> > + u16 data_addr = fn->fd.data_base_addr;
> > + const u8 ob_len = f03->rx_queue_length * RMI_F03_OB_SIZE;
> > + u8 obs[ob_len];
> > + u8 ob_status;
> > + u8 ob_data;
> > + unsigned int serio_flags;
> > + int i;
> > + int retval;
> > +
> > + /* Grab all of the data registers, and check them for data */
> > + retval = rmi_read_block(fn->rmi_dev, data_addr + RMI_F03_OB_OFFSET,
> > + &obs, ob_len);
> > + if (retval) {
> > + dev_err(&fn->dev, "%s: Failed to read F03 output buffers.\n",
> > + __func__);
> > + serio_interrupt(f03->serio, 0, SERIO_TIMEOUT);
> > + return retval;
> > + }
> > +
> > + for (i = 0; i < ob_len; i += RMI_F03_OB_SIZE) {
> > + ob_status = obs[i];
> > + ob_data = obs[i + RMI_F03_OB_DATA_OFFSET];
> > + serio_flags = 0;
> > +
> > + if (!(ob_status & RMI_F03_RX_DATA_OFB))
> > + continue;
> > +
> > + if (ob_status & RMI_F03_OB_FLAG_TIMEOUT)
> > + serio_flags |= SERIO_TIMEOUT;
> > + if (ob_status & RMI_F03_OB_FLAG_PARITY)
> > + serio_flags |= SERIO_PARITY;
> > +
> > + rmi_dbg(RMI_DEBUG_FN, &fn->dev,
> > + "%s: Received %.2hhx from PS2 guest T: %c P: %c\n",
> > + __func__, ob_data,
> > + serio_flags & SERIO_TIMEOUT ? 'Y' : 'N',
> > + serio_flags & SERIO_PARITY ? 'Y' : 'N');
> > +
> > + serio_interrupt(f03->serio, ob_data, serio_flags);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void rmi_f03_remove(struct rmi_function *fn)
> > +{
> > + struct f03_data *f03 = dev_get_drvdata(&fn->dev);
> > +
> > + serio_unregister_port(f03->serio);
> > +}
> > +
> > +struct rmi_function_handler rmi_f03_handler = {
> > + .driver = {
> > + .name = "rmi4_f03",
> > + },
> > + .func = 0x03,
> > + .probe = rmi_f03_probe,
> > + .config = rmi_f03_config,
> > + .attention = rmi_f03_attention,
> > + .remove = rmi_f03_remove,
> > +};
> > +
> > +MODULE_AUTHOR("Lyude Paul <thatslyude@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("RMI F03 module");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/uapi/linux/serio.h b/include/uapi/linux/serio.h
> > index f2447a8..7012178 100644
> > --- a/include/uapi/linux/serio.h
> > +++ b/include/uapi/linux/serio.h
> > @@ -30,6 +30,7 @@
> > #define SERIO_HIL_MLC 0x03
> > #define SERIO_PS_PSTHRU 0x05
> > #define SERIO_8042_XL 0x06
> > +#define SERIO_RMI_PSTHRU 0x07
> >
> > /*
> > * Serio protocols
> > --
> > 2.7.4
> >
>
> Thanks.
>

I saw that you pushed part of the series. Many thanks for that. I'll
work on the fixes for this one and submit v2 ASAP.

Cheers,
Benjamin

> --
> Dmitry

Next message: Mauro Carvalho Chehab: "Re: [PATCH 29/39] Annotate hardware config module parameters in drivers/staging/media/"
Previous message: Philipp Zabel: "Re: [i.MX6 DRM IPUv3] Regression 4.9-rc5: greenish screen with YUV420 video"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]