* Re: [PATCH V1] input: use device managed memory in da9052 touchscreen driver
From: Dmitry Torokhov @ 2014-01-10 16:36 UTC (permalink / raw)
To: Opensource [Anthony Olech]
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
Huqiu Liu, David Dajun Chen
In-Reply-To: <24DF37198A1E704D9811D8F72B87EB51AC050B52@NB-EX-MBX02.diasemi.com>
On Fri, Jan 10, 2014 at 09:02:10AM +0000, Opensource [Anthony Olech] wrote:
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> >
> > I am uncomfortable with the touchscreen portion of this driver ignoring the
> > framework of it's MFD core and mixing native IRQ management with the
> > ones done through the core.
> >
> > What would happen if somebody changes da9052_request_irq() to do some
> > thing more than it is doing now so that your open-coded duplicate of the
> > same in da9052_ts_probe() is no longer equivalent? Or
> > da9052_disable_irq() no longer works correctly with IRQs allocated by this
> > sub-module?
> > Thanks.
> > --
> > Dmitry
> Hi Dmitry,
>
> unfortunately the PMIC is a multifunction device and the component
> drivers come under different subsystem maintainers. Thus it is not
> possible to do one patch in one go to change them all.
This could be arranged if you really want to do that. You could post a
patch series and have various maintainers ack it and then Samuel could
take it all through MFD tree.
This is up to you however. The change does not fix any bugs and error
unwinding paths in the input portion of the driver are simple enough, so
I do not see a very strong reason for moving to managed devices. I said
that I would not be opposed to this (given that conversion is solid),
but that was never a request form me.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: Re: [PATCH] Introduce Naming Convention in Input Subsystem
From: Aniroop Mathur @ 2014-01-10 16:49 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org, Aniroop Mathur, cpgs .,
Anurag Aggarwal, Naveen Kumar, aniroop.mathur@gmail.com,
VIKAS KALA, Poorva Srivastava
Hello Mr. Torokhov,
Greetings!
On Thu, Jan 09, 2014 at 10:27:56AM +0530, Aniroop Mathur wrote:
> This patch allows user(driver) to set sysfs node name of input
> devices. To set sysfs node name, user(driver) just needs to set
> node_name_unique variable. If node_name_unique is not set, default
> name is given(as before). So, this patch is completely
> backward-compatible.
>
> Sysfs Input node name format is: input_
> Sysfs Event node name format is: event_
>
> This "name" is given by user and automatically, prefix(input and
> event) is added by input core.
>
> This name must be unique among all input devices and driver(user) has
> the responsibility to ensure it. If same name is used again for other
> input device, registration of that input device will fail because two
> input devices cannot have same name.
>
> Advantages of this patch are:
>
> 1. Reduces Booting Time of HAL/Upper-Layer because now HAL or
> Upper-Layer do not need to search input/event number corresponding to
> each input device in /dev/input/... This searching in /dev/input/ was
> taking too much time. (Especially in mobile devices, where there are
> many input devices (many sensors, touchscreen, etc), it reduces a lot
> of booting time)
I am sorry, how much time does it take to scan a directory of what, 20
devices? If it such a factor have udev create nodes that are easier for
you to locate, similarly how we already create nodes by-id and by-path.
For example you can encode major:minor in device name.
Re: (Aniroop Mathur)
Its correct that we can set name of a device node using udev.
Yes, this will change the name of device node(/dev/...) but not sysfs node.(/sys/class/input/...)
So now, the problem area will shift from dev path to sysfs path,
because now we dont know which sysfs node to refer for a particular input device
and hence HAL/Upper-Layer will need to search in /sys/class/input/... instead of /dev/... directory.
Moreover, as i know, udev is mainly for hot-pluggable devices, but my problem is for platform devices,
which are already present on the board during boot up. (Like in Embedded devices)
To avoid confusion and make the problem more clear,
I would like to explain the problem and my suggestion by taking an example:
Suppose in a mobile device, there are 10 embedded input devices as below:
Proximity --- /dev/input0 --- /sys/class/input/input0 --- /sys/class/input/event0
Magnetometer --- /dev/input1 --- /sys/class/input/input1 --- /sys/class/input/event1
Accelerometer --- /dev/input2 --- /sys/class/input/input2 --- /sys/class/input/event2
Touchscreen --- /dev/input3 --- /sys/class/input/input3 --- /sys/class/input/event3
... 6 more like this
(All these are created during boot up time)
Kernel has created all these nodes, so that HAL/UpperLayer can read or write values from it.
HAL/Upper-Layer needs to do main tasks like:
1. Read raw data - does through /dev/input<num>
2. Enable device - does through sys/class/input<num>/enable
3. Set delay - does through sys/class/input<num>/delay
and many more...
Now, Lets suppose we need to do these tasks for Accelerometer.
If dev node name is set, HAL can directly read value from it (no search required)
But for enabling the accelerometer device or set the delay of a hardware chip,
there is no direct way, HAL can know which input node to refer for accelerometer
because the input number is created dynamically as per device probe order,
so this input number can be anything (0,1,2,3...)
So HAL will need to search every input node and read its name attribute
and keep on searching until a match is found between the "attribute name" and "name passed as parameter".
Like for accelerometer, this searching needs to be done for all other input devices.
All of this part is done during booting and this takes a lot for time from booting perspective.
As I measured, if there are ten devices, it is taking 1 second to do all this searching. (for all devices)
So for 20 devices, i guess, it could take upto 2 seconds.
With naming convention, there is no need of search neither for dev path nor for sysfs path
because HAL directly know which node to refer for which input device
and hence this 1 second is reduced to 10ms or even less, therefore saving 990ms.
I believe, this is a very good time saving. (from device booting perspective)
(Is there any direct way, without scanning all nodes for every input device ?)
>
> 2. Improves Readabilty of input and event sysfs node paths because
> names are used instead of numbers.
I do not see why it is that important. If one wants overview
/proc/bus/input/devices gives nice picture.
Re: (Aniroop Mathur)
Its correct, we can get an overview from /proc/bus/input/devices.
And therefore using this, we can know input node number for every input device.
But there are many input devices and input numbers are not fixed,
so its quite difficult to memorize input number for all input devices.
Therefore, if a user needs to open some input node from sysfs path,
he needs to check /proc/bus/input/devices before opening because
he does not know the input number. Moreover, this applies for all other
input devices and hence a user need to check this every time.
It improves readabilty as below
Before: After patch:
/dev/input0 /dev/input_proximity
/dev/input1 /dev/input_accelerometer
...many more
/sys/class/input/input0 /sys/class/input/input_proximity
/sys/class/input/input1 /sys/class/input/input_accelerometer
...many more
/sys/class/input/event0 /sys/class/input/event_proximity
/sys/class/input/event1 /sys/class/input/event_accelerometer
...many more
So, just by looking, user can directly open or refer any input node.
(no need to refer any other path)
>
> 3. Removes Input Devices Dependency. If one input device probe fails,
> other input devices still work. Before this patch, if one input
> device probe fails before input_register_device, then input number of
> other input devices changes and due to this permission settings are
> disturbed and hence HAL or upper layer cannot open the required sysfs
> node because permission denied error comes.
I have only one suggestion here: fix your userspace so that does not
depend on device initialization ordering.
Re: (Aniroop Mathur)
We cannot fix userspace because these input/event/dev number are decided/allocated in kernel
as per device initialization ordering during boot up. (userspace has no role in it)
So, userspace is not aware, which exact input number corresponds to which input device
so it ends up searching/scanning every input node untill a match is found.
So, there is input device dependency which needs to be removed.
----------------------------
IOW I am totally unconvinced that this facility is needed.
Re: (Aniroop Mathur)
I hope my problem and suggestion is more clear and convincing now.
For reference, copying Patch again:
---
drivers/input/evdev.c | 11 ++++++++++-
drivers/input/input.c | 12 +++++++++++-
include/linux/input.h | 4 ++++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index b6ded17..b6a5848 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -1131,7 +1131,16 @@ static int evdev_connect(struct input_handler *handler, struct input_dev *dev,
/* Normalize device number if it falls into legacy range */
if (dev_no < EVDEV_MINOR_BASE + EVDEV_MINORS)
dev_no -= EVDEV_MINOR_BASE;
- dev_set_name(&evdev->dev, "event%d", dev_no);
+
+ /*
+ * As per user choice (driver),
+ * name of sysfs node is set, as mentioned in node_name_unique variable.
+ * If node_name_unique is not set, default name is given.
+ */
+ if (dev->node_name_unique)
+ dev_set_name(&evdev->dev, "event_%s", dev->node_name_unique);
+ else
+ dev_set_name(&evdev->dev, "event%d", dev_no);
evdev->handle.dev = input_get_device(dev);
evdev->handle.name = dev_name(&evdev->dev);
diff --git a/drivers/input/input.c b/drivers/input/input.c
index c044699..c8126b3 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -2073,7 +2073,17 @@ int input_register_device(struct input_dev *dev)
if (!dev->setkeycode)
dev->setkeycode = input_default_setkeycode;
- dev_set_name(&dev->dev, "input%ld",
+ /*
+ * As per user choice (driver),
+ * name of sysfs node is set, as mentioned in node_name_unique variable.
+ * If node_name_unique is not set, default name is given.
+ */
+ if (dev->node_name_unique) {
+ atomic_inc_return(&input_no);
+ dev_set_name(&dev->dev, "input_%s",
+ dev->node_name_unique);
+ } else
+ dev_set_name(&dev->dev, "input%ld",
(unsigned long) atomic_inc_return(&input_no) - 1);
error = device_add(&dev->dev);
diff --git a/include/linux/input.h b/include/linux/input.h
index 82ce323..fe44643 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -43,6 +43,9 @@ struct input_value {
* @uniq: unique identification code for the device (if device has it)
* @id: id of the device (struct input_id)
* @propbit: bitmap of device properties and quirks
+ * @node_name_unique: name of input and event sysfs device node (char *).
+ * This name must be unique among all input devices and driver(user)
+ * has the responsibility to ensure it (if using).
* @evbit: bitmap of types of events supported by the device (EV_KEY,
* EV_REL, etc.)
* @keybit: bitmap of keys/buttons this device has
@@ -123,6 +126,7 @@ struct input_dev {
const char *phys;
const char *uniq;
struct input_id id;
+ char *node_name_unique;
unsigned long propbit[BITS_TO_LONGS(INPUT_PROP_CNT)];
Thanks,
Aniroop Mathur
^ permalink raw reply related
* Re: Re: [PATCH] Introduce Naming Convention in Input Subsystem
From: Dmitry Torokhov @ 2014-01-10 19:11 UTC (permalink / raw)
To: Aniroop Mathur
Cc: linux-input@vger.kernel.org, cpgs ., Anurag Aggarwal,
Naveen Kumar, aniroop.mathur@gmail.com, VIKAS KALA,
Poorva Srivastava
In-Reply-To: <25201406.444611389372583656.JavaMail.weblogic@epml19>
Hi Aniroop,
On Fri, Jan 10, 2014 at 04:49:43PM +0000, Aniroop Mathur wrote:
> Hello Mr. Torokhov,
> Greetings!
>
> On Thu, Jan 09, 2014 at 10:27:56AM +0530, Aniroop Mathur wrote:
> > This patch allows user(driver) to set sysfs node name of input
> > devices. To set sysfs node name, user(driver) just needs to set
> > node_name_unique variable. If node_name_unique is not set, default
> > name is given(as before). So, this patch is completely
> > backward-compatible.
> >
> > Sysfs Input node name format is: input_
> > Sysfs Event node name format is: event_
> >
> > This "name" is given by user and automatically, prefix(input and
> > event) is added by input core.
> >
> > This name must be unique among all input devices and driver(user) has
> > the responsibility to ensure it. If same name is used again for other
> > input device, registration of that input device will fail because two
> > input devices cannot have same name.
> >
> > Advantages of this patch are:
> >
> > 1. Reduces Booting Time of HAL/Upper-Layer because now HAL or
> > Upper-Layer do not need to search input/event number corresponding to
> > each input device in /dev/input/... This searching in /dev/input/ was
> > taking too much time. (Especially in mobile devices, where there are
> > many input devices (many sensors, touchscreen, etc), it reduces a lot
> > of booting time)
>
> I am sorry, how much time does it take to scan a directory of what, 20
> devices? If it such a factor have udev create nodes that are easier for
> you to locate, similarly how we already create nodes by-id and by-path.
> For example you can encode major:minor in device name.
>
> Re: (Aniroop Mathur)
First of all, it would be great if you could use MUA that can properly
quote and wrap long lines...
> Its correct that we can set name of a device node using udev. Yes,
> this will change the name of device node(/dev/...) but not sysfs
> node.(/sys/class/input/...) So now, the problem area will shift from
> dev path to sysfs path, because now we dont know which sysfs node to
> refer for a particular input device and hence HAL/Upper-Layer will
> need to search in /sys/class/input/... instead of /dev/... directory.
[dtor@dtor-d630 ~]$ mkdir my-sysfs-view
[dtor@dtor-d630 ~]$ ln -s
/sys/devices/platform/i8042/serio1/input/input6
my-sysfs-view/input_touchpad
[dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/
capabilities/ event6/ modalias name power/
subsystem/ uniq
device/ id/ mouse1/ phys properties
uevent
[dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/
capabilities device event6 id modalias mouse1 name phys power
properties subsystem uevent uniq
[dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/event6/
dev device power subsystem uevent
Mmmmkay?
>
> Moreover, as i know, udev is mainly for hot-pluggable devices, but my
> problem is for platform devices, which are already present on the
> board during boot up. (Like in Embedded devices)
No, udev also manages those by requesting to replay all events that
happened dyuring boot.
>
> To avoid confusion and make the problem more clear,
> I would like to explain the problem and my suggestion by taking an example:
>
> Suppose in a mobile device, there are 10 embedded input devices as below:
> Proximity --- /dev/input0 --- /sys/class/input/input0 --- /sys/class/input/event0
> Magnetometer --- /dev/input1 --- /sys/class/input/input1 --- /sys/class/input/event1
> Accelerometer --- /dev/input2 --- /sys/class/input/input2 --- /sys/class/input/event2
> Touchscreen --- /dev/input3 --- /sys/class/input/input3 --- /sys/class/input/event3
> ... 6 more like this
> (All these are created during boot up time)
>
> Kernel has created all these nodes, so that HAL/UpperLayer can read or
> write values from it. HAL/Upper-Layer needs to do main tasks like:
> 1. Read raw data - does through /dev/input<num>
> 2. Enable device - does through sys/class/input<num>/enable
> 3. Set delay - does through sys/class/input<num>/delay
> and many more...
>
> Now, Lets suppose we need to do these tasks for Accelerometer.
>
> If dev node name is set, HAL can directly read value from it (no
> search required) But for enabling the accelerometer device or set the
> delay of a hardware chip, there is no direct way, HAL can know which
> input node to refer for accelerometer because the input number is
> created dynamically as per device probe order, so this input number
> can be anything (0,1,2,3...) So HAL will need to search every input
> node and read its name attribute and keep on searching until a match
> is found between the "attribute name" and "name passed as parameter".
> Like for accelerometer, this searching needs to be done for all other
> input devices. All of this part is done during booting and this takes
> a lot for time from booting perspective.
>
See the above. You can very easily create your own private 'view' of
sysfs, no kernel changes needed.
> As I measured, if there are ten devices, it is taking 1 second to do
> all this searching. (for all devices) So for 20 devices, i guess, it
> could take upto 2 seconds.
That seems _very_ high, maybe you need to profile your code a bit. To
search though 2 directories with less than a hundred files each should
not take 1 second.
>
> With naming convention, there is no need of search neither for dev
> path nor for sysfs path because HAL directly know which node to refer
> for which input device and hence this 1 second is reduced to 10ms or
> even less, therefore saving 990ms. I believe, this is a very good
> time saving. (from device booting perspective)
OK, so create your own sysfs view and use it to do direct lookups.
>
> (Is there any direct way, without scanning all nodes for every input
> device ?)
>
> >
> > 2. Improves Readabilty of input and event sysfs node paths because
> > names are used instead of numbers.
>
> I do not see why it is that important. If one wants overview
> /proc/bus/input/devices gives nice picture.
>
> Re: (Aniroop Mathur)
> Its correct, we can get an overview from /proc/bus/input/devices.
> And therefore using this, we can know input node number for every input device.
> But there are many input devices and input numbers are not fixed,
> so its quite difficult to memorize input number for all input devices.
> Therefore, if a user needs to open some input node from sysfs path,
> he needs to check /proc/bus/input/devices before opening because
> he does not know the input number. Moreover, this applies for all other
> input devices and hence a user need to check this every time.
>
> It improves readabilty as below
>
> Before: After patch:
> /dev/input0 /dev/input_proximity
> /dev/input1 /dev/input_accelerometer
> ...many more
>
> /sys/class/input/input0 /sys/class/input/input_proximity
> /sys/class/input/input1 /sys/class/input/input_accelerometer
> ...many more
>
> /sys/class/input/event0 /sys/class/input/event_proximity
> /sys/class/input/event1 /sys/class/input/event_accelerometer
> ...many more
>
> So, just by looking, user can directly open or refer any input node.
> (no need to refer any other path)
User as in end user or your HAL layer?
>
> >
> > 3. Removes Input Devices Dependency. If one input device probe fails,
> > other input devices still work. Before this patch, if one input
> > device probe fails before input_register_device, then input number of
> > other input devices changes and due to this permission settings are
> > disturbed and hence HAL or upper layer cannot open the required sysfs
> > node because permission denied error comes.
>
> I have only one suggestion here: fix your userspace so that does not
> depend on device initialization ordering.
>
> Re: (Aniroop Mathur)
> We cannot fix userspace because these input/event/dev number are
> decided/allocated in kernel as per device initialization ordering
> during boot up. (userspace has no role in it) So, userspace is not
> aware, which exact input number corresponds to which input device so
> it ends up searching/scanning every input node untill a match is
> found.
>
> So, there is input device dependency which needs to be removed.
Do not use numbers. We emit uevents describing the devices and there a
_lot_ of data there that helps identifying device, such as its path,
subsystem, name, etc.
>
> ----------------------------
>
> IOW I am totally unconvinced that this facility is needed.
>
> Re: (Aniroop Mathur)
> I hope my problem and suggestion is more clear and convincing now.
>
Not in the slightest, I am sorry.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: PDT scan cleanup
From: Christopher Heiny @ 2014-01-10 20:23 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires
In-Reply-To: <1389126821-4066-1-git-send-email-cheiny@synaptics.com>
On 01/07/2014 12:33 PM, Christopher Heiny wrote:
> Eliminates copy-paste code that handled scans of the Page Descriptor Table,
> replacing it with a single PDT scan routine that invokes a callback function.
> The scan routine is not static so that it can be used by the firmware update
> code (under development, not yet submitted).
>
> Updated the copyright dates while we were at it.
Hi Dmitry,
Could you apply this or provide some feedback on it? We've got a
pending patch that depends on it, and that pending work will bring the
driver back to a working (if not necessarily beautiful) state. I don't
want to submit it if this change isn't satisfactory, though.
Thanks,
Chris
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> ---
>
> drivers/input/rmi4/rmi_driver.c | 155 ++++++++++++++++++++--------------------
> drivers/input/rmi4/rmi_driver.h | 6 +-
> 2 files changed, 83 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index eb790ff..cbd6485 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2011-2013 Synaptics Incorporated
> + * Copyright (c) 2011-2014 Synaptics Incorporated
> * Copyright (c) 2011 Unixphere
> *
> * This driver provides the core support for a single RMI4-based device.
> @@ -566,85 +566,80 @@ err_free_mem:
> return error;
> }
>
> -/*
> - * Scan the PDT for F01 so we can force a reset before anything else
> - * is done. This forces the sensor into a known state, and also
> - * forces application of any pending updates from reflashing the
> - * firmware or configuration.
> - *
> - * We have to do this before actually building the PDT because the reflash
> - * updates (if any) might cause various registers to move around.
> - */
> -static int rmi_initial_reset(struct rmi_device *rmi_dev)
> +
> +#define RMI_SCAN_CONTINUE 0
> +#define RMI_SCAN_DONE 1
> +
> +static int rmi_initial_reset(struct rmi_device *rmi_dev,
> + void *clbk_ctx, struct pdt_entry *pdt_entry, int page)
> {
> - struct pdt_entry pdt_entry;
> - int page;
> - struct device *dev = &rmi_dev->dev;
> - bool done = false;
> - bool has_f01 = false;
> - int i;
> int retval;
> - const struct rmi_device_platform_data *pdata =
> - to_rmi_platform_data(rmi_dev);
>
> - dev_dbg(dev, "Initial reset.\n");
> + if (pdt_entry->function_number == 0x01) {
> + u16 cmd_addr = page + pdt_entry->command_base_addr;
> + u8 cmd_buf = RMI_DEVICE_RESET_CMD;
> + const struct rmi_device_platform_data *pdata =
> + to_rmi_platform_data(rmi_dev);
>
> - for (page = 0; (page <= RMI4_MAX_PAGE) && !done; page++) {
> - u16 page_start = RMI4_PAGE_SIZE * page;
> - u16 pdt_start = page_start + PDT_START_SCAN_LOCATION;
> - u16 pdt_end = page_start + PDT_END_SCAN_LOCATION;
> + retval = rmi_write_block(rmi_dev, cmd_addr, &cmd_buf, 1);
> + if (retval < 0) {
> + dev_err(&rmi_dev->dev,
> + "Initial reset failed. Code = %d.\n", retval);
> + return retval;
> + }
> + mdelay(pdata->reset_delay_ms);
>
> - done = true;
> - for (i = pdt_start; i >= pdt_end; i -= RMI_PDT_ENTRY_SIZE) {
> - retval = rmi_read_pdt_entry(rmi_dev, &pdt_entry, i);
> - if (retval < 0)
> - return retval;
> + return RMI_SCAN_DONE;
> + }
>
> - if (RMI4_END_OF_PDT(pdt_entry.function_number))
> - break;
> - done = false;
> + /* F01 should always be on page 0. If we don't find it there, fail. */
> + return (!page) ? RMI_SCAN_CONTINUE : -ENODEV;
> +}
>
> - if (pdt_entry.function_number == 0x01) {
> - u16 cmd_addr = page_start +
> - pdt_entry.command_base_addr;
> - u8 cmd_buf = RMI_DEVICE_RESET_CMD;
> - retval = rmi_write_block(rmi_dev, cmd_addr,
> - &cmd_buf, 1);
> - if (retval < 0) {
> - dev_err(dev, "Initial reset failed. Code = %d.\n",
> - retval);
> - return retval;
> - }
> - mdelay(pdata->reset_delay_ms);
> - done = true;
> - has_f01 = true;
> - break;
> - }
> - }
> - }
> +static int rmi_create_functions_clbk(struct rmi_device *rmi_dev,
> + void *clbk_ctx, struct pdt_entry *entry, int page)
> +{
> + int *irq_count = (int *)clbk_ctx;
>
> - if (!has_f01) {
> - dev_warn(dev, "WARNING: Failed to find F01 for initial reset.\n");
> - return -ENODEV;
> - }
> + return create_function(rmi_dev, entry, irq_count,
> + RMI4_PAGE_SIZE * page);
> +}
> +
> +static int rmi_create_functions(struct rmi_device *rmi_dev)
> +{
> + struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
> + struct device *dev = &rmi_dev->dev;
> + int irq_count = 0;
> + int retval;
> +
> + // XXX need to make sure we create F01 first...
> + // XXX or do we? It might not be required in the updated structure.
> + retval = rmi_scan_pdt(rmi_dev, &irq_count, rmi_create_functions_clbk);
> +
> + if (retval)
> + return retval;
> +
> + // TODO: I think we need to count the IRQs before creating the
> + // functions.
> + data->irq_count = irq_count;
> + data->num_of_irq_regs = (irq_count + 7) / 8;
>
> return 0;
> }
>
> -static int rmi_scan_pdt(struct rmi_device *rmi_dev)
> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
> + int (*rmi_pdt_scan_clbk)(struct rmi_device *rmi_dev,
> + void *clbk_ctx, struct pdt_entry *entry, int page))
> {
> - struct rmi_driver_data *data;
> + struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
> struct pdt_entry pdt_entry;
> int page;
> - struct device *dev = &rmi_dev->dev;
> - int irq_count = 0;
> - bool done = false;
> int i;
> - int retval;
> -
> - dev_dbg(dev, "Scanning PDT...\n");
> + bool done = false;
> + int retval = 0;
>
> - data = dev_get_drvdata(&rmi_dev->dev);
> + // TODO: With F01 and reflash as part of the core now, is this
> + // lock still required?
> mutex_lock(&data->pdt_mutex);
>
> for (page = 0; (page <= RMI4_MAX_PAGE) && !done; page++) {
> @@ -656,27 +651,27 @@ static int rmi_scan_pdt(struct rmi_device *rmi_dev)
> for (i = pdt_start; i >= pdt_end; i -= RMI_PDT_ENTRY_SIZE) {
> retval = rmi_read_pdt_entry(rmi_dev, &pdt_entry, i);
> if (retval < 0)
> - goto error_exit;
> + return retval;
>
> if (RMI4_END_OF_PDT(pdt_entry.function_number))
> break;
>
> - dev_dbg(dev, "Found F%02X on page %#04x\n",
> + dev_dbg(&rmi_dev->dev, "Found F%02X on page %#04x\n",
> pdt_entry.function_number, page);
> done = false;
>
> - // XXX need to make sure we create F01 first...
> - retval = create_function(rmi_dev,
> - &pdt_entry, &irq_count, page_start);
> -
> - if (retval)
> + retval = rmi_pdt_scan_clbk(rmi_dev, ctx,
> + &pdt_entry, page);
> + if (retval < 0) {
> goto error_exit;
> + } else if (retval == RMI_SCAN_DONE) {
> + done = true;
> + break;
> + }
> }
> done = done || data->f01_bootloader_mode;
> }
> - data->irq_count = irq_count;
> - data->num_of_irq_regs = (irq_count + 7) / 8;
> - dev_dbg(dev, "%s: Done with PDT scan.\n", __func__);
> +
> retval = 0;
>
> error_exit:
> @@ -684,6 +679,7 @@ error_exit:
> return retval;
> }
>
> +
> #ifdef CONFIG_PM_SLEEP
> static int rmi_driver_suspend(struct device *dev)
> {
> @@ -797,10 +793,15 @@ static int rmi_driver_probe(struct device *dev)
>
> /*
> * Right before a warm boot, the sensor might be in some unusual state,
> - * such as F54 diagnostics, or F34 bootloader mode. In order to clear
> - * the sensor to a known state, we issue a initial reset to clear any
> + * such as F54 diagnostics, or F34 bootloader mode after a firmware
> + * or configuration update. In order to clear the sensor to a known
> + * state and/or apply any updates, we issue a initial reset to clear any
> * previous settings and force it into normal operation.
> *
> + * We have to do this before actually building the PDT because
> + * the reflash updates (if any) might cause various registers to move
> + * around.
> + *
> * For a number of reasons, this initial reset may fail to return
> * within the specified time, but we'll still be able to bring up the
> * driver normally after that failure. This occurs most commonly in
> @@ -813,11 +814,11 @@ static int rmi_driver_probe(struct device *dev)
> */
> if (!pdata->reset_delay_ms)
> pdata->reset_delay_ms = DEFAULT_RESET_DELAY_MS;
> - retval = rmi_initial_reset(rmi_dev);
> + retval = rmi_scan_pdt(rmi_dev, NULL, rmi_initial_reset);
> if (retval)
> dev_warn(dev, "RMI initial reset failed! Continuing in spite of this.\n");
>
> - retval = rmi_scan_pdt(rmi_dev);
> + retval = rmi_create_functions(rmi_dev);
> if (retval) {
> dev_err(dev, "PDT scan for %s failed with code %d.\n",
> pdata->sensor_name, retval);
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
> index df9ddd8..f73be73 100644
> --- a/drivers/input/rmi4/rmi_driver.h
> +++ b/drivers/input/rmi4/rmi_driver.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2011-2013 Synaptics Incorporated
> + * Copyright (c) 2011-2014 Synaptics Incorporated
> * Copyright (c) 2011 Unixphere
> *
> * This program is free software; you can redistribute it and/or modify it
> @@ -108,6 +108,10 @@ struct pdt_entry {
> int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
> u16 pdt_address);
>
> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
> + int (*rmi_pdt_scan_clbk)(struct rmi_device *rmi_dev,
> + void *clbk_ctx, struct pdt_entry *entry, int page));
> +
> bool rmi_is_physical_driver(struct device_driver *);
> int rmi_register_physical_driver(void);
> void rmi_unregister_physical_driver(void);
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH v2] Input: add i2c/smbus driver for elan touchpad
From: Benson Leung @ 2014-01-10 20:30 UTC (permalink / raw)
To: Duson Lin
Cc: linux-kernel@vger.kernel.org, linux-input, Dmitry Torokhov,
agnescheng, phoenix, jeff.chuang
In-Reply-To: <1389064083-11018-1-git-send-email-dusonlin@emc.com.tw>
On Mon, Jan 6, 2014 at 7:08 PM, Duson Lin <dusonlin@emc.com.tw> wrote:
> This driver adds support for elan i2c/smbus touchpad found on some laptops PC
>
> Signed-off-by: Duson Lin <dusonlin@emc.com.tw>
Reviewed-by: Benson Leung <bleung@chromium.org>
--
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: PDT scan cleanup
From: Dmitry Torokhov @ 2014-01-10 20:31 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <52D056B3.406@synaptics.com>
On Friday, January 10, 2014 12:23:15 PM Christopher Heiny wrote:
> On 01/07/2014 12:33 PM, Christopher Heiny wrote:
> > Eliminates copy-paste code that handled scans of the Page Descriptor
> > Table,
> > replacing it with a single PDT scan routine that invokes a callback
> > function. The scan routine is not static so that it can be used by the
> > firmware update code (under development, not yet submitted).
> >
> > Updated the copyright dates while we were at it.
>
> Hi Dmitry,
>
> Could you apply this or provide some feedback on it? We've got a
> pending patch that depends on it, and that pending work will bring the
> driver back to a working (if not necessarily beautiful) state. I don't
> want to submit it if this change isn't satisfactory, though.
Speaking of the devil. I was just thinking about it and I wanted to ask
you to send me an example of how it is used as I can;t make my mind about
it.
In general it is OK to submit a few patches at a time even if they are
depend on each other - it gives me better context (as long as there
aren't 80 of those so that I down in them ;) ).
Thanks.
--
Dmitry
^ permalink raw reply
* Re: Re: [PATCH] Introduce Naming Convention in Input Subsystem
From: Aniroop Mathur @ 2014-01-10 21:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Aniroop Mathur, linux-input@vger.kernel.org, cpgs .,
Anurag Aggarwal, Naveen Kumar, VIKAS KALA, Poorva Srivastava
In-Reply-To: <20140110191108.GC10889@core.coreip.homeip.net>
Hello Mr. Torokhov,
Greetings!
First of all, So sorry, unfortunately i used HTML text again.
and Many thanks for all replies.
Sending email again in plain text.
On Sat, Jan 11, 2014 at 12:41 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Aniroop,
>
> On Fri, Jan 10, 2014 at 04:49:43PM +0000, Aniroop Mathur wrote:
>> Hello Mr. Torokhov,
>> Greetings!
>>
>> On Thu, Jan 09, 2014 at 10:27:56AM +0530, Aniroop Mathur wrote:
>> > This patch allows user(driver) to set sysfs node name of input
>> > devices. To set sysfs node name, user(driver) just needs to set
>> > node_name_unique variable. If node_name_unique is not set, default
>> > name is given(as before). So, this patch is completely
>> > backward-compatible.
>> >
>> > Sysfs Input node name format is: input_
>> > Sysfs Event node name format is: event_
>> >
>> > This "name" is given by user and automatically, prefix(input and
>> > event) is added by input core.
>> >
>> > This name must be unique among all input devices and driver(user) has
>> > the responsibility to ensure it. If same name is used again for other
>> > input device, registration of that input device will fail because two
>> > input devices cannot have same name.
>> >
>> > Advantages of this patch are:
>> >
>> > 1. Reduces Booting Time of HAL/Upper-Layer because now HAL or
>> > Upper-Layer do not need to search input/event number corresponding to
>> > each input device in /dev/input/... This searching in /dev/input/ was
>> > taking too much time. (Especially in mobile devices, where there are
>> > many input devices (many sensors, touchscreen, etc), it reduces a lot
>> > of booting time)
>>
>> I am sorry, how much time does it take to scan a directory of what, 20
>> devices? If it such a factor have udev create nodes that are easier for
>> you to locate, similarly how we already create nodes by-id and by-path.
>> For example you can encode major:minor in device name.
>>
>> Re: (Aniroop Mathur)
>
> First of all, it would be great if you could use MUA that can properly
> quote and wrap long lines...
>
>> Its correct that we can set name of a device node using udev. Yes,
>> this will change the name of device node(/dev/...) but not sysfs
>> node.(/sys/class/input/...) So now, the problem area will shift from
>> dev path to sysfs path, because now we dont know which sysfs node to
>> refer for a particular input device and hence HAL/Upper-Layer will
>> need to search in /sys/class/input/... instead of /dev/... directory.
>
> [dtor@dtor-d630 ~]$ mkdir my-sysfs-view
> [dtor@dtor-d630 ~]$ ln -s
> /sys/devices/platform/i8042/serio1/input/input6
> my-sysfs-view/input_touchpad
> [dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/
> capabilities/ event6/ modalias name power/
> subsystem/ uniq
> device/ id/ mouse1/ phys properties
> uevent
> [dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/
> capabilities device event6 id modalias mouse1 name phys power
> properties subsystem uevent uniq
> [dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/event6/
> dev device power subsystem uevent
>
> Mmmmkay?
>
Yes, agreed, we can use udev and soft links to achieve this.
But i think there is something more to take care.
So far, as per discussion, i understood that if an end user wants to use
node names instead of numbers, he/she has to do the following things:
1. Create rules for all input devices in udev rule file i.e. set atleast
unique id and unique name.
(end user need to determine unique id too for every input device)
2. Create links for all input device nodes using names.
(in probe function, after input_register_device)
By following above two steps, the file structure will look like:
devfs - /dev/input_proximity
sysfs - my-sysfs-view/input_proximity --> sys/class/input/input2
sysfs - my-sysfs-view/event_proximity --> sys/class/input/event2
But my concern is why to create trouble for end user to perform
and spend time for two extra steps, when an easy way is possible
to achieve the same task ?
With this patch, end user only need to set node_name_unique variable
and right after that, both for devfs and sysfs,same node name is set.
End user does not need to do or take care of any other extra work,
like creating entry in udev rules, creating links, etc
Also, with creating links for all input devices and checking udev rules
before actually creating a device node, will only increase computation
and time in kernel code.
My purpose is to avoid extra work load and directly create node names
within input subsystem. Also backward compatibility is there.
So i think, it is better than the other alternative way.
Isn't this more easy ? Is there any side-effect or drawback of this patch ?
>>
>> Moreover, as i know, udev is mainly for hot-pluggable devices, but my
>> problem is for platform devices, which are already present on the
>> board during boot up. (Like in Embedded devices)
>
> No, udev also manages those by requesting to replay all events that
> happened dyuring boot.
>
>>
>> To avoid confusion and make the problem more clear,
>> I would like to explain the problem and my suggestion by taking an example:
>>
>> Suppose in a mobile device, there are 10 embedded input devices as below:
>> Proximity --- /dev/input0 --- /sys/class/input/input0 --- /sys/class/input/event0
>> Magnetometer --- /dev/input1 --- /sys/class/input/input1 --- /sys/class/input/event1
>> Accelerometer --- /dev/input2 --- /sys/class/input/input2 --- /sys/class/input/event2
>> Touchscreen --- /dev/input3 --- /sys/class/input/input3 --- /sys/class/input/event3
>> ... 6 more like this
>> (All these are created during boot up time)
>>
>> Kernel has created all these nodes, so that HAL/UpperLayer can read or
>> write values from it. HAL/Upper-Layer needs to do main tasks like:
>> 1. Read raw data - does through /dev/input<num>
>> 2. Enable device - does through sys/class/input<num>/enable
>> 3. Set delay - does through sys/class/input<num>/delay
>> and many more...
>>
>> Now, Lets suppose we need to do these tasks for Accelerometer.
>>
>> If dev node name is set, HAL can directly read value from it (no
>> search required) But for enabling the accelerometer device or set the
>> delay of a hardware chip, there is no direct way, HAL can know which
>> input node to refer for accelerometer because the input number is
>> created dynamically as per device probe order, so this input number
>> can be anything (0,1,2,3...) So HAL will need to search every input
>> node and read its name attribute and keep on searching until a match
>> is found between the "attribute name" and "name passed as parameter".
>> Like for accelerometer, this searching needs to be done for all other
>> input devices. All of this part is done during booting and this takes
>> a lot for time from booting perspective.
>>
>
> See the above. You can very easily create your own private 'view' of
> sysfs, no kernel changes needed.
>
>> As I measured, if there are ten devices, it is taking 1 second to do
>> all this searching. (for all devices) So for 20 devices, i guess, it
>> could take upto 2 seconds.
>
> That seems _very_ high, maybe you need to profile your code a bit. To
> search though 2 directories with less than a hundred files each should
> not take 1 second.
>
In this i am including time to open a directory, close a directory, open file of
that directory, close file of that directory, searching and computation part.
Including all these, every time for each input device.
All this sums upto 1 second.
>>
>> With naming convention, there is no need of search neither for dev
>> path nor for sysfs path because HAL directly know which node to refer
>> for which input device and hence this 1 second is reduced to 10ms or
>> even less, therefore saving 990ms. I believe, this is a very good
>> time saving. (from device booting perspective)
>
> OK, so create your own sysfs view and use it to do direct lookups.
>
>>
>> (Is there any direct way, without scanning all nodes for every input
>> device ?)
>>
>> >
>> > 2. Improves Readabilty of input and event sysfs node paths because
>> > names are used instead of numbers.
>>
>> I do not see why it is that important. If one wants overview
>> /proc/bus/input/devices gives nice picture.
>>
>> Re: (Aniroop Mathur)
>> Its correct, we can get an overview from /proc/bus/input/devices.
>> And therefore using this, we can know input node number for every input device.
>> But there are many input devices and input numbers are not fixed,
>> so its quite difficult to memorize input number for all input devices.
>> Therefore, if a user needs to open some input node from sysfs path,
>> he needs to check /proc/bus/input/devices before opening because
>> he does not know the input number. Moreover, this applies for all other
>> input devices and hence a user need to check this every time.
>>
>> It improves readabilty as below
>>
>> Before: After patch:
>> /dev/input0 /dev/input_proximity
>> /dev/input1 /dev/input_accelerometer
>> ...many more
>>
>> /sys/class/input/input0 /sys/class/input/input_proximity
>> /sys/class/input/input1 /sys/class/input/input_accelerometer
>> ...many more
>>
>> /sys/class/input/event0 /sys/class/input/event_proximity
>> /sys/class/input/event1 /sys/class/input/event_accelerometer
>> ...many more
>>
>> So, just by looking, user can directly open or refer any input node.
>> (no need to refer any other path)
>
> User as in end user or your HAL layer?
>
End user.
>>
>> >
>> > 3. Removes Input Devices Dependency. If one input device probe fails,
>> > other input devices still work. Before this patch, if one input
>> > device probe fails before input_register_device, then input number of
>> > other input devices changes and due to this permission settings are
>> > disturbed and hence HAL or upper layer cannot open the required sysfs
>> > node because permission denied error comes.
>>
>> I have only one suggestion here: fix your userspace so that does not
>> depend on device initialization ordering.
>>
>> Re: (Aniroop Mathur)
>> We cannot fix userspace because these input/event/dev number are
>> decided/allocated in kernel as per device initialization ordering
>> during boot up. (userspace has no role in it) So, userspace is not
>> aware, which exact input number corresponds to which input device so
>> it ends up searching/scanning every input node untill a match is
>> found.
>>
>> So, there is input device dependency which needs to be removed.
>
> Do not use numbers. We emit uevents describing the devices and there a
> _lot_ of data there that helps identifying device, such as its path,
> subsystem, name, etc.
>
Sorry, I am not able to understand this point with respect to removing input
device dependency. Please elaborate a bit more.
>>
>> ----------------------------
>>
>> IOW I am totally unconvinced that this facility is needed.
>>
>> Re: (Aniroop Mathur)
>> I hope my problem and suggestion is more clear and convincing now.
>>
>
> Not in the slightest, I am sorry.
>
> Thanks.
>
> --
> Dmitry
Thanks,
Aniroop Mathur
^ permalink raw reply
* Re: Re: [PATCH] Introduce Naming Convention in Input Subsystem
From: Dmitry Torokhov @ 2014-01-10 21:46 UTC (permalink / raw)
To: Aniroop Mathur
Cc: Aniroop Mathur, linux-input@vger.kernel.org, cpgs .,
Anurag Aggarwal, Naveen Kumar, VIKAS KALA, Poorva Srivastava
In-Reply-To: <CADYu30_qxKV7SmUdUc1Y-sN+KDYbWyz6Yv_H6BsSA-koK9aWYQ@mail.gmail.com>
On Sat, Jan 11, 2014 at 02:55:33AM +0530, Aniroop Mathur wrote:
> Hello Mr. Torokhov,
> Greetings!
>
> First of all, So sorry, unfortunately i used HTML text again.
> and Many thanks for all replies.
>
> Sending email again in plain text.
>
>
> On Sat, Jan 11, 2014 at 12:41 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Aniroop,
> >
> > On Fri, Jan 10, 2014 at 04:49:43PM +0000, Aniroop Mathur wrote:
> >> Hello Mr. Torokhov,
> >> Greetings!
> >>
> >> On Thu, Jan 09, 2014 at 10:27:56AM +0530, Aniroop Mathur wrote:
> >> > This patch allows user(driver) to set sysfs node name of input
> >> > devices. To set sysfs node name, user(driver) just needs to set
> >> > node_name_unique variable. If node_name_unique is not set, default
> >> > name is given(as before). So, this patch is completely
> >> > backward-compatible.
> >> >
> >> > Sysfs Input node name format is: input_
> >> > Sysfs Event node name format is: event_
> >> >
> >> > This "name" is given by user and automatically, prefix(input and
> >> > event) is added by input core.
> >> >
> >> > This name must be unique among all input devices and driver(user) has
> >> > the responsibility to ensure it. If same name is used again for other
> >> > input device, registration of that input device will fail because two
> >> > input devices cannot have same name.
> >> >
> >> > Advantages of this patch are:
> >> >
> >> > 1. Reduces Booting Time of HAL/Upper-Layer because now HAL or
> >> > Upper-Layer do not need to search input/event number corresponding to
> >> > each input device in /dev/input/... This searching in /dev/input/ was
> >> > taking too much time. (Especially in mobile devices, where there are
> >> > many input devices (many sensors, touchscreen, etc), it reduces a lot
> >> > of booting time)
> >>
> >> I am sorry, how much time does it take to scan a directory of what, 20
> >> devices? If it such a factor have udev create nodes that are easier for
> >> you to locate, similarly how we already create nodes by-id and by-path.
> >> For example you can encode major:minor in device name.
> >>
> >> Re: (Aniroop Mathur)
> >
> > First of all, it would be great if you could use MUA that can properly
> > quote and wrap long lines...
> >
> >> Its correct that we can set name of a device node using udev. Yes,
> >> this will change the name of device node(/dev/...) but not sysfs
> >> node.(/sys/class/input/...) So now, the problem area will shift from
> >> dev path to sysfs path, because now we dont know which sysfs node to
> >> refer for a particular input device and hence HAL/Upper-Layer will
> >> need to search in /sys/class/input/... instead of /dev/... directory.
> >
> > [dtor@dtor-d630 ~]$ mkdir my-sysfs-view
> > [dtor@dtor-d630 ~]$ ln -s
> > /sys/devices/platform/i8042/serio1/input/input6
> > my-sysfs-view/input_touchpad
> > [dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/
> > capabilities/ event6/ modalias name power/
> > subsystem/ uniq
> > device/ id/ mouse1/ phys properties
> > uevent
> > [dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/
> > capabilities device event6 id modalias mouse1 name phys power
> > properties subsystem uevent uniq
> > [dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/event6/
> > dev device power subsystem uevent
> >
> > Mmmmkay?
> >
>
> Yes, agreed, we can use udev and soft links to achieve this.
> But i think there is something more to take care.
>
> So far, as per discussion, i understood that if an end user wants to use
> node names instead of numbers, he/she has to do the following things:
No, not the end user, system integrator, which is quite different
beast.
> 1. Create rules for all input devices in udev rule file i.e. set atleast
> unique id and unique name.
> (end user need to determine unique id too for every input device)
> 2. Create links for all input device nodes using names.
> (in probe function, after input_register_device)
>
> By following above two steps, the file structure will look like:
> devfs - /dev/input_proximity
> sysfs - my-sysfs-view/input_proximity --> sys/class/input/input2
> sysfs - my-sysfs-view/event_proximity --> sys/class/input/event2
>
> But my concern is why to create trouble for end user to perform
> and spend time for two extra steps, when an easy way is possible
> to achieve the same task ?
>
> With this patch, end user only need to set node_name_unique variable
> and right after that, both for devfs and sysfs,same node name is set.
> End user does not need to do or take care of any other extra work,
> like creating entry in udev rules, creating links, etc
>
> Also, with creating links for all input devices and checking udev rules
> before actually creating a device node, will only increase computation
> and time in kernel code.
So do not create links, use something else to track devices. You are
getting uevents, that is all you need.
>
> My purpose is to avoid extra work load and directly create node names
> within input subsystem. Also backward compatibility is there.
> So i think, it is better than the other alternative way.
> Isn't this more easy ? Is there any side-effect or drawback of this patch ?
Yes, there is huge side effect - it is maintenance nightmare, where one
driver can now cause failure for others. What if I have 2 proximity
sensors? 2 accelerometers? How will generic drivers select names that
will satisfy all boards that might use the chips out there? Are you
proposing to put this data in device tree for example? Board files?
IOW no, this is not right solution and the patch will not be accepted.
>
> >>
> >> Moreover, as i know, udev is mainly for hot-pluggable devices, but my
> >> problem is for platform devices, which are already present on the
> >> board during boot up. (Like in Embedded devices)
> >
> > No, udev also manages those by requesting to replay all events that
> > happened dyuring boot.
> >
> >>
> >> To avoid confusion and make the problem more clear,
> >> I would like to explain the problem and my suggestion by taking an example:
> >>
> >> Suppose in a mobile device, there are 10 embedded input devices as below:
> >> Proximity --- /dev/input0 --- /sys/class/input/input0 --- /sys/class/input/event0
> >> Magnetometer --- /dev/input1 --- /sys/class/input/input1 --- /sys/class/input/event1
> >> Accelerometer --- /dev/input2 --- /sys/class/input/input2 --- /sys/class/input/event2
> >> Touchscreen --- /dev/input3 --- /sys/class/input/input3 --- /sys/class/input/event3
> >> ... 6 more like this
> >> (All these are created during boot up time)
> >>
> >> Kernel has created all these nodes, so that HAL/UpperLayer can read or
> >> write values from it. HAL/Upper-Layer needs to do main tasks like:
> >> 1. Read raw data - does through /dev/input<num>
> >> 2. Enable device - does through sys/class/input<num>/enable
> >> 3. Set delay - does through sys/class/input<num>/delay
> >> and many more...
> >>
> >> Now, Lets suppose we need to do these tasks for Accelerometer.
> >>
> >> If dev node name is set, HAL can directly read value from it (no
> >> search required) But for enabling the accelerometer device or set the
> >> delay of a hardware chip, there is no direct way, HAL can know which
> >> input node to refer for accelerometer because the input number is
> >> created dynamically as per device probe order, so this input number
> >> can be anything (0,1,2,3...) So HAL will need to search every input
> >> node and read its name attribute and keep on searching until a match
> >> is found between the "attribute name" and "name passed as parameter".
> >> Like for accelerometer, this searching needs to be done for all other
> >> input devices. All of this part is done during booting and this takes
> >> a lot for time from booting perspective.
> >>
> >
> > See the above. You can very easily create your own private 'view' of
> > sysfs, no kernel changes needed.
> >
> >> As I measured, if there are ten devices, it is taking 1 second to do
> >> all this searching. (for all devices) So for 20 devices, i guess, it
> >> could take upto 2 seconds.
> >
> > That seems _very_ high, maybe you need to profile your code a bit. To
> > search though 2 directories with less than a hundred files each should
> > not take 1 second.
> >
>
> In this i am including time to open a directory, close a directory, open file of
> that directory, close file of that directory, searching and computation part.
> Including all these, every time for each input device.
> All this sums upto 1 second.
Why are you doing it one at at time? It appears that this happens in
build at boot up for you...
>
> >>
> >> With naming convention, there is no need of search neither for dev
> >> path nor for sysfs path because HAL directly know which node to refer
> >> for which input device and hence this 1 second is reduced to 10ms or
> >> even less, therefore saving 990ms. I believe, this is a very good
> >> time saving. (from device booting perspective)
> >
> > OK, so create your own sysfs view and use it to do direct lookups.
> >
> >>
> >> (Is there any direct way, without scanning all nodes for every input
> >> device ?)
> >>
> >> >
> >> > 2. Improves Readabilty of input and event sysfs node paths because
> >> > names are used instead of numbers.
> >>
> >> I do not see why it is that important. If one wants overview
> >> /proc/bus/input/devices gives nice picture.
> >>
> >> Re: (Aniroop Mathur)
> >> Its correct, we can get an overview from /proc/bus/input/devices.
> >> And therefore using this, we can know input node number for every input device.
> >> But there are many input devices and input numbers are not fixed,
> >> so its quite difficult to memorize input number for all input devices.
> >> Therefore, if a user needs to open some input node from sysfs path,
> >> he needs to check /proc/bus/input/devices before opening because
> >> he does not know the input number. Moreover, this applies for all other
> >> input devices and hence a user need to check this every time.
> >>
> >> It improves readabilty as below
> >>
> >> Before: After patch:
> >> /dev/input0 /dev/input_proximity
> >> /dev/input1 /dev/input_accelerometer
> >> ...many more
> >>
> >> /sys/class/input/input0 /sys/class/input/input_proximity
> >> /sys/class/input/input1 /sys/class/input/input_accelerometer
> >> ...many more
> >>
> >> /sys/class/input/event0 /sys/class/input/event_proximity
> >> /sys/class/input/event1 /sys/class/input/event_accelerometer
> >> ...many more
> >>
> >> So, just by looking, user can directly open or refer any input node.
> >> (no need to refer any other path)
> >
> > User as in end user or your HAL layer?
> >
>
> End user.
Why would end user care? He wants his touchscreen to work, not fiddle
with its settings, And we aleady discussed what system integrator should
do.
>
> >>
> >> >
> >> > 3. Removes Input Devices Dependency. If one input device probe fails,
> >> > other input devices still work. Before this patch, if one input
> >> > device probe fails before input_register_device, then input number of
> >> > other input devices changes and due to this permission settings are
> >> > disturbed and hence HAL or upper layer cannot open the required sysfs
> >> > node because permission denied error comes.
> >>
> >> I have only one suggestion here: fix your userspace so that does not
> >> depend on device initialization ordering.
> >>
> >> Re: (Aniroop Mathur)
> >> We cannot fix userspace because these input/event/dev number are
> >> decided/allocated in kernel as per device initialization ordering
> >> during boot up. (userspace has no role in it) So, userspace is not
> >> aware, which exact input number corresponds to which input device so
> >> it ends up searching/scanning every input node untill a match is
> >> found.
> >>
> >> So, there is input device dependency which needs to be removed.
> >
> > Do not use numbers. We emit uevents describing the devices and there a
> > _lot_ of data there that helps identifying device, such as its path,
> > subsystem, name, etc.
> >
>
> Sorry, I am not able to understand this point with respect to removing input
> device dependency. Please elaborate a bit more.
Look at the data that is passed in uevents that are sent either when new
input device is created, or when you request replay of such evenst,
realize that it is enough to identify the device and stop relying on
inputX names to remain static. That's it.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: PDT scan cleanup
From: Christopher Heiny @ 2014-01-10 21:50 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <1754181.lppeTWGTml@dtor-d630.eng.vmware.com>
On 01/10/2014 12:31 PM, Dmitry Torokhov wrote:
> On Friday, January 10, 2014 12:23:15 PM Christopher Heiny wrote:
>> >On 01/07/2014 12:33 PM, Christopher Heiny wrote:
>>> > >Eliminates copy-paste code that handled scans of the Page Descriptor
>>> > >Table,
>>> > >replacing it with a single PDT scan routine that invokes a callback
>>> > >function. The scan routine is not static so that it can be used by the
>>> > >firmware update code (under development, not yet submitted).
>>> > >
>>> > >Updated the copyright dates while we were at it.
>> >
>> >Hi Dmitry,
>> >
>> >Could you apply this or provide some feedback on it? We've got a
>> >pending patch that depends on it, and that pending work will bring the
>> >driver back to a working (if not necessarily beautiful) state. I don't
>> >want to submit it if this change isn't satisfactory, though.
>
> Speaking of the devil. I was just thinking about it and I wanted to ask
> you to send me an example of how it is used as I can;t make my mind about
> it.
>
> In general it is OK to submit a few patches at a time even if they are
> depend on each other - it gives me better context (as long as there
> aren't 80 of those so that I down in them ;) ).
>
> Thanks.
No problem!
Currently the rmi_driver.c iterates over the PDT twice during probe():
1) find F01 and reset the ASIC;
2) create and initialize the function devices & count the number of
interrupt sources.
followed by
3) a loop over the function devices allocating each one's interrupt
related bitmasks.
Unfortunately, depending on how the function drivers are loaded, a
function driver might access the device's and the function device's
bitmasks before step (3) is complete. This causes all kinds of
unintended consequences, none of which are amusing.
We considered fixing this by splitting the function device creation and
initialization, like so:
1) first to find F01 and reset the ASIC;
2) count the number of interrupt sources and create the function devices
followed by
3) a loop over the function devices to allocate their interrupt related
bitmasks and initialize the function devices
However, this led to function device setup being in two different
places, which obscured the code and could lead to bugs if someone added
new code in the wrong place (initialization instead of creation, or vice
versa).
The PDT scan loops have a lot of redundant boilerplate and were quite
large compared to the core code that was being iterated, obscuring just
what the loop was supposed to be doing. We found it clearer to
implement a single PDT scan routine, and put the logic for the different
step in callbacks. Plus it eliminated the maintenance headaches of
three (or more, see reflash, below) copy-paste PDT scan loops.
The pending patch (I'll send that right after this note) changes
rmi_driver.c the order to scan the PDT 3 times:
1) first to find F01 and reset the ASIC;
2) count the number of interrupt sources
3) create and initialize the function devices
Similar logic applies in the reflash code, which has rescan the PDT
after forcing the touch sensor firmware into bootloader mode (the
bootloader might have a different register map, unfortunately). Once we
had the scan+callback routine in rmi_driver.c, it only made sense for
the reflash library to use that as well.
Thanks,
Chris
^ permalink raw reply
* [PATCH] input: synaptics-rmi4 - Count IRQs before creating functions; save F01 container.
From: Christopher Heiny @ 2014-01-10 21:53 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires
Because creating a function can trigger events that result in the IRQ related
storage being accessed, we need to count the IRQs and allocate their storage
before the functions are created, rather than counting them as the functions
are created and allocating them afterwards. Since we know the number of IRQs
already, we can allocate the mask at function creation time, rather than in
a post-creation loop. Also, the F01 function_container is needed elsewhere,
so we need to save it here.
In order to keep the IRQ count logic sane in bootloader mode, we move the
check for bootloader mode from F01 initialization to the IRQ counting routine.
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/rmi4/rmi_driver.c | 129 +++++++++++++++++++++++++++++-----------
drivers/input/rmi4/rmi_f01.c | 11 +---
2 files changed, 96 insertions(+), 44 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index f5970f4..f2acd3a 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2011-2013 Synaptics Incorporated
+ * Copyright (c) 2011-2014 Synaptics Incorporated
* Copyright (c) 2011 Unixphere
*
* This driver provides the core support for a single RMI4-based device.
@@ -553,10 +553,19 @@ static int create_function(struct rmi_device *rmi_dev,
rmi_driver_copy_pdt_to_fd(pdt, &fn->fd, page_start);
+ error = rmi_driver_irq_get_mask(rmi_dev, fn);
+ if (error < 0) {
+ dev_err(dev, "%s: Failed to create irq_mask for F%02X.\n",
+ __func__, pdt->function_number);
+ return error;
+ }
+
error = rmi_register_function(fn);
if (error)
goto err_free_mem;
+ if (pdt->function_number == 0x01)
+ data->f01_container = fn;
list_add_tail(&fn->node, &data->function_list);
return 0;
@@ -566,10 +575,33 @@ err_free_mem:
return error;
}
-
#define RMI_SCAN_CONTINUE 0
#define RMI_SCAN_DONE 1
+/* Indicates that flash programming is enabled (bootloader mode). */
+#define RMI_F01_STATUS_BOOTLOADER(status) (!!((status) & 0x40))
+
+/*
+ * Given the PDT entry for F01, read the device status register to determine
+ * if we're stuck in bootloader mode or not.
+ *
+ */
+static int check_bootloader_mode(struct rmi_device *rmi_dev, struct pdt_entry *pdt,
+ u16 page_start)
+{
+ u8 device_status;
+ int retval = 0;
+
+ retval = rmi_read(rmi_dev, pdt->data_base_addr + page_start,
+ &device_status);
+ if (retval < 0) {
+ dev_err(&rmi_dev->dev, "Failed to read device status.\n");
+ return retval;
+ }
+
+ return RMI_F01_STATUS_BOOTLOADER(device_status);
+}
+
static int rmi_initial_reset(struct rmi_device *rmi_dev,
void *clbk_ctx, struct pdt_entry *pdt_entry, int page)
{
@@ -596,6 +628,23 @@ static int rmi_initial_reset(struct rmi_device *rmi_dev,
return (!page) ? RMI_SCAN_CONTINUE : -ENODEV;
}
+static int rmi_count_irqs(struct rmi_device *rmi_dev,
+ void * clbk_ctx, struct pdt_entry *pdt_entry, int page)
+{
+ struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
+
+ data->irq_count += pdt_entry->interrupt_source_count;
+ if (pdt_entry->function_number == 0x01) {
+ data->f01_bootloader_mode = check_bootloader_mode(rmi_dev,
+ pdt_entry, page);
+ if (data->f01_bootloader_mode)
+ dev_warn(&rmi_dev->dev,
+ "WARNING: RMI4 device is in bootloader mode!\n");
+ }
+
+ return RMI_SCAN_CONTINUE;
+}
+
static int rmi_create_functions_clbk(struct rmi_device *rmi_dev,
void *clbk_ctx, struct pdt_entry *entry, int page)
{
@@ -608,7 +657,6 @@ static int rmi_create_functions_clbk(struct rmi_device *rmi_dev,
static int rmi_create_functions(struct rmi_device *rmi_dev)
{
struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
- struct device *dev = &rmi_dev->dev;
int irq_count = 0;
int retval;
@@ -763,7 +811,6 @@ static int rmi_driver_probe(struct device *dev)
{
struct rmi_driver *rmi_driver;
struct rmi_driver_data *data = NULL;
- struct rmi_function *fn;
struct rmi_device_platform_data *pdata;
int retval = 0;
struct rmi_device *rmi_dev;
@@ -818,28 +865,6 @@ static int rmi_driver_probe(struct device *dev)
if (retval)
dev_warn(dev, "RMI initial reset failed! Continuing in spite of this.\n");
- retval = rmi_create_functions(rmi_dev);
- if (retval) {
- dev_err(dev, "PDT scan for %s failed with code %d.\n",
- pdata->sensor_name, retval);
- goto err_free_data;
- }
-
- if (!data->f01_container) {
- dev_err(dev, "missing F01 container!\n");
- retval = -EINVAL;
- goto err_free_data;
- }
-
- list_for_each_entry(fn, &data->function_list, node) {
- retval = rmi_driver_irq_get_mask(rmi_dev, fn);
- if (retval < 0) {
- dev_err(dev, "%s: Failed to create irq_mask.\n",
- __func__);
- goto err_free_data;
- }
- }
-
retval = rmi_read(rmi_dev, PDT_PROPERTIES_LOCATION, &data->pdt_props);
if (retval < 0) {
/*
@@ -850,6 +875,21 @@ static int rmi_driver_probe(struct device *dev)
PDT_PROPERTIES_LOCATION);
}
+ /*
+ * We need to count the IRQs and allocate their storage before scanning
+ * the PDT and creating the function entries, because adding a new
+ * function can trigger events that result in the IRQ related storage
+ * being accessed.
+ */
+ dev_dbg(dev, "Counting IRQs.\n");
+ retval = rmi_scan_pdt(rmi_dev, NULL, rmi_count_irqs);
+ if (retval) {
+ dev_err(dev, "IRQ counting for %s failed with code %d.\n",
+ pdata->sensor_name, retval);
+ goto err_free_data;
+ }
+ data->num_of_irq_regs = (data->irq_count + 7) / 8;
+
mutex_init(&data->irq_mutex);
data->irq_status = devm_kzalloc(dev,
BITS_TO_LONGS(data->irq_count)*sizeof(unsigned long),
@@ -869,6 +909,28 @@ static int rmi_driver_probe(struct device *dev)
goto err_free_data;
}
+ data->irq_mask_store = devm_kzalloc(dev,
+ BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
+ GFP_KERNEL);
+ if (!data->irq_mask_store) {
+ dev_err(dev, "Failed to allocate irq_mask_store.\n");
+ retval = -ENOMEM;
+ goto err_free_data;
+ }
+
+ retval = rmi_create_functions(rmi_dev);
+ if (retval) {
+ dev_err(dev, "Function creation failed with code %d.\n",
+ retval);
+ goto err_free_data;
+ }
+
+ if (!data->f01_container) {
+ dev_err(dev, "missing F01 container!\n");
+ retval = -EINVAL;
+ goto err_free_data;
+ }
+
retval = rmi_read_block(rmi_dev,
data->f01_container->fd.control_base_addr+1,
data->current_irq_mask, data->num_of_irq_regs);
@@ -878,14 +940,6 @@ static int rmi_driver_probe(struct device *dev)
goto err_free_data;
}
- data->irq_mask_store = devm_kzalloc(dev,
- BITS_TO_LONGS(data->irq_count)*sizeof(unsigned long),
- GFP_KERNEL);
- if (!data->irq_mask_store) {
- dev_err(dev, "Failed to allocate mask store.\n");
- retval = -ENOMEM;
- goto err_free_data;
- }
if (IS_ENABLED(CONFIG_PM)) {
data->pm_data = pdata->pm_data;
data->pre_suspend = pdata->pre_suspend;
@@ -951,6 +1005,13 @@ static int rmi_driver_probe(struct device *dev)
return 0;
err_free_data:
+ rmi_free_function_list(rmi_dev);
+ if (gpio_is_valid(pdata->attn_gpio))
+ gpio_free(pdata->attn_gpio);
+ devm_kfree(&rmi_dev->dev, data->irq_status);
+ devm_kfree(&rmi_dev->dev, data->current_irq_mask);
+ devm_kfree(&rmi_dev->dev, data->irq_mask_store);
+ devm_kfree(&rmi_dev->dev, data);
return retval;
}
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 628b082..c7f2360 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2011-2012 Synaptics Incorporated
+ * Copyright (c) 2011-2014 Synaptics Incorporated
* Copyright (c) 2011 Unixphere
*
* This program is free software; you can redistribute it and/or modify it
@@ -59,8 +59,6 @@ struct f01_basic_properties {
/* Most recent device status event */
#define RMI_F01_STATUS_CODE(status) ((status) & 0x0f)
-/* Indicates that flash programming is enabled (bootloader mode). */
-#define RMI_F01_STATUS_BOOTLOADER(status) (!!((status) & 0x40))
/* The device has lost its configuration for some reason. */
#define RMI_F01_STATUS_UNCONFIGURED(status) (!!((status) & 0x80))
@@ -358,13 +356,6 @@ static int rmi_f01_initialize(struct rmi_function *fn)
goto error_exit;
}
- driver_data->f01_bootloader_mode =
- RMI_F01_STATUS_BOOTLOADER(data->device_status);
- if (driver_data->f01_bootloader_mode)
- dev_warn(&rmi_dev->dev,
- "WARNING: RMI4 device is in bootloader mode!\n");
-
-
if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
dev_err(&fn->dev,
"Device was reset during configuration process, status: %#02x!\n",
^ permalink raw reply related
* Re: [PATCH 1/4] Input: synaptics-rmi4 - split of transport ops into a separate structure
From: Christopher Heiny @ 2014-01-10 23:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Linux Input, Linux Kernel
In-Reply-To: <1389339867-8399-1-git-send-email-dmitry.torokhov@gmail.com>
On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> Split off transport operations from rmi_transport_dev into a separate
> structure that will be shared between all devices using the same transport
> and use const pointer to access it.
>
> Change signature on transport methods so that length is using the proper
> tyep - size_t.
>
> Also rename rmi_transport_info to rmi_transport_stats and move protocol
> name (which is the only immutable piece of data there) into the transport
> device itself.
Acked-by: Christopher Heiny <cheiny@synaptics.com>
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/rmi4/rmi_bus.h | 64 ++++++++++++++++++++++++-----------------
> drivers/input/rmi4/rmi_driver.c | 8 +++---
> drivers/input/rmi4/rmi_i2c.c | 49 ++++++++++++++++---------------
> 3 files changed, 67 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
> index 3e8b57a..ccf26dc 100644
> --- a/drivers/input/rmi4/rmi_bus.h
> +++ b/drivers/input/rmi4/rmi_bus.h
> @@ -135,26 +135,25 @@ struct rmi_driver {
> #define to_rmi_driver(d) \
> container_of(d, struct rmi_driver, driver);
>
> -/** struct rmi_transport_info - diagnostic information about the RMI transport
> +/**
> + * struct rmi_transport_stats - diagnostic information about the RMI transport
> * device, used in the xport_info debugfs file.
> *
> * @proto String indicating the protocol being used.
> * @tx_count Number of transmit operations.
> - * @tx_bytes Number of bytes transmitted.
> * @tx_errs Number of errors encountered during transmit operations.
> + * @tx_bytes Number of bytes transmitted.
> * @rx_count Number of receive operations.
> - * @rx_bytes Number of bytes received.
> * @rx_errs Number of errors encountered during receive operations.
> - * @att_count Number of times ATTN assertions have been handled.
> + * @rx_bytes Number of bytes received.
> */
> -struct rmi_transport_info {
> - const char *proto;
> - long tx_count;
> - long tx_bytes;
> - long tx_errs;
> - long rx_count;
> - long rx_bytes;
> - long rx_errs;
> +struct rmi_transport_stats {
> + unsigned long tx_count;
> + unsigned long tx_errs;
> + size_t tx_bytes;
> + unsigned long rx_count;
> + unsigned long rx_errs;
> + size_t rx_bytes;
> };
>
> /**
> @@ -162,13 +161,14 @@ struct rmi_transport_info {
> *
> * @dev: Pointer to the communication device, e.g. i2c or spi
> * @rmi_dev: Pointer to the RMI device
> - * @write_block: Writing a block of data to the specified address
> - * @read_block: Read a block of data from the specified address.
> * @irq_thread: if not NULL, the sensor driver will use this instead of the
> * default irq_thread implementation.
> * @hard_irq: if not NULL, the sensor driver will use this for the hard IRQ
> * handling
> * @data: Private data pointer
> + * @proto_name: name of the transport protocol (SPI, i2c, etc)
> + * @ops: pointer to transport operations implementation
> + * @stats: transport statistics
> *
> * The RMI transport device implements the glue between different communication
> * buses such as I2C and SPI.
> @@ -178,20 +178,30 @@ struct rmi_transport_dev {
> struct device *dev;
> struct rmi_device *rmi_dev;
>
> - int (*write_block)(struct rmi_transport_dev *xport, u16 addr,
> - const void *buf, const int len);
> - int (*read_block)(struct rmi_transport_dev *xport, u16 addr,
> - void *buf, const int len);
> -
> - int (*enable_device) (struct rmi_transport_dev *xport);
> - void (*disable_device) (struct rmi_transport_dev *xport);
> -
> irqreturn_t (*irq_thread)(int irq, void *p);
> irqreturn_t (*hard_irq)(int irq, void *p);
>
> void *data;
>
> - struct rmi_transport_info info;
> + const char *proto_name;
> + const struct rmi_transport_ops *ops;
> + struct rmi_transport_stats stats;
> +};
> +
> +/**
> + * struct rmi_transport_ops - defines transport protocol operations.
> + *
> + * @write_block: Writing a block of data to the specified address
> + * @read_block: Read a block of data from the specified address.
> + */
> +struct rmi_transport_ops {
> + int (*write_block)(struct rmi_transport_dev *xport, u16 addr,
> + const void *buf, size_t len);
> + int (*read_block)(struct rmi_transport_dev *xport, u16 addr,
> + void *buf, size_t len);
> +
> + int (*enable_device) (struct rmi_transport_dev *xport);
> + void (*disable_device) (struct rmi_transport_dev *xport);
> };
>
> /**
> @@ -232,7 +242,7 @@ bool rmi_is_physical_device(struct device *dev);
> */
> static inline int rmi_read(struct rmi_device *d, u16 addr, void *buf)
> {
> - return d->xport->read_block(d->xport, addr, buf, 1);
> + return d->xport->ops->read_block(d->xport, addr, buf, 1);
> }
>
> /**
> @@ -248,7 +258,7 @@ static inline int rmi_read(struct rmi_device *d, u16 addr, void *buf)
> static inline int rmi_read_block(struct rmi_device *d, u16 addr, void *buf,
> const int len)
> {
> - return d->xport->read_block(d->xport, addr, buf, len);
> + return d->xport->ops->read_block(d->xport, addr, buf, len);
> }
>
> /**
> @@ -262,7 +272,7 @@ static inline int rmi_read_block(struct rmi_device *d, u16 addr, void *buf,
> */
> static inline int rmi_write(struct rmi_device *d, u16 addr, const u8 data)
> {
> - return d->xport->write_block(d->xport, addr, &data, 1);
> + return d->xport->ops->write_block(d->xport, addr, &data, 1);
> }
>
> /**
> @@ -278,7 +288,7 @@ static inline int rmi_write(struct rmi_device *d, u16 addr, const u8 data)
> static inline int rmi_write_block(struct rmi_device *d, u16 addr,
> const void *buf, const int len)
> {
> - return d->xport->write_block(d->xport, addr, buf, len);
> + return d->xport->ops->write_block(d->xport, addr, buf, len);
> }
>
> int rmi_register_transport_device(struct rmi_transport_dev *xport);
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index eb790ff..3483e5b 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -126,8 +126,8 @@ static void disable_sensor(struct rmi_device *rmi_dev)
> if (!data->irq)
> disable_polling(rmi_dev);
>
> - if (rmi_dev->xport->disable_device)
> - rmi_dev->xport->disable_device(rmi_dev->xport);
> + if (rmi_dev->xport->ops->disable_device)
> + rmi_dev->xport->ops->disable_device(rmi_dev->xport);
>
> if (data->irq) {
> disable_irq(data->irq);
> @@ -146,8 +146,8 @@ static int enable_sensor(struct rmi_device *rmi_dev)
> if (data->enabled)
> return 0;
>
> - if (rmi_dev->xport->enable_device) {
> - retval = rmi_dev->xport->enable_device(rmi_dev->xport);
> + if (rmi_dev->xport->ops->enable_device) {
> + retval = rmi_dev->xport->ops->enable_device(rmi_dev->xport);
> if (retval)
> return retval;
> }
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index 12aea8c..40badf3 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -61,11 +61,11 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
>
> dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
> txbuf[0], txbuf[1]);
> - xport->info.tx_count++;
> - xport->info.tx_bytes += sizeof(txbuf);
> + xport->stats.tx_count++;
> + xport->stats.tx_bytes += sizeof(txbuf);
> retval = i2c_master_send(client, txbuf, sizeof(txbuf));
> if (retval != sizeof(txbuf)) {
> - xport->info.tx_errs++;
> + xport->stats.tx_errs++;
> dev_err(&client->dev,
> "%s: set page failed: %d.", __func__, retval);
> return (retval < 0) ? retval : -EIO;
> @@ -75,12 +75,12 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
> }
>
> static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
> - const void *buf, const int len)
> + const void *buf, size_t len)
> {
> struct i2c_client *client = to_i2c_client(xport->dev);
> struct rmi_i2c_data *data = xport->data;
> + size_t tx_size = len + 1;
> int retval;
> - int tx_size = len + 1;
>
> mutex_lock(&data->page_mutex);
>
> @@ -106,13 +106,13 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
> }
>
> dev_dbg(&client->dev,
> - "writes %d bytes at %#06x: %*ph\n", len, addr, len, buf);
> + "writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf);
>
> - xport->info.tx_count++;
> - xport->info.tx_bytes += tx_size;
> + xport->stats.tx_count++;
> + xport->stats.tx_bytes += tx_size;
> retval = i2c_master_send(client, data->tx_buf, tx_size);
> if (retval < 0)
> - xport->info.tx_errs++;
> + xport->stats.tx_errs++;
> else
> retval--; /* don't count the address byte */
>
> @@ -121,9 +121,8 @@ exit:
> return retval;
> }
>
> -
> static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
> - void *buf, const int len)
> + void *buf, size_t len)
> {
> struct i2c_client *client = to_i2c_client(xport->dev);
> struct rmi_i2c_data *data = xport->data;
> @@ -140,31 +139,36 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
>
> dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
>
> - xport->info.tx_count++;
> - xport->info.tx_bytes += sizeof(txbuf);
> + xport->stats.tx_count++;
> + xport->stats.tx_bytes += sizeof(txbuf);
> retval = i2c_master_send(client, txbuf, sizeof(txbuf));
> if (retval != sizeof(txbuf)) {
> - xport->info.tx_errs++;
> + xport->stats.tx_errs++;
> retval = (retval < 0) ? retval : -EIO;
> goto exit;
> }
>
> - retval = i2c_master_recv(client, buf, len);
> + xport->stats.rx_count++;
> + xport->stats.rx_bytes += len;
>
> - xport->info.rx_count++;
> - xport->info.rx_bytes += len;
> + retval = i2c_master_recv(client, buf, len);
> if (retval < 0)
> - xport->info.rx_errs++;
> + xport->stats.rx_errs++;
> else
> dev_dbg(&client->dev,
> - "read %d bytes at %#06x: %*ph\n",
> - len, addr, len, buf);
> + "read %zd bytes at %#06x: %*ph\n",
> + len, addr, (int)len, buf);
>
> exit:
> mutex_unlock(&data->page_mutex);
> return retval;
> }
>
> +static const struct rmi_transport_ops rmi_i2c_ops = {
> + .write_block = rmi_i2c_write_block,
> + .read_block = rmi_i2c_read_block,
> +};
> +
> static int rmi_i2c_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -214,9 +218,8 @@ static int rmi_i2c_probe(struct i2c_client *client,
> xport->data = data;
> xport->dev = &client->dev;
>
> - xport->write_block = rmi_i2c_write_block;
> - xport->read_block = rmi_i2c_read_block;
> - xport->info.proto = "i2c";
> + xport->proto_name = "i2c";
> + xport->ops = &rmi_i2c_ops;
>
> mutex_init(&data->page_mutex);
>
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH 2/4] Input: synaptics-rmi4 - rework transport device allocation
From: Christopher Heiny @ 2014-01-10 23:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Linux Input, Linux Kernel
In-Reply-To: <1389339867-8399-2-git-send-email-dmitry.torokhov@gmail.com>
On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> Instead of allocating common and private part of transport device
> separately make private wrap common part and get rid of private data
> pointer in the transport device.
>
> Also rename rmi_i2c_data -> rmi_i2c_xport and data -> rmi_i2c.
Acked-by: Christopher Heiny <cheiny@synaptics.com>
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/rmi4/rmi_bus.h | 3 --
> drivers/input/rmi4/rmi_i2c.c | 112 +++++++++++++++++++++----------------------
> 2 files changed, 56 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
> index ccf26dc..decb479 100644
> --- a/drivers/input/rmi4/rmi_bus.h
> +++ b/drivers/input/rmi4/rmi_bus.h
> @@ -165,7 +165,6 @@ struct rmi_transport_stats {
> * default irq_thread implementation.
> * @hard_irq: if not NULL, the sensor driver will use this for the hard IRQ
> * handling
> - * @data: Private data pointer
> * @proto_name: name of the transport protocol (SPI, i2c, etc)
> * @ops: pointer to transport operations implementation
> * @stats: transport statistics
> @@ -181,8 +180,6 @@ struct rmi_transport_dev {
> irqreturn_t (*irq_thread)(int irq, void *p);
> irqreturn_t (*hard_irq)(int irq, void *p);
>
> - void *data;
> -
> const char *proto_name;
> const struct rmi_transport_ops *ops;
> struct rmi_transport_stats stats;
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index 40badf3..cdc8527 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -17,22 +17,25 @@
> #define BUFFER_SIZE_INCREMENT 32
>
> /**
> - * struct rmi_i2c_data - stores information for i2c communication
> + * struct rmi_i2c_xport - stores information for i2c communication
> + *
> + * @xport: The transport interface structure
> *
> * @page_mutex: Locks current page to avoid changing pages in unexpected ways.
> * @page: Keeps track of the current virtual page
> - * @xport: Pointer to the transport interface
> *
> * @tx_buf: Buffer used for transmitting data to the sensor over i2c.
> * @tx_buf_size: Size of the buffer
> */
> -struct rmi_i2c_data {
> +struct rmi_i2c_xport {
> + struct rmi_transport_dev xport;
> + struct i2c_client *client;
> +
> struct mutex page_mutex;
> int page;
> - struct rmi_transport_dev *xport;
>
> u8 *tx_buf;
> - int tx_buf_size;
> + size_t tx_buf_size;
> };
>
> #define RMI_PAGE_SELECT_REGISTER 0xff
> @@ -52,10 +55,10 @@ struct rmi_i2c_data {
> *
> * Returns zero on success, non-zero on failure.
> */
> -static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
> +static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page)
> {
> - struct i2c_client *client = to_i2c_client(xport->dev);
> - struct rmi_i2c_data *data = xport->data;
> + struct rmi_transport_dev *xport = &rmi_i2c->xport;
> + struct i2c_client *client = rmi_i2c->client;
> u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page};
> int retval;
>
> @@ -70,37 +73,40 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
> "%s: set page failed: %d.", __func__, retval);
> return (retval < 0) ? retval : -EIO;
> }
> - data->page = page;
> + rmi_i2c->page = page;
> return 0;
> }
>
> static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
> const void *buf, size_t len)
> {
> - struct i2c_client *client = to_i2c_client(xport->dev);
> - struct rmi_i2c_data *data = xport->data;
> + struct rmi_i2c_xport *rmi_i2c =
> + container_of(xport, struct rmi_i2c_xport, xport);
> + struct i2c_client *client = rmi_i2c->client;
> size_t tx_size = len + 1;
> int retval;
>
> - mutex_lock(&data->page_mutex);
> -
> - if (!data->tx_buf || data->tx_buf_size < tx_size) {
> - if (data->tx_buf)
> - devm_kfree(&client->dev, data->tx_buf);
> - data->tx_buf_size = tx_size + BUFFER_SIZE_INCREMENT;
> - data->tx_buf = devm_kzalloc(&client->dev, data->tx_buf_size,
> - GFP_KERNEL);
> - if (!data->tx_buf) {
> - data->tx_buf_size = 0;
> + mutex_lock(&rmi_i2c->page_mutex);
> +
> + if (!rmi_i2c->tx_buf || rmi_i2c->tx_buf_size < tx_size) {
> + if (rmi_i2c->tx_buf)
> + devm_kfree(&client->dev, rmi_i2c->tx_buf);
> + rmi_i2c->tx_buf_size = tx_size + BUFFER_SIZE_INCREMENT;
> + rmi_i2c->tx_buf = devm_kzalloc(&client->dev,
> + rmi_i2c->tx_buf_size,
> + GFP_KERNEL);
> + if (!rmi_i2c->tx_buf) {
> + rmi_i2c->tx_buf_size = 0;
> retval = -ENOMEM;
> goto exit;
> }
> }
> - data->tx_buf[0] = addr & 0xff;
> - memcpy(data->tx_buf + 1, buf, len);
>
> - if (RMI_I2C_PAGE(addr) != data->page) {
> - retval = rmi_set_page(xport, RMI_I2C_PAGE(addr));
> + rmi_i2c->tx_buf[0] = addr & 0xff;
> + memcpy(rmi_i2c->tx_buf + 1, buf, len);
> +
> + if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
> + retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> if (retval < 0)
> goto exit;
> }
> @@ -110,29 +116,30 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
>
> xport->stats.tx_count++;
> xport->stats.tx_bytes += tx_size;
> - retval = i2c_master_send(client, data->tx_buf, tx_size);
> + retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
> if (retval < 0)
> xport->stats.tx_errs++;
> else
> retval--; /* don't count the address byte */
>
> exit:
> - mutex_unlock(&data->page_mutex);
> + mutex_unlock(&rmi_i2c->page_mutex);
> return retval;
> }
>
> static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
> void *buf, size_t len)
> {
> - struct i2c_client *client = to_i2c_client(xport->dev);
> - struct rmi_i2c_data *data = xport->data;
> + struct rmi_i2c_xport *rmi_i2c =
> + container_of(xport, struct rmi_i2c_xport, xport);
> + struct i2c_client *client = rmi_i2c->client;
> u8 txbuf[1] = {addr & 0xff};
> int retval;
>
> - mutex_lock(&data->page_mutex);
> + mutex_lock(&rmi_i2c->page_mutex);
>
> - if (RMI_I2C_PAGE(addr) != data->page) {
> - retval = rmi_set_page(xport, RMI_I2C_PAGE(addr));
> + if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
> + retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> if (retval < 0)
> goto exit;
> }
> @@ -160,7 +167,7 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
> len, addr, (int)len, buf);
>
> exit:
> - mutex_unlock(&data->page_mutex);
> + mutex_unlock(&rmi_i2c->page_mutex);
> return retval;
> }
>
> @@ -174,14 +181,14 @@ static int rmi_i2c_probe(struct i2c_client *client,
> {
> const struct rmi_device_platform_data *pdata =
> dev_get_platdata(&client->dev);
> - struct rmi_transport_dev *xport;
> - struct rmi_i2c_data *data;
> + struct rmi_i2c_xport *rmi_i2c;
> int retval;
>
> if (!pdata) {
> dev_err(&client->dev, "no platform data\n");
> return -EINVAL;
> }
> +
> dev_dbg(&client->dev, "Probing %s at %#02x (GPIO %d).\n",
> pdata->sensor_name ? pdata->sensor_name : "-no name-",
> client->addr, pdata->attn_gpio);
> @@ -202,44 +209,36 @@ static int rmi_i2c_probe(struct i2c_client *client,
> }
> }
>
> - xport = devm_kzalloc(&client->dev, sizeof(struct rmi_transport_dev),
> + rmi_i2c = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_xport),
> GFP_KERNEL);
> -
> - if (!xport)
> + if (!rmi_i2c)
> return -ENOMEM;
>
> - data = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_data),
> - GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> -
> - data->xport = xport;
> -
> - xport->data = data;
> - xport->dev = &client->dev;
> + rmi_i2c->client = client;
> + mutex_init(&rmi_i2c->page_mutex);
>
> - xport->proto_name = "i2c";
> - xport->ops = &rmi_i2c_ops;
> -
> - mutex_init(&data->page_mutex);
> + rmi_i2c->xport.dev = &client->dev;
> + rmi_i2c->xport.proto_name = "i2c";
> + rmi_i2c->xport.ops = &rmi_i2c_ops;
>
> /*
> * Setting the page to zero will (a) make sure the PSR is in a
> * known state, and (b) make sure we can talk to the device.
> */
> - retval = rmi_set_page(xport, 0);
> + retval = rmi_set_page(rmi_i2c, 0);
> if (retval) {
> dev_err(&client->dev, "Failed to set page select to 0.\n");
> return retval;
> }
>
> - retval = rmi_register_transport_device(xport);
> + retval = rmi_register_transport_device(&rmi_i2c->xport);
> if (retval) {
> dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
> client->addr);
> goto err_gpio;
> }
> - i2c_set_clientdata(client, xport);
> +
> + i2c_set_clientdata(client, rmi_i2c);
>
> dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
> client->addr);
> @@ -248,16 +247,17 @@ static int rmi_i2c_probe(struct i2c_client *client,
> err_gpio:
> if (pdata->gpio_config)
> pdata->gpio_config(pdata->gpio_data, false);
> +
> return retval;
> }
>
> static int rmi_i2c_remove(struct i2c_client *client)
> {
> - struct rmi_transport_dev *xport = i2c_get_clientdata(client);
> const struct rmi_device_platform_data *pdata =
> dev_get_platdata(&client->dev);
> + struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
>
> - rmi_unregister_transport_device(xport);
> + rmi_unregister_transport_device(&rmi_i2c->xport);
>
> if (pdata->gpio_config)
> pdata->gpio_config(pdata->gpio_data, false);
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH 3/4] Input: synaptics-rmi4 - fix I2C functionality check
From: Christopher Heiny @ 2014-01-10 23:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Linux Input, Linux Kernel
In-Reply-To: <1389339867-8399-3-git-send-email-dmitry.torokhov@gmail.com>
On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> When adapter does not support required functionality (I2C_FUNC_I2C) we were
> returning 0 to the upper layers, making them believe that device bound
> successfully.
Acked-by: Christopher Heiny <cheiny@synaptics.com>
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/rmi4/rmi_i2c.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index cdc8527..c176218 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -193,11 +193,10 @@ static int rmi_i2c_probe(struct i2c_client *client,
> pdata->sensor_name ? pdata->sensor_name : "-no name-",
> client->addr, pdata->attn_gpio);
>
> - retval = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
> - if (!retval) {
> - dev_err(&client->dev, "i2c_check_functionality error %d.\n",
> - retval);
> - return retval;
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(&client->dev,
> + "adapter does not support required functionality.\n");
> + return -ENODEV;
> }
>
> if (pdata->gpio_config) {
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH 4/4] Input: synaptics-rmi4 - switch to using i2c_transfer()
From: Christopher Heiny @ 2014-01-10 23:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Linux Input, Linux Kernel
In-Reply-To: <1389339867-8399-4-git-send-email-dmitry.torokhov@gmail.com>
On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> Instead of using 2 separate transactions when reading from the device let's
> use i2c_transfer. Because we now have single point of failure I had to
> change how we collect statistics. I elected to drop control data from the
> stats and only track number of bytes read/written for the device data.
>
> Also, since we are not prepared to deal with short reads and writes change
> read_block_data and write_block_data to indicate error if we detect short
> transfers.
We tried this change once before a couple of years ago, but the
conversion was unsuccessful on some older platforms. I've tested it on
some more current platforms, though, and it works there. The old
platforms are running 2.6.xx series kernels, and don't look likely ever
to be updated, So....
Acked-by: Christopher Heiny <cheiny@synaptics.com>
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/rmi4/rmi_i2c.c | 71 ++++++++++++++++++++++++--------------------
> 1 file changed, 39 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index c176218..51f5bc8 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -57,22 +57,17 @@ struct rmi_i2c_xport {
> */
> static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page)
> {
> - struct rmi_transport_dev *xport = &rmi_i2c->xport;
> struct i2c_client *client = rmi_i2c->client;
> u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page};
> int retval;
>
> - dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
> - txbuf[0], txbuf[1]);
> - xport->stats.tx_count++;
> - xport->stats.tx_bytes += sizeof(txbuf);
> retval = i2c_master_send(client, txbuf, sizeof(txbuf));
> if (retval != sizeof(txbuf)) {
> - xport->stats.tx_errs++;
> dev_err(&client->dev,
> "%s: set page failed: %d.", __func__, retval);
> return (retval < 0) ? retval : -EIO;
> }
> +
> rmi_i2c->page = page;
> return 0;
> }
> @@ -107,22 +102,27 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
>
> if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
> retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> - if (retval < 0)
> + if (retval)
> goto exit;
> }
>
> + retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
> + if (retval == tx_size)
> + retval = 0;
> + else if (retval >= 0)
> + retval = -EIO;
> +
> +exit:
> dev_dbg(&client->dev,
> - "writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf);
> + "write %zd bytes at %#06x: %d (%*ph)\n",
> + len, addr, retval, (int)len, buf);
>
> xport->stats.tx_count++;
> - xport->stats.tx_bytes += tx_size;
> - retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
> - if (retval < 0)
> + if (retval)
> xport->stats.tx_errs++;
> else
> - retval--; /* don't count the address byte */
> + xport->stats.tx_bytes += len;
>
> -exit:
> mutex_unlock(&rmi_i2c->page_mutex);
> return retval;
> }
> @@ -133,40 +133,47 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
> struct rmi_i2c_xport *rmi_i2c =
> container_of(xport, struct rmi_i2c_xport, xport);
> struct i2c_client *client = rmi_i2c->client;
> - u8 txbuf[1] = {addr & 0xff};
> + u8 addr_offset = addr & 0xff;
> int retval;
> + struct i2c_msg msgs[] = {
> + {
> + .addr = client->addr,
> + .len = sizeof(addr_offset),
> + .buf = &addr_offset,
> + },
> + {
> + .addr = client->addr,
> + .flags = I2C_M_RD,
> + .len = len,
> + .buf = buf,
> + },
> + };
>
> mutex_lock(&rmi_i2c->page_mutex);
>
> if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
> retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> - if (retval < 0)
> + if (retval)
> goto exit;
> }
>
> - dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
> + retval = i2c_transfer(client->adapter, msgs, sizeof(msgs));
> + if (retval == sizeof(msgs))
> + retval = 0; /* success */
> + else if (retval >= 0)
> + retval = -EIO;
>
> - xport->stats.tx_count++;
> - xport->stats.tx_bytes += sizeof(txbuf);
> - retval = i2c_master_send(client, txbuf, sizeof(txbuf));
> - if (retval != sizeof(txbuf)) {
> - xport->stats.tx_errs++;
> - retval = (retval < 0) ? retval : -EIO;
> - goto exit;
> - }
> +exit:
> + dev_dbg(&client->dev,
> + "read %zd bytes at %#06x: %d (%*ph)\n",
> + len, addr, retval, (int)len, buf);
>
> xport->stats.rx_count++;
> - xport->stats.rx_bytes += len;
> -
> - retval = i2c_master_recv(client, buf, len);
> - if (retval < 0)
> + if (retval)
> xport->stats.rx_errs++;
> else
> - dev_dbg(&client->dev,
> - "read %zd bytes at %#06x: %*ph\n",
> - len, addr, (int)len, buf);
> + xport->stats.rx_bytes += len;
>
> -exit:
> mutex_unlock(&rmi_i2c->page_mutex);
> return retval;
> }
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* [PATCH v2 1/3] HID: sony: Add force-feedback support for the Dualshock 4
From: Frank Praznik @ 2014-01-11 20:12 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina
Adds the Dualshock 4 to the HID device list and enables force-feedback.
Adds a Dualshock 4 specific worker function since the Dualshock 4 needs a
different report than the Sixaxis.
The right motor in the Dualshock 4 is variable so the full rumble value
is now passed to the worker function and clamped there if necessary.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
Apply against jikos/hid.git/for-3.14/sony
drivers/hid/hid-core.c | 2 ++
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-sony.c | 39 ++++++++++++++++++++++++++++++++++++---
3 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 957d35b..70cc468 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1831,6 +1831,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER) },
{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER) },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER) },
{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGX_MOUSE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGP_MOUSE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, USB_DEVICE_ID_STEELSERIES_SRWS1) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 60336f06..ce24459 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -766,6 +766,7 @@
#define USB_DEVICE_ID_SONY_VAIO_VGP_MOUSE 0x0374
#define USB_DEVICE_ID_SONY_PS3_BDREMOTE 0x0306
#define USB_DEVICE_ID_SONY_PS3_CONTROLLER 0x0268
+#define USB_DEVICE_ID_SONY_PS4_CONTROLLER 0x05c4
#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
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index f57ab5e..8020d10 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)
@@ -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;
@@ -728,8 +752,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
}
else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
ret = sixaxis_set_operational_bt(hdev);
- else
+ else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
ret = 0;
+ INIT_WORK(&sc->state_worker, dualshock4_state_worker);
+ } else {
+ ret = 0;
+ }
if (ret < 0)
goto err_stop;
@@ -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
* [PATCH v2 2/3] HID: sony: Add LED controls for the Dualshock 4
From: Frank Praznik @ 2014-01-11 20:13 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina
Add LED lightbar controls for the Dualshock 4.
The Dualshock 4 light bar has 3 separate RGB LEDs that can range in
brightness from 0 to 255 so a full byte is now needed to store each LED's
state
Changed the module to support an arbitrary number of LEDs instead of being
hardcoded to 4.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
Apply against jikos/hid.git/for-3.14/sony
drivers/hid/hid-sony.c | 77 ++++++++++++++++++++++++++++++++------------------
1 file changed, 50 insertions(+), 27 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 8020d10..79e0d58 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,28 @@ 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);
+ int n;
- if (drv_data->quirks & BUZZ_CONTROLLER) {
+ 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 +500,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 +526,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 +545,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 +553,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;
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));
@@ -575,14 +583,22 @@ static int sony_leds_init(struct hid_device *hdev)
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;
+ max_brightness = 1;
+ }
+
/* 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 +610,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 +651,10 @@ static void sony_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 +679,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 v2 3/3] HID: sony: Rename worker function
From: Frank Praznik @ 2014-01-11 20:13 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina
Rename sony_state_worker to sixaxis_state_worker since the function is now
sixaxis specific.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
Apply against jikos/hid.git/for-3.14/sony
drivers/hid/hid-sony.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 79e0d58..1dfed23 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -632,7 +632,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[] = {
@@ -771,7 +771,7 @@ 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);
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH] ARM: Kirkwood: Add DT description of QNAP 419
From: Andrew Lunn @ 2014-01-11 21:04 UTC (permalink / raw)
To: Ian Campbell, devicetree, linux-input
Cc: Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Gregory Clement,
linux ARM, tbm
In-Reply-To: <1389174517.12612.87.camel@kazak.uk.xensource.com>
On Wed, Jan 08, 2014 at 09:48:37AM +0000, Ian Campbell wrote:
> On Mon, 2014-01-06 at 23:49 +0100, Andrew Lunn wrote:
Hi Ian
I added in the devicetree list and the input list and added my
thoughts below.
> The other issue I spotted is
> that /dev/input/by-path/platform-gpio-keys-event has
> become /dev/input/by-path/platform-gpio_keys.3-event. Is it considered
> valid for a by-path name to change? In particular the 3 here is
> apparently the node depth in the DTB, which doesn't make much logical
> sense as a "path" in this context I don't think (I expect it to be some
> sort of path through the hardware buses, perhaps my expectation is
> wrong?).
The ts41x-setup.c board file creates the gpio keys platform device
using the following structure:
static struct platform_device qnap_ts41x_button_device = {
.name = "gpio-keys",
.id = -1,
.num_resources = 0,
.dev = {
.platform_data = &qnap_ts41x_button_data,
}
};
The id of -1 causes platform_device_add() to set the device name to
plain "gpio-keys".
When using DT, the device name is created by the function
of_device_make_bus_id(). It has the following comment:
* This routine will first try using either the dcr-reg or the reg property
* value to derive a unique name. As a last resort it will use the node
* name followed by a unique number.
Since the gpio_keys node does not have a reg properties, it gets a
unique number appended to it. We end up with the device name
"gpio_keys.3"
So as it stands, it does not appear i can make the DT system use the
same device name as a board system.
But i'm also a little bit concerned by the "unique number" and this
ending up in /dev/input/by-path/platform-gpio_keys.3-event. Is this
path supposed to be stable? This unique number is not stable. An
unwitting change to the DT could cause its value to change. Do we need
to make it stable?
Andrew
^ permalink raw reply
* Re: [PATCH v2 1/3] HID: sony: Add force-feedback support for the Dualshock 4
From: simon @ 2014-01-12 0:25 UTC (permalink / raw)
To: Frank Praznik; +Cc: linux-input, Jiri Kosina
> Adds the Dualshock 4 to the HID device list and enables force-feedback.
Adds a Dualshock 4 specific worker function since the Dualshock 4 needs
a
> different report than the Sixaxis.
> The right motor in the Dualshock 4 is variable so the full rumble value
is now passed to the worker function and clamped there if necessary.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
I was able to build this and the LED patch against the 3.13rc7 kernel
having first applied this sequence of patches:
https://patchwork.kernel.org/patch/3203761/
I can confirm that FF/LEDs appears to work OK with USB connected
Dualshock4, Dualshock3(SixAxis) and 3rd part wired PS3 controller.
I wasn't able to get Dualshock4 working over BT, but I think that the
problem is with my system - I am yet to figure out getting the HIDP
connection to work properly.
I can use the depricated 'HIDD --connect xxx' to have LEDs are listed in
'/sys/class/leds', but they don't change when instructed. They stay solid
white.
Cheers,
Simon
Tested-by: Simon Wood <simon@mungewell.org>
^ permalink raw reply
* [PATCH 1/1] Drivers: input: serio:hyperv-keyoard: Handle 0xE1 prefix
From: K. Y. Srinivasan @ 2014-01-12 3:41 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, dmitry.torokhov, linux-input,
vojtech, olaf, apw, jasowang
Handle the 0xE1 prefix.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/input/serio/hyperv-keyboard.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
index 3a83c3c..6132619 100644
--- a/drivers/input/serio/hyperv-keyboard.c
+++ b/drivers/input/serio/hyperv-keyboard.c
@@ -160,7 +160,9 @@ static void hv_kbd_on_receive(struct hv_device *hv_dev,
if (info & IS_E0)
serio_interrupt(kbd_dev->hv_serio,
XTKBD_EMUL0, 0);
-
+ if (info & IS_E1)
+ serio_interrupt(kbd_dev->hv_serio,
+ XTKBD_EMUL1, 0);
scan_code = __le16_to_cpu(ks_msg->make_code);
if (info & IS_BREAK)
scan_code |= XTKBD_RELEASE;
--
1.7.4.1
^ permalink raw reply related
* [PATCH TRIVIAL] Input: logips2pp - Spelling s/reciver/receiver/
From: Geert Uytterhoeven @ 2014-01-12 13:01 UTC (permalink / raw)
To: Dmitry Torokhov, Jiri Kosina; +Cc: linux-input, Geert Uytterhoeven
From: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
---
drivers/input/mouse/logips2pp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/mouse/logips2pp.c b/drivers/input/mouse/logips2pp.c
index 84de2fc6acc1..136e222e2a16 100644
--- a/drivers/input/mouse/logips2pp.c
+++ b/drivers/input/mouse/logips2pp.c
@@ -220,7 +220,7 @@ static const struct ps2pp_info *get_model_info(unsigned char model)
{ 61, PS2PP_KIND_MX, /* MX700 */
PS2PP_WHEEL | PS2PP_SIDE_BTN | PS2PP_TASK_BTN |
PS2PP_EXTRA_BTN | PS2PP_NAV_BTN },
- { 66, PS2PP_KIND_MX, /* MX3100 reciver */
+ { 66, PS2PP_KIND_MX, /* MX3100 receiver */
PS2PP_WHEEL | PS2PP_SIDE_BTN | PS2PP_TASK_BTN |
PS2PP_EXTRA_BTN | PS2PP_NAV_BTN | PS2PP_HWHEEL },
{ 72, PS2PP_KIND_TRACKMAN, 0 }, /* T-CH11: TrackMan Marble */
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH TRIVIAL] Input: logips2pp - Spelling s/reciver/receiver/
From: Dmitry Torokhov @ 2014-01-12 19:09 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Jiri Kosina, linux-input, Geert Uytterhoeven
In-Reply-To: <1389531708-6095-1-git-send-email-geert@linux-m68k.org>
On Sun, Jan 12, 2014 at 02:01:48PM +0100, Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
Applied, thank you.
> ---
> drivers/input/mouse/logips2pp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/mouse/logips2pp.c b/drivers/input/mouse/logips2pp.c
> index 84de2fc6acc1..136e222e2a16 100644
> --- a/drivers/input/mouse/logips2pp.c
> +++ b/drivers/input/mouse/logips2pp.c
> @@ -220,7 +220,7 @@ static const struct ps2pp_info *get_model_info(unsigned char model)
> { 61, PS2PP_KIND_MX, /* MX700 */
> PS2PP_WHEEL | PS2PP_SIDE_BTN | PS2PP_TASK_BTN |
> PS2PP_EXTRA_BTN | PS2PP_NAV_BTN },
> - { 66, PS2PP_KIND_MX, /* MX3100 reciver */
> + { 66, PS2PP_KIND_MX, /* MX3100 receiver */
> PS2PP_WHEEL | PS2PP_SIDE_BTN | PS2PP_TASK_BTN |
> PS2PP_EXTRA_BTN | PS2PP_NAV_BTN | PS2PP_HWHEEL },
> { 72, PS2PP_KIND_TRACKMAN, 0 }, /* T-CH11: TrackMan Marble */
> --
> 1.7.9.5
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio:hyperv-keyoard: Handle 0xE1 prefix
From: Dmitry Torokhov @ 2014-01-12 19:14 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: olaf, gregkh, jasowang, linux-kernel, vojtech, linux-input, apw,
devel
In-Reply-To: <1389498110-4790-1-git-send-email-kys@microsoft.com>
On Sat, Jan 11, 2014 at 07:41:50PM -0800, K. Y. Srinivasan wrote:
> Handle the 0xE1 prefix.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Applied, thank you.
> ---
> drivers/input/serio/hyperv-keyboard.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
> index 3a83c3c..6132619 100644
> --- a/drivers/input/serio/hyperv-keyboard.c
> +++ b/drivers/input/serio/hyperv-keyboard.c
> @@ -160,7 +160,9 @@ static void hv_kbd_on_receive(struct hv_device *hv_dev,
> if (info & IS_E0)
> serio_interrupt(kbd_dev->hv_serio,
> XTKBD_EMUL0, 0);
> -
> + if (info & IS_E1)
> + serio_interrupt(kbd_dev->hv_serio,
> + XTKBD_EMUL1, 0);
> scan_code = __le16_to_cpu(ks_msg->make_code);
> if (info & IS_BREAK)
> scan_code |= XTKBD_RELEASE;
> --
> 1.7.4.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/4] Input: uinput: add full absinfo support
From: Dmitry Torokhov @ 2014-01-12 19:38 UTC (permalink / raw)
To: David Herrmann
Cc: linux-input, Jiri Kosina, Benjamin Tissoires, Peter Hutterer,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <1387295334-1744-2-git-send-email-dh.herrmann@gmail.com>
On Tue, Dec 17, 2013 at 04:48:51PM +0100, David Herrmann wrote:
> +
> +struct uinput_user_dev2 {
> + __u8 version;
It does not make sense to have version u8 since we going to have padding
(1 byte I believe) padding between name and id.
> + char name[UINPUT_MAX_NAME_SIZE];
> + struct input_id id;
> + __u32 ff_effects_max;
> + struct input_absinfo abs[ABS_CNT];
> +};
> +
> #endif /* _UAPI__UINPUT_H_ */
> --
> 1.8.5.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/4] Input: uinput: add full absinfo support
From: Dmitry Torokhov @ 2014-01-12 19:40 UTC (permalink / raw)
To: Peter Hutterer
Cc: David Herrmann, linux-input, Jiri Kosina, Benjamin Tissoires,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <20131218222732.GA6315@yabbi.redhat.com>
On Thu, Dec 19, 2013 at 08:27:32AM +1000, Peter Hutterer wrote:
> On Tue, Dec 17, 2013 at 04:48:51PM +0100, David Herrmann wrote:
> > +
> > + user_dev2->version = UINPUT_VERSION;
> > + memcpy(user_dev2->name, user_dev->name, UINPUT_MAX_NAME_SIZE);
> > + memcpy(&user_dev2->id, &user_dev->id, sizeof(struct input_id));
>
> you copy the id bits one-by-one into the input_dev but you memcpy it here.
> is this intentional?
That should simply be:
user_dev2->id = user_dev->id;
and in othe rplace as well I think.
>
> > + user_dev2->ff_effects_max = user_dev->ff_effects_max;
> > +
> > + for (i = 0; i < ABS_CNT; ++i) {
> > + user_dev2->abs[i].value = 0;
> > + user_dev2->abs[i].maximum = user_dev->absmax[i];
> > + user_dev2->abs[i].minimum = user_dev->absmin[i];
> > + user_dev2->abs[i].fuzz = user_dev->absfuzz[i];
> > + user_dev2->abs[i].flat = user_dev->absflat[i];
> > + user_dev2->abs[i].resolution = 0;
> > + }
> > +
> > + retval = uinput_setup_device(udev, user_dev2, ABS_CNT);
> >
> > - exit:
> > kfree(user_dev);
> > - return retval;
> > + kfree(user_dev2);
> > +
> > + return retval ? retval : count;
> > +}
> > +
> > +static int uinput_setup_device2(struct uinput_device *udev,
> > + const char __user *buffer, size_t count)
> > +{
> > + struct uinput_user_dev2 *user_dev2;
> > + int retval;
> > + size_t off, abscnt, max;
> > +
> > + /* The first revision of "uinput_user_dev2" is bigger than
> > + * "uinput_user_dev" and growing. Disallow any smaller payloads. */
> > + if (count <= sizeof(struct uinput_user_dev))
> > + return -EINVAL;
> > +
> > + /* rough check to avoid huge kernel space allocations */
> > + max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
> > + if (count > max)
> > + return -EINVAL;
> > +
> > + user_dev2 = memdup_user(buffer, count);
> > + if (IS_ERR(user_dev2))
> > + return PTR_ERR(user_dev2);
> > +
> > + if (user_dev2->version > UINPUT_VERSION) {
> > + retval = -EINVAL;
> > + } else {
> > + off = offsetof(struct uinput_user_dev2, abs);
> > + abscnt = (count - off) / sizeof(*user_dev2->abs);
> > + retval = uinput_setup_device(udev, user_dev2, abscnt);
> > + }
> > +
>
> I really wish uinput would be a bit easier to debug than just returning
> -EINVAL when it's not happy. having said that, the only errno that would
> remotely make sense is -ERANGE for count > max and even that is a bit meh.
Maybe we should add a few dev_dbg() and rely on having dynamic debug to
activate logging on demand?
Thanks.
--
Dmitry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox