| Message ID | 20250308-synaptics-rmi4-v3-2-215d3e7289a2@ixit.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Subject: [PATCH v3 0/7] Input: synaptics-rmi4: add quirks for third party touchscreen controllers | expand |
Hi David, On Sat, Mar 08, 2025 at 03:08:38PM +0100, David Heidelberg via B4 Relay wrote: > From: Caleb Connolly <caleb.connolly@linaro.org> > > Some third party rmi4-compatible ICs don't expose their PDT entries > very well. Add a few checks to skip duplicate entries as well as entries > for unsupported functions. > > This is required to support some phones with third party displays. > > Validated on a stock OnePlus 6T (original parts): > manufacturer: Synaptics, product: S3706B, fw id: 2852315 > > Co-developed-by: methanal <baclofen@tuta.io> > Signed-off-by: methanal <baclofen@tuta.io> > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > Signed-off-by: David Heidelberg <david@ixit.cz> > --- > drivers/input/rmi4/rmi_driver.c | 47 +++++++++++++++++++++++++++++++++++------ > drivers/input/rmi4/rmi_driver.h | 6 ++++++ > 2 files changed, 47 insertions(+), 6 deletions(-) > > diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c > index 2168b6cd7167334d44553c9c566f870a4e034179..51c23a407b2731d5b6eaefe9cae6288f97316e34 100644 > --- a/drivers/input/rmi4/rmi_driver.c > +++ b/drivers/input/rmi4/rmi_driver.c > @@ -493,12 +493,44 @@ static void rmi_driver_copy_pdt_to_fd(const struct pdt_entry *pdt, > fd->function_version = pdt->function_version; > } > > +static bool rmi_pdt_entry_is_valid(struct rmi_device *rmi_dev, > + struct pdt_scan_state *state, u8 fn) > +{ > + unsigned int i; > + > + switch (fn) { > + case 0x01: > + case 0x03: > + case 0x11: > + case 0x12: > + case 0x30: > + case 0x34: > + case 0x3a: > + case 0x54: > + case 0x55: This mean that we need to update this code any time there is new function introduced. I'd rather we did not do that. The driver should be able to handle unknown functions. > + break; > + > + default: > + rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev, > + "PDT has unknown function number %#02x\n", fn); > + return false; > + } > + > + for (i = 0; i < state->pdt_count; i++) { > + if (state->pdts[i] == fn) > + return false; > + } > + > + state->pdts[state->pdt_count++] = fn; Duplicate detection could be handled thorough a bitmap. Thanks.
Hi Dmitry, On 3/10/25 19:10, Dmitry Torokhov wrote: > Hi David, > > On Sat, Mar 08, 2025 at 03:08:38PM +0100, David Heidelberg via B4 Relay wrote: >> From: Caleb Connolly <caleb.connolly@linaro.org> >> >> Some third party rmi4-compatible ICs don't expose their PDT entries >> very well. Add a few checks to skip duplicate entries as well as entries >> for unsupported functions. >> >> This is required to support some phones with third party displays. >> >> Validated on a stock OnePlus 6T (original parts): >> manufacturer: Synaptics, product: S3706B, fw id: 2852315 >> >> Co-developed-by: methanal <baclofen@tuta.io> >> Signed-off-by: methanal <baclofen@tuta.io> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> Signed-off-by: David Heidelberg <david@ixit.cz> >> --- >> drivers/input/rmi4/rmi_driver.c | 47 +++++++++++++++++++++++++++++++++++------ >> drivers/input/rmi4/rmi_driver.h | 6 ++++++ >> 2 files changed, 47 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c >> index 2168b6cd7167334d44553c9c566f870a4e034179..51c23a407b2731d5b6eaefe9cae6288f97316e34 100644 >> --- a/drivers/input/rmi4/rmi_driver.c >> +++ b/drivers/input/rmi4/rmi_driver.c >> @@ -493,12 +493,44 @@ static void rmi_driver_copy_pdt_to_fd(const struct pdt_entry *pdt, >> fd->function_version = pdt->function_version; >> } >> >> +static bool rmi_pdt_entry_is_valid(struct rmi_device *rmi_dev, >> + struct pdt_scan_state *state, u8 fn) >> +{ >> + unsigned int i; >> + >> + switch (fn) { >> + case 0x01: >> + case 0x03: >> + case 0x11: >> + case 0x12: >> + case 0x30: >> + case 0x34: >> + case 0x3a: >> + case 0x54: >> + case 0x55: > > This mean that we need to update this code any time there is new > function introduced. I'd rather we did not do that. The driver should be > able to handle unknown functions. Synaptics don't produce new RMI4 based tech anymore afaik, they have moved on. So I don't think there will be new functions being added here. Unfortunately in my experience the fake touch ICs report completely random values here, so there really isn't a good way to handle this otherwise. What if this list rather than being hardcoded here was just using the fn_handlers[] array from rmi_bus.c? Kind regards, > >> + break; >> + >> + default: >> + rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev, >> + "PDT has unknown function number %#02x\n", fn); >> + return false; >> + } >> + >> + for (i = 0; i < state->pdt_count; i++) { >> + if (state->pdts[i] == fn) >> + return false; >> + } >> + >> + state->pdts[state->pdt_count++] = fn; > > Duplicate detection could be handled thorough a bitmap. > > Thanks. >
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c index 2168b6cd7167334d44553c9c566f870a4e034179..51c23a407b2731d5b6eaefe9cae6288f97316e34 100644 --- a/drivers/input/rmi4/rmi_driver.c +++ b/drivers/input/rmi4/rmi_driver.c @@ -493,12 +493,44 @@ static void rmi_driver_copy_pdt_to_fd(const struct pdt_entry *pdt, fd->function_version = pdt->function_version; } +static bool rmi_pdt_entry_is_valid(struct rmi_device *rmi_dev, + struct pdt_scan_state *state, u8 fn) +{ + unsigned int i; + + switch (fn) { + case 0x01: + case 0x03: + case 0x11: + case 0x12: + case 0x30: + case 0x34: + case 0x3a: + case 0x54: + case 0x55: + break; + + default: + rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev, + "PDT has unknown function number %#02x\n", fn); + return false; + } + + for (i = 0; i < state->pdt_count; i++) { + if (state->pdts[i] == fn) + return false; + } + + state->pdts[state->pdt_count++] = fn; + return true; +} + #define RMI_SCAN_CONTINUE 0 #define RMI_SCAN_DONE 1 static int rmi_scan_pdt_page(struct rmi_device *rmi_dev, int page, - int *empty_pages, + struct pdt_scan_state *state, void *ctx, int (*callback)(struct rmi_device *rmi_dev, void *ctx, @@ -521,6 +553,9 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev, if (RMI4_END_OF_PDT(pdt_entry.function_number)) break; + if (!rmi_pdt_entry_is_valid(rmi_dev, state, pdt_entry.function_number)) + continue; + retval = callback(rmi_dev, ctx, &pdt_entry); if (retval != RMI_SCAN_CONTINUE) return retval; @@ -531,11 +566,11 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev, * or more is found, stop scanning. */ if (addr == pdt_start) - ++*empty_pages; + ++state->empty_pages; else - *empty_pages = 0; + state->empty_pages = 0; - return (data->bootloader_mode || *empty_pages >= 2) ? + return (data->bootloader_mode || state->empty_pages >= 2) ? RMI_SCAN_DONE : RMI_SCAN_CONTINUE; } @@ -544,11 +579,11 @@ int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx, void *ctx, const struct pdt_entry *entry)) { int page; - int empty_pages = 0; + struct pdt_scan_state state = {0, 0, {0}}; int retval = RMI_SCAN_DONE; for (page = 0; page <= RMI4_MAX_PAGE; page++) { - retval = rmi_scan_pdt_page(rmi_dev, page, &empty_pages, + retval = rmi_scan_pdt_page(rmi_dev, page, &state, ctx, callback); if (retval != RMI_SCAN_CONTINUE) break; diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h index 3bfe9013043ef3dff46249095a5b3116c8f7d9a6..0ecbd19cefffe00ce7c8001db6479a2e213ac466 100644 --- a/drivers/input/rmi4/rmi_driver.h +++ b/drivers/input/rmi4/rmi_driver.h @@ -46,6 +46,12 @@ struct pdt_entry { u8 function_number; }; +struct pdt_scan_state { + u8 empty_pages; + u8 pdt_count; + u8 pdts[254]; +}; + #define RMI_REG_DESC_PRESENSE_BITS (32 * BITS_PER_BYTE) #define RMI_REG_DESC_SUBPACKET_BITS (37 * BITS_PER_BYTE)