* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Chen Gang @ 2014-07-15 0:34 UTC (permalink / raw)
To: Richard Weinberger
Cc: Lars-Peter Clausen, Greg Kroah-Hartman,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Benjamin Herrenschmidt,
teg-B22kvLQNl6c, Thierry Reding, Lennox Wu, Marek Vasut,
Liqin Chen, msalter-H+wXaHxf7aLQT0dZR+AlfA,
linux-pwm-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
knaack.h-Mmb7MZpHnFY, Martin Schwidefsky,
Mischa.Jonker-HKixBCOQz3hWk0Htik3J/w,
jic23-DgEjT+Ai2ygdnm+yROfE0A, arnd-r2nGTMty4D4,
Geert Uytterhoeven
In-Reply-To: <5A40E1FC-CA61-4AFF-B205-4BAC175AA7AC-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 07/14/2014 05:22 PM, Chen Gang wrote:
>
> 在 2014年7月14日，下午4:57，Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org> 写道：
>
>> Am 14.07.2014 10:48, schrieb Lars-Peter Clausen:
>>> On 07/14/2014 10:31 AM, Richard Weinberger wrote:
>>>> Am 13.07.2014 22:17, schrieb Greg Kroah-Hartman:
>>>>> On Sun, Jul 13, 2014 at 09:33:38PM +0200, Richard Weinberger wrote:
>>>>>> Maybe we could add COMPILE_TEST to the version string too?
>>>>>> Just to detect such kernels fast in user bug reports...
>>>>>
>>>>> What kind of bug report are you going to get?
>>>>
>>>> User manages to enable CONFIG_FOO by selecting COMPILE_TEST and
>>>> complains that it does not work. :)
>>>
>>> These drivers are typically drivers for some SoC peripheral and the device will simply physically not exist on a platform that does not provide HAS_IOMEM. This is not really any
>>> different from making the driver selectable via COMPILE_TEST for any other platform. To hit the issue you'd have to instantiate a device driver instance for a device that
>>> physically does not exist. This will always result in a failure.
>>
>> Okay, you have convinced me. :)
>>
After search the history patches, I found one related patch which made
by myself (when I am in Asianux):
"https://lkml.org/lkml/2013/7/1/641"
For me, it is a long discussion, and forced many members have to join
in. Please help check again.
Thanks.
>
> OK, thank all of you, and I shall send the related patch for it.
>
> I will try to finish it within this week.
>
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply
* Re: [PATCH v4 0/4] Add support for Lenovo Compact Keyboard
From: Antonio Ospite @ 2014-07-14 21:02 UTC (permalink / raw)
To: Jamie Lentin; +Cc: Jiri Kosina, Hans de Goede, linux-input, linux-kernel
In-Reply-To: <1405236262-8070-1-git-send-email-jm@lentin.co.uk>
On Sun, 13 Jul 2014 08:24:18 +0100
Jamie Lentin <jm@lentin.co.uk> wrote:
> This patchset follows on from my previous attempts to add support for
> these keyboards from Lenovo.
[...]
Hi Jamie,
in the next (and last) iteration add some context to the short commit
message, e.g. by adding a prefix:
HID: lenovo: ...
following the scheme of previous commits in the history.
> Cheers,
>
> Jamie Lentin (4):
> Rename hid-lenovo-tpkbd to hid-lenovo
> Make all base functions switch depending on product ID
> Style fixes
> Add support for Compact (Bluetooth|USB) keyboard with Trackpoint
Maybe rename Bluetooth to BT if this becomes too long with the prefix.
Thanks,
Antonio
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH v4 0/2] Input: Support in the elantech driver of the trackpoint present on for instance Lenovo L530
From: ulrik.debie-os @ 2014-07-14 19:03 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Hans de Goede, David Herrmann
In-Reply-To: <1402955589-23709-1-git-send-email-ulrik.debie-os@e2big.org>
Hi Dmitry,
could you please give feedback on this set of 2 patches ?
They have been updated and reviewed and this patch set v4 is 1 month old now.
I have some other patches waiting to make Fujitsu H730 work too
but it will rely for part of the functionality on this patch.
Thanks for your time and feedback !
Ulrik
On Mon, Jun 16, 2014 at 11:53:07PM +0200, Ulrik De Bie wrote:
> Date: Mon, 16 Jun 2014 23:53:07 +0200
> From: Ulrik De Bie <ulrik.debie-os@e2big.org>
> To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org, Hans de Goede <hdegoede@redhat.com>, David
> Herrmann <dh.herrmann@gmail.com>, ulrik.debie-os@e2big.org
> Subject: [PATCH v4 0/2] Input: Support in the elantech driver of the
> trackpoint present on for instance Lenovo L530
> X-Mailer: git-send-email 2.0.0
> X-Mailing-List: linux-input@vger.kernel.org
>
> Patch 1 adds support for trackpoint on elantech driver for v3 models.
> Patch 2 adds a psmouse_reset when the elantech probes fails. Patch 2 depends
> on Patch 1.
>
> Changes since v3:
> * Patch1: added (correct) error after input_allocate_device failure in elantech_init()
> * Patch2: added more explanation to the why
>
> Changes since v2:
> * psmouse_reset change is now moved to a separate patch
> * comments/white spaces/newlines cleanup
> * Unexpected trackpoint message warning now only printed once
> * removed some unnecessary casts
> * Deleted etd->trackpoint_present and use instead etd->tp_dev to indicate the
> presence of a trackpoint
> * Propagate the error when elantech_init fails
>
> Changes since v1:
> * New patch now with reference to 3.14rc1
> * Added etd->trackpoint_present to indicate presence of trackpoint (based
> on MSB of etd->capabilities[0])
> * trackpoint will only be registered now when MSB of etd->capabilities[0] is
> set; got confirmation that this is the indicator of trackpoint
> * Added input_unregister_device/input_free_device in elantech_disconnect()
> * Fixed a bug in cleaning up when elantech_init fails
> * Rename commit to be more specific (now also applicable to future elantech
> v3 models with trackpoint)
> * input device name 'TPPS/2 IBM TrackPoint' changed to
> 'Elantech PS/2 TrackPoint', this patch is not ibm/lenovo specific!
> * dev2 renamed to tp_dev to indicate that this is the trackpoint device
> * etd->phys renamed to etd->tp_phys
> * Added Lenovo 530 and Fujitsu H730 to the laptop list because those are now
> also known.
> * Added psmouse_reset at the end of elantech_init when it fails
> * Added warning when trackpoint packets are received with no trackpoint detected
>
> The patches are also available from:
> https://github.com/ulrikdb/linux/commit/53d8424bc2143d34c2be76064892079fe1917a9e
> https://github.com/ulrikdb/linux/commit/cf12fcc6cdf51a7aad48d056750478aecd9d7eca
>
>
> Ulrik De Bie (2):
> elantech: Add support for trackpoint found on some v3 models
> elantech: Call psmouse_reset when elantech probe fails
>
> drivers/input/mouse/elantech.c | 123 +++++++++++++++++++++++++++++++++++++++--
> drivers/input/mouse/elantech.h | 3 +
> 2 files changed, 122 insertions(+), 4 deletions(-)
>
> --
> 2.0.0
>
> --
> 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 v4] Input: Add driver for Microchip's CAP1106
From: Dmitry Torokhov @ 2014-07-14 18:46 UTC (permalink / raw)
To: Daniel Mack; +Cc: linux-input, broonie, dh.herrmann, devicetree, mark.rutland
In-Reply-To: <1405348082-24210-1-git-send-email-zonque@gmail.com>
On Mon, Jul 14, 2014 at 04:28:02PM +0200, Daniel Mack wrote:
> +
> +static int cap1106_i2c_remove(struct i2c_client *i2c_client)
> +{
> + struct cap1106_priv *priv = i2c_get_clientdata(i2c_client);
> +
> + input_unregister_device(priv->idev);
> +
No need to call unregister on managed input devices.
Otherwise if DT guys are happy with bindings I am happy with the patch.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: USB reset xhci_hcd for ELAN touchscreen
From: Johan Hovold @ 2014-07-14 18:25 UTC (permalink / raw)
To: Greg KH
Cc: Johan Hovold, Alan Stern, Bjørn Mork, Sarah Sharp,
Drew Von Spreecken, linux-usb, Mathias Nyman, Jiri Kosina,
linux-input
In-Reply-To: <20140709130733.GA10078@localhost>
[ +CC: Jiri and linux-input ]
On Wed, Jul 09, 2014 at 03:07:33PM +0200, Johan Hovold wrote:
> On Mon, Jul 07, 2014 at 10:34:30PM -0700, Greg Kroah-Hartman wrote:
> > On Mon, Jul 07, 2014 at 11:24:42AM +0200, Johan Hovold wrote:
>
> > > I ended up getting the same laptop so now I'm facing the same problem
> > > with repeated disconnects (especially during resume, which is really
> > > annoying as it adds about a minute to resume time), even with a
> > > hard-coded check-high speed "quirk" and the multitouch driver loaded.
> > >
> > > Did you manage to fix the disconnect issue? Are you seeing something
> > > similar at resume?
> >
> > No, it all seems to work for me. Here's my horrible "hack" I'm running
> > with on 3.16-rc.
> >
> > I have seen some issues at times when the touchscreen stops working on
> > resume, but it just seems "dead". An unload/load of the xhci-hcd module
> > always fixes that :)
>
> Ok. I'm effectively using the same hack as you, but I'm seeing these
> repeated disconnects during resume for about a minute still. Everything
> compiled in, so no modules (or fancy hibernate scripts) though.
>
> > I'll work on making this a "real" patch this week, when I dig out from
> > my pending patch queue...
>
> Ok, I'll wait for that, and will try to find time to debug this later.
The timeout I experienced during resume was due to the wifi firmware not
being compiled in, but during that minute the touchscreen device kept
disconnecting every other second.
I get one such disconnect before X is started during normal boot as
well. It appears that something has to open the input device at least
once or the device will keep disconnecting itself (e.g. not booting into
X also triggers the repeated disconnects).
A simple
cat /dev/input/by-path/pci-0000:00:14.0-usb-0:7:1.0-event
is enough to stop the device from disconnecting. But unless the device
is kept open (and the interrupt-in urb submitted) any input event (i.e.
touching the screen) causes the device to start disconnecting
repeatedly again.
Adding something like the below (on top of the check-highspeed quirk)
fixes the repeated disconnect when the device is not open. (It cannot
prevent the disconnects during a lengthy resume though as reset_resume
will not have run yet.)
Is there any better way to fix this?
Thanks,
Johan
>From 8101c0dfd42a232f1d2872de4f412d8d61d5646f Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Mon, 14 Jul 2014 18:43:31 +0200
Subject: [PATCH] HID: usbhid: add HID_QUIRK_IN
Add quirk to submit the interrupt-in urb already at start() rather than
at open().
This is needed for devices that disconnects from the bus unless the
interrupt endpoint has been polled at least once or when not responding
to input events.
---
drivers/hid/usbhid/hid-core.c | 26 +++++++++++++++++++++++---
include/linux/hid.h | 1 +
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 7b88f4c..4b5d986 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -82,7 +82,7 @@ static int hid_start_in(struct hid_device *hid)
struct usbhid_device *usbhid = hid->driver_data;
spin_lock_irqsave(&usbhid->lock, flags);
- if (hid->open > 0 &&
+ if ((hid->open > 0 || hid->quirks & HID_QUIRK_IN) &&
!test_bit(HID_DISCONNECTED, &usbhid->iofl) &&
!test_bit(HID_SUSPENDED, &usbhid->iofl) &&
!test_and_set_bit(HID_IN_RUNNING, &usbhid->iofl)) {
@@ -292,6 +292,8 @@ static void hid_irq_in(struct urb *urb)
case 0: /* success */
usbhid_mark_busy(usbhid);
usbhid->retry_delay = 0;
+ if ((hid->quirks & HID_QUIRK_IN) && !hid->open)
+ break;
hid_input_report(urb->context, HID_INPUT_REPORT,
urb->transfer_buffer,
urb->actual_length, 1);
@@ -734,8 +736,10 @@ void usbhid_close(struct hid_device *hid)
if (!--hid->open) {
spin_unlock_irq(&usbhid->lock);
hid_cancel_delayed_stuff(usbhid);
- usb_kill_urb(usbhid->urbin);
- usbhid->intf->needs_remote_wakeup = 0;
+ if (!(hid->quirks & HID_QUIRK_IN)) {
+ usb_kill_urb(usbhid->urbin);
+ usbhid->intf->needs_remote_wakeup = 0;
+ }
} else {
spin_unlock_irq(&usbhid->lock);
}
@@ -1133,6 +1137,20 @@ static int usbhid_start(struct hid_device *hid)
set_bit(HID_STARTED, &usbhid->iofl);
+ hid->quirks |= HID_QUIRK_IN; /* FIXME */
+ if (hid->quirks & HID_QUIRK_IN) {
+ ret = usb_autopm_get_interface(usbhid->intf);
+ if (ret)
+ goto fail;
+ usbhid->intf->needs_remote_wakeup = 1;
+ ret = hid_start_in(hid);
+ if (ret) {
+ dev_err(&hid->dev,
+ "failed to start in urb: %d\n", ret);
+ }
+ usb_autopm_put_interface(usbhid->intf);
+ }
+
/* Some keyboards don't work until their LEDs have been set.
* Since BIOSes do set the LEDs, it must be safe for any device
* that supports the keyboard boot protocol.
@@ -1165,6 +1183,8 @@ static void usbhid_stop(struct hid_device *hid)
if (WARN_ON(!usbhid))
return;
+ usbhid->intf->needs_remote_wakeup = 0;
+
clear_bit(HID_STARTED, &usbhid->iofl);
spin_lock_irq(&usbhid->lock); /* Sync with error and led handlers */
set_bit(HID_DISCONNECTED, &usbhid->iofl);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 77632cf..13f81ae 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -286,6 +286,7 @@ struct hid_item {
#define HID_QUIRK_HIDINPUT_FORCE 0x00000080
#define HID_QUIRK_NO_EMPTY_INPUT 0x00000100
#define HID_QUIRK_NO_INIT_INPUT_REPORTS 0x00000200
+#define HID_QUIRK_IN 0x00000400
#define HID_QUIRK_SKIP_OUTPUT_REPORTS 0x00010000
#define HID_QUIRK_SKIP_OUTPUT_REPORT_ID 0x00020000
#define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP 0x00040000
--
1.8.5.5
^ permalink raw reply related
* [PATCH v4] Input: Add driver for Microchip's CAP1106
From: Daniel Mack @ 2014-07-14 14:28 UTC (permalink / raw)
To: dtor
Cc: linux-input, broonie, dh.herrmann, devicetree, mark.rutland,
Daniel Mack
This patch adds a driver for Microchips CAP1106, an I2C driven, 6-channel
capacitive touch sensor.
For now, only the capacitive buttons are supported, and no specific
settings that can be tweaked for individual channels, except for the
device-wide sensitivity gain. The defaults seem to work just fine out of
the box, so I'll leave configurable parameters for someone who's in need
of them and who can actually measure the impact. All registers are
prepared, however. Many of them are just not used for now.
The implementation does not make any attempt to be compatible to platform
data driven boards, but fully depends on CONFIG_OF.
Power management functions are also left for volounteers with the ability
to actually test them.
Signed-off-by: Daniel Mack <zonque@gmail.com>
---
v2:
* Fix potential deadlocks pointed out by David.
v3:
* Use devm_request_threaded_irq() and get rid of the work struct.
* Drop ->open() and ->close() as it isn't actually needed for an
interrupt-driven device. This way, we can also get rid of the
locking entirely.
v4:
* Follow Mark Rutland's advices regarding the DTS bindings:
* Fix 'interrupt' node in the bindings documentation
* Drop the subnode structure, and store keycodes in an array named
'linux,keycodes'. This way, we no longer rely on subnodes
ordering.
* Use of_property_read_bool() to read boolean properties.
* Re-introduce ->open() and ->close() for the input device, for
power-saving purposes. Also, put the device to deep sleep at probe
time, so it draws less power until ->open() is called.
.../devicetree/bindings/input/cap1106.txt | 53 ++++
drivers/input/keyboard/Kconfig | 10 +
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/cap1106.c | 347 +++++++++++++++++++++
4 files changed, 411 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/cap1106.txt
create mode 100644 drivers/input/keyboard/cap1106.c
diff --git a/Documentation/devicetree/bindings/input/cap1106.txt b/Documentation/devicetree/bindings/input/cap1106.txt
new file mode 100644
index 0000000..4b46390
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/cap1106.txt
@@ -0,0 +1,53 @@
+Device tree bindings for Microchip CAP1106, 6 channel capacitive touch sensor
+
+The node for this driver must be a child of a I2C controller node, as the
+device communication via I2C only.
+
+Required properties:
+
+ compatible: Must be "microchip,cap1106"
+
+ reg: The I2C slave address of the device.
+ Only 0x28 is valid.
+
+ interrupts: Property describing the interrupt line the
+ device's ALERT#/CM_IRQ# pin is connected to.
+ The device only has one interrupt source.
+
+Optional properties:
+
+ autorepeat: Enables the Linux input system's autorepeat
+ feature on the input device.
+
+ microchip,sensor-gain: Defines the gain of the sensor circuitry. This
+ effectively controls the sensitivity, as a
+ smaller delta capacitance is required to
+ generate the same delta count values.
+ Valid values are 1, 2, 4, and 8.
+ By default, a gain of 1 is set.
+
+ linux,keycodes: Specifies an array of numeric keycode values to
+ be used for the channels. If this property is
+ omitted, KEY_A, KEY_B, etc are used as
+ defaults. The array must have exactly six
+ entries.
+
+Example:
+
+i2c_controller {
+ cap1106@28 {
+ compatible = "microchip,cap1106";
+ interrupt-parent = <&gpio1>;
+ interrupts = <0 0>;
+ reg = <0x28>;
+ autorepeat;
+ microchip,sensor-gain = <2>;
+
+ linux,keycodes = <103 /* KEY_UP */
+ 106 /* KEY_RIGHT */
+ 108 /* KEY_DOWN */
+ 105 /* KEY_LEFT */
+ 109 /* KEY_PAGEDOWN */
+ 104>; /* KEY_PAGEUP */
+ };
+}
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index f7e79b4..a3958c6 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -665,4 +665,14 @@ config KEYBOARD_CROS_EC
To compile this driver as a module, choose M here: the
module will be called cros_ec_keyb.
+config KEYBOARD_CAP1106
+ tristate "Microchip CAP1106 touch sensor"
+ depends on OF && I2C
+ select REGMAP_I2C
+ help
+ Say Y here to enable the CAP1106 touch sensor driver.
+
+ To compile this driver as a module, choose M here: the
+ module will be called cap1106.
+
endif
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 7504ae1..0a33456 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_KEYBOARD_AMIGA) += amikbd.o
obj-$(CONFIG_KEYBOARD_ATARI) += atakbd.o
obj-$(CONFIG_KEYBOARD_ATKBD) += atkbd.o
obj-$(CONFIG_KEYBOARD_BFIN) += bf54x-keys.o
+obj-$(CONFIG_KEYBOARD_CAP1106) += cap1106.o
obj-$(CONFIG_KEYBOARD_CLPS711X) += clps711x-keypad.o
obj-$(CONFIG_KEYBOARD_CROS_EC) += cros_ec_keyb.o
obj-$(CONFIG_KEYBOARD_DAVINCI) += davinci_keyscan.o
diff --git a/drivers/input/keyboard/cap1106.c b/drivers/input/keyboard/cap1106.c
new file mode 100644
index 0000000..5b416be
--- /dev/null
+++ b/drivers/input/keyboard/cap1106.c
@@ -0,0 +1,347 @@
+/*
+ * Input driver for Microchip CAP1106, 6 channel capacitive touch sensor
+ *
+ * http://www.microchip.com/wwwproducts/Devices.aspx?product=CAP1106
+ *
+ * (c) 2014 Daniel Mack <linux@zonque.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+#include <linux/of_irq.h>
+#include <linux/regmap.h>
+#include <linux/i2c.h>
+#include <linux/gpio/consumer.h>
+
+#define CAP1106_REG_MAIN_CONTROL 0x00
+#define CAP1106_REG_MAIN_CONTROL_GAIN_SHIFT (6)
+#define CAP1106_REG_MAIN_CONTROL_GAIN_MASK (0xc0)
+#define CAP1106_REG_MAIN_CONTROL_DLSEEP BIT(4)
+#define CAP1106_REG_GENERAL_STATUS 0x02
+#define CAP1106_REG_SENSOR_INPUT 0x03
+#define CAP1106_REG_NOISE_FLAG_STATUS 0x0a
+#define CAP1106_REG_SENOR_DELTA(X) (0x10 + (X))
+#define CAP1106_REG_SENSITIVITY_CONTROL 0x1f
+#define CAP1106_REG_CONFIG 0x20
+#define CAP1106_REG_SENSOR_ENABLE 0x21
+#define CAP1106_REG_SENSOR_CONFIG 0x22
+#define CAP1106_REG_SENSOR_CONFIG2 0x23
+#define CAP1106_REG_SAMPLING_CONFIG 0x24
+#define CAP1106_REG_CALIBRATION 0x25
+#define CAP1106_REG_INT_ENABLE 0x26
+#define CAP1106_REG_REPEAT_RATE 0x28
+#define CAP1106_REG_MT_CONFIG 0x2a
+#define CAP1106_REG_MT_PATTERN_CONFIG 0x2b
+#define CAP1106_REG_MT_PATTERN 0x2d
+#define CAP1106_REG_RECALIB_CONFIG 0x2f
+#define CAP1106_REG_SENSOR_THRESH(X) (0x30 + (X))
+#define CAP1106_REG_SENSOR_NOISE_THRESH 0x38
+#define CAP1106_REG_STANDBY_CHANNEL 0x40
+#define CAP1106_REG_STANDBY_CONFIG 0x41
+#define CAP1106_REG_STANDBY_SENSITIVITY 0x42
+#define CAP1106_REG_STANDBY_THRESH 0x43
+#define CAP1106_REG_CONFIG2 0x44
+#define CAP1106_REG_SENSOR_BASE_CNT(X) (0x50 + (X))
+#define CAP1106_REG_SENSOR_CALIB (0xb1 + (X))
+#define CAP1106_REG_SENSOR_CALIB_LSB1 0xb9
+#define CAP1106_REG_SENSOR_CALIB_LSB2 0xba
+#define CAP1106_REG_PRODUCT_ID 0xfd
+#define CAP1106_REG_MANUFACTURER_ID 0xfe
+#define CAP1106_REG_REVISION 0xff
+
+#define CAP1106_NUM_CHN 6
+#define CAP1106_PRODUCT_ID 0x55
+#define CAP1106_MANUFACTURER_ID 0x5d
+
+struct cap1106_priv {
+ struct regmap *regmap;
+ struct input_dev *idev;
+
+ /* config */
+ unsigned int keycodes[CAP1106_NUM_CHN];
+};
+
+static const struct reg_default cap1106_reg_defaults[] = {
+ { CAP1106_REG_MAIN_CONTROL, 0x00 },
+ { CAP1106_REG_GENERAL_STATUS, 0x00 },
+ { CAP1106_REG_SENSOR_INPUT, 0x00 },
+ { CAP1106_REG_NOISE_FLAG_STATUS, 0x00 },
+ { CAP1106_REG_SENSITIVITY_CONTROL, 0x2f },
+ { CAP1106_REG_CONFIG, 0x20 },
+ { CAP1106_REG_SENSOR_ENABLE, 0x3f },
+ { CAP1106_REG_SENSOR_CONFIG, 0xa4 },
+ { CAP1106_REG_SENSOR_CONFIG2, 0x07 },
+ { CAP1106_REG_SAMPLING_CONFIG, 0x39 },
+ { CAP1106_REG_CALIBRATION, 0x00 },
+ { CAP1106_REG_INT_ENABLE, 0x3f },
+ { CAP1106_REG_REPEAT_RATE, 0x3f },
+ { CAP1106_REG_MT_CONFIG, 0x80 },
+ { CAP1106_REG_MT_PATTERN_CONFIG, 0x00 },
+ { CAP1106_REG_MT_PATTERN, 0x3f },
+ { CAP1106_REG_RECALIB_CONFIG, 0x8a },
+ { CAP1106_REG_SENSOR_THRESH(0), 0x40 },
+ { CAP1106_REG_SENSOR_THRESH(1), 0x40 },
+ { CAP1106_REG_SENSOR_THRESH(2), 0x40 },
+ { CAP1106_REG_SENSOR_THRESH(3), 0x40 },
+ { CAP1106_REG_SENSOR_THRESH(4), 0x40 },
+ { CAP1106_REG_SENSOR_THRESH(5), 0x40 },
+ { CAP1106_REG_SENSOR_NOISE_THRESH, 0x01 },
+ { CAP1106_REG_STANDBY_CHANNEL, 0x00 },
+ { CAP1106_REG_STANDBY_CONFIG, 0x39 },
+ { CAP1106_REG_STANDBY_SENSITIVITY, 0x02 },
+ { CAP1106_REG_STANDBY_THRESH, 0x40 },
+ { CAP1106_REG_CONFIG2, 0x40 },
+ { CAP1106_REG_SENSOR_CALIB_LSB1, 0x00 },
+ { CAP1106_REG_SENSOR_CALIB_LSB2, 0x00 },
+};
+
+static bool cap1106_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case CAP1106_REG_MAIN_CONTROL:
+ case CAP1106_REG_SENSOR_INPUT:
+ case CAP1106_REG_SENOR_DELTA(0):
+ case CAP1106_REG_SENOR_DELTA(1):
+ case CAP1106_REG_SENOR_DELTA(2):
+ case CAP1106_REG_SENOR_DELTA(3):
+ case CAP1106_REG_SENOR_DELTA(4):
+ case CAP1106_REG_SENOR_DELTA(5):
+ case CAP1106_REG_PRODUCT_ID:
+ case CAP1106_REG_MANUFACTURER_ID:
+ case CAP1106_REG_REVISION:
+ return true;
+ }
+
+ return false;
+}
+
+static const struct regmap_config cap1106_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+
+ .max_register = CAP1106_REG_REVISION,
+ .reg_defaults = cap1106_reg_defaults,
+
+ .num_reg_defaults = ARRAY_SIZE(cap1106_reg_defaults),
+ .cache_type = REGCACHE_RBTREE,
+ .volatile_reg = cap1106_volatile_reg,
+};
+
+static irqreturn_t cap1106_thread_func(int irq_num, void *data)
+{
+ struct cap1106_priv *priv = data;
+ unsigned int status;
+ int ret, i;
+
+ /*
+ * Deassert interrupt. This needs to be done before reading the status
+ * registers, which will not carry valid values otherwise.
+ */
+ ret = regmap_update_bits(priv->regmap, CAP1106_REG_MAIN_CONTROL, 1, 0);
+ if (ret < 0)
+ goto out;
+
+ ret = regmap_read(priv->regmap, CAP1106_REG_SENSOR_INPUT, &status);
+ if (ret < 0)
+ goto out;
+
+ for (i = 0; i < CAP1106_NUM_CHN; i++)
+ input_report_key(priv->idev, priv->keycodes[i],
+ status & (1 << i));
+
+ input_sync(priv->idev);
+
+out:
+ return IRQ_HANDLED;
+}
+
+static int cap1106_set_sleep(struct cap1106_priv *priv, bool sleep)
+{
+ return regmap_update_bits(priv->regmap, CAP1106_REG_MAIN_CONTROL,
+ CAP1106_REG_MAIN_CONTROL_DLSEEP,
+ sleep ? CAP1106_REG_MAIN_CONTROL_DLSEEP : 0);
+}
+
+static int cap1106_input_open(struct input_dev *idev)
+{
+ struct cap1106_priv *priv = input_get_drvdata(idev);
+
+ return cap1106_set_sleep(priv, false);
+}
+
+static void cap1106_input_close(struct input_dev *idev)
+{
+ struct cap1106_priv *priv = input_get_drvdata(idev);
+
+ cap1106_set_sleep(priv, true);
+}
+
+static int cap1106_i2c_probe(struct i2c_client *i2c_client,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &i2c_client->dev;
+ struct cap1106_priv *priv;
+ struct device_node *node;
+ int i, ret, irq, gain = 0;
+ unsigned int val, rev;
+ u32 gain32, keycodes[CAP1106_NUM_CHN];
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->regmap = devm_regmap_init_i2c(i2c_client, &cap1106_regmap_config);
+ if (IS_ERR(priv->regmap))
+ return PTR_ERR(priv->regmap);
+
+ ret = regmap_read(priv->regmap, CAP1106_REG_PRODUCT_ID, &val);
+ if (ret < 0)
+ return ret;
+
+ if (val != CAP1106_PRODUCT_ID) {
+ dev_err(dev, "Product ID: Got 0x%02x, expected 0x%02x\n",
+ val, CAP1106_PRODUCT_ID);
+ return -ENODEV;
+ }
+
+ ret = regmap_read(priv->regmap, CAP1106_REG_MANUFACTURER_ID, &val);
+ if (ret < 0)
+ return ret;
+
+ if (val != CAP1106_MANUFACTURER_ID) {
+ dev_err(dev, "Manufacturer ID: Got 0x%02x, expected 0x%02x\n",
+ val, CAP1106_MANUFACTURER_ID);
+ return -ENODEV;
+ }
+
+ ret = regmap_read(priv->regmap, CAP1106_REG_REVISION, &rev);
+ if (ret < 0)
+ return ret;
+
+ dev_info(dev, "CAP1106 detected, revision 0x%02x\n", rev);
+ i2c_set_clientdata(i2c_client, priv);
+ node = dev->of_node;
+
+ if (!of_property_read_u32(node, "microchip,sensor-gain", &gain32)) {
+ if (is_power_of_2(gain32) && gain32 <= 8)
+ gain = ilog2(gain32);
+ else
+ dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
+ }
+
+ BUILD_BUG_ON(ARRAY_SIZE(keycodes) != ARRAY_SIZE(priv->keycodes));
+
+ /* Provide some useful defaults */
+ for (i = 0; i < ARRAY_SIZE(keycodes); i++)
+ keycodes[i] = KEY_A + i;
+
+ of_property_read_u32_array(node, "linux,keycodes",
+ keycodes, ARRAY_SIZE(keycodes));
+
+ for (i = 0; i < ARRAY_SIZE(keycodes); i++)
+ priv->keycodes[i] = keycodes[i];
+
+ ret = regmap_update_bits(priv->regmap, CAP1106_REG_MAIN_CONTROL,
+ CAP1106_REG_MAIN_CONTROL_GAIN_MASK,
+ gain << CAP1106_REG_MAIN_CONTROL_GAIN_SHIFT);
+ if (ret < 0)
+ return ret;
+
+ /* Disable autorepeat. The Linux input system has its own handling. */
+ ret = regmap_write(priv->regmap, CAP1106_REG_REPEAT_RATE, 0);
+ if (ret < 0)
+ return ret;
+
+ priv->idev = devm_input_allocate_device(dev);
+ if (!priv->idev)
+ return -ENOMEM;
+
+ priv->idev->name = "CAP1106 capacitive touch sensor";
+ priv->idev->id.bustype = BUS_I2C;
+ priv->idev->evbit[0] = BIT_MASK(EV_KEY);
+
+ if (of_property_read_bool(node, "autorepeat"))
+ __set_bit(EV_REP, priv->idev->evbit);
+
+ for (i = 0; i < CAP1106_NUM_CHN; i++) {
+ unsigned int code = priv->keycodes[i];
+ priv->idev->keybit[BIT_WORD(code)] |= BIT_MASK(code);
+ }
+
+ priv->idev->id.vendor = CAP1106_MANUFACTURER_ID;
+ priv->idev->id.product = CAP1106_PRODUCT_ID;
+ priv->idev->id.version = rev;
+
+ priv->idev->open = cap1106_input_open;
+ priv->idev->close = cap1106_input_close;
+
+ input_set_drvdata(priv->idev, priv);
+
+ /*
+ * Put the device in deep sleep mode for now.
+ * ->open() will bring it back once the it is actually needed.
+ */
+ cap1106_set_sleep(priv, true);
+
+ ret = input_register_device(priv->idev);
+ if (ret < 0)
+ return ret;
+
+ irq = irq_of_parse_and_map(node, 0);
+ if (!irq) {
+ dev_err(dev, "Unable to parse or map IRQ\n");
+ return -ENXIO;
+ }
+
+ ret = devm_request_threaded_irq(dev, irq, NULL, cap1106_thread_func,
+ IRQF_ONESHOT, dev_name(dev), priv);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int cap1106_i2c_remove(struct i2c_client *i2c_client)
+{
+ struct cap1106_priv *priv = i2c_get_clientdata(i2c_client);
+
+ input_unregister_device(priv->idev);
+
+ return 0;
+}
+
+static const struct of_device_id cap1106_dt_ids[] = {
+ { .compatible = "microchip,cap1106", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, cap1106_dt_ids);
+
+static const struct i2c_device_id cap1106_i2c_ids[] = {
+ { "cap1106", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, cap1106_i2c_ids);
+
+static struct i2c_driver cap1106_i2c_driver = {
+ .driver = {
+ .name = "cap1106",
+ .owner = THIS_MODULE,
+ .of_match_table = cap1106_dt_ids,
+ },
+ .id_table = cap1106_i2c_ids,
+ .probe = cap1106_i2c_probe,
+ .remove = cap1106_i2c_remove,
+};
+
+module_i2c_driver(cap1106_i2c_driver);
+
+MODULE_ALIAS("platform:" DRV_NAME);
+MODULE_DESCRIPTION("Microchip CAP1106 driver");
+MODULE_AUTHOR("Daniel Mack <linux@zonque.org>");
+MODULE_LICENSE("GPL v2");
--
1.9.3
^ permalink raw reply related
* Re: [PATCH v4 4/4] Add support for Compact (Bluetooth|USB) keyboard with Trackpoint
From: Antonio Ospite @ 2014-07-14 10:21 UTC (permalink / raw)
To: Jamie Lentin; +Cc: Jiri Kosina, Hans de Goede, linux-input, linux-kernel
In-Reply-To: <1405236262-8070-5-git-send-email-jm@lentin.co.uk>
On Sun, 13 Jul 2014 08:24:22 +0100
Jamie Lentin <jm@lentin.co.uk> wrote:
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
Almost there, I swear :)
> ---
> Documentation/ABI/testing/sysfs-driver-hid-lenovo | 12 ++
> drivers/hid/Kconfig | 2 +
> drivers/hid/hid-core.c | 2 +
> drivers/hid/hid-ids.h | 2 +
> drivers/hid/hid-lenovo.c | 203 ++++++++++++++++++++++
> include/linux/hid.h | 1 +
> 6 files changed, 222 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lenovo b/Documentation/ABI/testing/sysfs-driver-hid-lenovo
> index 57b92cb..53a0725 100644
> --- a/Documentation/ABI/testing/sysfs-driver-hid-lenovo
> +++ b/Documentation/ABI/testing/sysfs-driver-hid-lenovo
> @@ -4,18 +4,21 @@ Contact: linux-input@vger.kernel.org
> Description: This controls if mouse clicks should be generated if the trackpoint is quickly pressed. How fast this press has to be
> is being controlled by press_speed.
> Values are 0 or 1.
> + Applies to Thinkpad USB Keyboard with TrackPoint.
>
> What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/dragging
> Date: July 2011
> Contact: linux-input@vger.kernel.org
> Description: If this setting is enabled, it is possible to do dragging by pressing the trackpoint. This requires press_to_select to be enabled.
> Values are 0 or 1.
> + Applies to Thinkpad USB Keyboard with TrackPoint.
>
> What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/release_to_select
> Date: July 2011
> Contact: linux-input@vger.kernel.org
> Description: For details regarding this setting please refer to http://www.pc.ibm.com/ww/healthycomputing/trkpntb.html
> Values are 0 or 1.
> + Applies to Thinkpad USB Keyboard with TrackPoint.
>
> What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/select_right
> Date: July 2011
> @@ -23,16 +26,25 @@ Contact: linux-input@vger.kernel.org
> Description: This setting controls if the mouse click events generated by pressing the trackpoint (if press_to_select is enabled) generate
> a left or right mouse button click.
> Values are 0 or 1.
> + Applies to Thinkpad USB Keyboard with TrackPoint.
>
> What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/sensitivity
> Date: July 2011
> Contact: linux-input@vger.kernel.org
> Description: This file contains the trackpoint sensitivity.
> Values are decimal integers from 1 (lowest sensitivity) to 255 (highest sensitivity).
> + Applies to Thinkpad USB Keyboard with TrackPoint.
>
> What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/press_speed
> Date: July 2011
> Contact: linux-input@vger.kernel.org
> Description: This setting controls how fast the trackpoint needs to be pressed to generate a mouse click if press_to_select is enabled.
> Values are decimal integers from 1 (slowest) to 255 (fastest).
> + Applies to Thinkpad USB Keyboard with TrackPoint.
>
> +What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/fn_lock
> +Date: July 2014
> +Contact: linux-input@vger.kernel.org
> +Description: This setting controls whether Fn Lock is enabled on the keyboard (i.e. if F1 is Mute or F1)
> + Values are 0 or 1
> + Applies to ThinkPad Compact (USB|Bluetooth) Keyboard with TrackPoint.
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 9b7acfc..1e19292 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -343,6 +343,8 @@ config HID_LENOVO
> Thinkpad standalone keyboards, e.g:
> - ThinkPad USB Keyboard with TrackPoint (supports extra LEDs and trackpoint
> configuration)
> + - ThinkPad Compact Bluetooth Keyboard with TrackPoint (supports Fn keys)
> + - ThinkPad Compact USB Keyboard with TrackPoint (supports Fn keys)
>
> config HID_LOGITECH
> tristate "Logitech devices" if EXPERT
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 55841bd..81b3bb6 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1798,6 +1798,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_LCPOWER, USB_DEVICE_ID_LCPOWER_LC1000 ) },
> #if IS_ENABLED(CONFIG_HID_LENOVO)
> { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_CUSBKBD) },
> + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_CBTKBD) },
> #endif
> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER) },
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 6d00bb9..d2e2a96 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -560,6 +560,8 @@
>
> #define USB_VENDOR_ID_LENOVO 0x17ef
> #define USB_DEVICE_ID_LENOVO_TPKBD 0x6009
> +#define USB_DEVICE_ID_LENOVO_CUSBKBD 0x6047
> +#define USB_DEVICE_ID_LENOVO_CBTKBD 0x6048
>
> #define USB_VENDOR_ID_LG 0x1fd2
> #define USB_DEVICE_ID_LG_MULTITOUCH 0x0064
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index a1a693c..46c5db0 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -1,8 +1,11 @@
> /*
> * HID driver for Lenovo:
> * - ThinkPad USB Keyboard with TrackPoint (tpkbd)
> + * - ThinkPad Compact Bluetooth Keyboard with TrackPoint (cptkbd)
> + * - ThinkPad Compact USB Keyboard with TrackPoint (cptkbd)
> *
> * Copyright (c) 2012 Bernhard Seibold
> + * Copyright (c) 2014 Jamie Lentin <jm@lentin.co.uk>
> */
>
> /*
> @@ -33,6 +36,10 @@ struct lenovo_drvdata_tpkbd {
> int press_speed;
> };
>
> +struct lenovo_drvdata_cptkbd {
> + bool fn_lock;
> +};
> +
> #define map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
>
> static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
> @@ -48,6 +55,49 @@ static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
> return 0;
> }
>
> +static int lenovo_input_mapping_cptkbd(struct hid_device *hdev,
> + struct hid_input *hi, struct hid_field *field,
> + struct hid_usage *usage, unsigned long **bit, int *max)
> +{
> + /* HID_UP_LNVENDOR = USB, HID_UP_MSVENDOR = BT */
> + if ((usage->hid & HID_USAGE_PAGE) == HID_UP_MSVENDOR ||
> + (usage->hid & HID_USAGE_PAGE) == HID_UP_LNVENDOR) {
> + set_bit(EV_REP, hi->input->evbit);
> + switch (usage->hid & HID_USAGE) {
> + case 0x00f1: /* Fn-F4: Mic mute */
> + map_key_clear(KEY_MICMUTE);
> + return 1;
> + case 0x00f2: /* Fn-F5: Brightness down */
> + map_key_clear(KEY_BRIGHTNESSDOWN);
> + return 1;
> + case 0x00f3: /* Fn-F6: Brightness up */
> + map_key_clear(KEY_BRIGHTNESSUP);
> + return 1;
> + case 0x00f4: /* Fn-F7: External display (projector) */
> + map_key_clear(KEY_SWITCHVIDEOMODE);
> + return 1;
> + case 0x00f5: /* Fn-F8: Wireless */
> + map_key_clear(KEY_WLAN);
> + return 1;
> + case 0x00f6: /* Fn-F9: Control panel */
> + map_key_clear(KEY_CONFIG);
> + return 1;
> + case 0x00f8: /* Fn-F11: View open applications (3 boxes) */
> + map_key_clear(KEY_SCALE);
> + return 1;
> + case 0x00fa: /* Fn-Esc: Fn-lock toggle */
> + map_key_clear(KEY_FN_ESC);
> + return 1;
> + case 0x00fb: /* Fn-F12: Open My computer (6 boxes) USB-only */
> + /* NB: This mapping is invented in raw_event below */
> + map_key_clear(KEY_FILE);
> + return 1;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int lenovo_input_mapping(struct hid_device *hdev,
> struct hid_input *hi, struct hid_field *field,
> struct hid_usage *usage, unsigned long **bit, int *max)
> @@ -56,6 +106,10 @@ static int lenovo_input_mapping(struct hid_device *hdev,
> case USB_DEVICE_ID_LENOVO_TPKBD:
> return lenovo_input_mapping_tpkbd(hdev, hi, field,
> usage, bit, max);
> + case USB_DEVICE_ID_LENOVO_CUSBKBD:
> + case USB_DEVICE_ID_LENOVO_CBTKBD:
> + return lenovo_input_mapping_cptkbd(hdev, hi, field,
> + usage, bit, max);
> }
>
> return 0;
> @@ -63,6 +117,100 @@ static int lenovo_input_mapping(struct hid_device *hdev,
>
> #undef map_key_clear
>
> +/* Send a config command to the keyboard */
> +static int lenovo_send_cmd_cptkbd(struct hid_device *hdev,
> + unsigned char byte2, unsigned char byte3)
> +{
> + int ret;
> + unsigned char buf[] = {0x18, byte2, byte3};
> +
> + switch (hdev->product) {
> + case USB_DEVICE_ID_LENOVO_CUSBKBD:
> + ret = hid_hw_raw_request(hdev, 0x13, buf, sizeof(buf),
> + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> + break;
> + case USB_DEVICE_ID_LENOVO_CBTKBD:
> + ret = hid_hw_output_report(hdev, buf, sizeof(buf));
> + break;
> + default:
> + ret = -EINVAL;
>
Add an explicit break? For regularity.
> + }
> +
> + return ret < 0 ? ret : 0; /* BT returns 0, USB returns sizeof(buf) */
> +}
Alternatively you could just return 'ret' and check for < 0 in the
callers, leaving the comment and adding "On Success ...".
BTW, you never actually propagate the negative error codes.
I you don't want this function to just return bool (e.g. return (ret >=
0)), at least print the returned error code in the messages after the
callers.
> +
> +static void lenovo_features_set_cptkbd(struct hid_device *hdev)
> +{
> + struct lenovo_drvdata_cptkbd *tpcsc = hid_get_drvdata(hdev);
> +
> + if (lenovo_send_cmd_cptkbd(hdev, 0x05, tpcsc->fn_lock ? 0x01 : 0x00))
Try to avoid function calls in conditions.
> + hid_err(hdev, "Fn-lock setting failed\n");
Print 'ret' if lenovo_send_cmd_cptkbd() returns the negative error
codes.
> +}
> +
> +static ssize_t attr_fn_lock_show_cptkbd(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> + struct lenovo_drvdata_cptkbd *tpcsc = hid_get_drvdata(hdev);
> +
> + return snprintf(buf, PAGE_SIZE, "%u\n", tpcsc->fn_lock);
> +}
> +
> +static ssize_t attr_fn_lock_store_cptkbd(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> + struct lenovo_drvdata_cptkbd *tpcsc = hid_get_drvdata(hdev);
> + int value;
> +
> + if (kstrtoint(buf, 10, &value))
Function called in condition; however this is consistent with the other
invocation of kstrtoint() in the file, so you can leave it as it is as
an exception to the general rule.
> + return -EINVAL;
> + if (value < 0 || value > 1)
> + return -EINVAL;
> +
> + tpcsc->fn_lock = !!value;
> + lenovo_features_set_cptkbd(hdev);
> +
> + return count;
> +}
> +
> +static struct device_attribute dev_attr_fn_lock_cptkbd =
> + __ATTR(fn_lock, S_IWUSR | S_IRUGO,
> + attr_fn_lock_show_cptkbd,
> + attr_fn_lock_store_cptkbd);
> +
> +static struct attribute *lenovo_attributes_cptkbd[] = {
> + &dev_attr_fn_lock_cptkbd.attr,
> + NULL
> +};
> +
> +static const struct attribute_group lenovo_attr_group_cptkbd = {
> + .attrs = lenovo_attributes_cptkbd,
> +};
> +
> +static int lenovo_raw_event(struct hid_device *hdev,
> + struct hid_report *report, u8 *data, int size)
> +{
> + /*
> + * Compact USB keyboard's Fn-F12 report holds down many other keys, and
> + * its own key is outside the usage page range. Remove extra
> + * keypresses and remap to inside usage page.
> + */
> + if (unlikely(hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD
> + && size == 3
> + && data[0] == 0x15
> + && data[1] == 0x94
> + && data[2] == 0x01)) {
> + data[1] = 0x0;
> + data[2] = 0x4;
> + }
> +
> + return 0;
> +}
> +
> static int lenovo_features_set_tpkbd(struct hid_device *hdev)
> {
> struct hid_report *report;
> @@ -415,6 +563,44 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
> return 0;
> }
>
> +static int lenovo_probe_cptkbd(struct hid_device *hdev)
> +{
> + int ret;
> + struct lenovo_drvdata_cptkbd *tpcsc;
> +
> + tpcsc = devm_kzalloc(&hdev->dev, sizeof(*tpcsc), GFP_KERNEL);
> + if (tpcsc == NULL) {
> + hid_err(hdev, "can't alloc keyboard descriptor\n");
> + return -ENOMEM;
> + }
> + hid_set_drvdata(hdev, tpcsc);
> +
> + /* All the custom action happens on the mouse device for USB */
> + if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD
> + && hdev->type != HID_TYPE_USBMOUSE) {
> + hid_dbg(hdev, "Ignoring keyboard half of device\n");
> + return 0;
> + }
Maybe you can anticipate this check and avoid allocation in this case?
Not a big deal anyway.
> +
> + /*
> + * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
> + * regular keys
> + */
> + ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
> + if (ret)
> + hid_warn(hdev, "Failed to switch F7/9/11 into regular keys\n");
Print 'ret' here.
> +
> + /* Turn Fn-Lock on by default */
> + tpcsc->fn_lock = 1;
tpcsc->fn_lock = true;
> + lenovo_features_set_cptkbd(hdev);
> +
> + if (sysfs_create_group(&hdev->dev.kobj,
> + &lenovo_attr_group_cptkbd))
Function called in condition.
> + hid_warn(hdev, "Could not create sysfs group\n");
> +
> + return 0;
> +}
> +
> static int lenovo_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
> {
> @@ -436,6 +622,10 @@ static int lenovo_probe(struct hid_device *hdev,
> case USB_DEVICE_ID_LENOVO_TPKBD:
> ret = lenovo_probe_tpkbd(hdev);
> break;
> + case USB_DEVICE_ID_LENOVO_CUSBKBD:
> + case USB_DEVICE_ID_LENOVO_CBTKBD:
> + ret = lenovo_probe_cptkbd(hdev);
> + break;
> }
> if (ret)
> goto err_hid;
> @@ -463,12 +653,22 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
> hid_set_drvdata(hdev, NULL);
> }
>
> +static void lenovo_remove_cptkbd(struct hid_device *hdev)
> +{
> + sysfs_remove_group(&hdev->dev.kobj,
> + &lenovo_attr_group_cptkbd);
> +}
> +
> static void lenovo_remove(struct hid_device *hdev)
> {
> switch (hdev->product) {
> case USB_DEVICE_ID_LENOVO_TPKBD:
> lenovo_remove_tpkbd(hdev);
> break;
> + case USB_DEVICE_ID_LENOVO_CUSBKBD:
> + case USB_DEVICE_ID_LENOVO_CBTKBD:
> + lenovo_remove_cptkbd(hdev);
> + break;
> }
>
> hid_hw_stop(hdev);
> @@ -476,6 +676,8 @@ static void lenovo_remove(struct hid_device *hdev)
>
> static const struct hid_device_id lenovo_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_CUSBKBD) },
> + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_CBTKBD) },
> { }
> };
>
> @@ -487,6 +689,7 @@ static struct hid_driver lenovo_driver = {
> .input_mapping = lenovo_input_mapping,
> .probe = lenovo_probe,
> .remove = lenovo_remove,
> + .raw_event = lenovo_raw_event,
> };
> module_hid_driver(lenovo_driver);
>
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 77632cf..fca74f1 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -167,6 +167,7 @@ struct hid_item {
> #define HID_UP_MSVENDOR 0xff000000
> #define HID_UP_CUSTOM 0x00ff0000
> #define HID_UP_LOGIVENDOR 0xffbc0000
> +#define HID_UP_LNVENDOR 0xffa00000
> #define HID_UP_SENSOR 0x00200000
>
> #define HID_USAGE 0x0000ffff
> --
> 2.0.0
>
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH v2] Input: Add driver for Microchip's CAP1106
From: Daniel Mack @ 2014-07-14 10:20 UTC (permalink / raw)
To: Mark Rutland
Cc: dtor@mail.ru, linux-input@vger.kernel.org, broonie@kernel.org,
dh.herrmann@gmail.com, devicetree@vger.kernel.org
In-Reply-To: <20140714095247.GB4980@leverpostej>
Hi Mark,
thanks a lot for your feedback! Much appreciated.
On 07/14/2014 11:52 AM, Mark Rutland wrote:
> On Fri, Jul 11, 2014 at 11:06:33AM +0100, Daniel Mack wrote:
>> +++ b/Documentation/devicetree/bindings/input/cap1106.txt
>> @@ -0,0 +1,63 @@
>> +Device tree bindings for Microchip CAP1106, 6 channel capacitive touch sensor
>> +
>> +The node for this driver must be a child of a I2C controller node, as the
>> +device communication via I2C only.
>> +
>> +Required properties:
>> +
>> + compatible: Must be "microchip,cap1106"
>> + reg: The I2C slave address of the device.
>> + Only 0x28 is valid.
>> + interrupt: Node describing the interrupt line the device's
>> + ALERT#/CM_IRQ# pin is connected to.
>
> s/interrupt/interrupts/
>
> This is a _property_, not a node.
Yes, I reworded that description a couple of times. Together with
interrupt-parent, it's actually a property referencing a node ;) Will fix.
> I take it the device only has a single interrupt line?
Yes.
>> +
>> +Optional properties:
>> +
>> + autorepeat: Enables the Linux input system's autorepeat
>> + feature on the input device.
>> + microchip,sensor-gain: Defines the gain of the sensor circuitry. This
>> + effectively controls the sensitivity, as a
>> + smaller delta capacitance is required to
>> + generate the same delta count values.
>> + Valid values are 1, 2, 4, and 8.
>> + By default, a gain of 1 is set.
>
> Does the official documentation describe this in absolute terms?
The documentation describes the gain feature as "1, 2, 4, or 8", so I
kept it like this in the bindings. Internally, the register stores that
value in 2 bits. The driver takes care for the translation.
>> +To define details of each individual button channel, six subnodes can be
>> +specified. Inside each of those, the following property is valid:
>> +
>> + linux,keycode: Specify the numeric identifier of the keycode to be
>> + generated when this channel is activated. If this
>> + property is omitted, KEY_A, KEY_B, etc are used as
>> + defaults.
>
> What are those subnodes, and how are they told apart? Name, compatible?
>
> I strongly recommend against relying on the order of the DT. It's very
> easy for that to get messed up and makes things hard to read.
>
> Is is not possible to use linux,keymap and treat the buttons as a
> matrix keypad? Then you don't need subnodes at all, and you can have a
> sparse keymap if you want.
Hmm, I thought about that, but there are - in theory - more details that
could be specified per channel. I left those functions out for the first
version, as I have no good way to test them.
Hence, my idea was to have everything related to one channel nicely
grouped in a subnode. I've also seen bindings that rely on the order of
subnodes before, for example in regulator drivers.
But I agree with you that it makes board definitions error-prone.
linux,keycode feels a bit overkill here though, so I'd rather go for a
fixed-size linux,keycodes property. The number of entries is fixed,
anyway. Would you be fine with that?
>> +struct cap1106_priv {
>> + struct regmap *regmap;
>> + struct input_dev *idev;
>> + struct work_struct work;
>> + spinlock_t lock;
>> + bool closed:1;
>
> I don't think you're saving anything here by trying to have a bool
> bitfield.
That variable has been dropped in v3 already :)
>> + i = 0;
>> + for_each_child_of_node(node, child) {
>> + if (i == CAP1106_NUM_CHN) {
>> + dev_err(dev, "Too many button nodes.\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!of_property_read_u32(child, "linux,keycode", &tmp32))
>> + priv->keycodes[i] = tmp32;
>> +
>> + i++;
>> + }
>
> I don't think that it is a good idea to rely on the order of sub-nodes
> in the DTB.
Jup, I agree. As mentioned above, I'd like to solve that with
linux,keycode for now.
> [...]
>
>> + if (of_get_property(node, "autorepeat", NULL))
>> + __set_bit(EV_REP, priv->idev->evbit);
>
> Use of_property_read_bool.
Will do, thanks.
> /me mutters usual grumblings about the autorepeat property.
I took that from the gpio-keys driver. Is there a better way to denote
such a feature?
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH v4 3/4] Style fixes
From: Antonio Ospite @ 2014-07-14 10:19 UTC (permalink / raw)
To: Jamie Lentin; +Cc: Jiri Kosina, Hans de Goede, linux-input, linux-kernel
In-Reply-To: <1405236262-8070-4-git-send-email-jm@lentin.co.uk>
On Sun, 13 Jul 2014 08:24:21 +0100
Jamie Lentin <jm@lentin.co.uk> wrote:
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
Reviewed-by: Antonio Ospite <ao2@ao2.it>
> ---
> drivers/hid/hid-lenovo.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index d11e337..a1a693c 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -349,7 +349,7 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
> struct lenovo_drvdata_tpkbd *data_pointer;
> size_t name_sz = strlen(dev_name(dev)) + 16;
> char *name_mute, *name_micmute;
> - int i;
> + int i, ret;
Preferred style is one declaration per line, but this style is also
quite common in kernel, so it's not a big deal.
>
> /*
> * If this is the pointer half of the keyboard, input_mapping should
> @@ -369,10 +369,9 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
> if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 3, 0, 2))
> return -ENODEV;
>
> - if (sysfs_create_group(&hdev->dev.kobj,
> - &lenovo_attr_group_tpkbd)) {
> + ret = sysfs_create_group(&hdev->dev.kobj, &lenovo_attr_group_tpkbd);
> + if (ret)
> hid_warn(hdev, "Could not create sysfs group\n");
> - }
>
> data_pointer = devm_kzalloc(&hdev->dev,
> sizeof(struct lenovo_drvdata_tpkbd),
> --
> 2.0.0
>
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH v4 2/4] Make all base functions switch depending on product ID
From: Antonio Ospite @ 2014-07-14 10:19 UTC (permalink / raw)
To: Jamie Lentin; +Cc: Jiri Kosina, Hans de Goede, linux-input, linux-kernel
In-Reply-To: <1405236262-8070-3-git-send-email-jm@lentin.co.uk>
On Sun, 13 Jul 2014 08:24:20 +0100
Jamie Lentin <jm@lentin.co.uk> wrote:
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
The commit message could be improved, it's not obvious to me what "base
functions" means here, I am thinking to something like the following:
---------------------------------------------------------------------
HID: lenovo: prepare support for adding other devices
Add a common lenovo_input_mapping(), and call device specific functions
conditionally in order to ease the task of adding support for other
devices.
---------------------------------------------------------------------
This way you explain the WHAT and WHY and point that this is a
_preparatory_ patch, the HOW is explained by the code anyway.
> ---
> drivers/hid/hid-lenovo.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 0320b96..d11e337 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -1,5 +1,6 @@
> /*
> - * HID driver for Lenovo ThinkPad USB Keyboard with TrackPoint
> + * HID driver for Lenovo:
> + * - ThinkPad USB Keyboard with TrackPoint (tpkbd)
> *
> * Copyright (c) 2012 Bernhard Seibold
> */
> @@ -47,6 +48,19 @@ static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
> return 0;
> }
>
> +static int lenovo_input_mapping(struct hid_device *hdev,
> + struct hid_input *hi, struct hid_field *field,
> + struct hid_usage *usage, unsigned long **bit, int *max)
> +{
> + switch (hdev->product) {
> + case USB_DEVICE_ID_LENOVO_TPKBD:
> + return lenovo_input_mapping_tpkbd(hdev, hi, field,
> + usage, bit, max);
> + }
> +
> + return 0;
> +}
> +
> #undef map_key_clear
>
> static int lenovo_features_set_tpkbd(struct hid_device *hdev)
> @@ -337,6 +351,16 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
> char *name_mute, *name_micmute;
> int i;
>
> + /*
> + * If this is the pointer half of the keyboard, input_mapping should
s/pointer/trackpoint/ ? ;)
> + * have set drvdata to 1. Otherwise, it's the keyboard which needs
> + * nothing special doing to it.
> + */
> + if (!hid_get_drvdata(hdev))
> + return 0;
> +
> + hid_set_drvdata(hdev, NULL);
> +
> /* Validate required reports. */
> for (i = 0; i < 4; i++) {
> if (!hid_validate_values(hdev, HID_FEATURE_REPORT, 4, i, 1))
> @@ -409,12 +433,13 @@ static int lenovo_probe(struct hid_device *hdev,
> goto err;
> }
>
> - if (hid_get_drvdata(hdev)) {
> - hid_set_drvdata(hdev, NULL);
> + switch (hdev->product) {
> + case USB_DEVICE_ID_LENOVO_TPKBD:
> ret = lenovo_probe_tpkbd(hdev);
> - if (ret)
> - goto err_hid;
> + break;
> }
> + if (ret)
> + goto err_hid;
>
It is not immediately clear what this "ret" contains; the code seems to
be correct because if "ret" contains the previous return value of
hid_hw_start() it's from a successful invocation, but it took me some
reasoning to figure it out.
I'd leave the check right after the call to lenovo_probe_tpkbd(), and
repeat it when you add the other case it's slightly more code but it's
way more readable, it does not force you to remember the past or reason
on the switch logic when you read the code.
Anyhow, if you really want to have only one check after the switch, what
about adding a default case to the former switch:
default:
ret = 0;
break;
Having always the default case is also a good practice IMVHO, consider
adding it to the other switch statements too.
> return 0;
> err_hid:
> @@ -430,6 +455,9 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
> sysfs_remove_group(&hdev->dev.kobj,
> &lenovo_attr_group_tpkbd);
>
> + if (data_pointer == NULL)
> + return;
> +
This is OK here, as sysfs_remove_group() is safe to call
unconditionally (it will give warnings where there are no groups
AFAICS), but it could also go at the very start of lenovo_remove_tpkbd
(). It would be more symmetrical because you create the sysfs entries
only for the trackpoint half in the probe function. Maybe add a comment
like the useful one in the probe function.
> led_classdev_unregister(&data_pointer->led_micmute);
> led_classdev_unregister(&data_pointer->led_mute);
>
> @@ -438,8 +466,11 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
>
> static void lenovo_remove(struct hid_device *hdev)
> {
> - if (hid_get_drvdata(hdev))
> + switch (hdev->product) {
> + case USB_DEVICE_ID_LENOVO_TPKBD:
> lenovo_remove_tpkbd(hdev);
> + break;
> + }
>
> hid_hw_stop(hdev);
> }
> @@ -454,7 +485,7 @@ MODULE_DEVICE_TABLE(hid, lenovo_devices);
> static struct hid_driver lenovo_driver = {
> .name = "lenovo",
> .id_table = lenovo_devices,
> - .input_mapping = lenovo_input_mapping_tpkbd,
> + .input_mapping = lenovo_input_mapping,
> .probe = lenovo_probe,
> .remove = lenovo_remove,
> };
> --
> 2.0.0
>
>
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH v2] Input: Add driver for Microchip's CAP1106
From: Mark Rutland @ 2014-07-14 9:52 UTC (permalink / raw)
To: Daniel Mack
Cc: dtor@mail.ru, linux-input@vger.kernel.org, broonie@kernel.org,
dh.herrmann@gmail.com, devicetree@vger.kernel.org
In-Reply-To: <1405073193-21448-1-git-send-email-zonque@gmail.com>
On Fri, Jul 11, 2014 at 11:06:33AM +0100, Daniel Mack wrote:
> This patch adds a driver for Microchips CAP1106, an I2C driven, 6-channel
> capacitive touch sensor.
>
> For now, only the capacitive buttons are supported, and no specific
> settings that can be tweaked for individual channels, except for the
> device-wide sensitivity gain. The defaults seem to work just fine out of
> the box, so I'll leave configurable parameters for someone who's in need
> of them and who can actually measure the impact. All registers are
> prepared, however. Many of them are just not used for now.
>
> The implementation does not make any attempt to be compatible to platform
> data driven boards, but fully depends on CONFIG_OF.
>
> Power management functions are also left for volounteers with the ability
> to actually test them.
>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
> v2:
> * Fix potential deadlocks pointed out by David.
>
> .../devicetree/bindings/input/cap1106.txt | 63 ++++
> drivers/input/keyboard/Kconfig | 10 +
> drivers/input/keyboard/Makefile | 1 +
> drivers/input/keyboard/cap1106.c | 378 +++++++++++++++++++++
> 4 files changed, 452 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/cap1106.txt
> create mode 100644 drivers/input/keyboard/cap1106.c
>
> diff --git a/Documentation/devicetree/bindings/input/cap1106.txt b/Documentation/devicetree/bindings/input/cap1106.txt
> new file mode 100644
> index 0000000..57f5af3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/cap1106.txt
> @@ -0,0 +1,63 @@
> +Device tree bindings for Microchip CAP1106, 6 channel capacitive touch sensor
> +
> +The node for this driver must be a child of a I2C controller node, as the
> +device communication via I2C only.
> +
> +Required properties:
> +
> + compatible: Must be "microchip,cap1106"
> + reg: The I2C slave address of the device.
> + Only 0x28 is valid.
> + interrupt: Node describing the interrupt line the device's
> + ALERT#/CM_IRQ# pin is connected to.
s/interrupt/interrupts/
This is a _property_, not a node.
I take it the device only has a single interrupt line?
> +
> +Optional properties:
> +
> + autorepeat: Enables the Linux input system's autorepeat
> + feature on the input device.
> + microchip,sensor-gain: Defines the gain of the sensor circuitry. This
> + effectively controls the sensitivity, as a
> + smaller delta capacitance is required to
> + generate the same delta count values.
> + Valid values are 1, 2, 4, and 8.
> + By default, a gain of 1 is set.
Does the official documentation describe this in absolute terms?
> +
> +To define details of each individual button channel, six subnodes can be
> +specified. Inside each of those, the following property is valid:
> +
> + linux,keycode: Specify the numeric identifier of the keycode to be
> + generated when this channel is activated. If this
> + property is omitted, KEY_A, KEY_B, etc are used as
> + defaults.
What are those subnodes, and how are they told apart? Name, compatible?
I strongly recommend against relying on the order of the DT. It's very
easy for that to get messed up and makes things hard to read.
Is is not possible to use linux,keymap and treat the buttons as a
matrix keypad? Then you don't need subnodes at all, and you can have a
sparse keymap if you want.
[...]
> +struct cap1106_priv {
> + struct regmap *regmap;
> + struct input_dev *idev;
> + struct work_struct work;
> + spinlock_t lock;
> + bool closed:1;
I don't think you're saving anything here by trying to have a bool
bitfield.
[...]
> + i = 0;
> + for_each_child_of_node(node, child) {
> + if (i == CAP1106_NUM_CHN) {
> + dev_err(dev, "Too many button nodes.\n");
> + return -EINVAL;
> + }
> +
> + if (!of_property_read_u32(child, "linux,keycode", &tmp32))
> + priv->keycodes[i] = tmp32;
> +
> + i++;
> + }
I don't think that it is a good idea to rely on the order of sub-nodes
in the DTB.
[...]
> + if (of_get_property(node, "autorepeat", NULL))
> + __set_bit(EV_REP, priv->idev->evbit);
Use of_property_read_bool.
/me mutters usual grumblings about the autorepeat property.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v4 1/4] Rename hid-lenovo-tpkbd to hid-lenovo
From: Antonio Ospite @ 2014-07-14 9:42 UTC (permalink / raw)
To: Jamie Lentin; +Cc: Jiri Kosina, Hans de Goede, linux-input, linux-kernel
In-Reply-To: <1405236262-8070-2-git-send-email-jm@lentin.co.uk>
On Sun, 13 Jul 2014 08:24:19 +0100
Jamie Lentin <jm@lentin.co.uk> wrote:
> Rename module and all functions within so we can add support for other
> keyboards in the same file. Rename the _tp postfix to _tpkbd, to
> signify functions relevant to the TP USB keyboard.
>
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
Reviewed-by: Antonio Ospite <ao2@ao2.it>
Patch looks good to me, easier to validate now :)
A few ideas below for some possible _future_ cleanups, but no reason to
hold this one any longer IMHO.
Ciao,
Antonio
> ---
> ...er-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} | 0
> drivers/hid/Kconfig | 14 +-
> drivers/hid/Makefile | 2 +-
> drivers/hid/hid-core.c | 2 +-
> drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} | 185 +++++++++++----------
> 5 files changed, 102 insertions(+), 101 deletions(-)
> rename Documentation/ABI/testing/{sysfs-driver-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} (100%)
> rename drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} (64%)
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lenovo-tpkbd b/Documentation/ABI/testing/sysfs-driver-hid-lenovo
> similarity index 100%
> rename from Documentation/ABI/testing/sysfs-driver-hid-lenovo-tpkbd
> rename to Documentation/ABI/testing/sysfs-driver-hid-lenovo
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 800c8b6..9b7acfc 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -331,18 +331,18 @@ config HID_LCPOWER
> ---help---
> Support for LC-Power RC1000MCE RF remote control.
>
> -config HID_LENOVO_TPKBD
> - tristate "Lenovo ThinkPad USB Keyboard with TrackPoint"
> +config HID_LENOVO
> + tristate "Lenovo / Thinkpad devices"
> depends on HID
> select NEW_LEDS
> select LEDS_CLASS
> ---help---
> - Support for the Lenovo ThinkPad USB Keyboard with TrackPoint.
> + Support for Lenovo devices that are not fully compliant with HID standard.
>
> - Say Y here if you have a Lenovo ThinkPad USB Keyboard with TrackPoint
> - and would like to use device-specific features like changing the
> - sensitivity of the trackpoint, using the microphone mute button or
> - controlling the mute and microphone mute LEDs.
> + Say Y if you want support for the non-compliant features of the Lenovo
> + Thinkpad standalone keyboards, e.g:
This could become:
Say Y if you want support for the non-HID-compliant features of Lenovo
devices, e.g:
...
because, in principle, the driver is not only about keyboards anymore.
>
> + - ThinkPad USB Keyboard with TrackPoint (supports extra LEDs and trackpoint
> + configuration)
>
> config HID_LOGITECH
> tristate "Logitech devices" if EXPERT
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index a6fa6ba..5e96be3 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -59,7 +59,7 @@ obj-$(CONFIG_HID_KENSINGTON) += hid-kensington.o
> obj-$(CONFIG_HID_KEYTOUCH) += hid-keytouch.o
> obj-$(CONFIG_HID_KYE) += hid-kye.o
> obj-$(CONFIG_HID_LCPOWER) += hid-lcpower.o
> -obj-$(CONFIG_HID_LENOVO_TPKBD) += hid-lenovo-tpkbd.o
> +obj-$(CONFIG_HID_LENOVO) += hid-lenovo.o
> obj-$(CONFIG_HID_LOGITECH) += hid-logitech.o
> obj-$(CONFIG_HID_LOGITECH_DJ) += hid-logitech-dj.o
> obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 8ed66fd..55841bd 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1796,7 +1796,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_EASYPEN_M610X) },
> { HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
> { HID_USB_DEVICE(USB_VENDOR_ID_LCPOWER, USB_DEVICE_ID_LCPOWER_LC1000 ) },
> -#if IS_ENABLED(CONFIG_HID_LENOVO_TPKBD)
> +#if IS_ENABLED(CONFIG_HID_LENOVO)
> { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
> #endif
> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
> diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo.c
> similarity index 64%
> rename from drivers/hid/hid-lenovo-tpkbd.c
> rename to drivers/hid/hid-lenovo.c
> index 2d25b6c..0320b96 100644
> --- a/drivers/hid/hid-lenovo-tpkbd.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -20,8 +20,7 @@
>
> #include "hid-ids.h"
>
> -/* This is only used for the trackpoint part of the driver, hence _tp */
> -struct tpkbd_data_pointer {
> +struct lenovo_drvdata_tpkbd {
> int led_state;
> struct led_classdev led_mute;
> struct led_classdev led_micmute;
> @@ -35,7 +34,7 @@ struct tpkbd_data_pointer {
>
> #define map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
>
> -static int tpkbd_input_mapping(struct hid_device *hdev,
> +static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
> struct hid_input *hi, struct hid_field *field,
> struct hid_usage *usage, unsigned long **bit, int *max)
> {
> @@ -50,10 +49,10 @@ static int tpkbd_input_mapping(struct hid_device *hdev,
>
> #undef map_key_clear
>
> -static int tpkbd_features_set(struct hid_device *hdev)
> +static int lenovo_features_set_tpkbd(struct hid_device *hdev)
> {
> struct hid_report *report;
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
I think the word "pointer" here is misleading, sometimes in other parts
of the driver it is used as a synonym of trackpoint, but
lenovo_drvdata_tpkbd also contains data about leds, so here it looks
like it refers to C pointers. I'd call the variable tpkbd_data or
tpkbd_drvdata, throughout the driver. What do you think?
And the driver could standardize on the word "trackpoint" where
appropriate in order to eliminate any ambiguity.
I know, these are not your faults :)
>
> report = hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[4];
>
> @@ -69,23 +68,23 @@ static int tpkbd_features_set(struct hid_device *hdev)
> return 0;
> }
>
> -static ssize_t pointer_press_to_select_show(struct device *dev,
> +static ssize_t attr_press_to_select_show_tpkbd(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>
> return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->press_to_select);
> }
>
> -static ssize_t pointer_press_to_select_store(struct device *dev,
> +static ssize_t attr_press_to_select_store_tpkbd(struct device *dev,
> struct device_attribute *attr,
> const char *buf,
> size_t count)
> {
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
> int value;
>
> if (kstrtoint(buf, 10, &value))
> @@ -94,28 +93,28 @@ static ssize_t pointer_press_to_select_store(struct device *dev,
> return -EINVAL;
>
> data_pointer->press_to_select = value;
> - tpkbd_features_set(hdev);
> + lenovo_features_set_tpkbd(hdev);
>
> return count;
> }
>
> -static ssize_t pointer_dragging_show(struct device *dev,
> +static ssize_t attr_dragging_show_tpkbd(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>
> return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->dragging);
> }
>
> -static ssize_t pointer_dragging_store(struct device *dev,
> +static ssize_t attr_dragging_store_tpkbd(struct device *dev,
> struct device_attribute *attr,
> const char *buf,
> size_t count)
> {
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
> int value;
>
> if (kstrtoint(buf, 10, &value))
> @@ -124,28 +123,28 @@ static ssize_t pointer_dragging_store(struct device *dev,
> return -EINVAL;
>
> data_pointer->dragging = value;
> - tpkbd_features_set(hdev);
> + lenovo_features_set_tpkbd(hdev);
>
> return count;
> }
>
> -static ssize_t pointer_release_to_select_show(struct device *dev,
> +static ssize_t attr_release_to_select_show_tpkbd(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>
> return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->release_to_select);
> }
>
> -static ssize_t pointer_release_to_select_store(struct device *dev,
> +static ssize_t attr_release_to_select_store_tpkbd(struct device *dev,
> struct device_attribute *attr,
> const char *buf,
> size_t count)
> {
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
> int value;
>
> if (kstrtoint(buf, 10, &value))
> @@ -154,28 +153,28 @@ static ssize_t pointer_release_to_select_store(struct device *dev,
> return -EINVAL;
>
> data_pointer->release_to_select = value;
> - tpkbd_features_set(hdev);
> + lenovo_features_set_tpkbd(hdev);
>
> return count;
> }
>
> -static ssize_t pointer_select_right_show(struct device *dev,
> +static ssize_t attr_select_right_show_tpkbd(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>
> return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->select_right);
> }
>
> -static ssize_t pointer_select_right_store(struct device *dev,
> +static ssize_t attr_select_right_store_tpkbd(struct device *dev,
> struct device_attribute *attr,
> const char *buf,
> size_t count)
> {
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
> int value;
>
> if (kstrtoint(buf, 10, &value))
> @@ -184,119 +183,119 @@ static ssize_t pointer_select_right_store(struct device *dev,
> return -EINVAL;
>
> data_pointer->select_right = value;
> - tpkbd_features_set(hdev);
> + lenovo_features_set_tpkbd(hdev);
>
> return count;
> }
>
> -static ssize_t pointer_sensitivity_show(struct device *dev,
> +static ssize_t attr_sensitivity_show_tpkbd(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>
> return snprintf(buf, PAGE_SIZE, "%u\n",
> data_pointer->sensitivity);
> }
>
> -static ssize_t pointer_sensitivity_store(struct device *dev,
> +static ssize_t attr_sensitivity_store_tpkbd(struct device *dev,
> struct device_attribute *attr,
> const char *buf,
> size_t count)
> {
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
> int value;
>
> if (kstrtoint(buf, 10, &value) || value < 1 || value > 255)
> return -EINVAL;
>
> data_pointer->sensitivity = value;
> - tpkbd_features_set(hdev);
> + lenovo_features_set_tpkbd(hdev);
>
> return count;
> }
>
> -static ssize_t pointer_press_speed_show(struct device *dev,
> +static ssize_t attr_press_speed_show_tpkbd(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>
> return snprintf(buf, PAGE_SIZE, "%u\n",
> data_pointer->press_speed);
> }
>
> -static ssize_t pointer_press_speed_store(struct device *dev,
> +static ssize_t attr_press_speed_store_tpkbd(struct device *dev,
> struct device_attribute *attr,
> const char *buf,
> size_t count)
> {
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
> int value;
>
> if (kstrtoint(buf, 10, &value) || value < 1 || value > 255)
> return -EINVAL;
>
> data_pointer->press_speed = value;
> - tpkbd_features_set(hdev);
> + lenovo_features_set_tpkbd(hdev);
>
> return count;
> }
>
> -static struct device_attribute dev_attr_pointer_press_to_select =
> +static struct device_attribute dev_attr_press_to_select_tpkbd =
> __ATTR(press_to_select, S_IWUSR | S_IRUGO,
> - pointer_press_to_select_show,
> - pointer_press_to_select_store);
> + attr_press_to_select_show_tpkbd,
> + attr_press_to_select_store_tpkbd);
>
> -static struct device_attribute dev_attr_pointer_dragging =
> +static struct device_attribute dev_attr_dragging_tpkbd =
> __ATTR(dragging, S_IWUSR | S_IRUGO,
> - pointer_dragging_show,
> - pointer_dragging_store);
> + attr_dragging_show_tpkbd,
> + attr_dragging_store_tpkbd);
>
> -static struct device_attribute dev_attr_pointer_release_to_select =
> +static struct device_attribute dev_attr_release_to_select_tpkbd =
> __ATTR(release_to_select, S_IWUSR | S_IRUGO,
> - pointer_release_to_select_show,
> - pointer_release_to_select_store);
> + attr_release_to_select_show_tpkbd,
> + attr_release_to_select_store_tpkbd);
>
> -static struct device_attribute dev_attr_pointer_select_right =
> +static struct device_attribute dev_attr_select_right_tpkbd =
> __ATTR(select_right, S_IWUSR | S_IRUGO,
> - pointer_select_right_show,
> - pointer_select_right_store);
> + attr_select_right_show_tpkbd,
> + attr_select_right_store_tpkbd);
>
> -static struct device_attribute dev_attr_pointer_sensitivity =
> +static struct device_attribute dev_attr_sensitivity_tpkbd =
> __ATTR(sensitivity, S_IWUSR | S_IRUGO,
> - pointer_sensitivity_show,
> - pointer_sensitivity_store);
> + attr_sensitivity_show_tpkbd,
> + attr_sensitivity_store_tpkbd);
>
> -static struct device_attribute dev_attr_pointer_press_speed =
> +static struct device_attribute dev_attr_press_speed_tpkbd =
> __ATTR(press_speed, S_IWUSR | S_IRUGO,
> - pointer_press_speed_show,
> - pointer_press_speed_store);
> -
> -static struct attribute *tpkbd_attributes_pointer[] = {
> - &dev_attr_pointer_press_to_select.attr,
> - &dev_attr_pointer_dragging.attr,
> - &dev_attr_pointer_release_to_select.attr,
> - &dev_attr_pointer_select_right.attr,
> - &dev_attr_pointer_sensitivity.attr,
> - &dev_attr_pointer_press_speed.attr,
> + attr_press_speed_show_tpkbd,
> + attr_press_speed_store_tpkbd);
> +
> +static struct attribute *lenovo_attributes_tpkbd[] = {
> + &dev_attr_press_to_select_tpkbd.attr,
> + &dev_attr_dragging_tpkbd.attr,
> + &dev_attr_release_to_select_tpkbd.attr,
> + &dev_attr_select_right_tpkbd.attr,
> + &dev_attr_sensitivity_tpkbd.attr,
> + &dev_attr_press_speed_tpkbd.attr,
> NULL
> };
>
> -static const struct attribute_group tpkbd_attr_group_pointer = {
> - .attrs = tpkbd_attributes_pointer,
> +static const struct attribute_group lenovo_attr_group_tpkbd = {
> + .attrs = lenovo_attributes_tpkbd,
> };
>
> -static enum led_brightness tpkbd_led_brightness_get(
> +static enum led_brightness lenovo_led_brightness_get_tpkbd(
> struct led_classdev *led_cdev)
> {
> struct device *dev = led_cdev->dev->parent;
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
> int led_nr = 0;
>
> if (led_cdev == &data_pointer->led_micmute)
> @@ -307,12 +306,12 @@ static enum led_brightness tpkbd_led_brightness_get(
> : LED_OFF;
> }
>
> -static void tpkbd_led_brightness_set(struct led_classdev *led_cdev,
> +static void lenovo_led_brightness_set_tpkbd(struct led_classdev *led_cdev,
> enum led_brightness value)
> {
> struct device *dev = led_cdev->dev->parent;
> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
> struct hid_report *report;
> int led_nr = 0;
>
> @@ -330,10 +329,10 @@ static void tpkbd_led_brightness_set(struct led_classdev *led_cdev,
> hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> }
>
> -static int tpkbd_probe_tp(struct hid_device *hdev)
> +static int lenovo_probe_tpkbd(struct hid_device *hdev)
> {
> struct device *dev = &hdev->dev;
> - struct tpkbd_data_pointer *data_pointer;
> + struct lenovo_drvdata_tpkbd *data_pointer;
> size_t name_sz = strlen(dev_name(dev)) + 16;
> char *name_mute, *name_micmute;
> int i;
> @@ -347,12 +346,12 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
> return -ENODEV;
>
> if (sysfs_create_group(&hdev->dev.kobj,
> - &tpkbd_attr_group_pointer)) {
> + &lenovo_attr_group_tpkbd)) {
> hid_warn(hdev, "Could not create sysfs group\n");
> }
>
> data_pointer = devm_kzalloc(&hdev->dev,
> - sizeof(struct tpkbd_data_pointer),
> + sizeof(struct lenovo_drvdata_tpkbd),
> GFP_KERNEL);
> if (data_pointer == NULL) {
> hid_err(hdev, "Could not allocate memory for driver data\n");
> @@ -375,23 +374,25 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
> hid_set_drvdata(hdev, data_pointer);
>
> data_pointer->led_mute.name = name_mute;
> - data_pointer->led_mute.brightness_get = tpkbd_led_brightness_get;
> - data_pointer->led_mute.brightness_set = tpkbd_led_brightness_set;
> + data_pointer->led_mute.brightness_get = lenovo_led_brightness_get_tpkbd;
> + data_pointer->led_mute.brightness_set = lenovo_led_brightness_set_tpkbd;
> data_pointer->led_mute.dev = dev;
> led_classdev_register(dev, &data_pointer->led_mute);
>
> data_pointer->led_micmute.name = name_micmute;
> - data_pointer->led_micmute.brightness_get = tpkbd_led_brightness_get;
> - data_pointer->led_micmute.brightness_set = tpkbd_led_brightness_set;
> + data_pointer->led_micmute.brightness_get =
> + lenovo_led_brightness_get_tpkbd;
> + data_pointer->led_micmute.brightness_set =
> + lenovo_led_brightness_set_tpkbd;
> data_pointer->led_micmute.dev = dev;
> led_classdev_register(dev, &data_pointer->led_micmute);
>
> - tpkbd_features_set(hdev);
> + lenovo_features_set_tpkbd(hdev);
>
> return 0;
> }
>
> -static int tpkbd_probe(struct hid_device *hdev,
> +static int lenovo_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
> {
> int ret;
> @@ -410,7 +411,7 @@ static int tpkbd_probe(struct hid_device *hdev,
>
> if (hid_get_drvdata(hdev)) {
> hid_set_drvdata(hdev, NULL);
> - ret = tpkbd_probe_tp(hdev);
> + ret = lenovo_probe_tpkbd(hdev);
> if (ret)
> goto err_hid;
> }
> @@ -422,12 +423,12 @@ err:
> return ret;
> }
>
> -static void tpkbd_remove_tp(struct hid_device *hdev)
> +static void lenovo_remove_tpkbd(struct hid_device *hdev)
> {
> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>
> sysfs_remove_group(&hdev->dev.kobj,
> - &tpkbd_attr_group_pointer);
> + &lenovo_attr_group_tpkbd);
>
> led_classdev_unregister(&data_pointer->led_micmute);
> led_classdev_unregister(&data_pointer->led_mute);
> @@ -435,28 +436,28 @@ static void tpkbd_remove_tp(struct hid_device *hdev)
> hid_set_drvdata(hdev, NULL);
> }
>
> -static void tpkbd_remove(struct hid_device *hdev)
> +static void lenovo_remove(struct hid_device *hdev)
> {
> if (hid_get_drvdata(hdev))
> - tpkbd_remove_tp(hdev);
> + lenovo_remove_tpkbd(hdev);
>
> hid_hw_stop(hdev);
> }
>
> -static const struct hid_device_id tpkbd_devices[] = {
> +static const struct hid_device_id lenovo_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
> { }
> };
>
> -MODULE_DEVICE_TABLE(hid, tpkbd_devices);
> +MODULE_DEVICE_TABLE(hid, lenovo_devices);
>
> -static struct hid_driver tpkbd_driver = {
> - .name = "lenovo_tpkbd",
> - .id_table = tpkbd_devices,
> - .input_mapping = tpkbd_input_mapping,
> - .probe = tpkbd_probe,
> - .remove = tpkbd_remove,
> +static struct hid_driver lenovo_driver = {
> + .name = "lenovo",
> + .id_table = lenovo_devices,
> + .input_mapping = lenovo_input_mapping_tpkbd,
> + .probe = lenovo_probe,
> + .remove = lenovo_remove,
> };
> -module_hid_driver(tpkbd_driver);
> +module_hid_driver(lenovo_driver);
>
> MODULE_LICENSE("GPL");
> --
> 2.0.0
>
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Chen Gang @ 2014-07-14 9:22 UTC (permalink / raw)
To: Richard Weinberger
Cc: Lars-Peter Clausen, Greg Kroah-Hartman,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Benjamin Herrenschmidt,
teg-B22kvLQNl6c, Thierry Reding, Lennox Wu, Chen Gang,
Marek Vasut, Liqin Chen, msalter-H+wXaHxf7aLQT0dZR+AlfA,
linux-pwm-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
knaack.h-Mmb7MZpHnFY, Martin Schwidefsky,
Mischa.Jonker-HKixBCOQz3hWk0Htik3J/w,
jic23-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <53C39B66.4060500-/L3Ra7n9ekc@public.gmane.org>
在 2014年7月14日，下午4:57，Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org> 写道：
> Am 14.07.2014 10:48, schrieb Lars-Peter Clausen:
>> On 07/14/2014 10:31 AM, Richard Weinberger wrote:
>>> Am 13.07.2014 22:17, schrieb Greg Kroah-Hartman:
>>>> On Sun, Jul 13, 2014 at 09:33:38PM +0200, Richard Weinberger wrote:
>>>>> Maybe we could add COMPILE_TEST to the version string too?
>>>>> Just to detect such kernels fast in user bug reports...
>>>>
>>>> What kind of bug report are you going to get?
>>>
>>> User manages to enable CONFIG_FOO by selecting COMPILE_TEST and
>>> complains that it does not work. :)
>>
>> These drivers are typically drivers for some SoC peripheral and the device will simply physically not exist on a platform that does not provide HAS_IOMEM. This is not really any
>> different from making the driver selectable via COMPILE_TEST for any other platform. To hit the issue you'd have to instantiate a device driver instance for a device that
>> physically does not exist. This will always result in a failure.
>
> Okay, you have convinced me. :)
>
OK, thank all of you, and I shall send the related patch for it.
I will try to finish it within this week.
Thanks.
—
Chen Gang
Open, share, and attitude like air, water, and life which God blessed.--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Richard Weinberger @ 2014-07-14 8:57 UTC (permalink / raw)
To: Lars-Peter Clausen, Greg Kroah-Hartman
Cc: Marek Vasut, Liqin Chen, devel, Chen Gang, linux-pwm, linux-iio,
Benjamin Herrenschmidt, dmitry.torokhov,
linux-kernel@vger.kernel.org, teg, Martin Schwidefsky,
thierry.reding, jic23, msalter, knaack.h, linux-input,
Mischa.Jonker, Lennox Wu, linux-watchdog
In-Reply-To: <53C3994C.1010309@metafoo.de>
Am 14.07.2014 10:48, schrieb Lars-Peter Clausen:
> On 07/14/2014 10:31 AM, Richard Weinberger wrote:
>> Am 13.07.2014 22:17, schrieb Greg Kroah-Hartman:
>>> On Sun, Jul 13, 2014 at 09:33:38PM +0200, Richard Weinberger wrote:
>>>> Maybe we could add COMPILE_TEST to the version string too?
>>>> Just to detect such kernels fast in user bug reports...
>>>
>>> What kind of bug report are you going to get?
>>
>> User manages to enable CONFIG_FOO by selecting COMPILE_TEST and
>> complains that it does not work. :)
>
> These drivers are typically drivers for some SoC peripheral and the device will simply physically not exist on a platform that does not provide HAS_IOMEM. This is not really any
> different from making the driver selectable via COMPILE_TEST for any other platform. To hit the issue you'd have to instantiate a device driver instance for a device that
> physically does not exist. This will always result in a failure.
Okay, you have convinced me. :)
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Lars-Peter Clausen @ 2014-07-14 8:48 UTC (permalink / raw)
To: Richard Weinberger, Greg Kroah-Hartman
Cc: Marek Vasut, Liqin Chen, devel, Chen Gang, linux-pwm, linux-iio,
Benjamin Herrenschmidt, dmitry.torokhov,
linux-kernel@vger.kernel.org, teg, Martin Schwidefsky,
thierry.reding, jic23, msalter, knaack.h, linux-input,
Mischa.Jonker, Lennox Wu, linux-watchdog
In-Reply-To: <53C39569.9020802@nod.at>
On 07/14/2014 10:31 AM, Richard Weinberger wrote:
> Am 13.07.2014 22:17, schrieb Greg Kroah-Hartman:
>> On Sun, Jul 13, 2014 at 09:33:38PM +0200, Richard Weinberger wrote:
>>> Maybe we could add COMPILE_TEST to the version string too?
>>> Just to detect such kernels fast in user bug reports...
>>
>> What kind of bug report are you going to get?
>
> User manages to enable CONFIG_FOO by selecting COMPILE_TEST and
> complains that it does not work. :)
These drivers are typically drivers for some SoC peripheral and the device
will simply physically not exist on a platform that does not provide
HAS_IOMEM. This is not really any different from making the driver
selectable via COMPILE_TEST for any other platform. To hit the issue you'd
have to instantiate a device driver instance for a device that physically
does not exist. This will always result in a failure.
- Lars
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Richard Weinberger @ 2014-07-14 8:31 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Lars-Peter Clausen, dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Benjamin Herrenschmidt,
teg-B22kvLQNl6c, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, Lennox Wu,
Chen Gang, Marek Vasut, Liqin Chen,
msalter-H+wXaHxf7aLQT0dZR+AlfA, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
knaack.h-Mmb7MZpHnFY, Martin Schwidefsky,
Mischa.Jonker-HKixBCOQz3hWk0Htik3J/w,
jic23-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20140713201753.GA29955-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
Am 13.07.2014 22:17, schrieb Greg Kroah-Hartman:
> On Sun, Jul 13, 2014 at 09:33:38PM +0200, Richard Weinberger wrote:
>> Maybe we could add COMPILE_TEST to the version string too?
>> Just to detect such kernels fast in user bug reports...
>
> What kind of bug report are you going to get?
User manages to enable CONFIG_FOO by selecting COMPILE_TEST and
complains that it does not work. :)
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Thierry Reding @ 2014-07-14 8:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Marek Vasut, Liqin Chen, Lars-Peter Clausen, Chen Gang, linux-pwm,
linux-iio, Richard Weinberger, dmitry.torokhov,
linux-kernel@vger.kernel.org, jic23, teg, Martin Schwidefsky,
msalter, Benjamin Herrenschmidt, knaack.h, devel, linux-input,
Mischa.Jonker, Lennox Wu, linux-watchdog
In-Reply-To: <20140713192202.GA19090@kroah.com>
[-- Attachment #1.1: Type: text/plain, Size: 3061 bytes --]
On Sun, Jul 13, 2014 at 12:22:02PM -0700, Greg Kroah-Hartman wrote:
> On Sun, Jul 13, 2014 at 04:25:06PM +0200, Lars-Peter Clausen wrote:
> > On 07/13/2014 04:03 PM, Richard Weinberger wrote:
> > >Am 13.07.2014 15:56, schrieb Lars-Peter Clausen:
> > >>On 07/13/2014 03:40 PM, Richard Weinberger wrote:
> > >>>Am 13.07.2014 15:26, schrieb Lars-Peter Clausen:
> > >>>>On 07/13/2014 11:45 AM, Richard Weinberger wrote:
> > >>>>>Am 13.07.2014 11:27, schrieb Lennox Wu:
> > >>>>>>As I said before, some configurations don't make sense.
> > >>>>>
> > >>>>>If such a configuration can be achieved using allmod/yesconfig it has to be fixed.
> > >>>>>Chen's fixes seem reasonable as not all architectures support iomem.
> > >>>>
> > >>>>Maybe we should stub out ioremap() and friends when COMPILE_TEST is enabled to avoid these linker errors. That's in my opinion better than turning most of the 'depends on
> > >>>>COMPILE_TEST' into 'depends on COMPILE_TEST && HAS_IOMEM'. The issue comes up quite a lot and it is often overlooked when adding a driver that can be build when COMPILE_TEST is
> > >>>>enabled.
> > >>>
> > >>>And what should this stub do?
> > >>>Except calling BUG()...
> > >>
> > >>return NULL;
> > >>
> > >>It's for compile testing, it's not meant to work at runtime.
> > >
> > >Hm, I really don't like the idea of having a non-working kernel.
> > >IMHO either it should build _and_ run and nothing else.
> > >Greg, what do you think?
> >
> > The kernel will still be working fine and you can run it on a system. The
> > drivers which use ioremap() or similar are probably not instantiated on a
> > system that does not provide HAS_IOMEM. But even if it was the driver should
> > handle ioremap() returning NULL gracefully and abort the driver probe. That
> > said you'll probably not run a kernel that was built with COMPILE_TEST on
> > your real hardware since it contains so many drivers that are completely
> > useless on your hardware. The idea of COMPILE_TEST is to have as much
> > compile test exposure as possible to the code that is enabled by
> > COMPILE_TEST. Stubbing out ioremap() and friends when HAS_IOMEM is not set
> > and COMPILE_TEST is set makes it easier to get there.
>
> I run my kernels with COMPILE_TEST enabled as I need to build test
> things that I don't happen to use.
>
> I like the 'return NULL' option for this, it hits us all the time, might
> as well fix it properly like this so that we don't have to deal with
> Kconfig changes everywhere.
I agree. One nit, though: devm_ioremap_resource() returns an ERR_PTR()-
encoded error code, so the dummy should probably be returning something
like ERR_PTR(-ENOSYS) instead of NULL.
> Also put a big "This platform does not support IOMEM" error printk in
> there, so that people have a chance to figure out what is going on if
> they happen to run such a driver on a platform that can't support it.
Yes, that sounds like a very good idea and should be indication enough
of what exactly has gone wrong.
Thierry
[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 169 bytes --]
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Greg Kroah-Hartman @ 2014-07-13 20:17 UTC (permalink / raw)
To: Richard Weinberger
Cc: Marek Vasut, Liqin Chen, Lars-Peter Clausen, Chen Gang, linux-pwm,
linux-iio, Benjamin Herrenschmidt, dmitry.torokhov,
linux-kernel@vger.kernel.org, teg, Martin Schwidefsky,
thierry.reding, jic23, msalter, knaack.h, devel, linux-input,
Mischa.Jonker, Lennox Wu, linux-watchdog
In-Reply-To: <53C2DF12.6040507@nod.at>
On Sun, Jul 13, 2014 at 09:33:38PM +0200, Richard Weinberger wrote:
> Am 13.07.2014 21:22, schrieb Greg Kroah-Hartman:
> > On Sun, Jul 13, 2014 at 04:25:06PM +0200, Lars-Peter Clausen wrote:
> >> On 07/13/2014 04:03 PM, Richard Weinberger wrote:
> >>> Am 13.07.2014 15:56, schrieb Lars-Peter Clausen:
> >>>> On 07/13/2014 03:40 PM, Richard Weinberger wrote:
> >>>>> Am 13.07.2014 15:26, schrieb Lars-Peter Clausen:
> >>>>>> On 07/13/2014 11:45 AM, Richard Weinberger wrote:
> >>>>>>> Am 13.07.2014 11:27, schrieb Lennox Wu:
> >>>>>>>> As I said before, some configurations don't make sense.
> >>>>>>>
> >>>>>>> If such a configuration can be achieved using allmod/yesconfig it has to be fixed.
> >>>>>>> Chen's fixes seem reasonable as not all architectures support iomem.
> >>>>>>
> >>>>>> Maybe we should stub out ioremap() and friends when COMPILE_TEST is enabled to avoid these linker errors. That's in my opinion better than turning most of the 'depends on
> >>>>>> COMPILE_TEST' into 'depends on COMPILE_TEST && HAS_IOMEM'. The issue comes up quite a lot and it is often overlooked when adding a driver that can be build when COMPILE_TEST is
> >>>>>> enabled.
> >>>>>
> >>>>> And what should this stub do?
> >>>>> Except calling BUG()...
> >>>>
> >>>> return NULL;
> >>>>
> >>>> It's for compile testing, it's not meant to work at runtime.
> >>>
> >>> Hm, I really don't like the idea of having a non-working kernel.
> >>> IMHO either it should build _and_ run and nothing else.
> >>> Greg, what do you think?
> >>
> >> The kernel will still be working fine and you can run it on a system. The
> >> drivers which use ioremap() or similar are probably not instantiated on a
> >> system that does not provide HAS_IOMEM. But even if it was the driver should
> >> handle ioremap() returning NULL gracefully and abort the driver probe. That
> >> said you'll probably not run a kernel that was built with COMPILE_TEST on
> >> your real hardware since it contains so many drivers that are completely
> >> useless on your hardware. The idea of COMPILE_TEST is to have as much
> >> compile test exposure as possible to the code that is enabled by
> >> COMPILE_TEST. Stubbing out ioremap() and friends when HAS_IOMEM is not set
> >> and COMPILE_TEST is set makes it easier to get there.
> >
> > I run my kernels with COMPILE_TEST enabled as I need to build test
> > things that I don't happen to use.
> >
> > I like the 'return NULL' option for this, it hits us all the time, might
> > as well fix it properly like this so that we don't have to deal with
> > Kconfig changes everywhere.
> >
> > Also put a big "This platform does not support IOMEM" error printk in
> > there, so that people have a chance to figure out what is going on if
> > they happen to run such a driver on a platform that can't support it.
>
> Maybe we could add COMPILE_TEST to the version string too?
> Just to detect such kernels fast in user bug reports...
What kind of bug report are you going to get?
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Richard Weinberger @ 2014-07-13 19:33 UTC (permalink / raw)
To: Greg Kroah-Hartman, Lars-Peter Clausen
Cc: dmitry.torokhov, linux-iio, Benjamin Herrenschmidt, teg,
thierry.reding, Lennox Wu, Chen Gang, Marek Vasut, Liqin Chen,
msalter, linux-pwm, devel, linux-watchdog, linux-input,
linux-kernel@vger.kernel.org, knaack.h, Martin Schwidefsky,
Mischa.Jonker, jic23
In-Reply-To: <20140713192202.GA19090@kroah.com>
Am 13.07.2014 21:22, schrieb Greg Kroah-Hartman:
> On Sun, Jul 13, 2014 at 04:25:06PM +0200, Lars-Peter Clausen wrote:
>> On 07/13/2014 04:03 PM, Richard Weinberger wrote:
>>> Am 13.07.2014 15:56, schrieb Lars-Peter Clausen:
>>>> On 07/13/2014 03:40 PM, Richard Weinberger wrote:
>>>>> Am 13.07.2014 15:26, schrieb Lars-Peter Clausen:
>>>>>> On 07/13/2014 11:45 AM, Richard Weinberger wrote:
>>>>>>> Am 13.07.2014 11:27, schrieb Lennox Wu:
>>>>>>>> As I said before, some configurations don't make sense.
>>>>>>>
>>>>>>> If such a configuration can be achieved using allmod/yesconfig it has to be fixed.
>>>>>>> Chen's fixes seem reasonable as not all architectures support iomem.
>>>>>>
>>>>>> Maybe we should stub out ioremap() and friends when COMPILE_TEST is enabled to avoid these linker errors. That's in my opinion better than turning most of the 'depends on
>>>>>> COMPILE_TEST' into 'depends on COMPILE_TEST && HAS_IOMEM'. The issue comes up quite a lot and it is often overlooked when adding a driver that can be build when COMPILE_TEST is
>>>>>> enabled.
>>>>>
>>>>> And what should this stub do?
>>>>> Except calling BUG()...
>>>>
>>>> return NULL;
>>>>
>>>> It's for compile testing, it's not meant to work at runtime.
>>>
>>> Hm, I really don't like the idea of having a non-working kernel.
>>> IMHO either it should build _and_ run and nothing else.
>>> Greg, what do you think?
>>
>> The kernel will still be working fine and you can run it on a system. The
>> drivers which use ioremap() or similar are probably not instantiated on a
>> system that does not provide HAS_IOMEM. But even if it was the driver should
>> handle ioremap() returning NULL gracefully and abort the driver probe. That
>> said you'll probably not run a kernel that was built with COMPILE_TEST on
>> your real hardware since it contains so many drivers that are completely
>> useless on your hardware. The idea of COMPILE_TEST is to have as much
>> compile test exposure as possible to the code that is enabled by
>> COMPILE_TEST. Stubbing out ioremap() and friends when HAS_IOMEM is not set
>> and COMPILE_TEST is set makes it easier to get there.
>
> I run my kernels with COMPILE_TEST enabled as I need to build test
> things that I don't happen to use.
>
> I like the 'return NULL' option for this, it hits us all the time, might
> as well fix it properly like this so that we don't have to deal with
> Kconfig changes everywhere.
>
> Also put a big "This platform does not support IOMEM" error printk in
> there, so that people have a chance to figure out what is going on if
> they happen to run such a driver on a platform that can't support it.
Maybe we could add COMPILE_TEST to the version string too?
Just to detect such kernels fast in user bug reports...
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Greg Kroah-Hartman @ 2014-07-13 19:22 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Marek Vasut, Liqin Chen, devel, Chen Gang, linux-pwm, linux-iio,
Richard Weinberger, dmitry.torokhov, linux-kernel@vger.kernel.org,
jic23, teg, Martin Schwidefsky, thierry.reding, msalter,
Benjamin Herrenschmidt, knaack.h, linux-input, Mischa.Jonker,
Lennox Wu, linux-watchdog
In-Reply-To: <53C296C2.30304@metafoo.de>
On Sun, Jul 13, 2014 at 04:25:06PM +0200, Lars-Peter Clausen wrote:
> On 07/13/2014 04:03 PM, Richard Weinberger wrote:
> >Am 13.07.2014 15:56, schrieb Lars-Peter Clausen:
> >>On 07/13/2014 03:40 PM, Richard Weinberger wrote:
> >>>Am 13.07.2014 15:26, schrieb Lars-Peter Clausen:
> >>>>On 07/13/2014 11:45 AM, Richard Weinberger wrote:
> >>>>>Am 13.07.2014 11:27, schrieb Lennox Wu:
> >>>>>>As I said before, some configurations don't make sense.
> >>>>>
> >>>>>If such a configuration can be achieved using allmod/yesconfig it has to be fixed.
> >>>>>Chen's fixes seem reasonable as not all architectures support iomem.
> >>>>
> >>>>Maybe we should stub out ioremap() and friends when COMPILE_TEST is enabled to avoid these linker errors. That's in my opinion better than turning most of the 'depends on
> >>>>COMPILE_TEST' into 'depends on COMPILE_TEST && HAS_IOMEM'. The issue comes up quite a lot and it is often overlooked when adding a driver that can be build when COMPILE_TEST is
> >>>>enabled.
> >>>
> >>>And what should this stub do?
> >>>Except calling BUG()...
> >>
> >>return NULL;
> >>
> >>It's for compile testing, it's not meant to work at runtime.
> >
> >Hm, I really don't like the idea of having a non-working kernel.
> >IMHO either it should build _and_ run and nothing else.
> >Greg, what do you think?
>
> The kernel will still be working fine and you can run it on a system. The
> drivers which use ioremap() or similar are probably not instantiated on a
> system that does not provide HAS_IOMEM. But even if it was the driver should
> handle ioremap() returning NULL gracefully and abort the driver probe. That
> said you'll probably not run a kernel that was built with COMPILE_TEST on
> your real hardware since it contains so many drivers that are completely
> useless on your hardware. The idea of COMPILE_TEST is to have as much
> compile test exposure as possible to the code that is enabled by
> COMPILE_TEST. Stubbing out ioremap() and friends when HAS_IOMEM is not set
> and COMPILE_TEST is set makes it easier to get there.
I run my kernels with COMPILE_TEST enabled as I need to build test
things that I don't happen to use.
I like the 'return NULL' option for this, it hits us all the time, might
as well fix it properly like this so that we don't have to deal with
Kconfig changes everywhere.
Also put a big "This platform does not support IOMEM" error printk in
there, so that people have a chance to figure out what is going on if
they happen to run such a driver on a platform that can't support it.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] Input: ambakmi - Use managed interfaces
From: Julia Lawall @ 2014-07-13 18:29 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Julia Lawall, Himangi Saraogi, Dmitry Torokhov, linux-input,
linux-kernel
In-Reply-To: <20140713182123.GM21766@n2100.arm.linux.org.uk>
On Sun, 13 Jul 2014, Russell King - ARM Linux wrote:
> On Sun, Jul 13, 2014 at 08:11:29PM +0200, Julia Lawall wrote:
> > On Sun, 13 Jul 2014, Russell King - ARM Linux wrote:
> >
> > > On Sun, Jul 13, 2014 at 11:00:51PM +0530, Himangi Saraogi wrote:
> > > > @@ -23,6 +23,7 @@
> > > > #include <linux/clk.h>
> > > >
> > > > #include <asm/io.h>
> > > > +#include <linux/io.h>
> > >
> > > NAK - please include either linux/io.h _or_ asm/io.h but not both.
> > >
> > > > @@ -112,19 +113,10 @@ static int amba_kmi_probe(struct amba_device *dev,
> > > > {
> > > > struct amba_kmi_port *kmi;
> > > > struct serio *io;
> > > > - int ret;
> > > > -
> > > > - ret = amba_request_regions(dev, NULL);
> > > > - if (ret)
> > > > - return ret;
> > >
> > > I'm /really/ not happy about that going.
> >
> > Could you explain why? I looked at the code several times, and I couldn't
> > see how it was different than request_mem_region, which is merged into
> > devm_ioremap_resource.
>
> Check what gets used for the name of the resource.
>
> Now, consider that most devices when they are registered have their
> resource names set to the device name.
>
> Then realise that devm_ioremap_resource() uses the resource name or
> the device name again. So, what you end up with in /proc/iomem is
> a load of stupidity - you don't get to see there which drivers are
> making use of the resources, only a load of duplicated information
> about what devices are using the regions.
>
> This is contary to other bus types (like PCI) where the _device_ takes
> the non-busy parent resource, and the driver takes the busy resource
> using the _driver_ name, not the device name. The exception to this
> is network drivers which have in the past used the network device
> name.
>
> Here's an example. x86:
>
> fc000000-fc01ffff : 0000:00:19.0 <--- device name
> fc000000-fc01ffff : e1000e <--- driver name
> fc020000-fc023fff : 0000:00:1b.0 <--- device name
> fc020000-fc023fff : ICH HD audio <--- driver name
> fc024000-fc024fff : 0000:00:03.3
> fc025000-fc025fff : 0000:00:19.0 <--- device name
> fc025000-fc025fff : e1000e <--- driver name
> fc226000-fc2267ff : 0000:00:1f.2 <--- device name
> fc226000-fc2267ff : ahci <--- driver name
>
> etc. When using devm_ioremap_resource() this ends up as:
>
> 02184000-021841ff : /soc/aips-bus@02100000/usb@02184000
> 02184000-021841ff : /soc/aips-bus@02100000/usb@02184000
> 02184200-021843ff : /soc/aips-bus@02100000/usb@02184200
> 02184200-021843ff : /soc/aips-bus@02100000/usb@02184200
>
> which is really pointless duplicating the resource name like that. It
> conveys no additional useful information.
OK, thanks very much.
julia
^ permalink raw reply
* Re: [PATCH] Input: ambakmi - Use managed interfaces
From: Russell King - ARM Linux @ 2014-07-13 18:21 UTC (permalink / raw)
To: Julia Lawall; +Cc: Himangi Saraogi, Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <alpine.DEB.2.02.1407132010130.2060@localhost6.localdomain6>
On Sun, Jul 13, 2014 at 08:11:29PM +0200, Julia Lawall wrote:
> On Sun, 13 Jul 2014, Russell King - ARM Linux wrote:
>
> > On Sun, Jul 13, 2014 at 11:00:51PM +0530, Himangi Saraogi wrote:
> > > @@ -23,6 +23,7 @@
> > > #include <linux/clk.h>
> > >
> > > #include <asm/io.h>
> > > +#include <linux/io.h>
> >
> > NAK - please include either linux/io.h _or_ asm/io.h but not both.
> >
> > > @@ -112,19 +113,10 @@ static int amba_kmi_probe(struct amba_device *dev,
> > > {
> > > struct amba_kmi_port *kmi;
> > > struct serio *io;
> > > - int ret;
> > > -
> > > - ret = amba_request_regions(dev, NULL);
> > > - if (ret)
> > > - return ret;
> >
> > I'm /really/ not happy about that going.
>
> Could you explain why? I looked at the code several times, and I couldn't
> see how it was different than request_mem_region, which is merged into
> devm_ioremap_resource.
Check what gets used for the name of the resource.
Now, consider that most devices when they are registered have their
resource names set to the device name.
Then realise that devm_ioremap_resource() uses the resource name or
the device name again. So, what you end up with in /proc/iomem is
a load of stupidity - you don't get to see there which drivers are
making use of the resources, only a load of duplicated information
about what devices are using the regions.
This is contary to other bus types (like PCI) where the _device_ takes
the non-busy parent resource, and the driver takes the busy resource
using the _driver_ name, not the device name. The exception to this
is network drivers which have in the past used the network device
name.
Here's an example. x86:
fc000000-fc01ffff : 0000:00:19.0 <--- device name
fc000000-fc01ffff : e1000e <--- driver name
fc020000-fc023fff : 0000:00:1b.0 <--- device name
fc020000-fc023fff : ICH HD audio <--- driver name
fc024000-fc024fff : 0000:00:03.3
fc025000-fc025fff : 0000:00:19.0 <--- device name
fc025000-fc025fff : e1000e <--- driver name
fc226000-fc2267ff : 0000:00:1f.2 <--- device name
fc226000-fc2267ff : ahci <--- driver name
etc. When using devm_ioremap_resource() this ends up as:
02184000-021841ff : /soc/aips-bus@02100000/usb@02184000
02184000-021841ff : /soc/aips-bus@02100000/usb@02184000
02184200-021843ff : /soc/aips-bus@02100000/usb@02184200
02184200-021843ff : /soc/aips-bus@02100000/usb@02184200
which is really pointless duplicating the resource name like that. It
conveys no additional useful information.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH] Input: ambakmi - Use managed interfaces
From: Julia Lawall @ 2014-07-13 18:11 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Himangi Saraogi, Dmitry Torokhov, linux-input, linux-kernel,
julia.lawall
In-Reply-To: <20140713175308.GL21766@n2100.arm.linux.org.uk>
On Sun, 13 Jul 2014, Russell King - ARM Linux wrote:
> On Sun, Jul 13, 2014 at 11:00:51PM +0530, Himangi Saraogi wrote:
> > @@ -23,6 +23,7 @@
> > #include <linux/clk.h>
> >
> > #include <asm/io.h>
> > +#include <linux/io.h>
>
> NAK - please include either linux/io.h _or_ asm/io.h but not both.
>
> > @@ -112,19 +113,10 @@ static int amba_kmi_probe(struct amba_device *dev,
> > {
> > struct amba_kmi_port *kmi;
> > struct serio *io;
> > - int ret;
> > -
> > - ret = amba_request_regions(dev, NULL);
> > - if (ret)
> > - return ret;
>
> I'm /really/ not happy about that going.
Could you explain why? I looked at the code several times, and I couldn't
see how it was different than request_mem_region, which is merged into
devm_ioremap_resource.
thanks,
julia
>
> > -
> > - kmi = kzalloc(sizeof(struct amba_kmi_port), GFP_KERNEL);
> > - io = kzalloc(sizeof(struct serio), GFP_KERNEL);
> > - if (!kmi || !io) {
> > - ret = -ENOMEM;
> > - goto out;
> > - }
> >
> > + io = devm_kzalloc(&dev->dev, sizeof(struct serio), GFP_KERNEL);
> > + if (!io)
> > + return -ENOMEM;
> >
> > io->id.type = SERIO_8042;
> > io->write = amba_kmi_write;
> > @@ -135,32 +127,20 @@ static int amba_kmi_probe(struct amba_device *dev,
> > io->port_data = kmi;
> > io->dev.parent = &dev->dev;
> >
> > + kmi->base = devm_ioremap_resource(&dev->dev, &dev->res);
> > + if (IS_ERR(kmi->base))
> > + return PTR_ERR(kmi->base);
>
> NAK. Look carefully through the above changes to work out why. Please,
> if you don't understand what you're doing, or don't even bother with a
> build test of the code you've modified, don't even modify it in the
> first place.
>
> --
> FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
> according to speedtest.net.
>
^ permalink raw reply
* Re: [PATCH] Input: ambakmi - Use managed interfaces
From: Russell King - ARM Linux @ 2014-07-13 17:53 UTC (permalink / raw)
To: Himangi Saraogi; +Cc: Dmitry Torokhov, linux-input, linux-kernel, julia.lawall
In-Reply-To: <20140713173051.GA4299@himangi-Dell>
On Sun, Jul 13, 2014 at 11:00:51PM +0530, Himangi Saraogi wrote:
> @@ -23,6 +23,7 @@
> #include <linux/clk.h>
>
> #include <asm/io.h>
> +#include <linux/io.h>
NAK - please include either linux/io.h _or_ asm/io.h but not both.
> @@ -112,19 +113,10 @@ static int amba_kmi_probe(struct amba_device *dev,
> {
> struct amba_kmi_port *kmi;
> struct serio *io;
> - int ret;
> -
> - ret = amba_request_regions(dev, NULL);
> - if (ret)
> - return ret;
I'm /really/ not happy about that going.
> -
> - kmi = kzalloc(sizeof(struct amba_kmi_port), GFP_KERNEL);
> - io = kzalloc(sizeof(struct serio), GFP_KERNEL);
> - if (!kmi || !io) {
> - ret = -ENOMEM;
> - goto out;
> - }
>
> + io = devm_kzalloc(&dev->dev, sizeof(struct serio), GFP_KERNEL);
> + if (!io)
> + return -ENOMEM;
>
> io->id.type = SERIO_8042;
> io->write = amba_kmi_write;
> @@ -135,32 +127,20 @@ static int amba_kmi_probe(struct amba_device *dev,
> io->port_data = kmi;
> io->dev.parent = &dev->dev;
>
> + kmi->base = devm_ioremap_resource(&dev->dev, &dev->res);
> + if (IS_ERR(kmi->base))
> + return PTR_ERR(kmi->base);
NAK. Look carefully through the above changes to work out why. Please,
if you don't understand what you're doing, or don't even bother with a
build test of the code you've modified, don't even modify it in the
first place.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH] Input: ambakmi - Use managed interfaces
From: Himangi Saraogi @ 2014-07-13 17:30 UTC (permalink / raw)
To: Russell King, Dmitry Torokhov, linux-input, linux-kernel; +Cc: julia.lawall
This patch introduces the use of managed interfaces like devm_clk_get,
devm_kalloc etc. devm_ioremap_resource is used instead of
amba_request_regions and devm_ioremap. It also does away with the functions
to free the allocated memory in probe and remove functions. Also, some
labels in the probe function are removed.
Signed-off-by: Himangi Saraogi <himangi774@gmail.com>
Acked-by: Julia Lawall <julia.lawall@lip6.fr>
---
drivers/input/serio/ambakmi.c | 44 ++++++++++---------------------------------
1 file changed, 10 insertions(+), 34 deletions(-)
diff --git a/drivers/input/serio/ambakmi.c b/drivers/input/serio/ambakmi.c
index 8b748d9..405a175 100644
--- a/drivers/input/serio/ambakmi.c
+++ b/drivers/input/serio/ambakmi.c
@@ -23,6 +23,7 @@
#include <linux/clk.h>
#include <asm/io.h>
+#include <linux/io.h>
#include <asm/irq.h>
#define KMI_BASE (kmi->base)
@@ -112,19 +113,10 @@ static int amba_kmi_probe(struct amba_device *dev,
{
struct amba_kmi_port *kmi;
struct serio *io;
- int ret;
-
- ret = amba_request_regions(dev, NULL);
- if (ret)
- return ret;
-
- kmi = kzalloc(sizeof(struct amba_kmi_port), GFP_KERNEL);
- io = kzalloc(sizeof(struct serio), GFP_KERNEL);
- if (!kmi || !io) {
- ret = -ENOMEM;
- goto out;
- }
+ io = devm_kzalloc(&dev->dev, sizeof(struct serio), GFP_KERNEL);
+ if (!io)
+ return -ENOMEM;
io->id.type = SERIO_8042;
io->write = amba_kmi_write;
@@ -135,32 +127,20 @@ static int amba_kmi_probe(struct amba_device *dev,
io->port_data = kmi;
io->dev.parent = &dev->dev;
+ kmi->base = devm_ioremap_resource(&dev->dev, &dev->res);
+ if (IS_ERR(kmi->base))
+ return PTR_ERR(kmi->base);
kmi->io = io;
- kmi->base = ioremap(dev->res.start, resource_size(&dev->res));
- if (!kmi->base) {
- ret = -ENOMEM;
- goto out;
- }
- kmi->clk = clk_get(&dev->dev, "KMIREFCLK");
- if (IS_ERR(kmi->clk)) {
- ret = PTR_ERR(kmi->clk);
- goto unmap;
- }
+ kmi->clk = devm_clk_get(&dev->dev, "KMIREFCLK");
+ if (IS_ERR(kmi->clk))
+ return PTR_ERR(kmi->clk);
kmi->irq = dev->irq[0];
amba_set_drvdata(dev, kmi);
serio_register_port(kmi->io);
return 0;
-
- unmap:
- iounmap(kmi->base);
- out:
- kfree(kmi);
- kfree(io);
- amba_release_regions(dev);
- return ret;
}
static int amba_kmi_remove(struct amba_device *dev)
@@ -168,10 +148,6 @@ static int amba_kmi_remove(struct amba_device *dev)
struct amba_kmi_port *kmi = amba_get_drvdata(dev);
serio_unregister_port(kmi->io);
- clk_put(kmi->clk);
- iounmap(kmi->base);
- kfree(kmi);
- amba_release_regions(dev);
return 0;
}
--
1.9.1
^ permalink raw reply related
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