* Your urgent feedback is highly appreciated.
From: Eun-ji Ha @ 2023-05-22 21:14 UTC (permalink / raw)
To: linux-input
Good day,
My name is Mrs. Eun-ji Ha.
I would like to get in touch with you.
Did you receive my previous email for investment purposes?
Please send your telephone number to enhance communication.
Have a great day!
Best regards,
^ permalink raw reply
* Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer
From: Andy Shevchenko @ 2023-05-22 19:05 UTC (permalink / raw)
To: Paul Cercueil
Cc: Artur Rojek, Jonathan Cameron, Dmitry Torokhov, Chris Morgan,
linux-mips, linux-iio, linux-kernel, linux-input
In-Reply-To: <6b88623e44b2a98a2e5d8d6d2453f92eb1b673ae.camel@crapouillou.net>
On Mon, May 22, 2023 at 2:35 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le lundi 22 mai 2023 à 14:05 +0300, Andy Shevchenko a écrit :
> > On Mon, May 22, 2023 at 1:23 PM Paul Cercueil <paul@crapouillou.net>
> > wrote:
> > > Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit :
> > > > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > > > > <contact@artur-rojek.eu> wrote:
...
> > > > > > + u16 tdat[6];
> > > > > > + u32 val;
> > > > > > +
> > > > > > + memset(tdat, 0, ARRAY_SIZE(tdat));
> > > > >
> > > > > Yeah, as LKP tells us this should be sizeof() instead of
> > > > > ARRAY_SIZE().
> > > > >
> > > > > > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2)
> > > > > > {
> > > > > > + if (mask & 0x3) {
> > > > >
> > > > > (for the consistency it has to be GENMASK(), but see below)
> > > > >
> > > > > First of all, strictly speaking we should use the full mask
> > > > > without
> > > > > limiting it to the 0 element in the array (I'm talking about
> > > > > active_scan_mask).
> > > > >
> > > > > That said, we may actually use bit operations here in a better
> > > > > way,
> > > > > i.e.
> > > > >
> > > > > unsigned long mask = active_scan_mask[0] &
> > > > > (active_scan_mask[0] -
> > > > > 1);
> > > > >
> > > > > j = 0;
> > > > > for_each_set_bit(i, active_scan_mask, ...) {
> > > > > val = readl(...);
> > > > > /* Two channels per sample. Demux active. */
> > > > > tdat[j++] = val >> (16 * (i % 2));
> > > >
> > > > Alternatively
> > > >
> > > > /* Two channels per sample. Demux active. */
> > > > if (i % 2)
> > > > tdat[j++] = upper_16_bits(val);
> > > > else
> > > > tdat[j++] = lower_16_bits(val);
> > > >
> > > > which may be better to read.
> > >
> > > It's not if/else though. You would check (i % 2) for the upper 16
> > > bits,
> > > and (i % 1) for the lower 16 bits. Both can be valid at the same
> > > time.
(i can't be two bits at the same time in my proposal)
> > Are you sure? Have you looked into the proposed code carefully?
>
> Yes. I co-wrote the original code, I know what it's supposed to do.
Yes, but I'm talking about my version to which you commented and I
think it is the correct approach with 'else'. The problematic part in
my proposal is FIFO reading.
So, I have tried to come up with the working solution, but it seems
it's too premature optimization here that's not needed and code,
taking into account readability, will become a bit longer.
That said, let's go with your version for now (implying the GENMASK()
and upper/lower_16_bits() macros in use).
> > What probably can be done differently is the read part, that can be
> > called once. But looking at it I'm not sure how it's supposed to work
> > at all, since the address is always the same. How does the code and
> > hardware are in sync with the channels?
>
> It's a FIFO.
A-ha.
> > > > > }
> > > > >
> > > > > > + val = readl(adc->base +
> > > > > > JZ_ADC_REG_ADTCH);
> > > > > > + /* Two channels per sample. Demux
> > > > > > active.
> > > > > > */
> > > > > > + if (mask & BIT(0))
> > > > > > + tdat[i++] = val & 0xffff;
> > > > > > + if (mask & BIT(1))
> > > > > > + tdat[i++] = val >> 16;
> > > > > > + }
> > > > > > }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH 1/1] HID: apple: Option to swap only left side mod keys
From: Lasse Brun @ 2023-05-22 18:40 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: Lasse Brun, linux-input, linux-kernel
On the Keychron K8 keyboard, and probably others, the right side keys
should not be swapped to maintain PC layout.
Swapping the right side keys moves 'Super' before 'Alt gr' which is not
intended by the default Keychron layout or the ISO layout.
Signed-off-by: Lasse Brun <bruners@gmail.com>
---
drivers/hid/hid-apple.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index cc535d2d6e8c..d7b932925730 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -58,7 +58,7 @@ static unsigned int swap_opt_cmd;
module_param(swap_opt_cmd, uint, 0644);
MODULE_PARM_DESC(swap_opt_cmd, "Swap the Option (\"Alt\") and Command (\"Flag\") keys. "
"(For people who want to keep Windows PC keyboard muscle memory. "
- "[0] = as-is, Mac layout. 1 = swapped, Windows layout.)");
+ "[0] = as-is, Mac layout. 1 = swapped, Windows layout., 2 = swapped, Swap only left side)");
static unsigned int swap_ctrl_cmd;
module_param(swap_ctrl_cmd, uint, 0644);
@@ -319,6 +319,12 @@ static const struct apple_key_translation swapped_option_cmd_keys[] = {
{ }
};
+static const struct apple_key_translation swapped_option_cmd_left_keys[] = {
+ { KEY_LEFTALT, KEY_LEFTMETA },
+ { KEY_LEFTMETA, KEY_LEFTALT },
+ { }
+};
+
static const struct apple_key_translation swapped_ctrl_cmd_keys[] = {
{ KEY_LEFTCTRL, KEY_LEFTMETA },
{ KEY_LEFTMETA, KEY_LEFTCTRL },
@@ -416,7 +422,10 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
}
if (swap_opt_cmd) {
- trans = apple_find_translation(swapped_option_cmd_keys, code);
+ if (swap_opt_cmd == 2)
+ trans = apple_find_translation(swapped_option_cmd_left_keys, code);
+ else
+ trans = apple_find_translation(swapped_option_cmd_keys, code);
if (trans)
code = trans->to;
--
2.40.1
^ permalink raw reply related
* Re: [regression] Since kernel 6.3.1 logitech unify receiver not working properly
From: Linus Torvalds @ 2023-05-22 18:23 UTC (permalink / raw)
To: Linux regressions mailing list
Cc: Benjamin Tissoires, Bagas Sanjaya, Filipe Laíns,
Bastien Nocera, Jiri Kosina, open list:HID CORE LAYER, LKML,
guy.b
In-Reply-To: <55dda0bb-fe42-6dee-28ea-00121554d092@leemhuis.info>
[-- Attachment #1: Type: text/plain, Size: 2173 bytes --]
On Mon, May 22, 2023 at 5:38 AM Linux regression tracking (Thorsten
Leemhuis) <regressions@leemhuis.info> wrote:
>
> FWIW, in case anybody is interested in a status update: one reporter
> bisected the problem down to 586e8fede79 ("HID: logitech-hidpp: Retry
> commands when device is busy"); reverting that commit on-top of 6.3
> fixes the problem for that reporter. For that reporter things also work
> on 6.4-rc; but for someone else that is affected that's not the case.
Hmm. It's likely timing-dependent.
But that code is clearly buggy.
If the wait_event_timeout() returns early, the device hasn't replied,
but the code does
if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
5*HZ)) {
dbg_hid("%s:timeout waiting for response\n", __func__);
memset(response, 0, sizeof(struct hidpp_report));
ret = -ETIMEDOUT;
}
and then continues to look at the response _anyway_.
Now, depending on out hardening options, that response may have been
initialized by the compiler, or may just be random stack contents.
That bug is pre-existing (ie the problem was not introduced by that
commit), but who knows if the retry makes things worse (ie if it then
triggers on a retry, the response data will be the *previous*
response).
The whole "goto exit" games should be removed too, because we're in a
for-loop, and instead of "goto exit" it should just do "break".
IOW, something like this might be worth testing.
That said, while I think the code is buggy, I doubt this is the actual
cause of the problem people are reporting. But it would be lovely to
hear if the attached patch makes any difference, and I think this is
fixing a real - but unlikely - problem anyway.
And obviously it might be helpful to actually enable those dbg_hid()
messages, but I didn't look at what the magic config option to do so
was.
NOTE! Patch below *ENTIRELY* untested. I just looked at the code when
that commit was mentioned, and went "that's not right"...
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1524 bytes --]
drivers/hid/hid-logitech-hidpp.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0fcfd85fea0f..72bd62d2f984 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -306,7 +306,7 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
if (ret) {
dbg_hid("__hidpp_send_report returned err: %d\n", ret);
memset(response, 0, sizeof(struct hidpp_report));
- goto exit;
+ break;
}
if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
@@ -314,13 +314,14 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
dbg_hid("%s:timeout waiting for response\n", __func__);
memset(response, 0, sizeof(struct hidpp_report));
ret = -ETIMEDOUT;
+ break;
}
if (response->report_id == REPORT_ID_HIDPP_SHORT &&
response->rap.sub_id == HIDPP_ERROR) {
ret = response->rap.params[1];
dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
- goto exit;
+ break;
}
if ((response->report_id == REPORT_ID_HIDPP_LONG ||
@@ -329,13 +330,12 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
ret = response->fap.params[1];
if (ret != HIDPP20_ERROR_BUSY) {
dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
- goto exit;
+ break;
}
dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret);
}
}
-exit:
mutex_unlock(&hidpp->send_mutex);
return ret;
^ permalink raw reply related
* Re: [v2 2/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
From: Doug Anderson @ 2023-05-22 15:33 UTC (permalink / raw)
To: Cong Yang
Cc: benjamin.tissoires, devicetree, dmitry.torokhov, hsinyi, jikos,
linux-input, linux-kernel, mka
In-Reply-To: <20230520050649.2494497-3-yangcong5@huaqin.corp-partner.google.com>
Hi,
On Fri, May 19, 2023 at 10:07 PM Cong Yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> Add an ilitek touch screen chip ili9882t.
>
> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> ---
> .../devicetree/bindings/input/elan,ekth6915.yaml | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
I'm curious about the DT maintainers opinion here. Should this be a
new bindings file, or should it be together in the elan file. If
nothing else, I think the secondary voltage rail name is wrong. I took
a quick peek at a datasheet I found and I don't even see a 3.3V rail
going to the ili9882t. That makes it weird to reuse "vcc33-supply" for
a second supply...
-Doug
^ permalink raw reply
* Re: [v2 1/2] HID: i2c-hid: elan: Add ili9882t timing
From: Doug Anderson @ 2023-05-22 15:27 UTC (permalink / raw)
To: Cong Yang
Cc: benjamin.tissoires, devicetree, dmitry.torokhov, hsinyi, jikos,
linux-input, linux-kernel, mka
In-Reply-To: <20230520050649.2494497-2-yangcong5@huaqin.corp-partner.google.com>
Hi,
On Fri, May 19, 2023 at 10:07 PM Cong Yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> The ili9882t is a TDDI IC ((Touch with Display Driver)). It requires the
> panel reset gpio to be high before i2c commands. Use a longer delay in
> post_power_delay_ms to ensure the poweron sequence.
>
> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> ---
> drivers/hid/i2c-hid/i2c-hid-of-elan.c | 7 +++++++
> 1 file changed, 7 insertions(+)
This seems OK to me. The one thing I'd also do is to update the
Kconfig description to say that this driver is also used for Ilitek. I
think it's fine to keep the symbol name as I2C_HID_OF_ELAN but just
change the description.
-Doug
^ permalink raw reply
* Re: [regression] Since kernel 6.3.1 logitech unify receiver not working properly
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-05-22 12:38 UTC (permalink / raw)
To: Benjamin Tissoires, Bagas Sanjaya
Cc: Linux regressions mailing list, Filipe Laíns, Bastien Nocera,
Jiri Kosina, Linus Torvalds, open list:HID CORE LAYER, LKML,
guy.b
In-Reply-To: <CAO-hwJ+At1J_yUpX2q_dJekzZ-PoTDAvxmkTk_e4Yu0Z338bEA@mail.gmail.com>
On 16.05.23 15:34, Benjamin Tissoires wrote:
> On Tue, May 16, 2023 at 3:25 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>> On 5/11/23 18:58, Thorsten Leemhuis wrote:
>>> On 08.05.23 11:55, Linux regression tracking (Thorsten Leemhuis) wrote:
>>>> I noticed a regression report in bugzilla.kernel.org. As many (most?)
>>>> kernel developers don't keep an eye on it, I decided to forward it by mail.
>>>>
>>>> Quoting from https://bugzilla.kernel.org/show_bug.cgi?id=217412 :
>>>
>>> TWIMC: a few other users (three or four iirc) showed up in that ticket
>>> and reported problems with the receiver, albeit the symptoms are not
>>> exactly the same for all of them, so there might be more than one problem.
>>>
>>> I'll try to motivate the affected users to perform a bisection. But
>>> would be great if those with more knowledge about this code could
>>> briefly look into the ticket, maybe the details the users shared allows
>>> one of you to guess what causes this.
>>
>> Hmm,
>>
>> You noted in the similar report [1] that developers involved here
>> ignore this regressions. I wonder if Linus has to be hired in
>> this case, and if it is the case, let's take a look and hear closely what
>> he will say.
>
> Sigh... Not answering an email is bad, but maybe you can also
> understand that developers can take days off?
Yup, also a totally valid reason I forgot to mention in my reply last week.
> And it turns out that I was waiting for Bastien to chime in, but I can
> access his calendar too and just found out that he was AFK for the
> entire month, except for the first week, which I wasn't aware. May is
> a time where people in France have a lot of public holidays and is
> also the cut to use those time off or they expire.
Thx for that, knowing that Bastien is unavailable is really helpful.
> For me, I'll also be taking time off the rest of this week, so I won't
> be able to have a look at this until next week at the earliest.
Hope your enjoyed your days off!
FWIW, in case anybody is interested in a status update: one reporter
bisected the problem down to 586e8fede79 ("HID: logitech-hidpp: Retry
commands when device is busy"); reverting that commit on-top of 6.3
fixes the problem for that reporter. For that reporter things also work
on 6.4-rc; but for someone else that is affected that's not the case.
Makes me wonder if we deal with two different issues here. Just asked
where 6.4 does not work if reverting 586e8fede79 fixes things for them
as well.
For more details, see https://bugzilla.kernel.org/show_bug.cgi?id=217412
Ciao, Thorsten
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: mms114: Add linux,keycodes property for touch keys
From: Conor Dooley @ 2023-05-22 12:14 UTC (permalink / raw)
To: Artur Weber
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Linus Walleij,
linux-input, devicetree, linux-kernel, ~postmarketos/upstreaming
In-Reply-To: <20230521145843.19489-2-aweber.kernel@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]
On Sun, May 21, 2023 at 04:58:42PM +0200, Artur Weber wrote:
> MELFAS MMS114 and similar touchscreens have support for touch keys.
> Add the linux,keycodes property which can be used to set up the
> keycodes for the touch keys.
>
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Thanks,
Conor.
> ---
> .../devicetree/bindings/input/touchscreen/melfas,mms114.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/melfas,mms114.yaml b/Documentation/devicetree/bindings/input/touchscreen/melfas,mms114.yaml
> index fdd02898e249..07f9dd6b1c9c 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/melfas,mms114.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/melfas,mms114.yaml
> @@ -52,6 +52,11 @@ properties:
> touchscreen-swapped-x-y: true
> touchscreen-max-pressure: true
>
> + linux,keycodes:
> + description: Keycodes for the touch keys
> + minItems: 1
> + maxItems: 15
> +
> additionalProperties: false
>
> required:
> --
> 2.40.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v7 0/6] Add support for TI TPS65219 PMIC.
From: jerome Neanne @ 2023-05-22 11:57 UTC (permalink / raw)
To: Francesco Dolcini
Cc: lgirdwood, broonie, robh+dt, nm, kristo, dmitry.torokhov,
krzysztof.kozlowski+dt, catalin.marinas, will, lee, tony,
vigneshr, shawnguo, geert+renesas, dmitry.baryshkov,
marcel.ziswiler, vkoul, biju.das.jz, arnd, jeff, afd, khilman,
narmstrong, msp, j-keerthy, linux-kernel, devicetree,
linux-arm-kernel, linux-input, linux-omap
In-Reply-To: <753775e9-33f6-031f-8da5-2f65894f44fe@baylibre.com>
On 06/12/2022 20:22, jerome Neanne wrote:
>
>
> On 05/12/2022 19:08, Francesco Dolcini wrote:
>> On Fri, Nov 04, 2022 at 04:23:05PM +0100, Jerome Neanne wrote:
>>> Hi everyone,
>> Hello Jerome,
>>
>> are you planning to have also gpio support added to the driver?
FYI, this is under review:
https://lore.kernel.org/lkml/20230511-tps65219-add-gpio-support-v2-0-60feb64d649a@baylibre.com/
^ permalink raw reply
* Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer
From: Paul Cercueil @ 2023-05-22 11:35 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Artur Rojek, Jonathan Cameron, Dmitry Torokhov, Chris Morgan,
linux-mips, linux-iio, linux-kernel, linux-input
In-Reply-To: <CAHp75VdjAxAvmYVW4qgV2i91L31=Ctx4nx_eAe9+pqPFEArD3w@mail.gmail.com>
Hi Andy,
Le lundi 22 mai 2023 à 14:05 +0300, Andy Shevchenko a écrit :
> On Mon, May 22, 2023 at 1:23 PM Paul Cercueil <paul@crapouillou.net>
> wrote:
> > Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit :
> > > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > > > <contact@artur-rojek.eu> wrote:
>
> ...
>
> > > > > + u16 tdat[6];
> > > > > + u32 val;
> > > > > +
> > > > > + memset(tdat, 0, ARRAY_SIZE(tdat));
> > > >
> > > > Yeah, as LKP tells us this should be sizeof() instead of
> > > > ARRAY_SIZE().
> > > >
> > > > > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2)
> > > > > {
> > > > > + if (mask & 0x3) {
> > > >
> > > > (for the consistency it has to be GENMASK(), but see below)
> > > >
> > > > First of all, strictly speaking we should use the full mask
> > > > without
> > > > limiting it to the 0 element in the array (I'm talking about
> > > > active_scan_mask).
> > > >
> > > > That said, we may actually use bit operations here in a better
> > > > way,
> > > > i.e.
> > > >
> > > > unsigned long mask = active_scan_mask[0] &
> > > > (active_scan_mask[0] -
> > > > 1);
> > > >
> > > > j = 0;
> > > > for_each_set_bit(i, active_scan_mask, ...) {
> > > > val = readl(...);
> > > > /* Two channels per sample. Demux active. */
> > > > tdat[j++] = val >> (16 * (i % 2));
> > >
> > > Alternatively
> > >
> > > /* Two channels per sample. Demux active. */
> > > if (i % 2)
> > > tdat[j++] = upper_16_bits(val);
> > > else
> > > tdat[j++] = lower_16_bits(val);
> > >
> > > which may be better to read.
> >
> > It's not if/else though. You would check (i % 2) for the upper 16
> > bits,
> > and (i % 1) for the lower 16 bits. Both can be valid at the same
> > time.
>
> Are you sure? Have you looked into the proposed code carefully?
Yes. I co-wrote the original code, I know what it's supposed to do.
>
> What probably can be done differently is the read part, that can be
> called once. But looking at it I'm not sure how it's supposed to work
> at all, since the address is always the same. How does the code and
> hardware are in sync with the channels?
It's a FIFO.
Cheers,
-Paul
>
> > > > }
> > > >
> > > > > + val = readl(adc->base +
> > > > > JZ_ADC_REG_ADTCH);
> > > > > + /* Two channels per sample. Demux
> > > > > active.
> > > > > */
> > > > > + if (mask & BIT(0))
> > > > > + tdat[i++] = val & 0xffff;
> > > > > + if (mask & BIT(1))
> > > > > + tdat[i++] = val >> 16;
> > > > > + }
> > > > > }
>
^ permalink raw reply
* Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer
From: Andy Shevchenko @ 2023-05-22 11:05 UTC (permalink / raw)
To: Paul Cercueil
Cc: Artur Rojek, Jonathan Cameron, Dmitry Torokhov, Chris Morgan,
linux-mips, linux-iio, linux-kernel, linux-input
In-Reply-To: <9812499a8e017b8e01327069c8063e5f213bb1c8.camel@crapouillou.net>
On Mon, May 22, 2023 at 1:20 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le lundi 22 mai 2023 à 13:15 +0300, Andy Shevchenko a écrit :
> > On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu>
> > wrote:
...
> > > + memset(tdat, 0, ARRAY_SIZE(tdat));
> >
> > Yeah, as LKP tells us this should be sizeof() instead of
> > ARRAY_SIZE().
>
> Probably "u16 tdat[6] = { 0 };" would work too?
Without 0 also would work :-)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer
From: Andy Shevchenko @ 2023-05-22 11:05 UTC (permalink / raw)
To: Paul Cercueil
Cc: Artur Rojek, Jonathan Cameron, Dmitry Torokhov, Chris Morgan,
linux-mips, linux-iio, linux-kernel, linux-input
In-Reply-To: <2fc0874ce8a802aeb98e553b15e27fb4d4b75a1c.camel@crapouillou.net>
On Mon, May 22, 2023 at 1:23 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit :
> > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > > <contact@artur-rojek.eu> wrote:
...
> > > > + u16 tdat[6];
> > > > + u32 val;
> > > > +
> > > > + memset(tdat, 0, ARRAY_SIZE(tdat));
> > >
> > > Yeah, as LKP tells us this should be sizeof() instead of
> > > ARRAY_SIZE().
> > >
> > > > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
> > > > + if (mask & 0x3) {
> > >
> > > (for the consistency it has to be GENMASK(), but see below)
> > >
> > > First of all, strictly speaking we should use the full mask without
> > > limiting it to the 0 element in the array (I'm talking about
> > > active_scan_mask).
> > >
> > > That said, we may actually use bit operations here in a better way,
> > > i.e.
> > >
> > > unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] -
> > > 1);
> > >
> > > j = 0;
> > > for_each_set_bit(i, active_scan_mask, ...) {
> > > val = readl(...);
> > > /* Two channels per sample. Demux active. */
> > > tdat[j++] = val >> (16 * (i % 2));
> >
> > Alternatively
> >
> > /* Two channels per sample. Demux active. */
> > if (i % 2)
> > tdat[j++] = upper_16_bits(val);
> > else
> > tdat[j++] = lower_16_bits(val);
> >
> > which may be better to read.
>
> It's not if/else though. You would check (i % 2) for the upper 16 bits,
> and (i % 1) for the lower 16 bits. Both can be valid at the same time.
Are you sure? Have you looked into the proposed code carefully?
What probably can be done differently is the read part, that can be
called once. But looking at it I'm not sure how it's supposed to work
at all, since the address is always the same. How does the code and
hardware are in sync with the channels?
> > > }
> > >
> > > > + val = readl(adc->base +
> > > > JZ_ADC_REG_ADTCH);
> > > > + /* Two channels per sample. Demux active.
> > > > */
> > > > + if (mask & BIT(0))
> > > > + tdat[i++] = val & 0xffff;
> > > > + if (mask & BIT(1))
> > > > + tdat[i++] = val >> 16;
> > > > + }
> > > > }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 2/2] input: joystick: Fix buffer data parsing
From: Andy Shevchenko @ 2023-05-22 10:58 UTC (permalink / raw)
To: Artur Rojek
Cc: Paul Cercueil, Jonathan Cameron, Dmitry Torokhov, Chris Morgan,
linux-mips, linux-iio, linux-kernel, linux-input
In-Reply-To: <20230521225901.388455-3-contact@artur-rojek.eu>
On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu> wrote:
>
> Don't try to access buffer data of a channel by its scan index. Instead,
> calculate its offset in the buffer.
>
> This is necessary, as the scan index of a channel does not represent its
> position in a buffer - the buffer will contain data for enabled channels
> only, affecting data offsets and alignment.
>
> While at it, also fix minor style issue in probe.
a minor
the probe
> Reported-by: Chris Morgan <macromorgan@hotmail.com>
> Closes: https://lore.kernel.org/linux-input/20220408212857.9583-1-macroalpha82@gmail.com/
What is this tag? Anything documented? Otherwise use BugLink or Link.
...
> struct adc_joystick_axis *axes;
> struct iio_channel *chans;
> + int *offsets;
Why not unsigned? I.o.w. is there any meaning for negative values?
> int num_chans;
> bool polled;
...
> + off = joy->offsets[i];
> +
Move this blank line to be before the previous line.
> + if (off < 0)
> + return -EINVAL;
...
> case 1:
> - val = ((const u8 *)data)[idx];
> + val = *chan_data;
Might be also
get_unaligned((const u8 *)chan_data);
And with all this (see below) the chan_data actually can be declared
as const void *.
> break;
> case 2:
> - data_u16 = (const u16 *)data + idx;
> -
> /*
> * Data is aligned to the sample size by IIO core.
> * Call `get_unaligned_xe16` to hide type casting.
> */
> if (endianness == IIO_BE)
> - val = get_unaligned_be16(data_u16);
> + val = get_unaligned_be16(chan_data);
> else if (endianness == IIO_LE)
> - val = get_unaligned_le16(data_u16);
> + val = get_unaligned_le16(chan_data);
> else /* IIO_CPU */
> - val = *data_u16;
> + val = *(const u16 *)chan_data;
This probably needs to be
get_unaligned((const u16 *)chan_data);
for the sake of consistency with the above.
> break;
> default:
> return -EINVAL;
...
> +static int adc_joystick_si_cmp(const void *a, const void *b, const void *priv)
> +{
> + const struct iio_channel *chans = priv;
> +
> + return chans[*(int *)a].channel->scan_index -
> + chans[*(int *)b].channel->scan_index;
Discarding const?
> +}
...
> + offsets = kmalloc_array(count, sizeof(int), GFP_KERNEL);
sizeof(*offsets)
> + if (!offsets)
> + return ERR_PTR(-ENOMEM);
...
> + si_order = kmalloc_array(count, sizeof(int), GFP_KERNEL);
sizeof(*si_order)
> + if (!si_order) {
> + kfree(offsets);
> + return ERR_PTR(-ENOMEM);
> + }
...
> + /* Channels in buffer are ordered by scan index. Sort to match that. */
the buffer
> + sort_r(si_order, count, sizeof(int), adc_joystick_si_cmp, NULL, chans);
sizeof(*si_order) ?
sizeof(int) is a bit odd, the above will tell better without even
knowing the sort_r() parameters what it is about.
...
> + for (i = 0; i < count; ++i) {
> + idx = si_order[i];
> + ch = chans[idx].channel;
> + si = ch->scan_index;
> +
> + if (si < 0 || !test_bit(si, indio_dev->active_scan_mask)) {
> + offsets[idx] = -1;
> + continue;
> + }
> +
> + /* Channels sharing scan indices also share the samples. */
> + if (idx > 0 && si == chans[idx - 1].channel->scan_index) {
> + offsets[idx] = offsets[idx - 1];
> + continue;
> + }
> +
> + offsets[idx] = offset;
> + length = ch->scan_type.storagebits / 8;
BITS_PER_BYTE ?
> + if (ch->scan_type.repeat > 1)
> + length *= ch->scan_type.repeat;
> + /* Account for channel alignment. */
> + if (offset % length)
> + offset += length - (offset % length);
Would one of ALIGN() / rounddown / etc work here?
> + offset += length;
> + }
...
> + joy->offsets = adc_joystick_get_chan_offsets(joy->chans,
> + joy->num_chans);
> + if (IS_ERR(joy->offsets)) {
> + dev_err(devp, "Unable to allocate channel offsets\n");
> + return PTR_ERR(joy->offsets);
> + }
> +
> + return 0;
return PTR_ERR_OR_ZERO() ?
...
> error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
> joy->buffer);
> - if (error) {
> + if (error) {
> dev_err(dev, "Unable to add action\n");
> return error;
> }
Unrelated change.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH v5 16/44] Input: gameport: add ISA and HAS_IOPORT dependencies
From: Niklas Schnelle @ 2023-05-22 10:50 UTC (permalink / raw)
To: Arnd Bergmann, Dmitry Torokhov
Cc: Greg Kroah-Hartman, Bjorn Helgaas, Uwe Kleine-König,
Mauro Carvalho Chehab, Alan Stern, Rafael J. Wysocki,
Geert Uytterhoeven, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-kernel, linux-arch, linux-pci, Arnd Bergmann, linux-input
In-Reply-To: <20230522105049.1467313-1-schnelle@linux.ibm.com>
In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
not being declared. As ISA already implies HAS_IOPORT we can simply add
this dependency and guard sections of code using inb()/outb() as
alternative access methods.
Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
drivers/input/gameport/Kconfig | 4 +++-
include/linux/gameport.h | 9 +++++++--
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/input/gameport/Kconfig b/drivers/input/gameport/Kconfig
index 5a2c2fb3217d..fe73b26e647a 100644
--- a/drivers/input/gameport/Kconfig
+++ b/drivers/input/gameport/Kconfig
@@ -25,6 +25,7 @@ if GAMEPORT
config GAMEPORT_NS558
tristate "Classic ISA and PnP gameport support"
+ depends on ISA
help
Say Y here if you have an ISA or PnP gameport.
@@ -35,6 +36,7 @@ config GAMEPORT_NS558
config GAMEPORT_L4
tristate "PDPI Lightning 4 gamecard support"
+ depends on ISA
help
Say Y here if you have a PDPI Lightning 4 gamecard.
@@ -53,7 +55,7 @@ config GAMEPORT_EMU10K1
config GAMEPORT_FM801
tristate "ForteMedia FM801 gameport support"
- depends on PCI
+ depends on PCI && HAS_IOPORT
help
Say Y here if you have ForteMedia FM801 PCI audio controller
(Abit AU10, Genius Sound Maker, HP Workstation zx2000,
diff --git a/include/linux/gameport.h b/include/linux/gameport.h
index 8c2f00018e89..4d5720022b63 100644
--- a/include/linux/gameport.h
+++ b/include/linux/gameport.h
@@ -167,16 +167,21 @@ static inline void gameport_trigger(struct gameport *gameport)
{
if (gameport->trigger)
gameport->trigger(gameport);
+#ifdef CONFIG_HAS_IOPORT
else
outb(0xff, gameport->io);
+#endif
}
static inline unsigned char gameport_read(struct gameport *gameport)
{
if (gameport->read)
return gameport->read(gameport);
- else
- return inb(gameport->io);
+#ifdef CONFIG_HAS_IOPORT
+ return inb(gameport->io);
+#else
+ return 0xff;
+#endif
}
static inline int gameport_cooked_read(struct gameport *gameport, int *axes, int *buttons)
--
2.39.2
^ permalink raw reply related
* [PATCH v5 15/44] Input: add HAS_IOPORT dependencies
From: Niklas Schnelle @ 2023-05-22 10:50 UTC (permalink / raw)
To: Arnd Bergmann, Dmitry Torokhov
Cc: Greg Kroah-Hartman, Bjorn Helgaas, Uwe Kleine-König,
Mauro Carvalho Chehab, Alan Stern, Rafael J. Wysocki,
Geert Uytterhoeven, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-kernel, linux-arch, linux-pci, Arnd Bergmann, linux-input
In-Reply-To: <20230522105049.1467313-1-schnelle@linux.ibm.com>
In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
not being declared. We thus need to add HAS_IOPORT as dependency for
those drivers using them.
Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
drivers/input/serio/Kconfig | 1 +
drivers/input/touchscreen/Kconfig | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index f39b7b3f7942..17edc1597446 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -148,6 +148,7 @@ config HIL_MLC
config SERIO_PCIPS2
tristate "PCI PS/2 keyboard and PS/2 mouse controller"
depends on PCI
+ depends on HAS_IOPORT
help
Say Y here if you have a Mobility Docking station with PS/2
keyboard and mice ports.
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 143ff43c67ae..c2cbd332af1d 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -700,6 +700,7 @@ config TOUCHSCREEN_INEXIO
config TOUCHSCREEN_MK712
tristate "ICS MicroClock MK712 touchscreen"
+ depends on ISA
help
Say Y here if you have the ICS MicroClock MK712 touchscreen
controller chip in your system.
--
2.39.2
^ permalink raw reply related
* Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer
From: Paul Cercueil @ 2023-05-22 10:23 UTC (permalink / raw)
To: Andy Shevchenko, Artur Rojek
Cc: Jonathan Cameron, Dmitry Torokhov, Chris Morgan, linux-mips,
linux-iio, linux-kernel, linux-input
In-Reply-To: <CAHp75VedgVOA4qTJFeVuabKXBaB=y4Ss0fLu7a7J9GGgWFPqQg@mail.gmail.com>
Hi Andy,
Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit :
> On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > <contact@artur-rojek.eu> wrote:
>
> ...
>
> > > + u16 tdat[6];
> > > + u32 val;
> > > +
> > > + memset(tdat, 0, ARRAY_SIZE(tdat));
> >
> > Yeah, as LKP tells us this should be sizeof() instead of
> > ARRAY_SIZE().
> >
> > > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
> > > + if (mask & 0x3) {
> >
> > (for the consistency it has to be GENMASK(), but see below)
> >
> > First of all, strictly speaking we should use the full mask without
> > limiting it to the 0 element in the array (I'm talking about
> > active_scan_mask).
> >
> > That said, we may actually use bit operations here in a better way,
> > i.e.
> >
> > unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] -
> > 1);
> >
> > j = 0;
> > for_each_set_bit(i, active_scan_mask, ...) {
> > val = readl(...);
> > /* Two channels per sample. Demux active. */
> > tdat[j++] = val >> (16 * (i % 2));
>
> Alternatively
>
> /* Two channels per sample. Demux active. */
> if (i % 2)
> tdat[j++] = upper_16_bits(val);
> else
> tdat[j++] = lower_16_bits(val);
>
> which may be better to read.
It's not if/else though. You would check (i % 2) for the upper 16 bits,
and (i % 1) for the lower 16 bits. Both can be valid at the same time.
Cheers,
-Paul
>
> > }
> >
> > > + val = readl(adc->base +
> > > JZ_ADC_REG_ADTCH);
> > > + /* Two channels per sample. Demux active.
> > > */
> > > + if (mask & BIT(0))
> > > + tdat[i++] = val & 0xffff;
> > > + if (mask & BIT(1))
> > > + tdat[i++] = val >> 16;
> > > + }
> > > }
>
>
^ permalink raw reply
* Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer
From: Paul Cercueil @ 2023-05-22 10:20 UTC (permalink / raw)
To: Andy Shevchenko, Artur Rojek
Cc: Jonathan Cameron, Dmitry Torokhov, Chris Morgan, linux-mips,
linux-iio, linux-kernel, linux-input
In-Reply-To: <CAHp75VeLRHwcKQALwnBb-gqVeyxxH=_F40TserRXqo_kbaZzoQ@mail.gmail.com>
Hi,
Le lundi 22 mai 2023 à 13:15 +0300, Andy Shevchenko a écrit :
> On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu>
> wrote:
> >
> > Consumers expect the buffer to only contain enabled channels. While
> > preparing the buffer, the driver makes two mistakes:
> > 1) It inserts empty data for disabled channels.
> > 2) Each ADC readout contains samples for two 16-bit channels. If
> > either
> > of them is active, the whole 32-bit sample is pushed into the
> > buffer
> > as-is.
> >
> > Both of those issues cause the active channels to appear at the
> > wrong
> > offsets in the buffer. Fix the above by demuxing samples for active
> > channels only.
> >
> > This has remained unnoticed, as all the consumers so far were only
> > using
> > channels 0 and 1, leaving them unaffected by changes introduced in
> > this
> > commit.
>
> ...
>
> > + u16 tdat[6];
> > + u32 val;
> > +
> > + memset(tdat, 0, ARRAY_SIZE(tdat));
>
> Yeah, as LKP tells us this should be sizeof() instead of
> ARRAY_SIZE().
Probably "u16 tdat[6] = { 0 };" would work too?
Cheers,
-Paul
>
> > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
> > + if (mask & 0x3) {
>
> (for the consistency it has to be GENMASK(), but see below)
>
> First of all, strictly speaking we should use the full mask without
> limiting it to the 0 element in the array (I'm talking about
> active_scan_mask).
>
> That said, we may actually use bit operations here in a better way,
> i.e.
>
> unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] -
> 1);
>
> j = 0;
> for_each_set_bit(i, active_scan_mask, ...) {
> val = readl(...);
> /* Two channels per sample. Demux active. */
> tdat[j++] = val >> (16 * (i % 2));
> }
>
> > + val = readl(adc->base + JZ_ADC_REG_ADTCH);
> > + /* Two channels per sample. Demux active.
> > */
> > + if (mask & BIT(0))
> > + tdat[i++] = val & 0xffff;
> > + if (mask & BIT(1))
> > + tdat[i++] = val >> 16;
> > + }
> > }
>
^ permalink raw reply
* Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer
From: Andy Shevchenko @ 2023-05-22 10:18 UTC (permalink / raw)
To: Artur Rojek
Cc: Paul Cercueil, Jonathan Cameron, Dmitry Torokhov, Chris Morgan,
linux-mips, linux-iio, linux-kernel, linux-input
In-Reply-To: <CAHp75VeLRHwcKQALwnBb-gqVeyxxH=_F40TserRXqo_kbaZzoQ@mail.gmail.com>
On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu> wrote:
...
> > + u16 tdat[6];
> > + u32 val;
> > +
> > + memset(tdat, 0, ARRAY_SIZE(tdat));
>
> Yeah, as LKP tells us this should be sizeof() instead of ARRAY_SIZE().
>
> > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
> > + if (mask & 0x3) {
>
> (for the consistency it has to be GENMASK(), but see below)
>
> First of all, strictly speaking we should use the full mask without
> limiting it to the 0 element in the array (I'm talking about
> active_scan_mask).
>
> That said, we may actually use bit operations here in a better way, i.e.
>
> unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] - 1);
>
> j = 0;
> for_each_set_bit(i, active_scan_mask, ...) {
> val = readl(...);
> /* Two channels per sample. Demux active. */
> tdat[j++] = val >> (16 * (i % 2));
Alternatively
/* Two channels per sample. Demux active. */
if (i % 2)
tdat[j++] = upper_16_bits(val);
else
tdat[j++] = lower_16_bits(val);
which may be better to read.
> }
>
> > + val = readl(adc->base + JZ_ADC_REG_ADTCH);
> > + /* Two channels per sample. Demux active. */
> > + if (mask & BIT(0))
> > + tdat[i++] = val & 0xffff;
> > + if (mask & BIT(1))
> > + tdat[i++] = val >> 16;
> > + }
> > }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer
From: Andy Shevchenko @ 2023-05-22 10:15 UTC (permalink / raw)
To: Artur Rojek
Cc: Paul Cercueil, Jonathan Cameron, Dmitry Torokhov, Chris Morgan,
linux-mips, linux-iio, linux-kernel, linux-input
In-Reply-To: <20230521225901.388455-2-contact@artur-rojek.eu>
On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu> wrote:
>
> Consumers expect the buffer to only contain enabled channels. While
> preparing the buffer, the driver makes two mistakes:
> 1) It inserts empty data for disabled channels.
> 2) Each ADC readout contains samples for two 16-bit channels. If either
> of them is active, the whole 32-bit sample is pushed into the buffer
> as-is.
>
> Both of those issues cause the active channels to appear at the wrong
> offsets in the buffer. Fix the above by demuxing samples for active
> channels only.
>
> This has remained unnoticed, as all the consumers so far were only using
> channels 0 and 1, leaving them unaffected by changes introduced in this
> commit.
...
> + u16 tdat[6];
> + u32 val;
> +
> + memset(tdat, 0, ARRAY_SIZE(tdat));
Yeah, as LKP tells us this should be sizeof() instead of ARRAY_SIZE().
> + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
> + if (mask & 0x3) {
(for the consistency it has to be GENMASK(), but see below)
First of all, strictly speaking we should use the full mask without
limiting it to the 0 element in the array (I'm talking about
active_scan_mask).
That said, we may actually use bit operations here in a better way, i.e.
unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] - 1);
j = 0;
for_each_set_bit(i, active_scan_mask, ...) {
val = readl(...);
/* Two channels per sample. Demux active. */
tdat[j++] = val >> (16 * (i % 2));
}
> + val = readl(adc->base + JZ_ADC_REG_ADTCH);
> + /* Two channels per sample. Demux active. */
> + if (mask & BIT(0))
> + tdat[i++] = val & 0xffff;
> + if (mask & BIT(1))
> + tdat[i++] = val >> 16;
> + }
> }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH v2 2/2] input: joystick: Fix buffer data parsing
From: Artur Rojek @ 2023-05-21 22:59 UTC (permalink / raw)
To: Paul Cercueil, Jonathan Cameron, Dmitry Torokhov, Chris Morgan,
Andy Shevchenko
Cc: linux-mips, linux-iio, linux-kernel, linux-input, Artur Rojek
In-Reply-To: <20230521225901.388455-1-contact@artur-rojek.eu>
Don't try to access buffer data of a channel by its scan index. Instead,
calculate its offset in the buffer.
This is necessary, as the scan index of a channel does not represent its
position in a buffer - the buffer will contain data for enabled channels
only, affecting data offsets and alignment.
While at it, also fix minor style issue in probe.
Reported-by: Chris Morgan <macromorgan@hotmail.com>
Closes: https://lore.kernel.org/linux-input/20220408212857.9583-1-macroalpha82@gmail.com/
Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
Tested-by: Paul Cercueil <paul@crapouillou.net>
---
v2: - provide new implementation for calculating channel offsets
- cache the resulting offsets
- fix minor style issue in probe
- drop the "Fixes" tag
drivers/input/joystick/adc-joystick.c | 102 +++++++++++++++++++++++---
1 file changed, 90 insertions(+), 12 deletions(-)
diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
index c0deff5d4282..2f9f0cae8f95 100644
--- a/drivers/input/joystick/adc-joystick.c
+++ b/drivers/input/joystick/adc-joystick.c
@@ -10,6 +10,7 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/property.h>
+#include <linux/sort.h>
#include <asm/unaligned.h>
@@ -25,6 +26,7 @@ struct adc_joystick {
struct iio_cb_buffer *buffer;
struct adc_joystick_axis *axes;
struct iio_channel *chans;
+ int *offsets;
int num_chans;
bool polled;
};
@@ -47,35 +49,38 @@ static int adc_joystick_handle(const void *data, void *private)
{
struct adc_joystick *joy = private;
enum iio_endian endianness;
- int bytes, msb, val, idx, i;
- const u16 *data_u16;
+ int bytes, msb, val, off, i;
+ const u8 *chan_data;
bool sign;
bytes = joy->chans[0].channel->scan_type.storagebits >> 3;
for (i = 0; i < joy->num_chans; ++i) {
- idx = joy->chans[i].channel->scan_index;
endianness = joy->chans[i].channel->scan_type.endianness;
msb = joy->chans[i].channel->scan_type.realbits - 1;
sign = tolower(joy->chans[i].channel->scan_type.sign) == 's';
+ off = joy->offsets[i];
+
+ if (off < 0)
+ return -EINVAL;
+
+ chan_data = (const u8 *)data + off;
switch (bytes) {
case 1:
- val = ((const u8 *)data)[idx];
+ val = *chan_data;
break;
case 2:
- data_u16 = (const u16 *)data + idx;
-
/*
* Data is aligned to the sample size by IIO core.
* Call `get_unaligned_xe16` to hide type casting.
*/
if (endianness == IIO_BE)
- val = get_unaligned_be16(data_u16);
+ val = get_unaligned_be16(chan_data);
else if (endianness == IIO_LE)
- val = get_unaligned_le16(data_u16);
+ val = get_unaligned_le16(chan_data);
else /* IIO_CPU */
- val = *data_u16;
+ val = *(const u16 *)chan_data;
break;
default:
return -EINVAL;
@@ -94,6 +99,69 @@ static int adc_joystick_handle(const void *data, void *private)
return 0;
}
+static int adc_joystick_si_cmp(const void *a, const void *b, const void *priv)
+{
+ const struct iio_channel *chans = priv;
+
+ return chans[*(int *)a].channel->scan_index -
+ chans[*(int *)b].channel->scan_index;
+}
+
+static int *adc_joystick_get_chan_offsets(struct iio_channel *chans, int count)
+{
+ struct iio_dev *indio_dev = chans[0].indio_dev;
+ const struct iio_chan_spec *ch;
+ int *offsets, *si_order;
+ int idx, i, si, length, offset = 0;
+
+ offsets = kmalloc_array(count, sizeof(int), GFP_KERNEL);
+ if (!offsets)
+ return ERR_PTR(-ENOMEM);
+
+ si_order = kmalloc_array(count, sizeof(int), GFP_KERNEL);
+ if (!si_order) {
+ kfree(offsets);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ for (i = 0; i < count; ++i)
+ si_order[i] = i;
+ /* Channels in buffer are ordered by scan index. Sort to match that. */
+ sort_r(si_order, count, sizeof(int), adc_joystick_si_cmp, NULL, chans);
+
+ for (i = 0; i < count; ++i) {
+ idx = si_order[i];
+ ch = chans[idx].channel;
+ si = ch->scan_index;
+
+ if (si < 0 || !test_bit(si, indio_dev->active_scan_mask)) {
+ offsets[idx] = -1;
+ continue;
+ }
+
+ /* Channels sharing scan indices also share the samples. */
+ if (idx > 0 && si == chans[idx - 1].channel->scan_index) {
+ offsets[idx] = offsets[idx - 1];
+ continue;
+ }
+
+ offsets[idx] = offset;
+
+ length = ch->scan_type.storagebits / 8;
+ if (ch->scan_type.repeat > 1)
+ length *= ch->scan_type.repeat;
+
+ /* Account for channel alignment. */
+ if (offset % length)
+ offset += length - (offset % length);
+ offset += length;
+ }
+
+ kfree(si_order);
+
+ return offsets;
+}
+
static int adc_joystick_open(struct input_dev *dev)
{
struct adc_joystick *joy = input_get_drvdata(dev);
@@ -101,10 +169,19 @@ static int adc_joystick_open(struct input_dev *dev)
int ret;
ret = iio_channel_start_all_cb(joy->buffer);
- if (ret)
+ if (ret) {
dev_err(devp, "Unable to start callback buffer: %d\n", ret);
+ return ret;
+ }
- return ret;
+ joy->offsets = adc_joystick_get_chan_offsets(joy->chans,
+ joy->num_chans);
+ if (IS_ERR(joy->offsets)) {
+ dev_err(devp, "Unable to allocate channel offsets\n");
+ return PTR_ERR(joy->offsets);
+ }
+
+ return 0;
}
static void adc_joystick_close(struct input_dev *dev)
@@ -112,6 +189,7 @@ static void adc_joystick_close(struct input_dev *dev)
struct adc_joystick *joy = input_get_drvdata(dev);
iio_channel_stop_all_cb(joy->buffer);
+ kfree(joy->offsets);
}
static void adc_joystick_cleanup(void *data)
@@ -269,7 +347,7 @@ static int adc_joystick_probe(struct platform_device *pdev)
error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
joy->buffer);
- if (error) {
+ if (error) {
dev_err(dev, "Unable to add action\n");
return error;
}
--
2.40.1
^ permalink raw reply related
* [PATCH v2 0/2] iio/adc-joystick: buffer data parsing fixes
From: Artur Rojek @ 2023-05-21 22:58 UTC (permalink / raw)
To: Paul Cercueil, Jonathan Cameron, Dmitry Torokhov, Chris Morgan,
Andy Shevchenko
Cc: linux-mips, linux-iio, linux-kernel, linux-input, Artur Rojek
Hi all,
this is a long overdue v2 for fixes [1] related to adc-joystick channel
parsing.
Patch [1/2] now also addresses a case where a readout of ADC data
containing samples for two channels, where only one channel is active,
would push the data into a buffer without discarding the disabled
channel first.
Patch [2/2] addresses concerns about exposure to channels the clients
shouldn't know about. The IIO helpers from v1 are gone and the offsets
are now calculated based only on the channels adc-joystick has access
to.
Tested on GCW Zero (by me) and on Anbernic RG350 (by Paul).
[1] https://lore.kernel.org/all/20220817105643.95710-1-contact@artur-rojek.eu/
Cheers,
Artur
Artur Rojek (2):
iio/adc: ingenic: Fix channel offsets in buffer
input: joystick: Fix buffer data parsing
drivers/iio/adc/ingenic-adc.c | 20 +++--
drivers/input/joystick/adc-joystick.c | 102 +++++++++++++++++++++++---
2 files changed, 103 insertions(+), 19 deletions(-)
--
2.40.1
^ permalink raw reply
* [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer
From: Artur Rojek @ 2023-05-21 22:59 UTC (permalink / raw)
To: Paul Cercueil, Jonathan Cameron, Dmitry Torokhov, Chris Morgan,
Andy Shevchenko
Cc: linux-mips, linux-iio, linux-kernel, linux-input, Artur Rojek
In-Reply-To: <20230521225901.388455-1-contact@artur-rojek.eu>
Consumers expect the buffer to only contain enabled channels. While
preparing the buffer, the driver makes two mistakes:
1) It inserts empty data for disabled channels.
2) Each ADC readout contains samples for two 16-bit channels. If either
of them is active, the whole 32-bit sample is pushed into the buffer
as-is.
Both of those issues cause the active channels to appear at the wrong
offsets in the buffer. Fix the above by demuxing samples for active
channels only.
This has remained unnoticed, as all the consumers so far were only using
channels 0 and 1, leaving them unaffected by changes introduced in this
commit.
Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
Tested-by: Paul Cercueil <paul@crapouillou.net>
---
v2: - demux active channels from ADC readouts
- clarify in the commit description that this patch doesn't impact
existing consumers of this driver
drivers/iio/adc/ingenic-adc.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
index a7325dbbb99a..093710a7ad4c 100644
--- a/drivers/iio/adc/ingenic-adc.c
+++ b/drivers/iio/adc/ingenic-adc.c
@@ -802,13 +802,19 @@ static irqreturn_t ingenic_adc_irq(int irq, void *data)
struct ingenic_adc *adc = iio_priv(iio_dev);
unsigned long mask = iio_dev->active_scan_mask[0];
unsigned int i;
- u32 tdat[3];
-
- for (i = 0; i < ARRAY_SIZE(tdat); mask >>= 2, i++) {
- if (mask & 0x3)
- tdat[i] = readl(adc->base + JZ_ADC_REG_ADTCH);
- else
- tdat[i] = 0;
+ u16 tdat[6];
+ u32 val;
+
+ memset(tdat, 0, ARRAY_SIZE(tdat));
+ for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
+ if (mask & 0x3) {
+ val = readl(adc->base + JZ_ADC_REG_ADTCH);
+ /* Two channels per sample. Demux active. */
+ if (mask & BIT(0))
+ tdat[i++] = val & 0xffff;
+ if (mask & BIT(1))
+ tdat[i++] = val >> 16;
+ }
}
iio_push_to_buffers(iio_dev, tdat);
--
2.40.1
^ permalink raw reply related
* [PATCH 2/2] Input: mms114 - add support for touch keys
From: Artur Weber @ 2023-05-21 14:58 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring
Cc: Krzysztof Kozlowski, Linus Walleij, linux-input, devicetree,
linux-kernel, ~postmarketos/upstreaming, Artur Weber
In-Reply-To: <20230521145843.19489-1-aweber.kernel@gmail.com>
MELFAS MMS114 and similar touchscreens have support for touch keys.
Enable support of them in the driver. The keycodes to emit can be
controlled by the linux,keycodes DT property.
Sidenote - the MAX_TOUCHKEYS value is set to 15, as that's the
maximum value that the ID field can contain. I don't have access
to any datasheets that could confirm or deny whether this is accurate.
Most downstream drivers I've been able to find only use up to 2 keys
(though I did find a driver that mentioned up to 4, but only 2 were
used). They don't have any checks for a maximum keycode value, it is
just extracted from the ID bits (0xf mask).
The drivers I've been able to find also don't use touch ID 0; I assume
that it is never used, so the keycodes provided in the DT start from
touch ID 1. I suppose this is in-line with the regular behavior
for touch IDs in touchscreen events, as there the provided touch ID
is always lowered by 1, which would cause an overflow if it was 0...
Just in case, we quietly return if the touch ID is set to 0 here.
The implementation of the linux,keycodes property handling code was
adapted from the msg2638 driver.
Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
drivers/input/touchscreen/mms114.c | 88 +++++++++++++++++++++++++++---
1 file changed, 81 insertions(+), 7 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index ac12494c7930..14c0514a21ff 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -43,6 +43,7 @@
/* Touchscreen absolute values */
#define MMS114_MAX_AREA 0xff
+#define MMS114_MAX_TOUCHKEYS 15
#define MMS114_MAX_TOUCH 10
#define MMS114_EVENT_SIZE 8
#define MMS136_EVENT_SIZE 6
@@ -70,6 +71,9 @@ struct mms114_data {
unsigned int contact_threshold;
unsigned int moving_threshold;
+ u32 keycodes[MMS114_MAX_TOUCHKEYS];
+ int num_keycodes;
+
/* Use cache data for mode control register(write only) */
u8 cache_mode_control;
};
@@ -167,11 +171,6 @@ static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *tou
return;
}
- if (touch->type != MMS114_TYPE_TOUCHSCREEN) {
- dev_err(&client->dev, "Wrong touch type (%d)\n", touch->type);
- return;
- }
-
id = touch->id - 1;
x = touch->x_lo | touch->x_hi << 8;
y = touch->y_lo | touch->y_hi << 8;
@@ -191,9 +190,33 @@ static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *tou
}
}
+static void mms114_process_touchkey(struct mms114_data *data,
+ struct mms114_touch *touch)
+{
+ struct i2c_client *client = data->client;
+ struct input_dev *input_dev = data->input_dev;
+ unsigned int keycode_id;
+
+ if (touch->id == 0)
+ return;
+
+ if (touch->id > data->num_keycodes) {
+ dev_err(&client->dev, "Wrong touch id for touchkey (%d)\n",
+ touch->id);
+ return;
+ }
+
+ keycode_id = touch->id - 1;
+ dev_dbg(&client->dev, "keycode id: %d, pressed: %d\n", keycode_id,
+ touch->pressed);
+
+ input_report_key(input_dev, data->keycodes[keycode_id], touch->pressed);
+}
+
static irqreturn_t mms114_interrupt(int irq, void *dev_id)
{
struct mms114_data *data = dev_id;
+ struct i2c_client *client = data->client;
struct input_dev *input_dev = data->input_dev;
struct mms114_touch touch[MMS114_MAX_TOUCH];
int packet_size;
@@ -223,8 +246,22 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
if (error < 0)
goto out;
- for (index = 0; index < touch_size; index++)
- mms114_process_mt(data, touch + index);
+ for (index = 0; index < touch_size; index++) {
+ switch (touch[index].type) {
+ case MMS114_TYPE_TOUCHSCREEN:
+ mms114_process_mt(data, touch + index);
+ break;
+
+ case MMS114_TYPE_TOUCHKEY:
+ mms114_process_touchkey(data, touch + index);
+ break;
+
+ default:
+ dev_err(&client->dev, "Wrong touch type (%d)\n",
+ touch[index].type);
+ break;
+ }
+ }
input_mt_report_pointer_emulation(data->input_dev, true);
input_sync(data->input_dev);
@@ -446,6 +483,7 @@ static int mms114_probe(struct i2c_client *client)
struct input_dev *input_dev;
const void *match_data;
int error;
+ int i;
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
dev_err(&client->dev, "Not supported I2C adapter\n");
@@ -469,6 +507,42 @@ static int mms114_probe(struct i2c_client *client)
data->type = (enum mms_type)match_data;
+ data->num_keycodes = device_property_count_u32(&client->dev,
+ "linux,keycodes");
+ if (data->num_keycodes == -EINVAL) {
+ data->num_keycodes = 0;
+ } else if (data->num_keycodes < 0) {
+ dev_err(&client->dev,
+ "Unable to parse linux,keycodes property: %d\n",
+ data->num_keycodes);
+ return data->num_keycodes;
+ } else if (data->num_keycodes > MMS114_MAX_TOUCHKEYS) {
+ dev_warn(&client->dev,
+ "Found %d linux,keycodes but max is %zd, ignoring the rest\n",
+ data->num_keycodes, MMS114_MAX_TOUCHKEYS);
+ data->num_keycodes = MMS114_MAX_TOUCHKEYS;
+ }
+
+ if (data->num_keycodes > 0) {
+ error = device_property_read_u32_array(&client->dev,
+ "linux,keycodes",
+ data->keycodes,
+ data->num_keycodes);
+ if (error) {
+ dev_err(&client->dev,
+ "Unable to read linux,keycodes values: %d\n",
+ error);
+ return error;
+ }
+
+ input_dev->keycode = data->keycodes;
+ input_dev->keycodemax = data->num_keycodes;
+ input_dev->keycodesize = sizeof(data->keycodes[0]);
+ for (i = 0; i < data->num_keycodes; i++)
+ input_set_capability(input_dev,
+ EV_KEY, data->keycodes[i]);
+ }
+
input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
--
2.40.1
^ permalink raw reply related
* [PATCH 1/2] dt-bindings: mms114: Add linux,keycodes property for touch keys
From: Artur Weber @ 2023-05-21 14:58 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring
Cc: Krzysztof Kozlowski, Linus Walleij, linux-input, devicetree,
linux-kernel, ~postmarketos/upstreaming, Artur Weber
In-Reply-To: <20230521145843.19489-1-aweber.kernel@gmail.com>
MELFAS MMS114 and similar touchscreens have support for touch keys.
Add the linux,keycodes property which can be used to set up the
keycodes for the touch keys.
Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
.../devicetree/bindings/input/touchscreen/melfas,mms114.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/melfas,mms114.yaml b/Documentation/devicetree/bindings/input/touchscreen/melfas,mms114.yaml
index fdd02898e249..07f9dd6b1c9c 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/melfas,mms114.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/melfas,mms114.yaml
@@ -52,6 +52,11 @@ properties:
touchscreen-swapped-x-y: true
touchscreen-max-pressure: true
+ linux,keycodes:
+ description: Keycodes for the touch keys
+ minItems: 1
+ maxItems: 15
+
additionalProperties: false
required:
--
2.40.1
^ permalink raw reply related
* [PATCH 0/2] mms114: add support for touch keys
From: Artur Weber @ 2023-05-21 14:58 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring
Cc: Krzysztof Kozlowski, Linus Walleij, linux-input, devicetree,
linux-kernel, ~postmarketos/upstreaming, Artur Weber
MELFAS MMS114 and similar touchscreens have support for touch keys.
Enable support of them in the driver. The keycodes to emit can be
controlled by the linux,keycodes DT property.
Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
Artur Weber (2):
dt-bindings: mms114: Add linux,keycodes property for touch keys
Input: mms114 - add support for touch keys
.../input/touchscreen/melfas,mms114.yaml | 5 ++
drivers/input/touchscreen/mms114.c | 88 +++++++++++++++++--
2 files changed, 86 insertions(+), 7 deletions(-)
base-commit: f219050af06d83f436945880fc9c04db3bb2860f
--
2.40.1
^ 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