* Re: [PATCH] Input: ims-pcu - fix error unwinding path in application mode
From: Dmitry Torokhov @ 2014-01-04 5:29 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: linux-input, Chris Healy, linux-kernel
In-Reply-To: <CAHQ1cqGZT7TOCX+5yPhWt1MpsSbAfbr07SProgSxrn7RO=7aig@mail.gmail.com>
On Fri, Jan 03, 2014 at 09:13:10PM -0800, Andrey Smirnov wrote:
> Dmitry,
>
> Do you want this patch to go separatly or do you want me to bundle it
> with my other changes(probably the first one)?
I'll apply it separately.
> Also, offtopic, do you want me to backport this changes to 2.6 kernel
> that IMS uses or would you do it?
It should apply as is, no backport needed as far as I can tell.
--
Dmitry
^ permalink raw reply
* [PATCH] Input: ims-pcu - remove unneeded get_unaligned_xxx
From: Dmitry Torokhov @ 2014-01-04 5:28 UTC (permalink / raw)
To: linux-input; +Cc: Andrey Smirnov, linux-kernel
pcu->cmd_buf[IMS_PCU_DATA_OFFSET] is word aligned so we do not need to use
get_unaligned_le16 to access it.
Also let's add build time check to make sure it stays aligned.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/misc/ims-pcu.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 513ecdf..b8f1029 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -116,6 +116,8 @@ struct ims_pcu {
bool setup_complete; /* Input and LED devices have been created */
};
+#define IMS_PCU_CHECK_CMD_BUF_ALIGNMENT(offset, alignment) \
+ IS_ALIGNED(offsetof(struct ims_pcu, cmd_buf[offset]), alignment)
/*********************************************************************
* Buttons Input device support *
@@ -999,8 +1001,10 @@ ims_pcu_backlight_get_brightness(struct led_classdev *cdev)
/* Assume the LED is OFF */
brightness = LED_OFF;
} else {
- brightness =
- get_unaligned_le16(&pcu->cmd_buf[IMS_PCU_DATA_OFFSET]);
+ BUILD_BUG_ON(!IMS_PCU_CHECK_CMD_BUF_ALIGNMENT(
+ IMS_PCU_DATA_OFFSET, 2));
+ brightness = le16_to_cpup(
+ (__le16 *)&pcu->cmd_buf[IMS_PCU_DATA_OFFSET]);
}
mutex_unlock(&pcu->cmd_mutex);
--
1.8.4.2
--
Dmitry
^ permalink raw reply related
* Re: Another ALPS touchpad
From: Tommy Will @ 2014-01-04 5:27 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Yunkang Tang, Dylan Paul Thurston, linux-input
In-Reply-To: <5612072.Tm4gq8bhLq@dtor-d630.eng.vmware.com>
Hi Dmitry,
> Do you have anything in works to support this new model? Or it is
> not an ALPS device at all?
Yes, it's an ALPS device and we're now preparing the patch for this new one.
BTW, since it's a ClickPad without seperate buttons, simple resting
finger logic would be added.
--
Best Regards,
Tommy
^ permalink raw reply
* Re: [PATCH] Input: ims-pcu - fix error unwinding path in application mode
From: Andrey Smirnov @ 2014-01-04 5:13 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Chris Healy, linux-kernel
In-Reply-To: <20140104050058.GA20652@core.coreip.homeip.net>
Dmitry,
Do you want this patch to go separatly or do you want me to bundle it
with my other changes(probably the first one)?
Also, offtopic, do you want me to backport this changes to 2.6 kernel
that IMS uses or would you do it?
Thanks,
On Fri, Jan 3, 2014 at 9:00 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> We first create backlight and then input devices so we shoudl destroy them
> in opposite order when handling errors.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> Andrey, this fixup was split off from the patch I sent earlier today.
>
> drivers/input/misc/ims-pcu.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 4244f47..513ecdf 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1929,10 +1929,10 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
>
> return 0;
>
> -err_destroy_backlight:
> - ims_pcu_destroy_backlight(pcu);
> err_destroy_buttons:
> ims_pcu_destroy_buttons(pcu);
> +err_destroy_backlight:
> + ims_pcu_destroy_backlight(pcu);
> return error;
> }
>
> --
> 1.8.4.2
>
>
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH v2] ims-pcu: Add commands supported by the new version of the FW
From: Andrey Smirnov @ 2014-01-04 5:03 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <20140104044433.GA20272@core.coreip.homeip.net>
On Fri, Jan 3, 2014 at 8:44 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Jan 03, 2014 at 08:24:17PM -0800, Andrey Smirnov wrote:
>> On Fri, Jan 3, 2014 at 5:39 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > Hi Andrey,
>> >
>> > On Wed, Jan 01, 2014 at 04:47:01PM -0800, Andrey Smirnov wrote:
>> >> New version of the PCU firmware supports two new commands:
>> >> - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
>> >> registers of one finger navigation(OFN) chip present on the device
>> >> - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
>> >> registers of said chip.
>> >>
>> >> This commit adds two helper functions to use those commands and sysfs
>> >> attributes to use them. It also exposes some OFN configuration
>> >> parameters via sysfs.
>> >
>> > Thank you for making the changes. I do not still quite like the games we
>> > play with the OFN attributes, how about the patch below (on top of
>> > yours)?
>> >
>>
>> Yeah, I agree I like it the "two separate sysfs groups" group approach
>> better. The only small nitpick about your patch is that I think we
>> should use "get_unaligned_le16" instead of "le16_to_cpup"(In case
>> anyone decides to run the driver on SuperH or C6x DSPs from TI :-)).
>> Let me test it and if everything works as expected I'll apply you
>> patch, convert it to "get_unaligned_le16", squash and send v3 of the
>> patch.
>
> Why do we need get_unaligned_le16()? As far as I can see pcu->cmd_buf is
> aligned and therefore pcu->cmd_buf[2] is also aligned on word boundary.
* The "pcu" structure is allocated with kmalloc which doesn't give any
guarantees about address alignment.
* I am not sure if the cmd_buf field in that structure is aligned, and
even if it is, any future changes to that structure may shift its
offset.
* Also even if the data we are interested in is aligned on 2-byte
border, I think all those architectures require 4-byte border
alignment.
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* [PATCH] Input: ims-pcu - fix error unwinding path in application mode
From: Dmitry Torokhov @ 2014-01-04 5:00 UTC (permalink / raw)
To: linux-input; +Cc: Andrey Smirnov, Chris Healy, linux-kernel
We first create backlight and then input devices so we shoudl destroy them
in opposite order when handling errors.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
Andrey, this fixup was split off from the patch I sent earlier today.
drivers/input/misc/ims-pcu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 4244f47..513ecdf 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1929,10 +1929,10 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
return 0;
-err_destroy_backlight:
- ims_pcu_destroy_backlight(pcu);
err_destroy_buttons:
ims_pcu_destroy_buttons(pcu);
+err_destroy_backlight:
+ ims_pcu_destroy_backlight(pcu);
return error;
}
--
1.8.4.2
--
Dmitry
^ permalink raw reply related
* Re: [PATCH v2] ims-pcu: Add commands supported by the new version of the FW
From: Dmitry Torokhov @ 2014-01-04 4:44 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: linux-input, linux-kernel
In-Reply-To: <CAHQ1cqEvXn3u2joA7dNaiz6fmHELnKjZTWF2F-7vvvBAx+9Drw@mail.gmail.com>
On Fri, Jan 03, 2014 at 08:24:17PM -0800, Andrey Smirnov wrote:
> On Fri, Jan 3, 2014 at 5:39 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Andrey,
> >
> > On Wed, Jan 01, 2014 at 04:47:01PM -0800, Andrey Smirnov wrote:
> >> New version of the PCU firmware supports two new commands:
> >> - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
> >> registers of one finger navigation(OFN) chip present on the device
> >> - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
> >> registers of said chip.
> >>
> >> This commit adds two helper functions to use those commands and sysfs
> >> attributes to use them. It also exposes some OFN configuration
> >> parameters via sysfs.
> >
> > Thank you for making the changes. I do not still quite like the games we
> > play with the OFN attributes, how about the patch below (on top of
> > yours)?
> >
>
> Yeah, I agree I like it the "two separate sysfs groups" group approach
> better. The only small nitpick about your patch is that I think we
> should use "get_unaligned_le16" instead of "le16_to_cpup"(In case
> anyone decides to run the driver on SuperH or C6x DSPs from TI :-)).
> Let me test it and if everything works as expected I'll apply you
> patch, convert it to "get_unaligned_le16", squash and send v3 of the
> patch.
Why do we need get_unaligned_le16()? As far as I can see pcu->cmd_buf is
aligned and therefore pcu->cmd_buf[2] is also aligned on word boundary.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] ims-pcu: Add commands supported by the new version of the FW
From: Andrey Smirnov @ 2014-01-04 4:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <20140104013957.GA19702@core.coreip.homeip.net>
On Fri, Jan 3, 2014 at 5:39 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Andrey,
>
> On Wed, Jan 01, 2014 at 04:47:01PM -0800, Andrey Smirnov wrote:
>> New version of the PCU firmware supports two new commands:
>> - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
>> registers of one finger navigation(OFN) chip present on the device
>> - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
>> registers of said chip.
>>
>> This commit adds two helper functions to use those commands and sysfs
>> attributes to use them. It also exposes some OFN configuration
>> parameters via sysfs.
>
> Thank you for making the changes. I do not still quite like the games we
> play with the OFN attributes, how about the patch below (on top of
> yours)?
>
Yeah, I agree I like it the "two separate sysfs groups" group approach
better. The only small nitpick about your patch is that I think we
should use "get_unaligned_le16" instead of "le16_to_cpup"(In case
anyone decides to run the driver on SuperH or C6x DSPs from TI :-)).
Let me test it and if everything works as expected I'll apply you
patch, convert it to "get_unaligned_le16", squash and send v3 of the
patch.
Thanks,
Andrey
^ permalink raw reply
* Re: [PATCH v2] ims-pcu: Add commands supported by the new version of the FW
From: Dmitry Torokhov @ 2014-01-04 1:39 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: linux-input, linux-kernel
In-Reply-To: <1388623621-32245-1-git-send-email-andrew.smirnov@gmail.com>
Hi Andrey,
On Wed, Jan 01, 2014 at 04:47:01PM -0800, Andrey Smirnov wrote:
> New version of the PCU firmware supports two new commands:
> - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
> registers of one finger navigation(OFN) chip present on the device
> - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
> registers of said chip.
>
> This commit adds two helper functions to use those commands and sysfs
> attributes to use them. It also exposes some OFN configuration
> parameters via sysfs.
Thank you for making the changes. I do not still quite like the games we
play with the OFN attributes, how about the patch below (on top of
yours)?
Thanks.
--
Dmitry
Input: ims-pci - misc changes
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Split OFN code into separate named attribute group, tidy up attribute
handling.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/misc/ims-pcu.c | 330 ++++++++++++++++++++----------------------
1 file changed, 156 insertions(+), 174 deletions(-)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 4244f47..bd25a78 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1224,11 +1224,87 @@ ims_pcu_update_firmware_status_show(struct device *dev,
static DEVICE_ATTR(update_firmware_status, S_IRUGO,
ims_pcu_update_firmware_status_show, NULL);
-enum pcu_ofn_offsets {
- OFN_REG_RESULT_LSB_OFFSET = 2,
- OFN_REG_RESULT_MSB_OFFSET = 3,
+static struct attribute *ims_pcu_attrs[] = {
+ &ims_pcu_attr_part_number.dattr.attr,
+ &ims_pcu_attr_serial_number.dattr.attr,
+ &ims_pcu_attr_date_of_manufacturing.dattr.attr,
+ &ims_pcu_attr_fw_version.dattr.attr,
+ &ims_pcu_attr_bl_version.dattr.attr,
+ &ims_pcu_attr_reset_reason.dattr.attr,
+ &dev_attr_reset_device.attr,
+ &dev_attr_update_firmware.attr,
+ &dev_attr_update_firmware_status.attr,
+ NULL,
+};
+
+static umode_t ims_pcu_is_attr_visible(struct kobject *kobj,
+ struct attribute *attr, int n)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct usb_interface *intf = to_usb_interface(dev);
+ struct ims_pcu *pcu = usb_get_intfdata(intf);
+ umode_t mode = attr->mode;
+
+ if (pcu->bootloader_mode) {
+ if (attr != &dev_attr_update_firmware_status.attr &&
+ attr != &dev_attr_update_firmware.attr &&
+ attr != &dev_attr_reset_device.attr) {
+ mode = 0;
+ }
+ } else {
+ if (attr == &dev_attr_update_firmware_status.attr)
+ mode = 0;
+ }
+
+ return mode;
+}
+
+static struct attribute_group ims_pcu_attr_group = {
+ .is_visible = ims_pcu_is_attr_visible,
+ .attrs = ims_pcu_attrs,
};
+/* Support for a separate OFN attribute group */
+
+#define OFN_REG_RESULT_OFFSET 2
+
+static int ims_pcu_read_ofn_config(struct ims_pcu *pcu, u8 addr, u8 *data)
+{
+ int error;
+ u16 result;
+
+ error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
+ &addr, sizeof(addr));
+ if (error)
+ return error;
+
+ result = le16_to_cpup((__le16 *)&pcu->cmd_buf[OFN_REG_RESULT_OFFSET]);
+ if ((s16)result < 0)
+ return -EIO;
+
+ /* We only need LSB */
+ *data = pcu->cmd_buf[OFN_REG_RESULT_OFFSET];
+ return 0;
+}
+
+static int ims_pcu_write_ofn_config(struct ims_pcu *pcu, u8 addr, u8 data)
+{
+ u8 buffer[] = { addr, data };
+ int error;
+ u16 result;
+
+ error = ims_pcu_execute_command(pcu, OFN_SET_CONFIG,
+ &buffer, sizeof(buffer));
+ if (error)
+ return error;
+
+ result = le16_to_cpup((__le16 *)&pcu->cmd_buf[OFN_REG_RESULT_OFFSET]);
+ if ((s16)result < 0)
+ return -EIO;
+
+ return 0;
+}
+
static ssize_t ims_pcu_ofn_reg_data_show(struct device *dev,
struct device_attribute *dattr,
char *buf)
@@ -1236,24 +1312,16 @@ static ssize_t ims_pcu_ofn_reg_data_show(struct device *dev,
struct usb_interface *intf = to_usb_interface(dev);
struct ims_pcu *pcu = usb_get_intfdata(intf);
int error;
+ u8 data;
mutex_lock(&pcu->cmd_mutex);
-
- error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
- &pcu->ofn_reg_addr,
- sizeof(pcu->ofn_reg_addr));
- if (error >= 0) {
- const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
- pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
- if (result < 0)
- error = result;
- else
- error = scnprintf(buf, PAGE_SIZE, "%x\n", result);
- }
-
+ error = ims_pcu_read_ofn_config(pcu, pcu->ofn_reg_addr, &data);
mutex_unlock(&pcu->cmd_mutex);
- return error;
+ if (error)
+ return error;
+
+ return scnprintf(buf, PAGE_SIZE, "%x\n", data);
}
static ssize_t ims_pcu_ofn_reg_data_store(struct device *dev,
@@ -1264,33 +1332,19 @@ static ssize_t ims_pcu_ofn_reg_data_store(struct device *dev,
struct ims_pcu *pcu = usb_get_intfdata(intf);
int error;
u8 value;
- u8 buffer[2];
- s16 result;
error = kstrtou8(buf, 0, &value);
if (error)
return error;
- buffer[0] = pcu->ofn_reg_addr;
- buffer[1] = value;
-
mutex_lock(&pcu->cmd_mutex);
-
- error = ims_pcu_execute_command(pcu, OFN_SET_CONFIG,
- &buffer, sizeof(buffer));
-
- result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
- pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
-
+ error = ims_pcu_write_ofn_config(pcu, pcu->ofn_reg_addr, value);
mutex_unlock(&pcu->cmd_mutex);
- if (!error && result < 0)
- error = -ENOENT;
-
return error ?: count;
}
-static DEVICE_ATTR(ofn_reg_data, S_IRUGO | S_IWUSR,
+static DEVICE_ATTR(reg_data, S_IRUGO | S_IWUSR,
ims_pcu_ofn_reg_data_show, ims_pcu_ofn_reg_data_store);
static ssize_t ims_pcu_ofn_reg_addr_show(struct device *dev,
@@ -1328,47 +1382,47 @@ static ssize_t ims_pcu_ofn_reg_addr_store(struct device *dev,
return error ?: count;
}
-static DEVICE_ATTR(ofn_reg_addr, S_IRUGO | S_IWUSR,
+static DEVICE_ATTR(reg_addr, S_IRUGO | S_IWUSR,
ims_pcu_ofn_reg_addr_show, ims_pcu_ofn_reg_addr_store);
-static ssize_t ims_pcu_ofn_bit_show(u8 addr, u8 nr,
- struct device *dev,
+struct ims_pcu_ofn_bit_attribute {
+ struct device_attribute dattr;
+ u8 addr;
+ u8 nr;
+};
+
+static ssize_t ims_pcu_ofn_bit_show(struct device *dev,
struct device_attribute *dattr,
char *buf)
{
struct usb_interface *intf = to_usb_interface(dev);
struct ims_pcu *pcu = usb_get_intfdata(intf);
+ struct ims_pcu_ofn_bit_attribute *attr =
+ container_of(dattr, struct ims_pcu_ofn_bit_attribute, dattr);
int error;
+ u8 data;
mutex_lock(&pcu->cmd_mutex);
+ error = ims_pcu_read_ofn_config(pcu, attr->addr, &data);
+ mutex_unlock(&pcu->cmd_mutex);
- error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
- &addr, sizeof(addr));
- if (error >= 0) {
- const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
- pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
- if (result < 0)
- error = result;
- else
- error = scnprintf(buf, PAGE_SIZE, "%d\n",
- !!(result & (1 << nr)));
- }
+ if (error)
+ return error;
- mutex_unlock(&pcu->cmd_mutex);
- return error;
+ return scnprintf(buf, PAGE_SIZE, "%d\n", !!(data & (1 << attr->nr)));
}
-static ssize_t ims_pcu_ofn_bit_store(u8 addr, u8 nr,
- struct device *dev,
+static ssize_t ims_pcu_ofn_bit_store(struct device *dev,
struct device_attribute *dattr,
const char *buf, size_t count)
{
struct usb_interface *intf = to_usb_interface(dev);
struct ims_pcu *pcu = usb_get_intfdata(intf);
+ struct ims_pcu_ofn_bit_attribute *attr =
+ container_of(dattr, struct ims_pcu_ofn_bit_attribute, dattr);
int error;
int value;
- u8 contents;
-
+ u8 data;
error = kstrtoint(buf, 0, &value);
if (error)
@@ -1379,135 +1433,54 @@ static ssize_t ims_pcu_ofn_bit_store(u8 addr, u8 nr,
mutex_lock(&pcu->cmd_mutex);
- error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
- &addr, sizeof(addr));
- if (error < 0)
- goto exit;
-
- {
- const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
- pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
- if (result < 0) {
- error = result;
- goto exit;
- }
- contents = (u8) result;
- }
-
- if (value)
- contents |= 1 << nr;
- else
- contents &= ~(1 << nr);
+ error = ims_pcu_read_ofn_config(pcu, attr->addr, &data);
+ if (!error) {
+ if (value)
+ data |= 1U << attr->nr;
+ else
+ data &= ~(1U << attr->nr);
- {
- const u8 buffer[] = { addr, contents };
- error = ims_pcu_execute_command(pcu, OFN_SET_CONFIG,
- &buffer, sizeof(buffer));
+ error = ims_pcu_write_ofn_config(pcu, attr->addr, data);
}
-exit:
mutex_unlock(&pcu->cmd_mutex);
- if (!error) {
- const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
- pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
- error = result;
- }
-
return error ?: count;
}
+#define IMS_PCU_OFN_BIT_ATTR(_field, _addr, _nr) \
+struct ims_pcu_ofn_bit_attribute ims_pcu_ofn_attr_##_field = { \
+ .dattr = __ATTR(_field, S_IWUSR | S_IRUGO, \
+ ims_pcu_ofn_bit_show, ims_pcu_ofn_bit_store), \
+ .addr = _addr, \
+ .nr = _nr, \
+}
-#define IMS_PCU_BIT_ATTR(name, addr, nr) \
- static ssize_t ims_pcu_##name##_show(struct device *dev, \
- struct device_attribute *dattr, \
- char *buf) \
- { \
- return ims_pcu_ofn_bit_show(addr, nr, dev, dattr, buf); \
- } \
- \
- static ssize_t ims_pcu_##name##_store(struct device *dev, \
- struct device_attribute *dattr, \
- const char *buf, size_t count) \
- { \
- return ims_pcu_ofn_bit_store(addr, nr, dev, dattr, buf, count); \
- } \
- \
- static DEVICE_ATTR(name, S_IRUGO | S_IWUSR, \
- ims_pcu_##name##_show, ims_pcu_##name##_store)
-
-IMS_PCU_BIT_ATTR(ofn_engine_enable, 0x60, 7);
-IMS_PCU_BIT_ATTR(ofn_speed_enable, 0x60, 6);
-IMS_PCU_BIT_ATTR(ofn_assert_enable, 0x60, 5);
-IMS_PCU_BIT_ATTR(ofn_xyquant_enable, 0x60, 4);
-IMS_PCU_BIT_ATTR(ofn_xyscale_enable, 0x60, 1);
-
-IMS_PCU_BIT_ATTR(ofn_scale_x2, 0x63, 6);
-IMS_PCU_BIT_ATTR(ofn_scale_y2, 0x63, 7);
-
-static struct attribute *ims_pcu_attrs[] = {
- &ims_pcu_attr_part_number.dattr.attr,
- &ims_pcu_attr_serial_number.dattr.attr,
- &ims_pcu_attr_date_of_manufacturing.dattr.attr,
- &ims_pcu_attr_fw_version.dattr.attr,
- &ims_pcu_attr_bl_version.dattr.attr,
- &ims_pcu_attr_reset_reason.dattr.attr,
- &dev_attr_reset_device.attr,
- &dev_attr_update_firmware.attr,
- &dev_attr_update_firmware_status.attr,
-
-#define IMS_PCU_ATTRS_OFN_START_OFFSET (9)
-
- &dev_attr_ofn_reg_data.attr,
- &dev_attr_ofn_reg_addr.attr,
- &dev_attr_ofn_engine_enable.attr,
- &dev_attr_ofn_speed_enable.attr,
- &dev_attr_ofn_assert_enable.attr,
- &dev_attr_ofn_xyquant_enable.attr,
- &dev_attr_ofn_xyscale_enable.attr,
- &dev_attr_ofn_scale_x2.attr,
- &dev_attr_ofn_scale_y2.attr,
+static IMS_PCU_OFN_BIT_ATTR(engine_enable, 0x60, 7);
+static IMS_PCU_OFN_BIT_ATTR(speed_enable, 0x60, 6);
+static IMS_PCU_OFN_BIT_ATTR(assert_enable, 0x60, 5);
+static IMS_PCU_OFN_BIT_ATTR(xyquant_enable, 0x60, 4);
+static IMS_PCU_OFN_BIT_ATTR(xyscale_enable, 0x60, 1);
+
+static IMS_PCU_OFN_BIT_ATTR(scale_x2, 0x63, 6);
+static IMS_PCU_OFN_BIT_ATTR(scale_y2, 0x63, 7);
+
+static struct attribute *ims_pcu_ofn_attrs[] = {
+ &dev_attr_reg_data.attr,
+ &dev_attr_reg_addr.attr,
+ &ims_pcu_ofn_attr_engine_enable.dattr.attr,
+ &ims_pcu_ofn_attr_speed_enable.dattr.attr,
+ &ims_pcu_ofn_attr_assert_enable.dattr.attr,
+ &ims_pcu_ofn_attr_xyquant_enable.dattr.attr,
+ &ims_pcu_ofn_attr_xyscale_enable.dattr.attr,
+ &ims_pcu_ofn_attr_scale_x2.dattr.attr,
+ &ims_pcu_ofn_attr_scale_y2.dattr.attr,
NULL
};
-static umode_t ims_pcu_is_attr_visible(struct kobject *kobj,
- struct attribute *attr, int n)
-{
- struct device *dev = container_of(kobj, struct device, kobj);
- struct usb_interface *intf = to_usb_interface(dev);
- struct ims_pcu *pcu = usb_get_intfdata(intf);
- umode_t mode = attr->mode;
-
- if (pcu->bootloader_mode) {
- if (attr != &dev_attr_update_firmware_status.attr &&
- attr != &dev_attr_update_firmware.attr &&
- attr != &dev_attr_reset_device.attr) {
- mode = 0;
- }
- } else {
- if (attr == &dev_attr_update_firmware_status.attr) {
- mode = 0;
- } else if (pcu->setup_complete && pcu->device_id == 5) {
- /*
- PCU-B devices, both GEN_1 and GEN_2(device_id == 5),
- have no OFN sensor so exposing those attributes
- for them does not make any sense
- */
- int i;
- for (i = IMS_PCU_ATTRS_OFN_START_OFFSET; ims_pcu_attrs[i]; i++)
- if (attr == ims_pcu_attrs[i]) {
- mode = 0;
- break;
- }
- }
- }
-
- return mode;
-}
-
-static struct attribute_group ims_pcu_attr_group = {
- .is_visible = ims_pcu_is_attr_visible,
- .attrs = ims_pcu_attrs,
+static struct attribute_group ims_pcu_ofn_attr_group = {
+ .name = "ofn",
+ .attrs = ims_pcu_ofn_attrs,
};
static void ims_pcu_irq(struct urb *urb)
@@ -1908,6 +1881,13 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
/* Device appears to be operable, complete initialization */
pcu->device_no = atomic_inc_return(&device_no) - 1;
+ if (pcu->device_id != 5) {
+ error = sysfs_create_group(&pcu->dev->kobj,
+ &ims_pcu_attr_group);
+ if (error)
+ return error;
+ }
+
error = ims_pcu_setup_backlight(pcu);
if (error)
return error;
@@ -1925,14 +1905,12 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
pcu->setup_complete = true;
- sysfs_update_group(&pcu->dev->kobj, &ims_pcu_attr_group);
-
return 0;
-err_destroy_backlight:
- ims_pcu_destroy_backlight(pcu);
err_destroy_buttons:
ims_pcu_destroy_buttons(pcu);
+err_destroy_backlight:
+ ims_pcu_destroy_backlight(pcu);
return error;
}
@@ -1946,6 +1924,10 @@ static void ims_pcu_destroy_application_mode(struct ims_pcu *pcu)
ims_pcu_destroy_gamepad(pcu);
ims_pcu_destroy_buttons(pcu);
ims_pcu_destroy_backlight(pcu);
+
+ if (pcu->device_id != 5)
+ sysfs_remove_group(&pcu->dev->kobj,
+ &ims_pcu_ofn_attr_group);
}
}
^ permalink raw reply related
* Re: Another ALPS touchpad
From: Dmitry Torokhov @ 2014-01-03 23:39 UTC (permalink / raw)
To: Yunkang Tang, Tommy Will; +Cc: Dylan Paul Thurston, linux-input
In-Reply-To: <20140103045915.GB30008@whitehail.bostoncoop.net>
On Thursday, January 02, 2014 11:59:15 PM Dylan Paul Thurston wrote:
> I just bought a HP Revolve 810 G1 laptop. (This is a convertible
> ultrabook, normally sold with Windows 8.) The touchpad seems to be
> another ALPS touchpad, not yet recognized by the kernel; I tried the
> Debian package of 3.12 and linux-next-20131224 compiled from
> source. Without the proper driver, it is unusuably sensisitive,
> registering clicks when my hands are well away from the touchpad.
> The relevant line from dmesg is
>
> [ 2.776630] psmouse serio4: alps: Unknown ALPS touchpad: E7=73 03 0a,
> EC=88 b3 18
>
> Should I try hacking drivers/input/mouse/alps.c to see if one of the
> existing protocols works?
Tommy, Yunkang,
Do you have anything in works to support this new model? Or it is
not an ALPS device at all?
Thanks!
--
Dmitry
^ permalink raw reply
* [PATCH 2/2] HID: sony: Add LED controls for Dualshock 4
From: Frank Praznik @ 2014-01-03 21:03 UTC (permalink / raw)
To: linux-input
This patch builds on the previous patch and adds controls for the light
bar on the Dualshock 4. The light bar contains a red, green and blue LED
that can independently vary in brightness from 0 to 255. The LED system
in the module had to be extended to support a full byte per led for
storing the brightness level as well as storing a count of LEDs on each
device. Like the Sixaxis, LED status is set in the worker function by
setting certain bytes in the data packet.
Like the force feedback patch, this meant to be applied against the
for-3.14/sony branch in jiros/hid.git
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
drivers/hid/hid-sony.c | 77 ++++++++++++++++++++++++++++++++------------------
1 file changed, 49 insertions(+), 28 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index f42c866..445b5f2 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -40,7 +40,9 @@
#define PS3REMOTE BIT(4)
#define DUALSHOCK4_CONTROLLER BIT(5)
-#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER)
+#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER | DUALSHOCK4_CONTROLLER)
+
+#define MAX_LEDS 4
static const u8 sixaxis_rdesc_fixup[] = {
0x95, 0x13, 0x09, 0x01, 0x81, 0x02, 0x95, 0x0C,
@@ -227,7 +229,7 @@ static const unsigned int buzz_keymap[] = {
struct sony_sc {
struct hid_device *hdev;
- struct led_classdev *leds[4];
+ struct led_classdev *leds[MAX_LEDS];
unsigned long quirks;
struct work_struct state_worker;
@@ -236,7 +238,8 @@ struct sony_sc {
__u8 right;
#endif
- __u8 led_state;
+ __u8 led_state[MAX_LEDS];
+ __u8 led_count;
};
static __u8 *ps3remote_fixup(struct hid_device *hdev, __u8 *rdesc,
@@ -447,7 +450,7 @@ static int sixaxis_set_operational_bt(struct hid_device *hdev)
return hdev->hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
}
-static void buzz_set_leds(struct hid_device *hdev, int leds)
+static void buzz_set_leds(struct hid_device *hdev, const __u8 *leds)
{
struct list_head *report_list =
&hdev->report_enum[HID_OUTPUT_REPORT].report_list;
@@ -456,23 +459,27 @@ static void buzz_set_leds(struct hid_device *hdev, int leds)
__s32 *value = report->field[0]->value;
value[0] = 0x00;
- value[1] = (leds & 1) ? 0xff : 0x00;
- value[2] = (leds & 2) ? 0xff : 0x00;
- value[3] = (leds & 4) ? 0xff : 0x00;
- value[4] = (leds & 8) ? 0xff : 0x00;
+ value[1] = leds[0] ? 0xff : 0x00;
+ value[2] = leds[1] ? 0xff : 0x00;
+ value[3] = leds[2] ? 0xff : 0x00;
+ value[4] = leds[3] ? 0xff : 0x00;
value[5] = 0x00;
value[6] = 0x00;
hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
}
-static void sony_set_leds(struct hid_device *hdev, __u8 leds)
+static void sony_set_leds(struct hid_device *hdev, const __u8 *leds, int count)
{
struct sony_sc *drv_data = hid_get_drvdata(hdev);
-
- if (drv_data->quirks & BUZZ_CONTROLLER) {
+ int n;
+
+ BUG_ON(count > MAX_LEDS);
+
+ if (drv_data->quirks & BUZZ_CONTROLLER && count == 4) {
buzz_set_leds(hdev, leds);
- } else if (drv_data->quirks & SIXAXIS_CONTROLLER_USB) {
- drv_data->led_state = leds;
+ } else if ((drv_data->quirks & SIXAXIS_CONTROLLER_USB) || (drv_data->quirks & DUALSHOCK4_CONTROLLER)) {
+ for(n = 0; n < count; n++)
+ drv_data->led_state[n] = leds[n];
schedule_work(&drv_data->state_worker);
}
}
@@ -492,15 +499,11 @@ static void sony_led_set_brightness(struct led_classdev *led,
return;
}
- for (n = 0; n < 4; n++) {
+ for (n = 0; n < drv_data->led_count; n++) {
if (led == drv_data->leds[n]) {
- int on = !!(drv_data->led_state & (1 << n));
- if (value == LED_OFF && on) {
- drv_data->led_state &= ~(1 << n);
- sony_set_leds(hdev, drv_data->led_state);
- } else if (value != LED_OFF && !on) {
- drv_data->led_state |= (1 << n);
- sony_set_leds(hdev, drv_data->led_state);
+ if (value != drv_data->led_state[n]) {
+ drv_data->led_state[n] = value;
+ sony_set_leds(hdev, drv_data->led_state, drv_data->led_count);
}
break;
}
@@ -522,9 +525,9 @@ static enum led_brightness sony_led_get_brightness(struct led_classdev *led)
return LED_OFF;
}
- for (n = 0; n < 4; n++) {
+ for (n = 0; n < drv_data->led_count; n++) {
if (led == drv_data->leds[n]) {
- on = !!(drv_data->led_state & (1 << n));
+ on = !!(drv_data->led_state[n]);
break;
}
}
@@ -541,7 +544,7 @@ static void sony_leds_remove(struct hid_device *hdev)
drv_data = hid_get_drvdata(hdev);
BUG_ON(!(drv_data->quirks & SONY_LED_SUPPORT));
- for (n = 0; n < 4; n++) {
+ for (n = 0; n < drv_data->led_count; n++) {
led = drv_data->leds[n];
drv_data->leds[n] = NULL;
if (!led)
@@ -549,17 +552,21 @@ static void sony_leds_remove(struct hid_device *hdev)
led_classdev_unregister(led);
kfree(led);
}
+
+ drv_data->led_count = 0;
}
static int sony_leds_init(struct hid_device *hdev)
{
struct sony_sc *drv_data;
int n, ret = 0;
+ int max_brightness = 1;
struct led_classdev *led;
size_t name_sz;
char *name;
size_t name_len;
const char *name_fmt;
+ static const __u8 initial_values[MAX_LEDS] = { 0x00, 0x00, 0x00, 0x00 };
drv_data = hid_get_drvdata(hdev);
BUG_ON(!(drv_data->quirks & SONY_LED_SUPPORT));
@@ -574,15 +581,22 @@ static int sony_leds_init(struct hid_device *hdev)
name_len = strlen("::sony#");
name_fmt = "%s::sony%d";
}
+
+ if (drv_data->quirks & DUALSHOCK4_CONTROLLER) {
+ drv_data->led_count = 3;
+ max_brightness = 255;
+ }
+ else
+ drv_data->led_count = 4;
/* Clear LEDs as we have no way of reading their initial state. This is
* only relevant if the driver is loaded after somebody actively set the
* LEDs to on */
- sony_set_leds(hdev, 0x00);
+ sony_set_leds(hdev, initial_values, drv_data->led_count);
name_sz = strlen(dev_name(&hdev->dev)) + name_len + 1;
- for (n = 0; n < 4; n++) {
+ for (n = 0; n < drv_data->led_count; n++) {
led = kzalloc(sizeof(struct led_classdev) + name_sz, GFP_KERNEL);
if (!led) {
hid_err(hdev, "Couldn't allocate memory for LED %d\n", n);
@@ -594,7 +608,7 @@ static int sony_leds_init(struct hid_device *hdev)
snprintf(name, name_sz, name_fmt, dev_name(&hdev->dev), n + 1);
led->name = name;
led->brightness = 0;
- led->max_brightness = 1;
+ led->max_brightness = max_brightness;
led->brightness_get = sony_led_get_brightness;
led->brightness_set = sony_led_set_brightness;
@@ -635,7 +649,10 @@ static void sixaxis_state_worker(struct work_struct *work)
buf[5] = sc->left;
#endif
- buf[10] |= (sc->led_state & 0xf) << 1;
+ buf[10] |= sc->led_state[0] << 1;
+ buf[10] |= sc->led_state[1] << 2;
+ buf[10] |= sc->led_state[2] << 3;
+ buf[10] |= sc->led_state[3] << 4;
sc->hdev->hid_output_raw_report(sc->hdev, buf, sizeof(buf),
HID_OUTPUT_REPORT);
@@ -660,6 +677,10 @@ static void dualshock4_state_worker(struct work_struct *work)
buf[5] = sc->left;
#endif
+ buf[6] = sc->led_state[0];
+ buf[7] = sc->led_state[1];
+ buf[8] = sc->led_state[2];
+
sc->hdev->hid_output_raw_report(sc->hdev, buf, sizeof(buf),
HID_OUTPUT_REPORT);
}
--
1.8.3.2
^ permalink raw reply related
* [PATCH 1/2] HID: sony: Add force feedback for the Dualshock 4
From: Frank Praznik @ 2014-01-03 21:03 UTC (permalink / raw)
To: linux-input
This patch adds force feedback support for Sony's Dualshock 4 controller.
It does this by adding a device ID for the new controller and creating a
new dualshock4_worker function to send data commands formatted for the
controller. Unlike the Sixaxis, the Dualshock 4 requires a magnitude
value for the small motor so the actual value is now sent to the worker
function and the sixaxis worker was modified to clamp it to 1 or 0 as required.
This patch is built against the for-3.14/sony branch in jikos/hid.git and
is compatible with the recent fixes.
Signed-off-by Frank Praznik <frank.praznik@oh.rr.com>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-sony.c | 41 +++++++++++++++++++++++++++++++++++++----
2 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 60336f06..ebbd292 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -769,6 +769,7 @@
#define USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER 0x042f
#define USB_DEVICE_ID_SONY_BUZZ_CONTROLLER 0x0002
#define USB_DEVICE_ID_SONY_WIRELESS_BUZZ_CONTROLLER 0x1000
+#define USB_DEVICE_ID_SONY_PS4_CONTROLLER 0x05c4
#define USB_VENDOR_ID_SOUNDGRAPH 0x15c2
#define USB_DEVICE_ID_SOUNDGRAPH_IMON_FIRST 0x0034
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index f57ab5e..f42c866 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -38,6 +38,7 @@
#define SIXAXIS_CONTROLLER_BT BIT(2)
#define BUZZ_CONTROLLER BIT(3)
#define PS3REMOTE BIT(4)
+#define DUALSHOCK4_CONTROLLER BIT(5)
#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER)
@@ -615,7 +616,7 @@ error_leds:
return ret;
}
-static void sony_state_worker(struct work_struct *work)
+static void sixaxis_state_worker(struct work_struct *work)
{
struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
unsigned char buf[] = {
@@ -630,7 +631,7 @@ static void sony_state_worker(struct work_struct *work)
};
#ifdef CONFIG_SONY_FF
- buf[3] = sc->right;
+ buf[3] = sc->right ? 1 : 0;
buf[5] = sc->left;
#endif
@@ -640,6 +641,29 @@ static void sony_state_worker(struct work_struct *work)
HID_OUTPUT_REPORT);
}
+static void dualshock4_state_worker(struct work_struct *work)
+{
+ struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
+ unsigned char buf[] = {
+ 0x05,
+ 0x03, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00,
+ };
+
+#ifdef CONFIG_SONY_FF
+ buf[4] = sc->right;
+ buf[5] = sc->left;
+#endif
+
+ sc->hdev->hid_output_raw_report(sc->hdev, buf, sizeof(buf),
+ HID_OUTPUT_REPORT);
+}
+
#ifdef CONFIG_SONY_FF
static int sony_play_effect(struct input_dev *dev, void *data,
struct ff_effect *effect)
@@ -651,7 +675,7 @@ static int sony_play_effect(struct input_dev *dev, void *data,
return 0;
sc->left = effect->u.rumble.strong_magnitude / 256;
- sc->right = effect->u.rumble.weak_magnitude ? 1 : 0;
+ sc->right = effect->u.rumble.weak_magnitude / 256;
schedule_work(&sc->state_worker);
return 0;
@@ -724,10 +748,14 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
ret = sixaxis_set_operational_usb(hdev);
- INIT_WORK(&sc->state_worker, sony_state_worker);
+ INIT_WORK(&sc->state_worker, sixaxis_state_worker);
}
else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
ret = sixaxis_set_operational_bt(hdev);
+ else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
+ ret = 0;
+ INIT_WORK(&sc->state_worker, dualshock4_state_worker);
+ }
else
ret = 0;
@@ -787,6 +815,11 @@ static const struct hid_device_id sony_devices[] = {
/* Logitech Harmony Adapter for PS3 */
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_HARMONY_PS3),
.driver_data = PS3REMOTE },
+ /* Sony Dualshock 4 controllers for PS4 */
+ { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
+ .driver_data = DUALSHOCK4_CONTROLLER },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
+ .driver_data = DUALSHOCK4_CONTROLLER },
{ }
};
MODULE_DEVICE_TABLE(hid, sony_devices);
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH 1/4] input: Add new sun4i-lradc-keys drivers
From: Dmitry Torokhov @ 2014-01-03 18:23 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans de Goede, Heiko Stübner,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
linux-input-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell
In-Reply-To: <20140103173605.GN3144@lukather>
On Fri, Jan 03, 2014 at 06:36:05PM +0100, Maxime Ripard wrote:
> On Thu, Jan 02, 2014 at 11:36:33PM +0100, Hans de Goede wrote:
> > Hi,
> >
> > On 01/02/2014 09:20 PM, Maxime Ripard wrote:
> > >On Thu, Jan 02, 2014 at 02:45:29PM +0100, Hans de Goede wrote:
> > >>>Also, instead of inventing yet another vendor-specific property, why not re-use
> > >>>a button binding similar to gpio-keys like:
> > >>>
> > >>> lradc: lradc@01c22800 {
> > >>> compatible = "allwinner,sun4i-lradc-keys";
> > >>> reg = <0x01c22800 0x100>;
> > >>> interrupts = <31>;
> > >>> allwinner,chan0-step = <200>;
> > >>>
> > >>> #address-cells = <1>;
> > >>> #size-cells = <0>;
> > >>>
> > >>> button@0 {
> > >>> reg = <0>; /* your channel index from above */
> > >>> linux,code = <115>; /* already used as dt-property */
> > >>> };
> > >>>
> > >>> button@1 {
> > >>> reg = <1>;
> > >>> linux,code = <114>;
> > >>> };
> > >>
> > >>Ugh no. Having a vendor specific property which is KISS certainly
> > >>beats this, both wrt ease of writing dts files as well as wrt the
> > >>dts parsing code in the driver.
> > >
> > >I'd agree with Heiko here. This is pretty much the same construct
> > >that's already in use in other input drivers, like gpio-keys.
> >
> > In the gpio case there is a 1 on 1 relation between a single hw
> > entity (the gpio-pin) and a single keycode. Here there is 1 hw entity
> > which maps to an array of key-codes, certainly using an array rather
> > then a much more complicated construct is the correct data-structure
> > to represent this.
>
> You can build an array in your driver out of this very easily, it's 10
> lines in your probe. And you gain from this something that is more
> generic, can be shared by other similar drivers and is consistent with
> what is already in use.
How will it be shared? Surely not code-wise, but basically in spirit
only. It seems to me that the originally proposed binding is simple and
concise and works well for the driver.
>
> > >This is also something that can really easily be made generic,
> > >since this is something that is rather common.
> > >
> > >Speaking of which. I believe this should actually come in two
> > >different drivers:
> > > - The ADC driver itself, using IIO
> > > - A generic button handler driver on top of IIO.
> > >
> > > The fact that on most board this adc is used for buttons doesn't make
> > > any difference, it's actually a hardware designer choice, we should
> > > support that choice, but we should also be able to use it just as an
> > > ADC.
> >
> > No, this is not a generic adc, as mentioned in the commit msg, this
> > adc is specifically designed to be used this way.
> >
> > The adc won't start sampling data, and won't generate any interrupts
> > until a button is pressed. That is until the input voltage drops below
> > 2/3 of Vref, this is checked through a built-in analog comparator, which
> > hooks into the control logic.
> >
> > It has button down and button up interrupts, and can detect long
> > presses (unused) and generate a second type of down interrupt for those.
> >
> > This really is an input device, which happens to use an adc.
>
> Hmm, yes, ok.
>
> > >Carlo Caione already started to work on an IIO driver for the LRADC:
> > >https://github.com/carlocaione/linux/tree/sunxi-lradc
> > >maybe you can take over his work.
> >
> > That won't work because the adc won't sample if the input gets above
> > 2/3 of Vref. There may be some other mode which does not do that, but
> > that is not clearly documented.
> >
> > Even if an IIO driver turns out to be doable, I strongly believe that
> > having a separate input driver for this is best, since this device
> > was designed to be used as such. Building input on top of IIO would
> > mean polling the adc, while with my driver it actually generates
> > button down / up interrupts without any polling being involved.
>
> Not really. iio_channel_read calls the read_raw function (in that
> case) of your driver. If the read_raw function in your driver wants to
> poll the device, fine, but most of the time, it will just block
> waiting for an interrupt to come and return the data to the caller,
> which is obviously the saner behaviour, and you don't actually end up
> polling the device. Which is pretty much the architecture you're using
> already, just with an intermediate layer in between.
What is the benefit of the IIO layer if device can't really be used as
IIO? I am all for moving as many generic devices as we can to IIO but we
should recognize that sometimes the device is not an IIO device.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/4] input: Add new sun4i-lradc-keys drivers
From: Maxime Ripard @ 2014-01-03 17:36 UTC (permalink / raw)
To: Hans de Goede
Cc: Heiko Stübner,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Dmitry Torokhov, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
linux-input-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell
In-Reply-To: <52C5E9F1.9010700-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4230 bytes --]
On Thu, Jan 02, 2014 at 11:36:33PM +0100, Hans de Goede wrote:
> Hi,
>
> On 01/02/2014 09:20 PM, Maxime Ripard wrote:
> >On Thu, Jan 02, 2014 at 02:45:29PM +0100, Hans de Goede wrote:
> >>>Also, instead of inventing yet another vendor-specific property, why not re-use
> >>>a button binding similar to gpio-keys like:
> >>>
> >>> lradc: lradc@01c22800 {
> >>> compatible = "allwinner,sun4i-lradc-keys";
> >>> reg = <0x01c22800 0x100>;
> >>> interrupts = <31>;
> >>> allwinner,chan0-step = <200>;
> >>>
> >>> #address-cells = <1>;
> >>> #size-cells = <0>;
> >>>
> >>> button@0 {
> >>> reg = <0>; /* your channel index from above */
> >>> linux,code = <115>; /* already used as dt-property */
> >>> };
> >>>
> >>> button@1 {
> >>> reg = <1>;
> >>> linux,code = <114>;
> >>> };
> >>
> >>Ugh no. Having a vendor specific property which is KISS certainly
> >>beats this, both wrt ease of writing dts files as well as wrt the
> >>dts parsing code in the driver.
> >
> >I'd agree with Heiko here. This is pretty much the same construct
> >that's already in use in other input drivers, like gpio-keys.
>
> In the gpio case there is a 1 on 1 relation between a single hw
> entity (the gpio-pin) and a single keycode. Here there is 1 hw entity
> which maps to an array of key-codes, certainly using an array rather
> then a much more complicated construct is the correct data-structure
> to represent this.
You can build an array in your driver out of this very easily, it's 10
lines in your probe. And you gain from this something that is more
generic, can be shared by other similar drivers and is consistent with
what is already in use.
> >This is also something that can really easily be made generic,
> >since this is something that is rather common.
> >
> >Speaking of which. I believe this should actually come in two
> >different drivers:
> > - The ADC driver itself, using IIO
> > - A generic button handler driver on top of IIO.
> >
> > The fact that on most board this adc is used for buttons doesn't make
> > any difference, it's actually a hardware designer choice, we should
> > support that choice, but we should also be able to use it just as an
> > ADC.
>
> No, this is not a generic adc, as mentioned in the commit msg, this
> adc is specifically designed to be used this way.
>
> The adc won't start sampling data, and won't generate any interrupts
> until a button is pressed. That is until the input voltage drops below
> 2/3 of Vref, this is checked through a built-in analog comparator, which
> hooks into the control logic.
>
> It has button down and button up interrupts, and can detect long
> presses (unused) and generate a second type of down interrupt for those.
>
> This really is an input device, which happens to use an adc.
Hmm, yes, ok.
> >Carlo Caione already started to work on an IIO driver for the LRADC:
> >https://github.com/carlocaione/linux/tree/sunxi-lradc
> >maybe you can take over his work.
>
> That won't work because the adc won't sample if the input gets above
> 2/3 of Vref. There may be some other mode which does not do that, but
> that is not clearly documented.
>
> Even if an IIO driver turns out to be doable, I strongly believe that
> having a separate input driver for this is best, since this device
> was designed to be used as such. Building input on top of IIO would
> mean polling the adc, while with my driver it actually generates
> button down / up interrupts without any polling being involved.
Not really. iio_channel_read calls the read_raw function (in that
case) of your driver. If the read_raw function in your driver wants to
poll the device, fine, but most of the time, it will just block
waiting for an interrupt to come and return the data to the caller,
which is obviously the saner behaviour, and you don't actually end up
polling the device. Which is pretty much the architecture you're using
already, just with an intermediate layer in between.
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 1/4] input: Add new sun4i-lradc-keys drivers
From: Maxime Ripard @ 2014-01-03 17:15 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Hans de Goede, Heiko Stübner,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
linux-input-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell
In-Reply-To: <20140102203831.GA3239-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1897 bytes --]
On Thu, Jan 02, 2014 at 12:38:31PM -0800, Dmitry Torokhov wrote:
> On Thu, Jan 02, 2014 at 09:20:22PM +0100, Maxime Ripard wrote:
> > On Thu, Jan 02, 2014 at 02:45:29PM +0100, Hans de Goede wrote:
> > > >Also, instead of inventing yet another vendor-specific property, why not re-use
> > > >a button binding similar to gpio-keys like:
> > > >
> > > > lradc: lradc@01c22800 {
> > > > compatible = "allwinner,sun4i-lradc-keys";
> > > > reg = <0x01c22800 0x100>;
> > > > interrupts = <31>;
> > > > allwinner,chan0-step = <200>;
> > > >
> > > > #address-cells = <1>;
> > > > #size-cells = <0>;
> > > >
> > > > button@0 {
> > > > reg = <0>; /* your channel index from above */
> > > > linux,code = <115>; /* already used as dt-property */
> > > > };
> > > >
> > > > button@1 {
> > > > reg = <1>;
> > > > linux,code = <114>;
> > > > };
> > >
> > > Ugh no. Having a vendor specific property which is KISS certainly
> > > beats this, both wrt ease of writing dts files as well as wrt the
> > > dts parsing code in the driver.
> >
> > I'd agree with Heiko here. This is pretty much the same construct
> > that's already in use in other input drivers, like gpio-keys.
> >
> > This is also something that can really easily be made generic, since
> > this is something that is rather common.
>
> Except that button definition from gpio-keys does not use 'reg' property
> but rather gpio. I'd rather we did not cram non-applicable attributes
> into that definition just to make it "reusable" like that.
>
> I'd be OK with having similar (but not claiming to be the same) mappings
> though.
Yes, this is what I was meaning. Sorry if it was not clear enough.
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 3/3] HID: multitouch: add support of other generic collections in hid-mt
From: Jiri Kosina @ 2014-01-03 9:56 UTC (permalink / raw)
To: Henrik Rydberg
Cc: Benjamin Tissoires, Benjamin Tissoires, Edel Maks, linux-input,
linux-kernel
In-Reply-To: <52B5F96D.9050400@euromail.se>
On Sat, 21 Dec 2013, Henrik Rydberg wrote:
> > The ANTEC Touch Pad is a device which can switch from a multitouch
> > touchpad to a mouse. It thus presents several generic collections which
> > are currently ignored by hid-multitouch. Enable them by using the generic
> > protocol. Adding also a suffix for them depending on their application.
>
> In what way does this and the preceeding patches differ from "else if (is_pen ||
> export_all_inputs)"? Adding a functional pattern which then is converted to a
> generic case, such that the usual branching is bound to occur anyways, seems
> unnecessary.
Benjamin,
did you have chance to do any followup work on this?
If this is to go into 3.14, it's the time to have the thing finalized.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Another ALPS touchpad
From: Dylan Paul Thurston @ 2014-01-03 4:59 UTC (permalink / raw)
To: linux-input
I just bought a HP Revolve 810 G1 laptop. (This is a convertible
ultrabook, normally sold with Windows 8.) The touchpad seems to be
another ALPS touchpad, not yet recognized by the kernel; I tried the
Debian package of 3.12 and linux-next-20131224 compiled from
source. Without the proper driver, it is unusuably sensisitive,
registering clicks when my hands are well away from the touchpad.
The relevant line from dmesg is
[ 2.776630] psmouse serio4: alps: Unknown ALPS touchpad: E7=73 03 0a, EC=88 b3 18
Should I try hacking drivers/input/mouse/alps.c to see if one of the
existing protocols works?
Thanks,
Dylan Thurston
dpt@bostoncoop.net
^ permalink raw reply
* Re: [PATCH v2 06/10] Input: pm8xxx-vibrator - Add DT match table
From: Stephen Boyd @ 2014-01-03 1:27 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-kernel, linux-arm-msm, linux-arm-kernel, linux-input
In-Reply-To: <20140103011722.GA9300@core.coreip.homeip.net>
On 01/02/14 17:17, Dmitry Torokhov wrote:
> Hi Stephen,
>
> On Thu, Jan 02, 2014 at 04:37:36PM -0800, Stephen Boyd wrote:
>> The driver is only supported on DT enabled platforms. Convert the
>> driver to DT so that it can probe properly.
> I do not see MFD_PM8XXX depending on OF, should it be added if it only
> supported on DT?
No that would unnecessarily limit the compile coverage of this driver.
This one is so simple that it doesn't even use any OF APIs because it is
all hidden behind the platform bus.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH v2 06/10] Input: pm8xxx-vibrator - Add DT match table
From: Dmitry Torokhov @ 2014-01-03 1:17 UTC (permalink / raw)
To: Stephen Boyd; +Cc: linux-arm-msm, linux-kernel, linux-arm-kernel, linux-input
In-Reply-To: <1388709460-19222-7-git-send-email-sboyd@codeaurora.org>
Hi Stephen,
On Thu, Jan 02, 2014 at 04:37:36PM -0800, Stephen Boyd wrote:
> The driver is only supported on DT enabled platforms. Convert the
> driver to DT so that it can probe properly.
I do not see MFD_PM8XXX depending on OF, should it be added if it only
supported on DT?
Thanks.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
> drivers/input/misc/pm8xxx-vibrator.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index 28251560249d..458d51b88be5 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -142,6 +142,13 @@ static int pm8xxx_vib_play_effect(struct input_dev *dev, void *data,
> return 0;
> }
>
> +static const struct of_device_id pm8xxx_vib_id_table[] = {
> + { .compatible = "qcom,pm8058-vib" },
> + { .compatible = "qcom,pm8921-vib" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
> +
> static int pm8xxx_vib_probe(struct platform_device *pdev)
>
> {
> @@ -221,6 +228,7 @@ static struct platform_driver pm8xxx_vib_driver = {
> .name = "pm8xxx-vib",
> .owner = THIS_MODULE,
> .pm = &pm8xxx_vib_pm_ops,
> + .of_match_table = pm8xxx_vib_id_table,
> },
> };
> module_platform_driver(pm8xxx_vib_driver);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/7] Input: pmic8xxx-pwrkey - Migrate to regmap APIs
From: Mark Brown @ 2014-01-03 0:50 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Stephen Boyd, linux-arm-msm, linux-kernel, linux-arm-kernel,
linux-input
In-Reply-To: <20140102191757.GA2025@core.coreip.homeip.net>
[-- Attachment #1: Type: text/plain, Size: 772 bytes --]
On Thu, Jan 02, 2014 at 11:17:57AM -0800, Dmitry Torokhov wrote:
> On Thu, Jan 02, 2014 at 06:49:59PM +0000, Mark Brown wrote:
> > On Sun, Dec 15, 2013 at 03:33:37AM -0800, Dmitry Torokhov wrote:
> > > > + regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > > + if (!regmap)
> > > > + return -ENODEV;
> > > And you are leaking memory here...
> > He's not, dev_get_regmap() just gets a pointer to an existing regmap -
> > no reference is taken and nothing is allocated. It's a helper that's
> I was not talking about data returned by dev_get_regmap() but all other
> memory that was allocated before as this was pre devm conversion of the
> driver.
Ah, OK - I thought you meant that as that was the code immediately prior
to your reply. Sorry for the confusion.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH v2 10/10] devicetree: bindings: Document PM8921/8058 vibrators
From: Stephen Boyd @ 2014-01-03 0:37 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-kernel, linux-arm-msm, linux-arm-kernel, linux-input,
devicetree
In-Reply-To: <1388709460-19222-1-git-send-email-sboyd@codeaurora.org>
Document the vibration device found on PM8921 and PM8058 PMICs.
Cc: <devicetree@vger.kernel.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
.../devicetree/bindings/input/qcom,pm8xxx-vib.txt | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
new file mode 100644
index 000000000000..dca1b8872cf1
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
@@ -0,0 +1,16 @@
+Qualcomm PM8xxx PMIC Vibrator
+
+PROPERTIES
+
+- compatible:
+ Usage: required
+ Value type: <string>
+ Definition: must be one of:
+ "qcom,pm8058-vib"
+ "qcom,pm8921-vib"
+
+EXAMPLE
+
+ vibrator {
+ compatible = "qcom,pm8058-vib";
+ };
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH v2 09/10] devicetree: bindings: Document PM8921/8058 power keys
From: Stephen Boyd @ 2014-01-03 0:37 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-kernel, linux-arm-msm, linux-arm-kernel, linux-input,
devicetree
In-Reply-To: <1388709460-19222-1-git-send-email-sboyd@codeaurora.org>
Document the power key found on PM8921 and PM8058 PMICs.
Cc: <devicetree@vger.kernel.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
.../bindings/input/qcom,pm8xxx-pwrkey.txt | 39 ++++++++++++++++++++++
1 file changed, 39 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
new file mode 100644
index 000000000000..e124d9f33632
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-pwrkey.txt
@@ -0,0 +1,39 @@
+Qualcomm PM8xxx PMIC Power Key
+
+PROPERTIES
+
+- compatible:
+ Usage: required
+ Value type: <string>
+ Definition: must be one of:
+ "qcom,pm8058-pwrkey"
+ "qcom,pm8921-pwrkey"
+- interrupts:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: the first interrupt specifies the key release interrupt
+ and the second interrupt specifies the key press interrupt.
+ The format of the specifier is defined by the binding
+ document describing the node's interrupt parent.
+
+- debounce:
+ Usage: optional
+ Value type: <u32>
+ Definition: time in microseconds that key must be pressed or release
+ for state change interrupt to trigger.
+
+- pull-up:
+ Usage: optional
+ Value type: <empty>
+ Definition: presence of this property indicates that the KPDPWR_N pin
+ should be configured for pull up.
+
+EXAMPLE
+
+ pwrkey {
+ compatible = "qcom,pm8921-pwrkey";
+ interrupt-parent = <&pmicintc>;
+ interrupts = <50 1>, <51 1>;
+ debounce = <15625>;
+ pull-up;
+ };
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH v2 08/10] devicetree: bindings: Document PM8921/8058 keypads
From: Stephen Boyd @ 2014-01-03 0:37 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-kernel, linux-arm-msm, linux-arm-kernel, linux-input,
devicetree
In-Reply-To: <1388709460-19222-1-git-send-email-sboyd@codeaurora.org>
Document the keypad device found on PM8921 and PM8058 PMICs.
Cc: <devicetree@vger.kernel.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
.../bindings/input/qcom,pm8xxx-keypad.txt | 72 ++++++++++++++++++++++
1 file changed, 72 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
new file mode 100644
index 000000000000..aa5a9c6cf512
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
@@ -0,0 +1,72 @@
+Qualcomm PM8xxx PMIC Keypad
+
+PROPERTIES
+
+- compatible:
+ Usage: required
+ Value type: <string>
+ Definition: must be one of:
+ "qcom,pm8058-keypad"
+ "qcom,pm8921-keypad"
+- interrupts:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: the first interrupt specifies the key sense interrupt
+ and the second interrupt specifies the key stuck interrupt.
+ The format of the specifier is defined by the binding
+ document describing the node's interrupt parent.
+
+- linux,keymap:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: the linux keymap. More information can be found in
+ input/matrix-keymap.txt.
+
+- keypad,num-rows:
+ Usage: required
+ Value type: <u32>
+ Definition: number of rows in the keymap. More information can be found
+ in input/matrix-keymap.txt.
+
+- keypad,num-columns:
+ Usage: required
+ Value type: <u32>
+ Definition: number of columns in the keymap. More information can be
+ found in input/matrix-keymap.txt.
+
+- debounce:
+ Usage: optional
+ Value type: <u32>
+ Definition: time in microseconds that key must be pressed or release
+ for key sense interrupt to trigger.
+
+- scan-delay:
+ Usage: optional
+ Value type: <u32>
+ Definition: time in microseconds to pause between successive scans
+ of the matrix array.
+
+- row-hold:
+ Usage: optional
+ Value type: <u32>
+ Definition: time in nanoseconds to pause between scans of each row in
+ the matrix array.
+
+EXAMPLE
+
+ keypad {
+ compatible = "qcom,pm8921-keypad";
+ interrupt-parent = <&pmicintc>;
+ interrupts = <74 1>, <75 1>;
+ linux,keymap = <
+ MATRIX_KEY(0, 0, KEY_VOLUMEUP)
+ MATRIX_KEY(0, 1, KEY_VOLUMEDOWN)
+ MATRIX_KEY(0, 2, KEY_CAMERA_FOCUS)
+ MATRIX_KEY(0, 3, KEY_CAMERA)
+ >;
+ keypad,num-rows = <1>;
+ keypad,num-columns = <5>;
+ debounce = <15>;
+ scan-delay = <32>;
+ row-hold = <91500>;
+ };
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH v2 07/10] Input: pmic8xxx-keypad - Migrate to DT
From: Stephen Boyd @ 2014-01-03 0:37 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-kernel, linux-arm-msm, linux-arm-kernel, linux-input
In-Reply-To: <1388709460-19222-1-git-send-email-sboyd@codeaurora.org>
The driver is only supported on DT enabled platforms. Convert the
driver to DT so that it can probe properly.
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
drivers/input/keyboard/pmic8xxx-keypad.c | 148 ++++++++++++++++++-------------
include/linux/input/pmic8xxx-keypad.h | 52 -----------
2 files changed, 85 insertions(+), 115 deletions(-)
delete mode 100644 include/linux/input/pmic8xxx-keypad.h
diff --git a/drivers/input/keyboard/pmic8xxx-keypad.c b/drivers/input/keyboard/pmic8xxx-keypad.c
index c6d3d216ffa7..d2e43073d76e 100644
--- a/drivers/input/keyboard/pmic8xxx-keypad.c
+++ b/drivers/input/keyboard/pmic8xxx-keypad.c
@@ -20,9 +20,10 @@
#include <linux/delay.h>
#include <linux/mutex.h>
#include <linux/regmap.h>
+#include <linux/of.h>
+#include <linux/input/matrix_keypad.h>
#include <linux/mfd/pm8xxx/gpio.h>
-#include <linux/input/pmic8xxx-keypad.h>
#define PM8XXX_MAX_ROWS 18
#define PM8XXX_MAX_COLS 8
@@ -85,7 +86,8 @@
/**
* struct pmic8xxx_kp - internal keypad data structure
- * @pdata - keypad platform data pointer
+ * @num_cols - number of columns of keypad
+ * @num_rows - number of row of keypad
* @input - input device pointer for keypad
* @regmap - regmap handle
* @key_sense_irq - key press/release irq number
@@ -97,7 +99,8 @@
* @ctrl_reg - control register value
*/
struct pmic8xxx_kp {
- const struct pm8xxx_keypad_platform_data *pdata;
+ unsigned int num_rows;
+ unsigned int num_cols;
struct input_dev *input;
struct regmap *regmap;
int key_sense_irq;
@@ -116,9 +119,9 @@ static u8 pmic8xxx_col_state(struct pmic8xxx_kp *kp, u8 col)
{
/* all keys pressed on that particular row? */
if (col == 0x00)
- return 1 << kp->pdata->num_cols;
+ return 1 << kp->num_cols;
else
- return col & ((1 << kp->pdata->num_cols) - 1);
+ return col & ((1 << kp->num_cols) - 1);
}
/*
@@ -181,10 +184,10 @@ static int pmic8xxx_kp_read_matrix(struct pmic8xxx_kp *kp, u16 *new_state,
int rc, read_rows;
unsigned int scan_val;
- if (kp->pdata->num_rows < PM8XXX_MIN_ROWS)
+ if (kp->num_rows < PM8XXX_MIN_ROWS)
read_rows = PM8XXX_MIN_ROWS;
else
- read_rows = kp->pdata->num_rows;
+ read_rows = kp->num_rows;
pmic8xxx_chk_sync_read(kp);
@@ -228,13 +231,13 @@ static void __pmic8xxx_kp_scan_matrix(struct pmic8xxx_kp *kp, u16 *new_state,
{
int row, col, code;
- for (row = 0; row < kp->pdata->num_rows; row++) {
+ for (row = 0; row < kp->num_rows; row++) {
int bits_changed = new_state[row] ^ old_state[row];
if (!bits_changed)
continue;
- for (col = 0; col < kp->pdata->num_cols; col++) {
+ for (col = 0; col < kp->num_cols; col++) {
if (!(bits_changed & (1 << col)))
continue;
@@ -260,9 +263,9 @@ static bool pmic8xxx_detect_ghost_keys(struct pmic8xxx_kp *kp, u16 *new_state)
u16 check, row_state;
check = 0;
- for (row = 0; row < kp->pdata->num_rows; row++) {
+ for (row = 0; row < kp->num_rows; row++) {
row_state = (~new_state[row]) &
- ((1 << kp->pdata->num_cols) - 1);
+ ((1 << kp->num_cols) - 1);
if (hweight16(row_state) > 1) {
if (found_first == -1)
@@ -370,8 +373,13 @@ static irqreturn_t pmic8xxx_kp_irq(int irq, void *data)
return IRQ_HANDLED;
}
-static int pmic8xxx_kpd_init(struct pmic8xxx_kp *kp)
+static int pmic8xxx_kpd_init(struct pmic8xxx_kp *kp,
+ struct platform_device *pdev)
{
+ const struct device_node *of_node = pdev->dev.of_node;
+ unsigned int scan_delay_ms;
+ unsigned int row_hold_ns;
+ unsigned int debounce_ms;
int bits, rc, cycles;
u8 scan_val = 0, ctrl_val = 0;
static const u8 row_bits[] = {
@@ -379,18 +387,18 @@ static int pmic8xxx_kpd_init(struct pmic8xxx_kp *kp)
};
/* Find column bits */
- if (kp->pdata->num_cols < KEYP_CTRL_SCAN_COLS_MIN)
+ if (kp->num_cols < KEYP_CTRL_SCAN_COLS_MIN)
bits = 0;
else
- bits = kp->pdata->num_cols - KEYP_CTRL_SCAN_COLS_MIN;
+ bits = kp->num_cols - KEYP_CTRL_SCAN_COLS_MIN;
ctrl_val = (bits & KEYP_CTRL_SCAN_COLS_BITS) <<
KEYP_CTRL_SCAN_COLS_SHIFT;
/* Find row bits */
- if (kp->pdata->num_rows < KEYP_CTRL_SCAN_ROWS_MIN)
+ if (kp->num_rows < KEYP_CTRL_SCAN_ROWS_MIN)
bits = 0;
else
- bits = row_bits[kp->pdata->num_rows - KEYP_CTRL_SCAN_ROWS_MIN];
+ bits = row_bits[kp->num_rows - KEYP_CTRL_SCAN_ROWS_MIN];
ctrl_val |= (bits << KEYP_CTRL_SCAN_ROWS_SHIFT);
@@ -400,15 +408,44 @@ static int pmic8xxx_kpd_init(struct pmic8xxx_kp *kp)
return rc;
}
- bits = (kp->pdata->debounce_ms / 5) - 1;
+ if (of_property_read_u32(of_node, "scan-delay", &scan_delay_ms))
+ scan_delay_ms = MIN_SCAN_DELAY;
+
+ if (scan_delay_ms > MAX_SCAN_DELAY || scan_delay_ms < MIN_SCAN_DELAY ||
+ !is_power_of_2(scan_delay_ms)) {
+ dev_err(&pdev->dev, "invalid keypad scan time supplied\n");
+ return -EINVAL;
+ }
+
+ if (of_property_read_u32(of_node, "row-hold", &row_hold_ns))
+ row_hold_ns = MIN_ROW_HOLD_DELAY;
+
+ if (row_hold_ns > MAX_ROW_HOLD_DELAY ||
+ row_hold_ns < MIN_ROW_HOLD_DELAY ||
+ ((row_hold_ns % MIN_ROW_HOLD_DELAY) != 0)) {
+ dev_err(&pdev->dev, "invalid keypad row hold time supplied\n");
+ return -EINVAL;
+ }
+
+ if (of_property_read_u32(of_node, "debounce", &debounce_ms))
+ debounce_ms = MIN_DEBOUNCE_TIME;
+
+ if (((debounce_ms % 5) != 0) ||
+ debounce_ms > MAX_DEBOUNCE_TIME ||
+ debounce_ms < MIN_DEBOUNCE_TIME) {
+ dev_err(&pdev->dev, "invalid debounce time supplied\n");
+ return -EINVAL;
+ }
+
+ bits = (debounce_ms / 5) - 1;
scan_val |= (bits << KEYP_SCAN_DBOUNCE_SHIFT);
- bits = fls(kp->pdata->scan_delay_ms) - 1;
+ bits = fls(scan_delay_ms) - 1;
scan_val |= (bits << KEYP_SCAN_PAUSE_SHIFT);
/* Row hold time is a multiple of 32KHz cycles. */
- cycles = (kp->pdata->row_hold_ns * KEYP_CLOCK_FREQ) / NSEC_PER_SEC;
+ cycles = (row_hold_ns * KEYP_CLOCK_FREQ) / NSEC_PER_SEC;
scan_val |= (cycles << KEYP_SCAN_ROW_HOLD_SHIFT);
@@ -481,6 +518,13 @@ static void pmic8xxx_kp_close(struct input_dev *dev)
pmic8xxx_kp_disable(kp);
}
+static const struct of_device_id pm8xxx_match_table[] = {
+ { .compatible = "qcom,pm8058-keypad" },
+ { .compatible = "qcom,pm8921-keypad" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, pm8xxx_match_table);
+
/*
* keypad controller should be initialized in the following sequence
* only, otherwise it might get into FSM stuck state.
@@ -493,9 +537,9 @@ static void pmic8xxx_kp_close(struct input_dev *dev)
*/
static int pmic8xxx_kp_probe(struct platform_device *pdev)
{
- const struct pm8xxx_keypad_platform_data *pdata =
- dev_get_platdata(&pdev->dev);
- const struct matrix_keymap_data *keymap_data;
+ unsigned int rows, cols;
+ bool repeat;
+ bool wakeup;
struct pmic8xxx_kp *kp;
int rc;
unsigned int ctrl_val;
@@ -520,44 +564,20 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
.inv_int_pol = 1,
};
+ rc = matrix_keypad_parse_of_params(&pdev->dev, &rows, &cols);
+ if (rc)
+ return rc;
- if (!pdata || !pdata->num_cols || !pdata->num_rows ||
- pdata->num_cols > PM8XXX_MAX_COLS ||
- pdata->num_rows > PM8XXX_MAX_ROWS ||
- pdata->num_cols < PM8XXX_MIN_COLS) {
+ if (cols > PM8XXX_MAX_COLS || rows > PM8XXX_MAX_ROWS ||
+ cols < PM8XXX_MIN_COLS) {
dev_err(&pdev->dev, "invalid platform data\n");
return -EINVAL;
}
- if (!pdata->scan_delay_ms ||
- pdata->scan_delay_ms > MAX_SCAN_DELAY ||
- pdata->scan_delay_ms < MIN_SCAN_DELAY ||
- !is_power_of_2(pdata->scan_delay_ms)) {
- dev_err(&pdev->dev, "invalid keypad scan time supplied\n");
- return -EINVAL;
- }
-
- if (!pdata->row_hold_ns ||
- pdata->row_hold_ns > MAX_ROW_HOLD_DELAY ||
- pdata->row_hold_ns < MIN_ROW_HOLD_DELAY ||
- ((pdata->row_hold_ns % MIN_ROW_HOLD_DELAY) != 0)) {
- dev_err(&pdev->dev, "invalid keypad row hold time supplied\n");
- return -EINVAL;
- }
-
- if (!pdata->debounce_ms ||
- ((pdata->debounce_ms % 5) != 0) ||
- pdata->debounce_ms > MAX_DEBOUNCE_TIME ||
- pdata->debounce_ms < MIN_DEBOUNCE_TIME) {
- dev_err(&pdev->dev, "invalid debounce time supplied\n");
- return -EINVAL;
- }
-
- keymap_data = pdata->keymap_data;
- if (!keymap_data) {
- dev_err(&pdev->dev, "no keymap data supplied\n");
- return -EINVAL;
- }
+ repeat = !of_property_read_bool(pdev->dev.of_node,
+ "linux,input-no-autorepeat");
+ wakeup = !of_property_read_bool(pdev->dev.of_node,
+ "linux,keypad-wakeup");
kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL);
if (!kp)
@@ -569,7 +589,8 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, kp);
- kp->pdata = pdata;
+ kp->num_rows = rows;
+ kp->num_cols = cols;
kp->dev = &pdev->dev;
kp->input = devm_input_allocate_device(&pdev->dev);
@@ -590,8 +611,8 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
return kp->key_stuck_irq;
}
- kp->input->name = pdata->input_name ? : "PMIC8XXX keypad";
- kp->input->phys = pdata->input_phys_device ? : "pmic8xxx_keypad/input0";
+ kp->input->name = "PMIC8XXX keypad";
+ kp->input->phys = "pmic8xxx_keypad/input0";
kp->input->id.bustype = BUS_I2C;
kp->input->id.version = 0x0001;
@@ -601,7 +622,7 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
kp->input->open = pmic8xxx_kp_open;
kp->input->close = pmic8xxx_kp_close;
- rc = matrix_keypad_build_keymap(keymap_data, NULL,
+ rc = matrix_keypad_build_keymap(NULL, NULL,
PM8XXX_MAX_ROWS, PM8XXX_MAX_COLS,
kp->keycodes, kp->input);
if (rc) {
@@ -609,7 +630,7 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
return rc;
}
- if (pdata->rep)
+ if (repeat)
__set_bit(EV_REP, kp->input->evbit);
input_set_capability(kp->input, EV_MSC, MSC_SCAN);
@@ -619,7 +640,7 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
memset(kp->keystate, 0xff, sizeof(kp->keystate));
memset(kp->stuckstate, 0xff, sizeof(kp->stuckstate));
- rc = pmic8xxx_kpd_init(kp);
+ rc = pmic8xxx_kpd_init(kp, pdev);
if (rc < 0) {
dev_err(&pdev->dev, "unable to initialize keypad controller\n");
return rc;
@@ -669,7 +690,7 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
return rc;
}
- device_init_wakeup(&pdev->dev, pdata->wakeup);
+ device_init_wakeup(&pdev->dev, wakeup);
return 0;
}
@@ -722,9 +743,10 @@ static SIMPLE_DEV_PM_OPS(pm8xxx_kp_pm_ops,
static struct platform_driver pmic8xxx_kp_driver = {
.probe = pmic8xxx_kp_probe,
.driver = {
- .name = PM8XXX_KEYPAD_DEV_NAME,
+ .name = "pm8xxx-keypad",
.owner = THIS_MODULE,
.pm = &pm8xxx_kp_pm_ops,
+ .of_match_table = pm8xxx_match_table,
},
};
module_platform_driver(pmic8xxx_kp_driver);
diff --git a/include/linux/input/pmic8xxx-keypad.h b/include/linux/input/pmic8xxx-keypad.h
deleted file mode 100644
index 5f1e2f9ad959..000000000000
--- a/include/linux/input/pmic8xxx-keypad.h
+++ /dev/null
@@ -1,52 +0,0 @@
-/* Copyright (c) 2011, Code Aurora Forum. All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 and
- * only version 2 as published by the Free Software Foundation.
- *
- * 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.
- */
-
-#ifndef __PMIC8XXX_KEYPAD_H__
-#define __PMIC8XXX_KEYPAD_H__
-
-#include <linux/input/matrix_keypad.h>
-
-#define PM8XXX_KEYPAD_DEV_NAME "pm8xxx-keypad"
-
-/**
- * struct pm8xxx_keypad_platform_data - platform data for keypad
- * @keymap_data - matrix keymap data
- * @input_name - input device name
- * @input_phys_device - input device name
- * @num_cols - number of columns of keypad
- * @num_rows - number of row of keypad
- * @debounce_ms - debounce period in milliseconds
- * @scan_delay_ms - scan delay in milliseconds
- * @row_hold_ns - row hold period in nanoseconds
- * @wakeup - configure keypad as wakeup
- * @rep - enable or disable key repeat bit
- */
-struct pm8xxx_keypad_platform_data {
- const struct matrix_keymap_data *keymap_data;
-
- const char *input_name;
- const char *input_phys_device;
-
- unsigned int num_cols;
- unsigned int num_rows;
- unsigned int rows_gpio_start;
- unsigned int cols_gpio_start;
-
- unsigned int debounce_ms;
- unsigned int scan_delay_ms;
- unsigned int row_hold_ns;
-
- bool wakeup;
- bool rep;
-};
-
-#endif /*__PMIC8XXX_KEYPAD_H__ */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH v2 06/10] Input: pm8xxx-vibrator - Add DT match table
From: Stephen Boyd @ 2014-01-03 0:37 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-kernel, linux-arm-msm, linux-arm-kernel, linux-input
In-Reply-To: <1388709460-19222-1-git-send-email-sboyd@codeaurora.org>
The driver is only supported on DT enabled platforms. Convert the
driver to DT so that it can probe properly.
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
drivers/input/misc/pm8xxx-vibrator.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 28251560249d..458d51b88be5 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -142,6 +142,13 @@ static int pm8xxx_vib_play_effect(struct input_dev *dev, void *data,
return 0;
}
+static const struct of_device_id pm8xxx_vib_id_table[] = {
+ { .compatible = "qcom,pm8058-vib" },
+ { .compatible = "qcom,pm8921-vib" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
+
static int pm8xxx_vib_probe(struct platform_device *pdev)
{
@@ -221,6 +228,7 @@ static struct platform_driver pm8xxx_vib_driver = {
.name = "pm8xxx-vib",
.owner = THIS_MODULE,
.pm = &pm8xxx_vib_pm_ops,
+ .of_match_table = pm8xxx_vib_id_table,
},
};
module_platform_driver(pm8xxx_vib_driver);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox