* Re: [regression] Resume broken on T14s Gen1 (AMD) due to "Input: psmouse - add delay when deactivating for SMBus mode"
From: Thorsten Leemhuis @ 2023-10-03 7:30 UTC (permalink / raw)
To: Jeffery Miller
Cc: Andrew Duggan, Dmitry Torokhov, Benjamin Tissoires, linux-input,
linux-kernel, Linux kernel regressions list
In-Reply-To: <CAAzPG9MD+UQb_RdiMkPkpQGYe-arD1nMKWngMj4P5s3_zJvphQ@mail.gmail.com>
On 02.10.23 18:52, Jeffery Miller wrote:
> On Sat, Sep 30, 2023 at 4:04 AM Thorsten Leemhuis <linux@leemhuis.info> wrote:
>> """
>>> diff --git a/drivers/input/mouse/psmouse-smbus.c b/drivers/input/mouse/psmouse-smbus.c
>>> index 7b13de979908..fe12385bb856 100644
>>> --- a/drivers/input/mouse/psmouse-smbus.c
>>> +++ b/drivers/input/mouse/psmouse-smbus.c
>>> @@ -121,11 +121,11 @@ static psmouse_ret_t psmouse_smbus_process_byte(struct psmouse *psmouse)
>>>
>>> static void psmouse_activate_smbus_mode(struct psmouse_smbus_dev *smbdev)
>>> {
>>> - if (smbdev->need_deactivate) {
>>> - psmouse_deactivate(smbdev->psmouse);
>>> - /* Give the device time to switch into SMBus mode */
>>> - msleep(30);
>>> - }
>>> + if (smbdev->psmouse == NULL) {
>>> + printk("XXX: smbdev->psmouse is null\n");
>>> + } else {
>>> + printk("XXX: smbdev->psmouse is set\n");
>>> + }
>>> }
>>>
>>> static int psmouse_smbus_reconnect(struct psmouse *psmouse)
>> """
>>
>> During boot this prints "XXX: smbdev->psmouse is set". But it helped, as
>> the machine now resumes from s2idle again -- while printing "XXX:
>> smbdev->psmouse is null". And that should not be the case I assume. Or
>> did my brute force test go sideways due to my limited skills?
>
> This was a good test. You've identified where it is crashing.
>
> Maybe we could confirm that `psmouse->private` is not-NULL?:
> ```
> diff --git a/drivers/input/mouse/psmouse-smbus.c
> b/drivers/input/mouse/psmouse-smbus.c
> index 7b13de979908..432615df9ae8 100644
> --- a/drivers/input/mouse/psmouse-smbus.c
> +++ b/drivers/input/mouse/psmouse-smbus.c
> @@ -130,7 +130,10 @@ static void psmouse_activate_smbus_mode(struct
> psmouse_smbus_dev *smbdev)
>
> static int psmouse_smbus_reconnect(struct psmouse *psmouse)
> {
> - psmouse_activate_smbus_mode(psmouse->private);
> + if (psmouse->private == NULL) {
> + printk("XXX smbdev is null");
> + }
> + //psmouse_activate_smbus_mode(psmouse->private);
> return 0;
> }
> ```
This didn't print anything on resume, so `psmouse->private` apparently
is set.
Tried brute force again afterwards to find what might unset
smbdev->psmouse by adding printk statements to
psmouse_smbus_disconnect() and psmouse_smbus_cleanup() but those didn't
fire, so it must be something else I didn't spot.
Ciao, Thorsten
> On Thu, Sep 28, 2023 at 4:08 AM Thorsten Leemhuis <linux@leemhuis.info> wrote:
>>
>> On 27.09.23 19:23, Thorsten Leemhuis wrote:
>>> On 27.09.23 17:55, Jeffery Miller wrote:
>>>> On Wed, Sep 27, 2023 at 10:43 AM Jeffery Miller
>>>> <jefferymiller@google.com> wrote:
>>>>> On Wed, Sep 27, 2023 at 3:54 AM Thorsten Leemhuis <linux@leemhuis.info> wrote:
>>>>>>
>>>>>> My dmesg from a kernel with the revert:
>>>>>> https://www.leemhuis.info/files/misc/dmesg
>>>
>>> Thx for looking into this!
>>>
>>>>> In this dmesg output it shows that this is an elantech smbus device:
>>>>> ```
>>>>> [ 4.260415] psmouse serio1: elantech: assuming hardware version 4 (with firmware version 0x7f3001)
>>>>> [ 4.279297] psmouse serio1: elantech: Synaptics capabilities query result 0x90, 0x18, 0x0f.
>>>>> [ 4.292788] psmouse serio1: elantech: Elan sample query result 00, 80, c9
>>>>> [ 4.319184] psmouse serio1: elantech: Elan ic body: 0x10, current fw version: 0x3
>>>>> ...
>>>>> [ 4.346951] psmouse serio1: elantech: Trying to set up SMBus access
>>>>> [ 4.346986] psmouse serio1: elantech: SMbus companion is not ready yet
>>>>> [ 4.369993] input: ETPS/2 Elantech TrackPoint as /devices/platform/i8042/serio1/input/input7
>>>>> [ 4.376200] systemd[1]: bpf-lsm: LSM BPF program attached
>>>>> [ 4.385192] input: ETPS/2 Elantech Touchpad as /devices/platform/i8042/serio1/input/input5
>>>>> ```
>>>>> The change in 92e24e0e57f72e shouldn't affect the elantouch device as elantech_setup_smbus
>>>>> initializes `psmouse_smbus_init` with need_deactivate = false.
>>>
>>> Hmmm. Wondering if I should warm up the compiler again to recheck my
>>> result one more time[1].
>>
>> Just did that. Ran "make clean" and compiled mainline as of now
>> (633b47cb009d) and the machine does never resume from s2idle; then I
>> reverted 92e24e0e57f7 and compiled again (for completeness: without
>> running "make clean" beforehand) and with that kernel s2idle resume
>> works perfectly fine.
>>
>> Wondering if I or the compiler is doing something stupid here -- or if
>> we missed some small but important detail somewhere.
>>
>> Ciao, Thorsten
>>
>>>>> Did you store dmesg logs from boot without the applied patch?
>>>> I intended to ask if you have logs from a boot without 92e24e0e57f72e reverted.
>>>
>>> https://www.leemhuis.info/files/misc/dmesg-6.6-rc3-vanilla
>>>
>>>>> If the delay was being applied the timestamps should show the 30ms delay between
>>>>> `psmouse serio1: elantech: Trying to set up SMBus access`
>>>>> and
>>>>> `psmouse serio1: elantech: SMbus companion is not ready yet`
>>>
>>> Unless I missed something there is not difference. :-/
>>>
>>> Ciao, Thorsten
>>>
>>> [1] FWIW, this is my bisect log
>>>
>>> """
>>>> git bisect start
>>>> # status: waiting for both good and bad commits
>>>> # bad: [6465e260f48790807eef06b583b38ca9789b6072] Linux 6.6-rc3
>>>> git bisect bad 6465e260f48790807eef06b583b38ca9789b6072
>>>> # status: waiting for good commit(s), bad commit known
>>>> # good: [2dde18cd1d8fac735875f2e4987f11817cc0bc2c] Linux 6.5
>>>> git bisect good 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
>>>> # good: [4fb0dacb78c6a041bbd38ddd998df806af5c2c69] Merge tag 'sound-6.6-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
>>>> git bisect good 4fb0dacb78c6a041bbd38ddd998df806af5c2c69
>>>> # good: [307d59039fb26212a84a9aa6a134a7d2bdea34ca] Merge tag 'media/v6.6-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
>>>> git bisect good 307d59039fb26212a84a9aa6a134a7d2bdea34ca
>>>> # bad: [4a0fc73da97efd23a383ca839e6fe86410268f6b] Merge tag 's390-6.6-2' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux
>>>> git bisect bad 4a0fc73da97efd23a383ca839e6fe86410268f6b
>>>> # good: [e4f1b8202fb59c56a3de7642d50326923670513f] Merge tag 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost
>>>> git bisect good e4f1b8202fb59c56a3de7642d50326923670513f
>>>> # good: [5eea5820c7340d39e56e169e1b87199391105f6b] Merge tag 'mm-stable-2023-09-04-14-00' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
>>>> git bisect good 5eea5820c7340d39e56e169e1b87199391105f6b
>>>> # good: [65d6e954e37872fd9afb5ef3fc0481bb3c2f20f4] Merge tag 'gfs2-v6.5-rc5-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2
>>>> git bisect good 65d6e954e37872fd9afb5ef3fc0481bb3c2f20f4
>>>> # bad: [744a759492b5c57ff24a6e8aabe47b17ad8ee964] Merge tag 'input-for-v6.6-rc0' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input
>>>> git bisect bad 744a759492b5c57ff24a6e8aabe47b17ad8ee964
>>>> # good: [dbce1a7d5dce7318d8465b1e0d052ef1d2202237] Input: Explicitly include correct DT includes
>>>> git bisect good dbce1a7d5dce7318d8465b1e0d052ef1d2202237
>>>> # good: [29057cc5bddc785ea0a11534d7ad2546fa0872d3] Merge tag 'linux-watchdog-6.6-rc1' of git://www.linux-watchdog.org/linux-watchdog
>>>> git bisect good 29057cc5bddc785ea0a11534d7ad2546fa0872d3
>>>> # bad: [3e4bb047b23375a34dbf5885709ac3729d9cfb22] Input: qt2160 - convert to use devm_* api
>>>> git bisect bad 3e4bb047b23375a34dbf5885709ac3729d9cfb22
>>>> # good: [e175eae16c1bf92062f1f431a95f476a61a77c48] Input: mcs-touchkey - convert to use devm_* api
>>>> git bisect good e175eae16c1bf92062f1f431a95f476a61a77c48
>>>> # bad: [92e24e0e57f72e06c2df87116557331fd2d4dda2] Input: psmouse - add delay when deactivating for SMBus mode
>>>> git bisect bad 92e24e0e57f72e06c2df87116557331fd2d4dda2
>>>> # good: [8362bf82fb5441613aac7c6c9dbb6b83def6ad3b] Input: mcs-touchkey - fix uninitialized use of error in mcs_touchkey_probe()
>>>> git bisect good 8362bf82fb5441613aac7c6c9dbb6b83def6ad3b
>>>> # first bad commit: [92e24e0e57f72e06c2df87116557331fd2d4dda2] Input: psmouse - add delay when deactivating for SMBus mode
>>> """
>>>
>>>
>
>
^ permalink raw reply
* Re: [PATCH 2/3] Input: cap11xx - Convert to use maple tree register cache
From: Dmitry Torokhov @ 2023-10-03 6:16 UTC (permalink / raw)
To: Mark Brown; +Cc: linux-input, linux-kernel
In-Reply-To: <20231001-input-maple-v1-2-ed3716051431@kernel.org>
Hi Mark,
On Sun, Oct 01, 2023 at 01:43:39AM +0200, Mark Brown wrote:
> The maple tree register cache is based on a much more modern data structure
> than the rbtree cache and makes optimisation choices which are probably
> more appropriate for modern systems than those made by the rbtree cache.
>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> drivers/input/keyboard/cap11xx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
> index 39ed3b9ddc65..77843ad15d4c 100644
> --- a/drivers/input/keyboard/cap11xx.c
> +++ b/drivers/input/keyboard/cap11xx.c
> @@ -174,7 +174,7 @@ static const struct regmap_config cap11xx_regmap_config = {
> .reg_defaults = cap11xx_reg_defaults,
>
> .num_reg_defaults = ARRAY_SIZE(cap11xx_reg_defaults),
> - .cache_type = REGCACHE_RBTREE,
> + .cache_type = REGCACHE_MAPLE,
I do not think these driver care much about the cache type. Optimal one
might even depend on the architecture. I wonder if we could have
something like REGCACHE_DEFAULT to signal that whatever is the "best
default" implementation it should be used?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/7] dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc
From: Rob Herring @ 2023-10-02 19:23 UTC (permalink / raw)
To: Caleb Connolly
Cc: Dmitry Torokhov, Vincent Huang, methanal, linux-input, devicetree,
phone-devel, ~postmarketos/upstreaming, Jason A. Donenfeld,
Matthias Schiffer, Krzysztof Kozlowski, Conor Dooley
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-1-cc3c703f022d@linaro.org>
On Sat, Sep 30, 2023 at 06:08:45PM +0100, Caleb Connolly wrote:
> This new property allows devices to specify some register values which
> are missing on units with third party replacement displays. These
> displays use unofficial touch ICs which only implement a subset of the
> RMI4 specification.
>
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
> To: "Jason A. Donenfeld" <Jason@zx2c4.com>
> To: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> To: Rob Herring <robh+dt@kernel.org>
> To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> To: Conor Dooley <conor+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
> Documentation/devicetree/bindings/input/syna,rmi4.yaml | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/syna,rmi4.yaml b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
> index 4d4e1a8e36be..bd6beb67ac21 100644
> --- a/Documentation/devicetree/bindings/input/syna,rmi4.yaml
> +++ b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
> @@ -49,6 +49,16 @@ properties:
> description:
> Delay to wait after powering on the device.
>
> + syna,pdt-fallback-desc:
> + $ref: /schemas/types.yaml#/definitions/uint8-array
> + description:
> + An array of pairs of function number and version. These are used
array of pairs is a matrix, not array. Or it is just 2 values? Needs
some size constraints.
> + on some devices with replacement displays that use unofficial touch
> + controllers. These controllers do report the properties of their PDT
PDT?
> + entries, but leave the function_number and function_version registers
> + blank. These values should match exactly the 5th and 4th bytes of each
> + PDT entry from the original display's touch controller.
> +
> vdd-supply: true
> vio-supply: true
>
>
> --
> 2.42.0
>
^ permalink raw reply
* Re: [PATCH RFC v4 5/6] ARM: pxa: Convert gumstix Bluetooth to GPIO descriptors
From: Bartosz Golaszewski @ 2023-10-02 17:09 UTC (permalink / raw)
To: Duje Mihanović
Cc: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij, Andy Shevchenko,
Dmitry Torokhov, Mark Brown, linux-arm-kernel, linux-kernel,
linux-usb, linux-gpio, linux-input, linux-spi
In-Reply-To: <4528128.LvFx2qVVIh@radijator>
On Mon, Oct 2, 2023 at 4:53 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
>
> On Monday, October 2, 2023 9:42:52 AM CEST Bartosz Golaszewski wrote:
> > This changes the way this code works. You release the descriptor here,
> > it returns to the driver and can be re-requested by someone else. Its
> > value is also not guaranteed to remain as "active". Is this what you
> > want?
>
> Good point. Is it enough to not call gpiod_put() at the end or is it necessary
> to use a static gpio_desc instead of a local one?
>
Technically it's enough to not put it. It will live on but the
reference will be leaked and most likely this will be reported by
kmemleak. So static desc would make more sense.
Bart
^ permalink raw reply
* Re: [regression] Resume broken on T14s Gen1 (AMD) due to "Input: psmouse - add delay when deactivating for SMBus mode"
From: Jeffery Miller @ 2023-10-02 16:52 UTC (permalink / raw)
To: Thorsten Leemhuis
Cc: Andrew Duggan, Dmitry Torokhov, Benjamin Tissoires, linux-input,
linux-kernel, Linux kernel regressions list
In-Reply-To: <1b3f8dd2-6364-4f00-a33e-8b15b8911dbf@leemhuis.info>
Hello,
On Sat, Sep 30, 2023 at 4:04 AM Thorsten Leemhuis <linux@leemhuis.info> wrote:
>
> [FWIW, in case sending this in private happened accidentally, feel free
> to make this public again.]
This was unintentional. Replying back to the list.
> """
> > diff --git a/drivers/input/mouse/psmouse-smbus.c b/drivers/input/mouse/psmouse-smbus.c
> > index 7b13de979908..fe12385bb856 100644
> > --- a/drivers/input/mouse/psmouse-smbus.c
> > +++ b/drivers/input/mouse/psmouse-smbus.c
> > @@ -121,11 +121,11 @@ static psmouse_ret_t psmouse_smbus_process_byte(struct psmouse *psmouse)
> >
> > static void psmouse_activate_smbus_mode(struct psmouse_smbus_dev *smbdev)
> > {
> > - if (smbdev->need_deactivate) {
> > - psmouse_deactivate(smbdev->psmouse);
> > - /* Give the device time to switch into SMBus mode */
> > - msleep(30);
> > - }
> > + if (smbdev->psmouse == NULL) {
> > + printk("XXX: smbdev->psmouse is null\n");
> > + } else {
> > + printk("XXX: smbdev->psmouse is set\n");
> > + }
> > }
> >
> > static int psmouse_smbus_reconnect(struct psmouse *psmouse)
> """
>
> During boot this prints "XXX: smbdev->psmouse is set". But it helped, as
> the machine now resumes from s2idle again -- while printing "XXX:
> smbdev->psmouse is null". And that should not be the case I assume. Or
> did my brute force test go sideways due to my limited skills?
This was a good test. You've identified where it is crashing.
Maybe we could confirm that `psmouse->private` is not-NULL?:
```
diff --git a/drivers/input/mouse/psmouse-smbus.c
b/drivers/input/mouse/psmouse-smbus.c
index 7b13de979908..432615df9ae8 100644
--- a/drivers/input/mouse/psmouse-smbus.c
+++ b/drivers/input/mouse/psmouse-smbus.c
@@ -130,7 +130,10 @@ static void psmouse_activate_smbus_mode(struct
psmouse_smbus_dev *smbdev)
static int psmouse_smbus_reconnect(struct psmouse *psmouse)
{
- psmouse_activate_smbus_mode(psmouse->private);
+ if (psmouse->private == NULL) {
+ printk("XXX smbdev is null");
+ }
+ //psmouse_activate_smbus_mode(psmouse->private);
return 0;
}
```
Thanks,
Jeff
On Thu, Sep 28, 2023 at 4:08 AM Thorsten Leemhuis <linux@leemhuis.info> wrote:
>
> On 27.09.23 19:23, Thorsten Leemhuis wrote:
> > On 27.09.23 17:55, Jeffery Miller wrote:
> >> On Wed, Sep 27, 2023 at 10:43 AM Jeffery Miller
> >> <jefferymiller@google.com> wrote:
> >>> On Wed, Sep 27, 2023 at 3:54 AM Thorsten Leemhuis <linux@leemhuis.info> wrote:
> >>>>
> >>>> My dmesg from a kernel with the revert:
> >>>> https://www.leemhuis.info/files/misc/dmesg
> >
> > Thx for looking into this!
> >
> >>> In this dmesg output it shows that this is an elantech smbus device:
> >>> ```
> >>> [ 4.260415] psmouse serio1: elantech: assuming hardware version 4 (with firmware version 0x7f3001)
> >>> [ 4.279297] psmouse serio1: elantech: Synaptics capabilities query result 0x90, 0x18, 0x0f.
> >>> [ 4.292788] psmouse serio1: elantech: Elan sample query result 00, 80, c9
> >>> [ 4.319184] psmouse serio1: elantech: Elan ic body: 0x10, current fw version: 0x3
> >>> ...
> >>> [ 4.346951] psmouse serio1: elantech: Trying to set up SMBus access
> >>> [ 4.346986] psmouse serio1: elantech: SMbus companion is not ready yet
> >>> [ 4.369993] input: ETPS/2 Elantech TrackPoint as /devices/platform/i8042/serio1/input/input7
> >>> [ 4.376200] systemd[1]: bpf-lsm: LSM BPF program attached
> >>> [ 4.385192] input: ETPS/2 Elantech Touchpad as /devices/platform/i8042/serio1/input/input5
> >>> ```
> >>> The change in 92e24e0e57f72e shouldn't affect the elantouch device as elantech_setup_smbus
> >>> initializes `psmouse_smbus_init` with need_deactivate = false.
> >
> > Hmmm. Wondering if I should warm up the compiler again to recheck my
> > result one more time[1].
>
> Just did that. Ran "make clean" and compiled mainline as of now
> (633b47cb009d) and the machine does never resume from s2idle; then I
> reverted 92e24e0e57f7 and compiled again (for completeness: without
> running "make clean" beforehand) and with that kernel s2idle resume
> works perfectly fine.
>
> Wondering if I or the compiler is doing something stupid here -- or if
> we missed some small but important detail somewhere.
>
> Ciao, Thorsten
>
> >>> Did you store dmesg logs from boot without the applied patch?
> >> I intended to ask if you have logs from a boot without 92e24e0e57f72e reverted.
> >
> > https://www.leemhuis.info/files/misc/dmesg-6.6-rc3-vanilla
> >
> >>> If the delay was being applied the timestamps should show the 30ms delay between
> >>> `psmouse serio1: elantech: Trying to set up SMBus access`
> >>> and
> >>> `psmouse serio1: elantech: SMbus companion is not ready yet`
> >
> > Unless I missed something there is not difference. :-/
> >
> > Ciao, Thorsten
> >
> > [1] FWIW, this is my bisect log
> >
> > """
> >> git bisect start
> >> # status: waiting for both good and bad commits
> >> # bad: [6465e260f48790807eef06b583b38ca9789b6072] Linux 6.6-rc3
> >> git bisect bad 6465e260f48790807eef06b583b38ca9789b6072
> >> # status: waiting for good commit(s), bad commit known
> >> # good: [2dde18cd1d8fac735875f2e4987f11817cc0bc2c] Linux 6.5
> >> git bisect good 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> >> # good: [4fb0dacb78c6a041bbd38ddd998df806af5c2c69] Merge tag 'sound-6.6-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
> >> git bisect good 4fb0dacb78c6a041bbd38ddd998df806af5c2c69
> >> # good: [307d59039fb26212a84a9aa6a134a7d2bdea34ca] Merge tag 'media/v6.6-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
> >> git bisect good 307d59039fb26212a84a9aa6a134a7d2bdea34ca
> >> # bad: [4a0fc73da97efd23a383ca839e6fe86410268f6b] Merge tag 's390-6.6-2' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux
> >> git bisect bad 4a0fc73da97efd23a383ca839e6fe86410268f6b
> >> # good: [e4f1b8202fb59c56a3de7642d50326923670513f] Merge tag 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost
> >> git bisect good e4f1b8202fb59c56a3de7642d50326923670513f
> >> # good: [5eea5820c7340d39e56e169e1b87199391105f6b] Merge tag 'mm-stable-2023-09-04-14-00' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> >> git bisect good 5eea5820c7340d39e56e169e1b87199391105f6b
> >> # good: [65d6e954e37872fd9afb5ef3fc0481bb3c2f20f4] Merge tag 'gfs2-v6.5-rc5-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2
> >> git bisect good 65d6e954e37872fd9afb5ef3fc0481bb3c2f20f4
> >> # bad: [744a759492b5c57ff24a6e8aabe47b17ad8ee964] Merge tag 'input-for-v6.6-rc0' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input
> >> git bisect bad 744a759492b5c57ff24a6e8aabe47b17ad8ee964
> >> # good: [dbce1a7d5dce7318d8465b1e0d052ef1d2202237] Input: Explicitly include correct DT includes
> >> git bisect good dbce1a7d5dce7318d8465b1e0d052ef1d2202237
> >> # good: [29057cc5bddc785ea0a11534d7ad2546fa0872d3] Merge tag 'linux-watchdog-6.6-rc1' of git://www.linux-watchdog.org/linux-watchdog
> >> git bisect good 29057cc5bddc785ea0a11534d7ad2546fa0872d3
> >> # bad: [3e4bb047b23375a34dbf5885709ac3729d9cfb22] Input: qt2160 - convert to use devm_* api
> >> git bisect bad 3e4bb047b23375a34dbf5885709ac3729d9cfb22
> >> # good: [e175eae16c1bf92062f1f431a95f476a61a77c48] Input: mcs-touchkey - convert to use devm_* api
> >> git bisect good e175eae16c1bf92062f1f431a95f476a61a77c48
> >> # bad: [92e24e0e57f72e06c2df87116557331fd2d4dda2] Input: psmouse - add delay when deactivating for SMBus mode
> >> git bisect bad 92e24e0e57f72e06c2df87116557331fd2d4dda2
> >> # good: [8362bf82fb5441613aac7c6c9dbb6b83def6ad3b] Input: mcs-touchkey - fix uninitialized use of error in mcs_touchkey_probe()
> >> git bisect good 8362bf82fb5441613aac7c6c9dbb6b83def6ad3b
> >> # first bad commit: [92e24e0e57f72e06c2df87116557331fd2d4dda2] Input: psmouse - add delay when deactivating for SMBus mode
> > """
> >
> >
^ permalink raw reply related
* Re: [PATCH] HID: i2c-hid: fix handling of unpopulated devices
From: Doug Anderson @ 2023-10-02 16:00 UTC (permalink / raw)
To: Johan Hovold
Cc: Johan Hovold, Jiri Kosina, Benjamin Tissoires, linux-input,
linux-kernel, Maxime Ripard, Dmitry Torokhov, LinusW, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:GPIO SUBSYSTEM
In-Reply-To: <ZRrmVN3Rbz9PY8FW@hovoldconsulting.com>
Hi,
On Mon, Oct 2, 2023 at 8:48 AM Johan Hovold <johan@kernel.org> wrote:
>
> > In any case, the fact that there is a shared power rail / shared power
> > sequence is because the hardware designer intended them to either be
> > both off or both on. Whenever I asked the EEs that designed these
> > boards about leaving the touchscreen on while turning the panel power
> > off they always looked at me incredulously and asked why I would ever
> > do that. Although we can work around the hardware by powering the
> > panel in order to allow the touchscreen to be on, it's just not the
> > intention.
>
> I hear you, but users sometimes want do things with their hardware which
> may not have originally been intended (e.g. your kiosk example).
...and they can. I don't think it's totally unreasonable for userspace
in this case to take into account that they need to keep the panel
powered on (maybe with the screen black and the backlight off) if they
want the touchscreen kept on. If I was coding up userspace it wouldn't
surprise me at all if the touchscreen stopped working when the panel
was off.
I will further note that there is actually hardware where it's even
more difficult. On the same sc7180-trogdor laptops (and others as
well) the USB webcam is _also_ powered by the same power rail. When
you power the screen off then the USB webcam deenumerates. When you
power the screen on then it shows back up. It would be really weird if
somehow the USB webcam driver needed a link to the panel to try to
keep it powered.
-Doug
^ permalink raw reply
* [PATCH v2] HID: i2c-hid: fix handling of unpopulated devices
From: Johan Hovold @ 2023-10-02 15:58 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Johan Hovold, Douglas Anderson,
Maxime Ripard
A recent commit reordered probe so that the interrupt line is now
requested before making sure that the device exists.
This breaks machines like the Lenovo ThinkPad X13s which rely on the
HID driver to probe second-source devices and only register the variant
that is actually populated. Specifically, the interrupt line may now
already be (temporarily) claimed when doing asynchronous probing of the
touchpad:
genirq: Flags mismatch irq 191. 00082008 (hid-over-i2c) vs. 00082008 (hid-over-i2c)
i2c_hid_of 21-0015: Could not register for hid-over-i2c interrupt, irq = 191, ret = -16
i2c_hid_of: probe of 21-0015 failed with error -16
Fix this by restoring the old behaviour of first making sure the device
exists before requesting the interrupt line.
Note that something like this should probably be implemented also for
"panel followers", whose actual probe is currently effectively deferred
until the DRM panel is probed (e.g. by powering down the device after
making sure it exists and only then register it as a follower).
Fixes: 675cd877c952 ("HID: i2c-hid: Rearrange probe() to power things up later")
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
Changes in v2
- initialise ihid->is_panel_follower sooner to avoid repeated property
lookups and so that it can be used consistently throughout the driver
for code that differs for "panel followers"
Link to v1: https://lore.kernel.org/lkml/20230918125851.310-1-johan+linaro@kernel.org/
drivers/hid/i2c-hid/i2c-hid-core.c | 144 ++++++++++++++++-------------
1 file changed, 81 insertions(+), 63 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 9601c0605fd9..2735cd585af0 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -998,45 +998,29 @@ static int i2c_hid_core_resume(struct i2c_hid *ihid)
return hid_driver_reset_resume(hid);
}
-/**
- * __do_i2c_hid_core_initial_power_up() - First time power up of the i2c-hid device.
- * @ihid: The ihid object created during probe.
- *
- * This function is called at probe time.
- *
- * The initial power on is where we do some basic validation that the device
- * exists, where we fetch the HID descriptor, and where we create the actual
- * HID devices.
- *
- * Return: 0 or error code.
+/*
+ * Check that the device exists and parse the HID descriptor.
*/
-static int __do_i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
+static int __i2c_hid_core_probe(struct i2c_hid *ihid)
{
struct i2c_client *client = ihid->client;
struct hid_device *hid = ihid->hid;
int ret;
- ret = i2c_hid_core_power_up(ihid);
- if (ret)
- return ret;
-
/* Make sure there is something at this address */
ret = i2c_smbus_read_byte(client);
if (ret < 0) {
i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
- ret = -ENXIO;
- goto err;
+ return -ENXIO;
}
ret = i2c_hid_fetch_hid_descriptor(ihid);
if (ret < 0) {
dev_err(&client->dev,
"Failed to fetch the HID Descriptor\n");
- goto err;
+ return ret;
}
- enable_irq(client->irq);
-
hid->version = le16_to_cpu(ihid->hdesc.bcdVersion);
hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
hid->product = le16_to_cpu(ihid->hdesc.wProductID);
@@ -1050,17 +1034,49 @@ static int __do_i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
+ return 0;
+}
+
+static int i2c_hid_core_register_hid(struct i2c_hid *ihid)
+{
+ struct i2c_client *client = ihid->client;
+ struct hid_device *hid = ihid->hid;
+ int ret;
+
+ enable_irq(client->irq);
+
ret = hid_add_device(hid);
if (ret) {
if (ret != -ENODEV)
hid_err(client, "can't add hid device: %d\n", ret);
- goto err;
+ disable_irq(client->irq);
+ return ret;
}
return 0;
+}
+
+static int i2c_hid_core_probe_panel_follower(struct i2c_hid *ihid)
+{
+ int ret;
+
+ ret = i2c_hid_core_power_up(ihid);
+ if (ret)
+ return ret;
-err:
+ ret = __i2c_hid_core_probe(ihid);
+ if (ret)
+ goto err_power_down;
+
+ ret = i2c_hid_core_register_hid(ihid);
+ if (ret)
+ goto err_power_down;
+
+ return 0;
+
+err_power_down:
i2c_hid_core_power_down(ihid);
+
return ret;
}
@@ -1077,7 +1093,7 @@ static void ihid_core_panel_prepare_work(struct work_struct *work)
* steps.
*/
if (!hid->version)
- ret = __do_i2c_hid_core_initial_power_up(ihid);
+ ret = i2c_hid_core_probe_panel_follower(ihid);
else
ret = i2c_hid_core_resume(ihid);
@@ -1136,7 +1152,6 @@ static int i2c_hid_core_register_panel_follower(struct i2c_hid *ihid)
struct device *dev = &ihid->client->dev;
int ret;
- ihid->is_panel_follower = true;
ihid->panel_follower.funcs = &i2c_hid_core_panel_follower_funcs;
/*
@@ -1156,30 +1171,6 @@ static int i2c_hid_core_register_panel_follower(struct i2c_hid *ihid)
return 0;
}
-static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
-{
- /*
- * If we're a panel follower, we'll register and do our initial power
- * up when the panel turns on; otherwise we do it right away.
- */
- if (drm_is_panel_follower(&ihid->client->dev))
- return i2c_hid_core_register_panel_follower(ihid);
- else
- return __do_i2c_hid_core_initial_power_up(ihid);
-}
-
-static void i2c_hid_core_final_power_down(struct i2c_hid *ihid)
-{
- /*
- * If we're a follower, the act of unfollowing will cause us to be
- * powered down. Otherwise we need to manually do it.
- */
- if (ihid->is_panel_follower)
- drm_panel_remove_follower(&ihid->panel_follower);
- else
- i2c_hid_core_suspend(ihid, true);
-}
-
int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
u16 hid_descriptor_address, u32 quirks)
{
@@ -1211,6 +1202,7 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
ihid->ops = ops;
ihid->client = client;
ihid->wHIDDescRegister = cpu_to_le16(hid_descriptor_address);
+ ihid->is_panel_follower = drm_is_panel_follower(&client->dev);
init_waitqueue_head(&ihid->wait);
mutex_init(&ihid->reset_lock);
@@ -1224,14 +1216,10 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
return ret;
device_enable_async_suspend(&client->dev);
- ret = i2c_hid_init_irq(client);
- if (ret < 0)
- goto err_buffers_allocated;
-
hid = hid_allocate_device();
if (IS_ERR(hid)) {
ret = PTR_ERR(hid);
- goto err_irq;
+ goto err_free_buffers;
}
ihid->hid = hid;
@@ -1242,19 +1230,42 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
hid->bus = BUS_I2C;
hid->initial_quirks = quirks;
- ret = i2c_hid_core_initial_power_up(ihid);
+ /* Power on and probe unless device is a panel follower. */
+ if (!ihid->is_panel_follower) {
+ ret = i2c_hid_core_power_up(ihid);
+ if (ret < 0)
+ goto err_destroy_device;
+
+ ret = __i2c_hid_core_probe(ihid);
+ if (ret < 0)
+ goto err_power_down;
+ }
+
+ ret = i2c_hid_init_irq(client);
+ if (ret < 0)
+ goto err_power_down;
+
+ /*
+ * If we're a panel follower, we'll register when the panel turns on;
+ * otherwise we do it right away.
+ */
+ if (ihid->is_panel_follower)
+ ret = i2c_hid_core_register_panel_follower(ihid);
+ else
+ ret = i2c_hid_core_register_hid(ihid);
if (ret)
- goto err_mem_free;
+ goto err_free_irq;
return 0;
-err_mem_free:
- hid_destroy_device(hid);
-
-err_irq:
+err_free_irq:
free_irq(client->irq, ihid);
-
-err_buffers_allocated:
+err_power_down:
+ if (!ihid->is_panel_follower)
+ i2c_hid_core_power_down(ihid);
+err_destroy_device:
+ hid_destroy_device(hid);
+err_free_buffers:
i2c_hid_free_buffers(ihid);
return ret;
@@ -1266,7 +1277,14 @@ void i2c_hid_core_remove(struct i2c_client *client)
struct i2c_hid *ihid = i2c_get_clientdata(client);
struct hid_device *hid;
- i2c_hid_core_final_power_down(ihid);
+ /*
+ * If we're a follower, the act of unfollowing will cause us to be
+ * powered down. Otherwise we need to manually do it.
+ */
+ if (ihid->is_panel_follower)
+ drm_panel_remove_follower(&ihid->panel_follower);
+ else
+ i2c_hid_core_suspend(ihid, true);
hid = ihid->hid;
hid_destroy_device(hid);
--
2.41.0
^ permalink raw reply related
* Re: [PATCH] HID: i2c-hid: fix handling of unpopulated devices
From: Johan Hovold @ 2023-10-02 15:48 UTC (permalink / raw)
To: Doug Anderson
Cc: Johan Hovold, Jiri Kosina, Benjamin Tissoires, linux-input,
linux-kernel, Maxime Ripard, Dmitry Torokhov, LinusW, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:GPIO SUBSYSTEM
In-Reply-To: <CAD=FV=UHEeu3crTFEZDY+LDQZk07H8un7gCSs0jyCQJrGYkV=Q@mail.gmail.com>
On Mon, Oct 02, 2023 at 07:35:06AM -0700, Doug Anderson wrote:
> On Mon, Oct 2, 2023 at 5:09 AM Johan Hovold <johan@kernel.org> wrote:
> > Out of curiosity, are there any machines that actually need this
> > "panel-follower" API today, or are saying above that this is just
> > something that may be needed one day?
>
> Yes. See commit de0874165b83 ("drm/panel: Add a way for other devices
> to follow panel state") where I point to Cong Yang's original patch
> [1]. In that patch Cong was trying to make things work by assuming
> probe ordering and manually taking some of the power sequencing stuff
> out of some of the drivers in order to get things to work.
>
> [1] https://lore.kernel.org/r/20230519032316.3464732-1-yangcong5@huaqin.corp-partner.google.com
Ok, thanks for the pointer.
> > > > Don't you need to keep the touchscreen powered to support wakeup events
> > > > (e.g. when not closing the lid)?
> > >
> > > No. The only reason you'd use panel follower is if the hardware was
> > > designed such that the touchscreen needed to be power sequenced with
> > > the panel. If the touchscreen can stay powered when the panel is off
> > > then it is, by definition, not a panel follower.
> > >
> > > For a laptop I don't think most people expect the touchscreen to stay
> > > powered when the screen is off. I certainly wouldn't expect it. If the
> > > screen was off and I wanted to interact with the device, I would hit a
> > > key on the keyboard or touch the trackpad. When the people designing
> > > sc7180-trogdor chose to have the display and touchscreen share a power
> > > rail they made a conscious choice that they didn't need the
> > > touchscreen active when the screen was off.
> >
> > Sure, but that's a policy decision and not something that should be
> > hard-coded in our drivers.
>
> If the touchscreen and panel can be powered separately then, sure,
> it's a policy decision.
>
> In the cases where the touchscreen and panel need to be powered
> together I'd say it's more than a policy decision. Even if it wasn't,
> you have to make _some_ decision in the kernel. One could also argue
> that if you say that you're going to force the panel to be powered on
> whenever the touchscreen is on then that's just as much of a policy
> decision, isn't it?
I get your point, but with runtime pm suspending the touchpad after a
timeout it seems that would still be the most flexible alternative
which allows deferring the decision whether to support wakeup on
touch events to the user.
> In any case, the fact that there is a shared power rail / shared power
> sequence is because the hardware designer intended them to either be
> both off or both on. Whenever I asked the EEs that designed these
> boards about leaving the touchscreen on while turning the panel power
> off they always looked at me incredulously and asked why I would ever
> do that. Although we can work around the hardware by powering the
> panel in order to allow the touchscreen to be on, it's just not the
> intention.
I hear you, but users sometimes want do things with their hardware which
may not have originally been intended (e.g. your kiosk example).
> > > > But the main reason is still that requesting resources belongs in
> > > > probe() and should not be deferred to some later random time where you
> > > > cannot inform driver core of failures (e.g. for probe deferral if the
> > > > interrupt controller is not yet available).
> > >
> > > OK, I guess the -EPROBE_DEFER is technically possible though probably
> > > not likely in practice. ...so that's a good reason to make sure we
> > > request the IRQ in probe even in the "panel follower" case. I still
> > > beleive Benjamin would prefer that this was abstracted out and not in
> > > the actual probe() routine, but I guess we can wait to hear from him.
> >
> > I talked to Benjamin at Kernel Recipes last week and I don't think he
> > has any fundamental objections to the fix I'm proposing.
>
> Sure. I don't either though I'm hoping that we can come up with a more
> complete solution long term.
>
>
> > I prefer it as it makes the code easier to reason about and clearly
> > marks the code paths that differ in case the device is a "panel
> > follower". And since you said it also makes the code look more like what
> > you originally intended, then I guess you should be ok with it too?
>
> It looks OK to me. The biggest objection I have is just that I dislike
> it when code churns because two people disagree what the nicer style
> is. It just makes for bigger diffs and more work to review things.
Ok, but this isn't just about style as that initial_power_on() function
which does all the magic needs to be broken up to fix the regression
(unless you want to convolute the driver and defer resource lookups
until panel power-on).
I'll respin a v2 with that panel-property lookup change I mentioned and
hopefully we can get this fixed this week.
> > > One last idea I had while digging would be to wonder if we could
> > > somehow solve this case with "IRQF_PROBE_SHARED". I guess that doesn't
> > > work well together with "IRQF_NO_AUTOEN", but conceivably we could
> > > have the interrupt handler return "IRQ_NONE" if the initial power up
> > > never happened? I haven't spent much time poking with shared
> > > interrupts though, so I don't know if there are other side effects...
> >
> > Yeah, that doesn't seem right, though. The interrupt line is not really
> > shared, it's just that we need to check whether the device is populated
> > before requesting the interrupt.
>
> I'm not convinced that marking it as shared is any "less right" than
> extra work to request the interrupt after we've probed the device.
> Fundamentally both are taking into account that another touchscreen
> might be trying to probe with the same interrupt line.
If you need to start to thinking about rewriting your interrupt handler,
I'd say that qualifies as "less right". ;)
Johan
^ permalink raw reply
* [PATCH] hid: lenovo: Resend all settings on reset_resume for compact keyboards
From: Martin Kepplinger @ 2023-10-02 15:09 UTC (permalink / raw)
To: jikos, benjamin.tissoires, jm, linux-kernel
Cc: linux-input, stable, Martin Kepplinger
From: Jamie Lentin <jm@lentin.co.uk>
The USB Compact Keyboard variant requires a reset_resume function to
restore keyboard configuration after a suspend in some situations. Move
configuration normally done on probe to lenovo_features_set_cptkbd(), then
recycle this for use on reset_resume.
Without, the keyboard and driver would end up in an inconsistent state,
breaking middle-button scrolling amongst other problems, and twiddling
sysfs values wouldn't help as the middle-button mode won't be set until
the driver is reloaded.
Tested on a USB and Bluetooth Thinkpad Compact Keyboard.
CC: stable@vger.kernel.org
Fixes: 94eefa271323 ("HID: lenovo: Use native middle-button mode for compact keyboards")
Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
drivers/hid/hid-lenovo.c | 50 +++++++++++++++++++++++++++-------------
1 file changed, 34 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 44763c0da444..614320bff39f 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -521,6 +521,19 @@ static void lenovo_features_set_cptkbd(struct hid_device *hdev)
int ret;
struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
+ /*
+ * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
+ * regular keys
+ */
+ ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
+ if (ret)
+ hid_warn(hdev, "Failed to switch F7/9/11 mode: %d\n", ret);
+
+ /* Switch middle button to native mode */
+ ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
+ if (ret)
+ hid_warn(hdev, "Failed to switch middle button: %d\n", ret);
+
ret = lenovo_send_cmd_cptkbd(hdev, 0x05, cptkbd_data->fn_lock);
if (ret)
hid_err(hdev, "Fn-lock setting failed: %d\n", ret);
@@ -1126,22 +1139,6 @@ static int lenovo_probe_cptkbd(struct hid_device *hdev)
}
hid_set_drvdata(hdev, cptkbd_data);
- /*
- * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
- * regular keys (Compact only)
- */
- if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD ||
- hdev->product == USB_DEVICE_ID_LENOVO_CBTKBD) {
- ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
- if (ret)
- hid_warn(hdev, "Failed to switch F7/9/11 mode: %d\n", ret);
- }
-
- /* Switch middle button to native mode */
- ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
- if (ret)
- hid_warn(hdev, "Failed to switch middle button: %d\n", ret);
-
/* Set keyboard settings to known state */
cptkbd_data->middlebutton_state = 0;
cptkbd_data->fn_lock = true;
@@ -1264,6 +1261,24 @@ static int lenovo_probe(struct hid_device *hdev,
return ret;
}
+#ifdef CONFIG_PM
+static int lenovo_reset_resume(struct hid_device *hdev)
+{
+ switch (hdev->product) {
+ case USB_DEVICE_ID_LENOVO_CUSBKBD:
+ case USB_DEVICE_ID_LENOVO_TPIIUSBKBD:
+ if (hdev->type == HID_TYPE_USBMOUSE)
+ lenovo_features_set_cptkbd(hdev);
+
+ break;
+ default:
+ break;
+ }
+
+ return 0;
+}
+#endif
+
static void lenovo_remove_tpkbd(struct hid_device *hdev)
{
struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
@@ -1380,6 +1395,9 @@ static struct hid_driver lenovo_driver = {
.raw_event = lenovo_raw_event,
.event = lenovo_event,
.report_fixup = lenovo_report_fixup,
+#ifdef CONFIG_PM
+ .reset_resume = lenovo_reset_resume,
+#endif
};
module_hid_driver(lenovo_driver);
--
2.39.2
^ permalink raw reply related
* Re: [PATCH RFC v4 5/6] ARM: pxa: Convert gumstix Bluetooth to GPIO descriptors
From: Duje Mihanović @ 2023-10-02 14:52 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij, Andy Shevchenko,
Dmitry Torokhov, Mark Brown, linux-arm-kernel, linux-kernel,
linux-usb, linux-gpio, linux-input, linux-spi
In-Reply-To: <CAMRc=MdHv0YxSowMnqJ8xG1_w8dwTWVJV1K0b1jgectPTbOheQ@mail.gmail.com>
On Monday, October 2, 2023 9:42:52 AM CEST Bartosz Golaszewski wrote:
> This changes the way this code works. You release the descriptor here,
> it returns to the driver and can be re-requested by someone else. Its
> value is also not guaranteed to remain as "active". Is this what you
> want?
Good point. Is it enough to not call gpiod_put() at the end or is it necessary
to use a static gpio_desc instead of a local one?
Regards,
Duje
^ permalink raw reply
* Re: [PATCH v2 0/3] selftests/hid: fix building for older kernels
From: Benjamin Tissoires @ 2023-10-02 14:48 UTC (permalink / raw)
To: Justin Stitt
Cc: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Eduard Zingerman,
linux-input, linux-kselftest, linux-kernel, bpf
In-Reply-To: <CAFhGd8pEv32zp4RDsj_jeBjzP5hcsf4dP4Knueiw_UM8ZsqcKw@mail.gmail.com>
On Sep 26 2023, Justin Stitt wrote:
> Hey all,
>
> Gentle ping on this patch. Looking to get this patch and [1] slated
> for 6.7 wherein we can start getting cleaner kselftests builds.
>
> I do not think I am able to successfully run the hid/bpf selftests due
> to my kernel version being too low (and an inability to upgrade it as
> I'm on a corp rolling release). I'd appreciate some insight on how to
> get the tests running or if someone could actually build+run the tests
> with this patch applied.
I wanted to apply this series today, but it failed my own CI now with
the enums being already defined:
https://gitlab.freedesktop.org/bentiss/hid/-/jobs/49754306
I'll probably squash the following patch in 1/3, would you mind giving
it a test?
---
From 37feca6c0e84705ad65e621643206c287b63bb0a Mon Sep 17 00:00:00 2001
From: Benjamin Tissoires <bentiss@kernel.org>
Date: Mon, 2 Oct 2023 15:37:18 +0200
Subject: [PATCH] fix selftests/hid: ensure we can compile the tests on kernels
pre-6.3
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
.../selftests/hid/progs/hid_bpf_helpers.h | 30 ++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
index ab3b18ba48c4..feed5a991e05 100644
--- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
+++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
@@ -5,16 +5,44 @@
#ifndef __HID_BPF_HELPERS_H
#define __HID_BPF_HELPERS_H
-/* "undefine" structs in vmlinux.h, because we "override" them below */
+/* "undefine" structs and enums in vmlinux.h, because we "override" them below */
#define hid_bpf_ctx hid_bpf_ctx___not_used
#define hid_report_type hid_report_type___not_used
#define hid_class_request hid_class_request___not_used
#define hid_bpf_attach_flags hid_bpf_attach_flags___not_used
+#define HID_INPUT_REPORT HID_INPUT_REPORT___not_used
+#define HID_OUTPUT_REPORT HID_OUTPUT_REPORT___not_used
+#define HID_FEATURE_REPORT HID_FEATURE_REPORT___not_used
+#define HID_REPORT_TYPES HID_REPORT_TYPES___not_used
+#define HID_REQ_GET_REPORT HID_REQ_GET_REPORT___not_used
+#define HID_REQ_GET_IDLE HID_REQ_GET_IDLE___not_used
+#define HID_REQ_GET_PROTOCOL HID_REQ_GET_PROTOCOL___not_used
+#define HID_REQ_SET_REPORT HID_REQ_SET_REPORT___not_used
+#define HID_REQ_SET_IDLE HID_REQ_SET_IDLE___not_used
+#define HID_REQ_SET_PROTOCOL HID_REQ_SET_PROTOCOL___not_used
+#define HID_BPF_FLAG_NONE HID_BPF_FLAG_NONE___not_used
+#define HID_BPF_FLAG_INSERT_HEAD HID_BPF_FLAG_INSERT_HEAD·___not_used
+#define HID_BPF_FLAG_MAX HID_BPF_FLAG_MAX___not_used
+
#include "vmlinux.h"
+
#undef hid_bpf_ctx
#undef hid_report_type
#undef hid_class_request
#undef hid_bpf_attach_flags
+#undef HID_INPUT_REPORT
+#undef HID_OUTPUT_REPORT
+#undef HID_FEATURE_REPORT
+#undef HID_REPORT_TYPES
+#undef HID_REQ_GET_REPORT
+#undef HID_REQ_GET_IDLE
+#undef HID_REQ_GET_PROTOCOL
+#undef HID_REQ_SET_REPORT
+#undef HID_REQ_SET_IDLE
+#undef HID_REQ_SET_PROTOCOL
+#undef HID_BPF_FLAG_NONE
+#undef HID_BPF_FLAG_INSERT_HEAD
+#undef HID_BPF_FLAG_MAX
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
--
2.41.0
---
Cheers,
Benjamin
>
> On Sat, Sep 9, 2023 at 7:22 AM Justin Stitt <justinstitt@google.com> wrote:
> >
> > Hi, I am sending this series on behalf of myself and Benjamin Tissoires. There
> > existed an initial n=3 patch series which was later expanded to n=4 and
> > is now back to n=3 with some fixes added in and rebased against
> > mainline.
> >
> > This patch series aims to ensure that the hid/bpf selftests can be built
> > without errors.
> >
> > Here's Benjamin's initial cover letter for context:
> > | These fixes have been triggered by [0]:
> > | basically, if you do not recompile the kernel first, and are
> > | running on an old kernel, vmlinux.h doesn't have the required
> > | symbols and the compilation fails.
> > |
> > | The tests will fail if you run them on that very same machine,
> > | of course, but the binary should compile.
> > |
> > | And while I was sorting out why it was failing, I realized I
> > | could do a couple of improvements on the Makefile.
> > |
> > | [0] https://lore.kernel.org/linux-input/56ba8125-2c6f-a9c9-d498-0ca1c153dcb2@redhat.com/T/#t
> >
> > Changes from v1 -> v2:
> > - roll Justin's fix into patch 1/3
> > - add __attribute__((preserve_access_index)) (thanks Eduard)
> > - rebased onto mainline (2dde18cd1d8fac735875f2e4987f11817cc0bc2c)
> > - Link to v1: https://lore.kernel.org/all/20230825-wip-selftests-v1-0-c862769020a8@kernel.org/
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1698
> > Link: https://github.com/ClangBuiltLinux/continuous-integration2/issues/61
> > ---
> > Benjamin Tissoires (3):
> > selftests/hid: ensure we can compile the tests on kernels pre-6.3
> > selftests/hid: do not manually call headers_install
> > selftests/hid: force using our compiled libbpf headers
> >
> > tools/testing/selftests/hid/Makefile | 10 ++---
> > tools/testing/selftests/hid/progs/hid.c | 3 --
> > .../testing/selftests/hid/progs/hid_bpf_helpers.h | 49 ++++++++++++++++++++++
> > 3 files changed, 53 insertions(+), 9 deletions(-)
> > ---
> > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > change-id: 20230908-kselftest-09-08-56d7f4a8d5c4
> >
> > Best regards,
> > --
> > Justin Stitt <justinstitt@google.com>
> >
>
> [1]: https://lore.kernel.org/all/20230912-kselftest-param_test-c-v1-1-80a6cffc7374@google.com/
>
> Thanks
> Justin
^ permalink raw reply related
* Re: [PATCH] HID: i2c-hid: fix handling of unpopulated devices
From: Doug Anderson @ 2023-10-02 14:35 UTC (permalink / raw)
To: Johan Hovold
Cc: Johan Hovold, Jiri Kosina, Benjamin Tissoires, linux-input,
linux-kernel, Maxime Ripard, Dmitry Torokhov, LinusW, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:GPIO SUBSYSTEM
In-Reply-To: <ZRqzGA1F6JV-mlRL@hovoldconsulting.com>
Hi,
On Mon, Oct 2, 2023 at 5:09 AM Johan Hovold <johan@kernel.org> wrote:
>
> > > > > I skimmed the thread were you added this, but I'm not sure I saw any
> > > > > reason for why powering on the panel follower temporarily during probe
> > > > > would not work?
> > > >
> > > > My first instinct says we can't do this, but let's think about it...
> > > >
> > > > In general the "panel follower" API is designed to give all the
> > > > decision making about when to power things on and off to the panel
> > > > driver, which is controlled by DRM.
> > > >
> > > > The reason for this is from experience I had when dealing with the
> > > > Samsung ATNA33XC20 panel that's on "sc7180-trogdor-homestar". The TCON
> > > > on that panel tended to die if you didn't sequence it just right.
> > > > Specifically, if you were sending pixels to the panel and then stopped
> > > > then you absolutely needed to power the panel off and on again. Folks
> > > > I talked to even claimed that the panel was working "to spec" since,
> > > > in the "Power Sequencing" section of the eDP spec it clearly shows
> > > > that you _must_ turn the panel off and on again after you stop giving
> > > > it bits. ...this is despite the fact that no other panel I've worked
> > > > with cares. ;-)
> > > >
> > > > On homestar, since we didn't have the "panel follower" API, we ended
> > > > up adding cost to the hardware and putting the panel and touchscreens
> > > > on different power rails. However, I wanted to make sure that if we
> > > > ran into a similar situation in the future (or maybe if we were trying
> > > > to make hardware work that we didn't have control over) that we could
> > > > solve it.
>
> Out of curiosity, are there any machines that actually need this
> "panel-follower" API today, or are saying above that this is just
> something that may be needed one day?
Yes. See commit de0874165b83 ("drm/panel: Add a way for other devices
to follow panel state") where I point to Cong Yang's original patch
[1]. In that patch Cong was trying to make things work by assuming
probe ordering and manually taking some of the power sequencing stuff
out of some of the drivers in order to get things to work.
[1] https://lore.kernel.org/r/20230519032316.3464732-1-yangcong5@huaqin.corp-partner.google.com
> > > > The other reason for giving full control to the panel driver is just
> > > > how userspace usually works. Right now userspace tends to power off
> > > > panels if they're not used (like if a lid is closed on a laptop) but
> > > > doesn't necessarily power off the touchscreen. Thus if the touchscreen
> > > > has the ability to keep things powered on then we'd never get to a low
> > > > power state.
> > >
> > > Don't you need to keep the touchscreen powered to support wakeup events
> > > (e.g. when not closing the lid)?
> >
> > No. The only reason you'd use panel follower is if the hardware was
> > designed such that the touchscreen needed to be power sequenced with
> > the panel. If the touchscreen can stay powered when the panel is off
> > then it is, by definition, not a panel follower.
> >
> > For a laptop I don't think most people expect the touchscreen to stay
> > powered when the screen is off. I certainly wouldn't expect it. If the
> > screen was off and I wanted to interact with the device, I would hit a
> > key on the keyboard or touch the trackpad. When the people designing
> > sc7180-trogdor chose to have the display and touchscreen share a power
> > rail they made a conscious choice that they didn't need the
> > touchscreen active when the screen was off.
>
> Sure, but that's a policy decision and not something that should be
> hard-coded in our drivers.
If the touchscreen and panel can be powered separately then, sure,
it's a policy decision.
In the cases where the touchscreen and panel need to be powered
together I'd say it's more than a policy decision. Even if it wasn't,
you have to make _some_ decision in the kernel. One could also argue
that if you say that you're going to force the panel to be powered on
whenever the touchscreen is on then that's just as much of a policy
decision, isn't it?
In any case, the fact that there is a shared power rail / shared power
sequence is because the hardware designer intended them to either be
both off or both on. Whenever I asked the EEs that designed these
boards about leaving the touchscreen on while turning the panel power
off they always looked at me incredulously and asked why I would ever
do that. Although we can work around the hardware by powering the
panel in order to allow the touchscreen to be on, it's just not the
intention.
> > > But the main reason is still that requesting resources belongs in
> > > probe() and should not be deferred to some later random time where you
> > > cannot inform driver core of failures (e.g. for probe deferral if the
> > > interrupt controller is not yet available).
> >
> > OK, I guess the -EPROBE_DEFER is technically possible though probably
> > not likely in practice. ...so that's a good reason to make sure we
> > request the IRQ in probe even in the "panel follower" case. I still
> > beleive Benjamin would prefer that this was abstracted out and not in
> > the actual probe() routine, but I guess we can wait to hear from him.
>
> I talked to Benjamin at Kernel Recipes last week and I don't think he
> has any fundamental objections to the fix I'm proposing.
Sure. I don't either though I'm hoping that we can come up with a more
complete solution long term.
> I prefer it as it makes the code easier to reason about and clearly
> marks the code paths that differ in case the device is a "panel
> follower". And since you said it also makes the code look more like what
> you originally intended, then I guess you should be ok with it too?
It looks OK to me. The biggest objection I have is just that I dislike
it when code churns because two people disagree what the nicer style
is. It just makes for bigger diffs and more work to review things.
> > One last idea I had while digging would be to wonder if we could
> > somehow solve this case with "IRQF_PROBE_SHARED". I guess that doesn't
> > work well together with "IRQF_NO_AUTOEN", but conceivably we could
> > have the interrupt handler return "IRQ_NONE" if the initial power up
> > never happened? I haven't spent much time poking with shared
> > interrupts though, so I don't know if there are other side effects...
>
> Yeah, that doesn't seem right, though. The interrupt line is not really
> shared, it's just that we need to check whether the device is populated
> before requesting the interrupt.
I'm not convinced that marking it as shared is any "less right" than
extra work to request the interrupt after we've probed the device.
Fundamentally both are taking into account that another touchscreen
might be trying to probe with the same interrupt line.
-Doug
^ permalink raw reply
* Re: [PATCH] dt-bindings: input: syna,rmi4: Make "additionalProperties: true" explicit
From: Conor Dooley @ 2023-10-02 12:33 UTC (permalink / raw)
To: Rob Herring
Cc: Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley,
Jason A. Donenfeld, Matthias Schiffer, Vincent Huang, linux-input,
devicetree, linux-kernel
In-Reply-To: <20230926144249.4053202-1-robh@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]
On Tue, Sep 26, 2023 at 09:42:44AM -0500, Rob Herring wrote:
> Make it explicit that the not yet documented child nodes have additional
> properties and the child node schema is not complete.
Guess I missed this patch while reviewing the other additional property
stuff. Seems like an odd binding with the "Other functions, not
documented yet", but seems to be a relic of the original text bindings?
Patch seems reasonable in that context..
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Thanks,
Conor.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Documentation/devicetree/bindings/input/syna,rmi4.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/syna,rmi4.yaml b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
> index 4d4e1a8e36be..b522c8d3ce0d 100644
> --- a/Documentation/devicetree/bindings/input/syna,rmi4.yaml
> +++ b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
> @@ -164,6 +164,8 @@ patternProperties:
>
> "^rmi4-f[0-9a-f]+@[0-9a-f]+$":
> type: object
> + additionalProperties: true
> +
> description:
> Other functions, not documented yet.
>
> --
> 2.40.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid: fix handling of unpopulated devices
From: Johan Hovold @ 2023-10-02 12:10 UTC (permalink / raw)
To: Doug Anderson
Cc: Johan Hovold, Jiri Kosina, Benjamin Tissoires, linux-input,
linux-kernel, Maxime Ripard, Dmitry Torokhov, LinusW, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:GPIO SUBSYSTEM
In-Reply-To: <CAD=FV=USBJRzqxX9kBP8pp4LKRGpBee+jkHL=KmeQvyfBk2CVQ@mail.gmail.com>
On Fri, Sep 22, 2023 at 09:37:43AM -0700, Doug Anderson wrote:
> On Fri, Sep 22, 2023 at 2:08 AM Johan Hovold <johan@kernel.org> wrote:
> > On Wed, Sep 20, 2023 at 08:41:12AM -0700, Doug Anderson wrote:
> > > On Wed, Sep 20, 2023 at 12:26 AM Johan Hovold <johan@kernel.org> wrote:
> > > > On Tue, Sep 19, 2023 at 11:15:46AM -0700, Doug Anderson wrote:
> > > > > On Tue, Sep 19, 2023 at 12:07 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > > > As I alluded to in the commit message, you probably want to be able to
> > > > support second-source touchscreen panel followers as well at some point
> > > > and then deferring checking whether device is populated until the panel
> > > > is powered on is not going to work.
> > > > I skimmed the thread were you added this, but I'm not sure I saw any
> > > > reason for why powering on the panel follower temporarily during probe
> > > > would not work?
> > >
> > > My first instinct says we can't do this, but let's think about it...
> > >
> > > In general the "panel follower" API is designed to give all the
> > > decision making about when to power things on and off to the panel
> > > driver, which is controlled by DRM.
> > >
> > > The reason for this is from experience I had when dealing with the
> > > Samsung ATNA33XC20 panel that's on "sc7180-trogdor-homestar". The TCON
> > > on that panel tended to die if you didn't sequence it just right.
> > > Specifically, if you were sending pixels to the panel and then stopped
> > > then you absolutely needed to power the panel off and on again. Folks
> > > I talked to even claimed that the panel was working "to spec" since,
> > > in the "Power Sequencing" section of the eDP spec it clearly shows
> > > that you _must_ turn the panel off and on again after you stop giving
> > > it bits. ...this is despite the fact that no other panel I've worked
> > > with cares. ;-)
> > >
> > > On homestar, since we didn't have the "panel follower" API, we ended
> > > up adding cost to the hardware and putting the panel and touchscreens
> > > on different power rails. However, I wanted to make sure that if we
> > > ran into a similar situation in the future (or maybe if we were trying
> > > to make hardware work that we didn't have control over) that we could
> > > solve it.
Out of curiosity, are there any machines that actually need this
"panel-follower" API today, or are saying above that this is just
something that may be needed one day?
> > > The other reason for giving full control to the panel driver is just
> > > how userspace usually works. Right now userspace tends to power off
> > > panels if they're not used (like if a lid is closed on a laptop) but
> > > doesn't necessarily power off the touchscreen. Thus if the touchscreen
> > > has the ability to keep things powered on then we'd never get to a low
> > > power state.
> >
> > Don't you need to keep the touchscreen powered to support wakeup events
> > (e.g. when not closing the lid)?
>
> No. The only reason you'd use panel follower is if the hardware was
> designed such that the touchscreen needed to be power sequenced with
> the panel. If the touchscreen can stay powered when the panel is off
> then it is, by definition, not a panel follower.
>
> For a laptop I don't think most people expect the touchscreen to stay
> powered when the screen is off. I certainly wouldn't expect it. If the
> screen was off and I wanted to interact with the device, I would hit a
> key on the keyboard or touch the trackpad. When the people designing
> sc7180-trogdor chose to have the display and touchscreen share a power
> rail they made a conscious choice that they didn't need the
> touchscreen active when the screen was off.
Sure, but that's a policy decision and not something that should be
hard-coded in our drivers.
> For the other hardware I'm aware of that needs panel-follower there is
> a single external chip on the board that handles driving the panel and
> the touchscreen. The power sequencing requirements for this chip
> simply don't allow the touchscreen to be powered on while the display
> is off.
>
> One use case where I could intuitively think I might touch a
> touchscreen of a screen that was "off" would be a kiosk of some sort.
> It would make sense there to have two power rails. ...or, I suppose,
> userspace could just choose to turn the backlight off but keep the
> screen (and touchscreen) powered.
Right.
> > And if you close the lid with wakeup disabled, you should still be able
> > to power down the touchscreen as part of suspend, right?
> >
> > > The above all explains why panel followers like the touchscreen
> > > shouldn't be able to keep power on. However, you are specifically
> > > suggesting that we just turn the power on temporarily during probe. As
> > > I think about that, it might be possible? I guess you'd have to
> > > temporarily block DRM from changing the state of the panel while the
> > > touchscreen is probing. Then if the panel was off then you'd turn it
> > > on briefly, do your probe, and then turn it off again. If the panel
> > > was on then by blocking DRM you'd ensure that it stayed on. I'm not
> > > sure how palatable that would be or if there are any other tricky
> > > parts I'm not thinking about.
> >
> > As this would allow actually probing the touchscreen during probe(), as
> > the driver model expects, this seems like a better approach then
> > deferring probe and registration if it's at all doable.
>
> Yeah, I don't 100% know if it's doable but it seems possible.
> Certainly it's something for future investigation.
>
> Luckily, at the moment anything I'm aware of that truly needs panel
> follower also doesn't have multiple sources for a touchscreen.
Ok, so with the current panel-follower implementation you essentially
only waste a bit of memory in case of a non-populated touchscreen (e.g.
by keeping the platform and follower devices registered).
> > > > > Thinking that way, is there any reason you can't just move the
> > > > > i2c_hid_init_irq() into __do_i2c_hid_core_initial_power_up()? You
> > > > > could replace the call to enable_irq() with it and then remove the
> > > > > `IRQF_NO_AUTOEN` flag? I think that would also solve the issue if you
> > > > > wanted to use a 2nd source + the panel follower concept? Both devices
> > > > > would probe, but only one of them would actually grab the interrupt
> > > > > and only one of them would actually create real HID devices. We might
> > > > > need to do some work to keep from trying again at every poweron of the
> > > > > panel, but it would probably be workable? I think this would also be a
> > > > > smaller change...
> > > >
> > > > That was my first idea as well, but conceptually it is more correct to
> > > > request resources at probe time and not at some later point when you can
> > > > no longer fail probe.
> > > >
> > > > You'd also need to handle the fact that the interrupt may never have
> > > > been requested when remove() is called, which adds unnecessary
> > > > complexity.
> > >
> > > I don't think it's a lot of complexity, is it? Just an extra "if" statement...
> >
> > Well you'd need keep track of whether the interrupt has been requested
> > or not (and manage serialisation) yourself for a start.
>
> Sure. So I guess an "if" test plus a boolean state variable. I still
> don't think it's a lot of complexity.
I never said "a lot", I used the word "unnecessary". But how much it
adds also depends on whether you need additional synchronisation.
But again, the main point is that the "panel-follower" feature should
not complicate and obfuscate the driver's probe implementation. And
looking up resources belongs in probe().
> > But the main reason is still that requesting resources belongs in
> > probe() and should not be deferred to some later random time where you
> > cannot inform driver core of failures (e.g. for probe deferral if the
> > interrupt controller is not yet available).
>
> OK, I guess the -EPROBE_DEFER is technically possible though probably
> not likely in practice. ...so that's a good reason to make sure we
> request the IRQ in probe even in the "panel follower" case. I still
> beleive Benjamin would prefer that this was abstracted out and not in
> the actual probe() routine, but I guess we can wait to hear from him.
I talked to Benjamin at Kernel Recipes last week and I don't think he
has any fundamental objections to the fix I'm proposing.
I prefer it as it makes the code easier to reason about and clearly
marks the code paths that differ in case the device is a "panel
follower". And since you said it also makes the code look more like what
you originally intended, then I guess you should be ok with it too?
Looking at the patch again, I may do a v2 to only look up the "panel"
property once even if that's a really minor optimisation.
> One last idea I had while digging would be to wonder if we could
> somehow solve this case with "IRQF_PROBE_SHARED". I guess that doesn't
> work well together with "IRQF_NO_AUTOEN", but conceivably we could
> have the interrupt handler return "IRQ_NONE" if the initial power up
> never happened? I haven't spent much time poking with shared
> interrupts though, so I don't know if there are other side effects...
Yeah, that doesn't seem right, though. The interrupt line is not really
shared, it's just that we need to check whether the device is populated
before requesting the interrupt.
Johan
^ permalink raw reply
* Re: [PATCH RFC v4 6/6] input: ads7846: Move wait_for_sync() logic to driver
From: Mark Brown @ 2023-10-02 11:39 UTC (permalink / raw)
To: Duje Mihanović
Cc: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij,
Bartosz Golaszewski, Andy Shevchenko, Dmitry Torokhov,
linux-arm-kernel, linux-kernel, linux-usb, linux-gpio,
linux-input, linux-spi
In-Reply-To: <20231001-pxa-gpio-v4-6-0f3b975e6ed5@skole.hr>
[-- Attachment #1: Type: text/plain, Size: 308 bytes --]
On Sun, Oct 01, 2023 at 04:12:57PM +0200, Duje Mihanović wrote:
> If this code is left in the board file, the sync GPIO would have to be
> separated into another lookup table during conversion to the GPIO
> descriptor API (which is also done in this patch).
Acked-by: Mark Brown <broonie@kernel.org>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH V2 1/2] dt-bindings: input: Introduce Himax HID-over-SPI device
From: yang tylor @ 2023-10-02 10:44 UTC (permalink / raw)
To: Conor Dooley
Cc: Conor Dooley, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, jikos, benjamin.tissoires, linux-input, devicetree,
linux-kernel, poyuan_chang, hbarnor, jingyliang@chromium.org,
wuxy23, luolm1, hung poyu
In-Reply-To: <20230928-spectacle-civic-339c0d71d8d7@spud>
On Fri, Sep 29, 2023 at 12:56 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Sep 28, 2023 at 10:12:41AM +0800, yang tylor wrote:
> > On Tue, Sep 26, 2023 at 8:53 PM Conor Dooley <conor@kernel.org> wrote:
> > > On Tue, Sep 26, 2023 at 05:52:39PM +0800, yang tylor wrote:
> > > > On Tue, Sep 26, 2023 at 5:02 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > On Mon, Sep 25, 2023 at 06:16:29PM +0800, yang tylor wrote:
> > > > > > On Mon, Sep 25, 2023 at 4:41 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> > > > > > We have a default prefix firmware name(like himax_xxxx.bin) in the driver code.
> > > > >
> > > > > How do you intend generating the name of the firmware file? I assume the
> > > > > same firmware doesn't work on every IC, so you'll need to pick a
> > > > > different one depending on the compatible?
> > > > >
> > > > If considering a firmware library line-up for all the incoming panels
> > > > of this driver.
> > > > We would use PID as part of the file name. Because all the support panels would
> > > > have a unique PID associated. Which will make the firmware name like
> > > > himax_xxx_{$PID}.bin. The problem is, we need to know PID before firmware load
> > > > at no flash condition. Thus PID information is required in dts when
> > > > no-flash-flag
> > > > is specified.
> > >
> > > Firstly, where does the "xxx" come from?
> > > And you're making it sound more like having firmware-name is suitable
> > > for this use case, given you need to determine the name of the file to
> > > use based on something that is hardware specific but is not
> > > dynamically detectable.
> > Current driver patch uses a prefix name "himax_i2chid" which comes
> > from the previous project
> > and seems not suitable for this condition, so I use "xxx" and plan to
> > replace it in the next version.
> > For finding firmware, I think both solutions are reasonable.
> > - provide firmware name directly: implies no-flash and use user
> > specified firmware, no PID info.
> > - provide no-flash-flag and PID info: loading firmware from organized
> > names with PID info.
> > I prefer the 2nd solution, but it needs more properties in dts. 1st
> > has less properties and more
> > intuitive.
> >
> > I don't know which one is more acceptable by the community, as you
> > know I'm a newbie here.
>
> To be honest, I am not all that sure either! Does the panel id have
> value in its own right, or is that only used to determine the firmware
> filename?
Currently, PID stands for Panel/Project ID and is used for determining
the firmware filename only. We haven't come up with any new attribute that
may attach to it. The differences between panels are handled in firmware
dedicated to its PID.
> Also, if it does have value in its own right, rather than a "pid",
> should the panel be a child node of this hid device with its own
> compatible?
It may need a child node if we find it necessary to add attributes to each PID.
But currently we have no idea about it.
Thanks,
Tylor
^ permalink raw reply
* Re: [PATCH v7 3/4] Input: goodix-berlin - add I2C support for Goodix Berlin Touchscreen IC
From: kernel test robot @ 2023-10-02 10:04 UTC (permalink / raw)
To: Neil Armstrong, Dmitry Torokhov
Cc: oe-kbuild-all, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy,
linux-input, linux-arm-msm, devicetree, linux-kernel,
Neil Armstrong
In-Reply-To: <20231002-topic-goodix-berlin-upstream-initial-v7-3-792fb91f5e88@linaro.org>
Hi Neil,
kernel test robot noticed the following build errors:
[auto build test ERROR on 6465e260f48790807eef06b583b38ca9789b6072]
url: https://github.com/intel-lab-lkp/linux/commits/Neil-Armstrong/dt-bindings-input-document-Goodix-Berlin-Touchscreen-IC/20231002-145648
base: 6465e260f48790807eef06b583b38ca9789b6072
patch link: https://lore.kernel.org/r/20231002-topic-goodix-berlin-upstream-initial-v7-3-792fb91f5e88%40linaro.org
patch subject: [PATCH v7 3/4] Input: goodix-berlin - add I2C support for Goodix Berlin Touchscreen IC
config: nios2-allmodconfig (https://download.01.org/0day-ci/archive/20231002/202310021730.epucKAC1-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231002/202310021730.epucKAC1-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310021730.epucKAC1-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
drivers/input/touchscreen/goodix_berlin_core.c: In function 'goodix_berlin_checksum_valid':
>> drivers/input/touchscreen/goodix_berlin_core.c:50:16: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
50 | return FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK, cal_checksum) == r_checksum;
| ^~~~~~~~~
drivers/input/touchscreen/goodix_berlin_core.c: In function 'goodix_berlin_get_ic_info':
>> drivers/input/touchscreen/goodix_berlin_core.c:284:1: warning: the frame size of 1140 bytes is larger than 1024 bytes [-Wframe-larger-than=]
284 | }
| ^
cc1: some warnings being treated as errors
vim +/FIELD_GET +50 drivers/input/touchscreen/goodix_berlin_core.c
3fd649a6bbd95d Neil Armstrong 2023-10-02 15
3fd649a6bbd95d Neil Armstrong 2023-10-02 16 /*
3fd649a6bbd95d Neil Armstrong 2023-10-02 17 * Goodix "Berlin" Touchscreen ID driver
3fd649a6bbd95d Neil Armstrong 2023-10-02 18 *
3fd649a6bbd95d Neil Armstrong 2023-10-02 19 * This driver is distinct from goodix.c since hardware interface
3fd649a6bbd95d Neil Armstrong 2023-10-02 20 * is different enough to require a new driver.
3fd649a6bbd95d Neil Armstrong 2023-10-02 21 * None of the register address or data structure are close enough
3fd649a6bbd95d Neil Armstrong 2023-10-02 22 * to the previous generations.
3fd649a6bbd95d Neil Armstrong 2023-10-02 23 *
3fd649a6bbd95d Neil Armstrong 2023-10-02 24 * Currently only handles Multitouch events with already
3fd649a6bbd95d Neil Armstrong 2023-10-02 25 * programmed firmware and "config" for "Revision D" Berlin IC.
3fd649a6bbd95d Neil Armstrong 2023-10-02 26 *
3fd649a6bbd95d Neil Armstrong 2023-10-02 27 * Support is missing for:
3fd649a6bbd95d Neil Armstrong 2023-10-02 28 * - ESD Management
3fd649a6bbd95d Neil Armstrong 2023-10-02 29 * - Firmware update/flashing
3fd649a6bbd95d Neil Armstrong 2023-10-02 30 * - "Config" update/flashing
3fd649a6bbd95d Neil Armstrong 2023-10-02 31 * - Stylus Events
3fd649a6bbd95d Neil Armstrong 2023-10-02 32 * - Gesture Events
3fd649a6bbd95d Neil Armstrong 2023-10-02 33 * - Support for older revisions (A & B)
3fd649a6bbd95d Neil Armstrong 2023-10-02 34 */
3fd649a6bbd95d Neil Armstrong 2023-10-02 35
3fd649a6bbd95d Neil Armstrong 2023-10-02 36 static bool goodix_berlin_checksum_valid(const u8 *data, int size)
3fd649a6bbd95d Neil Armstrong 2023-10-02 37 {
3fd649a6bbd95d Neil Armstrong 2023-10-02 38 u32 cal_checksum = 0;
3fd649a6bbd95d Neil Armstrong 2023-10-02 39 u16 r_checksum;
3fd649a6bbd95d Neil Armstrong 2023-10-02 40 u32 i;
3fd649a6bbd95d Neil Armstrong 2023-10-02 41
3fd649a6bbd95d Neil Armstrong 2023-10-02 42 if (size < GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
3fd649a6bbd95d Neil Armstrong 2023-10-02 43 return false;
3fd649a6bbd95d Neil Armstrong 2023-10-02 44
3fd649a6bbd95d Neil Armstrong 2023-10-02 45 for (i = 0; i < size - GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE; i++)
3fd649a6bbd95d Neil Armstrong 2023-10-02 46 cal_checksum += data[i];
3fd649a6bbd95d Neil Armstrong 2023-10-02 47
3fd649a6bbd95d Neil Armstrong 2023-10-02 48 r_checksum = get_unaligned_le16(&data[i]);
3fd649a6bbd95d Neil Armstrong 2023-10-02 49
3fd649a6bbd95d Neil Armstrong 2023-10-02 @50 return FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK, cal_checksum) == r_checksum;
3fd649a6bbd95d Neil Armstrong 2023-10-02 51 }
3fd649a6bbd95d Neil Armstrong 2023-10-02 52
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 06/15] platform/x86/amd/pmf: Add support to get inputs from other subsystems
From: Ilpo Järvinen @ 2023-10-02 9:10 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <59b3db5f-3dc8-4eca-84ce-983774b984d4@amd.com>
[-- Attachment #1: Type: text/plain, Size: 8828 bytes --]
On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> On 9/26/2023 10:38 PM, Ilpo Järvinen wrote:
> > On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> >
> >> PMF driver sends changing inputs from each subystem to TA for evaluating
> >> the conditions in the policy binary.
> >>
> >> Add initial support of plumbing in the PMF driver for Smart PC to get
> >> information from other subsystems in the kernel.
> >>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> >> drivers/platform/x86/amd/pmf/Makefile | 2 +-
> >> drivers/platform/x86/amd/pmf/pmf.h | 18 ++++
> >> drivers/platform/x86/amd/pmf/spc.c | 118 ++++++++++++++++++++++++++
> >> drivers/platform/x86/amd/pmf/tee-if.c | 3 +
> >> 4 files changed, 140 insertions(+), 1 deletion(-)
> >> create mode 100644 drivers/platform/x86/amd/pmf/spc.c
> >>
> >> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
> >> index d2746ee7369f..6b26e48ce8ad 100644
> >> --- a/drivers/platform/x86/amd/pmf/Makefile
> >> +++ b/drivers/platform/x86/amd/pmf/Makefile
> >> @@ -7,4 +7,4 @@
> >> obj-$(CONFIG_AMD_PMF) += amd-pmf.o
> >> amd-pmf-objs := core.o acpi.o sps.o \
> >> auto-mode.o cnqf.o \
> >> - tee-if.o
> >> + tee-if.o spc.o
> >> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> >> index 81acf2a37366..e64b4d285624 100644
> >> --- a/drivers/platform/x86/amd/pmf/pmf.h
> >> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> >> @@ -146,6 +146,21 @@ struct smu_pmf_metrics {
> >> u16 infra_gfx_maxfreq; /* in MHz */
> >> u16 skin_temp; /* in centi-Celsius */
> >> u16 device_state;
> >> + u16 curtemp; /* in centi-Celsius */
> >> + u16 filter_alpha_value;
> >> + u16 avg_gfx_clkfrequency;
> >> + u16 avg_fclk_frequency;
> >> + u16 avg_gfx_activity;
> >> + u16 avg_socclk_frequency;
> >> + u16 avg_vclk_frequency;
> >> + u16 avg_vcn_activity;
> >> + u16 avg_dram_reads;
> >> + u16 avg_dram_writes;
> >> + u16 avg_socket_power;
> >> + u16 avg_core_power[2];
> >> + u16 avg_core_c0residency[16];
> >> + u16 spare1;
> >> + u32 metrics_counter;
> >> } __packed;
> >>
> >> enum amd_stt_skin_temp {
> >> @@ -592,4 +607,7 @@ extern const struct attribute_group cnqf_feature_attribute_group;
> >> int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
> >> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
> >> int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
> >> +
> >> +/* Smart PC - TA interfaces */
> >> +void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
> >> #endif /* PMF_H */
> >> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> >> new file mode 100644
> >> index 000000000000..08159cd5f853
> >> --- /dev/null
> >> +++ b/drivers/platform/x86/amd/pmf/spc.c
> >> @@ -0,0 +1,118 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * AMD Platform Management Framework Driver - Smart PC Capabilities
> >> + *
> >> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
> >> + * All Rights Reserved.
> >> + *
> >> + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> + * Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >> + */
> >> +
> >> +#include <acpi/button.h>
> >> +#include <linux/power_supply.h>
> >> +#include "pmf.h"
> >> +
> >> +static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> >> +{
> >> + u16 max, avg = 0;
> >> + int i;
> >> +
> >> + memset(dev->buf, 0, sizeof(dev->m_table));
> >> + amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
> >> + memcpy(&dev->m_table, dev->buf, sizeof(dev->m_table));
> >> +
> >> + in->ev_info.socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
> >> + in->ev_info.skin_temperature = dev->m_table.skin_temp;
> >> +
> >> + /* get the avg C0 residency of all the cores */
> >> + for (i = 0; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++)
> >> + avg += dev->m_table.avg_core_c0residency[i];
> >
> > Is this safe from overflow?
>
> Yes I think. Can you elaborate a bit more please if there a overflow
> and I am missing it?
You're adding 16 * u16 together into a single u16 here. If it overflows,
the average of averages calculated below will be pseudo-garbage. (I don't
know what's the maximum value in the field so it could be safe).
> Thanks,
> Shyam
>
> >
> >> +
> >> + /* get the max C0 residency of all the cores */
> >> + max = dev->m_table.avg_core_c0residency[0];
> >> + for (i = 1; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++) {
> >> + if (dev->m_table.avg_core_c0residency[i] > max)
> >> + max = dev->m_table.avg_core_c0residency[i];
Why this is done in the different loop? AFAICT, it could be in the same
loop as avg calculation above. (I know you start from = 1 but it won't
change the result when starting with 0 index).
> >> + }
> >> +
> >> + in->ev_info.avg_c0residency = avg / ARRAY_SIZE(dev->m_table.avg_core_c0residency);
--
i.
> >> + in->ev_info.max_c0residency = max;
> >> + in->ev_info.gfx_busy = dev->m_table.avg_gfx_activity;
> >> +}
> >> +
> >> +static const char * const pmf_battery_supply_name[] = {
> >> + "BATT",
> >> + "BAT0",
> >> +};
> >> +
> >> +static int get_battery_prop(enum power_supply_property prop)
> >> +{
> >> + union power_supply_propval value;
> >> + struct power_supply *psy;
> >> + int i, ret = -EINVAL;
> >> +
> >> + for (i = 0; i < ARRAY_SIZE(pmf_battery_supply_name); i++) {
> >> + psy = power_supply_get_by_name(pmf_battery_supply_name[i]);
> >> + if (!psy)
> >> + continue;
> >> +
> >> + ret = power_supply_get_property(psy, prop, &value);
> >> + if (ret) {
> >> + power_supply_put(psy);
> >> + return ret;
> >> + }
> >> + }
> >> +
> >> + return value.intval;
> >> +}
> >> +
> >> +static int amd_pmf_get_battery_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> >> +{
> >> + int val;
> >> +
> >> + val = get_battery_prop(POWER_SUPPLY_PROP_PRESENT);
> >> + if (val != 1)
> >> + return -EINVAL;
> >> +
> >> + in->ev_info.bat_percentage = get_battery_prop(POWER_SUPPLY_PROP_CAPACITY);
> >> + /* all values in mWh metrics */
> >> + in->ev_info.bat_design = get_battery_prop(POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN) / 1000;
> >> + in->ev_info.full_charge_capacity = get_battery_prop(POWER_SUPPLY_PROP_ENERGY_FULL) / 1000;
> >> + in->ev_info.drain_rate = get_battery_prop(POWER_SUPPLY_PROP_POWER_NOW) / 1000;
> >
> > You don't need literal, use the defines provided in linux/units.h.
> >
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int amd_pmf_get_slider_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> >> +{
> >> + int val;
> >> +
> >> + switch (dev->current_profile) {
> >> + case PLATFORM_PROFILE_PERFORMANCE:
> >> + val = TA_BEST_PERFORMANCE;
> >> + break;
> >> + case PLATFORM_PROFILE_BALANCED:
> >> + val = TA_BETTER_PERFORMANCE;
> >> + break;
> >> + case PLATFORM_PROFILE_LOW_POWER:
> >> + val = TA_BEST_BATTERY;
> >> + break;
> >> + default:
> >> + dev_err(dev->dev, "Unknown Platform Profile.\n");
> >> + return -EOPNOTSUPP;
> >> + }
> >> + in->ev_info.power_slider = val;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> >> +{
> >> + /* TA side lid open is 1 and close is 0, hence the ! here */
> >> + in->ev_info.lid_state = !acpi_lid_open();
> >> + in->ev_info.power_source = amd_pmf_get_power_source();
> >> + amd_pmf_get_smu_info(dev, in);
> >> + amd_pmf_get_battery_info(dev, in);
> >> + amd_pmf_get_slider_info(dev, in);
> >> +}
> >> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> >> index a8b05e746efd..eb25d5ce3a9a 100644
> >> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> >> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> >> @@ -113,6 +113,7 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> >> {
> >> struct ta_pmf_shared_memory *ta_sm = NULL;
> >> struct ta_pmf_enact_result *out = NULL;
> >> + struct ta_pmf_enact_table *in = NULL;
> >> struct tee_param param[MAX_TEE_PARAM];
> >> struct tee_ioctl_invoke_arg arg;
> >> int ret = 0;
> >> @@ -123,11 +124,13 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> >> memset(dev->shbuf, 0, dev->policy_sz);
> >> ta_sm = (struct ta_pmf_shared_memory *)dev->shbuf;
> >> out = &ta_sm->pmf_output.policy_apply_table;
> >> + in = &ta_sm->pmf_input.enact_table;
> >>
> >> memset(ta_sm, 0, sizeof(struct ta_pmf_shared_memory));
> >> ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER__ENACT_POLICIES;
> >> ta_sm->if_version = PMF_TA_IF_VERSION__MAJOR;
> >>
> >> + amd_pmf_populate_ta_inputs(dev, in);
> >> amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER__ENACT_POLICIES, &arg, param);
> >>
> >> ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
> >>
> >
>
^ permalink raw reply
* Re: [PATCH RFC v4 6/6] input: ads7846: Move wait_for_sync() logic to driver
From: Linus Walleij @ 2023-10-02 8:59 UTC (permalink / raw)
To: Duje Mihanović
Cc: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Bartosz Golaszewski,
Andy Shevchenko, Dmitry Torokhov, Mark Brown, linux-arm-kernel,
linux-kernel, linux-usb, linux-gpio, linux-input, linux-spi
In-Reply-To: <20231001-pxa-gpio-v4-6-0f3b975e6ed5@skole.hr>
On Sun, Oct 1, 2023 at 4:13 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
> If this code is left in the board file, the sync GPIO would have to be
> separated into another lookup table during conversion to the GPIO
> descriptor API (which is also done in this patch).
>
> The only user of this code (Sharp Spitz) is also converted in this
> patch.
>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
Looking good!
With Andy's nits fixed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH RFC v4 2/6] ARM: pxa: Convert Spitz LEDs to GPIO descriptors
From: Linus Walleij @ 2023-10-02 8:54 UTC (permalink / raw)
To: Duje Mihanović
Cc: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Bartosz Golaszewski,
Andy Shevchenko, Dmitry Torokhov, Mark Brown, linux-arm-kernel,
linux-kernel, linux-usb, linux-gpio, linux-input, linux-spi
In-Reply-To: <20231001-pxa-gpio-v4-2-0f3b975e6ed5@skole.hr>
On Sun, Oct 1, 2023 at 4:13 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
> Sharp's Spitz board still uses the legacy GPIO interface for configuring
> its two onboard LEDs.
>
> Convert them to use the GPIO descriptor interface.
>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
LGTM:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH RFC v4 5/6] ARM: pxa: Convert gumstix Bluetooth to GPIO descriptors
From: Bartosz Golaszewski @ 2023-10-02 7:42 UTC (permalink / raw)
To: Duje Mihanović
Cc: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij, Andy Shevchenko,
Dmitry Torokhov, Mark Brown, linux-arm-kernel, linux-kernel,
linux-usb, linux-gpio, linux-input, linux-spi
In-Reply-To: <20231001-pxa-gpio-v4-5-0f3b975e6ed5@skole.hr>
On Sun, Oct 1, 2023 at 4:13 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
>
> Gumstix still uses the legacy GPIO interface for resetting the Bluetooth
> device.
>
> Convert it to use the GPIO descriptor interface.
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> ---
> arch/arm/mach-pxa/gumstix.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/gumstix.c b/arch/arm/mach-pxa/gumstix.c
> index c9f0f62187bd..14e1b9274d7a 100644
> --- a/arch/arm/mach-pxa/gumstix.c
> +++ b/arch/arm/mach-pxa/gumstix.c
> @@ -20,8 +20,8 @@
> #include <linux/delay.h>
> #include <linux/mtd/mtd.h>
> #include <linux/mtd/partitions.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/gpio/machine.h>
> -#include <linux/gpio.h>
> #include <linux/err.h>
> #include <linux/clk.h>
>
> @@ -129,6 +129,9 @@ static void gumstix_udc_init(void)
> #endif
>
> #ifdef CONFIG_BT
> +GPIO_LOOKUP_SINGLE(gumstix_bt_gpio_table, "pxa2xx-uart.1", "pxa-gpio",
> + GPIO_GUMSTIX_BTRESET, "BTRST", GPIO_ACTIVE_LOW);
> +
> /* Normally, the bootloader would have enabled this 32kHz clock but many
> ** boards still have u-boot 1.1.4 so we check if it has been turned on and
> ** if not, we turn it on with a warning message. */
> @@ -153,24 +156,23 @@ static void gumstix_setup_bt_clock(void)
>
> static void __init gumstix_bluetooth_init(void)
> {
> - int err;
> + struct gpio_desc *desc;
> +
> + gpiod_add_lookup_table(&gumstix_bt_gpio_table);
>
> gumstix_setup_bt_clock();
>
> - err = gpio_request(GPIO_GUMSTIX_BTRESET, "BTRST");
> - if (err) {
> + desc = gpiod_get(&pxa_device_btuart.dev, "BTRST", GPIOD_OUT_HIGH);
> + if (IS_ERR(desc)) {
> pr_err("gumstix: failed request gpio for bluetooth reset\n");
> return;
> }
>
> - err = gpio_direction_output(GPIO_GUMSTIX_BTRESET, 1);
> - if (err) {
> - pr_err("gumstix: can't reset bluetooth\n");
> - return;
> - }
> - gpio_set_value(GPIO_GUMSTIX_BTRESET, 0);
> + gpiod_set_value(desc, 0);
> udelay(100);
> - gpio_set_value(GPIO_GUMSTIX_BTRESET, 1);
> + gpiod_set_value(desc, 1);
> +
> + gpiod_put(desc);
This changes the way this code works. You release the descriptor here,
it returns to the driver and can be re-requested by someone else. Its
value is also not guaranteed to remain as "active". Is this what you
want?
Bart
> }
> #else
> static void gumstix_bluetooth_init(void)
>
> --
> 2.42.0
>
>
^ permalink raw reply
* Re: [PATCH RFC v4 2/6] ARM: pxa: Convert Spitz LEDs to GPIO descriptors
From: Bartosz Golaszewski @ 2023-10-02 7:36 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Duje Mihanović, Daniel Mack, Haojian Zhuang, Robert Jarzmik,
Russell King, Alan Stern, Greg Kroah-Hartman, Linus Walleij,
Andy Shevchenko, Dmitry Torokhov, Mark Brown, linux-arm-kernel,
linux-kernel, linux-usb, linux-gpio, linux-input, linux-spi
In-Reply-To: <CAHp75VeYduD=uXpNKcxhwqFTkahUbz_Ockqi7KVO88cpeVHbQQ@mail.gmail.com>
On Sun, Oct 1, 2023 at 4:35 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Oct 1, 2023 at 5:13 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
> >
> > Sharp's Spitz board still uses the legacy GPIO interface for configuring
> > its two onboard LEDs.
> >
> > Convert them to use the GPIO descriptor interface.
>
> ...
>
> > static void __init spitz_leds_init(void)
> > {
> > + gpiod_add_lookup_table(&spitz_led_gpio_table);
> > platform_device_register(&spitz_led_device);
> > + spitz_gpio_leds[0].gpiod = gpiod_get_index(&spitz_led_device.dev,
> > + NULL, 0, GPIOD_ASIS);
> > + spitz_gpio_leds[1].gpiod = gpiod_get_index(&spitz_led_device.dev,
> > + NULL, 1, GPIOD_ASIS);
> > }
>
> What's the point of keeping a lookup table after we got descriptors out of it?
>
Normally the descriptors would be retrieved in drivers and so lookup
tables should stay in memory forever as static resources (just like
device-tree). We have recently added some "temporary" lookup tables to
address even worse hacks. The tables would be removed immediately
after the descriptor is retrieved simply because we used that hack in
drivers which may be unbound and re-bound resulting in adding
repeating lookup entries.
Here we're dealing with a board-file so a more classic approach of
having static lookup tables added once and never removed is in order.
So I'd leave it like this.
Bart
^ permalink raw reply
* [PATCH v7 4/4] Input: goodix-berlin - add SPI support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-02 6:54 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-input,
linux-arm-msm, devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20231002-topic-goodix-berlin-upstream-initial-v7-0-792fb91f5e88@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the SPI interface.
The driver doesn't use the regmap_spi code since the SPI messages
needs to be prefixed, thus this custom regmap code.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 +++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_spi.c | 173 ++++++++++++++++++++++++++
3 files changed, 188 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index cc7b88118158..c821fe3ee794 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -433,6 +433,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_I2C
To compile this driver as a module, choose M here: the
module will be called goodix_berlin_i2c.
+config TOUCHSCREEN_GOODIX_BERLIN_SPI
+ tristate "Goodix Berlin SPI touchscreen"
+ depends on SPI_MASTER
+ select REGMAP
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via SPI.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_spi.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 7ef677cf7a10..a81cb5aa21a5 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c
new file mode 100644
index 000000000000..1f57c3592918
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_spi.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <asm/unaligned.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include "goodix_berlin.h"
+
+#define GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN 1
+#define GOODIX_BERLIN_REGISTER_WIDTH 4
+#define GOODIX_BERLIN_SPI_READ_DUMMY_LEN 3
+#define GOODIX_BERLIN_SPI_READ_PREFIX_LEN (GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + \
+ GOODIX_BERLIN_REGISTER_WIDTH + \
+ GOODIX_BERLIN_SPI_READ_DUMMY_LEN)
+#define GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN (GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + \
+ GOODIX_BERLIN_REGISTER_WIDTH)
+
+#define GOODIX_BERLIN_SPI_WRITE_FLAG 0xF0
+#define GOODIX_BERLIN_SPI_READ_FLAG 0xF1
+
+static int goodix_berlin_spi_read(void *context, const void *reg_buf,
+ size_t reg_size, void *val_buf,
+ size_t val_size)
+{
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = reg_buf; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int error;
+
+ if (reg_size != GOODIX_BERLIN_REGISTER_WIDTH)
+ return -EINVAL;
+
+ buf = kzalloc(GOODIX_BERLIN_SPI_READ_PREFIX_LEN + val_size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ /* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */
+ buf[0] = GOODIX_BERLIN_SPI_READ_FLAG;
+ put_unaligned_be32(*reg, buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN);
+ memset(buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + GOODIX_BERLIN_REGISTER_WIDTH,
+ 0xff, GOODIX_BERLIN_SPI_READ_DUMMY_LEN);
+
+ xfers.tx_buf = buf;
+ xfers.rx_buf = buf;
+ xfers.len = GOODIX_BERLIN_SPI_READ_PREFIX_LEN + val_size;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ error = spi_sync(spi, &spi_msg);
+ if (error < 0)
+ dev_err(&spi->dev, "spi transfer error, %d", error);
+ else
+ memcpy(val_buf, buf + GOODIX_BERLIN_SPI_READ_PREFIX_LEN, val_size);
+
+ kfree(buf);
+ return error;
+}
+
+static int goodix_berlin_spi_write(void *context, const void *data,
+ size_t count)
+{
+ unsigned int len = count - GOODIX_BERLIN_REGISTER_WIDTH;
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = data; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int error;
+
+ buf = kzalloc(GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ buf[0] = GOODIX_BERLIN_SPI_WRITE_FLAG;
+ put_unaligned_be32(*reg, buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN);
+ memcpy(buf + GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN,
+ data + GOODIX_BERLIN_REGISTER_WIDTH, len);
+
+ xfers.tx_buf = buf;
+ xfers.len = GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN + len;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ error = spi_sync(spi, &spi_msg);
+ if (error < 0)
+ dev_err(&spi->dev, "spi transfer error, %d", error);
+
+ kfree(buf);
+ return error;
+}
+
+static const struct regmap_config goodix_berlin_spi_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .read = goodix_berlin_spi_read,
+ .write = goodix_berlin_spi_write,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_spi_input_id = {
+ .bustype = BUS_SPI,
+};
+
+static int goodix_berlin_spi_probe(struct spi_device *spi)
+{
+ struct regmap_config regmap_config;
+ struct regmap *regmap;
+ size_t max_size;
+ int error = 0;
+
+ spi->mode = SPI_MODE_0;
+ spi->bits_per_word = 8;
+ error = spi_setup(spi);
+ if (error)
+ return error;
+
+ max_size = spi_max_transfer_size(spi);
+
+ regmap_config = goodix_berlin_spi_regmap_conf;
+ regmap_config.max_raw_read = max_size - GOODIX_BERLIN_SPI_READ_PREFIX_LEN;
+ regmap_config.max_raw_write = max_size - GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN;
+
+ regmap = devm_regmap_init(&spi->dev, NULL, spi, ®map_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ return goodix_berlin_probe(&spi->dev, spi->irq,
+ &goodix_berlin_spi_input_id, regmap);
+}
+
+static const struct spi_device_id goodix_berlin_spi_ids[] = {
+ { "gt9916" },
+ { },
+};
+MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids);
+
+static const struct of_device_id goodix_berlin_spi_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match);
+
+static struct spi_driver goodix_berlin_spi_driver = {
+ .driver = {
+ .name = "goodix-berlin-spi",
+ .of_match_table = goodix_berlin_spi_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_spi_probe,
+ .id_table = goodix_berlin_spi_ids,
+};
+module_spi_driver(goodix_berlin_spi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin SPI Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v7 2/4] Input: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-02 6:54 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-input,
linux-arm-msm, devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20231002-topic-goodix-berlin-upstream-initial-v7-0-792fb91f5e88@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs.
These touchscreen ICs support SPI, I2C and I3C interface, up to
10 finger touch, stylus and gestures events.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
Support for advanced features like:
- Firmware & config update
- Stylus events
- Gestures events
- Previous revisions support (BerlinA or BerlinB)
is not included in current version.
The current support will work with currently flashed firmware
and config, and bail out if firmware or config aren't flashed yet.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 3 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin.h | 159 +++++++
drivers/input/touchscreen/goodix_berlin_core.c | 581 +++++++++++++++++++++++++
4 files changed, 744 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index e3e2324547b9..950da599ae1a 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -416,6 +416,9 @@ config TOUCHSCREEN_GOODIX
To compile this driver as a module, choose M here: the
module will be called goodix.
+config TOUCHSCREEN_GOODIX_BERLIN_CORE
+ tristate
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 62bd24f3ac8e..2e2f3e70cd2c 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
new file mode 100644
index 000000000000..235f44947a28
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin.h
@@ -0,0 +1,159 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Goodix Touchscreen Driver
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_berlin_berlin driver.
+ */
+
+#ifndef __GOODIX_BERLIN_H_
+#define __GOODIX_BERLIN_H_
+
+#include <linux/gpio/consumer.h>
+#include <linux/input.h>
+#include <linux/input/touchscreen.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sizes.h>
+
+#define GOODIX_BERLIN_MAX_TOUCH 10
+
+#define GOODIX_BERLIN_NORMAL_RESET_DELAY_MS 100
+
+#define GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN 8
+#define GOODIX_BERLIN_STATUS_OFFSET 0
+#define GOODIX_BERLIN_REQUEST_TYPE_OFFSET 2
+
+#define GOODIX_BERLIN_BYTES_PER_POINT 8
+#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE 2
+#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK GENMASK(15, 0)
+
+/* Read n finger events */
+#define GOODIX_BERLIN_IRQ_READ_LEN(n) (GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN + \
+ (GOODIX_BERLIN_BYTES_PER_POINT * (n)) + \
+ GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+
+#define GOODIX_BERLIN_TOUCH_EVENT BIT(7)
+#define GOODIX_BERLIN_REQUEST_EVENT BIT(6)
+#define GOODIX_BERLIN_TOUCH_COUNT_MASK GENMASK(3, 0)
+
+#define GOODIX_BERLIN_REQUEST_CODE_RESET 3
+
+#define GOODIX_BERLIN_POINT_TYPE_MASK GENMASK(3, 0)
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER 1
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS 3
+
+#define GOODIX_BERLIN_TOUCH_ID_MASK GENMASK(7, 4)
+
+#define GOODIX_BERLIN_DEV_CONFIRM_VAL 0xAA
+#define GOODIX_BERLIN_BOOTOPTION_ADDR 0x10000
+#define GOODIX_BERLIN_FW_VERSION_INFO_ADDR 0x10014
+
+#define GOODIX_BERLIN_IC_INFO_MAX_LEN SZ_1K
+#define GOODIX_BERLIN_IC_INFO_ADDR 0x10070
+
+struct goodix_berlin_fw_version {
+ u8 rom_pid[6];
+ u8 rom_vid[3];
+ u8 rom_vid_reserved;
+ u8 patch_pid[8];
+ u8 patch_vid[4];
+ u8 patch_vid_reserved;
+ u8 sensor_id;
+ u8 reserved[2];
+ __le16 checksum;
+} __packed;
+
+struct goodix_berlin_ic_info_version {
+ u8 info_customer_id;
+ u8 info_version_id;
+ u8 ic_die_id;
+ u8 ic_version_id;
+ __le32 config_id;
+ u8 config_version;
+ u8 frame_data_customer_id;
+ u8 frame_data_version_id;
+ u8 touch_data_customer_id;
+ u8 touch_data_version_id;
+ u8 reserved[3];
+} __packed;
+
+struct goodix_berlin_ic_info_feature {
+ __le16 freqhop_feature;
+ __le16 calibration_feature;
+ __le16 gesture_feature;
+ __le16 side_touch_feature;
+ __le16 stylus_feature;
+} __packed;
+
+struct goodix_berlin_ic_info_misc {
+ __le32 cmd_addr;
+ __le16 cmd_max_len;
+ __le32 cmd_reply_addr;
+ __le16 cmd_reply_len;
+ __le32 fw_state_addr;
+ __le16 fw_state_len;
+ __le32 fw_buffer_addr;
+ __le16 fw_buffer_max_len;
+ __le32 frame_data_addr;
+ __le16 frame_data_head_len;
+ __le16 fw_attr_len;
+ __le16 fw_log_len;
+ u8 pack_max_num;
+ u8 pack_compress_version;
+ __le16 stylus_struct_len;
+ __le16 mutual_struct_len;
+ __le16 self_struct_len;
+ __le16 noise_struct_len;
+ __le32 touch_data_addr;
+ __le16 touch_data_head_len;
+ __le16 point_struct_len;
+ __le16 reserved1;
+ __le16 reserved2;
+ __le32 mutual_rawdata_addr;
+ __le32 mutual_diffdata_addr;
+ __le32 mutual_refdata_addr;
+ __le32 self_rawdata_addr;
+ __le32 self_diffdata_addr;
+ __le32 self_refdata_addr;
+ __le32 iq_rawdata_addr;
+ __le32 iq_refdata_addr;
+ __le32 im_rawdata_addr;
+ __le16 im_readata_len;
+ __le32 noise_rawdata_addr;
+ __le16 noise_rawdata_len;
+ __le32 stylus_rawdata_addr;
+ __le16 stylus_rawdata_len;
+ __le32 noise_data_addr;
+ __le32 esd_addr;
+} __packed;
+
+struct goodix_berlin_touch_data {
+ u8 id;
+ u8 unused;
+ __le16 x;
+ __le16 y;
+ __le16 w;
+} __packed;
+
+struct goodix_berlin_core {
+ struct device *dev;
+ struct regmap *regmap;
+ struct regulator *avdd;
+ struct regulator *iovdd;
+ struct gpio_desc *reset_gpio;
+ struct touchscreen_properties props;
+ struct goodix_berlin_fw_version fw_version;
+ struct input_dev *input_dev;
+ int irq;
+
+ /* Runtime parameters extracted from IC_INFO buffer */
+ u32 touch_data_addr;
+};
+
+int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
+ struct regmap *regmap);
+
+extern const struct dev_pm_ops goodix_berlin_pm_ops;
+
+#endif
diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
new file mode 100644
index 000000000000..08c508677669
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_core.c
@@ -0,0 +1,581 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Touchscreen Driver
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <asm/unaligned.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/regmap.h>
+
+#include "goodix_berlin.h"
+
+/*
+ * Goodix "Berlin" Touchscreen ID driver
+ *
+ * This driver is distinct from goodix.c since hardware interface
+ * is different enough to require a new driver.
+ * None of the register address or data structure are close enough
+ * to the previous generations.
+ *
+ * Currently only handles Multitouch events with already
+ * programmed firmware and "config" for "Revision D" Berlin IC.
+ *
+ * Support is missing for:
+ * - ESD Management
+ * - Firmware update/flashing
+ * - "Config" update/flashing
+ * - Stylus Events
+ * - Gesture Events
+ * - Support for older revisions (A & B)
+ */
+
+static bool goodix_berlin_checksum_valid(const u8 *data, int size)
+{
+ u32 cal_checksum = 0;
+ u16 r_checksum;
+ u32 i;
+
+ if (size < GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+ return false;
+
+ for (i = 0; i < size - GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE; i++)
+ cal_checksum += data[i];
+
+ r_checksum = get_unaligned_le16(&data[i]);
+
+ return FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK, cal_checksum) == r_checksum;
+}
+
+static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
+ const u8 *data, int size)
+{
+ int i;
+
+ /*
+ * If the device is missing or doesn't respond the buffer
+ * could be filled with bus default line state, 0x00 or 0xff,
+ * so declare success the first time we encounter neither.
+ */
+ for (i = 0; i < size; i++)
+ if (data[i] > 0 && data[i] < 0xff)
+ return false;
+
+ return true;
+}
+
+static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
+{
+ u8 tx_buf[8], rx_buf[8];
+ int retry = 3;
+ int error;
+
+ memset(tx_buf, GOODIX_BERLIN_DEV_CONFIRM_VAL, sizeof(tx_buf));
+ while (retry--) {
+ error = regmap_raw_write(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, tx_buf,
+ sizeof(tx_buf));
+ if (error)
+ return error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, rx_buf,
+ sizeof(rx_buf));
+ if (error)
+ return error;
+
+ if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
+ return 0;
+
+ usleep_range(5000, 5100);
+ }
+
+ dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n", 8, rx_buf);
+
+ return -EINVAL;
+}
+
+static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
+{
+ int error = 0;
+
+ if (on) {
+ error = regulator_enable(cd->iovdd);
+ if (error) {
+ dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
+ return error;
+ }
+
+ /* Vendor waits 3ms for IOVDD to settle */
+ usleep_range(3000, 3100);
+
+ error = regulator_enable(cd->avdd);
+ if (error) {
+ dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
+ goto err_iovdd_disable;
+ }
+
+ /* Vendor waits 15ms for IOVDD to settle */
+ usleep_range(15000, 15100);
+
+ gpiod_set_value(cd->reset_gpio, 0);
+
+ /* Vendor waits 4ms for Firmware to initialize */
+ usleep_range(4000, 4100);
+
+ error = goodix_berlin_dev_confirm(cd);
+ if (error)
+ goto err_dev_reset;
+
+ /* Vendor waits 100ms for Firmware to fully boot */
+ msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
+
+ return 0;
+ }
+
+err_dev_reset:
+ gpiod_set_value(cd->reset_gpio, 1);
+
+ regulator_disable(cd->avdd);
+
+err_iovdd_disable:
+ regulator_disable(cd->iovdd);
+
+ return error;
+}
+
+static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
+{
+ u8 buf[sizeof(struct goodix_berlin_fw_version)];
+ int error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_FW_VERSION_INFO_ADDR, buf, sizeof(buf));
+ if (error) {
+ dev_err(cd->dev, "error reading fw version, %d\n", error);
+ return error;
+ }
+
+ if (!goodix_berlin_checksum_valid(buf, sizeof(buf))) {
+ dev_err(cd->dev, "invalid fw version: checksum error\n");
+ return -EINVAL;
+ }
+
+ memcpy(&cd->fw_version, buf, sizeof(cd->fw_version));
+
+ return 0;
+}
+
+/* Only extract necessary data for runtime */
+static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
+ const u8 *data, u16 length)
+{
+ struct goodix_berlin_ic_info_misc misc;
+ unsigned int offset = 0;
+ u8 param_num;
+
+ offset += sizeof(__le16); /* length */
+ offset += sizeof(struct goodix_berlin_ic_info_version);
+ offset += sizeof(struct goodix_berlin_ic_info_feature);
+
+ /* IC_INFO Parameters, variable width structure */
+ offset += 4 * sizeof(u8); /* drv_num, sen_num, button_num, force_num */
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* active_scan_rate_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* mutual_freq_num*/
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* self_tx_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* self_rx_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* stylus_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset + sizeof(misc) > length)
+ goto invalid_offset;
+
+ /* goodix_berlin_ic_info_misc */
+ memcpy(&misc, &data[offset], sizeof(misc));
+
+ cd->touch_data_addr = le32_to_cpu(misc.touch_data_addr);
+
+ return 0;
+
+invalid_offset:
+ dev_err(cd->dev, "ic_info length is invalid (offset %d length %d)\n",
+ offset, length);
+ return -EINVAL;
+}
+
+static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
+{
+ u8 afe_data[GOODIX_BERLIN_IC_INFO_MAX_LEN];
+ __le16 length_raw;
+ u16 length;
+ int error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ &length_raw, sizeof(length_raw));
+ if (error) {
+ dev_info(cd->dev, "failed get ic info length, %d\n", error);
+ return error;
+ }
+
+ length = le16_to_cpu(length_raw);
+ if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
+ dev_info(cd->dev, "invalid ic info length %d\n", length);
+ return -EINVAL;
+ }
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ afe_data, length);
+ if (error) {
+ dev_info(cd->dev, "failed get ic info data, %d\n", error);
+ return error;
+ }
+
+ /* check whether the data is valid (ex. bus default values) */
+ if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
+ dev_err(cd->dev, "fw info data invalid\n");
+ return -EINVAL;
+ }
+
+ if (!goodix_berlin_checksum_valid((const uint8_t *)afe_data, length)) {
+ dev_info(cd->dev, "fw info checksum error\n");
+ return -EINVAL;
+ }
+
+ error = goodix_berlin_convert_ic_info(cd, afe_data, length);
+ if (error)
+ return error;
+
+ /* check some key info */
+ if (!cd->touch_data_addr) {
+ dev_err(cd->dev, "touch_data_addr is null\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
+ const void *buf, int touch_num)
+{
+ const struct goodix_berlin_touch_data *touch_data = buf;
+ int i;
+
+ /* Check for data validity */
+ for (i = 0; i < touch_num; i++) {
+ unsigned int id;
+
+ id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, touch_data[i].id);
+
+ if (id >= GOODIX_BERLIN_MAX_TOUCH) {
+ dev_warn(cd->dev, "invalid finger id %d\n", id);
+ return;
+ }
+ }
+
+ /* Report finger touches */
+ for (i = 0; i < touch_num; i++) {
+ input_mt_slot(cd->input_dev,
+ FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
+ touch_data[i].id));
+ input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
+
+ touchscreen_report_pos(cd->input_dev, &cd->props,
+ __le16_to_cpu(touch_data[i].x),
+ __le16_to_cpu(touch_data[i].y),
+ true);
+ input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
+ __le16_to_cpu(touch_data[i].w));
+ }
+
+ input_mt_sync_frame(cd->input_dev);
+ input_sync(cd->input_dev);
+}
+
+static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
+ const void *pre_buf, u32 pre_buf_len)
+{
+ static u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
+ u8 point_type, touch_num;
+ int error;
+
+ /* copy pre-data to buffer */
+ memcpy(buffer, pre_buf, pre_buf_len);
+
+ touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
+ buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
+
+ if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
+ dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
+ return;
+ }
+
+ if (touch_num) {
+ /* read more data if more than 2 touch events */
+ if (unlikely(touch_num > 2)) {
+ error = regmap_raw_read(cd->regmap,
+ cd->touch_data_addr + pre_buf_len,
+ &buffer[pre_buf_len],
+ (touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
+ if (error) {
+ dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
+ error);
+ return;
+ }
+ }
+
+ point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
+ buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
+
+ if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
+ point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
+ dev_warn_once(cd->dev, "Stylus event type not handled\n");
+ return;
+ }
+
+ if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
+ touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
+ dev_err(cd->dev, "touch data checksum error, data: %*ph\n",
+ touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
+ &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
+ return;
+ }
+ }
+
+ goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
+ touch_num);
+}
+
+static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
+{
+ gpiod_set_value(cd->reset_gpio, 1);
+ usleep_range(2000, 2100);
+ gpiod_set_value(cd->reset_gpio, 0);
+
+ msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
+
+ return 0;
+}
+
+static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
+{
+ struct goodix_berlin_core *cd = data;
+ u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
+ u8 event_status;
+ int error;
+
+ /* First, read buffer with space for 2 touch events */
+ error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
+ GOODIX_BERLIN_IRQ_READ_LEN(2));
+ if (error) {
+ dev_err_ratelimited(cd->dev, "failed get event head data, %d\n", error);
+ return IRQ_HANDLED;
+ }
+
+ if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
+ return IRQ_HANDLED;
+
+ if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
+ dev_warn_ratelimited(cd->dev, "touch head checksum err : %*ph\n",
+ GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
+ return IRQ_HANDLED;
+ }
+
+ event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
+
+ if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
+ goodix_berlin_touch_handler(cd, buf, GOODIX_BERLIN_IRQ_READ_LEN(2));
+
+ if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
+ switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
+ case GOODIX_BERLIN_REQUEST_CODE_RESET:
+ goodix_berlin_request_handle_reset(cd);
+ break;
+
+ default:
+ dev_warn(cd->dev, "unsupported request code 0x%x\n",
+ buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
+ }
+ }
+
+ /* Clear up status field */
+ regmap_write(cd->regmap, cd->touch_data_addr, 0);
+
+ return IRQ_HANDLED;
+}
+
+static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
+ const struct input_id *id)
+{
+ struct input_dev *input_dev;
+ int error;
+
+ input_dev = devm_input_allocate_device(cd->dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ cd->input_dev = input_dev;
+ input_set_drvdata(input_dev, cd);
+
+ input_dev->name = "Goodix Berlin Capacitive TouchScreen";
+ input_dev->phys = "input/ts";
+
+ input_dev->id = *id;
+
+ input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
+ input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
+ input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+
+ touchscreen_parse_properties(cd->input_dev, true, &cd->props);
+
+ error = input_mt_init_slots(cd->input_dev, GOODIX_BERLIN_MAX_TOUCH,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+ if (error)
+ return error;
+
+ return input_register_device(cd->input_dev);
+}
+
+static int goodix_berlin_pm_suspend(struct device *dev)
+{
+ struct goodix_berlin_core *cd = dev_get_drvdata(dev);
+
+ disable_irq(cd->irq);
+
+ return goodix_berlin_power_on(cd, false);
+}
+
+static int goodix_berlin_pm_resume(struct device *dev)
+{
+ struct goodix_berlin_core *cd = dev_get_drvdata(dev);
+ int error;
+
+ error = goodix_berlin_power_on(cd, true);
+ if (error)
+ return error;
+
+ enable_irq(cd->irq);
+
+ return 0;
+}
+
+EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
+ goodix_berlin_pm_suspend,
+ goodix_berlin_pm_resume);
+
+static void goodix_berlin_power_off(void *data)
+{
+ struct goodix_berlin_core *cd = data;
+
+ goodix_berlin_power_on(cd, false);
+}
+
+int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
+ struct regmap *regmap)
+{
+ struct goodix_berlin_core *cd;
+ int error;
+
+ if (irq <= 0) {
+ dev_err(dev, "Missing interrupt number\n");
+ return -EINVAL;
+ }
+
+ cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
+ if (!cd)
+ return -ENOMEM;
+
+ cd->dev = dev;
+ cd->regmap = regmap;
+ cd->irq = irq;
+
+ cd->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(cd->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(cd->reset_gpio),
+ "Failed to request reset gpio\n");
+
+ cd->avdd = devm_regulator_get(dev, "avdd");
+ if (IS_ERR(cd->avdd))
+ return dev_err_probe(dev, PTR_ERR(cd->avdd),
+ "Failed to request avdd regulator\n");
+
+ cd->iovdd = devm_regulator_get(dev, "iovdd");
+ if (IS_ERR(cd->iovdd))
+ return dev_err_probe(dev, PTR_ERR(cd->iovdd),
+ "Failed to request iovdd regulator\n");
+
+ error = goodix_berlin_power_on(cd, true);
+ if (error) {
+ dev_err(dev, "failed power on");
+ return error;
+ }
+
+ error = devm_add_action_or_reset(dev, goodix_berlin_power_off, cd);
+ if (error)
+ return error;
+
+ error = goodix_berlin_read_version(cd);
+ if (error) {
+ dev_err(dev, "failed to get version info");
+ return error;
+ }
+
+ error = goodix_berlin_get_ic_info(cd);
+ if (error) {
+ dev_err(dev, "invalid ic info, abort");
+ return error;
+ }
+
+ error = goodix_berlin_input_dev_config(cd, id);
+ if (error) {
+ dev_err(dev, "failed set input device");
+ return error;
+ }
+
+ error = devm_request_threaded_irq(dev, irq, NULL,
+ goodix_berlin_threadirq_func,
+ IRQF_ONESHOT, "goodix-berlin", cd);
+ if (error) {
+ dev_err(dev, "request threaded irq failed: %d\n", error);
+ return error;
+ }
+
+ dev_set_drvdata(dev, cd);
+
+ dev_dbg(dev, "Goodix Berlin %s Touchscreen Controller", cd->fw_version.patch_pid);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(goodix_berlin_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v7 3/4] Input: goodix-berlin - add I2C support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-02 6:54 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-input,
linux-arm-msm, devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20231002-topic-goodix-berlin-upstream-initial-v7-0-792fb91f5e88@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the I2C interface.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 ++++++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_i2c.c | 69 +++++++++++++++++++++++++++
3 files changed, 84 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 950da599ae1a..cc7b88118158 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -419,6 +419,20 @@ config TOUCHSCREEN_GOODIX
config TOUCHSCREEN_GOODIX_BERLIN_CORE
tristate
+config TOUCHSCREEN_GOODIX_BERLIN_I2C
+ tristate "Goodix Berlin I2C touchscreen"
+ depends on I2C
+ select REGMAP_I2C
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via I2C.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_i2c.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 2e2f3e70cd2c..7ef677cf7a10 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_i2c.c b/drivers/input/touchscreen/goodix_berlin_i2c.c
new file mode 100644
index 000000000000..6407b2258eb1
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_i2c.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "goodix_berlin.h"
+
+#define I2C_MAX_TRANSFER_SIZE 256
+
+static const struct regmap_config goodix_berlin_i2c_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .max_raw_read = I2C_MAX_TRANSFER_SIZE,
+ .max_raw_write = I2C_MAX_TRANSFER_SIZE,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_i2c_input_id = {
+ .bustype = BUS_I2C,
+};
+
+static int goodix_berlin_i2c_probe(struct i2c_client *client)
+{
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init_i2c(client, &goodix_berlin_i2c_regmap_conf);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ return goodix_berlin_probe(&client->dev, client->irq,
+ &goodix_berlin_i2c_input_id, regmap);
+}
+
+static const struct i2c_device_id goodix_berlin_i2c_id[] = {
+ { "gt9916", 0 },
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, goodix_berlin_i2c_id);
+
+static const struct of_device_id goodix_berlin_i2c_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_i2c_of_match);
+
+static struct i2c_driver goodix_berlin_i2c_driver = {
+ .driver = {
+ .name = "goodix-berlin-i2c",
+ .of_match_table = goodix_berlin_i2c_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_i2c_probe,
+ .id_table = goodix_berlin_i2c_id,
+};
+module_i2c_driver(goodix_berlin_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin I2C Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox