* Re: [PATCH v7 0/7] VMware hypercalls enhancements
From: Alexey Makhalov @ 2024-04-04 23:32 UTC (permalink / raw)
To: hpa
Cc: linux-kernel, virtualization, bp, x86, dave.hansen, mingo, tglx,
netdev, richardcochran, linux-input, dmitry.torokhov, zackr,
linux-graphics-maintainer, pv-drivers, timothym, akaher,
dri-devel, daniel, airlied, tzimmermann, mripard,
maarten.lankhorst, horms, kirill.shutemov
In-Reply-To: <20240307212949.4166120-1-alexey.makhalov@broadcom.com>
Peter, can you please review version 7 of "x86/vmware: Add TDX hypercall support" patch.
It addresses the concern you had in previous version. Thanks.
^ permalink raw reply
* Re: [PATCH RESEND v3 2/2] Input: atmel_mxt_ts - support poweroff in suspend
From: Stefan Eichenberger @ 2024-04-05 8:15 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: nick, robh+dt, krzysztof.kozlowski+dt, conor+dt, nicolas.ferre,
alexandre.belloni, claudiu.beznea, linus.walleij, linux-input,
devicetree, linux-arm-kernel, linux-kernel, francesco.dolcini,
Stefan Eichenberger
In-Reply-To: <ZfSYp6aV6bRhlPUJ@google.com>
Hi Dmitry,
Thanks for the feedback, I had a first look at the changes and I'm not
sure if we would break some use cases. Therfore, here some questions.
On Fri, Mar 15, 2024 at 11:51:19AM -0700, Dmitry Torokhov wrote:
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 542a31448c8f..2d5655385702 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -317,6 +317,7 @@ struct mxt_data {
> > struct gpio_desc *reset_gpio;
> > struct gpio_desc *wake_gpio;
> > bool use_retrigen_workaround;
> > + bool poweroff_sleep;
> >
> > /* Cached parameters from object table */
> > u16 T5_address;
> > @@ -2799,15 +2800,18 @@ static int mxt_configure_objects(struct mxt_data *data,
> > dev_warn(dev, "Error %d updating config\n", error);
> > }
> >
> > - if (data->multitouch) {
> > - error = mxt_initialize_input_device(data);
> > - if (error)
> > - return error;
> > - } else {
> > - dev_warn(dev, "No touch object detected\n");
> > - }
> > + /* If input device is not already registered */
> > + if (!data->input_dev) {
> > + if (data->multitouch) {
> > + error = mxt_initialize_input_device(data);
> > + if (error)
> > + return error;
> > + } else {
> > + dev_warn(dev, "No touch object detected\n");
> > + }
> >
> > - mxt_debug_init(data);
> > + mxt_debug_init(data);
> > + }
> >
> > return 0;
> > }
> > @@ -3325,6 +3329,8 @@ static int mxt_probe(struct i2c_client *client)
> > msleep(MXT_RESET_INVALID_CHG);
> > }
> >
> > + data->poweroff_sleep = device_property_read_bool(&client->dev,
> > + "atmel,poweroff-sleep");
> > /*
> > * Controllers like mXT1386 have a dedicated WAKE line that could be
> > * connected to a GPIO or to I2C SCL pin, or permanently asserted low.
> > @@ -3387,12 +3393,21 @@ static int mxt_suspend(struct device *dev)
> > if (!input_dev)
> > return 0;
> >
> > - mutex_lock(&input_dev->mutex);
> > + if (!device_may_wakeup(dev) && data->poweroff_sleep) {
> > + if (data->reset_gpio)
> > + gpiod_set_value(data->reset_gpio, 1);
> >
> > - if (input_device_enabled(input_dev))
> > - mxt_stop(data);
> > + regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> > + data->regulators);
> > + data->T44_address = 0;
> > + } else {
> > + mutex_lock(&input_dev->mutex);
> > +
> > + if (input_device_enabled(input_dev))
> > + mxt_stop(data);
> >
> > - mutex_unlock(&input_dev->mutex);
> > + mutex_unlock(&input_dev->mutex);
> > + }
>
> This all should go into mxt_stop(), so that if device is closed, or
> inhibited, you power it down as well (if you can).
We would then have to power it up during probe to see if the device is
threre, read the configuration and power it down again afterwards until
the device is opened. If the device is in bootloader mode we would most
likely have to keep the power on all the time and never turn it off,
right?
>
> >
> > disable_irq(data->irq);
> >
> > @@ -3408,14 +3423,37 @@ static int mxt_resume(struct device *dev)
> > if (!input_dev)
> > return 0;
> >
> > - enable_irq(data->irq);
> > + if (!device_may_wakeup(dev) && data->poweroff_sleep) {
> > + int ret;
> >
> > - mutex_lock(&input_dev->mutex);
> > + ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> > + data->regulators);
> > + if (ret) {
> > + dev_err(dev, "failed to enable regulators: %d\n",
> > + ret);
> > + return ret;
> > + }
> > + msleep(MXT_BACKUP_TIME);
> >
> > - if (input_device_enabled(input_dev))
> > - mxt_start(data);
> > + if (data->reset_gpio) {
> > + /* Wait a while and then de-assert the RESET GPIO line */
> > + msleep(MXT_RESET_GPIO_TIME);
> > + gpiod_set_value(data->reset_gpio, 0);
> > + msleep(MXT_RESET_INVALID_CHG);
> > + }
> >
> > - mutex_unlock(&input_dev->mutex);
> > + /* This also enables the irq again */
> > + mxt_initialize(data);
>
> And this needs to go into mxt_start(). Also, we should make sure that
> once resume operation completes the device is fully operational. That
> means you should not request the firmware asynchronously in
> mxt_initialize() in case you are in the resume path. I think you should
> also unwind mxt_initialize() and mxt_configure_objects() to make it
> clear what is the part of initial initialization and what is part of
> re-initializing during resume. The configuration that is exposed to
> userspace (resolution, number of objects, other properties) should stay
> the same, the configuration of the chip itself (power mode, etc) should
> be restored.
Here we would most likely have to load the firmware (configuration)
synchronously all the time if the poweroff_sleep flag is set. Ths makes
sure that the device is ready when we open the device. Would this delay
be acceptable when opening the input device? Normally the configuration
is not that big and should load quite fast.
Regards,
Stefan
^ permalink raw reply
* [PATCH] Input: psmouse: add NULL check to psmouse_from_serio()
From: Takashi Iwai @ 2024-04-05 8:44 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
The serio drvdata can be still NULL while the PS/2 interrupt is
processed. This leaded to crash with a NULL dereference Oops, as
psmouse_from_serio() blindly assumes the non-NULL ps2dev object.
Add a NULL check and return NULL from psmouse_from_serio(). The
returned NULL is handled properly in the caller side, skipping the
rest gracefully.
The log in the bugzilla entry showed that the probe of synaptics
driver succeeded after that point. So this is a stop-gap solution.
Link: https://bugzilla.suse.com/show_bug.cgi?id=1219522
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
drivers/input/mouse/psmouse-base.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
index a0aac76b1e41..fdeee7578d18 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -120,6 +120,8 @@ struct psmouse *psmouse_from_serio(struct serio *serio)
{
struct ps2dev *ps2dev = serio_get_drvdata(serio);
+ if (!ps2dev)
+ return NULL;
return container_of(ps2dev, struct psmouse, ps2dev);
}
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 14/19] i2c: nomadik: drop owner assignment
From: Wolfram Sang @ 2024-04-05 8:53 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Russell King, Suzuki K Poulose, Mike Leach, James Clark,
Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Andi Shyti, Olivia Mackall, Herbert Xu, Vinod Koul,
Dmitry Torokhov, Miquel Raynal, Michal Simek, Eric Auger,
Alex Williamson, linux-kernel, coresight, linux-arm-kernel,
linux-stm32, linux-i2c, linux-crypto, dmaengine, linux-input, kvm
In-Reply-To: <20240326-module-owner-amba-v1-14-4517b091385b@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 277 bytes --]
On Tue, Mar 26, 2024 at 09:23:44PM +0100, Krzysztof Kozlowski wrote:
> Amba bus core already sets owner, so driver does not need to.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] mm: change inlined allocation helpers to account at the call site
From: Jan Kara @ 2024-04-05 9:53 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Kent Overstreet, Andrew Morton, Matthew Wilcox, joro, will,
trond.myklebust, anna, arnd, herbert, davem, jikos,
benjamin.tissoires, tytso, jack, dennis, tj, cl, jakub, penberg,
rientjes, iamjoonsoo.kim, vbabka, edumazet, kuba, pabeni, iommu,
linux-kernel, linux-nfs, linux-acpi, acpica-devel, linux-arch,
linux-crypto, bpf, linux-input, linux-ext4, linux-mm, netdev,
linux-security-module
In-Reply-To: <CAJuCfpF10COO2nh1nt3CcaZOFe4iSXszsup+a0qAEQ1ngyy5tQ@mail.gmail.com>
On Thu 04-04-24 16:16:15, Suren Baghdasaryan wrote:
> On Thu, Apr 4, 2024 at 4:01 PM Kent Overstreet
> <kent.overstreet@linux.dev> wrote:
> >
> > On Thu, Apr 04, 2024 at 03:41:50PM -0700, Andrew Morton wrote:
> > > On Thu, 4 Apr 2024 18:38:39 -0400 Kent Overstreet <kent.overstreet@linux.dev> wrote:
> > >
> > > > On Thu, Apr 04, 2024 at 11:33:22PM +0100, Matthew Wilcox wrote:
> > > > > On Thu, Apr 04, 2024 at 03:17:43PM -0700, Suren Baghdasaryan wrote:
> > > > > > Ironically, checkpatch generates warnings for these type casts:
> > > > > >
> > > > > > WARNING: unnecessary cast may hide bugs, see
> > > > > > http://c-faq.com/malloc/mallocnocast.html
> > > > > > #425: FILE: include/linux/dma-fence-chain.h:90:
> > > > > > + ((struct dma_fence_chain *)kmalloc(sizeof(struct dma_fence_chain),
> > > > > > GFP_KERNEL))
> > > > > >
> > > > > > I guess I can safely ignore them in this case (since we cast to the
> > > > > > expected type)?
> > > > >
> > > > > I find ignoring checkpatch to be a solid move 99% of the time.
> > > > >
> > > > > I really don't like the codetags. This is so much churn, and it could
> > > > > all be avoided by just passing in _RET_IP_ or _THIS_IP_ depending on
> > > > > whether we wanted to profile this function or its caller. vmalloc
> > > > > has done it this way since 2008 (OK, using __builtin_return_address())
> > > > > and lockdep has used _THIS_IP_ / _RET_IP_ since 2006.
> > > >
> > > > Except you can't. We've been over this; using that approach for tracing
> > > > is one thing, using it for actual accounting isn't workable.
> > >
> > > I missed that. There have been many emails. Please remind us of the
> > > reasoning here.
> >
> > I think it's on the other people claiming 'oh this would be so easy if
> > you just do it this other way' to put up some code - or at least more
> > than hot takes.
> >
> > But, since you asked - one of the main goals of this patchset was to be
> > fast enough to run in production, and if you do it by return address
> > then you've added at minimum a hash table lookup to every allocate and
> > free; if you do that, running it in production is completely out of the
> > question.
> >
> > Besides that - the issues with annotating and tracking the correct
> > callsite really don't go away, they just shift around a bit. It's true
> > that the return address approach would be easier initially, but that's
> > not all we're concerned with; we're concerned with making sure
> > allocations get accounted to the _correct_ callsite so that we're giving
> > numbers that you can trust, and by making things less explicit you make
> > that harder.
> >
> > Additionally: the alloc_hooks() macro is for more than this. It's also
> > for more usable fault injection - remember every thread we have where
> > people are begging for every allocation to be __GFP_NOFAIL - "oh, error
> > paths are hard to test, let's just get rid of them" - never mind that
> > actually do have to have error paths - but _per callsite_ selectable
> > fault injection will actually make it practical to test memory error
> > paths.
> >
> > And Kees working on stuff that'll make use of the alloc_hooks() macro
> > for segregating kmem_caches.
>
> Yeah, that pretty much summarizes it. Note that we don't have to make
> the conversions in this patch and accounting will still work but then
> all allocations from different callers will be accounted to the helper
> function and that's less useful than accounting at the call site.
> It's a sizable churn but the conversions are straight-forward and we
> do get accurate, performant and easy to use memory accounting.
OK, fair enough. I guess I can live with the allocation macros in jbd2 if
type safety is preserved. But please provide a short summary of why we need
these macros (e.g. instead of RET_IP approach) in the changelog (or at
least a link to some email explaining this if the explanation would get too
long). Because I was wondering about the same as Andrew (and yes, this is
because I wasn't really following the huge discussion last time).
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition
From: Lukasz Majczak @ 2024-04-05 10:24 UTC (permalink / raw)
To: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Kai-Heng Feng,
Johan Hovold, linux-input, linux-kernel
Cc: Lukasz Majczak, Radoslaw Biernacki
This patch extends the early bailout for probing procedure introduced in
commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before
really probing"), in order to cover devices
based on STM microcontrollers. For touchpads based on STM uC,
the probe sequence needs to take into account the increased response time
for i2c transaction if the device already entered a deep power state.
STM specify the wakeup time as 400us between positive strobe of
the clock line. Deep sleep is controlled by Touchpad FW,
though some devices enter it pretty early to conserve power
in case of lack of activity on the i2c bus.
Failing to follow this requirement will result in touchpad device not being
detected due initial transaction being dropped by STM i2c slave controller.
By adding additional try, first transaction will wake up the touchpad
STM controller, and the second will produce proper detection response.
v1->v2:
* Updated commit message with short sha of a base commit and proper tags
* Rearranged while loop to perform check only once
* Loosened sleeping range
Co-developed-by: Radoslaw Biernacki <rad@chromium.org>
Signed-off-by: Radoslaw Biernacki <rad@chromium.org>
Signed-off-by: Lukasz Majczak <lma@chromium.org>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 2df1ab3c31cc..ece1a5815e0b 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1013,9 +1013,17 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
struct i2c_client *client = ihid->client;
struct hid_device *hid = ihid->hid;
int ret;
+ int tries = 2;
+
+ while (true) {
+ /* Make sure there is something at this address */
+ ret = i2c_smbus_read_byte(client);
+ tries--;
+ if (tries == 0 || ret >= 0)
+ break;
+ usleep_range(400, 500);
+ }
- /* 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);
return -ENXIO;
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related
* Re: [PATCH 1/1] mm: change inlined allocation helpers to account at the call site
From: Matthew Wilcox @ 2024-04-05 12:44 UTC (permalink / raw)
To: Kent Overstreet
Cc: Andrew Morton, Suren Baghdasaryan, joro, will, trond.myklebust,
anna, arnd, herbert, davem, jikos, benjamin.tissoires, tytso,
jack, dennis, tj, cl, jakub, penberg, rientjes, iamjoonsoo.kim,
vbabka, edumazet, kuba, pabeni, iommu, linux-kernel, linux-nfs,
linux-acpi, acpica-devel, linux-arch, linux-crypto, bpf,
linux-input, linux-ext4, linux-mm, netdev, linux-security-module
In-Reply-To: <jpaw4hdd73ngt7mvtcdryqscivx6m2ic76ikfkcopceb47becp@vox5czt5bec3>
On Thu, Apr 04, 2024 at 07:00:51PM -0400, Kent Overstreet wrote:
> On Thu, Apr 04, 2024 at 03:41:50PM -0700, Andrew Morton wrote:
> > On Thu, 4 Apr 2024 18:38:39 -0400 Kent Overstreet <kent.overstreet@linux.dev> wrote:
> >
> > > On Thu, Apr 04, 2024 at 11:33:22PM +0100, Matthew Wilcox wrote:
> > > > On Thu, Apr 04, 2024 at 03:17:43PM -0700, Suren Baghdasaryan wrote:
> > > > > Ironically, checkpatch generates warnings for these type casts:
> > > > >
> > > > > WARNING: unnecessary cast may hide bugs, see
> > > > > http://c-faq.com/malloc/mallocnocast.html
> > > > > #425: FILE: include/linux/dma-fence-chain.h:90:
> > > > > + ((struct dma_fence_chain *)kmalloc(sizeof(struct dma_fence_chain),
> > > > > GFP_KERNEL))
> > > > >
> > > > > I guess I can safely ignore them in this case (since we cast to the
> > > > > expected type)?
> > > >
> > > > I find ignoring checkpatch to be a solid move 99% of the time.
> > > >
> > > > I really don't like the codetags. This is so much churn, and it could
> > > > all be avoided by just passing in _RET_IP_ or _THIS_IP_ depending on
> > > > whether we wanted to profile this function or its caller. vmalloc
> > > > has done it this way since 2008 (OK, using __builtin_return_address())
> > > > and lockdep has used _THIS_IP_ / _RET_IP_ since 2006.
> > >
> > > Except you can't. We've been over this; using that approach for tracing
> > > is one thing, using it for actual accounting isn't workable.
> >
> > I missed that. There have been many emails. Please remind us of the
> > reasoning here.
>
> I think it's on the other people claiming 'oh this would be so easy if
> you just do it this other way' to put up some code - or at least more
> than hot takes.
Well, /proc/vmallocinfo exists, and has existed since 2008, so this is
slightly more than a "hot take".
> But, since you asked - one of the main goals of this patchset was to be
> fast enough to run in production, and if you do it by return address
> then you've added at minimum a hash table lookup to every allocate and
> free; if you do that, running it in production is completely out of the
> question.
And yet vmalloc doesn't do that.
> Besides that - the issues with annotating and tracking the correct
> callsite really don't go away, they just shift around a bit. It's true
> that the return address approach would be easier initially, but that's
> not all we're concerned with; we're concerned with making sure
> allocations get accounted to the _correct_ callsite so that we're giving
> numbers that you can trust, and by making things less explicit you make
> that harder.
I'm not convinced that _THIS_IP_ is less precise than a codetag. They
do essentially the same thing, except that codetags embed the source
location in the file while _THIS_IP_ requires a tool like faddr2line
to decode kernel_clone+0xc0/0x430 into a file + line number.
> This is all stuff that I've explained before; let's please dial back on
> the whining - or I'll just bookmark this for next time...
Please stop mischaracterising serious thoughtful criticism as whining.
I don't understand what value codetags bring over using _THIS_IP_ and
_RET_IP_ and you need to explain that.
^ permalink raw reply
* Re: [PATCH 1/1] mm: change inlined allocation helpers to account at the call site
From: Suren Baghdasaryan @ 2024-04-05 13:47 UTC (permalink / raw)
To: Jan Kara
Cc: Kent Overstreet, Andrew Morton, Matthew Wilcox, joro, will,
trond.myklebust, anna, arnd, herbert, davem, jikos,
benjamin.tissoires, tytso, jack, dennis, tj, cl, jakub, penberg,
rientjes, iamjoonsoo.kim, vbabka, edumazet, kuba, pabeni, iommu,
linux-kernel, linux-nfs, linux-acpi, acpica-devel, linux-arch,
linux-crypto, bpf, linux-input, linux-ext4, linux-mm, netdev,
linux-security-module
In-Reply-To: <20240405095327.ufsimeplwahh6mem@quack3>
On Fri, Apr 5, 2024 at 2:53 AM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 04-04-24 16:16:15, Suren Baghdasaryan wrote:
> > On Thu, Apr 4, 2024 at 4:01 PM Kent Overstreet
> > <kent.overstreet@linux.dev> wrote:
> > >
> > > On Thu, Apr 04, 2024 at 03:41:50PM -0700, Andrew Morton wrote:
> > > > On Thu, 4 Apr 2024 18:38:39 -0400 Kent Overstreet <kent.overstreet@linux.dev> wrote:
> > > >
> > > > > On Thu, Apr 04, 2024 at 11:33:22PM +0100, Matthew Wilcox wrote:
> > > > > > On Thu, Apr 04, 2024 at 03:17:43PM -0700, Suren Baghdasaryan wrote:
> > > > > > > Ironically, checkpatch generates warnings for these type casts:
> > > > > > >
> > > > > > > WARNING: unnecessary cast may hide bugs, see
> > > > > > > http://c-faq.com/malloc/mallocnocast.html
> > > > > > > #425: FILE: include/linux/dma-fence-chain.h:90:
> > > > > > > + ((struct dma_fence_chain *)kmalloc(sizeof(struct dma_fence_chain),
> > > > > > > GFP_KERNEL))
> > > > > > >
> > > > > > > I guess I can safely ignore them in this case (since we cast to the
> > > > > > > expected type)?
> > > > > >
> > > > > > I find ignoring checkpatch to be a solid move 99% of the time.
> > > > > >
> > > > > > I really don't like the codetags. This is so much churn, and it could
> > > > > > all be avoided by just passing in _RET_IP_ or _THIS_IP_ depending on
> > > > > > whether we wanted to profile this function or its caller. vmalloc
> > > > > > has done it this way since 2008 (OK, using __builtin_return_address())
> > > > > > and lockdep has used _THIS_IP_ / _RET_IP_ since 2006.
> > > > >
> > > > > Except you can't. We've been over this; using that approach for tracing
> > > > > is one thing, using it for actual accounting isn't workable.
> > > >
> > > > I missed that. There have been many emails. Please remind us of the
> > > > reasoning here.
> > >
> > > I think it's on the other people claiming 'oh this would be so easy if
> > > you just do it this other way' to put up some code - or at least more
> > > than hot takes.
> > >
> > > But, since you asked - one of the main goals of this patchset was to be
> > > fast enough to run in production, and if you do it by return address
> > > then you've added at minimum a hash table lookup to every allocate and
> > > free; if you do that, running it in production is completely out of the
> > > question.
> > >
> > > Besides that - the issues with annotating and tracking the correct
> > > callsite really don't go away, they just shift around a bit. It's true
> > > that the return address approach would be easier initially, but that's
> > > not all we're concerned with; we're concerned with making sure
> > > allocations get accounted to the _correct_ callsite so that we're giving
> > > numbers that you can trust, and by making things less explicit you make
> > > that harder.
> > >
> > > Additionally: the alloc_hooks() macro is for more than this. It's also
> > > for more usable fault injection - remember every thread we have where
> > > people are begging for every allocation to be __GFP_NOFAIL - "oh, error
> > > paths are hard to test, let's just get rid of them" - never mind that
> > > actually do have to have error paths - but _per callsite_ selectable
> > > fault injection will actually make it practical to test memory error
> > > paths.
> > >
> > > And Kees working on stuff that'll make use of the alloc_hooks() macro
> > > for segregating kmem_caches.
> >
> > Yeah, that pretty much summarizes it. Note that we don't have to make
> > the conversions in this patch and accounting will still work but then
> > all allocations from different callers will be accounted to the helper
> > function and that's less useful than accounting at the call site.
> > It's a sizable churn but the conversions are straight-forward and we
> > do get accurate, performant and easy to use memory accounting.
>
> OK, fair enough. I guess I can live with the allocation macros in jbd2 if
> type safety is preserved. But please provide a short summary of why we need
> these macros (e.g. instead of RET_IP approach) in the changelog (or at
> least a link to some email explaining this if the explanation would get too
> long). Because I was wondering about the same as Andrew (and yes, this is
> because I wasn't really following the huge discussion last time).
Ack. I'll write up the explanation or if there is a good one already
in our previous discussion will add a link to it. Thanks!
>
> Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 1/1] mm: change inlined allocation helpers to account at the call site
From: Suren Baghdasaryan @ 2024-04-05 13:53 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Kent Overstreet, Andrew Morton, joro, will, trond.myklebust, anna,
arnd, herbert, davem, jikos, benjamin.tissoires, tytso, jack,
dennis, tj, cl, jakub, penberg, rientjes, iamjoonsoo.kim, vbabka,
edumazet, kuba, pabeni, iommu, linux-kernel, linux-nfs,
linux-acpi, acpica-devel, linux-arch, linux-crypto, bpf,
linux-input, linux-ext4, linux-mm, netdev, linux-security-module
In-Reply-To: <Zg_yHGKpw4HJHdpb@casper.infradead.org>
On Fri, Apr 5, 2024 at 5:44 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Apr 04, 2024 at 07:00:51PM -0400, Kent Overstreet wrote:
> > On Thu, Apr 04, 2024 at 03:41:50PM -0700, Andrew Morton wrote:
> > > On Thu, 4 Apr 2024 18:38:39 -0400 Kent Overstreet <kent.overstreet@linux.dev> wrote:
> > >
> > > > On Thu, Apr 04, 2024 at 11:33:22PM +0100, Matthew Wilcox wrote:
> > > > > On Thu, Apr 04, 2024 at 03:17:43PM -0700, Suren Baghdasaryan wrote:
> > > > > > Ironically, checkpatch generates warnings for these type casts:
> > > > > >
> > > > > > WARNING: unnecessary cast may hide bugs, see
> > > > > > http://c-faq.com/malloc/mallocnocast.html
> > > > > > #425: FILE: include/linux/dma-fence-chain.h:90:
> > > > > > + ((struct dma_fence_chain *)kmalloc(sizeof(struct dma_fence_chain),
> > > > > > GFP_KERNEL))
> > > > > >
> > > > > > I guess I can safely ignore them in this case (since we cast to the
> > > > > > expected type)?
> > > > >
> > > > > I find ignoring checkpatch to be a solid move 99% of the time.
> > > > >
> > > > > I really don't like the codetags. This is so much churn, and it could
> > > > > all be avoided by just passing in _RET_IP_ or _THIS_IP_ depending on
> > > > > whether we wanted to profile this function or its caller. vmalloc
> > > > > has done it this way since 2008 (OK, using __builtin_return_address())
> > > > > and lockdep has used _THIS_IP_ / _RET_IP_ since 2006.
> > > >
> > > > Except you can't. We've been over this; using that approach for tracing
> > > > is one thing, using it for actual accounting isn't workable.
> > >
> > > I missed that. There have been many emails. Please remind us of the
> > > reasoning here.
> >
> > I think it's on the other people claiming 'oh this would be so easy if
> > you just do it this other way' to put up some code - or at least more
> > than hot takes.
>
> Well, /proc/vmallocinfo exists, and has existed since 2008, so this is
> slightly more than a "hot take".
>
> > But, since you asked - one of the main goals of this patchset was to be
> > fast enough to run in production, and if you do it by return address
> > then you've added at minimum a hash table lookup to every allocate and
> > free; if you do that, running it in production is completely out of the
> > question.
>
> And yet vmalloc doesn't do that.
>
> > Besides that - the issues with annotating and tracking the correct
> > callsite really don't go away, they just shift around a bit. It's true
> > that the return address approach would be easier initially, but that's
> > not all we're concerned with; we're concerned with making sure
> > allocations get accounted to the _correct_ callsite so that we're giving
> > numbers that you can trust, and by making things less explicit you make
> > that harder.
>
> I'm not convinced that _THIS_IP_ is less precise than a codetag. They
> do essentially the same thing, except that codetags embed the source
> location in the file while _THIS_IP_ requires a tool like faddr2line
> to decode kernel_clone+0xc0/0x430 into a file + line number.
>
> > This is all stuff that I've explained before; let's please dial back on
> > the whining - or I'll just bookmark this for next time...
>
> Please stop mischaracterising serious thoughtful criticism as whining.
> I don't understand what value codetags bring over using _THIS_IP_ and
> _RET_IP_ and you need to explain that.
The conceptual difference between codetag and _THIS_IP_/_RET_IP_ is
that codetag injects counters at the call site, so you don't need to
spend time finding the appropriate counter to operate on during
allocation.
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add ft5426
From: Andy Shevchenko @ 2024-04-05 15:13 UTC (permalink / raw)
To: Andreas Kemnade
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
o.rempel, u.kleine-koenig, hdegoede, ye.xingchen, p.puschmann,
linux-input, devicetree, linux-kernel, caleb.connolly
In-Reply-To: <20240404222009.670685-3-andreas@kemnade.info>
On Fri, Apr 5, 2024 at 1:20 AM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> As ft5426 seems to be compatible with this driver, add it.
> Debug output during identification: Model "generic ft5x06 (79)", Rev. "
...
> @@ -1484,6 +1484,7 @@ static const struct of_device_id edt_ft5x06_of_match[] = {
> { .compatible = "edt,edt-ft5206", .data = &edt_ft5x06_data },
> { .compatible = "edt,edt-ft5306", .data = &edt_ft5x06_data },
> { .compatible = "edt,edt-ft5406", .data = &edt_ft5x06_data },
> + { .compatible = "focaltech,ft5426", .data = &edt_ft5506_data },
Why a different vendor prefix?
In case you need to use this one, keep the list sorted, currently this
splits the edt,* ones.
> { .compatible = "edt,edt-ft5506", .data = &edt_ft5506_data },
> { .compatible = "evervision,ev-ft5726", .data = &edt_ft5506_data },
> /* Note focaltech vendor prefix for compatibility with ft6236.c */
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add ft5426
From: Andreas Kemnade @ 2024-04-05 16:28 UTC (permalink / raw)
To: Andy Shevchenko
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
o.rempel, u.kleine-koenig, hdegoede, ye.xingchen, p.puschmann,
linux-input, devicetree, linux-kernel, caleb.connolly
In-Reply-To: <CAHp75VeZ9U_+1rJQjr4KvvzjYQGzfKtk+BK00vqvKcVn2-yP3g@mail.gmail.com>
On Fri, 5 Apr 2024 18:13:45 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Fri, Apr 5, 2024 at 1:20 AM Andreas Kemnade <andreas@kemnade.info> wrote:
> >
> > As ft5426 seems to be compatible with this driver, add it.
> > Debug output during identification: Model "generic ft5x06 (79)", Rev. "
>
> ...
>
> > @@ -1484,6 +1484,7 @@ static const struct of_device_id edt_ft5x06_of_match[] = {
> > { .compatible = "edt,edt-ft5206", .data = &edt_ft5x06_data },
> > { .compatible = "edt,edt-ft5306", .data = &edt_ft5x06_data },
> > { .compatible = "edt,edt-ft5406", .data = &edt_ft5x06_data },
> > + { .compatible = "focaltech,ft5426", .data = &edt_ft5506_data },
>
> Why a different vendor prefix?
> In case you need to use this one, keep the list sorted, currently this
> splits the edt,* ones.
>
How do I know whether to use evervision or edt instead?
I sorted by the numbers. Looking at datasheets for other controllers I see
https://www.displayfuture.com/Display/datasheet/controller/FT5x06.pdf
it only mentions FocalTech Systems Co., Ltd.
So how the vendor prefixes are derived?
Regards,
Andreas
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add ft5426
From: Andy Shevchenko @ 2024-04-05 17:21 UTC (permalink / raw)
To: Andreas Kemnade
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
o.rempel, u.kleine-koenig, hdegoede, ye.xingchen, p.puschmann,
linux-input, devicetree, linux-kernel, caleb.connolly
In-Reply-To: <20240405182832.4e457695@aktux>
On Fri, Apr 5, 2024 at 7:28 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> On Fri, 5 Apr 2024 18:13:45 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Apr 5, 2024 at 1:20 AM Andreas Kemnade <andreas@kemnade.info> wrote:
...
> > > @@ -1484,6 +1484,7 @@ static const struct of_device_id edt_ft5x06_of_match[] = {
> > > { .compatible = "edt,edt-ft5206", .data = &edt_ft5x06_data },
> > > { .compatible = "edt,edt-ft5306", .data = &edt_ft5x06_data },
> > > { .compatible = "edt,edt-ft5406", .data = &edt_ft5x06_data },
> > > + { .compatible = "focaltech,ft5426", .data = &edt_ft5506_data },
> >
> > Why a different vendor prefix?
> > In case you need to use this one, keep the list sorted, currently this
> > splits the edt,* ones.
> >
> How do I know whether to use evervision or edt instead?
Ask DT people, the vendor-prefixes lists both...
> I sorted by the numbers. Looking at datasheets for other controllers I see
> https://www.displayfuture.com/Display/datasheet/controller/FT5x06.pdf
> it only mentions FocalTech Systems Co., Ltd.
But does the driver use that? AFAICS it uses edt. Perhaps it's due to
a business split, not to my knowledge anyway.
> So how the vendor prefixes are derived?
Rob, Krzysztof?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add ft5426
From: Andreas Kemnade @ 2024-04-05 18:56 UTC (permalink / raw)
To: Andy Shevchenko
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
o.rempel, u.kleine-koenig, hdegoede, ye.xingchen, p.puschmann,
linux-input, devicetree, linux-kernel, caleb.connolly
In-Reply-To: <CAHp75VckoDheCN-KQ0KcSk9rE_-cXFUujurtA4sK6KAixDttQQ@mail.gmail.com>
On Fri, 5 Apr 2024 20:21:19 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Fri, Apr 5, 2024 at 7:28 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> > On Fri, 5 Apr 2024 18:13:45 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Fri, Apr 5, 2024 at 1:20 AM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> ...
>
> > > > @@ -1484,6 +1484,7 @@ static const struct of_device_id edt_ft5x06_of_match[] = {
> > > > { .compatible = "edt,edt-ft5206", .data = &edt_ft5x06_data },
> > > > { .compatible = "edt,edt-ft5306", .data = &edt_ft5x06_data },
> > > > { .compatible = "edt,edt-ft5406", .data = &edt_ft5x06_data },
> > > > + { .compatible = "focaltech,ft5426", .data = &edt_ft5506_data },
> > >
> > > Why a different vendor prefix?
> > > In case you need to use this one, keep the list sorted, currently this
> > > splits the edt,* ones.
> > >
> > How do I know whether to use evervision or edt instead?
>
> Ask DT people, the vendor-prefixes lists both...
>
> > I sorted by the numbers. Looking at datasheets for other controllers I see
> > https://www.displayfuture.com/Display/datasheet/controller/FT5x06.pdf
> > it only mentions FocalTech Systems Co., Ltd.
>
> But does the driver use that? AFAICS it uses edt. Perhaps it's due to
> a business split, not to my knowledge anyway.
>
well, the fact is that there were several tried to add duplicates to this
driver to the kernel using focaltech prefixes e.g.
https://lore.kernel.org/linux-input/47209259-9e57-f263-bf48-10f233c63b69@redhat.com/
My guess it is somehow about owner of the firmware in the chip vs the chip itself.
Regards,
Andreas
^ permalink raw reply
* Re: [PATCH 00/34] address all -Wunused-const warnings
From: patchwork-bot+netdevbpf @ 2024-04-06 5:20 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, arnd, mpe, christophe.leroy, dlemoal, jikos, gregkh,
minyard, peterhuewe, jarkko, kristo, sboyd, abbotti, hsweeten,
srinivas.pandruvada, lenb, rafael, john.allen, herbert, vkoul,
ardb, andersson, mdf, liviu.dudau, benjamin.tissoires, andi.shyti,
michael.hennerich, peda, lars, jic23, dmitry.torokhov,
markuss.broks, alexandre.torgue, lee, kuba, Shyam-sundar.S-k,
iyappan, yisen.zhuang, stf_xl, kvalo, sre, tony, broonie,
alexandre.belloni, chenxiang66, martin.petersen, neil.armstrong,
heiko, krzysztof.kozlowski, hvaibhav.linux, elder, jirislaby,
ychuang3, deller, hch, robin.murphy, rostedt, mhiramat, akpm,
keescook, trond.myklebust, anna, masahiroy, nathan, tiwai,
linuxppc-dev, linux-ide, openipmi-developer, linux-integrity,
linux-omap, linux-clk, linux-pm, linux-crypto, dmaengine,
linux-efi, linux-arm-msm, linux-fpga, dri-devel, linux-input,
linux-i2c, linux-iio, linux-stm32, linux-arm-kernel, netdev,
linux-leds, linux-wireless, linux-rtc, linux-scsi, linux-spi,
linux-amlogic, linux-rockchip, linux-samsung-soc, greybus-dev,
linux-staging, linux-serial, linux-usb, linux-fbdev, iommu,
linux-trace-kernel, kasan-dev, linux-hardening, linux-nfs,
linux-kbuild, alsa-devel, linux-sound
In-Reply-To: <20240403080702.3509288-1-arnd@kernel.org>
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 3 Apr 2024 10:06:18 +0200 you wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> Compilers traditionally warn for unused 'static' variables, but not
> if they are constant. The reason here is a custom for C++ programmers
> to define named constants as 'static const' variables in header files
> instead of using macros or enums.
>
> [...]
Here is the summary with links:
- [05/34] 3c515: remove unused 'mtu' variable
https://git.kernel.org/netdev/net-next/c/17b35355c2c6
- [19/34] sunrpc: suppress warnings for unused procfs functions
(no matching commit)
- [26/34] isdn: kcapi: don't build unused procfs code
https://git.kernel.org/netdev/net-next/c/91188544af06
- [28/34] net: xgbe: remove extraneous #ifdef checks
https://git.kernel.org/netdev/net-next/c/0ef416e045ad
- [33/34] drivers: remove incorrect of_match_ptr/ACPI_PTR annotations
(no matching commit)
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* [PATCH] HID: nintendo: Remove unused function
From: Jiapeng Chong @ 2024-04-07 2:11 UTC (permalink / raw)
To: djogorchock
Cc: jikos, bentiss, linux-input, linux-kernel, Jiapeng Chong,
Abaci Robot
The function are defined in the hid-nintendo.c file, but not called
elsewhere, so delete the unused function.
drivers/hid/hid-nintendo.c:697:20: warning: unused function 'joycon_device_has_usb'.
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=8704
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
---
drivers/hid/hid-nintendo.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 4b4ca990f968..b4a97803eca3 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -694,15 +694,6 @@ static inline bool joycon_device_is_n64con(struct joycon_ctlr *ctlr)
return ctlr->hdev->product == USB_DEVICE_ID_NINTENDO_N64CON;
}
-static inline bool joycon_device_has_usb(struct joycon_ctlr *ctlr)
-{
- return joycon_device_is_procon(ctlr) ||
- joycon_device_is_chrggrip(ctlr) ||
- joycon_device_is_snescon(ctlr) ||
- joycon_device_is_gencon(ctlr) ||
- joycon_device_is_n64con(ctlr);
-}
-
/*
* Controller type helpers
*
--
2.20.1.7.g153144c
^ permalink raw reply related
* [PATCH] HID: Remove the unused variable minor
From: Jiapeng Chong @ 2024-04-07 2:28 UTC (permalink / raw)
To: jikos; +Cc: bentiss, linux-input, linux-kernel, Jiapeng Chong, Abaci Robot
Variable minor is not effectively used, so delete it.
drivers/hid/hid-winwing.c:123:15: warning: variable 'minor' set but not used.
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=8705
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
---
drivers/hid/hid-winwing.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/hid/hid-winwing.c b/drivers/hid/hid-winwing.c
index d895c82a541d..0e224d1a6466 100644
--- a/drivers/hid/hid-winwing.c
+++ b/drivers/hid/hid-winwing.c
@@ -120,7 +120,6 @@ static int winwing_init_led(struct hid_device *hdev,
static int winwing_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
- unsigned int minor;
int ret;
ret = hid_parse(hdev);
@@ -135,8 +134,6 @@ static int winwing_probe(struct hid_device *hdev,
return ret;
}
- minor = ((struct hidraw *) hdev->hidraw)->minor;
-
return 0;
}
--
2.20.1.7.g153144c
^ permalink raw reply related
* [PATCH] HID: hid-steam: Add Deck IMU support
From: Max Maisel @ 2024-04-07 12:19 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input, linux-kernel; +Cc: mmm-1
The Deck's controller features an accelerometer and gyroscope which
send their measurement values by default in the main HID input report.
Expose both sensors to userspace through a separate evdev node as it
is done by the hid-nintendo and hid-playstation drivers.
Signed-off-by: Max Maisel <mmm-1@posteo.net>
---
This patch was tested on a Steam Deck running Arch Linux. With it,
applications using latest SDL2/3 git libraries will pick up the sensors
without hidraw access. This was tested against the antimicrox gamepad mapper.
Measurement value scaling was tested by moving the deck and a dualsense
controller simultaneously and comparing their reported values in
userspace with SDL3's testcontroller tool.
drivers/hid/hid-steam.c | 158 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 150 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index b08a5ab58528..af6e6c3b1356 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -66,6 +66,12 @@ static LIST_HEAD(steam_devices);
#define STEAM_DECK_TRIGGER_RESOLUTION 5461
/* Joystick runs are about 5 mm and 32768 units */
#define STEAM_DECK_JOYSTICK_RESOLUTION 6553
+/* Accelerometer has 16 bit resolution and a range of +/- 2g */
+#define STEAM_DECK_ACCEL_RES_PER_G 16384
+#define STEAM_DECK_ACCEL_RANGE 32768
+/* Gyroscope has 16 bit resolution and a range of +/- 2000 dps */
+#define STEAM_DECK_GYRO_RES_PER_DPS 16
+#define STEAM_DECK_GYRO_RANGE 32000
#define STEAM_PAD_FUZZ 256
@@ -288,6 +294,7 @@ struct steam_device {
struct mutex report_mutex;
unsigned long client_opened;
struct input_dev __rcu *input;
+ struct input_dev __rcu *sensors;
unsigned long quirks;
struct work_struct work_connect;
bool connected;
@@ -302,6 +309,7 @@ struct steam_device {
struct work_struct rumble_work;
u16 rumble_left;
u16 rumble_right;
+ unsigned int sensor_timestamp_us;
};
static int steam_recv_report(struct steam_device *steam,
@@ -825,6 +833,74 @@ static int steam_input_register(struct steam_device *steam)
return ret;
}
+static int steam_sensors_register(struct steam_device *steam)
+{
+ struct hid_device *hdev = steam->hdev;
+ struct input_dev *sensors;
+ int ret;
+
+ if (!(steam->quirks & STEAM_QUIRK_DECK))
+ return 0;
+
+ rcu_read_lock();
+ sensors = rcu_dereference(steam->sensors);
+ rcu_read_unlock();
+ if (sensors) {
+ dbg_hid("%s: already connected\n", __func__);
+ return 0;
+ }
+
+ sensors = input_allocate_device();
+ if (!sensors)
+ return -ENOMEM;
+
+ input_set_drvdata(sensors, steam);
+ sensors->dev.parent = &hdev->dev;
+
+ sensors->name = "Steam Deck Motion Sensors";
+ sensors->phys = hdev->phys;
+ sensors->uniq = steam->serial_no;
+ sensors->id.bustype = hdev->bus;
+ sensors->id.vendor = hdev->vendor;
+ sensors->id.product = hdev->product;
+ sensors->id.version = hdev->version;
+
+ __set_bit(INPUT_PROP_ACCELEROMETER, sensors->propbit);
+ __set_bit(EV_MSC, sensors->evbit);
+ __set_bit(MSC_TIMESTAMP, sensors->mscbit);
+
+ input_set_abs_params(sensors, ABS_X, -STEAM_DECK_ACCEL_RANGE,
+ STEAM_DECK_ACCEL_RANGE, 16, 0);
+ input_set_abs_params(sensors, ABS_Y, -STEAM_DECK_ACCEL_RANGE,
+ STEAM_DECK_ACCEL_RANGE, 16, 0);
+ input_set_abs_params(sensors, ABS_Z, -STEAM_DECK_ACCEL_RANGE,
+ STEAM_DECK_ACCEL_RANGE, 16, 0);
+ input_abs_set_res(sensors, ABS_X, STEAM_DECK_ACCEL_RES_PER_G);
+ input_abs_set_res(sensors, ABS_Y, STEAM_DECK_ACCEL_RES_PER_G);
+ input_abs_set_res(sensors, ABS_Z, STEAM_DECK_ACCEL_RES_PER_G);
+
+ input_set_abs_params(sensors, ABS_RX, -STEAM_DECK_GYRO_RANGE,
+ STEAM_DECK_GYRO_RANGE, 16, 0);
+ input_set_abs_params(sensors, ABS_RY, -STEAM_DECK_GYRO_RANGE,
+ STEAM_DECK_GYRO_RANGE, 16, 0);
+ input_set_abs_params(sensors, ABS_RZ, -STEAM_DECK_GYRO_RANGE,
+ STEAM_DECK_GYRO_RANGE, 16, 0);
+ input_abs_set_res(sensors, ABS_RX, STEAM_DECK_GYRO_RES_PER_DPS);
+ input_abs_set_res(sensors, ABS_RY, STEAM_DECK_GYRO_RES_PER_DPS);
+ input_abs_set_res(sensors, ABS_RZ, STEAM_DECK_GYRO_RES_PER_DPS);
+
+ ret = input_register_device(sensors);
+ if (ret)
+ goto sensors_register_fail;
+
+ rcu_assign_pointer(steam->sensors, sensors);
+ return 0;
+
+sensors_register_fail:
+ input_free_device(sensors);
+ return ret;
+}
+
static void steam_input_unregister(struct steam_device *steam)
{
struct input_dev *input;
@@ -838,6 +914,24 @@ static void steam_input_unregister(struct steam_device *steam)
input_unregister_device(input);
}
+static void steam_sensors_unregister(struct steam_device *steam)
+{
+ struct input_dev *sensors;
+
+ if (!(steam->quirks & STEAM_QUIRK_DECK))
+ return;
+
+ rcu_read_lock();
+ sensors = rcu_dereference(steam->sensors);
+ rcu_read_unlock();
+
+ if (!sensors)
+ return;
+ RCU_INIT_POINTER(steam->sensors, NULL);
+ synchronize_rcu();
+ input_unregister_device(sensors);
+}
+
static void steam_battery_unregister(struct steam_device *steam)
{
struct power_supply *battery;
@@ -890,18 +984,28 @@ static int steam_register(struct steam_device *steam)
spin_lock_irqsave(&steam->lock, flags);
client_opened = steam->client_opened;
spin_unlock_irqrestore(&steam->lock, flags);
+
if (!client_opened) {
steam_set_lizard_mode(steam, lizard_mode);
ret = steam_input_register(steam);
- } else
- ret = 0;
+ if (ret != 0)
+ goto steam_register_input_fail;
+ ret = steam_sensors_register(steam);
+ if (ret != 0)
+ goto steam_register_sensors_fail;
+ }
+ return 0;
+steam_register_sensors_fail:
+ steam_input_unregister(steam);
+steam_register_input_fail:
return ret;
}
static void steam_unregister(struct steam_device *steam)
{
steam_battery_unregister(steam);
+ steam_sensors_unregister(steam);
steam_input_unregister(steam);
if (steam->serial_no[0]) {
hid_info(steam->hdev, "Steam Controller '%s' disconnected",
@@ -1010,6 +1114,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
steam->client_opened++;
spin_unlock_irqrestore(&steam->lock, flags);
+ steam_sensors_unregister(steam);
steam_input_unregister(steam);
return 0;
@@ -1030,6 +1135,7 @@ static void steam_client_ll_close(struct hid_device *hdev)
if (connected) {
steam_set_lizard_mode(steam, lizard_mode);
steam_input_register(steam);
+ steam_sensors_register(steam);
}
}
@@ -1121,6 +1227,7 @@ static int steam_probe(struct hid_device *hdev,
INIT_DELAYED_WORK(&steam->mode_switch, steam_mode_switch_cb);
INIT_LIST_HEAD(&steam->list);
INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb);
+ steam->sensor_timestamp_us = 0;
/*
* With the real steam controller interface, do not connect hidraw.
@@ -1380,12 +1487,12 @@ static void steam_do_input_event(struct steam_device *steam,
* 18-19 | s16 | ABS_HAT0Y | left-pad Y value
* 20-21 | s16 | ABS_HAT1X | right-pad X value
* 22-23 | s16 | ABS_HAT1Y | right-pad Y value
- * 24-25 | s16 | -- | accelerometer X value
- * 26-27 | s16 | -- | accelerometer Y value
- * 28-29 | s16 | -- | accelerometer Z value
- * 30-31 | s16 | -- | gyro X value
- * 32-33 | s16 | -- | gyro Y value
- * 34-35 | s16 | -- | gyro Z value
+ * 24-25 | s16 | IMU ABS_X | accelerometer X value
+ * 26-27 | s16 | IMU ABS_Z | accelerometer Y value
+ * 28-29 | s16 | IMU ABS_Y | accelerometer Z value
+ * 30-31 | s16 | IMU ABS_RX | gyro X value
+ * 32-33 | s16 | IMU ABS_RZ | gyro Y value
+ * 34-35 | s16 | IMU ABS_RY | gyro Z value
* 36-37 | s16 | -- | quaternion W value
* 38-39 | s16 | -- | quaternion X value
* 40-41 | s16 | -- | quaternion Y value
@@ -1546,6 +1653,32 @@ static void steam_do_deck_input_event(struct steam_device *steam,
input_sync(input);
}
+static void steam_do_deck_sensors_event(struct steam_device *steam,
+ struct input_dev *sensors, u8 *data)
+{
+ /*
+ * The deck input report is received every 4 ms on average,
+ * with a jitter of +/- 4 ms even though the USB descriptor claims
+ * that it uses 1 kHz.
+ * Since the HID report does not include a sensor timestamp,
+ * use a fixed increment here.
+ *
+ * The reported sensors data is factory calibrated by default so
+ * no extra logic for handling calibratrion is necessary.
+ */
+ steam->sensor_timestamp_us += 4000;
+ input_event(sensors, EV_MSC, MSC_TIMESTAMP, steam->sensor_timestamp_us);
+
+ input_report_abs(sensors, ABS_X, steam_le16(data + 24));
+ input_report_abs(sensors, ABS_Z, -steam_le16(data + 26));
+ input_report_abs(sensors, ABS_Y, steam_le16(data + 28));
+ input_report_abs(sensors, ABS_RX, steam_le16(data + 30));
+ input_report_abs(sensors, ABS_RZ, -steam_le16(data + 32));
+ input_report_abs(sensors, ABS_RY, steam_le16(data + 34));
+
+ input_sync(sensors);
+}
+
/*
* The size for this message payload is 11.
* The known values are:
@@ -1583,6 +1716,7 @@ static int steam_raw_event(struct hid_device *hdev,
{
struct steam_device *steam = hid_get_drvdata(hdev);
struct input_dev *input;
+ struct input_dev *sensors;
struct power_supply *battery;
if (!steam)
@@ -1629,6 +1763,14 @@ static int steam_raw_event(struct hid_device *hdev,
if (likely(input))
steam_do_deck_input_event(steam, input, data);
rcu_read_unlock();
+
+ if (steam->quirks & STEAM_QUIRK_DECK) {
+ rcu_read_lock();
+ sensors = rcu_dereference(steam->sensors);
+ if (likely(sensors))
+ steam_do_deck_sensors_event(steam, sensors, data);
+ rcu_read_unlock();
+ }
break;
case ID_CONTROLLER_WIRELESS:
/*
base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f
--
2.44.0
^ permalink raw reply related
* Re: [PATCH AUTOSEL 6.8 78/98] input/touchscreen: imagis: add support for IST3032C
From: Sasha Levin @ 2024-04-07 23:48 UTC (permalink / raw)
To: Karel Balej
Cc: Markuss Broks, Dmitry Torokhov, linux-input, duje.mihanovic,
linux-kernel, stable
In-Reply-To: <D06ZCKKYTQM5.3OJ6HCLHW3DZ9@matfyz.cz>
On Sat, Mar 30, 2024 at 10:33:53AM +0100, Karel Balej wrote:
>Sasha,
>
>Sasha Levin, 2024-03-29T08:37:49-04:00:
>> From: Karel Balej <balejk@matfyz.cz>
>>
>> [ Upstream commit 90cb57a6c5717b83a110c0da720a03ee32ed255e ]
>>
>> IST3032C is a touchscreen chip used for instance in the
>> samsung,coreprimevelte smartphone, with which this was tested. Add the
>> chip specific information to the driver.
>>
>> Reviewed-by: Markuss Broks <markuss.broks@gmail.com>
>> Signed-off-by: Karel Balej <balejk@matfyz.cz>
>> Link: https://lore.kernel.org/r/20240301164659.13240-6-karelb@gimli.ms.mff.cuni.cz
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>> ---
>> drivers/input/touchscreen/imagis.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
>> index 9af8a6332ae67..e1fafa561ee38 100644
>> --- a/drivers/input/touchscreen/imagis.c
>> +++ b/drivers/input/touchscreen/imagis.c
>> @@ -11,6 +11,8 @@
>> #include <linux/property.h>
>> #include <linux/regulator/consumer.h>
>>
>> +#define IST3032C_WHOAMI 0x32c
>> +
>> #define IST3038B_REG_STATUS 0x20
>> #define IST3038B_REG_CHIPID 0x30
>> #define IST3038B_WHOAMI 0x30380b
>> @@ -363,6 +365,13 @@ static int imagis_resume(struct device *dev)
>>
>> static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume);
>>
>> +static const struct imagis_properties imagis_3032c_data = {
>> + .interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE,
>> + .touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
>> + .whoami_cmd = IST3038C_REG_CHIPID,
>> + .whoami_val = IST3032C_WHOAMI,
>> +};
>> +
>> static const struct imagis_properties imagis_3038b_data = {
>> .interrupt_msg_cmd = IST3038B_REG_STATUS,
>> .touch_coord_cmd = IST3038B_REG_STATUS,
>> @@ -380,6 +389,7 @@ static const struct imagis_properties imagis_3038c_data = {
>>
>> #ifdef CONFIG_OF
>> static const struct of_device_id imagis_of_match[] = {
>> + { .compatible = "imagis,ist3032c", .data = &imagis_3032c_data },
>> { .compatible = "imagis,ist3038b", .data = &imagis_3038b_data },
>> { .compatible = "imagis,ist3038c", .data = &imagis_3038c_data },
>> { },
>> --
>> 2.43.0
>
>sorry if I'm missing something, but I don't see why this should be
>backported: it doesn't fix anything, it's just adding support for new
>hardware.
>
>I can see that adding a device ID is permitted for -stable [1], but I
>thought it still has to bear some signs of a fix, such as maybe here
>[2].
It does not need to be a fix, it could just be plain device enablement.
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition
From: Doug Anderson @ 2024-04-08 3:18 UTC (permalink / raw)
To: Lukasz Majczak
Cc: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires, Hans de Goede,
Maxime Ripard, Kai-Heng Feng, Johan Hovold, linux-input,
linux-kernel, Radoslaw Biernacki
In-Reply-To: <20240405102436.3479210-1-lma@chromium.org>
Hi,
On Fri, Apr 5, 2024 at 3:24 AM Lukasz Majczak <lma@chromium.org> wrote:
>
> This patch extends the early bailout for probing procedure introduced in
> commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before
> really probing"), in order to cover devices
> based on STM microcontrollers. For touchpads based on STM uC,
> the probe sequence needs to take into account the increased response time
> for i2c transaction if the device already entered a deep power state.
> STM specify the wakeup time as 400us between positive strobe of
> the clock line. Deep sleep is controlled by Touchpad FW,
> though some devices enter it pretty early to conserve power
> in case of lack of activity on the i2c bus.
> Failing to follow this requirement will result in touchpad device not being
> detected due initial transaction being dropped by STM i2c slave controller.
> By adding additional try, first transaction will wake up the touchpad
> STM controller, and the second will produce proper detection response.
>
> v1->v2:
> * Updated commit message with short sha of a base commit and proper tags
> * Rearranged while loop to perform check only once
> * Loosened sleeping range
>
> Co-developed-by: Radoslaw Biernacki <rad@chromium.org>
> Signed-off-by: Radoslaw Biernacki <rad@chromium.org>
> Signed-off-by: Lukasz Majczak <lma@chromium.org>
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
As per my review comments in response to your v1 [1], this seems
reasonable to me.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
[1] https://lore.kernel.org/r/20240325105452.529921-1-lma@chromium.org
^ permalink raw reply
* Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition
From: Johan Hovold @ 2024-04-08 7:27 UTC (permalink / raw)
To: Lukasz Majczak
Cc: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Kai-Heng Feng,
Johan Hovold, linux-input, linux-kernel, Radoslaw Biernacki
In-Reply-To: <20240405102436.3479210-1-lma@chromium.org>
On Fri, Apr 05, 2024 at 10:24:36AM +0000, Lukasz Majczak wrote:
> This patch extends the early bailout for probing procedure introduced in
> commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before
> really probing"), in order to cover devices
> based on STM microcontrollers. For touchpads based on STM uC,
> the probe sequence needs to take into account the increased response time
> for i2c transaction if the device already entered a deep power state.
> STM specify the wakeup time as 400us between positive strobe of
> the clock line. Deep sleep is controlled by Touchpad FW,
> though some devices enter it pretty early to conserve power
> in case of lack of activity on the i2c bus.
> Failing to follow this requirement will result in touchpad device not being
> detected due initial transaction being dropped by STM i2c slave controller.
> By adding additional try, first transaction will wake up the touchpad
> STM controller, and the second will produce proper detection response.
Can you please explain why this would not a problem for all future
transactions as well?
If it is, then it sounds like this needs to be addressed in the i2c
driver. If not, then perhaps the problem is really that you just need a
delay after enabling the power supplies?
> v1->v2:
> * Updated commit message with short sha of a base commit and proper tags
> * Rearranged while loop to perform check only once
> * Loosened sleeping range
>
> Co-developed-by: Radoslaw Biernacki <rad@chromium.org>
> Signed-off-by: Radoslaw Biernacki <rad@chromium.org>
> Signed-off-by: Lukasz Majczak <lma@chromium.org>
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 2df1ab3c31cc..ece1a5815e0b 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -1013,9 +1013,17 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
> struct i2c_client *client = ihid->client;
> struct hid_device *hid = ihid->hid;
> int ret;
> + int tries = 2;
Nit: move above the 'ret' declaration to maintain reverse xmas style
ordering.
Johan
^ permalink raw reply
* [bug report] HID: playstation: DS4: Don't fail on calibration data request
From: Dan Carpenter @ 2024-04-08 7:41 UTC (permalink / raw)
To: max; +Cc: linux-input
Hello Max Staudt,
Commit a48a7cd85f55 ("HID: playstation: DS4: Don't fail on
calibration data request") from Feb 8, 2024 (linux-next), leads to
the following Smatch static checker warning:
drivers/hid/hid-playstation.c:1904 dualshock4_get_calibration_data() error: uninitialized symbol 'acc_x_minus'.
drivers/hid/hid-playstation.c:1904 dualshock4_get_calibration_data() error: uninitialized symbol 'acc_x_plus'.
drivers/hid/hid-playstation.c:1910 dualshock4_get_calibration_data() error: uninitialized symbol 'acc_y_minus'.
drivers/hid/hid-playstation.c:1910 dualshock4_get_calibration_data() error: uninitialized symbol 'acc_y_plus'.
drivers/hid/hid-playstation.c:1916 dualshock4_get_calibration_data() error: uninitialized symbol 'acc_z_minus'.
drivers/hid/hid-playstation.c:1916 dualshock4_get_calibration_data() error: uninitialized symbol 'acc_z_plus'.
drivers/hid/hid-playstation.c
1768 static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
1769 {
1770 struct hid_device *hdev = ds4->base.hdev;
1771 short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
1772 short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
1773 short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
1774 short gyro_speed_plus, gyro_speed_minus;
1775 short acc_x_plus, acc_x_minus;
1776 short acc_y_plus, acc_y_minus;
1777 short acc_z_plus, acc_z_minus;
1778 int speed_2x;
1779 int range_2g;
1780 int ret = 0;
1781 int i;
1782 uint8_t *buf;
1783
1784 if (ds4->base.hdev->bus == BUS_USB) {
1785 int retries;
1786
1787 buf = kzalloc(DS4_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
1788 if (!buf) {
1789 ret = -ENOMEM;
1790 goto no_buffer_tail_check;
acc_x_minus and friends are not initialized.
1791 }
1792
1793 /* We should normally receive the feature report data we asked
1794 * for, but hidraw applications such as Steam can issue feature
1795 * reports as well. In particular for Dongle reconnects, Steam
1796 * and this function are competing resulting in often receiving
1797 * data for a different HID report, so retry a few times.
1798 */
1799 for (retries = 0; retries < 3; retries++) {
1800 ret = ps_get_report(hdev, DS4_FEATURE_REPORT_CALIBRATION, buf,
1801 DS4_FEATURE_REPORT_CALIBRATION_SIZE, true);
1802 if (ret) {
1803 if (retries < 2) {
1804 hid_warn(hdev, "Retrying DualShock 4 get calibration report (0x02) request\n");
1805 continue;
1806 }
1807
1808 hid_warn(hdev, "Failed to retrieve DualShock4 calibration info: %d\n", ret);
1809 ret = -EILSEQ;
1810 } else {
1811 break;
1812 }
1813 }
1814 } else { /* Bluetooth */
1815 buf = kzalloc(DS4_FEATURE_REPORT_CALIBRATION_BT_SIZE, GFP_KERNEL);
1816 if (!buf) {
1817 ret = -ENOMEM;
1818 goto no_buffer_tail_check;
1819 }
1820
1821 ret = ps_get_report(hdev, DS4_FEATURE_REPORT_CALIBRATION_BT, buf,
1822 DS4_FEATURE_REPORT_CALIBRATION_BT_SIZE, true);
1823
1824 if (ret)
1825 hid_warn(hdev, "Failed to retrieve DualShock4 calibration info: %d\n", ret);
1826 }
1827
1828 /* Parse buffer. If the transfer failed, this safely copies zeroes. */
1829 gyro_pitch_bias = get_unaligned_le16(&buf[1]);
1830 gyro_yaw_bias = get_unaligned_le16(&buf[3]);
1831 gyro_roll_bias = get_unaligned_le16(&buf[5]);
1832 if (ds4->base.hdev->bus == BUS_USB) {
1833 gyro_pitch_plus = get_unaligned_le16(&buf[7]);
1834 gyro_pitch_minus = get_unaligned_le16(&buf[9]);
1835 gyro_yaw_plus = get_unaligned_le16(&buf[11]);
1836 gyro_yaw_minus = get_unaligned_le16(&buf[13]);
1837 gyro_roll_plus = get_unaligned_le16(&buf[15]);
1838 gyro_roll_minus = get_unaligned_le16(&buf[17]);
1839 } else {
1840 /* BT + Dongle */
1841 gyro_pitch_plus = get_unaligned_le16(&buf[7]);
1842 gyro_yaw_plus = get_unaligned_le16(&buf[9]);
1843 gyro_roll_plus = get_unaligned_le16(&buf[11]);
1844 gyro_pitch_minus = get_unaligned_le16(&buf[13]);
1845 gyro_yaw_minus = get_unaligned_le16(&buf[15]);
1846 gyro_roll_minus = get_unaligned_le16(&buf[17]);
1847 }
1848 gyro_speed_plus = get_unaligned_le16(&buf[19]);
1849 gyro_speed_minus = get_unaligned_le16(&buf[21]);
1850 acc_x_plus = get_unaligned_le16(&buf[23]);
1851 acc_x_minus = get_unaligned_le16(&buf[25]);
1852 acc_y_plus = get_unaligned_le16(&buf[27]);
1853 acc_y_minus = get_unaligned_le16(&buf[29]);
1854 acc_z_plus = get_unaligned_le16(&buf[31]);
1855 acc_z_minus = get_unaligned_le16(&buf[33]);
1856
1857 /*
1858 * Set gyroscope calibration and normalization parameters.
1859 * Data values will be normalized to 1/DS4_GYRO_RES_PER_DEG_S degree/s.
1860 */
1861 speed_2x = (gyro_speed_plus + gyro_speed_minus);
1862 ds4->gyro_calib_data[0].abs_code = ABS_RX;
1863 ds4->gyro_calib_data[0].bias = 0;
1864 ds4->gyro_calib_data[0].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
1865 ds4->gyro_calib_data[0].sens_denom = abs(gyro_pitch_plus - gyro_pitch_bias) +
1866 abs(gyro_pitch_minus - gyro_pitch_bias);
1867
1868 ds4->gyro_calib_data[1].abs_code = ABS_RY;
1869 ds4->gyro_calib_data[1].bias = 0;
1870 ds4->gyro_calib_data[1].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
1871 ds4->gyro_calib_data[1].sens_denom = abs(gyro_yaw_plus - gyro_yaw_bias) +
1872 abs(gyro_yaw_minus - gyro_yaw_bias);
1873
1874 ds4->gyro_calib_data[2].abs_code = ABS_RZ;
1875 ds4->gyro_calib_data[2].bias = 0;
1876 ds4->gyro_calib_data[2].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
1877 ds4->gyro_calib_data[2].sens_denom = abs(gyro_roll_plus - gyro_roll_bias) +
1878 abs(gyro_roll_minus - gyro_roll_bias);
1879
1880 /* Done parsing the buffer, so let's free it. */
1881 kfree(buf);
1882
1883 no_buffer_tail_check:
1884
1885 /*
1886 * Sanity check gyro calibration data. This is needed to prevent crashes
1887 * during report handling of virtual, clone or broken devices not implementing
1888 * calibration data properly.
1889 */
1890 for (i = 0; i < ARRAY_SIZE(ds4->gyro_calib_data); i++) {
1891 if (ds4->gyro_calib_data[i].sens_denom == 0) {
1892 hid_warn(hdev, "Invalid gyro calibration data for axis (%d), disabling calibration.",
1893 ds4->gyro_calib_data[i].abs_code);
1894 ds4->gyro_calib_data[i].bias = 0;
1895 ds4->gyro_calib_data[i].sens_numer = DS4_GYRO_RANGE;
1896 ds4->gyro_calib_data[i].sens_denom = S16_MAX;
1897 }
1898 }
1899
1900 /*
1901 * Set accelerometer calibration and normalization parameters.
1902 * Data values will be normalized to 1/DS4_ACC_RES_PER_G g.
1903 */
--> 1904 range_2g = acc_x_plus - acc_x_minus;
^^^^^^^^^^^^^^^^^^^^^^^^^
1905 ds4->accel_calib_data[0].abs_code = ABS_X;
1906 ds4->accel_calib_data[0].bias = acc_x_plus - range_2g / 2;
1907 ds4->accel_calib_data[0].sens_numer = 2*DS4_ACC_RES_PER_G;
1908 ds4->accel_calib_data[0].sens_denom = range_2g;
1909
1910 range_2g = acc_y_plus - acc_y_minus;
1911 ds4->accel_calib_data[1].abs_code = ABS_Y;
1912 ds4->accel_calib_data[1].bias = acc_y_plus - range_2g / 2;
1913 ds4->accel_calib_data[1].sens_numer = 2*DS4_ACC_RES_PER_G;
1914 ds4->accel_calib_data[1].sens_denom = range_2g;
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
From: Hans de Goede @ 2024-04-08 12:45 UTC (permalink / raw)
To: Mark Pearson
Cc: ilpo.jarvinen, hmh, dmitry.torokhov, ibm-acpi-devel,
platform-driver-x86, linux-input, linux-kernel, njoshi1, vsankar,
peter.hutterer
In-Reply-To: <20240324210817.192033-2-mpearson-lenovo@squebb.ca>
Hi,
On 3/24/24 10:07 PM, Mark Pearson wrote:
> Add support for new input events on Lenovo laptops that need exporting to
> user space.
>
> Lenovo trackpoints are adding the ability to generate a doubletap event.
> Add a new keycode to allow this to be used by userspace.
>
> Lenovo support is using FN+N with Windows to collect needed details for
> support cases. Add a keycode so that we'll be able to provide similar
> support on Linux.
>
> Suggested-by: Peter Hutterer <peter.hutterer@redhat.com>
>
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
> Signed-off-by: Vishnu Sankar <vsankar@lenovo.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Dmitry, can I have your ack for merging this change through the pdx86
tree (since the first driver using these is a pdx86 driver) ?
Regards,
Hans
> ---
> include/uapi/linux/input-event-codes.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 03edf2ccdf6c..bd3baca95749 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -686,6 +686,8 @@
> #define KEY_SIDEVU_SONAR 0x287
> #define KEY_NAV_INFO 0x288
> #define KEY_BRIGHTNESS_MENU 0x289
> +#define KEY_DOUBLECLICK 0x28a
> +#define KEY_SYS_DEBUG_INFO 0x28b
>
> /*
> * Some keyboards have keys which do not have a defined meaning, these keys
^ permalink raw reply
* Re: [PATCH 2/4] platform/x86: thinkpad_acpi: Support for trackpoint doubletap
From: Hans de Goede @ 2024-04-08 13:04 UTC (permalink / raw)
To: Mark Pearson
Cc: ilpo.jarvinen, hmh, dmitry.torokhov, ibm-acpi-devel,
platform-driver-x86, linux-input, linux-kernel, njoshi1, vsankar,
peter.hutterer
In-Reply-To: <20240324210817.192033-3-mpearson-lenovo@squebb.ca>
Hi Mark,
On 3/24/24 10:07 PM, Mark Pearson wrote:
> Lenovo trackpoints are adding the ability to generate a doubletap event.
> This handles the doubletap event and sends the KEY_DOUBLECLICK event to
> userspace.
>
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Vishnu Sankar <vsankar@lenovo.com>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 82429e59999d..2bbb32c898e9 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -232,6 +232,7 @@ enum tpacpi_hkey_event_t {
>
> /* Misc */
> TP_HKEY_EV_RFKILL_CHANGED = 0x7000, /* rfkill switch changed */
> + TP_HKEY_EV_TRACKPOINT_DOUBLETAP = 0x8036, /* doubletap on Trackpoint*/
> };
>
> /****************************************************************************
> @@ -4081,6 +4082,22 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
> break;
> }
> fallthrough; /* to default */
This now no longer fallsthrough to default. IMHO the best thing to do
here is add a new preparation patch which initializes known_ev to false
inside the while before the switch-case (together with the send_acpi_ev
and ignore_acpi_ev init). and then change this fallthrough to a break
in the preparation patch. You can then also remove the default case
altogether in this prep patch.
> + case 8:
> + /* 0x8036: Trackpoint doubletaps */
> + if (hkey == TP_HKEY_EV_TRACKPOINT_DOUBLETAP) {
> + send_acpi_ev = true;
> + ignore_acpi_ev = false;
These 2 values are set as the default above the switch-case, please
drop these 2 lines.
> + known_ev = true;
> + /* Send to user space */
> + mutex_lock(&tpacpi_inputdev_send_mutex);
> + input_report_key(tpacpi_inputdev, KEY_DOUBLECLICK, 1);
> + input_sync(tpacpi_inputdev);
> + input_report_key(tpacpi_inputdev, KEY_DOUBLECLICK, 0);
> + input_sync(tpacpi_inputdev);
> + mutex_unlock(&tpacpi_inputdev_send_mutex);
This code duplicates tpacpi_input_send_key(), what you want to do here
is define a hotkey_keycode_map scancode range for new 0x8xxx codes like how this
was done when extended scancodes where added to deal with the new 0x13xx hotkey
event codes for the 2017+ models.
See commit 696c6523ec8f ("platform/x86: thinkpad_acpi: add mapping for new hotkeys")
Despite re-using tpacpi_input_send_key() there are 2 reasons why we want
scancodes for these new "keys".
1. By adding the keys to the hotkey_keycode_map they automatically
also get input_set_capability(tpacpi_inputdev, EV_KEY, hotkey_keycode_map[i]);
called on them advertising to userspace that tpacpi_inputdev can actually
generate these keypresses. Something which is currently lacking from your
patch. Related to this did you test this with evtest? I think that the input
core will suppress the events when you do not set the capability ?
2. This allows remapping scancodes to different KEY_foo values with hwdb
entries.
Regards,
Hans
> + break;
> + }
> + fallthrough; /* to default */
> default:
> known_ev = false;
> }
^ permalink raw reply
* Re: [PATCH 3/4] platform/x86: thinkpad_acpi: Support for system debug info hotkey
From: Hans de Goede @ 2024-04-08 13:11 UTC (permalink / raw)
To: Mark Pearson
Cc: ilpo.jarvinen, hmh, dmitry.torokhov, ibm-acpi-devel,
platform-driver-x86, linux-input, linux-kernel, njoshi1, vsankar,
peter.hutterer
In-Reply-To: <20240324210817.192033-4-mpearson-lenovo@squebb.ca>
Hi,
On 3/24/24 10:08 PM, Mark Pearson wrote:
> New Lenovo platforms are adding the FN+N key to generate system debug
> details that support can use for collecting important details on any
> customer cases for Windows.
> Add the infrastructure so we can do the same on Linux by generating a
> SYS_DEBUG_INFO keycode to userspace.
>
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 2bbb32c898e9..854ce971bde2 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -1787,6 +1787,7 @@ enum { /* hot key scan codes (derived from ACPI DSDT) */
> TP_ACPI_HOTKEYSCAN_NOTIFICATION_CENTER,
> TP_ACPI_HOTKEYSCAN_PICKUP_PHONE,
> TP_ACPI_HOTKEYSCAN_HANGUP_PHONE,
> + TP_ACPI_HOTKEYSCAN_SYS_DEBUG_INFO = 81,
>
> /* Hotkey keymap size */
> TPACPI_HOTKEY_MAP_LEN
> @@ -3337,6 +3338,9 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
> KEY_NOTIFICATION_CENTER, /* Notification Center */
> KEY_PICKUP_PHONE, /* Answer incoming call */
> KEY_HANGUP_PHONE, /* Decline incoming call */
> + KEY_UNKNOWN, /* AMT Toggle (event), 0x31A */
> + KEY_UNKNOWN, KEY_UNKNOWN,
> + KEY_SYS_DEBUG_INFO, /* System debug info, 0x31D */
> },
> };
>
Looking at the next patch 0x131c is TP_HKEY_EV_DOUBLETAP_TOGGLE and 0x131a is
TP_HKEY_EV_AMT_TOGGLE based on this please change this to:
KEY_NOTIFICATION_CENTER, /* Notification Center */
KEY_PICKUP_PHONE, /* Answer incoming call */
KEY_HANGUP_PHONE, /* Decline incoming call */
KEY_UNKNOWN, /* TP_HKEY_EV_AMT_TOGGLE handled in driver, 0x31a */
KEY_UNKNOWN, /* ?, 0X31b */
KEY_UNKNOWN, /* TP_HKEY_EV_DOUBLETAP_TOGGLE handled in driver, 0x31c */
KEY_SYS_DEBUG_INFO, /* System debug info, 0x31d */
},
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 4/4] platform/x86: thinkpad_acpi: Support hotkey to disable trackpoint doubletap
From: Hans de Goede @ 2024-04-08 13:13 UTC (permalink / raw)
To: Mark Pearson
Cc: ilpo.jarvinen, hmh, dmitry.torokhov, ibm-acpi-devel,
platform-driver-x86, linux-input, linux-kernel, njoshi1, vsankar,
peter.hutterer
In-Reply-To: <20240324210817.192033-5-mpearson-lenovo@squebb.ca>
Hi,
On 3/24/24 10:08 PM, Mark Pearson wrote:
> The hotkey combination FN+G can be used to disable the trackpoint
> doubletap feature on Windows.
> Add matching functionality for Linux.
>
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Vishnu Sankar <vsankar@lenovo.com>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 24 +++++++++++++++++-------
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 854ce971bde2..21756aa3d28d 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -167,6 +167,7 @@ enum tpacpi_hkey_event_t {
> TP_HKEY_EV_VOL_MUTE = 0x1017, /* Mixer output mute */
> TP_HKEY_EV_PRIVACYGUARD_TOGGLE = 0x130f, /* Toggle priv.guard on/off */
> TP_HKEY_EV_AMT_TOGGLE = 0x131a, /* Toggle AMT on/off */
> + TP_HKEY_EV_DOUBLETAP_TOGGLE = 0x131c, /* Toggle trackpoint doubletap on/off */
> TP_HKEY_EV_PROFILE_TOGGLE = 0x131f, /* Toggle platform profile */
>
> /* Reasons for waking up from S3/S4 */
> @@ -354,6 +355,7 @@ static struct {
> u32 hotkey_poll_active:1;
> u32 has_adaptive_kbd:1;
> u32 kbd_lang:1;
> + u32 trackpoint_doubletap:1;
> struct quirk_entry *quirks;
> } tp_features;
>
> @@ -3598,6 +3600,9 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>
> hotkey_poll_setup_safe(true);
>
> + /* Enable doubletap by default */
> + tp_features.trackpoint_doubletap = 1;
> +
> return 0;
> }
>
> @@ -3739,6 +3744,7 @@ static bool hotkey_notify_extended_hotkey(const u32 hkey)
> case TP_HKEY_EV_PRIVACYGUARD_TOGGLE:
> case TP_HKEY_EV_AMT_TOGGLE:
> case TP_HKEY_EV_PROFILE_TOGGLE:
> + case TP_HKEY_EV_DOUBLETAP_TOGGLE:
> tpacpi_driver_event(hkey);
> return true;
> }
> @@ -4092,13 +4098,15 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
> send_acpi_ev = true;
> ignore_acpi_ev = false;
> known_ev = true;
> - /* Send to user space */
> - mutex_lock(&tpacpi_inputdev_send_mutex);
> - input_report_key(tpacpi_inputdev, KEY_DOUBLECLICK, 1);
> - input_sync(tpacpi_inputdev);
> - input_report_key(tpacpi_inputdev, KEY_DOUBLECLICK, 0);
> - input_sync(tpacpi_inputdev);
> - mutex_unlock(&tpacpi_inputdev_send_mutex);
> + if (tp_features.trackpoint_doubletap) {
> + /* Send to user space */
> + mutex_lock(&tpacpi_inputdev_send_mutex);
> + input_report_key(tpacpi_inputdev, KEY_DOUBLECLICK, 1);
> + input_sync(tpacpi_inputdev);
> + input_report_key(tpacpi_inputdev, KEY_DOUBLECLICK, 0);
> + input_sync(tpacpi_inputdev);
> + mutex_unlock(&tpacpi_inputdev_send_mutex);
> + }
> break;
> }
> fallthrough; /* to default */
This chunk will need to change after incorporating my review comments into
patch 2/4. With that said this looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> @@ -11228,6 +11236,8 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
> /* Notify user space the profile changed */
> platform_profile_notify();
> }
> + if (hkey_event == TP_HKEY_EV_DOUBLETAP_TOGGLE)
> + tp_features.trackpoint_doubletap = !tp_features.trackpoint_doubletap;
> }
>
> static void hotkey_driver_event(const unsigned int scancode)
^ permalink raw reply
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