* Re: [PATCH] Input: touchscreen - Add new Novatek NVT-ts driver
From: Hans de Goede @ 2023-03-09 13:04 UTC (permalink / raw)
To: Jeff LaBundy; +Cc: Dmitry Torokhov, linux-input
In-Reply-To: <Y/Q84gI1AnE6oqRU@nixie71>
Hi Jeff,
On 2/21/23 04:39, Jeff LaBundy wrote:
> Hi Hans,
>
> On Fri, Feb 17, 2023 at 04:07:49PM +0100, Hans de Goede wrote:
>> Add a new driver for the Novatek i2c touchscreen controller as found
>> on the Acer Iconia One 7 B1-750 tablet. Unfortunately the touchscreen
>> controller model-number is unknown. Even with the tablet opened up it
>> is impossible to read the model-number.
>>
>> Android calls this a "NVT-ts" touchscreen, but that may apply to other
>> Novatek controller models too.
>>
>> This appears to be the same controller as the one supported by
>> https://github.com/advx9600/android/blob/master/touchscreen/NVTtouch_Android4.0/NVTtouch.c
>> but unfortunately that does not give us a model-number either.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> This is a great driver; I have only a few comments below.
Thanks and thank you for the review!
>> ---
>> MAINTAINERS | 6 +
>> drivers/input/touchscreen/Kconfig | 10 +
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/novatek-nvt-ts.c | 288 +++++++++++++++++++++
>> 4 files changed, 305 insertions(+)
>> create mode 100644 drivers/input/touchscreen/novatek-nvt-ts.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 60c0ded06e3f..0c051a973e6b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -14835,6 +14835,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/wtarreau/nolibc.git
>> F: tools/include/nolibc/
>> F: tools/testing/selftests/nolibc/
>>
>> +NOVATEK NVT-TS I2C TOUCHSCREEN DRIVER
>> +M: Hans de Goede <hdegoede@redhat.com>
>> +L: linux-input@vger.kernel.org
>> +S: Maintained
>> +F: drivers/input/touchscreen/novatek-nvt-ts.c
>> +
>> NSDEPS
>> M: Matthias Maennich <maennich@google.com>
>> S: Maintained
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 68d99a112e14..59ca8bfe9a95 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -666,6 +666,16 @@ config TOUCHSCREEN_MTOUCH
>> To compile this driver as a module, choose M here: the
>> module will be called mtouch.
>>
>> +config TOUCHSCREEN_NOVATEK_NVT_TS
>> + tristate "Novatek NVT-ts touchscreen support"
>> + depends on I2C
>> + help
>> + Say Y here if you have a Novatek NVT-ts touchscreen.
>> + If unsure, say N.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called novatek-nvt-ts.
>> +
>> config TOUCHSCREEN_IMAGIS
>> tristate "Imagis touchscreen support"
>> depends on I2C
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 4968c370479a..41654239f89c 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -67,6 +67,7 @@ obj-$(CONFIG_TOUCHSCREEN_MMS114) += mms114.o
>> obj-$(CONFIG_TOUCHSCREEN_MSG2638) += msg2638.o
>> obj-$(CONFIG_TOUCHSCREEN_MTOUCH) += mtouch.o
>> obj-$(CONFIG_TOUCHSCREEN_MK712) += mk712.o
>> +obj-$(CONFIG_TOUCHSCREEN_NOVATEK_NVT_TS) += novatek-nvt-ts.o
>> obj-$(CONFIG_TOUCHSCREEN_HP600) += hp680_ts_input.o
>> obj-$(CONFIG_TOUCHSCREEN_HP7XX) += jornada720_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_IPAQ_MICRO) += ipaq-micro-ts.o
>> diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c
>> new file mode 100644
>> index 000000000000..2464c758ca14
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/novatek-nvt-ts.c
>> @@ -0,0 +1,288 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Driver for Novatek i2c touchscreen controller as found on
>> + * the Acer Iconia One 7 B1-750 tablet. The Touchscreen controller
>> + * model-number is unknown. Android calls this a "NVT-ts" touchscreen,
>> + * but that may apply to other Novatek controller models too.
>> + *
>> + * Copyright (c) 2023 Hans de Goede <hdegoede@redhat.com>
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/i2c.h>
>> +#include <linux/input.h>
>> +#include <linux/input/mt.h>
>> +#include <linux/input/touchscreen.h>
>> +#include <linux/module.h>
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#define NVT_TS_TOUCH_START 0x00
>> +#define NVT_TS_TOUCH_SIZE 6
>> +
>> +#define NVT_TS_PARAMETERS_START 0x78
>> +/* These are offsets from NVT_TS_PARAMETERS_START */
>> +#define NVT_TS_PARAMS_WIDTH 0x04
>> +#define NVT_TS_PARAMS_HEIGHT 0x06
>> +#define NVT_TS_PARAMS_MAX_TOUCH 0x09
>> +#define NVT_TS_PARAMS_MAX_BUTTONS 0x0a
>> +#define NVT_TS_PARAMS_IRQ_TYPE 0x0b
>> +#define NVT_TS_PARAMS_WAKE_TYPE 0x0c
>> +#define NVT_TS_PARAMS_CHIP_ID 0x0e
>> +#define NVT_TS_PARAMS_SIZE 0x0f
>> +
>> +#define NVT_TS_SUPPORTED_WAKE_TYPE 0x05
>> +#define NVT_TS_SUPPORTED_CHIP_ID 0x05
>> +
>> +#define NVT_TS_MAX_TOUCHES 10
>> +#define NVT_TS_MAX_SIZE 4096
>> +
>> +#define NVT_TS_TOUCH_NEW 1
>> +#define NVT_TS_TOUCH_UPDATE 2
>> +#define NVT_TS_TOUCH_RELEASE 3
>> +
>> +static const int nvt_ts_irq_type[4] = {
>> + IRQF_TRIGGER_RISING,
>> + IRQF_TRIGGER_FALLING,
>> + IRQF_TRIGGER_LOW,
>> + IRQF_TRIGGER_HIGH
>> +};
>> +
>> +struct nvt_ts_data {
>> + struct i2c_client *client;
>> + struct input_dev *input;
>> + struct gpio_desc *reset_gpio;
>> + struct touchscreen_properties prop;
>> + int max_touches;
>> + u8 buf[NVT_TS_TOUCH_SIZE * NVT_TS_MAX_TOUCHES];
>> +};
>> +
>> +static int nvt_ts_read_data(struct i2c_client *client, u8 reg, u8 *data, int count)
>> +{
>> + struct i2c_msg msg[2] = {
>> + {
>> + .addr = client->addr,
>> + .len = 1,
>> + .buf = ®
>
> Nit: there is no trailing comma here, yet one trails 'buf' below.
Ack, I'll add the trailing comma here too.
>> + },
>> + {
>> + .addr = client->addr,
>> + .flags = I2C_M_RD,
>> + .len = count,
>> + .buf = data,
>> + }
>> + };
>> + int ret;
>> +
>> + ret = i2c_transfer(client->adapter, msg, 2);
>> + if (ret != 2) {
>> + dev_err(&client->dev, "Error reading from 0x%02x: %d\n", reg, ret);
>> + return (ret < 0) ? ret : -EIO;
>> + }
>
> This is idiomatic, but I feel it is clearer to write ARRAY_SIZE(msg) instead
> of 2 throughout; this way the length is hard-coded only once.
Sure, will update for v2.
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t nvt_ts_irq(int irq, void *dev_id)
>> +{
>> + struct nvt_ts_data *data = dev_id;
>> + struct device *dev = &data->client->dev;
>> + int i, ret, slot, x, y;
>
> In input, return values for functions that only return zero on success tend to
> be named 'error'.
Ack, will update for v2.
>> + bool active;
>> + u8 *touch;
>> +
>> + ret = nvt_ts_read_data(data->client, NVT_TS_TOUCH_START, data->buf,
>> + data->max_touches * NVT_TS_TOUCH_SIZE);
>> + if (ret)
>> + return IRQ_HANDLED;
>> +
>> + for (i = 0; i < data->max_touches; i++) {
>> + touch = &data->buf[i * NVT_TS_TOUCH_SIZE];
>> +
>> + /* 0xff means no touch */
>> + if (touch[0] == 0xff)
>> + continue;
>> +
>> + slot = touch[0] >> 3;
>> + if (slot < 1 || slot > data->max_touches) {
>> + dev_warn(dev, "slot %d out of range, ignoring\n", slot);
>> + continue;
>> + }
>> +
>> + switch (touch[0] & 7) {
>
> With all other fields and values defined so nicely, it seems most clear to
> also define the bit field name in this case.
Ack, I'll add a define for this.
>
>> + case NVT_TS_TOUCH_NEW:
>> + case NVT_TS_TOUCH_UPDATE:
>> + active = true;
>> + break;
>> + case NVT_TS_TOUCH_RELEASE:
>> + active = false;
>> + break;
>> + default:
>> + dev_warn(dev, "slot %d unknown state %d\n", slot, touch[0] & 7);
>> + continue;
>> + }
>> +
>> + slot--;
>> + x = (touch[1] << 4) | (touch[3] >> 4);
>> + y = (touch[2] << 4) | (touch[3] & 0x0f);
>> +
>> + input_mt_slot(data->input, slot);
>> + input_mt_report_slot_state(data->input, MT_TOOL_FINGER, active);
>> + touchscreen_report_pos(data->input, &data->prop, x, y, true);
>> + }
>> +
>> + input_mt_sync_frame(data->input);
>> + input_sync(data->input);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int nvt_ts_start(struct input_dev *dev)
>> +{
>> + struct nvt_ts_data *data = input_get_drvdata(dev);
>> +
>> + enable_irq(data->client->irq);
>> + gpiod_set_value_cansleep(data->reset_gpio, 0);
>> +
>> + return 0;
>> +}
>> +
>> +static void nvt_ts_stop(struct input_dev *dev)
>> +{
>> + struct nvt_ts_data *data = input_get_drvdata(dev);
>> +
>> + disable_irq(data->client->irq);
>> + gpiod_set_value_cansleep(data->reset_gpio, 1);
>> +}
>> +
>> +static int nvt_ts_suspend(struct device *dev)
>> +{
>> + struct nvt_ts_data *data = i2c_get_clientdata(to_i2c_client(dev));
>> +
>> + mutex_lock(&data->input->mutex);
>> + if (input_device_enabled(data->input))
>> + nvt_ts_stop(data->input);
>> + mutex_unlock(&data->input->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static int nvt_ts_resume(struct device *dev)
>> +{
>> + struct nvt_ts_data *data = i2c_get_clientdata(to_i2c_client(dev));
>> +
>> + mutex_lock(&data->input->mutex);
>> + if (input_device_enabled(data->input))
>> + nvt_ts_start(data->input);
>> + mutex_unlock(&data->input->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(nvt_ts_pm_ops, nvt_ts_suspend, nvt_ts_resume);
>> +
>> +static int nvt_ts_probe(struct i2c_client *client)
>> +{
>> + struct device *dev = &client->dev;
>> + int ret, width, height, irq_type;
>> + struct nvt_ts_data *data;
>> + struct input_dev *input;
>> +
>> + if (!client->irq) {
>> + dev_err(dev, "Error no irq specified\n");
>> + return -EINVAL;
>> + }
>> +
>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + data->client = client;
>> + i2c_set_clientdata(client, data);
>> +
>> + data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>> + if (IS_ERR(data->reset_gpio))
>> + return dev_err_probe(dev, PTR_ERR(data->reset_gpio), "requesting reset GPIO\n");
>> +
>> + /* Wait for controller to come out of reset before params read */
>> + msleep(100);
>> + ret = nvt_ts_read_data(data->client, NVT_TS_PARAMETERS_START, data->buf,
>> + NVT_TS_PARAMS_SIZE);
>> + gpiod_set_value_cansleep(data->reset_gpio, 1); /* Put back in reset */
>> + if (ret)
>> + return ret;
>> +
>> + width = get_unaligned_be16(&data->buf[NVT_TS_PARAMS_WIDTH]);
>> + height = get_unaligned_be16(&data->buf[NVT_TS_PARAMS_HEIGHT]);
>> + data->max_touches = data->buf[NVT_TS_PARAMS_MAX_TOUCH];
>> + irq_type = data->buf[NVT_TS_PARAMS_IRQ_TYPE];
>> +
>> + if (width > NVT_TS_MAX_SIZE || height >= NVT_TS_MAX_SIZE ||
>> + data->max_touches > NVT_TS_MAX_TOUCHES ||
>> + irq_type >= ARRAY_SIZE(nvt_ts_irq_type) ||
>> + data->buf[NVT_TS_PARAMS_WAKE_TYPE] != NVT_TS_SUPPORTED_WAKE_TYPE ||
>> + data->buf[NVT_TS_PARAMS_CHIP_ID] != NVT_TS_SUPPORTED_CHIP_ID) {
>> + dev_err(dev, "Unsupported touchscreen parameters: %*ph\n",
>> + NVT_TS_PARAMS_SIZE, data->buf);
>> + return -EIO;
>
> Nit: because there was no I/O error here necessarily, but rather invalid or
> unacceptable values, I think -EINVAL is better here.
AFAIK -EINVAL is reserved for invalid function parameters, typically
on a syscall / ioctl. Where as here we are receiving invalid data,
which is more like a a checksum/crc error which is an IO error.
With that said I have no strong preference either way, so let me know
if you want me to switch to EINVAL for v2 or not.
>
>> + }
>> +
>> + dev_info(dev, "Detected %dx%d touchscreen with %d max touches\n",
>> + width, height, data->max_touches);
>
> This is also idiomatic, but this seems better as dev_dbg.
Ack, this was mostly useful during development, I'll change this
to dev_dbg() for v2.
>
>> +
>> + if (data->buf[NVT_TS_PARAMS_MAX_BUTTONS])
>> + dev_warn(dev, "Touchscreen buttons are not supported\n");
>> +
>> + input = devm_input_allocate_device(dev);
>> + if (!input)
>> + return -ENOMEM;
>> +
>> + input->name = client->name;
>> + input->id.bustype = BUS_I2C;
>> + input->open = nvt_ts_start;
>> + input->close = nvt_ts_stop;
>> + input->dev.parent = dev;
>
> devm_input_allocate_device() already sets the parent for us.
Ah, nice. I'll drop this line then.
>
>> +
>> + input_set_abs_params(input, ABS_MT_POSITION_X, 0, width - 1, 0, 0);
>> + input_set_abs_params(input, ABS_MT_POSITION_Y, 0, height - 1, 0, 0);
>> + touchscreen_parse_properties(input, true, &data->prop);
>> +
>> + ret = input_mt_init_slots(input, data->max_touches,
>> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
>> + if (ret)
>> + return ret;
>> +
>> + data->input = input;
>> + input_set_drvdata(input, data);
>> +
>> + ret = devm_request_threaded_irq(dev, client->irq, NULL, nvt_ts_irq,
>> + IRQF_ONESHOT | IRQF_NO_AUTOEN | nvt_ts_irq_type[irq_type],
>> + client->name, data);
>
> Interesting, it seems interrupt polarity is configurable?
On the controller-side, yes. The goodix touchscreens have much the same
thing.
> For my own
> understanding, what if there is an inverter on the board?
Then things break I guess since we program the GPIO input IRQs polarity
to match the controller polarity when then will be wrong.
Luckily this has never happened so far AFAIK (mostly talking goodix
here, since I know only 1 device with this new touchscreen).
> Is the
> expectation that the customer reprograms the controller's firmware?
>
>> + if (ret)
>> + return dev_err_probe(dev, ret, "requesting irq\n");
>
> dev_err_probe() tends not to be accepted in input, the argument being
> that the callers who can return EPROBE_DEFER be responsible for setting
> the reason as opposed to every driver calling a separate function that
> does so.
To me dev_err_probe() is not so much about setting the probe-defer
reason, it is is very useful because:
1) It deals with not logging afalse-postivive error msg on -EPROBE_DEFER and
you can return its return value, leading to much more compact code and
thus IMOH more readable code
2) It leads to a consistent format for the printed errors
To illustrate 1. without dev_err_probe() the reset_gpio request error
handling turns from this:
if (IS_ERR(data->reset_gpio))
return dev_err_probe(dev, PTR_ERR(data->reset_gpio), "requesting reset GPIO\n");
into:
if (IS_ERR(data->reset_gpio)) {
error = PTR_ERR(data->reset_gpio);
if (error == -EPROBE_DEFER)
return error;
dev_err(dev, "Error %d requesting reset GPIO\n", error);
return error;
}
Which is 7 lines vs 2 lines when using dev_err_probe() and more
importantly IMHO the error handling code using using dev_err_probe()
is just much more readable and thus more maintainable IMHO.
Which is why IMHO using dev_err_probe() for errors getting resources
is just much better.
So unless you feel really strongly about not using this I would
prefer to keep using dev_err_probe().
>> +
>> + return input_register_device(input);
>> +}
>> +
>> +static const struct i2c_device_id nvt_ts_i2c_id[] = {
>> + { "NVT-ts" },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, nvt_ts_i2c_id);
>> +
>> +static struct i2c_driver nvt_ts_driver = {
>> + .driver = {
>> + .name = "novatek-nvt-ts",
>> + .pm = &nvt_ts_pm_ops,
>
> I believe we need pm_sleep_ptr() here now.
Ack, will fix for v2.
>> + },
>> + .probe_new = nvt_ts_probe,
>> + .id_table = nvt_ts_i2c_id,
>> +};
>> +
>> +module_i2c_driver(nvt_ts_driver);
>> +
>> +MODULE_DESCRIPTION("Novatek NVT-ts touchscreen driver");
>> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.39.1
>>
Once more thank you for the review. If you can clarify what
you want for the EINVAL vs EIO and for the dev_err_probe()
review remarks then I'll prepare a v2.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
From: Andy Shevchenko @ 2023-03-09 11:56 UTC (permalink / raw)
To: Daniel Kaehn
Cc: Benjamin Tissoires, Hans de Goede, robh+dt,
krzysztof.kozlowski+dt, jikos, bartosz.golaszewski,
dmitry.torokhov, devicetree, linux-input, ethan.twardy
In-Reply-To: <ZAnGRtIhLZTrpDy7@smile.fi.intel.com>
On Thu, Mar 09, 2023 at 01:43:02PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 08, 2023 at 12:32:07PM -0600, Daniel Kaehn wrote:
> > On Wed, Mar 8, 2023 at 10:36 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Wed, Mar 08, 2023 at 06:30:46PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote:
> > > > > On Mar 08 2023, Daniel Kaehn wrote:
> > > > > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
> > > > > > <benjamin.tissoires@redhat.com> wrote:
...
> > > > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > > > > Package () {
> > > > > Package () { "cell-names", Package () { "i2c", "gpio" }
> > > > > }
> > > >
> > > > Yeah, looking at this, I think it still fragile. First of all, either this is
> > > > missing, or simply wrong. We would need to access indices. ACPI _ADR is in the
> > > > specification. As much as with PCI it may be considered reliable.
> > > >
> > > > So, that said, forget about it, and simply use _ADR as indicator of the node.
> > > > See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver.
> > >
> > > And that said, maybe CP2112 should simply re-use what MFD _already_ provides?
> >
> > Great point -- it definitely seems like this driver belongs in the mfd
> > directory to begin with.
>
> It can be iteratively converted later on.
>
> > It seems like aside from rewriting the CP2112 driver into an mfd
> > driver and two platform drivers,
> > my route forward for now would be to just do something like this (not
> > yet tested):
> >
> > + struct acpi_device *adev = ACPI_COMPANION(&hdev->dev);
> > + if (adev)
> > + ACPI_COMPANION_SET(&dev->adap.dev, acpi_find_child_device(adev,
> > 0x0, false));
>
> ACPI_COMPANION_SET() is something different to simple device_set_node().
> I would expect that in this driver we simply use the child fwnode as is.
> But since you are not using so called secondary fwnode, I believe it's
> fine for now.
>
> > + else
> > + device_set_node(&dev->adap.dev,
> > device_get_named_child_node(&hdev->dev, "i2c"));
> >
> > (and the same for the gpiochip)
> > The follow-up question -- does there exist something analogous to DT
> > bindings for ACPI devices,
> > other than the ACPI spec itself, where this should be documented? Or
> > will consumers truly have to
> > read the driver code to determine that _ADR 0 is I2C and _ADR 1 is
> > GPIO? (I haven't seen anything
> > in my search so far -- but knowing that it truly doesn't exist would
> > make me respect people developing
> > embedded ACPI-based systems all the more!)
The below misplaced, so here is the answer to the followup.
The _DSD heavily relies on the DT schemas and other standards such as MIPI.
For many cases there are no standards or any developed approaches.
Feel free to add a piece of documentation for the devices that are utilising
_ADR in ACPI (we have at least I²C/GPIO controller on Intel Quark —
drivers/mfd/intel_quark_i2c_gpio.c, and mentioned earlier the Diolan DLN-2 —
drivers/mfd/dln2.c).
> See how the acpi_get_local_address() is used in the 3 users of it.
>
> Ideally we need a new callback in the fwnode ops to return either
> (least) 32-bit of _ADR or "reg" property.
>
> Dunno, if "reg" is actually what suits here.
>
> That said, I would do something like (pseudo-code)
>
> device_for_each_child_node() {
> if (name == $NAME)
> $NAME->fwnode = child;
> else if (_ADR = $INDEX)
> $NAME->fwnode = child;
> }
>
> > Thanks for your patience in working through all of this, especially
> > considering how long of an email
> > chain this has become!
>
> You're welcome!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
From: Andy Shevchenko @ 2023-03-09 11:43 UTC (permalink / raw)
To: Daniel Kaehn
Cc: Benjamin Tissoires, Hans de Goede, robh+dt,
krzysztof.kozlowski+dt, jikos, bartosz.golaszewski,
dmitry.torokhov, devicetree, linux-input, ethan.twardy
In-Reply-To: <CAP+ZCCdziub809WKJ8-tAhYvg+axsiuXrvrZczj_x2K0bGzd7w@mail.gmail.com>
On Wed, Mar 08, 2023 at 12:32:07PM -0600, Daniel Kaehn wrote:
> On Wed, Mar 8, 2023 at 10:36 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Mar 08, 2023 at 06:30:46PM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote:
> > > > On Mar 08 2023, Daniel Kaehn wrote:
> > > > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
> > > > > <benjamin.tissoires@redhat.com> wrote:
...
> > > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > > > Package () {
> > > > Package () { "cell-names", Package () { "i2c", "gpio" }
> > > > }
> > >
> > > Yeah, looking at this, I think it still fragile. First of all, either this is
> > > missing, or simply wrong. We would need to access indices. ACPI _ADR is in the
> > > specification. As much as with PCI it may be considered reliable.
> > >
> > > So, that said, forget about it, and simply use _ADR as indicator of the node.
> > > See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver.
> >
> > And that said, maybe CP2112 should simply re-use what MFD _already_ provides?
>
> Great point -- it definitely seems like this driver belongs in the mfd
> directory to begin with.
It can be iteratively converted later on.
> It seems like aside from rewriting the CP2112 driver into an mfd
> driver and two platform drivers,
> my route forward for now would be to just do something like this (not
> yet tested):
>
> + struct acpi_device *adev = ACPI_COMPANION(&hdev->dev);
> + if (adev)
> + ACPI_COMPANION_SET(&dev->adap.dev, acpi_find_child_device(adev,
> 0x0, false));
ACPI_COMPANION_SET() is something different to simple device_set_node().
I would expect that in this driver we simply use the child fwnode as is.
But since you are not using so called secondary fwnode, I believe it's
fine for now.
> + else
> + device_set_node(&dev->adap.dev,
> device_get_named_child_node(&hdev->dev, "i2c"));
>
> (and the same for the gpiochip)
>
> The follow-up question -- does there exist something analogous to DT
> bindings for ACPI devices,
> other than the ACPI spec itself, where this should be documented? Or
> will consumers truly have to
> read the driver code to determine that _ADR 0 is I2C and _ADR 1 is
> GPIO? (I haven't seen anything
> in my search so far -- but knowing that it truly doesn't exist would
> make me respect people developing
> embedded ACPI-based systems all the more!)
See how the acpi_get_local_address() is used in the 3 users of it.
Ideally we need a new callback in the fwnode ops to return either
(least) 32-bit of _ADR or "reg" property.
Dunno, if "reg" is actually what suits here.
That said, I would do something like (pseudo-code)
device_for_each_child_node() {
if (name == $NAME)
$NAME->fwnode = child;
else if (_ADR = $INDEX)
$NAME->fwnode = child;
}
> Thanks for your patience in working through all of this, especially
> considering how long of an email
> chain this has become!
You're welcome!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH] HID: hidraw: Keep the report ID on buffer in get_report
From: Antoine C @ 2023-03-09 11:29 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input@vger.kernel.org
The ioctl syscall with arg HIDIOCGINPUT must not override
the report ID contained in the first byte of the buffer
and should offset the report data next to it.
Signed-off-by: Antoine Colombier <kernel@acolombier.dev>
---
Hello,
Apologies for the resend, I forgot to disable the HTML format on the
previous email. Please ignore the previous one.
This addresses the bug report in the hidapi: https://github.com/libusb/hidapi/issues/514
The patch was tested using the test snippets attached in the issue above
on 6.2.0-76060200-generic (PopOS 22.04)
drivers/hid/hidraw.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 197b1e7bf029..2c12f25817e6 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -231,9 +231,10 @@ static ssize_t hidraw_get_report(struct file *file,
char __user *buffer, size_t
if (ret < 0)
goto out_free;
+ count--;
len = (ret < count) ? ret : count;
- if (copy_to_user(buffer, buf, len)) {
+ if (copy_to_user(buffer + 1, buf, len)) {
ret = -EFAULT;
goto out_free;
}
^ permalink raw reply related
* Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
From: Benjamin Tissoires @ 2023-03-09 9:38 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Daniel Kaehn, Hans de Goede, robh+dt, krzysztof.kozlowski+dt,
jikos, bartosz.golaszewski, dmitry.torokhov, devicetree,
linux-input, ethan.twardy
In-Reply-To: <ZAi4NjqXTbLpVhPo@smile.fi.intel.com>
On Mar 08 2023, Andy Shevchenko wrote:
> On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote:
> > On Mar 08 2023, Daniel Kaehn wrote:
> > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
> > > <benjamin.tissoires@redhat.com> wrote:
> > > > But if I refer "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", the IRQ is never assigned.
> > > > With the parent (CP21), it works.
> > > >
> > > > So I wonder if the cp2112 driver is correctly assigning the gc->parent
> > > > field.
>
> > > Did you make a change to the CP2112 driver patch to look for uppercase
> > > "I2C" and "GPIO"?
> >
> > yes, sorry I should have mentioned it. This is the only modification I
> > have compared to the upstream kernel plus your patch series.
> >
> > > Otherwise, it won't assign those child nodes appropriately, and the
> > > gpiochip code will use
> > > the parent node by default if the gpiochip's fwnode isn't assigned (I believe).
> >
> > I don't think it's a fwnode issue, but a problem with the assignment of
> > the parent of the gc:
> > ---
> > dev->gc.parent = &hdev->dev;
> > ---
>
> I don't think so. The parent should point to the _physical_ device, which is
> CP2112, which is correct in my opinion.
>
Right. I tend to agree, and then the problem seems to be relying in
gpiolib-acpi.c
> > Because the function acpi_gpiochip_find() in drivers/gpio/gpiolib-acpi.c
> > compares the acpi handle returned by fetching the ACPI path
> > ("\\_SB_.PCI0.USB0.RHUB.CP21.GPIO") and the one of gc->parent, which in
> > the hid-cp2112 case is the HID device itself.
>
> We have specifically gc->fwnode for cases like this.
Looks like gpiolib-acpi.c doesn't care about fwnode at all.
if I do the following:
---
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index d8a421ce26a8..5aebc266426b 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -126,7 +126,7 @@ static bool acpi_gpio_deferred_req_irqs_done;
static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
{
- return gc->parent && device_match_acpi_handle(gc->parent, data);
+ return ACPI_HANDLE_FWNODE(gc->fwnode) == data;
}
/**
---
I can now directly reference the GPIO ACPI node in my GpioInt()
declaration. And AFAICT this should be safe to do because gpiolib ensure
that gc->fwnode is set, using the one from the parent if it is not set
previously.
I need to check if this works with my icelake laptop, and if so I'll
send it to the list.
The reason the intel gpios are working (the only one I checked) is
because the \\SB.GPI0 node refers to the pinctrl controller (driver
pinctrl-icelake.c in my case, which then creates a subdevice for
handling the gpio).
>
> ...
>
Cheers,
Benjamin
^ permalink raw reply related
* Re: [PATCH RFC 2/4] input: apple_z2: Add a driver for Apple Z2 touchscreens
From: Jeff LaBundy @ 2023-03-09 3:04 UTC (permalink / raw)
To: fnkl.kernel
Cc: Hector Martin, Sven Peter, Alyssa Rosenzweig, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, -, Henrik Rydberg,
linux-arm-kernel, linux-input, devicetree, linux-kernel
In-Reply-To: <20230223-z2-for-ml-v1-2-028f2b85dc15@gmail.com>
Hi Sasha,
Great work; this is an interesting driver. I have left some comments
throughout.
On Fri, Feb 24, 2023 at 11:20:07AM +0100, Sasha Finkelstein via B4 Relay wrote:
> From: Sasha Finkelstein <fnkl.kernel@gmail.com>
>
> Adds a driver for Apple touchscreens using the Z2 protocol.
>
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
> ---
> drivers/input/touchscreen/Kconfig | 13 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/apple_z2.c | 465 +++++++++++++++++++++++++++++++++++
> 3 files changed, 479 insertions(+)
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 68d99a112e14..76aeea837ab9 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -103,6 +103,19 @@ config TOUCHSCREEN_ADC
> To compile this driver as a module, choose M here: the
> module will be called resistive-adc-touch.ko.
>
> +config TOUCHSCREEN_APPLE_Z2
> + tristate "Apple Z2 touchscreens"
> + default ARCH_APPLE
> + depends on SPI && INPUT && OF
There is no need to depend on INPUT; the build will not enter TOUCHSCREEN_*
without it.
I also do not see any need to depend on OF as you are (correctly) using the
device property API. The of_device_id struct toward the end is not gated by
OF either.
> + help
> + Say Y here if you have an Apple device with
> + a touchscreen or a touchbar.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apple_z2.
> +
> config TOUCHSCREEN_AR1021_I2C
> tristate "Microchip AR1020/1021 i2c touchscreen"
> depends on I2C && OF
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 4968c370479a..d885b2c4e3a6 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7879_I2C) += ad7879-i2c.o
> obj-$(CONFIG_TOUCHSCREEN_AD7879_SPI) += ad7879-spi.o
> obj-$(CONFIG_TOUCHSCREEN_ADC) += resistive-adc-touch.o
> obj-$(CONFIG_TOUCHSCREEN_ADS7846) += ads7846.o
> +obj-$(CONFIG_TOUCHSCREEN_APPLE_Z2) += apple_z2.o
> obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C) += ar1021_i2c.o
> obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT) += atmel_mxt_ts.o
> obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR) += auo-pixcir-ts.o
> diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
> new file mode 100644
> index 000000000000..06f1d864a137
> --- /dev/null
> +++ b/drivers/input/touchscreen/apple_z2.c
> @@ -0,0 +1,465 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple Z2 touchscreen driver
> + *
> + * Copyright (C) The Asahi Linux Contributors
> + */
> +
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
Please sort these #includes alphabetically.
> +
> +#define Z2_NUM_FINGERS_OFFSET 16
> +#define Z2_FINGERS_OFFSET 24
> +#define Z2_TOUCH_STARTED 3
> +#define Z2_TOUCH_MOVED 4
> +#define Z2_CMD_READ_INTERRUPT_DATA 0xEB
> +#define Z2_HBPP_CMD_BLOB 0x3001
> +#define Z2_FW_MAGIC 0x5746325A
> +#define LOAD_COMMAND_INIT_PAYLOAD 0
> +#define LOAD_COMMAND_SEND_BLOB 1
> +#define LOAD_COMMAND_SEND_CALIBRATION 2
In my opinion, #defines are easier to read if the values are aligned vertically.
> +
> +struct apple_z2 {
> + struct spi_device *spidev;
> + struct gpio_desc *cs_gpio;
> + struct gpio_desc *reset_gpio;
> + struct input_dev *input_dev;
> + struct completion boot_irq;
> + int booted;
> + int counter;
> + int y_size;
> + const char *fw_name;
> + const char *cal_blob;
> + int cal_size;
> +};
> +
> +struct z2_finger {
> + u8 finger;
> + u8 state;
> + __le16 unknown2;
> + __le16 abs_x;
> + __le16 abs_y;
> + __le16 rel_x;
> + __le16 rel_y;
> + __le16 tool_major;
> + __le16 tool_minor;
> + __le16 orientation;
> + __le16 touch_major;
> + __le16 touch_minor;
> + __le16 unused[2];
> + __le16 pressure;
> + __le16 multi;
> +} __packed;
Please pick one namespace (i.e. apple_z2 or z2) and use it consistently throughout.
> +
> +struct z2_hbpp_blob_hdr {
> + u16 cmd;
> + u16 len;
> + u32 addr;
> + u16 checksum;
> +} __packed;
> +
> +struct z2_fw_hdr {
> + u32 magic;
> + u32 version;
> +} __packed;
> +
> +struct z2_read_interrupt_cmd {
> + u8 cmd;
> + u8 counter;
> + u8 unused[12];
> + __le16 checksum;
> +} __packed;
> +
> +static void apple_z2_parse_touches(struct apple_z2 *z2, char *msg, size_t msg_len)
> +{
> + int i;
> + int nfingers;
> + int slot;
> + int slot_valid;
> + struct z2_finger *fingers;
> +
> + if (msg_len <= Z2_NUM_FINGERS_OFFSET)
> + return;
> + nfingers = msg[Z2_NUM_FINGERS_OFFSET];
> + fingers = (struct z2_finger *)(msg + Z2_FINGERS_OFFSET);
> + for (i = 0; i < nfingers; i++) {
> + slot = input_mt_get_slot_by_key(z2->input_dev, fingers[i].finger);
> + if (slot < 0) {
> + dev_warn(&z2->spidev->dev, "unable to get slot for finger");
We need an '\n' at the end of this string.
> + continue;
> + }
> + slot_valid = fingers[i].state == Z2_TOUCH_STARTED ||
> + fingers[i].state == Z2_TOUCH_MOVED;
This logic was initially a bit hard to parse; I think it would be easier
to read if aligned as follows:
slot_valid = fingers[i].state == Z2_TOUCH_STARTED ||
fingers[i].state == Z2_TOUCH_MOVED;
> + input_mt_slot(z2->input_dev, slot);
> + input_mt_report_slot_state(z2->input_dev, MT_TOOL_FINGER, slot_valid);
> + if (!slot_valid)
> + continue;
> + input_report_abs(z2->input_dev, ABS_MT_POSITION_X,
> + le16_to_cpu(fingers[i].abs_x));
> + input_report_abs(z2->input_dev, ABS_MT_POSITION_Y,
> + z2->y_size - le16_to_cpu(fingers[i].abs_y));
> + input_report_abs(z2->input_dev, ABS_MT_WIDTH_MAJOR,
> + le16_to_cpu(fingers[i].tool_major));
> + input_report_abs(z2->input_dev, ABS_MT_WIDTH_MINOR,
> + le16_to_cpu(fingers[i].tool_minor));
> + input_report_abs(z2->input_dev, ABS_MT_ORIENTATION,
> + le16_to_cpu(fingers[i].orientation));
> + input_report_abs(z2->input_dev, ABS_MT_TOUCH_MAJOR,
> + le16_to_cpu(fingers[i].touch_major));
> + input_report_abs(z2->input_dev, ABS_MT_TOUCH_MINOR,
> + le16_to_cpu(fingers[i].touch_minor));
> + }
> + input_mt_sync_frame(z2->input_dev);
> + input_sync(z2->input_dev);
> +}
> +
> +static int apple_z2_spi_sync(struct apple_z2 *z2, struct spi_message *msg)
> +{
> + int err;
Did you not get a compiler warning about there not being a newline between
the declaration and subsequent code?
At any rate, please consider spacing this function out as follows:
int err;
gpiod_direction_output(...);
usleep_range(....);
err = ...;
usleep_range(...);
gpiod_direction_output(...);
return err;
As it stands, it's difficult to read.
> + gpiod_direction_output(z2->cs_gpio, 0);
> + usleep_range(1000, 2000);
> + err = spi_sync(z2->spidev, msg);
> + usleep_range(1000, 2000);
> + gpiod_direction_output(z2->cs_gpio, 1);
> + return err;
> +}
> +
Thinking about this more, why is the touchscreen driver responsible for
managing its own chip select? Shouldn't the SPI controller driver figure
out on its own if the chosen GPIO can be toggled by the native hardware
peripheral, or bit banged?
It seems the touchscreen driver is absorbing work that is missing in the
SPI controller driver. Please let me know in case I have misunderstood.
> +static int apple_z2_read_packet(struct apple_z2 *z2)
> +{
> + struct spi_message msg;
> + struct spi_transfer xfer;
> + struct z2_read_interrupt_cmd len_cmd;
> + char len_rx[16];
> + size_t pkt_len;
> + char *pkt_rx;
> + int err = 0;
No need to initialize this to zero.
> +
> + spi_message_init(&msg);
> + memset(&xfer, 0, sizeof(xfer));
> + memset(&len_cmd, 0, sizeof(len_cmd));
> + len_cmd.cmd = Z2_CMD_READ_INTERRUPT_DATA;
> + len_cmd.counter = z2->counter + 1;
> + len_cmd.checksum = cpu_to_le16(Z2_CMD_READ_INTERRUPT_DATA + 1 + z2->counter);
> + z2->counter = 1 - z2->counter;
> + xfer.tx_buf = &len_cmd;
> + xfer.rx_buf = len_rx;
> + xfer.len = sizeof(len_cmd);
> + spi_message_add_tail(&xfer, &msg);
> + err = apple_z2_spi_sync(z2, &msg);
> + if (err)
> + return err;
Again, please consider adding some newlines to spread out functionally
separate segments of code.
> +
> + pkt_len = ((len_rx[1] | len_rx[2] << 8) + 8) & (-4);
We can reduce some of this math with get_unaligned_le16().
> + pkt_rx = kzalloc(pkt_len, GFP_KERNEL);
> + if (!pkt_rx)
> + return -ENOMEM;
> +
> + spi_message_init(&msg);
> + xfer.rx_buf = pkt_rx;
> + xfer.len = pkt_len;
> + spi_message_add_tail(&xfer, &msg);
> + err = apple_z2_spi_sync(z2, &msg);
> +
> + if (!err)
> + apple_z2_parse_touches(z2, pkt_rx + 5, pkt_len - 5);
> +
> + kfree(pkt_rx);
> + return err;
> +}
> +
> +static irqreturn_t apple_z2_irq(int irq, void *data)
> +{
> + struct spi_device *spi = data;
> + struct apple_z2 *z2 = spi_get_drvdata(spi);
> +
> + if (!z2->booted)
> + complete(&z2->boot_irq);
> + else
> + apple_z2_read_packet(z2);
> +
> + return IRQ_HANDLED;
> +}
> +
> +
> +// Return value must be freed with kfree
If not already, please run checkpatch; it should have barked about
C99-style comments.
That being said, I think it is dangerous to expect the caller to free
the return value; it is ripe for bugs being introduced later in case
others contribute to this driver.
Stated another way, a better method of encapsulation is to limit this
function to working on the data, with the caller being responsible to
manage the memory which holds the data.
> +static char *apple_z2_build_cal_blob(struct apple_z2 *z2, u32 address, u32 *size)
> +{
> + u16 len_words = (z2->cal_size + 3) / 4;
> + u32 checksum = 0;
> + u16 checksum_hdr = 0;
> + char *data;
> + int i;
> + struct z2_hbpp_blob_hdr *hdr;
> +
> + *size = z2->cal_size + sizeof(struct z2_hbpp_blob_hdr) + 4;
> +
> + data = kzalloc(*size, GFP_KERNEL);
> + hdr = (struct z2_hbpp_blob_hdr *)data;
> + hdr->cmd = Z2_HBPP_CMD_BLOB;
> + hdr->len = len_words;
> + hdr->addr = address;
> +
> + for (i = 2; i < 8; i++)
> + checksum_hdr += data[i];
> +
> + hdr->checksum = checksum_hdr;
> + memcpy(data + 10, z2->cal_blob, z2->cal_size);
> +
> + for (i = 0; i < z2->cal_size; i++)
> + checksum += z2->cal_blob[i];
> +
> + *(u32 *)(data + z2->cal_size + 10) = checksum;
> +
> + return data;
> +}
> +
> +static int apple_z2_send_firmware_blob(struct apple_z2 *z2, const char *data, u32 size, u8 bpw)
> +{
> + struct spi_message msg;
> + struct spi_transfer blob_xfer, ack_xfer;
> + char int_ack[] = {0x1a, 0xa1};
> + char ack_rsp[] = {0, 0};
> + int err;
> +
> + spi_message_init(&msg);
> + memset(&blob_xfer, 0, sizeof(blob_xfer));
> + memset(&ack_xfer, 0, sizeof(ack_xfer));
> + blob_xfer.tx_buf = data;
> + blob_xfer.len = size;
> + blob_xfer.bits_per_word = bpw;
> + spi_message_add_tail(&blob_xfer, &msg);
> + ack_xfer.tx_buf = int_ack;
> + ack_xfer.rx_buf = ack_rsp;
> + ack_xfer.len = 2;
> + spi_message_add_tail(&ack_xfer, &msg);
> + reinit_completion(&z2->boot_irq);
> + err = apple_z2_spi_sync(z2, &msg);
> + if (err)
> + return err;
> + wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20));
> + return 0;
> +}
Same comments with regard to whitespace here; this function is difficult
to read and hence review.
> +
> +static int apple_z2_upload_firmware(struct apple_z2 *z2)
> +{
> + const struct firmware *fw;
> + struct z2_fw_hdr *fw_hdr;
> + size_t fw_idx = sizeof(struct z2_fw_hdr);
> + int err;
> + u32 load_cmd;
> + u32 size;
> + u32 address;
> + char *data;
> +
> + err = request_firmware(&fw, z2->fw_name, &z2->spidev->dev);
> + if (err)
> + return dev_err_probe(&z2->spidev->dev, err, "unable to load firmware");
In my experience, calling request_firmware() from probe is dangerous
especially if the driver is built-in and not a module. The reason is
the filesystem may not be ready; depending on the platform, you may
get -ENOENT or a deadlock.
If the device has no nonvolatile memory and firmware loading _must_
be kicked off from probe, request_firmware_nowait() with the rest
of the work offloaded to its callback is safer.
> +
> + fw_hdr = (struct z2_fw_hdr *)fw->data;
> + if (fw_hdr->magic != Z2_FW_MAGIC || fw_hdr->version != 1)
> + return dev_err_probe(&z2->spidev->dev, -EINVAL, "invalid firmware header");
> +
> + while (fw_idx < fw->size) {
> + if (fw->size - fw_idx < 8) {
> + err = dev_err_probe(&z2->spidev->dev, -EINVAL, "firmware malformed");
> + goto error;
> + }
> +
> + load_cmd = *(u32 *)(fw->data + fw_idx);
> + fw_idx += 4;
> + if (load_cmd == LOAD_COMMAND_INIT_PAYLOAD || load_cmd == LOAD_COMMAND_SEND_BLOB) {
> + size = *(u32 *)(fw->data + fw_idx);
> + fw_idx += 4;
> + if (fw->size - fw_idx < size) {
> + err = dev_err_probe(&z2->spidev->dev,
> + -EINVAL, "firmware malformed");
> + goto error;
> + }
> + err = apple_z2_send_firmware_blob(z2, fw->data + fw_idx,
> + size, load_cmd == LOAD_COMMAND_SEND_BLOB ? 16 : 8);
> + if (err)
> + goto error;
> + fw_idx += size;
> + } else if (load_cmd == 2) {
> + address = *(u32 *)(fw->data + fw_idx);
> + fw_idx += 4;
> + data = apple_z2_build_cal_blob(z2, address, &size);
> + err = apple_z2_send_firmware_blob(z2, data, size, 16);
> + kfree(data);
> + if (err)
> + goto error;
> + } else {
> + err = dev_err_probe(&z2->spidev->dev, -EINVAL, "firmware malformed");
> + goto error;
> + }
> + if (fw_idx % 4 != 0)
> + fw_idx += 4 - (fw_idx % 4);
> + }
> +
> +
> + z2->booted = 1;
> + apple_z2_read_packet(z2);
> + error:
> + release_firmware(fw);
> + return err;
> +}
> +
> +static int apple_z2_boot(struct apple_z2 *z2)
> +{
> + int err;
> + enable_irq(z2->spidev->irq);
> + gpiod_direction_output(z2->reset_gpio, 1);
> + wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20));
> + err = apple_z2_upload_firmware(z2);
> + if (err) { // Boot failed, let's put the device into reset just in case.
> + gpiod_direction_output(z2->reset_gpio, 0);
> + disable_irq(z2->spidev->irq);
> + }
> + return err;
> +}
> +
> +static int apple_z2_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct apple_z2 *z2;
> + int err;
In input, return values for functions that only return zero on success tend to
be named 'error'.
> + int x_size;
> + const char *device_name;
> +
> + z2 = devm_kzalloc(dev, sizeof(*z2), GFP_KERNEL);
> + if (!z2)
> + return -ENOMEM;
> +
> + z2->spidev = spi;
> + init_completion(&z2->boot_irq);
> + spi_set_drvdata(spi, z2);
> +
> + z2->cs_gpio = devm_gpiod_get_index(dev, "cs", 0, 0);
> + if (IS_ERR(z2->cs_gpio))
> + return dev_err_probe(dev, PTR_ERR(z2->cs_gpio), "unable to get cs");
dev_err_probe() tends not to be accepted in input, the argument being
that the callers who can return -EPROBE_DEFER be responsible for setting
the reason as opposed to every driver calling a separate function that
does so.
> +
> + z2->reset_gpio = devm_gpiod_get_index(dev, "reset", 0, 0);
> + if (IS_ERR(z2->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(z2->reset_gpio), "unable to get reset");
> +
> + err = devm_request_threaded_irq(dev, z2->spidev->irq, NULL,
> + apple_z2_irq, IRQF_ONESHOT | IRQF_NO_AUTOEN,
> + "apple-z2-irq", spi);
> + if (err < 0)
> + return dev_err_probe(dev, z2->spidev->irq, "unable to request irq");
> +
> + err = device_property_read_u32(dev, "touchscreen-size-x", &x_size);
> + if (err)
> + return dev_err_probe(dev, err, "unable to get touchscreen size");
> +
> + err = device_property_read_u32(dev, "touchscreen-size-y", &z2->y_size);
> + if (err)
> + return dev_err_probe(dev, err, "unable to get touchscreen size");
Please use touchscreen_parse_properties() instead of hard-coding these.
> +
> + err = device_property_read_string(dev, "apple,z2-device-name", &device_name);
> + if (err)
> + return dev_err_probe(dev, err, "unable to get device name");
> +
> + err = device_property_read_string(dev, "firmware-name", &z2->fw_name);
> + if (err)
> + return dev_err_probe(dev, err, "unable to get firmware name");
> +
> + z2->cal_blob = of_get_property(dev->of_node, "apple,z2-cal-blob", &z2->cal_size);
> + if (!z2->cal_blob)
> + return dev_err_probe(dev, -EINVAL, "unable to get calibration");
> +
> + z2->input_dev = devm_input_allocate_device(dev);
> + if (!z2->input_dev)
> + return -ENOMEM;
> + z2->input_dev->name = device_name;
> + z2->input_dev->phys = "apple_z2";
> + z2->input_dev->dev.parent = dev;
> + z2->input_dev->id.bustype = BUS_SPI;
> + input_set_abs_params(z2->input_dev, ABS_MT_POSITION_X, 0, x_size, 0, 0);
> + input_abs_set_res(z2->input_dev, ABS_MT_POSITION_X, 1);
> + input_set_abs_params(z2->input_dev, ABS_MT_POSITION_Y, 0, z2->y_size, 0, 0);
> + input_abs_set_res(z2->input_dev, ABS_MT_POSITION_Y, 1);
> + input_set_abs_params(z2->input_dev, ABS_MT_WIDTH_MAJOR, 0, 65535, 0, 0);
> + input_set_abs_params(z2->input_dev, ABS_MT_WIDTH_MINOR, 0, 65535, 0, 0);
> + input_set_abs_params(z2->input_dev, ABS_MT_TOUCH_MAJOR, 0, 65535, 0, 0);
> + input_set_abs_params(z2->input_dev, ABS_MT_TOUCH_MINOR, 0, 65535, 0, 0);
> + input_set_abs_params(z2->input_dev, ABS_MT_ORIENTATION, -32768, 32767, 0, 0);
> + input_mt_init_slots(z2->input_dev, 256, INPUT_MT_DIRECT);
Please check the return value of input_mt_init_slots().
> +
> + err = input_register_device(z2->input_dev);
> + if (err < 0)
> + return dev_err_probe(dev, err, "unable to register input device");
> +
> +
> + // Reset the device on probe
> + gpiod_direction_output(z2->reset_gpio, 0);
Technically you are de-asserting reset here, because gpiod is a logical API. A
'0' means take _out_ of reset. This means the reset GPIO's polarity in the dts
is backwards as well.
That being said, this specific call doesn't seem to be necessary since the value
matches the initial value in devm_gpiod_get_index(). So this call can be dropped,
and the polarity needs to be fixed in the call to devm_gpiod_get_index().
> + usleep_range(5000, 10000);
> + return apple_z2_boot(z2);
> +}
> +
> +static void apple_z2_remove(struct spi_device *spi)
> +{
> + struct apple_z2 *z2 = spi_get_drvdata(spi);
> +
> + disable_irq(z2->spidev->irq);
> + gpiod_direction_output(z2->reset_gpio, 0);
Same here; the polarity is wrong here. This should be set to 1, with the polarity
corrected in dts.
> +}
> +
> +static void apple_z2_shutdown(struct spi_device *spi)
> +{
> + struct apple_z2 *z2 = spi_get_drvdata(spi);
> +
> + disable_irq(z2->spidev->irq);
> + gpiod_direction_output(z2->reset_gpio, 0);
And here.
> +}
> +
> +static int apple_z2_suspend(struct device *dev)
> +{
> + apple_z2_shutdown(to_spi_device(dev));
> + return 0;
> +}
> +
> +static int apple_z2_resume(struct device *dev)
> +{
> + struct apple_z2 *z2 = spi_get_drvdata(to_spi_device(dev));
> +
> + return apple_z2_boot(z2);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(apple_z2_pm, apple_z2_suspend, apple_z2_resume);
> +
> +static const struct of_device_id apple_z2_of_match[] = {
> + { .compatible = "apple,z2-touchscreen" },
> + {},
No need for a comma after the sentinel because we would never add a line
below it.
> +};
> +MODULE_DEVICE_TABLE(of, apple_z2_of_match);
> +
> +static struct spi_device_id apple_z2_of_id[] = {
> + { .name = "z2-touchscreen" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, apple_z2_of_id);
> +
> +static struct spi_driver apple_z2_driver = {
> + .driver = {
> + .name = "apple-z2",
> + .pm = pm_sleep_ptr(&apple_z2_pm),
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(apple_z2_of_match),
> + },
> +
Nit: extraneous newline.
> + .id_table = apple_z2_of_id,
> + .probe = apple_z2_probe,
> + .remove = apple_z2_remove,
> + .shutdown = apple_z2_shutdown,
> +};
> +
> +module_spi_driver(apple_z2_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_FIRMWARE("apple/mtfw-*.bin");
>
> --
> Git-137.1)
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2] Add rumble support to latest xbox controllers
From: Bastien Nocera @ 2023-03-08 18:54 UTC (permalink / raw)
To: Siarhei Vishniakou
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <CAKF84v2hm-wRNonbMV9PMBghhSL1jTQ8ot7gTzspVpEv5d2SDw@mail.gmail.com>
On Wed, 2023-03-08 at 09:55 -0800, Siarhei Vishniakou wrote:
> Thanks Bastien!
>
> I can definitely add a link to the wikipedia page.
>
> Are you talking about adding the link to the commit message, or to
> hid-ids.h ?
I think the commit message would be enough so we know which device
we're talking about, but now that you mention it, maybe the source code
would be a good idea too.
As far as I've understood, and Benjamin can correct me, we don't need
to have IDs be in hid-ids.h because we don't need to declare them both
as a blocklist in the hid core, and then again in the driver itself.
My take is that the patches could move the IDs from hid-ids.h to hid-
microsoft.c as they're changed. You then wouldn't need the macros, just
add a comment for each of variants, and that Wikipedia article can be
linked above the whole XBox controller section.
What do you think?
>
>
> On Wed, Mar 8, 2023 at 2:23 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> >
> > On Tue, 2023-03-07 at 13:35 -0800, Siarhei Vishniakou wrote:
> > > Currently, rumble is only supported via bluetooth on a single
> > > xbox
> > > controller, called 'model 1708'. On the back of the device, it's
> > > named
> > > 'wireless controller for xbox one'. However, in 2021, Microsoft
> > > released
> > > a firmware update for this controller. As part of this update,
> > > the
> > > HID
> > > descriptor of the device changed. The product ID was also changed
> > > from
> > > 0x02fd to 0x0b20. On this controller, rumble was supported via
> > > hid-microsoft, which matched against the old product id (0x02fd).
> > > As
> > > a
> > > result, the firmware update broke rumble support on this
> > > controller.
> > >
> > > The hid-microsoft driver actually supports rumble on the new
> > > firmware,
> > > as well. So simply adding new product id is sufficient to bring
> > > back
> > > this support.
> > >
> > > After discussing further with the xbox team, it was pointed out
> > > that
> > > another xbox controller, xbox elite series 2, can be supported in
> > > a
> > > similar way.
> > >
> > > Add rumble support for all of these devices in this patch. Two of
> > > the
> > > devices have received firmware updates that caused their product
> > > id's
> > > to
> > > change. Both old and new firmware versions of these devices were
> > > tested.
> > >
> > > The tested controllers are:
> > >
> > > 1. 'wireless controller for xbox one', model 1708
> > > 2. 'xbox wireless controller', model 1914. This is also sometimes
> > > referred to as 'xbox series S|X'.
> > > 3. 'elite series 2', model 1797.
> > >
> > > The tested configurations are:
> > > 1. model 1708, pid 0x02fd (old firmware)
> > > 2. model 1708, pid 0x0b20 (new firmware)
> > > 3. model 1914, pid 0x0b13
> > > 4. model 1797, pid 0x0b05 (old firmware)
> > > 5. model 1797, pid 0x0b22 (new firmware)
> > >
> > > I verified rumble support on both bluetooth and usb.
> >
> > Looks good although I would personally have preferred separate
> > patches
> > for each controller.
> >
> > Reviewed-by: Bastien Nocera <hadess@hadess.net>
> >
> > Would a link to:
> > https://en.wikipedia.org/wiki/Xbox_Wireless_Controller#Summary
> > also be useful to make which model is which clearer in the minds of
> > future readers?
> >
> > Cheers
> >
> > >
> > > Signed-off-by: Siarhei Vishniakou <svv@google.com>
> > > Change-Id: I3337a7ab5f40759c85bf67bf0dbe5d4de31ce1ff
> > > ---
> > > drivers/hid/hid-ids.h | 6 +++++-
> > > drivers/hid/hid-microsoft.c | 11 ++++++++++-
> > > 2 files changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > index 053853a891c5..c9b75f8ba49a 100644
> > > --- a/drivers/hid/hid-ids.h
> > > +++ b/drivers/hid/hid-ids.h
> > > @@ -903,7 +903,11 @@
> > > #define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9
> > > #define USB_DEVICE_ID_MS_POWER_COVER 0x07da
> > > #define USB_DEVICE_ID_MS_SURFACE3_COVER 0x07de
> > > -#define USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER 0x02fd
> > > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708 0x02fd
> > > +#define
> > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE 0x0b20
> > > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914 0x0b13
> > > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797 0x0b05
> > > +#define
> > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE 0x0b22
> > > #define USB_DEVICE_ID_MS_PIXART_MOUSE 0x00cb
> > > #define USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS 0x02e0
> > >
> > > diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-
> > > microsoft.c
> > > index 071fd093a5f4..9345e2bfd56e 100644
> > > --- a/drivers/hid/hid-microsoft.c
> > > +++ b/drivers/hid/hid-microsoft.c
> > > @@ -446,7 +446,16 @@ static const struct hid_device_id
> > > ms_devices[] =
> > > {
> > > .driver_data = MS_PRESENTER },
> > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, 0x091B),
> > > .driver_data = MS_SURFACE_DIAL },
> > > - { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER),
> > > +
> > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708),
> > > + .driver_data = MS_QUIRK_FF },
> > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE),
> > > + .driver_data = MS_QUIRK_FF },
> > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914),
> > > + .driver_data = MS_QUIRK_FF },
> > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797),
> > > + .driver_data = MS_QUIRK_FF },
> > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE),
> > > .driver_data = MS_QUIRK_FF },
> > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS),
> > > .driver_data = MS_QUIRK_FF },
> >
^ permalink raw reply
* Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
From: Daniel Kaehn @ 2023-03-08 18:32 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Benjamin Tissoires, Hans de Goede, robh+dt,
krzysztof.kozlowski+dt, jikos, bartosz.golaszewski,
dmitry.torokhov, devicetree, linux-input, ethan.twardy
In-Reply-To: <ZAi5esmc158Bd2oL@smile.fi.intel.com>
On Wed, Mar 8, 2023 at 10:36 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Mar 08, 2023 at 06:30:46PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote:
> > > On Mar 08 2023, Daniel Kaehn wrote:
> > > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
> > > > <benjamin.tissoires@redhat.com> wrote:
>
> ...
>
> > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > > Package () {
> > > Package () { "cell-names", Package () { "i2c", "gpio" }
> > > }
> >
> > Yeah, looking at this, I think it still fragile. First of all, either this is
> > missing, or simply wrong. We would need to access indices. ACPI _ADR is in the
> > specification. As much as with PCI it may be considered reliable.
> >
> > So, that said, forget about it, and simply use _ADR as indicator of the node.
> > See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver.
>
> And that said, maybe CP2112 should simply re-use what MFD _already_ provides?
Great point -- it definitely seems like this driver belongs in the mfd
directory to begin with.
It seems like aside from rewriting the CP2112 driver into an mfd
driver and two platform drivers,
my route forward for now would be to just do something like this (not
yet tested):
+ struct acpi_device *adev = ACPI_COMPANION(&hdev->dev);
+ if (adev)
+ ACPI_COMPANION_SET(&dev->adap.dev, acpi_find_child_device(adev,
0x0, false));
+ else
+ device_set_node(&dev->adap.dev,
device_get_named_child_node(&hdev->dev, "i2c"));
(and the same for the gpiochip)
The follow-up question -- does there exist something analogous to DT
bindings for ACPI devices,
other than the ACPI spec itself, where this should be documented? Or
will consumers truly have to
read the driver code to determine that _ADR 0 is I2C and _ADR 1 is
GPIO? (I haven't seen anything
in my search so far -- but knowing that it truly doesn't exist would
make me respect people developing
embedded ACPI-based systems all the more!)
Thanks for your patience in working through all of this, especially
considering how long of an email
chain this has become!
Thanks,
Danny Kaehn
^ permalink raw reply
* Re: [PATCH v2] Add rumble support to latest xbox controllers
From: Siarhei Vishniakou @ 2023-03-08 17:55 UTC (permalink / raw)
To: Bastien Nocera; +Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <df8538e8a97cb4547db7da51a4359c03657ab79f.camel@hadess.net>
Thanks Bastien!
I can definitely add a link to the wikipedia page.
Are you talking about adding the link to the commit message, or to hid-ids.h ?
On Wed, Mar 8, 2023 at 2:23 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Tue, 2023-03-07 at 13:35 -0800, Siarhei Vishniakou wrote:
> > Currently, rumble is only supported via bluetooth on a single xbox
> > controller, called 'model 1708'. On the back of the device, it's
> > named
> > 'wireless controller for xbox one'. However, in 2021, Microsoft
> > released
> > a firmware update for this controller. As part of this update, the
> > HID
> > descriptor of the device changed. The product ID was also changed
> > from
> > 0x02fd to 0x0b20. On this controller, rumble was supported via
> > hid-microsoft, which matched against the old product id (0x02fd). As
> > a
> > result, the firmware update broke rumble support on this controller.
> >
> > The hid-microsoft driver actually supports rumble on the new
> > firmware,
> > as well. So simply adding new product id is sufficient to bring back
> > this support.
> >
> > After discussing further with the xbox team, it was pointed out that
> > another xbox controller, xbox elite series 2, can be supported in a
> > similar way.
> >
> > Add rumble support for all of these devices in this patch. Two of the
> > devices have received firmware updates that caused their product id's
> > to
> > change. Both old and new firmware versions of these devices were
> > tested.
> >
> > The tested controllers are:
> >
> > 1. 'wireless controller for xbox one', model 1708
> > 2. 'xbox wireless controller', model 1914. This is also sometimes
> > referred to as 'xbox series S|X'.
> > 3. 'elite series 2', model 1797.
> >
> > The tested configurations are:
> > 1. model 1708, pid 0x02fd (old firmware)
> > 2. model 1708, pid 0x0b20 (new firmware)
> > 3. model 1914, pid 0x0b13
> > 4. model 1797, pid 0x0b05 (old firmware)
> > 5. model 1797, pid 0x0b22 (new firmware)
> >
> > I verified rumble support on both bluetooth and usb.
>
> Looks good although I would personally have preferred separate patches
> for each controller.
>
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
>
> Would a link to:
> https://en.wikipedia.org/wiki/Xbox_Wireless_Controller#Summary
> also be useful to make which model is which clearer in the minds of
> future readers?
>
> Cheers
>
> >
> > Signed-off-by: Siarhei Vishniakou <svv@google.com>
> > Change-Id: I3337a7ab5f40759c85bf67bf0dbe5d4de31ce1ff
> > ---
> > drivers/hid/hid-ids.h | 6 +++++-
> > drivers/hid/hid-microsoft.c | 11 ++++++++++-
> > 2 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > index 053853a891c5..c9b75f8ba49a 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -903,7 +903,11 @@
> > #define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9
> > #define USB_DEVICE_ID_MS_POWER_COVER 0x07da
> > #define USB_DEVICE_ID_MS_SURFACE3_COVER 0x07de
> > -#define USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER 0x02fd
> > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708 0x02fd
> > +#define
> > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE 0x0b20
> > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914 0x0b13
> > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797 0x0b05
> > +#define
> > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE 0x0b22
> > #define USB_DEVICE_ID_MS_PIXART_MOUSE 0x00cb
> > #define USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS 0x02e0
> >
> > diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-
> > microsoft.c
> > index 071fd093a5f4..9345e2bfd56e 100644
> > --- a/drivers/hid/hid-microsoft.c
> > +++ b/drivers/hid/hid-microsoft.c
> > @@ -446,7 +446,16 @@ static const struct hid_device_id ms_devices[] =
> > {
> > .driver_data = MS_PRESENTER },
> > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, 0x091B),
> > .driver_data = MS_SURFACE_DIAL },
> > - { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER),
> > +
> > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708),
> > + .driver_data = MS_QUIRK_FF },
> > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE),
> > + .driver_data = MS_QUIRK_FF },
> > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914),
> > + .driver_data = MS_QUIRK_FF },
> > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797),
> > + .driver_data = MS_QUIRK_FF },
> > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE),
> > .driver_data = MS_QUIRK_FF },
> > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS),
> > .driver_data = MS_QUIRK_FF },
>
^ permalink raw reply
* Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
From: Andy Shevchenko @ 2023-03-08 16:36 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Daniel Kaehn, Hans de Goede, robh+dt, krzysztof.kozlowski+dt,
jikos, bartosz.golaszewski, dmitry.torokhov, devicetree,
linux-input, ethan.twardy
In-Reply-To: <ZAi4NjqXTbLpVhPo@smile.fi.intel.com>
On Wed, Mar 08, 2023 at 06:30:46PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote:
> > On Mar 08 2023, Daniel Kaehn wrote:
> > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
> > > <benjamin.tissoires@redhat.com> wrote:
...
> > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > Package () {
> > Package () { "cell-names", Package () { "i2c", "gpio" }
> > }
>
> Yeah, looking at this, I think it still fragile. First of all, either this is
> missing, or simply wrong. We would need to access indices. ACPI _ADR is in the
> specification. As much as with PCI it may be considered reliable.
>
> So, that said, forget about it, and simply use _ADR as indicator of the node.
> See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver.
And that said, maybe CP2112 should simply re-use what MFD _already_ provides?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
From: Andy Shevchenko @ 2023-03-08 16:30 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Daniel Kaehn, Hans de Goede, robh+dt, krzysztof.kozlowski+dt,
jikos, bartosz.golaszewski, dmitry.torokhov, devicetree,
linux-input, ethan.twardy
In-Reply-To: <20230308155527.jnrsowubvnk22ica@mail.corp.redhat.com>
On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote:
> On Mar 08 2023, Daniel Kaehn wrote:
> > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > > But if I refer "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", the IRQ is never assigned.
> > > With the parent (CP21), it works.
> > >
> > > So I wonder if the cp2112 driver is correctly assigning the gc->parent
> > > field.
> > Did you make a change to the CP2112 driver patch to look for uppercase
> > "I2C" and "GPIO"?
>
> yes, sorry I should have mentioned it. This is the only modification I
> have compared to the upstream kernel plus your patch series.
>
> > Otherwise, it won't assign those child nodes appropriately, and the
> > gpiochip code will use
> > the parent node by default if the gpiochip's fwnode isn't assigned (I believe).
>
> I don't think it's a fwnode issue, but a problem with the assignment of
> the parent of the gc:
> ---
> dev->gc.parent = &hdev->dev;
> ---
I don't think so. The parent should point to the _physical_ device, which is
CP2112, which is correct in my opinion.
> Because the function acpi_gpiochip_find() in drivers/gpio/gpiolib-acpi.c
> compares the acpi handle returned by fetching the ACPI path
> ("\\_SB_.PCI0.USB0.RHUB.CP21.GPIO") and the one of gc->parent, which in
> the hid-cp2112 case is the HID device itself.
We have specifically gc->fwnode for cases like this.
...
> Device (CP21) // the USB-hid & CP2112 shared node
> {
> Name (_ADR, One)
> Name (_DSD, Package ()
> {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () { "cell-names", Package () { "i2c", "gpio" }
> }
Yeah, looking at this, I think it still fragile. First of all, either this is
missing, or simply wrong. We would need to access indices. ACPI _ADR is in the
specification. As much as with PCI it may be considered reliable.
So, that said, forget about it, and simply use _ADR as indicator of the node.
See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver.
> })
>
> Device (I2C)
> {
> Name (_ADR, Zero)
> Name (_STA, 0x0F)
> }
>
> Device (GPIO)
> {
> Name (_ADR, One)
> Name (_STA, 0x0F)
>
> Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () { "gpio-hog", 1 },
> Package () { "gpios", Package () { 4, 0 } },
> Package () { "output-high", 1 },
> Package () { "line-name", "gpio4-pullup" },
> },
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () { "gpio-line-names", Package () {
> "",
> "",
> "irq-rmi4",
> "",
> "power", // set to 1 with gpio-hog above
> "",
> "",
> "",
> ""}},
> }
> })
> }
> }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] Input: xen-kbdfront - drop keys to shrink modalias
From: Jason Andryuk @ 2023-03-08 16:27 UTC (permalink / raw)
To: Mattijs Korpershoek
Cc: linux-kernel, xen-devel, Phillip Susi, stable, Dmitry Torokhov,
linux-input
In-Reply-To: <CAKf6xpsGy7rjK3Rkosevr3dD+64-WqCEzAecBbHEHpBMeAHh7A@mail.gmail.com>
On Wed, Mar 8, 2023 at 11:26 AM Jason Andryuk <jandryuk@gmail.com> wrote:
>
> On Thu, Dec 15, 2022 at 8:54 AM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
> >
> > On Fri, Dec 09, 2022 at 09:26, Jason Andryuk <jandryuk@gmail.com> wrote:
> >
> > > xen kbdfront registers itself as being able to deliver *any* key since
> > > it doesn't know what keys the backend may produce.
> > >
> > > Unfortunately, the generated modalias gets too large and uevent creation
> > > fails with -ENOMEM.
> > >
> > > This can lead to gdm not using the keyboard since there is no seat
> > > associated [1] and the debian installer crashing [2].
> > >
> > > Trim the ranges of key capabilities by removing some BTN_* ranges.
> > > While doing this, some neighboring undefined ranges are removed to trim
> > > it further.
> > >
> > > An upper limit of KEY_KBD_LCD_MENU5 is still too large. Use an upper
> > > limit of KEY_BRIGHTNESS_MENU.
> > >
> > > This removes:
> > > BTN_DPAD_UP(0x220)..BTN_DPAD_RIGHT(0x223)
> > > Empty space 0x224..0x229
> > >
> > > Empty space 0x28a..0x28f
> > > KEY_MACRO1(0x290)..KEY_MACRO30(0x2ad)
> > > KEY_MACRO_RECORD_START 0x2b0
> > > KEY_MACRO_RECORD_STOP 0x2b1
> > > KEY_MACRO_PRESET_CYCLE 0x2b2
> > > KEY_MACRO_PRESET1(0x2b3)..KEY_MACRO_PRESET3(0xb5)
> > > Empty space 0x2b6..0x2b7
> > > KEY_KBD_LCD_MENU1(0x2b8)..KEY_KBD_LCD_MENU5(0x2bc)
> > > Empty space 0x2bd..0x2bf
> > > BTN_TRIGGER_HAPPY(0x2c0)..BTN_TRIGGER_HAPPY40(0x2e7)
> > > Empty space 0x2e8..0x2ff
> > >
> > > The modalias shrinks from 2082 to 1550 bytes.
> > >
> > > A chunk of keys need to be removed to allow the keyboard to be used.
> > > This may break some functionality, but the hope is these macro keys are
> > > uncommon and don't affect any users.
> > >
> > > [1] https://github.com/systemd/systemd/issues/22944
> > > [2] https://lore.kernel.org/xen-devel/87o8dw52jc.fsf@vps.thesusis.net/T/
> > >
> > > Cc: Phillip Susi <phill@thesusis.net>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> >
> > Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>
> Thank you, Mattjis.
Thank you, Mattijs. My apologies.
Regards,
Jason
^ permalink raw reply
* Re: [PATCH v2] Input: xen-kbdfront - drop keys to shrink modalias
From: Jason Andryuk @ 2023-03-08 16:26 UTC (permalink / raw)
To: Mattijs Korpershoek
Cc: linux-kernel, xen-devel, Phillip Susi, stable, Dmitry Torokhov,
linux-input
In-Reply-To: <87359gkc1d.fsf@baylibre.com>
On Thu, Dec 15, 2022 at 8:54 AM Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> On Fri, Dec 09, 2022 at 09:26, Jason Andryuk <jandryuk@gmail.com> wrote:
>
> > xen kbdfront registers itself as being able to deliver *any* key since
> > it doesn't know what keys the backend may produce.
> >
> > Unfortunately, the generated modalias gets too large and uevent creation
> > fails with -ENOMEM.
> >
> > This can lead to gdm not using the keyboard since there is no seat
> > associated [1] and the debian installer crashing [2].
> >
> > Trim the ranges of key capabilities by removing some BTN_* ranges.
> > While doing this, some neighboring undefined ranges are removed to trim
> > it further.
> >
> > An upper limit of KEY_KBD_LCD_MENU5 is still too large. Use an upper
> > limit of KEY_BRIGHTNESS_MENU.
> >
> > This removes:
> > BTN_DPAD_UP(0x220)..BTN_DPAD_RIGHT(0x223)
> > Empty space 0x224..0x229
> >
> > Empty space 0x28a..0x28f
> > KEY_MACRO1(0x290)..KEY_MACRO30(0x2ad)
> > KEY_MACRO_RECORD_START 0x2b0
> > KEY_MACRO_RECORD_STOP 0x2b1
> > KEY_MACRO_PRESET_CYCLE 0x2b2
> > KEY_MACRO_PRESET1(0x2b3)..KEY_MACRO_PRESET3(0xb5)
> > Empty space 0x2b6..0x2b7
> > KEY_KBD_LCD_MENU1(0x2b8)..KEY_KBD_LCD_MENU5(0x2bc)
> > Empty space 0x2bd..0x2bf
> > BTN_TRIGGER_HAPPY(0x2c0)..BTN_TRIGGER_HAPPY40(0x2e7)
> > Empty space 0x2e8..0x2ff
> >
> > The modalias shrinks from 2082 to 1550 bytes.
> >
> > A chunk of keys need to be removed to allow the keyboard to be used.
> > This may break some functionality, but the hope is these macro keys are
> > uncommon and don't affect any users.
> >
> > [1] https://github.com/systemd/systemd/issues/22944
> > [2] https://lore.kernel.org/xen-devel/87o8dw52jc.fsf@vps.thesusis.net/T/
> >
> > Cc: Phillip Susi <phill@thesusis.net>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
>
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Thank you, Mattjis.
Any other thoughts? Can this patch be applied?
Thanks,
Jason
^ permalink raw reply
* Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
From: Andy Shevchenko @ 2023-03-08 16:20 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Daniel Kaehn, Hans de Goede, robh+dt, krzysztof.kozlowski+dt,
jikos, bartosz.golaszewski, dmitry.torokhov, devicetree,
linux-input, ethan.twardy
In-Reply-To: <20230308152611.tae2pnmflakrcyhh@mail.corp.redhat.com>
On Wed, Mar 08, 2023 at 04:26:11PM +0100, Benjamin Tissoires wrote:
> On Mar 07 2023, Andy Shevchenko wrote:
> > On Tue, Mar 07, 2023 at 03:48:52PM +0100, Benjamin Tissoires wrote:
> > > On Mar 07 2023, Daniel Kaehn wrote:
...
> So I wonder if the cp2112 driver is correctly assigning the gc->parent
> field.
Seems it needs custom fwnode
gc->fwnode = child_of_cp_which_is_gpio;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
From: Benjamin Tissoires @ 2023-03-08 15:55 UTC (permalink / raw)
To: Daniel Kaehn
Cc: Andy Shevchenko, Hans de Goede, robh+dt, krzysztof.kozlowski+dt,
jikos, bartosz.golaszewski, dmitry.torokhov, devicetree,
linux-input, ethan.twardy
In-Reply-To: <CAP+ZCCcntCn4yaVKtTxDuDRvPgLXfP1kC7mYe2qKuhSGzVZMog@mail.gmail.com>
On Mar 08 2023, Daniel Kaehn wrote:
> On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > But if I refer "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", the IRQ is never assigned.
> > With the parent (CP21), it works.
> >
> > So I wonder if the cp2112 driver is correctly assigning the gc->parent
> > field.
>
>
> Did you make a change to the CP2112 driver patch to look for uppercase
> "I2C" and "GPIO"?
yes, sorry I should have mentioned it. This is the only modification I
have compared to the upstream kernel plus your patch series.
> Otherwise, it won't assign those child nodes appropriately, and the
> gpiochip code will use
> the parent node by default if the gpiochip's fwnode isn't assigned (I believe).
I don't think it's a fwnode issue, but a problem with the assignment of
the parent of the gc:
---
dev->gc.parent = &hdev->dev;
---
Because the function acpi_gpiochip_find() in drivers/gpio/gpiolib-acpi.c
compares the acpi handle returned by fetching the ACPI path
("\\_SB_.PCI0.USB0.RHUB.CP21.GPIO") and the one of gc->parent, which in
the hid-cp2112 case is the HID device itself.
>
> If that was indeed your problem all along (and I'm not missing
> something else), sorry about that --
> I made a comment above, but didn't add much spacing around it to make
> it stand out (since I noticed you didn't reply to that part in your response)
Yeah, sorry I should have been explicit about this.
For reference, I am appending the full SSDT override which works.
Even if you don't have an i2c-hid device connected, this should at
least call the probe function in i2c-hid-core.c, which is a proof that
the ACPI binding is properly done (the first SMBus read will fail with a
timeout)
Also, I played around with the _DSD that Andy was mentioning (and some
others), hopefully this will help you getting the mapping from the
"cell-names" to the fwnode child index faster.
Cheers,
Benjamin
---
DefinitionBlock ("cp2112.aml", "SSDT", 5, "", "CP2112", 1)
{
External (_SB_.PCI0, DeviceObj)
Scope (\_SB_.PCI0)
{
Device (USB0)
{
Name (_ADR, 0x00010002) // _ADR: Address
Device (RHUB)
{
Name (_ADR, Zero)
Device (CP21) // the USB-hid & CP2112 shared node
{
Name (_ADR, One)
Name (_DSD, Package ()
{
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () { "cell-names", Package () {
"i2c",
"gpio",
}
}
}
})
Device (I2C)
{
Name (_ADR, Zero)
Name (_STA, 0x0F)
}
Device (GPIO)
{
Name (_ADR, One)
Name (_STA, 0x0F)
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () { "gpio-hog", 1 },
Package () { "gpios", Package () { 4, 0 } },
Package () { "output-high", 1 },
Package () { "line-name", "gpio4-pullup" },
},
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () { "gpio-line-names", Package () {
"",
"",
"irq-rmi4",
"",
"power", // set to 1 with gpio-hog above
"",
"",
"",
""}},
}
})
}
}
}
}
}
Scope (\_SB_.PCI0.USB0.RHUB.CP21.I2C)
{
Device (TPD0)
{
Name (_HID, "RMI40001")
Name (_CID, "PNP0C50")
Name (_STA, 0x0F)
Name (SBFB, ResourceTemplate ()
{
I2cSerialBusV2 (0x2c, ControllerInitiated, 100000,
AddressingMode7Bit, "\\_SB_.PCI0.USB0.RHUB.CP21.I2C",
0x00, ResourceConsumer,, Exclusive,
)
})
Name (SBFG, ResourceTemplate ()
{
GpioInt (Level, ActiveLow, Exclusive, PullDefault, 0x0000,
"\\_SB_.PCI0.USB0.RHUB.CP21", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0002
}
})
Name (_DSD, Package ()
{
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package ()
{
Package () { "irq-gpios", Package () { ^TPD0, 1, 0, 0 } },
}
})
Method(_CRS, 0x0, NotSerialized)
{
Return (ConcatenateResTemplate (SBFG, SBFB))
}
Method(_DSM, 0x4, Serialized)
{
// DSM UUID
switch (ToBuffer (Arg0))
{
// ACPI DSM UUID for HIDI2C
case (ToUUID ("3CDFF6F7-4267-4555-AD05-B30A3D8938DE"))
{
// DSM Function
switch (ToInteger (Arg2))
{
// Function 0: Query function, return based on revision
case(0)
{
// DSM Revision
switch (ToInteger (Arg1))
{
// Revision 1: Function 1 supported
case (1)
{
Return (Buffer (One) { 0x03 })
}
default
{
// Revision 2+: no functions supported
Return (Buffer (One) { 0x00 })
}
}
}
// Function 1 : HID Function
case(1)
{
// HID Descriptor Address
Return (0x0020)
}
default
{
// Functions 2+: not supported
Return (Buffer (One) { 0x00 })
}
}
}
default
{
// No other GUIDs supported
Return (Buffer (One) { 0x00 })
}
}
}
}
}
}
---
^ permalink raw reply
* Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
From: Daniel Kaehn @ 2023-03-08 15:37 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Andy Shevchenko, Hans de Goede, robh+dt, krzysztof.kozlowski+dt,
jikos, bartosz.golaszewski, dmitry.torokhov, devicetree,
linux-input, ethan.twardy
In-Reply-To: <20230308152611.tae2pnmflakrcyhh@mail.corp.redhat.com>
On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Mar 07 2023, Andy Shevchenko wrote:
> > On Tue, Mar 07, 2023 at 03:48:52PM +0100, Benjamin Tissoires wrote:
> > > On Mar 07 2023, Daniel Kaehn wrote:
> >
> > ...
> >
> > > So I can see that the device gets probed, and that all ACPI resources
> > > are tried to get the IRQ.
> > > Right now, I see that it's attempting to bind to the acpi resource in
> > > acpi_dev_resource_interrupt() (in file drivers/acpi/resources.c), but
> > > instead of having a ACPI_RESOURCE_TYPE_EXTENDED_IRQ I only get a
> > > ACPI_RESOURCE_TYPE_GPIO for the GpioInt() definition in the _CRS method.
> > >
> > > So I am missing the proper transition from GpioInt to IRQ in the acpi.
> >
> > I'm not sure I understand what this means.
> >
> > The Linux kernel takes either Interrupt() resource (which is
> > IOxAPIC / GIC / etc) or GpioInt() (which is GPIO based).
> >
> > In both cases I²C framework submits this into client's IRQ field.
> >
>
> I finally managed to get past the retrieval of the GpioInt.
>
> Turns out that the function acpi_get_gpiod() matches on the parent of
> the gpio_chip (gc->parent), which means that, with the current code and
> my SSDT, it matches on the HID CP2112 ACPI node, not the GPIO one.
>
> For reference (with lots of boiler plate removed):
>
> Device (CP21) { // the USB-hid & CP2112 shared node
> Device (GPIO) {
> Name (_ADR, One)
> Name (_STA, 0x0F)
>
> Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () { "gpio-line-names", Package () {
> "",
> "",
> "irq-rmi4",
> "",
> "power", // set to 1 with gpio-hog above
> "",
> "",
> "",
> ""}},
> }
> })
> }
>
> Scope (\_SB_.PCI0.USB0.RHUB.CP21.I2C)
> {
> Device (TPD0)
> {
> Name (_HID, "RMI40001")
> Name (_CID, "PNP0C50")
> Name (_STA, 0x0F)
>
> Name (SBFB, ResourceTemplate ()
> {
> I2cSerialBusV2 (0x00c, ControllerInitiated, 100000,
> AddressingMode7Bit, "\\_SB_.PCI0.USB0.RHUB.CP21.I2C",
> 0x00, ResourceConsumer,, Exclusive,
> )
> })
> Name (SBFG, ResourceTemplate ()
> {
> GpioInt (Level, ActiveLow, Exclusive, PullDefault, 0x0000,
> "\\_SB_.PCI0.USB0.RHUB.CP21", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0002
> }
> })
> ---
>
> But if I refer "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", the IRQ is never assigned.
> With the parent (CP21), it works.
>
> So I wonder if the cp2112 driver is correctly assigning the gc->parent
> field.
Did you make a change to the CP2112 driver patch to look for uppercase
"I2C" and "GPIO"?
Otherwise, it won't assign those child nodes appropriately, and the
gpiochip code will use
the parent node by default if the gpiochip's fwnode isn't assigned (I believe).
If that was indeed your problem all along (and I'm not missing
something else), sorry about that --
I made a comment above, but didn't add much spacing around it to make
it stand out (since I noticed you didn't reply to that part in your response)
> To get this to work -- I assume you had to change the driver to look
> for uppercase
> "GPIO" and "I2C", or some similar change?
Thanks,
Danny Kaehn
^ permalink raw reply
* Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
From: Benjamin Tissoires @ 2023-03-08 15:26 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Daniel Kaehn, Hans de Goede, robh+dt, krzysztof.kozlowski+dt,
jikos, bartosz.golaszewski, dmitry.torokhov, devicetree,
linux-input, ethan.twardy
In-Reply-To: <ZAeADcJWmJR+1ycJ@smile.fi.intel.com>
On Mar 07 2023, Andy Shevchenko wrote:
> On Tue, Mar 07, 2023 at 03:48:52PM +0100, Benjamin Tissoires wrote:
> > On Mar 07 2023, Daniel Kaehn wrote:
>
> ...
>
> > So I can see that the device gets probed, and that all ACPI resources
> > are tried to get the IRQ.
> > Right now, I see that it's attempting to bind to the acpi resource in
> > acpi_dev_resource_interrupt() (in file drivers/acpi/resources.c), but
> > instead of having a ACPI_RESOURCE_TYPE_EXTENDED_IRQ I only get a
> > ACPI_RESOURCE_TYPE_GPIO for the GpioInt() definition in the _CRS method.
> >
> > So I am missing the proper transition from GpioInt to IRQ in the acpi.
>
> I'm not sure I understand what this means.
>
> The Linux kernel takes either Interrupt() resource (which is
> IOxAPIC / GIC / etc) or GpioInt() (which is GPIO based).
>
> In both cases I²C framework submits this into client's IRQ field.
>
I finally managed to get past the retrieval of the GpioInt.
Turns out that the function acpi_get_gpiod() matches on the parent of
the gpio_chip (gc->parent), which means that, with the current code and
my SSDT, it matches on the HID CP2112 ACPI node, not the GPIO one.
For reference (with lots of boiler plate removed):
Device (CP21) { // the USB-hid & CP2112 shared node
Device (GPIO) {
Name (_ADR, One)
Name (_STA, 0x0F)
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () { "gpio-line-names", Package () {
"",
"",
"irq-rmi4",
"",
"power", // set to 1 with gpio-hog above
"",
"",
"",
""}},
}
})
}
Scope (\_SB_.PCI0.USB0.RHUB.CP21.I2C)
{
Device (TPD0)
{
Name (_HID, "RMI40001")
Name (_CID, "PNP0C50")
Name (_STA, 0x0F)
Name (SBFB, ResourceTemplate ()
{
I2cSerialBusV2 (0x00c, ControllerInitiated, 100000,
AddressingMode7Bit, "\\_SB_.PCI0.USB0.RHUB.CP21.I2C",
0x00, ResourceConsumer,, Exclusive,
)
})
Name (SBFG, ResourceTemplate ()
{
GpioInt (Level, ActiveLow, Exclusive, PullDefault, 0x0000,
"\\_SB_.PCI0.USB0.RHUB.CP21", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0002
}
})
---
But if I refer "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", the IRQ is never assigned.
With the parent (CP21), it works.
So I wonder if the cp2112 driver is correctly assigning the gc->parent
field.
Cheers,
Benjamin
^ permalink raw reply
* [PATCH v4 2/2] Input: hideep - Optionally reset controller work mode to native HiDeep protocol
From: Hans de Goede @ 2023-03-08 14:27 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Hans de Goede, linux-input
In-Reply-To: <20230308142750.52870-1-hdegoede@redhat.com>
The HiDeep IST940E touchscreen controller used on the Lenovo Yoga Book X90F
convertible comes up in HID mode by default.
This works well on the X91F Windows model where the touchscreen is
correctly described in ACPI and ACPI takes care of controlling
the reset GPIO and regulators.
But the X90F ships with Android and the ACPI tables on this model don't
describe the touchscreen. Instead this is hardcoded in the vendor kernel.
The vendor kernel uses the touchscreen in native HiDeep 20 (2.0?) protocol
mode and switches the controller to this mode by writing 0 to reg 0x081e.
Adjusting the i2c-hid code to deal with the reset-gpio and regulators on
this non devicetree (but rather broken ACPI) convertible is somewhat tricky
and the native protocol reports ABS_MT_PRESSURE and ABS_MT_TOUCH_MAJOR
which are not reported in HID mode, so it is preferable to use the native
mode.
Add support to the hideep driver to reset the work-mode to the native
HiDeep protocol to allow using it on the Lenovo Yoga Book X90F.
This is guarded behind a new "hideep,reset-work-mode" boolean property,
to avoid changing behavior on other devices.
For the record: I did test using the i2c-hid driver with some quick hacks
and it does work. The I2C-HID descriptor is available from address 0x0020,
just like on the X91F Windows model.
So far the new "hideep,reset-work-mode" property is only used on x86/ACPI
(non devicetree) devs. IOW it is not used in actual devicetree files.
The devicetree-bindings maintainers have requested properties like these
to not be added to the devicetree-bindings, so the new property is
deliberately not added to the existing devicetree-bindings.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Do not add the x86/acpi only property to the dt-bindings doc
Changes in v2:
- New patch in v2 of this patch-set
---
drivers/input/touchscreen/hideep.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/input/touchscreen/hideep.c b/drivers/input/touchscreen/hideep.c
index 5e6e43d56448..914973991098 100644
--- a/drivers/input/touchscreen/hideep.c
+++ b/drivers/input/touchscreen/hideep.c
@@ -35,6 +35,7 @@
#define HIDEEP_EVENT_ADDR 0x240
/* command list */
+#define HIDEEP_WORK_MODE 0x081e
#define HIDEEP_RESET_CMD 0x9800
/* event bit */
@@ -964,6 +965,17 @@ static const struct attribute_group hideep_ts_attr_group = {
.attrs = hideep_ts_sysfs_entries,
};
+static void hideep_set_work_mode(struct hideep_ts *ts)
+{
+ /*
+ * Reset touch report format to the native HiDeep 20 protocol if requested.
+ * This is necessary to make touchscreens which come up in I2C-HID mode
+ * work with this driver.
+ */
+ if (device_property_read_bool(&ts->client->dev, "hideep,reset-work-mode"))
+ regmap_write(ts->reg, HIDEEP_WORK_MODE, 0x00);
+}
+
static int hideep_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -987,6 +999,8 @@ static int hideep_resume(struct device *dev)
return error;
}
+ hideep_set_work_mode(ts);
+
enable_irq(client->irq);
return 0;
@@ -1063,6 +1077,8 @@ static int hideep_probe(struct i2c_client *client)
return error;
}
+ hideep_set_work_mode(ts);
+
error = hideep_init_input(ts);
if (error)
return error;
--
2.39.1
^ permalink raw reply related
* [PATCH v4 1/2] Input: hideep - Silence error in SW_RESET()
From: Hans de Goede @ 2023-03-08 14:27 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Hans de Goede, linux-input
In-Reply-To: <20230308142750.52870-1-hdegoede@redhat.com>
On some models the first HIDEEP_SYSCON_WDT_CON write alone is enough to
cause the controller to reset, causing the second write to fail:
i2c-hideep_ts: write to register 0x52000014 (0x000001) failed: -121
Switch this write to a raw hideep_pgm_w_mem() to avoid an error getting
logged in this case.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/touchscreen/hideep.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/hideep.c b/drivers/input/touchscreen/hideep.c
index bd454d93f1f7..5e6e43d56448 100644
--- a/drivers/input/touchscreen/hideep.c
+++ b/drivers/input/touchscreen/hideep.c
@@ -271,9 +271,14 @@ static int hideep_pgm_w_reg(struct hideep_ts *ts, u32 addr, u32 val)
#define SW_RESET_IN_PGM(clk) \
{ \
+ __be32 data = cpu_to_be32(0x01); \
hideep_pgm_w_reg(ts, HIDEEP_SYSCON_WDT_CNT, (clk)); \
hideep_pgm_w_reg(ts, HIDEEP_SYSCON_WDT_CON, 0x03); \
- hideep_pgm_w_reg(ts, HIDEEP_SYSCON_WDT_CON, 0x01); \
+ /* \
+ * The first write may already cause a reset, use a raw \
+ * write for the second write to avoid error logging. \
+ */ \
+ hideep_pgm_w_mem(ts, HIDEEP_SYSCON_WDT_CON, &data, 1); \
}
#define SET_FLASH_PIO(ce) \
--
2.39.1
^ permalink raw reply related
* [PATCH v4 0/2] Input: hideep - 2 HiDeep touchscreen patches
From: Hans de Goede @ 2023-03-08 14:27 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Hans de Goede, linux-input
Hi Dmitry,
Here are 2 patches for the HiDeep touchscreen driver.
Sorry about the quick version bump / spam, this is just
a rebase to fix a conflict and I really should have rebased
before sending out v3 yesterday.
Changes in v4:
- Rebase on v6.3-rc1
Changes in v3:
- Do not add the x86/acpi only "hideep,reset-work-mode" property to
the dt-bindings doc
Changes in v2:
- Add "Optionally reset controller work mode to native HiDeep protocol"
patch
Regards,
Hans
Hans de Goede (2):
Input: hideep - Silence error in SW_RESET()
Input: hideep - Optionally reset controller work mode to native HiDeep
protocol
drivers/input/touchscreen/hideep.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
--
2.39.1
^ permalink raw reply
* [PATCH V2 RESEND 3/3] Input: elantech - Fix incorrectly halved touchpad range on ELAN v3 touchpads
From: Eirin Nya @ 2023-03-08 13:13 UTC (permalink / raw)
To: dmitry.torokhov
Cc: mkorpershoek, phoenix, josh.chen, linux-input, linux-kernel,
Eirin Nya
In-Reply-To: <20230308131307.13832-1-nyanpasu256@gmail.com>
On Linux 6.1.15, on my laptop (Dell Inspiron 15R SE 7520) with an Elan
v3 touchpad (dmesg says "with firmware version 0x450f02"), the reported
size of my touchpad (in userspace by calling mtdev_configure() and
libevdev_get_abs_maximum(), in kernel space
elantech_device_info::x_max/y_max, either way 1470 by 700) is half that
of the actual touch range (2940 by 1400), and the upper half of my
touchpad reports negative values. As a result, with the Synaptics or
libinput X11 driver set to edge scrolling mode, the entire right half of
my touchpad has x-values past evdev's reported maximum size, and acts as
a giant scrollbar!
The problem is that elantech_setup_ps2() -> elantech_set_absolute_mode()
sets up absolute mode and doubles the hardware resolution (doubling the
hardware's maximum reported x/y coordinates and its response to
ETP_FW_ID_QUERY), *after* elantech_query_info() fetches the touchpad
coordinate system size using ETP_FW_ID_QUERY, which gets cached and
reported to userspace through ioctl(fd, EVIOCGABS(ABS_X/Y), ...). So the
touchpad size reported to userspace (and used to subtract vertical
coordinates from) is half the maximum position of actual touches.
This patch splits out a function elantech_query_range_v3() which fetches
*only* ETP_FW_ID_QUERY (touchpad size), and calls it a second time if
elantech_set_absolute_mode() enables double-size mode. This means the
first call is redundant and wasted if a second call occurs, but this
minimizes the need to restructure the driver.
Link: https://lore.kernel.org/linux-input/CAL57YxZNutUVxBtvbVWKMw-V2kqeVB5kTQ5BFdJmN=mdPq8Q8Q@mail.gmail.com/
Link: https://lore.kernel.org/linux-input/20221008093437.72d0f6b0@dell-void.nyanpasu256.gmail.com.beta.tailscale.net/
Fixes: 37548659bb22 ("Input: elantech - query the min/max information beforehand too")
Signed-off-by: Eirin Nya <nyanpasu256@gmail.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
drivers/input/mouse/elantech.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 263779c0313b..a2176f0fd36c 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -1006,6 +1006,9 @@ static void elantech_set_rate_restore_reg_07(struct psmouse *psmouse,
psmouse_err(psmouse, "restoring reg_07 failed\n");
}
+static int elantech_query_range_v3(struct psmouse *psmouse,
+ struct elantech_device_info *info);
+
/*
* Put the touchpad into absolute mode
*/
@@ -1047,6 +1050,14 @@ static int elantech_set_absolute_mode(struct psmouse *psmouse)
if (elantech_write_reg(psmouse, 0x10, etd->reg_10))
rc = -1;
+ /*
+ * If we boost hardware resolution, we have to re-query
+ * info->x_max and y_max.
+ */
+ if (etd->info.set_hw_resolution)
+ if (elantech_query_range_v3(psmouse, &etd->info))
+ rc = -1;
+
break;
case 4:
@@ -1671,6 +1682,20 @@ static int elantech_set_properties(struct elantech_device_info *info)
return 0;
}
+static int elantech_query_range_v3(struct psmouse *psmouse,
+ struct elantech_device_info *info)
+{
+ unsigned char param[3];
+
+ if (info->send_cmd(psmouse, ETP_FW_ID_QUERY, param))
+ return -EINVAL;
+
+ info->x_max = (0x0f & param[0]) << 8 | param[1];
+ info->y_max = (0xf0 & param[0]) << 4 | param[2];
+
+ return 0;
+}
+
static int elantech_query_info(struct psmouse *psmouse,
struct elantech_device_info *info)
{
@@ -1826,11 +1851,8 @@ static int elantech_query_info(struct psmouse *psmouse,
break;
case 3:
- if (info->send_cmd(psmouse, ETP_FW_ID_QUERY, param))
+ if (elantech_query_range_v3(psmouse, info))
return -EINVAL;
-
- info->x_max = (0x0f & param[0]) << 8 | param[1];
- info->y_max = (0xf0 & param[0]) << 4 | param[2];
break;
case 4:
--
2.39.2
^ permalink raw reply related
* [PATCH V2 RESEND 2/3] Input: elantech - Remove redundant field elantech_data::width
From: Eirin Nya @ 2023-03-08 13:13 UTC (permalink / raw)
To: dmitry.torokhov
Cc: mkorpershoek, phoenix, josh.chen, linux-input, linux-kernel,
Eirin Nya
In-Reply-To: <20230308131307.13832-1-nyanpasu256@gmail.com>
elantech_data::width is copied from elantech_device_info::width, and
neither is mutated after initialization. So remove the redundant
variable to prevent future bugs.
Signed-off-by: Eirin Nya <nyanpasu256@gmail.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
drivers/input/mouse/elantech.c | 4 +---
drivers/input/mouse/elantech.h | 1 -
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 79e31611fc6e..263779c0313b 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -691,7 +691,7 @@ static void process_packet_head_v4(struct psmouse *psmouse)
input_report_abs(dev, ABS_MT_POSITION_X, etd->mt[id].x);
input_report_abs(dev, ABS_MT_POSITION_Y, etd->mt[id].y);
input_report_abs(dev, ABS_MT_PRESSURE, pres);
- input_report_abs(dev, ABS_MT_TOUCH_MAJOR, traces * etd->width);
+ input_report_abs(dev, ABS_MT_TOUCH_MAJOR, traces * etd->info.width);
/* report this for backwards compatibility */
input_report_abs(dev, ABS_TOOL_WIDTH, traces);
@@ -1253,8 +1253,6 @@ static int elantech_set_input_params(struct psmouse *psmouse)
input_abs_set_res(dev, ABS_MT_POSITION_Y, info->y_res);
}
- etd->width = width;
-
return 0;
}
diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
index 1ec004f72d13..fb093134ead7 100644
--- a/drivers/input/mouse/elantech.h
+++ b/drivers/input/mouse/elantech.h
@@ -180,7 +180,6 @@ struct elantech_data {
unsigned char reg_25;
unsigned char reg_26;
unsigned int single_finger_reports;
- unsigned int width;
struct finger_pos mt[ETP_MAX_FINGERS];
unsigned char parity[256];
struct elantech_device_info info;
--
2.39.2
^ permalink raw reply related
* [PATCH V2 RESEND 1/3] Input: elantech - Remove redundant field elantech_data::y_max
From: Eirin Nya @ 2023-03-08 13:13 UTC (permalink / raw)
To: dmitry.torokhov
Cc: mkorpershoek, phoenix, josh.chen, linux-input, linux-kernel,
Eirin Nya
In-Reply-To: <20230308131307.13832-1-nyanpasu256@gmail.com>
elantech_data::y_max is copied from elantech_device_info::y_max, and
neither is mutated after initialization. So remove the redundant
variable to prevent future bugs when updating y_max.
Signed-off-by: Eirin Nya <nyanpasu256@gmail.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
drivers/input/mouse/elantech.c | 17 ++++++++---------
drivers/input/mouse/elantech.h | 1 -
2 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index ece97f8c6a3e..79e31611fc6e 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -360,7 +360,7 @@ static void elantech_report_absolute_v1(struct psmouse *psmouse)
input_report_abs(dev, ABS_X,
((packet[1] & 0x0c) << 6) | packet[2]);
input_report_abs(dev, ABS_Y,
- etd->y_max - (((packet[1] & 0x03) << 8) | packet[3]));
+ etd->info.y_max - (((packet[1] & 0x03) << 8) | packet[3]));
}
input_report_key(dev, BTN_TOOL_FINGER, fingers == 1);
@@ -435,7 +435,7 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
* byte 4: . . . . y11 y10 y9 y8
* byte 5: y7 y6 y5 y4 y3 y2 y1 y0
*/
- y1 = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
+ y1 = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
pres = (packet[1] & 0xf0) | ((packet[4] & 0xf0) >> 4);
width = ((packet[0] & 0x30) >> 2) | ((packet[3] & 0x30) >> 4);
@@ -450,7 +450,7 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
*/
x1 = (((packet[0] & 0x10) << 4) | packet[1]) << 2;
/* byte 2: ay7 ay6 ay5 ay4 ay3 ay2 ay1 ay0 */
- y1 = etd->y_max -
+ y1 = etd->info.y_max -
((((packet[0] & 0x20) << 3) | packet[2]) << 2);
/*
* byte 3: . . by8 bx8 . . . .
@@ -458,7 +458,7 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
*/
x2 = (((packet[3] & 0x10) << 4) | packet[4]) << 2;
/* byte 5: by7 by8 by5 by4 by3 by2 by1 by0 */
- y2 = etd->y_max -
+ y2 = etd->info.y_max -
((((packet[3] & 0x20) << 3) | packet[5]) << 2);
/* Unknown so just report sensible values */
@@ -579,7 +579,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
* byte 4: . . . . y11 y10 y9 y8
* byte 5: y7 y6 y5 y4 y3 y2 y1 y0
*/
- y1 = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
+ y1 = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
break;
case 2:
@@ -593,7 +593,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
* byte 4: . . . . ay11 ay10 ay9 ay8
* byte 5: ay7 ay6 ay5 ay4 ay3 ay2 ay1 ay0
*/
- etd->mt[0].y = etd->y_max -
+ etd->mt[0].y = etd->info.y_max -
(((packet[4] & 0x0f) << 8) | packet[5]);
/*
* wait for next packet
@@ -605,7 +605,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
x1 = etd->mt[0].x;
y1 = etd->mt[0].y;
x2 = ((packet[1] & 0x0f) << 8) | packet[2];
- y2 = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
+ y2 = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
break;
}
@@ -681,7 +681,7 @@ static void process_packet_head_v4(struct psmouse *psmouse)
return;
etd->mt[id].x = ((packet[1] & 0x0f) << 8) | packet[2];
- etd->mt[id].y = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
+ etd->mt[id].y = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
pres = (packet[1] & 0xf0) | ((packet[4] & 0xf0) >> 4);
traces = (packet[0] & 0xf0) >> 4;
@@ -1253,7 +1253,6 @@ static int elantech_set_input_params(struct psmouse *psmouse)
input_abs_set_res(dev, ABS_MT_POSITION_Y, info->y_res);
}
- etd->y_max = y_max;
etd->width = width;
return 0;
diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
index 571e6ca11d33..1ec004f72d13 100644
--- a/drivers/input/mouse/elantech.h
+++ b/drivers/input/mouse/elantech.h
@@ -180,7 +180,6 @@ struct elantech_data {
unsigned char reg_25;
unsigned char reg_26;
unsigned int single_finger_reports;
- unsigned int y_max;
unsigned int width;
struct finger_pos mt[ETP_MAX_FINGERS];
unsigned char parity[256];
--
2.39.2
^ permalink raw reply related
* [PATCH V2 RESEND 0/3] Input: elantech - Fix incorrectly halved touchpad range on ELAN v3 touchpads
From: Eirin Nya @ 2023-03-08 13:13 UTC (permalink / raw)
To: dmitry.torokhov
Cc: mkorpershoek, phoenix, josh.chen, linux-input, linux-kernel,
Eirin Nya
On Linux 6.1.15, on my laptop (Dell Inspiron 15R SE 7520) with an Elan
v3 touchpad, the reported size of my touchpad (1470 by 700) is half that
of the actual touch range (2940 by 1400), and the upper half of my
touchpad reports negative values. As a result, with the Synaptics or
libinput X11 driver set to edge scrolling mode, the entire right half of
my touchpad has x-values past evdev's reported maximum size, and acts as
a giant scrollbar!
The problem is that elantech_setup_ps2() -> elantech_set_absolute_mode()
sets up absolute mode and doubles the hardware resolution (doubling the
hardware's maximum reported x/y coordinates and its response to
ETP_FW_ID_QUERY), *after* elantech_query_info() fetches the touchpad
coordinate system size using ETP_FW_ID_QUERY, which gets cached and
reported to userspace through ioctl(fd, EVIOCGABS(ABS_X/Y), ...). So the
touchpad size reported to userspace (and used to subtract vertical
coordinates from) is half the maximum position of actual touches.
This patch series splits out a function elantech_query_range_v3() which
fetches *only* ETP_FW_ID_QUERY (touchpad size), and calls it a second
time if elantech_set_absolute_mode() enables double-size mode. This
means the first call is redundant and wasted if a second call occurs,
but this minimizes the need to restructure the driver.
A possible alternative approach is to restructure the driver and set
resolution before querying touchpad size. I did not attempt this myself,
and don't know how the Windows Dell Touchpad ELAN driver handles
double-resolution. See
https://lore.kernel.org/linux-input/20221008093437.72d0f6b0@dell-void.nyanpasu256.gmail.com.beta.tailscale.net/
for discussion.
Changes in v2:
- Fix commit summaries
- Add "Fixes:" tag holding commit that introduced bug
Tested on 6.1.15, rebased and built on dtor/next
d5f7638eb5fed0eb12e45a127764c4111b11c50e (though against 6.1.15 headers,
and the resulting binaries were not tested). The files changed
(drivers/input/mouse/elantech.c/h) have not been modified since 2021.
Link: https://lore.kernel.org/linux-input/CAL57YxZNutUVxBtvbVWKMw-V2kqeVB5kTQ5BFdJmN=mdPq8Q8Q@mail.gmail.com/
Link v1: https://lore.kernel.org/linux-input/20220929082119.22112-1-nyanpasu256@gmail.com/
Link v2: https://lore.kernel.org/linux-input/20221014111533.908-1-nyanpasu256@gmail.com/
Eirin Nya (3):
Input: elantech - Remove redundant field elantech_data::y_max
Input: elantech - Remove redundant field elantech_data::width
Input: elantech - Fix incorrectly halved touchpad range on ELAN v3
touchpads
drivers/input/mouse/elantech.c | 51 +++++++++++++++++++++++-----------
drivers/input/mouse/elantech.h | 2 --
2 files changed, 35 insertions(+), 18 deletions(-)
base-commit: d5f7638eb5fed0eb12e45a127764c4111b11c50e
--
2.39.2
^ permalink raw reply
* Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
From: Andy Shevchenko @ 2023-03-08 12:41 UTC (permalink / raw)
To: Daniel Kaehn
Cc: Benjamin Tissoires, Hans de Goede, robh+dt,
krzysztof.kozlowski+dt, jikos, bartosz.golaszewski,
dmitry.torokhov, devicetree, linux-input, ethan.twardy
In-Reply-To: <CAP+ZCCe6mdtNmg0QEtQKFCKMyhM9xRffFoMHtxHEGnjNOEAedg@mail.gmail.com>
On Tue, Mar 07, 2023 at 01:57:27PM -0600, Daniel Kaehn wrote:
> On Tue, Mar 7, 2023 at 12:14 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Mar 07, 2023 at 02:17:06PM +0100, Benjamin Tissoires wrote:
> > > On Mar 06 2023, Andy Shevchenko wrote:
...
> > > It is commonly accepted in the ACPI world that the names do not carry
> > > meaning AFAICT, and so I think I agree with Andy's initial comment
> > > regarding using indexes, not names to also fetch the I2C and GPIO nodes.
> > > You can probably have a fallback mechanism for when "i2c" is not
> > > present, or simply check if you are in DT or not and use the names only
> > > if we are in DT.
> >
> > The solution is to provide in the main node the list of cell names, that way
> > you will always know the indices:
> >
> > Device (DEV) {
> > _DSD
> > "cell-names" { "i2c", "gpio" } // index of the name is the
> > // index of the cell
> >
> > Device (I2C0) {
> > }
> >
> > Device (GPI0) {
> > }
> > }
> >
> > Problem solved.
>
> Just to make sure I'm understanding you correctly:
>
> Are you proposing that specifically this driver directly reads "cell-names"
> from the ACPI DSD to implement this indexing? Or are you proposing a
> kernel-wide mechanism of "overriding" a fwnode name with ACPI?
> (assuming this doesn't exist already, and I'm not just missing it in
> the kernel source)
>
> Or are you proposing something else entirely?
> (apologies if this should be obvious -- throwing up the ACPI newbie
> card again here :) )
Out of the three listed above I'm proposing the first one.
(Maybe it can be called 'reg-names', but it depends on the hardware)
Also look into hierarchical _DSD (it is similar to the children
of the device node without being separate devices).
> In any case, would this be something I should post to the email chain
> with DT and ACPI
> folks for opinions before I start to implement it?
May be, the -names approach is already used widely for clock, interrupt, reset,
phy and nvmem. So it's something we know about.
--
With Best Regards,
Andy Shevchenko
^ 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