* Re: [PATCH 3/4] ARM: ep93xx: move pinctrl interfaces into include/linux/soc
From: Linus Walleij @ 2019-04-23 10:41 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Hartley Sweeten, Alexander Sverdlin, Bartlomiej Zolnierkiewicz,
Jens Axboe, Dmitry Torokhov, Thierry Reding, Mark Brown,
Olof Johansson, Linux ARM, linux-kernel@vger.kernel.org,
linux-ide, Linux Input, linux-pwm,
moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...
In-Reply-To: <20190415192734.935387-3-arnd@arndb.de>
On Mon, Apr 15, 2019 at 9:30 PM Arnd Bergmann <arnd@arndb.de> wrote:
> ep93xx does not have a proper pinctrl driver, but does things
> ad-hoc through mach/platform.h, which is also used for setting
> up the boards.
>
> To avoid using mach/*.h headers completely, let's move the interfaces
> into include/linux/soc/. This is far from great, but gets the job
> done here, without the need for a proper pinctrl driver.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock
From: Al Viro @ 2019-04-23 11:06 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com
Cc: Mukesh Ojha, linux-input, linux-kernel, Gaurav Kohli,
Peter Hutterer, Martin Kepplinger, Paul E. McKenney
In-Reply-To: <20190423084944.gj2boxfcg7lp4zad@penguin>
On Tue, Apr 23, 2019 at 08:49:44AM +0000, dmitry.torokhov@gmail.com wrote:
> On Tue, Apr 23, 2019 at 12:51:13PM +0530, Mukesh Ojha wrote:
> > I have taken care this case from ioctl and release point of view.
> >
> > Even if the release gets called first it will make the
> > file->private_data=NULL.
> > and further call to ioctl will not be a problem as the check is already
> > there.
>
> Al, do we have any protections in VFS layer from userspace hanging onto
> a file descriptor and calling ioctl() on it even as another thread
> calls close() on the same fd?
>
> Should the issue be solved by individual drivers, or more careful
> accounting for currently running operations is needed at VFS layer?
Neither. An overlap of ->release() and ->ioctl() is possible only
if you've got memory corruption somewhere.
close() overlapping ioctl() is certainly possible, and won't trigger
that at all - sys_ioctl() holds onto reference to struct file, so
its refcount won't reach zero until we are done with it.
^ permalink raw reply
* Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock
From: Al Viro @ 2019-04-23 12:15 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com
Cc: Mukesh Ojha, linux-input, linux-kernel, Gaurav Kohli,
Peter Hutterer, Martin Kepplinger, Paul E. McKenney
In-Reply-To: <20190423110611.GL2217@ZenIV.linux.org.uk>
On Tue, Apr 23, 2019 at 12:06:12PM +0100, Al Viro wrote:
> On Tue, Apr 23, 2019 at 08:49:44AM +0000, dmitry.torokhov@gmail.com wrote:
> > On Tue, Apr 23, 2019 at 12:51:13PM +0530, Mukesh Ojha wrote:
>
> > > I have taken care this case from ioctl and release point of view.
> > >
> > > Even if the release gets called first it will make the
> > > file->private_data=NULL.
> > > and further call to ioctl will not be a problem as the check is already
> > > there.
> >
> > Al, do we have any protections in VFS layer from userspace hanging onto
> > a file descriptor and calling ioctl() on it even as another thread
> > calls close() on the same fd?
> >
> > Should the issue be solved by individual drivers, or more careful
> > accounting for currently running operations is needed at VFS layer?
>
> Neither. An overlap of ->release() and ->ioctl() is possible only
> if you've got memory corruption somewhere.
>
> close() overlapping ioctl() is certainly possible, and won't trigger
> that at all - sys_ioctl() holds onto reference to struct file, so
> its refcount won't reach zero until we are done with it.
I'd like to see a reproducer; _if_ we are really getting such overlaps,
we have something buggering struct file refcounts (e.g. stray fput()
eventually leading to dangling pointer in descriptor table) or an
outright memory corruption mangling descriptor table/kernel stack/
refcount/->private_data/etc.
Details, please - I tried to google some context for that, but hadn't
found anything useful ;-/
^ permalink raw reply
* Re: [PATCH] HID: intel-ish-hid: Add Comet Lake PCI device ID
From: Jiri Kosina @ 2019-04-23 12:29 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20190420020031.58410-1-srinivas.pandruvada@linux.intel.com>
On Fri, 19 Apr 2019, Srinivas Pandruvada wrote:
> Added Comet Lake PCI device ID to the supported device list.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/hid/intel-ish-hid/ipc/hw-ish.h | 1 +
> drivers/hid/intel-ish-hid/ipc/pci-ish.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/hid/intel-ish-hid/ipc/hw-ish.h b/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> index 08a8327dfd22..523c0cbd44a4 100644
> --- a/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> +++ b/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> @@ -31,6 +31,7 @@
> #define CNL_H_DEVICE_ID 0xA37C
> #define ICL_MOBILE_DEVICE_ID 0x34FC
> #define SPT_H_DEVICE_ID 0xA135
> +#define CML_LP_DEVICE_ID 0x02FC
>
> #define REVISION_ID_CHT_A0 0x6
> #define REVISION_ID_CHT_Ax_SI 0x0
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index a6e1ee744f4d..ac0a179daf23 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -40,6 +40,7 @@ static const struct pci_device_id ish_pci_tbl[] = {
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, CNL_H_DEVICE_ID)},
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, ICL_MOBILE_DEVICE_ID)},
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, SPT_H_DEVICE_ID)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, CML_LP_DEVICE_ID)},
> {0, }
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH] Input: walkera0701 - Fix possible NULL pointer dereference in walkera0701_detach
From: Yue Haibing @ 2019-04-23 14:56 UTC (permalink / raw)
To: dmitry.torokhov, sudip; +Cc: linux-kernel, linux-input, YueHaibing
From: YueHaibing <yuehaibing@huawei.com>
KASAN report this:
walkera0701: failed to allocate input device
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] SMP KASAN PTI
CPU: 1 PID: 5324 Comm: syz-executor.0 Tainted: G C 5.1.0-rc3+ #8
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
RIP: 0010:input_unregister_device+0x21/0xe0 drivers/input/input.c:2192
Code: 2e 0f 1f 84 00 00 00 00 00 53 48 89 fb e8 07 41 f6 fe 48 8d bb 20 07 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 84 c0 0f 8e 92 00 00 00 80 bb 20 07 00 00
RSP: 0018:ffff8881f58dfd30 EFLAGS: 00010206
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff82460ca9
RDX: 00000000000000e4 RSI: ffffc900013d3000 RDI: 0000000000000720
RBP: 0000000000000000 R08: ffffed103d30caf7 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: dffffc0000000000
R13: ffffffffc1633000 R14: ffffffffc086b320 R15: 1ffff1103eb1bfaf
FS: 00007fa407200700(0000) GS:ffff8881f7300000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000001b33924000 CR3: 00000001e270c006 CR4: 00000000007606e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
walkera0701_detach+0x8e/0xba [walkera0701]
port_detach+0x73/0x90 [parport]
bus_for_each_dev+0x154/0x1e0 drivers/base/bus.c:304
parport_unregister_driver+0x1f8/0x270 [parport]
__do_sys_delete_module kernel/module.c:1018 [inline]
__se_sys_delete_module kernel/module.c:961 [inline]
__x64_sys_delete_module+0x30c/0x480 kernel/module.c:961
do_syscall_64+0x9f/0x450 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x462e99
Code: f7 d8 64 89 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fa4071ffc58 EFLAGS: 00000246 ORIG_RAX: 00000000000000b0
RAX: ffffffffffffffda RBX: 000000000073bf00 RCX: 0000000000462e99
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000000200001c0
RBP: 0000000000000002 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fa4072006bc
R13: 00000000004bcca9 R14: 00000000006f6b48 R15: 00000000ffffffff
Modules linked in: walkera0701(-) tps65090_regulator intel_th mptbase adm1031 snd_soc_wm8753 snd_soc_core snd_pcm_dmaengine snd_pcm ac97_bus snd_compress rtc_ds1286 snd_seq_dummy snd_seq snd_timer snd_seq_device snd soundcore comedi(C) i2c_mux_ltc4306 i2c_mux max14577_regulator max14577 usbcore hid cmac mc13783_regulator mc13xxx_regulator_core mc13xxx_core of_mdio fixed_phy libphy iptable_security iptable_raw iptable_mangle iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 iptable_filter bpfilter ip6_vti ip_vti ip_gre ipip sit tunnel4 ip_tunnel hsr veth netdevsim vxcan batman_adv cfg80211 rfkill chnl_net caif nlmon dummy team bonding vcan bridge stp llc ip6_gre gre ip6_tunnel tunnel6 tun joydev mousedev ppdev tpm kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul crc32c_inte
l ghash_clmulni_intel ide_pci_generic aesni_intel aes_x86_64 piix crypto_simd cryptd input_leds ide_core psmouse glue_helper intel_agp serio_raw intel_gtt ata_generic agpgart i2c_piix4
pata_acpi parport_pc parport floppy rtc_cmos sch_fq_codel ip_tables x_tables sha1_ssse3 sha1_generic ipv6 [last unloaded: walkera0701]
Dumping ftrace buffer:
(ftrace buffer empty)
---[ end trace 17f6dd401f34af3e ]---
In walkera0701_attach(), if input_allocate_device failed,
w->input_dev is set to NULL. But in walkera0701_detach it
is not checked while passing to input_unregister_device(),
this will trigger a NULL pointer dereference issue.
There is also another possible use-after-free issue, when
input_register_device() fails, input_free_device be
called to free input dev, then in walkera0701_detach()
calling input_unregister_device will trigger use-after-free
while accessing input dev
This patch set w->parport to NULL on walkera0701_attach failed,
and only do detach in case attach success.
Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 221bcb24c653 ("Input: walkera0701 - use parallel port device model")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/input/joystick/walkera0701.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/input/joystick/walkera0701.c b/drivers/input/joystick/walkera0701.c
index dce313d..852b8c5 100644
--- a/drivers/input/joystick/walkera0701.c
+++ b/drivers/input/joystick/walkera0701.c
@@ -207,13 +207,13 @@ static void walkera0701_attach(struct parport *pp)
if (pp->number != walkera0701_pp_no) {
pr_debug("Not using parport%d.\n", pp->number);
- return;
+ goto err_out;
}
if (pp->irq == -1) {
pr_err("parport %d does not have interrupt assigned\n",
pp->number);
- return;
+ goto err_out;
}
w->parport = pp;
@@ -228,7 +228,7 @@ static void walkera0701_attach(struct parport *pp)
if (!w->pardevice) {
pr_err("failed to register parport device\n");
- return;
+ goto err_out;
}
if (parport_negotiate(w->pardevice->port, IEEE1284_MODE_COMPAT)) {
@@ -279,13 +279,15 @@ static void walkera0701_attach(struct parport *pp)
input_free_device(w->input_dev);
err_unregister_device:
parport_unregister_device(w->pardevice);
+err_out:
+ w->parport = NULL;
}
static void walkera0701_detach(struct parport *port)
{
struct walkera_dev *w = &w_dev;
- if (!w->pardevice || w->parport->number != port->number)
+ if (!w->parport)
return;
input_unregister_device(w->input_dev);
--
2.7.0
^ permalink raw reply related
* [PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set
From: Benjamin Tissoires @ 2019-04-23 15:46 UTC (permalink / raw)
To: Jiri Kosina, James Feeney, Peter Hutterer
Cc: linux-input, linux-kernel, Benjamin Tissoires, stable
Some old mice have a tendency to not accept the high resolution multiplier.
They reply with a -EPIPE which was previously ignored.
Force the call to resolution multiplier to be synchronous and actually
check for the answer. If this fails, consider the mouse like a normal one.
Fixes: 2dc702c991e377 ("HID: input: use the Resolution Multiplier for
high-resolution scrolling")
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1700071
Reported-and-tested-by: James Feeney <james@nurealm.net>
Cc: stable@vger.kernel.org # v5.0+
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-core.c | 7 +++-
drivers/hid/hid-input.c | 81 +++++++++++++++++++++++++----------------
include/linux/hid.h | 2 +-
3 files changed, 56 insertions(+), 34 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 76464aabd20c..92387fc0bf18 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1637,7 +1637,7 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
* Implement a generic .request() callback, using .raw_request()
* DO NOT USE in hid drivers directly, but through hid_hw_request instead.
*/
-void __hid_request(struct hid_device *hid, struct hid_report *report,
+int __hid_request(struct hid_device *hid, struct hid_report *report,
int reqtype)
{
char *buf;
@@ -1646,7 +1646,7 @@ void __hid_request(struct hid_device *hid, struct hid_report *report,
buf = hid_alloc_report_buf(report, GFP_KERNEL);
if (!buf)
- return;
+ return -ENOMEM;
len = hid_report_len(report);
@@ -1663,8 +1663,11 @@ void __hid_request(struct hid_device *hid, struct hid_report *report,
if (reqtype == HID_REQ_GET_REPORT)
hid_input_report(hid, report->type, buf, ret, 0);
+ ret = 0;
+
out:
kfree(buf);
+ return ret;
}
EXPORT_SYMBOL_GPL(__hid_request);
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 1fce0076e7dc..fadf76f0a386 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1542,52 +1542,71 @@ static void hidinput_close(struct input_dev *dev)
hid_hw_close(hid);
}
-static void hidinput_change_resolution_multipliers(struct hid_device *hid)
+static bool __hidinput_change_resolution_multipliers(struct hid_device *hid,
+ struct hid_report *report, bool use_logical_max)
{
- struct hid_report_enum *rep_enum;
- struct hid_report *rep;
struct hid_usage *usage;
+ bool update_needed = false;
int i, j;
- rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
- list_for_each_entry(rep, &rep_enum->report_list, list) {
- bool update_needed = false;
+ if (report->maxfield == 0)
+ return false;
- if (rep->maxfield == 0)
- continue;
+ /*
+ * If we have more than one feature within this report we
+ * need to fill in the bits from the others before we can
+ * overwrite the ones for the Resolution Multiplier.
+ */
+ if (report->maxfield > 1) {
+ hid_hw_request(hid, report, HID_REQ_GET_REPORT);
+ hid_hw_wait(hid);
+ }
- /*
- * If we have more than one feature within this report we
- * need to fill in the bits from the others before we can
- * overwrite the ones for the Resolution Multiplier.
+ for (i = 0; i < report->maxfield; i++) {
+ __s32 value = use_logical_max ?
+ report->field[i]->logical_maximum :
+ report->field[i]->logical_minimum;
+
+ /* There is no good reason for a Resolution
+ * Multiplier to have a count other than 1.
+ * Ignore that case.
*/
- if (rep->maxfield > 1) {
- hid_hw_request(hid, rep, HID_REQ_GET_REPORT);
- hid_hw_wait(hid);
- }
+ if (report->field[i]->report_count != 1)
+ continue;
- for (i = 0; i < rep->maxfield; i++) {
- __s32 logical_max = rep->field[i]->logical_maximum;
+ for (j = 0; j < report->field[i]->maxusage; j++) {
+ usage = &report->field[i]->usage[j];
- /* There is no good reason for a Resolution
- * Multiplier to have a count other than 1.
- * Ignore that case.
- */
- if (rep->field[i]->report_count != 1)
+ if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
continue;
- for (j = 0; j < rep->field[i]->maxusage; j++) {
- usage = &rep->field[i]->usage[j];
+ *report->field[i]->value = value;
+ update_needed = true;
+ }
+ }
+
+ return update_needed;
+}
- if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
- continue;
+static void hidinput_change_resolution_multipliers(struct hid_device *hid)
+{
+ struct hid_report_enum *rep_enum;
+ struct hid_report *rep;
+ int ret;
- *rep->field[i]->value = logical_max;
- update_needed = true;
+ rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
+ list_for_each_entry(rep, &rep_enum->report_list, list) {
+ bool update_needed = __hidinput_change_resolution_multipliers(hid,
+ rep, true);
+
+ if (update_needed) {
+ ret = __hid_request(hid, rep, HID_REQ_SET_REPORT);
+ if (ret) {
+ __hidinput_change_resolution_multipliers(hid,
+ rep, false);
+ return;
}
}
- if (update_needed)
- hid_hw_request(hid, rep, HID_REQ_SET_REPORT);
}
/* refresh our structs */
diff --git a/include/linux/hid.h b/include/linux/hid.h
index ac0c70b4ce10..5a24ebfb5b47 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -894,7 +894,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
unsigned int hidinput_count_leds(struct hid_device *hid);
__s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
void hid_output_report(struct hid_report *report, __u8 *data);
-void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
+int __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
struct hid_device *hid_allocate_device(void);
struct hid_report *hid_register_report(struct hid_device *device,
--
2.19.2
^ permalink raw reply related
* [PATCH 2/2] HID: input: fix assignment of .value
From: Benjamin Tissoires @ 2019-04-23 15:46 UTC (permalink / raw)
To: Jiri Kosina, James Feeney, Peter Hutterer
Cc: linux-input, linux-kernel, Benjamin Tissoires, stable
In-Reply-To: <20190423154615.18257-1-benjamin.tissoires@redhat.com>
The value field is actually an array of .maxfield. We should assign the
correct number to the correct usage.
Not that we never encounter a device that requires this ATM, but better
have the proper code path.
Fixes: 2dc702c991e377 ("HID: input: use the Resolution Multiplier for
high-resolution scrolling")
Cc: stable@vger.kernel.org # v5.0+
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-input.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index fadf76f0a386..6dd0294e1133 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1580,7 +1580,7 @@ static bool __hidinput_change_resolution_multipliers(struct hid_device *hid,
if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
continue;
- *report->field[i]->value = value;
+ report->field[i]->value[j] = value;
update_needed = true;
}
}
--
2.19.2
^ permalink raw reply related
* [PATCH v4 0/2] Basic DT support for Lenovo Miix 630
From: Jeffrey Hugo @ 2019-04-23 16:05 UTC (permalink / raw)
Cc: lee.jones, bjorn.andersson, dmitry.torokhov, robh+dt,
mark.rutland, agross, david.brown, jikos, benjamin.tissoires,
hdegoede, linux-input, devicetree, linux-arm-msm, linux-kernel,
Jeffrey Hugo
The Lenovo Miix 630 is one of three ARM based (specifically Qualcomm
MSM8998) laptops that comes with Windows, and seems to have a dedicated
following of folks intrested to get Linux up and running on it.
This series adds support for the basic functionality this is validated
towork using devicetree. Although the laptops do feed ACPI to Windows,
the existing MSM8998 support in mainline is DT based, so DT provides a
quick path to functionality while ACPI support is investigated.
The three devices are very similar, but do have differences in the set
of peripherals supported, so the idea is that the vast majority of the
support for all three can live in a common include, which should reduce
overall duplication. Adding support for the other two devices as a
follow on should involve minimal work.
The bleeding edge work for these laptops and work in progress can be
found at https://github.com/aarch64-laptops/prebuilt
v4:
-Changed the hid-quirks ELAN handling around per Benjamin Tissoires
-Dropped new DT binding
v3:
-Changed "clam" to "clamshell"
-Defined a dt binding for the combo Elan keyboard + touchpad device
-Adjusted the HID quirk to be correct for dt boot
-Removed extranious comment in board dts
-Fixed board level compatible
v2:
-Changed "cls" to "clam" since feedback indicated "cls" is too opaque,
but
"clamshell" is a mouthfull. "clam" seems to be a happy medium.
Jeffrey Hugo (3):
dt-bindings: input: add Elan 400 combo keyboard/touchpad over i2c
HID: quirks: Fix keyboard + touchpad on Lenovo Miix 630 for DT
arm64: dts: qcom: Add Lenovo Miix 630
.../bindings/input/elan,combo400-i2c.txt | 11 +
arch/arm64/boot/dts/qcom/Makefile | 1 +
.../boot/dts/qcom/msm8998-clamshell.dtsi | 278 ++++++++++++++++++
.../boot/dts/qcom/msm8998-lenovo-miix-630.dts | 30 ++
drivers/hid/hid-quirks.c | 3 +-
5 files changed, 322 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/input/elan,combo400-i2c.txt
create mode 100644 arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
create mode 100644 arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
--
2.17.1
^ permalink raw reply
* [PATCH v4 1/2] HID: quirks: Refactor ELAN 400 and 401 handling
From: Jeffrey Hugo @ 2019-04-23 16:06 UTC (permalink / raw)
To: dmitry.torokhov, jikos, benjamin.tissoires
Cc: lee.jones, bjorn.andersson, robh+dt, mark.rutland, agross,
david.brown, hdegoede, linux-input, devicetree, linux-arm-msm,
linux-kernel, Jeffrey Hugo
In-Reply-To: <20190423160543.9922-1-jeffrey.l.hugo@gmail.com>
There needs to be coordination between hid-quirks and the elan_i2c driver
about which devices are handled by what drivers. Currently, both use
whitelists, which results in valid devices being unhandled by default,
when they should not be rejected by hid-quirks. This is quickly becoming
an issue.
Since elan_i2c has a maintained whitelist of what devices it will handle,
use that to implement a blacklist in hid-quirks so that only the devices
that need to be handled by elan_i2c get rejected by hid-quirks, and
everything else is handled by default. The downside is the whitelist and
blacklist need to be kept in sync.
Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
---
drivers/hid/hid-quirks.c | 64 ++++++++++++++++++++++++-----
drivers/input/mouse/elan_i2c_core.c | 4 ++
2 files changed, 58 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 77ffba48cc73..656485e08eb7 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -987,17 +987,61 @@ bool hid_ignore(struct hid_device *hdev)
break;
case USB_VENDOR_ID_ELAN:
/*
- * Many Elan devices have a product id of 0x0401 and are handled
- * by the elan_i2c input driver. But the ACPI HID ELAN0800 dev
- * is not (and cannot be) handled by that driver ->
- * Ignore all 0x0401 devs except for the ELAN0800 dev.
+ * Blacklist of everything that gets handled by the elan_i2c
+ * input driver. This should be kept in sync with the whitelist
+ * that exists in that driver. This avoids disabling valid
+ * touchpads and other ELAN devices.
*/
- if (hdev->product == 0x0401 &&
- strncmp(hdev->name, "ELAN0800", 8) != 0)
- return true;
- /* Same with product id 0x0400 */
- if (hdev->product == 0x0400 &&
- strncmp(hdev->name, "QTEC0001", 8) != 0)
+ if ((hdev->product == 0x0401 || hdev->product == 0x0400) &&
+ (strncmp(hdev->name, "ELAN0000", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0100", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0600", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0601", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0602", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0603", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0604", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0605", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0606", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0607", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0608", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0609", 8) == 0 ||
+ strncmp(hdev->name, "ELAN060B", 8) == 0 ||
+ strncmp(hdev->name, "ELAN060C", 8) == 0 ||
+ strncmp(hdev->name, "ELAN060F", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0610", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0611", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0612", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0613", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0614", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0615", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0616", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0617", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0618", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0619", 8) == 0 ||
+ strncmp(hdev->name, "ELAN061A", 8) == 0 ||
+ strncmp(hdev->name, "ELAN061B", 8) == 0 ||
+ strncmp(hdev->name, "ELAN061C", 8) == 0 ||
+ strncmp(hdev->name, "ELAN061D", 8) == 0 ||
+ strncmp(hdev->name, "ELAN061E", 8) == 0 ||
+ strncmp(hdev->name, "ELAN061F", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0620", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0621", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0622", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0623", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0624", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0625", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0626", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0627", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0628", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0629", 8) == 0 ||
+ strncmp(hdev->name, "ELAN062A", 8) == 0 ||
+ strncmp(hdev->name, "ELAN062B", 8) == 0 ||
+ strncmp(hdev->name, "ELAN062C", 8) == 0 ||
+ strncmp(hdev->name, "ELAN062D", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0631", 8) == 0 ||
+ strncmp(hdev->name, "ELAN0632", 8) == 0 ||
+ strncmp(hdev->name, "ELAN1000", 8) == 0 ||
+ strncmp(hdev->name, "elan,ekth3000", 13) == 0))
return true;
break;
}
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index f9525d6f0bfe..3ded19528cd4 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -1332,6 +1332,10 @@ static const struct i2c_device_id elan_id[] = {
};
MODULE_DEVICE_TABLE(i2c, elan_id);
+/*
+ * when these whtielists get updated, the corresponding blacklist in hid-quirks
+ * needs to be updated to match.
+ */
#ifdef CONFIG_ACPI
static const struct acpi_device_id elan_acpi_id[] = {
{ "ELAN0000", 0 },
--
2.17.1
^ permalink raw reply related
* [PATCH v4 2/2] arm64: dts: qcom: Add Lenovo Miix 630
From: Jeffrey Hugo @ 2019-04-23 16:06 UTC (permalink / raw)
To: bjorn.andersson, robh+dt, mark.rutland, agross, david.brown
Cc: lee.jones, dmitry.torokhov, jikos, benjamin.tissoires, hdegoede,
linux-input, devicetree, linux-arm-msm, linux-kernel,
Jeffrey Hugo
In-Reply-To: <20190423160543.9922-1-jeffrey.l.hugo@gmail.com>
This adds the initial DT for the Lenovo Miix 630 laptop. Supported
functionality includes USB (host), microSD-card, keyboard, and trackpad.
Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
---
arch/arm64/boot/dts/qcom/Makefile | 1 +
.../boot/dts/qcom/msm8998-clamshell.dtsi | 278 ++++++++++++++++++
.../boot/dts/qcom/msm8998-lenovo-miix-630.dts | 30 ++
3 files changed, 309 insertions(+)
create mode 100644 arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
create mode 100644 arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
index 21d548f02d39..c3e4307bcbd4 100644
--- a/arch/arm64/boot/dts/qcom/Makefile
+++ b/arch/arm64/boot/dts/qcom/Makefile
@@ -6,6 +6,7 @@ dtb-$(CONFIG_ARCH_QCOM) += msm8916-mtp.dtb
dtb-$(CONFIG_ARCH_QCOM) += msm8992-bullhead-rev-101.dtb
dtb-$(CONFIG_ARCH_QCOM) += msm8994-angler-rev-101.dtb
dtb-$(CONFIG_ARCH_QCOM) += msm8996-mtp.dtb
+dtb-$(CONFIG_ARCH_QCOM) += msm8998-lenovo-miix-630.dtb
dtb-$(CONFIG_ARCH_QCOM) += msm8998-mtp.dtb
dtb-$(CONFIG_ARCH_QCOM) += sdm845-mtp.dtb
dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-1000.dtb
diff --git a/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi b/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
new file mode 100644
index 000000000000..1a341d4b1597
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2019, Jeffrey Hugo. All rights reserved. */
+
+/*
+ * Common include for MSM8998 clamshell devices, ie the Lenovo Miix 630,
+ * Asus NovaGo TP370QL, and HP Envy x2. All three devices are basically the
+ * same, with differences in peripherals.
+ */
+
+#include "msm8998.dtsi"
+#include "pm8998.dtsi"
+#include "pm8005.dtsi"
+
+/ {
+ chosen {
+ };
+
+ thermal-zones {
+ battery-thermal {
+ polling-delay-passive = <250>;
+ polling-delay = <1000>;
+
+ thermal-sensors = <&tsens0 0>;
+
+ trips {
+ battery_crit: trip0 {
+ temperature = <60000>;
+ hysteresis = <2000>;
+ type = "critical";
+ };
+ };
+ };
+
+ skin-thermal {
+ polling-delay-passive = <250>;
+ polling-delay = <1000>;
+
+ thermal-sensors = <&tsens1 5>;
+
+ trips {
+ skin_alert: trip0 {
+ temperature = <44000>;
+ hysteresis = <2000>;
+ type = "passive";
+ };
+
+ skip_crit: trip1 {
+ temperature = <70000>;
+ hysteresis = <2000>;
+ type = "critical";
+ };
+ };
+ };
+ };
+
+ vph_pwr: vph-pwr-regulator {
+ compatible = "regulator-fixed";
+ regulator-name = "vph_pwr";
+ regulator-always-on;
+ regulator-boot-on;
+ };
+};
+
+&qusb2phy {
+ status = "okay";
+
+ vdda-pll-supply = <&vreg_l12a_1p8>;
+ vdda-phy-dpdm-supply = <&vreg_l24a_3p075>;
+};
+
+&rpm_requests {
+ pm8998-regulators {
+ compatible = "qcom,rpm-pm8998-regulators";
+
+ vdd_s1-supply = <&vph_pwr>;
+ vdd_s2-supply = <&vph_pwr>;
+ vdd_s3-supply = <&vph_pwr>;
+ vdd_s4-supply = <&vph_pwr>;
+ vdd_s5-supply = <&vph_pwr>;
+ vdd_s6-supply = <&vph_pwr>;
+ vdd_s7-supply = <&vph_pwr>;
+ vdd_s8-supply = <&vph_pwr>;
+ vdd_s9-supply = <&vph_pwr>;
+ vdd_s10-supply = <&vph_pwr>;
+ vdd_s11-supply = <&vph_pwr>;
+ vdd_s12-supply = <&vph_pwr>;
+ vdd_s13-supply = <&vph_pwr>;
+ vdd_l1_l27-supply = <&vreg_s7a_1p025>;
+ vdd_l2_l8_l17-supply = <&vreg_s3a_1p35>;
+ vdd_l3_l11-supply = <&vreg_s7a_1p025>;
+ vdd_l4_l5-supply = <&vreg_s7a_1p025>;
+ vdd_l6-supply = <&vreg_s5a_2p04>;
+ vdd_l7_l12_l14_l15-supply = <&vreg_s5a_2p04>;
+ vdd_l9-supply = <&vph_pwr>;
+ vdd_l10_l23_l25-supply = <&vph_pwr>;
+ vdd_l13_l19_l21-supply = <&vph_pwr>;
+ vdd_l16_l28-supply = <&vph_pwr>;
+ vdd_l18_l22-supply = <&vph_pwr>;
+ vdd_l20_l24-supply = <&vph_pwr>;
+ vdd_l26-supply = <&vreg_s3a_1p35>;
+ vdd_lvs1_lvs2-supply = <&vreg_s4a_1p8>;
+
+ vreg_s3a_1p35: s3 {
+ regulator-min-microvolt = <1352000>;
+ regulator-max-microvolt = <1352000>;
+ };
+ vreg_s4a_1p8: s4 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-allow-set-load;
+ };
+ vreg_s5a_2p04: s5 {
+ regulator-min-microvolt = <1904000>;
+ regulator-max-microvolt = <2040000>;
+ };
+ vreg_s7a_1p025: s7 {
+ regulator-min-microvolt = <900000>;
+ regulator-max-microvolt = <1028000>;
+ };
+ vreg_l1a_0p875: l1 {
+ regulator-min-microvolt = <880000>;
+ regulator-max-microvolt = <880000>;
+ regulator-allow-set-load;
+ };
+ vreg_l2a_1p2: l2 {
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1200000>;
+ regulator-allow-set-load;
+ };
+ vreg_l3a_1p0: l3 {
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <1000000>;
+ };
+ vreg_l5a_0p8: l5 {
+ regulator-min-microvolt = <800000>;
+ regulator-max-microvolt = <800000>;
+ };
+ vreg_l6a_1p8: l6 {
+ regulator-min-microvolt = <1808000>;
+ regulator-max-microvolt = <1808000>;
+ };
+ vreg_l7a_1p8: l7 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ vreg_l8a_1p2: l8 {
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1200000>;
+ };
+ vreg_l9a_1p8: l9 {
+ regulator-min-microvolt = <1808000>;
+ regulator-max-microvolt = <2960000>;
+ };
+ vreg_l10a_1p8: l10 {
+ regulator-min-microvolt = <1808000>;
+ regulator-max-microvolt = <2960000>;
+ };
+ vreg_l11a_1p0: l11 {
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <1000000>;
+ };
+ vreg_l12a_1p8: l12 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ vreg_l13a_2p95: l13 {
+ regulator-min-microvolt = <1808000>;
+ regulator-max-microvolt = <2960000>;
+ };
+ vreg_l14a_1p88: l14 {
+ regulator-min-microvolt = <1880000>;
+ regulator-max-microvolt = <1880000>;
+ };
+ vreg_15a_1p8: l15 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ vreg_l16a_2p7: l16 {
+ regulator-min-microvolt = <2704000>;
+ regulator-max-microvolt = <2704000>;
+ };
+ vreg_l17a_1p3: l17 {
+ regulator-min-microvolt = <1304000>;
+ regulator-max-microvolt = <1304000>;
+ };
+ vreg_l18a_2p7: l18 {
+ regulator-min-microvolt = <2704000>;
+ regulator-max-microvolt = <2704000>;
+ };
+ vreg_l19a_3p0: l19 {
+ regulator-min-microvolt = <3008000>;
+ regulator-max-microvolt = <3008000>;
+ };
+ vreg_l20a_2p95: l20 {
+ regulator-min-microvolt = <2960000>;
+ regulator-max-microvolt = <2960000>;
+ regulator-allow-set-load;
+ };
+ vreg_l21a_2p95: l21 {
+ regulator-min-microvolt = <2960000>;
+ regulator-max-microvolt = <2960000>;
+ regulator-allow-set-load;
+ regulator-system-load = <800000>;
+ };
+ vreg_l22a_2p85: l22 {
+ regulator-min-microvolt = <2864000>;
+ regulator-max-microvolt = <2864000>;
+ };
+ vreg_l23a_3p3: l23 {
+ regulator-min-microvolt = <3312000>;
+ regulator-max-microvolt = <3312000>;
+ };
+ vreg_l24a_3p075: l24 {
+ regulator-min-microvolt = <3088000>;
+ regulator-max-microvolt = <3088000>;
+ };
+ vreg_l25a_3p3: l25 {
+ regulator-min-microvolt = <3104000>;
+ regulator-max-microvolt = <3312000>;
+ };
+ vreg_l26a_1p2: l26 {
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1200000>;
+ };
+ vreg_l28_3p0: l28 {
+ regulator-min-microvolt = <3008000>;
+ regulator-max-microvolt = <3008000>;
+ };
+
+ vreg_lvs1a_1p8: lvs1 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+
+ vreg_lvs2a_1p8: lvs2 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+
+ };
+};
+
+&tlmm {
+ gpio-reserved-ranges = <0 4>, <81 4>;
+
+ touchpad: touchpad {
+ config {
+ pins = "gpio123";
+ bias-pull-up; /* pull up */
+ };
+ };
+};
+
+&sdhc2 {
+ status = "okay";
+
+ vmmc-supply = <&vreg_l21a_2p95>;
+ vqmmc-supply = <&vreg_l13a_2p95>;
+
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&sdc2_clk_on &sdc2_cmd_on &sdc2_data_on &sdc2_cd_on>;
+ pinctrl-1 = <&sdc2_clk_off &sdc2_cmd_off &sdc2_data_off &sdc2_cd_off>;
+};
+
+&usb3 {
+ status = "okay";
+};
+
+&usb3_dwc3 {
+ dr_mode = "host"; /* Force to host until we have Type-C hooked up */
+};
+
+&usb3phy {
+ status = "okay";
+
+ vdda-phy-supply = <&vreg_l1a_0p875>;
+ vdda-pll-supply = <&vreg_l2a_1p2>;
+};
diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
new file mode 100644
index 000000000000..407c6a32911c
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2019, Jeffrey Hugo. All rights reserved. */
+
+/dts-v1/;
+
+#include "msm8998-clamshell.dtsi"
+
+/ {
+ model = "Lenovo Miix 630";
+ compatible = "lenovo,miix-630", "qcom,msm8998";
+};
+
+&blsp1_i2c6 {
+ status = "okay";
+
+ keyboard@3a {
+ compatible = "hid-over-i2c";
+ interrupt-parent = <&tlmm>;
+ interrupts = <0x79 IRQ_TYPE_LEVEL_LOW>;
+ reg = <0x3a>;
+ hid-descr-addr = <0x0001>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&touchpad>;
+ };
+};
+
+&sdhc2 {
+ cd-gpios = <&tlmm 95 GPIO_ACTIVE_HIGH>;
+};
--
2.17.1
^ permalink raw reply related
* RE: [PATCH][v2] drivers: hid: Add a module description line to the hid_hyperv driver
From: Michael Kelley @ 2019-04-23 16:38 UTC (permalink / raw)
To: Joseph Salisbury, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, jikos@kernel.org,
benjamin.tissoires@redhat.com
Cc: linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <324016691a62a13ce46c9ccd35c7e492bf609fd6.1555967348.git.joseph.salisbury@microsoft.com>
From: Joseph Salisbury <joseph.salisbury@microsoft.com> Sent: Monday, April 22, 2019 8:47 PM
>
> This patch only adds a MODULE_DESCRIPTION statement to the driver.
> This change is only cosmetic, so there should be no runtime impact.
>
>
> Signed-off-by: Joseph Salisbury <joseph.salisbury@microsoft.com>
> ---
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* RE: [PATCH][v2] drivers: input: serio: Add a module desription to the hyperv_keyboard driver
From: Michael Kelley @ 2019-04-23 16:39 UTC (permalink / raw)
To: Joseph Salisbury, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, dmitry.torokhov@gmail.com
Cc: linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <6a2c2e15e90138fcef67bed294d2e82d8d6f679b.1555967323.git.joseph.salisbury@microsoft.com>
From: Joseph Salisbury <joseph.salisbury@microsoft.com> Sent: Monday, April 22, 2019 8:47 PM
>
> This patch only adds a MODULE_DESCRIPTION statement to the driver.
> This change is only cosmetic, so there should be no runtime impact.
>
> Signed-off-by: Joseph Salisbury <joseph.salisbury@microsoft.com>
> ---
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* Re: [PATCH 2/2] HID: input: fix assignment of .value
From: James Feeney @ 2019-04-23 17:54 UTC (permalink / raw)
To: Sasha Levin, Benjamin Tissoires, Jiri Kosina
Cc: linux-input, linux-kernel, stable
In-Reply-To: <20190423172117.CC66520835@mail.kernel.org>
On 4/23/19 11:21 AM, Sasha Levin wrote:
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 2dc702c991e3 HID: input: use the Resolution Multiplier for high-resolution scrolling.
>
> The bot has tested the following trees: v5.0.9.
>
> v5.0.9: Failed to apply! Possible dependencies:
> 724f54bc0063 ("HID: input: make sure the wheel high resolution multiplier is set")
>
>
> How should we proceed with this patch?
>
> --
> Thanks,
> Sasha
>
This patch will only apply *after* the application of the first patch, since they "overlap".
Yes, that is probably not the best circumstance. I think Benjamin wanted to keep the issue in patch 2/2 distinct.
Was the first patch applied before attempting application of the second patch?
James
^ permalink raw reply
* Re: [PATCH 2/2] HID: input: fix assignment of .value
From: Sasha Levin @ 2019-04-23 19:36 UTC (permalink / raw)
To: James Feeney
Cc: Benjamin Tissoires, Jiri Kosina, linux-input, linux-kernel,
stable
In-Reply-To: <37009110-5a11-7ed4-3315-e87b98cc45b7@nurealm.net>
On Tue, Apr 23, 2019 at 11:54:07AM -0600, James Feeney wrote:
>On 4/23/19 11:21 AM, Sasha Levin wrote:
>> Hi,
>>
>> [This is an automated email]
>>
>> This commit has been processed because it contains a "Fixes:" tag,
>> fixing commit: 2dc702c991e3 HID: input: use the Resolution Multiplier for high-resolution scrolling.
>>
>> The bot has tested the following trees: v5.0.9.
>>
>> v5.0.9: Failed to apply! Possible dependencies:
>> 724f54bc0063 ("HID: input: make sure the wheel high resolution multiplier is set")
>>
>>
>> How should we proceed with this patch?
>>
>> --
>> Thanks,
>> Sasha
>>
>
>This patch will only apply *after* the application of the first patch, since they "overlap".
>
>Yes, that is probably not the best circumstance. I think Benjamin wanted to keep the issue in patch 2/2 distinct.
>
>Was the first patch applied before attempting application of the second patch?
My script messed up and listed it as a dependency (even though it's not
upstream) rather than applying it. I'll look into it, sorry for the
noise.
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH][v2] drivers: hid: Add a module description line to the hid_hyperv driver
From: Sasha Levin @ 2019-04-23 19:44 UTC (permalink / raw)
To: Michael Kelley
Cc: Joseph Salisbury, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
jikos@kernel.org, benjamin.tissoires@redhat.com,
linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <DM5PR2101MB09183A22984131883E033DA7D7230@DM5PR2101MB0918.namprd21.prod.outlook.com>
On Tue, Apr 23, 2019 at 04:38:05PM +0000, Michael Kelley wrote:
>From: Joseph Salisbury <joseph.salisbury@microsoft.com> Sent: Monday, April 22, 2019 8:47 PM
>>
>> This patch only adds a MODULE_DESCRIPTION statement to the driver.
>> This change is only cosmetic, so there should be no runtime impact.
>>
>>
>> Signed-off-by: Joseph Salisbury <joseph.salisbury@microsoft.com>
>> ---
>
>Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Queued up for hyperv-next, thanks!
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH][v2] drivers: input: serio: Add a module desription to the hyperv_keyboard driver
From: Sasha Levin @ 2019-04-23 19:45 UTC (permalink / raw)
To: Michael Kelley
Cc: Joseph Salisbury, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
dmitry.torokhov@gmail.com, linux-hyperv@vger.kernel.org,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <DM5PR2101MB0918B570C0BCFF4326428A24D7230@DM5PR2101MB0918.namprd21.prod.outlook.com>
On Tue, Apr 23, 2019 at 04:39:00PM +0000, Michael Kelley wrote:
>From: Joseph Salisbury <joseph.salisbury@microsoft.com> Sent: Monday, April 22, 2019 8:47 PM
>>
>> This patch only adds a MODULE_DESCRIPTION statement to the driver.
>> This change is only cosmetic, so there should be no runtime impact.
>>
>> Signed-off-by: Joseph Salisbury <joseph.salisbury@microsoft.com>
>> ---
>
>Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Queued up for hyperv-next, thanks!
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH v10 00/11] mfd: add support for max77650 PMIC
From: Pavel Machek @ 2019-04-24 8:31 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, linux-kernel, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski
In-Reply-To: <20190423090451.23711-1-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 863 bytes --]
On Tue 2019-04-23 11:04:40, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> This series adds support for max77650 ultra low-power PMIC. It provides
> the core mfd driver and a set of five sub-drivers for the regulator,
> power supply, gpio, leds and input subsystems.
>
> Patches 1-4 add the DT binding documents. Patch 5 documents mfd_add_devices().
> Patches 6-10 add all drivers. Last patch adds a MAINTAINERS entry for this
> device.
>
> The regulator part is already upstream.
I see v10 in my inbox... and acks from all over the place, but patches
not going in. Who takes these? I don't think I need another 10
versions in my inbox...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v10 00/11] mfd: add support for max77650 PMIC
From: Bartosz Golaszewski @ 2019-04-24 8:45 UTC (permalink / raw)
To: Pavel Machek, Lee Jones
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <20190424083127.GB6699@amd>
śr., 24 kwi 2019 o 10:31 Pavel Machek <pavel@denx.de> napisał(a):
>
> On Tue 2019-04-23 11:04:40, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > This series adds support for max77650 ultra low-power PMIC. It provides
> > the core mfd driver and a set of five sub-drivers for the regulator,
> > power supply, gpio, leds and input subsystems.
> >
> > Patches 1-4 add the DT binding documents. Patch 5 documents mfd_add_devices().
> > Patches 6-10 add all drivers. Last patch adds a MAINTAINERS entry for this
> > device.
> >
> > The regulator part is already upstream.
>
> I see v10 in my inbox... and acks from all over the place, but patches
> not going in. Who takes these? I don't think I need another 10
> versions in my inbox...
> Pavel
I think Lee Jones should take them through the MFD tree. With the ack
from Sebastian Reichel on the driver part and the example in the
charger binding fixed I think they are ready to be picked up.
Bart
^ permalink raw reply
* [PATCH 0/5] Add of_ functions for device_link_add()
From: Benjamin Gaignard @ 2019-04-24 10:19 UTC (permalink / raw)
To: rafael.j.wysocki, dmitry.torokhov, robh+dt, mark.rutland, hadess,
frowand.list, m.felsch, agx, yannick.fertre, arnd
Cc: linux-input, devicetree, linux-kernel, linux-stm32, broonie,
Benjamin Gaignard
It could happen that we need to control suspend/resume ordering between
devices without obvious consumer/supplier link. For example when touchscreens
and DSI panels share the same reset line, in this case we need to be sure
of pm_runtime operations ordering between those two devices to correctly
perform reset.
DSI panel and touchscreen aren't sharing any heriachical relationship (unlike
I2C client and I2C bus or regulator client and regulator provider) so we need
to describe this in device-tree.
This series introduce of_device_links_{add,remove} and devm_of_device_links_add()
helpers to find and parse 'links-add' property in a device-tree node.
It allows to create and remove links between consumer and suppliers from
device-tree data so consumers will be suspend before their suppliers and resume
after them.
Benjamin Gaignard (3):
of/device: Add of_ functions for device_link_{add,remove}
Input: edt-ft5x06: Document links-add property
Input: goodix: Document links-add property
Yannick Fertré (2):
input: edt-ft5x06 - Call devm_of_device_links_add() to create links
input: goodix - Call devm_of_device_links_add() to create links
.../bindings/input/touchscreen/edt-ft5x06.txt | 2 +
.../bindings/input/touchscreen/goodix.txt | 2 +
drivers/input/touchscreen/edt-ft5x06.c | 2 +
drivers/input/touchscreen/goodix.c | 3 +
drivers/of/device.c | 103 +++++++++++++++++++++
include/linux/of_device.h | 20 ++++
6 files changed, 132 insertions(+)
--
2.15.0
^ permalink raw reply
* [PATCH 1/5] of/device: Add of_ functions for device_link_{add,remove}
From: Benjamin Gaignard @ 2019-04-24 10:19 UTC (permalink / raw)
To: rafael.j.wysocki, dmitry.torokhov, robh+dt, mark.rutland, hadess,
frowand.list, m.felsch, agx, yannick.fertre, arnd
Cc: linux-input, devicetree, linux-kernel, linux-stm32, broonie,
Benjamin Gaignard
In-Reply-To: <20190424101913.1534-1-benjamin.gaignard@st.com>
Allows to create and remove links between consumer and suppliers from
device-tree data. Use 'links-add' property from consumer node to setup
a link with a list of suppliers.
Consumers will be suspend before their suppliers and resume after them.
Add devm_of_device_links_add() to automatically remove the links
when the device is unbound from the bus.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
drivers/of/device.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/of_device.h | 20 +++++++++
2 files changed, 123 insertions(+)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 3717f2a20d0d..011ba9bf7642 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -336,3 +336,106 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
return 0;
}
EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
+
+/**
+ * of_device_links_add - Create links between consumer and suppliers from
+ * device tree data
+ *
+ * @consumer: consumer device
+ *
+ * Returns 0 on success, < 0 on failure.
+ */
+int of_device_links_add(struct device *consumer)
+{
+ struct device_node *np;
+ struct platform_device *pdev;
+ int i = 0;
+
+ np = of_parse_phandle(consumer->of_node, "links-add", i++);
+ while (np) {
+ pdev = of_find_device_by_node(np);
+ of_node_put(np);
+ if (!pdev)
+ return -EINVAL;
+
+ device_link_add(consumer, &pdev->dev, DL_FLAG_STATELESS);
+ platform_device_put(pdev);
+
+ np = of_parse_phandle(consumer->of_node, "links-add", i++);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_device_links_add);
+
+/**
+ * of_device_links_remove - Remove links between consumer and suppliers from
+ * device tree data
+ *
+ * @consumer: consumer device
+ *
+ * Returns 0 on success, < 0 on failure.
+ */
+int of_device_links_remove(struct device *consumer)
+{
+ struct device_node *np;
+ struct platform_device *pdev;
+ int i = 0;
+
+ np = of_parse_phandle(consumer->of_node, "links-add", i++);
+ while (np) {
+ pdev = of_find_device_by_node(np);
+ of_node_put(np);
+ if (!pdev)
+ return -EINVAL;
+
+ device_link_remove(consumer, &pdev->dev);
+ platform_device_put(pdev);
+
+ np = of_parse_phandle(consumer->of_node, "links-add", i++);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_device_links_remove);
+
+static void devm_of_device_links_remove(struct device *dev, void *res)
+{
+ of_device_links_remove(*(struct device **)res);
+}
+
+/**
+ * devm_of_device_links_add - Create links between consumer and suppliers
+ * from device tree data
+ *
+ * @consumer: consumer device
+ *
+ * Returns 0 on success, < 0 on failure.
+ *
+ * Similar to of_device_links_add(), but will automatically call
+ * of_device_links_remove() when the device is unbound from the bus.
+ */
+int devm_of_device_links_add(struct device *consumer)
+{
+ struct device **ptr;
+ int ret;
+
+ if (!consumer)
+ return -EINVAL;
+
+ ptr = devres_alloc(devm_of_device_links_remove,
+ sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return -ENOMEM;
+
+ ret = of_device_links_add(consumer);
+ if (ret < 0) {
+ devres_free(ptr);
+ } else {
+ *ptr = consumer;
+ devres_add(consumer, ptr);
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(devm_of_device_links_add);
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 8d31e39dd564..ad01db6828e8 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -41,6 +41,11 @@ extern int of_device_request_module(struct device *dev);
extern void of_device_uevent(struct device *dev, struct kobj_uevent_env *env);
extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env);
+
+extern int of_device_links_add(struct device *consumer);
+extern int of_device_links_remove(struct device *consumer);
+extern int devm_of_device_links_add(struct device *consumer);
+
static inline void of_device_node_put(struct device *dev)
{
of_node_put(dev->of_node);
@@ -91,6 +96,21 @@ static inline int of_device_uevent_modalias(struct device *dev,
return -ENODEV;
}
+static int of_device_links_add(struct device *consumer)
+{
+ return 0;
+}
+
+static int of_device_links_remove(struct device *consumer)
+{
+ return 0;
+}
+
+static int devm_of_device_links_add(struct device *consumer)
+{
+ return 0;
+}
+
static inline void of_device_node_put(struct device *dev) { }
static inline const struct of_device_id *__of_match_device(
--
2.15.0
^ permalink raw reply related
* [PATCH 2/5] Input: edt-ft5x06: Document links-add property
From: Benjamin Gaignard @ 2019-04-24 10:19 UTC (permalink / raw)
To: rafael.j.wysocki, dmitry.torokhov, robh+dt, mark.rutland, hadess,
frowand.list, m.felsch, agx, yannick.fertre, arnd
Cc: linux-input, devicetree, linux-kernel, linux-stm32, broonie,
Benjamin Gaignard
In-Reply-To: <20190424101913.1534-1-benjamin.gaignard@st.com>
Explain the purpose of links_add property.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
index 870b8c5cce9b..38b84d2b32e6 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
@@ -60,6 +60,8 @@ Optional properties:
- touchscreen-inverted-x : See touchscreen.txt
- touchscreen-inverted-y : See touchscreen.txt
- touchscreen-swapped-x-y : See touchscreen.txt
+ - links-add : List of suppliers handles, suppliers will be
+ suspended after touchscreen device and resume before it.
Example:
polytouch: edt-ft5x06@38 {
--
2.15.0
^ permalink raw reply related
* [PATCH 3/5] input: edt-ft5x06 - Call devm_of_device_links_add() to create links
From: Benjamin Gaignard @ 2019-04-24 10:19 UTC (permalink / raw)
To: rafael.j.wysocki, dmitry.torokhov, robh+dt, mark.rutland, hadess,
frowand.list, m.felsch, agx, yannick.fertre, arnd
Cc: linux-input, devicetree, linux-kernel, linux-stm32, broonie,
Benjamin Gaignard
In-Reply-To: <20190424101913.1534-1-benjamin.gaignard@st.com>
From: Yannick Fertré <yannick.fertre@st.com>
Add a call to devm_of_device_links_add() to create links with suppliers
at probe time.
Signed-off-by: Yannick Fertré <yannick.fertre@st.com>
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
drivers/input/touchscreen/edt-ft5x06.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 702bfda7ee77..ac9f7e85efb0 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1167,6 +1167,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
i2c_set_clientdata(client, tsdata);
+ devm_of_device_links_add(&client->dev);
+
irq_flags = irq_get_trigger_type(client->irq);
if (irq_flags == IRQF_TRIGGER_NONE)
irq_flags = IRQF_TRIGGER_FALLING;
--
2.15.0
^ permalink raw reply related
* [PATCH 4/5] Input: goodix: Document links-add property
From: Benjamin Gaignard @ 2019-04-24 10:19 UTC (permalink / raw)
To: rafael.j.wysocki, dmitry.torokhov, robh+dt, mark.rutland, hadess,
frowand.list, m.felsch, agx, yannick.fertre, arnd
Cc: linux-input, devicetree, linux-kernel, linux-stm32, broonie,
Benjamin Gaignard
In-Reply-To: <20190424101913.1534-1-benjamin.gaignard@st.com>
Explain the purpose of links_add property.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index 8cf0b4d38a7e..0447151608c6 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -24,6 +24,8 @@ Optional properties:
- touchscreen-size-x
- touchscreen-size-y
- touchscreen-swapped-x-y
+ - links-add : List of suppliers handles, suppliers will be
+ suspended after goodix device and resume before it.
The touchscreen-* properties are documented in touchscreen.txt in this
directory.
--
2.15.0
^ permalink raw reply related
* [PATCH 5/5] input: goodix - Call devm_of_device_links_add() to create links
From: Benjamin Gaignard @ 2019-04-24 10:19 UTC (permalink / raw)
To: rafael.j.wysocki, dmitry.torokhov, robh+dt, mark.rutland, hadess,
frowand.list, m.felsch, agx, yannick.fertre, arnd
Cc: linux-input, devicetree, linux-kernel, linux-stm32, broonie,
Benjamin Gaignard
In-Reply-To: <20190424101913.1534-1-benjamin.gaignard@st.com>
From: Yannick Fertré <yannick.fertre@st.com>
Add a call to devm_of_device_links_add() to create links with suppliers
at probe time.
Signed-off-by: Yannick Fertré <yannick.fertre@st.com>
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
drivers/input/touchscreen/goodix.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f57d82220a88..9aefbfa39319 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -30,6 +30,7 @@
#include <linux/slab.h>
#include <linux/acpi.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <asm/unaligned.h>
struct goodix_ts_data;
@@ -812,6 +813,8 @@ static int goodix_ts_probe(struct i2c_client *client,
ts->chip = goodix_get_chip_data(ts->id);
+ devm_of_device_links_add(&client->dev);
+
if (ts->gpiod_int && ts->gpiod_rst) {
/* update device config */
ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL,
--
2.15.0
^ permalink raw reply related
* Re: [PATCH 1/3] mfd: apple-ibridge: Add Apple iBridge MFD driver.
From: Life is hard, and then you die @ 2019-04-24 10:47 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jiri Kosina, Benjamin Tissoires, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Lee Jones,
linux-input, linux-iio, linux-kernel
In-Reply-To: <20190422123426.2d0b4bdf@archlinux>
Hi Jonathan,
On Mon, Apr 22, 2019 at 12:34:26PM +0100, Jonathan Cameron wrote:
> On Sun, 21 Apr 2019 20:12:49 -0700
> Ronald Tschalär <ronald@innovation.ch> wrote:
>
> > The iBridge device provides access to several devices, including:
> > - the Touch Bar
> > - the iSight webcam
> > - the light sensor
> > - the fingerprint sensor
> >
> > This driver provides the core support for managing the iBridge device
> > and the access to the underlying devices. In particular, since the
> > functionality for the touch bar and light sensor is exposed via USB HID
> > interfaces, and the same HID device is used for multiple functions, this
> > driver provides a multiplexing layer that allows multiple HID drivers to
> > be registered for a given HID device. This allows the touch bar and ALS
> > driver to be separated out into their own modules.
> >
> > Signed-off-by: Ronald Tschalär <ronald@innovation.ch
> Hi Ronald,
>
> I've only taken a fairly superficial look at this. A few global
> things to note though.
Thanks for this review.
> 1. Please either use kernel-doc style for function descriptions, or
> do not. Right now you are sort of half way there.
Apologies, on re-reading the docs I realize what you mean here. Should
be fixed now (next rev).
> 2. There is quite a complex nest of separate structures being allocated,
> so think about whether they can be simplified. In particular
> use of container_of macros can allow a lot of forwards and backwards
> pointers to be dropped if you embed the various structures directly.
Done (see also below).
[snip]
> > +#define call_void_driver_func(drv_info, fn, ...) \
>
> This sort of macro may seem like a good idea because it saves a few lines
> of code. However, that comes at the cost of readability, so just
> put the code inline.
>
> > + do { \
> > + if ((drv_info)->driver->fn) \
> > + (drv_info)->driver->fn(__VA_ARGS__); \
> > + } while (0)
> > +
> > +#define call_driver_func(drv_info, fn, ret_type, ...) \
> > + ({ \
> > + ret_type rc = 0; \
> > + \
> > + if ((drv_info)->driver->fn) \
> > + rc = (drv_info)->driver->fn(__VA_ARGS__); \
> > + \
> > + rc; \
> > + })
Just to clarify, you're only talking about removing/inlining the
call_void_driver_func() macro, not the call_driver_func() macro,
right?
[snip]
> > +static struct appleib_hid_dev_info *
> > +appleib_add_device(struct appleib_device *ib_dev, struct hid_device *hdev,
> > + const struct hid_device_id *id)
> > +{
> > + struct appleib_hid_dev_info *dev_info;
> > + struct appleib_hid_drv_info *drv_info;
> > +
> > + /* allocate device-info for this device */
> > + dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL);
> > + if (!dev_info)
> > + return NULL;
> > +
> > + INIT_LIST_HEAD(&dev_info->drivers);
> > + dev_info->device = hdev;
> > + dev_info->device_id = id;
> > +
> > + /* notify all our sub drivers */
> > + mutex_lock(&ib_dev->update_lock);
> > +
> This is interesting. I'd like to see a comment here on what
> this flag is going to do.
I'm not sure I follow: update_lock is simply a mutex protecting all
driver and device update (i.e. add/remove) functions. Are you
therefore looking for something like:
/* protect driver and device lists against concurrent updates */
mutex_lock(&ib_dev->update_lock);
[snip]
> > +static int appleib_probe(struct acpi_device *acpi)
> > +{
> > + struct appleib_device *ib_dev;
> > + struct appleib_platform_data *pdata;
> Platform_data has a lot of historical meaning in Linux.
> Also you have things in here that are not platform related
> at all, such as the dev pointer. Hence I would rename it
> as device_data or private or something like that.
Ok. I guess I called in platform_data because it's stored in the mfd
cell's "platform_data" field. Anyway, changed it per your suggestion.
> > + int i;
> > + int ret;
> > +
> > + if (appleib_dev)
> This singleton bothers me a bit. I'm really not sure why it
> is necessary. You can just put a pointer to this in
> the pdata for the subdevs and I think that covers most of your
> usecases. It's generally a bad idea to limit things to one instance
> of a device unless that actually major simplifications.
> I'm not seeing them here.
Yes, this one is quite ugly. appleib_dev is static so that
appleib_hid_probe() can find it. I could not find any other way to
pass the appleib_dev instance to that probe function.
However, on looking at this again, I realized that hid_device_id has
a driver_data field which can be used for this; so if I added the
hid_driver and hid_device_id structs to the appleib_device (instead of
making them static like now) I could fill in the driver_data and avoid
this hack. This looks much cleaner.
Thanks for pointing this uglyness out again.
[snip]
> > + if (!ib_dev->subdevs) {
> > + ret = -ENOMEM;
> > + goto free_dev;
> > + }
> > +
> > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
>
> Might as well embed this in ib_dev as well.
Agreed.
> That would let
> you used container_of to avoid having to carry the ib_dev pointer
> around in side pdata.
I see. I guess my main reservation is that the functions exported to
the sub-drivers would now take a 'struct appleib_device_data *'
argument instead of a 'struct appleib_device *', which just seems a
bit unnatural. E.g.
int appleib_register_hid_driver(struct appleib_device_data *ib_ddata,
struct hid_driver *driver, void *data);
instead of (the current)
int appleib_register_hid_driver(struct appleib_device *ib_dev,
struct hid_driver *driver, void *data);
[snip]
> > + ret = mfd_add_devices(&acpi->dev, PLATFORM_DEVID_NONE,
> > + ib_dev->subdevs, ARRAY_SIZE(appleib_subdevs),
> > + NULL, 0, NULL);
> > + if (ret) {
> > + dev_err(LOG_DEV(ib_dev), "Error adding MFD devices: %d\n", ret);
> > + goto free_pdata;
> > + }
> > +
> > + acpi->driver_data = ib_dev;
> > + appleib_dev = ib_dev;
> > +
> > + ret = hid_register_driver(&appleib_hid_driver);
> > + if (ret) {
> > + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n",
> > + ret);
> > + goto rem_mfd_devs;
> > + }
> > +
> > + return 0;
> > +
> > +rem_mfd_devs:
> > + mfd_remove_devices(&acpi->dev);
> > +free_pdata:
> > + kfree(pdata);
> > +free_subdevs:
> > + kfree(ib_dev->subdevs);
> > +free_dev:
> > + appleib_dev = NULL;
> > + acpi->driver_data = NULL;
> Why at this point? It's not set to anything until much later in the
> probe flow.
If the hid_register_driver() call fails, we get here after driver_data
has been assigned. However, looking at this again, acpi->driver_data
is only used by the remove, suspend, and resume callbacks, and those
will not be called until a successful return from probe; therefore I
can safely move the setting of driver_data to after the
hid_register_driver() call and avoid having to set it to NULL in the
error cleanup.
> May be worth thinking about devm_ managed allocations
> to cleanup some of these allocations automatically and simplify
> the error handling.
Good point, thanks.
[snip]
> > +
> > + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> > + if (ACPI_FAILURE(rc))
> > + dev_warn(LOG_DEV(ib_dev), "SOCW(0) failed: %s\n",
>
> I can sort of see you might want to do the LOG_DEV for consistency
> but here I'm fairly sure it's just dev which might be clearer.
Sorry, you mean rename the macro LOG_DEV() to just DEV()?
Cheers,
Ronald
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox