* [PATCH] Input: psmouse - add resync_on_resume dmi check
From: Jonathan Denose @ 2023-11-02 12:52 UTC (permalink / raw)
To: LKML
Cc: jefferymiller, Jonathan Denose, Dmitry Torokhov, Raul Rangel,
linux-input
Some elantech touchpads consistently fail after resuming from
suspend at sanity_check in elantech_packet_check_v4. This means
the touchpad is completely unusable after suspend resume.
With different permutations of i8042 nomux, nopnp, reset, and noloop
kernel options enabled, and with crc_enabled the touchpad fails in
the same way.
Resyncing the touchpad after receiving the
PACKET_UNKNOWN/PSMOUSE_BAD_DATA return code allows the touchpad to
function correctly on resume. The touchpad fails to reconnect with
the serio reconnect no matter how many times it retries, so this
change skips over that retry sequence and goes directly to resync.
Signed-off-by: Jonathan Denose <jdenose@google.com>
---
drivers/input/mouse/psmouse-base.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
index a0aac76b1e41d..3c6eefcb9582f 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -12,6 +12,7 @@
#include <linux/bitops.h>
#include <linux/delay.h>
+#include <linux/dmi.h>
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/interrupt.h>
@@ -105,6 +106,16 @@ static struct attribute *psmouse_dev_attrs[] = {
ATTRIBUTE_GROUPS(psmouse_dev);
+static const struct dmi_system_id resync_on_resume[] = {
+ {
+ .ident = "Lenovo N24",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo N24"),
+ },
+ }
+};
+
/*
* psmouse_mutex protects all operations changing state of mouse
* (connecting, disconnecting, changing rate or resolution via
@@ -285,6 +296,12 @@ static int psmouse_handle_byte(struct psmouse *psmouse)
"%s at %s lost sync at byte %d\n",
psmouse->name, psmouse->phys,
psmouse->pktcnt);
+ if (dmi_check_system(resync_on_resume)) {
+ psmouse_notice(psmouse, "issuing resync request");
+ __psmouse_set_state(psmouse, PSMOUSE_RESYNCING);
+ psmouse_queue_work(psmouse, &psmouse->resync_work, 0);
+ return -EIO;
+ }
if (++psmouse->out_of_sync_cnt == psmouse->resetafter) {
__psmouse_set_state(psmouse, PSMOUSE_IGNORE);
psmouse_notice(psmouse,
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply related
* Re: [PATCH v10 2/4] Input: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-11-02 10:32 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, linux-input, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bastien Nocera, Hans de Goede, Henrik Rydberg,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <ZTnzorJ4h1zva1AZ@nixie71>
Hi Jeff,
On 26/10/2023 07:05, Jeff LaBundy wrote:
> Hi Neil,
>
> On Mon, Oct 23, 2023 at 05:03:46PM +0200, Neil Armstrong wrote:
>
> [...]
>
>> +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
>> +{
>> + __le16 length_raw;
>> + u8 *afe_data;
>> + u16 length;
>> + int error;
>> +
>> + afe_data = kzalloc(GOODIX_BERLIN_IC_INFO_MAX_LEN, GFP_KERNEL);
>> + if (!afe_data)
>> + return -ENOMEM;
>> +
>> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
>> + &length_raw, sizeof(length_raw));
>> + if (error) {
>> + dev_info(cd->dev, "failed get ic info length, %d\n", error);
>
> This should be dev_err().
Ack
>
>> + goto free_afe_data;
>> + }
>> +
>> + length = le16_to_cpu(length_raw);
>> + if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
>> + dev_info(cd->dev, "invalid ic info length %d\n", length);
>
> And here.
Ack
>
>> + error = -EINVAL;
>> + goto free_afe_data;
>> + }
>> +
>> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
>> + afe_data, length);
>> + if (error) {
>> + dev_info(cd->dev, "failed get ic info data, %d\n", error);
>> + return error;
>> + goto free_afe_data;
>> + }
>
> This return statement is left over from v9; the print should also be dev_err().
Ack
>
>> +
>> + /* check whether the data is valid (ex. bus default values) */
>> + if (goodix_berlin_is_dummy_data(cd, afe_data, length)) {
>> + dev_err(cd->dev, "fw info data invalid\n");
>> + error = -EINVAL;
>> + goto free_afe_data;
>> + }
>> +
>> + if (!goodix_berlin_checksum_valid(afe_data, length)) {
>> + dev_info(cd->dev, "fw info checksum error\n");
>
> And here.
Ack
>
>> + error = -EINVAL;
>> + goto free_afe_data;
>> + }
>> +
>> + error = goodix_berlin_convert_ic_info(cd, afe_data, length);
>> + if (error)
>> + goto free_afe_data;
>> +
>> + /* check some key info */
>> + if (!cd->touch_data_addr) {
>> + dev_err(cd->dev, "touch_data_addr is null\n");
>> + error = -EINVAL;
>> + goto free_afe_data;
>> + }
>> +
>> + return 0;
>> +
>> +free_afe_data:
>> + kfree(afe_data);
>> +
>> + return error;
>> +}
>
> [...]
>
>> +static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
>> +{
>> + gpiod_set_value(cd->reset_gpio, 1);
>> + usleep_range(2000, 2100);
>> + gpiod_set_value(cd->reset_gpio, 0);
>
> I see that now, this function is only called if the reset GPIO is defined,
> but there used to be another msleep() here that has since been dropped. Is
> that intentional?
Indeed, it was dropped, I'll add it back thx for noticing !
>
>> +
>> + return 0;
>> +}
>
> Kind regards,
> Jeff LaBundy
Thanks,
Neil
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Bagas Sanjaya @ 2023-11-02 7:51 UTC (permalink / raw)
To: Linux regressions mailing list, Jiri Kosina, David Revoy
Cc: jason.gerecke@wacom.com, jose.exposito89@gmail.com,
ilya.ostapyshyn@gmail.com, Nils Fuhler, Ping Cheng,
Peter Hutterer, Benjamin Tissoires, Linux Kernel Mailing List,
Linux Input Devices
In-Reply-To: <ba7aeb6b-19ee-4491-a60f-efc5216177a7@leemhuis.info>
On 02/11/2023 13:31, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 02.11.23 01:44, Bagas Sanjaya wrote:
>> On Wed, Nov 01, 2023 at 08:37:40PM +0100, Jiri Kosina wrote:
>>> On Wed, 1 Nov 2023, David Revoy wrote:
>>>
>>>> Hi Jason Gerecke, José Expósito, Jiri Kosina and Illia Ostapyshyn,
>>>>
>>>> I am emailing to draw your attention and expertise to a problem I had
>>>> earlier this week with my Xp-Pen Artist 24 Pro display tablet under
>>>> Fedora Linux 38 KDE after a kernel update.
>>>>
>> […]
>
>>>> Thank you very much if you can help me.
>> […]
>> Thanks for the report.
>>
>> David, can you resend the regression report as plain text email (preferably
>> with 276e14e6c3 people and regressions@lists.linux.dev Cc'ed)? You may need to
>> see kernel documentation [1] for how to configure your email client to send
>> plain text emails. Also, include in your report details from your blog post.
>
> Bagas, I know you mean well, but I think you are making things
> unnecessarily complicated for both David and the developers here. Sure,
> the mail Jiri quoted did not make it to lore, but whatever, for him it
> was apparently good enough; and I suspect this "quote forwarding to
> others" is good enough for the people he brought into the loop as well.
> Yes, there are developers that don't want to go to a website for
> details, but if that's the case it's likely better to ask for details in
> this thread instead of opening a second one. And regression tracking can
> work without a separate mail as well.
>
OK, thanks!
--
An old man doll... just what I always wanted! - Clara
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Linux regression tracking #adding (Thorsten Leemhuis) @ 2023-11-02 7:44 UTC (permalink / raw)
To: Jiri Kosina, David Revoy
Cc: jason.gerecke@wacom.com, jose.exposito89@gmail.com,
ilya.ostapyshyn@gmail.com, Nils Fuhler, Ping Cheng,
Peter Hutterer, Benjamin Tissoires, linux-kernel, linux-input,
Linux kernel regressions list
In-Reply-To: <nycvar.YFH.7.76.2311012033290.29220@cbobk.fhfr.pm>
[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]
[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]
On 01.11.23 20:37, Jiri Kosina wrote:
> On Wed, 1 Nov 2023, David Revoy wrote:
>
>> Hi Jason Gerecke, José Expósito, Jiri Kosina and Illia Ostapyshyn,
>>
>> I am emailing to draw your attention and expertise to a problem I had
>> earlier this week with my Xp-Pen Artist 24 Pro display tablet under
>> Fedora Linux 38 KDE after a kernel update.
>>
>> The second button on my stylus changed from a right-click (which I could
>> customise with xsetwacom or any GUI like kcm-tablet) to a button that
>> feels 'hardcoded' and now switches the whole device to an eraser mode.
>> This makes my main tool unusable.
>>
>> I don't have the skills to write a proper kernel bug report, workaround
>> or even identify the exact source of the issue. I have written a blog
>> post about this with more details here:
>> https://www.davidrevoy.com/article995/how-a-kernel-update-broke-my-stylus-need-help
>> , contacting you was something suggested by the comments.
>>
>> Thank you very much if you can help me.
Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:
#regzbot ^introduced 276e14e6c3
https://www.davidrevoy.com/article995/how-a-kernel-update-broke-my-stylus-need-help
#regzbot title HID: input: stylus of Xp-Pen Artist 24 Pro display tablet
changed behavior
#regzbot ignore-activity
This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.
Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-11-02 6:31 UTC (permalink / raw)
To: Bagas Sanjaya, Jiri Kosina, David Revoy
Cc: jason.gerecke@wacom.com, jose.exposito89@gmail.com,
ilya.ostapyshyn@gmail.com, Nils Fuhler, Ping Cheng,
Peter Hutterer, Benjamin Tissoires, Linux Kernel Mailing List,
Linux Input Devices, Linux Regressions
In-Reply-To: <ZULw6AcBaD6z2UZA@debian.me>
On 02.11.23 01:44, Bagas Sanjaya wrote:
> On Wed, Nov 01, 2023 at 08:37:40PM +0100, Jiri Kosina wrote:
>> On Wed, 1 Nov 2023, David Revoy wrote:
>>
>>> Hi Jason Gerecke, José Expósito, Jiri Kosina and Illia Ostapyshyn,
>>>
>>> I am emailing to draw your attention and expertise to a problem I had
>>> earlier this week with my Xp-Pen Artist 24 Pro display tablet under
>>> Fedora Linux 38 KDE after a kernel update.
>>>
> […]
>>> Thank you very much if you can help me.
> […]
> Thanks for the report.
>
> David, can you resend the regression report as plain text email (preferably
> with 276e14e6c3 people and regressions@lists.linux.dev Cc'ed)? You may need to
> see kernel documentation [1] for how to configure your email client to send
> plain text emails. Also, include in your report details from your blog post.
Bagas, I know you mean well, but I think you are making things
unnecessarily complicated for both David and the developers here. Sure,
the mail Jiri quoted did not make it to lore, but whatever, for him it
was apparently good enough; and I suspect this "quote forwarding to
others" is good enough for the people he brought into the loop as well.
Yes, there are developers that don't want to go to a website for
details, but if that's the case it's likely better to ask for details in
this thread instead of opening a second one. And regression tracking can
work without a separate mail as well.
Ciao, Thorsten
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Bagas Sanjaya @ 2023-11-02 0:44 UTC (permalink / raw)
To: Jiri Kosina, David Revoy
Cc: jason.gerecke@wacom.com, jose.exposito89@gmail.com,
ilya.ostapyshyn@gmail.com, Nils Fuhler, Ping Cheng,
Peter Hutterer, Benjamin Tissoires, Linux Kernel Mailing List,
Linux Input Devices, Linux Regressions
In-Reply-To: <nycvar.YFH.7.76.2311012033290.29220@cbobk.fhfr.pm>
[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]
On Wed, Nov 01, 2023 at 08:37:40PM +0100, Jiri Kosina wrote:
> On Wed, 1 Nov 2023, David Revoy wrote:
>
> > Hi Jason Gerecke, José Expósito, Jiri Kosina and Illia Ostapyshyn,
> >
> > I am emailing to draw your attention and expertise to a problem I had
> > earlier this week with my Xp-Pen Artist 24 Pro display tablet under
> > Fedora Linux 38 KDE after a kernel update.
> >
> > The second button on my stylus changed from a right-click (which I could
> > customise with xsetwacom or any GUI like kcm-tablet) to a button that
> > feels 'hardcoded' and now switches the whole device to an eraser mode.
> > This makes my main tool unusable.
> >
> > I don't have the skills to write a proper kernel bug report, workaround
> > or even identify the exact source of the issue. I have written a blog
> > post about this with more details here:
> > https://www.davidrevoy.com/article995/how-a-kernel-update-broke-my-stylus-need-help
> > , contacting you was something suggested by the comments.
> >
> > Thank you very much if you can help me.
>
> CCing a couple more people involved both in 276e14e6c3 and 87562fcd1342,
> and mailinglists.
>
> This is almost certainly the behavior introduced by 276e14e6c3, where
> previously the button was mapped to BTN_TOUCH, but now it's mapped to
> BTN_TOOL_RUBBER, causing user-visible change in behavior.
>
Thanks for the report.
David, can you resend the regression report as plain text email (preferably
with 276e14e6c3 people and regressions@lists.linux.dev Cc'ed)? You may need to
see kernel documentation [1] for how to configure your email client to send
plain text emails. Also, include in your report details from your blog post.
Thanks.
[1]: https://docs.kernel.org/process/email-clients.html
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
From: James Ogletree @ 2023-11-01 20:47 UTC (permalink / raw)
To: Jeff LaBundy
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Lee Jones, Fred Treven, Ben Bright, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <ZTiD5VUSi65OK4VK@nixie71>
Hi Jeff,
> On Oct 24, 2023, at 9:56 PM, Jeff LaBundy <jeff@labundy.com> wrote:
>
> On Wed, Oct 18, 2023 at 05:57:24PM +0000, James Ogletree wrote:
>>
>> +static irqreturn_t cs40l50_error(int irq, void *data);
>> +
>> +static const struct cs40l50_irq cs40l50_irqs[] = {
>> + CS40L50_IRQ(AMP_SHORT, "Amp short", error),
>> + CS40L50_IRQ(VIRT2_MBOX, "Mailbox", process_mbox),
>> + CS40L50_IRQ(TEMP_ERR, "Overtemperature", error),
>> + CS40L50_IRQ(BST_UVP, "Boost undervoltage", error),
>> + CS40L50_IRQ(BST_SHORT, "Boost short", error),
>> + CS40L50_IRQ(BST_ILIMIT, "Boost current limit", error),
>> + CS40L50_IRQ(UVLO_VDDBATT, "Boost UVLO", error),
>> + CS40L50_IRQ(GLOBAL_ERROR, "Global", error),
>> +};
>> +
>> +static irqreturn_t cs40l50_error(int irq, void *data)
>> +{
>> + struct cs40l50_private *cs40l50 = data;
>> +
>> + dev_err(cs40l50->dev, "%s error\n", cs40l50_irqs[irq].name);
>> +
>> + return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
>> +}
>> +
>> +static const struct regmap_irq cs40l50_reg_irqs[] = {
>> + CS40L50_REG_IRQ(IRQ1_INT_1, AMP_SHORT),
>> + CS40L50_REG_IRQ(IRQ1_INT_2, VIRT2_MBOX),
>> + CS40L50_REG_IRQ(IRQ1_INT_8, TEMP_ERR),
>> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_UVP),
>> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_SHORT),
>> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_ILIMIT),
>> + CS40L50_REG_IRQ(IRQ1_INT_10, UVLO_VDDBATT),
>> + CS40L50_REG_IRQ(IRQ1_INT_18, GLOBAL_ERROR),
>> +};
>> +
>> +static struct regmap_irq_chip cs40l50_irq_chip = {
>> + .name = "CS40L50 IRQ Controller",
>> +
>> + .status_base = CS40L50_IRQ1_INT_1,
>> + .mask_base = CS40L50_IRQ1_MASK_1,
>> + .ack_base = CS40L50_IRQ1_INT_1,
>> + .num_regs = 22,
>> +
>> + .irqs = cs40l50_reg_irqs,
>> + .num_irqs = ARRAY_SIZE(cs40l50_reg_irqs),
>> +
>> + .runtime_pm = true,
>> +};
>> +
>> +static int cs40l50_irq_init(struct cs40l50_private *cs40l50)
>> +{
>> + struct device *dev = cs40l50->dev;
>> + int error, i, irq;
>> +
>> + error = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
>> + IRQF_ONESHOT | IRQF_SHARED, 0,
>> + &cs40l50_irq_chip, &cs40l50->irq_data);
>> + if (error)
>> + return error;
>> +
>> + for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
>> + irq = regmap_irq_get_virq(cs40l50->irq_data, cs40l50_irqs[i].irq);
>> + if (irq < 0) {
>> + dev_err(dev, "Failed getting %s\n", cs40l50_irqs[i].name);
>> + return irq;
>> + }
>> +
>> + error = devm_request_threaded_irq(dev, irq, NULL,
>> + cs40l50_irqs[i].handler,
>> + IRQF_ONESHOT | IRQF_SHARED,
>> + cs40l50_irqs[i].name, cs40l50);
>> + if (error) {
>> + dev_err(dev, "Failed requesting %s\n", cs40l50_irqs[i].name);
>> + return error;
>> + }
>> + }
>
> This is kind of an uncommon design pattern; if anyone reads /proc/interrupts
> on their system, it's going to be full of L50 interrupts. Normally we declare
> a single IRQ, read the status register(s) from inside its handler and then
> act accordingly.
>
> What is the motivation for having one handler per interrupt status bit? If
> multiple bits are set at once, does the register get read multiple times and
> if so, does doing so clear any pending status? Or are the status registers
> write-to-clear instead of read-to-clear?
The reason I used the regmap_irq framework is that it takes care of
the reading and clearing of the status register, and yes it handles the
situation of multiple bits getting set at once. I think I will merge the IRQ
handlers into one for the next version. The fact of /proc/interrupts filling
up with these interrupts is not great and was something I overlooked,
though I think I see instances of drivers with similar amount of interrupts
upstream.
Best,
James
^ permalink raw reply
* Re: [PATCH v4 4/4] Input: cs40l50 - Add support for the CS40L50 haptic driver
From: James Ogletree @ 2023-11-01 20:47 UTC (permalink / raw)
To: Jeff LaBundy
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Lee Jones, Fred Treven, Ben Bright, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <ZTiFbmutojF0LRZU@nixie71>
Hi Jeff,
> On Oct 24, 2023, at 10:03 PM, Jeff LaBundy <jeff@labundy.com> wrote:
>>
>> +static const struct cs_dsp_client_ops cs40l50_cs_dsp_client_ops;
>> +
>> +static const struct cs_dsp_region cs40l50_dsp_regions[] = {
>> + {
>> + .type = WMFW_HALO_PM_PACKED,
>> + .base = CS40L50_DSP1_PMEM_0
>> + },
>> + {
>> + .type = WMFW_HALO_XM_PACKED,
>> + .base = CS40L50_DSP1_XMEM_PACKED_0
>> + },
>> + {
>> + .type = WMFW_HALO_YM_PACKED,
>> + .base = CS40L50_DSP1_YMEM_PACKED_0
>> + },
>> + {
>> + .type = WMFW_ADSP2_XM,
>> + .base = CS40L50_DSP1_XMEM_UNPACKED24_0
>> + },
>> + {
>> + .type = WMFW_ADSP2_YM,
>> + .base = CS40L50_DSP1_YMEM_UNPACKED24_0
>> + },
>> +};
>> +
>> +static int cs40l50_cs_dsp_init(struct cs40l50_private *cs40l50)
>> +{
>> + cs40l50->dsp.num = 1;
>> + cs40l50->dsp.type = WMFW_HALO;
>> + cs40l50->dsp.dev = cs40l50->dev;
>> + cs40l50->dsp.regmap = cs40l50->regmap;
>> + cs40l50->dsp.base = CS40L50_CORE_BASE;
>> + cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID;
>> + cs40l50->dsp.mem = cs40l50_dsp_regions;
>> + cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
>> + cs40l50->dsp.no_core_startstop = true;
>> + cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
>> +
>> + return cs_dsp_halo_init(&cs40l50->dsp);
>> +}
>> +
>> +static struct cs_hap_bank cs40l50_banks[] = {
>> + {
>> + .bank = WVFRM_BANK_RAM,
>> + .base_index = CS40L50_RAM_BANK_INDEX_START,
>> + .max_index = CS40L50_RAM_BANK_INDEX_START,
>> + },
>> + {
>> + .bank = WVFRM_BANK_ROM,
>> + .base_index = CS40L50_ROM_BANK_INDEX_START,
>> + .max_index = CS40L50_ROM_BANK_INDEX_END,
>> + },
>> + {
>> + .bank = WVFRM_BANK_OWT,
>> + .base_index = CS40L50_RTH_INDEX_START,
>> + .max_index = CS40L50_RTH_INDEX_END,
>> + },
>> +};
>
> These structs describe the DSP, and hence the silicon; they are not
> specific to the input/FF device. Presumably the DSP could run algorithms
> that support only the I2S streaming case as well (e.g. A2H); therefore,
> these seem more appropriately placed in the MFD.
Acknowledged, but would you consider the last struct “cs40l50_banks” as
an exception? It would go unused in a codec-only setup.
Best,
James
^ permalink raw reply
* Re: [PATCH v4 2/4] Input: cs40l50 - Add cirrus haptics base support
From: James Ogletree @ 2023-11-01 20:46 UTC (permalink / raw)
To: Jeff LaBundy
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Lee Jones, Fred Treven, Ben Bright, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <ZTh3qSAjIaj/oonc@nixie71>
Hi Jeff,
You’ve given such great feedback covering many aspects of the
driver, on this patch and the whole series, which has required very
careful consideration, so thank you for your patience.
> On Oct 24, 2023, at 9:04 PM, Jeff LaBundy <jeff@labundy.com> wrote:
>
> On Wed, Oct 18, 2023 at 05:57:23PM +0000, James Ogletree wrote:
>>
>> +static int cs_hap_pseq_init(struct cs_hap *haptics)
>> +{
>> + struct cs_hap_pseq_op *op;
>> + int error, i, num_words;
>> + u8 operation;
>> + u32 *words;
>> +
>> + if (!haptics->dsp.pseq_size || !haptics->dsp.pseq_reg)
>> + return 0;
>> +
>> + INIT_LIST_HEAD(&haptics->pseq_head);
>
> Anything that allocates or initializes an element that is normally held
> in a driver's private data, like a list head or mutex, belongs in probe()
> in my opinion. It's less of an issue here, but for more complex cases
> where we may set something up in probe() and tear it down in remove(),
> the driver is easier to maintain if helper functions such as cs_hap_pseq_init()
> only manipulate or organize the data, rather than absorb additional work.
I agree with your reasoning, however, doesn’t this then turn on the question of
who the rightful owner of the write sequencer code is? If the pseq code
belongs in the MFD driver, it ought to be moved to the driver’s private data
structure there, however if it fits here, then not so. A counter example would
be cs_dsp.c, a library which contains within it some INIT_LIST_HEAD calls.
Perhaps the dust should settle on that dispute, and in regards to that, please
read my comments below.
>> +
>> + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL);
>> + if (!words)
>> + return -ENOMEM;
>> +
>> + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg,
>> + words, haptics->dsp.pseq_size);
>> + if (error)
>> + goto err_free;
>> +
>> + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) {
>> + operation = FIELD_GET(PSEQ_OP_MASK, words[i]);
>> + switch (operation) {
>> + case PSEQ_OP_END:
>> + case PSEQ_OP_WRITE_UNLOCK:
>> + num_words = PSEQ_OP_END_WORDS;
>> + break;
>> + case PSEQ_OP_WRITE_ADDR8:
>> + case PSEQ_OP_WRITE_H16:
>> + case PSEQ_OP_WRITE_L16:
>> + num_words = PSEQ_OP_WRITE_X16_WORDS;
>> + break;
>> + case PSEQ_OP_WRITE_FULL:
>> + num_words = PSEQ_OP_WRITE_FULL_WORDS;
>> + break;
>> + default:
>> + error = -EINVAL;
>> + dev_err(haptics->dev, "Unsupported op: %u\n", operation);
>> + goto err_free;
>> + }
>> +
>> + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL);
>> + if (!op) {
>> + error = -ENOMEM;
>> + goto err_free;
>> + }
>> +
>> + op->size = num_words * sizeof(u32);
>> + memcpy(op->words, &words[i], op->size);
>> + op->offset = i * sizeof(u32);
>> + op->operation = operation;
>> + list_add(&op->list, &haptics->pseq_head);
>> +
>> + if (operation == PSEQ_OP_END)
>> + break;
>> + }
>> +
>> + if (operation != PSEQ_OP_END)
>> + error = -ENOENT;
>> +
>> +err_free:
>> + kfree(words);
>> +
>> + return error;
>> +}
>
> My first thought as I reviewed this patch was that this and the lot
> of pseq-related functions are not necessarily related to haptics, but
> rather CS40L50 register access and housekeeping in general.
>
> I seem to recall on L25 and friends that the the power-on sequencer,
> i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory
> that can play back a series of address/data pairs when the device
> comes out of hibernation, and any registers written during runtime
> must also be mirrored to the PSEQ for "playback" later. Is that still
> the case here?
>
> Assuming so, these functions seem like they belong in the MFD, not
> an input-specific library, because they will presumably be used by
> the codec driver as well, since that driver will write registers to
> set BCLK/LRCK ratio, etc.
>
> Therefore, I think it makes more sense for these functions to move to
> the MFD, which can then export them for use by the input/FF and codec
> children.
I think the problem with moving the write sequencer code to the MFD
driver is that it would go unused in a codec-only environment. We only
need to write to the PSEQ when we want to maintain register settings
throughout hibernation cycles, and it isn’t possible to hibernate when
there is data streaming to the device. So the PSEQ will never be used
in the codec driver.
This leaves either the input driver or the library, and it makes more
sense to be in the library since it is shared code with L26. This was
my reasoning, let me know whether you think it is sound.
> This leaves cirrus_haptics.* with only a few functions related to
> starting and stopping work, which seem specific enough to just live
> in cs40l50-vibra.c. Presumably many of those could be re-used by
> the L30 down the road, but in that case I think we'd be looking to
> actually re-use the L50 driver and simply add a compatible string
> for L30.
>
> I recall L30 has some overhead that L50 does not, which may make
> it hairy for cs40l50-vibra.c to support both. But in that case,
> you could always fork a cs40l30-vibra.c with its own compatible
> string, then have the L50 MFD selectively load the correct child
> based on device ID. To summarize, we should have:
>
> * drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery,
> IRQ handling, exported PSEQ functions, etc.
> * sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from
> the MFD.
> * drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop
> work, also uses PSEQ library from the MFD.
>
> And down the road, depending on complexity, maybe we also have:
>
> * drivers/input/misc/cs40l30-vibra.c: another input/FF driver that
> includes other functionality that didn't really fit in cs40l50-vibra.c;
> also uses PSEQ library from, and is loaded by, the original L50 MFD.
> If this driver duplicates small bits of cs40l50-vibra.c, it's not the
> end of the world.
>
> All of these files would #include include/linux/mfd/cs40l50.h. And
> finally, cirrus_haptics.* simply go away. Same idea, just slightly
> more scalable, and closer to common design patterns.
I understand that it is a common design pattern to selectively load
devices from the MFD driver. If I could summarize my thoughts on why
that would not be fitting here, it’s that the L26 and L50 share a ton of
input FF related work, and not enough “MFD core” related work.
Between errata differences, power supply configurations, regmap
configurations, interrupt register differences, it would seem to make for
a very awkward, scrambled MFD driver. Moreover, I think I will be moving
firmware download to the MFD driver, and that alone constitutes a
significant incompatibility because firmware downloading is compulsory
on L26, not so on L50.
On the other hand, I want to emphasize the amount that L26 and
L50 share when it comes to the Input FF callbacks. The worker
functions in cirrus_haptics.c are bare-bones for this first
submission, but were designed to be totally generic and scalable to
the needs of L26 and all future Cirrus input drivers. While it might appear
too specific for L26, everything currently in cirrus_haptics is usable by
L26 as-is.
For the above reasons I favor the current approach.
>> +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr,
>> + u32 data, bool update, u8 op_code)
>> +{
>> + struct cs_hap_pseq_op *op, *op_end, *op_new;
>> + struct cs_dsp_chunk ch;
>> + u32 pseq_bytes;
>> + int error;
>> +
>> + op_new = devm_kzalloc(haptics->dev, sizeof(*op_new), GFP_KERNEL);
>> + if (!op_new)
>> + return -ENOMEM;
>> +
>> + op_new->operation = op_code;
>> +
>> + ch = cs_dsp_chunk((void *) op_new->words,
>> + PSEQ_OP_WRITE_FULL_WORDS * sizeof(u32));
>> + cs_dsp_chunk_write(&ch, 8, op_code);
>> + switch (op_code) {
>> + case PSEQ_OP_WRITE_FULL:
>> + cs_dsp_chunk_write(&ch, 32, addr);
>> + cs_dsp_chunk_write(&ch, 32, data);
>> + break;
>> + case PSEQ_OP_WRITE_L16:
>> + case PSEQ_OP_WRITE_H16:
>> + cs_dsp_chunk_write(&ch, 24, addr);
>> + cs_dsp_chunk_write(&ch, 16, data);
>> + break;
>> + default:
>> + error = -EINVAL;
>> + goto op_new_free;
>> + }
>> +
>> + op_new->size = cs_dsp_chunk_bytes(&ch);
>> +
>> + if (update) {
>> + op = cs_hap_pseq_find_op(op_new, &haptics->pseq_head);
>> + if (!op) {
>> + error = -EINVAL;
>> + goto op_new_free;
>> + }
>> + }
>
> It seems we are relying on the developer to remember if he or she has
> already written 'addr' using a previous call to cs_hap_pseq_write(),
> then set the update flag accordingly; is that accurate?
>
> If so, that is a high risk for bugs to be introduced as the driver is
> maintained. Can we not search for an existing address/data entry upon
> any call to cs_hap_pseq_write() using cs_hap_pseq_find_op(), and add
> or replace a new address/data entry automatically?
There are currently sequences on L26 where we want to write to the same
address twice. In such cases we wouldn't want the driver to automatically
look up and update the first entry during the second write call, hence the
need for the update flag. I think with this use case in mind, updating
the entries automatically wouldn't be feasible.
>> +static void cs_hap_vibe_stop_worker(struct work_struct *work)
>> +{
>> + struct cs_hap *haptics = container_of(work, struct cs_hap,
>> + vibe_stop_work);
>> + int error;
>> +
>> + if (haptics->runtime_pm) {
>> + error = pm_runtime_resume_and_get(haptics->dev);
>> + if (error < 0) {
>> + haptics->stop_error = error;
>> + return;
>> + }
>> + }
>> +
>> + mutex_lock(&haptics->lock);
>> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
>> + haptics->dsp.stop_cmd);
>> + mutex_unlock(&haptics->lock);
>> +
>> + if (haptics->runtime_pm) {
>> + pm_runtime_mark_last_busy(haptics->dev);
>> + pm_runtime_put_autosuspend(haptics->dev);
>> + }
>> +
>> + haptics->stop_error = error;
>
> This seems like another argument for not separating the input/FF child
> from the meat of the driver; it just seems messy to pass around error
> codes within a driver's private data like this.
I removed the start_error and stop_error members. However I think the
erase_error and add_error need to stay. I think this is more of a symptom
of the Input FF layer requiring error reporting and having to use workqueues
for those Input FF callbacks, than it is to do with the separation of these
functions from the driver. Point being, even if these were moved, we would still
need these *_error members. Let me know if I understood you right here.
Best,
James
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Jiri Kosina @ 2023-11-01 19:37 UTC (permalink / raw)
To: David Revoy
Cc: jason.gerecke@wacom.com, jose.exposito89@gmail.com,
ilya.ostapyshyn@gmail.com, Nils Fuhler, Ping Cheng,
Peter Hutterer, Benjamin Tissoires, linux-kernel, linux-input
In-Reply-To: <kRKTNDYigUSblpNgSuZ2H4dX03Of1yD4j_L9GgbyKXcDqZ67yh5HOQfcd7_83U3jZuQzxpKT3L6FXcRkkZIGdl_-PQF14oIB0QmRSfvpc2k=@protonmail.com>
On Wed, 1 Nov 2023, David Revoy wrote:
> Hi Jason Gerecke, José Expósito, Jiri Kosina and Illia Ostapyshyn,
>
> I am emailing to draw your attention and expertise to a problem I had
> earlier this week with my Xp-Pen Artist 24 Pro display tablet under
> Fedora Linux 38 KDE after a kernel update.
>
> The second button on my stylus changed from a right-click (which I could
> customise with xsetwacom or any GUI like kcm-tablet) to a button that
> feels 'hardcoded' and now switches the whole device to an eraser mode.
> This makes my main tool unusable.
>
> I don't have the skills to write a proper kernel bug report, workaround
> or even identify the exact source of the issue. I have written a blog
> post about this with more details here:
> https://www.davidrevoy.com/article995/how-a-kernel-update-broke-my-stylus-need-help
> , contacting you was something suggested by the comments.
>
> Thank you very much if you can help me.
CCing a couple more people involved both in 276e14e6c3 and 87562fcd1342,
and mailinglists.
This is almost certainly the behavior introduced by 276e14e6c3, where
previously the button was mapped to BTN_TOUCH, but now it's mapped to
BTN_TOOL_RUBBER, causing user-visible change in behavior.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Trackpoint and touchpad on Lenovo ThinkPad P15v Gen3 AMD
From: Giuseppe Bilotta @ 2023-11-01 17:46 UTC (permalink / raw)
To: linux-input
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2111 bytes --]
Hello,
I have recently acquired a Lenovo ThinkPad P15v Gen3
(CPU is an AMD Ryzen 7 PRO 6850H).
I'm running kernel 6.5.8-1 from Debian.
On loading the psmouse module, I get:
psmouse serio1: synaptics: queried max coordinates: x [..5678], y [..4694]
psmouse serio1: synaptics: queried min coordinates: x [1266..], y [1162..]
psmouse serio1: synaptics: Your touchpad (PNP: LEN2161 PNP0f13) says it can support a different bus. If i2c-hid and hid-rmi are not used, you might want to try setting psmouse.synaptics_intertouch to 1 and report this to linux-input@vger.kernel.org.
psmouse serio1: synaptics: Touchpad model: 1, fw: 10.32, id: 0x1e2a1, caps: 0xf014a3/0x940300/0x12e800/0x500000, board id: 3471, fw id: 3972349
psmouse serio1: synaptics: serio: Synaptics pass-through port at isa0060/serio1/input0
psmouse serio2: trackpoint: Elan TrackPoint firmware: 0x12, buttons: 3/3
Of note, if the psmouse module is NOT loaded, I cannot access the trackpoint or touchpad on the machine,
and when loading the psmouse module with the synaptics_intertouch=1 option, I get
psmouse serio1: synaptics: queried max coordinates: x [..5678], y [..4694]
psmouse serio1: synaptics: queried min coordinates: x [1266..], y [1162..]
psmouse serio1: synaptics: Trying to set up SMBus access
psmouse serio1: synaptics: SMbus companion is not ready yet
psmouse serio1: synaptics: Touchpad model: 1, fw: 10.32, id: 0x1e2a1, caps: 0xf014a3/0x940300/0x12e800/0x500000, board id: 3471, fw id: 3972349
psmouse serio1: synaptics: serio: Synaptics pass-through port at isa0060/serio1/input0
psmouse serio3: trackpoint: Elan TrackPoint firmware: 0x12, buttons: 3/3
The mouse seems to work correclty with either option.
(possibly related to the “SMbus companion not ready” message when intertouch is enabled?)
Loading the driver with synaptics_intertouch=0 suppresses the SMbus companion message
(expected, I presume).
Also unloading and reloading the psmouse driver (regardless of the intertouch setting)
increases by one the serio to which the trackpoint is attached,
but I assume this is normal.
Best regards,
Giuseppe Bilotta
^ permalink raw reply
* [PATCH] Input: ads7846 - silent spi_device_id warnings
From: Sean Nyekjaer @ 2023-11-01 15:09 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Sean Nyekjaer, linux-input, linux-kernel
Add spi_device_id entries to silent following warnings:
SPI driver ads7846 has no spi_device_id for ti,tsc2046
SPI driver ads7846 has no spi_device_id for ti,ads7843
SPI driver ads7846 has no spi_device_id for ti,ads7845
SPI driver ads7846 has no spi_device_id for ti,ads7873
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
drivers/input/touchscreen/ads7846.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index d2bbb436a77d..30758fc0e029 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -1377,6 +1377,16 @@ static void ads7846_remove(struct spi_device *spi)
ads7846_stop(ts);
}
+static const struct spi_device_id ads7846_spi_ids[] = {
+ { "tsc2046" },
+ { "ads7843" },
+ { "ads7845" },
+ { "ads7846" },
+ { "ads7873" },
+ { },
+};
+MODULE_DEVICE_TABLE(spi, ads7846_spi_ids);
+
static struct spi_driver ads7846_driver = {
.driver = {
.name = "ads7846",
@@ -1384,6 +1394,7 @@ static struct spi_driver ads7846_driver = {
.pm = pm_sleep_ptr(&ads7846_pm),
.of_match_table = ads7846_dt_ids,
},
+ .id_table = ads7846_spi_ids,
.probe = ads7846_probe,
.remove = ads7846_remove,
};
--
2.42.0
^ permalink raw reply related
* Re: [PATCH v3] Input: xen-kbdfront - drop keys to shrink modalias
From: Jason Andryuk @ 2023-11-01 14:11 UTC (permalink / raw)
To: linux-kernel, Dmitry Torokhov
Cc: Phillip Susi, stable, Mattijs Korpershoek, linux-input
In-Reply-To: <20231011193444.81254-1-jandryuk@gmail.com>
Hi Dmitry,
Do you have any feedback or can you pick this up?
Thanks,
Jason
On Wed, Oct 11, 2023 at 3:34 PM Jason Andryuk <jandryuk@gmail.com> wrote:
>
> xen kbdfront registers itself as being able to deliver *any* key since
> it doesn't know what keys the backend may produce.
>
> Unfortunately, the generated modalias gets too large and uevent creation
> fails with -ENOMEM.
>
> This can lead to gdm not using the keyboard since there is no seat
> associated [1] and the debian installer crashing [2].
>
> Trim the ranges of key capabilities by removing some BTN_* ranges.
> While doing this, some neighboring undefined ranges are removed to trim
> it further.
>
> An upper limit of KEY_KBD_LCD_MENU5 is still too large. Use an upper
> limit of KEY_BRIGHTNESS_MENU.
>
> This removes:
> BTN_DPAD_UP(0x220)..BTN_DPAD_RIGHT(0x223)
> Empty space 0x224..0x229
>
> Empty space 0x28a..0x28f
> KEY_MACRO1(0x290)..KEY_MACRO30(0x2ad)
> KEY_MACRO_RECORD_START 0x2b0
> KEY_MACRO_RECORD_STOP 0x2b1
> KEY_MACRO_PRESET_CYCLE 0x2b2
> KEY_MACRO_PRESET1(0x2b3)..KEY_MACRO_PRESET3(0xb5)
> Empty space 0x2b6..0x2b7
> KEY_KBD_LCD_MENU1(0x2b8)..KEY_KBD_LCD_MENU5(0x2bc)
> Empty space 0x2bd..0x2bf
> BTN_TRIGGER_HAPPY(0x2c0)..BTN_TRIGGER_HAPPY40(0x2e7)
> Empty space 0x2e8..0x2ff
>
> The modalias shrinks from 2082 to 1550 bytes.
>
> A chunk of keys need to be removed to allow the keyboard to be used.
> This may break some functionality, but the hope is these macro keys are
> uncommon and don't affect any users.
>
> [1] https://github.com/systemd/systemd/issues/22944
> [2] https://lore.kernel.org/xen-devel/87o8dw52jc.fsf@vps.thesusis.net/T/
>
> Cc: Phillip Susi <phill@thesusis.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
> v3:
> Add Mattijs R-b
> Put /* and */ on separate lines
> ---
> drivers/input/misc/xen-kbdfront.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
> index 67f1c7364c95..d59ba8f9852e 100644
> --- a/drivers/input/misc/xen-kbdfront.c
> +++ b/drivers/input/misc/xen-kbdfront.c
> @@ -256,7 +256,16 @@ static int xenkbd_probe(struct xenbus_device *dev,
> __set_bit(EV_KEY, kbd->evbit);
> for (i = KEY_ESC; i < KEY_UNKNOWN; i++)
> __set_bit(i, kbd->keybit);
> - for (i = KEY_OK; i < KEY_MAX; i++)
> + /*
> + * In theory we want to go KEY_OK..KEY_MAX, but that grows the
> + * modalias line too long. There is a gap of buttons from
> + * BTN_DPAD_UP..BTN_DPAD_RIGHT and KEY_ALS_TOGGLE is the next
> + * defined. Then continue up to KEY_BRIGHTNESS_MENU as an upper
> + * limit.
> + */
> + for (i = KEY_OK; i < BTN_DPAD_UP; i++)
> + __set_bit(i, kbd->keybit);
> + for (i = KEY_ALS_TOGGLE; i <= KEY_BRIGHTNESS_MENU; i++)
> __set_bit(i, kbd->keybit);
>
> ret = input_register_device(kbd);
> --
> 2.41.0
>
^ permalink raw reply
* [PATCH] HID: sony: Remove usage of the deprecated ida_simple_xx() API
From: Christophe JAILLET @ 2023-11-01 9:38 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-kernel, kernel-janitors, Christophe JAILLET, linux-input
ida_alloc() and ida_free() should be preferred to the deprecated
ida_simple_get() and ida_simple_remove().
This is less verbose.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
drivers/hid/hid-sony.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index ebc0aa4e4345..55c0ad61d524 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1844,8 +1844,7 @@ static int sony_set_device_id(struct sony_sc *sc)
* All others are set to -1.
*/
if (sc->quirks & SIXAXIS_CONTROLLER) {
- ret = ida_simple_get(&sony_device_id_allocator, 0, 0,
- GFP_KERNEL);
+ ret = ida_alloc(&sony_device_id_allocator, GFP_KERNEL);
if (ret < 0) {
sc->device_id = -1;
return ret;
@@ -1861,7 +1860,7 @@ static int sony_set_device_id(struct sony_sc *sc)
static void sony_release_device_id(struct sony_sc *sc)
{
if (sc->device_id >= 0) {
- ida_simple_remove(&sony_device_id_allocator, sc->device_id);
+ ida_free(&sony_device_id_allocator, sc->device_id);
sc->device_id = -1;
}
}
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v6 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Thomas Weißschuh @ 2023-11-01 8:45 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jeff LaBundy, Shuah Khan,
linux-kernel-mentees, linux-kernel
In-Reply-To: <152d15c4-10bb-45fe-9b9c-b323535a921f@gmail.com>
Hi Anshul,
On 2023-11-01 09:50:36+0530, Anshul Dalal wrote:
> On 10/31/23 07:53, Thomas Weißschuh wrote:
> > Oct 31, 2023 03:10:50 Anshul Dalal <anshulusr@gmail.com>:
> >> Thanks for the review! The requested changes will be addressed in the
> >> next patch version though I had a few comments below:
> >>
> >> On 10/27/23 11:44, Thomas Weißschuh wrote:
> >>> Hi Anshul,
> >>>
> >>> thanks for the reworks!
> >>>
> >>> Some more comments inline.
> >>>
> >>> On 2023-10-27 10:48:11+0530, Anshul Dalal wrote:
> >
> > [..]
> >
> >>>> +struct seesaw_button_description {
> >>>> + unsigned int code;
> >>>> + unsigned int bit;
> >>>> +};
> >>>> +
> >>>> +static const struct seesaw_button_description seesaw_buttons[] = {
> >>>> + {
> >>>> + .code = BTN_EAST,
> >>>> + .bit = SEESAW_BUTTON_A,
> >>>> + },
> >>>> + {
> >>>> + .code = BTN_SOUTH,
> >>>> + .bit = SEESAW_BUTTON_B,
> >>>> + },
> >>>> + {
> >>>> + .code = BTN_NORTH,
> >>>> + .bit = SEESAW_BUTTON_X,
> >>>> + },
> >>>> + {
> >>>> + .code = BTN_WEST,
> >>>> + .bit = SEESAW_BUTTON_Y,
> >>>> + },
> >>>> + {
> >>>> + .code = BTN_START,
> >>>> + .bit = SEESAW_BUTTON_START,
> >>>> + },
> >>>> + {
> >>>> + .code = BTN_SELECT,
> >>>> + .bit = SEESAW_BUTTON_SELECT,
> >>>> + },
> >>>> +};
> >>>
> >>> This looks very much like a sparse keymap which can be implemented with
> >>> the helpers from <linux/input/sparse-keymap.h>.
> >>>
> >>
> >> When going through the API provided by sparse-keymap, I could only see
> >> the use for sparse_keymap_report_entry here. Which leads to the
> >> following refactored code:
> >>
> >> static const struct key_entry seesaw_buttons_new[] = {
> >> {KE_KEY, SEESAW_BUTTON_A, {BTN_SOUTH}},
> >> {KE_KEY, SEESAW_BUTTON_B, {BTN_EAST}},
> >
> > No braces I think.
> >
>
> Since the last field in key_entry is a union, the braces seem to be
> required.
Indeed.
To make the union more visible explicit this could be done:
{ KE_KEY, SEESAW_BUTTON_A, .keycode = BTN_SOUTH }
>
> >> ...
> >> };
> >>
> >> for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) {
> >> sparse_keymap_report_entry(input, &seesaw_buttons_new[i],
> >> data.button_state & BIT(seesaw_buttons_new[i].code),
> >> false);
> >> }
> >>
> >> I don't think this significantly improves the code unless you had some
> >> other way to use the API in mind.
> >
> > I thought about sparse_keymap_setup() and sparse_keymap_report_event().
> >
> > It does not significantly change the code but would be a standard API.
> >
>
> Thanks for pointing me in the right direction, do you think the
> following implementation of the API is acceptable for the driver. Since
> I couldn't find a driver for any similar device using the API in this
> manner.
>
> inside seesaw_probe():
>
> err = sparse_keymap_setup(seesaw->input_dev, seesaw_buttons_new, NULL);
> if (err) {
> dev_err(&client->dev,
> "failed to set up input device keymap: %d\n", err);
> return err;
> }
Yes, and it replaces the calls to input_set_capability().
> inside seesaw_poll():
>
> for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) {
> if (!sparse_keymap_report_event(
> input, seesaw_buttons_new[i].code,
> data.button_state & BIT(seesaw_buttons_new[i].code),
> false)) {
> dev_err_ratelimited(
> &input->dev,
> "failed to report event for keycode: %d",
> seesaw_buttons_new[i].keycode);
> return;
> }
> }
for_each_set_bit(i, (long *)&SEESAW_BUTTON_MASK, BITS_PER_TYPE(SEESAW_BUTTON_MASK))
sparse_keymap_report_event(input, BIT(i), data.button_state & BIT(i), false);
The sparse keymap takes care of the translation.
Notes:
SEESAW_BUTTON_MASK is now an actual variable instead of a macro.
It should be 'static const' in that case.
When using the sparse keymap APIs the driver also needs to depend on
INPUT_SPARSEKMAP.
Thomas
^ permalink raw reply
* Re: [PATCH v6 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-11-01 4:20 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Thomas Weißschuh, linux-input, devicetree, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff LaBundy,
Shuah Khan, linux-kernel-mentees, linux-kernel
In-Reply-To: <9c9f6171-f879-46f5-81d2-6764257a49eb@t-8ch.de>
On 10/31/23 07:53, Thomas Weißschuh wrote:
> Oct 31, 2023 03:10:50 Anshul Dalal <anshulusr@gmail.com>:
>
>> Hello Thomas,
>>
>> Thanks for the review! The requested changes will be addressed in the
>> next patch version though I had a few comments below:
>>
>> On 10/27/23 11:44, Thomas Weißschuh wrote:
>>> Hi Anshul,
>>>
>>> thanks for the reworks!
>>>
>>> Some more comments inline.
>>>
>>> On 2023-10-27 10:48:11+0530, Anshul Dalal wrote:
>
> [..]
>
>>>> +struct seesaw_button_description {
>>>> + unsigned int code;
>>>> + unsigned int bit;
>>>> +};
>>>> +
>>>> +static const struct seesaw_button_description seesaw_buttons[] = {
>>>> + {
>>>> + .code = BTN_EAST,
>>>> + .bit = SEESAW_BUTTON_A,
>>>> + },
>>>> + {
>>>> + .code = BTN_SOUTH,
>>>> + .bit = SEESAW_BUTTON_B,
>>>> + },
>>>> + {
>>>> + .code = BTN_NORTH,
>>>> + .bit = SEESAW_BUTTON_X,
>>>> + },
>>>> + {
>>>> + .code = BTN_WEST,
>>>> + .bit = SEESAW_BUTTON_Y,
>>>> + },
>>>> + {
>>>> + .code = BTN_START,
>>>> + .bit = SEESAW_BUTTON_START,
>>>> + },
>>>> + {
>>>> + .code = BTN_SELECT,
>>>> + .bit = SEESAW_BUTTON_SELECT,
>>>> + },
>>>> +};
>>>
>>> This looks very much like a sparse keymap which can be implemented with
>>> the helpers from <linux/input/sparse-keymap.h>.
>>>
>>
>> When going through the API provided by sparse-keymap, I could only see
>> the use for sparse_keymap_report_entry here. Which leads to the
>> following refactored code:
>>
>> static const struct key_entry seesaw_buttons_new[] = {
>> {KE_KEY, SEESAW_BUTTON_A, {BTN_SOUTH}},
>> {KE_KEY, SEESAW_BUTTON_B, {BTN_EAST}},
>
> No braces I think.
>
Since the last field in key_entry is a union, the braces seem to be
required.
>> ...
>> };
>>
>> for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) {
>> sparse_keymap_report_entry(input, &seesaw_buttons_new[i],
>> data.button_state & BIT(seesaw_buttons_new[i].code),
>> false);
>> }
>>
>> I don't think this significantly improves the code unless you had some
>> other way to use the API in mind.
>
> I thought about sparse_keymap_setup() and sparse_keymap_report_event().
>
> It does not significantly change the code but would be a standard API.
>
Thanks for pointing me in the right direction, do you think the
following implementation of the API is acceptable for the driver. Since
I couldn't find a driver for any similar device using the API in this
manner.
inside seesaw_probe():
err = sparse_keymap_setup(seesaw->input_dev, seesaw_buttons_new, NULL);
if (err) {
dev_err(&client->dev,
"failed to set up input device keymap: %d\n", err);
return err;
}
inside seesaw_poll():
for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) {
if (!sparse_keymap_report_event(
input, seesaw_buttons_new[i].code,
data.button_state & BIT(seesaw_buttons_new[i].code),
false)) {
dev_err_ratelimited(
&input->dev,
"failed to report event for keycode: %d",
seesaw_buttons_new[i].keycode);
return;
}
}
> Thomas
Best regards,
Anshul
^ permalink raw reply
* Re: [PATCH] Input: synaptics: enable InterTouch for ThinkPad L14 G1
From: José Pekkarinen @ 2023-10-31 12:39 UTC (permalink / raw)
To: dmitry.torokhov, rydberg, skhan
Cc: rrangel, amandhoot12, linux-kernel, linux-kernel-mentees,
linux-input
In-Reply-To: <5d857ff972e9203ef65ae2396c7285c0@foxhound.fi>
On 2023-10-16 18:51, José Pekkarinen wrote:
> On 2023-10-08 11:01, José Pekkarinen wrote:
>> Observed on dmesg of my laptop I see the following
>> output:
>>
>> [ 19.898700] psmouse serio1: synaptics: queried max coordinates: x
>> [..5678], y [..4694]
>> [ 19.936057] psmouse serio1: synaptics: queried min coordinates: x
>> [1266..], y [1162..]
>> [ 19.936076] psmouse serio1: synaptics: Your touchpad (PNP: LEN0411
>> PNP0f13) says it can support a different bus. If i2c-hid and hid-rmi
>> are not used, you might want to try setting
>> psmouse.synaptics_intertouch to 1 and report this to
>> linux-input@vger.kernel.org.
>> [ 20.008901] psmouse serio1: synaptics: Touchpad model: 1, fw:
>> 10.32, id: 0x1e2a1, caps: 0xf014a3/0x940300/0x12e800/0x500000, board
>> id: 3471, fw id: 2909640
>> [ 20.008925] psmouse serio1: synaptics: serio: Synaptics
>> pass-through port at isa0060/serio1/input0
>> [ 20.053344] input: SynPS/2 Synaptics TouchPad as
>> /devices/platform/i8042/serio1/input/input7
>> [ 20.397608] mousedev: PS/2 mouse device common for all mice
>>
>> This patch will add its pnp id to the smbus list to
>> produce the setup of intertouch for the device. After
>> applying, the ouput will look like:
>>
>> [ 19.168664] psmouse serio1: synaptics: queried max coordinates: x
>> [..5678], y [..4694]
>> [ 19.206311] psmouse serio1: synaptics: queried min coordinates: x
>> [1266..], y [1162..]
>> [ 19.206325] psmouse serio1: synaptics: Trying to set up SMBus
>> access
>> [ 19.209545] psmouse serio1: synaptics: SMbus companion is not ready
>> yet
>> [ 19.283845] psmouse serio1: synaptics: Touchpad model: 1, fw:
>> 10.32, id: 0x1e2a1, caps: 0xf014a3/0x940300/0x12e800/0x500000, board
>> id: 3471, fw id: 2909640
>> [ 19.283863] psmouse serio1: synaptics: serio: Synaptics
>> pass-through port at isa0060/serio1/input0
>> [ 19.328959] input: SynPS/2 Synaptics TouchPad as
>> /devices/platform/i8042/serio1/input/input8
>> [ 19.706164] mousedev: PS/2 mouse device common for all mice
>>
>> Signed-off-by: José Pekkarinen <jose.pekkarinen@foxhound.fi>
>> ---
>> drivers/input/mouse/synaptics.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/input/mouse/synaptics.c
>> b/drivers/input/mouse/synaptics.c
>> index ada299ec5bba..376a041c80b2 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -183,6 +183,7 @@ static const char * const smbus_pnp_ids[] = {
>> "LEN009b", /* T580 */
>> "LEN0402", /* X1 Extreme Gen 2 / P1 Gen 2 */
>> "LEN040f", /* P1 Gen 3 */
>> + "LEN0411", /* L14 Gen 1 */
>> "LEN200f", /* T450s */
>> "LEN2044", /* L470 */
>> "LEN2054", /* E480 */
>
> Any comments here?
>
> Thanks!
>
> José.
> _______________________________________________
> Linux-kernel-mentees mailing list
> Linux-kernel-mentees@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Ping.
José.
^ permalink raw reply
* [PATCH v2] HID: fix a crash in hid_debug_events_release
From: Charles Yi @ 2023-10-31 4:32 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Charles Yi
hid_debug_events_release() access released memory by
hid_device_release(). This is fixed by the patch.
When hid_debug_events_release() was being called, in most case,
hid_device_release() finish already, the memory of list->hdev
freed by hid_device_release(), if list->hdev memory
reallocate by others, and it's modified, zeroed, then
list->hdev->debug_list_lock occasioned crash come out.
The crash:
[ 120.728477][ T4396] kernel BUG at lib/list_debug.c:53!
[ 120.728505][ T4396] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[ 120.739806][ T4396] Modules linked in: bcmdhd dhd_static_buf 8822cu pcie_mhi r8168
[ 120.747386][ T4396] CPU: 1 PID: 4396 Comm: hidt_bridge Not tainted 5.10.110 #257
[ 120.754771][ T4396] Hardware name: Rockchip RK3588 EVB4 LP4 V10 Board (DT)
[ 120.761643][ T4396] pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
[ 120.768338][ T4396] pc : __list_del_entry_valid+0x98/0xac
[ 120.773730][ T4396] lr : __list_del_entry_valid+0x98/0xac
[ 120.779120][ T4396] sp : ffffffc01e62bb60
[ 120.783126][ T4396] x29: ffffffc01e62bb60 x28: ffffff818ce3a200
[ 120.789126][ T4396] x27: 0000000000000009 x26: 0000000000980000
[ 120.795126][ T4396] x25: ffffffc012431000 x24: ffffff802c6d4e00
[ 120.801125][ T4396] x23: ffffff8005c66f00 x22: ffffffc01183b5b8
[ 120.807125][ T4396] x21: ffffff819df2f100 x20: 0000000000000000
[ 120.813124][ T4396] x19: ffffff802c3f0700 x18: ffffffc01d2cd058
[ 120.819124][ T4396] x17: 0000000000000000 x16: 0000000000000000
[ 120.825124][ T4396] x15: 0000000000000004 x14: 0000000000003fff
[ 120.831123][ T4396] x13: ffffffc012085588 x12: 0000000000000003
[ 120.837123][ T4396] x11: 00000000ffffbfff x10: 0000000000000003
[ 120.843123][ T4396] x9 : 455103d46b329300 x8 : 455103d46b329300
[ 120.849124][ T4396] x7 : 74707572726f6320 x6 : ffffffc0124b8cb5
[ 120.855124][ T4396] x5 : ffffffffffffffff x4 : 0000000000000000
[ 120.861123][ T4396] x3 : ffffffc011cf4f90 x2 : ffffff81fee7b948
[ 120.867122][ T4396] x1 : ffffffc011cf4f90 x0 : 0000000000000054
[ 120.873122][ T4396] Call trace:
[ 120.876259][ T4396] __list_del_entry_valid+0x98/0xac
[ 120.881304][ T4396] hid_debug_events_release+0x48/0x12c
[ 120.886617][ T4396] full_proxy_release+0x50/0xbc
[ 120.891323][ T4396] __fput+0xdc/0x238
[ 120.895075][ T4396] ____fput+0x14/0x24
[ 120.898911][ T4396] task_work_run+0x90/0x148
[ 120.903268][ T4396] do_exit+0x1bc/0x8a4
[ 120.907193][ T4396] do_group_exit+0x8c/0xa4
[ 120.911458][ T4396] get_signal+0x468/0x744
[ 120.915643][ T4396] do_signal+0x84/0x280
[ 120.919650][ T4396] do_notify_resume+0xd0/0x218
[ 120.924262][ T4396] work_pending+0xc/0x3f0
Fixes: <cd667ce24796> (HID: use debugfs for events/reports dumping)
Signed-off-by: Charles Yi <be286@163.com>
---
Changes in V2:
- Add "Fixes:" tag and call trace to commit message.
---
drivers/hid/hid-core.c | 12 ++++++++++--
drivers/hid/hid-debug.c | 3 +++
include/linux/hid.h | 3 +++
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 8992e3c1e769..e0181218ad85 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -702,15 +702,22 @@ static void hid_close_report(struct hid_device *device)
* Free a device structure, all reports, and all fields.
*/
-static void hid_device_release(struct device *dev)
+void hiddev_free(struct kref *ref)
{
- struct hid_device *hid = to_hid_device(dev);
+ struct hid_device *hid = container_of(ref, struct hid_device, ref);
hid_close_report(hid);
kfree(hid->dev_rdesc);
kfree(hid);
}
+static void hid_device_release(struct device *dev)
+{
+ struct hid_device *hid = to_hid_device(dev);
+
+ kref_put(&hid->ref, hiddev_free);
+}
+
/*
* Fetch a report description item from the data stream. We support long
* items, though they are not used yet.
@@ -2846,6 +2853,7 @@ struct hid_device *hid_allocate_device(void)
spin_lock_init(&hdev->debug_list_lock);
sema_init(&hdev->driver_input_lock, 1);
mutex_init(&hdev->ll_open_lock);
+ kref_init(&hdev->ref);
hid_bpf_device_init(hdev);
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index e7ef1ea107c9..7dd83ec74f8a 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1135,6 +1135,7 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
goto out;
}
list->hdev = (struct hid_device *) inode->i_private;
+ kref_get(&list->hdev->ref);
file->private_data = list;
mutex_init(&list->read_mutex);
@@ -1227,6 +1228,8 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
list_del(&list->node);
spin_unlock_irqrestore(&list->hdev->debug_list_lock, flags);
kfifo_free(&list->hid_debug_fifo);
+
+ kref_put(&list->hdev->ref, hiddev_free);
kfree(list);
return 0;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 964ca1f15e3f..3b08a2957229 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -679,6 +679,7 @@ struct hid_device { /* device report descriptor */
struct list_head debug_list;
spinlock_t debug_list_lock;
wait_queue_head_t debug_wait;
+ struct kref ref;
unsigned int id; /* system unique id */
@@ -687,6 +688,8 @@ struct hid_device { /* device report descriptor */
#endif /* CONFIG_BPF */
};
+void hiddev_free(struct kref *ref);
+
#define to_hid_device(pdev) \
container_of(pdev, struct hid_device, dev)
--
2.34.1
^ permalink raw reply related
* [dtor-input:next] BUILD SUCCESS 28d3fe32354701decc3e76d89712569c269b5e4f
From: kernel test robot @ 2023-10-31 3:06 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
branch HEAD: 28d3fe32354701decc3e76d89712569c269b5e4f Input: walkera0701 - use module_parport_driver macro to simplify the code
elapsed time: 2787m
configs tested: 138
configs skipped: 2
The following configs have been built successfully.
More configs may be tested in the coming days.
tested configs:
alpha allnoconfig gcc
alpha allyesconfig gcc
alpha defconfig gcc
arc allmodconfig gcc
arc allnoconfig gcc
arc allyesconfig gcc
arc defconfig gcc
arc randconfig-001-20231029 gcc
arm allmodconfig gcc
arm allnoconfig gcc
arm allyesconfig gcc
arm defconfig gcc
arm randconfig-001-20231029 gcc
arm64 allmodconfig gcc
arm64 allnoconfig gcc
arm64 allyesconfig gcc
arm64 defconfig gcc
csky allmodconfig gcc
csky allnoconfig gcc
csky allyesconfig gcc
csky defconfig gcc
i386 allmodconfig gcc
i386 allnoconfig gcc
i386 allyesconfig gcc
i386 buildonly-randconfig-001-20231029 gcc
i386 buildonly-randconfig-002-20231029 gcc
i386 buildonly-randconfig-003-20231029 gcc
i386 buildonly-randconfig-004-20231029 gcc
i386 buildonly-randconfig-005-20231029 gcc
i386 buildonly-randconfig-006-20231029 gcc
i386 debian-10.3 gcc
i386 defconfig gcc
i386 randconfig-001-20231029 gcc
i386 randconfig-002-20231029 gcc
i386 randconfig-003-20231029 gcc
i386 randconfig-004-20231029 gcc
i386 randconfig-005-20231029 gcc
i386 randconfig-006-20231029 gcc
i386 randconfig-011-20231029 gcc
i386 randconfig-012-20231029 gcc
i386 randconfig-013-20231029 gcc
i386 randconfig-014-20231029 gcc
i386 randconfig-015-20231029 gcc
i386 randconfig-016-20231029 gcc
loongarch allmodconfig gcc
loongarch allnoconfig gcc
loongarch allyesconfig gcc
loongarch defconfig gcc
loongarch randconfig-001-20231029 gcc
m68k allmodconfig gcc
m68k allnoconfig gcc
m68k allyesconfig gcc
m68k defconfig gcc
m68k m5307c3_defconfig gcc
microblaze allmodconfig gcc
microblaze allnoconfig gcc
microblaze allyesconfig gcc
microblaze defconfig gcc
mips allmodconfig gcc
mips allnoconfig gcc
mips allyesconfig gcc
mips omega2p_defconfig clang
nios2 allmodconfig gcc
nios2 allnoconfig gcc
nios2 allyesconfig gcc
nios2 defconfig gcc
openrisc allmodconfig gcc
openrisc allnoconfig gcc
openrisc allyesconfig gcc
openrisc defconfig gcc
parisc allmodconfig gcc
parisc allnoconfig gcc
parisc allyesconfig gcc
parisc defconfig gcc
parisc64 defconfig gcc
powerpc allmodconfig gcc
powerpc allnoconfig gcc
powerpc allyesconfig gcc
powerpc klondike_defconfig gcc
riscv allmodconfig gcc
riscv allnoconfig gcc
riscv allyesconfig gcc
riscv defconfig gcc
riscv randconfig-001-20231029 gcc
riscv rv32_defconfig gcc
s390 allmodconfig gcc
s390 allnoconfig gcc
s390 allyesconfig gcc
s390 defconfig gcc
s390 randconfig-001-20231029 gcc
sh allmodconfig gcc
sh allnoconfig gcc
sh allyesconfig gcc
sh defconfig gcc
sparc allmodconfig gcc
sparc allnoconfig gcc
sparc allyesconfig gcc
sparc defconfig gcc
sparc randconfig-001-20231029 gcc
sparc sparc32_defconfig gcc
sparc64 allmodconfig gcc
sparc64 allyesconfig gcc
sparc64 defconfig gcc
um allmodconfig clang
um allnoconfig clang
um allyesconfig clang
um defconfig gcc
um i386_defconfig gcc
um x86_64_defconfig gcc
x86_64 allnoconfig gcc
x86_64 allyesconfig gcc
x86_64 buildonly-randconfig-001-20231029 gcc
x86_64 buildonly-randconfig-002-20231029 gcc
x86_64 buildonly-randconfig-003-20231029 gcc
x86_64 buildonly-randconfig-004-20231029 gcc
x86_64 buildonly-randconfig-005-20231029 gcc
x86_64 buildonly-randconfig-006-20231029 gcc
x86_64 defconfig gcc
x86_64 randconfig-001-20231029 gcc
x86_64 randconfig-002-20231029 gcc
x86_64 randconfig-003-20231029 gcc
x86_64 randconfig-004-20231029 gcc
x86_64 randconfig-005-20231029 gcc
x86_64 randconfig-006-20231029 gcc
x86_64 randconfig-011-20231029 gcc
x86_64 randconfig-012-20231029 gcc
x86_64 randconfig-013-20231029 gcc
x86_64 randconfig-014-20231029 gcc
x86_64 randconfig-015-20231029 gcc
x86_64 randconfig-016-20231029 gcc
x86_64 randconfig-071-20231029 gcc
x86_64 randconfig-072-20231029 gcc
x86_64 randconfig-073-20231029 gcc
x86_64 randconfig-074-20231029 gcc
x86_64 randconfig-075-20231029 gcc
x86_64 randconfig-076-20231029 gcc
x86_64 rhel-8.3-rust clang
x86_64 rhel-8.3 gcc
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v6 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Thomas Weißschuh @ 2023-10-31 2:23 UTC (permalink / raw)
To: Anshul Dalal
Cc: Thomas Weißschuh, linux-input, devicetree, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jeff LaBundy,
Shuah Khan, linux-kernel-mentees, linux-kernel
In-Reply-To: <efea5ae2-7e41-4b78-a283-1f907be560b0@gmail.com>
Oct 31, 2023 03:10:50 Anshul Dalal <anshulusr@gmail.com>:
> Hello Thomas,
>
> Thanks for the review! The requested changes will be addressed in the
> next patch version though I had a few comments below:
>
> On 10/27/23 11:44, Thomas Weißschuh wrote:
>> Hi Anshul,
>>
>> thanks for the reworks!
>>
>> Some more comments inline.
>>
>> On 2023-10-27 10:48:11+0530, Anshul Dalal wrote:
[..]
>>> +struct seesaw_button_description {
>>> + unsigned int code;
>>> + unsigned int bit;
>>> +};
>>> +
>>> +static const struct seesaw_button_description seesaw_buttons[] = {
>>> + {
>>> + .code = BTN_EAST,
>>> + .bit = SEESAW_BUTTON_A,
>>> + },
>>> + {
>>> + .code = BTN_SOUTH,
>>> + .bit = SEESAW_BUTTON_B,
>>> + },
>>> + {
>>> + .code = BTN_NORTH,
>>> + .bit = SEESAW_BUTTON_X,
>>> + },
>>> + {
>>> + .code = BTN_WEST,
>>> + .bit = SEESAW_BUTTON_Y,
>>> + },
>>> + {
>>> + .code = BTN_START,
>>> + .bit = SEESAW_BUTTON_START,
>>> + },
>>> + {
>>> + .code = BTN_SELECT,
>>> + .bit = SEESAW_BUTTON_SELECT,
>>> + },
>>> +};
>>
>> This looks very much like a sparse keymap which can be implemented with
>> the helpers from <linux/input/sparse-keymap.h>.
>>
>
> When going through the API provided by sparse-keymap, I could only see
> the use for sparse_keymap_report_entry here. Which leads to the
> following refactored code:
>
> static const struct key_entry seesaw_buttons_new[] = {
> {KE_KEY, SEESAW_BUTTON_A, {BTN_SOUTH}},
> {KE_KEY, SEESAW_BUTTON_B, {BTN_EAST}},
No braces I think.
> ...
> };
>
> for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) {
> sparse_keymap_report_entry(input, &seesaw_buttons_new[i],
> data.button_state & BIT(seesaw_buttons_new[i].code),
> false);
> }
>
> I don't think this significantly improves the code unless you had some
> other way to use the API in mind.
I thought about sparse_keymap_setup() and sparse_keymap_report_event().
It does not significantly change the code but would be a standard API.
Thomas
^ permalink raw reply
* Re: [PATCH v6 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-10-31 2:09 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: linux-input, devicetree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jeff LaBundy, Shuah Khan,
linux-kernel-mentees, linux-kernel
In-Reply-To: <d1dd2142-546f-42b7-8966-ab75fd4f8817@t-8ch.de>
Hello Thomas,
Thanks for the review! The requested changes will be addressed in the
next patch version though I had a few comments below:
On 10/27/23 11:44, Thomas Weißschuh wrote:
> Hi Anshul,
>
> thanks for the reworks!
>
> Some more comments inline.
>
> On 2023-10-27 10:48:11+0530, Anshul Dalal wrote:
>> Adds a driver for a mini gamepad that communicates over i2c, the gamepad
>> has bidirectional thumb stick input and six buttons.
>>
>> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
>> to transmit the ADC data for the joystick and digital pin state for the
>> buttons. I have only implemented the functionality required to receive the
>> thumb stick and button state.
>>
>> Steps in reading the gamepad state over i2c:
>> 1. Reset the registers
>> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
>> `BUTTON_MASK`: A bit-map for the six digital pins internally
>> connected to the joystick buttons.
>> 3. Enable internal pullup resistors for the `BUTTON_MASK`
>> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
>> 5. Poll the device for button and joystick state done by:
>> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
>>
>> Product page:
>> https://www.adafruit.com/product/5743
>> Arduino driver:
>> https://github.com/adafruit/Adafruit_Seesaw
>>
>> Driver tested on RPi Zero 2W
>>
>> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
>> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
>> ---
>> Changes for v6:
>> - Added TODO
>> - Removed `clang-format` directives
>> - Namespaced device buttons
>> - Removed `char physical_path[32]` field from `struct seesaw_gamepad`
>> - Added `packed` attribute to `struct seesaw_data`
>> - Moved from having booleans per button to single `u32 button_state`
>> - Added `seesaw_button_description` array to directly associate device
>> buttons with respective keycodes
>> - Added wrapper functions `seesaw_register_` around `i2c_master_` API
>> - Ratelimited input error reporting with `dev_err_ratelimited`
>> - Removed `of_device_id`
>>
>> Changes for v5:
>> - Added link to the datasheet
>> - Added debug log message when `seesaw_read_data` fails
>>
>> Changes for v4:
>> - Changed `1UL << BUTTON_` to BIT(BUTTON_)
>> - Removed `hardware_id` field from `struct seesaw_gamepad`
>> - Removed redundant checks for the number of bytes written and received by
>> `i2c_master_send` and `i2c_master_recv`
>> - Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
>> - Changed `result & (1UL << BUTTON_)` to
>> `test_bit(BUTTON_, (long *)&result)`
>> - Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
>> - Fixed formatting issues
>> - Changed button reporting:
>> Since the gamepad had the action buttons in a non-standard layout:
>> (X)
>> (Y) (A)
>> (B)
>> Therefore moved to using generic directional action button event codes
>> instead of BTN_[ABXY].
>>
>> Changes for v3:
>> - no updates
>>
>> Changes for v2:
>> adafruit-seesaw.c:
>> - Renamed file from 'adafruit_seesaw.c'
>> - Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
>> - Changed count parameter for receiving joystick x on line 118:
>> `2` to `sizeof(write_buf)`
>> - Fixed invalid buffer size on line 123 and 126:
>> `data->y` to `sizeof(data->y)`
>> - Added comment for the `mdelay(10)` on line 169
>> - Changed inconsistent indentation on line 271
>> Kconfig:
>> - Fixed indentation for the help text
>> - Updated module name
>> Makefile:
>> - Updated module object file name
>> MAINTAINERS:
>> - Updated file name for the driver and bindings
>>
>> MAINTAINERS | 7 +
>> drivers/input/joystick/Kconfig | 9 +
>> drivers/input/joystick/Makefile | 1 +
>> drivers/input/joystick/adafruit-seesaw.c | 310 +++++++++++++++++++++++
>> 4 files changed, 327 insertions(+)
>> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
>
> [..]
>
>> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
>> new file mode 100644
>> index 000000000000..1aa6fbe4fda4
>> --- /dev/null
>> +++ b/drivers/input/joystick/adafruit-seesaw.c
>> @@ -0,0 +1,310 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
>> + *
>> + * Driver for Adafruit Mini I2C Gamepad
>> + *
>> + * Based on the work of:
>> + * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
>> + *
>> + * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
>> + * Product page: https://www.adafruit.com/product/5743
>> + * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
>> + *
>> + * TODO:
>> + * - Add interrupt support
>> + */
>> +
>> +#include <asm-generic/unaligned.h>
>> +#include <linux/bits.h>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/input.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
>> +
>> +#define SEESAW_STATUS_BASE 0
>> +#define SEESAW_GPIO_BASE 1
>> +#define SEESAW_ADC_BASE 9
>> +
>> +#define SEESAW_GPIO_DIRCLR_BULK 3
>> +#define SEESAW_GPIO_BULK 4
>> +#define SEESAW_GPIO_BULK_SET 5
>> +#define SEESAW_GPIO_PULLENSET 11
>> +
>> +#define SEESAW_STATUS_HW_ID 1
>> +#define SEESAW_STATUS_SWRST 127
>> +
>> +#define SEESAW_ADC_OFFSET 7
>> +
>> +#define SEESAW_BUTTON_A 5
>> +#define SEESAW_BUTTON_B 1
>> +#define SEESAW_BUTTON_X 6
>> +#define SEESAW_BUTTON_Y 2
>> +#define SEESAW_BUTTON_START 16
>> +#define SEESAW_BUTTON_SELECT 0
>> +
>> +#define SEESAW_ANALOG_X 14
>> +#define SEESAW_ANALOG_Y 15
>> +
>> +#define SEESAW_JOYSTICK_MAX_AXIS 1023
>> +#define SEESAW_JOYSTICK_FUZZ 2
>> +#define SEESAW_JOYSTICK_FLAT 4
>> +
>> +#define SEESAW_GAMEPAD_POLL_INTERVAL 16
>> +#define SEESAW_GAMEPAD_POLL_MIN 8
>> +#define SEESAW_GAMEPAD_POLL_MAX 32
>> +
>> +u32 SEESAW_BUTTON_MASK = BIT(SEESAW_BUTTON_A) | BIT(SEESAW_BUTTON_B) |
>> + BIT(SEESAW_BUTTON_X) | BIT(SEESAW_BUTTON_Y) |
>> + BIT(SEESAW_BUTTON_START) | BIT(SEESAW_BUTTON_SELECT);
>> +
>> +struct seesaw_gamepad {
>> + struct input_dev *input_dev;
>> + struct i2c_client *i2c_client;
>> +};
>> +
>> +struct seesaw_data {
>> + __be16 x;
>> + __be16 y;
>
> The fact that these are big endian is now an implementation detail
> hidden within seesaw_read_data(), so the __be16 can go away.
>
>> + u32 button_state;
>> +} __packed;
>
> While this was requested by Jeff I don't think it's correct.
> The struct is never received in this form from the device.
> I guess he also got confused, like me, by the fact that data is directly
> read into the fields of the struct.
>
> See my comment seesaw_read_data().
>
>> +struct seesaw_button_description {
>> + unsigned int code;
>> + unsigned int bit;
>> +};
>> +
>> +static const struct seesaw_button_description seesaw_buttons[] = {
>> + {
>> + .code = BTN_EAST,
>> + .bit = SEESAW_BUTTON_A,
>> + },
>> + {
>> + .code = BTN_SOUTH,
>> + .bit = SEESAW_BUTTON_B,
>> + },
>> + {
>> + .code = BTN_NORTH,
>> + .bit = SEESAW_BUTTON_X,
>> + },
>> + {
>> + .code = BTN_WEST,
>> + .bit = SEESAW_BUTTON_Y,
>> + },
>> + {
>> + .code = BTN_START,
>> + .bit = SEESAW_BUTTON_START,
>> + },
>> + {
>> + .code = BTN_SELECT,
>> + .bit = SEESAW_BUTTON_SELECT,
>> + },
>> +};
>
> This looks very much like a sparse keymap which can be implemented with
> the helpers from <linux/input/sparse-keymap.h>.
>
When going through the API provided by sparse-keymap, I could only see
the use for sparse_keymap_report_entry here. Which leads to the
following refactored code:
static const struct key_entry seesaw_buttons_new[] = {
{KE_KEY, SEESAW_BUTTON_A, {BTN_SOUTH}},
{KE_KEY, SEESAW_BUTTON_B, {BTN_EAST}},
...
};
for (i = 0; i < ARRAY_SIZE(seesaw_buttons_new); i++) {
sparse_keymap_report_entry(input, &seesaw_buttons_new[i],
data.button_state & BIT(seesaw_buttons_new[i].code),
false);
}
I don't think this significantly improves the code unless you had some
other way to use the API in mind.
> Personally I prefer each keymap entry to be on a single line (without
> designated initializers, maybe with some alignment) to save a bunch of
> vertical space.
>
>> +
>> +static int seesaw_register_read(struct i2c_client *client, u8 register_high,
>> + u8 register_low, char *buf, int count)
>> +{
>> + int ret;
>> + u8 register_buf[2] = { register_high, register_low };
>> +
>> + ret = i2c_master_send(client, register_buf, sizeof(register_buf));
>> + if (ret < 0)
>> + return ret;
>> + ret = i2c_master_recv(client, buf, count);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int seesaw_register_write_u8(struct i2c_client *client, u8 register_high,
>> + u8 register_low, u8 value)
>> +{
>> + int ret;
>> + u8 write_buf[3] = { register_high, register_low, value };
>> +
>> + ret = i2c_master_send(client, write_buf, sizeof(write_buf));
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int seesaw_register_write_u32(struct i2c_client *client,
>> + u8 register_high, u8 register_low,
>> + u32 value)
>> +{
>> + int ret;
>> + u8 write_buf[6] = { register_high, register_low };
>> +
>> + put_unaligned_be32(value, write_buf + 2);
>> + ret = i2c_master_send(client, write_buf, sizeof(write_buf));
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
>> +{
>> + int ret;
>> + u8 read_buf[4];
>> +
>> + ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
>> + read_buf, sizeof(read_buf));
>> + if (ret)
>> + return ret;
>> +
>> + data->button_state = ~get_unaligned_be32(&read_buf);
>> +
>> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
>> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
>> + (char *)&data->x, sizeof(data->x));
>
> Nitpick: read into a dedicated local variable instead of 'data', as it's
> easier to understand.
>
>> + if (ret)
>> + return ret;
>> + /*
>> + * ADC reads left as max and right as 0, must be reversed since kernel
>> + * expects reports in opposite order.
>> + */
>> + data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
>> +
>> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
>> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
>> + (char *)&data->y, sizeof(data->y));
>> + if (ret)
>> + return ret;
>> + data->y = be16_to_cpu(data->y);
>> +
>> + return 0;
>> +}
>> +
>> +static void seesaw_poll(struct input_dev *input)
>> +{
>> + int err, i;
>> + struct seesaw_gamepad *private = input_get_drvdata(input);
>> + struct seesaw_data data;
>> +
>> + err = seesaw_read_data(private->i2c_client, &data);
>> + if (err != 0) {
>
> Everywhere else this is just 'if (err)'.
>
>> + dev_err_ratelimited(&input->dev,
>> + "failed to read joystick state: %d\n", err);
>> + return;
>> + }
>> +
>> + input_report_abs(input, ABS_X, data.x);
>> + input_report_abs(input, ABS_Y, data.y);
>> +
>> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
>> + input_report_key(input, seesaw_buttons[i].code,
>> + data.button_state &
>> + BIT(seesaw_buttons[i].bit));
>> + }
>> + input_sync(input);
>> +}
>> +
>> +static int seesaw_probe(struct i2c_client *client)
>> +{
>> + int err, i;
>> + u8 hardware_id;
>> + struct seesaw_gamepad *seesaw;
>> +
>> + err = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
>> + SEESAW_STATUS_SWRST, 0xFF);
>> + if (err)
>> + return err;
>> +
>> + /* Wait for the registers to reset before proceeding */
>> + mdelay(10);
>> +
>> + seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
>> + if (!seesaw)
>> + return -ENOMEM;
>> +
>> + err = seesaw_register_read(client, SEESAW_STATUS_BASE,
>> + SEESAW_STATUS_HW_ID, &hardware_id, 1);
>> + if (err)
>> + return err;
>> +
>> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
>> + hardware_id);
>> +
>> + /* Set Pin Mode to input and enable pull-up resistors */
>> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
>> + SEESAW_GPIO_DIRCLR_BULK,
>> + SEESAW_BUTTON_MASK);
>> + if (err)
>> + return err;
>> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
>> + SEESAW_GPIO_PULLENSET,
>> + SEESAW_BUTTON_MASK);
>> + if (err)
>> + return err;
>> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
>> + SEESAW_GPIO_BULK_SET,
>> + SEESAW_BUTTON_MASK);
>> + if (err)
>> + return err;
>> +
>> + seesaw->i2c_client = client;
>> + i2c_set_clientdata(client, seesaw);
>
> I'm not familiar with the i2c APIs but this clientdata seems to be
> unused.
>
>> +
>> + seesaw->input_dev = devm_input_allocate_device(&client->dev);
>> + if (!seesaw->input_dev)
>> + return -ENOMEM;
>> +
>> + seesaw->input_dev->id.bustype = BUS_I2C;
>> + seesaw->input_dev->name = "Adafruit Seesaw Gamepad";
>> + seesaw->input_dev->phys = "i2c/" SEESAW_DEVICE_NAME;
>> + input_set_drvdata(seesaw->input_dev, seesaw);
>> + input_set_abs_params(seesaw->input_dev, ABS_X, 0,
>> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
>> + SEESAW_JOYSTICK_FLAT);
>> + input_set_abs_params(seesaw->input_dev, ABS_Y, 0,
>> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
>> + SEESAW_JOYSTICK_FLAT);
>> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
>> + input_set_capability(seesaw->input_dev, EV_KEY,
>> + seesaw_buttons[i].code);
>> + }
>> +
>> + err = input_setup_polling(seesaw->input_dev, seesaw_poll);
>> + if (err) {
>> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
>> + return err;
>> + }
>> +
>> + input_set_poll_interval(seesaw->input_dev,
>> + SEESAW_GAMEPAD_POLL_INTERVAL);
>
> Why the linebreak on this line and not on the ones below?
>
It was clang-format trying to prevent the line from being 81 characters
long, would be fixed in the next patch.
>> + input_set_max_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MAX);
>> + input_set_min_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MIN);
>> +
>> + err = input_register_device(seesaw->input_dev);
>> + if (err) {
>> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id seesaw_id_table[] = {
>> + { SEESAW_DEVICE_NAME, 0 },
>> + { /* Sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
>> +
>> +static struct i2c_driver seesaw_driver = {
>> + .driver = {
>> + .name = SEESAW_DEVICE_NAME,
>> + },
>> + .id_table = seesaw_id_table,
>> + .probe = seesaw_probe,
>> +};
>> +module_i2c_driver(seesaw_driver);
>> +
>> +MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
>> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.42.0
>>
^ permalink raw reply
* Re: [PATCH v6 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-10-31 2:05 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Thomas Weißschuh, Jeff LaBundy, Shuah Khan,
linux-kernel-mentees, linux-kernel
In-Reply-To: <ZT9TqX0tfEKpHPV9@google.com>
Hello Dmitry,
Thanks for the review, the requested changes will be added in the next
patch version. I have added a few comments below:
On 10/30/23 12:26, Dmitry Torokhov wrote:
> Hi Anshul,
>
> On Fri, Oct 27, 2023 at 10:48:11AM +0530, Anshul Dalal wrote:
>> Adds a driver for a mini gamepad that communicates over i2c, the gamepad
>> has bidirectional thumb stick input and six buttons.
>>
>> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
>> to transmit the ADC data for the joystick and digital pin state for the
>> buttons. I have only implemented the functionality required to receive the
>> thumb stick and button state.
>>
>> Steps in reading the gamepad state over i2c:
>> 1. Reset the registers
>> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
>> `BUTTON_MASK`: A bit-map for the six digital pins internally
>> connected to the joystick buttons.
>> 3. Enable internal pullup resistors for the `BUTTON_MASK`
>> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
>> 5. Poll the device for button and joystick state done by:
>> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
>>
>> Product page:
>> https://www.adafruit.com/product/5743
>> Arduino driver:
>> https://github.com/adafruit/Adafruit_Seesaw
>>
>> Driver tested on RPi Zero 2W
>>
>> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
>> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
>> ---
>> Changes for v6:
>> - Added TODO
>> - Removed `clang-format` directives
>> - Namespaced device buttons
>> - Removed `char physical_path[32]` field from `struct seesaw_gamepad`
>> - Added `packed` attribute to `struct seesaw_data`
>> - Moved from having booleans per button to single `u32 button_state`
>> - Added `seesaw_button_description` array to directly associate device
>> buttons with respective keycodes
>> - Added wrapper functions `seesaw_register_` around `i2c_master_` API
>> - Ratelimited input error reporting with `dev_err_ratelimited`
>> - Removed `of_device_id`
>>
>> Changes for v5:
>> - Added link to the datasheet
>> - Added debug log message when `seesaw_read_data` fails
>>
>> Changes for v4:
>> - Changed `1UL << BUTTON_` to BIT(BUTTON_)
>> - Removed `hardware_id` field from `struct seesaw_gamepad`
>> - Removed redundant checks for the number of bytes written and received by
>> `i2c_master_send` and `i2c_master_recv`
>> - Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
>> - Changed `result & (1UL << BUTTON_)` to
>> `test_bit(BUTTON_, (long *)&result)`
>> - Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
>> - Fixed formatting issues
>> - Changed button reporting:
>> Since the gamepad had the action buttons in a non-standard layout:
>> (X)
>> (Y) (A)
>> (B)
>> Therefore moved to using generic directional action button event codes
>> instead of BTN_[ABXY].
>>
>> Changes for v3:
>> - no updates
>>
>> Changes for v2:
>> adafruit-seesaw.c:
>> - Renamed file from 'adafruit_seesaw.c'
>> - Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
>> - Changed count parameter for receiving joystick x on line 118:
>> `2` to `sizeof(write_buf)`
>> - Fixed invalid buffer size on line 123 and 126:
>> `data->y` to `sizeof(data->y)`
>> - Added comment for the `mdelay(10)` on line 169
>> - Changed inconsistent indentation on line 271
>> Kconfig:
>> - Fixed indentation for the help text
>> - Updated module name
>> Makefile:
>> - Updated module object file name
>> MAINTAINERS:
>> - Updated file name for the driver and bindings
>>
>> MAINTAINERS | 7 +
>> drivers/input/joystick/Kconfig | 9 +
>> drivers/input/joystick/Makefile | 1 +
>> drivers/input/joystick/adafruit-seesaw.c | 310 +++++++++++++++++++++++
>> 4 files changed, 327 insertions(+)
>> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 4cc6bf79fdd8..0595c832c248 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
>> W: https://ez.analog.com/linux-software-drivers
>> F: drivers/input/touchscreen/ad7879.c
>>
>> +ADAFRUIT MINI I2C GAMEPAD
>> +M: Anshul Dalal <anshulusr@gmail.com>
>> +L: linux-input@vger.kernel.org
>> +S: Maintained
>> +F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
>> +F: drivers/input/joystick/adafruit-seesaw.c
>> +
>> ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
>> M: Jiri Kosina <jikos@kernel.org>
>> S: Maintained
>> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
>> index ac6925ce8366..df9cd1830b29 100644
>> --- a/drivers/input/joystick/Kconfig
>> +++ b/drivers/input/joystick/Kconfig
>> @@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
>> To compile this driver as a module, choose M here: the
>> module will be called sensehat_joystick.
>>
>> +config JOYSTICK_SEESAW
>> + tristate "Adafruit Mini I2C Gamepad with Seesaw"
>> + depends on I2C
>> + help
>> + Say Y here if you want to use the Adafruit Mini I2C Gamepad.
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called adafruit-seesaw.
>> +
>> endif
>> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
>> index 3937535f0098..9976f596a920 100644
>> --- a/drivers/input/joystick/Makefile
>> +++ b/drivers/input/joystick/Makefile
>> @@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
>> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
>> obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
>> obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
>> +obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
>> obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
>> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
>> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
>> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
>> new file mode 100644
>> index 000000000000..1aa6fbe4fda4
>> --- /dev/null
>> +++ b/drivers/input/joystick/adafruit-seesaw.c
>> @@ -0,0 +1,310 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
>> + *
>> + * Driver for Adafruit Mini I2C Gamepad
>> + *
>> + * Based on the work of:
>> + * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
>> + *
>> + * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
>> + * Product page: https://www.adafruit.com/product/5743
>> + * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
>> + *
>> + * TODO:
>> + * - Add interrupt support
>> + */
>> +
>> +#include <asm-generic/unaligned.h>
>> +#include <linux/bits.h>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/input.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
>> +
>> +#define SEESAW_STATUS_BASE 0
>> +#define SEESAW_GPIO_BASE 1
>> +#define SEESAW_ADC_BASE 9
>> +
>> +#define SEESAW_GPIO_DIRCLR_BULK 3
>> +#define SEESAW_GPIO_BULK 4
>> +#define SEESAW_GPIO_BULK_SET 5
>> +#define SEESAW_GPIO_PULLENSET 11
>> +
>> +#define SEESAW_STATUS_HW_ID 1
>> +#define SEESAW_STATUS_SWRST 127
>> +
>> +#define SEESAW_ADC_OFFSET 7
>> +
>> +#define SEESAW_BUTTON_A 5
>> +#define SEESAW_BUTTON_B 1
>> +#define SEESAW_BUTTON_X 6
>> +#define SEESAW_BUTTON_Y 2
>> +#define SEESAW_BUTTON_START 16
>> +#define SEESAW_BUTTON_SELECT 0
>> +
>> +#define SEESAW_ANALOG_X 14
>> +#define SEESAW_ANALOG_Y 15
>> +
>> +#define SEESAW_JOYSTICK_MAX_AXIS 1023
>> +#define SEESAW_JOYSTICK_FUZZ 2
>> +#define SEESAW_JOYSTICK_FLAT 4
>> +
>> +#define SEESAW_GAMEPAD_POLL_INTERVAL 16
>> +#define SEESAW_GAMEPAD_POLL_MIN 8
>> +#define SEESAW_GAMEPAD_POLL_MAX 32
>> +
>> +u32 SEESAW_BUTTON_MASK = BIT(SEESAW_BUTTON_A) | BIT(SEESAW_BUTTON_B) |
>> + BIT(SEESAW_BUTTON_X) | BIT(SEESAW_BUTTON_Y) |
>> + BIT(SEESAW_BUTTON_START) | BIT(SEESAW_BUTTON_SELECT);
>> +
>> +struct seesaw_gamepad {
>> + struct input_dev *input_dev;
>> + struct i2c_client *i2c_client;
>> +};
>> +
>> +struct seesaw_data {
>> + __be16 x;
>> + __be16 y;
>> + u32 button_state;
>> +} __packed;
>> +
>> +struct seesaw_button_description {
>> + unsigned int code;
>> + unsigned int bit;
>> +};
>> +
>> +static const struct seesaw_button_description seesaw_buttons[] = {
>> + {
>> + .code = BTN_EAST,
>> + .bit = SEESAW_BUTTON_A,
>> + },
>> + {
>> + .code = BTN_SOUTH,
>> + .bit = SEESAW_BUTTON_B,
>> + },
>> + {
>> + .code = BTN_NORTH,
>> + .bit = SEESAW_BUTTON_X,
>> + },
>> + {
>> + .code = BTN_WEST,
>> + .bit = SEESAW_BUTTON_Y,
>> + },
>> + {
>> + .code = BTN_START,
>> + .bit = SEESAW_BUTTON_START,
>> + },
>> + {
>> + .code = BTN_SELECT,
>> + .bit = SEESAW_BUTTON_SELECT,
>> + },
>> +};
>> +
>> +static int seesaw_register_read(struct i2c_client *client, u8 register_high,
>> + u8 register_low, char *buf, int count)
>
> I am curious why we have 2 u8s instead of u16 that we send as __be16?
>
That's done to be consistent with the manufacturer's implementation of
the seesaw framework from the Arduino driver:
bool read(uint8_t regHigh, uint8_t regLow, uint8_t *buf, uint8_t num,
uint16_t delay = 250);
The spec uses register_high as a namespace for the actual register you
want to work with: register_low.
For example when reading for the hardware id of the device, we have
`SEESAW_STATUS_BASE` [0x00] as the register_high and
`SEESAW_STATUS_HW_ID` [0x01] as the register_low which could also be
`SEESAW_STATUS_VERSION` [0x02] if instead wanted to get the framework
version the device is running.
Changing the register_high to say `SEESAW_TIMER_BASE` [0x08] and
register_low to `SEESAW_TIMER_FREQ` [0x02] would now have the chip
output the timer frequency.
>> +{
>> + int ret;
>> + u8 register_buf[2] = { register_high, register_low };
>> +
>> + ret = i2c_master_send(client, register_buf, sizeof(register_buf));
>> + if (ret < 0)
>> + return ret;
>> + ret = i2c_master_recv(client, buf, count);
>
> I am curious can this be an i2c_transfer() with read/write messages
> instead (so 1 transaction)?
>
Yes! here's what that could look like:
struct i2c_msg message_buf[2] = {
{
.addr = client->addr,
.flags = client->flags,
.len = sizeof(register_buf),
.buf = register_buf
},
{
.addr = client->addr,
.flags = client->flags | I2C_M_RD,
.len = count,
.buf = buf
}
};
ret = i2c_transfer(client->adapter, message_buf, ARRAY_SIZE(message_buf));
if (ret < 0)
return ret;
I prefer this to the prior, let me know if I should go ahead with adding
this change.
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int seesaw_register_write_u8(struct i2c_client *client, u8 register_high,
>> + u8 register_low, u8 value)
>> +{
>> + int ret;
>> + u8 write_buf[3] = { register_high, register_low, value };
>> +
>> + ret = i2c_master_send(client, write_buf, sizeof(write_buf));
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int seesaw_register_write_u32(struct i2c_client *client,
>> + u8 register_high, u8 register_low,
>> + u32 value)
>> +{
>> + int ret;
>> + u8 write_buf[6] = { register_high, register_low };
>> +
>> + put_unaligned_be32(value, write_buf + 2);
>> + ret = i2c_master_send(client, write_buf, sizeof(write_buf));
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
>> +{
>> + int ret;
>> + u8 read_buf[4];
>
> Why is this u8 buffer and not __be32?
>
>> +
>> + ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
>> + read_buf, sizeof(read_buf));
>> + if (ret)
>> + return ret;
>> +
>> + data->button_state = ~get_unaligned_be32(&read_buf);
>
> If you use __be32 you would not need to use get_unaligned_be32.
>
In my testing on a Raspberry Pi Zero 2 W (arm64), that
get_unaligned_be32 still seems to be required even with read_buf as __be32.
>
>> +
>> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
>> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
>> + (char *)&data->x, sizeof(data->x));
>> + if (ret)
>> + return ret;
>> + /*
>> + * ADC reads left as max and right as 0, must be reversed since kernel
>> + * expects reports in opposite order.
>> + */
>> + data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
>> +
>> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
>> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
>> + (char *)&data->y, sizeof(data->y));
>> + if (ret)
>> + return ret;
>> + data->y = be16_to_cpu(data->y);
>
> This is endianness violation and sparse should have warned you about
> this. A variable can either carry BE data, LE data, or CPU-endianness
> data, but not both.
Would reading the data into an __be16 and then using be16_to_cpu() to
assign it to data->y and data->x be an acceptable solution?
> I'd recommend combining seesaw_read_data() with
> seesaw_poll() into something like seesaw_report_events() and using
> temporaries for x and y and button data, and do not store them in the
> private structure at all.
>
The `struct seesaw_data` here is already temporary in seesaw_poll()
which then gets populated by seesaw_read_data(). I think the separation
of both functions is a better approach since it provides a convenient
error handler in case anything with the hardware goes wrong as:
err = seesaw_read_data(private->i2c_client, &data);
if (err) {
dev_err_ratelimited(&input->dev,
"failed to read joystick state: %d\n", err);
return;
}
>> +
>> + return 0;
>> +}
>> +
>> +static void seesaw_poll(struct input_dev *input)
>> +{
>> + int err, i;
>> + struct seesaw_gamepad *private = input_get_drvdata(input);
>> + struct seesaw_data data;
>> +
>> + err = seesaw_read_data(private->i2c_client, &data);
>> + if (err != 0) {
>> + dev_err_ratelimited(&input->dev,
>> + "failed to read joystick state: %d\n", err);
>> + return;
>> + }
>> +
>> + input_report_abs(input, ABS_X, data.x);
>> + input_report_abs(input, ABS_Y, data.y);
>> +
>> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
>> + input_report_key(input, seesaw_buttons[i].code,
>> + data.button_state &
>> + BIT(seesaw_buttons[i].bit));
>> + }
>> + input_sync(input);
>> +}
>> +
>> +static int seesaw_probe(struct i2c_client *client)
>> +{
>> + int err, i;
>> + u8 hardware_id;
>> + struct seesaw_gamepad *seesaw;
>> +
>> + err = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
>> + SEESAW_STATUS_SWRST, 0xFF);
>> + if (err)
>> + return err;
>> +
>> + /* Wait for the registers to reset before proceeding */
>> + mdelay(10);
>
> Can this be usleep_range() instead? No need to block CPU.
>
>> +
>> + seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
>> + if (!seesaw)
>> + return -ENOMEM;
>> +
>> + err = seesaw_register_read(client, SEESAW_STATUS_BASE,
>> + SEESAW_STATUS_HW_ID, &hardware_id, 1);
>
> sizeof(hardware_id)
>
>> + if (err)
>> + return err;
>> +
>> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
>> + hardware_id);
>> +
>> + /* Set Pin Mode to input and enable pull-up resistors */
>> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
>> + SEESAW_GPIO_DIRCLR_BULK,
>> + SEESAW_BUTTON_MASK);
>> + if (err)
>> + return err;
>> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
>> + SEESAW_GPIO_PULLENSET,
>> + SEESAW_BUTTON_MASK);
>> + if (err)
>> + return err;
>> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
>> + SEESAW_GPIO_BULK_SET,
>> + SEESAW_BUTTON_MASK);
>> + if (err)
>> + return err;
>> +
>> + seesaw->i2c_client = client;
>> + i2c_set_clientdata(client, seesaw);
>> +
>> + seesaw->input_dev = devm_input_allocate_device(&client->dev);
>> + if (!seesaw->input_dev)
>> + return -ENOMEM;
>> +
>> + seesaw->input_dev->id.bustype = BUS_I2C;
>> + seesaw->input_dev->name = "Adafruit Seesaw Gamepad";
>> + seesaw->input_dev->phys = "i2c/" SEESAW_DEVICE_NAME;
>> + input_set_drvdata(seesaw->input_dev, seesaw);
>> + input_set_abs_params(seesaw->input_dev, ABS_X, 0,
>> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
>> + SEESAW_JOYSTICK_FLAT);
>> + input_set_abs_params(seesaw->input_dev, ABS_Y, 0,
>> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
>> + SEESAW_JOYSTICK_FLAT);
>> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
>> + input_set_capability(seesaw->input_dev, EV_KEY,
>> + seesaw_buttons[i].code);
>> + }
>> +
>> + err = input_setup_polling(seesaw->input_dev, seesaw_poll);
>> + if (err) {
>> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
>> + return err;
>> + }
>> +
>> + input_set_poll_interval(seesaw->input_dev,
>> + SEESAW_GAMEPAD_POLL_INTERVAL);
>> + input_set_max_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MAX);
>> + input_set_min_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MIN);
>> +
>> + err = input_register_device(seesaw->input_dev);
>> + if (err) {
>> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id seesaw_id_table[] = {
>> + { SEESAW_DEVICE_NAME, 0 },
>> + { /* Sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
>> +
>> +static struct i2c_driver seesaw_driver = {
>> + .driver = {
>> + .name = SEESAW_DEVICE_NAME,
>> + },
>> + .id_table = seesaw_id_table,
>> + .probe = seesaw_probe,
>> +};
>> +module_i2c_driver(seesaw_driver);
>> +
>> +MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
>> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.42.0
>>
>
> Thanks.
>
^ permalink raw reply
* Re: [PATCH v6 01/36] dt-bindings: input: qcom,pm8921-keypad: convert to YAML format
From: Rob Herring @ 2023-10-30 20:04 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Krzysztof Kozlowski,
linux-arm-msm, devicetree, linux-input, Dmitry Torokhov,
Krzysztof Kozlowski
In-Reply-To: <20230928110309.1212221-2-dmitry.baryshkov@linaro.org>
On Thu, Sep 28, 2023 at 02:02:34PM +0300, Dmitry Baryshkov wrote:
> Convert the bindings for the keypad subdevices of Qualcomm PM8921 and
> PM8058 PMICs from text to YAML format.
>
> While doing the conversion also drop the linux,keypad-no-autorepeat
> The property was never used by DT files. Both input and DT binding
> maintainers consider that bindings should switch to assertive
> (linux,autorepeat) instead of negating (no-autorepeat) property.
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> .../bindings/input/qcom,pm8921-keypad.yaml | 89 ++++++++++++++++++
> .../bindings/input/qcom,pm8xxx-keypad.txt | 90 -------------------
> 2 files changed, 89 insertions(+), 90 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml
> delete mode 100644 Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
As this is still warning in linux-next, applied.
Rob
^ permalink raw reply
* Re: [PATCH v3 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
From: Dmitry Torokhov @ 2023-10-30 8:49 UTC (permalink / raw)
To: Kamel Bouhara
Cc: Marco Felsch, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, linux-kernel, devicetree,
mark.satterthwaite, bartp, hannah.rossiter, Thomas Petazzoni,
Gregory Clement, bsp-development.geo
In-Reply-To: <20231022193529.GC3072@kb-xps>
On Sun, Oct 22, 2023 at 09:35:29PM +0200, Kamel Bouhara wrote:
> On Fri, Oct 20, 2023 at 02:03:10PM +0200, Marco Felsch wrote:
> > > +
> > > +static int
> > > +axiom_i2c_write(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
> > > +{
> > > + struct axiom_data *ts = i2c_get_clientdata(client);
> > > + struct axiom_cmd_header cmd_header;
> > > + struct i2c_msg msg[2];
> > > + int ret;
> > > +
> > > + cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
> > > + cmd_header.length = cpu_to_le16(len);
> > > + cmd_header.read = 0;
> > > +
> > > + msg[0].addr = client->addr;
> > > + msg[0].flags = 0;
> > > + msg[0].len = sizeof(cmd_header);
> > > + msg[0].buf = (u8 *)&cmd_header;
> > > +
> > > + msg[1].addr = client->addr;
> > > + msg[1].flags = 0;
> > > + msg[1].len = len;
> > > + msg[1].buf = (char *)buf;
> >
> > Please check the "comms protocol app note", for write it is not allowed
> > to send a stop, so the whole data must be send in one i2c_msg.
> >
>
> Well I only have the "Programmer's Guide", I'll have to check that as it
> really seems to works as it yet.
As far as I know we only send "stop" on the last message in a sequence
of messages in i2c_transfer() unless it is explicitly requested with
I2C_M_STOP flag.
...
> > > +
> > > +static void axiom_i2c_remove(struct i2c_client *client)
> > > +{
> > > + struct axiom_data *ts = i2c_get_clientdata(client);
> > > +
> > > + input_unregister_device(ts->input_dev);
> >
> > This can be part of devm_add_action_or_reset() and we could remove the
> > .remove() callback for this driver.
> >
>
> Sure, thanks for the tips :)!
Actually input devices allocated with devm do not need to be explicitly
inregistered, so you do not need to bother with
devm_add_action_or_reset() and simply delete axiom_i2c_remove().
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v6 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Dmitry Torokhov @ 2023-10-30 6:56 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Thomas Weißschuh, Jeff LaBundy, Shuah Khan,
linux-kernel-mentees, linux-kernel
In-Reply-To: <20231027051819.81333-2-anshulusr@gmail.com>
Hi Anshul,
On Fri, Oct 27, 2023 at 10:48:11AM +0530, Anshul Dalal wrote:
> Adds a driver for a mini gamepad that communicates over i2c, the gamepad
> has bidirectional thumb stick input and six buttons.
>
> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> to transmit the ADC data for the joystick and digital pin state for the
> buttons. I have only implemented the functionality required to receive the
> thumb stick and button state.
>
> Steps in reading the gamepad state over i2c:
> 1. Reset the registers
> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
> `BUTTON_MASK`: A bit-map for the six digital pins internally
> connected to the joystick buttons.
> 3. Enable internal pullup resistors for the `BUTTON_MASK`
> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
> 5. Poll the device for button and joystick state done by:
> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> Driver tested on RPi Zero 2W
>
> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
> Changes for v6:
> - Added TODO
> - Removed `clang-format` directives
> - Namespaced device buttons
> - Removed `char physical_path[32]` field from `struct seesaw_gamepad`
> - Added `packed` attribute to `struct seesaw_data`
> - Moved from having booleans per button to single `u32 button_state`
> - Added `seesaw_button_description` array to directly associate device
> buttons with respective keycodes
> - Added wrapper functions `seesaw_register_` around `i2c_master_` API
> - Ratelimited input error reporting with `dev_err_ratelimited`
> - Removed `of_device_id`
>
> Changes for v5:
> - Added link to the datasheet
> - Added debug log message when `seesaw_read_data` fails
>
> Changes for v4:
> - Changed `1UL << BUTTON_` to BIT(BUTTON_)
> - Removed `hardware_id` field from `struct seesaw_gamepad`
> - Removed redundant checks for the number of bytes written and received by
> `i2c_master_send` and `i2c_master_recv`
> - Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
> - Changed `result & (1UL << BUTTON_)` to
> `test_bit(BUTTON_, (long *)&result)`
> - Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
> - Fixed formatting issues
> - Changed button reporting:
> Since the gamepad had the action buttons in a non-standard layout:
> (X)
> (Y) (A)
> (B)
> Therefore moved to using generic directional action button event codes
> instead of BTN_[ABXY].
>
> Changes for v3:
> - no updates
>
> Changes for v2:
> adafruit-seesaw.c:
> - Renamed file from 'adafruit_seesaw.c'
> - Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
> - Changed count parameter for receiving joystick x on line 118:
> `2` to `sizeof(write_buf)`
> - Fixed invalid buffer size on line 123 and 126:
> `data->y` to `sizeof(data->y)`
> - Added comment for the `mdelay(10)` on line 169
> - Changed inconsistent indentation on line 271
> Kconfig:
> - Fixed indentation for the help text
> - Updated module name
> Makefile:
> - Updated module object file name
> MAINTAINERS:
> - Updated file name for the driver and bindings
>
> MAINTAINERS | 7 +
> drivers/input/joystick/Kconfig | 9 +
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/adafruit-seesaw.c | 310 +++++++++++++++++++++++
> 4 files changed, 327 insertions(+)
> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4cc6bf79fdd8..0595c832c248 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
> W: https://ez.analog.com/linux-software-drivers
> F: drivers/input/touchscreen/ad7879.c
>
> +ADAFRUIT MINI I2C GAMEPAD
> +M: Anshul Dalal <anshulusr@gmail.com>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
> +F: drivers/input/joystick/adafruit-seesaw.c
> +
> ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
> M: Jiri Kosina <jikos@kernel.org>
> S: Maintained
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index ac6925ce8366..df9cd1830b29 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
> To compile this driver as a module, choose M here: the
> module will be called sensehat_joystick.
>
> +config JOYSTICK_SEESAW
> + tristate "Adafruit Mini I2C Gamepad with Seesaw"
> + depends on I2C
> + help
> + Say Y here if you want to use the Adafruit Mini I2C Gamepad.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called adafruit-seesaw.
> +
> endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 3937535f0098..9976f596a920 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
> +obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
> obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
> new file mode 100644
> index 000000000000..1aa6fbe4fda4
> --- /dev/null
> +++ b/drivers/input/joystick/adafruit-seesaw.c
> @@ -0,0 +1,310 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
> + *
> + * Driver for Adafruit Mini I2C Gamepad
> + *
> + * Based on the work of:
> + * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
> + *
> + * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
> + * Product page: https://www.adafruit.com/product/5743
> + * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
> + *
> + * TODO:
> + * - Add interrupt support
> + */
> +
> +#include <asm-generic/unaligned.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
> +
> +#define SEESAW_STATUS_BASE 0
> +#define SEESAW_GPIO_BASE 1
> +#define SEESAW_ADC_BASE 9
> +
> +#define SEESAW_GPIO_DIRCLR_BULK 3
> +#define SEESAW_GPIO_BULK 4
> +#define SEESAW_GPIO_BULK_SET 5
> +#define SEESAW_GPIO_PULLENSET 11
> +
> +#define SEESAW_STATUS_HW_ID 1
> +#define SEESAW_STATUS_SWRST 127
> +
> +#define SEESAW_ADC_OFFSET 7
> +
> +#define SEESAW_BUTTON_A 5
> +#define SEESAW_BUTTON_B 1
> +#define SEESAW_BUTTON_X 6
> +#define SEESAW_BUTTON_Y 2
> +#define SEESAW_BUTTON_START 16
> +#define SEESAW_BUTTON_SELECT 0
> +
> +#define SEESAW_ANALOG_X 14
> +#define SEESAW_ANALOG_Y 15
> +
> +#define SEESAW_JOYSTICK_MAX_AXIS 1023
> +#define SEESAW_JOYSTICK_FUZZ 2
> +#define SEESAW_JOYSTICK_FLAT 4
> +
> +#define SEESAW_GAMEPAD_POLL_INTERVAL 16
> +#define SEESAW_GAMEPAD_POLL_MIN 8
> +#define SEESAW_GAMEPAD_POLL_MAX 32
> +
> +u32 SEESAW_BUTTON_MASK = BIT(SEESAW_BUTTON_A) | BIT(SEESAW_BUTTON_B) |
> + BIT(SEESAW_BUTTON_X) | BIT(SEESAW_BUTTON_Y) |
> + BIT(SEESAW_BUTTON_START) | BIT(SEESAW_BUTTON_SELECT);
> +
> +struct seesaw_gamepad {
> + struct input_dev *input_dev;
> + struct i2c_client *i2c_client;
> +};
> +
> +struct seesaw_data {
> + __be16 x;
> + __be16 y;
> + u32 button_state;
> +} __packed;
> +
> +struct seesaw_button_description {
> + unsigned int code;
> + unsigned int bit;
> +};
> +
> +static const struct seesaw_button_description seesaw_buttons[] = {
> + {
> + .code = BTN_EAST,
> + .bit = SEESAW_BUTTON_A,
> + },
> + {
> + .code = BTN_SOUTH,
> + .bit = SEESAW_BUTTON_B,
> + },
> + {
> + .code = BTN_NORTH,
> + .bit = SEESAW_BUTTON_X,
> + },
> + {
> + .code = BTN_WEST,
> + .bit = SEESAW_BUTTON_Y,
> + },
> + {
> + .code = BTN_START,
> + .bit = SEESAW_BUTTON_START,
> + },
> + {
> + .code = BTN_SELECT,
> + .bit = SEESAW_BUTTON_SELECT,
> + },
> +};
> +
> +static int seesaw_register_read(struct i2c_client *client, u8 register_high,
> + u8 register_low, char *buf, int count)
I am curious why we have 2 u8s instead of u16 that we send as __be16?
> +{
> + int ret;
> + u8 register_buf[2] = { register_high, register_low };
> +
> + ret = i2c_master_send(client, register_buf, sizeof(register_buf));
> + if (ret < 0)
> + return ret;
> + ret = i2c_master_recv(client, buf, count);
I am curious can this be an i2c_transfer() with read/write messages
instead (so 1 transaction)?
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int seesaw_register_write_u8(struct i2c_client *client, u8 register_high,
> + u8 register_low, u8 value)
> +{
> + int ret;
> + u8 write_buf[3] = { register_high, register_low, value };
> +
> + ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int seesaw_register_write_u32(struct i2c_client *client,
> + u8 register_high, u8 register_low,
> + u32 value)
> +{
> + int ret;
> + u8 write_buf[6] = { register_high, register_low };
> +
> + put_unaligned_be32(value, write_buf + 2);
> + ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
> +{
> + int ret;
> + u8 read_buf[4];
Why is this u8 buffer and not __be32?
> +
> + ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
> + read_buf, sizeof(read_buf));
> + if (ret)
> + return ret;
> +
> + data->button_state = ~get_unaligned_be32(&read_buf);
If you use __be32 you would not need to use get_unaligned_be32.
> +
> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
> + (char *)&data->x, sizeof(data->x));
> + if (ret)
> + return ret;
> + /*
> + * ADC reads left as max and right as 0, must be reversed since kernel
> + * expects reports in opposite order.
> + */
> + data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
> +
> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
> + (char *)&data->y, sizeof(data->y));
> + if (ret)
> + return ret;
> + data->y = be16_to_cpu(data->y);
This is endianness violation and sparse should have warned you about
this. A variable can either carry BE data, LE data, or CPU-endianness
data, but not both. I'd recommend combining seesaw_read_data() with
seesaw_poll() into something like seesaw_report_events() and using
temporaries for x and y and button data, and do not store them in the
private structure at all.
> +
> + return 0;
> +}
> +
> +static void seesaw_poll(struct input_dev *input)
> +{
> + int err, i;
> + struct seesaw_gamepad *private = input_get_drvdata(input);
> + struct seesaw_data data;
> +
> + err = seesaw_read_data(private->i2c_client, &data);
> + if (err != 0) {
> + dev_err_ratelimited(&input->dev,
> + "failed to read joystick state: %d\n", err);
> + return;
> + }
> +
> + input_report_abs(input, ABS_X, data.x);
> + input_report_abs(input, ABS_Y, data.y);
> +
> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
> + input_report_key(input, seesaw_buttons[i].code,
> + data.button_state &
> + BIT(seesaw_buttons[i].bit));
> + }
> + input_sync(input);
> +}
> +
> +static int seesaw_probe(struct i2c_client *client)
> +{
> + int err, i;
> + u8 hardware_id;
> + struct seesaw_gamepad *seesaw;
> +
> + err = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
> + SEESAW_STATUS_SWRST, 0xFF);
> + if (err)
> + return err;
> +
> + /* Wait for the registers to reset before proceeding */
> + mdelay(10);
Can this be usleep_range() instead? No need to block CPU.
> +
> + seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
> + if (!seesaw)
> + return -ENOMEM;
> +
> + err = seesaw_register_read(client, SEESAW_STATUS_BASE,
> + SEESAW_STATUS_HW_ID, &hardware_id, 1);
sizeof(hardware_id)
> + if (err)
> + return err;
> +
> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
> + hardware_id);
> +
> + /* Set Pin Mode to input and enable pull-up resistors */
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_DIRCLR_BULK,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_PULLENSET,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_BULK_SET,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> +
> + seesaw->i2c_client = client;
> + i2c_set_clientdata(client, seesaw);
> +
> + seesaw->input_dev = devm_input_allocate_device(&client->dev);
> + if (!seesaw->input_dev)
> + return -ENOMEM;
> +
> + seesaw->input_dev->id.bustype = BUS_I2C;
> + seesaw->input_dev->name = "Adafruit Seesaw Gamepad";
> + seesaw->input_dev->phys = "i2c/" SEESAW_DEVICE_NAME;
> + input_set_drvdata(seesaw->input_dev, seesaw);
> + input_set_abs_params(seesaw->input_dev, ABS_X, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + input_set_abs_params(seesaw->input_dev, ABS_Y, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
> + input_set_capability(seesaw->input_dev, EV_KEY,
> + seesaw_buttons[i].code);
> + }
> +
> + err = input_setup_polling(seesaw->input_dev, seesaw_poll);
> + if (err) {
> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
> + return err;
> + }
> +
> + input_set_poll_interval(seesaw->input_dev,
> + SEESAW_GAMEPAD_POLL_INTERVAL);
> + input_set_max_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MAX);
> + input_set_min_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MIN);
> +
> + err = input_register_device(seesaw->input_dev);
> + if (err) {
> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id seesaw_id_table[] = {
> + { SEESAW_DEVICE_NAME, 0 },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
> +
> +static struct i2c_driver seesaw_driver = {
> + .driver = {
> + .name = SEESAW_DEVICE_NAME,
> + },
> + .id_table = seesaw_id_table,
> + .probe = seesaw_probe,
> +};
> +module_i2c_driver(seesaw_driver);
> +
> +MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
> +MODULE_LICENSE("GPL");
> --
> 2.42.0
>
Thanks.
--
Dmitry
^ 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