* Re: [PATCH 0/4] HID: implement new transport-driver callbacks
From: David Herrmann @ 2014-01-29 8:37 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Frank Praznik, open list:HID CORE LAYER, Benjamin Tissoires
In-Reply-To: <alpine.LNX.2.00.1401282153141.22492@pobox.suse.cz>
Hi
On Tue, Jan 28, 2014 at 9:53 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 22 Jan 2014, David Herrmann wrote:
>
>> > These patches are originally the work of David Herrmann who suggested that I
>> > update and submit them as their functionality is required for some pending
>> > patches to the hid-sony driver.
>> >
>> > These patches implement the SET/GET_REPORT and raw intr OUTPUT requests for all
>> > transport drivers. It adds two callbacks to the hid_ll_driver struct:
>> >
>> > int (*raw_request)(struct hid_device *hdev, unsigned char reportnum,
>> > __u8 *buf, size_t len, unsigned char rtype, int reqtype);
>> >
>> > int (*output_report)(struct hid_device *hdev, __u8 *buf, size_t len);
>> >
>> > along with the necessary support fuctions in the USBHID, and HIDP drivers.
>> >
>> > UHID is not converted yet.
>>
>> Thanks for picking it up. As background, people should read my HID
>> summary which originally was part of this series:
>> http://cgit.freedesktop.org/~dvdhrm/linux/tree/Documentation/hid/hid-transport.txt?h=hid&id=86c08bb28302bb31dcd3b9aaf22b222f890397e0
>>
>> Our current hid_output_raw_report() callbacks are implemented
>> differently in the USBHID, HIDP, I2CHID and UHID backends (I even
>> think they're all mutually different). We cannot easily change these
>> as drivers actually depend on the backends to do it differently.
>> Therefore, I proposed the raw_request() and output_report() functions.
>> raw_request() is basically the same as request() but takes a raw
>> buffer instead of an hid_report. output_report() is what HIDP
>> currently does with hid_output_raw_report() and sends the report as
>> asynchronous intr report.
>>
>> The plan should be to use request(), raw_request() and output_report()
>> exclusively and carefully port drivers to use them. Once we're done,
>> we can remove hid_output_raw_report() (and any other legacy). This
>> should guarantee, that drivers can choose between ctrl-SET_REPORT and
>> intr-OUTPUT_REPORT messages without depending on the underlying
>> backend to choose the right one.
>
> I haven't finished reviewing the patchset yet, but anyway -- David, can I
> consider your e-mail as a potential Acked-by:?
Yes, definitely. It's basically my 1-year old patch split into 3.
Thanks
David
^ permalink raw reply
* Re: [PATCH] Bluetooth: hidp: make sure input buffers are big enough
From: David Herrmann @ 2014-01-29 8:36 UTC (permalink / raw)
To: Jiri Kosina
Cc: Marcel Holtmann, open list:HID CORE LAYER,
linux-bluetooth@vger.kernel.org development, Gustavo F. Padovan
In-Reply-To: <alpine.LNX.2.00.1401282152140.22492@pobox.suse.cz>
Hi
On Tue, Jan 28, 2014 at 9:53 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Tue, 7 Jan 2014, Jiri Kosina wrote:
>
>> > > So doing kzalloc(rsize, GFP_ATOMIC) in the HID-core for now, and copying
>> > > the buffer around, seems like only viable solution for now, with the
>> > > outlook of removing this ugliness once hid-core honors 'size' properly.
>> >
>> > Should I resend the patches and move it to hid_input_report() for now?
>> > Or are you getting something in yourself?
>>
>> Due to various reasons I will not have access to any testing HW for the
>> upcoming 2-3 days, so if you can cook something up in that timeframe, it'd
>> be appreciated.
>>
>> Otherwise I'll be working on it by the end of this week.
>
> David,
>
> just got back to this, finally ... did you have time to work on this at
> all, or should I just start from scratch?
Sorry, no. Fosdem is this weekend and I needed to get my code ready
for that. But I'll finally have time again next week.
Thanks
David
^ permalink raw reply
* Re: [PATCH] module: allow multiple calls to MODULE_DEVICE_TABLE() per module
From: Rusty Russell @ 2014-01-29 1:35 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-input, kay, Tom Gundersen, Dmitry Torokhov
In-Reply-To: <1390916546-27155-1-git-send-email-teg@jklm.no>
Tom Gundersen <teg@jklm.no> writes:
> Commit 78551277e4df5: "Input: i8042 - add PNP modaliases" had a bug, where the
> second call to MODULE_DEVICE_TABLE() overrode the first resulting in not all
> the modaliases being exposed.
>
> This fixes the problem by including the name of the device_id table in the
> __mod_*_device_table alias, allowing us to export several device_id tables
> per module.
Thanks, applied.
I also put this on top:
module: remove MODULE_GENERIC_TABLE
MODULE_DEVICE_TABLE() calles MODULE_GENERIC_TABLE(); make it do the
work directly. This also removes a wart introduced in the last patch,
where the alias is defined to be an unknown struct type "struct
type##__##name##_device_id" instead of "struct type##_device_id" (it's
an extern so GCC doesn't care, but it's wrong).
The other user of MODULE_GENERIC_TABLE (ISAPNP_CARD_TABLE) is unused,
so delete it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
diff --git a/include/linux/isapnp.h b/include/linux/isapnp.h
index e2d28b026a8c..3c77bf9b1efd 100644
--- a/include/linux/isapnp.h
+++ b/include/linux/isapnp.h
@@ -56,10 +56,6 @@
#define ISAPNP_DEVICE_ID(_va, _vb, _vc, _function) \
{ .vendor = ISAPNP_VENDOR(_va, _vb, _vc), .function = ISAPNP_FUNCTION(_function) }
-/* export used IDs outside module */
-#define ISAPNP_CARD_TABLE(name) \
- MODULE_GENERIC_TABLE(isapnp_card, name)
-
struct isapnp_card_id {
unsigned long driver_data; /* data private to the driver */
unsigned short card_vendor, card_device;
diff --git a/include/linux/module.h b/include/linux/module.h
index ad18f6006f86..5686b37e11d6 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -82,15 +82,6 @@ void sort_extable(struct exception_table_entry *start,
void sort_main_extable(void);
void trim_init_extable(struct module *m);
-#ifdef MODULE
-#define MODULE_GENERIC_TABLE(gtype, name) \
-extern const struct gtype##_id __mod_##gtype##_table \
- __attribute__ ((unused, alias(__stringify(name))))
-
-#else /* !MODULE */
-#define MODULE_GENERIC_TABLE(gtype, name)
-#endif
-
/* Generic info of form tag = "info" */
#define MODULE_INFO(tag, info) __MODULE_INFO(tag, tag, info)
@@ -141,8 +132,14 @@ extern const struct gtype##_id __mod_##gtype##_table \
/* What your module does. */
#define MODULE_DESCRIPTION(_description) MODULE_INFO(description, _description)
-#define MODULE_DEVICE_TABLE(type, name) \
- MODULE_GENERIC_TABLE(type##__##name##_device, name)
+#ifdef MODULE
+/* Creates an alias so file2alias.c can find device table. */
+#define MODULE_DEVICE_TABLE(type, name) \
+ extern const struct type##_device_id __mod_##type##__##name##_device_table \
+ __attribute__ ((unused, alias(__stringify(name))))
+#else /* !MODULE */
+#define MODULE_DEVICE_TABLE(type, name)
+#endif
/* Version of form [<epoch>:]<version>[-<extra-version>].
* Or for CVS/RCS ID version, everything but the number is stripped.
^ permalink raw reply related
* Re: [PATCH 0/4] HID: implement new transport-driver callbacks
From: Jiri Kosina @ 2014-01-28 20:53 UTC (permalink / raw)
To: David Herrmann
Cc: Frank Praznik, open list:HID CORE LAYER, Benjamin Tissoires
In-Reply-To: <CANq1E4RHBpbnQ6bGtT-uv7LYmOCw=khACsNWCyoqpKCPXrJJrQ@mail.gmail.com>
On Wed, 22 Jan 2014, David Herrmann wrote:
> > These patches are originally the work of David Herrmann who suggested that I
> > update and submit them as their functionality is required for some pending
> > patches to the hid-sony driver.
> >
> > These patches implement the SET/GET_REPORT and raw intr OUTPUT requests for all
> > transport drivers. It adds two callbacks to the hid_ll_driver struct:
> >
> > int (*raw_request)(struct hid_device *hdev, unsigned char reportnum,
> > __u8 *buf, size_t len, unsigned char rtype, int reqtype);
> >
> > int (*output_report)(struct hid_device *hdev, __u8 *buf, size_t len);
> >
> > along with the necessary support fuctions in the USBHID, and HIDP drivers.
> >
> > UHID is not converted yet.
>
> Thanks for picking it up. As background, people should read my HID
> summary which originally was part of this series:
> http://cgit.freedesktop.org/~dvdhrm/linux/tree/Documentation/hid/hid-transport.txt?h=hid&id=86c08bb28302bb31dcd3b9aaf22b222f890397e0
>
> Our current hid_output_raw_report() callbacks are implemented
> differently in the USBHID, HIDP, I2CHID and UHID backends (I even
> think they're all mutually different). We cannot easily change these
> as drivers actually depend on the backends to do it differently.
> Therefore, I proposed the raw_request() and output_report() functions.
> raw_request() is basically the same as request() but takes a raw
> buffer instead of an hid_report. output_report() is what HIDP
> currently does with hid_output_raw_report() and sends the report as
> asynchronous intr report.
>
> The plan should be to use request(), raw_request() and output_report()
> exclusively and carefully port drivers to use them. Once we're done,
> we can remove hid_output_raw_report() (and any other legacy). This
> should guarantee, that drivers can choose between ctrl-SET_REPORT and
> intr-OUTPUT_REPORT messages without depending on the underlying
> backend to choose the right one.
I haven't finished reviewing the patchset yet, but anyway -- David, can I
consider your e-mail as a potential Acked-by:?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] Bluetooth: hidp: make sure input buffers are big enough
From: Jiri Kosina @ 2014-01-28 20:53 UTC (permalink / raw)
To: David Herrmann
Cc: Marcel Holtmann, open list:HID CORE LAYER,
linux-bluetooth@vger.kernel.org development, Gustavo F. Padovan
In-Reply-To: <alpine.LNX.2.00.1401071811530.4962@pobox.suse.cz>
On Tue, 7 Jan 2014, Jiri Kosina wrote:
> > > So doing kzalloc(rsize, GFP_ATOMIC) in the HID-core for now, and copying
> > > the buffer around, seems like only viable solution for now, with the
> > > outlook of removing this ugliness once hid-core honors 'size' properly.
> >
> > Should I resend the patches and move it to hid_input_report() for now?
> > Or are you getting something in yourself?
>
> Due to various reasons I will not have access to any testing HW for the
> upcoming 2-3 days, so if you can cook something up in that timeframe, it'd
> be appreciated.
>
> Otherwise I'll be working on it by the end of this week.
David,
just got back to this, finally ... did you have time to work on this at
all, or should I just start from scratch?
> > Given the amount of reports on the list and bugzilla, I think we should
> > get this fix in asap. We can always fix it properly in -next.
>
> Agreed.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: multitouch: add FocalTech FTxxxx support
From: Jiri Kosina @ 2014-01-28 19:50 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Henrik Rydberg, linux-input, linux-kernel
In-Reply-To: <1390512794-3825-1-git-send-email-benjamin.tissoires@redhat.com>
On Thu, 23 Jan 2014, Benjamin Tissoires wrote:
> This is a Win7 device which does not work correctly with the default
> settings (not the previous default BT).
> However, the quirk ALWAYS_TRUE makes it working like a charm.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>
> Hi Jiri,
>
> well, it's been a long time since I did not add those kind of small patches.
> And I really hope not having to do some gain soon :)
World is a crude place :)
Applied, thanks Benjamin.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 1/2] HID: sony: Add battery status reporting for the Sixaxis and Dualshock 4 controllers.
From: Jiri Kosina @ 2014-01-28 19:39 UTC (permalink / raw)
To: Frank Praznik; +Cc: linux-input
In-Reply-To: <1390835857-4523-1-git-send-email-frank.praznik@oh.rr.com>
On Mon, 27 Jan 2014, Frank Praznik wrote:
> Add battery status reporting for the Sixaxis and Dualshock 4 controllers.
I have applied both patches, thanks Frank.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] hid-microsoft: Add ID's for Surface Type/Touch Cover 2
From: Jiri Kosina @ 2014-01-28 15:21 UTC (permalink / raw)
To: Reyad Attiyat; +Cc: linux-input, linux-kernel, Benjamin Tissoires
In-Reply-To: <52E021E8.8060608@gmail.com>
On Wed, 22 Jan 2014, Reyad Attiyat wrote:
> This patch fixes bug 64811 (https://bugzilla.kernel.org/show_bug.cgi?id=64811)
> The Microsoft Surface Type/Touch cover 2 devices have the flag
> HID_DG_CONTACTID in their reports.This causes the device to bind to the
> hid-multitouch driver, which doesn't handle generic keyboard/mouse input
> events.
> The patch adds the hardware id's of the device to hid-microsoft and to the HID
> special driver array, which makes the device get handled by
> hid-generic/hid-input properly.
>
> Singed-off-by: Reyad Attiyat <reyad.attiyat@gmail.com>
> Reviewed-by: Benjamin Tissoires<benjamin.tissoires@redhat.com>
I have now applied the patch.
Please fix your mail client for your next submissions so that it doesn't
whitespace-damage the patches.
I have fixed it now by hand. See Documentation/email-clients.txt for some
hints on how to do this properly next time.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: usbhid: quirk for CY-TM75 75 inch Touch Overlay
From: Jiri Kosina @ 2014-01-28 15:12 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Yufeng Shen, linux-input, linux-kernel@vger.kernel.org, linux-usb,
Andrew de los Reyes
In-Reply-To: <CAN+gG=F3EQ4o99HW3Qid52_Kp1g-yziW1yeNetP5mE9DmEmS2w@mail.gmail.com>
On Mon, 27 Jan 2014, Benjamin Tissoires wrote:
> > There is timeout error during initialization:
> > kernel: [ 11.733104] hid-multitouch 0003:1870:0110.0001: usb_submit_urb(ctrl) failed: -1
> > kernel: [ 11.734093] hid-multitouch 0003:1870:0110.0001: timeout initializing reports
> >
> > Adding quirk HID_QUIRK_NO_INIT_REPORTS can solve the problem.
>
> Could you please test the quirk HID_QUIRK_NO_INIT_INPUT_REPORTS instead?
> The patch is perfectly fine, but usually when it fails, only the input
> reports are non-readable, whereas features should. This way, you will
> still get the value of the features, which _may_ be sensible for the
> maxcontact information.
>
> BTW, HID_QUIRK_NO_INIT_INPUT_REPORTS has been introduced in v3.12 and
> is the default for Win 8 certified devices.
Ah, I have noticed this only after pushing the patch out. Good point.
Yufeng, if you test with HID_QUIRK_NO_INIT_INPUT_REPORTS successfully,
please let me know, and I'll apply a followup patch on top.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: usbhid: quirk for CY-TM75 75 inch Touch Overlay
From: Jiri Kosina @ 2014-01-28 15:11 UTC (permalink / raw)
To: Yufeng Shen; +Cc: linux-input, linux-kernel, linux-usb, adlr
In-Reply-To: <1390863766-14491-1-git-send-email-miletus@chromium.org>
On Mon, 27 Jan 2014, Yufeng Shen wrote:
> There is timeout error during initialization:
> kernel: [ 11.733104] hid-multitouch 0003:1870:0110.0001: usb_submit_urb(ctrl) failed: -1
> kernel: [ 11.734093] hid-multitouch 0003:1870:0110.0001: timeout initializing reports
>
> Adding quirk HID_QUIRK_NO_INIT_REPORTS can solve the problem.
>
> Signed-off-by: Yufeng Shen <miletus@chromium.org>
Applied.
> ---
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/usbhid/hid-quirks.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index f9304cb..ece2997 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -636,6 +636,7 @@
>
> #define USB_VENDOR_ID_NEXIO 0x1870
> #define USB_DEVICE_ID_NEXIO_MULTITOUCH_420 0x010d
> +#define USB_DEVICE_ID_NEXIO_MULTITOUCH_PTI0750 0x0110
>
> #define USB_VENDOR_ID_NEXTWINDOW 0x1926
> #define USB_DEVICE_ID_NEXTWINDOW_TOUCHSCREEN 0x0003
> diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
> index 0db9a67..be5270a 100644
> --- a/drivers/hid/usbhid/hid-quirks.c
> +++ b/drivers/hid/usbhid/hid-quirks.c
> @@ -74,6 +74,7 @@ static const struct hid_blacklist {
> { USB_VENDOR_ID_FREESCALE, USB_DEVICE_ID_FREESCALE_MX28, HID_QUIRK_NOGET },
> { USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS, HID_QUIRK_NOGET },
> { USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GX680R_LED_PANEL, HID_QUIRK_NO_INIT_REPORTS },
> + { USB_VENDOR_ID_NEXIO, USB_DEVICE_ID_NEXIO_MULTITOUCH_PTI0750, HID_QUIRK_NO_INIT_REPORTS },
> { USB_VENDOR_ID_NOVATEK, USB_DEVICE_ID_NOVATEK_MOUSE, HID_QUIRK_NO_INIT_REPORTS },
> { USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN, HID_QUIRK_NO_INIT_REPORTS },
> { USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN1, HID_QUIRK_NO_INIT_REPORTS },
> --
> 1.8.5.3
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid: add runtime PM support
From: Benjamin Tissoires @ 2014-01-28 14:19 UTC (permalink / raw)
To: Mika Westerberg
Cc: linux-input, Jiri Kosina, Benjamin Tissoires,
linux-kernel@vger.kernel.org
In-Reply-To: <20140128091258.GY18029@intel.com>
On Tue, Jan 28, 2014 at 4:12 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Jan 27, 2014 at 10:36:25PM -0500, Benjamin Tissoires wrote:
>> On Tue, Jan 14, 2014 at 5:13 AM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > This patch adds runtime PM support for the HID over I2C driver. When the
>> > i2c-hid device is first opened we power it on and on the last close we
>> > power it off.
>> >
>> > The implementation is not the most power efficient because it needs some
>> > interaction from the userspace (e.g close the device node whenever we are
>> > no more interested in getting events), nevertheless it allows us to save
>> > some power and works with devices that are not wake capable.
>> >
>>
>> Hi Mika,
>>
>> I am a little bit puzzled here. The commit message just says that you
>> changed the implementation of the power saving with the exact same
>> behavior... At least that's what I understand.
>> Currently, the devices should be put on sleep if nobody is reading,
>> and back alive if a reader arrives.
>> I think there is a gain with the patch, but my knowledge of the pm
>> subsystem is far too limited to see it :(
>
> Yes, there should be gain. If there is power domain involved like, ACPI in
> our case, runtime suspending the device on close will let ACPI to move the
> device to D3cold (e.g full off) which saves more power.
Oh, right. So, if you could just put this last sentence in the commit
message, that would be great.
>
>> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> > ---
>> > drivers/hid/i2c-hid/i2c-hid.c | 81 ++++++++++++++++++++++++++++---------------
>> > 1 file changed, 54 insertions(+), 27 deletions(-)
>> >
>> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> > index d1f81f52481a..ff767d03d60e 100644
>> > --- a/drivers/hid/i2c-hid/i2c-hid.c
>> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> > @@ -25,6 +25,7 @@
>> > #include <linux/delay.h>
>> > #include <linux/slab.h>
>> > #include <linux/pm.h>
>> > +#include <linux/pm_runtime.h>
>> > #include <linux/device.h>
>> > #include <linux/wait.h>
>> > #include <linux/err.h>
>> > @@ -454,10 +455,18 @@ static void i2c_hid_init_reports(struct hid_device *hid)
>> > return;
>> > }
>> >
>> > + /*
>> > + * The device must be powered on while we fetch initial reports
>> > + * from it.
>> > + */
>> > + pm_runtime_get_sync(&client->dev);
>> > +
>> > list_for_each_entry(report,
>> > &hid->report_enum[HID_FEATURE_REPORT].report_list, list)
>> > i2c_hid_init_report(report, inbuf, ihid->bufsize);
>> >
>> > + pm_runtime_put(&client->dev);
>> > +
>> > kfree(inbuf);
>> > }
>> >
>> > @@ -703,8 +712,8 @@ static int i2c_hid_open(struct hid_device *hid)
>> >
>> > mutex_lock(&i2c_hid_open_mut);
>> > if (!hid->open++) {
>> > - ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
>> > - if (ret) {
>> > + ret = pm_runtime_get_sync(&client->dev);
>>
>> dummy question (kind of late here...). Is there a counter of how many
>> get/put has been called in pm_runtime which could allow us to get rid
>> of the hid->open count?
>
> There is a counter but I'm not sure if drivers are supposed to use that. I
> would prefer not to use that.
ok
>
>>
>> > + if (ret < 0) {
>> > hid->open--;
>> > goto done;
>> > }
>> > @@ -712,7 +721,7 @@ static int i2c_hid_open(struct hid_device *hid)
>> > }
>> > done:
>> > mutex_unlock(&i2c_hid_open_mut);
>> > - return ret;
>> > + return ret < 0 ? ret : 0;
>> > }
>> >
>> > static void i2c_hid_close(struct hid_device *hid)
>> > @@ -729,37 +738,17 @@ static void i2c_hid_close(struct hid_device *hid)
>> > clear_bit(I2C_HID_STARTED, &ihid->flags);
>> >
>> > /* Save some power */
>> > - i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
>> > + pm_runtime_put(&client->dev);
>> > }
>> > mutex_unlock(&i2c_hid_open_mut);
>> > }
>> >
>> > -static int i2c_hid_power(struct hid_device *hid, int lvl)
>> > -{
>> > - struct i2c_client *client = hid->driver_data;
>> > - struct i2c_hid *ihid = i2c_get_clientdata(client);
>> > - int ret = 0;
>> > -
>> > - i2c_hid_dbg(ihid, "%s lvl:%d\n", __func__, lvl);
>> > -
>> > - switch (lvl) {
>> > - case PM_HINT_FULLON:
>> > - ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
>> > - break;
>> > - case PM_HINT_NORMAL:
>> > - ret = i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
>> > - break;
>> > - }
>> > - return ret;
>> > -}
>> > -
>> > static struct hid_ll_driver i2c_hid_ll_driver = {
>> > .parse = i2c_hid_parse,
>> > .start = i2c_hid_start,
>> > .stop = i2c_hid_stop,
>> > .open = i2c_hid_open,
>> > .close = i2c_hid_close,
>> > - .power = i2c_hid_power,
>>
>> If I understand correctly, here you are trying to fix hidraw (with
>> i2c_hid tramsport) which used to set_power on/off twice with the first
>> reader, right?
>
> Right.
>
>> I don't think we have other i2c_hid users of hid_hw_power, but I am a
>> little bit worried of simply removing the callback.
>
> OK.
>
>> What if we just change i2c_hid_set_power in i2c_hid_power by the
>> corresponding pm_runtime calls?
>
> Sure, I'll change that in the next version.
>
>>
>> > .request = i2c_hid_request,
>> > };
>> >
>> > @@ -973,13 +962,17 @@ static int i2c_hid_probe(struct i2c_client *client,
>> > if (ret < 0)
>> > goto err;
>> >
>> > + pm_runtime_get_noresume(&client->dev);
>> > + pm_runtime_set_active(&client->dev);
>> > + pm_runtime_enable(&client->dev);
>> > +
>> > ret = i2c_hid_fetch_hid_descriptor(ihid);
>> > if (ret < 0)
>> > - goto err;
>> > + goto err_pm;
>> >
>> > ret = i2c_hid_init_irq(client);
>> > if (ret < 0)
>> > - goto err;
>> > + goto err_pm;
>> >
>> > hid = hid_allocate_device();
>> > if (IS_ERR(hid)) {
>> > @@ -1010,6 +1003,7 @@ static int i2c_hid_probe(struct i2c_client *client,
>> > goto err_mem_free;
>> > }
>> >
>> > + pm_runtime_put(&client->dev);
>> > return 0;
>> >
>> > err_mem_free:
>> > @@ -1018,6 +1012,10 @@ err_mem_free:
>> > err_irq:
>> > free_irq(client->irq, ihid);
>> >
>> > +err_pm:
>> > + pm_runtime_put_noidle(&client->dev);
>> > + pm_runtime_disable(&client->dev);
>> > +
>> > err:
>> > i2c_hid_free_buffers(ihid);
>> > kfree(ihid);
>> > @@ -1029,6 +1027,11 @@ static int i2c_hid_remove(struct i2c_client *client)
>> > struct i2c_hid *ihid = i2c_get_clientdata(client);
>> > struct hid_device *hid;
>> >
>> > + pm_runtime_get_sync(&client->dev);
>> > + pm_runtime_disable(&client->dev);
>> > + pm_runtime_set_suspended(&client->dev);
>> > + pm_runtime_put_noidle(&client->dev);
>> > +
>> > hid = ihid->hid;
>> > hid_destroy_device(hid);
>> >
>> > @@ -1074,7 +1077,31 @@ static int i2c_hid_resume(struct device *dev)
>> > }
>> > #endif
>> >
>> > -static SIMPLE_DEV_PM_OPS(i2c_hid_pm, i2c_hid_suspend, i2c_hid_resume);
>> > +#ifdef CONFIG_PM_RUNTIME
>> > +static int i2c_hid_runtime_suspend(struct device *dev)
>> > +{
>> > + struct i2c_client *client = to_i2c_client(dev);
>> > +
>> > + i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
>> > + disable_irq(client->irq);
>> > + return 0;
>> > +}
>> > +
>> > +static int i2c_hid_runtime_resume(struct device *dev)
>> > +{
>> > + struct i2c_client *client = to_i2c_client(dev);
>> > +
>> > + enable_irq(client->irq);
>> > + i2c_hid_set_power(client, I2C_HID_PWR_ON);
>> > + return 0;
>> > +}
>>
>> These two functions looks very similar to i2c_hid_suspend and
>> i2c_hid_resume, without the reset and the irq_wake :(
>> So, my question here is can we use some common code for them?
>> It may not be possible regarding CONFIG_PM_RUNTIME and
>> CONFIG_PM_SLEEP, but it still looks ugly to me.
>
> It looks ugly, I agree and we can probably reuse some code in the
> callbacks. I'll look into that.
well, anyway, if this involve something even more ugly, we can for
sure stick with this version :)
>
>> I tested this today, and it works, so you should be right, but I'd
>> like to have your opinion on this.
>
> Thanks for testing and comments.
>
> I'll prepare a new version with the suggested changes if you are OK with my
> explanations ;-)
Sure I am. Thanks for handling the whole ACPI part of this module. I
was really pleased the other day to see that the touchscreen of a Bay
trail tablet is now working out of the box :) I still have many other
problems with this tablet, but still, having the input working is
always good with a tablet...
Cheers,
Benjamin
^ permalink raw reply
* [PATCH] module: allow multiple calls to MODULE_DEVICE_TABLE() per module
From: Tom Gundersen @ 2014-01-28 13:42 UTC (permalink / raw)
To: linux-kernel
Cc: linux-input, kay, Tom Gundersen, Dmitry Torokhov, Rusty Russell
In-Reply-To: <87k3dkzy4t.fsf@rustcorp.com.au>
Commit 78551277e4df5: "Input: i8042 - add PNP modaliases" had a bug, where the
second call to MODULE_DEVICE_TABLE() overrode the first resulting in not all
the modaliases being exposed.
This fixes the problem by including the name of the device_id table in the
__mod_*_device_table alias, allowing us to export several device_id tables
per module.
Suggested-by: Kay Sievers <kay@vrfy.org>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Tom Gundersen <teg@jklm.no>
---
include/linux/module.h | 2 +-
scripts/mod/file2alias.c | 14 +++++++++-----
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 15cd6b1..7732d76 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -143,7 +143,7 @@ extern const struct gtype##_id __mod_##gtype##_table \
#define MODULE_DESCRIPTION(_description) MODULE_INFO(description, _description)
#define MODULE_DEVICE_TABLE(type,name) \
- MODULE_GENERIC_TABLE(type##_device,name)
+ MODULE_GENERIC_TABLE(type##__##name##_device,name)
/* Version of form [<epoch>:]<version>[-<extra-version>].
Or for CVS/RCS ID version, everything but the number is stripped.
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 2370863..6778381 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -42,7 +42,7 @@ typedef unsigned char __u8;
/* This array collects all instances that use the generic do_table */
struct devtable {
- const char *device_id; /* name of table, __mod_<name>_device_table. */
+ const char *device_id; /* name of table, __mod_<name>__*_device_table. */
unsigned long id_size;
void *function;
};
@@ -146,7 +146,8 @@ static void device_id_check(const char *modname, const char *device_id,
if (size % id_size || size < id_size) {
fatal("%s: sizeof(struct %s_device_id)=%lu is not a modulo "
- "of the size of section __mod_%s_device_table=%lu.\n"
+ "of the size of "
+ "section __mod_%s__<identifier>_device_table=%lu.\n"
"Fix definition of struct %s_device_id "
"in mod_devicetable.h\n",
modname, device_id, id_size, device_id, size, device_id);
@@ -1206,7 +1207,7 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
{
void *symval;
char *zeros = NULL;
- const char *name;
+ const char *name, *identifier;
unsigned int namelen;
/* We're looking for a section relative symbol */
@@ -1217,7 +1218,7 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
if (ELF_ST_TYPE(sym->st_info) != STT_OBJECT)
return;
- /* All our symbols are of form <prefix>__mod_XXX_device_table. */
+ /* All our symbols are of form <prefix>__mod_<name>__<identifier>_device_table. */
name = strstr(symname, "__mod_");
if (!name)
return;
@@ -1227,7 +1228,10 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
return;
if (strcmp(name + namelen - strlen("_device_table"), "_device_table"))
return;
- namelen -= strlen("_device_table");
+ identifier = strstr(name, "__");
+ if (!identifier)
+ return;
+ namelen = identifier - name;
/* Handle all-NULL symbols allocated into .bss */
if (info->sechdrs[get_secindex(info, sym)].sh_type & SHT_NOBITS) {
--
1.8.5.3
^ permalink raw reply related
* Re: [RFC][PATCH] module: allow multiple calls to MODULE_DEVICE_TABLE() per module
From: Tom Gundersen @ 2014-01-28 12:38 UTC (permalink / raw)
To: Rusty Russell
Cc: LKML, linux-input@vger.kernel.org, Kay Sievers, Dmitry Torokhov,
Greg Kroah-Hartman
In-Reply-To: <87k3dkzy4t.fsf@rustcorp.com.au>
Hi Rusty,
On Tue, Jan 28, 2014 at 2:35 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Tom Gundersen <teg@jklm.no> writes:
>> Commit 78551277e4df5: "Input: i8042 - add PNP modaliases" had a bug, where the
>> second call to MODULE_DEVICE_TABLE() overrode the first resulting in not all
>> the modaliases being exposed.
>
> No Signed-off-by?
Sorry about that, I'll resend in a minute with the signoff.
Cheers,
Tom
^ permalink raw reply
* Re: Can I limit the scope of an input device to specific processes ?
From: Dr. Zimmermann @ 2014-01-28 12:28 UTC (permalink / raw)
To: linux-input
In-Reply-To: <CANq1E4ReNtJgC7zArLo=Bo87YZXOukFTTuxjYf_nupfmUkCL+A@mail.gmail.com>
Am 28.01.2014 12:15, schrieb David Herrmann:
> Hi
>
> On Tue, Jan 28, 2014 at 11:20 AM, Dr. Zimmermann
> <R.Zimmermann@uke.uni-hamburg.de> wrote:
>> Hi,
>>
>> we've got a special type of keyboard which should only be available in one
>> specific process and not publicly, as usually keyboards are.
>>
>> Is it possible to limit the scope of a specific keyboard?
>>
>> Or is there a more suited forum where to ask?
Hi David,
>
> There is no such interface in the kernel. However, with careful
> access-management, you can get what you want by just allowing your
> single process access to the device. But I guess you are working with
> an X11 system, so a description of what you actually want to achieve
> would help a lot.
Thnx for Your quick reply.
(Yes, we are using X. No, the problem is not specific to X: It'll also
occur when You switch between system consoles.)
For research purposes we are measuring human brain activity under the
influence of external stimuli (visual/auditory/somatosensory). The
subject has to reply to tasks using 'button boxes' which are implemented
as keyboards. Usually, type and time of the buttonpress are of
importance (and will probably change further procedere). Keyboard
responses usually go to that window (or system console) which has the
current focus or is active and *this* is something, we do not want.
Sometimes we show visual stimuli on one screen(#1) and operate the
system from another screen(#2). Sometimes we even have no visual stimuli
at all, but like to read subject responses and control the stimuli from
within a control window. In all cases it would be helpful if the
response-boxes would not automatically send their output to the active
window resp. application.
Helpful would be, if the output of the response-box-keyboards
* could be restricted to a specific process
* is not sent automatically, but only to a program which 'opened' the device
* is not sent automatically, but has to be read from a device.
(Could this be done preserving the exact timepoint of each buttonpress?)
Hope that was not too lengthy...
>
> Thanks
> David
>
Thanks, Roger
--
Besuchen Sie uns auf: www.uke.de
_____________________________________________________________________
Universitätsklinikum Hamburg-Eppendorf; Körperschaft des öffentlichen Rechts; Gerichtsstand: Hamburg
Vorstandsmitglieder: Prof. Dr. Christian Gerloff (Vertreter des Vorsitzenden), Prof. Dr. Dr. Uwe Koch-Gromus, Joachim Prölß, Rainer Schoppik
_____________________________________________________________________
SAVE PAPER - THINK BEFORE PRINTING
^ permalink raw reply
* Re: Can I limit the scope of an input device to specific processes ?
From: David Herrmann @ 2014-01-28 11:15 UTC (permalink / raw)
To: R.Zimmermann; +Cc: open list:HID CORE LAYER
In-Reply-To: <52E78481.6040606@UKE.Uni-Hamburg.de>
Hi
On Tue, Jan 28, 2014 at 11:20 AM, Dr. Zimmermann
<R.Zimmermann@uke.uni-hamburg.de> wrote:
> Hi,
>
> we've got a special type of keyboard which should only be available in one
> specific process and not publicly, as usually keyboards are.
>
> Is it possible to limit the scope of a specific keyboard?
>
> Or is there a more suited forum where to ask?
There is no such interface in the kernel. However, with careful
access-management, you can get what you want by just allowing your
single process access to the device. But I guess you are working with
an X11 system, so a description of what you actually want to achieve
would help a lot.
Thanks
David
^ permalink raw reply
* Can I limit the scope of an input device to specific processes ?
From: Dr. Zimmermann @ 2014-01-28 10:20 UTC (permalink / raw)
To: linux-input
In-Reply-To: <S1754733AbaA1IxR/20140128085317Z+41@vger.kernel.org>
[-- Attachment #1: Type: text/plain, Size: 740 bytes --]
Hi,
we've got a special type of keyboard which should only be available in
one specific process and not publicly, as usually keyboards are.
Is it possible to limit the scope of a specific keyboard?
Or is there a more suited forum where to ask?
Regards, Roger
--
Besuchen Sie uns auf: www.uke.de
_____________________________________________________________________
Universitätsklinikum Hamburg-Eppendorf; Körperschaft des öffentlichen Rechts; Gerichtsstand: Hamburg
Vorstandsmitglieder: Prof. Dr. Christian Gerloff (Vertreter des Vorsitzenden), Prof. Dr. Dr. Uwe Koch-Gromus, Joachim Prölß, Rainer Schoppik
_____________________________________________________________________
SAVE PAPER - THINK BEFORE PRINTING
[-- Attachment #2: R_Zimmermann.vcf --]
[-- Type: text/x-vcard, Size: 427 bytes --]
begin:vcard
fn:Dr. Roger Zimmermann
n:Zimmermann;Dr. Roger
org;quoted-printable;quoted-printable:Zentrum f=C3=BCr exp. Medizin;Institut f=C3=BCr Neuro- und Pathophysiologie
adr;quoted-printable:;;Martinistra=C3=9Fe 52;Hamburg;;20246;Germany
email;internet:R.Zimmermann@UKE.Uni-Hamburg.de
tel;work:..49 (0)40 42803 5351
tel;fax:..49 (0)40 42803 7255
x-mozilla-html:FALSE
url:http://www.uke.uni-hamburg.de
version:2.1
end:vcard
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid: add runtime PM support
From: Mika Westerberg @ 2014-01-28 9:12 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, Jiri Kosina, Benjamin Tissoires,
linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=Fa+H53V1evKXiWK6OpWj29pF4z8rGJAXWs3C7mHm1mLQ@mail.gmail.com>
On Mon, Jan 27, 2014 at 10:36:25PM -0500, Benjamin Tissoires wrote:
> On Tue, Jan 14, 2014 at 5:13 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > This patch adds runtime PM support for the HID over I2C driver. When the
> > i2c-hid device is first opened we power it on and on the last close we
> > power it off.
> >
> > The implementation is not the most power efficient because it needs some
> > interaction from the userspace (e.g close the device node whenever we are
> > no more interested in getting events), nevertheless it allows us to save
> > some power and works with devices that are not wake capable.
> >
>
> Hi Mika,
>
> I am a little bit puzzled here. The commit message just says that you
> changed the implementation of the power saving with the exact same
> behavior... At least that's what I understand.
> Currently, the devices should be put on sleep if nobody is reading,
> and back alive if a reader arrives.
> I think there is a gain with the patch, but my knowledge of the pm
> subsystem is far too limited to see it :(
Yes, there should be gain. If there is power domain involved like, ACPI in
our case, runtime suspending the device on close will let ACPI to move the
device to D3cold (e.g full off) which saves more power.
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> > drivers/hid/i2c-hid/i2c-hid.c | 81 ++++++++++++++++++++++++++++---------------
> > 1 file changed, 54 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index d1f81f52481a..ff767d03d60e 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -25,6 +25,7 @@
> > #include <linux/delay.h>
> > #include <linux/slab.h>
> > #include <linux/pm.h>
> > +#include <linux/pm_runtime.h>
> > #include <linux/device.h>
> > #include <linux/wait.h>
> > #include <linux/err.h>
> > @@ -454,10 +455,18 @@ static void i2c_hid_init_reports(struct hid_device *hid)
> > return;
> > }
> >
> > + /*
> > + * The device must be powered on while we fetch initial reports
> > + * from it.
> > + */
> > + pm_runtime_get_sync(&client->dev);
> > +
> > list_for_each_entry(report,
> > &hid->report_enum[HID_FEATURE_REPORT].report_list, list)
> > i2c_hid_init_report(report, inbuf, ihid->bufsize);
> >
> > + pm_runtime_put(&client->dev);
> > +
> > kfree(inbuf);
> > }
> >
> > @@ -703,8 +712,8 @@ static int i2c_hid_open(struct hid_device *hid)
> >
> > mutex_lock(&i2c_hid_open_mut);
> > if (!hid->open++) {
> > - ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
> > - if (ret) {
> > + ret = pm_runtime_get_sync(&client->dev);
>
> dummy question (kind of late here...). Is there a counter of how many
> get/put has been called in pm_runtime which could allow us to get rid
> of the hid->open count?
There is a counter but I'm not sure if drivers are supposed to use that. I
would prefer not to use that.
>
> > + if (ret < 0) {
> > hid->open--;
> > goto done;
> > }
> > @@ -712,7 +721,7 @@ static int i2c_hid_open(struct hid_device *hid)
> > }
> > done:
> > mutex_unlock(&i2c_hid_open_mut);
> > - return ret;
> > + return ret < 0 ? ret : 0;
> > }
> >
> > static void i2c_hid_close(struct hid_device *hid)
> > @@ -729,37 +738,17 @@ static void i2c_hid_close(struct hid_device *hid)
> > clear_bit(I2C_HID_STARTED, &ihid->flags);
> >
> > /* Save some power */
> > - i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
> > + pm_runtime_put(&client->dev);
> > }
> > mutex_unlock(&i2c_hid_open_mut);
> > }
> >
> > -static int i2c_hid_power(struct hid_device *hid, int lvl)
> > -{
> > - struct i2c_client *client = hid->driver_data;
> > - struct i2c_hid *ihid = i2c_get_clientdata(client);
> > - int ret = 0;
> > -
> > - i2c_hid_dbg(ihid, "%s lvl:%d\n", __func__, lvl);
> > -
> > - switch (lvl) {
> > - case PM_HINT_FULLON:
> > - ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
> > - break;
> > - case PM_HINT_NORMAL:
> > - ret = i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
> > - break;
> > - }
> > - return ret;
> > -}
> > -
> > static struct hid_ll_driver i2c_hid_ll_driver = {
> > .parse = i2c_hid_parse,
> > .start = i2c_hid_start,
> > .stop = i2c_hid_stop,
> > .open = i2c_hid_open,
> > .close = i2c_hid_close,
> > - .power = i2c_hid_power,
>
> If I understand correctly, here you are trying to fix hidraw (with
> i2c_hid tramsport) which used to set_power on/off twice with the first
> reader, right?
Right.
> I don't think we have other i2c_hid users of hid_hw_power, but I am a
> little bit worried of simply removing the callback.
OK.
> What if we just change i2c_hid_set_power in i2c_hid_power by the
> corresponding pm_runtime calls?
Sure, I'll change that in the next version.
>
> > .request = i2c_hid_request,
> > };
> >
> > @@ -973,13 +962,17 @@ static int i2c_hid_probe(struct i2c_client *client,
> > if (ret < 0)
> > goto err;
> >
> > + pm_runtime_get_noresume(&client->dev);
> > + pm_runtime_set_active(&client->dev);
> > + pm_runtime_enable(&client->dev);
> > +
> > ret = i2c_hid_fetch_hid_descriptor(ihid);
> > if (ret < 0)
> > - goto err;
> > + goto err_pm;
> >
> > ret = i2c_hid_init_irq(client);
> > if (ret < 0)
> > - goto err;
> > + goto err_pm;
> >
> > hid = hid_allocate_device();
> > if (IS_ERR(hid)) {
> > @@ -1010,6 +1003,7 @@ static int i2c_hid_probe(struct i2c_client *client,
> > goto err_mem_free;
> > }
> >
> > + pm_runtime_put(&client->dev);
> > return 0;
> >
> > err_mem_free:
> > @@ -1018,6 +1012,10 @@ err_mem_free:
> > err_irq:
> > free_irq(client->irq, ihid);
> >
> > +err_pm:
> > + pm_runtime_put_noidle(&client->dev);
> > + pm_runtime_disable(&client->dev);
> > +
> > err:
> > i2c_hid_free_buffers(ihid);
> > kfree(ihid);
> > @@ -1029,6 +1027,11 @@ static int i2c_hid_remove(struct i2c_client *client)
> > struct i2c_hid *ihid = i2c_get_clientdata(client);
> > struct hid_device *hid;
> >
> > + pm_runtime_get_sync(&client->dev);
> > + pm_runtime_disable(&client->dev);
> > + pm_runtime_set_suspended(&client->dev);
> > + pm_runtime_put_noidle(&client->dev);
> > +
> > hid = ihid->hid;
> > hid_destroy_device(hid);
> >
> > @@ -1074,7 +1077,31 @@ static int i2c_hid_resume(struct device *dev)
> > }
> > #endif
> >
> > -static SIMPLE_DEV_PM_OPS(i2c_hid_pm, i2c_hid_suspend, i2c_hid_resume);
> > +#ifdef CONFIG_PM_RUNTIME
> > +static int i2c_hid_runtime_suspend(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > +
> > + i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
> > + disable_irq(client->irq);
> > + return 0;
> > +}
> > +
> > +static int i2c_hid_runtime_resume(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > +
> > + enable_irq(client->irq);
> > + i2c_hid_set_power(client, I2C_HID_PWR_ON);
> > + return 0;
> > +}
>
> These two functions looks very similar to i2c_hid_suspend and
> i2c_hid_resume, without the reset and the irq_wake :(
> So, my question here is can we use some common code for them?
> It may not be possible regarding CONFIG_PM_RUNTIME and
> CONFIG_PM_SLEEP, but it still looks ugly to me.
It looks ugly, I agree and we can probably reuse some code in the
callbacks. I'll look into that.
> I tested this today, and it works, so you should be right, but I'd
> like to have your opinion on this.
Thanks for testing and comments.
I'll prepare a new version with the suggested changes if you are OK with my
explanations ;-)
^ permalink raw reply
* Re: [PATCH 0/3] Input: add dt support to zforce driver
From: Dmitry Torokhov @ 2014-01-28 6:45 UTC (permalink / raw)
To: Heiko Stübner; +Cc: Henrik Rydberg, linux-input
In-Reply-To: <2587208.XYbbI8xJiH@phil>
Hi Heiko,
On Thu, Jan 09, 2014 at 10:19:15PM +0100, Heiko Stübner wrote:
> This adds the binding documentation and necessary parsing code to make
> zforce based touchscreen usable on devicetree platforms.
>
> Heiko Stuebner (3):
> dt-bindings: bindings for zforce touchscreens
> Input: zforce: Use internal pdata pointer instead of dev_get_platdata
> Input: zforce: add devicetree support
>
> .../bindings/input/touchscreen/zforce_ts.txt | 30 ++++++++++
> .../devicetree/bindings/vendor-prefixes.txt | 1 +
> drivers/input/touchscreen/zforce_ts.c | 63 ++++++++++++++++++--
> 3 files changed, 89 insertions(+), 5 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
>
I applied the 2nd one and then folded parts of 1st into the 3rd, leaving
changes to devicetree/bindings/vendor-prefixes.txt out. Please resubmit
them to DT maintainers.
Thanks.
--
Dmitry
--
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
^ permalink raw reply
* Re: Input: zforce - fix various small issues
From: Dmitry Torokhov @ 2014-01-28 6:44 UTC (permalink / raw)
To: Luis Ortega; +Cc: heiko, yongjun_wei, linux-input, linux-kernel
In-Reply-To: <1390848373-7723-1-git-send-email-luiorpe1@upv.es>
On Mon, Jan 27, 2014 at 07:46:09PM +0100, Luis Ortega wrote:
> As a kernel newbie and owner of a Barnes & Noble e-reader I was
> curious to review this driver to learn more about the touchscreen.
>
> The first two patches are fairly innocuous whereas the last two
> slightly modify the code to fix two small issues I discovered.
> I don't have the setup to test them but they look logically correct
> to me.
>
> [PATCH 1/4] Input: zforce - fix spelling errors
> [PATCH 2/4] Input: zforce - fix lines exceeding 80 columns
> [PATCH 3/4] Input: zforce - Remove unnecessary payload data checks
> [PATCH 4/4] Input: zforce - reduce stack memory allocated to frames
Applied all 4, thank you.
--
Dmitry
^ permalink raw reply
* RE: [PATCH] Input: i8042-io - Exclude mips platforms when allocating/deallocating IO regions.
From: Raghu Gandham @ 2014-01-28 6:25 UTC (permalink / raw)
To: Aaro Koskinen, Dmitry Torokhov
Cc: linux-input@vger.kernel.org, linux-mips@linux-mips.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20140127202435.GA589@drone.musicnaut.iki.fi>
Hi Aaro,
>
> On Sun, Jan 26, 2014 at 10:56:38PM -0800, Dmitry Torokhov wrote:
> > On Mon, Jan 27, 2014 at 12:32:36AM +0000, Raghu Gandham wrote:
> > > > On Sat, Jan 25, 2014 at 11:01:54AM -0800, Raghu Gandham wrote:
> > > > > The standard IO regions are already reserved by the platform
> > > > > code on most MIPS devices(malta, cobalt, sni). The Commit
> > > > > 197a1e96c8be5b6005145af3a4c0e45e2d651444
> > > > > ("Input: i8042-io - fix up region handling on MIPS") introduced
> > > > > a bug on these MIPS platforms causing i8042 driver to fail when
> > > > > trying to reserve IO ports.
> > > > > Prior to the above mentioned commit request_region is skipped on
> > > > > MIPS but release_region is called.
> > > > >
> > > > > This patch reverts commit
> > > > > 197a1e96c8be5b6005145af3a4c0e45e2d651444
> > > > > and also avoids calling release_region for MIPS.
> > > >
> > > > The problem is that IO regions are reserved on _most_, but not
> > > > _all_ devices.
> > > > MIPS should figure out what they want to do with i8042 registers
> > > > and be consistent on all devices.
> > >
> > > Please examine the attached patch which went upstream in April of 2004.
> > > Since then it had been a convention not to call request_region
> > > routine in
> > > i8042 for MIPS. The attached patch had a glitch that it guarded
> > > request_region in i8042-io.h but skipped guarding release_region in
> > > i8042-io.h. I believe that the issue Aaro saw was due to this
> > > glitch. Below is the error quoted in Aaro's commit message.
> > >
> > > [ 2.112000] Trying to free nonexistent resource <0000000000000060-
> 000000000000006f>
> > >
> > > My patch reinstates the convention followed on MIPS devices along
> > > with fixing Aaro's issue.
> >
> > I assume that Aaro did test his patch and on his box request_region()
> > succeeds. That would indicate that various MIPS sub-arches still not
> > settled on the topic.
>
> request_region() succeeds on Loongson and OCTEON.
This would mean that before your patch in oct of 2012, Loongson and Octeon
were not reserving the IO space for i8042.
>
> On OCTEONs without PCI, request_region() will fail which is correct as there
> is no I/O space.
>
> I wasn't aware of that 2004 patch (it pre-dates GIT history of mainline Linux).
> Why the regions are already reserved by the platform code?
The only information I have is the comment before request_region in i8042-io.h that
touching data register on some platforms is flaky. If your patch was primarily aimed at
addressing the error message from release_region, the current patch I uploaded should
also take care of it too.
Thanks,
Raghu
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid: add runtime PM support
From: Benjamin Tissoires @ 2014-01-28 3:36 UTC (permalink / raw)
To: Mika Westerberg
Cc: linux-input, Jiri Kosina, Benjamin Tissoires,
linux-kernel@vger.kernel.org
In-Reply-To: <1389694438-18614-1-git-send-email-mika.westerberg@linux.intel.com>
On Tue, Jan 14, 2014 at 5:13 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> This patch adds runtime PM support for the HID over I2C driver. When the
> i2c-hid device is first opened we power it on and on the last close we
> power it off.
>
> The implementation is not the most power efficient because it needs some
> interaction from the userspace (e.g close the device node whenever we are
> no more interested in getting events), nevertheless it allows us to save
> some power and works with devices that are not wake capable.
>
Hi Mika,
I am a little bit puzzled here. The commit message just says that you
changed the implementation of the power saving with the exact same
behavior... At least that's what I understand.
Currently, the devices should be put on sleep if nobody is reading,
and back alive if a reader arrives.
I think there is a gain with the patch, but my knowledge of the pm
subsystem is far too limited to see it :(
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> drivers/hid/i2c-hid/i2c-hid.c | 81 ++++++++++++++++++++++++++++---------------
> 1 file changed, 54 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index d1f81f52481a..ff767d03d60e 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -25,6 +25,7 @@
> #include <linux/delay.h>
> #include <linux/slab.h>
> #include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> #include <linux/device.h>
> #include <linux/wait.h>
> #include <linux/err.h>
> @@ -454,10 +455,18 @@ static void i2c_hid_init_reports(struct hid_device *hid)
> return;
> }
>
> + /*
> + * The device must be powered on while we fetch initial reports
> + * from it.
> + */
> + pm_runtime_get_sync(&client->dev);
> +
> list_for_each_entry(report,
> &hid->report_enum[HID_FEATURE_REPORT].report_list, list)
> i2c_hid_init_report(report, inbuf, ihid->bufsize);
>
> + pm_runtime_put(&client->dev);
> +
> kfree(inbuf);
> }
>
> @@ -703,8 +712,8 @@ static int i2c_hid_open(struct hid_device *hid)
>
> mutex_lock(&i2c_hid_open_mut);
> if (!hid->open++) {
> - ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
> - if (ret) {
> + ret = pm_runtime_get_sync(&client->dev);
dummy question (kind of late here...). Is there a counter of how many
get/put has been called in pm_runtime which could allow us to get rid
of the hid->open count?
> + if (ret < 0) {
> hid->open--;
> goto done;
> }
> @@ -712,7 +721,7 @@ static int i2c_hid_open(struct hid_device *hid)
> }
> done:
> mutex_unlock(&i2c_hid_open_mut);
> - return ret;
> + return ret < 0 ? ret : 0;
> }
>
> static void i2c_hid_close(struct hid_device *hid)
> @@ -729,37 +738,17 @@ static void i2c_hid_close(struct hid_device *hid)
> clear_bit(I2C_HID_STARTED, &ihid->flags);
>
> /* Save some power */
> - i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
> + pm_runtime_put(&client->dev);
> }
> mutex_unlock(&i2c_hid_open_mut);
> }
>
> -static int i2c_hid_power(struct hid_device *hid, int lvl)
> -{
> - struct i2c_client *client = hid->driver_data;
> - struct i2c_hid *ihid = i2c_get_clientdata(client);
> - int ret = 0;
> -
> - i2c_hid_dbg(ihid, "%s lvl:%d\n", __func__, lvl);
> -
> - switch (lvl) {
> - case PM_HINT_FULLON:
> - ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
> - break;
> - case PM_HINT_NORMAL:
> - ret = i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
> - break;
> - }
> - return ret;
> -}
> -
> static struct hid_ll_driver i2c_hid_ll_driver = {
> .parse = i2c_hid_parse,
> .start = i2c_hid_start,
> .stop = i2c_hid_stop,
> .open = i2c_hid_open,
> .close = i2c_hid_close,
> - .power = i2c_hid_power,
If I understand correctly, here you are trying to fix hidraw (with
i2c_hid tramsport) which used to set_power on/off twice with the first
reader, right?
I don't think we have other i2c_hid users of hid_hw_power, but I am a
little bit worried of simply removing the callback.
What if we just change i2c_hid_set_power in i2c_hid_power by the
corresponding pm_runtime calls?
> .request = i2c_hid_request,
> };
>
> @@ -973,13 +962,17 @@ static int i2c_hid_probe(struct i2c_client *client,
> if (ret < 0)
> goto err;
>
> + pm_runtime_get_noresume(&client->dev);
> + pm_runtime_set_active(&client->dev);
> + pm_runtime_enable(&client->dev);
> +
> ret = i2c_hid_fetch_hid_descriptor(ihid);
> if (ret < 0)
> - goto err;
> + goto err_pm;
>
> ret = i2c_hid_init_irq(client);
> if (ret < 0)
> - goto err;
> + goto err_pm;
>
> hid = hid_allocate_device();
> if (IS_ERR(hid)) {
> @@ -1010,6 +1003,7 @@ static int i2c_hid_probe(struct i2c_client *client,
> goto err_mem_free;
> }
>
> + pm_runtime_put(&client->dev);
> return 0;
>
> err_mem_free:
> @@ -1018,6 +1012,10 @@ err_mem_free:
> err_irq:
> free_irq(client->irq, ihid);
>
> +err_pm:
> + pm_runtime_put_noidle(&client->dev);
> + pm_runtime_disable(&client->dev);
> +
> err:
> i2c_hid_free_buffers(ihid);
> kfree(ihid);
> @@ -1029,6 +1027,11 @@ static int i2c_hid_remove(struct i2c_client *client)
> struct i2c_hid *ihid = i2c_get_clientdata(client);
> struct hid_device *hid;
>
> + pm_runtime_get_sync(&client->dev);
> + pm_runtime_disable(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
> + pm_runtime_put_noidle(&client->dev);
> +
> hid = ihid->hid;
> hid_destroy_device(hid);
>
> @@ -1074,7 +1077,31 @@ static int i2c_hid_resume(struct device *dev)
> }
> #endif
>
> -static SIMPLE_DEV_PM_OPS(i2c_hid_pm, i2c_hid_suspend, i2c_hid_resume);
> +#ifdef CONFIG_PM_RUNTIME
> +static int i2c_hid_runtime_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> +
> + i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
> + disable_irq(client->irq);
> + return 0;
> +}
> +
> +static int i2c_hid_runtime_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> +
> + enable_irq(client->irq);
> + i2c_hid_set_power(client, I2C_HID_PWR_ON);
> + return 0;
> +}
These two functions looks very similar to i2c_hid_suspend and
i2c_hid_resume, without the reset and the irq_wake :(
So, my question here is can we use some common code for them?
It may not be possible regarding CONFIG_PM_RUNTIME and
CONFIG_PM_SLEEP, but it still looks ugly to me.
I tested this today, and it works, so you should be right, but I'd
like to have your opinion on this.
Cheers,
Benjamin
> +#endif
> +
> +static const struct dev_pm_ops i2c_hid_pm = {
> + SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_suspend, i2c_hid_resume)
> + SET_RUNTIME_PM_OPS(i2c_hid_runtime_suspend, i2c_hid_runtime_resume,
> + NULL)
> +};
>
> static const struct i2c_device_id i2c_hid_id_table[] = {
> { "hid", 0 },
> --
> 1.8.5.2
^ permalink raw reply
* Re: [PATCH] HID: usbhid: quirk for CY-TM75 75 inch Touch Overlay
From: Benjamin Tissoires @ 2014-01-28 2:57 UTC (permalink / raw)
To: Yufeng Shen
Cc: linux-input, Jiri Kosina, linux-kernel@vger.kernel.org, linux-usb,
Andrew de los Reyes
In-Reply-To: <1390863766-14491-1-git-send-email-miletus@chromium.org>
Hi,
On Mon, Jan 27, 2014 at 6:02 PM, Yufeng Shen <miletus@chromium.org> wrote:
> There is timeout error during initialization:
> kernel: [ 11.733104] hid-multitouch 0003:1870:0110.0001: usb_submit_urb(ctrl) failed: -1
> kernel: [ 11.734093] hid-multitouch 0003:1870:0110.0001: timeout initializing reports
>
> Adding quirk HID_QUIRK_NO_INIT_REPORTS can solve the problem.
Could you please test the quirk HID_QUIRK_NO_INIT_INPUT_REPORTS instead?
The patch is perfectly fine, but usually when it fails, only the input
reports are non-readable, whereas features should. This way, you will
still get the value of the features, which _may_ be sensible for the
maxcontact information.
BTW, HID_QUIRK_NO_INIT_INPUT_REPORTS has been introduced in v3.12 and
is the default for Win 8 certified devices.
Cheers,
Benjamin
>
> Signed-off-by: Yufeng Shen <miletus@chromium.org>
> ---
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/usbhid/hid-quirks.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index f9304cb..ece2997 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -636,6 +636,7 @@
>
> #define USB_VENDOR_ID_NEXIO 0x1870
> #define USB_DEVICE_ID_NEXIO_MULTITOUCH_420 0x010d
> +#define USB_DEVICE_ID_NEXIO_MULTITOUCH_PTI0750 0x0110
>
> #define USB_VENDOR_ID_NEXTWINDOW 0x1926
> #define USB_DEVICE_ID_NEXTWINDOW_TOUCHSCREEN 0x0003
> diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
> index 0db9a67..be5270a 100644
> --- a/drivers/hid/usbhid/hid-quirks.c
> +++ b/drivers/hid/usbhid/hid-quirks.c
> @@ -74,6 +74,7 @@ static const struct hid_blacklist {
> { USB_VENDOR_ID_FREESCALE, USB_DEVICE_ID_FREESCALE_MX28, HID_QUIRK_NOGET },
> { USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS, HID_QUIRK_NOGET },
> { USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GX680R_LED_PANEL, HID_QUIRK_NO_INIT_REPORTS },
> + { USB_VENDOR_ID_NEXIO, USB_DEVICE_ID_NEXIO_MULTITOUCH_PTI0750, HID_QUIRK_NO_INIT_REPORTS },
> { USB_VENDOR_ID_NOVATEK, USB_DEVICE_ID_NOVATEK_MOUSE, HID_QUIRK_NO_INIT_REPORTS },
> { USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN, HID_QUIRK_NO_INIT_REPORTS },
> { USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN1, HID_QUIRK_NO_INIT_REPORTS },
> --
> 1.8.5.3
>
> --
> 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
^ permalink raw reply
* Re: [RFC][PATCH] module: allow multiple calls to MODULE_DEVICE_TABLE() per module
From: Rusty Russell @ 2014-01-28 1:34 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tom Gundersen
Cc: linux-kernel, linux-input, kay, Dmitry Torokhov
In-Reply-To: <20140127195422.GB21018@kroah.com>
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Mon, Jan 27, 2014 at 08:09:55PM +0100, Tom Gundersen wrote:
>> Commit 78551277e4df5: "Input: i8042 - add PNP modaliases" had a bug, where the
>> second call to MODULE_DEVICE_TABLE() overrode the first resulting in not all
>> the modaliases being exposed.
>>
>> This fixes the problem by including the name of the device_id table in the
>> __mod_*_device_table alias, allowing us to export several device_id tables
>> per module.
>>
>> Suggested-by: Kay Sievers <kay@vrfy.org>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> ---
>> include/linux/module.h | 2 +-
>> scripts/mod/file2alias.c | 14 +++++++++-----
>> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> Ah, very nice, I've wanted this for a while now, it would make a number
> of different drivers much smaller and simpler to add new device ids to
> (no multiple lists of ids, one for the module loader and one for the
> sub-driver that is in the single file.)
You never asked :(
I've applied, this, but I'm actually amazed this patch works. C is
weird sometimes. It changes declarations of the form:
extern const struct pci_device_id __mod_pci_device_id_table
__attribute__ ((unused, alias("e1000_pci_tbl"));
Into:
extern const struct pci__e1000_pci_tbl_device_id __mod_pci__e1000_pci_tbl_device_id_table
__attribute__ ((unused, alias("e1000_pci_tbl"));
Now, that's a completely unknown type, but gcc doesn't care because it's
just an extern declaration. It does insert the alias, which is all we
care about.
We would normally use a special section for this, so it's mainly
historical. Now we have DEFINE_PCI_DEVICE_TABLE etc, we should
use those to put it in a special section (eg. "pci_ids") and
grab that directly.
Volunteers welcome :)
Rusty.
^ permalink raw reply
* Re: [RFC][PATCH] module: allow multiple calls to MODULE_DEVICE_TABLE() per module
From: Rusty Russell @ 2014-01-28 1:35 UTC (permalink / raw)
To: linux-kernel
Cc: linux-input, kay, Tom Gundersen, Dmitry Torokhov,
Greg Kroah-Hartman
In-Reply-To: <1390849795-2155-1-git-send-email-teg@jklm.no>
Tom Gundersen <teg@jklm.no> writes:
> Commit 78551277e4df5: "Input: i8042 - add PNP modaliases" had a bug, where the
> second call to MODULE_DEVICE_TABLE() overrode the first resulting in not all
> the modaliases being exposed.
No Signed-off-by?
Thanks,
Rusty.
>
> This fixes the problem by including the name of the device_id table in the
> __mod_*_device_table alias, allowing us to export several device_id tables
> per module.
>
> Suggested-by: Kay Sievers <kay@vrfy.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> ---
> include/linux/module.h | 2 +-
> scripts/mod/file2alias.c | 14 +++++++++-----
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 15cd6b1..7732d76 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -143,7 +143,7 @@ extern const struct gtype##_id __mod_##gtype##_table \
> #define MODULE_DESCRIPTION(_description) MODULE_INFO(description, _description)
>
> #define MODULE_DEVICE_TABLE(type,name) \
> - MODULE_GENERIC_TABLE(type##_device,name)
> + MODULE_GENERIC_TABLE(type##__##name##_device,name)
>
> /* Version of form [<epoch>:]<version>[-<extra-version>].
> Or for CVS/RCS ID version, everything but the number is stripped.
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 2370863..6778381 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -42,7 +42,7 @@ typedef unsigned char __u8;
>
> /* This array collects all instances that use the generic do_table */
> struct devtable {
> - const char *device_id; /* name of table, __mod_<name>_device_table. */
> + const char *device_id; /* name of table, __mod_<name>__*_device_table. */
> unsigned long id_size;
> void *function;
> };
> @@ -146,7 +146,8 @@ static void device_id_check(const char *modname, const char *device_id,
>
> if (size % id_size || size < id_size) {
> fatal("%s: sizeof(struct %s_device_id)=%lu is not a modulo "
> - "of the size of section __mod_%s_device_table=%lu.\n"
> + "of the size of "
> + "section __mod_%s__<identifier>_device_table=%lu.\n"
> "Fix definition of struct %s_device_id "
> "in mod_devicetable.h\n",
> modname, device_id, id_size, device_id, size, device_id);
> @@ -1206,7 +1207,7 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
> {
> void *symval;
> char *zeros = NULL;
> - const char *name;
> + const char *name, *identifier;
> unsigned int namelen;
>
> /* We're looking for a section relative symbol */
> @@ -1217,7 +1218,7 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
> if (ELF_ST_TYPE(sym->st_info) != STT_OBJECT)
> return;
>
> - /* All our symbols are of form <prefix>__mod_XXX_device_table. */
> + /* All our symbols are of form <prefix>__mod_<name>__<identifier>_device_table. */
> name = strstr(symname, "__mod_");
> if (!name)
> return;
> @@ -1227,7 +1228,10 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
> return;
> if (strcmp(name + namelen - strlen("_device_table"), "_device_table"))
> return;
> - namelen -= strlen("_device_table");
> + identifier = strstr(name, "__");
> + if (!identifier)
> + return;
> + namelen = identifier - name;
>
> /* Handle all-NULL symbols allocated into .bss */
> if (info->sechdrs[get_secindex(info, sym)].sh_type & SHT_NOBITS) {
> --
> 1.8.5.3
^ permalink raw reply
* Re: [PATCH v2] input synaptics-rmi4: PDT scan cleanup
From: Christopher Heiny @ 2014-01-28 2:28 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <20140127063038.GA32605@core.coreip.homeip.net>
On 01/26/2014 10:30 PM, Dmitry Torokhov wrote:> Hi Christopher,
Hi Dmitry,
Sorry if this reply is oddly formatted - I'm having some trouble with my
Thunderbird settings today.
>
> On Wed, Jan 22, 2014 at 04:56:09PM -0800, Christopher Heiny wrote:
>> Eliminates copy-paste code that handled scans of the Page Descriptor
>> Table, replacing it with a single PDT scan routine that invokes a
>> callback function. The scan routine is not static so that it can be
>> used by the firmware update code (under development, not yet submitted).
>>
>> Symbols that no longer needed to be public were moved into rmi_driver.c.
>>
>> Updated the copyright dates and eliminate C++ style comments while we
>> were at it.
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>
> I compared the 2 versions that you posted and I realized that losing
> error codes from lower levels by reducing them to RMI_SCAN_ERROR is not
> the best option, so I took your first patch as a basis and tried to
> rework it a bit so that we:
>
> - do not have special logic in generic scan routine regarding bootloader
> mode,
If an RMI4 device reaches the point of calling rmi_create_functions()
while still in bootloader mode, it means the main firmware is so broken
that a reflash is required in order to recover. To support the reflash,
you need to scan all of Page 0 and create the associated functions for
F34 and F01. But the patch as written will stop creating functions
after the first one it finds on Page 0, creating one of those, but not
the other.
Additionally, anything scanning the PDT when in bootloader mode (whether
it's the IRQ counter, the rmi_create_functions(), or the reflash module)
needs to stop after scanning Page 0. I think it's tidier to put that
check in rmi_scan_pdt_page(), rather than include it in every callback
implementation. See annotations below.
> and
>
> - do not leave with mutex locked in case of PDT read error.
>
> I also added start page address to PDT structure so we do not have to
> pass it around so much.
Hmmmm. Since we're no longer treating struct pdt_entry as a bitmapped
struct and doing reads directly into it, we should look into whether it
makes sense to merge struct pdt_entry and struct
rmi_function_descriptor. But that's something for later.
> Please let me know if the patch below makes sense to you (only compile
> tested, as always).
I had to apply the patch manually to my test tree because that contains
some other pending changes. I think that was done correctly, and with
the changes described below, it works.
Cheers,
Chris
>
> Thanks!
>
> Dmitry
>
> Input: input synaptics-rmi4 - PDT scan cleanup
>
> From: Christopher Heiny <cheiny@synaptics.com>
>
> Eliminates copy-paste code that handled scans of the Page Descriptor
Table,
> replacing it with a single PDT scan routine that invokes a callback
> function.
> The scan routine is not static so that it can be used by the firmware
> update
> code (under development, not yet submitted).
>
> Updated the copyright dates while we were at it.
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> --
> drivers/input/rmi4/rmi_driver.c | 258
> ++++++++++++++++++++-------------------
> drivers/input/rmi4/rmi_driver.h | 3 2 files changed, 132
> insertions(+), 129 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_driver.c
> b/drivers/input/rmi4/rmi_driver.c
> index c01a6b8..b9eb8a5 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2011-2013 Synaptics Incorporated
> + * Copyright (c) 2011-2014 Synaptics Incorporated
> * Copyright (c) 2011 Unixphere
> *
> * This driver provides the core support for a single RMI4-based
device.
> @@ -467,7 +467,8 @@ static int rmi_driver_reset_handler(struct
> rmi_device *rmi_dev)
> * Construct a function's IRQ mask. This should be called once and
> stored.
> */
> int rmi_driver_irq_get_mask(struct rmi_device *rmi_dev,
> - struct rmi_function *fn) {
> + struct rmi_function *fn)
> +{
> int i;
> struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
> @@ -491,12 +492,13 @@ int rmi_read_pdt_entry(struct rmi_device
> *rmi_dev, struct pdt_entry *entry,
> int error;
> error = rmi_read_block(rmi_dev, pdt_address, buf,
> RMI_PDT_ENTRY_SIZE);
> - if (error < 0) {
> + if (error) {
> dev_err(&rmi_dev->dev, "Read PDT entry at %#06x failed, code:
> %d.\n",
> pdt_address, error);
> return error;
> }
> + entry->page_start = pdt_address / RMI4_PAGE_SIZE;
> entry->query_base_addr = buf[0];
> entry->command_base_addr = buf[1];
> entry->control_base_addr = buf[2];
> @@ -509,27 +511,115 @@ int rmi_read_pdt_entry(struct rmi_device
> *rmi_dev, struct pdt_entry *entry,
> }
> EXPORT_SYMBOL_GPL(rmi_read_pdt_entry);
> -static void rmi_driver_copy_pdt_to_fd(struct pdt_entry *pdt,
> - struct rmi_function_descriptor *fd,
> - u16 page_start)
> +static void rmi_driver_copy_pdt_to_fd(const struct pdt_entry *pdt,
> + struct rmi_function_descriptor *fd)
> {
> - fd->query_base_addr = pdt->query_base_addr + page_start;
> - fd->command_base_addr = pdt->command_base_addr + page_start;
> - fd->control_base_addr = pdt->control_base_addr + page_start;
> - fd->data_base_addr = pdt->data_base_addr + page_start;
> + fd->query_base_addr = pdt->query_base_addr + pdt->page_start;
> + fd->command_base_addr = pdt->command_base_addr + pdt->page_start;
> + fd->control_base_addr = pdt->control_base_addr + pdt->page_start;
> + fd->data_base_addr = pdt->data_base_addr + pdt->page_start;
> fd->function_number = pdt->function_number;
> fd->interrupt_source_count = pdt->interrupt_source_count;
> fd->function_version = pdt->function_version;
> }
> -static int create_function(struct rmi_device *rmi_dev,
> - struct pdt_entry *pdt,
> - int *current_irq_count,
> - u16 page_start)
> +#define RMI_SCAN_CONTINUE 0
> +#define RMI_SCAN_DONE 1
> +
> +static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
> + int page,
> + void *ctx,
> + int (*callback)(struct rmi_device *rmi_dev,
> + void *ctx,
> + const struct pdt_entry *entry))
> +{
Add this for use below
struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
> + struct pdt_entry pdt_entry;
> + u16 page_start = RMI4_PAGE_SIZE * page;
> + u16 pdt_start = page_start + PDT_START_SCAN_LOCATION;
> + u16 pdt_end = page_start + PDT_END_SCAN_LOCATION;
> + u16 addr;
> + int error;
> + int retval;
> +
> + for (addr = pdt_start; addr >= pdt_end; addr -=
RMI_PDT_ENTRY_SIZE) {
> + error = rmi_read_pdt_entry(rmi_dev, &pdt_entry, addr);
> + if (error)
> + return error;
> +
> + if (RMI4_END_OF_PDT(pdt_entry.function_number))
> + break;
> +
> + dev_dbg(&rmi_dev->dev, "Found F%02X on page %#04x\n",
> + pdt_entry.function_number, page);
> +
> + retval = callback(rmi_dev, ctx, &pdt_entry);
> + if (retval != RMI_SCAN_CONTINUE)
> + return retval;
> + }
> +
> + return RMI_SCAN_CONTINUE;
Suggest instead to do:
return data->f01_bootloader_mode ? RMI_SCAN_DONE : RMI_SCAN_CONTINUE;
> +}
> +
> +static int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
> + int (*callback)(struct rmi_device *rmi_dev,
> + void *ctx,
> + const struct pdt_entry *entry))
> +{
> + struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
> + int page;
> + int retval = RMI_SCAN_DONE;
> +
> + /*
> + * TODO: With F01 and reflash as part of the core now, is this
> + * lock still required?
> + */
> + mutex_lock(&data->pdt_mutex);
> +
> + for (page = 0; page <= RMI4_MAX_PAGE; page++) {
> + retval = rmi_scan_pdt_page(rmi_dev, page, ctx, callback);
> + if (retval != RMI_SCAN_CONTINUE)
> + break;
> + }
> +
> + mutex_unlock(&data->pdt_mutex);
> +
> + return retval < 0 ? retval : 0;
> +}
> +
> +static int rmi_initial_reset(struct rmi_device *rmi_dev,
> + void *ctx, const struct pdt_entry *pdt)
> +{
> + int error;
> +
> + if (pdt->function_number == 0x01) {
> + u16 cmd_addr = pdt->page_start + pdt->command_base_addr;
> + u8 cmd_buf = RMI_DEVICE_RESET_CMD;
> + const struct rmi_device_platform_data *pdata =
> + to_rmi_platform_data(rmi_dev);
> +
> + error = rmi_write_block(rmi_dev, cmd_addr, &cmd_buf, 1);
> + if (error) {
> + dev_err(&rmi_dev->dev,
> + "Initial reset failed. Code = %d.\n", error);
> + return error;
> + }
> +
> + mdelay(pdata->reset_delay_ms);
> +
> + return RMI_SCAN_DONE;
> + }
> +
> + /* F01 should always be on page 0. If we don't find it there,
fail. */
> + return pdt->page_start == 0 ? RMI_SCAN_CONTINUE : -ENODEV;
> +}
> +
> +static int rmi_create_function(struct rmi_device *rmi_dev,
> + void *ctx, const struct pdt_entry *pdt)
> {
> struct device *dev = &rmi_dev->dev;
> struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
> struct rmi_device_platform_data *pdata =
> to_rmi_platform_data(rmi_dev);
> + int *current_irq_count = ctx;
> struct rmi_function *fn;
> int error;
> @@ -551,7 +641,7 @@ static int create_function(struct rmi_device
> *rmi_dev,
> fn->irq_pos = *current_irq_count;
> *current_irq_count += fn->num_of_irqs;
> - rmi_driver_copy_pdt_to_fd(pdt, &fn->fd, page_start);
> + rmi_driver_copy_pdt_to_fd(pdt, &fn->fd);
> error = rmi_register_function(fn);
> if (error)
> @@ -559,131 +649,38 @@ static int create_function(struct rmi_device
> *rmi_dev,
> list_add_tail(&fn->node, &data->function_list);
> - return 0;
> + return data->f01_bootloader_mode ? RMI_SCAN_DONE :
RMI_SCAN_CONTINUE;
As described previously, this is a bug.
> err_free_mem:
> kfree(fn);
> return error;
> }
> -/*
> - * Scan the PDT for F01 so we can force a reset before anything else
> - * is done. This forces the sensor into a known state, and also
> - * forces application of any pending updates from reflashing the
> - * firmware or configuration.
> - *
> - * We have to do this before actually building the PDT because the
reflash
> - * updates (if any) might cause various registers to move around.
> - */
> -static int rmi_initial_reset(struct rmi_device *rmi_dev)
> +static int rmi_create_functions(struct rmi_device *rmi_dev)
> {
> - struct pdt_entry pdt_entry;
> - int page;
> - struct device *dev = &rmi_dev->dev;
> - bool done = false;
> - bool has_f01 = false;
> - int i;
> - int retval;
> - const struct rmi_device_platform_data *pdata =
> - to_rmi_platform_data(rmi_dev);
> -
> - dev_dbg(dev, "Initial reset.\n");
> -
> - for (page = 0; (page <= RMI4_MAX_PAGE) && !done; page++) {
> - u16 page_start = RMI4_PAGE_SIZE * page;
> - u16 pdt_start = page_start + PDT_START_SCAN_LOCATION;
> - u16 pdt_end = page_start + PDT_END_SCAN_LOCATION;
> -
> - done = true;
> - for (i = pdt_start; i >= pdt_end; i -= RMI_PDT_ENTRY_SIZE) {
> - retval = rmi_read_pdt_entry(rmi_dev, &pdt_entry, i);
> - if (retval < 0)
> - return retval;
> -
> - if (RMI4_END_OF_PDT(pdt_entry.function_number))
> - break;
> - done = false;
> -
> - if (pdt_entry.function_number == 0x01) {
> - u16 cmd_addr = page_start +
> - pdt_entry.command_base_addr;
> - u8 cmd_buf = RMI_DEVICE_RESET_CMD;
> - retval = rmi_write_block(rmi_dev, cmd_addr,
> - &cmd_buf, 1);
> - if (retval < 0) {
> - dev_err(dev, "Initial reset failed. Code = %d.\n",
> - retval);
> - return retval;
> - }
> - mdelay(pdata->reset_delay_ms);
> - done = true;
> - has_f01 = true;
> - break;
> - }
> - }
> - }
> -
> - if (!has_f01) {
> - dev_warn(dev, "WARNING: Failed to find F01 for initial
reset.\n");
> - return -ENODEV;
> - }
> -
> - return 0;
> -}
> -
> -static int rmi_scan_pdt(struct rmi_device *rmi_dev)
> -{
> - struct rmi_driver_data *data;
> - struct pdt_entry pdt_entry;
> - int page;
> - struct device *dev = &rmi_dev->dev;
> + struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
> int irq_count = 0;
> - bool done = false;
> - int i;
> int retval;
> - dev_dbg(dev, "Scanning PDT...\n");
> -
> - data = dev_get_drvdata(&rmi_dev->dev);
> - mutex_lock(&data->pdt_mutex);
> -
> - for (page = 0; (page <= RMI4_MAX_PAGE) && !done; page++) {
> - u16 page_start = RMI4_PAGE_SIZE * page;
> - u16 pdt_start = page_start + PDT_START_SCAN_LOCATION;
> - u16 pdt_end = page_start + PDT_END_SCAN_LOCATION;
> -
> - done = true;
> - for (i = pdt_start; i >= pdt_end; i -= RMI_PDT_ENTRY_SIZE) {
> - retval = rmi_read_pdt_entry(rmi_dev, &pdt_entry, i);
> - if (retval < 0)
> - goto error_exit;
> -
> - if (RMI4_END_OF_PDT(pdt_entry.function_number))
> - break;
> -
> - dev_dbg(dev, "Found F%02X on page %#04x\n",
> - pdt_entry.function_number, page);
> - done = false;
> -
> - // XXX need to make sure we create F01 first...
> - retval = create_function(rmi_dev,
> - &pdt_entry, &irq_count, page_start);
> + /*
> + * XXX need to make sure we create F01 first...
> + * XXX or do we? It might not be required in the updated structure.
> + */
> + retval = rmi_scan_pdt(rmi_dev, &irq_count, rmi_create_function);
> + if (retval < 0)
> + return retval;
> - if (retval)
> - goto error_exit;
> - }
> - done = done || data->f01_bootloader_mode;
> - }
> + /*
> + * TODO: I think we need to count the IRQs before creating the
> + * functions.
> + */
> data->irq_count = irq_count;
> data->num_of_irq_regs = (irq_count + 7) / 8;
> - dev_dbg(dev, "%s: Done with PDT scan.\n", __func__);
> - retval = 0;
> -error_exit:
> - mutex_unlock(&data->pdt_mutex);
> - return retval;
> + return 0;
> }
> +
> #ifdef CONFIG_PM_SLEEP
> static int rmi_driver_suspend(struct device *dev)
> {
> @@ -797,10 +794,15 @@ static int rmi_driver_probe(struct device *dev)
> /*
> * Right before a warm boot, the sensor might be in some unusual
> state,
> - * such as F54 diagnostics, or F34 bootloader mode. In order to
clear
> - * the sensor to a known state, we issue a initial reset to
clear any
> + * such as F54 diagnostics, or F34 bootloader mode after a firmware
> + * or configuration update. In order to clear the sensor to a known
> + * state and/or apply any updates, we issue a initial reset to
> clear any
> * previous settings and force it into normal operation.
> *
> + * We have to do this before actually building the PDT because
> + * the reflash updates (if any) might cause various registers to
move
> + * around.
> + *
> * For a number of reasons, this initial reset may fail to return
> * within the specified time, but we'll still be able to bring
up the
> * driver normally after that failure. This occurs most
commonly in
> @@ -813,11 +815,11 @@ static int rmi_driver_probe(struct device *dev)
> */
> if (!pdata->reset_delay_ms)
> pdata->reset_delay_ms = DEFAULT_RESET_DELAY_MS;
> - retval = rmi_initial_reset(rmi_dev);
> + retval = rmi_scan_pdt(rmi_dev, NULL, rmi_initial_reset);
> if (retval)
> dev_warn(dev, "RMI initial reset failed! Continuing in spite
> of this.\n");
> - retval = rmi_scan_pdt(rmi_dev);
> + retval = rmi_create_functions(rmi_dev);
> if (retval) {
> dev_err(dev, "PDT scan for %s failed with code %d.\n",
> pdata->sensor_name, retval);
> diff --git a/drivers/input/rmi4/rmi_driver.h
> b/drivers/input/rmi4/rmi_driver.h
> index 4f44a54..9ecd31b 100644
> --- a/drivers/input/rmi4/rmi_driver.h
> +++ b/drivers/input/rmi4/rmi_driver.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2011-2013 Synaptics Incorporated
> + * Copyright (c) 2011-2014 Synaptics Incorporated
> * Copyright (c) 2011 Unixphere
> *
> * This program is free software; you can redistribute it and/or
> modify it
> @@ -94,6 +94,7 @@ struct rmi_driver_data {
> #define RMI4_END_OF_PDT(id) ((id) == 0x00 || (id) == 0xff)
> struct pdt_entry {
> + u16 page_start;
> u8 query_base_addr;
> u8 command_base_addr;
> u8 control_base_addr;
^ 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