* [PATCH -next 2/2] Input: synaptics-rmi4 - Propagate correct number of rx and tx electrodes to F54
2016-10-01 3:22 [PATCH -next 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning Guenter Roeck
@ 2016-10-01 3:22 ` Guenter Roeck
2016-10-25 0:59 ` Andrew Duggan
2016-10-17 21:30 ` [-next, 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning Guenter Roeck
2016-10-25 0:59 ` [PATCH -next " Andrew Duggan
2 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2016-10-01 3:22 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Chris Healy, Nick Dyer, Mauro Carvalho Chehab, Hans Verkuil,
linux-kernel, linux-input, Guenter Roeck, Andrew Duggan
F54 diagnostics report functions provide data based on the number of
enabled rx and tx electrodes, which is not identical to the number of
electrodes reported with F54:Query0 and F54:Query1. Those values report
the number of supported electrodes, not the number of enabled electrodes.
The number of enabled electrodes can be determined by analyzing F55:Ctrl1
(sensor receiver assignment) and F55:Ctrl2 (sensor transmitter assignment).
Propagate the number of enabled electrodes from F55 to F54 to avoid
corrupted output if not all electrodes are enabled.
Fixes: 3bbacb89704fi ("[media] Input: synaptics-rmi4 - add support for F54 ...")
Cc: Nick Dyer <nick@shmanahar.org>
Cc: Andrew Duggan <aduggan@synaptics.com>
Cc: Chris Healy <cphealy@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
Notes:
- The Fixes: SHA might not be accurate if the originating branch is rebased.
- This patch depends on the preceding patch (which, strictly speaking, does
not fix anything).
drivers/input/rmi4/Kconfig | 1 +
drivers/input/rmi4/rmi_f54.c | 14 ++++++++++----
drivers/input/rmi4/rmi_f55.c | 7 +++++++
include/linux/rmi.h | 3 +++
4 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
index 11ede43c9936..d7129928cde6 100644
--- a/drivers/input/rmi4/Kconfig
+++ b/drivers/input/rmi4/Kconfig
@@ -67,6 +67,7 @@ config RMI4_F54
depends on RMI4_CORE
depends on VIDEO_V4L2=y || (RMI4_CORE=m && VIDEO_V4L2=m)
select VIDEOBUF2_VMALLOC
+ select RMI4_F55
help
Say Y here if you want to add support for RMI4 function 54
diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index cf805b960866..9cb3aa733f0f 100644
--- a/drivers/input/rmi4/rmi_f54.c
+++ b/drivers/input/rmi4/rmi_f54.c
@@ -216,8 +216,10 @@ static int rmi_f54_request_report(struct rmi_function *fn, u8 report_type)
static size_t rmi_f54_get_report_size(struct f54_data *f54)
{
- u8 rx = f54->num_rx_electrodes ? : f54->num_rx_electrodes;
- u8 tx = f54->num_tx_electrodes ? : f54->num_tx_electrodes;
+ struct rmi_device *rmi_dev = f54->fn->rmi_dev;
+ struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+ u8 rx = drv_data->num_rx_electrodes ? : f54->num_rx_electrodes;
+ u8 tx = drv_data->num_tx_electrodes ? : f54->num_tx_electrodes;
size_t size;
switch (rmi_f54_get_reptype(f54, f54->input)) {
@@ -401,6 +403,10 @@ static int rmi_f54_vidioc_enum_input(struct file *file, void *priv,
static int rmi_f54_set_input(struct f54_data *f54, unsigned int i)
{
+ struct rmi_device *rmi_dev = f54->fn->rmi_dev;
+ struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+ u8 rx = drv_data->num_rx_electrodes ? : f54->num_rx_electrodes;
+ u8 tx = drv_data->num_tx_electrodes ? : f54->num_tx_electrodes;
struct v4l2_pix_format *f = &f54->format;
enum rmi_f54_report_type reptype;
int ret;
@@ -415,8 +421,8 @@ static int rmi_f54_set_input(struct f54_data *f54, unsigned int i)
f54->input = i;
- f->width = f54->num_rx_electrodes;
- f->height = f54->num_tx_electrodes;
+ f->width = rx;
+ f->height = tx;
f->field = V4L2_FIELD_NONE;
f->colorspace = V4L2_COLORSPACE_RAW;
f->bytesperline = f->width * sizeof(u16);
diff --git a/drivers/input/rmi4/rmi_f55.c b/drivers/input/rmi4/rmi_f55.c
index 268fa904205a..b95a5d60ba96 100644
--- a/drivers/input/rmi4/rmi_f55.c
+++ b/drivers/input/rmi4/rmi_f55.c
@@ -41,6 +41,8 @@ struct f55_data {
static int rmi_f55_detect(struct rmi_function *fn)
{
+ struct rmi_device *rmi_dev = fn->rmi_dev;
+ struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
struct f55_data *f55;
int error;
@@ -60,6 +62,9 @@ static int rmi_f55_detect(struct rmi_function *fn)
f55->cfg_num_rx_electrodes = f55->num_rx_electrodes;
f55->cfg_num_tx_electrodes = f55->num_rx_electrodes;
+ drv_data->num_rx_electrodes = f55->cfg_num_rx_electrodes;
+ drv_data->num_tx_electrodes = f55->cfg_num_rx_electrodes;
+
if (f55->qry[F55_PHYS_CHAR_OFFSET] & F55_CAP_SENSOR_ASSIGN) {
int i, total;
u8 buf[256];
@@ -81,6 +86,7 @@ static int rmi_f55_detect(struct rmi_function *fn)
total++;
}
f55->cfg_num_rx_electrodes = total;
+ drv_data->num_rx_electrodes = total;
}
error = rmi_read_block(fn->rmi_dev,
@@ -93,6 +99,7 @@ static int rmi_f55_detect(struct rmi_function *fn)
total++;
}
f55->cfg_num_tx_electrodes = total;
+ drv_data->num_tx_electrodes = total;
}
}
diff --git a/include/linux/rmi.h b/include/linux/rmi.h
index e0aca1476001..45734f1343b3 100644
--- a/include/linux/rmi.h
+++ b/include/linux/rmi.h
@@ -345,6 +345,9 @@ struct rmi_driver_data {
u8 pdt_props;
u8 bsr;
+ u8 num_rx_electrodes;
+ u8 num_tx_electrodes;
+
bool enabled;
void *data;
--
2.5.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH -next 2/2] Input: synaptics-rmi4 - Propagate correct number of rx and tx electrodes to F54
2016-10-01 3:22 ` [PATCH -next 2/2] Input: synaptics-rmi4 - Propagate correct number of rx and tx electrodes to F54 Guenter Roeck
@ 2016-10-25 0:59 ` Andrew Duggan
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Duggan @ 2016-10-25 0:59 UTC (permalink / raw)
To: Guenter Roeck, Dmitry Torokhov
Cc: Chris Healy, Nick Dyer, Mauro Carvalho Chehab, Hans Verkuil,
linux-kernel, linux-input
On 09/30/2016 08:22 PM, Guenter Roeck wrote:
> F54 diagnostics report functions provide data based on the number of
> enabled rx and tx electrodes, which is not identical to the number of
> electrodes reported with F54:Query0 and F54:Query1. Those values report
> the number of supported electrodes, not the number of enabled electrodes.
> The number of enabled electrodes can be determined by analyzing F55:Ctrl1
> (sensor receiver assignment) and F55:Ctrl2 (sensor transmitter assignment).
>
> Propagate the number of enabled electrodes from F55 to F54 to avoid
> corrupted output if not all electrodes are enabled.
>
> Fixes: 3bbacb89704fi ("[media] Input: synaptics-rmi4 - add support for F54 ...")
> Cc: Nick Dyer <nick@shmanahar.org>
> Cc: Andrew Duggan <aduggan@synaptics.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Andrew Duggan <aduggan@synaptics.com>
> ---
> Notes:
> - The Fixes: SHA might not be accurate if the originating branch is rebased.
> - This patch depends on the preceding patch (which, strictly speaking, does
> not fix anything).
>
> drivers/input/rmi4/Kconfig | 1 +
> drivers/input/rmi4/rmi_f54.c | 14 ++++++++++----
> drivers/input/rmi4/rmi_f55.c | 7 +++++++
> include/linux/rmi.h | 3 +++
> 4 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
> index 11ede43c9936..d7129928cde6 100644
> --- a/drivers/input/rmi4/Kconfig
> +++ b/drivers/input/rmi4/Kconfig
> @@ -67,6 +67,7 @@ config RMI4_F54
> depends on RMI4_CORE
> depends on VIDEO_V4L2=y || (RMI4_CORE=m && VIDEO_V4L2=m)
> select VIDEOBUF2_VMALLOC
> + select RMI4_F55
> help
> Say Y here if you want to add support for RMI4 function 54
>
> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> index cf805b960866..9cb3aa733f0f 100644
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c
> @@ -216,8 +216,10 @@ static int rmi_f54_request_report(struct rmi_function *fn, u8 report_type)
>
> static size_t rmi_f54_get_report_size(struct f54_data *f54)
> {
> - u8 rx = f54->num_rx_electrodes ? : f54->num_rx_electrodes;
> - u8 tx = f54->num_tx_electrodes ? : f54->num_tx_electrodes;
> + struct rmi_device *rmi_dev = f54->fn->rmi_dev;
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + u8 rx = drv_data->num_rx_electrodes ? : f54->num_rx_electrodes;
> + u8 tx = drv_data->num_tx_electrodes ? : f54->num_tx_electrodes;
> size_t size;
>
> switch (rmi_f54_get_reptype(f54, f54->input)) {
> @@ -401,6 +403,10 @@ static int rmi_f54_vidioc_enum_input(struct file *file, void *priv,
>
> static int rmi_f54_set_input(struct f54_data *f54, unsigned int i)
> {
> + struct rmi_device *rmi_dev = f54->fn->rmi_dev;
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + u8 rx = drv_data->num_rx_electrodes ? : f54->num_rx_electrodes;
> + u8 tx = drv_data->num_tx_electrodes ? : f54->num_tx_electrodes;
> struct v4l2_pix_format *f = &f54->format;
> enum rmi_f54_report_type reptype;
> int ret;
> @@ -415,8 +421,8 @@ static int rmi_f54_set_input(struct f54_data *f54, unsigned int i)
>
> f54->input = i;
>
> - f->width = f54->num_rx_electrodes;
> - f->height = f54->num_tx_electrodes;
> + f->width = rx;
> + f->height = tx;
> f->field = V4L2_FIELD_NONE;
> f->colorspace = V4L2_COLORSPACE_RAW;
> f->bytesperline = f->width * sizeof(u16);
> diff --git a/drivers/input/rmi4/rmi_f55.c b/drivers/input/rmi4/rmi_f55.c
> index 268fa904205a..b95a5d60ba96 100644
> --- a/drivers/input/rmi4/rmi_f55.c
> +++ b/drivers/input/rmi4/rmi_f55.c
> @@ -41,6 +41,8 @@ struct f55_data {
>
> static int rmi_f55_detect(struct rmi_function *fn)
> {
> + struct rmi_device *rmi_dev = fn->rmi_dev;
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> struct f55_data *f55;
> int error;
>
> @@ -60,6 +62,9 @@ static int rmi_f55_detect(struct rmi_function *fn)
> f55->cfg_num_rx_electrodes = f55->num_rx_electrodes;
> f55->cfg_num_tx_electrodes = f55->num_rx_electrodes;
>
> + drv_data->num_rx_electrodes = f55->cfg_num_rx_electrodes;
> + drv_data->num_tx_electrodes = f55->cfg_num_rx_electrodes;
> +
> if (f55->qry[F55_PHYS_CHAR_OFFSET] & F55_CAP_SENSOR_ASSIGN) {
> int i, total;
> u8 buf[256];
> @@ -81,6 +86,7 @@ static int rmi_f55_detect(struct rmi_function *fn)
> total++;
> }
> f55->cfg_num_rx_electrodes = total;
> + drv_data->num_rx_electrodes = total;
> }
>
> error = rmi_read_block(fn->rmi_dev,
> @@ -93,6 +99,7 @@ static int rmi_f55_detect(struct rmi_function *fn)
> total++;
> }
> f55->cfg_num_tx_electrodes = total;
> + drv_data->num_tx_electrodes = total;
> }
> }
>
> diff --git a/include/linux/rmi.h b/include/linux/rmi.h
> index e0aca1476001..45734f1343b3 100644
> --- a/include/linux/rmi.h
> +++ b/include/linux/rmi.h
> @@ -345,6 +345,9 @@ struct rmi_driver_data {
> u8 pdt_props;
> u8 bsr;
>
> + u8 num_rx_electrodes;
> + u8 num_tx_electrodes;
> +
> bool enabled;
>
> void *data;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [-next, 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning
2016-10-01 3:22 [PATCH -next 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning Guenter Roeck
2016-10-01 3:22 ` [PATCH -next 2/2] Input: synaptics-rmi4 - Propagate correct number of rx and tx electrodes to F54 Guenter Roeck
@ 2016-10-17 21:30 ` Guenter Roeck
2016-10-20 22:51 ` Nick Dyer
2016-10-25 0:59 ` [PATCH -next " Andrew Duggan
2 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2016-10-17 21:30 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Chris Healy, Nick Dyer, Mauro Carvalho Chehab, Hans Verkuil,
linux-kernel, linux-input
On Fri, Sep 30, 2016 at 08:22:47PM -0700, Guenter Roeck wrote:
> Sensor tuning support is needed to determine the number of enabled
> tx and rx electrodes for use in F54 functions.
>
> The number of enabled electrodes is not identical to the total number
> of electrodes as reported with F55:Query0 and F55:Query1. It has to be
> calculated by analyzing F55:Ctrl1 (sensor receiver assignment) and
> F55:Ctrl2 (sensor transmitter assignment).
>
> Support for additional sensor tuning functions may be added later.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Ping ... any comments on this patch and on
https://patchwork.kernel.org/patch/9359061/ ?
Both patches now apply to mainline.
Thanks,
Guenter
> ---
> This patch applies to next-20160930.
>
> 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_f55.c | 127 ++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 141 insertions(+)
> create mode 100644 drivers/input/rmi4/rmi_f55.c
>
> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
> index 4c8a55857e00..11ede43c9936 100644
> --- a/drivers/input/rmi4/Kconfig
> +++ b/drivers/input/rmi4/Kconfig
> @@ -72,3 +72,12 @@ config RMI4_F54
>
> Function 54 provides access to various diagnostic features in certain
> RMI4 touch sensors.
> +
> +config RMI4_F55
> + bool "RMI4 Function 55 (Sensor tuning)"
> + depends on RMI4_CORE
> + help
> + Say Y here if you want to add support for RMI4 function 55
> +
> + Function 55 provides access to the RMI4 touch sensor tuning
> + mechanism.
> diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
> index 0bafc8502c4b..96f8e0c21e3b 100644
> --- a/drivers/input/rmi4/Makefile
> +++ b/drivers/input/rmi4/Makefile
> @@ -8,6 +8,7 @@ rmi_core-$(CONFIG_RMI4_F11) += rmi_f11.o
> rmi_core-$(CONFIG_RMI4_F12) += rmi_f12.o
> rmi_core-$(CONFIG_RMI4_F30) += rmi_f30.o
> rmi_core-$(CONFIG_RMI4_F54) += rmi_f54.o
> +rmi_core-$(CONFIG_RMI4_F55) += rmi_f55.o
>
> # Transports
> obj-$(CONFIG_RMI4_I2C) += rmi_i2c.o
> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> index ef8c747c35e7..82b7d4960858 100644
> --- a/drivers/input/rmi4/rmi_bus.c
> +++ b/drivers/input/rmi4/rmi_bus.c
> @@ -314,6 +314,9 @@ static struct rmi_function_handler *fn_handlers[] = {
> #ifdef CONFIG_RMI4_F54
> &rmi_f54_handler,
> #endif
> +#ifdef CONFIG_RMI4_F55
> + &rmi_f55_handler,
> +#endif
> };
>
> static void __rmi_unregister_function_handlers(int start_idx)
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> index 8dfbebe9bf86..a65cf70f61e2 100644
> --- a/drivers/input/rmi4/rmi_driver.h
> +++ b/drivers/input/rmi4/rmi_driver.h
> @@ -103,4 +103,5 @@ extern struct rmi_function_handler rmi_f11_handler;
> extern struct rmi_function_handler rmi_f12_handler;
> extern struct rmi_function_handler rmi_f30_handler;
> extern struct rmi_function_handler rmi_f54_handler;
> +extern struct rmi_function_handler rmi_f55_handler;
> #endif
> diff --git a/drivers/input/rmi4/rmi_f55.c b/drivers/input/rmi4/rmi_f55.c
> new file mode 100644
> index 000000000000..268fa904205a
> --- /dev/null
> +++ b/drivers/input/rmi4/rmi_f55.c
> @@ -0,0 +1,127 @@
> +/*
> + * Copyright (c) 2012-2015 Synaptics Incorporated
> + * Copyright (C) 2016 Zodiac Inflight Innovations
> + *
> + * 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/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/kernel.h>
> +#include <linux/rmi.h>
> +#include <linux/slab.h>
> +#include "rmi_driver.h"
> +
> +#define F55_NAME "rmi4_f55"
> +
> +/* F55 data offsets */
> +#define F55_NUM_RX_OFFSET 0
> +#define F55_NUM_TX_OFFSET 1
> +#define F55_PHYS_CHAR_OFFSET 2
> +
> +/* Fixed sizes of reports */
> +#define F55_QUERY_LEN 17
> +
> +/* F55 capabilities */
> +#define F55_CAP_SENSOR_ASSIGN BIT(0)
> +
> +struct f55_data {
> + struct rmi_function *fn;
> +
> + u8 qry[F55_QUERY_LEN];
> + u8 num_rx_electrodes;
> + u8 cfg_num_rx_electrodes;
> + u8 num_tx_electrodes;
> + u8 cfg_num_tx_electrodes;
> +};
> +
> +static int rmi_f55_detect(struct rmi_function *fn)
> +{
> + struct f55_data *f55;
> + int error;
> +
> + f55 = dev_get_drvdata(&fn->dev);
> +
> + error = rmi_read_block(fn->rmi_dev, fn->fd.query_base_addr,
> + &f55->qry, sizeof(f55->qry));
> + if (error) {
> + dev_err(&fn->dev, "%s: Failed to query F55 properties\n",
> + __func__);
> + return error;
> + }
> +
> + f55->num_rx_electrodes = f55->qry[F55_NUM_RX_OFFSET];
> + f55->num_tx_electrodes = f55->qry[F55_NUM_TX_OFFSET];
> +
> + f55->cfg_num_rx_electrodes = f55->num_rx_electrodes;
> + f55->cfg_num_tx_electrodes = f55->num_rx_electrodes;
> +
> + if (f55->qry[F55_PHYS_CHAR_OFFSET] & F55_CAP_SENSOR_ASSIGN) {
> + int i, total;
> + u8 buf[256];
> +
> + /*
> + * Calculate the number of enabled receive and transmit
> + * electrodes by reading F55:Ctrl1 (sensor receiver assignment)
> + * and F55:Ctrl2 (sensor transmitter assignment). The number of
> + * enabled electrodes is the sum of all field entries with a
> + * value other than 0xff.
> + */
> + error = rmi_read_block(fn->rmi_dev,
> + fn->fd.control_base_addr + 1,
> + buf, f55->num_rx_electrodes);
> + if (!error) {
> + total = 0;
> + for (i = 0; i < f55->num_rx_electrodes; i++) {
> + if (buf[i] != 0xff)
> + total++;
> + }
> + f55->cfg_num_rx_electrodes = total;
> + }
> +
> + error = rmi_read_block(fn->rmi_dev,
> + fn->fd.control_base_addr + 2,
> + buf, f55->num_tx_electrodes);
> + if (!error) {
> + total = 0;
> + for (i = 0; i < f55->num_tx_electrodes; i++) {
> + if (buf[i] != 0xff)
> + total++;
> + }
> + f55->cfg_num_tx_electrodes = total;
> + }
> + }
> +
> + rmi_dbg(RMI_DEBUG_FN, &fn->dev, "F55 num_rx_electrodes: %d (raw %d)\n",
> + f55->cfg_num_rx_electrodes, f55->num_rx_electrodes);
> + rmi_dbg(RMI_DEBUG_FN, &fn->dev, "F55 num_tx_electrodes: %d (raw %d)\n",
> + f55->cfg_num_tx_electrodes, f55->num_tx_electrodes);
> +
> + return 0;
> +}
> +
> +static int rmi_f55_probe(struct rmi_function *fn)
> +{
> + struct f55_data *f55;
> +
> + f55 = devm_kzalloc(&fn->dev, sizeof(struct f55_data), GFP_KERNEL);
> + if (!f55)
> + return -ENOMEM;
> +
> + f55->fn = fn;
> + dev_set_drvdata(&fn->dev, f55);
> +
> + return rmi_f55_detect(fn);
> +}
> +
> +struct rmi_function_handler rmi_f55_handler = {
> + .driver = {
> + .name = F55_NAME,
> + },
> + .func = 0x55,
> + .probe = rmi_f55_probe,
> +};
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [-next, 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning
2016-10-17 21:30 ` [-next, 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning Guenter Roeck
@ 2016-10-20 22:51 ` Nick Dyer
2016-10-20 23:28 ` Christopher Heiny
0 siblings, 1 reply; 12+ messages in thread
From: Nick Dyer @ 2016-10-20 22:51 UTC (permalink / raw)
To: Guenter Roeck
Cc: Dmitry Torokhov, Chris Healy, Christopher Heiny, Andrew Duggan,
Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, linux-input
On Mon, Oct 17, 2016 at 02:30:08PM -0700, Guenter Roeck wrote:
> On Fri, Sep 30, 2016 at 08:22:47PM -0700, Guenter Roeck wrote:
> > Sensor tuning support is needed to determine the number of enabled
> > tx and rx electrodes for use in F54 functions.
> >
> > The number of enabled electrodes is not identical to the total number
> > of electrodes as reported with F55:Query0 and F55:Query1. It has to be
> > calculated by analyzing F55:Ctrl1 (sensor receiver assignment) and
> > F55:Ctrl2 (sensor transmitter assignment).
> >
> > Support for additional sensor tuning functions may be added later.
> >
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> Ping ... any comments on this patch and on
> https://patchwork.kernel.org/patch/9359061/ ?
>
> Both patches now apply to mainline.
>
> Thanks,
> Guenter
Hi Guenter-
I've reviewed and tested (on S7300 and S7813) both these patches now
- you can add my sign-off.
However, on the S7813 firmware, F55 is on PDT page 3, and nothing
on page 2, so the default behaviour of the mainline driver means it is
not initialised.
So I think we need to revert this change in mainline:
https://patchwork.kernel.org/patch/3796971/
See below the PDT scan with it reverted and some debug added.
Christopher/Andrew: is there a better heuristic than scanning all 255
pages, given that some firmwares contain gaps?
cheers
Nick
[ 2.181199] rmi4_physical rmi4-00: Creating functions.
[ 2.181210] rmi4_physical rmi4-00: rmi_scan_pdt page 0
[ 2.181221] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 233
[ 2.182218] rmi4_physical rmi4-00: rmi_read_pdt_entry: F34 V2
[ 2.182230] rmi4_physical rmi4-00: Initializing F34.
[ 2.182325] rmi4_physical rmi4-00: Registered F34.
[ 2.182337] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 227
[ 2.183003] rmi4_physical rmi4-00: rmi_read_pdt_entry: F01 V0
[ 2.183014] rmi4_physical rmi4-00: Initializing F01.
[ 2.187358] rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer: Synaptics, product: s7813, fw id: 2174259
[ 2.198822] rmi4_physical rmi4-00: Registered F01.
[ 2.198834] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 221
[ 2.199494] rmi4_physical rmi4-00: rmi_read_pdt_entry: F12 V0
[ 2.199505] rmi4_physical rmi4-00: Initializing F12.
[ 2.199612] rmi4_f12 rmi4-00.fn12: rmi_f12_probe
[ 2.210721] rmi4_physical rmi4-00: Registered F12.
[ 2.210732] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 215
[ 2.211393] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
[ 2.211404] rmi4_physical rmi4-00: rmi_scan_pdt_page end of page
[ 2.211414] rmi4_physical rmi4-00: rmi_scan_pdt page 1
[ 2.211424] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 489
[ 2.212419] rmi4_physical rmi4-00: rmi_read_pdt_entry: F54 V0
[ 2.212431] rmi4_physical rmi4-00: Initializing F54.
[ 2.214241] rmi4_f54 rmi4-00.fn54: F54 num_rx_electrodes: 60
[ 2.214253] rmi4_f54 rmi4-00.fn54: F54 num_tx_electrodes: 36
[ 2.214263] rmi4_f54 rmi4-00.fn54: F54 capabilities: 0x44
[ 2.214274] rmi4_f54 rmi4-00.fn54: F54 clock rate: 0x5aa0
[ 2.214283] rmi4_f54 rmi4-00.fn54: F54 family: 0x2
[ 2.214695] rmi4_physical rmi4-00: Registered F54.
[ 2.214708] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 483
[ 2.215372] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
[ 2.215384] rmi4_physical rmi4-00: rmi_scan_pdt_page end of page
[ 2.215395] rmi4_physical rmi4-00: rmi_scan_pdt page 2
[ 2.215405] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 745
[ 2.216404] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
[ 2.216415] rmi4_physical rmi4-00: rmi_scan_pdt_page end of page
[ 2.216426] rmi4_physical rmi4-00: rmi_scan_pdt page 3
[ 2.216436] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 1001
[ 2.217431] rmi4_physical rmi4-00: rmi_read_pdt_entry: F55 V0
[ 2.217442] rmi4_physical rmi4-00: Initializing F55.
[ 2.224189] rmi4_f55 rmi4-00.fn55: F55 num_rx_electrodes: 48 (raw 60)
[ 2.224201] rmi4_f55 rmi4-00.fn55: F55 num_tx_electrodes: 30 (raw 36)
[ 2.224220] rmi4_physical rmi4-00: Registered F55.
[ 2.224231] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 995
[ 2.224889] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
[ 2.224900] rmi4_physical rmi4-00: rmi_scan_pdt_page end of page
[ 2.224911] rmi4_physical rmi4-00: rmi_scan_pdt page 4
[ 2.224921] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 1257
[ 2.225915] rmi4_physical rmi4-00: rmi_read_pdt_entry: F51 V1
[ 2.225927] rmi4_physical rmi4-00: Initializing F51.
[ 2.226005] rmi4_physical rmi4-00: Registered F51.
[ 2.226016] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 1251
[ 2.226677] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
[ 2.226689] rmi4_physical rmi4-00: rmi_scan_pdt_page end of page
[ 2.226699] rmi4_physical rmi4-00: rmi_scan_pdt page 5
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [-next, 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning
2016-10-20 22:51 ` Nick Dyer
@ 2016-10-20 23:28 ` Christopher Heiny
[not found] ` <CAFXsbZo5SDVZSBJL5MV4Y4GFDQC9UNaQLHaxEeWBRydBppif9Q@mail.gmail.com>
2016-10-21 22:03 ` Guenter Roeck
0 siblings, 2 replies; 12+ messages in thread
From: Christopher Heiny @ 2016-10-20 23:28 UTC (permalink / raw)
To: Nick Dyer, Guenter Roeck
Cc: Dmitry Torokhov, Chris Healy, Andrew Duggan,
Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, linux-input
On Thu, 2016-10-20 at 23:51 +0100, Nick Dyer wrote:
> On Mon, Oct 17, 2016 at 02:30:08PM -0700, Guenter Roeck wrote:
> >
> > On Fri, Sep 30, 2016 at 08:22:47PM -0700, Guenter Roeck wrote:
> > >
> > > Sensor tuning support is needed to determine the number of
> > > enabled
> > > tx and rx electrodes for use in F54 functions.
> > >
> > > The number of enabled electrodes is not identical to the total
> > > number
> > > of electrodes as reported with F55:Query0 and F55:Query1. It has
> > > to be
> > > calculated by analyzing F55:Ctrl1 (sensor receiver assignment)
> > > and
> > > F55:Ctrl2 (sensor transmitter assignment).
> > >
> > > Support for additional sensor tuning functions may be added
> > > later.
> > >
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >
> > Ping ... any comments on this patch and on
> > https://patchwork.kernel.org/patch/9359061/ ?
> >
> > Both patches now apply to mainline.
> >
> > Thanks,
> > Guenter
>
> Hi Guenter-
>
> I've reviewed and tested (on S7300 and S7813) both these patches now
> - you can add my sign-off.
>
> However, on the S7813 firmware, F55 is on PDT page 3, and nothing
> on page 2, so the default behaviour of the mainline driver means it
> is
> not initialised.
>
> So I think we need to revert this change in mainline:
> https://patchwork.kernel.org/patch/3796971/
>
> See below the PDT scan with it reverted and some debug added.
>
> Christopher/Andrew: is there a better heuristic than scanning all 255
> pages, given that some firmwares contain gaps?
It's difficult to say. It is against the RMI4 spec for there to be
gaps in the pages - you're supposed to be able to scan until you hit a
page with an empty PDT, and then stop.
Since F55 is hardcoded to page 3 for this firmware, it may be a
customer specific deviation. This may have been done to accommodate a
customer-written driver that did not scan the PDT, but instead always
looked for F55 on page 3. This idea is supported by the existence of
the F51 custom function on page 4, since F51 almost always requires
customer driver code to handle it.
In my opinion, the Non-standard bit should have been set in the PDT to
indicate that special handling was required, but that wasn't done in
this case.
Anyway, given that this sort of thing has escaped into the wild, I'm
unsure what to advise. Just off the top of my head, one possibility is
to have a "keep-going" option to scan the first 128 pages (0x00 through
0x7F), regardless of whether an empty page is encountered. This could
be triggered either by a product ID on the "known goofy list", or by a
boot/load time flag. I'm sure there are other possibilities, though.
Cheers,
Chris
>
> cheers
>
> Nick
>
> [ 2.181199] rmi4_physical rmi4-00: Creating functions.
> [ 2.181210] rmi4_physical rmi4-00: rmi_scan_pdt page 0
> [ 2.181221] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 233
> [ 2.182218] rmi4_physical rmi4-00: rmi_read_pdt_entry: F34 V2
> [ 2.182230] rmi4_physical rmi4-00: Initializing F34.
> [ 2.182325] rmi4_physical rmi4-00: Registered F34.
> [ 2.182337] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 227
> [ 2.183003] rmi4_physical rmi4-00: rmi_read_pdt_entry: F01 V0
> [ 2.183014] rmi4_physical rmi4-00: Initializing F01.
> [ 2.187358] rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer:
> Synaptics, product: s7813, fw id: 2174259
> [ 2.198822] rmi4_physical rmi4-00: Registered F01.
> [ 2.198834] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 221
> [ 2.199494] rmi4_physical rmi4-00: rmi_read_pdt_entry: F12 V0
> [ 2.199505] rmi4_physical rmi4-00: Initializing F12.
> [ 2.199612] rmi4_f12 rmi4-00.fn12: rmi_f12_probe
> [ 2.210721] rmi4_physical rmi4-00: Registered F12.
> [ 2.210732] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 215
> [ 2.211393] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
> [ 2.211404] rmi4_physical rmi4-00: rmi_scan_pdt_page end of page
> [ 2.211414] rmi4_physical rmi4-00: rmi_scan_pdt page 1
> [ 2.211424] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 489
> [ 2.212419] rmi4_physical rmi4-00: rmi_read_pdt_entry: F54 V0
> [ 2.212431] rmi4_physical rmi4-00: Initializing F54.
> [ 2.214241] rmi4_f54 rmi4-00.fn54: F54 num_rx_electrodes: 60
> [ 2.214253] rmi4_f54 rmi4-00.fn54: F54 num_tx_electrodes: 36
> [ 2.214263] rmi4_f54 rmi4-00.fn54: F54 capabilities: 0x44
> [ 2.214274] rmi4_f54 rmi4-00.fn54: F54 clock rate: 0x5aa0
> [ 2.214283] rmi4_f54 rmi4-00.fn54: F54 family: 0x2
> [ 2.214695] rmi4_physical rmi4-00: Registered F54.
> [ 2.214708] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 483
> [ 2.215372] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
> [ 2.215384] rmi4_physical rmi4-00: rmi_scan_pdt_page end of page
> [ 2.215395] rmi4_physical rmi4-00: rmi_scan_pdt page 2
> [ 2.215405] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 745
> [ 2.216404] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
> [ 2.216415] rmi4_physical rmi4-00: rmi_scan_pdt_page end of page
> [ 2.216426] rmi4_physical rmi4-00: rmi_scan_pdt page 3
> [ 2.216436] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 1001
> [ 2.217431] rmi4_physical rmi4-00: rmi_read_pdt_entry: F55 V0
> [ 2.217442] rmi4_physical rmi4-00: Initializing F55.
> [ 2.224189] rmi4_f55 rmi4-00.fn55: F55 num_rx_electrodes: 48 (raw
> 60)
> [ 2.224201] rmi4_f55 rmi4-00.fn55: F55 num_tx_electrodes: 30 (raw
> 36)
> [ 2.224220] rmi4_physical rmi4-00: Registered F55.
> [ 2.224231] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 995
> [ 2.224889] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
> [ 2.224900] rmi4_physical rmi4-00: rmi_scan_pdt_page end of page
> [ 2.224911] rmi4_physical rmi4-00: rmi_scan_pdt page 4
> [ 2.224921] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 1257
> [ 2.225915] rmi4_physical rmi4-00: rmi_read_pdt_entry: F51 V1
> [ 2.225927] rmi4_physical rmi4-00: Initializing F51.
> [ 2.226005] rmi4_physical rmi4-00: Registered F51.
> [ 2.226016] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 1251
> [ 2.226677] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
> [ 2.226689] rmi4_physical rmi4-00: rmi_scan_pdt_page end of page
> [ 2.226699] rmi4_physical rmi4-00: rmi_scan_pdt page 5
^ permalink raw reply [flat|nested] 12+ messages in thread[parent not found: <CAFXsbZo5SDVZSBJL5MV4Y4GFDQC9UNaQLHaxEeWBRydBppif9Q@mail.gmail.com>]
* Re: [-next, 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning
[not found] ` <CAFXsbZo5SDVZSBJL5MV4Y4GFDQC9UNaQLHaxEeWBRydBppif9Q@mail.gmail.com>
@ 2016-10-21 18:25 ` Christopher Heiny
0 siblings, 0 replies; 12+ messages in thread
From: Christopher Heiny @ 2016-10-21 18:25 UTC (permalink / raw)
To: Chris Healy
Cc: Nick Dyer, Guenter Roeck, Dmitry Torokhov, Andrew Duggan,
Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, linux-input
On Thu, 2016-10-20 at 16:31 -0700, Chris Healy wrote:
> As a little background, in the case Nick is referring to with the
> S7813, it was actually with a stock Synaptics Eval Board and stock
> Firmware provided by Synaptics.
>
> Not sure if that is relevant or not.
Yes, it's quite relevant - thanks for the info!
I've checked with the firmware team and the QA team. This definitely
appears to be a f/w misconfiguration that escaped the QA process. QA
is updating their flow to check for this, and FW is looking into how it
happened.
In addition to the workaround I mentioned earlier, another possibility
would be to produce a fixed, spec-compliant firmware image and flash it
onto your S7813. However, that doesn't eliminate the issue that there
might have been other QA escapes that would need to be handled (should
they arise).
Chris
>
> On Thu, Oct 20, 2016 at 4:28 PM, Christopher Heiny <cheiny@synaptics.
> com> wrote:
> > On Thu, 2016-10-20 at 23:51 +0100, Nick Dyer wrote:
> > > On Mon, Oct 17, 2016 at 02:30:08PM -0700, Guenter Roeck wrote:
> > > >
> > > > On Fri, Sep 30, 2016 at 08:22:47PM -0700, Guenter Roeck wrote:
> > > > >
> > > > > Sensor tuning support is needed to determine the number of
> > > > > enabled
> > > > > tx and rx electrodes for use in F54 functions.
> > > > >
> > > > > The number of enabled electrodes is not identical to the
> > total
> > > > > number
> > > > > of electrodes as reported with F55:Query0 and F55:Query1. It
> > has
> > > > > to be
> > > > > calculated by analyzing F55:Ctrl1 (sensor receiver
> > assignment)
> > > > > and
> > > > > F55:Ctrl2 (sensor transmitter assignment).
> > > > >
> > > > > Support for additional sensor tuning functions may be added
> > > > > later.
> > > > >
> > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > >
> > > > Ping ... any comments on this patch and on
> > > > https://patchwork.kernel.org/patch/9359061/ ?
> > > >
> > > > Both patches now apply to mainline.
> > > >
> > > > Thanks,
> > > > Guenter
> > >
> > > Hi Guenter-
> > >
> > > I've reviewed and tested (on S7300 and S7813) both these patches
> > now
> > > - you can add my sign-off.
> > >
> > > However, on the S7813 firmware, F55 is on PDT page 3, and nothing
> > > on page 2, so the default behaviour of the mainline driver means
> > it
> > > is
> > > not initialised.
> > >
> > > So I think we need to revert this change in mainline:
> > > https://patchwork.kernel.org/patch/3796971/
> > >
> > > See below the PDT scan with it reverted and some debug added.
> > >
> > > Christopher/Andrew: is there a better heuristic than scanning all
> > 255
> > > pages, given that some firmwares contain gaps?
> >
> > It's difficult to say. It is against the RMI4 spec for there to be
> > gaps in the pages - you're supposed to be able to scan until you
> > hit a
> > page with an empty PDT, and then stop.
> >
> > Since F55 is hardcoded to page 3 for this firmware, it may be a
> > customer specific deviation. This may have been done to
> > accommodate a
> > customer-written driver that did not scan the PDT, but instead
> > always
> > looked for F55 on page 3. This idea is supported by the existence
> > of
> > the F51 custom function on page 4, since F51 almost always requires
> > customer driver code to handle it.
> >
> > In my opinion, the Non-standard bit should have been set in the PDT
> > to
> > indicate that special handling was required, but that wasn't done
> > in
> > this case.
> >
> > Anyway, given that this sort of thing has escaped into the wild,
> > I'm
> > unsure what to advise. Just off the top of my head, one
> > possibility is
> > to have a "keep-going" option to scan the first 128 pages (0x00
> > through
> > 0x7F), regardless of whether an empty page is encountered. This
> > could
> > be triggered either by a product ID on the "known goofy list", or
> > by a
> > boot/load time flag. I'm sure there are other possibilities,
> > though.
> >
> > Cheers,
> > Chris
> >
> >
> > >
> > > cheers
> > >
> > > Nick
> > >
> > > [ 2.181199] rmi4_physical rmi4-00: Creating functions.
> > > [ 2.181210] rmi4_physical rmi4-00: rmi_scan_pdt page 0
> > > [ 2.181221] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 233
> > > [ 2.182218] rmi4_physical rmi4-00: rmi_read_pdt_entry: F34 V2
> > > [ 2.182230] rmi4_physical rmi4-00: Initializing F34.
> > > [ 2.182325] rmi4_physical rmi4-00: Registered F34.
> > > [ 2.182337] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 227
> > > [ 2.183003] rmi4_physical rmi4-00: rmi_read_pdt_entry: F01 V0
> > > [ 2.183014] rmi4_physical rmi4-00: Initializing F01.
> > > [ 2.187358] rmi4_f01 rmi4-00.fn01: found RMI device,
> > manufacturer:
> > > Synaptics, product: s7813, fw id: 2174259
> > > [ 2.198822] rmi4_physical rmi4-00: Registered F01.
> > > [ 2.198834] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 221
> > > [ 2.199494] rmi4_physical rmi4-00: rmi_read_pdt_entry: F12 V0
> > > [ 2.199505] rmi4_physical rmi4-00: Initializing F12.
> > > [ 2.199612] rmi4_f12 rmi4-00.fn12: rmi_f12_probe
> > > [ 2.210721] rmi4_physical rmi4-00: Registered F12.
> > > [ 2.210732] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 215
> > > [ 2.211393] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
> > > [ 2.211404] rmi4_physical rmi4-00: rmi_scan_pdt_page end of
> > page
> > > [ 2.211414] rmi4_physical rmi4-00: rmi_scan_pdt page 1
> > > [ 2.211424] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 489
> > > [ 2.212419] rmi4_physical rmi4-00: rmi_read_pdt_entry: F54 V0
> > > [ 2.212431] rmi4_physical rmi4-00: Initializing F54.
> > > [ 2.214241] rmi4_f54 rmi4-00.fn54: F54 num_rx_electrodes: 60
> > > [ 2.214253] rmi4_f54 rmi4-00.fn54: F54 num_tx_electrodes: 36
> > > [ 2.214263] rmi4_f54 rmi4-00.fn54: F54 capabilities: 0x44
> > > [ 2.214274] rmi4_f54 rmi4-00.fn54: F54 clock rate: 0x5aa0
> > > [ 2.214283] rmi4_f54 rmi4-00.fn54: F54 family: 0x2
> > > [ 2.214695] rmi4_physical rmi4-00: Registered F54.
> > > [ 2.214708] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 483
> > > [ 2.215372] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
> > > [ 2.215384] rmi4_physical rmi4-00: rmi_scan_pdt_page end of
> > page
> > > [ 2.215395] rmi4_physical rmi4-00: rmi_scan_pdt page 2
> > > [ 2.215405] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 745
> > > [ 2.216404] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
> > > [ 2.216415] rmi4_physical rmi4-00: rmi_scan_pdt_page end of
> > page
> > > [ 2.216426] rmi4_physical rmi4-00: rmi_scan_pdt page 3
> > > [ 2.216436] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 1001
> > > [ 2.217431] rmi4_physical rmi4-00: rmi_read_pdt_entry: F55 V0
> > > [ 2.217442] rmi4_physical rmi4-00: Initializing F55.
> > > [ 2.224189] rmi4_f55 rmi4-00.fn55: F55 num_rx_electrodes: 48
> > (raw
> > > 60)
> > > [ 2.224201] rmi4_f55 rmi4-00.fn55: F55 num_tx_electrodes: 30
> > (raw
> > > 36)
> > > [ 2.224220] rmi4_physical rmi4-00: Registered F55.
> > > [ 2.224231] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 995
> > > [ 2.224889] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
> > > [ 2.224900] rmi4_physical rmi4-00: rmi_scan_pdt_page end of
> > page
> > > [ 2.224911] rmi4_physical rmi4-00: rmi_scan_pdt page 4
> > > [ 2.224921] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 1257
> > > [ 2.225915] rmi4_physical rmi4-00: rmi_read_pdt_entry: F51 V1
> > > [ 2.225927] rmi4_physical rmi4-00: Initializing F51.
> > > [ 2.226005] rmi4_physical rmi4-00: Registered F51.
> > > [ 2.226016] rmi4_physical rmi4-00: rmi_scan_pdt_page addr 1251
> > > [ 2.226677] rmi4_physical rmi4-00: rmi_read_pdt_entry: F00 V0
> > > [ 2.226689] rmi4_physical rmi4-00: rmi_scan_pdt_page end of
> > page
> > > [ 2.226699] rmi4_physical rmi4-00: rmi_scan_pdt page 5
> >
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [-next, 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning
2016-10-20 23:28 ` Christopher Heiny
[not found] ` <CAFXsbZo5SDVZSBJL5MV4Y4GFDQC9UNaQLHaxEeWBRydBppif9Q@mail.gmail.com>
@ 2016-10-21 22:03 ` Guenter Roeck
1 sibling, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2016-10-21 22:03 UTC (permalink / raw)
To: Christopher Heiny
Cc: Nick Dyer, Dmitry Torokhov, Chris Healy, Andrew Duggan,
Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, linux-input
On Thu, Oct 20, 2016 at 04:28:39PM -0700, Christopher Heiny wrote:
> On Thu, 2016-10-20 at 23:51 +0100, Nick Dyer wrote:
> > On Mon, Oct 17, 2016 at 02:30:08PM -0700, Guenter Roeck wrote:
> > >
> > > On Fri, Sep 30, 2016 at 08:22:47PM -0700, Guenter Roeck wrote:
> > > >
> > > > Sensor tuning support is needed to determine the number of
> > > > enabled
> > > > tx and rx electrodes for use in F54 functions.
> > > >
> > > > The number of enabled electrodes is not identical to the total
> > > > number
> > > > of electrodes as reported with F55:Query0 and F55:Query1. It has
> > > > to be
> > > > calculated by analyzing F55:Ctrl1 (sensor receiver assignment)
> > > > and
> > > > F55:Ctrl2 (sensor transmitter assignment).
> > > >
> > > > Support for additional sensor tuning functions may be added
> > > > later.
> > > >
> > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > >
> > > Ping ... any comments on this patch and on
> > > https://patchwork.kernel.org/patch/9359061/ ?
> > >
> > > Both patches now apply to mainline.
> > >
> > > Thanks,
> > > Guenter
> >
> > Hi Guenter-
> >
> > I've reviewed and tested (on S7300 and S7813) both these patches now
> > - you can add my sign-off.
> >
> > However, on the S7813 firmware, F55 is on PDT page 3, and nothing
> > on page 2, so the default behaviour of the mainline driver means it
> > is
> > not initialised.
> >
> > So I think we need to revert this change in mainline:
> > https://patchwork.kernel.org/patch/3796971/
> >
> > See below the PDT scan with it reverted and some debug added.
> >
> > Christopher/Andrew: is there a better heuristic than scanning all 255
> > pages, given that some firmwares contain gaps?
>
> It's difficult to say. It is against the RMI4 spec for there to be
> gaps in the pages - you're supposed to be able to scan until you hit a
> page with an empty PDT, and then stop.
>
> Since F55 is hardcoded to page 3 for this firmware, it may be a
> customer specific deviation. This may have been done to accommodate a
> customer-written driver that did not scan the PDT, but instead always
> looked for F55 on page 3. This idea is supported by the existence of
> the F51 custom function on page 4, since F51 almost always requires
> customer driver code to handle it.
>
> In my opinion, the Non-standard bit should have been set in the PDT to
> indicate that special handling was required, but that wasn't done in
> this case.
>
> Anyway, given that this sort of thing has escaped into the wild, I'm
> unsure what to advise. Just off the top of my head, one possibility is
> to have a "keep-going" option to scan the first 128 pages (0x00 through
> 0x7F), regardless of whether an empty page is encountered. This could
> be triggered either by a product ID on the "known goofy list", or by a
> boot/load time flag. I'm sure there are other possibilities, though.
>
Maybe introduce quirks if the problematic device and/or problematic firmware
version can be identified ? Not sure if scanning 128 pages would be necessary,
though - requiring two empty pages to stop scanning might be sufficient.
Guenter
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -next 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning
2016-10-01 3:22 [PATCH -next 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning Guenter Roeck
2016-10-01 3:22 ` [PATCH -next 2/2] Input: synaptics-rmi4 - Propagate correct number of rx and tx electrodes to F54 Guenter Roeck
2016-10-17 21:30 ` [-next, 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning Guenter Roeck
@ 2016-10-25 0:59 ` Andrew Duggan
2016-10-25 3:13 ` Guenter Roeck
2 siblings, 1 reply; 12+ messages in thread
From: Andrew Duggan @ 2016-10-25 0:59 UTC (permalink / raw)
To: Guenter Roeck, Dmitry Torokhov
Cc: Chris Healy, Nick Dyer, Mauro Carvalho Chehab, Hans Verkuil,
linux-kernel, linux-input
Hi Guenter,
I have a couple of comments below.
On 09/30/2016 08:22 PM, Guenter Roeck wrote:
> Sensor tuning support is needed to determine the number of enabled
> tx and rx electrodes for use in F54 functions.
>
> The number of enabled electrodes is not identical to the total number
> of electrodes as reported with F55:Query0 and F55:Query1. It has to be
> calculated by analyzing F55:Ctrl1 (sensor receiver assignment) and
> F55:Ctrl2 (sensor transmitter assignment).
>
> Support for additional sensor tuning functions may be added later.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> This patch applies to next-20160930.
>
> 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_f55.c | 127 ++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 141 insertions(+)
> create mode 100644 drivers/input/rmi4/rmi_f55.c
>
> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
> index 4c8a55857e00..11ede43c9936 100644
> --- a/drivers/input/rmi4/Kconfig
> +++ b/drivers/input/rmi4/Kconfig
> @@ -72,3 +72,12 @@ config RMI4_F54
>
> Function 54 provides access to various diagnostic features in certain
> RMI4 touch sensors.
> +
> +config RMI4_F55
> + bool "RMI4 Function 55 (Sensor tuning)"
> + depends on RMI4_CORE
> + help
> + Say Y here if you want to add support for RMI4 function 55
> +
> + Function 55 provides access to the RMI4 touch sensor tuning
> + mechanism.
> diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
> index 0bafc8502c4b..96f8e0c21e3b 100644
> --- a/drivers/input/rmi4/Makefile
> +++ b/drivers/input/rmi4/Makefile
> @@ -8,6 +8,7 @@ rmi_core-$(CONFIG_RMI4_F11) += rmi_f11.o
> rmi_core-$(CONFIG_RMI4_F12) += rmi_f12.o
> rmi_core-$(CONFIG_RMI4_F30) += rmi_f30.o
> rmi_core-$(CONFIG_RMI4_F54) += rmi_f54.o
> +rmi_core-$(CONFIG_RMI4_F55) += rmi_f55.o
>
> # Transports
> obj-$(CONFIG_RMI4_I2C) += rmi_i2c.o
> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> index ef8c747c35e7..82b7d4960858 100644
> --- a/drivers/input/rmi4/rmi_bus.c
> +++ b/drivers/input/rmi4/rmi_bus.c
> @@ -314,6 +314,9 @@ static struct rmi_function_handler *fn_handlers[] = {
> #ifdef CONFIG_RMI4_F54
> &rmi_f54_handler,
> #endif
> +#ifdef CONFIG_RMI4_F55
> + &rmi_f55_handler,
> +#endif
> };
>
> static void __rmi_unregister_function_handlers(int start_idx)
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> index 8dfbebe9bf86..a65cf70f61e2 100644
> --- a/drivers/input/rmi4/rmi_driver.h
> +++ b/drivers/input/rmi4/rmi_driver.h
> @@ -103,4 +103,5 @@ extern struct rmi_function_handler rmi_f11_handler;
> extern struct rmi_function_handler rmi_f12_handler;
> extern struct rmi_function_handler rmi_f30_handler;
> extern struct rmi_function_handler rmi_f54_handler;
> +extern struct rmi_function_handler rmi_f55_handler;
> #endif
> diff --git a/drivers/input/rmi4/rmi_f55.c b/drivers/input/rmi4/rmi_f55.c
> new file mode 100644
> index 000000000000..268fa904205a
> --- /dev/null
> +++ b/drivers/input/rmi4/rmi_f55.c
> @@ -0,0 +1,127 @@
> +/*
> + * Copyright (c) 2012-2015 Synaptics Incorporated
> + * Copyright (C) 2016 Zodiac Inflight Innovations
> + *
> + * 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/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
This is incidental, but I don't think i2c.h needs to be included here
since this file shouldn't contain anything i2c specific. Its not that
big a deal, but I noticed it so I thought I would mention it.
> +#include <linux/input.h>
> +#include <linux/kernel.h>
> +#include <linux/rmi.h>
> +#include <linux/slab.h>
> +#include "rmi_driver.h"
> +
> +#define F55_NAME "rmi4_f55"
> +
> +/* F55 data offsets */
> +#define F55_NUM_RX_OFFSET 0
> +#define F55_NUM_TX_OFFSET 1
> +#define F55_PHYS_CHAR_OFFSET 2
> +
> +/* Fixed sizes of reports */
> +#define F55_QUERY_LEN 17
How did you chose the number 17? The number of F55 query registers
present will depend on how the firmware is configured so the total
length of query registers can change. Right now this driver is only
using the first three F55 query registers which will always be present
so that not an issue. But, beyond query 2 not all query registers are
guaranteed to be present.
Everything else looks correct.
Andrew
> +
> +/* F55 capabilities */
> +#define F55_CAP_SENSOR_ASSIGN BIT(0)
> +
> +struct f55_data {
> + struct rmi_function *fn;
> +
> + u8 qry[F55_QUERY_LEN];
> + u8 num_rx_electrodes;
> + u8 cfg_num_rx_electrodes;
> + u8 num_tx_electrodes;
> + u8 cfg_num_tx_electrodes;
> +};
> +
> +static int rmi_f55_detect(struct rmi_function *fn)
> +{
> + struct f55_data *f55;
> + int error;
> +
> + f55 = dev_get_drvdata(&fn->dev);
> +
> + error = rmi_read_block(fn->rmi_dev, fn->fd.query_base_addr,
> + &f55->qry, sizeof(f55->qry));
> + if (error) {
> + dev_err(&fn->dev, "%s: Failed to query F55 properties\n",
> + __func__);
> + return error;
> + }
> +
> + f55->num_rx_electrodes = f55->qry[F55_NUM_RX_OFFSET];
> + f55->num_tx_electrodes = f55->qry[F55_NUM_TX_OFFSET];
> +
> + f55->cfg_num_rx_electrodes = f55->num_rx_electrodes;
> + f55->cfg_num_tx_electrodes = f55->num_rx_electrodes;
> +
> + if (f55->qry[F55_PHYS_CHAR_OFFSET] & F55_CAP_SENSOR_ASSIGN) {
> + int i, total;
> + u8 buf[256];
> +
> + /*
> + * Calculate the number of enabled receive and transmit
> + * electrodes by reading F55:Ctrl1 (sensor receiver assignment)
> + * and F55:Ctrl2 (sensor transmitter assignment). The number of
> + * enabled electrodes is the sum of all field entries with a
> + * value other than 0xff.
> + */
> + error = rmi_read_block(fn->rmi_dev,
> + fn->fd.control_base_addr + 1,
> + buf, f55->num_rx_electrodes);
> + if (!error) {
> + total = 0;
> + for (i = 0; i < f55->num_rx_electrodes; i++) {
> + if (buf[i] != 0xff)
> + total++;
> + }
> + f55->cfg_num_rx_electrodes = total;
> + }
> +
> + error = rmi_read_block(fn->rmi_dev,
> + fn->fd.control_base_addr + 2,
> + buf, f55->num_tx_electrodes);
> + if (!error) {
> + total = 0;
> + for (i = 0; i < f55->num_tx_electrodes; i++) {
> + if (buf[i] != 0xff)
> + total++;
> + }
> + f55->cfg_num_tx_electrodes = total;
> + }
> + }
> +
> + rmi_dbg(RMI_DEBUG_FN, &fn->dev, "F55 num_rx_electrodes: %d (raw %d)\n",
> + f55->cfg_num_rx_electrodes, f55->num_rx_electrodes);
> + rmi_dbg(RMI_DEBUG_FN, &fn->dev, "F55 num_tx_electrodes: %d (raw %d)\n",
> + f55->cfg_num_tx_electrodes, f55->num_tx_electrodes);
> +
> + return 0;
> +}
> +
> +static int rmi_f55_probe(struct rmi_function *fn)
> +{
> + struct f55_data *f55;
> +
> + f55 = devm_kzalloc(&fn->dev, sizeof(struct f55_data), GFP_KERNEL);
> + if (!f55)
> + return -ENOMEM;
> +
> + f55->fn = fn;
> + dev_set_drvdata(&fn->dev, f55);
> +
> + return rmi_f55_detect(fn);
> +}
> +
> +struct rmi_function_handler rmi_f55_handler = {
> + .driver = {
> + .name = F55_NAME,
> + },
> + .func = 0x55,
> + .probe = rmi_f55_probe,
> +};
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH -next 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning
2016-10-25 0:59 ` [PATCH -next " Andrew Duggan
@ 2016-10-25 3:13 ` Guenter Roeck
2016-10-25 18:26 ` Andrew Duggan
0 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2016-10-25 3:13 UTC (permalink / raw)
To: Andrew Duggan, Dmitry Torokhov
Cc: Chris Healy, Nick Dyer, Mauro Carvalho Chehab, Hans Verkuil,
linux-kernel, linux-input
Hi Andrew,
On 10/24/2016 05:59 PM, Andrew Duggan wrote:
> Hi Guenter,
>
> I have a couple of comments below.
>
Thanks a lot for the feedback.
> On 09/30/2016 08:22 PM, Guenter Roeck wrote:
>> Sensor tuning support is needed to determine the number of enabled
>> tx and rx electrodes for use in F54 functions.
>>
>> The number of enabled electrodes is not identical to the total number
>> of electrodes as reported with F55:Query0 and F55:Query1. It has to be
>> calculated by analyzing F55:Ctrl1 (sensor receiver assignment) and
>> F55:Ctrl2 (sensor transmitter assignment).
>>
>> Support for additional sensor tuning functions may be added later.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> This patch applies to next-20160930.
>>
>> 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_f55.c | 127 ++++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 141 insertions(+)
>> create mode 100644 drivers/input/rmi4/rmi_f55.c
>>
>> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
>> index 4c8a55857e00..11ede43c9936 100644
>> --- a/drivers/input/rmi4/Kconfig
>> +++ b/drivers/input/rmi4/Kconfig
>> @@ -72,3 +72,12 @@ config RMI4_F54
>> Function 54 provides access to various diagnostic features in certain
>> RMI4 touch sensors.
>> +
>> +config RMI4_F55
>> + bool "RMI4 Function 55 (Sensor tuning)"
>> + depends on RMI4_CORE
>> + help
>> + Say Y here if you want to add support for RMI4 function 55
>> +
>> + Function 55 provides access to the RMI4 touch sensor tuning
>> + mechanism.
>> diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
>> index 0bafc8502c4b..96f8e0c21e3b 100644
>> --- a/drivers/input/rmi4/Makefile
>> +++ b/drivers/input/rmi4/Makefile
>> @@ -8,6 +8,7 @@ rmi_core-$(CONFIG_RMI4_F11) += rmi_f11.o
>> rmi_core-$(CONFIG_RMI4_F12) += rmi_f12.o
>> rmi_core-$(CONFIG_RMI4_F30) += rmi_f30.o
>> rmi_core-$(CONFIG_RMI4_F54) += rmi_f54.o
>> +rmi_core-$(CONFIG_RMI4_F55) += rmi_f55.o
>> # Transports
>> obj-$(CONFIG_RMI4_I2C) += rmi_i2c.o
>> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
>> index ef8c747c35e7..82b7d4960858 100644
>> --- a/drivers/input/rmi4/rmi_bus.c
>> +++ b/drivers/input/rmi4/rmi_bus.c
>> @@ -314,6 +314,9 @@ static struct rmi_function_handler *fn_handlers[] = {
>> #ifdef CONFIG_RMI4_F54
>> &rmi_f54_handler,
>> #endif
>> +#ifdef CONFIG_RMI4_F55
>> + &rmi_f55_handler,
>> +#endif
>> };
>> static void __rmi_unregister_function_handlers(int start_idx)
>> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
>> index 8dfbebe9bf86..a65cf70f61e2 100644
>> --- a/drivers/input/rmi4/rmi_driver.h
>> +++ b/drivers/input/rmi4/rmi_driver.h
>> @@ -103,4 +103,5 @@ extern struct rmi_function_handler rmi_f11_handler;
>> extern struct rmi_function_handler rmi_f12_handler;
>> extern struct rmi_function_handler rmi_f30_handler;
>> extern struct rmi_function_handler rmi_f54_handler;
>> +extern struct rmi_function_handler rmi_f55_handler;
>> #endif
>> diff --git a/drivers/input/rmi4/rmi_f55.c b/drivers/input/rmi4/rmi_f55.c
>> new file mode 100644
>> index 000000000000..268fa904205a
>> --- /dev/null
>> +++ b/drivers/input/rmi4/rmi_f55.c
>> @@ -0,0 +1,127 @@
>> +/*
>> + * Copyright (c) 2012-2015 Synaptics Incorporated
>> + * Copyright (C) 2016 Zodiac Inflight Innovations
>> + *
>> + * 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/bitops.h>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>
> This is incidental, but I don't think i2c.h needs to be included here since this file shouldn't contain anything i2c specific. Its not that big a deal, but I noticed it so I thought I would mention it.
>
Makes sense. delay.h and input.h seem to be unnecessary too.
I'll remove those if/when I resubmit.
>> +#include <linux/input.h>
>> +#include <linux/kernel.h>
>> +#include <linux/rmi.h>
>> +#include <linux/slab.h>
>> +#include "rmi_driver.h"
>> +
>> +#define F55_NAME "rmi4_f55"
>> +
>> +/* F55 data offsets */
>> +#define F55_NUM_RX_OFFSET 0
>> +#define F55_NUM_TX_OFFSET 1
>> +#define F55_PHYS_CHAR_OFFSET 2
>> +
>> +/* Fixed sizes of reports */
>> +#define F55_QUERY_LEN 17
>
> How did you chose the number 17? The number of F55 query registers present will depend on how the firmware is configured so the total length of query registers can change. Right now this driver is only using the first three F55 query registers which will always be present so that not an issue. But, beyond query 2 not all query registers are guaranteed to be present.
>
According to the information I have, the maximum size is 17.
Do you have a better idea on how to handle the dynamic length ? Or a better number ?
Should I only read the minimum ? Or the number we actually need (3) at this point ?
Or just name the define F55_QUERY_MAXLEN and change the comment to "maximum size
of report" ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH -next 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning
2016-10-25 3:13 ` Guenter Roeck
@ 2016-10-25 18:26 ` Andrew Duggan
2016-10-26 2:41 ` Guenter Roeck
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Duggan @ 2016-10-25 18:26 UTC (permalink / raw)
To: Guenter Roeck, Dmitry Torokhov
Cc: Chris Healy, Nick Dyer, Mauro Carvalho Chehab, Hans Verkuil,
linux-kernel, linux-input
On 10/24/2016 08:13 PM, Guenter Roeck wrote:
> Hi Andrew,
>
> On 10/24/2016 05:59 PM, Andrew Duggan wrote:
>> Hi Guenter,
>>
>> I have a couple of comments below.
>>
>
> Thanks a lot for the feedback.
>
>> On 09/30/2016 08:22 PM, Guenter Roeck wrote:
>>> Sensor tuning support is needed to determine the number of enabled
>>> tx and rx electrodes for use in F54 functions.
>>>
>>> The number of enabled electrodes is not identical to the total number
>>> of electrodes as reported with F55:Query0 and F55:Query1. It has to be
>>> calculated by analyzing F55:Ctrl1 (sensor receiver assignment) and
>>> F55:Ctrl2 (sensor transmitter assignment).
>>>
>>> Support for additional sensor tuning functions may be added later.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> This patch applies to next-20160930.
>>>
>>> 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_f55.c | 127
>>> ++++++++++++++++++++++++++++++++++++++++
>>> 5 files changed, 141 insertions(+)
>>> create mode 100644 drivers/input/rmi4/rmi_f55.c
>>>
>>> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
>>> index 4c8a55857e00..11ede43c9936 100644
>>> --- a/drivers/input/rmi4/Kconfig
>>> +++ b/drivers/input/rmi4/Kconfig
>>> @@ -72,3 +72,12 @@ config RMI4_F54
>>> Function 54 provides access to various diagnostic features
>>> in certain
>>> RMI4 touch sensors.
>>> +
>>> +config RMI4_F55
>>> + bool "RMI4 Function 55 (Sensor tuning)"
>>> + depends on RMI4_CORE
>>> + help
>>> + Say Y here if you want to add support for RMI4 function 55
>>> +
>>> + Function 55 provides access to the RMI4 touch sensor tuning
>>> + mechanism.
>>> diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
>>> index 0bafc8502c4b..96f8e0c21e3b 100644
>>> --- a/drivers/input/rmi4/Makefile
>>> +++ b/drivers/input/rmi4/Makefile
>>> @@ -8,6 +8,7 @@ rmi_core-$(CONFIG_RMI4_F11) += rmi_f11.o
>>> rmi_core-$(CONFIG_RMI4_F12) += rmi_f12.o
>>> rmi_core-$(CONFIG_RMI4_F30) += rmi_f30.o
>>> rmi_core-$(CONFIG_RMI4_F54) += rmi_f54.o
>>> +rmi_core-$(CONFIG_RMI4_F55) += rmi_f55.o
>>> # Transports
>>> obj-$(CONFIG_RMI4_I2C) += rmi_i2c.o
>>> diff --git a/drivers/input/rmi4/rmi_bus.c
>>> b/drivers/input/rmi4/rmi_bus.c
>>> index ef8c747c35e7..82b7d4960858 100644
>>> --- a/drivers/input/rmi4/rmi_bus.c
>>> +++ b/drivers/input/rmi4/rmi_bus.c
>>> @@ -314,6 +314,9 @@ static struct rmi_function_handler
>>> *fn_handlers[] = {
>>> #ifdef CONFIG_RMI4_F54
>>> &rmi_f54_handler,
>>> #endif
>>> +#ifdef CONFIG_RMI4_F55
>>> + &rmi_f55_handler,
>>> +#endif
>>> };
>>> static void __rmi_unregister_function_handlers(int start_idx)
>>> diff --git a/drivers/input/rmi4/rmi_driver.h
>>> b/drivers/input/rmi4/rmi_driver.h
>>> index 8dfbebe9bf86..a65cf70f61e2 100644
>>> --- a/drivers/input/rmi4/rmi_driver.h
>>> +++ b/drivers/input/rmi4/rmi_driver.h
>>> @@ -103,4 +103,5 @@ extern struct rmi_function_handler rmi_f11_handler;
>>> extern struct rmi_function_handler rmi_f12_handler;
>>> extern struct rmi_function_handler rmi_f30_handler;
>>> extern struct rmi_function_handler rmi_f54_handler;
>>> +extern struct rmi_function_handler rmi_f55_handler;
>>> #endif
>>> diff --git a/drivers/input/rmi4/rmi_f55.c
>>> b/drivers/input/rmi4/rmi_f55.c
>>> new file mode 100644
>>> index 000000000000..268fa904205a
>>> --- /dev/null
>>> +++ b/drivers/input/rmi4/rmi_f55.c
>>> @@ -0,0 +1,127 @@
>>> +/*
>>> + * Copyright (c) 2012-2015 Synaptics Incorporated
>>> + * Copyright (C) 2016 Zodiac Inflight Innovations
>>> + *
>>> + * 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/bitops.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/i2c.h>
>>
>> This is incidental, but I don't think i2c.h needs to be included here
>> since this file shouldn't contain anything i2c specific. Its not that
>> big a deal, but I noticed it so I thought I would mention it.
>>
>
> Makes sense. delay.h and input.h seem to be unnecessary too.
> I'll remove those if/when I resubmit.
>
>>> +#include <linux/input.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/rmi.h>
>>> +#include <linux/slab.h>
>>> +#include "rmi_driver.h"
>>> +
>>> +#define F55_NAME "rmi4_f55"
>>> +
>>> +/* F55 data offsets */
>>> +#define F55_NUM_RX_OFFSET 0
>>> +#define F55_NUM_TX_OFFSET 1
>>> +#define F55_PHYS_CHAR_OFFSET 2
>>> +
>>> +/* Fixed sizes of reports */
>>> +#define F55_QUERY_LEN 17
>>
>> How did you chose the number 17? The number of F55 query registers
>> present will depend on how the firmware is configured so the total
>> length of query registers can change. Right now this driver is only
>> using the first three F55 query registers which will always be
>> present so that not an issue. But, beyond query 2 not all query
>> registers are guaranteed to be present.
>>
>
> According to the information I have, the maximum size is 17.
>
> Do you have a better idea on how to handle the dynamic length ? Or a
> better number ?
> Should I only read the minimum ? Or the number we actually need (3) at
> this point ?
> Or just name the define F55_QUERY_MAXLEN and change the comment to
> "maximum size
> of report" ?
>
I would just read the three registers which you are using. Those
registers will be present in F55 so it is safe to just use them.
Beyond F55 Query2 is where registers may or not may not be present.
If in the future we need query registers beyond Query2 then we would
probably do something similar to what we do for F11 in
rmi_f11_get_query_parameters().
Andrew
> Thanks,
> Guenter
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH -next 1/2] Input: synaptics-rmi4 - add support for F55 sensor tuning
2016-10-25 18:26 ` Andrew Duggan
@ 2016-10-26 2:41 ` Guenter Roeck
0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2016-10-26 2:41 UTC (permalink / raw)
To: Andrew Duggan, Dmitry Torokhov
Cc: Chris Healy, Nick Dyer, Mauro Carvalho Chehab, Hans Verkuil,
linux-kernel, linux-input
On 10/25/2016 11:26 AM, Andrew Duggan wrote:
> On 10/24/2016 08:13 PM, Guenter Roeck wrote:
>> Hi Andrew,
>>
>> On 10/24/2016 05:59 PM, Andrew Duggan wrote:
>>> Hi Guenter,
>>>
>>> I have a couple of comments below.
>>>
>>
>> Thanks a lot for the feedback.
>>
>>> On 09/30/2016 08:22 PM, Guenter Roeck wrote:
>>>> Sensor tuning support is needed to determine the number of enabled
>>>> tx and rx electrodes for use in F54 functions.
>>>>
>>>> The number of enabled electrodes is not identical to the total number
>>>> of electrodes as reported with F55:Query0 and F55:Query1. It has to be
>>>> calculated by analyzing F55:Ctrl1 (sensor receiver assignment) and
>>>> F55:Ctrl2 (sensor transmitter assignment).
>>>>
>>>> Support for additional sensor tuning functions may be added later.
>>>>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>> This patch applies to next-20160930.
>>>>
>>>> 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_f55.c | 127 ++++++++++++++++++++++++++++++++++++++++
>>>> 5 files changed, 141 insertions(+)
>>>> create mode 100644 drivers/input/rmi4/rmi_f55.c
>>>>
>>>> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
>>>> index 4c8a55857e00..11ede43c9936 100644
>>>> --- a/drivers/input/rmi4/Kconfig
>>>> +++ b/drivers/input/rmi4/Kconfig
>>>> @@ -72,3 +72,12 @@ config RMI4_F54
>>>> Function 54 provides access to various diagnostic features in certain
>>>> RMI4 touch sensors.
>>>> +
>>>> +config RMI4_F55
>>>> + bool "RMI4 Function 55 (Sensor tuning)"
>>>> + depends on RMI4_CORE
>>>> + help
>>>> + Say Y here if you want to add support for RMI4 function 55
>>>> +
>>>> + Function 55 provides access to the RMI4 touch sensor tuning
>>>> + mechanism.
>>>> diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
>>>> index 0bafc8502c4b..96f8e0c21e3b 100644
>>>> --- a/drivers/input/rmi4/Makefile
>>>> +++ b/drivers/input/rmi4/Makefile
>>>> @@ -8,6 +8,7 @@ rmi_core-$(CONFIG_RMI4_F11) += rmi_f11.o
>>>> rmi_core-$(CONFIG_RMI4_F12) += rmi_f12.o
>>>> rmi_core-$(CONFIG_RMI4_F30) += rmi_f30.o
>>>> rmi_core-$(CONFIG_RMI4_F54) += rmi_f54.o
>>>> +rmi_core-$(CONFIG_RMI4_F55) += rmi_f55.o
>>>> # Transports
>>>> obj-$(CONFIG_RMI4_I2C) += rmi_i2c.o
>>>> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
>>>> index ef8c747c35e7..82b7d4960858 100644
>>>> --- a/drivers/input/rmi4/rmi_bus.c
>>>> +++ b/drivers/input/rmi4/rmi_bus.c
>>>> @@ -314,6 +314,9 @@ static struct rmi_function_handler *fn_handlers[] = {
>>>> #ifdef CONFIG_RMI4_F54
>>>> &rmi_f54_handler,
>>>> #endif
>>>> +#ifdef CONFIG_RMI4_F55
>>>> + &rmi_f55_handler,
>>>> +#endif
>>>> };
>>>> static void __rmi_unregister_function_handlers(int start_idx)
>>>> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
>>>> index 8dfbebe9bf86..a65cf70f61e2 100644
>>>> --- a/drivers/input/rmi4/rmi_driver.h
>>>> +++ b/drivers/input/rmi4/rmi_driver.h
>>>> @@ -103,4 +103,5 @@ extern struct rmi_function_handler rmi_f11_handler;
>>>> extern struct rmi_function_handler rmi_f12_handler;
>>>> extern struct rmi_function_handler rmi_f30_handler;
>>>> extern struct rmi_function_handler rmi_f54_handler;
>>>> +extern struct rmi_function_handler rmi_f55_handler;
>>>> #endif
>>>> diff --git a/drivers/input/rmi4/rmi_f55.c b/drivers/input/rmi4/rmi_f55.c
>>>> new file mode 100644
>>>> index 000000000000..268fa904205a
>>>> --- /dev/null
>>>> +++ b/drivers/input/rmi4/rmi_f55.c
>>>> @@ -0,0 +1,127 @@
>>>> +/*
>>>> + * Copyright (c) 2012-2015 Synaptics Incorporated
>>>> + * Copyright (C) 2016 Zodiac Inflight Innovations
>>>> + *
>>>> + * 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/bitops.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/i2c.h>
>>>
>>> This is incidental, but I don't think i2c.h needs to be included here since this file shouldn't contain anything i2c specific. Its not that big a deal, but I noticed it so I thought I would mention it.
>>>
>>
>> Makes sense. delay.h and input.h seem to be unnecessary too.
>> I'll remove those if/when I resubmit.
>>
>>>> +#include <linux/input.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/rmi.h>
>>>> +#include <linux/slab.h>
>>>> +#include "rmi_driver.h"
>>>> +
>>>> +#define F55_NAME "rmi4_f55"
>>>> +
>>>> +/* F55 data offsets */
>>>> +#define F55_NUM_RX_OFFSET 0
>>>> +#define F55_NUM_TX_OFFSET 1
>>>> +#define F55_PHYS_CHAR_OFFSET 2
>>>> +
>>>> +/* Fixed sizes of reports */
>>>> +#define F55_QUERY_LEN 17
>>>
>>> How did you chose the number 17? The number of F55 query registers present will depend on how the firmware is configured so the total length of query registers can change. Right now this driver is only using the first three F55 query registers which will always be present so that not an issue. But, beyond query 2 not all query registers are guaranteed to be present.
>>>
>>
>> According to the information I have, the maximum size is 17.
>>
>> Do you have a better idea on how to handle the dynamic length ? Or a better number ?
>> Should I only read the minimum ? Or the number we actually need (3) at this point ?
>> Or just name the define F55_QUERY_MAXLEN and change the comment to "maximum size
>> of report" ?
>>
>
> I would just read the three registers which you are using. Those registers will be present in F55 so it is safe to just use them.
> Beyond F55 Query2 is where registers may or not may not be present.
>
> If in the future we need query registers beyond Query2 then we would probably do something similar to what we do for F11 in rmi_f11_get_query_parameters().
>
ok, I'll do that.
Thanks,
Guenter
> Andrew
>
>> Thanks,
>> Guenter
>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread