From: Jeff LaBundy <hidden> Date: 2021-03-13 19:14:07
This series continues recent work to further enhance and optimize the Azoteq
IQS550/572/525 trackpad/touchscreen controller driver. In addition to having
been made a bit smaller, the driver now supports some additional use-cases.
Patches 3 and 8 are based on [1] and [2], respectively. Also included in the
series is an updated binding, now presented in YAML.
[1] https://patchwork.kernel.org/patch/12028203/
[2] https://patchwork.kernel.org/patch/12028223/
Jeff LaBundy (9):
Input: iqs5xx - update vendor's URL
Input: iqs5xx - optimize axis definition and validation
Input: iqs5xx - expose firmware revision to user space
Input: iqs5xx - remove superfluous revision validation
Input: iqs5xx - close bootloader using hardware reset
Input: iqs5xx - prevent interrupt storm during removal
Input: iqs5xx - suspend or resume regardless of users
Input: iqs5xx - make reset GPIO optional
dt-bindings: input: iqs5xx: Convert to YAML
.../input/touchscreen/azoteq,iqs5xx.yaml | 75 ++++++
.../bindings/input/touchscreen/iqs5xx.txt | 80 ------
drivers/input/touchscreen/iqs5xx.c | 238 +++++++-----------
3 files changed, 164 insertions(+), 229 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/azoteq,iqs5xx.yaml
delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/iqs5xx.txt
--
2.17.1
From: Jeff LaBundy <hidden> Date: 2021-03-13 19:14:06
Set the maximum ABS_MT_PRESSURE value and use the existing U16_MAX
definition instead of a magic number to validate ABS_MT_POSITION_X
and ABS_MT_POSITION_Y.
Also use input_set_abs_params() rather than input_abs_set_max() to
avoid having to call input_set_capability() separately.
Signed-off-by: Jeff LaBundy <redacted>
---
Changes in v2:
- None
drivers/input/touchscreen/iqs5xx.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-03-13 19:14:06
Replace 'http' with 'https' and correct the spelling of the nearby
word 'datasheet'.
Signed-off-by: Jeff LaBundy <redacted>
---
Changes in v2:
- None
drivers/input/touchscreen/iqs5xx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Jeff LaBundy <hidden> Date: 2021-03-13 19:14:07
Add the read-only 'fw_info' attribute which reports information
about the device's firmware in the following format:
a.b.c.d:e.f
Where:
a = Product number (e.g. 40 for IQS550)
b = Project number (e.g. 15)
c = Firmware revision (major)
d = Firmware revision (minor)
e = Customer-assigned exported file version (major)
f = Customer-assigned exported file version (minor)
As part of the corresponding rework to uses of 'bl_status', the
IQS5XX_BL_STATUS_RESET definition is dropped with 0 used in its
place instead.
Signed-off-by: Jeff LaBundy <redacted>
---
Changes in v2:
- None
drivers/input/touchscreen/iqs5xx.c | 56 +++++++++++++++++++++---------
1 file changed, 40 insertions(+), 16 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-03-13 19:14:36
The bootloader can be closed using the 'execute' command (0x02) or
hardware reset. Rather than using the former option for successful
firmware update procedures and reserving the latter for recovering
the device upon failure, simply use hardware reset for all cases.
The post-bootloader initialization delay increases marginally when
triggered by a hardware reset, so increase the wait time to ensure
the device does not subsequently fail to respond.
As part of this change, refactor the return path to avoid an extra
assignment and to make the logic a bit smaller.
Signed-off-by: Jeff LaBundy <redacted>
---
Changes in v2:
- None
drivers/input/touchscreen/iqs5xx.c | 24 +++++++-----------------
1 file changed, 7 insertions(+), 17 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-03-13 19:14:36
The vendor-assigned firmware project number is restricted to the
generic project number (15); however the vendor may assign other
project numbers to specific applications and customers.
These custom project numbers may be based on forwards-compatible
firmware revision 1.x. However, the driver unnecessarily rejects
anything older than firmware revision 2.0.
To support other applications, remove these unnecessarily strict
checks and enter the bootloader only for truly incompatible A000
devices.
Signed-off-by: Jeff LaBundy <redacted>
---
Changes in v2:
- None
drivers/input/touchscreen/iqs5xx.c | 40 ++++++------------------------
1 file changed, 8 insertions(+), 32 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-03-13 19:14:36
The device's hardware reset pin is only required if the platform
must be able to update the device's firmware.
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.
In that case, the 'fw_file' attribute is hidden because there is
no way to open the bootloader. The logic is extended to the case
in which the device does not advertise bootloader support in the
first place.
Last but not least, remove the hardware reset performed at probe
because there is no reason to reset the device manually. A power
on reset function already ensures a clean reset at start-up.
Signed-off-by: Jeff LaBundy <redacted>
---
Changes in v2:
- None
drivers/input/touchscreen/iqs5xx.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-03-13 19:14:37
The device should be allowed to enter its lowest-power state during
suspend, even if there are no users. Therefore, drop the check from
iqs5xx_suspend().
It follows that the same check must be removed from iqs5xx_resume()
since users are not guaranteed to be present upon resume, and there
would be no way to power the device back up.
This change makes iqs5xx_suspend() and iqs5xx_resume() both smaller
and easier to follow. And because these are the only functions that
call iqs5xx_set_state() now, call device_may_wakeup() from there to
avoid duplicate logic.
While here, collapse the return path for iqs5xx_set_state() to save
a few lines of code.
Signed-off-by: Jeff LaBundy <redacted>
---
Changes in v2:
- None
drivers/input/touchscreen/iqs5xx.c | 41 +++---------------------------
1 file changed, 4 insertions(+), 37 deletions(-)
From: Jeff LaBundy <hidden> Date: 2021-03-13 19:14:37
This patch converts the legacy text-based binding document to YAML
format. Extraneous details and touchscreen properties that weren't
actually supported have been dropped.
The reset GPIO has since been made optional in the driver; this is
now reflected here as well.
Signed-off-by: Jeff LaBundy <redacted>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v2:
- Added vendor prefix to filename and $id
- Added Reviewed-by trailer
.../input/touchscreen/azoteq,iqs5xx.yaml | 75 +++++++++++++++++
.../bindings/input/touchscreen/iqs5xx.txt | 80 -------------------
2 files changed, 75 insertions(+), 80 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/azoteq,iqs5xx.yaml
delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/iqs5xx.txt
@@ -1,80 +0,0 @@-Azoteq IQS550/572/525 Trackpad/Touchscreen Controller--Required properties:--- compatible : Must be equal to one of the following:- "azoteq,iqs550"- "azoteq,iqs572"- "azoteq,iqs525"--- reg : I2C slave address for the device.--- interrupts : GPIO to which the device's active-high RDY- output is connected (see [0]).--- reset-gpios : GPIO to which the device's active-low NRST- input is connected (see [1]).--Optional properties:--- touchscreen-min-x : See [2].--- touchscreen-min-y : See [2].--- touchscreen-size-x : See [2]. If this property is omitted, the- maximum x-coordinate is specified by the- device's "X Resolution" register.--- touchscreen-size-y : See [2]. If this property is omitted, the- maximum y-coordinate is specified by the- device's "Y Resolution" register.--- touchscreen-max-pressure : See [2]. Pressure is expressed as the sum of- the deltas across all channels impacted by a- touch event. A channel's delta is calculated- as its count value minus a reference, where- the count value is inversely proportional to- the channel's capacitance.--- touchscreen-fuzz-x : See [2].--- touchscreen-fuzz-y : See [2].--- touchscreen-fuzz-pressure : See [2].--- touchscreen-inverted-x : See [2]. Inversion is applied relative to that- which may already be specified by the device's- FLIP_X and FLIP_Y register fields.--- touchscreen-inverted-y : See [2]. Inversion is applied relative to that- which may already be specified by the device's- FLIP_X and FLIP_Y register fields.--- touchscreen-swapped-x-y : See [2]. Swapping is applied relative to that- which may already be specified by the device's- SWITCH_XY_AXIS register field.--[0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt-[1]: Documentation/devicetree/bindings/gpio/gpio.txt-[2]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt--Example:-- &i2c1 {- /* ... */-- touchscreen@74 {- compatible = "azoteq,iqs550";- reg = <0x74>;- interrupt-parent = <&gpio>;- interrupts = <17 4>;- reset-gpios = <&gpio 27 1>;-- touchscreen-size-x = <640>;- touchscreen-size-y = <480>;-- touchscreen-max-pressure = <16000>;- };-- /* ... */- };--
From: Jeff LaBundy <hidden> Date: 2021-03-13 19:14:37
Unsolicited I2C communication causes the device to assert an interrupt; as
such the IRQ is disabled before any registers are written in iqs5xx_open()
and iqs5xx_close().
After the driver is unloaded, however, i2c_device_remove() sets the IRQ to
zero before any handlers may call input_close_device() while the device is
unregistered. This keeps iqs5xx_close() from disabling the IRQ, leading to
an interrupt storm during removal.
Placing input_register_device() in front of devm_request_threaded_irq() to
free the IRQ before iqs5xx_close() is called does not cover the case where
firmware is updated at the factory and the input device is registered well
after the driver has already probed.
The solution, therefore, is to remove the open and close callbacks as they
do not buy much in the first place. The device already starts in an active
state, then drops into a low-power mode based on activity.
As an added benefit, this change allows the 250-ms delay in initialization
to be removed as iqs5xx_open() no longer follows immediately. Instead, the
delay is replaced with a mere 50-us delay which allows the interrupt to be
deasserted before the handler is registered.
Signed-off-by: Jeff LaBundy <redacted>
---
Changes in v2:
- None
drivers/input/touchscreen/iqs5xx.c | 25 +------------------------
1 file changed, 1 insertion(+), 24 deletions(-)
Hi Jeff,
On Sat, Mar 13, 2021 at 01:12:33PM -0600, Jeff LaBundy wrote:
Unsolicited I2C communication causes the device to assert an interrupt; as
such the IRQ is disabled before any registers are written in iqs5xx_open()
and iqs5xx_close().
After the driver is unloaded, however, i2c_device_remove() sets the IRQ to
zero before any handlers may call input_close_device() while the device is
unregistered. This keeps iqs5xx_close() from disabling the IRQ, leading to
an interrupt storm during removal.
Placing input_register_device() in front of devm_request_threaded_irq() to
free the IRQ before iqs5xx_close() is called does not cover the case where
firmware is updated at the factory and the input device is registered well
after the driver has already probed.
The solution, therefore, is to remove the open and close callbacks as they
do not buy much in the first place. The device already starts in an active
state, then drops into a low-power mode based on activity.
No, this is not the proper solution. We should rather fix i2c bus (and
really all the other buses with non-trivial probe and remove) so that it
is compatible with devres/devm. I wanted to do this for a while and I
guess we really need this. Could you please try the patch below and see
if it fixes your issue?
Thanks.
--
Dmitry
i2c: ensure timely release of driver-allocated resources
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
More and more drivers rely on devres to manage their resources, however if
bus' probe() and release() are not trivial and control some of resources as
well (for example enable or disable clocks, or attach device to a power
domain, we need to make sure that driver-allocated resources are released
immediately after driver's remove() method returns, and not postponed until
driver core gets around to releasing resources. To fix that we open a new
devres group before calling driver's probe() and explicitly release it when
we return from driver's remove().
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/i2c/i2c-core-base.c | 19 ++++++++++++++++++-
include/linux/i2c.h | 3 +++
2 files changed, 21 insertions(+), 1 deletion(-)
@@ -518,6 +518,11 @@ static int i2c_device_probe(struct device *dev)if(status)gotoerr_clear_wakeup_irq;+client->devres_group_id=devres_open_group(&client->dev,NULL,+GFP_KERNEL);+if(!client->devres_group_id)+gotoerr_detach_pm_domain;+/**Whentherearenomoreusersofprobe(),*renameprobe_newtoprobe.
@@ -530,11 +535,21 @@ static int i2c_device_probe(struct device *dev)elsestatus=-EINVAL;+/*+*Notethatwearenotclosingthedevresgroupopenedaboveso+*evenresourcesthatwereattachedtothedeviceafterprobeis+*runarereleasedwheni2c_device_remove()isexecuted.Thisis+*neededassomedriverswouldallocateadditionalresources,+*forexamplewhenupdatingfirmware.+*/+if(status)-gotoerr_detach_pm_domain;+gotoerr_release_driver_resources;return0;+err_release_driver_resources:+devres_release_group(&client->dev,client->devres_group_id);err_detach_pm_domain:dev_pm_domain_detach(&client->dev,true);err_clear_wakeup_irq:
@@ -563,6 +578,8 @@ static int i2c_device_remove(struct device *dev)dev_warn(dev,"remove failed (%pe), will be ignored\n",ERR_PTR(status));}+devres_release_group(&client->dev,client->devres_group_id);+dev_pm_domain_detach(&client->dev,true);dev_pm_clear_wake_irq(&client->dev);
From: Jeff LaBundy <hidden> Date: 2021-03-15 03:39:32
Hi Dmitry,
On Sat, Mar 13, 2021 at 10:21:27PM -0800, Dmitry Torokhov wrote:
Hi Jeff,
On Sat, Mar 13, 2021 at 01:12:33PM -0600, Jeff LaBundy wrote:
quoted
Unsolicited I2C communication causes the device to assert an interrupt; as
such the IRQ is disabled before any registers are written in iqs5xx_open()
and iqs5xx_close().
After the driver is unloaded, however, i2c_device_remove() sets the IRQ to
zero before any handlers may call input_close_device() while the device is
unregistered. This keeps iqs5xx_close() from disabling the IRQ, leading to
an interrupt storm during removal.
Placing input_register_device() in front of devm_request_threaded_irq() to
free the IRQ before iqs5xx_close() is called does not cover the case where
firmware is updated at the factory and the input device is registered well
after the driver has already probed.
The solution, therefore, is to remove the open and close callbacks as they
do not buy much in the first place. The device already starts in an active
state, then drops into a low-power mode based on activity.
No, this is not the proper solution. We should rather fix i2c bus (and
really all the other buses with non-trivial probe and remove) so that it
is compatible with devres/devm. I wanted to do this for a while and I
guess we really need this. Could you please try the patch below and see
if it fixes your issue?
Thank you for this suggestion; to be honest I had not considered how other
drivers may suffer a similar fate and I agree with your approach. I tested
your patch and it addresses my issue.
That being said, I would still advocate for this patch because of the other
reasons mentioned: the open/close callbacks do not happen to buy much since
the device effectively "opens" and (almost) "closes" automatically based on
touch events, and getting rid of the callbacks lets probe finish faster and
cleans up the code a bit.
Perhaps as a compromise, I can squash this and the next patch, and speak to
these points in a consolidated commit message?
Thanks.
--
Dmitry
Kind regards,
Jeff LaBundy
i2c: ensure timely release of driver-allocated resources
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
More and more drivers rely on devres to manage their resources, however if
bus' probe() and release() are not trivial and control some of resources as
well (for example enable or disable clocks, or attach device to a power
domain, we need to make sure that driver-allocated resources are released
immediately after driver's remove() method returns, and not postponed until
driver core gets around to releasing resources. To fix that we open a new
devres group before calling driver's probe() and explicitly release it when
we return from driver's remove().
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
@@ -518,6 +518,11 @@ static int i2c_device_probe(struct device *dev)if(status)gotoerr_clear_wakeup_irq;+client->devres_group_id=devres_open_group(&client->dev,NULL,+GFP_KERNEL);+if(!client->devres_group_id)+gotoerr_detach_pm_domain;+/**Whentherearenomoreusersofprobe(),*renameprobe_newtoprobe.
@@ -530,11 +535,21 @@ static int i2c_device_probe(struct device *dev)elsestatus=-EINVAL;+/*+*Notethatwearenotclosingthedevresgroupopenedaboveso+*evenresourcesthatwereattachedtothedeviceafterprobeis+*runarereleasedwheni2c_device_remove()isexecuted.Thisis+*neededassomedriverswouldallocateadditionalresources,+*forexamplewhenupdatingfirmware.+*/+if(status)-gotoerr_detach_pm_domain;+gotoerr_release_driver_resources;return0;+err_release_driver_resources:+devres_release_group(&client->dev,client->devres_group_id);err_detach_pm_domain:dev_pm_domain_detach(&client->dev,true);err_clear_wakeup_irq:
@@ -563,6 +578,8 @@ static int i2c_device_remove(struct device *dev)dev_warn(dev,"remove failed (%pe), will be ignored\n",ERR_PTR(status));}+devres_release_group(&client->dev,client->devres_group_id);+dev_pm_domain_detach(&client->dev,true);dev_pm_clear_wake_irq(&client->dev);
On Sat, Mar 13, 2021 at 01:12:30PM -0600, Jeff LaBundy wrote:
Add the read-only 'fw_info' attribute which reports information
about the device's firmware in the following format:
a.b.c.d:e.f
Where:
a = Product number (e.g. 40 for IQS550)
b = Project number (e.g. 15)
c = Firmware revision (major)
d = Firmware revision (minor)
e = Customer-assigned exported file version (major)
f = Customer-assigned exported file version (minor)
As part of the corresponding rework to uses of 'bl_status', the
IQS5XX_BL_STATUS_RESET definition is dropped with 0 used in its
place instead.
Signed-off-by: Jeff LaBundy <redacted>
On Sat, Mar 13, 2021 at 01:12:29PM -0600, Jeff LaBundy wrote:
Set the maximum ABS_MT_PRESSURE value and use the existing U16_MAX
definition instead of a magic number to validate ABS_MT_POSITION_X
and ABS_MT_POSITION_Y.
Also use input_set_abs_params() rather than input_abs_set_max() to
avoid having to call input_set_capability() separately.
Signed-off-by: Jeff LaBundy <redacted>
On Sat, Mar 13, 2021 at 01:12:31PM -0600, Jeff LaBundy wrote:
The vendor-assigned firmware project number is restricted to the
generic project number (15); however the vendor may assign other
project numbers to specific applications and customers.
These custom project numbers may be based on forwards-compatible
firmware revision 1.x. However, the driver unnecessarily rejects
anything older than firmware revision 2.0.
To support other applications, remove these unnecessarily strict
checks and enter the bootloader only for truly incompatible A000
devices.
Signed-off-by: Jeff LaBundy <redacted>
On Sat, Mar 13, 2021 at 01:12:32PM -0600, Jeff LaBundy wrote:
The bootloader can be closed using the 'execute' command (0x02) or
hardware reset. Rather than using the former option for successful
firmware update procedures and reserving the latter for recovering
the device upon failure, simply use hardware reset for all cases.
The post-bootloader initialization delay increases marginally when
triggered by a hardware reset, so increase the wait time to ensure
the device does not subsequently fail to respond.
As part of this change, refactor the return path to avoid an extra
assignment and to make the logic a bit smaller.
Signed-off-by: Jeff LaBundy <redacted>