* Re: [PATCH v3 2/2] Input: cap11xx - add advanced sensitivity settings
From: kernel test robot @ 2023-06-26 18:49 UTC (permalink / raw)
To: Jiri Valek - 2N, krzysztof.kozlowski+dt, dmitry.torokhov
Cc: oe-kbuild-all, jiriv, devicetree, linux-input, linux-kernel,
robh+dt, u.kleine-koenig
In-Reply-To: <20230626130006.850254-3-jiriv@axis.com>
Hi Jiri,
kernel test robot noticed the following build warnings:
[auto build test WARNING on dtor-input/next]
[also build test WARNING on dtor-input/for-linus linus/master v6.4 next-20230626]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jiri-Valek-2N/dt-bindings-input-microchip-cap11xx-add-advanced-sensitivity-settings/20230626-210123
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
patch link: https://lore.kernel.org/r/20230626130006.850254-3-jiriv%40axis.com
patch subject: [PATCH v3 2/2] Input: cap11xx - add advanced sensitivity settings
config: i386-randconfig-i013-20230626 (https://download.01.org/0day-ci/archive/20230627/202306270246.llRp2LOP-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230627/202306270246.llRp2LOP-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306270246.llRp2LOP-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/input/keyboard/cap11xx.c: In function 'cap11xx_i2c_probe':
>> drivers/input/keyboard/cap11xx.c:521:23: warning: unused variable 'irq' [-Wunused-variable]
521 | int i, error, irq;
| ^~~
vim +/irq +521 drivers/input/keyboard/cap11xx.c
514
515 static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
516 {
517 const struct i2c_device_id *id = i2c_client_get_device_id(i2c_client);
518 struct device *dev = &i2c_client->dev;
519 struct cap11xx_priv *priv;
520 const struct cap11xx_hw_model *cap;
> 521 int i, error, irq;
522 unsigned int val, rev;
523
524 if (id->driver_data >= ARRAY_SIZE(cap11xx_devices)) {
525 dev_err(dev, "Invalid device ID %lu\n", id->driver_data);
526 return -EINVAL;
527 }
528
529 cap = &cap11xx_devices[id->driver_data];
530 if (!cap || !cap->num_channels) {
531 dev_err(dev, "Invalid device configuration\n");
532 return -EINVAL;
533 }
534
535 priv = devm_kzalloc(dev,
536 struct_size(priv, keycodes, cap->num_channels),
537 GFP_KERNEL);
538 if (!priv)
539 return -ENOMEM;
540
541 priv->dev = dev;
542
543 priv->regmap = devm_regmap_init_i2c(i2c_client, &cap11xx_regmap_config);
544 if (IS_ERR(priv->regmap))
545 return PTR_ERR(priv->regmap);
546
547 error = regmap_read(priv->regmap, CAP11XX_REG_PRODUCT_ID, &val);
548 if (error)
549 return error;
550
551 if (val != cap->product_id) {
552 dev_err(dev, "Product ID: Got 0x%02x, expected 0x%02x\n",
553 val, cap->product_id);
554 return -ENXIO;
555 }
556
557 error = regmap_read(priv->regmap, CAP11XX_REG_MANUFACTURER_ID, &val);
558 if (error)
559 return error;
560
561 if (val != CAP11XX_MANUFACTURER_ID) {
562 dev_err(dev, "Manufacturer ID: Got 0x%02x, expected 0x%02x\n",
563 val, CAP11XX_MANUFACTURER_ID);
564 return -ENXIO;
565 }
566
567 error = regmap_read(priv->regmap, CAP11XX_REG_REVISION, &rev);
568 if (error < 0)
569 return error;
570
571 dev_info(dev, "CAP11XX detected, model %s, revision 0x%02x\n",
572 id->name, rev);
573
574 priv->model = cap;
575 priv->id = id->driver_data;
576
577 dev_info(dev, "CAP11XX device detected, model %s, revision 0x%02x\n",
578 id->name, rev);
579
580 error = cap11xx_init_keys(priv);
581 if (error)
582 return error;
583
584 priv->idev = devm_input_allocate_device(dev);
585 if (!priv->idev)
586 return -ENOMEM;
587
588 priv->idev->name = "CAP11XX capacitive touch sensor";
589 priv->idev->id.bustype = BUS_I2C;
590 priv->idev->evbit[0] = BIT_MASK(EV_KEY);
591
592 if (of_property_read_bool(dev->of_node, "autorepeat"))
593 __set_bit(EV_REP, priv->idev->evbit);
594
595 for (i = 0; i < cap->num_channels; i++)
596 __set_bit(priv->keycodes[i], priv->idev->keybit);
597
598 __clear_bit(KEY_RESERVED, priv->idev->keybit);
599
600 priv->idev->keycode = priv->keycodes;
601 priv->idev->keycodesize = sizeof(priv->keycodes[0]);
602 priv->idev->keycodemax = cap->num_channels;
603
604 priv->idev->id.vendor = CAP11XX_MANUFACTURER_ID;
605 priv->idev->id.product = cap->product_id;
606 priv->idev->id.version = rev;
607
608 priv->idev->open = cap11xx_input_open;
609 priv->idev->close = cap11xx_input_close;
610
611 error = cap11xx_init_leds(dev, priv, cap->num_leds);
612 if (error)
613 return error;
614
615 input_set_drvdata(priv->idev, priv);
616
617 /*
618 * Put the device in deep sleep mode for now.
619 * ->open() will bring it back once the it is actually needed.
620 */
621 cap11xx_set_sleep(priv, true);
622
623 error = input_register_device(priv->idev);
624 if (error)
625 return error;
626
627 error = devm_request_threaded_irq(dev, i2c_client->irq, NULL,
628 cap11xx_thread_func, IRQF_ONESHOT, dev_name(dev), priv);
629 if (error)
630 return error;
631
632 return 0;
633 }
634
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH] dt-bindings: input: mediatek,pmic-keys: Fix typo in "linux,keycodes" property name
From: Rob Herring @ 2023-06-26 16:25 UTC (permalink / raw)
To: Rob Herring
Cc: Conor Dooley, linux-input, Dmitry Torokhov, Matthias Brugger,
devicetree, Krzysztof Kozlowski, linux-mediatek, linux-kernel,
AngeloGioacchino Del Regno, Chen Zhong, linux-arm-kernel
In-Reply-To: <20230613201040.2823802-1-robh@kernel.org>
On Tue, 13 Jun 2023 14:10:40 -0600, Rob Herring wrote:
> "linux-keycodes" is the wrong property name and is unused. It should be
> "linux,keycodes" instead.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Documentation/devicetree/bindings/input/mediatek,pmic-keys.yaml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid: Block a rogue device on ASUS TUF A16
From: Limonciello, Mario @ 2023-06-26 15:10 UTC (permalink / raw)
To: Friedrich Vock, linux-input, Natikar, Basavaraj,
S-k, Shyam-sundar
Cc: Jiri Kosina, Benjamin Tissoires
In-Reply-To: <b067c9a1-cc07-2781-9a9d-71488ec3ddba@amd.com>
On 6/18/2023 10:05 PM, Mario Limonciello wrote:
> On 6/15/23 07:41, Friedrich Vock wrote:
>> Hi,
>>
>> sorry for taking so long to reply.
>>
>> On 02.06.23 20:43, Limonciello, Mario wrote:
>>> + some AMD guys
>>>
>>> On 5/30/2023 10:40 AM, Friedrich Vock wrote:
>>>> On these laptops, there seems to be a device that, when probed by
>>>> i2c-hid, constantly sends bogus interrupts and interferes with the
>>>> keyboard controller. When the device is enabled, it takes the keyboard
>>>> around 8 seconds to register that keys are being pressed or released.
>>>
>>> Do you know what interrupt is firing constantly?
>>> Presumably it is the GPIO controller master interrupt, right?
>>> And it's for GPIO 7 (guessed from acpidump on one of the bug
>>> reports).
>>>
>>> To confirm check /proc/interrupts.
>> Seems likely that you guessed correctly. The corresponsing line in
>> /proc/interrupts (with the interrupts counts omitted):
>> 71: amd_gpio 7 ITE5570:00
>
> OK.
I had asked in the past for R/W everything output to compare to
/sys/kernel/debug/gpio.
I've added more explicit instructions how to get this to
the kernel bugzilla 217336 – keyboard not working Asus TUF FA617NS
(kernel.org) <https://bugzilla.kernel.org/show_bug.cgi?id=217336#c126>
>
>>>
>>> If it's not obvious which GPIO is firing there is also a dynamic
>>> debug statement in pinctrl-amd.c that may be helpful to figure
>>> this out.
>>>
>>> I would also suspect in Windows this doesn't happen. If possible
>>> can you confirm that? Check in Device Manager what driver is bound
>>> to this device. Is it "inbox" from Microsoft or is it an ASUS
>>> specific driver?
>>>
>>> I wonder if the GPIO controller got programmed differently in
>>> Windows for some reason. We may want to confirm the values for
>>> GPIO registers from /sys/kernel/debug/gpio in Linux against those
>>> that are programmed in Windows.
>>>
>>> This can be accomplished using R/W everything in Windows.
>>
>> Unfortunately I don't have Windows installed on this system. I know some
>> people with the Ryzen 9 7940HS model which might, I'll ask them if they
>> can give me the configuration on Windows and Linux.
>
> OK, I suggest directing everyone over to the bugs I linked and we
> should gather the relevant GPIO controller register dumps from Windows
> and Linux on the same hardware there.
>
>>
>>>
>>>>
>>>> Nothing I tried seemed to make the device work, and preventing the
>>>> device from being probed doesn't seem to break any functionality of
>>>> the laptop.
>>>>
>>>> Signed-off-by: Friedrich Vock <friedrich.vock@gmx.de>
>>>
>>> There are a few bug reports that popped up around this issue that
>>> should
>>> probably also be tagged.
>>>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217336
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217493
>>>
>>>> ---
>>>> drivers/hid/i2c-hid/i2c-hid-core.c | 5 +++
>>>> drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c | 48
>>>> ++++++++++++++++++++++++
>>>> drivers/hid/i2c-hid/i2c-hid.h | 3 ++
>>>> 3 files changed, 56 insertions(+)
>>>>
>>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
>>>> b/drivers/hid/i2c-hid/i2c-hid-core.c
>>>> index efbba0465eef..5f0686d058df 100644
>>>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>>>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>>>> @@ -1035,6 +1035,11 @@ int i2c_hid_core_probe(struct i2c_client
>>>> *client, struct i2chid_ops *ops,
>>>>
>>>> ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
>>>>
>>>> + if (i2c_hid_device_blocked(hid->vendor, hid->product)) {
>>>> + ret = -ENODEV;
>>>> + goto err_irq;
>>>> + }
>>>> +
>>> The thing I worry about here is that an unserviced interrupt can
>>> prevent the
>>> hardware from going into proper low power states; particularly at
>>> runtime.
>>>
>>> I think we should better understand what's going on before going down
>>> this
>>> path of just ignoring it.
>> Yeah, I guess I should've searched more for a proper explanation/fix
>> before submitting hacks like this. Let's see if this can be fixed in a
>> cleaner manner than preemptively disabling parts of the system.
>
> Can you please have a try with linux-next or apply this series:
> https://lore.kernel.org/linux-gpio/20230421120625.3366-1-mario.limonciello@amd.com/
>
>
> That does change some of the configuration for the GPIO controller
> registers to align how windows is handling debouncing.
>
> I don't think it will change the outcome of your bug, but I'd love
> to be surprised.
Any updates for this?
>
>>>> ret = hid_add_device(hid);
>>>> if (ret) {
>>>> if (ret != -ENODEV)
>>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
>>>> b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
>>>> index 210f17c3a0be..d2c2806b64b4 100644
>>>> --- a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
>>>> +++ b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
>>>> @@ -440,6 +440,38 @@ static const struct dmi_system_id
>>>> i2c_hid_dmi_quirk_table[] = {
>>>> { } /* Terminate list */
>>>> };
>>>>
>>>> +static const struct hid_device_id i2c_hid_blocked_ite_device = {
>>>> + HID_DEVICE(BUS_I2C, HID_GROUP_GENERIC, USB_VENDOR_ID_ITE, 0x8051)
>>>> +};
>>>> +
>>>> +/*
>>>> + * This list contains devices that can't be activated at all, for
>>>> example
>>>> + * because activating them breaks other important parts of the
>>>> system.
>>>> + */
>>>> +static const struct dmi_system_id i2c_hid_dmi_block_table[] = {
>>>> + /*
>>>> + * On ASUS TUF Gaming A16 laptops, there is a device that will
>>>> make the
>>>> + * keyboard controller stop working correctly and flood the CPU
>>>> with bogus
>>>> + * interrupts.
>>>> + */
>>>> + {
>>>> + .ident = "ASUS TUF Gaming A16 (Ryzen 7 7735HS)",
>>>> + .matches = {
>>>> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>>>> + DMI_MATCH(DMI_PRODUCT_NAME, "FA617NS"),
>>>> + },
>>>> + .driver_data = (void *)&i2c_hid_blocked_ite_device,
>>>> + },
>>>> + {
>>>> + .ident = "ASUS TUF Gaming A16 (Ryzen 9 7940HS)",
>>>> + .matches = {
>>>> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>>>> + DMI_MATCH(DMI_PRODUCT_NAME, "FA617XS"),
>>>> + },
>>>> + .driver_data = (void *)&i2c_hid_blocked_ite_device,
>>>> + },
>>>> + { } /* Terminate list */
>>> If this *does* end up being the best solution, I think it's better
>>> to patch in the DMI to gpiolib-acpi.c similar to other quirks for
>>> floating
>>> GPIOs. Example:
>>>
>>> https://github.com/torvalds/linux/blob/master/drivers/gpio/gpiolib-acpi.c#L1654
>>>
>>>
>>>
>>>> +};
>>>>
>>>> struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t
>>>> *i2c_name)
>>>> {
>>>> @@ -492,3 +524,19 @@ u32 i2c_hid_get_dmi_quirks(const u16 vendor,
>>>> const u16 product)
>>>>
>>>> return quirks;
>>>> }
>>>> +
>>>> +bool i2c_hid_device_blocked(const u16 vendor, const u16 product)
>>>> +{
>>>> + const struct dmi_system_id *system_id =
>>>> + dmi_first_match(i2c_hid_dmi_block_table);
>>>> +
>>>> + if (system_id) {
>>>> + const struct hid_device_id *device_id =
>>>> + (struct hid_device_id *)(system_id->driver_data);
>>>> +
>>>> + if (device_id && device_id->vendor == vendor &&
>>>> + device_id->product == product)
>>>> + return true;
>>>> + }
>>>> + return false;
>>>> +}
>>>> diff --git a/drivers/hid/i2c-hid/i2c-hid.h
>>>> b/drivers/hid/i2c-hid/i2c-hid.h
>>>> index 2c7b66d5caa0..e17bdd758f39 100644
>>>> --- a/drivers/hid/i2c-hid/i2c-hid.h
>>>> +++ b/drivers/hid/i2c-hid/i2c-hid.h
>>>> @@ -10,6 +10,7 @@ struct i2c_hid_desc
>>>> *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name);
>>>> char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
>>>> unsigned int *size);
>>>> u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product);
>>>> +bool i2c_hid_device_blocked(const u16 vendor, const u16 product);
>>>> #else
>>>> static inline struct i2c_hid_desc
>>>> *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name)
>>>> @@ -19,6 +20,8 @@ static inline char
>>>> *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
>>>> { return NULL; }
>>>> static inline u32 i2c_hid_get_dmi_quirks(const u16 vendor, const
>>>> u16 product)
>>>> { return 0; }
>>>> +static inline bool i2c_hid_device_blocked(const u16 vendor, const
>>>> u16 product)
>>>> +{ return false; }
>>>> #endif
>>>>
>>>> /**
>>>> --
>>>> 2.40.1
>>>>
>>>>
>
^ permalink raw reply
* Re: [PATCH v2] HID: wacom: Use ktime_t rather than int when dealing with timestamps
From: bentiss @ 2023-06-26 14:54 UTC (permalink / raw)
To: linux-input, linux-kernel, Benjamin Tissoires, Jiri Kosina,
Jason Gerecke
Cc: Benjamin Tissoires, Ping Cheng, Aaron Armstrong Skomra,
Joshua Dickens, Peter Hutterer, Jason Gerecke, stable
In-Reply-To: <20230608213828.2108-1-jason.gerecke@wacom.com>
From: Benjamin Tissoires <bentiss@kernel.org>
On Thu, 08 Jun 2023 14:38:28 -0700, Jason Gerecke wrote:
> Code which interacts with timestamps needs to use the ktime_t type
> returned by functions like ktime_get. The int type does not offer
> enough space to store these values, and attempting to use it is a
> recipe for problems. In this particular case, overflows would occur
> when calculating/storing timestamps leading to incorrect values being
> reported to userspace. In some cases these bad timestamps cause input
> handling in userspace to appear hung.
>
> [...]
Updated the "from" email from your patch.
It would be nice if you could fix your workflow (I know you well enough
to know what is your gmail address, but not having to fix this by hand
would be preferable)
Applied to hid/hid.git (for-6.5/wacom), thanks!
[1/1] HID: wacom: Use ktime_t rather than int when dealing with timestamps
https://git.kernel.org/hid/hid/c/9a6c0e28e215
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: rework one more time the retries attempts
From: Benjamin Tissoires @ 2023-06-26 14:02 UTC (permalink / raw)
To: Bastien Nocera
Cc: Greg KH, Filipe Laíns, Jiri Kosina, linux-input,
linux-kernel, stable
In-Reply-To: <31ce32e018a9fa410e9e1f3e5900621b16a56091.camel@hadess.net>
On Sun, Jun 25, 2023 at 10:30 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Fri, 2023-06-23 at 10:37 +0200, Benjamin Tissoires wrote:
> >
> > On Jun 21 2023, Greg KH wrote:
> > >
> > > On Wed, Jun 21, 2023 at 11:42:30AM +0200, Benjamin Tissoires wrote:
> > > > Make the code looks less like Pascal.
> > > >
> > > > Extract the internal code inside a helper function, fix the
> > > > initialization of the parameters used in the helper function
> > > > (`hidpp->answer_available` was not reset and `*response` wasn't
> > > > too),
> > > > and use a `do {...} while();` loop.
> > > >
> > > > Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when
> > > > device is busy")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > ---
> > > > as requested by
> > > > https://lore.kernel.org/all/CAHk-=wiMbF38KCNhPFiargenpSBoecSXTLQACKS2UMyo_Vu2ww@mail.gmail.com/
> > > > This is a rewrite of that particular piece of code.
> > > > ---
> > > > drivers/hid/hid-logitech-hidpp.c | 102 +++++++++++++++++++++++--
> > > > --------------
> > > > 1 file changed, 61 insertions(+), 41 deletions(-)
> > > >
> > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> > > > logitech-hidpp.c
> > > > index dfe8e09a18de..3d1ffe199f08 100644
> > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > @@ -275,21 +275,20 @@ static int __hidpp_send_report(struct
> > > > hid_device *hdev,
> > > > }
> > > >
> > > > /*
> > > > - * hidpp_send_message_sync() returns 0 in case of success, and
> > > > something else
> > > > - * in case of a failure.
> > > > - * - If ' something else' is positive, that means that an error
> > > > has been raised
> > > > - * by the protocol itself.
> > > > - * - If ' something else' is negative, that means that we had a
> > > > classic error
> > > > - * (-ENOMEM, -EPIPE, etc...)
> > > > + * Effectively send the message to the device, waiting for its
> > > > answer.
> > > > + *
> > > > + * Must be called with hidpp->send_mutex locked
> > > > + *
> > > > + * Same return protocol than hidpp_send_message_sync():
> > > > + * - success on 0
> > > > + * - negative error means transport error
> > > > + * - positive value means protocol error
> > > > */
> > > > -static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> > > > +static int __do_hidpp_send_message_sync(struct hidpp_device
> > > > *hidpp,
> > > > struct hidpp_report *message,
> > > > struct hidpp_report *response)
> > >
> > > __must_hold(&hidpp->send_mutex) ?
> > >
> >
> > Good point. I'll add this in v2.
> >
> > I'm still waiting for some feedback from the people who particpated
> > in
> > the original BZ, but the new bug is harder to reproduce. Anyway,
> > there
> > is no rush IMO.
>
> The problem is only ever going to show up in very limited circumstances
> after the logic fix was applied.
>
> You need a hardware problem (such as the controller being too busy to
> answer) to trigger the problems fixed by this patch. I don't see a way
> to reliably reproduce it unless you inject that hardware error.
>
Some people on the Bz were able to reproduce with multiple reboots.
But it's not as urgent as previously, and we were close to the 6.4
final when I sent it. I'll make sure this goes into 6.5 and gets
proper stable backports FWIW.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: rework one more time the retries attempts
From: Benjamin Tissoires @ 2023-06-26 14:01 UTC (permalink / raw)
To: Bastien Nocera
Cc: Filipe Laíns, Jiri Kosina, linux-input, linux-kernel, stable
In-Reply-To: <df4cc4a907c6d617036aea6da6f06de6bba30ca1.camel@hadess.net>
On Sun, Jun 25, 2023 at 10:30 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Wed, 2023-06-21 at 11:42 +0200, Benjamin Tissoires wrote:
> > Make the code looks less like Pascal.
>
> Honestly, while this was written in jest in an email is fine, putting
> this in the commit message is quite insulting.
>
> The "retry" patch tried to fix real world problems by making minimal
> code changes, eg. avoiding the review problem that the present patch
> has, and even then, all of us missed the logic bug.
>
> I also haven't written any Pascal code since 1996.
Apologies for that. I honestly took Linus' remark to myself only,
because I was fixing your fix on my original code.
And while initially fixing your for loop, I should have realized that
this was very hard to follow, because of the "if (sth; sth < 1 && foo
&& bar; sth+=1)".
I'll amend v2
>
> > Extract the internal code inside a helper function, fix the
> > initialization of the parameters used in the helper function
> > (`hidpp->answer_available` was not reset and `*response` wasn't too),
>
> "wasn't either".
>
> > and use a `do {...} while();` loop.
> >
> > Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device
> > is busy")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> > as requested by
> > https://lore.kernel.org/all/CAHk-=wiMbF38KCNhPFiargenpSBoecSXTLQACKS2UMyo_Vu2ww@mail.gmail.com/
> > This is a rewrite of that particular piece of code.
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 102 +++++++++++++++++++++++------
> > ----------
> > 1 file changed, 61 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> > logitech-hidpp.c
> > index dfe8e09a18de..3d1ffe199f08 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -275,21 +275,20 @@ static int __hidpp_send_report(struct
> > hid_device *hdev,
> > }
> >
> > /*
> > - * hidpp_send_message_sync() returns 0 in case of success, and
> > something else
> > - * in case of a failure.
> > - * - If ' something else' is positive, that means that an error has
> > been raised
> > - * by the protocol itself.
> > - * - If ' something else' is negative, that means that we had a
> > classic error
> > - * (-ENOMEM, -EPIPE, etc...)
> > + * Effectively send the message to the device, waiting for its
> > answer.
> > + *
> > + * Must be called with hidpp->send_mutex locked
> > + *
> > + * Same return protocol than hidpp_send_message_sync():
> > + * - success on 0
> > + * - negative error means transport error
> > + * - positive value means protocol error
> > */
> > -static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> > +static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
> > struct hidpp_report *message,
> > struct hidpp_report *response)
> > {
> > - int ret = -1;
> > - int max_retries = 3;
> > -
> > - mutex_lock(&hidpp->send_mutex);
> > + int ret;
> >
> > hidpp->send_receive_buf = response;
> > hidpp->answer_available = false;
> > @@ -300,41 +299,62 @@ static int hidpp_send_message_sync(struct
> > hidpp_device *hidpp,
> > */
> > *response = *message;
> >
> > - for (; max_retries != 0 && ret; max_retries--) {
> > - ret = __hidpp_send_report(hidpp->hid_dev, message);
> > + ret = __hidpp_send_report(hidpp->hid_dev, message);
> > + if (ret) {
> > + dbg_hid("__hidpp_send_report returned err: %d\n",
> > ret);
> > + memset(response, 0, sizeof(struct hidpp_report));
> > + return ret;
> > + }
> >
> > - if (ret) {
> > - dbg_hid("__hidpp_send_report returned err:
> > %d\n", ret);
> > - memset(response, 0, sizeof(struct
> > hidpp_report));
> > - break;
> > - }
> > + if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
> > + 5*HZ)) {
> > + dbg_hid("%s:timeout waiting for response\n",
> > __func__);
> > + memset(response, 0, sizeof(struct hidpp_report));
> > + return -ETIMEDOUT;
> > + }
> >
> > - if (!wait_event_timeout(hidpp->wait, hidpp-
> > >answer_available,
> > - 5*HZ)) {
> > - dbg_hid("%s:timeout waiting for response\n",
> > __func__);
> > - memset(response, 0, sizeof(struct
> > hidpp_report));
> > - ret = -ETIMEDOUT;
> > - break;
> > - }
> > + if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> > + response->rap.sub_id == HIDPP_ERROR) {
> > + ret = response->rap.params[1];
> > + dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
> > + return ret;
> > + }
> >
> > - if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> > - response->rap.sub_id == HIDPP_ERROR) {
> > - ret = response->rap.params[1];
> > - dbg_hid("%s:got hidpp error %02X\n",
> > __func__, ret);
> > + if ((response->report_id == REPORT_ID_HIDPP_LONG ||
> > + response->report_id == REPORT_ID_HIDPP_VERY_LONG) &&
> > + response->fap.feature_index == HIDPP20_ERROR) {
> > + ret = response->fap.params[1];
> > + dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__,
> > ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * hidpp_send_message_sync() returns 0 in case of success, and
> > something else
> > + * in case of a failure.
> > + * - If ' something else' is positive, that means that an error has
> > been raised
> > + * by the protocol itself.
> > + * - If ' something else' is negative, that means that we had a
> > classic error
> > + * (-ENOMEM, -EPIPE, etc...)
>
> Do we really need to re-explain the possible return values that were
> already explained above __do_hidpp_send_message_sync()?
Right, maybe we don't need to duplicate the comment after all.
>
> If we do, why don't also do it for hidpp_send_fap_command_sync() and
> hidpp_send_rap_command_sync(), or their callers?
In a way it would make sense to do, because this is non standard.
>
> If it's absolutely necessary, a "see __do_hidpp_send_message_sync()"
> should be enough.
Good point.
>
> I've double-checked that none of the existing callers expected a
> partially filled in "response" struct on error.
>
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
Thanks!
Cheers,
Benjamin
>
> > + */
> > +static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> > + struct hidpp_report *message,
> > + struct hidpp_report *response)
> > +{
> > + int ret;
> > + int max_retries = 3;
> > +
> > + mutex_lock(&hidpp->send_mutex);
> > +
> > + do {
> > + ret = __do_hidpp_send_message_sync(hidpp, message,
> > response);
> > + if (ret != HIDPP20_ERROR_BUSY)
> > break;
> > - }
> >
> > - if ((response->report_id == REPORT_ID_HIDPP_LONG ||
> > - response->report_id ==
> > REPORT_ID_HIDPP_VERY_LONG) &&
> > - response->fap.feature_index == HIDPP20_ERROR) {
> > - ret = response->fap.params[1];
> > - if (ret != HIDPP20_ERROR_BUSY) {
> > - dbg_hid("%s:got hidpp 2.0 error
> > %02X\n", __func__, ret);
> > - break;
> > - }
> > - dbg_hid("%s:got busy hidpp 2.0 error %02X,
> > retrying\n", __func__, ret);
> > - }
> > - }
> > + dbg_hid("%s:got busy hidpp 2.0 error %02X,
> > retrying\n", __func__, ret);
> > + } while (--max_retries);
> >
> > mutex_unlock(&hidpp->send_mutex);
> > return ret;
> >
> > ---
> > base-commit: b98ec211af5508457e2b1c4cc99373630a83fa81
> > change-id: 20230621-logitech-fixes-a4c0e66ea2ad
> >
> > Best regards,
>
^ permalink raw reply
* Re: [PATCH v3 1/4] Input: ts-overlay - Add touchscreen overlay object handling
From: Jeff LaBundy @ 2023-06-26 13:47 UTC (permalink / raw)
To: Javier Carrasco
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch, linux-kernel,
linux-input, devicetree
In-Reply-To: <89cbb534-9371-c2be-0bad-776b51476ce8@wolfvision.net>
Hi Javier,
On Mon, Jun 26, 2023 at 12:31:21PM +0200, Javier Carrasco wrote:
> Hi Jeff,
>
> On 26.06.23 04:35, Jeff LaBundy wrote:
> > Hi Javier,
> >
> > On Fri, Jun 16, 2023 at 09:28:51AM +0200, Javier Carrasco wrote:
> >> Some touchscreens provide mechanical overlays with different objects
> >> like buttons or clipped touchscreen surfaces.
> >>
> >> In order to support these objects, add a series of helper functions
> >> to the input subsystem to transform them into overlay objects via
> >> device tree nodes.
> >>
> >> These overlay objects consume the raw touch events and report the
> >> expected input events depending on the object properties.
> >>
> >> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> >> ---
> >
> > Excellent work; it's great to see this series move along.
> >
> >> MAINTAINERS | 7 +
> >> drivers/input/touchscreen/Kconfig | 9 +
> >> drivers/input/touchscreen/Makefile | 1 +
> >> drivers/input/touchscreen/ts-overlay.c | 356 +++++++++++++++++++++++++++++++++
> >> include/linux/input/ts-overlay.h | 43 ++++
> >> 5 files changed, 416 insertions(+)
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 7e0b87d5aa2e..db9427926a4c 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -21434,6 +21434,13 @@ W: https://github.com/srcres258/linux-doc
> >> T: git git://github.com/srcres258/linux-doc.git doc-zh-tw
> >> F: Documentation/translations/zh_TW/
> >>
> >> +TOUCHSCREEN OVERLAY OBJECTS
> >> +M: Javier Carrasco <javier.carrasco@wolfvision.net>
> >> +L: linux-input@vger.kernel.org
> >> +S: Maintained
> >> +F: drivers/input/touchscreen/ts-overlay.c
> >> +F: include/linux/input/ts-overlay.h
> >> +
> >> TTY LAYER
> >> M: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> M: Jiri Slaby <jirislaby@kernel.org>
> >> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> >> index 143ff43c67ae..8382a4d68326 100644
> >> --- a/drivers/input/touchscreen/Kconfig
> >> +++ b/drivers/input/touchscreen/Kconfig
> >> @@ -1388,4 +1388,13 @@ config TOUCHSCREEN_HIMAX_HX83112B
> >> To compile this driver as a module, choose M here: the
> >> module will be called himax_hx83112b.
> >>
> >> +config TOUCHSCREEN_TS_OVERLAY
> >> + bool "Touchscreen Overlay Objects"
> >> + help
> >> + Say Y here if you are using a touchscreen driver that supports
> >> + printed overlays with keys or a clipped touchscreen area.
> >> +
> >> + Should be selected by the touchscren drivers that support
> >> + this feature.
> >> +
> >> endif
> >> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> >> index 159cd5136fdb..f554826706ff 100644
> >> --- a/drivers/input/touchscreen/Makefile
> >> +++ b/drivers/input/touchscreen/Makefile
> >> @@ -117,3 +117,4 @@ obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW) += raspberrypi-ts.o
> >> obj-$(CONFIG_TOUCHSCREEN_IQS5XX) += iqs5xx.o
> >> obj-$(CONFIG_TOUCHSCREEN_ZINITIX) += zinitix.o
> >> obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX83112B) += himax_hx83112b.o
> >> +obj-$(CONFIG_TOUCHSCREEN_TS_OVERLAY) += ts-overlay.o
> >
> > It seems like this feature is useful for any two-dimensional touch surface
> > (e.g. trackpads) and not just touchscreens. For that reason, the touchscreen
> > helpers in touchscreen.c were moved out of input/touchscreen and into input/
> > such that they are not guarded by CONFIG_INPUT_TOUCHSCREEN. A growing number
> > of devices in input/misc are taking advantage of these.
> >
> > Therefore, I think this feature should follow suit and be available to any
> > input device as is the case for touchscreen.c. As written, the newly updated
> > binding is misleading because one may believe that any device that includes
> > touchscreen.yaml can define an overlay child, but the code does not currently
> > support this.
> >
> > To that end, it seems like touch-overlay would be a more descriptive name as
> > well. I understand that the name has changed once already, so hopefully this
> > feedback is not too annoying :)
> >
> changing names is no problem at all as long as it makes the feature more
> comprehensible and/or takes more suitable devices into account, which
> would be the case. So I will move the code from touchscreen to input and
> I will update the names and descriptions to make them more generic.
>
> I guess then I will need to move the properties to a separate binding
> for this feature because it will not be an addition to the touchscreen
> bindings anymore, right?
Technically this feature was never bound to touchscreen.yaml in the first place.
touchscreen.yaml defines scalar properties under a parent input device; your new
binding defines a child node and subsequent properties under the same, or another
parent input device.
That said, it is highly unlikely that one would use your feature without also
using the properties from touchscreen.yaml. It is perfectly fine in my opinion,
and perhaps more convenient, to define the overlay child in touchscreen.yaml as
you have done so that binding authors need not reference two files.
I do agree that it seems more natural for code living in input to be bound by
bindings in dt-bindings/input and not dt-bindings/input/touchscreen/, but there
was push back when the same question came up for touchscreen.yaml [1] some time
ago.
[1] https://patchwork.kernel.org/patch/12042037/
>
> >> diff --git a/drivers/input/touchscreen/ts-overlay.c b/drivers/input/touchscreen/ts-overlay.c
> >> new file mode 100644
> >> index 000000000000..7afa77d86c1f
> >> --- /dev/null
> >> +++ b/drivers/input/touchscreen/ts-overlay.c
> >> @@ -0,0 +1,356 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Helper functions for overlay objects on touchscreens
> >> + *
> >> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> >> + */
> >> +
> >> +#include <linux/property.h>
> >> +#include <linux/input.h>
> >> +#include <linux/input/mt.h>
> >> +#include <linux/module.h>
> >> +#include <linux/input/ts-overlay.h>
> >> +
> >> +enum ts_overlay_valid_objects {
> >> + TOUCHSCREEN,
> >> + BUTTON,
> >
> > Please namespace these, i.e. TOUCH_OVERLAY_*.
> >
> >> +};
> >> +
> >> +static const char *const ts_overlay_names[] = {
> >> + [TOUCHSCREEN] = "overlay-touchscreen",
> >
> > I'm a little confused why we need new code for this particular function; it's
> > what touchscreen-min-x/y and touchscreen-size-x/y were meant to define. Why
> > can't we keep using those?
> >
> According to the bindings, touchscreen-min-x/y define the minimum
> reported values, but the overlay-touchscreen is actually setting a new
> origin. Therefore I might be misusing those properties. On the other
> hand touchscreen-size-x/y would make more sense, but I also considered
> the case where someone would like to describe the real size of the
> touchscreen outside of the overlay node as well as the clipped size
> inside the node. In that case using the same property twice would be
> confusing.
> So in the end I thought that the origin/size properties are more precise
> and applicable for all objects and not only the overlay touchscreen.
> These properties are needed for the buttons anyways and in the future
> more overlay would use the same properties.
Ah, I understand now. touchscreen-min-x/y define the lower limits of the axes
reported to input but they don't move the origin. I'm aligned with the reason
to introduce this function.
This does beg the question as to whether we need two separate types of children
and related parsing code. Can we not simply have one overlay definition, and
make the decision as to whether we are dealing with a border or virtual button
based on whether 'linux,code' is present?
>
> >> + [BUTTON] = "overlay-buttons",
> >> +};
> >> +
> >> +struct ts_overlay_shape {
> >> + u32 x_origin;
> >> + u32 y_origin;
> >> + u32 x_size;
> >> + u32 y_size;
> >> +};
> >> +
> >> +struct ts_overlay_button {
> >> + struct ts_overlay_shape shape;
> >> + u32 key;
> >> + bool pressed;
> >> + int slot;
> >> +};
> >> +
> >> +static int ts_overlay_get_shape_properties(struct fwnode_handle *child_node,
> >> + struct ts_overlay_shape *shape)
> >> +{
> >> + int rc;
> >
> > In input, the common practice is to use 'error' for return values that are either
> > zero or negative. The reasoning is because the variable quite literally represents
> > an error, or lack thereof. And then:
> >
> > error = ...
> > if (error)
> > return error;
> >
> >> +
> >> + rc = fwnode_property_read_u32(child_node, "x-origin", &shape->x_origin);
> >> + if (rc < 0)
> >> + return rc;
> >
> > It seems like all of these properties are required; if so, please update the
> > binding to make this clear.
> >
> > If the binding is correct and these properties are in fact optional, then you
> > must evaluate fwnode_property_read_u32() against -EINVAL.
> >
> If I end up writing new bindings for this feature, it will be more clear
> what is required and what not because I will not be part only of the
> touchscreen anymore. These properties will be required.
The question of whether to split the overlay child definition from touchscreen.yaml
is orthogonal to this point. If the code fails in the absence of these properties,
then you must add a "required:" heading within the overlay child node definition to
call out these properties.
> >> +
> >> + rc = fwnode_property_read_u32(child_node, "y-origin", &shape->y_origin);
> >> + if (rc < 0)
> >> + return rc;
> >> +
> >> + rc = fwnode_property_read_u32(child_node, "x-size", &shape->x_size);
> >> + if (rc < 0)
> >> + return rc;
> >> +
> >> + rc = fwnode_property_read_u32(child_node, "y-size", &shape->y_size);
> >> + if (rc < 0)
> >> + return rc;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int ts_overlay_get_button_properties(struct device *dev,
> >> + struct fwnode_handle *child_node,
> >> + struct ts_overlay_button *btn)
> >> +{
> >> + struct fwnode_handle *child_btn;
> >> + int rc;
> >> + int j = 0;
> >> +
> >> + fwnode_for_each_child_node(child_node, child_btn) {
> >> + rc = ts_overlay_get_shape_properties(child_btn, &btn[j].shape);
> >> + if (rc < 0)
> >> + goto button_prop_cleanup;
> >> +
> >> + rc = fwnode_property_read_u32(child_btn, "linux,code",
> >> + &btn[j].key);
> >> + if (rc < 0)
> >> + goto button_prop_cleanup;
> >
> > The binding needs to list this property as required, too.
> >
> Do you mean "linux,code"? It is already listed with the same pattern
> that some other bindings use:
>
> linux,code: true
>
> Is that not right? I did not want to redefine an existing property that
> other bindings already make use of.
These are separate things. You must additionally list 'linux,code' under a
'required:' heading if the code fails without the property defined. If you
adopt the idea above to decide whether we are dealing with a border or button
based on the presence of this property, then it goes back to being optional
of course.
> >> +
> >> + dev_info(dev, "Added button at (%u, %u), size %ux%u, code=%u\n",
> >> + btn[j].shape.x_origin, btn[j].shape.y_origin,
> >> + btn[j].shape.x_size, btn[j].shape.y_size, btn[j].key);
> >
> > This seems like a dev_dbg() to me.
> >
> >> + j++;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +button_prop_cleanup:
> >> + fwnode_handle_put(child_btn);
> >> + return rc;
> >> +}
> >> +
> >> +void ts_overlay_set_button_caps(struct ts_overlay_map *map,
> >> + struct input_dev *dev)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i < map->button_count; i++)
> >> + input_set_capability(dev, EV_KEY, map->buttons[i].key);
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_set_button_caps);
> >
> > I don't see a need to expose this function and require participating drivers
> > to call it; we should just have one over-arching function for processing the
> > overlay(s), akin to touchscreen_parse_properties but for the button input
> > device in case the driver separates the button and touchscreen input devices.
> >
> > That one function would then be responsible for parsing the overlay(s) and
> > calling input_set_capability() on each button.
> >
> >> +
> >> +static int ts_overlay_count_buttons(struct device *dev)
> >> +{
> >> + struct fwnode_handle *child_node;
> >> + struct fwnode_handle *child_button;
> >
> > These names confused me; they're both children, but only the second is aptly
> > named. How about child_overlay and child_button, or perhaps overlay_node and
> > button_node?
> >
> >> + int count = 0;
> >> +
> >> + child_node = device_get_named_child_node(dev, ts_overlay_names[BUTTON]);
> >> + if (!child_node)
> >> + return 0;
> >> +
> >> + fwnode_for_each_child_node(child_node, child_button)
> >> + count++;
> >> + fwnode_handle_put(child_node);
> >> +
> >> + return count;
> >> +}
> >> +
> >> +static int ts_overlay_map_touchscreen(struct device *dev,
> >> + struct ts_overlay_map *map)
> >> +{
> >> + struct fwnode_handle *child;
> >
> > Same here; there are two layers of children, so please use more descriptive
> > variable names.
> >
> >> + int rc = 0;
> >> +
> >> + child = device_get_named_child_node(dev, ts_overlay_names[TOUCHSCREEN]);
> >> + if (!child)
> >> + goto touchscreen_ret;
> >
> > I don't think we need a label here; just return 0 directly.
> >
> >> +
> >> + map->touchscreen =
> >> + devm_kzalloc(dev, sizeof(*map->touchscreen), GFP_KERNEL);
> >> + if (!map->touchscreen) {
> >> + rc = -ENOMEM;
> >> + goto touchscreen_handle;
> >> + }
> >> + rc = ts_overlay_get_shape_properties(child, map->touchscreen);
> >> + if (rc < 0)
> >> + goto touchscreen_free;
> >> +
> >> + map->overlay_touchscreen = true;
> >> + dev_info(dev, "Added overlay touchscreen at (%u, %u), size %u x %u\n",
> >> + map->touchscreen->x_origin, map->touchscreen->y_origin,
> >> + map->touchscreen->x_size, map->touchscreen->y_size);
> >
> > dev_dbg()
> >
> >> +
> >> + rc = 0;
> >
> > rc (error) can only be zero if we have gotten this far; I don't see a need
> > to assign it here.
> >
> >> + goto touchscreen_handle;
> >
> > Please think about whether this can be reorganized to prevent jumping over
> > small bits of code; I found it hard to follow. Maybe one or more tasks can
> > be offloaded to a helper function?
> >
> >> +
> >> +touchscreen_free:
> >> + devm_kfree(dev, map->touchscreen);
> >
> > This set off a red flag; it's unclear that it's necessary. Regardless of
> > whether the participating driver is smart enough to bail during probe()
> > if the overlay parsing fails, or it happily continues, this memory will
> > get freed when the driver tied to 'dev' is torn down.
> >
> > Calling devm_kfree() is generally limited to special cases where you are
> > dynamically reallocating memory at runtime. In case I have misunderstood
> > the intent, please let me know.
> >
> Would devm_kfree() not free the memory immediately if the parsing fails,
> making it available for any process instead of waiting until the driver
> is torn down, which might never happen? Otherwise that chunk of memory
> will not be accessible even though it is useless because the operation
> failed, right? Or am I missing something?
If the participating driver does not immediately fail to probe (and hence
free all of its device-managed resources) upon failure to parse this or any
other required properties, that is a bug in that driver.
> >> +touchscreen_handle:
> >> + fwnode_handle_put(child);
> >> +touchscreen_ret:
> >> + return rc;
> >> +}
> >> +
> >> +static int ts_overlay_map_buttons(struct device *dev,
> >> + struct ts_overlay_map *map,
> >> + struct input_dev *input)
> >> +{
> >> + struct fwnode_handle *child;
> >> + u32 button_count;
> >> + int rc = 0;
> >> +
> >> + button_count = ts_overlay_count_buttons(dev);
> >> + if (button_count) {
> >> + map->buttons = devm_kcalloc(dev, button_count,
> >> + sizeof(*map->buttons), GFP_KERNEL);
> >> + if (!map->buttons) {
> >> + rc = -ENOMEM;
> >> + goto map_buttons_ret;
> >> + }
> >> + child = device_get_named_child_node(dev,
> >> + ts_overlay_names[BUTTON]);
> >> + if (unlikely(!child))
> >> + goto map_buttons_free;
> >> +
> >> + rc = ts_overlay_get_button_properties(dev, child, map->buttons);
> >> + if (rc < 0)
> >> + goto map_buttons_free;
> >> +
> >> + map->button_count = button_count;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +map_buttons_free:
> >> + devm_kfree(dev, map->buttons);
> >> +map_buttons_ret:
> >> + return rc;
> >> +}
> >> +
> >> +static bool ts_overlay_defined_objects(struct device *dev)
> >> +{
> >> + struct fwnode_handle *child;
> >> + int i;
> >> +
> >> + for (i = 0; i < ARRAY_SIZE(ts_overlay_names); i++) {
> >> + child = device_get_named_child_node(dev, ts_overlay_names[i]);
> >> + if (child) {
> >> + fwnode_handle_put(child);
> >> + return true;
> >> + }
> >> + fwnode_handle_put(child);
> >> + }
> >> +
> >> + return false;
> >> +}
> >> +
> >> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
> >> + struct input_dev *input)
> >> +{
> >> + struct ts_overlay_map *map = NULL;
> >> + int rc;
> >> +
> >> + if (!ts_overlay_defined_objects(dev))
> >> + return NULL;
> >> +
> >> + map = devm_kzalloc(dev, sizeof(*map), GFP_KERNEL);
> >> + if (!map) {
> >> + rc = -ENOMEM;
> >> + goto objects_err;
> >> + }
> >> + rc = ts_overlay_map_touchscreen(dev, map);
> >> + if (rc < 0)
> >> + goto objects_free;
> >> +
> >> + rc = ts_overlay_map_buttons(dev, map, input);
> >> + if (rc < 0)
> >> + goto objects_free;
> >> +
> >> + return map;
> >> +
> >> +objects_free:
> >> + devm_kfree(dev, map);
> >> +objects_err:
> >> + return ERR_PTR(rc);
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_map_objects);
> >> +
> >> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y)
> >> +{
> >> + *x = map->touchscreen->x_size - 1;
> >> + *y = map->touchscreen->y_size - 1;
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_get_touchscreen_abs);
> >> +
> >> +static bool ts_overlay_shape_event(struct ts_overlay_shape *shape, u32 x, u32 y)
> >> +{
> >> + if (!shape)
> >> + return false;
> >> +
> >> + if (x >= shape->x_origin && x < (shape->x_origin + shape->x_size) &&
> >> + y >= shape->y_origin && y < (shape->y_origin + shape->y_size))
> >> + return true;
> >> +
> >> + return false;
> >> +}
> >> +
> >> +static bool ts_overlay_touchscreen_event(struct ts_overlay_shape *touchscreen,
> >> + u32 *x, u32 *y)
> >> +{
> >> + if (ts_overlay_shape_event(touchscreen, *x, *y)) {
> >> + *x -= touchscreen->x_origin;
> >> + *y -= touchscreen->y_origin;
> >> + return true;
> >> + }
> >> +
> >> + return false;
> >> +}
> >> +
> >> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map)
> >> +{
> >> + if (!map || !map->overlay_touchscreen)
> >> + return false;
> >> +
> >> + return true;
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_mapped_touchscreen);
> >> +
> >> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map)
> >> +{
> >> + if (!map || !map->button_count)
> >> + return false;
> >> +
> >> + return true;
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_mapped_buttons);
> >> +
> >> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y)
> >> +{
> >> + if (!ts_overlay_mapped_touchscreen(map))
> >> + return true;
> >> +
> >> + if (!ts_overlay_touchscreen_event(map->touchscreen, x, y))
> >> + return false;
> >> +
> >> + return true;
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_mt_on_touchscreen);
> >> +
> >> +bool ts_overlay_button_press(struct ts_overlay_map *map,
> >> + struct input_dev *input, u32 x, u32 y, u32 slot)
> >> +{
> >> + int i;
> >> +
> >> + if (!ts_overlay_mapped_buttons(map))
> >> + return false;
> >> +
> >> + for (i = 0; i < map->button_count; i++) {
> >> + if (ts_overlay_shape_event(&map->buttons[i].shape, x, y)) {
> >> + input_report_key(input, map->buttons[i].key, 1);
> >> + map->buttons[i].pressed = true;
> >> + map->buttons[i].slot = slot;
> >> + return true;
> >> + }
> >> + }
> >> +
> >> + return false;
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_button_press);
> >
> > The level of abstraction here does not seem quite right. Rather than force
> > each participating driver to call a press and release function, I think it
> > is better to expose something like touch_overlay_process_buttons() which
> > then handles the press and release events internally.
> >
> > You're also relying on each individual driver to decide whether a touch
> > coordinate is inside or outside the overlay, and selectively call the press
> > and release functions OR report coordinates which is non-optimal.
> >
> > To me, this says we actually need one wrapper function that accepts handles
> > to both the touchscreen and button input devices (which may be the same at
> > the driver's discretion) as well as the coordinates. If the coordinate is
> > within an overlay area, handle press/release; if not, call touchscreen_report_pos().
> >
> >> +
> >> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot)
> >> +{
> >> + int i;
> >> +
> >> + if (!map || !map->button_count)
> >> + return false;
> >> +
> >> + for (i = 0; i < map->button_count; i++) {
> >> + if (map->buttons[i].pressed && map->buttons[i].slot == slot)
> >> + return true;
> >> + }
> >> +
> >> + return false;
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_is_button_slot);
> >> +
> >> +void ts_overlay_button_release(struct ts_overlay_map *map,
> >> + struct input_dev *input, u32 slot)
> >> +{
> >> + int i;
> >> +
> >> + if (!map || !map->button_count)
> >> + return;
> >> +
> >> + for (i = 0; i < map->button_count; i++) {
> >> + if (map->buttons[i].pressed && map->buttons[i].slot == slot) {
> >> + input_report_key(input, map->buttons[i].key, 0);
> >> + map->buttons[i].pressed = false;
> >> + }
> >> + }
> >> +}
> >> +EXPORT_SYMBOL(ts_overlay_button_release);
> >> +
> >> +MODULE_LICENSE("GPL");
> >> +MODULE_DESCRIPTION("Helper functions for overlay objects on touchscreens");
> >> diff --git a/include/linux/input/ts-overlay.h b/include/linux/input/ts-overlay.h
> >> new file mode 100644
> >> index 000000000000..b75df0dec3ab
> >> --- /dev/null
> >> +++ b/include/linux/input/ts-overlay.h
> >> @@ -0,0 +1,43 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +/*
> >> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> >> + */
> >> +
> >> +#ifndef _TS_OVERLAY
> >> +#define _TS_OVERLAY
> >> +
> >> +#include <linux/types.h>
> >> +
> >> +struct input_dev;
> >> +struct device;
> >> +
> >> +struct ts_overlay_map {
> >> + struct ts_overlay_shape *touchscreen;
> >> + bool overlay_touchscreen;
> >> + struct ts_overlay_button *buttons;
> >> + u32 button_count;
> >> +};
> >> +
> >> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
> >> + struct input_dev *input);
> >> +
> >> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y);
> >> +
> >> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map);
> >> +
> >> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map);
> >> +
> >> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y);
> >> +
> >> +bool ts_overlay_button_press(struct ts_overlay_map *map,
> >> + struct input_dev *input, u32 x, u32 y, u32 slot);
> >> +
> >> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot);
> >> +
> >> +void ts_overlay_button_release(struct ts_overlay_map *map,
> >> + struct input_dev *input, u32 slot);
> >> +
> >> +void ts_overlay_set_button_caps(struct ts_overlay_map *map,
> >> + struct input_dev *dev);
> >> +
> >> +#endif
> >>
> >> --
> >> 2.39.2
> >>
> >
> > Kind regards,
> > Jeff LaBundy
>
> Thanks again for your feedback, I really appreciate it. All the comments
> without a reply can be taken as acknowledged and accepted as they are
> without further discussion. I will work on them for the next version.
Sure thing! Thank you for your efforts.
>
> Thank you for your time and best regards,
> Javier Carrasco
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 4/4] input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-26 13:20 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <ZJmMJxXxLrC9Xevi@nixie71>
Hi,
On 26/06/2023 15:01, Jeff LaBundy wrote:
> Hi Neil,
>
> On Mon, Jun 26, 2023 at 09:02:16AM +0200, Neil Armstrong wrote:
>
> [...]
>
>>>> +static int goodix_berlin_spi_probe(struct spi_device *spi)
>>>> +{
>>>> + struct regmap_config *regmap_config;
>>>> + struct regmap *regmap;
>>>> + size_t max_size;
>>>> + int error = 0;
>>>> +
>>>> + regmap_config = devm_kmemdup(&spi->dev, &goodix_berlin_spi_regmap_conf,
>>>> + sizeof(*regmap_config), GFP_KERNEL);
>>>> + if (!regmap_config)
>>>> + return -ENOMEM;
>>>
>>> Is there any reason we cannot simply pass goodix_berlin_spi_regmap_conf to
>>> devm_regmap_init() below? Why to duplicate and pass the copy?
>>>
>>> For reference, BMP280 in IIO is a similar example of a device with regmap
>>> sitting atop a bespoke SPI protocol; it does not seem to take this extra
>>> step.
>>
>> The goodix_berlin_spi_regmap_conf copy is modified after with the correct
>> max raw read/write size, and I'm not a fan of modifying a global structure
>> that could be use for multiple probes, I can make a copy in a stack variable
>> if it feels simpler.
>
> Ah, that makes sense; in that case, the existing implementation seems fine
> to me. No changes necessary.
>
> Correct me if I'm wrong, but the stack variable method wouldn't work since
> that memory is gone after goodix_berlin_spi_probe() returns.
The config is only needed for the devm_regmap_init() duration, so keeping
the memory allocated for the whole lifetime of the device seems useless.
Neil
>
> Kind regards,
> Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: cap11xx - add advanced sensitivity settings
From: Jiri Valek - 2N @ 2023-06-26 13:16 UTC (permalink / raw)
To: Krzysztof Kozlowski, krzysztof.kozlowski+dt, dmitry.torokhov
Cc: devicetree, linux-input, linux-kernel, robh+dt, u.kleine-koenig
In-Reply-To: <a980885a-3379-9278-2652-1a1b68983709@linaro.org>
On 6/26/23 14:37, Krzysztof Kozlowski wrote:
> On 26/06/2023 13:37, Jiri Valek - 2N wrote:
>> Add support for advanced sensitivity settings that allows more precise
>> tunig of touch buttons. Input-treshold allows to set the sensitivity for
>> each channel separately. Also add signal guard feature for CAP129x chips.
>>
>> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> NAK. Stop adding fake entries.
>
> Best regards,
> Krzysztof
>
Sorry to you and also to Dmitry, I incorrectly used "Reviewed-by" tag.
I wanted to add Dmitry as reviewer and give him some credits.
I hope that "Reported-by" is better, but I'd rather put no tags.
BR
Jiri
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: microchip,cap11xx: add advanced sensitivity settings
From: Krzysztof Kozlowski @ 2023-06-26 13:16 UTC (permalink / raw)
To: Jiri Valek - 2N, krzysztof.kozlowski+dt, dmitry.torokhov
Cc: devicetree, linux-input, linux-kernel, robh+dt, u.kleine-koenig
In-Reply-To: <a6d6e4cb-b4d1-8ef3-31ce-2d09c9535dc1@axis.com>
On 26/06/2023 15:14, Jiri Valek - 2N wrote:
> On 6/26/23 14:41, Krzysztof Kozlowski wrote:
>> On 26/06/2023 14:36, Krzysztof Kozlowski wrote:
>>> On 26/06/2023 13:37, Jiri Valek - 2N wrote:
>>>> Add support for advanced sensitivity settings and signal guard feature.
>>>>
>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>
>>> What? How did you get it?!?
>>
>> To clarify - your previous patch was not working, not tested and not
>> correct. I pointed this out and I really wonder how from pointing out
>> errors, you figured out that I give your patch green light!
>>
>> Best regards,
>> Krzysztof
>>
> Sorry I incorrectly used "Reviewed-by" tag. I wanted to add you as reviewer
> and give you some credits. I hope that "Reported-by" is better, but I'd
> rather put no tags.
https://elixir.bootlin.com/linux/v6.4/source/Documentation/process/submitting-patches.rst#L500
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: microchip,cap11xx: add advanced sensitivity settings
From: Jiri Valek - 2N @ 2023-06-26 13:14 UTC (permalink / raw)
To: Krzysztof Kozlowski, krzysztof.kozlowski+dt, dmitry.torokhov
Cc: devicetree, linux-input, linux-kernel, robh+dt, u.kleine-koenig
In-Reply-To: <cd833975-7dd8-95a5-4bde-3dcdf9cf65a1@linaro.org>
On 6/26/23 14:41, Krzysztof Kozlowski wrote:
> On 26/06/2023 14:36, Krzysztof Kozlowski wrote:
>> On 26/06/2023 13:37, Jiri Valek - 2N wrote:
>>> Add support for advanced sensitivity settings and signal guard feature.
>>>
>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> What? How did you get it?!?
>
> To clarify - your previous patch was not working, not tested and not
> correct. I pointed this out and I really wonder how from pointing out
> errors, you figured out that I give your patch green light!
>
> Best regards,
> Krzysztof
>
Sorry I incorrectly used "Reviewed-by" tag. I wanted to add you as reviewer
and give you some credits. I hope that "Reported-by" is better, but I'd
rather put no tags.
BR
Jiri
^ permalink raw reply
* Re: [PATCH v3 4/4] input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
From: Jeff LaBundy @ 2023-06-26 13:01 UTC (permalink / raw)
To: Neil Armstrong
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <e36a697f-a54d-7bdf-1e18-38f72ec2966c@linaro.org>
Hi Neil,
On Mon, Jun 26, 2023 at 09:02:16AM +0200, Neil Armstrong wrote:
[...]
> > > +static int goodix_berlin_spi_probe(struct spi_device *spi)
> > > +{
> > > + struct regmap_config *regmap_config;
> > > + struct regmap *regmap;
> > > + size_t max_size;
> > > + int error = 0;
> > > +
> > > + regmap_config = devm_kmemdup(&spi->dev, &goodix_berlin_spi_regmap_conf,
> > > + sizeof(*regmap_config), GFP_KERNEL);
> > > + if (!regmap_config)
> > > + return -ENOMEM;
> >
> > Is there any reason we cannot simply pass goodix_berlin_spi_regmap_conf to
> > devm_regmap_init() below? Why to duplicate and pass the copy?
> >
> > For reference, BMP280 in IIO is a similar example of a device with regmap
> > sitting atop a bespoke SPI protocol; it does not seem to take this extra
> > step.
>
> The goodix_berlin_spi_regmap_conf copy is modified after with the correct
> max raw read/write size, and I'm not a fan of modifying a global structure
> that could be use for multiple probes, I can make a copy in a stack variable
> if it feels simpler.
Ah, that makes sense; in that case, the existing implementation seems fine
to me. No changes necessary.
Correct me if I'm wrong, but the stack variable method wouldn't work since
that memory is gone after goodix_berlin_spi_probe() returns.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH v3 2/2] Input: cap11xx - add advanced sensitivity settings
From: Jiri Valek - 2N @ 2023-06-26 13:00 UTC (permalink / raw)
To: krzysztof.kozlowski+dt, dmitry.torokhov
Cc: jiriv, devicetree, linux-input, linux-kernel, robh+dt,
u.kleine-koenig
In-Reply-To: <20230626130006.850254-1-jiriv@axis.com>
Add support for advanced sensitivity settings that allows more precise
tunig of touch buttons. Input-treshold allows to set the sensitivity for
each channel separately. Also add signal guard feature for CAP129x chips.
Signed-off-by: Jiri Valek - 2N <jiriv@axis.com>
---
drivers/input/keyboard/cap11xx.c | 250 ++++++++++++++++++++++++-------
1 file changed, 197 insertions(+), 53 deletions(-)
diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
index 040696d0e49c..9e5391da0d51 100644
--- a/drivers/input/keyboard/cap11xx.c
+++ b/drivers/input/keyboard/cap11xx.c
@@ -14,6 +14,7 @@
#include <linux/regmap.h>
#include <linux/i2c.h>
#include <linux/gpio/consumer.h>
+#include <linux/bitfield.h>
#define CAP11XX_REG_MAIN_CONTROL 0x00
#define CAP11XX_REG_MAIN_CONTROL_GAIN_SHIFT (6)
@@ -24,6 +25,7 @@
#define CAP11XX_REG_NOISE_FLAG_STATUS 0x0a
#define CAP11XX_REG_SENOR_DELTA(X) (0x10 + (X))
#define CAP11XX_REG_SENSITIVITY_CONTROL 0x1f
+#define CAP11XX_REG_SENSITIVITY_CONTROL_DELTA_SENSE_MASK 0x70
#define CAP11XX_REG_CONFIG 0x20
#define CAP11XX_REG_SENSOR_ENABLE 0x21
#define CAP11XX_REG_SENSOR_CONFIG 0x22
@@ -32,6 +34,7 @@
#define CAP11XX_REG_CALIBRATION 0x26
#define CAP11XX_REG_INT_ENABLE 0x27
#define CAP11XX_REG_REPEAT_RATE 0x28
+#define CAP11XX_REG_SIGNAL_GUARD_ENABLE 0x29
#define CAP11XX_REG_MT_CONFIG 0x2a
#define CAP11XX_REG_MT_PATTERN_CONFIG 0x2b
#define CAP11XX_REG_MT_PATTERN 0x2d
@@ -47,6 +50,8 @@
#define CAP11XX_REG_SENSOR_BASE_CNT(X) (0x50 + (X))
#define CAP11XX_REG_LED_POLARITY 0x73
#define CAP11XX_REG_LED_OUTPUT_CONTROL 0x74
+#define CAP11XX_REG_CALIB_SENSITIVITY_CONFIG 0x80
+#define CAP11XX_REG_CALIB_SENSITIVITY_CONFIG2 0x81
#define CAP11XX_REG_LED_DUTY_CYCLE_1 0x90
#define CAP11XX_REG_LED_DUTY_CYCLE_2 0x91
@@ -78,12 +83,20 @@ struct cap11xx_led {
struct cap11xx_priv {
struct regmap *regmap;
+ struct device *dev;
struct input_dev *idev;
+ const struct cap11xx_hw_model *model;
+ u8 id;
struct cap11xx_led *leds;
int num_leds;
/* config */
+ u8 analog_gain;
+ u8 sensitivity_delta_sense;
+ u8 signal_guard_inputs_mask;
+ u32 thresholds[8];
+ u32 calib_sensitivities[8];
u32 keycodes[];
};
@@ -181,6 +194,178 @@ static const struct regmap_config cap11xx_regmap_config = {
.volatile_reg = cap11xx_volatile_reg,
};
+static int
+cap11xx_write_calib_sens_config_1(struct cap11xx_priv *priv)
+{
+ return regmap_write(priv->regmap,
+ CAP11XX_REG_CALIB_SENSITIVITY_CONFIG,
+ (priv->calib_sensitivities[3] << 6) |
+ (priv->calib_sensitivities[2] << 4) |
+ (priv->calib_sensitivities[1] << 2) |
+ priv->calib_sensitivities[0]);
+}
+
+static int
+cap11xx_write_calib_sens_config_2(struct cap11xx_priv *priv)
+{
+ return regmap_write(priv->regmap,
+ CAP11XX_REG_CALIB_SENSITIVITY_CONFIG2,
+ (priv->calib_sensitivities[7] << 6) |
+ (priv->calib_sensitivities[6] << 4) |
+ (priv->calib_sensitivities[5] << 2) |
+ priv->calib_sensitivities[4]);
+}
+
+static int
+cap11xx_init_keys(struct cap11xx_priv *priv)
+{
+ struct device_node *node = priv->dev->of_node;
+ struct device *dev = priv->dev;
+ int i, error;
+ u32 u32_val;
+
+ if (!node) {
+ dev_err(dev, "Corresponding DT entry is not available\n");
+ return -ENODEV;
+ }
+
+ if (!of_property_read_u32(node, "microchip,sensor-gain", &u32_val)) {
+ if (priv->model->no_gain) {
+ dev_warn(dev,
+ "This model doesn't support 'sensor-gain'\n");
+ } else if (is_power_of_2(u32_val) && u32_val <= 8) {
+ priv->analog_gain = (u8)ilog2(u32_val);
+
+ error = regmap_update_bits(priv->regmap,
+ CAP11XX_REG_MAIN_CONTROL,
+ CAP11XX_REG_MAIN_CONTROL_GAIN_MASK,
+ priv->analog_gain << CAP11XX_REG_MAIN_CONTROL_GAIN_SHIFT);
+ if (error)
+ return error;
+ } else {
+ dev_err(dev, "Invalid sensor-gain value %u\n", u32_val);
+ return -EINVAL;
+ }
+ }
+
+ if (of_property_read_bool(node, "microchip,irq-active-high")) {
+ if (priv->id == CAP1106 ||
+ priv->id == CAP1126 ||
+ priv->id == CAP1188) {
+ error = regmap_update_bits(priv->regmap,
+ CAP11XX_REG_CONFIG2,
+ CAP11XX_REG_CONFIG2_ALT_POL,
+ 0);
+ if (error)
+ return error;
+ } else {
+ dev_warn(dev,
+ "This model doesn't support 'irq-active-high'\n");
+ }
+ }
+
+ if (!of_property_read_u32(node,
+ "microchip,sensitivity-delta-sense", &u32_val)) {
+ if (!is_power_of_2(u32_val) || u32_val > 128) {
+ dev_err(dev, "Invalid sensitivity-delta-sense value %u\n", u32_val);
+ return -EINVAL;
+ }
+
+ priv->sensitivity_delta_sense = (u8)ilog2(u32_val);
+ u32_val = ~(FIELD_PREP(CAP11XX_REG_SENSITIVITY_CONTROL_DELTA_SENSE_MASK,
+ priv->sensitivity_delta_sense));
+
+ error = regmap_update_bits(priv->regmap,
+ CAP11XX_REG_SENSITIVITY_CONTROL,
+ CAP11XX_REG_SENSITIVITY_CONTROL_DELTA_SENSE_MASK,
+ u32_val);
+ if (error)
+ return error;
+ }
+
+ if (!of_property_read_u32_array(node, "microchip,input-treshold",
+ priv->thresholds, priv->model->num_channels)) {
+ for (i = 0; i < priv->model->num_channels; i++) {
+ if (priv->thresholds[i] > 127) {
+ dev_err(dev, "Invalid input-treshold value %u\n",
+ priv->thresholds[i]);
+ return -EINVAL;
+ }
+
+ error = regmap_write(priv->regmap,
+ CAP11XX_REG_SENSOR_THRESH(i),
+ priv->thresholds[i]);
+ if (error)
+ return error;
+ }
+ }
+
+ if (!of_property_read_u32_array(node, "microchip,calib-sensitivity",
+ priv->calib_sensitivities, priv->model->num_channels)) {
+ if (priv->id == CAP1293 || priv->id == CAP1298) {
+ for (i = 0; i < priv->model->num_channels; i++) {
+ if (!is_power_of_2(priv->calib_sensitivities[i]) ||
+ priv->calib_sensitivities[i] > 4) {
+ dev_err(dev, "Invalid calib-sensitivity value %u\n",
+ priv->calib_sensitivities[i]);
+ return -EINVAL;
+ }
+ priv->calib_sensitivities[i] = ilog2(priv->calib_sensitivities[i]);
+ }
+
+ error = cap11xx_write_calib_sens_config_1(priv);
+ if (error)
+ return error;
+
+ if (priv->id == CAP1298) {
+ error = cap11xx_write_calib_sens_config_2(priv);
+ if (error)
+ return error;
+ }
+ } else {
+ dev_warn(dev,
+ "This model doesn't support 'calib-sensitivity'\n");
+ }
+ }
+
+ for (i = 0; i < priv->model->num_channels; i++) {
+ if (!of_property_read_u32_index(node, "microchip,signal-guard",
+ i, &u32_val)) {
+ if (u32_val > 1)
+ return -EINVAL;
+ if (u32_val)
+ priv->signal_guard_inputs_mask |= 0x01 << i;
+ }
+ }
+
+ if (priv->signal_guard_inputs_mask) {
+ if (priv->id == CAP1293 || priv->id == CAP1298) {
+ error = regmap_write(priv->regmap,
+ CAP11XX_REG_SIGNAL_GUARD_ENABLE,
+ priv->signal_guard_inputs_mask);
+ if (error)
+ return error;
+ } else {
+ dev_warn(dev,
+ "This model doesn't support 'signal-guard'\n");
+ }
+ }
+
+ /* Provide some useful defaults */
+ for (i = 0; i < priv->model->num_channels; i++)
+ priv->keycodes[i] = KEY_A + i;
+
+ of_property_read_u32_array(node, "linux,keycodes",
+ priv->keycodes, priv->model->num_channels);
+
+ /* Disable autorepeat. The Linux input system has its own handling. */
+ error = regmap_write(priv->regmap, CAP11XX_REG_REPEAT_RATE, 0);
+ if (error)
+ return error;
+
+ return 0;
+}
+
static irqreturn_t cap11xx_thread_func(int irq_num, void *data)
{
struct cap11xx_priv *priv = data;
@@ -332,11 +517,9 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
const struct i2c_device_id *id = i2c_client_get_device_id(i2c_client);
struct device *dev = &i2c_client->dev;
struct cap11xx_priv *priv;
- struct device_node *node;
const struct cap11xx_hw_model *cap;
- int i, error, irq, gain = 0;
+ int i, error, irq;
unsigned int val, rev;
- u32 gain32;
if (id->driver_data >= ARRAY_SIZE(cap11xx_devices)) {
dev_err(dev, "Invalid device ID %lu\n", id->driver_data);
@@ -355,6 +538,8 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
if (!priv)
return -ENOMEM;
+ priv->dev = dev;
+
priv->regmap = devm_regmap_init_i2c(i2c_client, &cap11xx_regmap_config);
if (IS_ERR(priv->regmap))
return PTR_ERR(priv->regmap);
@@ -384,50 +569,15 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
return error;
dev_info(dev, "CAP11XX detected, model %s, revision 0x%02x\n",
- id->name, rev);
- node = dev->of_node;
-
- if (!of_property_read_u32(node, "microchip,sensor-gain", &gain32)) {
- if (cap->no_gain)
- dev_warn(dev,
- "This version doesn't support sensor gain\n");
- else if (is_power_of_2(gain32) && gain32 <= 8)
- gain = ilog2(gain32);
- else
- dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
- }
+ id->name, rev);
- if (id->driver_data == CAP1106 ||
- id->driver_data == CAP1126 ||
- id->driver_data == CAP1188) {
- if (of_property_read_bool(node, "microchip,irq-active-high")) {
- error = regmap_update_bits(priv->regmap,
- CAP11XX_REG_CONFIG2,
- CAP11XX_REG_CONFIG2_ALT_POL,
- 0);
- if (error)
- return error;
- }
- }
-
- /* Provide some useful defaults */
- for (i = 0; i < cap->num_channels; i++)
- priv->keycodes[i] = KEY_A + i;
-
- of_property_read_u32_array(node, "linux,keycodes",
- priv->keycodes, cap->num_channels);
+ priv->model = cap;
+ priv->id = id->driver_data;
- if (!cap->no_gain) {
- error = regmap_update_bits(priv->regmap,
- CAP11XX_REG_MAIN_CONTROL,
- CAP11XX_REG_MAIN_CONTROL_GAIN_MASK,
- gain << CAP11XX_REG_MAIN_CONTROL_GAIN_SHIFT);
- if (error)
- return error;
- }
+ dev_info(dev, "CAP11XX device detected, model %s, revision 0x%02x\n",
+ id->name, rev);
- /* Disable autorepeat. The Linux input system has its own handling. */
- error = regmap_write(priv->regmap, CAP11XX_REG_REPEAT_RATE, 0);
+ error = cap11xx_init_keys(priv);
if (error)
return error;
@@ -439,7 +589,7 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
priv->idev->id.bustype = BUS_I2C;
priv->idev->evbit[0] = BIT_MASK(EV_KEY);
- if (of_property_read_bool(node, "autorepeat"))
+ if (of_property_read_bool(dev->of_node, "autorepeat"))
__set_bit(EV_REP, priv->idev->evbit);
for (i = 0; i < cap->num_channels; i++)
@@ -474,14 +624,8 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
if (error)
return error;
- irq = irq_of_parse_and_map(node, 0);
- if (!irq) {
- dev_err(dev, "Unable to parse or map IRQ\n");
- return -ENXIO;
- }
-
- error = devm_request_threaded_irq(dev, irq, NULL, cap11xx_thread_func,
- IRQF_ONESHOT, dev_name(dev), priv);
+ error = devm_request_threaded_irq(dev, i2c_client->irq, NULL,
+ cap11xx_thread_func, IRQF_ONESHOT, dev_name(dev), priv);
if (error)
return error;
--
2.25.1
^ permalink raw reply related
* [PATCH v3 1/2] dt-bindings: input: microchip,cap11xx: add advanced sensitivity settings
From: Jiri Valek - 2N @ 2023-06-26 13:00 UTC (permalink / raw)
To: krzysztof.kozlowski+dt, dmitry.torokhov
Cc: jiriv, devicetree, linux-input, linux-kernel, robh+dt,
u.kleine-koenig
In-Reply-To: <20230626130006.850254-1-jiriv@axis.com>
Add support for advanced sensitivity settings and signal guard feature.
Signed-off-by: Jiri Valek - 2N <jiriv@axis.com>
---
.../bindings/input/microchip,cap11xx.yaml | 77 +++++++++++++++++--
1 file changed, 72 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
index 5fa625b5c5fb..b69dac1fba0e 100644
--- a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
+++ b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
@@ -45,13 +45,13 @@ properties:
Enables the Linux input system's autorepeat feature on the input device.
linux,keycodes:
- minItems: 6
- maxItems: 6
+ minItems: 3
+ maxItems: 8
description: |
Specifies an array of numeric keycode values to
be used for the channels. If this property is
omitted, KEY_A, KEY_B, etc are used as defaults.
- The array must have exactly six entries.
+ The number of entries must correspond to the number of channels.
microchip,sensor-gain:
$ref: /schemas/types.yaml#/definitions/uint32
@@ -70,6 +70,55 @@ properties:
open drain. This property allows using the active
high push-pull output.
+ microchip,sensitivity-delta-sense:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 32
+ enum: [1, 2, 4, 8, 16, 32, 64, 128]
+ description:
+ Optional parameter. Controls the sensitivity multiplier of a touch detection.
+ At the more sensitive settings, touches are detected for a smaller delta
+ capacitance corresponding to a “lighter” touch.
+
+ microchip,signal-guard:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 3
+ maxItems: 8
+ items:
+ minimum: 0
+ maximum: 1
+ description: |
+ Optional parameter supported only for CAP129x.
+ 0 - off
+ 1 - on
+ The signal guard isolates the signal from virtual grounds.
+ If enabled then the behavior of the channel is changed to signal guard.
+ The number of entries must correspond to the number of channels.
+
+ microchip,input-treshold:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 3
+ maxItems: 8
+ items:
+ minimum: 0
+ maximum: 127
+ description:
+ Optional parameter. Specifies the delta threshold that is used to
+ determine if a touch has been detected.
+ The number of entries must correspond to the number of channels.
+
+ microchip,calib-sensitivity:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 3
+ maxItems: 8
+ items:
+ minimum: 1
+ maximum: 4
+ description:
+ Optional parameter supported only for CAP129x. Specifies an array of
+ numeric values that controls the gain used by the calibration routine to
+ enable sensor inputs to be more sensitive for proximity detection.
+ The number of entries must correspond to the number of channels.
+
patternProperties:
"^led@[0-7]$":
type: object
@@ -98,10 +147,23 @@ allOf:
compatible:
contains:
enum:
- - microchip,cap1106
+ - microchip,cap1188
then:
patternProperties:
- "^led@[0-7]$": false
+ "^led@[0-7]$": true
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - microchip,cap1293
+ - microchip,cap1298
+
+ then:
+ properties:
+ microchip,signal-guard: true
+ microchip,calib-sensitivity: true
required:
- compatible
@@ -122,6 +184,11 @@ examples:
reg = <0x28>;
autorepeat;
microchip,sensor-gain = <2>;
+ microchip,sensitivity-delta-sense = <16>;
+
+ microchip,signal-guard = <0>, <0>, <0>, <0>, <0>, <0>;
+ microchip,input-treshold = <21>, <18>, <46>, <46>, <46>, <21>;
+ microchip,calib-sensitivity = <1>, <2>, <2>, <1>, <1>, <2>;
linux,keycodes = <103>, /* KEY_UP */
<106>, /* KEY_RIGHT */
--
2.25.1
^ permalink raw reply related
* [PATCH v3 0/2] Input: cap11xx add advanced sensitivity settings
From: Jiri Valek - 2N @ 2023-06-26 13:00 UTC (permalink / raw)
To: krzysztof.kozlowski+dt, dmitry.torokhov
Cc: jiriv, devicetree, linux-input, linux-kernel, robh+dt,
u.kleine-koenig
PATCH 1 - add documentation for new options
PATCH 2 - add support for advanced settings into driver
Changes in v2:
- Removed "sensitivity-base-shift" parameter (not HW propertie) in PATCH 2.
- Used IRQ from I2C subsystem instead of parsing it again.
- Fixed some documentation issues in PATCH 1
Changes in v3:
- Remove incorrectly used "Reviewed-by" tag in PATCH 1 and 2
Jiri Valek - 2N (2):
dt-bindings: input: microchip,cap11xx: add advanced sensitivity
settings
Input: cap11xx - add advanced sensitivity settings
.../bindings/input/microchip,cap11xx.yaml | 77 +++++-
drivers/input/keyboard/cap11xx.c | 250 ++++++++++++++----
2 files changed, 269 insertions(+), 58 deletions(-)
--
2.25.1
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: microchip,cap11xx: add advanced sensitivity settings
From: Krzysztof Kozlowski @ 2023-06-26 12:41 UTC (permalink / raw)
To: Jiri Valek - 2N, krzysztof.kozlowski+dt, dmitry.torokhov
Cc: devicetree, linux-input, linux-kernel, robh+dt, u.kleine-koenig
In-Reply-To: <0503b8fd-e8ce-ffda-577a-b851a9eebb07@linaro.org>
On 26/06/2023 14:36, Krzysztof Kozlowski wrote:
> On 26/06/2023 13:37, Jiri Valek - 2N wrote:
>> Add support for advanced sensitivity settings and signal guard feature.
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> What? How did you get it?!?
To clarify - your previous patch was not working, not tested and not
correct. I pointed this out and I really wonder how from pointing out
errors, you figured out that I give your patch green light!
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: cap11xx - add advanced sensitivity settings
From: Krzysztof Kozlowski @ 2023-06-26 12:37 UTC (permalink / raw)
To: Jiri Valek - 2N, krzysztof.kozlowski+dt, dmitry.torokhov
Cc: devicetree, linux-input, linux-kernel, robh+dt, u.kleine-koenig
In-Reply-To: <20230626113740.809871-3-jiriv@axis.com>
On 26/06/2023 13:37, Jiri Valek - 2N wrote:
> Add support for advanced sensitivity settings that allows more precise
> tunig of touch buttons. Input-treshold allows to set the sensitivity for
> each channel separately. Also add signal guard feature for CAP129x chips.
>
> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
NAK. Stop adding fake entries.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: microchip,cap11xx: add advanced sensitivity settings
From: Krzysztof Kozlowski @ 2023-06-26 12:36 UTC (permalink / raw)
To: Jiri Valek - 2N, krzysztof.kozlowski+dt, dmitry.torokhov
Cc: devicetree, linux-input, linux-kernel, robh+dt, u.kleine-koenig
In-Reply-To: <20230626113740.809871-2-jiriv@axis.com>
On 26/06/2023 13:37, Jiri Valek - 2N wrote:
> Add support for advanced sensitivity settings and signal guard feature.
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
What? How did you get it?!?
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v2 1/2] dt-bindings: input: microchip,cap11xx: add advanced sensitivity settings
From: Jiri Valek - 2N @ 2023-06-26 11:37 UTC (permalink / raw)
To: krzysztof.kozlowski+dt, dmitry.torokhov
Cc: jiriv, devicetree, linux-input, linux-kernel, robh+dt,
u.kleine-koenig, Krzysztof Kozlowski
In-Reply-To: <20230626113740.809871-1-jiriv@axis.com>
Add support for advanced sensitivity settings and signal guard feature.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Jiri Valek - 2N <jiriv@axis.com>
---
.../bindings/input/microchip,cap11xx.yaml | 77 +++++++++++++++++--
1 file changed, 72 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
index 5fa625b5c5fb..b69dac1fba0e 100644
--- a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
+++ b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
@@ -45,13 +45,13 @@ properties:
Enables the Linux input system's autorepeat feature on the input device.
linux,keycodes:
- minItems: 6
- maxItems: 6
+ minItems: 3
+ maxItems: 8
description: |
Specifies an array of numeric keycode values to
be used for the channels. If this property is
omitted, KEY_A, KEY_B, etc are used as defaults.
- The array must have exactly six entries.
+ The number of entries must correspond to the number of channels.
microchip,sensor-gain:
$ref: /schemas/types.yaml#/definitions/uint32
@@ -70,6 +70,55 @@ properties:
open drain. This property allows using the active
high push-pull output.
+ microchip,sensitivity-delta-sense:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 32
+ enum: [1, 2, 4, 8, 16, 32, 64, 128]
+ description:
+ Optional parameter. Controls the sensitivity multiplier of a touch detection.
+ At the more sensitive settings, touches are detected for a smaller delta
+ capacitance corresponding to a “lighter” touch.
+
+ microchip,signal-guard:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 3
+ maxItems: 8
+ items:
+ minimum: 0
+ maximum: 1
+ description: |
+ Optional parameter supported only for CAP129x.
+ 0 - off
+ 1 - on
+ The signal guard isolates the signal from virtual grounds.
+ If enabled then the behavior of the channel is changed to signal guard.
+ The number of entries must correspond to the number of channels.
+
+ microchip,input-treshold:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 3
+ maxItems: 8
+ items:
+ minimum: 0
+ maximum: 127
+ description:
+ Optional parameter. Specifies the delta threshold that is used to
+ determine if a touch has been detected.
+ The number of entries must correspond to the number of channels.
+
+ microchip,calib-sensitivity:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 3
+ maxItems: 8
+ items:
+ minimum: 1
+ maximum: 4
+ description:
+ Optional parameter supported only for CAP129x. Specifies an array of
+ numeric values that controls the gain used by the calibration routine to
+ enable sensor inputs to be more sensitive for proximity detection.
+ The number of entries must correspond to the number of channels.
+
patternProperties:
"^led@[0-7]$":
type: object
@@ -98,10 +147,23 @@ allOf:
compatible:
contains:
enum:
- - microchip,cap1106
+ - microchip,cap1188
then:
patternProperties:
- "^led@[0-7]$": false
+ "^led@[0-7]$": true
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - microchip,cap1293
+ - microchip,cap1298
+
+ then:
+ properties:
+ microchip,signal-guard: true
+ microchip,calib-sensitivity: true
required:
- compatible
@@ -122,6 +184,11 @@ examples:
reg = <0x28>;
autorepeat;
microchip,sensor-gain = <2>;
+ microchip,sensitivity-delta-sense = <16>;
+
+ microchip,signal-guard = <0>, <0>, <0>, <0>, <0>, <0>;
+ microchip,input-treshold = <21>, <18>, <46>, <46>, <46>, <21>;
+ microchip,calib-sensitivity = <1>, <2>, <2>, <1>, <1>, <2>;
linux,keycodes = <103>, /* KEY_UP */
<106>, /* KEY_RIGHT */
--
2.25.1
^ permalink raw reply related
* [PATCH v2 2/2] Input: cap11xx - add advanced sensitivity settings
From: Jiri Valek - 2N @ 2023-06-26 11:37 UTC (permalink / raw)
To: krzysztof.kozlowski+dt, dmitry.torokhov
Cc: jiriv, devicetree, linux-input, linux-kernel, robh+dt,
u.kleine-koenig
In-Reply-To: <20230626113740.809871-1-jiriv@axis.com>
Add support for advanced sensitivity settings that allows more precise
tunig of touch buttons. Input-treshold allows to set the sensitivity for
each channel separately. Also add signal guard feature for CAP129x chips.
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Jiri Valek - 2N <jiriv@axis.com>
---
drivers/input/keyboard/cap11xx.c | 250 ++++++++++++++++++++++++-------
1 file changed, 197 insertions(+), 53 deletions(-)
diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
index 040696d0e49c..9e5391da0d51 100644
--- a/drivers/input/keyboard/cap11xx.c
+++ b/drivers/input/keyboard/cap11xx.c
@@ -14,6 +14,7 @@
#include <linux/regmap.h>
#include <linux/i2c.h>
#include <linux/gpio/consumer.h>
+#include <linux/bitfield.h>
#define CAP11XX_REG_MAIN_CONTROL 0x00
#define CAP11XX_REG_MAIN_CONTROL_GAIN_SHIFT (6)
@@ -24,6 +25,7 @@
#define CAP11XX_REG_NOISE_FLAG_STATUS 0x0a
#define CAP11XX_REG_SENOR_DELTA(X) (0x10 + (X))
#define CAP11XX_REG_SENSITIVITY_CONTROL 0x1f
+#define CAP11XX_REG_SENSITIVITY_CONTROL_DELTA_SENSE_MASK 0x70
#define CAP11XX_REG_CONFIG 0x20
#define CAP11XX_REG_SENSOR_ENABLE 0x21
#define CAP11XX_REG_SENSOR_CONFIG 0x22
@@ -32,6 +34,7 @@
#define CAP11XX_REG_CALIBRATION 0x26
#define CAP11XX_REG_INT_ENABLE 0x27
#define CAP11XX_REG_REPEAT_RATE 0x28
+#define CAP11XX_REG_SIGNAL_GUARD_ENABLE 0x29
#define CAP11XX_REG_MT_CONFIG 0x2a
#define CAP11XX_REG_MT_PATTERN_CONFIG 0x2b
#define CAP11XX_REG_MT_PATTERN 0x2d
@@ -47,6 +50,8 @@
#define CAP11XX_REG_SENSOR_BASE_CNT(X) (0x50 + (X))
#define CAP11XX_REG_LED_POLARITY 0x73
#define CAP11XX_REG_LED_OUTPUT_CONTROL 0x74
+#define CAP11XX_REG_CALIB_SENSITIVITY_CONFIG 0x80
+#define CAP11XX_REG_CALIB_SENSITIVITY_CONFIG2 0x81
#define CAP11XX_REG_LED_DUTY_CYCLE_1 0x90
#define CAP11XX_REG_LED_DUTY_CYCLE_2 0x91
@@ -78,12 +83,20 @@ struct cap11xx_led {
struct cap11xx_priv {
struct regmap *regmap;
+ struct device *dev;
struct input_dev *idev;
+ const struct cap11xx_hw_model *model;
+ u8 id;
struct cap11xx_led *leds;
int num_leds;
/* config */
+ u8 analog_gain;
+ u8 sensitivity_delta_sense;
+ u8 signal_guard_inputs_mask;
+ u32 thresholds[8];
+ u32 calib_sensitivities[8];
u32 keycodes[];
};
@@ -181,6 +194,178 @@ static const struct regmap_config cap11xx_regmap_config = {
.volatile_reg = cap11xx_volatile_reg,
};
+static int
+cap11xx_write_calib_sens_config_1(struct cap11xx_priv *priv)
+{
+ return regmap_write(priv->regmap,
+ CAP11XX_REG_CALIB_SENSITIVITY_CONFIG,
+ (priv->calib_sensitivities[3] << 6) |
+ (priv->calib_sensitivities[2] << 4) |
+ (priv->calib_sensitivities[1] << 2) |
+ priv->calib_sensitivities[0]);
+}
+
+static int
+cap11xx_write_calib_sens_config_2(struct cap11xx_priv *priv)
+{
+ return regmap_write(priv->regmap,
+ CAP11XX_REG_CALIB_SENSITIVITY_CONFIG2,
+ (priv->calib_sensitivities[7] << 6) |
+ (priv->calib_sensitivities[6] << 4) |
+ (priv->calib_sensitivities[5] << 2) |
+ priv->calib_sensitivities[4]);
+}
+
+static int
+cap11xx_init_keys(struct cap11xx_priv *priv)
+{
+ struct device_node *node = priv->dev->of_node;
+ struct device *dev = priv->dev;
+ int i, error;
+ u32 u32_val;
+
+ if (!node) {
+ dev_err(dev, "Corresponding DT entry is not available\n");
+ return -ENODEV;
+ }
+
+ if (!of_property_read_u32(node, "microchip,sensor-gain", &u32_val)) {
+ if (priv->model->no_gain) {
+ dev_warn(dev,
+ "This model doesn't support 'sensor-gain'\n");
+ } else if (is_power_of_2(u32_val) && u32_val <= 8) {
+ priv->analog_gain = (u8)ilog2(u32_val);
+
+ error = regmap_update_bits(priv->regmap,
+ CAP11XX_REG_MAIN_CONTROL,
+ CAP11XX_REG_MAIN_CONTROL_GAIN_MASK,
+ priv->analog_gain << CAP11XX_REG_MAIN_CONTROL_GAIN_SHIFT);
+ if (error)
+ return error;
+ } else {
+ dev_err(dev, "Invalid sensor-gain value %u\n", u32_val);
+ return -EINVAL;
+ }
+ }
+
+ if (of_property_read_bool(node, "microchip,irq-active-high")) {
+ if (priv->id == CAP1106 ||
+ priv->id == CAP1126 ||
+ priv->id == CAP1188) {
+ error = regmap_update_bits(priv->regmap,
+ CAP11XX_REG_CONFIG2,
+ CAP11XX_REG_CONFIG2_ALT_POL,
+ 0);
+ if (error)
+ return error;
+ } else {
+ dev_warn(dev,
+ "This model doesn't support 'irq-active-high'\n");
+ }
+ }
+
+ if (!of_property_read_u32(node,
+ "microchip,sensitivity-delta-sense", &u32_val)) {
+ if (!is_power_of_2(u32_val) || u32_val > 128) {
+ dev_err(dev, "Invalid sensitivity-delta-sense value %u\n", u32_val);
+ return -EINVAL;
+ }
+
+ priv->sensitivity_delta_sense = (u8)ilog2(u32_val);
+ u32_val = ~(FIELD_PREP(CAP11XX_REG_SENSITIVITY_CONTROL_DELTA_SENSE_MASK,
+ priv->sensitivity_delta_sense));
+
+ error = regmap_update_bits(priv->regmap,
+ CAP11XX_REG_SENSITIVITY_CONTROL,
+ CAP11XX_REG_SENSITIVITY_CONTROL_DELTA_SENSE_MASK,
+ u32_val);
+ if (error)
+ return error;
+ }
+
+ if (!of_property_read_u32_array(node, "microchip,input-treshold",
+ priv->thresholds, priv->model->num_channels)) {
+ for (i = 0; i < priv->model->num_channels; i++) {
+ if (priv->thresholds[i] > 127) {
+ dev_err(dev, "Invalid input-treshold value %u\n",
+ priv->thresholds[i]);
+ return -EINVAL;
+ }
+
+ error = regmap_write(priv->regmap,
+ CAP11XX_REG_SENSOR_THRESH(i),
+ priv->thresholds[i]);
+ if (error)
+ return error;
+ }
+ }
+
+ if (!of_property_read_u32_array(node, "microchip,calib-sensitivity",
+ priv->calib_sensitivities, priv->model->num_channels)) {
+ if (priv->id == CAP1293 || priv->id == CAP1298) {
+ for (i = 0; i < priv->model->num_channels; i++) {
+ if (!is_power_of_2(priv->calib_sensitivities[i]) ||
+ priv->calib_sensitivities[i] > 4) {
+ dev_err(dev, "Invalid calib-sensitivity value %u\n",
+ priv->calib_sensitivities[i]);
+ return -EINVAL;
+ }
+ priv->calib_sensitivities[i] = ilog2(priv->calib_sensitivities[i]);
+ }
+
+ error = cap11xx_write_calib_sens_config_1(priv);
+ if (error)
+ return error;
+
+ if (priv->id == CAP1298) {
+ error = cap11xx_write_calib_sens_config_2(priv);
+ if (error)
+ return error;
+ }
+ } else {
+ dev_warn(dev,
+ "This model doesn't support 'calib-sensitivity'\n");
+ }
+ }
+
+ for (i = 0; i < priv->model->num_channels; i++) {
+ if (!of_property_read_u32_index(node, "microchip,signal-guard",
+ i, &u32_val)) {
+ if (u32_val > 1)
+ return -EINVAL;
+ if (u32_val)
+ priv->signal_guard_inputs_mask |= 0x01 << i;
+ }
+ }
+
+ if (priv->signal_guard_inputs_mask) {
+ if (priv->id == CAP1293 || priv->id == CAP1298) {
+ error = regmap_write(priv->regmap,
+ CAP11XX_REG_SIGNAL_GUARD_ENABLE,
+ priv->signal_guard_inputs_mask);
+ if (error)
+ return error;
+ } else {
+ dev_warn(dev,
+ "This model doesn't support 'signal-guard'\n");
+ }
+ }
+
+ /* Provide some useful defaults */
+ for (i = 0; i < priv->model->num_channels; i++)
+ priv->keycodes[i] = KEY_A + i;
+
+ of_property_read_u32_array(node, "linux,keycodes",
+ priv->keycodes, priv->model->num_channels);
+
+ /* Disable autorepeat. The Linux input system has its own handling. */
+ error = regmap_write(priv->regmap, CAP11XX_REG_REPEAT_RATE, 0);
+ if (error)
+ return error;
+
+ return 0;
+}
+
static irqreturn_t cap11xx_thread_func(int irq_num, void *data)
{
struct cap11xx_priv *priv = data;
@@ -332,11 +517,9 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
const struct i2c_device_id *id = i2c_client_get_device_id(i2c_client);
struct device *dev = &i2c_client->dev;
struct cap11xx_priv *priv;
- struct device_node *node;
const struct cap11xx_hw_model *cap;
- int i, error, irq, gain = 0;
+ int i, error, irq;
unsigned int val, rev;
- u32 gain32;
if (id->driver_data >= ARRAY_SIZE(cap11xx_devices)) {
dev_err(dev, "Invalid device ID %lu\n", id->driver_data);
@@ -355,6 +538,8 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
if (!priv)
return -ENOMEM;
+ priv->dev = dev;
+
priv->regmap = devm_regmap_init_i2c(i2c_client, &cap11xx_regmap_config);
if (IS_ERR(priv->regmap))
return PTR_ERR(priv->regmap);
@@ -384,50 +569,15 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
return error;
dev_info(dev, "CAP11XX detected, model %s, revision 0x%02x\n",
- id->name, rev);
- node = dev->of_node;
-
- if (!of_property_read_u32(node, "microchip,sensor-gain", &gain32)) {
- if (cap->no_gain)
- dev_warn(dev,
- "This version doesn't support sensor gain\n");
- else if (is_power_of_2(gain32) && gain32 <= 8)
- gain = ilog2(gain32);
- else
- dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
- }
+ id->name, rev);
- if (id->driver_data == CAP1106 ||
- id->driver_data == CAP1126 ||
- id->driver_data == CAP1188) {
- if (of_property_read_bool(node, "microchip,irq-active-high")) {
- error = regmap_update_bits(priv->regmap,
- CAP11XX_REG_CONFIG2,
- CAP11XX_REG_CONFIG2_ALT_POL,
- 0);
- if (error)
- return error;
- }
- }
-
- /* Provide some useful defaults */
- for (i = 0; i < cap->num_channels; i++)
- priv->keycodes[i] = KEY_A + i;
-
- of_property_read_u32_array(node, "linux,keycodes",
- priv->keycodes, cap->num_channels);
+ priv->model = cap;
+ priv->id = id->driver_data;
- if (!cap->no_gain) {
- error = regmap_update_bits(priv->regmap,
- CAP11XX_REG_MAIN_CONTROL,
- CAP11XX_REG_MAIN_CONTROL_GAIN_MASK,
- gain << CAP11XX_REG_MAIN_CONTROL_GAIN_SHIFT);
- if (error)
- return error;
- }
+ dev_info(dev, "CAP11XX device detected, model %s, revision 0x%02x\n",
+ id->name, rev);
- /* Disable autorepeat. The Linux input system has its own handling. */
- error = regmap_write(priv->regmap, CAP11XX_REG_REPEAT_RATE, 0);
+ error = cap11xx_init_keys(priv);
if (error)
return error;
@@ -439,7 +589,7 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
priv->idev->id.bustype = BUS_I2C;
priv->idev->evbit[0] = BIT_MASK(EV_KEY);
- if (of_property_read_bool(node, "autorepeat"))
+ if (of_property_read_bool(dev->of_node, "autorepeat"))
__set_bit(EV_REP, priv->idev->evbit);
for (i = 0; i < cap->num_channels; i++)
@@ -474,14 +624,8 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
if (error)
return error;
- irq = irq_of_parse_and_map(node, 0);
- if (!irq) {
- dev_err(dev, "Unable to parse or map IRQ\n");
- return -ENXIO;
- }
-
- error = devm_request_threaded_irq(dev, irq, NULL, cap11xx_thread_func,
- IRQF_ONESHOT, dev_name(dev), priv);
+ error = devm_request_threaded_irq(dev, i2c_client->irq, NULL,
+ cap11xx_thread_func, IRQF_ONESHOT, dev_name(dev), priv);
if (error)
return error;
--
2.25.1
^ permalink raw reply related
* [PATCH v2 0/2] Input: cap11xx add advanced sensitivity settings
From: Jiri Valek - 2N @ 2023-06-26 11:37 UTC (permalink / raw)
To: krzysztof.kozlowski+dt, dmitry.torokhov
Cc: jiriv, devicetree, linux-input, linux-kernel, robh+dt,
u.kleine-koenig
PATCH 1 - add documentation for new options
PATCH 2 - add support for advanced settings into driver
Changes in v2:
- Removed "sensitivity-base-shift" parameter (not HW propertie) in PATCH 2.
- Used IRQ from I2C subsystem instead of parsing it again.
- Fixed some documentation issues in PATCH 1
Jiri Valek - 2N (2):
dt-bindings: input: microchip,cap11xx: add advanced sensitivity
settings
Input: cap11xx - add advanced sensitivity settings
.../bindings/input/microchip,cap11xx.yaml | 77 +++++-
drivers/input/keyboard/cap11xx.c | 250 ++++++++++++++----
2 files changed, 269 insertions(+), 58 deletions(-)
--
2.25.1
^ permalink raw reply
* Re: [PATCH v3 1/4] Input: ts-overlay - Add touchscreen overlay object handling
From: Javier Carrasco @ 2023-06-26 10:31 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch, linux-kernel,
linux-input, devicetree
In-Reply-To: <ZJj5VcHvfMMWMRx8@nixie71>
Hi Jeff,
On 26.06.23 04:35, Jeff LaBundy wrote:
> Hi Javier,
>
> On Fri, Jun 16, 2023 at 09:28:51AM +0200, Javier Carrasco wrote:
>> Some touchscreens provide mechanical overlays with different objects
>> like buttons or clipped touchscreen surfaces.
>>
>> In order to support these objects, add a series of helper functions
>> to the input subsystem to transform them into overlay objects via
>> device tree nodes.
>>
>> These overlay objects consume the raw touch events and report the
>> expected input events depending on the object properties.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
>> ---
>
> Excellent work; it's great to see this series move along.
>
>> MAINTAINERS | 7 +
>> drivers/input/touchscreen/Kconfig | 9 +
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/ts-overlay.c | 356 +++++++++++++++++++++++++++++++++
>> include/linux/input/ts-overlay.h | 43 ++++
>> 5 files changed, 416 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7e0b87d5aa2e..db9427926a4c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -21434,6 +21434,13 @@ W: https://github.com/srcres258/linux-doc
>> T: git git://github.com/srcres258/linux-doc.git doc-zh-tw
>> F: Documentation/translations/zh_TW/
>>
>> +TOUCHSCREEN OVERLAY OBJECTS
>> +M: Javier Carrasco <javier.carrasco@wolfvision.net>
>> +L: linux-input@vger.kernel.org
>> +S: Maintained
>> +F: drivers/input/touchscreen/ts-overlay.c
>> +F: include/linux/input/ts-overlay.h
>> +
>> TTY LAYER
>> M: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> M: Jiri Slaby <jirislaby@kernel.org>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 143ff43c67ae..8382a4d68326 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -1388,4 +1388,13 @@ config TOUCHSCREEN_HIMAX_HX83112B
>> To compile this driver as a module, choose M here: the
>> module will be called himax_hx83112b.
>>
>> +config TOUCHSCREEN_TS_OVERLAY
>> + bool "Touchscreen Overlay Objects"
>> + help
>> + Say Y here if you are using a touchscreen driver that supports
>> + printed overlays with keys or a clipped touchscreen area.
>> +
>> + Should be selected by the touchscren drivers that support
>> + this feature.
>> +
>> endif
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 159cd5136fdb..f554826706ff 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -117,3 +117,4 @@ obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW) += raspberrypi-ts.o
>> obj-$(CONFIG_TOUCHSCREEN_IQS5XX) += iqs5xx.o
>> obj-$(CONFIG_TOUCHSCREEN_ZINITIX) += zinitix.o
>> obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX83112B) += himax_hx83112b.o
>> +obj-$(CONFIG_TOUCHSCREEN_TS_OVERLAY) += ts-overlay.o
>
> It seems like this feature is useful for any two-dimensional touch surface
> (e.g. trackpads) and not just touchscreens. For that reason, the touchscreen
> helpers in touchscreen.c were moved out of input/touchscreen and into input/
> such that they are not guarded by CONFIG_INPUT_TOUCHSCREEN. A growing number
> of devices in input/misc are taking advantage of these.
>
> Therefore, I think this feature should follow suit and be available to any
> input device as is the case for touchscreen.c. As written, the newly updated
> binding is misleading because one may believe that any device that includes
> touchscreen.yaml can define an overlay child, but the code does not currently
> support this.
>
> To that end, it seems like touch-overlay would be a more descriptive name as
> well. I understand that the name has changed once already, so hopefully this
> feedback is not too annoying :)
>
changing names is no problem at all as long as it makes the feature more
comprehensible and/or takes more suitable devices into account, which
would be the case. So I will move the code from touchscreen to input and
I will update the names and descriptions to make them more generic.
I guess then I will need to move the properties to a separate binding
for this feature because it will not be an addition to the touchscreen
bindings anymore, right?
>> diff --git a/drivers/input/touchscreen/ts-overlay.c b/drivers/input/touchscreen/ts-overlay.c
>> new file mode 100644
>> index 000000000000..7afa77d86c1f
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/ts-overlay.c
>> @@ -0,0 +1,356 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Helper functions for overlay objects on touchscreens
>> + *
>> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
>> + */
>> +
>> +#include <linux/property.h>
>> +#include <linux/input.h>
>> +#include <linux/input/mt.h>
>> +#include <linux/module.h>
>> +#include <linux/input/ts-overlay.h>
>> +
>> +enum ts_overlay_valid_objects {
>> + TOUCHSCREEN,
>> + BUTTON,
>
> Please namespace these, i.e. TOUCH_OVERLAY_*.
>
>> +};
>> +
>> +static const char *const ts_overlay_names[] = {
>> + [TOUCHSCREEN] = "overlay-touchscreen",
>
> I'm a little confused why we need new code for this particular function; it's
> what touchscreen-min-x/y and touchscreen-size-x/y were meant to define. Why
> can't we keep using those?
>
According to the bindings, touchscreen-min-x/y define the minimum
reported values, but the overlay-touchscreen is actually setting a new
origin. Therefore I might be misusing those properties. On the other
hand touchscreen-size-x/y would make more sense, but I also considered
the case where someone would like to describe the real size of the
touchscreen outside of the overlay node as well as the clipped size
inside the node. In that case using the same property twice would be
confusing.
So in the end I thought that the origin/size properties are more precise
and applicable for all objects and not only the overlay touchscreen.
These properties are needed for the buttons anyways and in the future
more overlay would use the same properties.
>> + [BUTTON] = "overlay-buttons",
>> +};
>> +
>> +struct ts_overlay_shape {
>> + u32 x_origin;
>> + u32 y_origin;
>> + u32 x_size;
>> + u32 y_size;
>> +};
>> +
>> +struct ts_overlay_button {
>> + struct ts_overlay_shape shape;
>> + u32 key;
>> + bool pressed;
>> + int slot;
>> +};
>> +
>> +static int ts_overlay_get_shape_properties(struct fwnode_handle *child_node,
>> + struct ts_overlay_shape *shape)
>> +{
>> + int rc;
>
> In input, the common practice is to use 'error' for return values that are either
> zero or negative. The reasoning is because the variable quite literally represents
> an error, or lack thereof. And then:
>
> error = ...
> if (error)
> return error;
>
>> +
>> + rc = fwnode_property_read_u32(child_node, "x-origin", &shape->x_origin);
>> + if (rc < 0)
>> + return rc;
>
> It seems like all of these properties are required; if so, please update the
> binding to make this clear.
>
> If the binding is correct and these properties are in fact optional, then you
> must evaluate fwnode_property_read_u32() against -EINVAL.
>
If I end up writing new bindings for this feature, it will be more clear
what is required and what not because I will not be part only of the
touchscreen anymore. These properties will be required.
>> +
>> + rc = fwnode_property_read_u32(child_node, "y-origin", &shape->y_origin);
>> + if (rc < 0)
>> + return rc;
>> +
>> + rc = fwnode_property_read_u32(child_node, "x-size", &shape->x_size);
>> + if (rc < 0)
>> + return rc;
>> +
>> + rc = fwnode_property_read_u32(child_node, "y-size", &shape->y_size);
>> + if (rc < 0)
>> + return rc;
>> +
>> + return 0;
>> +}
>> +
>> +static int ts_overlay_get_button_properties(struct device *dev,
>> + struct fwnode_handle *child_node,
>> + struct ts_overlay_button *btn)
>> +{
>> + struct fwnode_handle *child_btn;
>> + int rc;
>> + int j = 0;
>> +
>> + fwnode_for_each_child_node(child_node, child_btn) {
>> + rc = ts_overlay_get_shape_properties(child_btn, &btn[j].shape);
>> + if (rc < 0)
>> + goto button_prop_cleanup;
>> +
>> + rc = fwnode_property_read_u32(child_btn, "linux,code",
>> + &btn[j].key);
>> + if (rc < 0)
>> + goto button_prop_cleanup;
>
> The binding needs to list this property as required, too.
>
Do you mean "linux,code"? It is already listed with the same pattern
that some other bindings use:
linux,code: true
Is that not right? I did not want to redefine an existing property that
other bindings already make use of.
>> +
>> + dev_info(dev, "Added button at (%u, %u), size %ux%u, code=%u\n",
>> + btn[j].shape.x_origin, btn[j].shape.y_origin,
>> + btn[j].shape.x_size, btn[j].shape.y_size, btn[j].key);
>
> This seems like a dev_dbg() to me.
>
>> + j++;
>> + }
>> +
>> + return 0;
>> +
>> +button_prop_cleanup:
>> + fwnode_handle_put(child_btn);
>> + return rc;
>> +}
>> +
>> +void ts_overlay_set_button_caps(struct ts_overlay_map *map,
>> + struct input_dev *dev)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < map->button_count; i++)
>> + input_set_capability(dev, EV_KEY, map->buttons[i].key);
>> +}
>> +EXPORT_SYMBOL(ts_overlay_set_button_caps);
>
> I don't see a need to expose this function and require participating drivers
> to call it; we should just have one over-arching function for processing the
> overlay(s), akin to touchscreen_parse_properties but for the button input
> device in case the driver separates the button and touchscreen input devices.
>
> That one function would then be responsible for parsing the overlay(s) and
> calling input_set_capability() on each button.
>
>> +
>> +static int ts_overlay_count_buttons(struct device *dev)
>> +{
>> + struct fwnode_handle *child_node;
>> + struct fwnode_handle *child_button;
>
> These names confused me; they're both children, but only the second is aptly
> named. How about child_overlay and child_button, or perhaps overlay_node and
> button_node?
>
>> + int count = 0;
>> +
>> + child_node = device_get_named_child_node(dev, ts_overlay_names[BUTTON]);
>> + if (!child_node)
>> + return 0;
>> +
>> + fwnode_for_each_child_node(child_node, child_button)
>> + count++;
>> + fwnode_handle_put(child_node);
>> +
>> + return count;
>> +}
>> +
>> +static int ts_overlay_map_touchscreen(struct device *dev,
>> + struct ts_overlay_map *map)
>> +{
>> + struct fwnode_handle *child;
>
> Same here; there are two layers of children, so please use more descriptive
> variable names.
>
>> + int rc = 0;
>> +
>> + child = device_get_named_child_node(dev, ts_overlay_names[TOUCHSCREEN]);
>> + if (!child)
>> + goto touchscreen_ret;
>
> I don't think we need a label here; just return 0 directly.
>
>> +
>> + map->touchscreen =
>> + devm_kzalloc(dev, sizeof(*map->touchscreen), GFP_KERNEL);
>> + if (!map->touchscreen) {
>> + rc = -ENOMEM;
>> + goto touchscreen_handle;
>> + }
>> + rc = ts_overlay_get_shape_properties(child, map->touchscreen);
>> + if (rc < 0)
>> + goto touchscreen_free;
>> +
>> + map->overlay_touchscreen = true;
>> + dev_info(dev, "Added overlay touchscreen at (%u, %u), size %u x %u\n",
>> + map->touchscreen->x_origin, map->touchscreen->y_origin,
>> + map->touchscreen->x_size, map->touchscreen->y_size);
>
> dev_dbg()
>
>> +
>> + rc = 0;
>
> rc (error) can only be zero if we have gotten this far; I don't see a need
> to assign it here.
>
>> + goto touchscreen_handle;
>
> Please think about whether this can be reorganized to prevent jumping over
> small bits of code; I found it hard to follow. Maybe one or more tasks can
> be offloaded to a helper function?
>
>> +
>> +touchscreen_free:
>> + devm_kfree(dev, map->touchscreen);
>
> This set off a red flag; it's unclear that it's necessary. Regardless of
> whether the participating driver is smart enough to bail during probe()
> if the overlay parsing fails, or it happily continues, this memory will
> get freed when the driver tied to 'dev' is torn down.
>
> Calling devm_kfree() is generally limited to special cases where you are
> dynamically reallocating memory at runtime. In case I have misunderstood
> the intent, please let me know.
>
Would devm_kfree() not free the memory immediately if the parsing fails,
making it available for any process instead of waiting until the driver
is torn down, which might never happen? Otherwise that chunk of memory
will not be accessible even though it is useless because the operation
failed, right? Or am I missing something?
>> +touchscreen_handle:
>> + fwnode_handle_put(child);
>> +touchscreen_ret:
>> + return rc;
>> +}
>> +
>> +static int ts_overlay_map_buttons(struct device *dev,
>> + struct ts_overlay_map *map,
>> + struct input_dev *input)
>> +{
>> + struct fwnode_handle *child;
>> + u32 button_count;
>> + int rc = 0;
>> +
>> + button_count = ts_overlay_count_buttons(dev);
>> + if (button_count) {
>> + map->buttons = devm_kcalloc(dev, button_count,
>> + sizeof(*map->buttons), GFP_KERNEL);
>> + if (!map->buttons) {
>> + rc = -ENOMEM;
>> + goto map_buttons_ret;
>> + }
>> + child = device_get_named_child_node(dev,
>> + ts_overlay_names[BUTTON]);
>> + if (unlikely(!child))
>> + goto map_buttons_free;
>> +
>> + rc = ts_overlay_get_button_properties(dev, child, map->buttons);
>> + if (rc < 0)
>> + goto map_buttons_free;
>> +
>> + map->button_count = button_count;
>> + }
>> +
>> + return 0;
>> +
>> +map_buttons_free:
>> + devm_kfree(dev, map->buttons);
>> +map_buttons_ret:
>> + return rc;
>> +}
>> +
>> +static bool ts_overlay_defined_objects(struct device *dev)
>> +{
>> + struct fwnode_handle *child;
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(ts_overlay_names); i++) {
>> + child = device_get_named_child_node(dev, ts_overlay_names[i]);
>> + if (child) {
>> + fwnode_handle_put(child);
>> + return true;
>> + }
>> + fwnode_handle_put(child);
>> + }
>> +
>> + return false;
>> +}
>> +
>> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
>> + struct input_dev *input)
>> +{
>> + struct ts_overlay_map *map = NULL;
>> + int rc;
>> +
>> + if (!ts_overlay_defined_objects(dev))
>> + return NULL;
>> +
>> + map = devm_kzalloc(dev, sizeof(*map), GFP_KERNEL);
>> + if (!map) {
>> + rc = -ENOMEM;
>> + goto objects_err;
>> + }
>> + rc = ts_overlay_map_touchscreen(dev, map);
>> + if (rc < 0)
>> + goto objects_free;
>> +
>> + rc = ts_overlay_map_buttons(dev, map, input);
>> + if (rc < 0)
>> + goto objects_free;
>> +
>> + return map;
>> +
>> +objects_free:
>> + devm_kfree(dev, map);
>> +objects_err:
>> + return ERR_PTR(rc);
>> +}
>> +EXPORT_SYMBOL(ts_overlay_map_objects);
>> +
>> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y)
>> +{
>> + *x = map->touchscreen->x_size - 1;
>> + *y = map->touchscreen->y_size - 1;
>> +}
>> +EXPORT_SYMBOL(ts_overlay_get_touchscreen_abs);
>> +
>> +static bool ts_overlay_shape_event(struct ts_overlay_shape *shape, u32 x, u32 y)
>> +{
>> + if (!shape)
>> + return false;
>> +
>> + if (x >= shape->x_origin && x < (shape->x_origin + shape->x_size) &&
>> + y >= shape->y_origin && y < (shape->y_origin + shape->y_size))
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +static bool ts_overlay_touchscreen_event(struct ts_overlay_shape *touchscreen,
>> + u32 *x, u32 *y)
>> +{
>> + if (ts_overlay_shape_event(touchscreen, *x, *y)) {
>> + *x -= touchscreen->x_origin;
>> + *y -= touchscreen->y_origin;
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map)
>> +{
>> + if (!map || !map->overlay_touchscreen)
>> + return false;
>> +
>> + return true;
>> +}
>> +EXPORT_SYMBOL(ts_overlay_mapped_touchscreen);
>> +
>> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map)
>> +{
>> + if (!map || !map->button_count)
>> + return false;
>> +
>> + return true;
>> +}
>> +EXPORT_SYMBOL(ts_overlay_mapped_buttons);
>> +
>> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y)
>> +{
>> + if (!ts_overlay_mapped_touchscreen(map))
>> + return true;
>> +
>> + if (!ts_overlay_touchscreen_event(map->touchscreen, x, y))
>> + return false;
>> +
>> + return true;
>> +}
>> +EXPORT_SYMBOL(ts_overlay_mt_on_touchscreen);
>> +
>> +bool ts_overlay_button_press(struct ts_overlay_map *map,
>> + struct input_dev *input, u32 x, u32 y, u32 slot)
>> +{
>> + int i;
>> +
>> + if (!ts_overlay_mapped_buttons(map))
>> + return false;
>> +
>> + for (i = 0; i < map->button_count; i++) {
>> + if (ts_overlay_shape_event(&map->buttons[i].shape, x, y)) {
>> + input_report_key(input, map->buttons[i].key, 1);
>> + map->buttons[i].pressed = true;
>> + map->buttons[i].slot = slot;
>> + return true;
>> + }
>> + }
>> +
>> + return false;
>> +}
>> +EXPORT_SYMBOL(ts_overlay_button_press);
>
> The level of abstraction here does not seem quite right. Rather than force
> each participating driver to call a press and release function, I think it
> is better to expose something like touch_overlay_process_buttons() which
> then handles the press and release events internally.
>
> You're also relying on each individual driver to decide whether a touch
> coordinate is inside or outside the overlay, and selectively call the press
> and release functions OR report coordinates which is non-optimal.
>
> To me, this says we actually need one wrapper function that accepts handles
> to both the touchscreen and button input devices (which may be the same at
> the driver's discretion) as well as the coordinates. If the coordinate is
> within an overlay area, handle press/release; if not, call touchscreen_report_pos().
>
>> +
>> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot)
>> +{
>> + int i;
>> +
>> + if (!map || !map->button_count)
>> + return false;
>> +
>> + for (i = 0; i < map->button_count; i++) {
>> + if (map->buttons[i].pressed && map->buttons[i].slot == slot)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +EXPORT_SYMBOL(ts_overlay_is_button_slot);
>> +
>> +void ts_overlay_button_release(struct ts_overlay_map *map,
>> + struct input_dev *input, u32 slot)
>> +{
>> + int i;
>> +
>> + if (!map || !map->button_count)
>> + return;
>> +
>> + for (i = 0; i < map->button_count; i++) {
>> + if (map->buttons[i].pressed && map->buttons[i].slot == slot) {
>> + input_report_key(input, map->buttons[i].key, 0);
>> + map->buttons[i].pressed = false;
>> + }
>> + }
>> +}
>> +EXPORT_SYMBOL(ts_overlay_button_release);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Helper functions for overlay objects on touchscreens");
>> diff --git a/include/linux/input/ts-overlay.h b/include/linux/input/ts-overlay.h
>> new file mode 100644
>> index 000000000000..b75df0dec3ab
>> --- /dev/null
>> +++ b/include/linux/input/ts-overlay.h
>> @@ -0,0 +1,43 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
>> + */
>> +
>> +#ifndef _TS_OVERLAY
>> +#define _TS_OVERLAY
>> +
>> +#include <linux/types.h>
>> +
>> +struct input_dev;
>> +struct device;
>> +
>> +struct ts_overlay_map {
>> + struct ts_overlay_shape *touchscreen;
>> + bool overlay_touchscreen;
>> + struct ts_overlay_button *buttons;
>> + u32 button_count;
>> +};
>> +
>> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
>> + struct input_dev *input);
>> +
>> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y);
>> +
>> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map);
>> +
>> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map);
>> +
>> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y);
>> +
>> +bool ts_overlay_button_press(struct ts_overlay_map *map,
>> + struct input_dev *input, u32 x, u32 y, u32 slot);
>> +
>> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot);
>> +
>> +void ts_overlay_button_release(struct ts_overlay_map *map,
>> + struct input_dev *input, u32 slot);
>> +
>> +void ts_overlay_set_button_caps(struct ts_overlay_map *map,
>> + struct input_dev *dev);
>> +
>> +#endif
>>
>> --
>> 2.39.2
>>
>
> Kind regards,
> Jeff LaBundy
Thanks again for your feedback, I really appreciate it. All the comments
without a reply can be taken as acknowledged and accepted as they are
without further discussion. I will work on them for the next version.
Thank you for your time and best regards,
Javier Carrasco
^ permalink raw reply
* Re: [PATCH v4 02/24] Input: gpio-vibra - Simplify with dev_err_probe()
From: Linus Walleij @ 2023-06-26 7:14 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Dmitry Torokhov, Hans de Goede, Bastien Nocera, Sangwon Jee,
Eugen Hristev, Mika Penttilä, linux-input, linux-kernel,
platform-driver-x86, Andi Shyti, Andy Shevchenko, Andy Shevchenko
In-Reply-To: <20230625162817.100397-3-krzysztof.kozlowski@linaro.org>
On Sun, Jun 25, 2023 at 6:28 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> Common pattern of handling deferred probe can be simplified with
> dev_err_probe(). Less code and also it prints the error value.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v4 01/24] Input: gpio_keys_polled - Simplify with dev_err_probe()
From: Linus Walleij @ 2023-06-26 7:13 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Dmitry Torokhov, Hans de Goede, Bastien Nocera, Sangwon Jee,
Eugen Hristev, Mika Penttilä, linux-input, linux-kernel,
platform-driver-x86, Andi Shyti, Andy Shevchenko, Andy Shevchenko
In-Reply-To: <20230625162817.100397-2-krzysztof.kozlowski@linaro.org>
On Sun, Jun 25, 2023 at 6:28 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> Common pattern of handling deferred probe can be simplified with
> dev_err_probe(). Less code and also it prints the error value.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v3 4/4] input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-26 7:02 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <ZJiXopmFr4dPbqll@nixie71>
Hi Jeff,
On 25/06/2023 21:38, Jeff LaBundy wrote:
> Hi Neil,
>
> On Thu, Jun 22, 2023 at 04:29:02PM +0200, Neil Armstrong wrote:
>> Add initial support for the new Goodix "Berlin" touchscreen ICs
>> over the SPI interface.
>>
>> The driver doesn't use the regmap_spi code since the SPI messages
>> needs to be prefixed, thus this custom regmap code.
>>
>> This initial driver is derived from the Goodix goodix_ts_berlin
>> available at [1] and [2] and only supports the GT9916 IC
>> present on the Qualcomm SM8550 MTP & QRD touch panel.
>>
>> The current implementation only supports BerlinD, aka GT9916.
>>
>> [1] https://github.com/goodix/goodix_ts_berlin
>> [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>
> Just a few comments below, then feel free to add:
>
> Reviewed-by: Jeff LaBundy <jeff@labundy.com>
>
>> drivers/input/touchscreen/Kconfig | 13 ++
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/goodix_berlin_spi.c | 172 ++++++++++++++++++++++++++
>> 3 files changed, 186 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 5e21cca6025d..2d86615e5090 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -435,6 +435,19 @@ config TOUCHSCREEN_GOODIX_BERLIN_I2C
>> To compile this driver as a module, choose M here: the
>> module will be called goodix_berlin_i2c.
>>
>> +config TOUCHSCREEN_GOODIX_BERLIN_SPI
>> + tristate "Goodix Berlin SPI touchscreen"
>> + depends on SPI_MASTER
>
> select REGMAP
>
> (keep "depends on SPI_MASTER")
Ack, indeed it looks cleaner to do that
>
>> + select TOUCHSCREEN_GOODIX_BERLIN_CORE
>> + help
>> + Say Y here if you have a Goodix Berlin IC connected to
>> + your system via SPI.
>> +
>> + If unsure, say N.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called goodix_berlin_spi.
>> +
>> config TOUCHSCREEN_HIDEEP
>> tristate "HiDeep Touch IC"
>> depends on I2C
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 921a2da0c2be..29524e8a83db 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
>> +obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o
>> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
>> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
>> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
>> diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c
>> new file mode 100644
>> index 000000000000..3a1bc251b32d
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/goodix_berlin_spi.c
>> @@ -0,0 +1,172 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Goodix Berlin Touchscreen Driver
>> + *
>> + * Copyright (C) 2020 - 2021 Goodix, Inc.
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Based on goodix_ts_berlin driver.
>> + */
>> +#include <asm/unaligned.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include "goodix_berlin.h"
>> +
>> +#define SPI_TRANS_PREFIX_LEN 1
>> +#define REGISTER_WIDTH 4
>> +#define SPI_READ_DUMMY_LEN 3
>> +#define SPI_READ_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH + SPI_READ_DUMMY_LEN)
>> +#define SPI_WRITE_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH)
>> +
>> +#define SPI_WRITE_FLAG 0xF0
>> +#define SPI_READ_FLAG 0xF1
>
> Please namespace all of these as you have done in the core driver.
Ack
>
>> +
>> +static int goodix_berlin_spi_read(void *context, const void *reg_buf,
>> + size_t reg_size, void *val_buf,
>> + size_t val_size)
>> +{
>> + struct spi_device *spi = context;
>> + struct spi_transfer xfers;
>> + struct spi_message spi_msg;
>> + const u32 *reg = reg_buf; /* reg is stored as native u32 at start of buffer */
>> + u8 *buf;
>> + int ret;
>
> int error;
>
>> +
>> + if (reg_size != REGISTER_WIDTH)
>> + return -EINVAL;
>> +
>> + buf = kzalloc(SPI_READ_PREFIX_LEN + val_size, GFP_KERNEL);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + spi_message_init(&spi_msg);
>> + memset(&xfers, 0, sizeof(xfers));
>> +
>> + /* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */
>> + buf[0] = SPI_READ_FLAG;
>> + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN);
>> + memset(buf + SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH, 0xff,
>> + SPI_READ_DUMMY_LEN);
>> +
>> + xfers.tx_buf = buf;
>> + xfers.rx_buf = buf;
>> + xfers.len = SPI_READ_PREFIX_LEN + val_size;
>> + xfers.cs_change = 0;
>> + spi_message_add_tail(&xfers, &spi_msg);
>> +
>> + ret = spi_sync(spi, &spi_msg);
>
> error = spi_sync(...);
>
>> + if (ret < 0)
>
> if (error)
>
>> + dev_err(&spi->dev, "transfer error:%d", ret);
>> + else
>> + memcpy(val_buf, buf + SPI_READ_PREFIX_LEN, val_size);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static int goodix_berlin_spi_write(void *context, const void *data,
>> + size_t count)
>> +{
>> + unsigned int len = count - REGISTER_WIDTH;
>> + struct spi_device *spi = context;
>> + struct spi_transfer xfers;
>> + struct spi_message spi_msg;
>> + const u32 *reg = data; /* reg is stored as native u32 at start of buffer */
>> + u8 *buf;
>> + int ret;
>
> Same comments here regarding 'error' vs. 'ret'.
Seems I forgot to do the rename here, thanks for pointing it!
>
>> +
>> + buf = kzalloc(SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + spi_message_init(&spi_msg);
>> + memset(&xfers, 0, sizeof(xfers));
>> +
>> + buf[0] = SPI_WRITE_FLAG;
>> + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN);
>> + memcpy(buf + SPI_WRITE_PREFIX_LEN, data + REGISTER_WIDTH, len);
>> +
>> + xfers.tx_buf = buf;
>> + xfers.len = SPI_WRITE_PREFIX_LEN + len;
>> + xfers.cs_change = 0;
>> + spi_message_add_tail(&xfers, &spi_msg);
>> +
>> + ret = spi_sync(spi, &spi_msg);
>> + if (ret < 0)
>> + dev_err(&spi->dev, "transfer error:%d", ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static const struct regmap_config goodix_berlin_spi_regmap_conf = {
>> + .reg_bits = 32,
>> + .val_bits = 8,
>> + .read = goodix_berlin_spi_read,
>> + .write = goodix_berlin_spi_write,
>> +};
>> +
>> +/* vendor & product left unassigned here, should probably be updated from fw info */
>> +static const struct input_id goodix_berlin_spi_input_id = {
>> + .bustype = BUS_SPI,
>> +};
>> +
>> +static int goodix_berlin_spi_probe(struct spi_device *spi)
>> +{
>> + struct regmap_config *regmap_config;
>> + struct regmap *regmap;
>> + size_t max_size;
>> + int error = 0;
>> +
>> + regmap_config = devm_kmemdup(&spi->dev, &goodix_berlin_spi_regmap_conf,
>> + sizeof(*regmap_config), GFP_KERNEL);
>> + if (!regmap_config)
>> + return -ENOMEM;
>
> Is there any reason we cannot simply pass goodix_berlin_spi_regmap_conf to
> devm_regmap_init() below? Why to duplicate and pass the copy?
>
> For reference, BMP280 in IIO is a similar example of a device with regmap
> sitting atop a bespoke SPI protocol; it does not seem to take this extra
> step.
The goodix_berlin_spi_regmap_conf copy is modified after with the correct
max raw read/write size, and I'm not a fan of modifying a global structure
that could be use for multiple probes, I can make a copy in a stack variable
if it feels simpler.
>
>> +
>> + spi->mode = SPI_MODE_0;
>> + spi->bits_per_word = 8;
>> + error = spi_setup(spi);
>> + if (error)
>> + return error;
>> +
>> + max_size = spi_max_transfer_size(spi);
>> + regmap_config->max_raw_read = max_size - SPI_READ_PREFIX_LEN;
>> + regmap_config->max_raw_write = max_size - SPI_WRITE_PREFIX_LEN;
>> +
>> + regmap = devm_regmap_init(&spi->dev, NULL, spi, regmap_config);
>> + if (IS_ERR(regmap))
>> + return PTR_ERR(regmap);
>> +
>> + return goodix_berlin_probe(&spi->dev, spi->irq,
>> + &goodix_berlin_spi_input_id, regmap);
>> +}
>> +
>> +static const struct spi_device_id goodix_berlin_spi_ids[] = {
>> + { "gt9916" },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids);
>> +
>> +static const struct of_device_id goodix_berlin_spi_of_match[] = {
>> + { .compatible = "goodix,gt9916", },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match);
>> +
>> +static struct spi_driver goodix_berlin_spi_driver = {
>> + .driver = {
>> + .name = "goodix-berlin-spi",
>> + .of_match_table = goodix_berlin_spi_of_match,
>> + .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
>> + },
>> + .probe = goodix_berlin_spi_probe,
>> + .id_table = goodix_berlin_spi_ids,
>> +};
>> +module_spi_driver(goodix_berlin_spi_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Goodix Berlin SPI Touchscreen driver");
>> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>>
>> --
>> 2.34.1
>>
>
> Kind regards,
> Jeff LaBundy
Thanks,
Neil
^ 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