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
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 <redacted>
Cc: Chris Healy <redacted>
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(-)
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>
From: Nick Dyer <nick@shmanahar.org> Date: 2016-10-20 22:51:16
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:
quoted
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>
From: Christopher Heiny <hidden> Date: 2016-10-20 23:38:17
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:
quoted
On Fri, Sep 30, 2016 at 08:22:47PM -0700, Guenter Roeck wrote:
quoted
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>
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
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:
quoted
On Mon, Oct 17, 2016 at 02:30:08PM -0700, Guenter Roeck wrote:
quoted
On Fri, Sep 30, 2016 at 08:22:47PM -0700, Guenter Roeck wrote:
quoted
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>
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
From: Andrew Duggan <hidden> Date: 2016-10-25 00:59:43
Hi Guenter,
I have a couple of comments below.
On 09/30/2016 08:22 PM, Guenter Roeck wrote:
quoted hunk
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
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.
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
quoted hunk
++/* 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,+};
From: Andrew Duggan <hidden> Date: 2016-10-25 01:00:00
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 <redacted>
Cc: Chris Healy <redacted>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Andrew Duggan <redacted>
quoted hunk
---
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(-)
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:
quoted
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
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.
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
From: Andrew Duggan <hidden> Date: 2016-10-25 18:26:24
On 10/24/2016 08:13 PM, Guenter Roeck wrote:
Hi Andrew,
On 10/24/2016 05:59 PM, Andrew Duggan wrote:
quoted
Hi Guenter,
I have a couple of comments below.
Thanks a lot for the feedback.
quoted
On 09/30/2016 08:22 PM, Guenter Roeck wrote:
quoted
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
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.
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
Hi Andrew,
On 10/24/2016 05:59 PM, Andrew Duggan wrote:
quoted
Hi Guenter,
I have a couple of comments below.
Thanks a lot for the feedback.
quoted
On 09/30/2016 08:22 PM, Guenter Roeck wrote:
quoted
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
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.
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().