* Re: [PATCH 1/2] input: ti_tsc: Enable shared IRQ for TSC
From: Dmitry Torokhov @ 2013-08-05 16:12 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Zubair Lutfullah, jic23, linux-iio, linux-kernel, linux-input,
gregkh, Russ.Dill
In-Reply-To: <51FE361B.5030900@kernel.org>
On Sun, Aug 04, 2013 at 12:08:11PM +0100, Jonathan Cameron wrote:
> On 07/27/13 00:51, Zubair Lutfullah wrote:
> > From: "Patil, Rachna" <rachna@ti.com>
> >
> > Touchscreen and ADC share the same IRQ line from parent MFD core.
> > Previously only Touchscreen was interrupt based.
> > With continuous mode support added in ADC driver, driver requires
> > interrupt to process the ADC samples, so enable shared IRQ flag bit for
> > touchscreen.
> >
> > Signed-off-by: Patil, Rachna <rachna@ti.com>
> > Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
> > Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> I'd rather this went via input independent of the other patch
> (if not there all that will happen is one or other driver will
> fail to probe if both are attempted?)
>
> Can take it through IIO but only with a Dmitry Ack.
>
> > ---
> > drivers/input/touchscreen/ti_am335x_tsc.c | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> > index e1c5300..68d1250 100644
> > --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> > +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> > @@ -260,8 +260,18 @@ static irqreturn_t titsc_irq(int irq, void *dev)
> > unsigned int fsm;
> >
> > status = titsc_readl(ts_dev, REG_IRQSTATUS);
> > - if (status & IRQENB_FIFO0THRES) {
> > -
> > + /*
> > + * ADC and touchscreen share the IRQ line.
> > + * FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
> > + * interrupts are used by ADC,
> > + * hence return from touchscreen IRQ handler if FIFO1
> > + * related interrupts occurred.
> > + */
> > + if ((status & IRQENB_FIFO1THRES) ||
> > + (status & IRQENB_FIFO1OVRRUN) ||
> > + (status & IRQENB_FIFO1UNDRFLW))
> > + return IRQ_NONE;
> > + else if (status & IRQENB_FIFO0THRES) {
What happens if both parts have data at the same time? Can both
IRQENB_FIFO1THRES and IRQENB_FIFO0THRES be signalled? What will happen
in this case?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] HID: hid-logitech-dj, querying_devices was never set
From: Benjamin Tissoires @ 2013-08-05 14:40 UTC (permalink / raw)
To: Jiri Kosina
Cc: Nestor Lopez Casado, Andrew de los Reyes, joseph.salisbury,
linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <alpine.LNX.2.00.1308051516181.17512@pobox.suse.cz>
On Mon, Aug 5, 2013 at 3:22 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Fri, 2 Aug 2013, Benjamin Tissoires wrote:
>
>> > Could you please elaborate? (and put an elaborate description to revert
>> > commit log perhaps?)
>>
>> Sure, so here is the revert commit log:
>>
>> --
>>
>> Commit "HID: hid-logitech-dj, querying_devices was never set" activate
>> a flag which guarantees that we do not ask the receiver for too many
>> enumeration. When the flag is set, each following enumeration call is
>> discarded (the usb request is not forwarded to the receiver). The flag
>> is then released when the driver receive a pairing information event,
>> which normally follows the enumeration request.
>> However, the USB3 bug makes the driver think the enumeration request
>> has been forwarded to the receiver. However, it is actually not the
>> case because the USB stack returns -EPIPE. So, when a new unknown
>> device appears, the workaround consisting in asking for a new
>> enumeration is not working anymore: this new enumeration is discarded
>> because of the flag, which is never reset.
>>
>> A solution could be to trigger a timeout before releasing it, but for
>> now, let's just revert the patch.
>>
>> --
>
> Thanks Benjamin.
>
> I'd like to have a bit more clarification about the USB3 bug, as this
> whole issue is not completely clear to me.
>
> To be more specific -- when exactly do we receive -EPIPE, why do we
> receive it and why do we not handle it properly?
Sure, I'll try (though the more I think of it, the more it seems
blurry to me :( ).
So the initial probe function in hid-logitech-dj was implemented by
using a direct call to hid_output_raw_report(). This call was
synchronous, so we did get the -EPIPE return code. Then, the probe()
function returns the -EPIPE error, cleaning the receiver and
unregister it from the hid bus.
However, now, we use hid_hw_request(), which is asynchronous (from
what I can read). At least, this code returns "void" as the set_report
command seems to be scheduled for later handling. In usbhid, when the
queue is flushed, I did not found a way to retrieve the error code...
So basically, the -EPIPE is received in usbhid_restart_ctrl_queue(),
but nothing notifies hid-logitech-dj from the error. In the end, the
probe() function returns without error code, but the receiver never
received the notification.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 2/2] HID: hid-logitech-dj, querying_devices was never set
From: Jiri Kosina @ 2013-08-05 13:22 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Nestor Lopez Casado, Andrew de los Reyes, joseph.salisbury,
linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=FV=OjnktkET-_KM_iTPStsWCKdj8fpyktnJczusFz0=w@mail.gmail.com>
On Fri, 2 Aug 2013, Benjamin Tissoires wrote:
> > Could you please elaborate? (and put an elaborate description to revert
> > commit log perhaps?)
>
> Sure, so here is the revert commit log:
>
> --
>
> Commit "HID: hid-logitech-dj, querying_devices was never set" activate
> a flag which guarantees that we do not ask the receiver for too many
> enumeration. When the flag is set, each following enumeration call is
> discarded (the usb request is not forwarded to the receiver). The flag
> is then released when the driver receive a pairing information event,
> which normally follows the enumeration request.
> However, the USB3 bug makes the driver think the enumeration request
> has been forwarded to the receiver. However, it is actually not the
> case because the USB stack returns -EPIPE. So, when a new unknown
> device appears, the workaround consisting in asking for a new
> enumeration is not working anymore: this new enumeration is discarded
> because of the flag, which is never reset.
>
> A solution could be to trigger a timeout before releasing it, but for
> now, let's just revert the patch.
>
> --
Thanks Benjamin.
I'd like to have a bit more clarification about the USB3 bug, as this
whole issue is not completely clear to me.
To be more specific -- when exactly do we receive -EPIPE, why do we
receive it and why do we not handle it properly?
Thanks again,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] hid: add driver for IBM/Lenovo ScrollPoint mice
From: Jiri Kosina @ 2013-08-05 12:00 UTC (permalink / raw)
To: Peter De Wachter; +Cc: linux-input, linux-kernel, Bernhard Seibold
In-Reply-To: <1372804951-7135-1-git-send-email-pdewacht@gmail.com>
[ Bernhard added to CC ]
On Wed, 3 Jul 2013, Peter De Wachter wrote:
> IBM Scrollpoint have a trackpoint-like gizmo instead of a scroll wheel,
> which allows for two-dimensional scrolling. This driver provides an
> input mapping to make that work.
>
> The scrollpoint is also much more sensitive than a typical mouse wheel.
> X can use this to provide smooth scrolling, but the kernel doesn't seem
> to have a way to communicate this fact. As a result this mouse simply
> scrolls way too fast. For now I've created an xorg patch to provide a
> manual setting (fdo #48118).
>
> Signed-off-by: Peter De Wachter <pdewacht@gmail.com>
Hi,
thanks for the driver.
I'd like to ask you to put a few more effort into this, unifying this a
little bit more with the existing driver for Lenovo devices we aready have
in the tree -- hid-lenovo-tpkbd.
As most of the drivers are divided on a per-vendor basis anyway, what I'd
probably like to see the most would be:
- drop HID_LENOVO_TPKBD (*)
- create HID_LENOVO
- rename your driver to hid-lenovo-scrollpoint
- build both hid-lenovo-tpkbd and hid-lenovo-scrollpoint when HID_LENOVO
is set
(*) or keep it for compatibility reasons, but make it just an alias for
CONFIG_HID_LENOVO.
Sounds feasible?
> ---
> drivers/hid/Kconfig | 8 ++++++
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-core.c | 7 +++++
> drivers/hid/hid-ids.h | 7 +++++
> drivers/hid/hid-scrollpoint.c | 67 +++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 90 insertions(+)
> create mode 100644 drivers/hid/hid-scrollpoint.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index fb52f3f..fdc49ca 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -593,6 +593,14 @@ config HID_SAMSUNG
> ---help---
> Support for Samsung InfraRed remote control or keyboards.
>
> +config HID_SCROLLPOINT
> + tristate "IBM/Lenovo ScrollPoint mice"
> + depends on HID
> + ---help---
> + Support for IBM/Lenovo ScrollPoint mice.
> + Say Y here if you have a ScrollPoint mouse and want horizontal
> + scrolling to work.
> +
> config HID_SONY
> tristate "Sony PS3 controller"
> depends on USB_HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 2065694..4a4c279 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_HID_ROCCAT) += hid-roccat.o hid-roccat-common.o \
> hid-roccat-lua.o hid-roccat-pyra.o hid-roccat-savu.o
> obj-$(CONFIG_HID_SAITEK) += hid-saitek.o
> obj-$(CONFIG_HID_SAMSUNG) += hid-samsung.o
> +obj-$(CONFIG_HID_SCROLLPOINT) += hid-scrollpoint.o
> obj-$(CONFIG_HID_SMARTJOYPLUS) += hid-sjoy.o
> obj-$(CONFIG_HID_SONY) += hid-sony.o
> obj-$(CONFIG_HID_SPEEDLINK) += hid-speedlink.o
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 264f550..003f419 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1584,6 +1584,13 @@ static const struct hid_device_id hid_have_special_driver[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_3) },
> { HID_USB_DEVICE(USB_VENDOR_ID_HOLTEK, USB_DEVICE_ID_HOLTEK_ON_LINE_GRIP) },
> { HID_USB_DEVICE(USB_VENDOR_ID_HOLTEK_ALT, USB_DEVICE_ID_HOLTEK_ALT_KEYBOARD) },
> +#if IS_ENABLED(CONFIG_HID_SCROLLPOINT)
> + { HID_USB_DEVICE(USB_VENDOR_ID_IBM, USB_DEVICE_ID_IBM_SCROLLPOINT_III) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_IBM, USB_DEVICE_ID_IBM_SCROLLPOINT_PRO) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_IBM, USB_DEVICE_ID_IBM_SCROLLPOINT_OPTICAL) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_IBM, USB_DEVICE_ID_IBM_SCROLLPOINT_800DPI_OPTICAL) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_IBM, USB_DEVICE_ID_IBM_SCROLLPOINT_800DPI_OPTICAL_PRO) },
> +#endif
> { HID_USB_DEVICE(USB_VENDOR_ID_JESS2, USB_DEVICE_ID_JESS2_COLOR_RUMBLE_PAD) },
> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ION, USB_DEVICE_ID_ICADE) },
> { HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 38535c9..f813fab 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -441,6 +441,13 @@
> #define USB_VENDOR_ID_HOLTEK_ALT 0x04d9
> #define USB_DEVICE_ID_HOLTEK_ALT_KEYBOARD 0xa055
>
> +#define USB_VENDOR_ID_IBM 0x04b3
> +#define USB_DEVICE_ID_IBM_SCROLLPOINT_III 0x3100
> +#define USB_DEVICE_ID_IBM_SCROLLPOINT_PRO 0x3103
> +#define USB_DEVICE_ID_IBM_SCROLLPOINT_OPTICAL 0x3105
> +#define USB_DEVICE_ID_IBM_SCROLLPOINT_800DPI_OPTICAL 0x3108
> +#define USB_DEVICE_ID_IBM_SCROLLPOINT_800DPI_OPTICAL_PRO 0x3109
> +
> #define USB_VENDOR_ID_IMATION 0x0718
> #define USB_DEVICE_ID_DISC_STAKKA 0xd000
>
> diff --git a/drivers/hid/hid-scrollpoint.c b/drivers/hid/hid-scrollpoint.c
> new file mode 100644
> index 0000000..48e7696
> --- /dev/null
> +++ b/drivers/hid/hid-scrollpoint.c
> @@ -0,0 +1,67 @@
> +/*
> + * HID driver for IBM/Lenovo ScrollPoint mice
> + *
> + * Copyright (c) 2012 Peter De Wachter <pdewacht@gmail.com>
> + */
> +
> +/*
> + * 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/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +
> +#include "hid-ids.h"
> +
> +static int scrollpoint_input_mapping(struct hid_device *hdev,
> + struct hid_input *hi, struct hid_field *field,
> + struct hid_usage *usage, unsigned long **bit, int *max)
> +{
> + if (usage->hid == HID_GD_Z) {
> + hid_map_usage(hi, usage, bit, max, EV_REL, REL_HWHEEL);
> + return 1;
> + }
> + return 0;
> +}
> +
> +static const struct hid_device_id scrollpoint_devices[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_IBM,
> + USB_DEVICE_ID_IBM_SCROLLPOINT_III) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_IBM,
> + USB_DEVICE_ID_IBM_SCROLLPOINT_PRO) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_IBM,
> + USB_DEVICE_ID_IBM_SCROLLPOINT_OPTICAL) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_IBM,
> + USB_DEVICE_ID_IBM_SCROLLPOINT_800DPI_OPTICAL) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_IBM,
> + USB_DEVICE_ID_IBM_SCROLLPOINT_800DPI_OPTICAL_PRO) },
> + { }
> +};
> +MODULE_DEVICE_TABLE(hid, scrollpoint_devices);
> +
> +static struct hid_driver scrollpoint_driver = {
> + .name = "scrollpoint",
> + .id_table = scrollpoint_devices,
> + .input_mapping = scrollpoint_input_mapping
> +};
> +
> +static int __init scrollpoint_init(void)
> +{
> + return hid_register_driver(&scrollpoint_driver);
> +}
> +
> +static void __exit scrollpoint_exit(void)
> +{
> + hid_unregister_driver(&scrollpoint_driver);
> +}
> +
> +module_init(scrollpoint_init);
> +module_exit(scrollpoint_exit);
> +
> +MODULE_AUTHOR("Peter De Wachter");
> +MODULE_DESCRIPTION("IBM/Lenovo ScrollPoint mouse driver");
> +MODULE_LICENSE("GPL");
> --
> 1.8.3.2
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: wiimote: work around broken DRM_KAI on GEN10
From: Jiri Kosina @ 2013-08-05 11:46 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Nicolas Adenis-Lamarre
In-Reply-To: <1375635010-2468-1-git-send-email-dh.herrmann@gmail.com>
On Sun, 4 Aug 2013, David Herrmann wrote:
> GEN10 and earlier devices seem to not support DRM_KAI if we run in basic
> IR mode. Use DRM_KAIE instead. This might increases overhead slightly as
> the extension port is read and streamed but we stream accelerometer data
> constantly, too, so this is negligible.
>
> Note that our parsers are hardcoded on IR-formats, so we cannot actually
> use 96-bit IR DRMs for basic IR data. We would have to adjust the parsers.
> But as only GEN20 and newer support this, we simply avoid mixed DRMs.
>
> This fixes a bug where GEN10 devices didn't provide IR data if
> accelerometer and IR are enabled simultaneously. As a workaround, you can
> enable DRM_KAIE without this patch via (disables device power-management):
> echo "37" >/sys/kernel/debug/hid/<dev>/drm
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> Reported-by: Nicolas Adenis-Lamarre <nicolas.adenis.lamarre@gmail.com>
Applied, thanks.
> ---
> drivers/hid/hid-wiimote-core.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 0c06054..6602098 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -212,10 +212,12 @@ static __u8 select_drm(struct wiimote_data *wdata)
>
> if (ir == WIIPROTO_FLAG_IR_BASIC) {
> if (wdata->state.flags & WIIPROTO_FLAG_ACCEL) {
> - if (ext)
> - return WIIPROTO_REQ_DRM_KAIE;
> - else
> - return WIIPROTO_REQ_DRM_KAI;
> + /* GEN10 and ealier devices bind IR formats to DRMs.
> + * Hence, we cannot use DRM_KAI here as it might be
> + * bound to IR_EXT. Use DRM_KAIE unconditionally so we
> + * work with all devices and our parsers can use the
> + * fixed formats, too. */
> + return WIIPROTO_REQ_DRM_KAIE;
> } else {
> return WIIPROTO_REQ_DRM_KIE;
> }
> --
> 1.8.3.4
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH][v3.2.y][v3.5.y][3.8.y] Input: elantech - fix for newer hardware versions (v7)
From: Luis Henriques @ 2013-08-05 10:57 UTC (permalink / raw)
To: Joseph Salisbury
Cc: Ben Hutchings, Kamal Mostafa, dmitry.torokhov, rydberg,
kendatsuba, linux-input, stable, linux-kernel, Kernel Team
In-Reply-To: <51E9B4A1.3080205@canonical.com>
Joseph Salisbury <joseph.salisbury@canonical.com> writes:
> Hello,
>
> Please consider including upstream commit
> 9eebed7de660c0b5ab129a9de4f89d20b60de68c in the next v3.2.y, v3.5.y and
> v3.8.y releases.
>
> It was included upstream as of v3.11-rc1. It has been tested and
> confirmed to resolve http://bugs.launchpad.net/bugs/1166442 .
>
> commit 9eebed7de660c0b5ab129a9de4f89d20b60de68c
> Author: Matteo Delfino <kendatsuba@gmail.com>
> Date: Sat Jul 6 21:52:26 2013 -0700
>
> Input: elantech - fix for newer hardware versions (v7)
>
>
> Sincerely,
> Joseph Salisbury
>
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks, I'm queuing it for the 3.5 kernel.
Cheers,
--
Luis
^ permalink raw reply
* Re: [PATCH] hid-holtekff: don't push static constants on stack for %*ph
From: Jiri Kosina @ 2013-08-05 9:30 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-input
In-Reply-To: <1375441680-13593-1-git-send-email-andriy.shevchenko@linux.intel.com>
On Fri, 2 Aug 2013, Andy Shevchenko wrote:
> There is no need to pass constants via stack. The width may be explicitly
> specified in the format.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/hid/hid-holtekff.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-holtekff.c b/drivers/hid/hid-holtekff.c
> index 9a8f051..9325545 100644
> --- a/drivers/hid/hid-holtekff.c
> +++ b/drivers/hid/hid-holtekff.c
> @@ -98,7 +98,7 @@ static void holtekff_send(struct holtekff_device *holtekff,
> holtekff->field->value[i] = data[i];
> }
>
> - dbg_hid("sending %*ph\n", 7, data);
> + dbg_hid("sending %7ph\n", data);
>
> hid_hw_request(hid, holtekff->field->report, HID_REQ_SET_REPORT);
Applied, thanks Andy.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'
From: Jiri Kosina @ 2013-08-05 9:26 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Andy Shevchenko, linux-input, Mika Westerberg
In-Reply-To: <51FBFDA0.4010205@redhat.com>
On Fri, 2 Aug 2013, Benjamin Tissoires wrote:
> > With only one condition if dynamic debug is enabled in kernel.
> > So, it would be nice to gather opinions, however, the decision is
> > totally depends on type of user who wants to debug the module.
> >
> Yep, I agree that Jiri's opinion would be helpful.
>
> Meanwhile, we could also wait a little for more i2c-hid to hit the
> market and to be widely tested, and then remove the debug flag. We
> should also remove the debug events in get_i2c_hid_get_input() which
> would pollute systems without dynamic debugging but with the DEBUG
> config still enabled.
Well, it doesn't seem to make too much sense to me to have generic/bus use
hid_dbg() (although it's used very rarely) and a "sub-driver" use
something else.
Basically the options I see:
- keep i2c-hid as is
- convert the whole drivers/hid to dev_dbg()
- introduce another hid debugfs file in parallel to rdesc and events for
these types of messages
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 1/2] i2c-hid: don't push static constants on stack for %*ph
From: Jiri Kosina @ 2013-08-05 9:21 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Andy Shevchenko, linux-input, Mika Westerberg, Benjamin Tissoires
In-Reply-To: <CAN+gG=FQqAGV1ykq9RBzwkbp6vLAJwLP1L1y76NcyUP_iYZ6cQ@mail.gmail.com>
On Fri, 2 Aug 2013, Benjamin Tissoires wrote:
> <andriy.shevchenko@linux.intel.com> wrote:
> > There is no need to pass constants via stack. The width may be explicitly
> > specified in the format.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH 4/4] Input: pwm-beeper: add CONFIG_PM_SLEEP to suspend/resume
From: Jingoo Han @ 2013-08-05 6:57 UTC (permalink / raw)
To: 'Dmitry Torokhov'
Cc: 'Dmitry Torokhov', linux-input, Jingoo Han
Add CONFIG_PM_SLEEP to suspend/resume functions to fix the build
warnings when CONFIG_PM_SLEEP is not selected. This is because
sleep PM callbacks defined by SET_SYSTEM_SLEEP_PM_OPS are only used
when the CONFIG_PM_SLEEP is enabled.
drivers/input/misc/pwm-beeper.c:147:12: warning: 'pwm_beeper_suspend' defined but not used [-Wunused-function]
drivers/input/misc/pwm-beeper.c:157:12: warning: 'pwm_beeper_resume' defined but not used [-Wunused-function]
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/input/misc/pwm-beeper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index a37f0c9..2ff4d1c 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -143,7 +143,7 @@ static int pwm_beeper_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
static int pwm_beeper_suspend(struct device *dev)
{
struct pwm_beeper *beeper = dev_get_drvdata(dev);
--
1.7.10.4
^ permalink raw reply related
* [PATCH 3/4] Input: eeti_ts: add CONFIG_PM_SLEEP to suspend/resume
From: Jingoo Han @ 2013-08-05 6:56 UTC (permalink / raw)
To: 'Dmitry Torokhov'
Cc: 'Dmitry Torokhov', linux-input, Jingoo Han
Add CONFIG_PM_SLEEP to suspend/resume functions to fix the build
warnings when CONFIG_PM_SLEEP is not selected. This is because
sleep PM callbacks defined by SET_SYSTEM_SLEEP_PM_OPS are only used
when the CONFIG_PM_SLEEP is enabled.
drivers/input/touchscreen/eeti_ts.c:268:12: warning: 'eeti_ts_suspend' defined but not used [-Wunused-function]
drivers/input/touchscreen/eeti_ts.c:287:12: warning: 'eeti_ts_resume' defined but not used [-Wunused-function]
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/input/touchscreen/eeti_ts.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/eeti_ts.c b/drivers/input/touchscreen/eeti_ts.c
index 8fe5086..a6453b1 100644
--- a/drivers/input/touchscreen/eeti_ts.c
+++ b/drivers/input/touchscreen/eeti_ts.c
@@ -264,7 +264,7 @@ static int eeti_ts_remove(struct i2c_client *client)
return 0;
}
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
static int eeti_ts_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -315,7 +315,7 @@ MODULE_DEVICE_TABLE(i2c, eeti_ts_id);
static struct i2c_driver eeti_ts_driver = {
.driver = {
.name = "eeti_ts",
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
.pm = &eeti_ts_pm,
#endif
},
--
1.7.10.4
^ permalink raw reply related
* [PATCH 2/4] Input: cy8ctmg110_ts: add CONFIG_PM_SLEEP to suspend/resume
From: Jingoo Han @ 2013-08-05 6:56 UTC (permalink / raw)
To: 'Dmitry Torokhov'
Cc: 'Dmitry Torokhov', linux-input, Jingoo Han
Add CONFIG_PM_SLEEP to suspend/resume functions to fix the build
warnings when CONFIG_PM_SLEEP is not selected. This is because
sleep PM callbacks defined by SET_SYSTEM_SLEEP_PM_OPS are only used
when the CONFIG_PM_SLEEP is enabled.
drivers/input/touchscreen/cy8ctmg110_ts.c:295:12: warning: 'cy8ctmg110_suspend' defined but not used [-Wunused-function]
drivers/input/touchscreen/cy8ctmg110_ts.c:309:12: warning: 'cy8ctmg110_resume' defined but not used [-Wunused-function]
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/input/touchscreen/cy8ctmg110_ts.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/cy8ctmg110_ts.c b/drivers/input/touchscreen/cy8ctmg110_ts.c
index 96e0eedc..093c274 100644
--- a/drivers/input/touchscreen/cy8ctmg110_ts.c
+++ b/drivers/input/touchscreen/cy8ctmg110_ts.c
@@ -291,7 +291,7 @@ err_free_mem:
return err;
}
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
static int cy8ctmg110_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -351,7 +351,7 @@ static struct i2c_driver cy8ctmg110_driver = {
.driver = {
.owner = THIS_MODULE,
.name = CY8CTMG110_DRIVER_NAME,
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
.pm = &cy8ctmg110_pm,
#endif
},
--
1.7.10.4
^ permalink raw reply related
* [PATCH 1/4] Input: max7359: add CONFIG_PM_SLEEP to suspend/resume
From: Jingoo Han @ 2013-08-05 6:55 UTC (permalink / raw)
To: 'Dmitry Torokhov'
Cc: 'Dmitry Torokhov', linux-input, 'Jingoo Han'
Add CONFIG_PM_SLEEP to suspend/resume functions to fix the build
warnings when CONFIG_PM_SLEEP is not selected. This is because
sleep PM callbacks defined by SET_SYSTEM_SLEEP_PM_OPS are only used
when the CONFIG_PM_SLEEP is enabled.
drivers/input/keyboard/max7359_keypad.c:275:12: warning: 'max7359_suspend' defined but not used [-Wunused-function]
drivers/input/keyboard/max7359_keypad.c:287:12: warning: 'max7359_resume' defined but not used [-Wunused-function]
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/input/keyboard/max7359_keypad.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/max7359_keypad.c b/drivers/input/keyboard/max7359_keypad.c
index 7c7af2b..bc2cdaf 100644
--- a/drivers/input/keyboard/max7359_keypad.c
+++ b/drivers/input/keyboard/max7359_keypad.c
@@ -271,7 +271,7 @@ static int max7359_remove(struct i2c_client *client)
return 0;
}
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
static int max7359_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
--
1.7.10.4
^ permalink raw reply related
* [PATCH] HID: wiimote: work around broken DRM_KAI on GEN10
From: David Herrmann @ 2013-08-04 16:50 UTC (permalink / raw)
To: linux-input; +Cc: Nicolas Adenis-Lamarre, Jiri Kosina, David Herrmann
GEN10 and earlier devices seem to not support DRM_KAI if we run in basic
IR mode. Use DRM_KAIE instead. This might increases overhead slightly as
the extension port is read and streamed but we stream accelerometer data
constantly, too, so this is negligible.
Note that our parsers are hardcoded on IR-formats, so we cannot actually
use 96-bit IR DRMs for basic IR data. We would have to adjust the parsers.
But as only GEN20 and newer support this, we simply avoid mixed DRMs.
This fixes a bug where GEN10 devices didn't provide IR data if
accelerometer and IR are enabled simultaneously. As a workaround, you can
enable DRM_KAIE without this patch via (disables device power-management):
echo "37" >/sys/kernel/debug/hid/<dev>/drm
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Reported-by: Nicolas Adenis-Lamarre <nicolas.adenis.lamarre@gmail.com>
---
drivers/hid/hid-wiimote-core.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index 0c06054..6602098 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -212,10 +212,12 @@ static __u8 select_drm(struct wiimote_data *wdata)
if (ir == WIIPROTO_FLAG_IR_BASIC) {
if (wdata->state.flags & WIIPROTO_FLAG_ACCEL) {
- if (ext)
- return WIIPROTO_REQ_DRM_KAIE;
- else
- return WIIPROTO_REQ_DRM_KAI;
+ /* GEN10 and ealier devices bind IR formats to DRMs.
+ * Hence, we cannot use DRM_KAI here as it might be
+ * bound to IR_EXT. Use DRM_KAIE unconditionally so we
+ * work with all devices and our parsers can use the
+ * fixed formats, too. */
+ return WIIPROTO_REQ_DRM_KAIE;
} else {
return WIIPROTO_REQ_DRM_KIE;
}
--
1.8.3.4
^ permalink raw reply related
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling and trigger support
From: Zubair Lutfullah : @ 2013-08-04 13:11 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, Russ.Dill-l0cyMroinI0
In-Reply-To: <51FE357C.2040401-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Sun, Aug 04, 2013 at 12:05:32PM +0100, Jonathan Cameron wrote:
> I'm afraid the tree has moved on a bit so this will need rebasing
> against what is currently in iio.git.
The patches apply on top of fixes-to greg branch because of a recent fix
in there. Which branch should I rebase them on?
Also, I've squashed a few more bug fixes I have found since then.
Will update accordingly next time I send.
>
> I would like a description in here of what form of buffered sampling the
> driver/device provides. Does it run on it's own internal clock? E.g.
> are we dealing with a fairly autonomous device with a data ready trigger?
> Is the driver to always be driven from an external trigger?
> This looks like there is no explicit trigger at all (which is an unusual
> case). I vaguely remember discussing this before and concluding it was
> probably valid for this device (as it is a hardware fifo pushed into a software
> one). If nothing else generic_buffer.c won't cover this case so might need
> appropriately updating with a 'don't bother with an explicit trigger' option.
>
> Having this stuff in the git commit log would be good as well as available
> at time of review. It's also usually best to assume everyone has forgotten
> everything about the driver inbetween reviews ;)
>
> Nearly there.
>
> Jonathan
>
The trigger here is simply to give certain functionality like an oscilloscope
trigger. The control is given to the userspace application via IIO triggers
(so sysfs/gpio/timer etc can be used)
A buffer of samples is read on one trigger.
An alternative would be the buffer being sampled and read as soon as the
buffer is enabled. Which would lead to a buffer enable/disable if another
buffer is to be read.
This is NOT a data ready trigger like in spi adcs.
The TSCADC module is inside the processor. Hardware FIFO interrupts and handler
reads samples from fifo and pushes to software FIFO.
At the moment, I use sysfs triggers to check this.
I'm still not sure on how to connect the gpio trigger driver by IIO.
generic_buffer.c reads the samples with a simple sysfs trigger.
So.
Additional description in the git log only?
I'll add following in log message.
"Any IIO trigger can be used to trigger driver to read a buffer
of samples from enabled channels."
Responses on comments on actual code is inline below.
Thanks
Zubair Lutfullah
> > +++ b/drivers/iio/adc/ti_am335x_adc.c
> > @@ -26,14 +26,25 @@
> > #include <linux/of_device.h>
> > #include <linux/iio/machine.h>
> Err, what is the machine header doing in here?
> (clearly my review of the original driver missed a few
> things).
I have no idea.
Old stuff. I'll remove and see if it changes anything..
>
> > #include <linux/iio/driver.h>
> > -
> Cleaner to group the headers appropriately.
Ok
> > @@ -251,6 +451,10 @@ static int tiadc_probe(struct platform_device *pdev)
> >
> > return 0;
> >
> The error handling in this function is thoroughly scrambled up. Please review
> it and fix it.
Sure.
>
> > +err_unregister:
> This label is too generic to make for easy review.
Ok.
> > +++ b/include/linux/mfd/ti_am335x_tscadc.h
>
> Clearly you are working in keeping with the current driver
> but these defines could really do with appropriate prefixes
> to make them more specific to the driver. You are almost
> certain that one of these will turn up in a general header
> at some point.
>
I understand your point.
But this header is common and affects mfd, input and iio.
Changing all of them now is going to be a massive(impossible) pain..
And adding new defines should conform to the naming of the previous.
What should I do?
> > @@ -46,17 +46,23 @@
> > /* Step Enable */
> > #define STEPENB_MASK (0x1FFFF << 0)
> > #define STEPENB(val) ((val) << 0)
> > +#define ENB(val) (1 << (val))
> > +#define STPENB_STEPENB STEPENB(0x1FFFF)
> > +#define STPENB_STEPENB_TC STEPENB(0x1FFF)
> >
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling and trigger support
From: Jonathan Cameron @ 2013-08-04 11:05 UTC (permalink / raw)
To: Zubair Lutfullah
Cc: jic23-KWPb1pKIrIJaa/9Udqfwiw, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, Russ.Dill-l0cyMroinI0
In-Reply-To: <1374882674-18403-3-git-send-email-zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 07/27/13 00:51, Zubair Lutfullah wrote:
> Previously the driver had only one-shot reading functionality.
> This patch adds triggered buffer support to the driver.
> A buffer of samples can now be read via /dev/iio.
>
> Patil Rachna (TI) laid the ground work for ADC HW register access.
> Russ Dill (TI) fixed bugs in the driver relevant to FIFOs and IRQs.
>
> I fixed channel scanning so multiple ADC channels can be read
> simultaneously and pushed to userspace.
> Restructured the driver to fit IIO ABI.
> And added trigger support.
>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> Signed-off-by: Russ Dill <Russ.Dill-l0cyMroinI0@public.gmane.org>
I'm afraid the tree has moved on a bit so this will need rebasing
against what is currently in iio.git.
Some other bits and bobs inline.
I would like a description in here of what form of buffered sampling the
driver/device provides. Does it run on it's own internal clock? E.g.
are we dealing with a fairly autonomous device with a data ready trigger?
Is the driver to always be driven from an external trigger?
This looks like there is no explicit trigger at all (which is an unusual
case). I vaguely remember discussing this before and concluding it was
probably valid for this device (as it is a hardware fifo pushed into a software
one). If nothing else generic_buffer.c won't cover this case so might need
appropriately updating with a 'don't bother with an explicit trigger' option.
Having this stuff in the git commit log would be good as well as available
at time of review. It's also usually best to assume everyone has forgotten
everything about the driver inbetween reviews ;)
Nearly there.
Jonathan
> ---
> drivers/iio/adc/ti_am335x_adc.c | 334 +++++++++++++++++++++++++++-------
> include/linux/mfd/ti_am335x_tscadc.h | 13 +-
> 2 files changed, 285 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 3ceac3e..630ce85 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -26,14 +26,25 @@
> #include <linux/of_device.h>
> #include <linux/iio/machine.h>
Err, what is the machine header doing in here?
(clearly my review of the original driver missed a few
things).
> #include <linux/iio/driver.h>
> -
Cleaner to group the headers appropriately.
> #include <linux/mfd/ti_am335x_tscadc.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/wait.h>
> +#include <linux/sched.h>
>
> struct tiadc_device {
> struct ti_tscadc_dev *mfd_tscadc;
> int channels;
> u8 channel_line[8];
> u8 channel_step[8];
> + struct work_struct poll_work;
> + wait_queue_head_t wq_data_avail;
> + bool data_avail;
> + u32 *inputbuffer;
> + int sample_count;
> + int irq;
> };
>
> static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
> @@ -56,27 +67,28 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
> return step_en;
> }
>
> -static void tiadc_step_config(struct tiadc_device *adc_dev)
> +static void tiadc_step_config(struct iio_dev *indio_dev)
> {
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> unsigned int stepconfig;
> - int i, steps;
> + int i, steps, chan;
>
> /*
> * There are 16 configurable steps and 8 analog input
> * lines available which are shared between Touchscreen and ADC.
> - *
> * Steps backwards i.e. from 16 towards 0 are used by ADC
> * depending on number of input lines needed.
> * Channel would represent which analog input
> * needs to be given to ADC to digitalize data.
> */
> -
> steps = TOTAL_STEPS - adc_dev->channels;
> - stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
> + if (iio_buffer_enabled(indio_dev))
> + stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
> + | STEPCONFIG_MODE_SWCNT;
> + else
> + stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
>
> for (i = 0; i < adc_dev->channels; i++) {
> - int chan;
> -
> chan = adc_dev->channel_line[i];
> tiadc_writel(adc_dev, REG_STEPCONFIG(steps),
> stepconfig | STEPCONFIG_INP(chan));
> @@ -85,7 +97,190 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
> adc_dev->channel_step[i] = steps;
> steps++;
> }
> +}
> +
> +static irqreturn_t tiadc_irq(int irq, void *private)
> +{
> + struct iio_dev *idev = private;
> + struct tiadc_device *adc_dev = iio_priv(idev);
> + unsigned int status, config;
> + status = tiadc_readl(adc_dev, REG_IRQSTATUS);
> +
> + /* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
> + if (status & IRQENB_FIFO1OVRRUN) {
> + config = tiadc_readl(adc_dev, REG_CTRL);
> + config &= ~(CNTRLREG_TSCSSENB);
> + tiadc_writel(adc_dev, REG_CTRL, config);
> + tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN |
> + IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
> + tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
> + return IRQ_HANDLED;
> + } else if (status & IRQENB_FIFO1THRES) {
> + /* Wake adc_work that pushes FIFO data to iio buffer */
> + tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
> + adc_dev->data_avail = 1;
> + wake_up_interruptible(&adc_dev->wq_data_avail);
> + return IRQ_HANDLED;
> + } else
> + return IRQ_NONE;
> +}
> +
> +static irqreturn_t tiadc_trigger_h(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + unsigned int config;
> +
> + schedule_work(&adc_dev->poll_work);
> + config = tiadc_readl(adc_dev, REG_CTRL);
> + tiadc_writel(adc_dev, REG_CTRL, config & ~CNTRLREG_TSCSSENB);
> + tiadc_writel(adc_dev, REG_CTRL, config | CNTRLREG_TSCSSENB);
> +
> + tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES |
> + IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
> + tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES
> + | IRQENB_FIFO1OVRRUN);
> +
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + return iio_sw_buffer_preenable(indio_dev);
> +}
> +
> +static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + struct iio_buffer *buffer = indio_dev->buffer;
> + unsigned int enb, stepnum;
> + u8 bit;
> +
> + tiadc_step_config(indio_dev);
> + tiadc_writel(adc_dev, REG_SE, 0x00);
> + for_each_set_bit(bit, buffer->scan_mask,
> + adc_dev->channels) {
> + struct iio_chan_spec const *chan = indio_dev->channels + bit;
> + /*
> + * There are a total of 16 steps available
> + * that are shared between ADC and touchscreen.
> + * We start configuring from step 16 to 0 incase of
> + * ADC. Hence the relation between input channel
> + * and step for ADC would be as below.
> + */
> + stepnum = chan->channel + 9;
> + enb = tiadc_readl(adc_dev, REG_SE);
> + enb |= (1 << stepnum);
> + tiadc_writel(adc_dev, REG_SE, enb);
> + }
> +
> + return iio_triggered_buffer_postenable(indio_dev);
> +}
> +
> +static int tiadc_buffer_predisable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + int fifo1count, i, read, config;
> +
> + config = tiadc_readl(adc_dev, REG_CTRL);
> + config &= ~(CNTRLREG_TSCSSENB);
> + tiadc_writel(adc_dev, REG_CTRL, config);
> + tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
> + IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW));
> + tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB_TC);
> +
> + /* Flush FIFO of any leftover data */
> + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> + for (i = 0; i < fifo1count; i++)
> + read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> + return iio_triggered_buffer_predisable(indio_dev);
> +}
> +
> +static int tiadc_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + int config;
> +
> + tiadc_step_config(indio_dev);
> + config = tiadc_readl(adc_dev, REG_CTRL);
> + tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
> +
> + return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops tiadc_buffer_setup_ops = {
> + .preenable = &tiadc_buffer_preenable,
> + .postenable = &tiadc_buffer_postenable,
> + .predisable = &tiadc_buffer_predisable,
> + .postdisable = &tiadc_buffer_postdisable,
> +};
> +
> +static void tiadc_adc_work(struct work_struct *work_s)
> +{
> + struct tiadc_device *adc_dev =
> + container_of(work_s, struct tiadc_device, poll_work);
> + struct iio_dev *indio_dev = iio_priv_to_dev(adc_dev);
> + struct iio_buffer *buffer = indio_dev->buffer;
> + int i, j, k, fifo1count, read;
> + unsigned int config;
> + int size_to_acquire = buffer->access->get_length(buffer);
> + int sample_count = 0;
> + u32 *data;
> +
> + adc_dev->data_avail = 0;
> + data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> + if (data == NULL)
> + goto out;
> +
> + while (sample_count < size_to_acquire) {
> + tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
> + tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES);
> +
> + wait_event_interruptible(adc_dev->wq_data_avail,
> + (adc_dev->data_avail == 1));
> + adc_dev->data_avail = 0;
> +
> + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> + if (fifo1count * sizeof(u32) <
> + buffer->access->get_bytes_per_datum(buffer))
> + continue;
> +
> + sample_count = sample_count + fifo1count;
> + for (k = 0; k < fifo1count; k = k + i) {
> + for (i = 0, j = 0; i < (indio_dev->scan_bytes)/4; i++) {
> + read = tiadc_readl(adc_dev, REG_FIFO1);
> + data[i] = read & FIFOREAD_DATA_MASK;
> + }
> + iio_push_to_buffers(indio_dev, (u8 *) data);
> + }
> + }
> +out:
> + tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
> + IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW));
> + config = tiadc_readl(adc_dev, REG_CTRL);
> + tiadc_writel(adc_dev, REG_CTRL, config & ~CNTRLREG_TSCSSENB);
> +}
>
> +irqreturn_t tiadc_iio_pollfunc(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + int i, fifo1count, read;
> +
> + tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
> + IRQENB_FIFO1OVRRUN |
> + IRQENB_FIFO1UNDRFLW));
> +
> + /* Flush FIFO before trigger */
> + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> + for (i = 0; i < fifo1count; i++)
> + read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> + return IRQ_WAKE_THREAD;
> }
>
> static const char * const chan_name_ain[] = {
> @@ -120,13 +315,13 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
> chan->channel = adc_dev->channel_line[i];
> chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> chan->datasheet_name = chan_name_ain[chan->channel];
> + chan->scan_index = i;
> chan->scan_type.sign = 'u';
> chan->scan_type.realbits = 12;
> chan->scan_type.storagebits = 32;
> }
>
> indio_dev->channels = chan_array;
> -
> return 0;
> }
>
> @@ -141,58 +336,48 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> {
> struct tiadc_device *adc_dev = iio_priv(indio_dev);
> int i, map_val;
> - unsigned int fifo1count, read, stepid;
> - u32 step = UINT_MAX;
> - bool found = false;
> - u32 step_en;
> - unsigned long timeout = jiffies + usecs_to_jiffies
> - (IDLE_TIMEOUT * adc_dev->channels);
> - step_en = get_adc_step_mask(adc_dev);
> - am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
> -
> - /* Wait for ADC sequencer to complete sampling */
> - while (tiadc_readl(adc_dev, REG_ADCFSM) & SEQ_STATUS) {
> - if (time_after(jiffies, timeout))
> - return -EAGAIN;
> - }
> - map_val = chan->channel + TOTAL_CHANNELS;
> -
> - /*
> - * When the sub-system is first enabled,
> - * the sequencer will always start with the
> - * lowest step (1) and continue until step (16).
> - * For ex: If we have enabled 4 ADC channels and
> - * currently use only 1 out of them, the
> - * sequencer still configures all the 4 steps,
> - * leading to 3 unwanted data.
> - * Hence we need to flush out this data.
> - */
> + unsigned int fifo1count, read, stepid, step_en;
>
> - for (i = 0; i < ARRAY_SIZE(adc_dev->channel_step); i++) {
> - if (chan->channel == adc_dev->channel_line[i]) {
> - step = adc_dev->channel_step[i];
> - break;
> - }
> - }
> - if (WARN_ON_ONCE(step == UINT_MAX))
> - return -EINVAL;
> -
> - fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> - for (i = 0; i < fifo1count; i++) {
> - read = tiadc_readl(adc_dev, REG_FIFO1);
> - stepid = read & FIFOREAD_CHNLID_MASK;
> - stepid = stepid >> 0x10;
> -
> - if (stepid == map_val) {
> - read = read & FIFOREAD_DATA_MASK;
> - found = true;
> - *val = read;
> + if (iio_buffer_enabled(indio_dev))
> + return -EBUSY;
> + else {
> + unsigned long timeout = jiffies + usecs_to_jiffies
> + (IDLE_TIMEOUT * adc_dev->channels);
> + step_en = get_adc_step_mask(adc_dev);
> + am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
> +
> + /* Wait for ADC sequencer to complete sampling */
> + while (tiadc_readl(adc_dev, REG_ADCFSM) & SEQ_STATUS) {
> + if (time_after(jiffies, timeout))
> + return -EAGAIN;
> + }
> + map_val = chan->channel + TOTAL_CHANNELS;
> +
> + /*
> + * When the sub-system is first enabled,
> + * the sequencer will always start with the
> + * lowest step (1) and continue until step (16).
> + * For ex: If we have enabled 4 ADC channels and
> + * currently use only 1 out of them, the
> + * sequencer still configures all the 4 steps,
> + * leading to 3 unwanted data.
> + * Hence we need to flush out this data.
> + */
> +
> + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> + for (i = 0; i < fifo1count; i++) {
> + read = tiadc_readl(adc_dev, REG_FIFO1);
> + stepid = read & FIFOREAD_CHNLID_MASK;
> + stepid = stepid >> 0x10;
> +
> + if (stepid == map_val) {
> + read = read & FIFOREAD_DATA_MASK;
> + *val = read;
> + return IIO_VAL_INT;
> + }
> }
> + return -EAGAIN;
> }
> -
> - if (found == false)
> - return -EBUSY;
> - return IIO_VAL_INT;
> }
>
> static const struct iio_info tiadc_info = {
> @@ -231,18 +416,33 @@ static int tiadc_probe(struct platform_device *pdev)
> channels++;
> }
> adc_dev->channels = channels;
> + adc_dev->irq = adc_dev->mfd_tscadc->irq;
>
> indio_dev->dev.parent = &pdev->dev;
> indio_dev->name = dev_name(&pdev->dev);
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->info = &tiadc_info;
>
> - tiadc_step_config(adc_dev);
> + tiadc_step_config(indio_dev);
> + tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
>
> err = tiadc_channel_init(indio_dev, adc_dev->channels);
> if (err < 0)
> goto err_free_device;
>
> + INIT_WORK(&adc_dev->poll_work, &tiadc_adc_work);
> + init_waitqueue_head(&adc_dev->wq_data_avail);
> +
> + err = request_irq(adc_dev->irq, tiadc_irq, IRQF_SHARED,
> + indio_dev->name, indio_dev);
> + if (err)
> + goto err_free_irq;
> +
> + err = iio_triggered_buffer_setup(indio_dev, &tiadc_iio_pollfunc,
> + &tiadc_trigger_h, &tiadc_buffer_setup_ops);
> + if (err)
> + goto err_unregister;
> +
> err = iio_device_register(indio_dev);
> if (err)
> goto err_free_channels;
> @@ -251,6 +451,10 @@ static int tiadc_probe(struct platform_device *pdev)
>
> return 0;
>
The error handling in this function is thoroughly scrambled up. Please review
it and fix it.
> +err_unregister:
This label is too generic to make for easy review.
> + iio_buffer_unregister(indio_dev);
> +err_free_irq:
> + free_irq(adc_dev->irq, indio_dev);
> err_free_channels:
> tiadc_channels_remove(indio_dev);
> err_free_device:
> @@ -265,7 +469,9 @@ static int tiadc_remove(struct platform_device *pdev)
> struct tiadc_device *adc_dev = iio_priv(indio_dev);
> u32 step_en;
>
> + free_irq(adc_dev->irq, indio_dev);
> iio_device_unregister(indio_dev);
> + iio_buffer_unregister(indio_dev);
> tiadc_channels_remove(indio_dev);
>
> step_en = get_adc_step_mask(adc_dev);
> @@ -303,10 +509,16 @@ static int tiadc_resume(struct device *dev)
>
> /* Make sure ADC is powered up */
> restore = tiadc_readl(adc_dev, REG_CTRL);
> - restore &= ~(CNTRLREG_POWERDOWN);
> + restore &= ~(CNTRLREG_TSCSSENB);
> tiadc_writel(adc_dev, REG_CTRL, restore);
>
> - tiadc_step_config(adc_dev);
> + tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
> + tiadc_step_config(indio_dev);
> +
> + /* Make sure ADC is powered up */
> + restore &= ~(CNTRLREG_POWERDOWN);
> + restore |= CNTRLREG_TSCSSENB;
> + tiadc_writel(adc_dev, REG_CTRL, restore);
>
> return 0;
> }
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index db1791b..cb61027 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
Clearly you are working in keeping with the current driver
but these defines could really do with appropriate prefixes
to make them more specific to the driver. You are almost
certain that one of these will turn up in a general header
at some point.
> @@ -46,17 +46,23 @@
> /* Step Enable */
> #define STEPENB_MASK (0x1FFFF << 0)
> #define STEPENB(val) ((val) << 0)
> +#define ENB(val) (1 << (val))
> +#define STPENB_STEPENB STEPENB(0x1FFFF)
> +#define STPENB_STEPENB_TC STEPENB(0x1FFF)
>
> /* IRQ enable */
> #define IRQENB_HW_PEN BIT(0)
> #define IRQENB_FIFO0THRES BIT(2)
> #define IRQENB_FIFO1THRES BIT(5)
> #define IRQENB_PENUP BIT(9)
> +#define IRQENB_FIFO1OVRRUN BIT(6)
> +#define IRQENB_FIFO1UNDRFLW BIT(7)
>
> /* Step Configuration */
> #define STEPCONFIG_MODE_MASK (3 << 0)
> #define STEPCONFIG_MODE(val) ((val) << 0)
> #define STEPCONFIG_MODE_HWSYNC STEPCONFIG_MODE(2)
> +#define STEPCONFIG_MODE_SWCNT STEPCONFIG_MODE(1)
> #define STEPCONFIG_AVG_MASK (7 << 2)
> #define STEPCONFIG_AVG(val) ((val) << 2)
> #define STEPCONFIG_AVG_16 STEPCONFIG_AVG(4)
> @@ -124,7 +130,8 @@
> #define MAX_CLK_DIV 7
> #define TOTAL_STEPS 16
> #define TOTAL_CHANNELS 8
> -
> +#define FIFO1_THRESHOLD 19
> +#define FIFO_SIZE 64
> /*
> * ADC runs at 3MHz, and it takes
> * 15 cycles to latch one data output.
> @@ -153,6 +160,10 @@ struct ti_tscadc_dev {
>
> /* adc device */
> struct adc_device *adc;
> +
> + /* Context save */
> + unsigned int irqstat;
> + unsigned int ctrl;
> };
>
> static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
>
^ permalink raw reply
* Re: [PATCH 1/2] input: ti_tsc: Enable shared IRQ for TSC
From: Jonathan Cameron @ 2013-08-04 11:08 UTC (permalink / raw)
To: Zubair Lutfullah
Cc: jic23, linux-iio, linux-kernel, linux-input, gregkh, Russ.Dill,
Torokhov
In-Reply-To: <1374882674-18403-2-git-send-email-zubair.lutfullah@gmail.com>
On 07/27/13 00:51, Zubair Lutfullah wrote:
> From: "Patil, Rachna" <rachna@ti.com>
>
> Touchscreen and ADC share the same IRQ line from parent MFD core.
> Previously only Touchscreen was interrupt based.
> With continuous mode support added in ADC driver, driver requires
> interrupt to process the ADC samples, so enable shared IRQ flag bit for
> touchscreen.
>
> Signed-off-by: Patil, Rachna <rachna@ti.com>
> Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
I'd rather this went via input independent of the other patch
(if not there all that will happen is one or other driver will
fail to probe if both are attempted?)
Can take it through IIO but only with a Dmitry Ack.
> ---
> drivers/input/touchscreen/ti_am335x_tsc.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index e1c5300..68d1250 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -260,8 +260,18 @@ static irqreturn_t titsc_irq(int irq, void *dev)
> unsigned int fsm;
>
> status = titsc_readl(ts_dev, REG_IRQSTATUS);
> - if (status & IRQENB_FIFO0THRES) {
> -
> + /*
> + * ADC and touchscreen share the IRQ line.
> + * FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
> + * interrupts are used by ADC,
> + * hence return from touchscreen IRQ handler if FIFO1
> + * related interrupts occurred.
> + */
> + if ((status & IRQENB_FIFO1THRES) ||
> + (status & IRQENB_FIFO1OVRRUN) ||
> + (status & IRQENB_FIFO1UNDRFLW))
> + return IRQ_NONE;
> + else if (status & IRQENB_FIFO0THRES) {
> titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
>
> if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
> @@ -315,7 +325,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
> }
>
> if (irqclr) {
> - titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
> + titsc_writel(ts_dev, REG_IRQSTATUS, (status | irqclr));
> am335x_tsc_se_update(ts_dev->mfd_tscadc);
> return IRQ_HANDLED;
> }
> @@ -389,7 +399,7 @@ static int titsc_probe(struct platform_device *pdev)
> }
>
> err = request_irq(ts_dev->irq, titsc_irq,
> - 0, pdev->dev.driver->name, ts_dev);
> + IRQF_SHARED, pdev->dev.driver->name, ts_dev);
> if (err) {
> dev_err(&pdev->dev, "failed to allocate irq.\n");
> goto err_free_mem;
>
^ permalink raw reply
* Re: [PATCH v6] Input: sysrq - DT binding for key sequence
From: Dmitry Torokhov @ 2013-08-04 0:30 UTC (permalink / raw)
To: Grant Likely
Cc: Mathieu Poirier, Rob Herring, devicetree@vger.kernel.org,
John Stultz, linux-input@vger.kernel.org, kernel-team
In-Reply-To: <CACxGe6u9-sEzxdDzc8T2HFNMqh-LHbTepUHqDF_tLLjxSxSG4Q@mail.gmail.com>
On Mon, Jul 29, 2013 at 10:09:54PM -0600, Grant Likely wrote:
> On Mon, Jul 29, 2013 at 10:33 AM, <mathieu.poirier@linaro.org> wrote:
> > From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
> >
> > Adding a simple device tree binding for the specification of key sequences.
> > Definition of the keys found in the sequence are located in
> > 'include/uapi/linux/input.h'.
> >
> > For the sysrq driver, holding the sequence of keys down for a specific amount of time
> > will reset the system.
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>
> Acked-by: Grant Likely <grant.likely@linaro.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* [PATCH 1/1] input: ideapad_slidebar: new input driver
From: Andrey Moiseev @ 2013-08-03 10:26 UTC (permalink / raw)
To: linux-input
ideapad-slidebar is a new driver which enables slidebars on some
Lenovo IdeaPad laptops.
Fixes this: https://bugzilla.kernel.org/show_bug.cgi?id=16004
Registers 'IdeaPad Slidebar' input device and
/sys/devices/platform/ideapad_slidebar/slidebar_mode
for switching slidebar's modes.
Now works on:
* Lenovo IdeaPad Y550
* Lenovo IdeaPad Y550P
Driver source: https://github.com/o2genum/ideapad-slidebar.git
Signed-off-by: Andrey Moiseev <o2g.org.ru@gmail.com>
---
drivers/input/misc/Kconfig | 9 +
drivers/input/misc/Makefile | 1 +
drivers/input/misc/ideapad_slidebar.c | 378 ++++++++++++++++++++++++++++++++++
3 files changed, 388 insertions(+)
create mode 100644 drivers/input/misc/ideapad_slidebar.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 0b541cd..45729a9 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -647,4 +647,13 @@ config INPUT_SIRFSOC_ONKEY
If unsure, say N.
+config INPUT_IDEAPAD_SLIDEBAR
+ tristate "IdeaPad Laptop Slidebar"
+ depends on INPUT
+ help
+ Input driver for slidebars on some Lenovo IdeaPad laptops.
+
+ If you have an IdeaPad laptop with a slidebar, say Y or M here.
+ Module name is ideapad_slidebar.
+
endif
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 829de43..0ebfb6d 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -61,3 +61,4 @@ obj-$(CONFIG_INPUT_WISTRON_BTNS) += wistron_btns.o
obj-$(CONFIG_INPUT_WM831X_ON) += wm831x-on.o
obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND) += xen-kbdfront.o
obj-$(CONFIG_INPUT_YEALINK) += yealink.o
+obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR) += ideapad_slidebar.o
diff --git a/drivers/input/misc/ideapad_slidebar.c b/drivers/input/misc/ideapad_slidebar.c
new file mode 100644
index 0000000..78294fd
--- /dev/null
+++ b/drivers/input/misc/ideapad_slidebar.c
@@ -0,0 +1,378 @@
+/*
+ * Input driver for slidebars on some Lenovo IdeaPad laptops
+ *
+ * Copyright (C) 2013 Andrey Moiseev <o2g.org.ru@gmail.com>
+ *
+ * Reverse-engineered from Lenovo SlideNav software (SBarHook.dll).
+ *
+ * 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.
+ *
+ * Trademarks are the property of their respective owners.
+ */
+
+/* Currently tested and works on:
+ * Lenovo IdeaPad Y550
+ * Lenovo IdeaPad Y550P
+ *
+ * Other models can be added easily. To test,
+ * load with 'force' parameter set 'true'.
+ *
+ * LEDs blinking and input mode are managed via sysfs,
+ * (hex, unsigned byte value):
+ * /sys/devices/platform/ideapad_slidebar/slidebar_mode
+ *
+ * The value is in byte range, however, I only figured out
+ * how bits 0b10011001 work. Some other bits, probably,
+ * are meaningfull too.
+ *
+ * Possible states:
+ *
+ * STD_INT, ONMOV_INT, OFF_INT, LAST_POLL, OFF_POLL
+ *
+ * Meaning:
+ * released touched
+ * STD 'heartbeat' lights follow the finger
+ * ONMOV no lights lights follow the finger
+ * LAST at last pos lights follow the finger
+ * OFF no lights no lights
+ *
+ * INT all input events are generated, interrupts are used
+ * POLL no input events by default, to get them,
+ * send 0b10000000 (read below)
+ *
+ * Commands: write
+ *
+ * All | 0b01001 -> STD_INT
+ * possible | 0b10001 -> ONMOV_INT
+ * states | 0b01000 -> OFF_INT
+ *
+ * | 0b0 -> LAST_POLL
+ * STD_INT or ONMOV_INT |
+ * | 0b1 -> STD_INT
+ *
+ * | 0b0 -> OFF_POLL
+ * OFF_INT or OFF_POLL |
+ * | 0b1 -> OFF_INT
+ *
+ * Any state | 0b10000000 -> if the slidebar has updated data,
+ * produce one input event (last position),
+ * switch to respective POLL mode
+ * (like 0x0), if not in POLL mode yet.
+ *
+ * Get current state: read
+ *
+ * masked by 0x11 read value means:
+ *
+ * 0x00 LAST
+ * 0x01 STD
+ * 0x10 OFF
+ * 0x11 ONMOV
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/dmi.h>
+#include <linux/spinlock.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/input.h>
+#include <linux/io.h>
+
+static bool force;
+module_param(force, bool, 0);
+MODULE_PARM_DESC(force, "Force driver load, ignore DMI data");
+
+#define KBD_IRQ 1
+
+spinlock_t sio_lock = __SPIN_LOCK_UNLOCKED(sio_lock);
+
+static int prev_scancode;
+static int touched;
+static struct input_dev *slidebar_input_dev;
+static struct platform_device *slidebar_platform_dev;
+
+/* Hardware interacting */
+static int slidebar_pos_get(void)
+{
+ int res;
+ unsigned long flags;
+ spin_lock_irqsave(&sio_lock, flags);
+ outb(0xf4, 0xff29);
+ outb(0xbf, 0xff2a);
+ res = inb(0xff2b);
+ spin_unlock_irqrestore(&sio_lock, flags);
+ return res;
+}
+
+static unsigned char slidebar_mode_get(void)
+{
+ int res;
+ unsigned long flags;
+ spin_lock_irqsave(&sio_lock, flags);
+ outb(0xf7, 0xff29);
+ outb(0x8b, 0xff2a);
+ res = inb(0xff2b);
+ spin_unlock_irqrestore(&sio_lock, flags);
+ return res;
+}
+
+static void slidebar_mode_set(unsigned char mode)
+{
+ unsigned long flags;
+ spin_lock_irqsave(&sio_lock, flags);
+ outb(0xf7, 0xff29);
+ outb(0x8b, 0xff2a);
+ outb(mode, 0xff2b);
+ spin_unlock_irqrestore(&sio_lock, flags);
+}
+
+/* Keyboard handler */
+static irq_handler_t kbd_irq_handler(int irq, void *dev_id,
+ struct pt_regs *regs)
+{
+ /* Scancodes: e03b on move, bb on release */
+ int scancode = inb(0x60);
+ if (scancode == 0xbb) {
+ touched = 0;
+ input_report_key(slidebar_input_dev, BTN_TOUCH, 0);
+ input_sync(slidebar_input_dev);
+ } else if (prev_scancode == 0xe0 && scancode == 0x3b) {
+ if (!touched)
+ input_report_key(slidebar_input_dev, BTN_TOUCH, 1);
+ touched = 1;
+ input_report_abs(slidebar_input_dev, ABS_X, slidebar_pos_get());
+ input_sync(slidebar_input_dev);
+ }
+ prev_scancode = scancode;
+ return (irq_handler_t) IRQ_HANDLED;
+}
+
+/* Input device */
+static int setup_input_dev(void)
+{
+ int err;
+ err = request_irq(KBD_IRQ, (irq_handler_t) kbd_irq_handler, IRQF_SHARED,
+ "ideapad_slidebar", (void *)(kbd_irq_handler));
+ if (err) {
+ pr_err("ideapad_slidebar: Can't allocate irq %d\n", KBD_IRQ);
+ return -EBUSY;
+ }
+
+ slidebar_input_dev = input_allocate_device();
+ if (!slidebar_input_dev) {
+ pr_err("ideapad_slidebar: Not enough memory\n");
+ err = -ENOMEM;
+ goto err_free_irq;
+ }
+
+ slidebar_input_dev->name = "IdeaPad Slidebar";
+ slidebar_input_dev->id.bustype = BUS_HOST;
+ slidebar_input_dev->dev.parent = &slidebar_platform_dev->dev;
+ input_set_capability(slidebar_input_dev, EV_KEY, BTN_TOUCH);
+ input_set_capability(slidebar_input_dev, EV_ABS, ABS_X);
+ input_alloc_absinfo(slidebar_input_dev);
+ input_set_abs_params(slidebar_input_dev, ABS_X, 0, 0xff, 0, 0);
+
+ err = input_register_device(slidebar_input_dev);
+ if (err) {
+ pr_err("ideapad_slidebar: Failed to register device\n");
+ goto err_free_dev;
+ }
+ return 0;
+
+err_free_dev:
+ input_free_device(slidebar_input_dev);
+err_free_irq:
+ free_irq(KBD_IRQ, (void *)(kbd_irq_handler));
+ return err;
+}
+
+static void remove_input_dev(void)
+{
+ free_irq(KBD_IRQ, (void *)(kbd_irq_handler));
+ input_unregister_device(slidebar_input_dev);
+ input_free_device(slidebar_input_dev);
+}
+
+/* Sysfs slidebar_mode interface */
+static ssize_t show_slidebar_mode(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%x\n", slidebar_mode_get());
+}
+
+static ssize_t store_slidebar_mode(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int mode;
+ if (!count)
+ return 0;
+
+ if (sscanf(buf, "%x", &mode) != 1)
+ return -EINVAL;
+
+ slidebar_mode_set(mode);
+ return count;
+}
+
+static DEVICE_ATTR(slidebar_mode, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH,
+ show_slidebar_mode, store_slidebar_mode);
+
+static struct attribute *ideapad_attributes[] = {
+ &dev_attr_slidebar_mode.attr,
+ NULL
+};
+
+static struct attribute_group ideapad_attribute_group = {
+ .attrs = ideapad_attributes
+};
+
+static int setup_slidebar_mode_dev(void)
+{
+ return sysfs_create_group(&slidebar_platform_dev->dev.kobj,
+ &ideapad_attribute_group);
+}
+
+static void remove_slidebar_mode_dev(void)
+{
+ return sysfs_remove_group(&slidebar_platform_dev->dev.kobj,
+ &ideapad_attribute_group);
+}
+
+/* Platform device */
+static int setup_platform_dev(void)
+{
+ int err;
+ slidebar_platform_dev = platform_device_alloc("ideapad_slidebar", -1);
+ if (!slidebar_platform_dev) {
+ pr_err("ideapad_slidebar: Not enough memory\n");
+ return -ENOMEM;
+ }
+
+ err = platform_device_add(slidebar_platform_dev);
+ if (err) {
+ pr_err("ideapad_slidebar: Failed to register plarform device\n");
+ goto err_free_platform_device;
+ }
+ return 0;
+
+err_free_platform_device:
+ platform_device_put(slidebar_platform_dev);
+ return err;
+}
+
+static void remove_platform_dev(void)
+{
+ platform_device_del(slidebar_platform_dev);
+ platform_device_put(slidebar_platform_dev);
+}
+
+/* Platform driver */
+static struct platform_driver slidebar_drv = {
+ .driver = {
+ .name = "ideapad_slidebar",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int register_platform_drv(void)
+{
+ int err;
+ err = platform_driver_register(&slidebar_drv);
+ if (err)
+ pr_err("ideapad_slidebar: Failed to register platform driver\n");
+
+ return err;
+}
+
+static void unregister_platform_drv(void)
+{
+ platform_driver_unregister(&slidebar_drv);
+}
+
+/* DMI */
+static int ideapad_dmi_check(const struct dmi_system_id *id)
+{
+ pr_info("ideapad_slidebar: Laptop model '%s'\n", id->ident);
+ return 1;
+}
+
+static struct dmi_system_id __initdata ideapad_dmi_table[] = {
+ {
+ .ident = "Lenovo IdeaPad Y550",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "20017"),
+ DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo IdeaPad Y550")
+ },
+ .callback = ideapad_dmi_check
+ },
+ {
+ .ident = "Lenovo IdeaPad Y550P",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "20035"),
+ DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo IdeaPad Y550P")
+ },
+ .callback = ideapad_dmi_check
+ }
+};
+
+/* Init and cleanup */
+static int __init slidebar_init(void)
+{
+ int err;
+
+ if (!force && !dmi_check_system(ideapad_dmi_table))
+ return -ENODEV;
+
+ err = setup_platform_dev();
+ if (err)
+ return err;
+
+ err = register_platform_drv();
+ if (err)
+ goto err_remove_platform_dev;
+
+ err = setup_input_dev();
+ if (err)
+ goto err_unregister_platform_drv;
+
+ err = setup_slidebar_mode_dev();
+ if (err)
+ goto err_remove_input_dev;
+ return 0;
+
+err_remove_input_dev:
+ remove_input_dev();
+err_unregister_platform_drv:
+ unregister_platform_drv();
+err_remove_platform_dev:
+ remove_platform_dev();
+ return err;
+}
+
+static void __exit slidebar_exit(void)
+{
+ remove_slidebar_mode_dev();
+ remove_input_dev();
+ remove_platform_dev();
+ unregister_platform_drv();
+}
+
+module_init(slidebar_init);
+module_exit(slidebar_exit);
+
+MODULE_AUTHOR("Andrey Moiseev <o2g.org.ru@gmail.com>");
+MODULE_DESCRIPTION("Slidebar input support for some Lenovo IdeaPad laptops");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("0.1");
+
+MODULE_ALIAS("dmi:*:svnLENOVO:pn20017:pvrLenovoIdeaPadY550:*");
+MODULE_ALIAS("dmi:*:svnLENOVO:pn20035:pvrLenovoIdeaPadY550P:*");
--
1.8.3.3
^ permalink raw reply related
* RE: [PATCH 25/35] Input: touchscreen: use dev_get_platdata()
From: Duan Fugang-B38611 @ 2013-08-02 23:31 UTC (permalink / raw)
To: Jingoo Han, 'Dmitry Torokhov'
Cc: 'Dmitry Torokhov', linux-input@vger.kernel.org
In-Reply-To: <005401ce8cfc$f70d1500$e5273f00$@samsung.com>
From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@vger.kernel.org] On Behalf Of Jingoo Han
Data: Tuesday, July 30, 2013 4:16 PM
> Use the wrapper function for retrieving the platform data instead of
> accessing dev->platform_data directly.
The serial patches look good. But for the serial patch set, it is better to submit one patch summary for them.
Acked-by: Fugang Duan <B38611@freescale.com>
^ permalink raw reply
* Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'
From: Benjamin Tissoires @ 2013-08-02 18:42 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-input, Jiri Kosina, Mika Westerberg
In-Reply-To: <1375468274.31118.87.camel@smile>
On 02/08/13 20:31, Andy Shevchenko wrote:
> On Fri, 2013-08-02 at 20:14 +0200, Benjamin Tissoires wrote:
>> On 02/08/13 16:49, Andy Shevchenko wrote:
>>> On Fri, 2013-08-02 at 16:30 +0200, Benjamin Tissoires wrote:
>
> []
>
>>>> Moreover, the dev_dbg calls are compiled out if DEBUG symbol is not
>>>> set.
>>>
>>> Yes, and what is the difference between previously used
>>> if (debug)
>>> dev_dbg(...)
>>>
>>> ?
>>
>> That's because the previous code was directly using
>> dev_printk(KERN_DEBUG,...) and not dev_dbg() within the condition.
>>
>> This call does not use dynamic debugging or does not get compiled out if
>> DEBUG is not set.
>
> Yeah, I've checked this just after I sent the message.
>
>> Also, to my defence, I can add that I developed this driver on an ARM
>> board without dynamic debugging support enabled... :)
>
>
> So, the problem is DYNAMIC_DEBUG is not default option in kernel (and I
> could imagine why).
>
> I checked few distros for that: Debian/Ubuntu - off,
> Fedora/OpenSuse/ArchLinux - on.
>
> Just to understand what kind of user might run this with debug? Is it
> developer or person who knows the subject? If it's not a such person, it
> would be difficult to him or her to enable dynamic debug in distros
> like Ubuntu, which sucks.
I'm afraid people reporting this kind of bugs are most of the time
regular users, who just bought a brand new laptop, and who send a bug
report through their distribution's bugzilla... :(
I'm not expecting that much of developers or person who knows the
subject to actually report the problem, because most of the time a
normal user reports "my touchpad is not working". At least, that's what
I have mostly seen in the input subsystem.
>
>>>> So, if we ever have to change this debug variable, I would prefer
>>>> using the hid debug environment which would at least limit the number
>>>> of debug outputs to the HID subsystem.
>>>
>>> Usually when I see such code I understood it was written in
>>> pre-dynamic-debug epoch. So, my point is to switch to dynamic debug and
>>> use it efficiently.
>>
>> Ok, so if you can guarantee me that adding the proper kernel parameter
>> will allow me to retrieve all the i2c-hid logs from the boot, then I'd
>> be happy to ack this. However, I have no way to test this right now, so
>> I'll need to trust you (that's why I'm asking you to do proper testing).
>
> With only one condition if dynamic debug is enabled in kernel.
> So, it would be nice to gather opinions, however, the decision is
> totally depends on type of user who wants to debug the module.
>
Yep, I agree that Jiri's opinion would be helpful.
Meanwhile, we could also wait a little for more i2c-hid to hit the
market and to be widely tested, and then remove the debug flag. We
should also remove the debug events in get_i2c_hid_get_input() which
would pollute systems without dynamic debugging but with the DEBUG
config still enabled.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 2/2] HID: hid-logitech-dj, querying_devices was never set
From: Benjamin Tissoires @ 2013-08-02 18:31 UTC (permalink / raw)
To: Jiri Kosina
Cc: Nestor Lopez Casado, Andrew de los Reyes, joseph.salisbury,
linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <alpine.LNX.2.00.1308020309340.24017@pobox.suse.cz>
On Fri, Aug 2, 2013 at 3:11 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Thu, 1 Aug 2013, Benjamin Tissoires wrote:
>
>> > Set querying_devices flag to true when we start the enumeration
>> > process.
>> >
>> > This was missing from the original patch. It never produced
>> > undesirable effects as it is highly improbable to have a second
>> > enumeration triggered while a first one was still in progress.
>> >
>> > Signed-off-by: Nestor Lopez Casado <nlopezcasad@logitech.com>
>> > ---
>> > drivers/hid/hid-logitech-dj.c | 2 ++
>> > 1 file changed, 2 insertions(+)
>> >
>> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>> > index 0d13389..d4657a5 100644
>> > --- a/drivers/hid/hid-logitech-dj.c
>> > +++ b/drivers/hid/hid-logitech-dj.c
>> > @@ -488,6 +488,8 @@ static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev)
>> > if (djrcv_dev->querying_devices)
>> > return 0;
>> >
>> > + djrcv_dev->querying_devices = true;
>> > +
>>
>> Unfortunately, this breaks the fallback mechanism :(
>> We tried to add the two patches in Fedora [1], but this doesn't fix
>> the bug because the driver actually things that it already asked for
>> the enumeration, but as we get the -EPIPE error, the request was never
>> sent.
>>
>> So, Jiri, if you were to submit that series to Linus (or Greg) for
>> fixing the bug, please just drop this second patch.
>
> It's already on its way to Linus (he hasn't pulled yet though) ... which
> is not a big deal per se, I can always push a revert, but I have to admit
> I don't understand the breakage it is causing at all.
>
> Could you please elaborate? (and put an elaborate description to revert
> commit log perhaps?)
Sure, so here is the revert commit log:
--
Commit "HID: hid-logitech-dj, querying_devices was never set" activate
a flag which guarantees that we do not ask the receiver for too many
enumeration. When the flag is set, each following enumeration call is
discarded (the usb request is not forwarded to the receiver). The flag
is then released when the driver receive a pairing information event,
which normally follows the enumeration request.
However, the USB3 bug makes the driver think the enumeration request
has been forwarded to the receiver. However, it is actually not the
case because the USB stack returns -EPIPE. So, when a new unknown
device appears, the workaround consisting in asking for a new
enumeration is not working anymore: this new enumeration is discarded
because of the flag, which is never reset.
A solution could be to trigger a timeout before releasing it, but for
now, let's just revert the patch.
--
Does that makes it more understandable? I'm sorry I was not clear last
time, I was trying to catch this up between two sessions at GUADEC.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'
From: Andy Shevchenko @ 2013-08-02 18:31 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input, Jiri Kosina, Mika Westerberg
In-Reply-To: <51FBF71E.1040802@redhat.com>
On Fri, 2013-08-02 at 20:14 +0200, Benjamin Tissoires wrote:
> On 02/08/13 16:49, Andy Shevchenko wrote:
> > On Fri, 2013-08-02 at 16:30 +0200, Benjamin Tissoires wrote:
[]
> >> Moreover, the dev_dbg calls are compiled out if DEBUG symbol is not
> >> set.
> >
> > Yes, and what is the difference between previously used
> > if (debug)
> > dev_dbg(...)
> >
> > ?
>
> That's because the previous code was directly using
> dev_printk(KERN_DEBUG,...) and not dev_dbg() within the condition.
>
> This call does not use dynamic debugging or does not get compiled out if
> DEBUG is not set.
Yeah, I've checked this just after I sent the message.
> Also, to my defence, I can add that I developed this driver on an ARM
> board without dynamic debugging support enabled... :)
So, the problem is DYNAMIC_DEBUG is not default option in kernel (and I
could imagine why).
I checked few distros for that: Debian/Ubuntu - off,
Fedora/OpenSuse/ArchLinux - on.
Just to understand what kind of user might run this with debug? Is it
developer or person who knows the subject? If it's not a such person, it
would be difficult to him or her to enable dynamic debug in distros
like Ubuntu, which sucks.
> >> So, if we ever have to change this debug variable, I would prefer
> >> using the hid debug environment which would at least limit the number
> >> of debug outputs to the HID subsystem.
> >
> > Usually when I see such code I understood it was written in
> > pre-dynamic-debug epoch. So, my point is to switch to dynamic debug and
> > use it efficiently.
>
> Ok, so if you can guarantee me that adding the proper kernel parameter
> will allow me to retrieve all the i2c-hid logs from the boot, then I'd
> be happy to ack this. However, I have no way to test this right now, so
> I'll need to trust you (that's why I'm asking you to do proper testing).
With only one condition if dynamic debug is enabled in kernel.
So, it would be nice to gather opinions, however, the decision is
totally depends on type of user who wants to debug the module.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'
From: Benjamin Tissoires @ 2013-08-02 18:14 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Benjamin Tissoires, linux-input, Jiri Kosina, Mika Westerberg
In-Reply-To: <1375454987.31118.77.camel@smile>
On 02/08/13 16:49, Andy Shevchenko wrote:
> On Fri, 2013-08-02 at 16:30 +0200, Benjamin Tissoires wrote:
>> On Fri, Aug 2, 2013 at 1:07 PM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>> We have nice dynamic debug framework to enable or disable debug messaging at
>>> run time. So, instead of an additional module parameter let's use that framework
>>> and call dev_dbg() unconditionally in the driver.
>>>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> ---
>>
>> Well, when I introduced those debug variables, I had in mind the fact
>> that the driver was not widely tested, and that I may need to ask for
>> traces from users. I'm afraid that relying on dev_dbg will create a
>> lot more noise when we will want to understand the HID/i2c problems.
>
> You have only those messages on the debug level (frankly, only one is
> outside of if (debug) condition).
Yes, and this is normal:
I want to be able to ask people to boot with i2c_hid.debug=1, and then
retrieve their debug traces.
And the one which is not guarded by the debug condition is a message
that will appear only once at boot (so it will not spam the debug output
-- which is not the case by the others).
>
>
>> Moreover, the dev_dbg calls are compiled out if DEBUG symbol is not
>> set.
>
> Yes, and what is the difference between previously used
> if (debug)
> dev_dbg(...)
>
> ?
That's because the previous code was directly using
dev_printk(KERN_DEBUG,...) and not dev_dbg() within the condition.
This call does not use dynamic debugging or does not get compiled out if
DEBUG is not set.
Also, to my defence, I can add that I developed this driver on an ARM
board without dynamic debugging support enabled... :)
>
>
>> So, if we ever have to change this debug variable, I would prefer
>> using the hid debug environment which would at least limit the number
>> of debug outputs to the HID subsystem.
>
> Usually when I see such code I understood it was written in
> pre-dynamic-debug epoch. So, my point is to switch to dynamic debug and
> use it efficiently.
Ok, so if you can guarantee me that adding the proper kernel parameter
will allow me to retrieve all the i2c-hid logs from the boot, then I'd
be happy to ack this. However, I have no way to test this right now, so
I'll need to trust you (that's why I'm asking you to do proper testing).
Cheers,
Benjamin
>>
>>> drivers/hid/i2c-hid/i2c-hid.c | 59 ++++++++++++++++++-------------------------
>>> 1 file changed, 24 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>>> index 05d4f96..5f50fc7 100644
>>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>>> @@ -47,17 +47,6 @@
>>> #define I2C_HID_PWR_ON 0x00
>>> #define I2C_HID_PWR_SLEEP 0x01
>>>
>>> -/* debug option */
>>> -static bool debug;
>>> -module_param(debug, bool, 0444);
>>> -MODULE_PARM_DESC(debug, "print a lot of debug information");
>>> -
>>> -#define i2c_hid_dbg(ihid, fmt, arg...) \
>>> -do { \
>>> - if (debug) \
>>> - dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg); \
>>> -} while (0)
>>> -
>>> struct i2c_hid_desc {
>>> __le16 wHIDDescLength;
>>> __le16 bcdVersion;
>>> @@ -177,7 +166,7 @@ static int __i2c_hid_command(struct i2c_client *client,
>>> memcpy(cmd->data + length, args, args_len);
>>> length += args_len;
>>>
>>> - i2c_hid_dbg(ihid, "%s: cmd=%*ph\n", __func__, length, cmd->data);
>>> + dev_dbg(&client->dev, "%s: cmd=%*ph\n", __func__, length, cmd->data);
>>>
>>> msg[0].addr = client->addr;
>>> msg[0].flags = client->flags & I2C_M_TEN;
>>> @@ -207,12 +196,12 @@ static int __i2c_hid_command(struct i2c_client *client,
>>> ret = 0;
>>>
>>> if (wait) {
>>> - i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
>>> + dev_dbg(&client->dev, "%s: waiting...\n", __func__);
>>> if (!wait_event_timeout(ihid->wait,
>>> !test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
>>> msecs_to_jiffies(5000)))
>>> ret = -ENODATA;
>>> - i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
>>> + dev_dbg(&client->dev, "%s: finished.\n", __func__);
>>> }
>>>
>>> return ret;
>>> @@ -235,7 +224,7 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
>>> int args_len = 0;
>>> u16 readRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
>>>
>>> - i2c_hid_dbg(ihid, "%s\n", __func__);
>>> + dev_dbg(&client->dev, "%s\n", __func__);
>>>
>>> if (reportID >= 0x0F) {
>>> args[args_len++] = reportID;
>>> @@ -276,7 +265,7 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
>>> size /* args */;
>>> int index = 0;
>>>
>>> - i2c_hid_dbg(ihid, "%s\n", __func__);
>>> + dev_dbg(&client->dev, "%s\n", __func__);
>>>
>>> if (reportID >= 0x0F) {
>>> args[index++] = reportID;
>>> @@ -316,10 +305,9 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
>>>
>>> static int i2c_hid_set_power(struct i2c_client *client, int power_state)
>>> {
>>> - struct i2c_hid *ihid = i2c_get_clientdata(client);
>>> int ret;
>>>
>>> - i2c_hid_dbg(ihid, "%s\n", __func__);
>>> + dev_dbg(&client->dev, "%s\n", __func__);
>>>
>>> ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
>>> 0, NULL, 0, NULL, 0);
>>> @@ -331,16 +319,15 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
>>>
>>> static int i2c_hid_hwreset(struct i2c_client *client)
>>> {
>>> - struct i2c_hid *ihid = i2c_get_clientdata(client);
>>> int ret;
>>>
>>> - i2c_hid_dbg(ihid, "%s\n", __func__);
>>> + dev_dbg(&client->dev, "%s\n", __func__);
>>>
>>> ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
>>> if (ret)
>>> return ret;
>>>
>>> - i2c_hid_dbg(ihid, "resetting...\n");
>>> + dev_dbg(&client->dev, "resetting...\n");
>>>
>>> ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
>>> if (ret) {
>>> @@ -354,15 +341,16 @@ static int i2c_hid_hwreset(struct i2c_client *client)
>>>
>>> static void i2c_hid_get_input(struct i2c_hid *ihid)
>>> {
>>> + struct i2c_client *client = ihid->client;
>>> int ret, ret_size;
>>> int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
>>>
>>> - ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
>>> + ret = i2c_master_recv(client, ihid->inbuf, size);
>>> if (ret != size) {
>>> if (ret < 0)
>>> return;
>>>
>>> - dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n",
>>> + dev_err(&client->dev, "%s: got %d data instead of %d\n",
>>> __func__, ret, size);
>>> return;
>>> }
>>> @@ -377,12 +365,12 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
>>> }
>>>
>>> if (ret_size > size) {
>>> - dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
>>> + dev_err(&client->dev, "%s: incomplete report (%d/%d)\n",
>>> __func__, size, ret_size);
>>> return;
>>> }
>>>
>>> - i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
>>> + dev_dbg(&client->dev, "input: %*ph\n", ret_size, ihid->inbuf);
>>>
>>> if (test_bit(I2C_HID_STARTED, &ihid->flags))
>>> hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
>>> @@ -423,7 +411,8 @@ static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
>>> report->id, buffer, size))
>>> return;
>>>
>>> - i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
>>> + dev_dbg(&client->dev, "report (len=%d): %*ph\n", size,
>>> + size, ihid->inbuf);
>>>
>>> ret_size = buffer[0] | (buffer[1] << 8);
>>>
>>> @@ -618,7 +607,7 @@ static int i2c_hid_parse(struct hid_device *hid)
>>> int ret;
>>> int tries = 3;
>>>
>>> - i2c_hid_dbg(ihid, "entering %s\n", __func__);
>>> + dev_dbg(&client->dev, "entering %s\n", __func__);
>>>
>>> rsize = le16_to_cpu(hdesc->wReportDescLength);
>>> if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
>>> @@ -642,7 +631,7 @@ static int i2c_hid_parse(struct hid_device *hid)
>>> return -ENOMEM;
>>> }
>>>
>>> - i2c_hid_dbg(ihid, "asking HID report descriptor\n");
>>> + dev_dbg(&client->dev, "asking HID report descriptor\n");
>>>
>>> ret = i2c_hid_command(client, &hid_report_descr_cmd, rdesc, rsize);
>>> if (ret) {
>>> @@ -651,7 +640,7 @@ static int i2c_hid_parse(struct hid_device *hid)
>>> return -EIO;
>>> }
>>>
>>> - i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
>>> + dev_dbg(&client->dev, "Report Descriptor: %*ph\n", rsize, rdesc);
>>>
>>> ret = hid_parse_report(hid, rdesc, rsize);
>>> kfree(rdesc);
>>> @@ -741,10 +730,9 @@ static void i2c_hid_close(struct hid_device *hid)
>>> 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);
>>> + dev_dbg(&client->dev, "%s lvl:%d\n", __func__, lvl);
>>>
>>> switch (lvl) {
>>> case PM_HINT_FULLON:
>>> @@ -801,8 +789,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
>>> * bytes 2-3 -> bcdVersion (has to be 1.00) */
>>> ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer, 4);
>>>
>>> - i2c_hid_dbg(ihid, "%s, ihid->hdesc_buffer: %4ph\n", __func__,
>>> - ihid->hdesc_buffer);
>>> + dev_dbg(&client->dev, "%s, ihid->hdesc_buffer: %4ph\n", __func__,
>>> + ihid->hdesc_buffer);
>>>
>>> if (ret) {
>>> dev_err(&client->dev,
>>> @@ -832,7 +820,7 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
>>> return -ENODEV;
>>> }
>>>
>>> - i2c_hid_dbg(ihid, "Fetching the HID descriptor\n");
>>> + dev_dbg(&client->dev, "Fetching the HID descriptor\n");
>>>
>>> ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer,
>>> dsize);
>>> @@ -841,7 +829,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
>>> return -ENODEV;
>>> }
>>>
>>> - i2c_hid_dbg(ihid, "HID Descriptor: %*ph\n", dsize, ihid->hdesc_buffer);
>>> + dev_dbg(&client->dev, "HID Descriptor: %*ph\n", dsize,
>>> + ihid->hdesc_buffer);
>>>
>>> return 0;
>>> }
>>> --
>>> 1.8.4.rc0
>>>
>>> --
>>> 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: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'
From: Andy Shevchenko @ 2013-08-02 14:49 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, Jiri Kosina, Mika Westerberg, Benjamin Tissoires
In-Reply-To: <CAN+gG=Ec=jJy7HxRW08up+=Z8C0Lz4B8bhPcP+7__zuONnCAgw@mail.gmail.com>
On Fri, 2013-08-02 at 16:30 +0200, Benjamin Tissoires wrote:
> On Fri, Aug 2, 2013 at 1:07 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > We have nice dynamic debug framework to enable or disable debug messaging at
> > run time. So, instead of an additional module parameter let's use that framework
> > and call dev_dbg() unconditionally in the driver.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
>
> Well, when I introduced those debug variables, I had in mind the fact
> that the driver was not widely tested, and that I may need to ask for
> traces from users. I'm afraid that relying on dev_dbg will create a
> lot more noise when we will want to understand the HID/i2c problems.
You have only those messages on the debug level (frankly, only one is
outside of if (debug) condition).
> Moreover, the dev_dbg calls are compiled out if DEBUG symbol is not
> set.
Yes, and what is the difference between previously used
if (debug)
dev_dbg(...)
?
> So, if we ever have to change this debug variable, I would prefer
> using the hid debug environment which would at least limit the number
> of debug outputs to the HID subsystem.
Usually when I see such code I understood it was written in
pre-dynamic-debug epoch. So, my point is to switch to dynamic debug and
use it efficiently.
>
> Cheers,
> Benjamin
>
> > drivers/hid/i2c-hid/i2c-hid.c | 59 ++++++++++++++++++-------------------------
> > 1 file changed, 24 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index 05d4f96..5f50fc7 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -47,17 +47,6 @@
> > #define I2C_HID_PWR_ON 0x00
> > #define I2C_HID_PWR_SLEEP 0x01
> >
> > -/* debug option */
> > -static bool debug;
> > -module_param(debug, bool, 0444);
> > -MODULE_PARM_DESC(debug, "print a lot of debug information");
> > -
> > -#define i2c_hid_dbg(ihid, fmt, arg...) \
> > -do { \
> > - if (debug) \
> > - dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg); \
> > -} while (0)
> > -
> > struct i2c_hid_desc {
> > __le16 wHIDDescLength;
> > __le16 bcdVersion;
> > @@ -177,7 +166,7 @@ static int __i2c_hid_command(struct i2c_client *client,
> > memcpy(cmd->data + length, args, args_len);
> > length += args_len;
> >
> > - i2c_hid_dbg(ihid, "%s: cmd=%*ph\n", __func__, length, cmd->data);
> > + dev_dbg(&client->dev, "%s: cmd=%*ph\n", __func__, length, cmd->data);
> >
> > msg[0].addr = client->addr;
> > msg[0].flags = client->flags & I2C_M_TEN;
> > @@ -207,12 +196,12 @@ static int __i2c_hid_command(struct i2c_client *client,
> > ret = 0;
> >
> > if (wait) {
> > - i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
> > + dev_dbg(&client->dev, "%s: waiting...\n", __func__);
> > if (!wait_event_timeout(ihid->wait,
> > !test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
> > msecs_to_jiffies(5000)))
> > ret = -ENODATA;
> > - i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
> > + dev_dbg(&client->dev, "%s: finished.\n", __func__);
> > }
> >
> > return ret;
> > @@ -235,7 +224,7 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
> > int args_len = 0;
> > u16 readRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
> >
> > - i2c_hid_dbg(ihid, "%s\n", __func__);
> > + dev_dbg(&client->dev, "%s\n", __func__);
> >
> > if (reportID >= 0x0F) {
> > args[args_len++] = reportID;
> > @@ -276,7 +265,7 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
> > size /* args */;
> > int index = 0;
> >
> > - i2c_hid_dbg(ihid, "%s\n", __func__);
> > + dev_dbg(&client->dev, "%s\n", __func__);
> >
> > if (reportID >= 0x0F) {
> > args[index++] = reportID;
> > @@ -316,10 +305,9 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
> >
> > static int i2c_hid_set_power(struct i2c_client *client, int power_state)
> > {
> > - struct i2c_hid *ihid = i2c_get_clientdata(client);
> > int ret;
> >
> > - i2c_hid_dbg(ihid, "%s\n", __func__);
> > + dev_dbg(&client->dev, "%s\n", __func__);
> >
> > ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
> > 0, NULL, 0, NULL, 0);
> > @@ -331,16 +319,15 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
> >
> > static int i2c_hid_hwreset(struct i2c_client *client)
> > {
> > - struct i2c_hid *ihid = i2c_get_clientdata(client);
> > int ret;
> >
> > - i2c_hid_dbg(ihid, "%s\n", __func__);
> > + dev_dbg(&client->dev, "%s\n", __func__);
> >
> > ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
> > if (ret)
> > return ret;
> >
> > - i2c_hid_dbg(ihid, "resetting...\n");
> > + dev_dbg(&client->dev, "resetting...\n");
> >
> > ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
> > if (ret) {
> > @@ -354,15 +341,16 @@ static int i2c_hid_hwreset(struct i2c_client *client)
> >
> > static void i2c_hid_get_input(struct i2c_hid *ihid)
> > {
> > + struct i2c_client *client = ihid->client;
> > int ret, ret_size;
> > int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
> >
> > - ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
> > + ret = i2c_master_recv(client, ihid->inbuf, size);
> > if (ret != size) {
> > if (ret < 0)
> > return;
> >
> > - dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n",
> > + dev_err(&client->dev, "%s: got %d data instead of %d\n",
> > __func__, ret, size);
> > return;
> > }
> > @@ -377,12 +365,12 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
> > }
> >
> > if (ret_size > size) {
> > - dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
> > + dev_err(&client->dev, "%s: incomplete report (%d/%d)\n",
> > __func__, size, ret_size);
> > return;
> > }
> >
> > - i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
> > + dev_dbg(&client->dev, "input: %*ph\n", ret_size, ihid->inbuf);
> >
> > if (test_bit(I2C_HID_STARTED, &ihid->flags))
> > hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
> > @@ -423,7 +411,8 @@ static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
> > report->id, buffer, size))
> > return;
> >
> > - i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
> > + dev_dbg(&client->dev, "report (len=%d): %*ph\n", size,
> > + size, ihid->inbuf);
> >
> > ret_size = buffer[0] | (buffer[1] << 8);
> >
> > @@ -618,7 +607,7 @@ static int i2c_hid_parse(struct hid_device *hid)
> > int ret;
> > int tries = 3;
> >
> > - i2c_hid_dbg(ihid, "entering %s\n", __func__);
> > + dev_dbg(&client->dev, "entering %s\n", __func__);
> >
> > rsize = le16_to_cpu(hdesc->wReportDescLength);
> > if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
> > @@ -642,7 +631,7 @@ static int i2c_hid_parse(struct hid_device *hid)
> > return -ENOMEM;
> > }
> >
> > - i2c_hid_dbg(ihid, "asking HID report descriptor\n");
> > + dev_dbg(&client->dev, "asking HID report descriptor\n");
> >
> > ret = i2c_hid_command(client, &hid_report_descr_cmd, rdesc, rsize);
> > if (ret) {
> > @@ -651,7 +640,7 @@ static int i2c_hid_parse(struct hid_device *hid)
> > return -EIO;
> > }
> >
> > - i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
> > + dev_dbg(&client->dev, "Report Descriptor: %*ph\n", rsize, rdesc);
> >
> > ret = hid_parse_report(hid, rdesc, rsize);
> > kfree(rdesc);
> > @@ -741,10 +730,9 @@ static void i2c_hid_close(struct hid_device *hid)
> > 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);
> > + dev_dbg(&client->dev, "%s lvl:%d\n", __func__, lvl);
> >
> > switch (lvl) {
> > case PM_HINT_FULLON:
> > @@ -801,8 +789,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> > * bytes 2-3 -> bcdVersion (has to be 1.00) */
> > ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer, 4);
> >
> > - i2c_hid_dbg(ihid, "%s, ihid->hdesc_buffer: %4ph\n", __func__,
> > - ihid->hdesc_buffer);
> > + dev_dbg(&client->dev, "%s, ihid->hdesc_buffer: %4ph\n", __func__,
> > + ihid->hdesc_buffer);
> >
> > if (ret) {
> > dev_err(&client->dev,
> > @@ -832,7 +820,7 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> > return -ENODEV;
> > }
> >
> > - i2c_hid_dbg(ihid, "Fetching the HID descriptor\n");
> > + dev_dbg(&client->dev, "Fetching the HID descriptor\n");
> >
> > ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer,
> > dsize);
> > @@ -841,7 +829,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> > return -ENODEV;
> > }
> >
> > - i2c_hid_dbg(ihid, "HID Descriptor: %*ph\n", dsize, ihid->hdesc_buffer);
> > + dev_dbg(&client->dev, "HID Descriptor: %*ph\n", dsize,
> > + ihid->hdesc_buffer);
> >
> > return 0;
> > }
> > --
> > 1.8.4.rc0
> >
> > --
> > 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
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ 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