* [PATCH v4 2/7] dt-bindings: mfd: mt6397: Add bindings for MT6392 PMIC
From: Fabien Parent @ 2019-06-19 14:20 UTC (permalink / raw)
To: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w,
lee.jones-QSEj5FYQhm4dnm+yROfE0A,
lgirdwood-Re5JQEeQqe8AvxtiuMwx3w, broonie-DgEjT+Ai2ygdnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Fabien Parent,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20190619142013.20913-1-fparent-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Add the currently supported bindings for the MT6392 PMIC.
Signed-off-by: Fabien Parent <fparent-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-for-MFD-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
V4:
* No change
V3:
* No change
V2:
* New patch
---
Documentation/devicetree/bindings/mfd/mt6397.txt | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/mt6397.txt b/Documentation/devicetree/bindings/mfd/mt6397.txt
index 0ebd08af777d..aa6d2eb0eb19 100644
--- a/Documentation/devicetree/bindings/mfd/mt6397.txt
+++ b/Documentation/devicetree/bindings/mfd/mt6397.txt
@@ -17,7 +17,10 @@ Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
This document describes the binding for MFD device and its sub module.
Required properties:
-compatible: "mediatek,mt6397" or "mediatek,mt6323"
+compatible: Should be one of:
+ - "mediatek,mt6397"
+ - "mediatek,mt6392"
+ - "mediatek,mt6323"
Optional subnodes:
@@ -28,6 +31,8 @@ Optional subnodes:
Required properties:
- compatible: "mediatek,mt6397-regulator"
see Documentation/devicetree/bindings/regulator/mt6397-regulator.txt
+ - compatible: "mediatek,mt6392-regulator"
+ see Documentation/devicetree/bindings/regulator/mt6392-regulator.txt
- compatible: "mediatek,mt6323-regulator"
see Documentation/devicetree/bindings/regulator/mt6323-regulator.txt
- codec
@@ -43,7 +48,10 @@ Optional subnodes:
- keys
Required properties:
- - compatible: "mediatek,mt6397-keys" or "mediatek,mt6323-keys"
+ - compatible: Should be one of:
+ - "mediatek,mt6397-keys"
+ - "mediatek,mt6392-keys"
+ - "mediatek,mt6323-keys"
see Documentation/devicetree/bindings/input/mtk-pmic-keys.txt
Example:
--
2.20.1
^ permalink raw reply related
* [PATCH v4 1/7] dt-bindings: regulator: add support for MT6392
From: Fabien Parent @ 2019-06-19 14:20 UTC (permalink / raw)
To: robh+dt, mark.rutland, matthias.bgg, lee.jones, lgirdwood,
broonie
Cc: dmitry.torokhov, linux-input, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, Fabien Parent, Rob Herring
In-Reply-To: <20190619142013.20913-1-fparent@baylibre.com>
Add binding documentation of the regulator for MT6392 SoCs.
Signed-off-by: Fabien Parent <fparent@baylibre.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v4:
* No change
v3:
* No change
v2:
* Use 'pmic' as node name for the pmic.
* Use 'regulators' as node name for the regulators
* use dash instead of underscore for regulator's node names.
---
.../bindings/regulator/mt6392-regulator.txt | 220 ++++++++++++++++++
1 file changed, 220 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/mt6392-regulator.txt
diff --git a/Documentation/devicetree/bindings/regulator/mt6392-regulator.txt b/Documentation/devicetree/bindings/regulator/mt6392-regulator.txt
new file mode 100644
index 000000000000..d03c0707fabc
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/mt6392-regulator.txt
@@ -0,0 +1,220 @@
+Mediatek MT6392 Regulator
+
+Required properties:
+- compatible: "mediatek,mt6392-regulator"
+- mt6392regulator: List of regulators provided by this controller. It is named
+ according to its regulator type, buck_<name> and ldo_<name>.
+ The definition for each of these nodes is defined using the standard binding
+ for regulators at Documentation/devicetree/bindings/regulator/regulator.txt.
+
+The valid names for regulators are::
+BUCK:
+ buck_vproc, buck_vsys, buck_vcore
+LDO:
+ ldo_vxo22, ldo_vaud22, ldo_vcama, ldo_vaud28, ldo_vadc18, ldo_vcn35,
+ ldo_vio28. ldo_vusb, ldo_vmc, ldo_vmch, ldo_vemc3v3, ldo_vgp1, ldo_vgp2,
+ ldo_vcn18, ldo_vcamaf, ldo_vm, ldo_vio18, ldo_vcamd, ldo_vcamio, ldo_vm25,
+ ldo_vefuse
+
+Example:
+ pmic {
+ compatible = "mediatek,mt6392", "mediatek,mt6323";
+ mediatek,system-power-controller;
+
+ regulator {
+ compatible = "mediatek,mt6392-regulator";
+
+ mt6392_vproc_reg: buck-vproc {
+ regulator-name = "buck_vproc";
+ regulator-min-microvolt = < 700000>;
+ regulator-max-microvolt = <1350000>;
+ regulator-ramp-delay = <12500>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ mt6392_vsys_reg: buck-vsys {
+ regulator-name = "buck_vsys";
+ regulator-min-microvolt = <1400000>;
+ regulator-max-microvolt = <2987500>;
+ regulator-ramp-delay = <25000>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ mt6392_vcore_reg: buck-vcore {
+ regulator-name = "buck_vcore";
+ regulator-min-microvolt = < 700000>;
+ regulator-max-microvolt = <1350000>;
+ regulator-ramp-delay = <12500>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ mt6392_vxo22_reg: ldo-vxo22 {
+ regulator-name = "ldo_vxo22";
+ regulator-min-microvolt = <2200000>;
+ regulator-max-microvolt = <2200000>;
+ regulator-enable-ramp-delay = <110>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ mt6392_vaud22_reg: ldo-vaud22 {
+ regulator-name = "ldo_vaud22";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <2200000>;
+ regulator-enable-ramp-delay = <264>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ mt6392_vcama_reg: ldo-vcama {
+ regulator-name = "ldo_vcama";
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <2800000>;
+ regulator-enable-ramp-delay = <264>;
+ };
+
+ mt6392_vaud28_reg: ldo-vaud28 {
+ regulator-name = "ldo_vaud28";
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <2800000>;
+ regulator-enable-ramp-delay = <264>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ mt6392_vadc18_reg: ldo-vadc18 {
+ regulator-name = "ldo_vadc18";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-enable-ramp-delay = <264>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ mt6392_vcn35_reg: ldo-vcn35 {
+ regulator-name = "ldo_vcn35";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3600000>;
+ regulator-enable-ramp-delay = <264>;
+ };
+
+ mt6392_vio28_reg: ldo-vio28 {
+ regulator-name = "ldo_vio28";
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <2800000>;
+ regulator-enable-ramp-delay = <264>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ mt6392_vusb_reg: ldo-vusb {
+ regulator-name = "ldo_vusb";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-enable-ramp-delay = <264>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ mt6392_vmc_reg: ldo-vmc {
+ regulator-name = "ldo_vmc";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-enable-ramp-delay = <264>;
+ regulator-boot-on;
+ };
+
+ mt6392_vmch_reg: ldo-vmch {
+ regulator-name = "ldo_vmch";
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-enable-ramp-delay = <264>;
+ regulator-boot-on;
+ };
+
+ mt6392_vemc3v3_reg: ldo-vemc3v3 {
+ regulator-name = "ldo_vemc3v3";
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-enable-ramp-delay = <264>;
+ regulator-boot-on;
+ };
+
+ mt6392_vgp1_reg: ldo-vgp1 {
+ regulator-name = "ldo_vgp1";
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-enable-ramp-delay = <264>;
+ };
+
+ mt6392_vgp2_reg: ldo-vgp2 {
+ regulator-name = "ldo_vgp2";
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-enable-ramp-delay = <264>;
+ };
+
+ mt6392_vcn18_reg: ldo-vcn18 {
+ regulator-name = "ldo_vcn18";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-enable-ramp-delay = <264>;
+ };
+
+ mt6392_vcamaf_reg: ldo-vcamaf {
+ regulator-name = "ldo_vcamaf";
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-enable-ramp-delay = <264>;
+ };
+
+ mt6392_vm_reg: ldo-vm {
+ regulator-name = "ldo_vm";
+ regulator-min-microvolt = <1240000>;
+ regulator-max-microvolt = <1390000>;
+ regulator-enable-ramp-delay = <264>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ mt6392_vio18_reg: ldo-vio18 {
+ regulator-name = "ldo_vio18";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-enable-ramp-delay = <264>;
+ regulator-always-on;
+ regulator-boot-on;
+ };
+
+ mt6392_vcamd_reg: ldo-vcamd {
+ regulator-name = "ldo_vcamd";
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-enable-ramp-delay = <264>;
+ };
+
+ mt6392_vcamio_reg: ldo-vcamio {
+ regulator-name = "ldo_vcamio";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-enable-ramp-delay = <264>;
+ };
+
+ mt6392_vm25_reg: ldo-vm25 {
+ regulator-name = "ldo_vm25";
+ regulator-min-microvolt = <2500000>;
+ regulator-max-microvolt = <2500000>;
+ regulator-enable-ramp-delay = <264>;
+ };
+
+ mt6392_vefuse_reg: ldo-vefuse {
+ regulator-name = "ldo_vefuse";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <2000000>;
+ regulator-enable-ramp-delay = <264>;
+ };
+ };
+ };
--
2.20.1
^ permalink raw reply related
* [PATCH v4 0/7] mt6392: Add support for MediaTek MT6392 PMIC
From: Fabien Parent @ 2019-06-19 14:20 UTC (permalink / raw)
To: robh+dt, mark.rutland, matthias.bgg, lee.jones, lgirdwood,
broonie
Cc: dmitry.torokhov, linux-input, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, Fabien Parent
This patch series aims at bringing support for the MediaTek MT6392 PMIC. This
PMIC is used on the MT8516 Pumpkin board.
This patch series adds support for the following features:
* PMIC keys
* regulator
* RTC
Fabien Parent (7):
dt-bindings: regulator: add support for MT6392
dt-bindings: mfd: mt6397: Add bindings for MT6392 PMIC
dt-bindings: input: mtk-pmic-keys: add MT6392 binding definition
mfd: mt6397: Add support for MT6392 pmic
regulator: mt6392: Add support for MT6392 regulator
input: keyboard: mtk-pmic-keys: add MT6392 support
arm64: dts: mt6392: Add PMIC mt6392 dtsi
.../bindings/input/mtk-pmic-keys.txt | 11 +-
.../devicetree/bindings/mfd/mt6397.txt | 12 +-
.../bindings/regulator/mt6392-regulator.txt | 220 ++++++++
arch/arm64/boot/dts/mediatek/mt6392.dtsi | 208 ++++++++
drivers/input/keyboard/mtk-pmic-keys.c | 14 +
drivers/mfd/mt6397-core.c | 47 ++
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/mt6392-regulator.c | 490 ++++++++++++++++++
include/linux/mfd/mt6392/core.h | 42 ++
include/linux/mfd/mt6392/registers.h | 487 +++++++++++++++++
include/linux/regulator/mt6392-regulator.h | 40 ++
12 files changed, 1575 insertions(+), 6 deletions(-)
create mode 100644 Documentation/devicetree/bindings/regulator/mt6392-regulator.txt
create mode 100644 arch/arm64/boot/dts/mediatek/mt6392.dtsi
create mode 100644 drivers/regulator/mt6392-regulator.c
create mode 100644 include/linux/mfd/mt6392/core.h
create mode 100644 include/linux/mfd/mt6392/registers.h
create mode 100644 include/linux/regulator/mt6392-regulator.h
--
2.20.1
^ permalink raw reply
* input: Device Tree Properties for Captouch Button Device Registers
From: Ken Sloat @ 2019-06-19 12:55 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com
Cc: Kasun Beddewela, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Ken Sloat
Hello Dmitry,
We have a new input driver we are currently working on and would like to submit
to the Linux kernel when we finish it. Specifically, this is a cap touch IC which
implements potentially multiple individual proximity and cap touch buttons (which
would be reported like key events as seems to be the standard). A couple of questions:
1. What is the preferred/proper method to expose the many registers that
these devices have via device tree?
These devices have dozens of registers, many of which might be needed depending
on the individual application. It wouldn't be useful in the majority of cases to provide
default values in the driver as the registers are custom tuned to the individual application.
2. Where should this device live? I am guessing in input/misc?
Thanks,
Ken Sloat
^ permalink raw reply
* Re: [PATCH] PM: suspend: Rename pm_suspend_via_s2idle()
From: Rafael J. Wysocki @ 2019-06-19 8:29 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Rafael J. Wysocki, Linux PM, LKML, linux-input
In-Reply-To: <20190619001905.GA62571@dtor-ws>
On Wed, Jun 19, 2019 at 2:19 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Rafael,
>
> On Tue, Jun 18, 2019 at 10:18:28AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The name of pm_suspend_via_s2idle() is confusing, as it doesn't
> > reflect the purpose of the function precisely enough and it is
> > very similar to pm_suspend_via_firmware(), which has a different
> > purpose, so rename it as pm_suspend_default_s2idle() and update
> > its only caller, i8042_register_ports(), accordingly.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> I assume you'll take it through your tree...
I will.
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks!
^ permalink raw reply
* Re: [PATCH resend] Input: iforce - Add the Saitek R440 Force Wheel
From: Dmitry Torokhov @ 2019-06-19 0:24 UTC (permalink / raw)
To: Tim Schumacher; +Cc: linux-input, linux-kernel
In-Reply-To: <20190612140903.20058-1-timschumi@gmx.de>
On Wed, Jun 12, 2019 at 04:09:03PM +0200, Tim Schumacher wrote:
> Signed-off-by: Tim Schumacher <timschumi@gmx.de>
Applied, thank you.
> ---
> Please note that I do NOT own this device.
>
> I'm adding this based on the fact that this is an iforce-based
> device and that the Windows driver for the R440 works for the
> Logitech WingMan Formula Force after replacing the device/vendor
> IDs (I got the vendor/device IDs from there as well).
>
> Please don't add this patch if adding devices based on that is
> not ok.
>
> This patch is a resend of the patch I sent back in November,
> which apparently went unnoticed.
> ---
> drivers/input/joystick/iforce/iforce-main.c | 1 +
> drivers/input/joystick/iforce/iforce-usb.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/input/joystick/iforce/iforce-main.c b/drivers/input/joystick/iforce/iforce-main.c
> index 55f5b7bb4cac..e000e7d5b4c1 100644
> --- a/drivers/input/joystick/iforce/iforce-main.c
> +++ b/drivers/input/joystick/iforce/iforce-main.c
> @@ -55,6 +55,7 @@ static struct iforce_device iforce_device[] = {
> { 0x05ef, 0x8888, "AVB Top Shot Force Feedback Racing Wheel", btn_wheel, abs_wheel, ff_iforce }, //?
> { 0x061c, 0xc0a4, "ACT LABS Force RS", btn_wheel, abs_wheel, ff_iforce }, //?
> { 0x061c, 0xc084, "ACT LABS Force RS", btn_wheel, abs_wheel, ff_iforce },
> + { 0x06a3, 0xff04, "Saitek R440 Force Wheel", btn_wheel, abs_wheel, ff_iforce }, //?
> { 0x06f8, 0x0001, "Guillemot Race Leader Force Feedback", btn_wheel, abs_wheel, ff_iforce }, //?
> { 0x06f8, 0x0001, "Guillemot Jet Leader Force Feedback", btn_joystick, abs_joystick_rudder, ff_iforce },
> { 0x06f8, 0x0004, "Guillemot Force Feedback Racing Wheel", btn_wheel, abs_wheel, ff_iforce }, //?
> diff --git a/drivers/input/joystick/iforce/iforce-usb.c b/drivers/input/joystick/iforce/iforce-usb.c
> index f1569ae8381b..afbcd1a522d4 100644
> --- a/drivers/input/joystick/iforce/iforce-usb.c
> +++ b/drivers/input/joystick/iforce/iforce-usb.c
> @@ -202,6 +202,7 @@ static const struct usb_device_id iforce_usb_ids[] = {
> { USB_DEVICE(0x05ef, 0x8888) }, /* AVB Top Shot FFB Racing Wheel */
> { USB_DEVICE(0x061c, 0xc0a4) }, /* ACT LABS Force RS */
> { USB_DEVICE(0x061c, 0xc084) }, /* ACT LABS Force RS */
> + { USB_DEVICE(0x06a3, 0xff04) }, /* Saitek R440 Force Wheel */
> { USB_DEVICE(0x06f8, 0x0001) }, /* Guillemot Race Leader Force Feedback */
> { USB_DEVICE(0x06f8, 0x0003) }, /* Guillemot Jet Leader Force Feedback */
> { USB_DEVICE(0x06f8, 0x0004) }, /* Guillemot Force Feedback Racing Wheel */
> --
> 2.22.0
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 01/20] Input: iforce - remove "being used" silliness
From: Dmitry Torokhov @ 2019-06-19 0:24 UTC (permalink / raw)
To: Tim Schumacher; +Cc: linux-input, linux-kernel
In-Reply-To: <3505434a-f5ec-de50-5f94-7347c6926f40@gmx.de>
On Wed, Jun 12, 2019 at 04:44:00PM +0200, Tim Schumacher wrote:
> On 19.09.18 19:10, Dmitry Torokhov wrote:
> > On Wed, Sep 19, 2018 at 04:51:26PM +0200, Tim Schumacher wrote:
> >> On 18.09.18 02:47, Dmitry Torokhov wrote:
> >>> The kernel is supposed to handle multiple devices, static flags
> >>> in packet handling code will never work.
> >>>
> >>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>> ---
> >>>
> >>> This is a random assortment of iforce patches that I made a few weeks back.
> >>>
> >>> Tim, I do not have hardware, so I was bound to screw it up, but if you
> >>> have some time I'd appreciate if you try them out (and if I indeed broke
> >>> things if you could identify issues that would be even more awesome).
> >>>
> >>> Thanks!
> >>>
> >> Hello Dmitry,
> >>
> >> I tested those patches and I didn't find any obvious issues. The basic functions
> >> do work (i.e. buttons and axes, don't have a HAT so I can't test that), force
> >> feedback seems to work to the extent it was before (I only have fftest though,
> >> no games that support force feedback). I'll go through a few more applications
> >> and see if anything not obvious is broken.
> >>
> >
> > Thank you for taking a look.
> >
> >> Unfortunately, I only have that one wheel and I can only test USB connections
> >> at the moment (unless I find a proper serial adaptor, but I'm not sure if that
> >> would even work).
> >>
> >> Are those patches planned to go into 4.19 or are they intended to be merged in
> >> the next development cycle?
> >
> > Definitely not 4.19. Could be 4.20 or 4.21...
> >
> > Thanks.
> >
>
> Hello Dmitry,
>
> as a followup to the last E-Mail I sent about this topic, the complete chain
> of patches has been in my personal tree since September of 2018 and there
> have been no issues so far as well. I tested a few more games and utilities,
> and I was able to test with multiple wheels simultaneously as well.
>
> In the end it's still your decision, but imo those patches should be fine
Thank you Tim, I'll put your name down as Tested-by then.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: gpio_keys - use struct_size() in devm_kzalloc()
From: Dmitry Torokhov @ 2019-06-19 0:20 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: linux-input, linux-kernel
In-Reply-To: <20190618152325.GA21504@embeddedor>
On Tue, Jun 18, 2019 at 10:23:25AM -0500, Gustavo A. R. Silva wrote:
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
>
> struct gpio_keys_drvdata {
> ...
> struct gpio_button_data data[0];
> };
>
>
> size = sizeof(struct gpio_keys_drvdata) + count * sizeof(struct gpio_button_data);
> instance = devm_kzalloc(dev, size, GFP_KERNEL);
>
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
>
> instance = devm_kzalloc(dev, struct_size(instance, data, count), GFP_KERNEL);
>
> Notice that, in this case, variable size is not necessary, hence it
> is removed.
>
> This code was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Applied, thank you.
> ---
> drivers/input/keyboard/gpio_keys.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 6cd199e8a370..c186c2552b04 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -774,7 +774,6 @@ static int gpio_keys_probe(struct platform_device *pdev)
> struct fwnode_handle *child = NULL;
> struct gpio_keys_drvdata *ddata;
> struct input_dev *input;
> - size_t size;
> int i, error;
> int wakeup = 0;
>
> @@ -784,9 +783,8 @@ static int gpio_keys_probe(struct platform_device *pdev)
> return PTR_ERR(pdata);
> }
>
> - size = sizeof(struct gpio_keys_drvdata) +
> - pdata->nbuttons * sizeof(struct gpio_button_data);
> - ddata = devm_kzalloc(dev, size, GFP_KERNEL);
> + ddata = devm_kzalloc(dev, struct_size(ddata, data, pdata->nbuttons),
> + GFP_KERNEL);
> if (!ddata) {
> dev_err(dev, "failed to allocate state\n");
> return -ENOMEM;
> --
> 2.21.0
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH] PM: suspend: Rename pm_suspend_via_s2idle()
From: Dmitry Torokhov @ 2019-06-19 0:19 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux PM, LKML, linux-input
In-Reply-To: <7812857.KkDK7346ep@kreacher>
Hi Rafael,
On Tue, Jun 18, 2019 at 10:18:28AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The name of pm_suspend_via_s2idle() is confusing, as it doesn't
> reflect the purpose of the function precisely enough and it is
> very similar to pm_suspend_via_firmware(), which has a different
> purpose, so rename it as pm_suspend_default_s2idle() and update
> its only caller, i8042_register_ports(), accordingly.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
I assume you'll take it through your tree...
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
--
Dmitry
^ permalink raw reply
* [git pull] Input updates for v5.2-rc5
From: Dmitry Torokhov @ 2019-06-19 0:16 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-input
Hi Linus,
Please pull from:
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
to receive updates for the input subsystem. Just a few small fixups;
switching a couple of Thinkpads to SMBus for touchpads as PS/2 emulation
is not working well.
Changelog:
---------
Aaron Ma (1):
Input: elantech - enable middle button support on 2 ThinkPads
Alexander Mikhaylenko (1):
Input: synaptics - enable SMBus on ThinkPad E480 and E580
Andrey Smirnov (1):
Input: uinput - add compat ioctl number translation for UI_*_FF_UPLOAD
Anson Huang (1):
Input: imx_keypad - make sure keyboard can always wake up system
Daniel Smith (1):
Input: silead - add MSSL0017 to acpi_device_id
Jeff LaBundy (1):
Input: iqs5xx - get axis info before calling input_mt_init_slots()
Ravi Chandra Sadineni (1):
Input: elan_i2c - increment wakeup count if wake source
Diffstat:
--------
drivers/input/keyboard/imx_keypad.c | 18 ++++++++++++++----
drivers/input/misc/uinput.c | 22 ++++++++++++++++++++--
drivers/input/mouse/elan_i2c_core.c | 2 ++
drivers/input/mouse/elantech.c | 2 ++
drivers/input/mouse/synaptics.c | 2 ++
drivers/input/touchscreen/iqs5xx.c | 24 +++++++++++++-----------
drivers/input/touchscreen/silead.c | 1 +
7 files changed, 54 insertions(+), 17 deletions(-)
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] Input: gpio_keys - use struct_size() in devm_kzalloc()
From: Gustavo A. R. Silva @ 2019-06-18 15:23 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Gustavo A. R. Silva
One of the more common cases of allocation size calculations is finding
the size of a structure that has a zero-sized array at the end, along
with memory for some number of elements for that array. For example:
struct gpio_keys_drvdata {
...
struct gpio_button_data data[0];
};
size = sizeof(struct gpio_keys_drvdata) + count * sizeof(struct gpio_button_data);
instance = devm_kzalloc(dev, size, GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can
now use the new struct_size() helper:
instance = devm_kzalloc(dev, struct_size(instance, data, count), GFP_KERNEL);
Notice that, in this case, variable size is not necessary, hence it
is removed.
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/input/keyboard/gpio_keys.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 6cd199e8a370..c186c2552b04 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -774,7 +774,6 @@ static int gpio_keys_probe(struct platform_device *pdev)
struct fwnode_handle *child = NULL;
struct gpio_keys_drvdata *ddata;
struct input_dev *input;
- size_t size;
int i, error;
int wakeup = 0;
@@ -784,9 +783,8 @@ static int gpio_keys_probe(struct platform_device *pdev)
return PTR_ERR(pdata);
}
- size = sizeof(struct gpio_keys_drvdata) +
- pdata->nbuttons * sizeof(struct gpio_button_data);
- ddata = devm_kzalloc(dev, size, GFP_KERNEL);
+ ddata = devm_kzalloc(dev, struct_size(ddata, data, pdata->nbuttons),
+ GFP_KERNEL);
if (!ddata) {
dev_err(dev, "failed to allocate state\n");
return -ENOMEM;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH] Input: cros_ec_keyb: mask out extra flags in event_type
From: Enric Balletbo i Serra @ 2019-06-18 8:43 UTC (permalink / raw)
To: Ting Shen, LKML
Cc: Nicolas Boichat, Pi-Hsun Shih, Enrico Granata, Heiko Stuebner,
Guenter Roeck, Brian Norris, Benson Leung, Dmitry Torokhov,
linux-input, Colin Ian King
In-Reply-To: <20190614065438.142867-1-phoenixshen@chromium.org>
On 14/6/19 8:54, Ting Shen wrote:
> http://crosreview.com/1341159 added a EC_MKBP_HAS_MORE_EVENTS flag to
> the event_type field, the receiver side should mask out this extra bit when
> processing the event.
>
> Signed-off-by: Ting Shen <phoenixshen@chromium.org>
>
> ---
>
> drivers/input/keyboard/cros_ec_keyb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index d5600118159835..38cb6d82d8fe67 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -237,7 +237,7 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
> if (queued_during_suspend && !device_may_wakeup(ckdev->dev))
> return NOTIFY_OK;
>
> - switch (ckdev->ec->event_data.event_type) {
> + switch (ckdev->ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK) {
> case EC_MKBP_EVENT_KEY_MATRIX:
> pm_wakeup_event(ckdev->dev, 0);
>
>
Applied for chrome-platform-5.3
Thanks,
~ Enric
^ permalink raw reply
* Re: 答复: 答复: 答复: 答复: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.
From: Hui Wang @ 2019-06-18 8:19 UTC (permalink / raw)
To: Pali Rohár, Xiaoxiao Liu
Cc: dmitry.torokhov@gmail.com, XiaoXiao Liu, peter.hutterer@who-t.net,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
Xiaojian Cao, zhangfp1@lenovo.com, Naoki Saito, Hideo Kawase
In-Reply-To: <20190617074902.bg2emodbmjkkfldd@pali>
On 2019/6/17 下午3:49, Pali Rohár wrote:
> On Monday 17 June 2019 01:29:17 Xiaoxiao Liu wrote:
>> Hi Pali,
>>
>> Since design architecture change of CS19, input device connection has been changed to below architecture,
>> Touchpad has been moved to I2C connection.
>>
>> kernel/host <--PS/2--> EC <--PS/2--> external PS/2 mouse
>> | |
>> | <--PS/2--> trackstick
>> |
>> <--I2C--> Touchpad
> Hi, thank you for explanation!
>
> So in our case, ALPS device should not be put into passthrough mode as
> there is no device after it.
>
>> In the past TrackPoint does not show in the device list because of TrackPoint was hidden device of Touchpad.
>> But from CS19, TrackPoint is directly connecting with PS2 port,
>> 3 bytes packet does not need to take affect by other vendors Touchpad format.
>> So alps.c is no need for CS19 device.
> So if trackpoint.c driver is working fine with this configuration, it is
> just needed to instruct alps.c to not take this device.
Hello Pali,
When you have time, could you take a look at the patch of v4? It is
implemented according to our discussion.
Thanks,
Hui.
>> Best Regards
>> Shona
>> -----邮件原件-----
>> 发件人: Pali Rohár <pali.rohar@gmail.com>
>> 发送时间: Wednesday, June 12, 2019 1:39 AM
>> 收件人: dmitry.torokhov@gmail.com
>> 抄送: Hui Wang <hui.wang@canonical.com>; 劉 曉曉 Xiaoxiao Liu <xiaoxiao.liu-1@cn.alps.com>; XiaoXiao Liu <sliuuxiaonxiao@gmail.com>; peter.hutterer@who-t.net; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; 曹 曉建 Xiaojian Cao <xiaojian.cao@cn.alps.com>; zhangfp1@lenovo.com; 斉藤 直樹 Naoki Saito <naoki.saito@alpsalpine.com>; 川瀬 英夫 Hideo Kawase <hideo.kawase@alpsalpine.com>
>
^ permalink raw reply
* [PATCH] PM: suspend: Rename pm_suspend_via_s2idle()
From: Rafael J. Wysocki @ 2019-06-18 8:18 UTC (permalink / raw)
To: Linux PM; +Cc: LKML, linux-input, Dmitry Torokhov
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
The name of pm_suspend_via_s2idle() is confusing, as it doesn't
reflect the purpose of the function precisely enough and it is
very similar to pm_suspend_via_firmware(), which has a different
purpose, so rename it as pm_suspend_default_s2idle() and update
its only caller, i8042_register_ports(), accordingly.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/input/serio/i8042.c | 2 +-
include/linux/suspend.h | 4 ++--
kernel/power/suspend.c | 6 +++---
3 files changed, 6 insertions(+), 6 deletions(-)
Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -282,7 +282,7 @@ static inline bool idle_should_enter_s2i
return unlikely(s2idle_state == S2IDLE_STATE_ENTER);
}
-extern bool pm_suspend_via_s2idle(void);
+extern bool pm_suspend_default_s2idle(void);
extern void __init pm_states_init(void);
extern void s2idle_set_ops(const struct platform_s2idle_ops *ops);
extern void s2idle_wake(void);
@@ -314,7 +314,7 @@ static inline void pm_set_suspend_via_fi
static inline void pm_set_resume_via_firmware(void) {}
static inline bool pm_suspend_via_firmware(void) { return false; }
static inline bool pm_resume_via_firmware(void) { return false; }
-static inline bool pm_suspend_via_s2idle(void) { return false; }
+static inline bool pm_suspend_default_s2idle(void) { return false; }
static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -62,16 +62,16 @@ enum s2idle_states __read_mostly s2idle_
static DEFINE_RAW_SPINLOCK(s2idle_lock);
/**
- * pm_suspend_via_s2idle - Check if suspend-to-idle is the default suspend.
+ * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend.
*
* Return 'true' if suspend-to-idle has been selected as the default system
* suspend method.
*/
-bool pm_suspend_via_s2idle(void)
+bool pm_suspend_default_s2idle(void)
{
return mem_sleep_current == PM_SUSPEND_TO_IDLE;
}
-EXPORT_SYMBOL_GPL(pm_suspend_via_s2idle);
+EXPORT_SYMBOL_GPL(pm_suspend_default_s2idle);
void s2idle_set_ops(const struct platform_s2idle_ops *ops)
{
Index: linux-pm/drivers/input/serio/i8042.c
===================================================================
--- linux-pm.orig/drivers/input/serio/i8042.c
+++ linux-pm/drivers/input/serio/i8042.c
@@ -1410,7 +1410,7 @@ static void __init i8042_register_ports(
* behavior on many platforms using suspend-to-RAM (ACPI S3)
* by default.
*/
- if (pm_suspend_via_s2idle() && i == I8042_KBD_PORT_NO)
+ if (pm_suspend_default_s2idle() && i == I8042_KBD_PORT_NO)
device_set_wakeup_enable(&serio->dev, true);
}
}
^ permalink raw reply
* Re: [PATCH] platform/x86: touchscreen_dmi: Update Hi10 Air filter
From: Andy Shevchenko @ 2019-06-17 15:01 UTC (permalink / raw)
To: Hans de Goede
Cc: Christian Oder, Darren Hart, Andy Shevchenko, linux-input,
Platform Driver, Linux Kernel Mailing List
In-Reply-To: <2819fd39-ce88-c50f-1fbf-f527d71d63e4@redhat.com>
On Mon, Jun 17, 2019 at 3:52 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 17-06-19 14:38, Andy Shevchenko wrote:
> > On Mon, Jun 17, 2019 at 3:37 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> By some reason patchwork doesn't have a trace of this.
> > I meant, Hans, your message, the patch itself is there.
>
> Weird, no idea why this is happening.
For sake of testing, can you bounce the first message you sent here?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] platform/x86: touchscreen_dmi: Update Hi10 Air filter
From: Hans de Goede @ 2019-06-17 12:52 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Christian Oder, Darren Hart, Andy Shevchenko, linux-input,
Platform Driver, Linux Kernel Mailing List
In-Reply-To: <CAHp75VeSPYakPF-xUcaVOmnMEpv-UZFSrERMSCBi53ov5oA=0g@mail.gmail.com>
Hi,
On 17-06-19 14:38, Andy Shevchenko wrote:
> On Mon, Jun 17, 2019 at 3:37 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>>
>> On Wed, Jun 12, 2019 at 3:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 12-06-19 14:40, Christian Oder wrote:
>>>> Turns out the Hi10 Air is built by multiple companies so using Hampoo
>>>> as a filter is not enough to cover all variants.
>>>>
>>>> This has been verified as working on the Hampoo and Morshow version.
>>>>
>>>> Signed-off-by: Christian Oder <me@myself5.de>
>>>
>>> Patch looks good to me:
>>>
>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>
>
>> By some reason patchwork doesn't have a trace of this.
>
> I meant, Hans, your message, the patch itself is there.
Weird, no idea why this is happening.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH] platform/x86: touchscreen_dmi: Update Hi10 Air filter
From: Andy Shevchenko @ 2019-06-17 12:38 UTC (permalink / raw)
To: Hans de Goede
Cc: Christian Oder, Darren Hart, Andy Shevchenko, linux-input,
Platform Driver, Linux Kernel Mailing List
In-Reply-To: <CAHp75Vdzf7bMQq2WP59Pux6QXD4GTcPLjryEecAsHJiAEewjcA@mail.gmail.com>
On Mon, Jun 17, 2019 at 3:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jun 12, 2019 at 3:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi,
> >
> > On 12-06-19 14:40, Christian Oder wrote:
> > > Turns out the Hi10 Air is built by multiple companies so using Hampoo
> > > as a filter is not enough to cover all variants.
> > >
> > > This has been verified as working on the Hampoo and Morshow version.
> > >
> > > Signed-off-by: Christian Oder <me@myself5.de>
> >
> > Patch looks good to me:
> >
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>
> By some reason patchwork doesn't have a trace of this.
I meant, Hans, your message, the patch itself is there.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] platform/x86: touchscreen_dmi: Update Hi10 Air filter
From: Andy Shevchenko @ 2019-06-17 12:37 UTC (permalink / raw)
To: Hans de Goede
Cc: Christian Oder, Darren Hart, Andy Shevchenko, linux-input,
Platform Driver, Linux Kernel Mailing List
In-Reply-To: <736848fd-1c45-0bd9-bfd1-747c716bd953@redhat.com>
On Wed, Jun 12, 2019 at 3:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12-06-19 14:40, Christian Oder wrote:
> > Turns out the Hi10 Air is built by multiple companies so using Hampoo
> > as a filter is not enough to cover all variants.
> >
> > This has been verified as working on the Hampoo and Morshow version.
> >
> > Signed-off-by: Christian Oder <me@myself5.de>
>
> Patch looks good to me:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
By some reason patchwork doesn't have a trace of this.
>
> Regards,
>
> Hans
>
>
> > ---
> > drivers/platform/x86/touchscreen_dmi.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
> > index b662cb2d7cd5..61e7c4987d0d 100644
> > --- a/drivers/platform/x86/touchscreen_dmi.c
> > +++ b/drivers/platform/x86/touchscreen_dmi.c
> > @@ -597,7 +597,8 @@ static const struct dmi_system_id touchscreen_dmi_table[] = {
> > /* Chuwi Hi10 Air */
> > .driver_data = (void *)&chuwi_hi10_air_data,
> > .matches = {
> > - DMI_MATCH(DMI_BOARD_VENDOR, "Hampoo"),
> > + DMI_MATCH(DMI_SYS_VENDOR, "CHUWI INNOVATION AND TECHNOLOGY(SHENZHEN)CO.LTD"),
> > + DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
> > DMI_MATCH(DMI_PRODUCT_SKU, "P1W6_C109D_B"),
> > },
> > },
> >
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: 答复: 答复: 答复: 答复: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.
From: Pali Rohár @ 2019-06-17 7:49 UTC (permalink / raw)
To: Xiaoxiao Liu
Cc: dmitry.torokhov@gmail.com, Hui Wang, XiaoXiao Liu,
peter.hutterer@who-t.net, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Xiaojian Cao, zhangfp1@lenovo.com,
Naoki Saito, Hideo Kawase
In-Reply-To: <OSBPR01MB4855BD8471A591BD75BDECA0DAEB0@OSBPR01MB4855.jpnprd01.prod.outlook.com>
On Monday 17 June 2019 01:29:17 Xiaoxiao Liu wrote:
> Hi Pali,
>
> Since design architecture change of CS19, input device connection has been changed to below architecture,
> Touchpad has been moved to I2C connection.
>
> kernel/host <--PS/2--> EC <--PS/2--> external PS/2 mouse
> | |
> | <--PS/2--> trackstick
> |
> <--I2C--> Touchpad
Hi, thank you for explanation!
So in our case, ALPS device should not be put into passthrough mode as
there is no device after it.
> In the past TrackPoint does not show in the device list because of TrackPoint was hidden device of Touchpad.
> But from CS19, TrackPoint is directly connecting with PS2 port,
> 3 bytes packet does not need to take affect by other vendors Touchpad format.
> So alps.c is no need for CS19 device.
So if trackpoint.c driver is working fine with this configuration, it is
just needed to instruct alps.c to not take this device.
> Best Regards
> Shona
> -----邮件原件-----
> 发件人: Pali Rohár <pali.rohar@gmail.com>
> 发送时间: Wednesday, June 12, 2019 1:39 AM
> 收件人: dmitry.torokhov@gmail.com
> 抄送: Hui Wang <hui.wang@canonical.com>; 劉 曉曉 Xiaoxiao Liu <xiaoxiao.liu-1@cn.alps.com>; XiaoXiao Liu <sliuuxiaonxiao@gmail.com>; peter.hutterer@who-t.net; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; 曹 曉建 Xiaojian Cao <xiaojian.cao@cn.alps.com>; zhangfp1@lenovo.com; 斉藤 直樹 Naoki Saito <naoki.saito@alpsalpine.com>; 川瀬 英夫 Hideo Kawase <hideo.kawase@alpsalpine.com>
> 主题: Re: 答复: 答复: 答复: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.
>
> On Tuesday 11 June 2019 10:32:28 dmitry.torokhov@gmail.com wrote:
> > On Tue, Jun 11, 2019 at 07:17:07PM +0200, Pali Rohár wrote:
> > > On Tuesday 11 June 2019 10:07:07 dmitry.torokhov@gmail.com wrote:
> > > > On Tue, Jun 11, 2019 at 09:23:33AM +0200, Pali Rohár wrote:
> > > > > On Tuesday 11 June 2019 12:32:33 Hui Wang wrote:
> > > > > > On 2019/6/11 上午11:23, Hui Wang wrote:
> > > > > > > On 2019/6/11 上午11:05, Xiaoxiao Liu wrote:
> > > > > > > > Hi Pali,
> > > > > > > >
> > > > > > > > I discussed with our FW team about this problem.
> > > > > > > > We think the V8 method means a touchpad feature and does
> > > > > > > > not fit the CS19 trackpoint device.
> > > > > > > > CS19 TrackPoint needn't use any Absolute (Raw) mode and
> > > > > > > > is usually use standard mouse data.
> > > > > > > > CS19 TrackPoint device is a completely different device
> > > > > > > > with DualPoint device of Dell/HP.
> > > > > > > > CS19 TrackPoint device is independent of Touchpad.
> > > > > > > > (Touchpad is connecting by I2C, TrackPoint is directly
> > > > > > > > connecting with PS2 port.) And it has completely another FW.
> > > > > > > >
> > > > > > > > So we think it is better to use the mouse mode for CS19 trackpoint.
> > > > > > >
> > > > > > > Maybe here is some mis-understanding, the mouse mode here
> > > > > > > doesn't mean we use psmouse-base.c for cs19 (bare ps/2
> > > > > > > mouse), we plan to use trackpoint.c to drive this HW, so
> > > > > > > this trackpoint has all features a trackpoint should have.
> > > > > > >
> > > > > > And I sent a patch one month ago to let the the trackpoint.c
> > > > > > to drive this
> > > > > > HW: https://www.spinics.net/lists/linux-input/msg61341.html,
> > > > > > maybe that patch is reference.
> > > > >
> > > > > So instead of creating blacklist, you should check for
> > > > > TP_VARIANT_ALPS in alps.c and disallow its usage.
> > > > >
> > > > > Or maybe better, move trackpoint.c detect code before alsp.c
> > > > > detect code in psmouse-base. And no changes in alps.c are needed.
> > > >
> > > > I'd be very cautions of moving around the protocol detection. It
> > > > is very fragile, so if we can detect trackpoint-only case in
> > > > alps.c and skip on to trackpoint I would prefer it.
> > >
> > > The main problem is that proposed trackpoint-only check in alps.c is
> > > basically what trackpoint.c is doing for checking if device is
> > > trackpoint (via function trackpoint_start_protocol()).
> > >
> > > So I'm not sure now what is the best solution...
> >
> > Unfortunately currently trackpoint is being probed only after we tried
> > Elan, Genius, and Logitech PS2++ protocols, and I am not sure if
> > moving trackpoint around will disturb them or not.
> >
> > I do not think there is much code duplication by pulling limited
> > version of trackpoint detection code into alps.c and then have it bail
> > out when it sees trackpoint-only device so trackpoint.c can handle it properly.
>
> Ok. Seems that it is the best solution.
>
> The last question is, should be use ALPS or Trackpoint protocol? Because it looks like that device can be configured to one or other.
>
> What are pros and cons of these two protocols?
>
> --
> Pali Rohár
> pali.rohar@gmail.com
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* 答复: 答复: 答复: 答复: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.
From: Xiaoxiao Liu @ 2019-06-17 1:29 UTC (permalink / raw)
To: Pali Rohár, dmitry.torokhov@gmail.com
Cc: Hui Wang, XiaoXiao Liu, peter.hutterer@who-t.net,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
Xiaojian Cao, zhangfp1@lenovo.com, Naoki Saito, Hideo Kawase
In-Reply-To: <20190611173856.jjwoagud6doxvpy3@pali>
Hi Pali,
Since design architecture change of CS19, input device connection has been changed to below architecture,
Touchpad has been moved to I2C connection.
kernel/host <--PS/2--> EC <--PS/2--> external PS/2 mouse
| |
| <--PS/2--> trackstick
|
<--I2C--> Touchpad
In the past TrackPoint does not show in the device list because of TrackPoint was hidden device of Touchpad.
But from CS19, TrackPoint is directly connecting with PS2 port,
3 bytes packet does not need to take affect by other vendors Touchpad format.
So alps.c is no need for CS19 device.
Best Regards
Shona
-----邮件原件-----
发件人: Pali Rohár <pali.rohar@gmail.com>
发送时间: Wednesday, June 12, 2019 1:39 AM
收件人: dmitry.torokhov@gmail.com
抄送: Hui Wang <hui.wang@canonical.com>; 劉 曉曉 Xiaoxiao Liu <xiaoxiao.liu-1@cn.alps.com>; XiaoXiao Liu <sliuuxiaonxiao@gmail.com>; peter.hutterer@who-t.net; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; 曹 曉建 Xiaojian Cao <xiaojian.cao@cn.alps.com>; zhangfp1@lenovo.com; 斉藤 直樹 Naoki Saito <naoki.saito@alpsalpine.com>; 川瀬 英夫 Hideo Kawase <hideo.kawase@alpsalpine.com>
主题: Re: 答复: 答复: 答复: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.
On Tuesday 11 June 2019 10:32:28 dmitry.torokhov@gmail.com wrote:
> On Tue, Jun 11, 2019 at 07:17:07PM +0200, Pali Rohár wrote:
> > On Tuesday 11 June 2019 10:07:07 dmitry.torokhov@gmail.com wrote:
> > > On Tue, Jun 11, 2019 at 09:23:33AM +0200, Pali Rohár wrote:
> > > > On Tuesday 11 June 2019 12:32:33 Hui Wang wrote:
> > > > > On 2019/6/11 上午11:23, Hui Wang wrote:
> > > > > > On 2019/6/11 上午11:05, Xiaoxiao Liu wrote:
> > > > > > > Hi Pali,
> > > > > > >
> > > > > > > I discussed with our FW team about this problem.
> > > > > > > We think the V8 method means a touchpad feature and does
> > > > > > > not fit the CS19 trackpoint device.
> > > > > > > CS19 TrackPoint needn't use any Absolute (Raw) mode and
> > > > > > > is usually use standard mouse data.
> > > > > > > CS19 TrackPoint device is a completely different device
> > > > > > > with DualPoint device of Dell/HP.
> > > > > > > CS19 TrackPoint device is independent of Touchpad.
> > > > > > > (Touchpad is connecting by I2C, TrackPoint is directly
> > > > > > > connecting with PS2 port.) And it has completely another FW.
> > > > > > >
> > > > > > > So we think it is better to use the mouse mode for CS19 trackpoint.
> > > > > >
> > > > > > Maybe here is some mis-understanding, the mouse mode here
> > > > > > doesn't mean we use psmouse-base.c for cs19 (bare ps/2
> > > > > > mouse), we plan to use trackpoint.c to drive this HW, so
> > > > > > this trackpoint has all features a trackpoint should have.
> > > > > >
> > > > > And I sent a patch one month ago to let the the trackpoint.c
> > > > > to drive this
> > > > > HW: https://www.spinics.net/lists/linux-input/msg61341.html,
> > > > > maybe that patch is reference.
> > > >
> > > > So instead of creating blacklist, you should check for
> > > > TP_VARIANT_ALPS in alps.c and disallow its usage.
> > > >
> > > > Or maybe better, move trackpoint.c detect code before alsp.c
> > > > detect code in psmouse-base. And no changes in alps.c are needed.
> > >
> > > I'd be very cautions of moving around the protocol detection. It
> > > is very fragile, so if we can detect trackpoint-only case in
> > > alps.c and skip on to trackpoint I would prefer it.
> >
> > The main problem is that proposed trackpoint-only check in alps.c is
> > basically what trackpoint.c is doing for checking if device is
> > trackpoint (via function trackpoint_start_protocol()).
> >
> > So I'm not sure now what is the best solution...
>
> Unfortunately currently trackpoint is being probed only after we tried
> Elan, Genius, and Logitech PS2++ protocols, and I am not sure if
> moving trackpoint around will disturb them or not.
>
> I do not think there is much code duplication by pulling limited
> version of trackpoint detection code into alps.c and then have it bail
> out when it sees trackpoint-only device so trackpoint.c can handle it properly.
Ok. Seems that it is the best solution.
The last question is, should be use ALPS or Trackpoint protocol? Because it looks like that device can be configured to one or other.
What are pros and cons of these two protocols?
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH v3 3/3] iio: Add PAT9125 optical tracker sensor
From: Jonathan Cameron @ 2019-06-16 15:39 UTC (permalink / raw)
To: Alexandre Mergnat
Cc: robh+dt, mark.rutland, linux-kernel, linux-iio,
baylibre-upstreaming, dmitry.torokhov, linux-input
In-Reply-To: <20190610092945.6330-4-amergnat@baylibre.com>
On Mon, 10 Jun 2019 11:29:45 +0200
Alexandre Mergnat <amergnat@baylibre.com> wrote:
> This adds support for PixArt Imaging’s miniature low power optical
> navigation chip using LASER light source enabling digital surface tracking.
>
> Feature and datasheet: [0]
>
> This IIO driver allows to read delta or relative position on X and Y axis:
> - The position relative to where the system started can be taken through
> punctual "read_raw" which will issue a read in the device registers to
> get the delta between last/current read and return the addition of all
> the deltas.
> - The delta can be retrieved using triggered buffer subscription
> (i.e. iio_readdev). The buffer payload is:
> |32 bits delta X|32 bits delta Y|timestamp|.
>
> The possible I2C addresses are 0x73, 0x75 and 0x79.
>
> X and Y axis CPI resolution can be get/set independently through IIO_SCALE.
> The range value is 0-255 which means:
> - 0 to ~1,275 Counts Per Inch on flat surface.
> - 0 to ~630 Counts Per Rev on 1.0mm diameter STS shaft at 1.0mm distance.
> More details on the datasheet.
>
> The "position" directory is added to contain drivers which can provide
> position data.
>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>
> [0]: http://www.pixart.com/products-detail/72/PAT9125EL-TKIT___TKMT
>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
Hi Alexandre,
Getting close but a few more bits and pieces inline.
I'm a little confused on why the buffered reads are giving the delta
values whilst we have fixed up the sysfs reads to give the more useful
position values (and hence not need to know when last read was).
We should be consistent and give positions from the buffered path as well.
Thanks,
Jonathan
> ---
> drivers/iio/Kconfig | 1 +
> drivers/iio/Makefile | 1 +
> drivers/iio/position/Kconfig | 18 ++
> drivers/iio/position/Makefile | 6 +
> drivers/iio/position/pat9125.c | 499 +++++++++++++++++++++++++++++++++
> 5 files changed, 525 insertions(+)
> create mode 100644 drivers/iio/position/Kconfig
> create mode 100644 drivers/iio/position/Makefile
> create mode 100644 drivers/iio/position/pat9125.c
>
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 1d736a4952ab..23d9780640e7 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -85,6 +85,7 @@ source "drivers/iio/light/Kconfig"
> source "drivers/iio/magnetometer/Kconfig"
> source "drivers/iio/multiplexer/Kconfig"
> source "drivers/iio/orientation/Kconfig"
> +source "drivers/iio/position/Kconfig"
> if IIO_TRIGGER
> source "drivers/iio/trigger/Kconfig"
> endif #IIO_TRIGGER
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index bff682ad1cfb..1712011c0f4a 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -31,6 +31,7 @@ obj-y += light/
> obj-y += magnetometer/
> obj-y += multiplexer/
> obj-y += orientation/
> +obj-y += position/
> obj-y += potentiometer/
> obj-y += potentiostat/
> obj-y += pressure/
> diff --git a/drivers/iio/position/Kconfig b/drivers/iio/position/Kconfig
> new file mode 100644
> index 000000000000..1cf28896511c
> --- /dev/null
> +++ b/drivers/iio/position/Kconfig
> @@ -0,0 +1,18 @@
> +#
> +# Optical tracker sensors
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menu "Optical tracker sensors"
> +
> +config PAT9125
> + tristate "Optical tracker PAT9125 I2C driver"
> + depends on I2C
> + select IIO_BUFFER
> + help
> + Say yes here to build support for PAT9125 optical tracker
> + sensors.
> +
> + To compile this driver as a module, say M here: the module will
> + be called pat9125.
> +endmenu
> diff --git a/drivers/iio/position/Makefile b/drivers/iio/position/Makefile
> new file mode 100644
> index 000000000000..cf294917ae2c
> --- /dev/null
> +++ b/drivers/iio/position/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for industrial I/O Optical tracker sensor drivers
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_PAT9125) += pat9125.o
> diff --git a/drivers/iio/position/pat9125.c b/drivers/iio/position/pat9125.c
> new file mode 100644
> index 000000000000..22bf729bec9b
> --- /dev/null
> +++ b/drivers/iio/position/pat9125.c
> @@ -0,0 +1,499 @@
> +// SPDX-License-Identifier: (GPL-2.0)
> +/*
> + * Copyright (C) 2019 BayLibre, SAS
> + * Author: Alexandre Mergnat <amergnat@baylibre.com>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +/* I2C Address function to ID pin*/
> +#define PAT9125_I2C_ADDR_HI 0x73
> +#define PAT9125_I2C_ADDR_LO 0x75
> +#define PAT9125_I2C_ADDR_NC 0x79
> +
> +/* Registers */
> +#define PAT9125_PRD_ID1_REG 0x00
> +#define PAT9125_PRD_ID2_REG 0x01
> +#define PAT9125_MOTION_STATUS_REG 0x02
> +#define PAT9125_DELTA_X_LO_REG 0x03
> +#define PAT9125_DELTA_Y_LO_REG 0x04
> +#define PAT9125_OP_MODE_REG 0x05
> +#define PAT9125_CONFIG_REG 0x06
> +#define PAT9125_WRITE_PROTEC_REG 0x09
> +#define PAT9125_SLEEP1_REG 0x0A
> +#define PAT9125_SLEEP2_REG 0x0B
> +#define PAT9125_RES_X_REG 0x0D
> +#define PAT9125_RES_Y_REG 0x0E
> +#define PAT9125_DELTA_XY_HI_REG 0x12
> +#define PAT9125_SHUTER_REG 0x14
> +#define PAT9125_FRAME_AVG_REG 0x17
> +#define PAT9125_ORIENTATION_REG 0x19
> +
> +/* Bits */
> +#define PAT9125_VALID_MOTION_DATA_BIT BIT(7)
> +#define PAT9125_RESET_BIT BIT(7)
> +
> +/* Registers' values */
> +#define PAT9125_SENSOR_ID_VAL 0x31
> +#define PAT9125_DISABLE_WRITE_PROTECT_VAL 0x5A
> +#define PAT9125_ENABLE_WRITE_PROTECT_VAL 0x00
> +
> +/* Default Value of sampled value size */
> +#define PAT9125_SAMPLED_VAL_BIT_SIZE 12
> +
> +struct pat9125_data {
> + struct regmap *regmap;
> + struct iio_trigger *indio_trig; /* Motion detection */
> + s32 delta_x;
> + s32 delta_y;
> + s32 position_x;
> + s32 position_y;
> + bool sampling;
> +};
> +
> +static const struct iio_chan_spec pat9125_channels[] = {
> + {
> + .type = IIO_DISTANCE,
> + .modified = 1,
> + .channel2 = IIO_MOD_X,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .scan_index = 0,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 32,
> + .storagebits = 32,
> + .endianness = IIO_CPU,
> + },
> + },
> + {
> + .type = IIO_DISTANCE,
> + .modified = 1,
> + .channel2 = IIO_MOD_Y,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .scan_index = 1,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 32,
> + .storagebits = 32,
> + .endianness = IIO_CPU,
> + },
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(2),
> +};
> +
> +/**
> + * pat9125_write_pretected_reg() - Write value in protected register.
> + *
> + * @regmap: Pointer to I2C register map.
> + * @reg_addr: Register address.
> + * @reg_value: Value to be write in register.
> + *
> + * A value of zero will be returned on success, a negative errno will
> + * be returned in error cases.
> + */
> +static int pat9125_write_pretected_reg(struct iio_dev *indio_dev,
> + u8 reg_addr, u8 reg_value)
> +{
> + struct pat9125_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_write(data->regmap,
> + PAT9125_WRITE_PROTEC_REG,
> + PAT9125_DISABLE_WRITE_PROTECT_VAL);
> + if (ret < 0) {
> + dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n",
> + PAT9125_WRITE_PROTEC_REG, ret);
> + return ret;
> + }
> +
> + ret = regmap_write(data->regmap, reg_addr, reg_value);
> + if (ret < 0) {
> + dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n",
> + reg_addr, ret);
> + return ret;
> + }
> +
> + ret = regmap_write(data->regmap,
> + PAT9125_WRITE_PROTEC_REG,
> + PAT9125_ENABLE_WRITE_PROTECT_VAL);
> + if (ret < 0) {
> + dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n",
> + PAT9125_WRITE_PROTEC_REG, ret);
> + return ret;
We'll probably get a patch from some script for this so nice to
cleanup now.
regmap_write always returns 0 for success and negative otherwise.
Hence we can just check if (ret). Having done that we can also drop
the return ret above out of the brackets and save a few lines of code
by getting rid of the return 0 that follows.
Just checkign if (ret) in general is probably a worthwhile cleanup
for these regmap calls.
> + }
> + return 0;
> +}
> +/**
> + * pat9125_read_delta() - Read delta value, update delta & position data.
> + *
> + * @data: Driver's data structure.
> + *
> + * A value of zero will be returned on success, a negative errno will
> + * be returned in error cases.
> + */
> +static int pat9125_read_delta(struct pat9125_data *data)
> +{
> + struct regmap *regmap = data->regmap;
> + int status = 0;
> + int val_x = 0;
> + int val_y = 0;
> + int val_high_nibbles = 0;
> + int ret;
> +
> + ret = regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status);
> + if (ret < 0)
> + return ret;
> +
> + /* Check if motion is detected */
> + if (status & PAT9125_VALID_MOTION_DATA_BIT) {
> + ret = regmap_read(regmap, PAT9125_DELTA_X_LO_REG, &val_x);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_read(regmap, PAT9125_DELTA_Y_LO_REG, &val_y);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_read(regmap, PAT9125_DELTA_XY_HI_REG,
> + &val_high_nibbles);
> + if (ret < 0)
> + return ret;
> +
> + val_x |= (val_high_nibbles << 4) & 0xF00;
> + val_y |= (val_high_nibbles << 8) & 0xF00;
> + val_x = sign_extend32(val_x,
> + PAT9125_SAMPLED_VAL_BIT_SIZE - 1);
> + val_y = sign_extend32(val_y,
> + PAT9125_SAMPLED_VAL_BIT_SIZE - 1);
> + data->position_x += val_x;
> + data->position_y += val_y;
> + data->delta_x = val_x;
> + data->delta_y = val_y;
> + }
> + return 0;
> +}
> +
> +/**
> + * pat9125_read_raw() - Sample and return the value(s)
> + * function to the associated channel info enum.
> + **/
> +static int pat9125_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct pat9125_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = pat9125_read_delta(data);
> + if (ret)
> + return ret;
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + *val = data->position_x;
> + return IIO_VAL_INT;
> + case IIO_MOD_Y:
> + *val = data->position_y;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + ret = regmap_read(data->regmap, PAT9125_RES_X_REG, val);
> + if (ret)
> + return ret;
> + else
> + return IIO_VAL_INT;
> + case IIO_MOD_Y:
> + ret = regmap_read(data->regmap, PAT9125_RES_Y_REG, val);
> + if (ret)
> + return ret;
> + else
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +/**
> + * pat9125_write_raw() - Write the value(s)
> + * function to the associated channel info enum.
> + **/
*/
Tidy these up throughout. If you are going to do
kernel-doc (which is good) you should also document all of the
parameters.
> +static int pat9125_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + ret = pat9125_write_pretected_reg(indio_dev,
> + PAT9125_RES_X_REG, val);
> + return ret;
> + case IIO_MOD_Y:
> + ret = pat9125_write_pretected_reg(indio_dev,
> + PAT9125_RES_Y_REG, val);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static irqreturn_t pat9125_threaded_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct pat9125_data *data = iio_priv(indio_dev);
> + u8 buf[16]; /* Payload: Delta_X (4) | Delta_Y (4) | Timestamp (8) */
> + int ret;
> + s64 timestamp;
> +
> + data->sampling = true;
> + ret = pat9125_read_delta(data);
> + if (ret) {
> + dev_err(indio_dev->dev.parent, "Read delta failed %d\n", ret);
> + return IRQ_NONE;
> + }
> + timestamp = iio_get_time_ns(indio_dev);
> + *((s32 *)&buf[0]) = data->delta_x;
> + *((s32 *)&buf[sizeof(s32)]) = data->delta_y;
Why are we putting the delta values in the buffer rather than the positions?
> + data->delta_x = 0;
> + data->delta_y = 0;
> + iio_push_to_buffers_with_timestamp(indio_dev, buf, timestamp);
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * pat9125_threaded_event_handler() - Threaded motion detection event handler
> + * @irq: The irq being handled.
> + * @private: struct iio_device pointer for the device.
> + */
> +static irqreturn_t pat9125_threaded_event_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct pat9125_data *data = iio_priv(indio_dev);
> +
> + iio_trigger_poll_chained(data->indio_trig);
> + return IRQ_HANDLED;
> +}
> +
> +static int pat9125_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct pat9125_data *data = iio_priv(indio_dev);
> + int ret = 0;
> +
> + ret = iio_triggered_buffer_postenable(indio_dev);
> + if (ret)
> + return ret;
> + /* Release interrupt pin on the device */
> + return pat9125_read_delta(data);
If this has an error, we should be handling the unwind of
iio_triggered_buffer_postenable. That is
iio_triggered_buffer_predisable.
(Alexandru has been cleaning these up recently so I'm more
aware of this than normal!)
> +}
> +
> +static const struct iio_buffer_setup_ops pat9125_buffer_ops = {
> + .postenable = pat9125_buffer_postenable,
> + .predisable = iio_triggered_buffer_predisable,
> +};
> +
One blank line is almost always enough (nitpick of the day ;)
> +
> +static const struct regmap_config pat9125_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +};
> +
> +static const struct iio_info pat9125_info = {
> + .read_raw = pat9125_read_raw,
> + .write_raw = pat9125_write_raw,
> +};
> +
> +/*
> + * To detect if a new value is available, register status is checked. This
> + * method is safer than using a flag on GPIO IRQ to track event while sampling
> + * because falling edge is missed when device trig just after a read reg value
> + * (that happen for fast motions or high CPI setting).
So we have an edge triggered interrupt that doesn't have a 'minimum low'
period? If so then the only safe way to handle it would be as a level
interrupt. Can you do that here?
(I once had the delights of a sensor like this tied to a edge sensitive only
interrupt, but thankfully those are a rare thing these days).
> + *
> + * Note: To avoid infinite loop in "iio_trigger_notify_done" when it si not in
is
> + * buffer mode and kernel warning due to nested IRQ thread,
> + * this function must return 0.
> + */
> +static int pat9125_trig_try_reenable(struct iio_trigger *trig)
> +{
> + struct pat9125_data *data = iio_trigger_get_drvdata(trig);
> + struct regmap *regmap = data->regmap;
> + int status = 0;
> +
> + if (data->sampling) {
> + regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status);
> + if (status & PAT9125_VALID_MOTION_DATA_BIT) {
> + data->sampling = false;
So we only ever do 2 reads? Why can't we be unlucky on timing
twice in a row?
> + iio_trigger_poll_chained(data->indio_trig);
> + return 0;
> + }
> + }
> + data->sampling = false;
> + return 0;
> +}
> +
> +static const struct iio_trigger_ops pat9125_trigger_ops = {
> + .try_reenable = pat9125_trig_try_reenable,
> +};
> +
> +static int pat9125_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct pat9125_data *data;
> + struct iio_dev *indio_dev;
> + int ret, sensor_pid;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev) {
> + dev_err(&client->dev, "IIO device allocation failed\n");
> + return -ENOMEM;
> + }
> +
> + data = iio_priv(indio_dev);
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->name = id->name;
> + indio_dev->channels = pat9125_channels;
> + indio_dev->num_channels = ARRAY_SIZE(pat9125_channels);
> + indio_dev->info = &pat9125_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> + pat9125_threaded_trigger_handler, &pat9125_buffer_ops);
> + if (ret) {
> + dev_err(&client->dev, "unable to setup triggered buffer\n");
> + return ret;
> + }
> +
> + data->indio_trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
> + indio_dev->name, indio_dev->id);
> + if (!data->indio_trig)
> + return -ENOMEM;
> + data->indio_trig->dev.parent = &client->dev;
> + data->indio_trig->ops = &pat9125_trigger_ops;
> + iio_trigger_set_drvdata(data->indio_trig, data);
> + ret = devm_iio_trigger_register(&client->dev, data->indio_trig);
> + if (ret) {
> + dev_err(&client->dev, "unable to register trigger\n");
> + return ret;
> + }
> +
> + data->regmap = devm_regmap_init_i2c(client, &pat9125_regmap_config);
> + if (IS_ERR(data->regmap)) {
> + dev_err(&client->dev, "regmap init failed %ld\n",
> + PTR_ERR(data->regmap));
> + return PTR_ERR(data->regmap);
> + }
> +
> + /* Check device ID */
> + ret = regmap_read(data->regmap, PAT9125_PRD_ID1_REG, &sensor_pid);
> + if (ret < 0) {
> + dev_err(&client->dev, "register 0x%x access failed %d\n",
> + PAT9125_PRD_ID1_REG, ret);
> + return ret;
> + }
> + if (sensor_pid != PAT9125_SENSOR_ID_VAL)
> + return -ENODEV;
> +
> + /* Switch to bank0 (Magic number)*/
> + ret = regmap_write(data->regmap, 0x7F, 0x00);
> + if (ret < 0) {
> + dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n",
> + 0x7F, ret);
> + return ret;
> + }
> +
> + /* Software reset */
> + ret = regmap_write_bits(data->regmap,
> + PAT9125_CONFIG_REG,
> + PAT9125_RESET_BIT,
> + 1);
> + if (ret < 0) {
> + dev_err(&client->dev, "register 0x%x access failed %d\n",
> + PAT9125_CONFIG_REG, ret);
> + return ret;
> + }
> +
> + msleep(20);
> +
> + /* Init GPIO IRQ */
> + if (client->irq) {
> + ret = devm_request_threaded_irq(&client->dev,
> + client->irq,
> + NULL,
> + pat9125_threaded_event_handler,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + "pat9125",
> + indio_dev);
> + if (ret) {
> + dev_err(&client->dev, "GPIO IRQ init failed\n");
> + return ret;
> + }
> + }
> +
> + ret = devm_iio_device_register(&client->dev, indio_dev);
> + if (ret) {
> + dev_err(&client->dev, "IIO device register failed\n");
> + return ret;
> + }
> +
> + i2c_set_clientdata(client, indio_dev);
Why? You have now gotten rid of remove (which is great).
I'm guessing that was using i2c_get_clientdata?
> + return 0;
> +}
> +
> +static const struct i2c_device_id pat9125_id[] = {
> + { "pat9125", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, pat9125_id);
> +
> +static const unsigned short normal_i2c[] = {
> + PAT9125_I2C_ADDR_HI,
> + PAT9125_I2C_ADDR_LO,
> + PAT9125_I2C_ADDR_NC,
> + I2C_CLIENT_END
> +};
> +
> +static struct i2c_driver pat9125_driver = {
> + .driver = {
> + .name = "pat9125",
> + },
> + .probe = pat9125_probe,
> + .address_list = normal_i2c,
> + .id_table = pat9125_id,
> +};
> +
> +module_i2c_driver(pat9125_driver);
> +
> +MODULE_AUTHOR("Alexandre Mergnat <amergnat@baylibre.com>");
> +MODULE_DESCRIPTION("Optical Tracking sensor");
> +MODULE_LICENSE("GPL");
^ permalink raw reply
* Re: [PATCH v2 3/3] iio: light: apple-ib-als: Add driver for ALS on iBridge chip.
From: Jonathan Cameron @ 2019-06-16 14:52 UTC (permalink / raw)
To: Ronald Tschalär
Cc: Jiri Kosina, Benjamin Tissoires, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, linux-input,
linux-iio, linux-kernel
In-Reply-To: <20190612083400.1015-4-ronald@innovation.ch>
On Wed, 12 Jun 2019 01:34:00 -0700
Ronald Tschalär <ronald@innovation.ch> wrote:
> On 2016/2017 MacBook Pro's with a Touch Bar the ALS is attached to,
> and exposed via the iBridge device. This provides the driver for that
> sensor.
>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
Hi Ronald,
One thing that we should perhaps document more clearly in IIO is that
it is acceptable to not have triggers if they don't make any sense.
In this particular case, you have one basically to give a way of saying
to move into a more continuous sampling mode from a polled one (I think).
For that just use the buffer enable callbacks.
It'll be much cleaner without the trigger.
A few other suggestions inline. In particularly I'm not that keen on the
appleals_device having a pointer to the iio device which then has
a pointer back again. I 'think' you can just reorder things a bit and
embed the appleals_device structure in the iio_dev private field directly
and avoid the dance between the different structures.
Thanks,
Jonathan
> ---
> drivers/iio/light/Kconfig | 12 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/apple-ib-als.c | 607 +++++++++++++++++++++++++++++++
> 3 files changed, 620 insertions(+)
> create mode 100644 drivers/iio/light/apple-ib-als.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 5190eacfeb0a..b477aa5d2024 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -64,6 +64,18 @@ config APDS9960
> To compile this driver as a module, choose M here: the
> module will be called apds9960
>
> +config APPLE_IBRIDGE_ALS
> + tristate "Apple iBridge ambient light sensor"
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + depends on HID_APPLE_IBRIDGE
> + help
> + Say Y here to build the driver for the Apple iBridge ALS
> + sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apple-ib-als.
> +
> config BH1750
> tristate "ROHM BH1750 ambient light sensor"
> depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index e40794fbb435..cd6cd5ba6da5 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_AL3320A) += al3320a.o
> obj-$(CONFIG_APDS9300) += apds9300.o
> obj-$(CONFIG_APDS9960) += apds9960.o
> +obj-$(CONFIG_APPLE_IBRIDGE_ALS) += apple-ib-als.o
> obj-$(CONFIG_BH1750) += bh1750.o
> obj-$(CONFIG_BH1780) += bh1780.o
> obj-$(CONFIG_CM32181) += cm32181.o
> diff --git a/drivers/iio/light/apple-ib-als.c b/drivers/iio/light/apple-ib-als.c
> new file mode 100644
> index 000000000000..b84be0076e0f
> --- /dev/null
> +++ b/drivers/iio/light/apple-ib-als.c
> @@ -0,0 +1,607 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple Ambient Light Sensor Driver
> + *
> + * Copyright (c) 2017-2018 Ronald Tschalär
> + */
> +
> +/*
> + * MacBookPro models with an iBridge chip (13,[23] and 14,[23]) have an
> + * ambient light sensor that is exposed via one of the USB interfaces on
> + * the iBridge as a standard HID light sensor. However, we cannot use the
> + * existing hid-sensor-als driver, for two reasons:
> + *
> + * 1. The hid-sensor-als driver is part of the hid-sensor-hub which in turn
> + * is a hid driver, but you can't have more than one hid driver per hid
> + * device, which is a problem because the touch bar also needs to
> + * register as a driver for this hid device.
> + *
> + * 2. While the hid-sensors-als driver stores sensor readings received via
> + * interrupt in an iio buffer, reads on the sysfs
> + * .../iio:deviceX/in_illuminance_YYY attribute result in a get of the
> + * feature report; however, in the case of this sensor here the
> + * illuminance field of that report is always 0. Instead, the input
> + * report needs to be requested.
> + */
> +
> +#define dev_fmt(fmt) "als: " fmt
> +
> +#include <linux/apple-ibridge.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/hid-sensor-ids.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#define APPLEALS_DYN_SENS 0 /* our dynamic sensitivity */
> +#define APPLEALS_DEF_CHANGE_SENS APPLEALS_DYN_SENS
> +
> +struct appleals_device {
> + struct hid_device *hid_dev;
> + struct hid_report *cfg_report;
> + struct hid_field *illum_field;
> + struct iio_dev *iio_dev;
> + int cur_sensitivity;
> + int cur_hysteresis;
> + bool events_enabled;
> +};
> +
> +static struct hid_driver appleals_hid_driver;
> +
> +/*
> + * This is a primitive way to get a relative sensitivity, one where we get
> + * notified when the value changes by a certain percentage rather than some
> + * absolute value. MacOS somehow manages to configure the sensor to work this
> + * way (with a 15% relative sensitivity), but I haven't been able to figure
> + * out how so far. So until we do, this provides a less-than-perfect
> + * simulation.
> + *
> + * When the brightness value is within one of the ranges, the sensitivity is
> + * set to that range's sensitivity. But in order to reduce flapping when the
> + * brightness is right on the border between two ranges, the ranges overlap
> + * somewhat (by at least one sensitivity), and sensitivity is only changed if
> + * the value leaves the current sensitivity's range.
> + *
> + * The values chosen for the map are somewhat arbitrary: a compromise of not
> + * too many ranges (and hence changing the sensitivity) but not too small or
> + * large of a percentage of the min and max values in the range (currently
> + * from 7.5% to 30%, i.e. within a factor of 2 of 15%), as well as just plain
> + * "this feels reasonable to me".
> + */
> +struct appleals_sensitivity_map {
> + int sensitivity;
> + int illum_low;
> + int illum_high;
> +};
> +
> +static const struct appleals_sensitivity_map appleals_sensitivity_map[] = {
> + { 1, 0, 14 },
> + { 3, 10, 40 },
> + { 9, 30, 120 },
> + { 27, 90, 360 },
> + { 81, 270, 1080 },
> + { 243, 810, 3240 },
> + { 729, 2430, 9720 },
> +};
> +
> +static int appleals_compute_sensitivity(int cur_illum, int cur_sens)
> +{
> + const struct appleals_sensitivity_map *entry;
> + int i;
> +
> + /* see if we're still in current range */
> + for (i = 0; i < ARRAY_SIZE(appleals_sensitivity_map); i++) {
> + entry = &appleals_sensitivity_map[i];
> +
> + if (entry->sensitivity == cur_sens &&
> + entry->illum_low <= cur_illum &&
> + entry->illum_high >= cur_illum)
> + return cur_sens;
> + else if (entry->sensitivity > cur_sens)
> + break;
> + }
> +
> + /* not in current range, so find new sensitivity */
> + for (i = 0; i < ARRAY_SIZE(appleals_sensitivity_map); i++) {
> + entry = &appleals_sensitivity_map[i];
> +
> + if (entry->illum_low <= cur_illum &&
> + entry->illum_high >= cur_illum)
> + return entry->sensitivity;
> + }
> +
> + /* hmm, not in table, so assume we are above highest range */
> + i = ARRAY_SIZE(appleals_sensitivity_map) - 1;
> + return appleals_sensitivity_map[i].sensitivity;
> +}
> +
> +static int appleals_get_field_value_for_usage(struct hid_field *field,
> + unsigned int usage)
> +{
> + int u;
> +
> + if (!field)
> + return -1;
> +
> + for (u = 0; u < field->maxusage; u++) {
> + if (field->usage[u].hid == usage)
> + return u + field->logical_minimum;
> + }
> +
> + return -1;
> +}
> +
> +static __s32 appleals_get_field_value(struct appleals_device *als_dev,
> + struct hid_field *field)
> +{
> + bool powered_on = !hid_hw_power(als_dev->hid_dev, PM_HINT_FULLON);
> +
> + hid_hw_request(als_dev->hid_dev, field->report, HID_REQ_GET_REPORT);
> + hid_hw_wait(als_dev->hid_dev);
> +
> + if (powered_on)
> + hid_hw_power(als_dev->hid_dev, PM_HINT_NORMAL);
> +
> + return field->value[0];
> +}
> +
> +static void appleals_set_field_value(struct appleals_device *als_dev,
> + struct hid_field *field, __s32 value)
> +{
> + hid_set_field(field, 0, value);
> + hid_hw_request(als_dev->hid_dev, field->report, HID_REQ_SET_REPORT);
> +}
> +
> +static int appleals_get_config(struct appleals_device *als_dev,
> + unsigned int field_usage, __s32 *value)
> +{
> + struct hid_field *field;
> +
> + field = appleib_find_report_field(als_dev->cfg_report, field_usage);
> + if (!field)
> + return -EINVAL;
> +
> + *value = appleals_get_field_value(als_dev, field);
> +
> + return 0;
> +}
> +
> +static int appleals_set_config(struct appleals_device *als_dev,
> + unsigned int field_usage, __s32 value)
> +{
> + struct hid_field *field;
> +
> + field = appleib_find_report_field(als_dev->cfg_report, field_usage);
> + if (!field)
> + return -EINVAL;
> +
> + appleals_set_field_value(als_dev, field, value);
> +
> + return 0;
> +}
> +
> +static int appleals_set_enum_config(struct appleals_device *als_dev,
> + unsigned int field_usage,
> + unsigned int value_usage)
> +{
> + struct hid_field *field;
> + int value;
> +
> + field = appleib_find_report_field(als_dev->cfg_report, field_usage);
> + if (!field)
> + return -EINVAL;
> +
> + value = appleals_get_field_value_for_usage(field, value_usage);
> + if (value >= 0)
> + appleals_set_field_value(als_dev, field, value);
> +
> + return 0;
> +}
> +
> +static void appleals_update_dyn_sensitivity(struct appleals_device *als_dev,
> + __s32 value)
> +{
> + int new_sens;
> + int rc;
> +
> + new_sens = appleals_compute_sensitivity(value,
> + als_dev->cur_sensitivity);
> + if (new_sens != als_dev->cur_sensitivity) {
> + rc = appleals_set_config(als_dev,
> + HID_USAGE_SENSOR_LIGHT_ILLUM |
> + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS,
> + new_sens);
> + if (!rc)
> + als_dev->cur_sensitivity = new_sens;
> + }
> +}
> +
> +static void appleals_push_new_value(struct appleals_device *als_dev,
> + __s32 value)
> +{
> + __s32 buf[2] = { value, value };
> +
> + iio_push_to_buffers(als_dev->iio_dev, buf);
> +
> + if (als_dev->cur_hysteresis == APPLEALS_DYN_SENS)
> + appleals_update_dyn_sensitivity(als_dev, value);
> +}
> +
> +static int appleals_hid_event(struct hid_device *hdev, struct hid_field *field,
> + struct hid_usage *usage, __s32 value)
> +{
> + struct appleals_device *als_dev = hid_get_drvdata(hdev);
> + int rc = 0;
> +
> + if ((usage->hid & HID_USAGE_PAGE) != HID_UP_SENSOR)
> + return 0;
> +
> + if (usage->hid == HID_USAGE_SENSOR_LIGHT_ILLUM) {
> + appleals_push_new_value(als_dev, value);
> + rc = 1;
Direct return here would be more readable, then return 0 below.
> + }
> +
> + return rc;
> +}
> +
> +static int appleals_enable_events(struct iio_trigger *trig, bool enable)
> +{
> + struct appleals_device *als_dev = iio_trigger_get_drvdata(trig);
> + int value;
> +
> + appleals_set_enum_config(als_dev, HID_USAGE_SENSOR_PROP_REPORT_STATE,
> + enable ? HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM :
> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM);
> + als_dev->events_enabled = enable;
> +
> + /* if the sensor was enabled, push an initial value */
> + if (enable) {
> + value = appleals_get_field_value(als_dev, als_dev->illum_field);
> + appleals_push_new_value(als_dev, value);
> + }
> +
> + return 0;
> +}
> +
> +static int appleals_read_raw(struct iio_dev *iio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct appleals_device **priv = iio_priv(iio_dev);
> + struct appleals_device *als_dev = *priv;
> + __s32 value;
> + int rc;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + *val = appleals_get_field_value(als_dev, als_dev->illum_field);
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + rc = appleals_get_config(als_dev,
> + HID_USAGE_SENSOR_PROP_REPORT_INTERVAL,
> + &value);
> + if (rc)
> + return rc;
> +
> + /* interval is in ms; val is in HZ, val2 in µHZ */
> + value = 1000000000 / value;
> + *val = value / 1000000;
> + *val2 = value - (*val * 1000000);
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +
> + case IIO_CHAN_INFO_HYSTERESIS:
> + if (als_dev->cur_hysteresis == APPLEALS_DYN_SENS) {
> + *val = als_dev->cur_hysteresis;
> + return IIO_VAL_INT;
> + }
> +
> + rc = appleals_get_config(als_dev,
> + HID_USAGE_SENSOR_LIGHT_ILLUM |
> + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS,
> + val);
> + if (!rc) {
> + als_dev->cur_sensitivity = *val;
> + als_dev->cur_hysteresis = *val;
> + }
> + return rc ? rc : IIO_VAL_INT;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int appleals_write_raw(struct iio_dev *iio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct appleals_device **priv = iio_priv(iio_dev);
> + struct appleals_device *als_dev = *priv;
> + __s32 illum;
> + int rc;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + rc = appleals_set_config(als_dev,
> + HID_USAGE_SENSOR_PROP_REPORT_INTERVAL,
> + 1000000000 / (val * 1000000 + val2));
> + return rc;
> +
> + case IIO_CHAN_INFO_HYSTERESIS:
> + if (val == APPLEALS_DYN_SENS) {
Hysteresis normally takes a value, this looks like a magic number being
pushed through the interface?
> + if (als_dev->cur_hysteresis != APPLEALS_DYN_SENS) {
> + als_dev->cur_hysteresis = val;
> + illum = appleals_get_field_value(als_dev,
> + als_dev->illum_field);
> + appleals_update_dyn_sensitivity(als_dev, illum);
> + }
> +
> + return 0;
> + }
> +
> + rc = appleals_set_config(als_dev,
> + HID_USAGE_SENSOR_LIGHT_ILLUM |
> + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS,
> + val);
> + if (!rc) {
> + als_dev->cur_sensitivity = val;
> + als_dev->cur_hysteresis = val;
> + }
> +
> + return rc;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_chan_spec appleals_channels[] = {
> + {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_LIGHT_BOTH,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
This would be unusual. What are the units of this intensity measurement?
Mostly these are the values prior to a proprietary algorithm and have no
particular assigned units that are documented anywhere. As such for
most light sensors this is raw.
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS),
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 32,
> + .storagebits = 32,
> + },
> + .scan_index = 0,
> + },
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS),
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 32,
> + .storagebits = 32,
> + },
> + .scan_index = 1,
> + }
> +};
> +
> +static const struct iio_trigger_ops appleals_trigger_ops = {
> + .set_trigger_state = &appleals_enable_events,
> +};
> +
> +static const struct iio_info appleals_info = {
> + .read_raw = &appleals_read_raw,
> + .write_raw = &appleals_write_raw,
> +};
> +
> +static void appleals_config_sensor(struct appleals_device *als_dev,
> + bool events_enabled, int sensitivity)
> +{
> + struct hid_field *field;
> + bool powered_on;
> + __s32 val;
> +
> + powered_on = !hid_hw_power(als_dev->hid_dev, PM_HINT_FULLON);
> +
> + hid_hw_request(als_dev->hid_dev, als_dev->cfg_report,
> + HID_REQ_GET_REPORT);
> + hid_hw_wait(als_dev->hid_dev);
> +
> + field = appleib_find_report_field(als_dev->cfg_report,
> + HID_USAGE_SENSOR_PROY_POWER_STATE);
> + val = appleals_get_field_value_for_usage(field,
> + HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM);
> + if (val >= 0)
> + hid_set_field(field, 0, val);
> +
> + field = appleib_find_report_field(als_dev->cfg_report,
> + HID_USAGE_SENSOR_PROP_REPORT_STATE);
> + val = appleals_get_field_value_for_usage(field,
> + events_enabled ?
> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM :
> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM);
> + if (val >= 0)
> + hid_set_field(field, 0, val);
> +
> + field = appleib_find_report_field(als_dev->cfg_report,
> + HID_USAGE_SENSOR_PROP_REPORT_INTERVAL);
> + hid_set_field(field, 0, field->logical_minimum);
> +
> + /*
> + * Set initial change sensitivity; if dynamic, enabling trigger will set
> + * it instead.
> + */
> + if (sensitivity != APPLEALS_DYN_SENS) {
> + field = appleib_find_report_field(als_dev->cfg_report,
> + HID_USAGE_SENSOR_LIGHT_ILLUM |
> + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS);
> +
> + hid_set_field(field, 0, sensitivity);
> + }
> +
> + hid_hw_request(als_dev->hid_dev, als_dev->cfg_report,
> + HID_REQ_SET_REPORT);
> +
> + if (powered_on)
> + hid_hw_power(als_dev->hid_dev, PM_HINT_NORMAL);
> +}
> +
> +static int appleals_config_iio(struct appleals_device *als_dev)
> +{
> + struct iio_dev *iio_dev;
> + struct iio_trigger *iio_trig;
> + struct appleals_device **priv;
> + struct device *parent = &als_dev->hid_dev->dev;
> + int rc;
> +
> + iio_dev = devm_iio_device_alloc(parent, sizeof(als_dev));
> + if (!iio_dev)
> + return -ENOMEM;
Hmm. So we are using the private space of the iio device to just
hold a pointer...
Normally we try to avoid this if at all possible as we end
up with loops of pointers that normally indicate a someone
convoluted driver structure. I've not checked all the paths
here to be sure if we can just embed the actual als_dev
structure in here and always have the iio_dev available in
any callbacks etc.
> +
> + priv = iio_priv(iio_dev);
> + *priv = als_dev;
> +
> + iio_dev->channels = appleals_channels;
> + iio_dev->num_channels = ARRAY_SIZE(appleals_channels);
> + iio_dev->dev.parent = parent;
> + iio_dev->info = &appleals_info;
> + iio_dev->name = "als";
Whilst I suppose not many people will have additional ALS devices connected
to these laptops, we normally try to give a slightly more specific name
than this.
> + iio_dev->modes = INDIO_DIRECT_MODE;
> +
> + rc = devm_iio_triggered_buffer_setup(parent, iio_dev,
> + &iio_pollfunc_store_time, NULL,
This is unusual. You have registered all the infrastructure for a triggered
buffer, but then didn't actually provide a function to put any data in it.
It is perfectly acceptable to just skip the trigger if it doesn't make sense
for a given device. If you are doing that, then register the buffer directly
not using this helper.
> + NULL);
> + if (rc) {
> + hid_err(als_dev->hid_dev,
> + "Failed to set up iio triggered buffer: %d\n", rc);
> + return rc;
> + }
> +
> + iio_trig = devm_iio_trigger_alloc(parent, "%s-dev%d", iio_dev->name,
> + iio_dev->id);
> + if (!iio_trig)
> + return -ENOMEM;
> +
> + iio_trig->dev.parent = parent;
> + iio_trig->ops = &appleals_trigger_ops;
> + iio_trigger_set_drvdata(iio_trig, als_dev);
> +
> + rc = devm_iio_trigger_register(parent, iio_trig);
> + if (rc) {
> + hid_err(als_dev->hid_dev,
> + "Failed to register iio trigger: %d\n",
> + rc);
> + return rc;
> + }
What is the purpose of this trigger? It doesn't seem to 'do' anything. There
is no function calling iio_trigger_poll* so it's not acting as a trigger.
> +
> + rc = devm_iio_device_register(parent, iio_dev);
> + if (rc) {
> + hid_err(als_dev->hid_dev, "Failed to register iio device: %d\n",
> + rc);
> + return rc;
> + }
> +
> + als_dev->iio_dev = iio_dev;
> +
> + return 0;
> +}
> +
> +static int appleals_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct appleals_device *als_dev;
> + struct hid_field *state_field;
> + struct hid_field *illum_field;
> + int rc;
> +
> + /* find als fields and reports */
> + rc = hid_parse(hdev);
> + if (rc) {
> + hid_err(hdev, "als: hid parse failed (%d)\n", rc);
> + return rc;
> + }
> +
> + state_field = appleib_find_hid_field(hdev, HID_USAGE_SENSOR_ALS,
> + HID_USAGE_SENSOR_PROP_REPORT_STATE);
> + illum_field = appleib_find_hid_field(hdev, HID_USAGE_SENSOR_ALS,
> + HID_USAGE_SENSOR_LIGHT_ILLUM);
> + if (!state_field || !illum_field)
> + return -ENODEV;
> +
> + hid_dbg(hdev, "Found ambient light sensor\n");
> +
> + /* initialize device */
> + als_dev = devm_kzalloc(&hdev->dev, sizeof(*als_dev), GFP_KERNEL);
> + if (!als_dev)
> + return -ENOMEM;
> +
> + als_dev->hid_dev = hdev;
> + als_dev->cfg_report = state_field->report;
> + als_dev->illum_field = illum_field;
> +
> + als_dev->cur_hysteresis = APPLEALS_DEF_CHANGE_SENS;
> + als_dev->cur_sensitivity = APPLEALS_DEF_CHANGE_SENS;
> +
> + hid_set_drvdata(hdev, als_dev);
> +
> + rc = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> + if (rc) {
> + hid_err(hdev, "als: hw start failed (%d)\n", rc);
> + return rc;
> + }
> +
> + hid_device_io_start(hdev);
> + appleals_config_sensor(als_dev, false, als_dev->cur_sensitivity);
> + hid_device_io_stop(hdev);
> +
> + rc = appleals_config_iio(als_dev);
> + if (rc)
> + return rc;
> +
> + return hid_hw_open(hdev);
> +}
> +
> +#ifdef CONFIG_PM
> +static int appleals_reset_resume(struct hid_device *hdev)
> +{
> + struct appleals_device *als_dev = hid_get_drvdata(hdev);
> + __s32 illum;
> +
> + appleals_config_sensor(als_dev, als_dev->events_enabled,
> + als_dev->cur_sensitivity);
> +
> + illum = appleals_get_field_value(als_dev, als_dev->illum_field);
> + appleals_push_new_value(als_dev, illum);
> +
> + return 0;
> +}
> +#endif
> +
> +static const struct hid_device_id appleals_hid_ids[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> + USB_DEVICE_ID_IBRIDGE_ALS) },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(hid, appleals_hid_ids);
> +
> +static struct hid_driver appleals_hid_driver = {
> + .name = "apple-ib-als",
> + .id_table = appleals_hid_ids,
> + .probe = appleals_probe,
> + .event = appleals_hid_event,
> +#ifdef CONFIG_PM
> + .reset_resume = appleals_reset_resume,
> +#endif
> +};
> +
> +module_hid_driver(appleals_hid_driver);
> +
> +MODULE_AUTHOR("Ronald Tschalär");
> +MODULE_DESCRIPTION("Apple iBridge ALS driver");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply
* Re: [PATCH v2 2/3] HID: apple-ib-tb: Add driver for the Touch Bar on MacBook Pro's.
From: Jonathan Cameron @ 2019-06-16 14:28 UTC (permalink / raw)
To: Ronald Tschalär
Cc: Jiri Kosina, Benjamin Tissoires, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, linux-input,
linux-iio, linux-kernel
In-Reply-To: <20190612083400.1015-3-ronald@innovation.ch>
On Wed, 12 Jun 2019 01:33:59 -0700
Ronald Tschalär <ronald@innovation.ch> wrote:
> This driver enables basic touch bar functionality: enabling it, switching
> between modes on FN key press, and dimming and turning the display
> off/on when idle/active.
>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
A few minor comments inline from me, but as before well outside of my
areas of knowledge!
Jonathan
> ---
> drivers/hid/Kconfig | 10 +
> drivers/hid/Makefile | 1 +
> drivers/hid/apple-ib-tb.c | 1389 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 1400 insertions(+)
> create mode 100644 drivers/hid/apple-ib-tb.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 545d3691fc1c..7621c2500d71 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -149,6 +149,16 @@ config HID_APPLE_IBRIDGE
> To compile this driver as a module, choose M here: the
> module will be called apple-ibridge.
>
> +config HID_APPLE_IBRIDGE_TB
> + tristate "Apple iBridge Touch Bar"
> + depends on HID_APPLE_IBRIDGE
> + ---help---
> + Say Y here if you want support for the Touch Bar on recent
> + MacBook Pros.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apple-ib-tb.
> +
> config HID_APPLEIR
> tristate "Apple infrared receiver"
> depends on (USB_HID)
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index a4da5663a541..0c46e5f70db1 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_HID_ALPS) += hid-alps.o
> obj-$(CONFIG_HID_ACRUX) += hid-axff.o
> obj-$(CONFIG_HID_APPLE) += hid-apple.o
> obj-$(CONFIG_HID_APPLE_IBRIDGE) += apple-ibridge.o
> +obj-$(CONFIG_HID_APPLE_IBRIDGE_TB) += apple-ib-tb.o
> obj-$(CONFIG_HID_APPLEIR) += hid-appleir.o
> obj-$(CONFIG_HID_ASUS) += hid-asus.o
> obj-$(CONFIG_HID_AUREAL) += hid-aureal.o
> diff --git a/drivers/hid/apple-ib-tb.c b/drivers/hid/apple-ib-tb.c
> new file mode 100644
> index 000000000000..6daee80060ce
> --- /dev/null
> +++ b/drivers/hid/apple-ib-tb.c
> @@ -0,0 +1,1389 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple Touch Bar Driver
> + *
> + * Copyright (c) 2017-2018 Ronald Tschalär
> + */
> +
> +/*
> + * Recent MacBookPro models (13,[23] and 14,[23]) have a touch bar, which
> + * is exposed via several USB interfaces. MacOS supports a fancy mode
> + * where arbitrary buttons can be defined; this driver currently only
> + * supports the simple mode that consists of 3 predefined layouts
> + * (escape-only, esc + special keys, and esc + function keys).
> + *
> + * The first USB HID interface supports two reports, an input report that
> + * is used to report the key presses, and an output report which can be
> + * used to set the touch bar "mode": touch bar off (in which case no touches
> + * are reported at all), escape key only, escape + 12 function keys, and
> + * escape + several special keys (including brightness, audio volume,
> + * etc). The second interface supports several, complex reports, most of
> + * which are unknown at this time, but one of which has been determined to
> + * allow for controlling of the touch bar's brightness: off (though touches
> + * are still reported), dimmed, and full brightness. This driver makes
> + * use of these two reports.
> + */
> +
> +#define dev_fmt(fmt) "tb: " fmt
> +
> +#include <linux/apple-ibridge.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/input.h>
> +#include <linux/jiffies.h>
> +#include <linux/ktime.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb.h>
> +#include <linux/workqueue.h>
> +
> +#define HID_UP_APPLE 0xff120000
> +#define HID_USAGE_MODE (HID_UP_CUSTOM | 0x0004)
> +#define HID_USAGE_APPLE_APP (HID_UP_APPLE | 0x0001)
> +#define HID_USAGE_DISP (HID_UP_APPLE | 0x0021)
> +#define HID_USAGE_DISP_AUX1 (HID_UP_APPLE | 0x0020)
> +
> +#define APPLETB_MAX_TB_KEYS 13 /* ESC, F1-F12 */
> +
> +#define APPLETB_CMD_MODE_ESC 0
> +#define APPLETB_CMD_MODE_FN 1
> +#define APPLETB_CMD_MODE_SPCL 2
> +#define APPLETB_CMD_MODE_OFF 3
> +#define APPLETB_CMD_MODE_UPD 254
> +#define APPLETB_CMD_MODE_NONE 255
> +
> +#define APPLETB_CMD_DISP_ON 1
> +#define APPLETB_CMD_DISP_DIM 2
> +#define APPLETB_CMD_DISP_OFF 4
> +#define APPLETB_CMD_DISP_UPD 254
> +#define APPLETB_CMD_DISP_NONE 255
> +
> +#define APPLETB_FN_MODE_FKEYS 0
> +#define APPLETB_FN_MODE_NORM 1
> +#define APPLETB_FN_MODE_INV 2
> +#define APPLETB_FN_MODE_SPCL 3
> +#define APPLETB_FN_MODE_MAX APPLETB_FN_MODE_SPCL
> +
> +#define APPLETB_DEVID_KEYBOARD 1
> +#define APPLETB_DEVID_TOUCHPAD 2
> +
> +#define APPLETB_MAX_DIM_TIME 30
> +
> +static int appletb_tb_def_idle_timeout = 5 * 60;
> +module_param_named(idle_timeout, appletb_tb_def_idle_timeout, int, 0444);
> +MODULE_PARM_DESC(idle_timeout, "Default touch bar idle timeout:\n"
> + " >0 - turn touch bar display off after no keyboard, trackpad, or touch bar input has been received for this many seconds;\n"
> + " the display will be turned back on as soon as new input is received\n"
> + " 0 - turn touch bar display off (input does not turn it on again)\n"
> + " -1 - turn touch bar display on (does not turn off automatically)\n"
> + " -2 - disable touch bar completely");
> +
> +static int appletb_tb_def_dim_timeout = -2;
> +module_param_named(dim_timeout, appletb_tb_def_dim_timeout, int, 0444);
> +MODULE_PARM_DESC(dim_timeout, "Default touch bar dim timeout:\n"
> + " >0 - dim touch bar display after no keyboard, trackpad, or touch bar input has been received for this many seconds\n"
> + " the display will be returned to full brightness as soon as new input is received\n"
> + " 0 - dim touch bar display (input does not return it to full brightness)\n"
> + " -1 - disable timeout (touch bar never dimmed)\n"
> + " [-2] - calculate timeout based on idle-timeout");
> +
> +static int appletb_tb_def_fn_mode = APPLETB_FN_MODE_NORM;
> +module_param_named(fnmode, appletb_tb_def_fn_mode, int, 0444);
> +MODULE_PARM_DESC(fnmode, "Default Fn key mode:\n"
> + " 0 - function-keys only\n"
> + " [1] - fn key switches from special to function-keys\n"
> + " 2 - inverse of 1\n"
> + " 3 - special keys only");
> +
> +static ssize_t idle_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf);
> +static ssize_t idle_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size);
> +static DEVICE_ATTR_RW(idle_timeout);
> +
> +static ssize_t dim_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf);
> +static ssize_t dim_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size);
> +static DEVICE_ATTR_RW(dim_timeout);
> +
> +static ssize_t fnmode_show(struct device *dev, struct device_attribute *attr,
> + char *buf);
> +static ssize_t fnmode_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t size);
> +static DEVICE_ATTR_RW(fnmode);
> +
> +static struct attribute *appletb_attrs[] = {
> + &dev_attr_idle_timeout.attr,
> + &dev_attr_dim_timeout.attr,
> + &dev_attr_fnmode.attr,
> + NULL,
Documentation for these?
> +};
> +
> +static const struct attribute_group appletb_attr_group = {
> + .attrs = appletb_attrs,
> +};
> +
...
> +/*
> + * While the mode functionality is listed as a valid hid report in the usb
> + * interface descriptor, it's not sent that way. Instead it's sent with
> + * different request-type and without a leading report-id in the data. Hence
> + * we need to send it as a custom usb control message rather via any of the
> + * standard hid_hw_*request() functions.
> + */
> +static int appletb_set_tb_mode(struct appletb_device *tb_dev,
> + unsigned char mode)
> +{
> + void *buf;
> + bool autopm_off = false;
> + int rc;
> +
> + if (!tb_dev->mode_iface.hdev)
> + return -ENOTCONN;
> +
> + buf = kmemdup(&mode, 1, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + autopm_off = appletb_disable_autopm(tb_dev->mode_iface.hdev);
> +
> + rc = appletb_send_usb_ctrl(&tb_dev->mode_iface,
> + USB_DIR_OUT | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE,
Odd indentation.
> + tb_dev->mode_field->report, buf, 1);
> + if (rc < 0)
> + dev_err(tb_dev->log_dev,
> + "Failed to set touch bar mode to %u (%d)\n", mode, rc);
> +
> + if (autopm_off)
> + hid_hw_power(tb_dev->mode_iface.hdev, PM_HINT_NORMAL);
> +
> + kfree(buf);
> +
> + return rc;
> +}
...
> +
> +static int appletb_set_tb_disp(struct appletb_device *tb_dev,
> + unsigned char disp)
> +{
> + struct hid_report *report;
> + int rc;
> +
> + if (!tb_dev->disp_iface.hdev)
> + return -ENOTCONN;
> +
> + report = tb_dev->disp_field->report;
> +
> + rc = hid_set_field(tb_dev->disp_field_aux1, 0, 1);
> + if (!rc)
> + rc = hid_set_field(tb_dev->disp_field, 0, disp);
This stacked error handling is less readable than just having two
separate error blocks with very similar comments.
> + if (rc) {
> + dev_err(tb_dev->log_dev,
> + "Failed to set display report fields (%d)\n", rc);
> + return rc;
> + }
> +
> + /*
> + * Keep the USB interface powered on while the touch bar display is on
> + * for better responsiveness.
> + */
> + if (disp != APPLETB_CMD_DISP_OFF && !tb_dev->tb_autopm_off)
> + tb_dev->tb_autopm_off =
> + appletb_disable_autopm(report->device);
> +
> + rc = appletb_send_hid_report(&tb_dev->disp_iface, report);
> +
Nitpick. For consistency should probably not be a blank line here.
> + if (rc < 0)
> + dev_err(tb_dev->log_dev,
> + "Failed to set touch bar display to %u (%d)\n", disp,
> + rc);
> +
> + if (disp == APPLETB_CMD_DISP_OFF && tb_dev->tb_autopm_off) {
> + hid_hw_power(tb_dev->disp_iface.hdev, PM_HINT_NORMAL);
> + tb_dev->tb_autopm_off = false;
> + }
> +
> + return rc;
> +}
> +
...
> +static int appletb_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct appletb_device *tb_dev = appletb_dev;
> + struct appletb_iface_info *iface_info;
> + unsigned long flags;
> + int rc;
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + if (!tb_dev->log_dev)
> + tb_dev->log_dev = &hdev->dev;
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +
> + hid_set_drvdata(hdev, tb_dev);
> +
> + /* initialize the report info */
> + rc = hid_parse(hdev);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "als: hid parse failed (%d)\n", rc);
> + goto error;
> + }
> +
> + rc = appletb_extract_report_info(tb_dev, hdev);
> + if (rc < 0)
> + goto error;
> +
> + rc = hid_hw_start(hdev, HID_CONNECT_DRIVER | HID_CONNECT_HIDINPUT);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "hw start failed (%d)\n", rc);
> + goto clear_iface_info;
> + }
> +
> + rc = hid_hw_open(hdev);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "hw open failed (%d)\n", rc);
> + goto stop_hid;
> + }
> +
> + /* do setup if we have both interfaces */
> + if (appletb_test_and_mark_active(tb_dev)) {
> + /* initialize the touch bar */
> + if (appletb_tb_def_fn_mode >= 0 &&
> + appletb_tb_def_fn_mode <= APPLETB_FN_MODE_MAX)
> + tb_dev->fn_mode = appletb_tb_def_fn_mode;
> + else
> + tb_dev->fn_mode = APPLETB_FN_MODE_NORM;
> + appletb_set_idle_timeout(tb_dev, appletb_tb_def_idle_timeout);
> + appletb_set_dim_timeout(tb_dev, appletb_tb_def_dim_timeout);
> + tb_dev->last_event_time = ktime_get();
> +
> + tb_dev->pnd_tb_mode = APPLETB_CMD_MODE_UPD;
> + tb_dev->pnd_tb_disp = APPLETB_CMD_DISP_UPD;
> +
> + appletb_update_touchbar(tb_dev, false);
> +
> + /* set up the input handler */
> + tb_dev->inp_handler.event = appletb_inp_event;
> + tb_dev->inp_handler.connect = appletb_inp_connect;
> + tb_dev->inp_handler.disconnect = appletb_inp_disconnect;
> + tb_dev->inp_handler.name = "appletb";
> + tb_dev->inp_handler.id_table = appletb_input_devices;
> + tb_dev->inp_handler.private = tb_dev;
> +
> + rc = input_register_handler(&tb_dev->inp_handler);
> + if (rc) {
> + dev_err(tb_dev->log_dev,
> + "Unable to register keyboard handler (%d)\n",
> + rc);
> + goto mark_inactive;
> + }
> +
> + /* initialize sysfs attributes */
> + rc = sysfs_create_group(&tb_dev->mode_iface.hdev->dev.kobj,
> + &appletb_attr_group);
> + if (rc) {
> + dev_err(tb_dev->log_dev,
> + "Failed to create sysfs attributes (%d)\n", rc);
> + goto unreg_handler;
> + }
> +
> + dev_dbg(tb_dev->log_dev, "Touchbar activated\n");
> + }
> +
> + return 0;
> +
> +unreg_handler:
> + input_unregister_handler(&tb_dev->inp_handler);
> +mark_inactive:
> + appletb_test_and_mark_inactive(tb_dev, hdev);
> + cancel_delayed_work_sync(&tb_dev->tb_work);
> + hid_hw_close(hdev);
> +stop_hid:
> + hid_hw_stop(hdev);
> +clear_iface_info:
> + iface_info = appletb_get_iface_info(tb_dev, hdev);
> + if (iface_info) {
> + usb_put_intf(iface_info->usb_iface);
> + iface_info->usb_iface = NULL;
> + iface_info->hdev = NULL;
> + }
It might be cleaner to put this block into a utility function
named appropriately to make it clear it's undoing stuff from
the *_extract_report_info call. Initially I wondered where
this stuff we are undoing came from.
> +error:
> + return rc;
> +}
> +
> +static void appletb_remove(struct hid_device *hdev)
> +{
> + struct appletb_device *tb_dev = hid_get_drvdata(hdev);
> + struct appletb_iface_info *iface_info;
> + unsigned long flags;
> +
> + if (appletb_test_and_mark_inactive(tb_dev, hdev)) {
> + sysfs_remove_group(&tb_dev->mode_iface.hdev->dev.kobj,
> + &appletb_attr_group);
> +
> + input_unregister_handler(&tb_dev->inp_handler);
> +
> + cancel_delayed_work_sync(&tb_dev->tb_work);
> + appletb_set_tb_mode(tb_dev, APPLETB_CMD_MODE_OFF);
> + appletb_set_tb_disp(tb_dev, APPLETB_CMD_DISP_ON);
> +
> + if (tb_dev->tb_autopm_off)
> + hid_hw_power(tb_dev->disp_iface.hdev, PM_HINT_NORMAL);
> +
> + dev_info(tb_dev->log_dev, "Touchbar deactivated\n");
> + }
> +
> + hid_hw_close(hdev);
> + hid_hw_stop(hdev);
> +
> + iface_info = appletb_get_iface_info(tb_dev, hdev);
> + if (iface_info) {
> + usb_put_intf(iface_info->usb_iface);
> + iface_info->usb_iface = NULL;
> + iface_info->hdev = NULL;
> + }
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + if (tb_dev->log_dev == &hdev->dev) {
> + if (tb_dev->mode_iface.hdev)
> + tb_dev->log_dev = &tb_dev->mode_iface.hdev->dev;
> + else if (tb_dev->disp_iface.hdev)
> + tb_dev->log_dev = &tb_dev->disp_iface.hdev->dev;
> + else
> + tb_dev->log_dev = NULL;
> + }
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +}
...
^ permalink raw reply
* Re: [PATCH v2 1/3] HID: apple-ibridge: Add Apple iBridge HID driver.
From: Jonathan Cameron @ 2019-06-16 14:01 UTC (permalink / raw)
To: Ronald Tschalär
Cc: Jiri Kosina, Benjamin Tissoires, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, linux-input,
linux-iio, linux-kernel
In-Reply-To: <20190612083400.1015-2-ronald@innovation.ch>
On Wed, 12 Jun 2019 01:33:58 -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 creates virtual HID devices, one per real HID device and
> sub-driver pair (for a total of 4 virtual HID devices). The sub-drivers
> then bind to these virtual HID devices.
>
> This way the Touch Bar and ALS drivers can be kept in their own modules,
> while at the same time making them look very much like as if they were
> connected to the real HID devices; e.g. in particular the Touch Bar
> driver is aware of the fact that it is dealing with two HID devices that
> need to managed differently.
>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
Hi Ronald,
I'm far from a hid expert and was only reading this for background on
the ALS sensor driver... Anyhow, there are some comments in here
that needs some follow up or formatting into kernel comment style.
Nitpicks inline!
Jonathan
> ---
> drivers/hid/Kconfig | 14 +
> drivers/hid/Makefile | 1 +
> drivers/hid/apple-ibridge.c | 585 ++++++++++++++++++++++++++++++++++
> drivers/hid/hid-ids.h | 1 +
> include/linux/apple-ibridge.h | 23 ++
> 5 files changed, 624 insertions(+)
> create mode 100644 drivers/hid/apple-ibridge.c
> create mode 100644 include/linux/apple-ibridge.h
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 4ca0cdfa6b33..545d3691fc1c 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -135,6 +135,20 @@ config HID_APPLE
> Say Y here if you want support for keyboards of Apple iBooks, PowerBooks,
> MacBooks, MacBook Pros and Apple Aluminum.
>
> +config HID_APPLE_IBRIDGE
> + tristate "Apple iBridge"
> + depends on ACPI
> + depends on USB_HID
> + depends on X86 || COMPILE_TEST
> + help
> + This module provides the core support for the Apple T1 chip found
> + on recent MacBookPro's, also known as the iBridge. The drivers for
> + the Touch Bar (apple-ib-tb) and light sensor (apple-ib-als) need to
> + be enabled separately.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apple-ibridge.
> +
> config HID_APPLEIR
> tristate "Apple infrared receiver"
> depends on (USB_HID)
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 170163b41303..a4da5663a541 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_HID_ACCUTOUCH) += hid-accutouch.o
> obj-$(CONFIG_HID_ALPS) += hid-alps.o
> obj-$(CONFIG_HID_ACRUX) += hid-axff.o
> obj-$(CONFIG_HID_APPLE) += hid-apple.o
> +obj-$(CONFIG_HID_APPLE_IBRIDGE) += apple-ibridge.o
> obj-$(CONFIG_HID_APPLEIR) += hid-appleir.o
> obj-$(CONFIG_HID_ASUS) += hid-asus.o
> obj-$(CONFIG_HID_AUREAL) += hid-aureal.o
> diff --git a/drivers/hid/apple-ibridge.c b/drivers/hid/apple-ibridge.c
> new file mode 100644
> index 000000000000..565f080a38d6
> --- /dev/null
> +++ b/drivers/hid/apple-ibridge.c
> @@ -0,0 +1,585 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple iBridge Driver
> + *
> + * Copyright (c) 2018 Ronald Tschalär
> + */
> +
> +/**
> + * DOC: Overview
> + *
> + * MacBookPro models with a Touch Bar (13,[23] and 14,[23]) have an Apple
> + * iBridge chip (also known as T1 chip) which exposes the touch bar,
> + * built-in webcam (iSight), ambient light sensor, and Secure Enclave
> + * Processor (SEP) for TouchID. It shows up in the system as a USB device
> + * with 3 configurations: 'Default iBridge Interfaces', 'Default iBridge
> + * Interfaces(OS X)', and 'Default iBridge Interfaces(Recovery)'. While
> + * the second one is used by MacOS to provide the fancy touch bar
> + * functionality with custom buttons etc, this driver just uses the first.
> + *
> + * In the first (default after boot) configuration, 4 usb interfaces are
> + * exposed: 2 related to the webcam, and 2 USB HID interfaces representing
> + * the touch bar and the ambient light sensor. The webcam interfaces are
> + * already handled by the uvcvideo driver; furthermore, the handling of the
> + * input reports when "keys" on the touch bar are pressed is already handled
> + * properly by the generic USB HID core. This leaves the management of the
> + * touch bar modes (e.g. switching between function and special keys when the
> + * FN key is pressed), the touch bar display (dimming and turning off), the
> + * key-remapping when the FN key is pressed, and handling of the light sensor.
> + *
> + * This driver is implemented as a HID driver that creates virtual HID devices
> + * for the ALS and touch bar functionality, and the ALS and touch bar drivers
> + * are HID drivers which in turn attach to these virtual HID devices. This
> + * driver then relays the calls on the real HID devices to the virtual ones,
> + * and visa versa.
> + *
> + * Lastly, this driver also takes care of the power-management for the
> + * iBridge when suspending and resuming.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/apple-ibridge.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +
> +#include "hid-ids.h"
> +#include "../hid/usbhid/usbhid.h"
> +
> +#define APPLEIB_BASIC_CONFIG 1
> +
> +#define LOG_DEV(ib_dev) (&(ib_dev)->acpi_dev->dev)
> +
> +static struct hid_device_id appleib_sub_hid_ids[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> + USB_DEVICE_ID_IBRIDGE_TB) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> + USB_DEVICE_ID_IBRIDGE_ALS) },
> +};
> +
> +struct appleib_device {
> + struct acpi_device *acpi_dev;
> + acpi_handle asoc_socw;
> +};
> +
> +struct appleib_hid_dev_info {
> + struct hid_device *hdev;
> + struct hid_device *sub_hdevs[ARRAY_SIZE(appleib_sub_hid_ids)];
> +};
> +
> +static void appleib_remove_device(struct hid_device *hdev)
> +{
> + struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++)
> + hid_destroy_device(hdev_info->sub_hdevs[i]);
> +
> + hid_set_drvdata(hdev, NULL);
> +}
> +
> +/**
> + * appleib_forward_int_op() - Forward a hid-driver callback to all drivers on
> + * all virtual HID devices attached to the given real HID device.
> + * @hdev the real hid-device
> + * @forward a function that calls the callback on the given driver
> + * @args arguments for the forward function
> + *
> + * This is for callbacks that return a status as an int.
> + *
> + * Returns: 0 on success, or the first error returned by the @forward function.
> + */
> +static int appleib_forward_int_op(struct hid_device *hdev,
> + int (*forward)(struct hid_driver *,
> + struct hid_device *, void *),
> + void *args)
> +{
> + struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
> + struct hid_device *sub_hdev;
> + int rc = 0;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> + sub_hdev = hdev_info->sub_hdevs[i];
> + if (sub_hdev->driver) {
> + rc = forward(sub_hdev->driver, sub_hdev, args);
> + if (rc)
> + break;
> + }
> +
> + break;
> + }
> +
> + return rc;
> +}
> +
> +static int appleib_hid_raw_event(struct hid_device *hdev,
> + struct hid_report *report, u8 *data, int size)
> +{
> + struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++)
> + hid_input_report(hdev_info->sub_hdevs[i], report->type, data,
> + size, 0);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int appleib_hid_suspend_fwd(struct hid_driver *drv,
> + struct hid_device *hdev, void *args)
> +{
> + int rc = 0;
> +
> + if (drv->suspend)
> + rc = drv->suspend(hdev, *(pm_message_t *)args);
> +
> + return rc;
> +}
> +
> +static int appleib_hid_suspend(struct hid_device *hdev, pm_message_t message)
> +{
> + return appleib_forward_int_op(hdev, appleib_hid_suspend_fwd, &message);
> +}
> +
> +static int appleib_hid_resume_fwd(struct hid_driver *drv,
> + struct hid_device *hdev, void *args)
> +{
> + int rc = 0;
> +
> + if (drv->resume)
> + rc = drv->resume(hdev);
> +
> + return rc;
> +}
> +
> +static int appleib_hid_resume(struct hid_device *hdev)
> +{
> + return appleib_forward_int_op(hdev, appleib_hid_resume_fwd, NULL);
> +}
> +
> +static int appleib_hid_reset_resume_fwd(struct hid_driver *drv,
> + struct hid_device *hdev, void *args)
> +{
> + int rc = 0;
> +
> + if (drv->reset_resume)
> + rc = drv->reset_resume(hdev);
> +
> + return rc;
> +}
> +
> +static int appleib_hid_reset_resume(struct hid_device *hdev)
> +{
> + return appleib_forward_int_op(hdev, appleib_hid_reset_resume_fwd, NULL);
> +}
> +#endif /* CONFIG_PM */
> +
> +/**
> + * appleib_find_report_field() - Find the field in the report with the given
> + * usage.
> + * @report: the report to search
> + * @field_usage: the usage of the field to search for
> + *
> + * Returns: the hid field if found, or NULL if none found.
> + */
> +struct hid_field *appleib_find_report_field(struct hid_report *report,
> + unsigned int field_usage)
> +{
> + int f, u;
> +
> + for (f = 0; f < report->maxfield; f++) {
> + struct hid_field *field = report->field[f];
> +
> + if (field->logical == field_usage)
> + return field;
> +
> + for (u = 0; u < field->maxusage; u++) {
> + if (field->usage[u].hid == field_usage)
> + return field;
> + }
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(appleib_find_report_field);
> +
> +/**
> + * appleib_find_hid_field() - Search all the reports of the device for the
> + * field with the given usage.
> + * @hdev: the device whose reports to search
> + * @application: the usage of application collection that the field must
> + * belong to
> + * @field_usage: the usage of the field to search for
> + *
> + * Returns: the hid field if found, or NULL if none found.
> + */
> +struct hid_field *appleib_find_hid_field(struct hid_device *hdev,
> + unsigned int application,
> + unsigned int field_usage)
> +{
> + static const int report_types[] = { HID_INPUT_REPORT, HID_OUTPUT_REPORT,
> + HID_FEATURE_REPORT };
> + struct hid_report *report;
> + struct hid_field *field;
> + int t;
> +
> + for (t = 0; t < ARRAY_SIZE(report_types); t++) {
> + struct list_head *report_list =
> + &hdev->report_enum[report_types[t]].report_list;
> + list_for_each_entry(report, report_list, list) {
> + if (report->application != application)
> + continue;
> +
> + field = appleib_find_report_field(report, field_usage);
> + if (field)
> + return field;
> + }
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(appleib_find_hid_field);
> +
> +static int appleib_ll_start(struct hid_device *hdev)
> +{
> + return 0;
> +}
> +
> +static void appleib_ll_stop(struct hid_device *hdev)
> +{
> +}
> +
> +static int appleib_ll_open(struct hid_device *hdev)
> +{
> + // TODO: allow event reporting
Comment syntax :)
> + return 0;
> +}
> +
> +static void appleib_ll_close(struct hid_device *hdev)
> +{
> + // TODO: disallow event reporting
> +}
> +
> +static int appleib_ll_power(struct hid_device *hdev, int level)
> +{
> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> + return hid_hw_power(hdev_info->hdev, level);
> +}
> +
> +static int appleib_ll_parse(struct hid_device *hdev)
> +{
> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> + return hid_parse_report(hdev, hdev_info->hdev->rdesc,
> + hdev_info->hdev->rsize);
> +}
> +
> +static void appleib_ll_request(struct hid_device *hdev,
> + struct hid_report *report, int reqtype)
> +{
> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> + hid_hw_request(hdev_info->hdev, report, reqtype);
> +}
> +
> +static int appleib_ll_wait(struct hid_device *hdev)
> +{
> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> + hid_hw_wait(hdev_info->hdev);
> + return 0;
> +}
> +
> +static int appleib_ll_raw_request(struct hid_device *hdev,
> + unsigned char reportnum, __u8 *buf,
> + size_t len, unsigned char rtype, int reqtype)
> +{
> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> + return hid_hw_raw_request(hdev_info->hdev, reportnum, buf, len, rtype,
> + reqtype);
> +}
> +
> +static int appleib_ll_output_report(struct hid_device *hdev, __u8 *buf,
> + size_t len)
> +{
> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> + return hid_hw_output_report(hdev_info->hdev, buf, len);
> +}
> +
> +static struct hid_ll_driver appleib_ll_driver = {
> + .start = appleib_ll_start,
> + .stop = appleib_ll_stop,
> + .open = appleib_ll_open,
> + .close = appleib_ll_close,
> + .power = appleib_ll_power,
> + .parse = appleib_ll_parse,
> + .request = appleib_ll_request,
> + .wait = appleib_ll_wait,
> + .raw_request = appleib_ll_raw_request,
> + .output_report = appleib_ll_output_report,
> +};
> +
> +static struct hid_device *
> +appleib_add_sub_dev(struct appleib_hid_dev_info *hdev_info,
> + struct hid_device_id *dev_id)
> +{
> + struct hid_device *sub_hdev;
> + int rc;
> +
> + sub_hdev = hid_allocate_device();
> + if (IS_ERR(sub_hdev))
> + return sub_hdev;
> +
> + sub_hdev->dev.parent = &hdev_info->hdev->dev;
> +
> + sub_hdev->bus = dev_id->bus;
> + sub_hdev->group = dev_id->group;
> + sub_hdev->vendor = dev_id->vendor;
> + sub_hdev->product = dev_id->product;
> +
> + sub_hdev->ll_driver = &appleib_ll_driver;
> +
> + snprintf(sub_hdev->name, sizeof(sub_hdev->name),
> + "iBridge Virtual HID %s/%04x:%04x",
> + dev_name(sub_hdev->dev.parent), sub_hdev->vendor,
> + sub_hdev->product);
> +
> + sub_hdev->driver_data = hdev_info;
> +
> + rc = hid_add_device(sub_hdev);
> + if (rc) {
> + hid_destroy_device(sub_hdev);
> + return ERR_PTR(rc);
> + }
> +
> + return sub_hdev;
> +}
> +
> +static struct appleib_hid_dev_info *appleib_add_device(struct hid_device *hdev)
> +{
> + struct appleib_hid_dev_info *hdev_info;
> + int i;
> +
> + hdev_info = devm_kzalloc(&hdev->dev, sizeof(*hdev_info), GFP_KERNEL);
> + if (!hdev_info)
> + return ERR_PTR(-ENOMEM);
> +
> + hdev_info->hdev = hdev;
> +
> + for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> + hdev_info->sub_hdevs[i] =
> + appleib_add_sub_dev(hdev_info, &appleib_sub_hid_ids[i]);
> +
> + if (IS_ERR(hdev_info->sub_hdevs[i])) {
> + while (i-- > 0)
> + hid_destroy_device(hdev_info->sub_hdevs[i]);
> + return (void *)hdev_info->sub_hdevs[i];
> + }
> + }
> +
> + return hdev_info;
> +}
> +
> +static int appleib_hid_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct appleib_hid_dev_info *hdev_info;
> + struct usb_device *udev;
> + int rc;
> +
> + /* check and set usb config first */
> + udev = hid_to_usb_dev(hdev);
> +
> + if (udev->actconfig->desc.bConfigurationValue != APPLEIB_BASIC_CONFIG) {
> + rc = usb_driver_set_configuration(udev, APPLEIB_BASIC_CONFIG);
> + return rc ? rc : -ENODEV;
> + }
> +
> + rc = hid_parse(hdev);
> + if (rc) {
> + hid_err(hdev, "ib: hid parse failed (%d)\n", rc);
> + goto error;
> + }
> +
> + rc = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> + if (rc) {
> + hid_err(hdev, "ib: hw start failed (%d)\n", rc);
> + goto error;
> + }
> +
> + hdev_info = appleib_add_device(hdev);
> + if (IS_ERR(hdev_info)) {
> + rc = PTR_ERR(hdev_info);
> + goto stop_hw;
> + }
> +
> + hid_set_drvdata(hdev, hdev_info);
> +
> + rc = hid_hw_open(hdev);
> + if (rc) {
> + hid_err(hdev, "ib: failed to open hid: %d\n", rc);
> + goto remove_dev;
> + }
> +
> + return 0;
> +
> +remove_dev:
> + appleib_remove_device(hdev);
> +stop_hw:
> + hid_hw_stop(hdev);
> +error:
> + return rc;
> +}
> +
> +static void appleib_hid_remove(struct hid_device *hdev)
> +{
> + hid_hw_close(hdev);
> + appleib_remove_device(hdev);
> + hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id appleib_hid_ids[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
> + { },
> +};
> +
> +static struct hid_driver appleib_hid_driver = {
> + .name = "apple-ibridge-hid",
> + .id_table = appleib_hid_ids,
> + .probe = appleib_hid_probe,
> + .remove = appleib_hid_remove,
> + .raw_event = appleib_hid_raw_event,
> +#ifdef CONFIG_PM
> + .suspend = appleib_hid_suspend,
> + .resume = appleib_hid_resume,
> + .reset_resume = appleib_hid_reset_resume,
> +#endif
> +};
> +
> +static struct appleib_device *appleib_alloc_device(struct acpi_device *acpi_dev)
> +{
> + struct appleib_device *ib_dev;
> + acpi_status sts;
> +
> + ib_dev = devm_kzalloc(&acpi_dev->dev, sizeof(*ib_dev), GFP_KERNEL);
> + if (!ib_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + ib_dev->acpi_dev = acpi_dev;
> +
> + /* get iBridge acpi power control method for suspend/resume */
> + sts = acpi_get_handle(acpi_dev->handle, "SOCW", &ib_dev->asoc_socw);
> + if (ACPI_FAILURE(sts)) {
> + dev_err(LOG_DEV(ib_dev),
> + "Error getting handle for ASOC.SOCW method: %s\n",
> + acpi_format_exception(sts));
> + return ERR_PTR(-ENXIO);
> + }
> +
> + /* ensure iBridge is powered on */
> + sts = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1);
> + if (ACPI_FAILURE(sts))
> + dev_warn(LOG_DEV(ib_dev), "SOCW(1) failed: %s\n",
> + acpi_format_exception(sts));
> +
> + return ib_dev;
> +}
> +
> +static int appleib_probe(struct acpi_device *acpi)
> +{
> + struct appleib_device *ib_dev;
> + int ret;
> +
> + ib_dev = appleib_alloc_device(acpi);
> + if (IS_ERR(ib_dev))
> + return PTR_ERR(ib_dev);
> +
> + ret = hid_register_driver(&appleib_hid_driver);
> + if (ret) {
> + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n",
> + ret);
> + return ret;
> + }
> +
> + acpi->driver_data = ib_dev;
> +
> + return 0;
> +}
> +
> +static int appleib_remove(struct acpi_device *acpi)
> +{
> + hid_unregister_driver(&appleib_hid_driver);
> +
> + return 0;
> +}
> +
> +static int appleib_suspend(struct device *dev)
> +{
> + struct appleib_device *ib_dev;
> + int rc;
> +
> + ib_dev = acpi_driver_data(to_acpi_device(dev));
> +
> + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> + if (ACPI_FAILURE(rc))
> + dev_warn(dev, "SOCW(0) failed: %s\n",
> + acpi_format_exception(rc));
> +
> + return 0;
> +}
> +
> +static int appleib_resume(struct device *dev)
> +{
> + struct appleib_device *ib_dev;
> + int rc;
> +
> + ib_dev = acpi_driver_data(to_acpi_device(dev));
> +
> + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1);
> + if (ACPI_FAILURE(rc))
> + dev_warn(dev, "SOCW(1) failed: %s\n",
> + acpi_format_exception(rc));
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops appleib_pm = {
> + .suspend = appleib_suspend,
> + .resume = appleib_resume,
> + .restore = appleib_resume,
> +};
> +
> +static const struct acpi_device_id appleib_acpi_match[] = {
> + { "APP7777", 0 },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, appleib_acpi_match);
> +
> +static struct acpi_driver appleib_driver = {
> + .name = "apple-ibridge",
> + .class = "topcase", /* ? */
It's always nice to have a ? but what is the question?
> + .owner = THIS_MODULE,
> + .ids = appleib_acpi_match,
> + .ops = {
> + .add = appleib_probe,
> + .remove = appleib_remove,
> + },
> + .drv = {
> + .pm = &appleib_pm,
> + },
> +};
> +
> +module_acpi_driver(appleib_driver)
> +
> +MODULE_AUTHOR("Ronald Tschalär");
> +MODULE_DESCRIPTION("Apple iBridge driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index adce58f24f76..f963bb02c477 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -177,6 +177,7 @@
> #define USB_DEVICE_ID_APPLE_IRCONTROL3 0x8241
> #define USB_DEVICE_ID_APPLE_IRCONTROL4 0x8242
> #define USB_DEVICE_ID_APPLE_IRCONTROL5 0x8243
> +#define USB_DEVICE_ID_APPLE_IBRIDGE 0x8600
>
> #define USB_VENDOR_ID_ASUS 0x0486
> #define USB_DEVICE_ID_ASUS_T91MT 0x0185
> diff --git a/include/linux/apple-ibridge.h b/include/linux/apple-ibridge.h
> new file mode 100644
> index 000000000000..07ded8c68776
> --- /dev/null
> +++ b/include/linux/apple-ibridge.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Apple iBridge Driver
> + *
> + * Copyright (c) 2018 Ronald Tschalär
> + */
> +
> +#ifndef __LINUX_APPLE_IBRDIGE_H
> +#define __LINUX_APPLE_IBRDIGE_H
> +
> +#include <linux/hid.h>
> +
> +#define USB_VENDOR_ID_LINUX_FOUNDATION 0x1d6b
> +#define USB_DEVICE_ID_IBRIDGE_TB 0x0301
> +#define USB_DEVICE_ID_IBRIDGE_ALS 0x0302
> +
> +struct hid_field *appleib_find_report_field(struct hid_report *report,
> + unsigned int field_usage);
> +struct hid_field *appleib_find_hid_field(struct hid_device *hdev,
> + unsigned int application,
> + unsigned int field_usage);
> +
> +#endif
^ permalink raw reply
* [PATCH] HID: uclogic: Add support for Huion HS64 tablet
From: Kyle Godbey @ 2019-06-15 23:15 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input; +Cc: linux-kernel, Kyle Godbey
Add support for Huion HS64 drawing tablet to hid-uclogic
Signed-off-by: Kyle Godbey <me@kyle.ee>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-uclogic-core.c | 2 ++
drivers/hid/hid-uclogic-params.c | 2 ++
3 files changed, 5 insertions(+)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 84e0c78d73cd..86ed32dd18ca 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -569,6 +569,7 @@
#define USB_VENDOR_ID_HUION 0x256c
#define USB_DEVICE_ID_HUION_TABLET 0x006e
+#define USB_DEVICE_ID_HUION_HS64 0x006d
#define USB_VENDOR_ID_IBM 0x04b3
#define USB_DEVICE_ID_IBM_SCROLLPOINT_III 0x3100
diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
index 8fe02d81265d..914fb527ae7a 100644
--- a/drivers/hid/hid-uclogic-core.c
+++ b/drivers/hid/hid-uclogic-core.c
@@ -369,6 +369,8 @@ static const struct hid_device_id uclogic_devices[] = {
USB_DEVICE_ID_UCLOGIC_TABLET_TWHA60) },
{ HID_USB_DEVICE(USB_VENDOR_ID_HUION,
USB_DEVICE_ID_HUION_TABLET) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_HUION,
+ USB_DEVICE_ID_HUION_HS64) },
{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC,
USB_DEVICE_ID_HUION_TABLET) },
{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC,
diff --git a/drivers/hid/hid-uclogic-params.c b/drivers/hid/hid-uclogic-params.c
index 0187c9f8fc22..273d784fff66 100644
--- a/drivers/hid/hid-uclogic-params.c
+++ b/drivers/hid/hid-uclogic-params.c
@@ -977,6 +977,8 @@ int uclogic_params_init(struct uclogic_params *params,
/* FALL THROUGH */
case VID_PID(USB_VENDOR_ID_HUION,
USB_DEVICE_ID_HUION_TABLET):
+ case VID_PID(USB_VENDOR_ID_HUION,
+ USB_DEVICE_ID_HUION_HS64):
case VID_PID(USB_VENDOR_ID_UCLOGIC,
USB_DEVICE_ID_HUION_TABLET):
case VID_PID(USB_VENDOR_ID_UCLOGIC,
--
2.21.0
^ 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