Linux Input/HID development
 help / color / mirror / Atom feed
* Re: [PATCH 2/2 ] Surface Pro 2 HID sensor, wacom, keyboard/multitouch composite device
From: Derya @ 2014-03-07 15:58 UTC (permalink / raw)
  To: linux-input, benjamin.tissoires, jkosina
In-Reply-To: <5319EB7C.5070209@yahoo.de>

Enumeration quirks for Surface Pro 2 sensor-hub

Signed-off-by: Derya <derya.kiran@yahoo.de>
---
  drivers/hid/hid-sensor-hub.c | 9 +++++++++
  1 file changed, 9 insertions(+)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 9c22e14..16f4bb8 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -668,6 +668,15 @@ static const struct hid_device_id 
sensor_hub_devices[] = {
      { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_STM_0,
              USB_DEVICE_ID_STM_HID_SENSOR),
              .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
+    { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, 
USB_VENDOR_ID_MICROSOFT,
+             USB_DEVICE_ID_MS_TOUCH_COVER_2),
+            .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
+    { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, 
USB_VENDOR_ID_MICROSOFT,
+             USB_DEVICE_ID_MS_TYPE_COVER_2),
+            .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
+    { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, 
USB_VENDOR_ID_MICROSOFT,
+             USB_DEVICE_ID_MS_SURFACE_PRO_2),
+            .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
      { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, HID_ANY_ID,
               HID_ANY_ID) },
      { }
-- 
1.8.3.2

^ permalink raw reply related

* Re: [PATCH 0/2 ] Surface Pro 2 HID sensor, wacom, keyboard/multitouch composite device
From: Benjamin Tissoires @ 2014-03-07 16:51 UTC (permalink / raw)
  To: Derya; +Cc: linux-input, Benjamin Tissoires, Jiri Kosina
In-Reply-To: <5319EB7C.5070209@yahoo.de>

Hi Derya,

On Fri, Mar 7, 2014 at 10:53 AM, Derya <derya.kiran@yahoo.de> wrote:
> The MS Surface Pro 2 has a very annoying USB composite device on port 2.3.
> It has 3 interfaces:
> - Interface 0 is the sensor-hub
> - Interface 1 is the wacom digitizer² (it's one without finger input, there
> is an atmel digitizer on port 2.4 for finger input)
> - Interface 2 is the keyboard, if a keyboard cover is attached.

Oh... yes, indeed, it's gona be tricky :)

>
> This USB composite device changes it product id depending if and which
> keyboard cover is attached.
> Each of this hid devices contains several collections, this complicated
> everything.
> I have uploaded a lsusb output to: http://pastebin.com/Jun5sa2t

Thanks

>
> I have to say, that I'm neither a developer nor a programmer, this means
> this things are beyond my ken, please excuse if my remarks aren't accurate.

But you still made the patches, so you are a programmer :)

>
> The Touch and Type Covers (2) didn't work out of the box. Someone one the
> ubuntuforums made a patch which adds them to the microsoft special driver,
> but this leads to loading that driver for all the 3 interfaces and prevents
> loading of hid-sensor-hub for interface 0, if a keyboard cover is attached.
> Sadly this patch is submitted to the upstream kernel source.

Well, if it breaks things, it's still time to revert it.

> Without that patch the keyboard covers are loading hid-multitouch instead of
> hid-generic. This misbehaviour is a result of the fancy hid collections,
> that the keyboards have. With hid-multitouch neither the keyboard nor the
> touchpad of the cover works². I added an if clause to hid_scan_input_usage
> to prevent loading of hid-multitouch for the keyboards. With hid-generic
> keyboard and touchpad are working (they come up as one input)

More comment on that in the patch

>
> We also need the HID_QUIRK_NOGET for this usb composite device, without it
> hid-sensor-hub fails with a submit urb failure evertime a keyboard cover is
> (de)attached and it takes some seconds until the keyboard and wacom
> digitizer responds.

Can you test if HID_QUIRK_NO_INIT_INPUT_REPORTS works. It's a little
bit less a hammer than NO_GET, and I am pretty sure that this is what
Windows does by default.

>
> The second patch adds HID_SENSOR_HUB_ENUM_QUIRK for the Surface Pro 2's
> sensor-hub. There is still a bug with the sensors and the Surface Pro 2, but
> I didn't dig into it yet (hid-sensor-magn-3d fails to setup attributes)
>
> Regards,
>
> Derya
>
>
>
> 1 I'm also working to get the wacom driver working. At the moment the stylus
> works with hid-generic(if my patch is applied, without it use
> hid-microsoft). I got it working with wacom driver without disturbing the
> other interfaces, but the wacom interface contains also some fancy
> collection. The wacom driver doesn't care of them, this leads to losing the
> on device volume and left meta keys. With hid-generic they work, but the
> input events get distorted after the use of the eraser. But, this is another
> story...

For the record, I am tempted (and some people at wacom too) to switch
the Wacom devices to use hid-wacom instead of wacom.ko. This may solve
your problems here.

>
> 2 @Benjamin Tissoires
> I have tried your patches for fancy collection in hid-multitouch. It seems
> to be the right way to solve the problem with the keyboard (better than my
> approach to exclude this devices in hid_scan_input_usage), but it has the
> some drawbacks at the moment. It splits the input into 5 pieces. 2 seperate
> keyboard inputs that leads into losing the caps lock led. The touchpad is
> mapped as a mouse. There is also a consumer device, which gives me no input
> and an unkown device, but no multitouch device. There is a HID_DG_INPUTMODE
> out of range error in dmesg. Please, contact me, if you need some logs.

With the lsusb output I should be able to conduct more tests. But I
may not have time to do them.
Did you used the latest patch applied this week by Jiri?

Cheers,
Benjamin

>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 1/2 ] Surface Pro 2 HID sensor, wacom, keyboard/multitouch composite device
From: Benjamin Tissoires @ 2014-03-07 17:00 UTC (permalink / raw)
  To: Derya; +Cc: linux-input, Benjamin Tissoires, Jiri Kosina
In-Reply-To: <5319EC31.90202@yahoo.de>

On Fri, Mar 7, 2014 at 10:56 AM, Derya <derya.kiran@yahoo.de> wrote:
> The Surface Pro 2 has a very annoying USB composite device on port 2.3.
> It has 3 interfaces, interface 0 is the hid-sensor-hub, interface 1 the
> wacom
> digitizer and if a keyboard is attached, then it is on interface 3. This USB
> composite device changes it's product id, it depends on if and which
> keyboard
> cover is attached. Adding the Type-/Touch Cover 2 to hid_have_special_driver
> prevents loading the hid-sensor-hub driver for interface 0, if a cover is
> attached (commit 117309c51dca42121f70cacec801511b76acf75c).

This patch obviously reverts this commit and add different features.
I would be happy if you split it in two:
- the first one would be the revert (given by "git revert" with a
clear commit message) so we will have a clear view that this patch was
broken
- then you do the additions (you will have to re-add things in
hid-ids.h, but it's not a big deal).

> All 3 interfaces are loaded with the microsoft special driver. Interface 3,
> the keyboard covers, has a HID_DG_CONTACTID. This leads to loading of
> hid-multitouch for the keyboard, if we remove it from the special driver
> list.
> Neither the keyboard not the touchpad works with hid-multitouch.
> I add a check for vendor and product id in hid_scan_input_usage to prevent
> the loading of hid-multitouch. By this way it loads hid-generic for
> the keyboards without effecting the other two interfaces.
> The HID_QUIRK_NOGET is needed for hid-sensor-hub, without it Submit urb
> fails
> and it takes some seconds until the devices on this port get responsible
> whenever a keyboard cover is (de)attached.
>
> Signed-off-by: Derya <derya.kiran@yahoo.de>
> ---
>  drivers/hid/hid-core.c          | 5 ++---
>  drivers/hid/hid-ids.h           | 1 +
>  drivers/hid/hid-microsoft.c     | 4 ----
>  drivers/hid/usbhid/hid-quirks.c | 3 +++
>  4 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index cc32a6f..2d60a1a 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -685,7 +685,8 @@ static void hid_scan_input_usage(struct hid_parser
> *parser, u32 usage)
>      struct hid_device *hid = parser->device;
>
>      if (usage == HID_DG_CONTACTID)
> -        hid->group = HID_GROUP_MULTITOUCH;
> +        if (!(hid->vendor == USB_VENDOR_ID_MICROSOFT && (hid->product ==
> USB_DEVICE_ID_MS_TYPE_COVER_2 || hid->product ==
> USB_DEVICE_ID_MS_TOUCH_COVER_2)))
> +            hid->group = HID_GROUP_MULTITOUCH;

Please don't do such vendor specific tests here.

I'd be happy if you put at the end of hid_scan_report()

    /*
    * Handle vendor specific handlings
    */
    if ((hid->vendor == USB_VENDOR_ID_MICROSOFT) &&
        (hid->product == USB_DEVICE_ID_MS_TYPE_COVER_2 ||
         hid->product == USB_DEVICE_ID_MS_TOUCH_COVER_2) &&
        (hid->group != HID_GROUP_MULTITOUCH))
        hid->group = HID_GROUP_GENERIC;

this way, we can add other vendors quirks (who said Wacom? :-P )

>  }
>
>  static void hid_scan_feature_usage(struct hid_parser *parser, u32 usage)
> @@ -1780,8 +1781,6 @@ static const struct hid_device_id
> hid_have_special_driver[] = {
>      { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_PRESENTER_8K_USB) },
>      { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_DIGITAL_MEDIA_3K) },
>      { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_WIRELESS_OPTICAL_DESKTOP_3_0) },
> -    { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_TYPE_COVER_2) },
> -    { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,

this should be in the revert commit as mentioned

> USB_DEVICE_ID_MS_TOUCH_COVER_2) },
>      { HID_USB_DEVICE(USB_VENDOR_ID_MONTEREY, USB_DEVICE_ID_GENIUS_KB29E) },
>      { HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, USB_DEVICE_ID_NTRIG_TOUCH_SCREEN)
> },
>      { HID_USB_DEVICE(USB_VENDOR_ID_NTRIG,
> USB_DEVICE_ID_NTRIG_TOUCH_SCREEN_1) },
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 22f28d6..0aa9f7e 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -624,6 +624,7 @@
>  #define USB_DEVICE_ID_MS_PRESENTER_8K_USB    0x0713
>  #define USB_DEVICE_ID_MS_DIGITAL_MEDIA_3K    0x0730
>  #define USB_DEVICE_ID_MS_COMFORT_MOUSE_4500    0x076c
> +#define USB_DEVICE_ID_MS_SURFACE_PRO_2    0x0799        //Surface Pro 2's
> MICROSOFT SAM device without a keyboard cover attached, changes product id

please do not use "//" but /* ... */ for the comments.

Also, we are "limited" to 80 chars per line, so this comment should go
above the two declarations.

> with cover
>  #define USB_DEVICE_ID_MS_TOUCH_COVER_2    0x07a7
>  #define USB_DEVICE_ID_MS_TYPE_COVER_2    0x07a9
>
> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index 404a3a8..c6ef6ee 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c
> @@ -208,10 +208,6 @@ static const struct hid_device_id ms_devices[] = {
>          .driver_data = MS_NOGET },
>      { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_COMFORT_MOUSE_4500),
>          .driver_data = MS_DUPLICATE_USAGES },
> -    { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_TYPE_COVER_2),
> -        .driver_data = 0 },
> -    { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_TOUCH_COVER_2),
> -        .driver_data = 0 },

revert patch

>
>      { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> USB_DEVICE_ID_MS_PRESENTER_8K_BT),
>          .driver_data = MS_PRESENTER },
> diff --git a/drivers/hid/usbhid/hid-quirks.c
> b/drivers/hid/usbhid/hid-quirks.c
> index dbd8387..2b7fd92 100644
> --- a/drivers/hid/usbhid/hid-quirks.c
> +++ b/drivers/hid/usbhid/hid-quirks.c
> @@ -73,6 +73,9 @@ static const struct hid_blacklist {
>      { USB_VENDOR_ID_FORMOSA, USB_DEVICE_ID_FORMOSA_IR_RECEIVER,
> HID_QUIRK_NO_INIT_REPORTS },
>      { USB_VENDOR_ID_FREESCALE, USB_DEVICE_ID_FREESCALE_MX28,
> HID_QUIRK_NOGET },
>      { USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS, HID_QUIRK_NOGET },
> +    { USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_SURFACE_PRO_2,
> HID_QUIRK_NOGET },

please test HID_QUIRK_NO_INIT_INPUT_REPORTS

Good work!

Cheers,
Benjamin

> +    { USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_2,
> HID_QUIRK_NOGET },
> +    { USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TOUCH_COVER_2,
> HID_QUIRK_NOGET },
>      { USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GX680R_LED_PANEL,
> HID_QUIRK_NO_INIT_REPORTS },
>      { USB_VENDOR_ID_NEXIO, USB_DEVICE_ID_NEXIO_MULTITOUCH_PTI0750,
> HID_QUIRK_NO_INIT_REPORTS },
>      { USB_VENDOR_ID_NOVATEK, USB_DEVICE_ID_NOVATEK_MOUSE,
> HID_QUIRK_NO_INIT_REPORTS },
> --
> 1.8.3.2
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/2 ] Surface Pro 2 HID sensor, wacom, keyboard/multitouch composite device
From: Benjamin Tissoires @ 2014-03-07 17:01 UTC (permalink / raw)
  To: Derya; +Cc: linux-input, Benjamin Tissoires, Jiri Kosina
In-Reply-To: <5319EC8D.9070302@yahoo.de>

On Fri, Mar 7, 2014 at 10:58 AM, Derya <derya.kiran@yahoo.de> wrote:
> Enumeration quirks for Surface Pro 2 sensor-hub
>
> Signed-off-by: Derya <derya.kiran@yahoo.de>
> ---
>  drivers/hid/hid-sensor-hub.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 9c22e14..16f4bb8 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -668,6 +668,15 @@ static const struct hid_device_id sensor_hub_devices[]
> = {
>      { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_STM_0,
>              USB_DEVICE_ID_STM_HID_SENSOR),
>              .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
> +    { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB,
> USB_VENDOR_ID_MICROSOFT,
> +             USB_DEVICE_ID_MS_TOUCH_COVER_2),
> +            .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
> +    { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB,
> USB_VENDOR_ID_MICROSOFT,
> +             USB_DEVICE_ID_MS_TYPE_COVER_2),
> +            .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
> +    { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB,
> USB_VENDOR_ID_MICROSOFT,
> +             USB_DEVICE_ID_MS_SURFACE_PRO_2),
> +            .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},

this seems to be pretty heavy to have all three product id in the sensor hub.
Is it mandatory?

Cheers,
Benjamin

>      { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, HID_ANY_ID,
>               HID_ANY_ID) },
>      { }
> --
> 1.8.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [RFC V1 2/2] input: misc: da9063: OnKey driver
From: Dmitry Torokhov @ 2014-03-07 17:46 UTC (permalink / raw)
  To: Opensource [Steve Twiss]; +Cc: David Dajun Chen, LKML-INPUT, LKML-KERNEL
In-Reply-To: <ce5a43ad4c028b1553206ee51a27822a614f3404.1394195865.git.stwiss.opensource@diasemi.com>

Hi Steve,

On Fri, Mar 07, 2014 at 12:37:45PM +0000, Opensource [Steve Twiss] wrote:
> From: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
> 
> Add the OnKey driver for DA9063.
> 
> 
> Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
> ---
> Checks performed with next-20140307/scripts/checkpatch.pl
>  da9063-onkey.c            total: 0 errors, 0 warnings, 202 lines checked
>  Kconfig                   total: 0 errors, 11 warnings, 679 lines checked
>  Makefile                  total: 0 errors, 0 warnings, 66 lines checked
> 
> Hello,
> 
> This is a RFC for the Dialog DA9063 Onkey driver.
> 
> Dependencies:
> 
> This driver makes use of the name field "ONKEY" as part of the
> function call: platform_get_irq_byname();
> 
> This driver is therefore dependent on this change to the name field
> in the properties of the the OnKey IORESOURCE_IRQ resource structure
> (part of the mfd_cell Onkey resource inside da9063-core.c). This
> change will be added as part of the final set when I submit all patches
> as a single PATCH set.
> 
> This patch applies against kernel version linux-next next-20140307
>  
> Regards,
> Steve Twiss, Dialog Semiconductor Ltd.
> 
> 
> 
>  drivers/input/misc/Kconfig        |   10 ++
>  drivers/input/misc/Makefile       |    1 +
>  drivers/input/misc/da9063-onkey.c |  202 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 213 insertions(+)
>  create mode 100644 drivers/input/misc/da9063-onkey.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 762e6d2..3deb008 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -522,6 +522,16 @@ config INPUT_DA9055_ONKEY
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called da9055_onkey.
>  
> +config INPUT_DA9063_ONKEY
> +	tristate "Dialog DA9063 OnKey"
> +	depends on MFD_DA9063
> +	help
> +	  Support the ONKEY of Dialog DA9063 Power Management IC as an
> +	  input device reporting power button statue.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called da9063-onkey.
> +
>  config INPUT_DM355EVM
>  	tristate "TI DaVinci DM355 EVM Keypad and IR Remote"
>  	depends on MFD_DM355EVM_MSP
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index cda71fc..f40caa7 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_INPUT_CMA3000_I2C)		+= cma3000_d0x_i2c.o
>  obj-$(CONFIG_INPUT_COBALT_BTNS)		+= cobalt_btns.o
>  obj-$(CONFIG_INPUT_DA9052_ONKEY)	+= da9052_onkey.o
>  obj-$(CONFIG_INPUT_DA9055_ONKEY)	+= da9055_onkey.o
> +obj-$(CONFIG_INPUT_DA9063_ONKEY)	+= da9063-onkey.o
>  obj-$(CONFIG_INPUT_DM355EVM)		+= dm355evm_keys.o
>  obj-$(CONFIG_INPUT_GP2A)		+= gp2ap002a00f.o
>  obj-$(CONFIG_INPUT_GPIO_BEEPER)		+= gpio-beeper.o
> diff --git a/drivers/input/misc/da9063-onkey.c b/drivers/input/misc/da9063-onkey.c
> new file mode 100644
> index 0000000..654f52b
> --- /dev/null
> +++ b/drivers/input/misc/da9063-onkey.c
> @@ -0,0 +1,202 @@
> +/* da9063-onkey.c - Onkey device driver for DA9063
> + * Copyright (C) 2013  Dialog Semiconductor Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/workqueue.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/da9063/core.h>
> +#include <linux/mfd/da9063/pdata.h>
> +#include <linux/mfd/da9063/registers.h>
> +
> +struct da9063_onkey {
> +	struct	da9063 *hw;
> +	struct delayed_work work;
> +	struct	input_dev *input;
> +	bool key_power;
> +};
> +
> +static void da9063_poll_on(struct work_struct *work)
> +{
> +	struct da9063_onkey *onkey = container_of(work, struct da9063_onkey,
> +						  work.work);
> +	unsigned int val;
> +	int poll = 1;

Please use booleans for boolean data:

	bool poll = true;

> +	int ret;
> +
> +	/* poll to see when the pin is deasserted */
> +	ret = regmap_read(onkey->hw->regmap, DA9063_REG_STATUS_A, &val);
> +	if (ret < 0) {
> +		dev_err(&onkey->input->dev,
> +			"Failed to read ON status: %d\n", ret);
> +		goto err_poll;
> +	}
> +
> +	if (!(val & DA9063_NONKEY)) {
> +		ret = regmap_update_bits(onkey->hw->regmap,
> +					 DA9063_REG_CONTROL_B,
> +					 DA9063_NONKEY_LOCK, 0);
> +		if (ret < 0) {
> +			dev_err(&onkey->input->dev,
> +				"Failed to reset the Key Delay %d\n", ret);
> +			goto err_poll;
> +		}
> +
> +		input_report_key(onkey->input, KEY_POWER, 0);
> +		input_sync(onkey->input);

This is confusing. Why we report key only if we successfully did the
first write but we do not care about the second?

> +
> +		/* unmask the onkey interrupt again */
> +		ret = regmap_update_bits(onkey->hw->regmap,
> +					 DA9063_REG_IRQ_MASK_A,
> +					 DA9063_NONKEY, 0);
> +		if (ret < 0) {
> +			dev_err(&onkey->input->dev,
> +				"Failed to unmask the onkey IRQ: %d\n", ret);
> +			goto err_poll;
> +		}
> +
> +		poll = 0;

		poll = false;

> +	}
> +
> +err_poll:
> +	if (poll)
> +		schedule_delayed_work(&onkey->work, 50);
> +}
> +
> +static irqreturn_t da9063_onkey_irq_handler(int irq, void *data)
> +{
> +	struct da9063_onkey *onkey = data;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(onkey->hw->regmap, DA9063_REG_STATUS_A, &val);
> +	if (onkey->key_power && (ret >= 0) && (val & DA9063_NONKEY)) {
> +		ret = regmap_update_bits(onkey->hw->regmap,
> +					 DA9063_REG_IRQ_MASK_A,
> +					 DA9063_NONKEY, 1);
> +		if (ret < 0)
> +			dev_err(&onkey->input->dev,
> +				"Failed to mask the onkey IRQ: %d\n", ret);
> +
> +		input_report_key(onkey->input, KEY_POWER, 1);
> +		input_sync(onkey->input);
> +
> +		schedule_delayed_work(&onkey->work, 0);
> +		dev_dbg(&onkey->input->dev, "KEY_POWER pressed.\n");
> +	} else {
> +		input_report_key(onkey->input, KEY_SLEEP, 1);

You need input_sync()  here as well.

> +		input_report_key(onkey->input, KEY_SLEEP, 0);
> +		input_sync(onkey->input);
> +		dev_dbg(&onkey->input->dev, "KEY_SLEEP pressed.\n");
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int da9063_onkey_probe(struct platform_device *pdev)
> +{
> +	struct da9063 *da9063 = dev_get_drvdata(pdev->dev.parent);
> +	struct da9063_pdata *pdata = dev_get_platdata(da9063->dev);
> +	struct da9063_onkey *onkey;
> +	bool kp_tmp = true;
> +	int ret = 0;
> +	int irq;
> +
> +	if (pdata)
> +		kp_tmp = pdata->key_power;
> +
> +	if (!kp_tmp)
> +		dev_err(&pdev->dev,
> +			"Software power down key is not set.\n");

Why is this an error?

> +
> +	onkey = devm_kzalloc(&pdev->dev, sizeof(struct da9063_onkey),
> +			     GFP_KERNEL);
> +	if (!onkey) {
> +		dev_err(&pdev->dev, "Failed to allocate memory.\n");
> +		return -ENOMEM;
> +	}
> +
> +	INIT_DELAYED_WORK(&onkey->work, da9063_poll_on);
> +
> +	onkey->input = devm_input_allocate_device(&pdev->dev);
> +	if (!onkey->input) {
> +		dev_err(&pdev->dev, "Failed to allocated input device.\n");
> +		return -ENOMEM;
> +	}
> +
> +	irq = platform_get_irq_byname(pdev, "ONKEY");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get platform IRQ.\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +					da9063_onkey_irq_handler,
> +					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +					"ONKEY", onkey);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Failed to request input device IRQ.\n");
> +		return ret;
> +	}
> +
> +	onkey->hw = da9063;
> +	onkey->key_power = kp_tmp;
> +	onkey->input->evbit[0] = BIT_MASK(EV_KEY);
> +	onkey->input->name = DA9063_DRVNAME_ONKEY;
> +	onkey->input->phys = DA9063_DRVNAME_ONKEY "/input0";
> +	onkey->input->dev.parent = &pdev->dev;
> +
> +	if (onkey->key_power)
> +		input_set_capability(onkey->input, EV_KEY, KEY_POWER);
> +	input_set_capability(onkey->input, EV_KEY, KEY_SLEEP);
> +
> +	ret = input_register_device(onkey->input);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Failed to register input device.\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, onkey);
> +	return ret;
> +}
> +
> +static int da9063_onkey_remove(struct platform_device *pdev)
> +{
> +	struct	da9063_onkey *onkey = platform_get_drvdata(pdev);
> +	cancel_delayed_work_sync(&onkey->work);

This is racy. Nothing stops IRQ from firing again and rescheduling the
work item. You are also missing canceling work item in error path pf
probe().

> +	input_unregister_device(onkey->input);
> +	return 0;
> +}
> +
> +static struct platform_driver da9063_onkey_driver = {
> +	.probe	= da9063_onkey_probe,
> +	.remove	= da9063_onkey_remove,
> +	.driver	= {
> +		.name	= DA9063_DRVNAME_ONKEY,
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +module_platform_driver(da9063_onkey_driver);
> +
> +MODULE_AUTHOR("S Twiss <stwiss.opensource@diasemi.com>");
> +MODULE_DESCRIPTION("Onkey device driver for Dialog DA9063");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" DA9063_DRVNAME_ONKEY);
> -- 
> end-of-patch for RFC V1
> 

Thanks.

-- 
Dmitry

^ permalink raw reply

* Re: [PATCH] input: Use platform-provided devices as i8042 serio parents
From: Dmitry Torokhov @ 2014-03-07 17:54 UTC (permalink / raw)
  To: Matthew Garrett; +Cc: linux-input, linux-kernel, benjamin.tissoires
In-Reply-To: <1393174996-27764-1-git-send-email-matthew.garrett@nebula.com>

Hi Matthew,

On Sun, Feb 23, 2014 at 12:03:16PM -0500, Matthew Garrett wrote:
> i8042 devices exposed via platform firmware interfaces such as ACPI or
> Device Tree may provide additional information of use to userspace. Right
> now we don't associate the serio devices with the firmware device, and so
> there's no straightforward way for userspace to make use of that
> information. This patch simply moves the serio parent device to the firmware
> provided device.

Hmm, are we sure it will not mess up power management now that children
serio ports are disconnected from i8042 device?

I also wonder if we need to restructure the whole thing so that we
create AUX and KBD ports separately and convert i8042 code into a
library of sorts...

Thanks.

-- 
Dmitry

^ permalink raw reply

* Re: [PATCH 0/2 ] Surface Pro 2 HID sensor, wacom, keyboard/multitouch composite device
From: Benjamin Tissoires @ 2014-03-07 17:56 UTC (permalink / raw)
  To: Derya; +Cc: linux-input, Benjamin Tissoires, Jiri Kosina
In-Reply-To: <531A06FC.4030906@yahoo.de>

On Fri, Mar 7, 2014 at 12:50 PM, Derya <derya.kiran@yahoo.de> wrote:
>
> Am 07.03.2014 17:51, schrieb Benjamin Tissoires:
>
>> Hi Derya,
>>
>> On Fri, Mar 7, 2014 at 10:53 AM, Derya <derya.kiran@yahoo.de> wrote:
>>>
>>> The MS Surface Pro 2 has a very annoying USB composite device on port
>>> 2.3.
>>> It has 3 interfaces:
>>> - Interface 0 is the sensor-hub
>>> - Interface 1 is the wacom digitizer² (it's one without finger input,
>>> there
>>> is an atmel digitizer on port 2.4 for finger input)
>>> - Interface 2 is the keyboard, if a keyboard cover is attached.
>>
>> Oh... yes, indeed, it's gona be tricky :)
>>
>>> This USB composite device changes it product id depending if and which
>>> keyboard cover is attached.
>>> Each of this hid devices contains several collections, this complicated
>>> everything.
>>> I have uploaded a lsusb output to: http://pastebin.com/Jun5sa2t
>>
>> Thanks
>>
>>> I have to say, that I'm neither a developer nor a programmer, this means
>>> this things are beyond my ken, please excuse if my remarks aren't
>>> accurate.
>>
>> But you still made the patches, so you are a programmer :)
>>
>>> The Touch and Type Covers (2) didn't work out of the box. Someone one the
>>> ubuntuforums made a patch which adds them to the microsoft special
>>> driver,
>>> but this leads to loading that driver for all the 3 interfaces and
>>> prevents
>>> loading of hid-sensor-hub for interface 0, if a keyboard cover is
>>> attached.
>>> Sadly this patch is submitted to the upstream kernel source.
>>
>> Well, if it breaks things, it's still time to revert it.
>>
>>> Without that patch the keyboard covers are loading hid-multitouch instead
>>> of
>>> hid-generic. This misbehaviour is a result of the fancy hid collections,
>>> that the keyboards have. With hid-multitouch neither the keyboard nor the
>>> touchpad of the cover works². I added an if clause to
>>> hid_scan_input_usage
>>> to prevent loading of hid-multitouch for the keyboards. With hid-generic
>>> keyboard and touchpad are working (they come up as one input)
>>
>> More comment on that in the patch
>
>
> I will follow your comments and update the patch.
>
>>
>>> We also need the HID_QUIRK_NOGET for this usb composite device, without
>>> it
>>> hid-sensor-hub fails with a submit urb failure evertime a keyboard cover
>>> is
>>> (de)attached and it takes some seconds until the keyboard and wacom
>>> digitizer responds.
>>
>> Can you test if HID_QUIRK_NO_INIT_INPUT_REPORTS works. It's a little
>> bit less a hammer than NO_GET, and I am pretty sure that this is what
>> Windows does by default.
>>
>
> Sure, I can give it a try :-)
>
>>> The second patch adds HID_SENSOR_HUB_ENUM_QUIRK for the Surface Pro 2's
>>> sensor-hub. There is still a bug with the sensors and the Surface Pro 2,
>>> but
>>> I didn't dig into it yet (hid-sensor-magn-3d fails to setup attributes)
>>>
>>> Regards,
>>>
>>> Derya
>>>
>>>
>>>
>>> 1 I'm also working to get the wacom driver working. At the moment the
>>> stylus
>>> works with hid-generic(if my patch is applied, without it use
>>> hid-microsoft). I got it working with wacom driver without disturbing the
>>> other interfaces, but the wacom interface contains also some fancy
>>> collection. The wacom driver doesn't care of them, this leads to losing
>>> the
>>> on device volume and left meta keys. With hid-generic they work, but the
>>> input events get distorted after the use of the eraser. But, this is
>>> another
>>> story...
>>
>> For the record, I am tempted (and some people at wacom too) to switch
>> the Wacom devices to use hid-wacom instead of wacom.ko. This may solve
>> your problems here.
>
> I've looked into hid-wacom and thought, it would be better to use it instead
> of wacom.ko, but then I saw it contains only wireless device. It seemed to
> be the wrong place. The other thing is, if I use hid-wacom, then I have to
> add that USB composite device again to the special driver list, like with
> hid-microsoft, and this leads us to the same problem. It would load
> hid-wacom for all 3 interfaces. Or am I wrong? With wacom.ko I'm using the
> macro USB_DEVICE_INTERFACE_NUMBER (instead of USB_DEVICE and
> USB_DEVICE_AND_INTERFACE_INFO, which are used for other wacon devices) to
> applied the driver only to the wacom interface without disturbing the other
> ones.
> I will try to use hid-wacom, but this will take some time.
>

You can, but you will loose your time I think. I just mentioned that
because I mainly wanted to warn Jiri and others on the list that some
people are thinking of it.
But As you said there is not the thing you need currently for this
device, so if you can manage to use wacom.ko, that's fine.

>
>>> 2 @Benjamin Tissoires
>>> I have tried your patches for fancy collection in hid-multitouch. It
>>> seems
>>> to be the right way to solve the problem with the keyboard (better than
>>> my
>>> approach to exclude this devices in hid_scan_input_usage), but it has the
>>> some drawbacks at the moment. It splits the input into 5 pieces. 2
>>> seperate
>>> keyboard inputs that leads into losing the caps lock led. The touchpad is
>>> mapped as a mouse. There is also a consumer device, which gives me no
>>> input
>>> and an unkown device, but no multitouch device. There is a
>>> HID_DG_INPUTMODE
>>> out of range error in dmesg. Please, contact me, if you need some logs.
>>
>> With the lsusb output I should be able to conduct more tests. But I
>> may not have time to do them.
>> Did you used the latest patch applied this week by Jiri?
>
>
> I tried it with this http://www.spinics.net/lists/linux-input/msg30018.html
> patch set.

Alright, this is the last one which has been applied by Jiri.
I'll definitively have to check this out them :(

Cheers,
Benjamin

>
>>
>> Cheers,
>> Benjamin
>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 0/2 ] Surface Pro 2 HID sensor, wacom, keyboard/multitouch composite device
From: Derya @ 2014-03-07 17:50 UTC (permalink / raw)
  To: Benjamin Tissoires; +Cc: linux-input, Benjamin Tissoires, Jiri Kosina
In-Reply-To: <CAN+gG=GdRwOkZsNWT9H1tRtjTy9bMDkKv1KiMe0tEcThOAvEQA@mail.gmail.com>


Am 07.03.2014 17:51, schrieb Benjamin Tissoires:
> Hi Derya,
>
> On Fri, Mar 7, 2014 at 10:53 AM, Derya <derya.kiran@yahoo.de> wrote:
>> The MS Surface Pro 2 has a very annoying USB composite device on port 2.3.
>> It has 3 interfaces:
>> - Interface 0 is the sensor-hub
>> - Interface 1 is the wacom digitizer² (it's one without finger input, there
>> is an atmel digitizer on port 2.4 for finger input)
>> - Interface 2 is the keyboard, if a keyboard cover is attached.
> Oh... yes, indeed, it's gona be tricky :)
>
>> This USB composite device changes it product id depending if and which
>> keyboard cover is attached.
>> Each of this hid devices contains several collections, this complicated
>> everything.
>> I have uploaded a lsusb output to: http://pastebin.com/Jun5sa2t
> Thanks
>
>> I have to say, that I'm neither a developer nor a programmer, this means
>> this things are beyond my ken, please excuse if my remarks aren't accurate.
> But you still made the patches, so you are a programmer :)
>
>> The Touch and Type Covers (2) didn't work out of the box. Someone one the
>> ubuntuforums made a patch which adds them to the microsoft special driver,
>> but this leads to loading that driver for all the 3 interfaces and prevents
>> loading of hid-sensor-hub for interface 0, if a keyboard cover is attached.
>> Sadly this patch is submitted to the upstream kernel source.
> Well, if it breaks things, it's still time to revert it.
>
>> Without that patch the keyboard covers are loading hid-multitouch instead of
>> hid-generic. This misbehaviour is a result of the fancy hid collections,
>> that the keyboards have. With hid-multitouch neither the keyboard nor the
>> touchpad of the cover works². I added an if clause to hid_scan_input_usage
>> to prevent loading of hid-multitouch for the keyboards. With hid-generic
>> keyboard and touchpad are working (they come up as one input)
> More comment on that in the patch

I will follow your comments and update the patch.
>
>> We also need the HID_QUIRK_NOGET for this usb composite device, without it
>> hid-sensor-hub fails with a submit urb failure evertime a keyboard cover is
>> (de)attached and it takes some seconds until the keyboard and wacom
>> digitizer responds.
> Can you test if HID_QUIRK_NO_INIT_INPUT_REPORTS works. It's a little
> bit less a hammer than NO_GET, and I am pretty sure that this is what
> Windows does by default.
>

Sure, I can give it a try :-)
>> The second patch adds HID_SENSOR_HUB_ENUM_QUIRK for the Surface Pro 2's
>> sensor-hub. There is still a bug with the sensors and the Surface Pro 2, but
>> I didn't dig into it yet (hid-sensor-magn-3d fails to setup attributes)
>>
>> Regards,
>>
>> Derya
>>
>>
>>
>> 1 I'm also working to get the wacom driver working. At the moment the stylus
>> works with hid-generic(if my patch is applied, without it use
>> hid-microsoft). I got it working with wacom driver without disturbing the
>> other interfaces, but the wacom interface contains also some fancy
>> collection. The wacom driver doesn't care of them, this leads to losing the
>> on device volume and left meta keys. With hid-generic they work, but the
>> input events get distorted after the use of the eraser. But, this is another
>> story...
> For the record, I am tempted (and some people at wacom too) to switch
> the Wacom devices to use hid-wacom instead of wacom.ko. This may solve
> your problems here.
I've looked into hid-wacom and thought, it would be better to use it 
instead of wacom.ko, but then I saw it contains only wireless device. It 
seemed to be the wrong place. The other thing is, if I use hid-wacom, 
then I have to add that USB composite device again to the special driver 
list, like with hid-microsoft, and this leads us to the same problem. It 
would load hid-wacom for all 3 interfaces. Or am I wrong? With wacom.ko 
I'm using the macro USB_DEVICE_INTERFACE_NUMBER (instead of USB_DEVICE 
and USB_DEVICE_AND_INTERFACE_INFO, which are used for other wacon 
devices) to applied the driver only to the wacom interface without 
disturbing the other ones.
I will try to use hid-wacom, but this will take some time.

>> 2 @Benjamin Tissoires
>> I have tried your patches for fancy collection in hid-multitouch. It seems
>> to be the right way to solve the problem with the keyboard (better than my
>> approach to exclude this devices in hid_scan_input_usage), but it has the
>> some drawbacks at the moment. It splits the input into 5 pieces. 2 seperate
>> keyboard inputs that leads into losing the caps lock led. The touchpad is
>> mapped as a mouse. There is also a consumer device, which gives me no input
>> and an unkown device, but no multitouch device. There is a HID_DG_INPUTMODE
>> out of range error in dmesg. Please, contact me, if you need some logs.
> With the lsusb output I should be able to conduct more tests. But I
> may not have time to do them.
> Did you used the latest patch applied this week by Jiri?

I tried it with this 
http://www.spinics.net/lists/linux-input/msg30018.html patch set.
>
> Cheers,
> Benjamin
>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/2 ] Surface Pro 2 HID sensor, wacom, keyboard/multitouch composite device
From: Derya @ 2014-03-07 17:57 UTC (permalink / raw)
  To: Benjamin Tissoires; +Cc: linux-input, Benjamin Tissoires, Jiri Kosina
In-Reply-To: <CAN+gG=EpfxeOGvUUQvLxRL9o_SWSmRJkuD7qDc6JkUz1aP_GNw@mail.gmail.com>


Am 07.03.2014 18:01, schrieb Benjamin Tissoires:
> On Fri, Mar 7, 2014 at 10:58 AM, Derya <derya.kiran@yahoo.de> wrote:
>> Enumeration quirks for Surface Pro 2 sensor-hub
>>
>> Signed-off-by: Derya <derya.kiran@yahoo.de>
>> ---
>>   drivers/hid/hid-sensor-hub.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
>> index 9c22e14..16f4bb8 100644
>> --- a/drivers/hid/hid-sensor-hub.c
>> +++ b/drivers/hid/hid-sensor-hub.c
>> @@ -668,6 +668,15 @@ static const struct hid_device_id sensor_hub_devices[]
>> = {
>>       { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_STM_0,
>>               USB_DEVICE_ID_STM_HID_SENSOR),
>>               .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
>> +    { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB,
>> USB_VENDOR_ID_MICROSOFT,
>> +             USB_DEVICE_ID_MS_TOUCH_COVER_2),
>> +            .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
>> +    { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB,
>> USB_VENDOR_ID_MICROSOFT,
>> +             USB_DEVICE_ID_MS_TYPE_COVER_2),
>> +            .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
>> +    { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB,
>> USB_VENDOR_ID_MICROSOFT,
>> +             USB_DEVICE_ID_MS_SURFACE_PRO_2),
>> +            .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
> this seems to be pretty heavy to have all three product id in the sensor hub.
> Is it mandatory?

We can post prune this until I have the time to have a closer look into 
the sensor stuff. The problem is the product id changes with the covers. 
We can use HID_ANY_ID instead of the product id, but that will apply to 
other MS devices, too. There are more MS Covers out there, I don't know 
how the others behave and I don't know, if the Surface Pro 1 needs it also.
>
> Cheers,
> Benjamin
>
>>       { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, HID_ANY_ID,
>>                HID_ANY_ID) },
>>       { }
>> --
>> 1.8.3.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


^ permalink raw reply

* Re: [PATCH 1/7] mfd: AXP20x: Add mfd driver for AXP20x PMIC
From: Maxime Ripard @ 2014-03-07 18:09 UTC (permalink / raw)
  To: Carlo Caione
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	hdegoede-H+wXaHxf7aLQT0dZR+AlfA, emilio-0Z03zUJReD5OxF6Tv1QG9Q,
	wens-jdAy2FN1RRM, sameo-VuQAYsv1563Yd54FQh9/CA,
	lee.jones-QSEj5FYQhm4dnm+yROfE0A,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	lgirdwood-Re5JQEeQqe8AvxtiuMwx3w, broonie-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <1393692352-10839-2-git-send-email-carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 16724 bytes --]

Hi,

On Sat, Mar 01, 2014 at 05:45:46PM +0100, Carlo Caione wrote:
> This patch introduces the preliminary support for PMICs X-Powers AXP202
> and AXP209. The AXP209 and AXP202 are the PMUs (Power Management Unit)
> used by A10, A13 and A20 SoCs and developed by X-Powers, a sister company
> of Allwinner.
> 
> The core enables support for two subsystems:
> - PEK (Power Enable Key)
> - Regulators
> 
> Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm/configs/sunxi_defconfig |   1 +
>  drivers/mfd/Kconfig              |  12 ++
>  drivers/mfd/Makefile             |   1 +
>  drivers/mfd/axp20x.c             | 250 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/axp20x.h       | 180 ++++++++++++++++++++++++++++
>  5 files changed, 444 insertions(+)
>  create mode 100644 drivers/mfd/axp20x.c
>  create mode 100644 include/linux/mfd/axp20x.h
> 
> diff --git a/arch/arm/configs/sunxi_defconfig b/arch/arm/configs/sunxi_defconfig
> index 3e2259b..f8aa7e6 100644
> --- a/arch/arm/configs/sunxi_defconfig
> +++ b/arch/arm/configs/sunxi_defconfig
> @@ -52,6 +52,7 @@ CONFIG_GPIO_SYSFS=y
>  # CONFIG_HWMON is not set
>  CONFIG_WATCHDOG=y
>  CONFIG_SUNXI_WATCHDOG=y
> +CONFIG_MFD_AXP20X=y
>  # CONFIG_USB_SUPPORT is not set
>  CONFIG_NEW_LEDS=y
>  CONFIG_LEDS_CLASS=y

Please provide a separate patch for both sunxi_defconfig and
multi_v7_defconfig.

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index dd67158..33d38c4 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -59,6 +59,18 @@ config MFD_AAT2870_CORE
>  	  additional drivers must be enabled in order to use the
>  	  functionality of the device.
>  
> +config MFD_AXP20X
> +	bool "X-Powers AXP20X"
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	depends on I2C=y
> +	help
> +	  If you say Y here you get support for the AXP20X.
> +	  This driver provides common support for accessing the device,
> +	  additional drivers must be enabled in order to use the
> +	  functionality of the device.
> +
>  config MFD_CROS_EC
>  	tristate "ChromeOS Embedded Controller"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 8a28dc9..371020e 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_PMIC_DA9052)	+= da9052-irq.o
>  obj-$(CONFIG_PMIC_DA9052)	+= da9052-core.o
>  obj-$(CONFIG_MFD_DA9052_SPI)	+= da9052-spi.o
>  obj-$(CONFIG_MFD_DA9052_I2C)	+= da9052-i2c.o
> +obj-$(CONFIG_MFD_AXP20X)	+= axp20x.o
>  
>  obj-$(CONFIG_MFD_LP8788)	+= lp8788.o lp8788-irq.o
>  
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> new file mode 100644
> index 0000000..92e5b0f
> --- /dev/null
> +++ b/drivers/mfd/axp20x.c
> @@ -0,0 +1,250 @@
> +/*
> + * axp20x.c - mfd core driver for the X-Powers AXP202 and AXP209
> + *
> + * Author: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/mfd/axp20x.h>
> +#include <linux/mfd/core.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +
> +#define AXP20X_OFF	0x80
> +
> +static const struct regmap_range axp20x_writeable_ranges[] = {
> +	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
> +	regmap_reg_range(AXP20X_DCDC_MODE, AXP20X_FG_RES),
> +};
> +
> +static const struct regmap_range axp20x_volatile_ranges[] = {
> +	regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IRQ5_STATE),
> +};
> +
> +static const struct regmap_access_table axp20x_writeable_table = {
> +	.yes_ranges	= axp20x_writeable_ranges,
> +	.n_yes_ranges	= ARRAY_SIZE(axp20x_writeable_ranges),
> +};
> +
> +static const struct regmap_access_table axp20x_volatile_table = {
> +	.yes_ranges	= axp20x_volatile_ranges,
> +	.n_yes_ranges	= ARRAY_SIZE(axp20x_volatile_ranges),
> +};
> +
> +static struct resource axp20x_pek_resources[] = {
> +	{
> +		.name	= "PEK_DBR",
> +		.start	= AXP20X_IRQ_PEK_RIS_EDGE,
> +		.end	= AXP20X_IRQ_PEK_RIS_EDGE,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +	{
> +		.name	= "PEK_DBF",
> +		.start	= AXP20X_IRQ_PEK_FAL_EDGE,
> +		.end	= AXP20X_IRQ_PEK_FAL_EDGE,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static const struct regmap_config axp20x_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.wr_table	= &axp20x_writeable_table,
> +	.volatile_table	= &axp20x_volatile_table,
> +	.max_register	= AXP20X_FG_RES,
> +	.cache_type	= REGCACHE_RBTREE,
> +};
> +
> +#define AXP20X_IRQ(_irq, _off, _mask) \
> +	[AXP20X_IRQ_##_irq] = { .reg_offset = (_off), .mask = BIT(_mask) }
> +
> +static const struct regmap_irq axp20x_regmap_irqs[] = {
> +	AXP20X_IRQ(ACIN_OVER_V,		0, 7),
> +	AXP20X_IRQ(ACIN_PLUGIN,		0, 6),
> +	AXP20X_IRQ(ACIN_REMOVAL,	0, 5),
> +	AXP20X_IRQ(VBUS_OVER_V,		0, 4),
> +	AXP20X_IRQ(VBUS_PLUGIN,		0, 3),
> +	AXP20X_IRQ(VBUS_REMOVAL,	0, 2),
> +	AXP20X_IRQ(VBUS_V_LOW,		0, 1),
> +	AXP20X_IRQ(BATT_PLUGIN,		1, 7),
> +	AXP20X_IRQ(BATT_REMOVAL,	1, 6),
> +	AXP20X_IRQ(BATT_ENT_ACT_MODE,	1, 5),
> +	AXP20X_IRQ(BATT_EXIT_ACT_MODE,	1, 4),
> +	AXP20X_IRQ(CHARG,		1, 3),
> +	AXP20X_IRQ(CHARG_DONE,		1, 2),
> +	AXP20X_IRQ(BATT_TEMP_HIGH,	1, 1),
> +	AXP20X_IRQ(BATT_TEMP_LOW,	1, 0),
> +	AXP20X_IRQ(DIE_TEMP_HIGH,	2, 7),
> +	AXP20X_IRQ(CHARG_I_LOW,		2, 6),
> +	AXP20X_IRQ(DCDC1_V_LONG,	2, 5),
> +	AXP20X_IRQ(DCDC2_V_LONG,	2, 4),
> +	AXP20X_IRQ(DCDC3_V_LONG,	2, 3),
> +	AXP20X_IRQ(PEK_SHORT,		2, 1),
> +	AXP20X_IRQ(PEK_LONG,		2, 0),
> +	AXP20X_IRQ(N_OE_PWR_ON,		3, 7),
> +	AXP20X_IRQ(N_OE_PWR_OFF,	3, 6),
> +	AXP20X_IRQ(VBUS_VALID,		3, 5),
> +	AXP20X_IRQ(VBUS_NOT_VALID,	3, 4),
> +	AXP20X_IRQ(VBUS_SESS_VALID,	3, 3),
> +	AXP20X_IRQ(VBUS_SESS_END,	3, 2),
> +	AXP20X_IRQ(LOW_PWR_LVL1,	3, 1),
> +	AXP20X_IRQ(LOW_PWR_LVL2,	3, 0),
> +	AXP20X_IRQ(TIMER,		4, 7),
> +	AXP20X_IRQ(PEK_RIS_EDGE,	4, 6),
> +	AXP20X_IRQ(PEK_FAL_EDGE,	4, 5),
> +	AXP20X_IRQ(GPIO3_INPUT,		4, 3),
> +	AXP20X_IRQ(GPIO2_INPUT,		4, 2),
> +	AXP20X_IRQ(GPIO1_INPUT,		4, 1),
> +	AXP20X_IRQ(GPIO0_INPUT,		4, 0),
> +};
> +
> +static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
> +	.name			= "axp20x_irq_chip",
> +	.status_base		= AXP20X_IRQ1_STATE,
> +	.ack_base		= AXP20X_IRQ1_STATE,
> +	.mask_base		= AXP20X_IRQ1_EN,
> +	.num_regs		= 5,
> +	.irqs			= axp20x_regmap_irqs,
> +	.num_irqs		= ARRAY_SIZE(axp20x_regmap_irqs),
> +	.mask_invert		= true,
> +	.init_ack_masked	= true,
> +};
> +
> +static struct mfd_cell axp20x_cells[] = {
> +	{
> +		.name		= "axp20x-pek",
> +		.of_compatible	= "x-powers,axp20x-pek",

You need to add the x-powers prefix to
Documentation/devicetree/bindings/vendor-prefixes.txt, preferably in a
separate patch.

> +		.num_resources	= ARRAY_SIZE(axp20x_pek_resources),
> +		.resources	= axp20x_pek_resources,
> +	}, {
> +		.name		= "axp20x-regulator",
> +	},
> +};
> +
> +const struct of_device_id axp20x_of_match[] = {
> +	{ .compatible = "x-powers,axp202", .data = (void *) AXP202_ID },
> +	{ .compatible = "x-powers,axp209", .data = (void *) AXP209_ID },
> +	{ },
> +};
> +
> +static struct axp20x_dev *axp20x_pm_power_off;
> +static void axp20x_power_off(void)
> +{
> +	regmap_write(axp20x_pm_power_off->regmap, AXP20X_OFF_CTRL,
> +		     AXP20X_OFF);
> +}
> +
> +static int axp20x_i2c_probe(struct i2c_client *i2c,
> +			 const struct i2c_device_id *id)
> +{
> +	struct axp20x_dev *axp20x;
> +	const struct of_device_id *of_id;
> +	struct device_node *node = i2c->dev.of_node;
> +	int ret;
> +
> +	axp20x = devm_kzalloc(&i2c->dev, sizeof(*axp20x), GFP_KERNEL);
> +	if (!axp20x)
> +		return -ENOMEM;
> +
> +	of_id = of_match_device(axp20x_of_match, &i2c->dev);
> +	if (!of_id) {
> +		dev_err(&i2c->dev, "Unable to setup AXP20X data\n");
> +		return -ENODEV;
> +	}
> +	axp20x->variant = (int) of_id->data;
> +
> +	axp20x->i2c_client = i2c;
> +	axp20x->dev = &i2c->dev;
> +	dev_set_drvdata(axp20x->dev, axp20x);
> +
> +	axp20x->regmap = devm_regmap_init_i2c(i2c, &axp20x_regmap_config);
> +	if (IS_ERR(axp20x->regmap)) {
> +		ret = PTR_ERR(axp20x->regmap);
> +		dev_err(&i2c->dev, "regmap init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	axp20x->irq = i2c->irq;
> +	ret = regmap_add_irq_chip(axp20x->regmap, axp20x->irq,
> +				  IRQF_ONESHOT | IRQF_SHARED, -1,
> +				  &axp20x_regmap_irq_chip,
> +				  &axp20x->regmap_irqc);
> +	if (ret) {
> +		dev_err(&i2c->dev, "failed to add irq chip: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mfd_add_devices(axp20x->dev, -1, axp20x_cells,
> +			      ARRAY_SIZE(axp20x_cells), NULL, 0, NULL);
> +	if (ret) {
> +		dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
> +		goto mfd_err;
> +	}
> +
> +	axp20x->pm_off = of_property_read_bool(node, "axp,system-power-controller");
> +
> +	if (axp20x->pm_off && !pm_power_off) {
> +		axp20x_pm_power_off = axp20x;
> +		pm_power_off = axp20x_power_off;
> +	}

I don't think we have any other way to actually power down the board.

Is there any reason why we would not need this property?

> +	dev_info(&i2c->dev, "AXP20X driver loaded\n");
> +
> +	return 0;
> +
> +mfd_err:
> +	regmap_del_irq_chip(axp20x->irq, axp20x->regmap_irqc);
> +
> +	return ret;
> +}
> +
> +static int axp20x_i2c_remove(struct i2c_client *i2c)
> +{
> +	struct axp20x_dev *axp20x = i2c_get_clientdata(i2c);
> +
> +	if (axp20x == axp20x_pm_power_off) {
> +		axp20x_pm_power_off = NULL;
> +		pm_power_off = NULL;
> +	}
> +
> +	mfd_remove_devices(axp20x->dev);
> +	regmap_del_irq_chip(axp20x->i2c_client->irq, axp20x->regmap_irqc);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id axp20x_i2c_id[] = {
> +	{ "axp202", AXP202_ID },
> +	{ "axp209", AXP209_ID },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, axp20x_i2c_id);
> +
> +static struct i2c_driver axp20x_i2c_driver = {
> +	.driver = {
> +		.name	= "axp20x",
> +		.owner	= THIS_MODULE,
> +		.of_match_table	= of_match_ptr(axp20x_of_match),
> +	},
> +	.probe		= axp20x_i2c_probe,
> +	.remove		= axp20x_i2c_remove,
> +	.id_table	= axp20x_i2c_id,
> +};
> +
> +module_i2c_driver(axp20x_i2c_driver);
> +
> +MODULE_DESCRIPTION("PMIC MFD core driver for AXP20X");
> +MODULE_AUTHOR("Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> new file mode 100644
> index 0000000..fcef32c
> --- /dev/null
> +++ b/include/linux/mfd/axp20x.h
> @@ -0,0 +1,180 @@
> +/*
> + * Functions to access AXP20X power management chip.
> + *
> + * Copyright (C) 2013, Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __LINUX_MFD_AXP20X_H
> +#define __LINUX_MFD_AXP20X_H
> +
> +#define AXP202_ID			0
> +#define AXP209_ID			1
> +
> +#define AXP20X_DATACACHE(m)		(0x04 + (m))
> +
> +/* Power supply */
> +#define AXP20X_PWR_INPUT_STATUS		0x00
> +#define AXP20X_PWR_OP_MODE		0x01
> +#define AXP20X_USB_OTG_STATUS		0x02
> +#define AXP20X_PWR_OUT_CTRL		0x12
> +#define AXP20X_DCDC2_V_OUT		0x23
> +#define AXP20X_DCDC2_LDO3_V_SCAL	0x25
> +#define AXP20X_DCDC3_V_OUT		0x27
> +#define AXP20X_LDO24_V_OUT		0x28
> +#define AXP20X_LDO3_V_OUT		0x29
> +#define AXP20X_VBUS_IPSOUT_MGMT		0x30
> +#define AXP20X_V_OFF			0x31
> +#define AXP20X_OFF_CTRL			0x32
> +#define AXP20X_CHRG_CTRL1		0x33
> +#define AXP20X_CHRG_CTRL2		0x34
> +#define AXP20X_CHRG_BAK_CTRL		0x35
> +#define AXP20X_PEK_KEY			0x36
> +#define AXP20X_DCDC_FREQ		0x37
> +#define AXP20X_V_LTF_CHRG		0x38
> +#define AXP20X_V_HTF_CHRG		0x39
> +#define AXP20X_APS_WARN_L1		0x3a
> +#define AXP20X_APS_WARN_L2		0x3b
> +#define AXP20X_V_LTF_DISCHRG		0x3c
> +#define AXP20X_V_HTF_DISCHRG		0x3d
> +
> +/* Interrupt */
> +#define AXP20X_IRQ1_EN			0x40
> +#define AXP20X_IRQ2_EN			0x41
> +#define AXP20X_IRQ3_EN			0x42
> +#define AXP20X_IRQ4_EN			0x43
> +#define AXP20X_IRQ5_EN			0x44
> +#define AXP20X_IRQ1_STATE		0x48
> +#define AXP20X_IRQ2_STATE		0x49
> +#define AXP20X_IRQ3_STATE		0x4a
> +#define AXP20X_IRQ4_STATE		0x4b
> +#define AXP20X_IRQ5_STATE		0x4c
> +
> +/* ADC */
> +#define AXP20X_ACIN_V_ADC_H		0x56
> +#define AXP20X_ACIN_V_ADC_L		0x57
> +#define AXP20X_ACIN_I_ADC_H		0x58
> +#define AXP20X_ACIN_I_ADC_L		0x59
> +#define AXP20X_VBUS_V_ADC_H		0x5a
> +#define AXP20X_VBUS_V_ADC_L		0x5b
> +#define AXP20X_VBUS_I_ADC_H		0x5c
> +#define AXP20X_VBUS_I_ADC_L		0x5d
> +#define AXP20X_TEMP_ADC_H		0x5e
> +#define AXP20X_TEMP_ADC_L		0x5f
> +#define AXP20X_TS_IN_H			0x62
> +#define AXP20X_TS_IN_L			0x63
> +#define AXP20X_GPIO0_V_ADC_H		0x64
> +#define AXP20X_GPIO0_V_ADC_L		0x65
> +#define AXP20X_GPIO1_V_ADC_H		0x66
> +#define AXP20X_GPIO1_V_ADC_L		0x67
> +#define AXP20X_PWR_BATT_H		0x70
> +#define AXP20X_PWR_BATT_M		0x71
> +#define AXP20X_PWR_BATT_L		0x72
> +#define AXP20X_BATT_V_H			0x78
> +#define AXP20X_BATT_V_L			0x79
> +#define AXP20X_BATT_CHRG_I_H		0x7a
> +#define AXP20X_BATT_CHRG_I_L		0x7b
> +#define AXP20X_BATT_DISCHRG_I_H		0x7c
> +#define AXP20X_BATT_DISCHRG_I_L		0x7d
> +#define AXP20X_IPSOUT_V_HIGH_H		0x7e
> +#define AXP20X_IPSOUT_V_HIGH_L		0x7f
> +
> +/* Power supply */
> +#define AXP20X_DCDC_MODE		0x80
> +#define AXP20X_ADC_EN1			0x82
> +#define AXP20X_ADC_EN2			0x83
> +#define AXP20X_ADC_RATE			0x84
> +#define AXP20X_GPIO10_IN_RANGE		0x85
> +#define AXP20X_GPIO1_ADC_IRQ_RIS	0x86
> +#define AXP20X_GPIO1_ADC_IRQ_FAL	0x87
> +#define AXP20X_TIMER_CTRL		0x8a
> +#define AXP20X_VBUS_MON			0x8b
> +#define AXP20X_OVER_TMP			0x8f
> +
> +/* GPIO */
> +#define AXP20X_GPIO0_CTRL		0x90
> +#define AXP20X_LDO5_V_OUT		0x91
> +#define AXP20X_GPIO1_CTRL		0x92
> +#define AXP20X_GPIO2_CTRL		0x93
> +#define AXP20X_GPIO20_SS		0x94
> +#define AXP20X_GPIO3_CTRL		0x95
> +
> +/* Battery */
> +#define AXP20X_CHRG_CC_31_24		0xb0
> +#define AXP20X_CHRG_CC_23_16		0xb1
> +#define AXP20X_CHRG_CC_15_8		0xb2
> +#define AXP20X_CHRG_CC_7_0		0xb3
> +#define AXP20X_DISCHRG_CC_31_24		0xb4
> +#define AXP20X_DISCHRG_CC_23_16		0xb5
> +#define AXP20X_DISCHRG_CC_15_8		0xb6
> +#define AXP20X_DISCHRG_CC_7_0		0xb7
> +#define AXP20X_CC_CTRL			0xb8
> +#define AXP20X_FG_RES			0xb9
> +
> +/* Regulators IDs */
> +enum {
> +	AXP20X_LDO1 = 0,
> +	AXP20X_LDO2,
> +	AXP20X_LDO3,
> +	AXP20X_LDO4,
> +	AXP20X_LDO5,
> +	AXP20X_DCDC2,
> +	AXP20X_DCDC3,
> +	AXP20X_REG_ID_MAX,
> +};
> +
> +/* IRQs */
> +enum {
> +	AXP20X_IRQ_ACIN_OVER_V = 1,
> +	AXP20X_IRQ_ACIN_PLUGIN,
> +	AXP20X_IRQ_ACIN_REMOVAL,
> +	AXP20X_IRQ_VBUS_OVER_V,
> +	AXP20X_IRQ_VBUS_PLUGIN,
> +	AXP20X_IRQ_VBUS_REMOVAL,
> +	AXP20X_IRQ_VBUS_V_LOW,
> +	AXP20X_IRQ_BATT_PLUGIN,
> +	AXP20X_IRQ_BATT_REMOVAL,
> +	AXP20X_IRQ_BATT_ENT_ACT_MODE,
> +	AXP20X_IRQ_BATT_EXIT_ACT_MODE,
> +	AXP20X_IRQ_CHARG,
> +	AXP20X_IRQ_CHARG_DONE,
> +	AXP20X_IRQ_BATT_TEMP_HIGH,
> +	AXP20X_IRQ_BATT_TEMP_LOW,
> +	AXP20X_IRQ_DIE_TEMP_HIGH,
> +	AXP20X_IRQ_CHARG_I_LOW,
> +	AXP20X_IRQ_DCDC1_V_LONG,
> +	AXP20X_IRQ_DCDC2_V_LONG,
> +	AXP20X_IRQ_DCDC3_V_LONG,
> +	AXP20X_IRQ_PEK_SHORT = 22,
> +	AXP20X_IRQ_PEK_LONG,
> +	AXP20X_IRQ_N_OE_PWR_ON,
> +	AXP20X_IRQ_N_OE_PWR_OFF,
> +	AXP20X_IRQ_VBUS_VALID,
> +	AXP20X_IRQ_VBUS_NOT_VALID,
> +	AXP20X_IRQ_VBUS_SESS_VALID,
> +	AXP20X_IRQ_VBUS_SESS_END,
> +	AXP20X_IRQ_LOW_PWR_LVL1,
> +	AXP20X_IRQ_LOW_PWR_LVL2,
> +	AXP20X_IRQ_TIMER,
> +	AXP20X_IRQ_PEK_RIS_EDGE,
> +	AXP20X_IRQ_PEK_FAL_EDGE,
> +	AXP20X_IRQ_GPIO3_INPUT,
> +	AXP20X_IRQ_GPIO2_INPUT,
> +	AXP20X_IRQ_GPIO1_INPUT,
> +	AXP20X_IRQ_GPIO0_INPUT,
> +};
> +
> +struct axp20x_dev {
> +	struct device			*dev;
> +	struct i2c_client		*i2c_client;
> +	struct regmap			*regmap;
> +	struct regmap_irq_chip_data	*regmap_irqc;
> +	int				variant;
> +	int				irq;
> +	bool				pm_off;
> +};
> +
> +#endif /* __LINUX_MFD_AXP20X_H */
> -- 
> 1.8.3.2
> 

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* Re: [PATCH 2/7] mfd: AXP20x: Add bindings documentation
From: Maxime Ripard @ 2014-03-07 18:13 UTC (permalink / raw)
  To: Carlo Caione
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	hdegoede-H+wXaHxf7aLQT0dZR+AlfA, emilio-0Z03zUJReD5OxF6Tv1QG9Q,
	wens-jdAy2FN1RRM, sameo-VuQAYsv1563Yd54FQh9/CA,
	lee.jones-QSEj5FYQhm4dnm+yROfE0A,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	lgirdwood-Re5JQEeQqe8AvxtiuMwx3w, broonie-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <1393692352-10839-3-git-send-email-carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 3827 bytes --]

On Sat, Mar 01, 2014 at 05:45:47PM +0100, Carlo Caione wrote:
> Bindings documentation for the AXP20x driver. In this file also two
> sub-nodes (PEK and regulators) are documented.
> 
> Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/mfd/axp20x.txt | 93 ++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/axp20x.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/axp20x.txt b/Documentation/devicetree/bindings/mfd/axp20x.txt
> new file mode 100644
> index 0000000..ae3e3c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/axp20x.txt
> @@ -0,0 +1,93 @@
> +* axp20x device tree bindings
> +
> +The axp20x family current members :-
> +axp202 (X-Powers)
> +axp209 (X-Powers)
> +
> +Required properties:
> +- compatible 			: Should be "x-powers,axp202" or "x-powers,axp209"
> +- interrupt-controller 		: axp20x has its own internal IRQs
> +- #interrupt-cells 		: Should be set to 1
> +- interrupt-parent 		: The parent interrupt controller
> +- interrupts 			: Interrupt specifiers for interrupt sources
> +- reg 				: The I2C slave address for the AXP chip
> +- axp,system-power-controller	: Telling whether or not this pmic is
> +				  controlling the system power
> +
> +Sub-nodes:
> +* regulators : Contain the regulator nodes. The regulators are bound using
> +	       their name as listed here: dcdc2, dcdc3, ldo1, ldo2, ldo3,
> +	       ldo4, ldo5.
> +	       The bindings details of individual regulator device can be found in:
> +	       Documentation/devicetree/bindings/regulator/regulator.txt with the
> +	       exception of:
> +
> +	- dcdc-freq		: defines the work frequency of DC-DC in KHz
> +				  (range: 750-1875)
> +	- dcdc-workmode		: 1 for PWM mode, 0 for AUTO mode

You don't seem to always set this. You should mention that it is
optional, and which default value it has.

> +
> +* axp20x-pek : Power Enable Key
> +	- compatible		: should be "x-powers,axp20x-pek"

Why is this needed for?

Plus, please don't use any generic, or pattern matching compatibles,
but rather precise ones, so that if it is needed, we can add any quirk
we want.


> +Example:
> +
> +axp: axp20x@34 {
> +	reg = <0x34>;
> +	interrupt-parent = <&nmi_intc>;
> +	interrupts = <0 8>;
> +
> +	axp,system-power-controller;
> +
> +	compatible = "x-powers,axp209";
> +	interrupt-controller;
> +	#interrupt-cells = <1>;
> +
> +	axp20x-pek {
> +		compatible = "x-powers,axp20x-pek";
> +	};
> +
> +	regulators {
> +		dcdc-freq = "8";
> +
> +		axp_dcdc2: dcdc2 {
> +			regulator-min-microvolt = <700000>;
> +			regulator-max-microvolt = <2275000>;
> +			dcdc-workmode = <0>;
> +			regulator-always-on;
> +		};
> +
> +		axp_dcdc3: dcdc3 {
> +			regulator-min-microvolt = <700000>;
> +			regulator-max-microvolt = <3500000>;
> +			dcdc-workmode = <0>;
> +			regulator-always-on;
> +		};
> +
> +		axp_ldo1: ldo1 {
> +			regulator-min-microvolt = <1300000>;
> +			regulator-max-microvolt = <1300000>;
> +		};
> +
> +		axp_ldo2: ldo2 {
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <3300000>;
> +			regulator-always-on;
> +		};
> +
> +		axp_ldo3: ldo3 {
> +			regulator-min-microvolt = <700000>;
> +			regulator-max-microvolt = <3500000>;
> +		};
> +
> +		axp_ldo4: ldo4 {
> +			regulator-min-microvolt = <1250000>;
> +			regulator-max-microvolt = <3300000>;
> +		};
> +
> +		axp_ldo5: ldo5 {
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <3300000>;
> +		};
> +	};
> +};
> +
> -- 
> 1.8.3.2
> 

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* Re: [PATCH 3/7] ARM: dts: cubieboard2: Add AXP209 support
From: Maxime Ripard @ 2014-03-07 18:13 UTC (permalink / raw)
  To: Carlo Caione
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	hdegoede-H+wXaHxf7aLQT0dZR+AlfA, emilio-0Z03zUJReD5OxF6Tv1QG9Q,
	wens-jdAy2FN1RRM, sameo-VuQAYsv1563Yd54FQh9/CA,
	lee.jones-QSEj5FYQhm4dnm+yROfE0A,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	lgirdwood-Re5JQEeQqe8AvxtiuMwx3w, broonie-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <1393692352-10839-4-git-send-email-carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

On Sat, Mar 01, 2014 at 05:45:48PM +0100, Carlo Caione wrote:
> AXP209 is the PMU used by Cubieboard2. This patch enable the core
> support in the dts file.
> This patch requires: "ARM: sun7i/sun6i: irqchip: Add irqchip driver for
> NMI controller"

That shouldn't be in the commit log itself.

> 
> Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> index 5c51cb8..234b14b 100644
> --- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> @@ -53,6 +53,20 @@
>  			pinctrl-names = "default";
>  			pinctrl-0 = <&i2c0_pins_a>;
>  			status = "okay";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			axp: axp20x@34 {
> +				reg = <0x34>;
> +				interrupt-parent = <&nmi_intc>;
> +				interrupts = <0 8>;
> +
> +				axp,system-power-controller;
> +
> +				compatible = "x-powers,axp209";
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +			};
>  		};
>  
>  		i2c1: i2c@01c2b000 {
> -- 
> 1.8.3.2
> 

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* Re: [PATCH 4/7] input: misc: Add driver for AXP20x Power Enable Key
From: Maxime Ripard @ 2014-03-07 18:18 UTC (permalink / raw)
  To: Carlo Caione
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	hdegoede-H+wXaHxf7aLQT0dZR+AlfA, emilio-0Z03zUJReD5OxF6Tv1QG9Q,
	wens-jdAy2FN1RRM, sameo-VuQAYsv1563Yd54FQh9/CA,
	lee.jones-QSEj5FYQhm4dnm+yROfE0A,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	lgirdwood-Re5JQEeQqe8AvxtiuMwx3w, broonie-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <1393692352-10839-5-git-send-email-carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 11051 bytes --]

On Sat, Mar 01, 2014 at 05:45:49PM +0100, Carlo Caione wrote:
> This patch add support for the Power Enable Key found on MFD AXP202 and
> AXP209. Besides the basic support for the button, the driver adds two
> entries in sysfs to configure the time delay for power on/off.
> 
> Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm/configs/sunxi_defconfig |   2 +

Again, please provide a separate patch for both sunxi_defconfig and
multi_v7_defconfig.

(This can be the same patches than the one enabling the MFD though)

>  drivers/input/misc/Kconfig       |  11 ++
>  drivers/input/misc/Makefile      |   1 +
>  drivers/input/misc/axp20x-pek.c  | 265 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 279 insertions(+)
>  create mode 100644 drivers/input/misc/axp20x-pek.c
> 
> diff --git a/arch/arm/configs/sunxi_defconfig b/arch/arm/configs/sunxi_defconfig
> index f8aa7e6..d59c826 100644
> --- a/arch/arm/configs/sunxi_defconfig
> +++ b/arch/arm/configs/sunxi_defconfig
> @@ -39,6 +39,8 @@ CONFIG_SUN4I_EMAC=y
>  # CONFIG_NET_VENDOR_STMICRO is not set
>  # CONFIG_NET_VENDOR_WIZNET is not set
>  # CONFIG_WLAN is not set
> +CONFIG_INPUT_MISC=y
> +CONFIG_INPUT_AXP20X_PEK=y
>  CONFIG_SERIAL_8250=y
>  CONFIG_SERIAL_8250_CONSOLE=y
>  CONFIG_SERIAL_8250_NR_UARTS=8
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 5f4967d..c9438ac 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -384,6 +384,17 @@ config INPUT_RETU_PWRBUTTON
>  	  To compile this driver as a module, choose M here. The module will
>  	  be called retu-pwrbutton.
>  
> +config INPUT_AXP20X_PEK
> +	tristate "X-Powers AXP20X power button driver"
> +	depends on MFD_AXP20X
> +	help
> +	  Say Y here if you want to enable power key reporting via the
> +	  AXP20X PMIC.
> +
> +	  To compile this driver as a module, choose M here. The module will
> +	  be called axp20x-pek.
> +
> +
>  config INPUT_TWL4030_PWRBUTTON
>  	tristate "TWL4030 Power button Driver"
>  	depends on TWL4030_CORE
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 0ebfb6d..41d5403 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_INPUT_POWERMATE)		+= powermate.o
>  obj-$(CONFIG_INPUT_PWM_BEEPER)		+= pwm-beeper.o
>  obj-$(CONFIG_INPUT_RB532_BUTTON)	+= rb532_button.o
>  obj-$(CONFIG_INPUT_RETU_PWRBUTTON)	+= retu-pwrbutton.o
> +obj-$(CONFIG_INPUT_AXP20X_PEK)		+= axp20x-pek.o
>  obj-$(CONFIG_INPUT_GPIO_ROTARY_ENCODER)	+= rotary_encoder.o
>  obj-$(CONFIG_INPUT_SGI_BTNS)		+= sgi_btns.o
>  obj-$(CONFIG_INPUT_SIRFSOC_ONKEY)	+= sirfsoc-onkey.o
> diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
> new file mode 100644
> index 0000000..799275d
> --- /dev/null
> +++ b/drivers/input/misc/axp20x-pek.c
> @@ -0,0 +1,265 @@
> +/*
> + * axp20x power button driver.
> + *
> + * Copyright (C) 2013 Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
> + *
> + * This file is subject to the terms and conditions of the GNU General
> + * Public License. See the file "COPYING" in the main directory of this
> + * archive for more details.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/irq.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/axp20x.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define AXP20X_PEK_STARTUP_MASK		(0xc0)
> +#define AXP20X_PEK_SHUTDOWN_MASK	(0x03)
> +
> +static const char const *startup_time[] = { "128mS", "3S" , "1S", "2S" };
> +static const char const *shutdown_time[] = { "4S", "6S" , "8S", "10S" };

I guess these are seconds. If so, a lowercase s in needed.

> +
> +struct axp20x_pek {
> +	struct axp20x_dev *axp20x;
> +	struct input_dev *input;
> +	int irq_dbr;
> +	int irq_dbf;
> +};
> +
> +struct axp20x_pek_ext_attr {
> +	const char const **str;
> +	unsigned int mask;
> +};
> +
> +static struct axp20x_pek_ext_attr axp20x_pek_startup_ext_attr = {
> +	.str	= startup_time,
> +	.mask	= AXP20X_PEK_STARTUP_MASK,
> +};
> +
> +static struct axp20x_pek_ext_attr axp20x_pek_shutdown_ext_attr = {
> +	.str	= shutdown_time,
> +	.mask	= AXP20X_PEK_SHUTDOWN_MASK,
> +};
> +
> +static struct axp20x_pek_ext_attr *get_axp_ext_attr(struct device_attribute *attr)
> +{
> +	return container_of(attr, struct dev_ext_attribute, attr)->var;
> +}
> +
> +ssize_t axp20x_show_ext_attr(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
> +	struct axp20x_pek_ext_attr *axp20x_ea = get_axp_ext_attr(attr);
> +	unsigned int val;
> +	int ret, i;
> +	int cnt = 0;
> +
> +	ret = regmap_read(axp20x_pek->axp20x->regmap, AXP20X_PEK_KEY, &val);
> +	if (ret != 0)
> +		return ret;
> +
> +	val &= axp20x_ea->mask;
> +	val >>= ffs(axp20x_ea->mask) - 1;
> +
> +	for (i = 0; i < 4; i++) {
> +		if (val == i)
> +			cnt += sprintf(buf + cnt, "[%s] ", axp20x_ea->str[i]);
> +		else
> +			cnt += sprintf(buf + cnt, "%s ", axp20x_ea->str[i]);
> +	}
> +
> +	cnt += sprintf(buf + cnt, "\n");
> +
> +	return cnt;
> +}
> +
> +ssize_t axp20x_store_ext_attr(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
> +	struct axp20x_pek_ext_attr *axp20x_ea = get_axp_ext_attr(attr);
> +	char val_str[20];
> +	int ret, i;
> +	size_t len;
> +
> +	val_str[sizeof(val_str) - 1] = '\0';
> +	strncpy(val_str, buf, sizeof(val_str) - 1);
> +	len = strlen(val_str);
> +
> +	if (len && val_str[len - 1] == '\n')
> +		val_str[len - 1] = '\0';
> +
> +	for (i = 0; i < 4; i++) {
> +		if (!strcmp(val_str, axp20x_ea->str[i])) {
> +			ret = regmap_update_bits(axp20x_pek->axp20x->regmap,
> +						 AXP20X_PEK_KEY,
> +						 axp20x_ea->mask, i);
> +			if (ret != 0)
> +				return -EINVAL;
> +			return count;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static struct dev_ext_attribute axp20x_dev_attr_startup = {
> +	.attr	= __ATTR(startup, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
> +	.var	= &axp20x_pek_startup_ext_attr
> +};
> +
> +static struct dev_ext_attribute axp20x_dev_attr_shutdown = {
> +	__ATTR(shutdown, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
> +	&axp20x_pek_shutdown_ext_attr
> +};
> +
> +static struct attribute *dev_attrs[] = {
> +	&axp20x_dev_attr_startup.attr.attr,
> +	&axp20x_dev_attr_shutdown.attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group dev_attr_group = {
> +	.attrs	= dev_attrs,
> +};
> +
> +static const struct attribute_group *dev_attr_groups[] = {
> +	&dev_attr_group,
> +	NULL,
> +};
> +
> +static irqreturn_t axp20x_pek_irq(int irq, void *pwr)
> +{
> +	struct input_dev *idev = pwr;
> +	struct axp20x_pek *axp20x_pek = input_get_drvdata(idev);
> +
> +	if (irq == axp20x_pek->irq_dbr)
> +		input_report_key(idev, KEY_POWER, true);
> +	else if (irq == axp20x_pek->irq_dbf)
> +		input_report_key(idev, KEY_POWER, false);
> +
> +	input_sync(idev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int axp20x_pek_probe(struct platform_device *pdev)
> +{
> +	struct axp20x_pek *axp20x_pek;
> +	struct axp20x_dev *axp20x;
> +	struct input_dev *idev;
> +	int error;
> +
> +	axp20x_pek = devm_kzalloc(&pdev->dev, sizeof(struct axp20x_pek),
> +				  GFP_KERNEL);
> +	if (!axp20x_pek)
> +		return -ENOMEM;
> +
> +	axp20x_pek->axp20x = dev_get_drvdata(pdev->dev.parent);
> +	axp20x = axp20x_pek->axp20x;
> +
> +	axp20x_pek->irq_dbr = platform_get_irq_byname(pdev, "PEK_DBR");
> +	if (axp20x_pek->irq_dbr < 0) {
> +		dev_err(&pdev->dev, "No IRQ for PEK_DBR, error=%d\n",
> +				axp20x_pek->irq_dbr);
> +		return axp20x_pek->irq_dbr;
> +	}
> +	axp20x_pek->irq_dbr = regmap_irq_get_virq(axp20x->regmap_irqc,
> +						  axp20x_pek->irq_dbr);
> +
> +	axp20x_pek->irq_dbf = platform_get_irq_byname(pdev, "PEK_DBF");
> +	if (axp20x_pek->irq_dbf < 0) {
> +		dev_err(&pdev->dev, "No IRQ for PEK_DBF, error=%d\n",
> +				axp20x_pek->irq_dbf);
> +		return axp20x_pek->irq_dbf;
> +	}
> +	axp20x_pek->irq_dbf = regmap_irq_get_virq(axp20x->regmap_irqc,
> +						  axp20x_pek->irq_dbf);
> +
> +	axp20x_pek->input = devm_input_allocate_device(&pdev->dev);
> +	if (!axp20x_pek->input)
> +		return -ENOMEM;
> +
> +	idev = axp20x_pek->input;
> +
> +	idev->name = "axp20x-pek";
> +	idev->phys = "m1kbd/input2";
> +	idev->dev.parent = &pdev->dev;
> +
> +	input_set_capability(idev, EV_KEY, KEY_POWER);
> +
> +	input_set_drvdata(idev, axp20x_pek);
> +
> +	error = devm_request_threaded_irq(&pdev->dev, axp20x_pek->irq_dbr,
> +					  NULL, axp20x_pek_irq, 0,
> +					  "axp20x-pek-dbr", idev);
> +	if (error) {
> +		dev_err(axp20x->dev, "Failed to request dbr IRQ#%d: %d\n",
> +			axp20x_pek->irq_dbr, error);
> +
> +		return error;
> +	}
> +
> +	error = devm_request_threaded_irq(&pdev->dev, axp20x_pek->irq_dbf,
> +					  NULL, axp20x_pek_irq, 0,
> +					  "axp20x-pek-dbf", idev);
> +	if (error) {
> +		dev_err(axp20x->dev, "Failed to request dbf IRQ#%d: %d\n",
> +			axp20x_pek->irq_dbf, error);
> +		return error;
> +	}
> +
> +	idev->dev.groups = dev_attr_groups;
> +	error = input_register_device(idev);
> +	if (error) {
> +		dev_err(axp20x->dev, "Can't register input device: %d\n", error);
> +		return error;
> +	}
> +
> +	platform_set_drvdata(pdev, axp20x_pek);
> +
> +	return 0;
> +}
> +
> +static int axp20x_pek_remove(struct platform_device *pdev)
> +{
> +	struct axp20x_pek *axp20x_pek = platform_get_drvdata(pdev);
> +
> +	input_unregister_device(axp20x_pek->input);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id axp20x_pek_match[] = {
> +	{ .compatible = "x-powers,axp20x-pek", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, axp20x_pek_match);
> +
> +static struct platform_driver axp20x_pek_driver = {
> +	.probe		= axp20x_pek_probe,
> +	.remove		= axp20x_pek_remove,
> +	.driver		= {
> +		.name		= "axp20x-pek",
> +		.owner		= THIS_MODULE,
> +		.of_match_table	= axp20x_pek_match,
> +	},
> +};
> +module_platform_driver(axp20x_pek_driver);
> +
> +MODULE_DESCRIPTION("axp20x Power Button");
> +MODULE_AUTHOR("Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>");
> +MODULE_LICENSE("GPL");
> -- 
> 1.8.3.2
> 

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* Re: [PATCH 0/2 ] Surface Pro 2 HID sensor, wacom, keyboard/multitouch composite device
From: Tolga Cakir @ 2014-03-07 18:20 UTC (permalink / raw)
  To: Derya; +Cc: linux-input, benjamin.tissoires, jkosina
In-Reply-To: <5319EB7C.5070209@yahoo.de>

2014-03-07 16:53 GMT+01:00 Derya <derya.kiran@yahoo.de>:
> The MS Surface Pro 2 has a very annoying USB composite device on port 2.3.
> It has 3 interfaces:
> - Interface 0 is the sensor-hub
> - Interface 1 is the wacom digitizer² (it's one without finger input, there
> is an atmel digitizer on port 2.4 for finger input)
> - Interface 2 is the keyboard, if a keyboard cover is attached.
>
> This USB composite device changes it product id depending if and which
> keyboard cover is attached.
> Each of this hid devices contains several collections, this complicated
> everything.
> I have uploaded a lsusb output to: http://pastebin.com/Jun5sa2t
>
> We also need the HID_QUIRK_NOGET for this usb composite device, without it
> hid-sensor-hub fails with a submit urb failure evertime a keyboard cover is
> (de)attached and it takes some seconds until the keyboard and wacom
> digitizer responds.

I had a similar issue while implementing Sidewinder X4 support. Can
you check the output of hid->type for each of these interfaces? I was
able to differentiate between the interfaces on my devices by checking
"hid->type", this might help here, too.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 6/7] regulator: AXP20x: Add support for regulators subsystem
From: Maxime Ripard @ 2014-03-07 18:22 UTC (permalink / raw)
  To: Carlo Caione
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	hdegoede-H+wXaHxf7aLQT0dZR+AlfA, emilio-0Z03zUJReD5OxF6Tv1QG9Q,
	wens-jdAy2FN1RRM, sameo-VuQAYsv1563Yd54FQh9/CA,
	lee.jones-QSEj5FYQhm4dnm+yROfE0A,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	lgirdwood-Re5JQEeQqe8AvxtiuMwx3w, broonie-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <1393692352-10839-7-git-send-email-carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

On Sat, Mar 01, 2014 at 05:45:51PM +0100, Carlo Caione wrote:
> +static struct platform_driver axp20x_regulator_driver = {
> +	.probe	= axp20x_regulator_probe,
> +	.remove	= axp20x_regulator_remove,
> +	.driver	= {
> +		.name		= "axp20x-regulator",
> +		.owner		= THIS_MODULE,
> +	},
> +};
> +
> +static int __init axp20x_regulator_init(void)
> +{
> +	return platform_driver_register(&axp20x_regulator_driver);
> +}
> +
> +subsys_initcall(axp20x_regulator_init);

Hmmm, really?

I thought the AXP was only connected through I2C? How is that a
platform device then?

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* Suspending access to opened/active /dev/nodes during application runtime
From: Lukasz Pawelczyk @ 2014-03-07 18:45 UTC (permalink / raw)
  To: linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	libvir-list-H+wXaHxf7aLQT0dZR+AlfA,
	lxc-devel-cunTk1MwBs9qMoObBWhMNEqPaTDuhLve2LY78lusg7I,
	systemd-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, David Herrmann


[-- Attachment #1.1: Type: text/plain, Size: 2346 bytes --]

Problem:
Has anyone thought about a mechanism to limit/remove an access to a
device during an application runtime? Meaning we have an application
that has an open file descriptor to some /dev/node and depending on
*something* it gains or looses the access to it gracefully (with or
without a notification, but without any fatal consequences).

Example:
LXC. Imagine we have 2 separate containers. Both running full operating
systems. Specifically with 2 X servers. Both running concurrently of
course. Both need the same input devices (e.g. we have just one mouse).
This creates a security problem when we want to have completely separate
environments. One container is active (being displayed on a monitor and
controlled with a mouse) while the other container runs evtest
/dev/input/something and grabs the secret password user typed in the
other.

Solutions:
The complete solution would comprise of 2 parts:
- a mechanism that would allow to temporally "hide" a device from an
open file descriptor.
- a mechanism for deciding whether application/process/namespace should
have an access to a specific device at a specific moment

Let's focus on the first problem only, as it would need to be solved
first anyway.  I haven't found anything that would allow me to do
it. There are a lot mechanisms that make it possible to restrict an
access during open():
- DAC
- ACL (controlled by hand or with uaccess)
- LSM (in general)
- device cgroups
But all of those can't do a thing when the device is already opened and
an application has a file descriptor.  I don't see such mechanism in
kernel sources either.

I do imagine that it would not be possible for every device to handle
such a thing (dri comes to mind) without breaking something (graphics
card state in dri example). But there is class of simple input/output
devices that would handle this without problems.

I did implement some proof-of-concept solution for an evdev driver by
allowing or disallowing events that go to evdev_client structure using
some arbitrary condition. But this is far from a generic solution.

My proof-of-concept is somewhat similar to this (I just found it):
http://www.spinics.net/lists/linux-input/msg25547.html
Though a little bit wider in scope. But neither is flawless nor
generic.

Has anyone had any thoughts about a similar problem?


-- 
Regards
Havner

[-- Attachment #1.2: Type: text/html, Size: 3239 bytes --]

[-- Attachment #2: Type: text/plain, Size: 194 bytes --]

_______________________________________________
lxc-devel mailing list
lxc-devel-cunTk1MwBs9qMoObBWhMNEqPaTDuhLve2LY78lusg7I@public.gmane.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

^ permalink raw reply

* Suspending access to opened/active /dev/nodes during application runtime
From: Lukasz Pawelczyk @ 2014-03-07 18:46 UTC (permalink / raw)
  To: linux-input, linux-kernel, libvir-list, lxc-devel, systemd-devel,
	David Herrmann

Problem:
Has anyone thought about a mechanism to limit/remove an access to a
device during an application runtime? Meaning we have an application
that has an open file descriptor to some /dev/node and depending on
*something* it gains or looses the access to it gracefully (with or
without a notification, but without any fatal consequences).

Example:
LXC. Imagine we have 2 separate containers. Both running full operating
systems. Specifically with 2 X servers. Both running concurrently of
course. Both need the same input devices (e.g. we have just one mouse).
This creates a security problem when we want to have completely separate
environments. One container is active (being displayed on a monitor and
controlled with a mouse) while the other container runs evtest
/dev/input/something and grabs the secret password user typed in the
other.

Solutions:
The complete solution would comprise of 2 parts:
- a mechanism that would allow to temporally "hide" a device from an
open file descriptor.
- a mechanism for deciding whether application/process/namespace should
have an access to a specific device at a specific moment

Let's focus on the first problem only, as it would need to be solved
first anyway.  I haven't found anything that would allow me to do
it. There are a lot mechanisms that make it possible to restrict an
access during open():
- DAC
- ACL (controlled by hand or with uaccess)
- LSM (in general)
- device cgroups
But all of those can't do a thing when the device is already opened and
an application has a file descriptor.  I don't see such mechanism in
kernel sources either.

I do imagine that it would not be possible for every device to handle
such a thing (dri comes to mind) without breaking something (graphics
card state in dri example). But there is class of simple input/output
devices that would handle this without problems.

I did implement some proof-of-concept solution for an evdev driver by
allowing or disallowing events that go to evdev_client structure using
some arbitrary condition. But this is far from a generic solution.

My proof-of-concept is somewhat similar to this (I just found it):
http://www.spinics.net/lists/linux-input/msg25547.html
Though a little bit wider in scope. But neither is flawless nor
generic.

Has anyone had any thoughts about a similar problem?


-- 
Regards
Havner

^ permalink raw reply

* Re: [systemd-devel] Suspending access to opened/active /dev/nodes during application runtime
From: Greg KH @ 2014-03-07 19:09 UTC (permalink / raw)
  To: Lukasz Pawelczyk
  Cc: linux-input, linux-kernel, libvir-list, lxc-devel, systemd-devel,
	David Herrmann
In-Reply-To: <E7B2439D-FF36-4353-8E1A-C58E5C33A89F@gmail.com>

On Fri, Mar 07, 2014 at 07:46:44PM +0100, Lukasz Pawelczyk wrote:
> Problem:
> Has anyone thought about a mechanism to limit/remove an access to a
> device during an application runtime? Meaning we have an application
> that has an open file descriptor to some /dev/node and depending on
> *something* it gains or looses the access to it gracefully (with or
> without a notification, but without any fatal consequences).
> 
> Example:
> LXC. Imagine we have 2 separate containers. Both running full operating
> systems. Specifically with 2 X servers. Both running concurrently of
> course. Both need the same input devices (e.g. we have just one mouse).

Stop right there.

If they "both" need an input device, then they should use the "shared"
input device stream, i.e. evdev.

And it goes the same for every type of device the kernel is exposing to
userspace, if you want to "share" them, then you need to work on
changing the kernel to be able to handle shared devices.

And odds are, you will get back a big "as-if" comment from the kernel
developers, as for almost all devices, they can't be shared, for very
good reasons.

So work down the list of devices you really need access to, and either
work to provide a way for the kernel to mediate them, or, work to only
have one "container" access to one device, and not have all containers
access to it at the same time.

This has been discussed many times in the past, on mailing lists and in
person at the Linux Plumbers conference last year.  This isn't a systemd
issue, it is a "you are using the kernel in ways it was not designed to
be used" issue.

good luck, you will need it...

greg k-h

^ permalink raw reply

* Re: [PATCH] input: Use platform-provided devices as i8042 serio parents
From: Matthew Garrett @ 2014-03-07 19:24 UTC (permalink / raw)
  To: dmitry.torokhov@gmail.com
  Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	benjamin.tissoires@redhat.com
In-Reply-To: <20140307175415.GB25283@core.coreip.homeip.net>

On Fri, 2014-03-07 at 09:54 -0800, Dmitry Torokhov wrote:
> Hi Matthew,
> 
> On Sun, Feb 23, 2014 at 12:03:16PM -0500, Matthew Garrett wrote:
> > i8042 devices exposed via platform firmware interfaces such as ACPI or
> > Device Tree may provide additional information of use to userspace. Right
> > now we don't associate the serio devices with the firmware device, and so
> > there's no straightforward way for userspace to make use of that
> > information. This patch simply moves the serio parent device to the firmware
> > provided device.
> 
> Hmm, are we sure it will not mess up power management now that children
> serio ports are disconnected from i8042 device?

Urh. Yeah, that's a point - we probably no longer guarantee that the
i8042 resume code runs before any driver resume code. That's
inconvenient. We could probably add resume callbacks to the PNP devices,
but then we need to ensure that they only run once. I think we can do
that without too much trouble, but it's not going to be beautiful.

-- 
Matthew Garrett <matthew.garrett@nebula.com>

^ permalink raw reply

* Re: Suspending access to opened/active /dev/nodes during application runtime
From: Lennart Poettering @ 2014-03-07 19:24 UTC (permalink / raw)
  To: Lukasz Pawelczyk
  Cc: systemd-devel, libvir-list, linux-input, linux-kernel, lxc-devel
In-Reply-To: <9D7BA6C9-9F1F-4D09-8F4F-E7DA4720FF97@gmail.com>

On Fri, 07.03.14 19:45, Lukasz Pawelczyk (havner@gmail.com) wrote:

> Problem:
> Has anyone thought about a mechanism to limit/remove an access to a
> device during an application runtime? Meaning we have an application
> that has an open file descriptor to some /dev/node and depending on
> *something* it gains or looses the access to it gracefully (with or
> without a notification, but without any fatal consequences).

logind can mute input devices as sessions are switched, to enable
unpriviliged X11 and wayland compositors.

> Example:
> LXC. Imagine we have 2 separate containers. Both running full operating
> systems. Specifically with 2 X servers. Both running concurrently of

Well, devices are not namespaced on Linux (with the single exception of
network devices). An X server needs device access, hence this doesn't
fly at all.

When you enumerate devices with libudev in a container they will never
be marked as "initialized" and you do not get any udev hotplug events in
containers, and you don#t have the host's udev db around, nor would it
make any sense to you if you had. X11 and friends rely on udev
however...

Before you think about doing something like this, you need to fix the
kernel to provide namespaced devices (good luck!)

> course. Both need the same input devices (e.g. we have just one mouse).
> This creates a security problem when we want to have completely separate
> environments. One container is active (being displayed on a monitor and
> controlled with a mouse) while the other container runs evtest
> /dev/input/something and grabs the secret password user typed in the
> other.

logind can do this for you between sessions. But such a container setup
will never work without proper device namespacing.

> Solutions:
> The complete solution would comprise of 2 parts:
> - a mechanism that would allow to temporally "hide" a device from an
> open file descriptor.
> - a mechanism for deciding whether application/process/namespace should
> have an access to a specific device at a specific moment

Well, there's no point in inventing any "mechanisms" like this, as long
as devices are not namespaced in the kernel, so that userspace in
containers can enumerate/probe/identify/... things correctly...

Lennart

-- 
Lennart Poettering, Red Hat

^ permalink raw reply

* Re: Suspending access to opened/active /dev/nodes during application runtime
From: Lukasz Pawelczyk @ 2014-03-07 20:45 UTC (permalink / raw)
  To: Greg KH; +Cc: systemd-devel, libvir-list, linux-input, linux-kernel, lxc-devel
In-Reply-To: <20140307190939.GA8082@kroah.com>


On 7 Mar 2014, at 20:09, Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Mar 07, 2014 at 07:46:44PM +0100, Lukasz Pawelczyk wrote:
>> Problem:
>> Has anyone thought about a mechanism to limit/remove an access to a
>> device during an application runtime? Meaning we have an application
>> that has an open file descriptor to some /dev/node and depending on
>> *something* it gains or looses the access to it gracefully (with or
>> without a notification, but without any fatal consequences).
>> 
>> Example:
>> LXC. Imagine we have 2 separate containers. Both running full operating
>> systems. Specifically with 2 X servers. Both running concurrently of
>> course. Both need the same input devices (e.g. we have just one mouse).
> 
> Stop right there.
> 
> If they "both" need an input device, then they should use the "shared"
> input device stream, i.e. evdev.
> 
> And it goes the same for every type of device the kernel is exposing to
> userspace, if you want to "share" them, then you need to work on
> changing the kernel to be able to handle shared devices.

I think you might have misunderstood me. They are using a shared input stream (evdev in this case). The problem is I don’t want them to eavesdrop on each other. So it’s not about making it to work. It’s about making them to work „in turns”.

> And odds are, you will get back a big "as-if" comment from the kernel
> developers, as for almost all devices, they can't be shared, for very
> good reasons.

Evdev devices can.


-- 
Regards,
Havner

^ permalink raw reply

* Re: [systemd-devel] Suspending access to opened/active /dev/nodes during application runtime
From: Lukasz Pawelczyk @ 2014-03-07 20:51 UTC (permalink / raw)
  To: Lennart Poettering
  Cc: linux-input, linux-kernel, libvir-list, lxc-devel, systemd-devel,
	David Herrmann
In-Reply-To: <20140307192410.GA24453@tango.0pointer.de>


On 7 Mar 2014, at 20:24, Lennart Poettering <mzerqung@0pointer.de> wrote:

> On Fri, 07.03.14 19:45, Lukasz Pawelczyk (havner@gmail.com) wrote:
> 
>> Problem:
>> Has anyone thought about a mechanism to limit/remove an access to a
>> device during an application runtime? Meaning we have an application
>> that has an open file descriptor to some /dev/node and depending on
>> *something* it gains or looses the access to it gracefully (with or
>> without a notification, but without any fatal consequences).
> 
> logind can mute input devices as sessions are switched, to enable
> unpriviliged X11 and wayland compositors.

Would you please elaborate on this? Where is this mechanism? How does it work without kernel space support? Is there some kernel space support I’m not aware of?

>> Example:
>> LXC. Imagine we have 2 separate containers. Both running full operating
>> systems. Specifically with 2 X servers. Both running concurrently of
> 
> Well, devices are not namespaced on Linux (with the single exception of
> network devices). An X server needs device access, hence this doesn't
> fly at all.
> 
> When you enumerate devices with libudev in a container they will never
> be marked as "initialized" and you do not get any udev hotplug events in
> containers, and you don#t have the host's udev db around, nor would it
> make any sense to you if you had. X11 and friends rely on udev
> however...
> 
> Before you think about doing something like this, you need to fix the
> kernel to provide namespaced devices (good luck!)

Precisly! That’s the generic idea. I’m not for implementing it though at this moment. I just wanted to know whether anybody actually though about it or maybe someone is interested in starting such a work, etc.

>> course. Both need the same input devices (e.g. we have just one mouse).
>> This creates a security problem when we want to have completely separate
>> environments. One container is active (being displayed on a monitor and
>> controlled with a mouse) while the other container runs evtest
>> /dev/input/something and grabs the secret password user typed in the
>> other.
> 
> logind can do this for you between sessions. But such a container setup
> will never work without proper device namespacing.

So how can it do it when there is no kernel support? You mean it could be doing this if the support were there?

>> Solutions:
>> The complete solution would comprise of 2 parts:
>> - a mechanism that would allow to temporally "hide" a device from an
>> open file descriptor.
>> - a mechanism for deciding whether application/process/namespace should
>> have an access to a specific device at a specific moment
> 
> Well, there's no point in inventing any "mechanisms" like this, as long
> as devices are not namespaced in the kernel, so that userspace in
> containers can enumerate/probe/identify/... things correctly…

True. My point is about kernel space implementation. Like I wrote. I haven’t seen anything like this in kernel source and I’m well away it should be done there.
I would just like to know if anybody is interested in this, if anybody started or would like to start such a thing.

I do understand that systemd/logind would only provide a mechanism for determining who should have an access and who shouldn’t (or to be more specific it would utilize some kernel space configuration like cgroups). But the work itself has to be done in kernel space.

-- 
Regards,
Havner



--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [systemd-devel] Suspending access to opened/active /dev/nodes during application runtime
From: Greg KH @ 2014-03-07 20:55 UTC (permalink / raw)
  To: Lukasz Pawelczyk
  Cc: linux-input, linux-kernel, libvir-list, lxc-devel, systemd-devel,
	David Herrmann
In-Reply-To: <93871091-7845-4BCC-A841-F724BEDF6E4B@gmail.com>

On Fri, Mar 07, 2014 at 09:45:28PM +0100, Lukasz Pawelczyk wrote:
> 
> On 7 Mar 2014, at 20:09, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Fri, Mar 07, 2014 at 07:46:44PM +0100, Lukasz Pawelczyk wrote:
> >> Problem:
> >> Has anyone thought about a mechanism to limit/remove an access to a
> >> device during an application runtime? Meaning we have an application
> >> that has an open file descriptor to some /dev/node and depending on
> >> *something* it gains or looses the access to it gracefully (with or
> >> without a notification, but without any fatal consequences).
> >> 
> >> Example:
> >> LXC. Imagine we have 2 separate containers. Both running full operating
> >> systems. Specifically with 2 X servers. Both running concurrently of
> >> course. Both need the same input devices (e.g. we have just one mouse).
> > 
> > Stop right there.
> > 
> > If they "both" need an input device, then they should use the "shared"
> > input device stream, i.e. evdev.
> > 
> > And it goes the same for every type of device the kernel is exposing to
> > userspace, if you want to "share" them, then you need to work on
> > changing the kernel to be able to handle shared devices.
> 
> I think you might have misunderstood me. They are using a shared input
> stream (evdev in this case). The problem is I don’t want them to
> eavesdrop on each other. So it’s not about making it to work. It’s
> about making them to work „in turns”.

See Lennart's comment about namespaces for devices, and how the kernel
doesn't support it, for the answer to this.

Sorry, not going to happen, use real virtual machines if you want to do
this.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH v3] input synaptics-rmi4: Add F30 support
From: Christopher Heiny @ 2014-03-08  0:35 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
	Vivian Ly, Daniel Rosenberg, Linus Walleij, Allie Xiong,
	Benjamin Tissoires, David Herrmann, Jiri Kosina

RMI4 Function 0x30 provides support for GPIOs, LEDs and mechanical
buttons.  In particular, the mechanical button support is used in
an increasing number of touchpads.

Note - this updates the patch of 2014/03/04 to address feedback
received.

Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
Signed-off-by: Allie Xiong <axiong@synaptics.com>
Acked-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Linux Walleij <linus.walleij@linaro.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>

---

 drivers/input/rmi4/Kconfig   |  12 ++
 drivers/input/rmi4/Makefile  |   1 +
 drivers/input/rmi4/rmi_f30.c | 399 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/rmi.h          |   7 +-
 4 files changed, 418 insertions(+), 1 deletion(-)

diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
index d0c7b6e..14d5f49 100644
--- a/drivers/input/rmi4/Kconfig
+++ b/drivers/input/rmi4/Kconfig
@@ -63,3 +63,15 @@ config RMI4_F11_PEN
 	  If your system is not recognizing pen touches and you know your
 	  sensor supports pen input, you probably want to turn this feature
 	  off.
+
+config RMI4_F30
+        tristate "RMI4 Function 30 (GPIO LED)"
+        depends on RMI4_CORE
+        help
+          Say Y here if you want to add support for RMI4 function 30.
+
+          Function 30 provides GPIO and LED support for RMI4 devices. This
+	  includes support for buttons on TouchPads and ClickPads.
+
+          To compile this driver as a module, choose M here: the
+          module will be called rmi-f30.
diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
index 5c6bad5..ecffd72 100644
--- a/drivers/input/rmi4/Makefile
+++ b/drivers/input/rmi4/Makefile
@@ -3,6 +3,7 @@ rmi_core-y := rmi_bus.o rmi_driver.o rmi_f01.o
 
 # Function drivers
 obj-$(CONFIG_RMI4_F11) += rmi_f11.o
+obj-$(CONFIG_RMI4_F30) += rmi_f30.o
 
 # Transports
 obj-$(CONFIG_RMI4_I2C) += rmi_i2c.o
diff --git a/drivers/input/rmi4/rmi_f30.c b/drivers/input/rmi4/rmi_f30.c
new file mode 100644
index 0000000..754da61
--- /dev/null
+++ b/drivers/input/rmi4/rmi_f30.c
@@ -0,0 +1,399 @@
+/*
+ * Copyright (c) 2012 - 2014 Synaptics Incorporated
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/rmi.h>
+#include <linux/input.h>
+#include <linux/slab.h>
+#include <linux/gpio.h>
+#include <linux/leds.h>
+#include "rmi_driver.h"
+
+#define RMI_F30_QUERY_SIZE			2
+
+/* Defs for Query 0 */
+#define RMI_F30_EXTENDED_PATTERNS		0x01
+#define RMI_F30_HAS_MAPPABLE_BUTTONS		(1 << 1)
+#define RMI_F30_HAS_LED				(1 << 2)
+#define RMI_F30_HAS_GPIO			(1 << 3)
+#define RMI_F30_HAS_HAPTIC			(1 << 4)
+#define RMI_F30_HAS_GPIO_DRV_CTL		(1 << 5)
+#define RMI_F30_HAS_MECH_MOUSE_BTNS		(1 << 6)
+
+/* Defs for Query 1 */
+#define RMI_F30_GPIO_LED_COUNT			0x1F
+
+/* Defs for Control Registers */
+#define RMI_F30_CTRL_1_GPIO_DEBOUNCE		0x01
+#define RMI_F30_CTRL_1_HALT			(1 << 4)
+#define RMI_F30_CTRL_1_HALTED			(1 << 5)
+#define RMI_F30_CTRL_10_NUM_MECH_MOUSE_BTNS	0x03
+
+struct rmi_f30_ctrl_data {
+	int address;
+	int length;
+	u8 *regs;
+};
+
+#define RMI_F30_CTRL_MAX_REGS		32
+#define RMI_F30_CTRL_MAX_BYTES		((RMI_F30_CTRL_MAX_REGS + 7) >> 3)
+#define RMI_F30_CTRL_MAX_REG_BLOCKS	11
+
+#define RMI_F30_CTRL_REGS_MAX_SIZE (RMI_F30_CTRL_MAX_BYTES		\
+					+ 1				\
+					+ RMI_F30_CTRL_MAX_BYTES	\
+					+ RMI_F30_CTRL_MAX_BYTES	\
+					+ RMI_F30_CTRL_MAX_BYTES	\
+					+ 6				\
+					+ RMI_F30_CTRL_MAX_REGS		\
+					+ RMI_F30_CTRL_MAX_REGS		\
+					+ RMI_F30_CTRL_MAX_BYTES	\
+					+ 1				\
+					+ 1)
+
+struct f30_data {
+	/* Query Data */
+	bool has_extended_pattern;
+	bool has_mappable_buttons;
+	bool has_led;
+	bool has_gpio;
+	bool has_haptic;
+	bool has_gpio_driver_control;
+	bool has_mech_mouse_btns;
+	u8 gpioled_count;
+
+	u8 register_count;
+
+	/* Control Register Data */
+	struct rmi_f30_ctrl_data ctrl[RMI_F30_CTRL_MAX_REG_BLOCKS];
+	u8 ctrl_regs[RMI_F30_CTRL_REGS_MAX_SIZE];
+	u32 ctrl_regs_size;
+
+	u8 data_regs[RMI_F30_CTRL_MAX_BYTES];
+	u16 *gpioled_key_map;
+	u8 *gpioled_sense_map;
+
+	char input_phys[NAME_BUFFER_SIZE];
+	struct input_dev *input;
+};
+
+static int rmi_f30_read_control_parameters(struct rmi_function *fn,
+						struct f30_data *f30)
+{
+	struct rmi_device *rmi_dev = fn->rmi_dev;
+	int error = 0;
+
+	error = rmi_read_block(rmi_dev, fn->fd.control_base_addr,
+				f30->ctrl_regs, f30->ctrl_regs_size);
+	if (error) {
+		dev_err(&rmi_dev->dev, "%s : Could not read control registers at 0x%x error (%d)\n",
+			__func__, fn->fd.control_base_addr, error);
+		return error;
+	}
+
+	return 0;
+}
+
+static int rmi_f30_attention(struct rmi_function *fn, unsigned long *irq_bits)
+{
+	struct f30_data *f30 = dev_get_drvdata(&fn->dev);
+	int retval;
+	int gpiled = 0;
+	int value = 0;
+	int i;
+	int reg_num;
+
+	/* Read the gpi led data. */
+	retval = rmi_read_block(fn->rmi_dev, fn->fd.data_base_addr,
+		f30->data_regs, f30->register_count);
+
+	if (retval) {
+		dev_err(&fn->dev, "%s: Failed to read F30 data registers.\n",
+			__func__);
+		return retval;
+	}
+
+	for (reg_num = 0; reg_num < f30->register_count; ++reg_num) {
+		for (i = 0; gpiled < f30->gpioled_count && i < 8; ++i,
+			++gpiled) {
+			if (f30->gpioled_key_map[gpiled] != 0) {
+				value = (((f30->data_regs[reg_num] >> i) & 0x01)
+					 == f30->gpioled_sense_map[gpiled]);
+
+				dev_dbg(&fn->dev,
+					"%s: call input report key (0x%04x) value (0x%02x)",
+					__func__,
+					f30->gpioled_key_map[gpiled], value);
+				input_report_key(f30->input,
+					f30->gpioled_key_map[gpiled],
+					value);
+			}
+
+		}
+	}
+
+	input_sync(f30->input); /* sync after groups of events */
+	return 0;
+}
+
+static int rmi_f30_register_device(struct rmi_function *fn)
+{
+	int i;
+	int rc;
+	struct rmi_device *rmi_dev = fn->rmi_dev;
+	struct f30_data *f30 = dev_get_drvdata(&fn->dev);
+	struct rmi_driver *driver = fn->rmi_dev->driver;
+	struct input_dev *input_dev;
+
+	input_dev = devm_input_allocate_device(&fn->dev);
+	if (!input_dev) {
+		dev_err(&fn->dev, "Failed to allocate input device.\n");
+		return -ENOMEM;
+	}
+
+	f30->input = input_dev;
+
+	if (driver->set_input_params) {
+		rc = driver->set_input_params(rmi_dev, input_dev);
+		if (rc < 0) {
+			dev_err(&fn->dev, "%s: Error in setting input device.\n",
+				__func__);
+			return rc;
+		}
+	}
+	sprintf(f30->input_phys, "%s/input0", dev_name(&fn->dev));
+	input_dev->phys = f30->input_phys;
+	input_dev->dev.parent = &rmi_dev->dev;
+	input_set_drvdata(input_dev, f30);
+
+	set_bit(EV_SYN, input_dev->evbit);
+	set_bit(EV_KEY, input_dev->evbit);
+
+	input_dev->keycode = f30->gpioled_key_map;
+	input_dev->keycodesize = sizeof(u16);
+	input_dev->keycodemax = f30->gpioled_count;
+
+	for (i = 0; i < f30->gpioled_count; i++) {
+		if (f30->gpioled_key_map[i] != 0)
+			input_set_capability(input_dev, EV_KEY,
+						f30->gpioled_key_map[i]);
+	}
+
+	rc = input_register_device(input_dev);
+	if (rc < 0) {
+		dev_err(&fn->dev, "Failed to register input device.\n");
+		return rc;
+	}
+	return 0;
+}
+
+static int rmi_f30_config(struct rmi_function *fn)
+{
+	struct f30_data *f30 = dev_get_drvdata(&fn->dev);
+	int error;
+
+	/* Write Control Register values back to device */
+	error = rmi_write_block(fn->rmi_dev, fn->fd.control_base_addr,
+				f30->ctrl_regs, f30->ctrl_regs_size);
+	if (error) {
+		dev_err(&fn->rmi_dev->dev,
+			"%s : Could not write control registers at 0x%x error (%d)\n",
+			__func__, fn->fd.control_base_addr, error);
+		return error;
+	}
+
+	return 0;
+}
+
+static inline void rmi_f30_set_ctrl_data(struct rmi_f30_ctrl_data *ctrl,
+					int *ctrl_addr, int len, u8 **reg)
+{
+	ctrl->address = *ctrl_addr;
+	ctrl->length = len;
+	ctrl->regs = *reg;
+	*ctrl_addr += len;
+	*reg += len;
+}
+
+static inline int rmi_f30_initialize(struct rmi_function *fn)
+{
+	struct f30_data *f30;
+	struct rmi_device *rmi_dev = fn->rmi_dev;
+	const struct rmi_device_platform_data *pdata;
+	int retval = 0;
+	int control_address;
+	u8 buf[RMI_F30_QUERY_SIZE];
+	u8 *ctrl_reg;
+	u8 *map_memory;
+
+	f30 = devm_kzalloc(&fn->dev, sizeof(struct f30_data),
+			   GFP_KERNEL);
+	if (!f30) {
+		dev_err(&fn->dev, "Failed to allocate f30_data.\n");
+		return -ENOMEM;
+	}
+	dev_set_drvdata(&fn->dev, f30);
+
+	retval = rmi_read_block(fn->rmi_dev, fn->fd.query_base_addr, buf,
+				RMI_F30_QUERY_SIZE);
+
+	if (retval) {
+		dev_err(&fn->dev, "Failed to read query register.\n");
+		return retval;
+	}
+
+	f30->has_extended_pattern = buf[0] & RMI_F30_EXTENDED_PATTERNS;
+	f30->has_mappable_buttons = buf[0] & RMI_F30_HAS_MAPPABLE_BUTTONS;
+	f30->has_led = buf[0] & RMI_F30_HAS_LED;
+	f30->has_gpio = buf[0] & RMI_F30_HAS_GPIO;
+	f30->has_haptic = buf[0] & RMI_F30_HAS_HAPTIC;
+	f30->has_gpio_driver_control = buf[0] & RMI_F30_HAS_GPIO_DRV_CTL;
+	f30->has_mech_mouse_btns = buf[0] & RMI_F30_HAS_MECH_MOUSE_BTNS;
+	f30->gpioled_count = buf[1] & RMI_F30_GPIO_LED_COUNT;
+
+	f30->register_count = (f30->gpioled_count + 7) >> 3;
+
+	control_address = fn->fd.control_base_addr;
+	ctrl_reg = f30->ctrl_regs;
+
+	/* Allocate buffers for the control registers */
+	if (f30->has_led && f30->has_led)
+		rmi_f30_set_ctrl_data(&f30->ctrl[0], &control_address,
+					f30->register_count, &ctrl_reg);
+
+	rmi_f30_set_ctrl_data(&f30->ctrl[1], &control_address, sizeof(u8),
+				&ctrl_reg);
+
+	if (f30->has_gpio) {
+		rmi_f30_set_ctrl_data(&f30->ctrl[2], &control_address,
+					f30->register_count, &ctrl_reg);
+
+		rmi_f30_set_ctrl_data(&f30->ctrl[3], &control_address,
+					f30->register_count, &ctrl_reg);
+	}
+
+	if (f30->has_led) {
+		int ctrl5_len;
+
+		rmi_f30_set_ctrl_data(&f30->ctrl[4], &control_address,
+					f30->register_count, &ctrl_reg);
+
+		if (f30->has_extended_pattern)
+			ctrl5_len = 6;
+		else
+			ctrl5_len = 2;
+
+		rmi_f30_set_ctrl_data(&f30->ctrl[5], &control_address,
+					ctrl5_len, &ctrl_reg);
+	}
+
+	if (f30->has_led || f30->has_gpio_driver_control) {
+		/* control 6 uses a byte per gpio/led */
+		rmi_f30_set_ctrl_data(&f30->ctrl[6], &control_address,
+					f30->gpioled_count, &ctrl_reg);
+	}
+
+	if (f30->has_mappable_buttons) {
+		/* control 7 uses a byte per gpio/led */
+		rmi_f30_set_ctrl_data(&f30->ctrl[7], &control_address,
+					f30->gpioled_count, &ctrl_reg);
+	}
+
+	if (f30->has_haptic) {
+		rmi_f30_set_ctrl_data(&f30->ctrl[8], &control_address,
+					f30->register_count, &ctrl_reg);
+
+		rmi_f30_set_ctrl_data(&f30->ctrl[9], &control_address,
+					sizeof(u8), &ctrl_reg);
+	}
+
+	if (f30->has_mech_mouse_btns)
+		rmi_f30_set_ctrl_data(&f30->ctrl[10], &control_address,
+					sizeof(u8), &ctrl_reg);
+
+	f30->ctrl_regs_size = ctrl_reg - f30->ctrl_regs
+				?: RMI_F30_CTRL_REGS_MAX_SIZE;
+
+	map_memory = devm_kzalloc(&fn->dev,
+				(f30->gpioled_count
+				* (sizeof(u16) + sizeof(u8))),
+				GFP_KERNEL);
+	if (!map_memory) {
+		dev_err(&fn->dev, "Failed to allocate gpioled map memory.\n");
+		return -ENOMEM;
+	}
+
+	f30->gpioled_key_map = (u16 *)map_memory;
+	f30->gpioled_sense_map = map_memory + sizeof(u16)
+					* f30->gpioled_count;
+
+	pdata = rmi_get_platform_data(rmi_dev);
+	if (pdata) {
+		if (!pdata->gpioled_map) {
+			dev_warn(&fn->dev,
+				"%s - gpioled_map is NULL", __func__);
+		} else if (!pdata->gpioled_map->map) {
+			dev_warn(&fn->dev,
+				 "Platform Data button map is missing!\n");
+		} else {
+			int i;
+			for (i = 0; i < f30->gpioled_count
+				|| i < pdata->gpioled_map->ngpioleds; i++) {
+				f30->gpioled_key_map[i] =
+					pdata->gpioled_map->map[i].button;
+				f30->gpioled_sense_map[i] =
+					pdata->gpioled_map->map[i].sense;
+			}
+		}
+	}
+
+	retval = rmi_f30_read_control_parameters(fn, f30);
+	if (retval < 0) {
+		dev_err(&fn->dev,
+			"Failed to initialize F19 control params.\n");
+		return retval;
+	}
+
+	return 0;
+}
+
+static int rmi_f30_probe(struct rmi_function *fn)
+{
+	int rc;
+
+	rc = rmi_f30_initialize(fn);
+	if (rc < 0)
+		goto error_exit;
+
+	rc = rmi_f30_register_device(fn);
+	if (rc < 0)
+		goto error_exit;
+
+	return 0;
+
+error_exit:
+	return rc;
+
+}
+
+static struct rmi_function_handler rmi_f30_handler = {
+	.driver = {
+		.name = "rmi_f30",
+	},
+	.func = 0x30,
+	.probe = rmi_f30_probe,
+	.config = rmi_f30_config,
+	.attention = rmi_f30_attention,
+};
+
+module_rmi_driver(rmi_f30_handler);
+
+MODULE_AUTHOR("Allie Xiong <axiong@synaptics.com>");
+MODULE_AUTHOR("Andrew Duggan <aduggan@synaptics.com>");
+MODULE_DESCRIPTION("RMI F30 module");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/rmi.h b/include/linux/rmi.h
index 735e978..889a627 100644
--- a/include/linux/rmi.h
+++ b/include/linux/rmi.h
@@ -139,9 +139,14 @@ struct rmi_button_map {
 	u8 *map;
 };
 
+struct rmi_f30_button {
+	u16 button;
+	int sense;
+};
+
 struct rmi_f30_gpioled_map {
 	u8 ngpioleds;
-	u8 *map;
+	struct rmi_f30_button *map;
 };
 
 /**

^ permalink raw reply related

* [PATCH] input synaptics-rmi4: stop scanning PDT after blank page
From: Christopher Heiny @ 2014-03-08  2:04 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
	Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
	David Herrmann, Jiri Kosina

When scanning the Page Descriptor Table, the end of the PDT is marked by a
page where the first PDT entry is empty.  Stop scanning when we find this
page.

Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Linux Walleij <linus.walleij@linaro.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>

---
 drivers/input/rmi4/rmi_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 2172c80..dd3ccf5 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -505,7 +505,8 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
 			return retval;
 	}
 
-	return data->f01_bootloader_mode ? RMI_SCAN_DONE : RMI_SCAN_CONTINUE;
+	return (data->f01_bootloader_mode || addr == pdt_start) ?
+					RMI_SCAN_DONE : RMI_SCAN_CONTINUE;
 }
 
 static int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox