From: Jeff LaBundy <hidden> Date: 2021-01-18 20:48:04
This series includes a variety of minor enhancements and optimizations to
the Azoteq IQS550/572/525 trackpad/touchscreen controller driver, some of
which are based on learnings during recent work with other Azoteq devices.
As a result, the driver has shrunk a bit despite having gained additional
functionality.
Jeff LaBundy (10):
input: iqs5xx: Minor cosmetic improvements
input: iqs5xx: Preserve bootloader errors
input: iqs5xx: Accommodate bootloader latency
input: iqs5xx: Expose firmware revision to user space
input: iqs5xx: Re-initialize device upon warm reset
input: iqs5xx: Simplify axis setup logic
input: iqs5xx: Eliminate unnecessary register read
input: iqs5xx: Allow more time for ATI to complete
input: iqs5xx: Make reset GPIO optional
input: iqs5xx: Allow device to be a wake-up source
drivers/input/touchscreen/iqs5xx.c | 231 ++++++++++++++++++-------------------
1 file changed, 110 insertions(+), 121 deletions(-)
--
2.7.4
From: Jeff LaBundy <hidden> Date: 2021-01-18 20:47:33
The device's firmware accommodates a revision field that customers
can assign when firmware is exported from the vendor's development
tool. Having the ability to read this field from user space can be
useful during development.
As such, promote the fw_file attribute from W/O to R/W. Writing to
the attribute pushes firmware to the device as normal, but reading
from it will now return the customer-assigned revision field as an
unsigned integer (e.g. 256 = 1.0, 257 = 1.1 and so on).
Signed-off-by: Jeff LaBundy <redacted>
---
drivers/input/touchscreen/iqs5xx.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
From: Jeff LaBundy <hidden> Date: 2021-01-18 20:47:53
The device may be inadvertently reset during runtime in the event
of ESD strike, etc. To protect against this case, acknowledge the
SHOW_RESET interrupt and re-initialize the device.
To facilitate this change, expand the range of registers that are
read in the interrupt handler to include the system status fields.
Also, update the unrelated (but nearby) SUSPEND register field to
use the BIT() macro. The remaining register fields are cleaned up
in another patch.
Signed-off-by: Jeff LaBundy <redacted>
---
drivers/input/touchscreen/iqs5xx.c | 51 ++++++++++++++++++++++++++++++--------
1 file changed, 40 insertions(+), 11 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-01-18 20:48:04
Copyrights are generally followed by the name of a person or a
company (i.e. the copyright holder) but that was not done here.
Fix this by squashing the 'copyright' and 'author' lines.
Also, trim some leading whitespace ahead of the parameters for
the fw_file_store() function and re-align them for readability.
Signed-off-by: Jeff LaBundy <redacted>
---
drivers/input/touchscreen/iqs5xx.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-01-18 20:48:04
Instead of relying on firmware to enable important register fields
and performing read-modify-write operations to additionally enable
the fields the driver cares about, it's much simpler just to write
all of the pertinent fields explicitly.
This avoids an unnecessary register read operation at start-up and
makes way for the iqs5xx_read_byte() helper to be dropped.
Signed-off-by: Jeff LaBundy <redacted>
---
drivers/input/touchscreen/iqs5xx.c | 31 ++++++++++++-------------------
1 file changed, 12 insertions(+), 19 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-01-18 20:48:08
The present implementation manipulates axis swap and inversion fields
in the device to more or less duplicate what touchscreen_report_pos()
does. The resulting logic is convoluted and difficult to follow.
Instead report the maximum X and Y coordinates in earnest as they are
read from the device, then let touchscreen_parse_properties() fix the
axes up as necessary. Finally, use touchscreen_report_pos() to report
the transformed coordinates.
Last but not least, the maximum X and Y coordinates are not functions
of the number of rows/columns that comprise the touch surface. Either
coordinate is simply limited to 1 below what is reported for absolute
X or Y coordinates when no fingers are present (0xFFFF).
Signed-off-by: Jeff LaBundy <redacted>
---
drivers/input/touchscreen/iqs5xx.c | 100 ++++++++-----------------------------
1 file changed, 21 insertions(+), 79 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-01-18 20:51:35
After user space writes the fw_file attribute to push new firmware
to the device, the driver calls iqs5xx_dev_init() to re-initialize
the device with the updated firmware or recover the device in case
the update failed.
In the case of the latter, however, iqs5xx_fw_file_write() returns
zero (success) so long as iqs5xx_dev_init() does not fail, and any
error encountered during the update process is lost. Solve this by
saving the error before calling iqs5xx_dev_init().
Signed-off-by: Jeff LaBundy <redacted>
---
drivers/input/touchscreen/iqs5xx.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
From: Jeff LaBundy <hidden> Date: 2021-01-18 20:53:55
Avoid placing the device in suspend mode (from which it cannot
generate interrupts) if it is defined as a wake-up source. The
device is still permitted to enter a low-power sensing mode on
its own.
Signed-off-by: Jeff LaBundy <redacted>
---
drivers/input/touchscreen/iqs5xx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-01-18 20:54:04
The bootloader NAK's all I2C communication after the first 64-byte
bulk write if the bus frequency is equal to 400 kHz. This prevents
the platform from pushing updated firmware to the device.
The vendor's USB bootloader programming dongle appears to insert a
delay between the "open" command and the first 64-byte bulk write.
Adding a similar delay to the driver seems to eliminate the issue.
Furthermore, the dongle does not access the bootloader immediately
after powering up the device. Follow suit by adding a delay before
the "open" command to avoid wasted retries at 400 kHz.
Signed-off-by: Jeff LaBundy <redacted>
---
drivers/input/touchscreen/iqs5xx.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-01-18 20:54:04
The device's hardware reset pin is only required if the platform
must be able to update the device's firmware on the fly.
As such, demote the reset GPIO to optional in support of devices
that ship with pre-programmed firmware and don't route the reset
pin back to the SOC.
If user space attempts to push updated firmware which would rely
upon the reset pin to wake the bootloader, attempts to reach the
bootloader are simply NAK'd and the device resumes normally.
Signed-off-by: Jeff LaBundy <redacted>
---
drivers/input/touchscreen/iqs5xx.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-01-18 20:54:04
After the device is initialized, it runs ATI (calibration) during
which it cannot readily respond to I2C communication. To keep the
open and close callbacks from writing to the device too soon, the
driver waits 100 ms before returning from probe.
The vendor reports that ATI may actually take up to 250 ms to run
(including margin), so increase the delay accordingly. Update the
comments to clarify the reason for the delay as well.
Signed-off-by: Jeff LaBundy <redacted>
---
drivers/input/touchscreen/iqs5xx.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
On Mon, Jan 18, 2021 at 02:43:37PM -0600, Jeff LaBundy wrote:
Copyrights are generally followed by the name of a person or a
company (i.e. the copyright holder) but that was not done here.
Fix this by squashing the 'copyright' and 'author' lines.
Also, trim some leading whitespace ahead of the parameters for
the fw_file_store() function and re-align them for readability.
Signed-off-by: Jeff LaBundy <redacted>
On Mon, Jan 18, 2021 at 02:43:38PM -0600, Jeff LaBundy wrote:
After user space writes the fw_file attribute to push new firmware
to the device, the driver calls iqs5xx_dev_init() to re-initialize
the device with the updated firmware or recover the device in case
the update failed.
In the case of the latter, however, iqs5xx_fw_file_write() returns
zero (success) so long as iqs5xx_dev_init() does not fail, and any
error encountered during the update process is lost. Solve this by
saving the error before calling iqs5xx_dev_init().
Signed-off-by: Jeff LaBundy <redacted>
On Mon, Jan 18, 2021 at 02:43:39PM -0600, Jeff LaBundy wrote:
The bootloader NAK's all I2C communication after the first 64-byte
bulk write if the bus frequency is equal to 400 kHz. This prevents
the platform from pushing updated firmware to the device.
The vendor's USB bootloader programming dongle appears to insert a
delay between the "open" command and the first 64-byte bulk write.
Adding a similar delay to the driver seems to eliminate the issue.
Furthermore, the dongle does not access the bootloader immediately
after powering up the device. Follow suit by adding a delay before
the "open" command to avoid wasted retries at 400 kHz.
Signed-off-by: Jeff LaBundy <redacted>
Hi Jeff,
On Mon, Jan 18, 2021 at 02:43:40PM -0600, Jeff LaBundy wrote:
The device's firmware accommodates a revision field that customers
can assign when firmware is exported from the vendor's development
tool. Having the ability to read this field from user space can be
useful during development.
As such, promote the fw_file attribute from W/O to R/W. Writing to
the attribute pushes firmware to the device as normal, but reading
from it will now return the customer-assigned revision field as an
unsigned integer (e.g. 256 = 1.0, 257 = 1.1 and so on).
No, let's not overload this attribute and instead create a dedicated
fw_version or similar read-only attribute to expose active firmware
version to userspace.
Thanks.
--
Dmitry
On Mon, Jan 18, 2021 at 02:43:41PM -0600, Jeff LaBundy wrote:
The device may be inadvertently reset during runtime in the event
of ESD strike, etc. To protect against this case, acknowledge the
SHOW_RESET interrupt and re-initialize the device.
To facilitate this change, expand the range of registers that are
read in the interrupt handler to include the system status fields.
Also, update the unrelated (but nearby) SUSPEND register field to
use the BIT() macro. The remaining register fields are cleaned up
in another patch.
Added linux/bits.h include and applied, thank you.
--
Dmitry
On Mon, Jan 18, 2021 at 02:43:42PM -0600, Jeff LaBundy wrote:
The present implementation manipulates axis swap and inversion fields
in the device to more or less duplicate what touchscreen_report_pos()
does. The resulting logic is convoluted and difficult to follow.
Instead report the maximum X and Y coordinates in earnest as they are
read from the device, then let touchscreen_parse_properties() fix the
axes up as necessary. Finally, use touchscreen_report_pos() to report
the transformed coordinates.
Last but not least, the maximum X and Y coordinates are not functions
of the number of rows/columns that comprise the touch surface. Either
coordinate is simply limited to 1 below what is reported for absolute
X or Y coordinates when no fingers are present (0xFFFF).
Signed-off-by: Jeff LaBundy <redacted>
On Mon, Jan 18, 2021 at 02:43:43PM -0600, Jeff LaBundy wrote:
Instead of relying on firmware to enable important register fields
and performing read-modify-write operations to additionally enable
the fields the driver cares about, it's much simpler just to write
all of the pertinent fields explicitly.
This avoids an unnecessary register read operation at start-up and
makes way for the iqs5xx_read_byte() helper to be dropped.
Signed-off-by: Jeff LaBundy <redacted>
On Mon, Jan 18, 2021 at 02:43:44PM -0600, Jeff LaBundy wrote:
After the device is initialized, it runs ATI (calibration) during
which it cannot readily respond to I2C communication. To keep the
open and close callbacks from writing to the device too soon, the
driver waits 100 ms before returning from probe.
The vendor reports that ATI may actually take up to 250 ms to run
(including margin), so increase the delay accordingly. Update the
comments to clarify the reason for the delay as well.
Signed-off-by: Jeff LaBundy <redacted>
On Mon, Jan 18, 2021 at 02:43:45PM -0600, Jeff LaBundy wrote:
The device's hardware reset pin is only required if the platform
must be able to update the device's firmware on the fly.
As such, demote the reset GPIO to optional in support of devices
that ship with pre-programmed firmware and don't route the reset
pin back to the SOC.
If user space attempts to push updated firmware which would rely
upon the reset pin to wake the bootloader, attempts to reach the
bootloader are simply NAK'd and the device resumes normally.
Can we maybe make the firmware attribute invisible in this case? Or
return early instead of failing to enter bootloader mode?
Thanks.
--
Dmitry
On Mon, Jan 18, 2021 at 02:43:46PM -0600, Jeff LaBundy wrote:
Avoid placing the device in suspend mode (from which it cannot
generate interrupts) if it is defined as a wake-up source. The
device is still permitted to enter a low-power sensing mode on
its own.
Signed-off-by: Jeff LaBundy <redacted>
From: Jeff LaBundy <hidden> Date: 2021-01-26 19:14:46
Hi Dmitry,
On Sun, Jan 24, 2021 at 08:43:46PM -0800, Dmitry Torokhov wrote:
On Mon, Jan 18, 2021 at 02:43:45PM -0600, Jeff LaBundy wrote:
quoted
The device's hardware reset pin is only required if the platform
must be able to update the device's firmware on the fly.
As such, demote the reset GPIO to optional in support of devices
that ship with pre-programmed firmware and don't route the reset
pin back to the SOC.
If user space attempts to push updated firmware which would rely
upon the reset pin to wake the bootloader, attempts to reach the
bootloader are simply NAK'd and the device resumes normally.
Can we maybe make the firmware attribute invisible in this case? Or
return early instead of failing to enter bootloader mode?
I almost sent the second alternative, but instead I liked the idea of
restricting the check for reset_gpio to the only method that actually
uses it. That way, nothing outside of iqs5xx_reset() has to check for
the GPIO and can just fail gracefully in its absence.
That being said, either of your suggestions work just as well; let me
take another stab at it.
From: Jeff LaBundy <hidden> Date: 2021-01-26 19:14:46
Hi Dmitry,
Thank you for taking a look at this series and I do apologize for the
complaint from the bot this morning.
On Sun, Jan 24, 2021 at 08:22:01PM -0800, Dmitry Torokhov wrote:
Hi Jeff,
On Mon, Jan 18, 2021 at 02:43:40PM -0600, Jeff LaBundy wrote:
quoted
The device's firmware accommodates a revision field that customers
can assign when firmware is exported from the vendor's development
tool. Having the ability to read this field from user space can be
useful during development.
As such, promote the fw_file attribute from W/O to R/W. Writing to
the attribute pushes firmware to the device as normal, but reading
from it will now return the customer-assigned revision field as an
unsigned integer (e.g. 256 = 1.0, 257 = 1.1 and so on).
No, let's not overload this attribute and instead create a dedicated
fw_version or similar read-only attribute to expose active firmware
version to userspace.
Not a problem; I'll create a new R/O attribute for this purpose.