* Re: [PATCH 12/14] iio: adc: at91-sama5d2_adc: support for position and pressure channels
From: Jonathan Cameron @ 2018-01-14 10:47 UTC (permalink / raw)
To: Ludovic Desroches
Cc: Eugen Hristev, nicolas.ferre, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, linux-input,
dmitry.torokhov
In-Reply-To: <20180108141252.GC2425@rfolt0960.corp.atmel.com>
On Mon, 8 Jan 2018 15:12:52 +0100
Ludovic Desroches <ludovic.desroches@microchip.com> wrote:
> Hi Jonathan, Eugen,
>
> On Sat, Jan 06, 2018 at 03:05:37PM +0000, Jonathan Cameron wrote:
> > On Thu, 4 Jan 2018 17:17:54 +0200
> > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> >
> > > On 29.12.2017 19:02, Jonathan Cameron wrote:
> > > > On Fri, 22 Dec 2017 17:07:19 +0200
> > > > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> > > >
> > > >> The ADC IP supports position and pressure measurements for a touchpad
> > > >> connected on channels 0,1,2,3 for a 4-wire touchscreen with pressure
> > > >> measurement support.
> > > >> Using the inkern API, a driver can request a trigger and read the
> > > >> channel values from the ADC.
> > > >> The implementation provides a trigger named "touch" which can be
> > > >> connected to a consumer driver.
> > > >> Once a driver connects and attaches a pollfunc to this trigger, the
> > > >> configure trigger callback is called, and then the ADC driver will
> > > >> initialize pad measurement.
> > > >> First step is to enable touchscreen 4wire support and enable
> > > >> pen detect IRQ.
> > > >> Once a pen is detected, a periodic trigger is setup to trigger every
> > > >> 2 ms (e.g.) and sample the resistive touchscreen values. The trigger poll
> > > >> is called, and the consumer driver is then woke up, and it can read the
> > > >> respective channels for the values : X, and Y for position and pressure
> > > >> channel.
> > > >> Because only one trigger can be active in hardware in the same time,
> > > >> while touching the pad, the ADC will block any attempt to use the
> > > >> triggered buffer. Same, conversions using the software trigger are also
> > > >> impossible (since the periodic trigger is setup).
> > > >> If some driver wants to attach while the trigger is in use, it will
> > > >> also fail.
> > > >> Once the pen is not detected anymore, the trigger is free for use (hardware
> > > >> or software trigger, with or without DMA).
> > > >> Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled.
> > > >>
> > > >> Some parts of this patch are based on initial original work by
> > > >> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy
> > > >>
> > > > OK, so comments inline.
> > > >
> > > > What I'm missing currently though is an explanation of why the slightly
> > > > more standard arrangement of using a callback buffer doesn't work here.
> > > > The only addition I think you need to do that is to allow a consumer to
> > > > request a particular trigger. I also think some of the other provisions
> > > > could be handled using standard features and slightly reducing the flexibility.
> > > > I don't know for example if it's useful to allow other channels to be
> > > > read when touch is not in progress or not.
> > > >
> > > > So restrictions:
> > > >
> > > > 1. Touch screen channels can only be read when touch is enabled.
> > > > - use the available_scan_masks to control this. Or the callback that lets
> > > > you do the same dynamically.
> > > > 2. You need to push these channels to your consumer driver.
> > > > - register a callback buffer rather than jumping through the hoops to
> > > > insert your own pollfunc. That will call a function in your
> > > > consumer, providing the data from the 3 channels directly.
> > > > 3. You need to make sure it is using the right driver. For that you
> > > > will I think need a new interface.
> > > >
> > > > Various other comments inline. I may well be missing something as this is
> > > > a fair bit of complex code to read - if so then next version should have
> > > > a clear cover letter describing why this more standard approach can't be
> > > > used.
> > >
> > > Hello Jonathan and thanks for the review of my patch series,
> > >
> > > before starting and working over the required modifications and
> > > suggestions that you sent me, I want to be a little more explicit about
> > > the design of my implementation.
> > > Hope this will clarify some things, and maybe I can as well understand
> > > better what you have in mind to support this feature set.
> > >
> > > Why have I picked a pollfunction: We discussed a while back on the
> > > mailing list that you do not have an inkern mechanism to expose the
> > > triggers to other drivers, and that it may be a good idea to have it for
> > > such kind of actually multi function device, instead of having a MFD
> > > driver, an ADC driver, and an Input driver, all sharing the same
> > > register map, the same IRQ , etc, with some kind of synchronization to
> > > avoid stepping on each other for the hardware resource.
> >
> > No disagreement with that principle.
> >
> > > So I considered to expose the trigger by attaching and detaching
> > > pollfunctions to it. Which is the main thing what we use a trigger for.
> >
> > Hmm. It's definitely one approach. But we do already have other drivers
> > where the trigger is controlled by a consumer and to my mind that
> > is a cleaner approach as it doesn't short cut the equivalent of
> > doing it from userspace.
> >
> > drivers/iio/potentiostat/lmp91000.c does something similar though
> > for a rather different use. You need your consumer interface
> > to get the handle to the trigger in this case
> > (the lmp91000 is actually providing the trigger rather than
> > consuming it).
> >
> >
> > >
> > > So, what I had in mind, was to create a consumer driver that will
> > > request triggers from the IIO device just like other drivers request
> > > channels (part which is already done in IIO).
> > > In order to do this I had to somehow wake up the consumer driver when
> > > new data was available from the touchscreen. So, having the IRQ only in
> > > the ADC device, and then on Pen detect and No pen detect just start or
> > > stop the periodic trigger, which needs to be polled. The magic part is
> > > that the consumer driver has a poll function already attached to this
> > > trigger, so the poll function is just called every time we have new
> > > data. The poll function is attached as an irq handler, and then we can
> > > reuse all the read_raw data by using a scheduled work from the consumer
> > > driver, to read the channels.
> >
> > If you had done this via a callback buffer the only difference is that
> > the pollfunc would have been a standard one pulling the relevant channels
> > and passing them on down to the buffer interface which could then decide
> > what to do with them.
> >
> > > To do this, the ADC registers a special trigger named "touch trigger"
> > > which is never enabled by the ADC driver. Instead, when a pollfunc is
> > > attached to it, the attach function will also configure it with enabled
> > > state.
> >
> > Whilst it might not make sense to enable it in the touch screen driver
> > I'm not sure there is strictly any reason to prevent it being so used.
> >
> > > In the ADC, this means to start the touchscreen functionality. If
> > > the touch is requested, it will standby and wait for pen detect IRQ.
> > > Once we have pen detect, we can use a periodic trigger to sample the
> > > touch data, and poll the "touch" trigger. The consumer driver will wake
> > > up and schedule a work , that will use the standard read raw interface
> > > (inkern) that will read three virtual channels (position + pressure).
> > > They are not actual hardware channels, as the touch information is being
> > > received on channels 0,1,2,3, but reading these virtual channels will
> > > read from different registers inside the ADC IP ( x position, y
> > > position, pressure), do some computations on the data, and feed the
> > > consumer with the values , hiding the behind the scenes hardware
> > > specific calculations.
> >
> > I wouldn't worry about whether they are real channels or not. This
> > is really similar to a differential ADC (some of those do the differential
> > digitally). Light sensors often have a number of 'real' channels used
> > to derive (via hideous non linear calculations) the illuminance as
> > it's hard to build a light sensor with the same sensitivity as the human
> > eye. We have worse 'non real' channels as well such as activity channels
> > on some the accelerometers that report if it thinks you are walking /
> > running etc.
> >
> > > After trigger is polled , the ADC will resume normal functionality, and
> > > the consumer driver will continue to sleep.
> >
> > So this is where I'm unsure. Do you actually have a usecase where it
> > makes the sense to read from the ADC only when there is no touch? Any
> > system doing that has an obvious denial of service attack - touch the
> > screen.
> >
>
> You're right. We have an issue in this case due to the hardware. Using
> touchscreen has side effects on other channels. We can use only one
> trigger for all the channels. The situation would have been better with
> a trigger dedicated to the touchscreen.
>
> At the moment, we have not really stated about the exclusive use or not
> of the touchscreen. We suppose we can get some customers wanting to use
> both touchscreen and ADC. Eugen tried to deal with this case but, as you
> noticed, it can lead to DoS.
It's a restriction people aren't going to expect unfortunately...
>
> > > We need to have a periodic trigger to sample the data because the actual
> > > analog to digital conversion inside the IP block needs to be triggered.
> > > The touchscreen data measurements cannot happen in hardware without
> > > being triggered. If I try with a hrtimer to get a periodic IRQ to just
> > > read the data, it will never be ready. The datasheet states that the
> > > touchscreen measurements "will be attached to the conversion sequence".
> > > So the periodic trigger is forcing a conversion sequence. This could be
> > > done with a software trigger as well, but why the hassle to start it
> > > every 2 milliseconds (or other time interval), if we can do it by
> > > periodic trigger ?
> >
> > Ah, one reason here would be to allow separate consumers to use the
> > device. In that case you'd run with a periodic trigger all the time
> > and have two buffers attached, the buffer_cb that is feeding your
> > touchscreen and another buffer to deal with the other channels
> > (presumably the standard one an IIO device has when using buffered
> > interfaces).
>
> The issue is that we are sharing the periodic trigger so we have to use
> the same period for both usage.
Whilst a somewhat irritating restriction, it's probably not disastrous for
most ADC uses.
Jonathan
>
> Regards
>
> Ludovic
>
> >
> > The buffer demux would ensure the data from the right channels
> > ends up in the right place. It makes it look to the buffer
> > consumer like it is the only thing using / controlling the data
> > flow.
> >
> > > Once we get the No pen IRQ, we stop the periodic trigger and it can be
> > > used in another purpose (software or external as of now in the driver,
> > > in the future we can add PWM trigger and Timer trigger)
> >
> > This case isn't really useful though as any other use is denied
> > access when touch occurs.
> >
> > I'll summarise what I think would work for this below.
> >
> > >
> > > In short, the ADC in Sama5D2 also supports touchscreen, and in
> > > touchscreen mode , 4 of the channels are being used for this purpose.
> > > This however, doesn't stop the ADC to use the other channels . The
> > > hardware has 12 total single channels and they can be paired to have 6
> > > more differential channels. The only thing that is blocked is the
> > > trigger, but only if the pen is touching (when we start the periodic
> > > trigger to sample the touchscreen). If the pen is not touching, an
> > > external trigger or software trigger can be used without any issues (so
> > > why limit the functionality, if this is available from hardware ?).
> > > Because of the reason I discussed above (touchscreen sequence must be
> > > triggered), we cannot use another trigger in the same time.
> > >
> > >
> > > I see your idea with the callback buffer and it's worth exploring.
> > > Mainly this series was to actually show you what I had in mind about
> > > supporting the resistive touchscreen, and to give you some actually
> > > working code/patch, so we can discuss based on real implementation, not
> > > just suppositions.
> >
> > That side of things is fine.
> >
> > >
> > > You are right in many of the other comments that you said, and I will
> > > come up with a v2 to this series. For now, I need to know if this is a
> > > good or right direction in which I am going, or I should try to change
> > > all the mechanism to callback buffer ? Or maybe I am totally in a bad
> > > direction ?
> > > The requirements are that the consumer driver needs to be somehow woke
> > > up for every new touch data available, and report to the input
> > > subsystem. As it was done before, the at91 old driver, just creates and
> > > registers an input device by itself, and then reports the position and
> > > touches. I was thinking that with this trigger consumer implementation,
> > > things can be better in terms of subsystem separation and support.
> > >
> > > Thanks again and let me know of your thoughts,
> > >
> > > Eugen
> >
> > So a couple of things come to mind on how I'd structure this.
> > So what we have (very briefly)
> >
> > No touch screen case:
> > * Generic ADC using all sorts of different triggers
> >
> > Touch screen only case:
> > * Interrupt to indicate pen on / off
> > * A need to do a periodic trigger of the device but only
> > useful when touch is in progress.
> >
> > Touch screen and other users:
> > * Interrupt to indicate pen on / off
> > * Periodic trigger needed for touchscreen when touch in progress.
> > * Do not have denial of service on other channels.
> >
> > First two cases are easy enough by having a magic trigger, third
> > case is harder.
> > If we have the touchscreen then I would drop support for direct access to
> > to ADC channels whilst it's in use (so no sysfs - or emulate it if you
> > really want it by stashing results from scans done when touch is in
> > progress).
> >
> > Have your touch screen channels just as normal additional channels,
> > but only via the buffered interface (no _RAW attribute).
> > If someone sets up to read them via buffered interface with
> > a different trigger I think they'll get values - whether they
> > are right is dependent (if I understand correctly) on whether
> > there is a touch in progress. So no harm done and it'll make
> > the logic simpler.
> >
> > The moment touch is opened and acquires the IIO channels
> > fix the trigger (may need new interface) to the periodic one
> > that you were enabling and disabling on touch.
> > Things get dicey if there is an existing user so you may
> > have to do it on driver probe rather than open of the input
> > device if we effectively want touch to have the highest
> > priority use of the ADC.
> >
> > If other channels are enabled for buffered mode then note
> > this in the driver and have the periodic trigger on all the
> > time (to ensure they keep getting read) This will pass
> > garbage to your touch screen driver, but it'll remove it due
> > to the pressure value being too low so no harm there.
> >
> > Normal path will work for non touch channels (and in theory
> > the touch ones if they are turned on) via IIO buffer
> > interface. It'll be restricted in form due to the needs of
> > the touch driver, but better than nothing and should cover
> > most usecases.
> >
> > Now the interrupt on / off on touch bit becomes an optimization
> > in the case of only the buffer_cb being attached.
> >
> > I think that fits cleanly in the current IIO framework and
> > looks more similar to our existing provider consumer approaches.
> >
> > Still needs the hooks to get hold of the trigger though so
> > as to be able to tell the ADC which one to use. So rather
> > than being a trigger consumer interface, it's more of a trigger
> > configuration interface.. Exact term doesn't matter though.
> >
> > Jonathan
> >
> > >
> > >
> > >
> > > [...]
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] input: Add driver for USB ELAN Touchpad
From: Alexandrov Stansilav @ 2018-01-13 23:33 UTC (permalink / raw)
To: linux-input; +Cc: linux-kernel, jikos, benjamin.tissoires
This is driver for usb touchpad found on HP Pavilion x2 10-p0xx
laptop. On this device keyboard and touchpad connected as a single
usb device with two interfaces: keyboard, which exposes ordinary keys
and second interface is touchpad which also contains FlightMode button and
audio mute led (which physically placed on keyboard for some reason).
Initially, this touchpad works in mouse emulation mode, this driver will
switch it to touchpad mode, which can track 5 fingers and can report
coordinates for two of them.
Signed-off-by: Alexandrov Stansilav <neko@nya.ai>
---
drivers/hid/Kconfig | 8 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-elan.c | 421 +++++++++++++++++++++++++++++++++++++++++++++++
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-quirks.c | 3 +
5 files changed, 434 insertions(+)
create mode 100644 drivers/hid/hid-elan.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 9058dbc4d..8452d1bc9 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -274,6 +274,14 @@ config HID_EMS_FF
Currently the following devices are known to be supported:
- Trio Linker Plus II
+config HID_ELAN
+ tristate "ELAN USB Touchpad Support"
+ depends on LEDS_CLASS && USB_HID
+ ---help---
+ Say Y to enable support for the USB ELAN touchpad
+ Currently the following devices are known to be supported:
+ - HP Pavilion X2 10-p0XX.
+
config HID_ELECOM
tristate "ELECOM HID devices"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index eb13b9e92..713601c7b 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_HID_CP2112) += hid-cp2112.o
obj-$(CONFIG_HID_CYPRESS) += hid-cypress.o
obj-$(CONFIG_HID_DRAGONRISE) += hid-dr.o
obj-$(CONFIG_HID_EMS_FF) += hid-emsff.o
+obj-$(CONFIG_HID_ELAN) += hid-elan.o
obj-$(CONFIG_HID_ELECOM) += hid-elecom.o
obj-$(CONFIG_HID_ELO) += hid-elo.o
obj-$(CONFIG_HID_EZKEY) += hid-ezkey.o
diff --git a/drivers/hid/hid-elan.c b/drivers/hid/hid-elan.c
new file mode 100644
index 000000000..803a72578
--- /dev/null
+++ b/drivers/hid/hid-elan.c
@@ -0,0 +1,421 @@
+/*
+ * HID Driver for ELAN Touchpad
+ *
+ * Currently only supports touchpad found on HP Pavilion X2 10
+ *
+ * Copyright (c) 2016 Alexandrov Stanislav <neko@nya.ai>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/hid.h>
+#include <linux/input/mt.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/usb.h>
+
+#include "hid-ids.h"
+
+#define ELAN_SINGLE_FINGER 0x81
+#define ELAN_MT_FIRST_FINGER 0x82
+#define ELAN_MT_SECOND_FINGER 0x83
+#define ELAN_INPUT_REPORT_SIZE 8
+
+#define ELAN_MUTE_LED_REPORT 0xBC
+#define ELAN_LED_REPORT_SIZE 8
+
+struct elan_touchpad_settings {
+ u8 max_fingers;
+ u16 max_x;
+ u16 max_y;
+ u8 max_area_x;
+ u8 max_area_y;
+ u8 max_w;
+ int usb_bInterfaceNumber;
+};
+
+struct elan_drvdata {
+ struct input_dev *input;
+ u8 prev_report[ELAN_INPUT_REPORT_SIZE];
+ struct led_classdev mute_led;
+ u8 mute_led_state;
+ struct elan_touchpad_settings *settings;
+};
+
+static int is_not_elan_touchpad(struct hid_device *hdev)
+{
+ struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
+ struct elan_drvdata *drvdata = hid_get_drvdata(hdev);
+
+ return (intf->altsetting->desc.bInterfaceNumber != drvdata->settings->usb_bInterfaceNumber);
+}
+
+static int elan_input_mapping(struct hid_device *hdev, struct hid_input *hi,
+ struct hid_field *field, struct hid_usage *usage,
+ unsigned long **bit, int *max)
+{
+ if (is_not_elan_touchpad(hdev))
+ return 0;
+
+ if (field->report->id == ELAN_SINGLE_FINGER ||
+ field->report->id == ELAN_MT_FIRST_FINGER ||
+ field->report->id == ELAN_MT_SECOND_FINGER)
+ return -1;
+
+ return 0;
+}
+
+static int elan_input_configured(struct hid_device *hdev, struct hid_input *hi)
+{
+ int ret;
+ struct input_dev *input;
+ struct elan_drvdata *drvdata = hid_get_drvdata(hdev);
+
+ if (is_not_elan_touchpad(hdev))
+ return 0;
+
+ input = devm_input_allocate_device(&hdev->dev);
+ if (!input)
+ return -ENOMEM;
+
+ input->name = "Elan Touchpad";
+ input->phys = hdev->phys;
+ input->uniq = hdev->uniq;
+ input->id.bustype = hdev->bus;
+ input->id.vendor = hdev->vendor;
+ input->id.product = hdev->product;
+ input->id.version = hdev->version;
+ input->dev.parent = &hdev->dev;
+
+ input_set_abs_params(input, ABS_MT_POSITION_X, 0,
+ drvdata->settings->max_x, 0, 0);
+ input_set_abs_params(input, ABS_MT_POSITION_Y, 0,
+ drvdata->settings->max_y, 0, 0);
+ input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
+ drvdata->settings->max_fingers, 0, 0);
+ input_set_abs_params(input, ABS_TOOL_WIDTH, 0,
+ drvdata->settings->max_w, 0, 0);
+
+ __set_bit(BTN_LEFT, input->keybit);
+ __set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
+
+ ret = input_mt_init_slots(input, drvdata->settings->max_fingers,
+ INPUT_MT_POINTER);
+ if (ret) {
+ hid_err(hdev, "Failed to init elan MT slots: %d\n", ret);
+ return ret;
+ }
+
+ ret = input_register_device(input);
+ if (ret) {
+ hid_err(hdev, "Failed to register elan input device: %d\n",
+ ret);
+ input_free_device(input);
+ return ret;
+ }
+
+ drvdata->input = input;
+
+ return 0;
+}
+
+static void elan_report_mt_slot(struct elan_drvdata *drvdata, u8 *data,
+ unsigned int slot_num)
+{
+ struct input_dev *input = drvdata->input;
+ int x, y, w;
+
+ bool active = !!data;
+
+ input_mt_slot(input, slot_num);
+ input_mt_report_slot_state(input, MT_TOOL_FINGER, active);
+ if (active) {
+ x = ((data[0] & 0xF0) << 4) | data[1];
+ y = drvdata->settings->max_y -
+ (((data[0] & 0x07) << 8) | data[2]);
+ w = data[4];
+
+ input_report_abs(input, ABS_MT_POSITION_X, x);
+ input_report_abs(input, ABS_MT_POSITION_Y, y);
+ input_report_abs(input, ABS_TOOL_WIDTH, w);
+ }
+}
+
+static void elan_report_input(struct elan_drvdata *drvdata, u8 *data)
+{
+ int i;
+ struct input_dev *input = drvdata->input;
+
+ /*
+ * There is 3 types of reports: for single touch,
+ * for multitouch - first finger and for multitouch - second finger
+ *
+ * packet structure for ELAN_SINGLE_FINGER and ELAN_MT_FIRST_FINGER:
+ *
+ * byte 1: 1 0 0 0 0 0 0 1 // 0x81 or 0x82
+ * byte 2: 0 0 0 0 0 0 0 0 // looks like unused
+ * byte 3: f5 f4 f3 f2 f1 0 0 L
+ * byte 4: x12 x11 x10 x9 0? y11 y10 y9
+ * byte 5: x8 x7 x6 x5 x4 x3 x2 x1
+ * byte 6: y8 y7 y6 y5 y4 y3 y2 y1
+ * byte 7: sy4 sy3 sy2 sy1 sx4 sx3 sx2 sx1
+ * byte 8: w8 w7 w6 w5 w4 w3 w2 w1
+ *
+ * packet structure for ELAN_MT_SECOND_FINGER:
+ *
+ * byte 1: 1 0 0 0 0 0 1 1 // 0x83
+ * byte 2: x12 x11 x10 x9 0 y11 y10 y9
+ * byte 3: x8 x7 x6 x5 x4 x3 x2 x1
+ * byte 4: y8 y7 y6 y5 y4 y3 y2 y1
+ * byte 5: sy4 sy3 sy2 sy1 sx4 sx3 sx2 sx1
+ * byte 6: w8 w7 w6 w5 w4 w3 w2 w1
+ * byte 7: 0 0 0 0 0 0 0 0
+ * byte 8: 0 0 0 0 0 0 0 0
+ *
+ * f5-f1: finger touch bits
+ * L: clickpad button
+ * sy / sx: not sure yet, but this looks like rectangular
+ * area for finger
+ * w: looks like finger width
+ */
+
+ if (data[0] == ELAN_SINGLE_FINGER) {
+ for (i = 0; i < drvdata->settings->max_fingers; i++) {
+ if (data[2] & BIT(i + 3))
+ elan_report_mt_slot(drvdata, data + 3, i);
+ else
+ elan_report_mt_slot(drvdata, NULL, i);
+ }
+ input_report_key(input, BTN_LEFT, data[2] & 0x01);
+ }
+ /*
+ * When touched with two fingers Elan touchpad will emit two HID reports
+ * first is ELAN_MT_FIRST_FINGER and second is ELAN_MT_SECOND_FINGER
+ * we will save ELAN_MT_FIRST_FINGER report and wait for
+ * ELAN_MT_SECOND_FINGER to finish multitouch
+ */
+ if (data[0] == ELAN_MT_FIRST_FINGER) {
+ memcpy(drvdata->prev_report, data,
+ sizeof(drvdata->prev_report));
+ return;
+ }
+
+ if (data[0] == ELAN_MT_SECOND_FINGER) {
+ int first = 0;
+ u8 *prev_report = drvdata->prev_report;
+
+ if (prev_report[0] != ELAN_MT_FIRST_FINGER)
+ return;
+
+ for (i = 0; i < drvdata->settings->max_fingers; i++) {
+ if (prev_report[2] & BIT(i + 3)) {
+ if (!first) {
+ first = 1;
+ elan_report_mt_slot(drvdata, prev_report + 3, i);
+ } else {
+ elan_report_mt_slot(drvdata, data + 1, i);
+ }
+ } else {
+ elan_report_mt_slot(drvdata, NULL, i);
+ }
+ }
+ input_report_key(input, BTN_LEFT, prev_report[2] & 0x01);
+ }
+
+ input_mt_sync_frame(input);
+ input_sync(input);
+}
+
+static int elan_raw_event(struct hid_device *hdev,
+ struct hid_report *report, u8 *data, int size)
+{
+ struct elan_drvdata *drvdata = hid_get_drvdata(hdev);
+
+ if (is_not_elan_touchpad(hdev))
+ return 0;
+
+ if (data[0] == ELAN_SINGLE_FINGER ||
+ data[0] == ELAN_MT_FIRST_FINGER ||
+ data[0] == ELAN_MT_SECOND_FINGER) {
+ if (size == ELAN_INPUT_REPORT_SIZE) {
+ elan_report_input(drvdata, data);
+ return 1;
+ }
+ }
+
+ return 0;
+}
+
+static int elan_start_multitouch(struct hid_device *hdev)
+{
+ int ret;
+
+ /*
+ * This byte sequence will enable multitouch mode and disable
+ * mouse emulation
+ */
+ const unsigned char buf[] = { 0x0D, 0x00, 0x03, 0x21, 0x00 };
+ unsigned char *dmabuf = kmemdup(buf, sizeof(buf), GFP_KERNEL);
+
+ if (!dmabuf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(hdev, dmabuf[0], dmabuf, sizeof(buf),
+ HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+
+ kfree(dmabuf);
+
+ if (ret != sizeof(buf)) {
+ hid_err(hdev, "Failed to start multitouch: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static enum led_brightness elan_mute_led_get_brigtness(struct led_classdev *led_cdev)
+{
+ struct device *dev = led_cdev->dev->parent;
+ struct hid_device *hdev = to_hid_device(dev);
+ struct elan_drvdata *drvdata = hid_get_drvdata(hdev);
+
+ return drvdata->mute_led_state;
+}
+
+static int elan_mute_led_set_brigtness(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ int ret;
+ u8 led_state;
+ struct device *dev = led_cdev->dev->parent;
+ struct hid_device *hdev = to_hid_device(dev);
+ struct elan_drvdata *drvdata = hid_get_drvdata(hdev);
+
+ unsigned char *dmabuf = kzalloc(ELAN_LED_REPORT_SIZE, GFP_KERNEL);
+
+ if (!dmabuf)
+ return -ENOMEM;
+
+ led_state = !!value;
+
+ dmabuf[0] = ELAN_MUTE_LED_REPORT;
+ dmabuf[1] = 0x02;
+ dmabuf[2] = led_state;
+
+ ret = hid_hw_raw_request(hdev, dmabuf[0], dmabuf, ELAN_LED_REPORT_SIZE,
+ HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+
+ kfree(dmabuf);
+
+ if (ret != ELAN_LED_REPORT_SIZE) {
+ hid_err(hdev, "Failed to set mute led brightness: %d\n", ret);
+ return ret;
+ }
+
+ drvdata->mute_led_state = led_state;
+ return 0;
+}
+
+static int elan_init_mute_led(struct hid_device *hdev)
+{
+ struct elan_drvdata *drvdata = hid_get_drvdata(hdev);
+ struct led_classdev *mute_led = &drvdata->mute_led;
+
+ mute_led->name = "elan:red:mute";
+ mute_led->brightness_get = elan_mute_led_get_brigtness;
+ mute_led->brightness_set_blocking = elan_mute_led_set_brigtness;
+ mute_led->max_brightness = LED_ON;
+ mute_led->dev = &hdev->dev;
+
+ return devm_led_classdev_register(&hdev->dev, mute_led);
+}
+
+static int elan_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+ int ret;
+ struct elan_drvdata *drvdata;
+
+ drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
+
+ if (!drvdata)
+ return -ENOMEM;
+
+ drvdata->settings = (struct elan_touchpad_settings *)id->driver_data;
+ hid_set_drvdata(hdev, drvdata);
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev, "Hid Parse failed\n");
+ return ret;
+ }
+
+ ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ if (ret) {
+ hid_err(hdev, "Hid hw start failed\n");
+ return ret;
+ }
+
+ if (is_not_elan_touchpad(hdev))
+ return 0;
+
+ if (!drvdata->input) {
+ hid_err(hdev, "Input device is not registred\n");
+ ret = -ENAVAIL;
+ goto err;
+ }
+
+ ret = elan_start_multitouch(hdev);
+ if (ret)
+ goto err;
+
+ ret = elan_init_mute_led(hdev);
+ if (ret)
+ goto err;
+
+ return 0;
+err:
+ hid_hw_stop(hdev);
+ return ret;
+}
+
+static void elan_remove(struct hid_device *hdev)
+{
+ hid_hw_stop(hdev);
+}
+
+static const struct elan_touchpad_settings hp_x2_10_touchpad_data = {
+ .max_fingers = 5,
+ .max_x = 2930,
+ .max_y = 1250,
+ .max_area_x = 15,
+ .max_area_y = 15,
+ .max_w = 255,
+ .usb_bInterfaceNumber = 1,
+};
+
+static const struct hid_device_id elan_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELAN, USB_DEVICE_ID_HP_X2_10_COVER),
+ (kernel_ulong_t)&hp_x2_10_touchpad_data},
+ { }
+};
+
+MODULE_DEVICE_TABLE(hid, elan_devices);
+
+static struct hid_driver elan_driver = {
+ .name = "elan",
+ .id_table = elan_devices,
+ .input_mapping = elan_input_mapping,
+ .input_configured = elan_input_configured,
+ .raw_event = elan_raw_event,
+ .probe = elan_probe,
+ .remove = elan_remove,
+};
+
+module_hid_driver(elan_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alexandrov Stanislav");
+MODULE_DESCRIPTION("Driver for HID ELAN Touchpads");
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5207be9bd..0b638e5fc 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -368,6 +368,7 @@
#define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_A001 0xa001
#define USB_VENDOR_ID_ELAN 0x04f3
+#define USB_DEVICE_ID_HP_X2_10_COVER 0x0755
#define USB_VENDOR_ID_ELECOM 0x056e
#define USB_DEVICE_ID_ELECOM_BM084 0x0061
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 245345ae6..762efa7c3 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -333,6 +333,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
#endif
+#if IS_ENABLED(CONFIG_HID_ELAN)
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELAN, USB_DEVICE_ID_HP_X2_10_COVER) },
+#endif
#if IS_ENABLED(CONFIG_HID_ELECOM)
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRED) },
--
2.13.6
^ permalink raw reply related
* [PATCH v3] input: pxrc: new driver for PhoenixRC Flight Controller Adapter
From: Marcus Folkesson @ 2018-01-13 20:15 UTC (permalink / raw)
To: Dmitry Torokhov, Jonathan Corbet, Marcus Folkesson,
Tomohiro Yoshidomi, David Herrmann, Philippe Ombredanne,
Kate Stewart, Greg Kroah-Hartman
Cc: linux-input, linux-doc, linux-kernel
This driver let you plug in your RC controller to the adapter and
use it as input device in various RC simulators.
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
v3:
- Use RUDDER and MISC instead of TILT_X and TILT_Y
- Drop kref and anchor
- Rework URB handling
- Add PM support
v2:
- Change module license to GPLv2 to match SPDX tag
Documentation/input/devices/pxrc.rst | 57 +++++++
drivers/input/joystick/Kconfig | 9 +
drivers/input/joystick/Makefile | 1 +
drivers/input/joystick/pxrc.c | 320 +++++++++++++++++++++++++++++++++++
4 files changed, 387 insertions(+)
create mode 100644 Documentation/input/devices/pxrc.rst
create mode 100644 drivers/input/joystick/pxrc.c
diff --git a/Documentation/input/devices/pxrc.rst b/Documentation/input/devices/pxrc.rst
new file mode 100644
index 000000000000..ca11f646bae8
--- /dev/null
+++ b/Documentation/input/devices/pxrc.rst
@@ -0,0 +1,57 @@
+=======================================================
+pxrc - PhoenixRC Flight Controller Adapter
+=======================================================
+
+:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
+
+This driver let you use your own RC controller plugged into the
+adapter that comes with PhoenixRC [1]_ or other compatible adapters.
+
+The adapter supports 7 analog channels and 1 digital input switch.
+
+Notes
+=====
+
+Many RC controllers is able to configure which stick goes to which channel.
+This is also configurable in most simulators, so a matching is not necessary.
+
+The driver is generating the following input event for analog channels:
+
++---------+----------------+
+| Channel | Event |
++=========+================+
+| 1 | ABS_X |
++---------+----------------+
+| 2 | ABS_Y |
++---------+----------------+
+| 3 | ABS_RX |
++---------+----------------+
+| 4 | ABS_RY |
++---------+----------------+
+| 5 | ABS_RUDDER |
++---------+----------------+
+| 6 | ABS_THROTTLE |
++---------+----------------+
+| 7 | ABS_MISC |
++---------+----------------+
+
+The digital input switch is generated as an `BTN_A` event.
+
+Manual Testing
+==============
+
+To test this driver's functionality you may use `input-event` which is part of
+the `input layer utilities` suite [2]_.
+
+For example::
+
+ > modprobe pxrc
+ > input-events <devnr>
+
+To print all input events from input `devnr`.
+
+References
+==========
+
+.. [1] http://www.phoenix-sim.com/
+.. [2] https://www.kraxel.org/cgit/input/
diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index f3c2f6ea8b44..18ab6dafff41 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -351,4 +351,13 @@ config JOYSTICK_PSXPAD_SPI_FF
To drive rumble motor a dedicated power supply is required.
+config JOYSTICK_PXRC
+ tristate "PhoenixRC Flight Controller Adapter"
+ depends on USB_ARCH_HAS_HCD
+ select USB
+ help
+ Say Y here if you want to use the PhoenixRC Flight Controller Adapter.
+
+ To compile this driver as a module, choose M here: the
+ module will be called pxrc.
endif
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 67651efda2e1..dd0492ebbed7 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
+obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
new file mode 100644
index 000000000000..98d9b8184c46
--- /dev/null
+++ b/drivers/input/joystick/pxrc.c
@@ -0,0 +1,320 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Phoenix RC Flight Controller Adapter
+ *
+ * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/usb.h>
+#include <linux/usb/input.h>
+#include <linux/mutex.h>
+#include <linux/input.h>
+
+#define PXRC_VENDOR_ID (0x1781)
+#define PXRC_PRODUCT_ID (0x0898)
+
+static const struct usb_device_id pxrc_table[] = {
+ { USB_DEVICE(PXRC_VENDOR_ID, PXRC_PRODUCT_ID) },
+ { }
+};
+MODULE_DEVICE_TABLE(usb, pxrc_table);
+
+struct pxrc {
+ struct input_dev *input;
+ struct usb_device *udev;
+ struct usb_interface *intf;
+ struct urb *urb;
+ __u8 epaddr;
+ char phys[64];
+ unsigned char *data;
+ size_t bsize;
+};
+
+static void pxrc_usb_irq(struct urb *urb)
+{
+ struct pxrc *pxrc = urb->context;
+ int error;
+
+ switch (urb->status) {
+ case 0:
+ /* success */
+ break;
+ case -ETIME:
+ /* this urb is timing out */
+ dev_dbg(&pxrc->intf->dev,
+ "%s - urb timed out - was the device unplugged?\n",
+ __func__);
+ return;
+ case -ECONNRESET:
+ case -ENOENT:
+ case -ESHUTDOWN:
+ case -EPIPE:
+ /* this urb is terminated, clean up */
+ dev_dbg(&pxrc->intf->dev, "%s - urb shutting down with status: %d\n",
+ __func__, urb->status);
+ return;
+ default:
+ dev_dbg(&pxrc->intf->dev, "%s - nonzero urb status received: %d\n",
+ __func__, urb->status);
+ goto exit;
+ }
+
+ if (urb->actual_length == 8) {
+ input_report_abs(pxrc->input, ABS_X, pxrc->data[0]);
+ input_report_abs(pxrc->input, ABS_Y, pxrc->data[2]);
+ input_report_abs(pxrc->input, ABS_RX, pxrc->data[3]);
+ input_report_abs(pxrc->input, ABS_RY, pxrc->data[4]);
+ input_report_abs(pxrc->input, ABS_RUDDER, pxrc->data[5]);
+ input_report_abs(pxrc->input, ABS_THROTTLE, pxrc->data[6]);
+ input_report_abs(pxrc->input, ABS_MISC, pxrc->data[7]);
+
+ input_report_key(pxrc->input, BTN_A, pxrc->data[1]);
+ }
+
+exit:
+ /* Resubmit to fetch new fresh URBs */
+ error = usb_submit_urb(urb, GFP_ATOMIC);
+ if (error && error != -EPERM)
+ dev_err(&pxrc->intf->dev,
+ "%s - usb_submit_urb failed with result: %d",
+ __func__, error);
+}
+
+static int pxrc_open(struct input_dev *input)
+{
+ struct pxrc *pxrc = input_get_drvdata(input);
+ int retval;
+
+ retval = usb_autopm_get_interface(pxrc->intf);
+ if (retval) {
+ dev_err(&pxrc->intf->dev,
+ "%s - usb_autopm_get_interface failed, error: %d\n",
+ __func__, retval);
+ return retval;
+ }
+
+ retval = usb_submit_urb(pxrc->urb, GFP_KERNEL);
+ if (retval) {
+ dev_err(&pxrc->intf->dev,
+ "%s - usb_submit_urb failed, error: %d\n",
+ __func__, retval);
+ retval = -EIO;
+ goto out;
+ }
+
+ pxrc->intf->needs_remote_wakeup = 1;
+
+out:
+ usb_autopm_put_interface(pxrc->intf);
+ return retval;
+}
+
+static void pxrc_close(struct input_dev *input)
+{
+ struct pxrc *pxrc = input_get_drvdata(input);
+ int autopm_error;
+
+ autopm_error = usb_autopm_get_interface(pxrc->intf);
+
+ usb_kill_urb(pxrc->urb);
+ pxrc->intf->needs_remote_wakeup = 0;
+
+ if (!autopm_error)
+ usb_autopm_put_interface(pxrc->intf);
+}
+
+static int pxrc_usb_init(struct pxrc *pxrc)
+{
+ struct usb_endpoint_descriptor *epirq;
+ unsigned int pipe;
+ int retval;
+
+ /* Set up the endpoint information */
+ /* This device only has an interrupt endpoint */
+ retval = usb_find_common_endpoints(pxrc->intf->cur_altsetting,
+ NULL, NULL, &epirq, NULL);
+ if (retval) {
+ dev_err(&pxrc->intf->dev,
+ "Could not find endpoint\n");
+ goto error;
+ }
+
+ pxrc->bsize = usb_endpoint_maxp(epirq);
+ pxrc->epaddr = epirq->bEndpointAddress;
+ pxrc->data = devm_kmalloc(&pxrc->intf->dev, pxrc->bsize, GFP_KERNEL);
+ if (!pxrc->data) {
+ retval = -ENOMEM;
+ goto error;
+ }
+
+ usb_set_intfdata(pxrc->intf, pxrc);
+ usb_make_path(pxrc->udev, pxrc->phys, sizeof(pxrc->phys));
+ strlcat(pxrc->phys, "/input0", sizeof(pxrc->phys));
+
+ pxrc->urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (!pxrc->urb) {
+ retval = -ENOMEM;
+ goto error;
+ }
+
+ pipe = usb_rcvintpipe(pxrc->udev, pxrc->epaddr),
+ usb_fill_int_urb(pxrc->urb, pxrc->udev, pipe, pxrc->data, pxrc->bsize,
+ pxrc_usb_irq, pxrc, 1);
+
+error:
+ return retval;
+
+
+}
+
+
+static int pxrc_input_init(struct pxrc *pxrc)
+{
+ pxrc->input = devm_input_allocate_device(&pxrc->intf->dev);
+ if (pxrc->input == NULL) {
+ dev_err(&pxrc->intf->dev, "couldn't allocate input device\n");
+ return -ENOMEM;
+ }
+
+ pxrc->input->name = "PXRC Flight Controller Adapter";
+ pxrc->input->phys = pxrc->phys;
+ usb_to_input_id(pxrc->udev, &pxrc->input->id);
+
+ pxrc->input->open = pxrc_open;
+ pxrc->input->close = pxrc_close;
+
+ input_set_capability(pxrc->input, EV_KEY, BTN_A);
+ input_set_abs_params(pxrc->input, ABS_X, 0, 255, 0, 0);
+ input_set_abs_params(pxrc->input, ABS_Y, 0, 255, 0, 0);
+ input_set_abs_params(pxrc->input, ABS_RX, 0, 255, 0, 0);
+ input_set_abs_params(pxrc->input, ABS_RY, 0, 255, 0, 0);
+ input_set_abs_params(pxrc->input, ABS_RUDDER, 0, 255, 0, 0);
+ input_set_abs_params(pxrc->input, ABS_THROTTLE, 0, 255, 0, 0);
+ input_set_abs_params(pxrc->input, ABS_MISC, 0, 255, 0, 0);
+
+ input_set_drvdata(pxrc->input, pxrc);
+
+ return input_register_device(pxrc->input);
+}
+
+static int pxrc_probe(struct usb_interface *intf,
+ const struct usb_device_id *id)
+{
+ struct pxrc *pxrc;
+ int retval;
+
+ pxrc = devm_kzalloc(&intf->dev, sizeof(*pxrc), GFP_KERNEL);
+
+ if (!pxrc)
+ return -ENOMEM;
+
+ pxrc->udev = usb_get_dev(interface_to_usbdev(intf));
+ pxrc->intf = intf;
+
+ retval = pxrc_usb_init(pxrc);
+ if (retval)
+ goto error;
+
+ retval = pxrc_input_init(pxrc);
+ if (retval)
+ goto err_free_urb;
+
+ return 0;
+
+err_free_urb:
+ usb_free_urb(pxrc->urb);
+
+error:
+ return retval;
+}
+
+static void pxrc_disconnect(struct usb_interface *intf)
+{
+ struct pxrc *pxrc = usb_get_intfdata(intf);
+
+ usb_free_urb(pxrc->urb);
+ usb_set_intfdata(intf, NULL);
+}
+
+static int pxrc_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct pxrc *pxrc = usb_get_intfdata(intf);
+ struct input_dev *input_dev = pxrc->input;
+
+ mutex_lock(&input_dev->mutex);
+ usb_kill_urb(pxrc->urb);
+ mutex_unlock(&input_dev->mutex);
+
+ return 0;
+}
+
+static int pxrc_resume(struct usb_interface *intf)
+{
+ struct pxrc *pxrc = usb_get_intfdata(intf);
+ struct input_dev *input_dev = pxrc->input;
+ int retval = 0;
+
+ mutex_lock(&input_dev->mutex);
+
+ if (input_dev->users && usb_submit_urb(pxrc->urb, GFP_NOIO) < 0)
+ retval = -EIO;
+
+ mutex_unlock(&input_dev->mutex);
+
+ return retval;
+}
+
+static int pxrc_pre_reset(struct usb_interface *intf)
+{
+ struct pxrc *pxrc = usb_get_intfdata(intf);
+ struct input_dev *input_dev = pxrc->input;
+
+ mutex_lock(&input_dev->mutex);
+ usb_kill_urb(pxrc->urb);
+
+ return 0;
+}
+
+static int pxrc_post_reset(struct usb_interface *intf)
+{
+ struct pxrc *pxrc = usb_get_intfdata(intf);
+ struct input_dev *input_dev = pxrc->input;
+ int retval = 0;
+
+ if (input_dev->users && usb_submit_urb(pxrc->urb, GFP_NOIO) < 0)
+ retval = -EIO;
+
+ mutex_unlock(&input_dev->mutex);
+
+ return retval;
+}
+
+static int pxrc_reset_resume(struct usb_interface *intf)
+{
+ return pxrc_resume(intf);
+}
+
+static struct usb_driver pxrc_driver = {
+ .name = "pxrc",
+ .probe = pxrc_probe,
+ .disconnect = pxrc_disconnect,
+ .id_table = pxrc_table,
+ .suspend = pxrc_suspend,
+ .resume = pxrc_resume,
+ .pre_reset = pxrc_pre_reset,
+ .post_reset = pxrc_post_reset,
+ .reset_resume = pxrc_reset_resume,
+ .supports_autosuspend = 1,
+};
+
+module_usb_driver(pxrc_driver);
+
+MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
+MODULE_DESCRIPTION("PhoenixRC Flight Controller Adapter");
+MODULE_LICENSE("GPL v2");
--
2.15.1
^ permalink raw reply related
* Re: [PATCH] Input: trackpoint - force 3 buttons if 0 button is reported
From: Aaron Ma @ 2018-01-13 2:34 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <20180112181830.ccpkltghgkj3oh3e@dtor-ws>
Will your patch go to stable kernel?
If yes, that's fine.
^ permalink raw reply
* [PATCH v2] Input: mms114 - drop platform data and use generic APIs
From: Simon Shields @ 2018-01-13 2:04 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andi Shyti, linux-input, devicetree, Rob Herring, Simon Shields
The MMS114 platform data has no in-tree users, so drop it,
and make the driver depend on CONFIG_OF.
Switch to using the standard touchscreen properties via
touchscreen_parse_properties(), and move the old DT parsing code
to use device_property_*() APIs.
Finally, use touchscreen_report_pos to report x/y coordinates
and drop the custom x/y inversion code.
Signed-off-by: Simon Shields <simon@lineageos.org>
---
.../bindings/input/touchscreen/mms114.txt | 29 ++--
drivers/input/touchscreen/Kconfig | 1 +
drivers/input/touchscreen/mms114.c | 152 +++++++++------------
include/linux/platform_data/mms114.h | 24 ----
4 files changed, 83 insertions(+), 123 deletions(-)
delete mode 100644 include/linux/platform_data/mms114.h
diff --git a/Documentation/devicetree/bindings/input/touchscreen/mms114.txt b/Documentation/devicetree/bindings/input/touchscreen/mms114.txt
index 89d4c56c5671..8f9f9f38eff4 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/mms114.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/mms114.txt
@@ -4,14 +4,18 @@ Required properties:
- compatible: must be "melfas,mms114"
- reg: I2C address of the chip
- interrupts: interrupt to which the chip is connected
-- x-size: horizontal resolution of touchscreen
-- y-size: vertical resolution of touchscreen
+- touchscreen-size-x: See [1]
+- touchscreen-size-y: See [1]
Optional properties:
-- contact-threshold:
-- moving-threshold:
-- x-invert: invert X axis
-- y-invert: invert Y axis
+- touchscreen-fuzz-x: See [1]
+- touchscreen-fuzz-y: See [1]
+- touchscreen-fuzz-pressure: See [1]
+- touchscreen-inverted-x: See [1]
+- touchscreen-inverted-y: See [1]
+- touchscreen-swapped-x-y: See [1]
+
+[1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
Example:
@@ -22,12 +26,13 @@ Example:
compatible = "melfas,mms114";
reg = <0x48>;
interrupts = <39 0>;
- x-size = <720>;
- y-size = <1280>;
- contact-threshold = <10>;
- moving-threshold = <10>;
- x-invert;
- y-invert;
+ touchscreen-size-x = <720>;
+ touchscreen-size-y = <1280>;
+ touchscreen-fuzz-x = <10>;
+ touchscreen-fuzz-y = <10>;
+ touchscreen-fuzz-pressure = <10>;
+ touchscreen-inverted-x;
+ touchscreen-inverted-y;
};
/* ... */
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 38a226f9fcbd..f7ea5522ef91 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -524,6 +524,7 @@ config TOUCHSCREEN_MCS5000
config TOUCHSCREEN_MMS114
tristate "MELFAS MMS114 touchscreen"
depends on I2C
+ depends on OF
help
Say Y here if you have the MELFAS MMS114 touchscreen controller
chip in your system.
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index e5eeb6311f7d..6276a3387cd4 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -12,8 +12,8 @@
#include <linux/of.h>
#include <linux/i2c.h>
#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
#include <linux/interrupt.h>
-#include <linux/platform_data/mms114.h>
#include <linux/regulator/consumer.h>
#include <linux/slab.h>
@@ -55,7 +55,9 @@ struct mms114_data {
struct input_dev *input_dev;
struct regulator *core_reg;
struct regulator *io_reg;
- const struct mms114_platform_data *pdata;
+ struct touchscreen_properties props;
+ unsigned int contact_threshold;
+ unsigned int moving_threshold;
/* Use cache data for mode control register(write only) */
u8 cache_mode_control;
@@ -143,7 +145,6 @@ static int mms114_write_reg(struct mms114_data *data, unsigned int reg,
static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *touch)
{
- const struct mms114_platform_data *pdata = data->pdata;
struct i2c_client *client = data->client;
struct input_dev *input_dev = data->input_dev;
unsigned int id;
@@ -163,16 +164,6 @@ static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *tou
id = touch->id - 1;
x = touch->x_lo | touch->x_hi << 8;
y = touch->y_lo | touch->y_hi << 8;
- if (x > pdata->x_size || y > pdata->y_size) {
- dev_dbg(&client->dev,
- "Wrong touch coordinates (%d, %d)\n", x, y);
- return;
- }
-
- if (pdata->x_invert)
- x = pdata->x_size - x;
- if (pdata->y_invert)
- y = pdata->y_size - y;
dev_dbg(&client->dev,
"id: %d, type: %d, pressed: %d, x: %d, y: %d, width: %d, strength: %d\n",
@@ -183,9 +174,8 @@ static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *tou
input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, touch->pressed);
if (touch->pressed) {
+ touchscreen_report_pos(input_dev, &data->props, x, y, true);
input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, touch->width);
- input_report_abs(input_dev, ABS_MT_POSITION_X, x);
- input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
input_report_abs(input_dev, ABS_MT_PRESSURE, touch->strength);
}
}
@@ -263,7 +253,7 @@ static int mms114_get_version(struct mms114_data *data)
static int mms114_setup_regs(struct mms114_data *data)
{
- const struct mms114_platform_data *pdata = data->pdata;
+ const struct touchscreen_properties *props = &data->props;
int val;
int error;
@@ -275,32 +265,32 @@ static int mms114_setup_regs(struct mms114_data *data)
if (error < 0)
return error;
- val = (pdata->x_size >> 8) & 0xf;
- val |= ((pdata->y_size >> 8) & 0xf) << 4;
+ val = (props->max_x >> 8) & 0xf;
+ val |= ((props->max_y >> 8) & 0xf) << 4;
error = mms114_write_reg(data, MMS114_XY_RESOLUTION_H, val);
if (error < 0)
return error;
- val = pdata->x_size & 0xff;
+ val = props->max_x & 0xff;
error = mms114_write_reg(data, MMS114_X_RESOLUTION, val);
if (error < 0)
return error;
- val = pdata->y_size & 0xff;
+ val = props->max_x & 0xff;
error = mms114_write_reg(data, MMS114_Y_RESOLUTION, val);
if (error < 0)
return error;
- if (pdata->contact_threshold) {
+ if (data->contact_threshold) {
error = mms114_write_reg(data, MMS114_CONTACT_THRESHOLD,
- pdata->contact_threshold);
+ data->contact_threshold);
if (error < 0)
return error;
}
- if (pdata->moving_threshold) {
+ if (data->moving_threshold) {
error = mms114_write_reg(data, MMS114_MOVING_THRESHOLD,
- pdata->moving_threshold);
+ data->moving_threshold);
if (error < 0)
return error;
}
@@ -335,9 +325,6 @@ static int mms114_start(struct mms114_data *data)
return error;
}
- if (data->pdata->cfg_pin)
- data->pdata->cfg_pin(true);
-
enable_irq(client->irq);
return 0;
@@ -350,9 +337,6 @@ static void mms114_stop(struct mms114_data *data)
disable_irq(client->irq);
- if (data->pdata->cfg_pin)
- data->pdata->cfg_pin(false);
-
error = regulator_disable(data->io_reg);
if (error)
dev_warn(&client->dev, "Failed to disable vdd: %d\n", error);
@@ -376,67 +360,43 @@ static void mms114_input_close(struct input_dev *dev)
mms114_stop(data);
}
-#ifdef CONFIG_OF
-static struct mms114_platform_data *mms114_parse_dt(struct device *dev)
+static int mms114_parse_dt(struct mms114_data *data)
{
- struct mms114_platform_data *pdata;
- struct device_node *np = dev->of_node;
-
- if (!np)
- return NULL;
+ struct device *dev = &data->client->dev;
+ struct touchscreen_properties *props = &data->props;
- pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
- if (!pdata) {
- dev_err(dev, "failed to allocate platform data\n");
- return NULL;
+ if (device_property_read_u32(dev, "x-size", &props->max_x)) {
+ dev_dbg(dev, "failed to get legacy x-size property\n");
+ return -EINVAL;
}
- if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
- dev_err(dev, "failed to get x-size property\n");
- return NULL;
+ if (device_property_read_u32(dev, "y-size", &props->max_y)) {
+ dev_dbg(dev, "failed to get legacy y-size property\n");
+ return -EINVAL;
}
- if (of_property_read_u32(np, "y-size", &pdata->y_size)) {
- dev_err(dev, "failed to get y-size property\n");
- return NULL;
- }
+ device_property_read_u32(dev, "contact-threshold",
+ &data->contact_threshold);
+ device_property_read_u32(dev, "moving-threshold",
+ &data->moving_threshold);
- of_property_read_u32(np, "contact-threshold",
- &pdata->contact_threshold);
- of_property_read_u32(np, "moving-threshold",
- &pdata->moving_threshold);
+ if (device_property_read_bool(dev, "x-invert"))
+ props->invert_x = true;
+ if (device_property_read_bool(dev, "y-invert"))
+ props->invert_y = true;
- if (of_find_property(np, "x-invert", NULL))
- pdata->x_invert = true;
- if (of_find_property(np, "y-invert", NULL))
- pdata->y_invert = true;
+ props->swap_x_y = false;
- return pdata;
-}
-#else
-static inline struct mms114_platform_data *mms114_parse_dt(struct device *dev)
-{
- return NULL;
+ return 0;
}
-#endif
static int mms114_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
- const struct mms114_platform_data *pdata;
struct mms114_data *data;
struct input_dev *input_dev;
int error;
- pdata = dev_get_platdata(&client->dev);
- if (!pdata)
- pdata = mms114_parse_dt(&client->dev);
-
- if (!pdata) {
- dev_err(&client->dev, "Need platform data\n");
- return -EINVAL;
- }
-
if (!i2c_check_functionality(client->adapter,
I2C_FUNC_PROTOCOL_MANGLING)) {
dev_err(&client->dev,
@@ -453,8 +413,39 @@ static int mms114_probe(struct i2c_client *client,
}
data->client = client;
+
+ input_set_capability(input_dev, EV_KEY, BTN_TOUCH);
+ input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
+ input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
+ input_set_capability(input_dev, EV_ABS, ABS_MT_PRESSURE);
+ input_set_capability(input_dev, EV_ABS, ABS_MT_TOUCH_MAJOR);
+
+ input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
+
+ if (mms114_parse_dt(data) < 0) {
+ /* No valid legacy binding found, try the common one */
+ touchscreen_parse_properties(input_dev, true, &data->props);
+
+ /* The firmware handles movement and pressure fuzz, so
+ * don't duplicate that in software.
+ */
+ data->moving_threshold =
+ input_dev->absinfo[ABS_MT_POSITION_X].fuzz;
+ data->contact_threshold =
+ input_dev->absinfo[ABS_MT_PRESSURE].fuzz;
+
+ input_dev->absinfo[ABS_MT_POSITION_X].fuzz = 0;
+ input_dev->absinfo[ABS_MT_POSITION_Y].fuzz = 0;
+ input_dev->absinfo[ABS_MT_PRESSURE].fuzz = 0;
+ } else {
+ input_set_abs_params(input_dev, ABS_MT_POSITION_X,
+ 0, data->props.max_x, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
+ 0, data->props.max_y, 0, 0);
+
+ }
+
data->input_dev = input_dev;
- data->pdata = pdata;
input_dev->name = "MELFAS MMS114 Touchscreen";
input_dev->id.bustype = BUS_I2C;
@@ -462,21 +453,10 @@ static int mms114_probe(struct i2c_client *client,
input_dev->open = mms114_input_open;
input_dev->close = mms114_input_close;
- __set_bit(EV_ABS, input_dev->evbit);
- __set_bit(EV_KEY, input_dev->evbit);
- __set_bit(BTN_TOUCH, input_dev->keybit);
- input_set_abs_params(input_dev, ABS_X, 0, data->pdata->x_size, 0, 0);
- input_set_abs_params(input_dev, ABS_Y, 0, data->pdata->y_size, 0, 0);
-
/* For multi touch */
input_mt_init_slots(input_dev, MMS114_MAX_TOUCH, 0);
input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR,
0, MMS114_MAX_AREA, 0, 0);
- input_set_abs_params(input_dev, ABS_MT_POSITION_X,
- 0, data->pdata->x_size, 0, 0);
- input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
- 0, data->pdata->y_size, 0, 0);
- input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
input_set_drvdata(input_dev, data);
i2c_set_clientdata(client, data);
@@ -567,13 +547,11 @@ static const struct i2c_device_id mms114_id[] = {
};
MODULE_DEVICE_TABLE(i2c, mms114_id);
-#ifdef CONFIG_OF
static const struct of_device_id mms114_dt_match[] = {
{ .compatible = "melfas,mms114" },
{ }
};
MODULE_DEVICE_TABLE(of, mms114_dt_match);
-#endif
static struct i2c_driver mms114_driver = {
.driver = {
diff --git a/include/linux/platform_data/mms114.h b/include/linux/platform_data/mms114.h
deleted file mode 100644
index 5722ebfb2738..000000000000
--- a/include/linux/platform_data/mms114.h
+++ /dev/null
@@ -1,24 +0,0 @@
-/*
- * Copyright (C) 2012 Samsung Electronics Co.Ltd
- * Author: Joonyoung Shim <jy0922.shim@samsung.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundationr
- */
-
-#ifndef __LINUX_MMS114_H
-#define __LINUX_MMS114_H
-
-struct mms114_platform_data {
- unsigned int x_size;
- unsigned int y_size;
- unsigned int contact_threshold;
- unsigned int moving_threshold;
- bool x_invert;
- bool y_invert;
-
- void (*cfg_pin)(bool);
-};
-
-#endif /* __LINUX_MMS114_H */
--
2.15.1
^ permalink raw reply related
* Re: [PATCH] HID: asus: only support backlight when it's not driven by WMI
From: Mustafa Kuscu @ 2018-01-12 19:33 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Daniel Drake, Jiri Kosina, Benjamin Tissoires, linux-input,
Linux Upstreaming Team, Carlo Caione, Corentin Chary,
acpi4asus-user, Platform Driver
In-Reply-To: <CAHp75Vc962G3rufA_D1GY=Giy7u0vri4_vQnFPYCxZ_w6N_2GQ@mail.gmail.com>
On Tue, Jan 9, 2018 at 10:58 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jan 9, 2018 at 9:41 PM, Daniel Drake <drake@endlessm.com> wrote:
>> On Tue, Jan 9, 2018 at 1:32 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> Imagine someone who on possession of laptop where it's not needed
>>> having old kernel configuration.
>>> Building new kernel with old configuration will bring a regression.
>>>
>>> Selection is also not a solution since we don't need all crap in
>>> kernel because of some particular case.
>>>
>>> So, NO.
>>
>> If I can't use depends nor select, then what options are left? What
>> alternative solutions do you have in mind?
>
> If you would able to move code under corresponding WMI driver, make it
> use any means of autodetection (it looks like you found a way via
> checking return code of method call) and
> then rely on distributions that they enable necessary modules
> (HID_ASUS, ASUS_WMI or alike).
>
> --
> With Best Regards,
> Andy Shevchenko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
It is interesting that the backlight buttons did not work for you. I
think your keyboard (0b05:1837)is a
USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD2.
I also have a ROG laptop (GL553VD) whose keyboard (0b05:1854) is being
identified as USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1. Its keyboard
backlight shortcuts are enabled only after applying the following
patch. I am wondering why this harmless one liner did not find its way
into the modules tree. It's difficult to apply the patch to every keep
up with the latest. Maybe other ASUS people confirm this and help
pushing it further..
Kind Regards...
Signed-off-by: Mustafa C Kuscu <mustafakuscu@gmail.com>
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index a4a3c38bc145..39068eddc03c 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -593,7 +593,7 @@ static const struct hid_device_id asus_devices[] = {
{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
USB_DEVICE_ID_ASUSTEK_I2C_TOUCHPAD), I2C_TOUCHPAD_QUIRKS },
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
- USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1) },
+ USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1), QUIRK_USE_KBD_BACKLIGHT },
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD2), QUIRK_USE_KBD_BACKLIGHT },
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
^ permalink raw reply related
* Re: [PATCH] Input: trackpoint - force 3 buttons if 0 button is reported
From: Dmitry Torokhov @ 2018-01-12 18:18 UTC (permalink / raw)
To: Aaron Ma; +Cc: linux-input, linux-kernel
In-Reply-To: <06bbeca0-8a90-8935-9be3-f154db0b9fe9@canonical.com>
Hi Aaron,
On Fri, Jan 12, 2018 at 08:45:17PM +0800, Aaron Ma wrote:
> Only this laptop had been confirmed is ALPS: 0102 – FF02 trackpoint, it
> return 0 of extended button.
> I saw the other device that returned 0, but don't have it now, so the ID
> can not be checked.
>
> ThinkPad always have 3 buttons installed, I suggest always set 0x33 for
> button info when the button info returned 0.
I am really uncomfortable at us chipping away all validations in
trackpont protocol. The newer trackpoints (Elan, ALPS, etc) will be
taken care of with my patch that will not even try to fetch extended
button data for 0x02 - 0x04 extended ID devices, but unless we have a
concrete example of 0x01 responding with 0 for button info I will not
apply this patch.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: trackpoint - force 3 buttons if 0 button is reported
From: Aaron Ma @ 2018-01-12 12:45 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <20180112083940.p36qj6ltzzwtuhng@dtor-ws>
Only this laptop had been confirmed is ALPS: 0102 – FF02 trackpoint, it
return 0 of extended button.
I saw the other device that returned 0, but don't have it now, so the ID
can not be checked.
ThinkPad always have 3 buttons installed, I suggest always set 0x33 for
button info when the button info returned 0.
^ permalink raw reply
* Re: [PATCH] Input: trackpoint - force 3 buttons if 0 button is reported
From: Dmitry Torokhov @ 2018-01-12 8:39 UTC (permalink / raw)
To: Aaron Ma; +Cc: linux-input, linux-kernel
In-Reply-To: <20180112064658.25019-1-aaron.ma@canonical.com>
On Fri, Jan 12, 2018 at 02:46:58PM +0800, Aaron Ma wrote:
> Lenovo introduced trackpoint compatible sticks with minimum PS/2 commands.
> Some of these sticks with 3 buttons always return 0 when reading
> extended button info, set it as 3 buttons to enable middle button.
Does it only happen with newer ELan/ALPS/NXP trackpoints, or also with
trackpoints advertising the original 0x010E version? What devices
respond with 0 as button info?
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
> drivers/input/mouse/trackpoint.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
> index 0871010f18d5..00c0d1706567 100644
> --- a/drivers/input/mouse/trackpoint.c
> +++ b/drivers/input/mouse/trackpoint.c
> @@ -383,6 +383,10 @@ int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
> if (trackpoint_read(ps2dev, TP_EXT_BTN, &button_info)) {
> psmouse_warn(psmouse, "failed to get extended button data, assuming 3 buttons\n");
> button_info = 0x33;
> + } else if (!button_info) {
> + psmouse_warn(psmouse,
> + "got no extended button data, assuming 3 buttons\n");
> + button_info = 0x33;
> }
>
> psmouse->private = kzalloc(sizeof(struct trackpoint_data), GFP_KERNEL);
> --
> 2.14.3
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v3] Input: goodix - disable IRQs while suspended
From: Dmitry Torokhov @ 2018-01-12 8:37 UTC (permalink / raw)
To: Hans de Goede; +Cc: Bastien Nocera, Benjamin Tissoires, linux-input
In-Reply-To: <20180111172407.7558-1-hdegoede@redhat.com>
On Thu, Jan 11, 2018 at 06:24:07PM +0100, Hans de Goede wrote:
> We should not try to do any i2c transfers before the controller is
> resumed (which happens before our resume method gets called).
>
> So we need to disable our IRQ while suspended to enforce this. The
> code paths for devices with GPIOs for the int and reset pins already
> disable the IRQ the through goodix_free_irq().
>
> This commit also disables the IRQ while suspended for devices without
> GPIOs for the int and reset pins.
>
> This fixes the i2c bus sometimes getting stuck after a suspend/resume
> causing the touchscreen to sometimes not work after a suspend/resume.
> This has been tested on a GPD pocked device.
>
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://www.reddit.com/r/GPDPocket/comments/7niut2/fix_for_broken_touch_after_resume_all_linux/
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
Applied, thank you.
> ---
> Changes in v2:
> -Mention the problems this fix + devices tested on in commit msg
> -Add BugLinks and Tested-by to commit msg
>
> Changes in v3:
> -Fix typo in commit msg
> -Add Bastien's Reviewed-by
> ---
> drivers/input/touchscreen/goodix.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index 69d0b8cbc71f..ecec8eb17f28 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -878,8 +878,10 @@ static int __maybe_unused goodix_suspend(struct device *dev)
> int error;
>
> /* We need gpio pins to suspend/resume */
> - if (!ts->gpiod_int || !ts->gpiod_rst)
> + if (!ts->gpiod_int || !ts->gpiod_rst) {
> + disable_irq(client->irq);
> return 0;
> + }
>
> wait_for_completion(&ts->firmware_loading_complete);
>
> @@ -919,8 +921,10 @@ static int __maybe_unused goodix_resume(struct device *dev)
> struct goodix_ts_data *ts = i2c_get_clientdata(client);
> int error;
>
> - if (!ts->gpiod_int || !ts->gpiod_rst)
> + if (!ts->gpiod_int || !ts->gpiod_rst) {
> + enable_irq(client->irq);
> return 0;
> + }
>
> /*
> * Exit sleep mode by outputting HIGH level to INT pin
> --
> 2.14.3
>
--
Dmitry
^ permalink raw reply
* Re: [RESEND PATCH v2] input: pxrc: new driver for PhoenixRC Flight Controller Adapter
From: Dmitry Torokhov @ 2018-01-12 8:05 UTC (permalink / raw)
To: Marcus Folkesson
Cc: Jonathan Corbet, Tomohiro Yoshidomi, David Herrmann,
Philippe Ombredanne, Kate Stewart, Greg Kroah-Hartman,
linux-input, linux-doc, linux-kernel
In-Reply-To: <20180111120742.GA32505@gmail.com>
On Thu, Jan 11, 2018 at 01:07:42PM +0100, Marcus Folkesson wrote:
> Hi Dmitry,
>
> Thank you for your review!
>
> On Wed, Jan 10, 2018 at 04:37:14PM -0800, Dmitry Torokhov wrote:
> > Hi Marcus,
> >
> > On Wed, Jan 10, 2018 at 02:11:39PM +0100, Marcus Folkesson wrote:
> > > This driver let you plug in your RC controller to the adapter and
> > > use it as input device in various RC simulators.
> > >
> > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > ---
> > > v2:
> > > - Change module license to GPLv2 to match SPDX tag
> > >
> > > Documentation/input/devices/pxrc.rst | 57 ++++++++
> > > drivers/input/joystick/Kconfig | 9 ++
> > > drivers/input/joystick/Makefile | 1 +
> > > drivers/input/joystick/pxrc.c | 254 +++++++++++++++++++++++++++++++++++
> > > 4 files changed, 321 insertions(+)
> > > create mode 100644 Documentation/input/devices/pxrc.rst
> > > create mode 100644 drivers/input/joystick/pxrc.c
> > >
> > > diff --git a/Documentation/input/devices/pxrc.rst b/Documentation/input/devices/pxrc.rst
> > > new file mode 100644
> > > index 000000000000..0ec466c58958
> > > --- /dev/null
> > > +++ b/Documentation/input/devices/pxrc.rst
> > > @@ -0,0 +1,57 @@
> > > +=======================================================
> > > +pxrc - PhoenixRC Flight Controller Adapter
> > > +=======================================================
> > > +
> > > +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > +
> > > +This driver let you use your own RC controller plugged into the
> > > +adapter that comes with PhoenixRC [1]_ or other compatible adapters.
> > > +
> > > +The adapter supports 7 analog channels and 1 digital input switch.
> > > +
> > > +Notes
> > > +=====
> > > +
> > > +Many RC controllers is able to configure which stick goes to which channel.
> > > +This is also configurable in most simulators, so a matching is not necessary.
> > > +
> > > +The driver is generating the following input event for analog channels:
> > > +
> > > ++---------+----------------+
> > > +| Channel | Event |
> > > ++=========+================+
> > > +| 1 | ABS_X |
> > > ++---------+----------------+
> > > +| 2 | ABS_Y |
> > > ++---------+----------------+
> > > +| 3 | ABS_RX |
> > > ++---------+----------------+
> > > +| 4 | ABS_RY |
> > > ++---------+----------------+
> > > +| 5 | ABS_TILT_X |
> > > ++---------+----------------+
> > > +| 6 | ABS_TILT_Y |
> > > ++---------+----------------+
> >
> > TILT are normally reserved for stylus/pen. Do we have better event codes
> > maybe? Is there a picture of the RC so I can make more sense of the
> > proposed event assignment?
> >
>
> Ok, maybe RUDDER and MISC?
OK.
> The driver is actually for an "adapter" [1] that you connect your RC to.
> The RC is then mapping its sticks/buttons to different channels. Unlike other
> joysticks, this mapping is not fixed but something you setup in your RC.
>
> For example, I'm using a Turnigy 9xr[2].
>
> The RC is typically used with a RC Flight Simulator software (I'm using
> Heli-X [3] when testing) which also map the channels to events (throttle,
> rudder and so on).
>
>
> > > +| 7 | ABS_THROTTLE |
> > > ++---------+----------------+
> > > +
> > > +The digital input switch is generated as an `BTN_A` event.
> > > +
> > > +Manual Testing
> > > +==============
> > > +
> > > +To test this driver's functionality you may use `input-event` which is part of
> > > +the `input layer utilities` suite [2]_.
> > > +
> > > +For example::
> > > +
> > > + > modprobe pxrc
> > > + > input-events <devnr>
> > > +
> > > +To print all input events from input `devnr`.
> > > +
> > > +References
> > > +==========
> > > +
> > > +.. [1] http://www.phoenix-sim.com/
> > > +.. [2] https://www.kraxel.org/cgit/input/
> > > diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> > > index f3c2f6ea8b44..18ab6dafff41 100644
> > > --- a/drivers/input/joystick/Kconfig
> > > +++ b/drivers/input/joystick/Kconfig
> > > @@ -351,4 +351,13 @@ config JOYSTICK_PSXPAD_SPI_FF
> > >
> > > To drive rumble motor a dedicated power supply is required.
> > >
> > > +config JOYSTICK_PXRC
> > > + tristate "PhoenixRC Flight Controller Adapter"
> > > + depends on USB_ARCH_HAS_HCD
> > > + select USB
> > > + help
> > > + Say Y here if you want to use the PhoenixRC Flight Controller Adapter.
> > > +
> > > + To compile this driver as a module, choose M here: the
> > > + module will be called pxrc.
> > > endif
> > > diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> > > index 67651efda2e1..dd0492ebbed7 100644
> > > --- a/drivers/input/joystick/Makefile
> > > +++ b/drivers/input/joystick/Makefile
> > > @@ -23,6 +23,7 @@ obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
> > > obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
> > > obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
> > > obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> > > +obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> > > obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> > > obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> > > obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
> > > diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> > > new file mode 100644
> > > index 000000000000..2bec99df97e2
> > > --- /dev/null
> > > +++ b/drivers/input/joystick/pxrc.c
> > > @@ -0,0 +1,254 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Driver for Phoenix RC Flight Controller Adapter
> > > + *
> > > + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> > > + *
> > > + */
> > > +
> > > +#include <linux/kernel.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/module.h>
> > > +#include <linux/kref.h>
> > > +#include <linux/uaccess.h>
> > > +#include <linux/usb.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/input.h>
> > > +
> > > +#define PXRC_VENDOR_ID (0x1781)
> > > +#define PXRC_PRODUCT_ID (0x0898)
> > > +
> > > +static const struct usb_device_id pxrc_table[] = {
> > > + { USB_DEVICE(PXRC_VENDOR_ID, PXRC_PRODUCT_ID) },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(usb, pxrc_table);
> > > +
> > > +struct usb_pxrc {
> > > + struct input_dev *input_dev;
> > > + struct usb_device *udev;
> > > + struct usb_interface *interface;
> > > + struct usb_anchor anchor;
> > > + __u8 epaddr;
> > > + char phys[64];
> > > + unsigned char *data;
> > > + size_t bsize;
> > > + struct kref kref;
> > > +};
> > > +
> > > +#define to_pxrc_dev(d) container_of(d, struct usb_pxrc, kref)
> > > +static void pxrc_delete(struct kref *kref)
> > > +{
> > > + struct usb_pxrc *pxrc = to_pxrc_dev(kref);
> > > +
> > > + usb_put_dev(pxrc->udev);
> > > +}
> > > +
> > > +static void pxrc_usb_irq(struct urb *urb)
> > > +{
> > > + struct usb_pxrc *pxrc = urb->context;
> > > +
> > > + switch (urb->status) {
> > > + case 0:
> > > + /* success */
> > > + break;
> > > + case -ETIME:
> > > + /* this urb is timing out */
> > > + dev_dbg(&pxrc->interface->dev,
> > > + "%s - urb timed out - was the device unplugged?\n",
> > > + __func__);
> > > + return;
> > > + case -ECONNRESET:
> > > + case -ENOENT:
> > > + case -ESHUTDOWN:
> > > + case -EPIPE:
> > > + /* this urb is terminated, clean up */
> > > + dev_dbg(&pxrc->interface->dev, "%s - urb shutting down with status: %d\n",
> > > + __func__, urb->status);
> > > + return;
> > > + default:
> > > + dev_dbg(&pxrc->interface->dev, "%s - nonzero urb status received: %d\n",
> > > + __func__, urb->status);
> > > + goto exit;
> > > + }
> > > +
> > > + if (urb->actual_length == 8) {
> > > + input_report_abs(pxrc->input_dev, ABS_X, pxrc->data[0]);
> > > + input_report_abs(pxrc->input_dev, ABS_Y, pxrc->data[2]);
> > > + input_report_abs(pxrc->input_dev, ABS_RX, pxrc->data[3]);
> > > + input_report_abs(pxrc->input_dev, ABS_RY, pxrc->data[4]);
> > > + input_report_abs(pxrc->input_dev, ABS_TILT_X, pxrc->data[5]);
> > > + input_report_abs(pxrc->input_dev, ABS_TILT_Y, pxrc->data[6]);
> > > + input_report_abs(pxrc->input_dev, ABS_THROTTLE, pxrc->data[7]);
> > > +
> > > + input_report_key(pxrc->input_dev, BTN_A, pxrc->data[1]);
> > > + }
> > > +
> > > +exit:
> > > + /* Resubmit to fetch new fresh URBs */
> > > + usb_anchor_urb(urb, &pxrc->anchor);
> > > + if (usb_submit_urb(urb, GFP_ATOMIC) < 0)
> > > + usb_unanchor_urb(urb);
> > > +}
> > > +
> > > +static int pxrc_submit_intr_urb(struct usb_pxrc *pxrc)
> > > +{
> > > + struct urb *urb;
> > > + unsigned int pipe;
> > > + int err;
> > > +
> > > + urb = usb_alloc_urb(0, GFP_KERNEL);
> > > + if (!urb)
> > > + return -ENOMEM;
> > > +
> > > + pipe = usb_rcvintpipe(pxrc->udev, pxrc->epaddr),
> > > + usb_fill_int_urb(urb, pxrc->udev, pipe, pxrc->data, pxrc->bsize,
> > > + pxrc_usb_irq, pxrc, 1);
> > > + usb_anchor_urb(urb, &pxrc->anchor);
> > > + err = usb_submit_urb(urb, GFP_KERNEL);
> > > + if (err < 0)
> > > + usb_unanchor_urb(urb);
> > > +
> > > + usb_free_urb(urb);
> > > + return err;
> > > +}
> > > +
> > > +static int pxrc_open(struct input_dev *input)
> > > +{
> > > + struct usb_pxrc *pxrc = input_get_drvdata(input);
> > > + int err;
> > > +
> > > + err = pxrc_submit_intr_urb(pxrc);
> > > + if (err < 0)
> > > + goto error;
> > > +
> > > + kref_get(&pxrc->kref);
> > > + return 0;
> > > +
> > > +error:
> > > + usb_kill_anchored_urbs(&pxrc->anchor);
> > > + return err;
> > > +}
> > > +
> > > +static void pxrc_close(struct input_dev *input)
> > > +{
> > > + struct usb_pxrc *pxrc = input_get_drvdata(input);
> > > +
> > > + usb_kill_anchored_urbs(&pxrc->anchor);
> > > + kref_put(&pxrc->kref, pxrc_delete);
> >
> > This is way to complex and I am not sure why you need to anchor URBs and
> > have a separate refcounting, etc. I do not think it is even safe with
> > module unloading as kref might be in flight as you run through module
> > unload and devm will destroy most of the objects.
> >
> > Why don't you simply use usb_kill_urb() here and be done.
> > input_unregister_device() that is called as part of devm unwind on
> > device unbind will make sure that pxrc_close() is called.
> >
> > Thanks.
> >
>
> Ok, I drop the refcounting. pxrc_delete() did more before I changed most
> allocations to devm_*.
> I tried to get the refcounting similiar to /drivers/usb/usb-skeleton.c.
>
> I have tried different approaches with the create/delete URBs.
> At first I had the URB in the usb_pxrc struct that I allocated in the
> probe function where I also submitted the URB.
>
> This worked but I found it waste to submit URBs when noone has opened the
> device, so I moved the submitting to pxrc_open().
> The problem was that the same URB was submitted multiple times (gives a
> warning) upon multiple calls to open(2).
open() should only be called once, unless close() was called. So as long
as you kill the URB in close(), you should not see warnings.
>
> So I removed the URB from usb_pxrc and used an anchor to still have a
> reference to it for deletion. This part is heavily inspired by
> drivers/bluetooth/bpa10x.c.
> Maybe I should do it differently?
>
> I must admit that I'm not very familiar with USB drivers.
I'd probably look at drivers/input/mouse/synaptics_usb.c and structured
your driver similarly.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: multi-touch fix for ALPS touchpads ("SS4 plus" variant)
From: Dmitry Torokhov @ 2018-01-12 7:43 UTC (permalink / raw)
To: Masaki Ota
Cc: Nir Perry, Pali Rohár, linux-kernel@vger.kernel.org,
linux-input@vger.kernel.org
In-Reply-To: <TYXPR01MB0719F851EB4CC5EA8375845EC7170@TYXPR01MB0719.jpnprd01.prod.outlook.com>
On Fri, Jan 12, 2018 at 01:02:55AM +0000, Masaki Ota wrote:
> Hi, Nir,
>
> Wow, thank you for fixing the bug.
> Your code is correct!
Great, I am putting you down as "Reviewed-by" then. Thanks!
>
> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Nir Perry [mailto:nirperry@gmail.com]
> Sent: Saturday, January 06, 2018 8:55 PM
> To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; Dmitry Torokhov <dmitry.torokhov@gmail.com>; Pali Rohár <pali.rohar@gmail.com>
> Cc: linux-kernel@vger.kernel.org; linux-input@vger.kernel.org
> Subject: [PATCH] input: multi-touch fix for ALPS touchpads ("SS4 plus" variant)
>
> Hi all,
>
> I think a minor "typo" bug was accidentally introduced to ALPS touchpad driver by a previous bug-fix (commit 4a646580f793d19717f7e034c8d473b509c27d49, "Input: ALPS - fix two-finger scroll breakage in right side on ALPS touchpad").
> It breaks how multi-touch events are decoded on some ALPS touchpads, so for example tapping with three-fingers can no longer be used to emulate middle-mouse-button (the kernel doesn't recognize this as the proper event, and doesn't report it correctly to userspace).
> This affects touchpads that use SS4 "plus" protocol variant, like those found on Dell E7270 & E7470 laptops (tested on E7270).
>
> The cause of the problem
> ------------------------------------------
> First, probably due to a typo, the code in alps_decode_ss4_v2() for case SS4_PACKET_ID_MULTI used inconsistent indices to "f->mt[]". You can see 0 & 1 are used for the "if" part but 2 & 3 are used for the "else" part, which I believe is a typo.
> Second, in the previous patch, new macros were introduced to decode X coordinates specific to the SS4 "plus" variant, but the macro to define the maximum X value wasn't changed accordingly. The macros to decode X values for "plus" variant are effectively shifted right by 1 bit, but the max wasn't shifted too. This causes the driver to incorrectly handle "no data" cases, which also interfered with how multi-touch was handled. To fix it - I created new SS4 "plus" macros for the max value - SS4_PLUS_MFPACKET_NO_AX & SS4_PLUS_MFPACKET_NO_AX_BL. To make the change a little more readable, I moved also the Y-max lines so they are closer to the X-max lines.
> To get three-finger tap to work both changes are required.
>
> The included patch was generated against the mainline tree today, but was also tested against the 4.14 kernel branch. I've included in this e-mail the people involved with the old patch from August, plus Pali Rohár who is listed as the ALPS PS/2 touchpad driver reviewer (in the maintainers file).
>
> Fixes: 4a646580f793d19717f7e034c8d473b509c27d49 ("Input: ALPS - fix two-finger scroll breakage in right side on ALPS touchpad")
>
> Regards,
> Nir
>
> Signed-off-by: Nir Perry <nirperry@gmail.com> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c index 579b899..dbe57da 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -1250,29 +1250,32 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
> case SS4_PACKET_ID_MULTI:
> if (priv->flags & ALPS_BUTTONPAD) {
> if (IS_SS4PLUS_DEV(priv->dev_id)) {
> - f->mt[0].x = SS4_PLUS_BTL_MF_X_V2(p, 0);
> - f->mt[1].x = SS4_PLUS_BTL_MF_X_V2(p, 1);
> + f->mt[2].x = SS4_PLUS_BTL_MF_X_V2(p, 0);
> + f->mt[3].x = SS4_PLUS_BTL_MF_X_V2(p, 1);
> + no_data_x = SS4_PLUS_MFPACKET_NO_AX_BL;
> } else {
> f->mt[2].x = SS4_BTL_MF_X_V2(p, 0);
> f->mt[3].x = SS4_BTL_MF_X_V2(p, 1);
> + no_data_x = SS4_MFPACKET_NO_AX_BL;
> }
> + no_data_y = SS4_MFPACKET_NO_AY_BL;
>
> f->mt[2].y = SS4_BTL_MF_Y_V2(p, 0);
> f->mt[3].y = SS4_BTL_MF_Y_V2(p, 1);
> - no_data_x = SS4_MFPACKET_NO_AX_BL;
> - no_data_y = SS4_MFPACKET_NO_AY_BL;
> } else {
> if (IS_SS4PLUS_DEV(priv->dev_id)) {
> - f->mt[0].x = SS4_PLUS_STD_MF_X_V2(p, 0);
> - f->mt[1].x = SS4_PLUS_STD_MF_X_V2(p, 1);
> + f->mt[2].x = SS4_PLUS_STD_MF_X_V2(p, 0);
> + f->mt[3].x = SS4_PLUS_STD_MF_X_V2(p, 1);
> + no_data_x = SS4_PLUS_MFPACKET_NO_AX;
> } else {
> - f->mt[0].x = SS4_STD_MF_X_V2(p, 0);
> - f->mt[1].x = SS4_STD_MF_X_V2(p, 1);
> + f->mt[2].x = SS4_STD_MF_X_V2(p, 0);
> + f->mt[3].x = SS4_STD_MF_X_V2(p, 1);
> + no_data_x = SS4_MFPACKET_NO_AX;
> }
> + no_data_y = SS4_MFPACKET_NO_AY;
> +
> f->mt[2].y = SS4_STD_MF_Y_V2(p, 0);
> f->mt[3].y = SS4_STD_MF_Y_V2(p, 1);
> - no_data_x = SS4_MFPACKET_NO_AX;
> - no_data_y = SS4_MFPACKET_NO_AY;
> }
>
> f->first_mp = 0;
> diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h index c80a7c7..3dfae83 100644
> --- a/drivers/input/mouse/alps.h
> +++ b/drivers/input/mouse/alps.h
> @@ -145,6 +145,8 @@ enum SS4_PACKET_ID {
> #define SS4_MFPACKET_NO_AY 4080 /* Y-Coordinate value */
> #define SS4_MFPACKET_NO_AX_BL 8176 /* Buttonless X-Coordinate value */
> #define SS4_MFPACKET_NO_AY_BL 4088 /* Buttonless Y-Coordinate value */
> +#define SS4_PLUS_MFPACKET_NO_AX 4080 /* SS4 PLUS, x */
> +#define SS4_PLUS_MFPACKET_NO_AX_BL 4088 /* Buttonless SS4 PLUS, x */
>
> /*
> * enum V7_PACKET_ID - defines the packet type for V7
--
Dmitry
^ permalink raw reply
* [PATCH] Input: trackpoint - force 3 buttons if 0 button is reported
From: Aaron Ma @ 2018-01-12 6:46 UTC (permalink / raw)
To: dmitry.torokhov, linux-input, linux-kernel
Lenovo introduced trackpoint compatible sticks with minimum PS/2 commands.
Some of these sticks with 3 buttons always return 0 when reading
extended button info, set it as 3 buttons to enable middle button.
Cc: stable@vger.kernel.org
Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
drivers/input/mouse/trackpoint.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index 0871010f18d5..00c0d1706567 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -383,6 +383,10 @@ int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
if (trackpoint_read(ps2dev, TP_EXT_BTN, &button_info)) {
psmouse_warn(psmouse, "failed to get extended button data, assuming 3 buttons\n");
button_info = 0x33;
+ } else if (!button_info) {
+ psmouse_warn(psmouse,
+ "got no extended button data, assuming 3 buttons\n");
+ button_info = 0x33;
}
psmouse->private = kzalloc(sizeof(struct trackpoint_data), GFP_KERNEL);
--
2.14.3
^ permalink raw reply related
* RE: [PATCH] input: multi-touch fix for ALPS touchpads ("SS4 plus" variant)
From: Masaki Ota @ 2018-01-12 1:02 UTC (permalink / raw)
To: Nir Perry, Dmitry Torokhov, Pali Rohár
Cc: linux-kernel@vger.kernel.org, linux-input@vger.kernel.org
In-Reply-To: <CAE1KSOPo=uQbE4tCpZF4UzXRsfzA1gTqW4-UURGjt2u2+ZH_Mg@mail.gmail.com>
Hi, Nir,
Wow, thank you for fixing the bug.
Your code is correct!
Best Regards,
Masaki Ota
-----Original Message-----
From: Nir Perry [mailto:nirperry@gmail.com]
Sent: Saturday, January 06, 2018 8:55 PM
To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; Dmitry Torokhov <dmitry.torokhov@gmail.com>; Pali Rohár <pali.rohar@gmail.com>
Cc: linux-kernel@vger.kernel.org; linux-input@vger.kernel.org
Subject: [PATCH] input: multi-touch fix for ALPS touchpads ("SS4 plus" variant)
Hi all,
I think a minor "typo" bug was accidentally introduced to ALPS touchpad driver by a previous bug-fix (commit 4a646580f793d19717f7e034c8d473b509c27d49, "Input: ALPS - fix two-finger scroll breakage in right side on ALPS touchpad").
It breaks how multi-touch events are decoded on some ALPS touchpads, so for example tapping with three-fingers can no longer be used to emulate middle-mouse-button (the kernel doesn't recognize this as the proper event, and doesn't report it correctly to userspace).
This affects touchpads that use SS4 "plus" protocol variant, like those found on Dell E7270 & E7470 laptops (tested on E7270).
The cause of the problem
------------------------------------------
First, probably due to a typo, the code in alps_decode_ss4_v2() for case SS4_PACKET_ID_MULTI used inconsistent indices to "f->mt[]". You can see 0 & 1 are used for the "if" part but 2 & 3 are used for the "else" part, which I believe is a typo.
Second, in the previous patch, new macros were introduced to decode X coordinates specific to the SS4 "plus" variant, but the macro to define the maximum X value wasn't changed accordingly. The macros to decode X values for "plus" variant are effectively shifted right by 1 bit, but the max wasn't shifted too. This causes the driver to incorrectly handle "no data" cases, which also interfered with how multi-touch was handled. To fix it - I created new SS4 "plus" macros for the max value - SS4_PLUS_MFPACKET_NO_AX & SS4_PLUS_MFPACKET_NO_AX_BL. To make the change a little more readable, I moved also the Y-max lines so they are closer to the X-max lines.
To get three-finger tap to work both changes are required.
The included patch was generated against the mainline tree today, but was also tested against the 4.14 kernel branch. I've included in this e-mail the people involved with the old patch from August, plus Pali Rohár who is listed as the ALPS PS/2 touchpad driver reviewer (in the maintainers file).
Fixes: 4a646580f793d19717f7e034c8d473b509c27d49 ("Input: ALPS - fix two-finger scroll breakage in right side on ALPS touchpad")
Regards,
Nir
Signed-off-by: Nir Perry <nirperry@gmail.com> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c index 579b899..dbe57da 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -1250,29 +1250,32 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
case SS4_PACKET_ID_MULTI:
if (priv->flags & ALPS_BUTTONPAD) {
if (IS_SS4PLUS_DEV(priv->dev_id)) {
- f->mt[0].x = SS4_PLUS_BTL_MF_X_V2(p, 0);
- f->mt[1].x = SS4_PLUS_BTL_MF_X_V2(p, 1);
+ f->mt[2].x = SS4_PLUS_BTL_MF_X_V2(p, 0);
+ f->mt[3].x = SS4_PLUS_BTL_MF_X_V2(p, 1);
+ no_data_x = SS4_PLUS_MFPACKET_NO_AX_BL;
} else {
f->mt[2].x = SS4_BTL_MF_X_V2(p, 0);
f->mt[3].x = SS4_BTL_MF_X_V2(p, 1);
+ no_data_x = SS4_MFPACKET_NO_AX_BL;
}
+ no_data_y = SS4_MFPACKET_NO_AY_BL;
f->mt[2].y = SS4_BTL_MF_Y_V2(p, 0);
f->mt[3].y = SS4_BTL_MF_Y_V2(p, 1);
- no_data_x = SS4_MFPACKET_NO_AX_BL;
- no_data_y = SS4_MFPACKET_NO_AY_BL;
} else {
if (IS_SS4PLUS_DEV(priv->dev_id)) {
- f->mt[0].x = SS4_PLUS_STD_MF_X_V2(p, 0);
- f->mt[1].x = SS4_PLUS_STD_MF_X_V2(p, 1);
+ f->mt[2].x = SS4_PLUS_STD_MF_X_V2(p, 0);
+ f->mt[3].x = SS4_PLUS_STD_MF_X_V2(p, 1);
+ no_data_x = SS4_PLUS_MFPACKET_NO_AX;
} else {
- f->mt[0].x = SS4_STD_MF_X_V2(p, 0);
- f->mt[1].x = SS4_STD_MF_X_V2(p, 1);
+ f->mt[2].x = SS4_STD_MF_X_V2(p, 0);
+ f->mt[3].x = SS4_STD_MF_X_V2(p, 1);
+ no_data_x = SS4_MFPACKET_NO_AX;
}
+ no_data_y = SS4_MFPACKET_NO_AY;
+
f->mt[2].y = SS4_STD_MF_Y_V2(p, 0);
f->mt[3].y = SS4_STD_MF_Y_V2(p, 1);
- no_data_x = SS4_MFPACKET_NO_AX;
- no_data_y = SS4_MFPACKET_NO_AY;
}
f->first_mp = 0;
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h index c80a7c7..3dfae83 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -145,6 +145,8 @@ enum SS4_PACKET_ID {
#define SS4_MFPACKET_NO_AY 4080 /* Y-Coordinate value */
#define SS4_MFPACKET_NO_AX_BL 8176 /* Buttonless X-Coordinate value */
#define SS4_MFPACKET_NO_AY_BL 4088 /* Buttonless Y-Coordinate value */
+#define SS4_PLUS_MFPACKET_NO_AX 4080 /* SS4 PLUS, x */
+#define SS4_PLUS_MFPACKET_NO_AX_BL 4088 /* Buttonless SS4 PLUS, x */
/*
* enum V7_PACKET_ID - defines the packet type for V7
^ permalink raw reply
* [PATCH v3] Input: goodix - disable IRQs while suspended
From: Hans de Goede @ 2018-01-11 17:24 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Benjamin Tissoires
Cc: Hans de Goede, linux-input
We should not try to do any i2c transfers before the controller is
resumed (which happens before our resume method gets called).
So we need to disable our IRQ while suspended to enforce this. The
code paths for devices with GPIOs for the int and reset pins already
disable the IRQ the through goodix_free_irq().
This commit also disables the IRQ while suspended for devices without
GPIOs for the int and reset pins.
This fixes the i2c bus sometimes getting stuck after a suspend/resume
causing the touchscreen to sometimes not work after a suspend/resume.
This has been tested on a GPD pocked device.
BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
BugLink: https://www.reddit.com/r/GPDPocket/comments/7niut2/fix_for_broken_touch_after_resume_all_linux/
Tested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Bastien Nocera <hadess@hadess.net>
---
Changes in v2:
-Mention the problems this fix + devices tested on in commit msg
-Add BugLinks and Tested-by to commit msg
Changes in v3:
-Fix typo in commit msg
-Add Bastien's Reviewed-by
---
drivers/input/touchscreen/goodix.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 69d0b8cbc71f..ecec8eb17f28 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -878,8 +878,10 @@ static int __maybe_unused goodix_suspend(struct device *dev)
int error;
/* We need gpio pins to suspend/resume */
- if (!ts->gpiod_int || !ts->gpiod_rst)
+ if (!ts->gpiod_int || !ts->gpiod_rst) {
+ disable_irq(client->irq);
return 0;
+ }
wait_for_completion(&ts->firmware_loading_complete);
@@ -919,8 +921,10 @@ static int __maybe_unused goodix_resume(struct device *dev)
struct goodix_ts_data *ts = i2c_get_clientdata(client);
int error;
- if (!ts->gpiod_int || !ts->gpiod_rst)
+ if (!ts->gpiod_int || !ts->gpiod_rst) {
+ enable_irq(client->irq);
return 0;
+ }
/*
* Exit sleep mode by outputting HIGH level to INT pin
--
2.14.3
^ permalink raw reply related
* Re: [PATCH v2] Input: goodix - disable IRQs while suspended
From: Hans de Goede @ 2018-01-11 17:23 UTC (permalink / raw)
To: Bastien Nocera, Dmitry Torokhov, Benjamin Tissoires; +Cc: linux-input
In-Reply-To: <1515685954.3261.3.camel@hadess.net>
Hi,
On 11-01-18 16:52, Bastien Nocera wrote:
> On Thu, 2018-01-11 at 15:12 +0100, Hans de Goede wrote:
>> We should not try to do any i2c transfers before the controller is
>> resumed (which happens before our resume method gets called).
>>
>> So we need to disable our IRQ while suspended to enforce this. The
>> code paths for devices with GPIOs for the int and reset pins already
>> disable the IRQ the through goodix_free_irq().
>>
>> This commit also disables the IRQ while suspended for devices without
>> GPIOs for the int and reset pins.
>>
>> This fixes the i2c bus sometimes getting stuck after a suspend/resume
>> causing the touchscreen to sometimes not work after a suspend/resume.
>> This has been tested on a GPD pocked device.
>
> pocket?
Erm, yes.
Regards,
Hans
>
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
>
>> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
>> BugLink: https://www.reddit.com/r/GPDPocket/comments/7niut2/fix_for_b
>> roken_touch_after_resume_all_linux/
>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Mention the problems this fix + devices tested on in commit msg
>> -Add BugLinks and Tested-by to commit msg
>> ---
>> drivers/input/touchscreen/goodix.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/goodix.c
>> b/drivers/input/touchscreen/goodix.c
>> index 69d0b8cbc71f..ecec8eb17f28 100644
>> --- a/drivers/input/touchscreen/goodix.c
>> +++ b/drivers/input/touchscreen/goodix.c
>> @@ -878,8 +878,10 @@ static int __maybe_unused goodix_suspend(struct
>> device *dev)
>> int error;
>>
>> /* We need gpio pins to suspend/resume */
>> - if (!ts->gpiod_int || !ts->gpiod_rst)
>> + if (!ts->gpiod_int || !ts->gpiod_rst) {
>> + disable_irq(client->irq);
>> return 0;
>> + }
>>
>> wait_for_completion(&ts->firmware_loading_complete);
>>
>> @@ -919,8 +921,10 @@ static int __maybe_unused goodix_resume(struct
>> device *dev)
>> struct goodix_ts_data *ts = i2c_get_clientdata(client);
>> int error;
>>
>> - if (!ts->gpiod_int || !ts->gpiod_rst)
>> + if (!ts->gpiod_int || !ts->gpiod_rst) {
>> + enable_irq(client->irq);
>> return 0;
>> + }
>>
>> /*
>> * Exit sleep mode by outputting HIGH level to INT pin
^ permalink raw reply
* Re: [PATCH v2] Input: goodix - disable IRQs while suspended
From: Bastien Nocera @ 2018-01-11 15:52 UTC (permalink / raw)
To: Hans de Goede, Dmitry Torokhov, Benjamin Tissoires; +Cc: linux-input
In-Reply-To: <20180111141211.16888-1-hdegoede@redhat.com>
On Thu, 2018-01-11 at 15:12 +0100, Hans de Goede wrote:
> We should not try to do any i2c transfers before the controller is
> resumed (which happens before our resume method gets called).
>
> So we need to disable our IRQ while suspended to enforce this. The
> code paths for devices with GPIOs for the int and reset pins already
> disable the IRQ the through goodix_free_irq().
>
> This commit also disables the IRQ while suspended for devices without
> GPIOs for the int and reset pins.
>
> This fixes the i2c bus sometimes getting stuck after a suspend/resume
> causing the touchscreen to sometimes not work after a suspend/resume.
> This has been tested on a GPD pocked device.
pocket?
Reviewed-by: Bastien Nocera <hadess@hadess.net>
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://www.reddit.com/r/GPDPocket/comments/7niut2/fix_for_b
> roken_touch_after_resume_all_linux/
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Mention the problems this fix + devices tested on in commit msg
> -Add BugLinks and Tested-by to commit msg
> ---
> drivers/input/touchscreen/goodix.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 69d0b8cbc71f..ecec8eb17f28 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -878,8 +878,10 @@ static int __maybe_unused goodix_suspend(struct
> device *dev)
> int error;
>
> /* We need gpio pins to suspend/resume */
> - if (!ts->gpiod_int || !ts->gpiod_rst)
> + if (!ts->gpiod_int || !ts->gpiod_rst) {
> + disable_irq(client->irq);
> return 0;
> + }
>
> wait_for_completion(&ts->firmware_loading_complete);
>
> @@ -919,8 +921,10 @@ static int __maybe_unused goodix_resume(struct
> device *dev)
> struct goodix_ts_data *ts = i2c_get_clientdata(client);
> int error;
>
> - if (!ts->gpiod_int || !ts->gpiod_rst)
> + if (!ts->gpiod_int || !ts->gpiod_rst) {
> + enable_irq(client->irq);
> return 0;
> + }
>
> /*
> * Exit sleep mode by outputting HIGH level to INT pin
^ permalink raw reply
* Re: [PATCH 2/2] HID: asus: Fix special function keys on T200TA
From: Jiri Kosina @ 2018-01-11 14:21 UTC (permalink / raw)
To: Hans de Goede; +Cc: Benjamin Tissoires, linux-input
In-Reply-To: <aea8b618-0e9a-8516-fb6a-d3409b361edf@redhat.com>
On Thu, 11 Jan 2018, Hans de Goede wrote:
> >> Just like on the T100TA the T200TA HID descriptors for the 0xff32
> >> Asus vendor usage page need a small fixup. But on the T200TA the HID
> >> descriptors are larger because they have descrriptors for one more
> >> (unused) HID report appended.
> >>
> >> Extend the T100TA descriptor fixup to also check for the T200TA's
> >> descriptors size.
> >
> > Sigh, both are:
> > Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Thanks. Jiri, what is the status of these ?
Over past weeks I have been completely swamped in the Meltdown/Spectre
happines.
I hope to resume merging HID patches any time soon.
Sorry for the delays,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH v2] Input: goodix - disable IRQs while suspended
From: Hans de Goede @ 2018-01-11 14:12 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Benjamin Tissoires
Cc: Hans de Goede, linux-input
We should not try to do any i2c transfers before the controller is
resumed (which happens before our resume method gets called).
So we need to disable our IRQ while suspended to enforce this. The
code paths for devices with GPIOs for the int and reset pins already
disable the IRQ the through goodix_free_irq().
This commit also disables the IRQ while suspended for devices without
GPIOs for the int and reset pins.
This fixes the i2c bus sometimes getting stuck after a suspend/resume
causing the touchscreen to sometimes not work after a suspend/resume.
This has been tested on a GPD pocked device.
BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
BugLink: https://www.reddit.com/r/GPDPocket/comments/7niut2/fix_for_broken_touch_after_resume_all_linux/
Tested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Mention the problems this fix + devices tested on in commit msg
-Add BugLinks and Tested-by to commit msg
---
drivers/input/touchscreen/goodix.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 69d0b8cbc71f..ecec8eb17f28 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -878,8 +878,10 @@ static int __maybe_unused goodix_suspend(struct device *dev)
int error;
/* We need gpio pins to suspend/resume */
- if (!ts->gpiod_int || !ts->gpiod_rst)
+ if (!ts->gpiod_int || !ts->gpiod_rst) {
+ disable_irq(client->irq);
return 0;
+ }
wait_for_completion(&ts->firmware_loading_complete);
@@ -919,8 +921,10 @@ static int __maybe_unused goodix_resume(struct device *dev)
struct goodix_ts_data *ts = i2c_get_clientdata(client);
int error;
- if (!ts->gpiod_int || !ts->gpiod_rst)
+ if (!ts->gpiod_int || !ts->gpiod_rst) {
+ enable_irq(client->irq);
return 0;
+ }
/*
* Exit sleep mode by outputting HIGH level to INT pin
--
2.14.3
^ permalink raw reply related
* Re: [PATCH 2/2] HID: asus: Fix special function keys on T200TA
From: Hans de Goede @ 2018-01-11 14:10 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input
In-Reply-To: <CAO-hwJKyO+fWno7OaHBdki3pmCr183wh4NexkjYa73Jqi_ni7g@mail.gmail.com>
Hi,
On 05-01-18 18:00, Benjamin Tissoires wrote:
> On Fri, Jan 5, 2018 at 12:09 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Just like on the T100TA the T200TA HID descriptors for the 0xff32
>> Asus vendor usage page need a small fixup. But on the T200TA the HID
>> descriptors are larger because they have descrriptors for one more
>> (unused) HID report appended.
>>
>> Extend the T100TA descriptor fixup to also check for the T200TA's
>> descriptors size.
>
> Sigh, both are:
> Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Thanks. Jiri, what is the status of these ?
Regards,
Hans
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/hid/hid-asus.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index 07525bc99b6a..88b9703318e4 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
>> @@ -715,9 +715,10 @@ static __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>> hid_info(hdev, "Fixing up Asus notebook report descriptor\n");
>> rdesc[55] = 0xdd;
>> }
>> - /* For the T100TA keyboard dock */
>> + /* For the T100TA/T200TA keyboard dock */
>> if (drvdata->quirks & QUIRK_T100_KEYBOARD &&
>> - *rsize == 76 && rdesc[73] == 0x81 && rdesc[74] == 0x01) {
>> + (*rsize == 76 || *rsize == 101) &&
>> + rdesc[73] == 0x81 && rdesc[74] == 0x01) {
>> hid_info(hdev, "Fixing up Asus T100 keyb report descriptor\n");
>> rdesc[74] &= ~HID_MAIN_ITEM_CONSTANT;
>> }
>> --
>> 2.14.3
>>
^ permalink raw reply
* Re: [PATCH] Input: goodix - disable IRQs while suspended
From: Hans de Goede @ 2018-01-11 14:08 UTC (permalink / raw)
To: Bastien Nocera, Dmitry Torokhov, Benjamin Tissoires; +Cc: linux-input
In-Reply-To: <1515593698.2523.56.camel@hadess.net>
Hi,
On 10-01-18 15:14, Bastien Nocera wrote:
> On Wed, 2018-01-10 at 12:34 +0100, Hans de Goede wrote:
>> We should not try to do any i2c transfers before the controller is
>> resumed (which happens before our resume method gets called).
>>
>> So we need to disable our IRQ while suspended to enforce this. The
>> code paths for devices with GPIOs for the int and reset pins already
>> disable the IRQ the through goodix_free_irq().
>>
>> This commit also disables the IRQ while suspended for devices without
>> GPIOs for the int and reset pins.
>
> Do disable_irq() and enable_irq() require to be paired,
Yes.
> and if so, are
> goodix_suspend() and goodix_resume() guaranteed to be paired?
Yes I believe they are, many i2c drivers call disable / enable_irq
from there suspend/resume handler.
> Other than that, looks good to me. Please add the name of the device(s)
> you tested this on in the changelog and add your tested-by, to show
> it's not a "compile-only check" kind of commit.
Ok, v2 with amended commit messages coming up.
Regards,
Hans
>
> Cheers
>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/input/touchscreen/goodix.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/goodix.c
>> b/drivers/input/touchscreen/goodix.c
>> index 69d0b8cbc71f..ecec8eb17f28 100644
>> --- a/drivers/input/touchscreen/goodix.c
>> +++ b/drivers/input/touchscreen/goodix.c
>> @@ -878,8 +878,10 @@ static int __maybe_unused goodix_suspend(struct
>> device *dev)
>> int error;
>>
>> /* We need gpio pins to suspend/resume */
>> - if (!ts->gpiod_int || !ts->gpiod_rst)
>> + if (!ts->gpiod_int || !ts->gpiod_rst) {
>> + disable_irq(client->irq);
>> return 0;
>> + }
>>
>> wait_for_completion(&ts->firmware_loading_complete);
>>
>> @@ -919,8 +921,10 @@ static int __maybe_unused goodix_resume(struct
>> device *dev)
>> struct goodix_ts_data *ts = i2c_get_clientdata(client);
>> int error;
>>
>> - if (!ts->gpiod_int || !ts->gpiod_rst)
>> + if (!ts->gpiod_int || !ts->gpiod_rst) {
>> + enable_irq(client->irq);
>> return 0;
>> + }
>>
>> /*
>> * Exit sleep mode by outputting HIGH level to INT pin
^ permalink raw reply
* Re: [RESEND PATCH v2] input: pxrc: new driver for PhoenixRC Flight Controller Adapter
From: Marcus Folkesson @ 2018-01-11 12:07 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jonathan Corbet, Tomohiro Yoshidomi, David Herrmann,
Philippe Ombredanne, Kate Stewart, Greg Kroah-Hartman,
linux-input, linux-doc, linux-kernel
In-Reply-To: <20180111003714.34kgsbdtaqjwuyis@dtor-ws>
Hi Dmitry,
Thank you for your review!
On Wed, Jan 10, 2018 at 04:37:14PM -0800, Dmitry Torokhov wrote:
> Hi Marcus,
>
> On Wed, Jan 10, 2018 at 02:11:39PM +0100, Marcus Folkesson wrote:
> > This driver let you plug in your RC controller to the adapter and
> > use it as input device in various RC simulators.
> >
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > ---
> > v2:
> > - Change module license to GPLv2 to match SPDX tag
> >
> > Documentation/input/devices/pxrc.rst | 57 ++++++++
> > drivers/input/joystick/Kconfig | 9 ++
> > drivers/input/joystick/Makefile | 1 +
> > drivers/input/joystick/pxrc.c | 254 +++++++++++++++++++++++++++++++++++
> > 4 files changed, 321 insertions(+)
> > create mode 100644 Documentation/input/devices/pxrc.rst
> > create mode 100644 drivers/input/joystick/pxrc.c
> >
> > diff --git a/Documentation/input/devices/pxrc.rst b/Documentation/input/devices/pxrc.rst
> > new file mode 100644
> > index 000000000000..0ec466c58958
> > --- /dev/null
> > +++ b/Documentation/input/devices/pxrc.rst
> > @@ -0,0 +1,57 @@
> > +=======================================================
> > +pxrc - PhoenixRC Flight Controller Adapter
> > +=======================================================
> > +
> > +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > +
> > +This driver let you use your own RC controller plugged into the
> > +adapter that comes with PhoenixRC [1]_ or other compatible adapters.
> > +
> > +The adapter supports 7 analog channels and 1 digital input switch.
> > +
> > +Notes
> > +=====
> > +
> > +Many RC controllers is able to configure which stick goes to which channel.
> > +This is also configurable in most simulators, so a matching is not necessary.
> > +
> > +The driver is generating the following input event for analog channels:
> > +
> > ++---------+----------------+
> > +| Channel | Event |
> > ++=========+================+
> > +| 1 | ABS_X |
> > ++---------+----------------+
> > +| 2 | ABS_Y |
> > ++---------+----------------+
> > +| 3 | ABS_RX |
> > ++---------+----------------+
> > +| 4 | ABS_RY |
> > ++---------+----------------+
> > +| 5 | ABS_TILT_X |
> > ++---------+----------------+
> > +| 6 | ABS_TILT_Y |
> > ++---------+----------------+
>
> TILT are normally reserved for stylus/pen. Do we have better event codes
> maybe? Is there a picture of the RC so I can make more sense of the
> proposed event assignment?
>
Ok, maybe RUDDER and MISC?
The driver is actually for an "adapter" [1] that you connect your RC to.
The RC is then mapping its sticks/buttons to different channels. Unlike other
joysticks, this mapping is not fixed but something you setup in your RC.
For example, I'm using a Turnigy 9xr[2].
The RC is typically used with a RC Flight Simulator software (I'm using
Heli-X [3] when testing) which also map the channels to events (throttle,
rudder and so on).
> > +| 7 | ABS_THROTTLE |
> > ++---------+----------------+
> > +
> > +The digital input switch is generated as an `BTN_A` event.
> > +
> > +Manual Testing
> > +==============
> > +
> > +To test this driver's functionality you may use `input-event` which is part of
> > +the `input layer utilities` suite [2]_.
> > +
> > +For example::
> > +
> > + > modprobe pxrc
> > + > input-events <devnr>
> > +
> > +To print all input events from input `devnr`.
> > +
> > +References
> > +==========
> > +
> > +.. [1] http://www.phoenix-sim.com/
> > +.. [2] https://www.kraxel.org/cgit/input/
> > diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> > index f3c2f6ea8b44..18ab6dafff41 100644
> > --- a/drivers/input/joystick/Kconfig
> > +++ b/drivers/input/joystick/Kconfig
> > @@ -351,4 +351,13 @@ config JOYSTICK_PSXPAD_SPI_FF
> >
> > To drive rumble motor a dedicated power supply is required.
> >
> > +config JOYSTICK_PXRC
> > + tristate "PhoenixRC Flight Controller Adapter"
> > + depends on USB_ARCH_HAS_HCD
> > + select USB
> > + help
> > + Say Y here if you want to use the PhoenixRC Flight Controller Adapter.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called pxrc.
> > endif
> > diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> > index 67651efda2e1..dd0492ebbed7 100644
> > --- a/drivers/input/joystick/Makefile
> > +++ b/drivers/input/joystick/Makefile
> > @@ -23,6 +23,7 @@ obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
> > obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
> > obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
> > obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> > +obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> > obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> > obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> > obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
> > diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> > new file mode 100644
> > index 000000000000..2bec99df97e2
> > --- /dev/null
> > +++ b/drivers/input/joystick/pxrc.c
> > @@ -0,0 +1,254 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for Phoenix RC Flight Controller Adapter
> > + *
> > + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> > + *
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/errno.h>
> > +#include <linux/slab.h>
> > +#include <linux/module.h>
> > +#include <linux/kref.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/usb.h>
> > +#include <linux/mutex.h>
> > +#include <linux/input.h>
> > +
> > +#define PXRC_VENDOR_ID (0x1781)
> > +#define PXRC_PRODUCT_ID (0x0898)
> > +
> > +static const struct usb_device_id pxrc_table[] = {
> > + { USB_DEVICE(PXRC_VENDOR_ID, PXRC_PRODUCT_ID) },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(usb, pxrc_table);
> > +
> > +struct usb_pxrc {
> > + struct input_dev *input_dev;
> > + struct usb_device *udev;
> > + struct usb_interface *interface;
> > + struct usb_anchor anchor;
> > + __u8 epaddr;
> > + char phys[64];
> > + unsigned char *data;
> > + size_t bsize;
> > + struct kref kref;
> > +};
> > +
> > +#define to_pxrc_dev(d) container_of(d, struct usb_pxrc, kref)
> > +static void pxrc_delete(struct kref *kref)
> > +{
> > + struct usb_pxrc *pxrc = to_pxrc_dev(kref);
> > +
> > + usb_put_dev(pxrc->udev);
> > +}
> > +
> > +static void pxrc_usb_irq(struct urb *urb)
> > +{
> > + struct usb_pxrc *pxrc = urb->context;
> > +
> > + switch (urb->status) {
> > + case 0:
> > + /* success */
> > + break;
> > + case -ETIME:
> > + /* this urb is timing out */
> > + dev_dbg(&pxrc->interface->dev,
> > + "%s - urb timed out - was the device unplugged?\n",
> > + __func__);
> > + return;
> > + case -ECONNRESET:
> > + case -ENOENT:
> > + case -ESHUTDOWN:
> > + case -EPIPE:
> > + /* this urb is terminated, clean up */
> > + dev_dbg(&pxrc->interface->dev, "%s - urb shutting down with status: %d\n",
> > + __func__, urb->status);
> > + return;
> > + default:
> > + dev_dbg(&pxrc->interface->dev, "%s - nonzero urb status received: %d\n",
> > + __func__, urb->status);
> > + goto exit;
> > + }
> > +
> > + if (urb->actual_length == 8) {
> > + input_report_abs(pxrc->input_dev, ABS_X, pxrc->data[0]);
> > + input_report_abs(pxrc->input_dev, ABS_Y, pxrc->data[2]);
> > + input_report_abs(pxrc->input_dev, ABS_RX, pxrc->data[3]);
> > + input_report_abs(pxrc->input_dev, ABS_RY, pxrc->data[4]);
> > + input_report_abs(pxrc->input_dev, ABS_TILT_X, pxrc->data[5]);
> > + input_report_abs(pxrc->input_dev, ABS_TILT_Y, pxrc->data[6]);
> > + input_report_abs(pxrc->input_dev, ABS_THROTTLE, pxrc->data[7]);
> > +
> > + input_report_key(pxrc->input_dev, BTN_A, pxrc->data[1]);
> > + }
> > +
> > +exit:
> > + /* Resubmit to fetch new fresh URBs */
> > + usb_anchor_urb(urb, &pxrc->anchor);
> > + if (usb_submit_urb(urb, GFP_ATOMIC) < 0)
> > + usb_unanchor_urb(urb);
> > +}
> > +
> > +static int pxrc_submit_intr_urb(struct usb_pxrc *pxrc)
> > +{
> > + struct urb *urb;
> > + unsigned int pipe;
> > + int err;
> > +
> > + urb = usb_alloc_urb(0, GFP_KERNEL);
> > + if (!urb)
> > + return -ENOMEM;
> > +
> > + pipe = usb_rcvintpipe(pxrc->udev, pxrc->epaddr),
> > + usb_fill_int_urb(urb, pxrc->udev, pipe, pxrc->data, pxrc->bsize,
> > + pxrc_usb_irq, pxrc, 1);
> > + usb_anchor_urb(urb, &pxrc->anchor);
> > + err = usb_submit_urb(urb, GFP_KERNEL);
> > + if (err < 0)
> > + usb_unanchor_urb(urb);
> > +
> > + usb_free_urb(urb);
> > + return err;
> > +}
> > +
> > +static int pxrc_open(struct input_dev *input)
> > +{
> > + struct usb_pxrc *pxrc = input_get_drvdata(input);
> > + int err;
> > +
> > + err = pxrc_submit_intr_urb(pxrc);
> > + if (err < 0)
> > + goto error;
> > +
> > + kref_get(&pxrc->kref);
> > + return 0;
> > +
> > +error:
> > + usb_kill_anchored_urbs(&pxrc->anchor);
> > + return err;
> > +}
> > +
> > +static void pxrc_close(struct input_dev *input)
> > +{
> > + struct usb_pxrc *pxrc = input_get_drvdata(input);
> > +
> > + usb_kill_anchored_urbs(&pxrc->anchor);
> > + kref_put(&pxrc->kref, pxrc_delete);
>
> This is way to complex and I am not sure why you need to anchor URBs and
> have a separate refcounting, etc. I do not think it is even safe with
> module unloading as kref might be in flight as you run through module
> unload and devm will destroy most of the objects.
>
> Why don't you simply use usb_kill_urb() here and be done.
> input_unregister_device() that is called as part of devm unwind on
> device unbind will make sure that pxrc_close() is called.
>
> Thanks.
>
Ok, I drop the refcounting. pxrc_delete() did more before I changed most
allocations to devm_*.
I tried to get the refcounting similiar to /drivers/usb/usb-skeleton.c.
I have tried different approaches with the create/delete URBs.
At first I had the URB in the usb_pxrc struct that I allocated in the
probe function where I also submitted the URB.
This worked but I found it waste to submit URBs when noone has opened the
device, so I moved the submitting to pxrc_open().
The problem was that the same URB was submitted multiple times (gives a
warning) upon multiple calls to open(2).
So I removed the URB from usb_pxrc and used an anchor to still have a
reference to it for deletion. This part is heavily inspired by
drivers/bluetooth/bpa10x.c.
Maybe I should do it differently?
I must admit that I'm not very familiar with USB drivers.
> > +}
> > +
> > +static int pxrc_input_init(struct usb_pxrc *pxrc)
> > +{
> > + pxrc->input_dev = devm_input_allocate_device(&pxrc->interface->dev);
> > + if (pxrc->input_dev == NULL) {
> > + dev_err(&pxrc->interface->dev, "couldn't allocate input device\n");
> > + return -ENOMEM;
> > + }
> > +
> > + pxrc->input_dev->name = "PXRC Flight Controller Adapter";
> > + pxrc->input_dev->phys = pxrc->phys;
> > + pxrc->input_dev->id.bustype = BUS_USB;
> > + pxrc->input_dev->id.vendor = PXRC_VENDOR_ID;
> > + pxrc->input_dev->id.product = PXRC_PRODUCT_ID;
> > + pxrc->input_dev->id.version = 0x01;
>
> usb_to_input_id(udev, &pxrc->input_dev->id);
>
Will do, thanks.
> > +
> > + pxrc->input_dev->open = pxrc_open;
> > + pxrc->input_dev->close = pxrc_close;
> > +
> > + pxrc->input_dev->evbit[0] = BIT_MASK(EV_ABS) | BIT_MASK(EV_KEY);
> > + pxrc->input_dev->absbit[0] = BIT_MASK(ABS_X) |
> > + BIT_MASK(ABS_Y) |
> > + BIT_MASK(ABS_RY) |
> > + BIT_MASK(ABS_RX) |
> > + BIT_MASK(ABS_THROTTLE) |
> > + BIT_MASK(ABS_TILT_X) |
> > + BIT_MASK(ABS_TILT_Y);
>
> Not needed, it will be done as part of input_set_abs_params() below.
>
Ok
> > +
> > + pxrc->input_dev->keybit[BIT_WORD(BTN_JOYSTICK)] = BIT_MASK(BTN_A);
>
> input_set_capability(pxrc->input_dev, EV_KEY, BTN_A);
>
Ok
> > +
> > + input_set_abs_params(pxrc->input_dev, ABS_X, 0, 255, 0, 0);
> > + input_set_abs_params(pxrc->input_dev, ABS_Y, 0, 255, 0, 0);
> > + input_set_abs_params(pxrc->input_dev, ABS_RX, 0, 255, 0, 0);
> > + input_set_abs_params(pxrc->input_dev, ABS_RY, 0, 255, 0, 0);
> > + input_set_abs_params(pxrc->input_dev, ABS_TILT_X, 0, 255, 0, 0);
> > + input_set_abs_params(pxrc->input_dev, ABS_TILT_Y, 0, 255, 0, 0);
> > + input_set_abs_params(pxrc->input_dev, ABS_THROTTLE, 0, 255, 0, 0);
> > +
> > + input_set_drvdata(pxrc->input_dev, pxrc);
> > +
> > + return input_register_device(pxrc->input_dev);
> > +}
> > +
> > +static int pxrc_probe(struct usb_interface *interface,
> > + const struct usb_device_id *id)
> > +{
> > + struct usb_pxrc *pxrc;
> > + struct usb_endpoint_descriptor *epirq;
> > + int retval;
> > +
> > + pxrc = devm_kzalloc(&interface->dev, sizeof(*pxrc), GFP_KERNEL);
> > +
> > + if (!pxrc)
> > + return -ENOMEM;
> > +
> > + kref_init(&pxrc->kref);
> > + init_usb_anchor(&pxrc->anchor);
> > +
> > + pxrc->udev = usb_get_dev(interface_to_usbdev(interface));
> > + pxrc->interface = interface;
> > +
> > + /* Set up the endpoint information */
> > + /* This device only has an interrupt endpoint */
> > + retval = usb_find_common_endpoints(interface->cur_altsetting,
> > + NULL, NULL, &epirq, NULL);
> > + if (retval) {
> > + dev_err(&interface->dev,
> > + "Could not find endpoint\n");
> > + goto error;
> > + }
> > +
> > + pxrc->bsize = usb_endpoint_maxp(epirq);
> > + pxrc->epaddr = epirq->bEndpointAddress;
> > + pxrc->data = devm_kmalloc(&interface->dev, pxrc->bsize, GFP_KERNEL);
> > + if (!pxrc->data) {
> > + retval = -ENOMEM;
> > + goto error;
> > + }
> > +
> > + usb_set_intfdata(interface, pxrc);
> > + usb_make_path(pxrc->udev, pxrc->phys, sizeof(pxrc->phys));
> > +
> > + retval = pxrc_input_init(pxrc);
> > + if (retval)
> > + goto error;
> > +
> > + return 0;
> > +
> > +error:
> > + /* this frees allocated memory */
> > + kref_put(&pxrc->kref, pxrc_delete);
> > +
> > + return retval;
> > +}
> > +
> > +static void pxrc_disconnect(struct usb_interface *interface)
> > +{
> > + struct usb_pxrc *pxrc = usb_get_intfdata(interface);
> > +
> > + kref_put(&pxrc->kref, pxrc_delete);
> > +}
> > +
> > +static struct usb_driver pxrc_driver = {
> > + .name = "pxrc",
> > + .probe = pxrc_probe,
> > + .disconnect = pxrc_disconnect,
> > + .id_table = pxrc_table,
> > +};
> > +
> > +module_usb_driver(pxrc_driver);
> > +
> > +MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
> > +MODULE_DESCRIPTION("PhoenixRC Flight Controller Adapter");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.15.1
> >
>
> Thanks.
>
> --
> Dmitry
Best Regards
Marcus Folkesson
[1] https://images.okr.ro/auctions.v3/400_400/2010/09/24/2/e/630212971498401813252942-8740-400_400.jpg
[2] https://hobbyking.com/media/catalog/product/cache/1/image/9df78eab33525d08d6e5fb8d27136e95/legacy/catalog/9xr_ft.jpg
[3] http://www.heli-x.info/cms/[3] http://www.heli-x.info/cms/[3]
http://www.heli-x.info/cms/
^ permalink raw reply
* Re: [RESEND PATCH v2] input: pxrc: new driver for PhoenixRC Flight Controller Adapter
From: Dmitry Torokhov @ 2018-01-11 0:37 UTC (permalink / raw)
To: Marcus Folkesson
Cc: Jonathan Corbet, Tomohiro Yoshidomi, David Herrmann,
Philippe Ombredanne, Kate Stewart, Greg Kroah-Hartman,
linux-input, linux-doc, linux-kernel
In-Reply-To: <20180110131139.7238-1-marcus.folkesson@gmail.com>
Hi Marcus,
On Wed, Jan 10, 2018 at 02:11:39PM +0100, Marcus Folkesson wrote:
> This driver let you plug in your RC controller to the adapter and
> use it as input device in various RC simulators.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
> v2:
> - Change module license to GPLv2 to match SPDX tag
>
> Documentation/input/devices/pxrc.rst | 57 ++++++++
> drivers/input/joystick/Kconfig | 9 ++
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/pxrc.c | 254 +++++++++++++++++++++++++++++++++++
> 4 files changed, 321 insertions(+)
> create mode 100644 Documentation/input/devices/pxrc.rst
> create mode 100644 drivers/input/joystick/pxrc.c
>
> diff --git a/Documentation/input/devices/pxrc.rst b/Documentation/input/devices/pxrc.rst
> new file mode 100644
> index 000000000000..0ec466c58958
> --- /dev/null
> +++ b/Documentation/input/devices/pxrc.rst
> @@ -0,0 +1,57 @@
> +=======================================================
> +pxrc - PhoenixRC Flight Controller Adapter
> +=======================================================
> +
> +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> +
> +This driver let you use your own RC controller plugged into the
> +adapter that comes with PhoenixRC [1]_ or other compatible adapters.
> +
> +The adapter supports 7 analog channels and 1 digital input switch.
> +
> +Notes
> +=====
> +
> +Many RC controllers is able to configure which stick goes to which channel.
> +This is also configurable in most simulators, so a matching is not necessary.
> +
> +The driver is generating the following input event for analog channels:
> +
> ++---------+----------------+
> +| Channel | Event |
> ++=========+================+
> +| 1 | ABS_X |
> ++---------+----------------+
> +| 2 | ABS_Y |
> ++---------+----------------+
> +| 3 | ABS_RX |
> ++---------+----------------+
> +| 4 | ABS_RY |
> ++---------+----------------+
> +| 5 | ABS_TILT_X |
> ++---------+----------------+
> +| 6 | ABS_TILT_Y |
> ++---------+----------------+
TILT are normally reserved for stylus/pen. Do we have better event codes
maybe? Is there a picture of the RC so I can make more sense of the
proposed event assignment?
> +| 7 | ABS_THROTTLE |
> ++---------+----------------+
> +
> +The digital input switch is generated as an `BTN_A` event.
> +
> +Manual Testing
> +==============
> +
> +To test this driver's functionality you may use `input-event` which is part of
> +the `input layer utilities` suite [2]_.
> +
> +For example::
> +
> + > modprobe pxrc
> + > input-events <devnr>
> +
> +To print all input events from input `devnr`.
> +
> +References
> +==========
> +
> +.. [1] http://www.phoenix-sim.com/
> +.. [2] https://www.kraxel.org/cgit/input/
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index f3c2f6ea8b44..18ab6dafff41 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -351,4 +351,13 @@ config JOYSTICK_PSXPAD_SPI_FF
>
> To drive rumble motor a dedicated power supply is required.
>
> +config JOYSTICK_PXRC
> + tristate "PhoenixRC Flight Controller Adapter"
> + depends on USB_ARCH_HAS_HCD
> + select USB
> + help
> + Say Y here if you want to use the PhoenixRC Flight Controller Adapter.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called pxrc.
> endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 67651efda2e1..dd0492ebbed7 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
> obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
> obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> +obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
> diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> new file mode 100644
> index 000000000000..2bec99df97e2
> --- /dev/null
> +++ b/drivers/input/joystick/pxrc.c
> @@ -0,0 +1,254 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Phoenix RC Flight Controller Adapter
> + *
> + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/kref.h>
> +#include <linux/uaccess.h>
> +#include <linux/usb.h>
> +#include <linux/mutex.h>
> +#include <linux/input.h>
> +
> +#define PXRC_VENDOR_ID (0x1781)
> +#define PXRC_PRODUCT_ID (0x0898)
> +
> +static const struct usb_device_id pxrc_table[] = {
> + { USB_DEVICE(PXRC_VENDOR_ID, PXRC_PRODUCT_ID) },
> + { }
> +};
> +MODULE_DEVICE_TABLE(usb, pxrc_table);
> +
> +struct usb_pxrc {
> + struct input_dev *input_dev;
> + struct usb_device *udev;
> + struct usb_interface *interface;
> + struct usb_anchor anchor;
> + __u8 epaddr;
> + char phys[64];
> + unsigned char *data;
> + size_t bsize;
> + struct kref kref;
> +};
> +
> +#define to_pxrc_dev(d) container_of(d, struct usb_pxrc, kref)
> +static void pxrc_delete(struct kref *kref)
> +{
> + struct usb_pxrc *pxrc = to_pxrc_dev(kref);
> +
> + usb_put_dev(pxrc->udev);
> +}
> +
> +static void pxrc_usb_irq(struct urb *urb)
> +{
> + struct usb_pxrc *pxrc = urb->context;
> +
> + switch (urb->status) {
> + case 0:
> + /* success */
> + break;
> + case -ETIME:
> + /* this urb is timing out */
> + dev_dbg(&pxrc->interface->dev,
> + "%s - urb timed out - was the device unplugged?\n",
> + __func__);
> + return;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + case -EPIPE:
> + /* this urb is terminated, clean up */
> + dev_dbg(&pxrc->interface->dev, "%s - urb shutting down with status: %d\n",
> + __func__, urb->status);
> + return;
> + default:
> + dev_dbg(&pxrc->interface->dev, "%s - nonzero urb status received: %d\n",
> + __func__, urb->status);
> + goto exit;
> + }
> +
> + if (urb->actual_length == 8) {
> + input_report_abs(pxrc->input_dev, ABS_X, pxrc->data[0]);
> + input_report_abs(pxrc->input_dev, ABS_Y, pxrc->data[2]);
> + input_report_abs(pxrc->input_dev, ABS_RX, pxrc->data[3]);
> + input_report_abs(pxrc->input_dev, ABS_RY, pxrc->data[4]);
> + input_report_abs(pxrc->input_dev, ABS_TILT_X, pxrc->data[5]);
> + input_report_abs(pxrc->input_dev, ABS_TILT_Y, pxrc->data[6]);
> + input_report_abs(pxrc->input_dev, ABS_THROTTLE, pxrc->data[7]);
> +
> + input_report_key(pxrc->input_dev, BTN_A, pxrc->data[1]);
> + }
> +
> +exit:
> + /* Resubmit to fetch new fresh URBs */
> + usb_anchor_urb(urb, &pxrc->anchor);
> + if (usb_submit_urb(urb, GFP_ATOMIC) < 0)
> + usb_unanchor_urb(urb);
> +}
> +
> +static int pxrc_submit_intr_urb(struct usb_pxrc *pxrc)
> +{
> + struct urb *urb;
> + unsigned int pipe;
> + int err;
> +
> + urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!urb)
> + return -ENOMEM;
> +
> + pipe = usb_rcvintpipe(pxrc->udev, pxrc->epaddr),
> + usb_fill_int_urb(urb, pxrc->udev, pipe, pxrc->data, pxrc->bsize,
> + pxrc_usb_irq, pxrc, 1);
> + usb_anchor_urb(urb, &pxrc->anchor);
> + err = usb_submit_urb(urb, GFP_KERNEL);
> + if (err < 0)
> + usb_unanchor_urb(urb);
> +
> + usb_free_urb(urb);
> + return err;
> +}
> +
> +static int pxrc_open(struct input_dev *input)
> +{
> + struct usb_pxrc *pxrc = input_get_drvdata(input);
> + int err;
> +
> + err = pxrc_submit_intr_urb(pxrc);
> + if (err < 0)
> + goto error;
> +
> + kref_get(&pxrc->kref);
> + return 0;
> +
> +error:
> + usb_kill_anchored_urbs(&pxrc->anchor);
> + return err;
> +}
> +
> +static void pxrc_close(struct input_dev *input)
> +{
> + struct usb_pxrc *pxrc = input_get_drvdata(input);
> +
> + usb_kill_anchored_urbs(&pxrc->anchor);
> + kref_put(&pxrc->kref, pxrc_delete);
This is way to complex and I am not sure why you need to anchor URBs and
have a separate refcounting, etc. I do not think it is even safe with
module unloading as kref might be in flight as you run through module
unload and devm will destroy most of the objects.
Why don't you simply use usb_kill_urb() here and be done.
input_unregister_device() that is called as part of devm unwind on
device unbind will make sure that pxrc_close() is called.
Thanks.
> +}
> +
> +static int pxrc_input_init(struct usb_pxrc *pxrc)
> +{
> + pxrc->input_dev = devm_input_allocate_device(&pxrc->interface->dev);
> + if (pxrc->input_dev == NULL) {
> + dev_err(&pxrc->interface->dev, "couldn't allocate input device\n");
> + return -ENOMEM;
> + }
> +
> + pxrc->input_dev->name = "PXRC Flight Controller Adapter";
> + pxrc->input_dev->phys = pxrc->phys;
> + pxrc->input_dev->id.bustype = BUS_USB;
> + pxrc->input_dev->id.vendor = PXRC_VENDOR_ID;
> + pxrc->input_dev->id.product = PXRC_PRODUCT_ID;
> + pxrc->input_dev->id.version = 0x01;
usb_to_input_id(udev, &pxrc->input_dev->id);
> +
> + pxrc->input_dev->open = pxrc_open;
> + pxrc->input_dev->close = pxrc_close;
> +
> + pxrc->input_dev->evbit[0] = BIT_MASK(EV_ABS) | BIT_MASK(EV_KEY);
> + pxrc->input_dev->absbit[0] = BIT_MASK(ABS_X) |
> + BIT_MASK(ABS_Y) |
> + BIT_MASK(ABS_RY) |
> + BIT_MASK(ABS_RX) |
> + BIT_MASK(ABS_THROTTLE) |
> + BIT_MASK(ABS_TILT_X) |
> + BIT_MASK(ABS_TILT_Y);
Not needed, it will be done as part of input_set_abs_params() below.
> +
> + pxrc->input_dev->keybit[BIT_WORD(BTN_JOYSTICK)] = BIT_MASK(BTN_A);
input_set_capability(pxrc->input_dev, EV_KEY, BTN_A);
> +
> + input_set_abs_params(pxrc->input_dev, ABS_X, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input_dev, ABS_Y, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input_dev, ABS_RX, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input_dev, ABS_RY, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input_dev, ABS_TILT_X, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input_dev, ABS_TILT_Y, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input_dev, ABS_THROTTLE, 0, 255, 0, 0);
> +
> + input_set_drvdata(pxrc->input_dev, pxrc);
> +
> + return input_register_device(pxrc->input_dev);
> +}
> +
> +static int pxrc_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct usb_pxrc *pxrc;
> + struct usb_endpoint_descriptor *epirq;
> + int retval;
> +
> + pxrc = devm_kzalloc(&interface->dev, sizeof(*pxrc), GFP_KERNEL);
> +
> + if (!pxrc)
> + return -ENOMEM;
> +
> + kref_init(&pxrc->kref);
> + init_usb_anchor(&pxrc->anchor);
> +
> + pxrc->udev = usb_get_dev(interface_to_usbdev(interface));
> + pxrc->interface = interface;
> +
> + /* Set up the endpoint information */
> + /* This device only has an interrupt endpoint */
> + retval = usb_find_common_endpoints(interface->cur_altsetting,
> + NULL, NULL, &epirq, NULL);
> + if (retval) {
> + dev_err(&interface->dev,
> + "Could not find endpoint\n");
> + goto error;
> + }
> +
> + pxrc->bsize = usb_endpoint_maxp(epirq);
> + pxrc->epaddr = epirq->bEndpointAddress;
> + pxrc->data = devm_kmalloc(&interface->dev, pxrc->bsize, GFP_KERNEL);
> + if (!pxrc->data) {
> + retval = -ENOMEM;
> + goto error;
> + }
> +
> + usb_set_intfdata(interface, pxrc);
> + usb_make_path(pxrc->udev, pxrc->phys, sizeof(pxrc->phys));
> +
> + retval = pxrc_input_init(pxrc);
> + if (retval)
> + goto error;
> +
> + return 0;
> +
> +error:
> + /* this frees allocated memory */
> + kref_put(&pxrc->kref, pxrc_delete);
> +
> + return retval;
> +}
> +
> +static void pxrc_disconnect(struct usb_interface *interface)
> +{
> + struct usb_pxrc *pxrc = usb_get_intfdata(interface);
> +
> + kref_put(&pxrc->kref, pxrc_delete);
> +}
> +
> +static struct usb_driver pxrc_driver = {
> + .name = "pxrc",
> + .probe = pxrc_probe,
> + .disconnect = pxrc_disconnect,
> + .id_table = pxrc_table,
> +};
> +
> +module_usb_driver(pxrc_driver);
> +
> +MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
> +MODULE_DESCRIPTION("PhoenixRC Flight Controller Adapter");
> +MODULE_LICENSE("GPL v2");
> --
> 2.15.1
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: snvs_pwrkey: add clock support
From: Dmitry Torokhov @ 2018-01-11 0:23 UTC (permalink / raw)
To: Anson Huang; +Cc: keescook, linux-input, linux-kernel
In-Reply-To: <1515575422-8843-1-git-send-email-Anson.Huang@nxp.com>
Hi Anson,
On Wed, Jan 10, 2018 at 05:10:22PM +0800, Anson Huang wrote:
> Add optional clock enable/disable support for the SNVS pwrkey,
> which is required for accessing SNVS block on some i.MX SoCs
> like i.MX7D.
>
> If SNVS clock is required, it needs to be passed from dtb file
> and SNVS pwrkey driver will enable it.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> drivers/input/keyboard/snvs_pwrkey.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 53c768b..51642aa 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -21,6 +21,7 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_address.h>
> +#include <linux/clk.h>
> #include <linux/platform_device.h>
> #include <linux/mfd/syscon.h>
> #include <linux/regmap.h>
> @@ -43,6 +44,7 @@ struct pwrkey_drv_data {
> int wakeup;
> struct timer_list check_timer;
> struct input_dev *input;
> + struct clk *clk;
> };
>
> static void imx_imx_snvs_check_for_events(struct timer_list *t)
> @@ -129,6 +131,18 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> + pdata->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(pdata->clk)) {
> + pdata->clk = NULL;
This is not right, as you do not handle the deferrals properly, not
differentiate between missing clock and other fatal errors.
Also, it looks like we need something like devm_clk_get_optional() for
your use case. Have you looked into adding such API?
> + } else {
> + error = clk_prepare_enable(pdata->clk);
> + if (error) {
> + dev_err(&pdev->dev,
> + "Could not prepare or enable the snvs clock\n");
> + return error;
> + }
> + }
> +
> regmap_update_bits(pdata->snvs, SNVS_LPCR_REG, SNVS_LPCR_DEP_EN, SNVS_LPCR_DEP_EN);
>
> /* clear the unexpected interrupt before driver ready */
> @@ -139,7 +153,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> input = devm_input_allocate_device(&pdev->dev);
> if (!input) {
> dev_err(&pdev->dev, "failed to allocate the input device\n");
> - return -ENOMEM;
> + error = -ENOMEM;
> + goto error_snvs_pwrkey_device_register;
Instead of mixing automatic and manual resource release please install a
custom devm action disabling the clock. This will also ensure that we
disable the clock when unbinding device.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: of_touchscreen: add MODULE_LICENSE
From: Dmitry Torokhov @ 2018-01-10 19:30 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-input, linux-kernel
In-Reply-To: <20180110163137.543730-1-arnd@arndb.de>
On Wed, Jan 10, 2018 at 05:31:26PM +0100, Arnd Bergmann wrote:
> The lack of the MODULE_LICENSE tag can lead to a warning here:
>
> WARNING: modpost: missing MODULE_LICENSE() in drivers/input/touchscreen/of_touchscreen.o
>
> I'm adding a license and description tag, but no MODULE_AUTHOR()
> as this file is a collection of standalone helper functions that
> were all added by different developers.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied, thank you.
> ---
> drivers/input/touchscreen/of_touchscreen.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> index 8d7f9c8f2771..9642f103b726 100644
> --- a/drivers/input/touchscreen/of_touchscreen.c
> +++ b/drivers/input/touchscreen/of_touchscreen.c
> @@ -13,6 +13,7 @@
> #include <linux/input.h>
> #include <linux/input/mt.h>
> #include <linux/input/touchscreen.h>
> +#include <linux/module.h>
>
> static bool touchscreen_get_prop_u32(struct device *dev,
> const char *property,
> @@ -185,3 +186,6 @@ void touchscreen_report_pos(struct input_dev *input,
> input_report_abs(input, multitouch ? ABS_MT_POSITION_Y : ABS_Y, y);
> }
> EXPORT_SYMBOL(touchscreen_report_pos);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Device-tree helpers functions for touchscreen devices");
> --
> 2.9.0
>
--
Dmitry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox