* Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix
From: Christopher Heiny @ 2014-02-12 3:03 UTC (permalink / raw)
To: Courtney Cavin
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina
In-Reply-To: <20140212012606.GY1706@sonymobile.com>
On 02/11/2014 05:26 PM, Courtney Cavin wrote:
> On Wed, Feb 12, 2014 at 12:13:00AM +0100, Christopher Heiny wrote:
>> Correctly free driver related data when initialization fails.
>>
>> Trivial: Clarify a diagnostic message.
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Cc: Linux Walleij <linus.walleij@linaro.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_f01.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
>> index 381ad60..e4a6df9 100644
>> --- a/drivers/input/rmi4/rmi_f01.c
>> +++ b/drivers/input/rmi4/rmi_f01.c
>> @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>>
>> f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
>> if (!f01) {
>> - dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
>> + dev_err(&fn->dev, "Failed to allocate f01_data.\n");
>
> Just remove this printout, as it won't help any user in the case of OOM.
> Additionally, there's already plenty of (more useful) information
> printed out if kmalloc fails.
Good point. There's similar messages in a number of places, so we'll do
a single patch to clean them all up at once.
>
>> return -ENOMEM;
>> }
>>
>> @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>> GFP_KERNEL);
>> if (!f01->device_control.interrupt_enable) {
>> dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
>> + devm_kfree(&fn->dev, f01);
>
> Unnecessary, devres_release_all() will release this, from:
> - really_probe() -> rmi_function_probe() -> rmi_f01_probe()
> - driver_probe_device()
> - __driver_attach()
> - driver_attach()
> - bus_add_driver()
> - driver_register()
> - __rmi_register_function_handler()
As mentioned before, we've received a lot of conflicting advice on
devm_k*. Thanks very much for the clarification - it makes more sense now.
>> return -ENOMEM;
>> }
>> fn->data = f01;
>> @@ -381,7 +382,8 @@ static int rmi_f01_initialize(struct rmi_function *fn)
>> return 0;
>>
>> error_exit:
>> - kfree(data);
>> + devm_kfree(&fn->dev, data->device_control.interrupt_enable);
>> + devm_kfree(&fn->dev, data);
>
> Same as above for these two.
>
>> return error;
>> }
>>
>
> Generally devm_ release functions are unnecessary to call, as all
> resources will get released on driver detach, whether abnormal or not.
>
> -Courtney
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.
From: Courtney Cavin @ 2014-02-12 2:49 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina
In-Reply-To: <52FAD9D5.5070900@synaptics.com>
On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote:
> On 02/11/2014 05:59 PM, Courtney Cavin wrote:
> > On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:
> >> For rmi_sensor and rmi_function device_types, use put_device() and
> >> the assocated device_type.release() function to clean up related
> >> structures and storage in the correct and safe order.
> >>
> >> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >> Cc: Linux Walleij <linus.walleij@linaro.org>
> >> Cc: David Herrmann <dh.herrmann@gmail.com>
> >> Cc: Jiri Kosina <jkosina@suse.cz>
> >> Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
> >
> > I'm not a huge fan of you taking my patches, re-formatting them and
> > sending them as your own. More out of principle then actually caring
> > about ownership. You at least cc'd me on this one....
>
> Sorry - no slight was intended at all! I wasn't sure what the protocol
> was for picking up an idea from someone else's patch and building on
> that idea, so I just went with the CC. I definitely prefer to attribute
> sources correctly - if you could clarify what should be done (beyond the
> CC) to acknowledge the author of the original patch, I'd appreciate it.
Sure. In short, follow Documentation/SubmittingPatches , esp. section
12) Sign your work.
Generally the patch should read something like the following:
From: Original Author <original.author@example.org>
*BLURB*
Signed-off-by: Original Author <original.author@example.org>
[additional.author@example.org: changed x and y]
Signed-off-by: Additional Author <additional.author@example.org>
Assuming the original author actually signed-off the patch in the first
place, of course. The square bracket part is optional, but can be
helpful for reviewers.
I'm somewhat surprised that you are not aware of this procedure, as this
is how Dmitry has replied to some of your patches in the past.
-Courtney
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.
From: Christopher Heiny @ 2014-02-12 2:17 UTC (permalink / raw)
To: Courtney Cavin
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina
In-Reply-To: <20140212015929.GZ1706@sonymobile.com>
On 02/11/2014 05:59 PM, Courtney Cavin wrote:
> On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:
>> For rmi_sensor and rmi_function device_types, use put_device() and
>> the assocated device_type.release() function to clean up related
>> structures and storage in the correct and safe order.
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Cc: Linux Walleij <linus.walleij@linaro.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>> Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
>
> I'm not a huge fan of you taking my patches, re-formatting them and
> sending them as your own. More out of principle then actually caring
> about ownership. You at least cc'd me on this one....
Sorry - no slight was intended at all! I wasn't sure what the protocol
was for picking up an idea from someone else's patch and building on
that idea, so I just went with the CC. I definitely prefer to attribute
sources correctly - if you could clarify what should be done (beyond the
CC) to acknowledge the author of the original patch, I'd appreciate it.
>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_bus.c | 65 +++++++++++++++--------------------------
>> drivers/input/rmi4/rmi_driver.c | 11 ++-----
>> 2 files changed, 25 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
>> index 96a76e7..1b9ad80 100644
>> --- a/drivers/input/rmi4/rmi_bus.c
>> +++ b/drivers/input/rmi4/rmi_bus.c
> [...]
>> @@ -185,6 +153,23 @@ static void rmi_function_teardown_debugfs(struct rmi_function *fn)
>> }
>>
>> #endif
>> +static void rmi_release_function(struct device *dev)
>> +{
>> + struct rmi_function *fn = to_rmi_function(dev);
>> + rmi_function_teardown_debugfs(fn);
>> + kfree(fn->irq_mask);
>
> If you are going to do this, then you need to remove the other call to
> free this mask in rmi_free_function_list().
Okidoki.
>
>> + kfree(fn);
>> +}
>
> -Courtney
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.
From: Courtney Cavin @ 2014-02-12 1:59 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina
In-Reply-To: <1392160410-8293-1-git-send-email-cheiny@synaptics.com>
On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:
> For rmi_sensor and rmi_function device_types, use put_device() and
> the assocated device_type.release() function to clean up related
> structures and storage in the correct and safe order.
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
I'm not a huge fan of you taking my patches, re-formatting them and
sending them as your own. More out of principle then actually caring
about ownership. You at least cc'd me on this one....
>
> ---
>
> drivers/input/rmi4/rmi_bus.c | 65 +++++++++++++++--------------------------
> drivers/input/rmi4/rmi_driver.c | 11 ++-----
> 2 files changed, 25 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> index 96a76e7..1b9ad80 100644
> --- a/drivers/input/rmi4/rmi_bus.c
> +++ b/drivers/input/rmi4/rmi_bus.c
[...]
> @@ -185,6 +153,23 @@ static void rmi_function_teardown_debugfs(struct rmi_function *fn)
> }
>
> #endif
> +static void rmi_release_function(struct device *dev)
> +{
> + struct rmi_function *fn = to_rmi_function(dev);
> + rmi_function_teardown_debugfs(fn);
> + kfree(fn->irq_mask);
If you are going to do this, then you need to remove the other call to
free this mask in rmi_free_function_list().
> + kfree(fn);
> +}
-Courtney
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix
From: Courtney Cavin @ 2014-02-12 1:26 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina
In-Reply-To: <1392160380-8221-1-git-send-email-cheiny@synaptics.com>
On Wed, Feb 12, 2014 at 12:13:00AM +0100, Christopher Heiny wrote:
> Correctly free driver related data when initialization fails.
>
> Trivial: Clarify a diagnostic message.
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
>
> ---
>
> drivers/input/rmi4/rmi_f01.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> index 381ad60..e4a6df9 100644
> --- a/drivers/input/rmi4/rmi_f01.c
> +++ b/drivers/input/rmi4/rmi_f01.c
> @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>
> f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
> if (!f01) {
> - dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
> + dev_err(&fn->dev, "Failed to allocate f01_data.\n");
Just remove this printout, as it won't help any user in the case of OOM.
Additionally, there's already plenty of (more useful) information
printed out if kmalloc fails.
> return -ENOMEM;
> }
>
> @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
> GFP_KERNEL);
> if (!f01->device_control.interrupt_enable) {
> dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
> + devm_kfree(&fn->dev, f01);
Unnecessary, devres_release_all() will release this, from:
- really_probe() -> rmi_function_probe() -> rmi_f01_probe()
- driver_probe_device()
- __driver_attach()
- driver_attach()
- bus_add_driver()
- driver_register()
- __rmi_register_function_handler()
> return -ENOMEM;
> }
> fn->data = f01;
> @@ -381,7 +382,8 @@ static int rmi_f01_initialize(struct rmi_function *fn)
> return 0;
>
> error_exit:
> - kfree(data);
> + devm_kfree(&fn->dev, data->device_control.interrupt_enable);
> + devm_kfree(&fn->dev, data);
Same as above for these two.
> return error;
> }
>
Generally devm_ release functions are unnecessary to call, as all
resources will get released on driver detach, whether abnormal or not.
-Courtney
^ permalink raw reply
* [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.
From: Christopher Heiny @ 2014-02-11 23:13 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, David Herrmann, Jiri Kosina,
Courtney Cavin
For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Linux Walleij <linus.walleij@linaro.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
---
drivers/input/rmi4/rmi_bus.c | 65 +++++++++++++++--------------------------
drivers/input/rmi4/rmi_driver.c | 11 ++-----
2 files changed, 25 insertions(+), 51 deletions(-)
diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index 96a76e7..1b9ad80 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c
@@ -30,23 +30,6 @@ static struct dentry *rmi_debugfs_root;
* purpose. For example F11 is a 2D touch sensor while F01 is a generic
* function present in every RMI device.
*/
-
-static void rmi_release_device(struct device *dev)
-{
- struct rmi_device *rmi_dev = to_rmi_device(dev);
- kfree(rmi_dev);
-}
-
-struct device_type rmi_device_type = {
- .name = "rmi_sensor",
- .release = rmi_release_device,
-};
-
-bool rmi_is_physical_device(struct device *dev)
-{
- return dev->type == &rmi_device_type;
-}
-
#ifdef CONFIG_RMI4_DEBUG
static void rmi_physical_setup_debugfs(struct rmi_device *rmi_dev)
@@ -94,8 +77,7 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
return -EINVAL;
}
- rmi_dev = devm_kzalloc(xport->dev,
- sizeof(struct rmi_device), GFP_KERNEL);
+ rmi_dev = kzalloc(sizeof(struct rmi_device), GFP_KERNEL);
if (!rmi_dev)
return -ENOMEM;
@@ -112,8 +94,10 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
rmi_physical_setup_debugfs(rmi_dev);
error = device_register(&rmi_dev->dev);
- if (error)
+ if (error) {
+ put_device(&rmi_dev->dev);
return error;
+ }
dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
pdata->sensor_name, dev_name(&rmi_dev->dev));
@@ -131,7 +115,6 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
{
struct rmi_device *rmi_dev = xport->rmi_dev;
- rmi_physical_teardown_debugfs(rmi_dev);
device_unregister(&rmi_dev->dev);
}
EXPORT_SYMBOL(rmi_unregister_transport_device);
@@ -139,21 +122,6 @@ EXPORT_SYMBOL(rmi_unregister_transport_device);
/* Function specific stuff */
-static void rmi_release_function(struct device *dev)
-{
- struct rmi_function *fn = to_rmi_function(dev);
- kfree(fn);
-}
-
-struct device_type rmi_function_type = {
- .name = "rmi_function",
- .release = rmi_release_function,
-};
-
-bool rmi_is_function_device(struct device *dev)
-{
- return dev->type == &rmi_function_type;
-}
#ifdef CONFIG_RMI4_DEBUG
@@ -185,6 +153,23 @@ static void rmi_function_teardown_debugfs(struct rmi_function *fn)
}
#endif
+static void rmi_release_function(struct device *dev)
+{
+ struct rmi_function *fn = to_rmi_function(dev);
+ rmi_function_teardown_debugfs(fn);
+ kfree(fn->irq_mask);
+ kfree(fn);
+}
+
+struct device_type rmi_function_type = {
+ .name = "rmi_function",
+ .release = rmi_release_function,
+};
+
+bool rmi_is_function_device(struct device *dev)
+{
+ return dev->type == &rmi_function_type;
+}
static int rmi_function_match(struct device *dev, struct device_driver *drv)
{
@@ -240,21 +225,17 @@ int rmi_register_function(struct rmi_function *fn)
dev_err(&rmi_dev->dev,
"Failed device_register function device %s\n",
dev_name(&fn->dev));
- goto error_exit;
+ put_device(&fn->dev);
+ return error;
}
dev_dbg(&rmi_dev->dev, "Registered F%02X.\n", fn->fd.function_number);
return 0;
-
-error_exit:
- rmi_function_teardown_debugfs(fn);
- return error;
}
void rmi_unregister_function(struct rmi_function *fn)
{
- rmi_function_teardown_debugfs(fn);
device_unregister(&fn->dev);
}
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 34f19e9..43575a1 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -674,8 +674,7 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
if (!fn->irq_mask) {
dev_err(dev, "%s: Failed to create irq_mask for F%02X.\n",
__func__, pdt->function_number);
- error = -ENOMEM;
- goto err_free_mem;
+ return -ENOMEM;
}
for (i = 0; i < fn->num_of_irqs; i++)
@@ -683,7 +682,7 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
error = rmi_register_function(fn);
if (error)
- goto err_free_irq_mask;
+ return error;
if (pdt->function_number == 0x01)
data->f01_container = fn;
@@ -691,12 +690,6 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
list_add_tail(&fn->node, &data->function_list);
return RMI_SCAN_CONTINUE;
-
-err_free_irq_mask:
- kfree(fn->irq_mask);
-err_free_mem:
- kfree(fn);
- return error;
}
#ifdef CONFIG_PM_SLEEP
^ permalink raw reply related
* [PATCH] input synaptics-rmi4: rmi_f01.c storage fix
From: Christopher Heiny @ 2014-02-11 23:13 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, David Herrmann, Jiri Kosina
Correctly free driver related data when initialization fails.
Trivial: Clarify a diagnostic message.
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Linux Walleij <linus.walleij@linaro.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>
---
drivers/input/rmi4/rmi_f01.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 381ad60..e4a6df9 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
if (!f01) {
- dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
+ dev_err(&fn->dev, "Failed to allocate f01_data.\n");
return -ENOMEM;
}
@@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
GFP_KERNEL);
if (!f01->device_control.interrupt_enable) {
dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
+ devm_kfree(&fn->dev, f01);
return -ENOMEM;
}
fn->data = f01;
@@ -381,7 +382,8 @@ static int rmi_f01_initialize(struct rmi_function *fn)
return 0;
error_exit:
- kfree(data);
+ devm_kfree(&fn->dev, data->device_control.interrupt_enable);
+ devm_kfree(&fn->dev, data);
return error;
}
^ permalink raw reply related
* Re: [PATCH V1] da9052: ONKEY: use correct register bit for key status
From: Dmitry Torokhov @ 2014-02-11 17:06 UTC (permalink / raw)
To: Opensource [Anthony Olech]
Cc: Paul Gortmaker, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, David Dajun Chen
In-Reply-To: <24DF37198A1E704D9811D8F72B87EB51BCFD52F3@NB-EX-MBX02.diasemi.com>
On Tue, Feb 11, 2014 at 04:57:47PM +0000, Opensource [Anthony Olech] wrote:
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: 11 February 2014 16:29
> ..[snip]..
> > > static void da9052_onkey_query(struct da9052_onkey *onkey) {
> > > - int key_stat;
> > > + int ret, key_stat;
> > >
> > > - key_stat = da9052_reg_read(onkey->da9052,
> > DA9052_EVENT_B_REG);
> > > - if (key_stat < 0) {
> > > + ret = da9052_reg_read(onkey->da9052, DA9052_STATUS_A_REG);
> > > + if (ret < 0) {
> > > dev_err(onkey->da9052->dev,
> > > - "Failed to read onkey event %d\n", key_stat);
> > > + "Failed to read onkey event err=%d\n", ret);
> > > + key_stat = false;
> >
> > Why do you need this assignment? Also, key_stat is integer, why are we
> > using boolean values for it?
> >
> > > } else {
> > > /*
> > > * Since interrupt for deassertion of ONKEY pin is not
> > > * generated, onkey event state determines the onkey
> > > * button state.
> > > */
> > > - key_stat &= DA9052_EVENTB_ENONKEY;
> > > + if (ret & DA9052_STATUSA_NONKEY)
> > > + key_stat = false;
> > > + else
> > > + key_stat = true;
> >
> > It seems to me that the relevant changes are replacement of
> > DA9052_EVENT_B_REG -> DA9052_STATUS_A_REG in da9052_reg_read()
> > and doing:
> >
> > key_stat &= DA9052_STATUSA_NONKEY;
> > input_report_key(onkey->input, KEY_POWER, !key_stat);
> >
> > Right?
>
> Right as far as the register and the bit within it.
>
> However, should the register read fail due to the PMIC not responding
> on the i2c bus then the previous code would just loop round the work
> queue. By explicitly using key_stat as a boolean it makes the code
> very clear that in the case of failure a 'false' value is set.
Then it should be defined as boolean.
> As it
> was the negative value of the error code would be treated as true,
> since -EFAULT being non-zero will be treated as true.
This is however not what your patch description said it does; it only
mentioned the incorrect register assignment. Maybe we shoudl do the
following then:
...
} else {
bool pressed = !(ret & DA9052_STATUSA_NONKEY);
input_report_key(onkey->input, KEY_POWER, pressed);
input_sync(onkey->input);
if (pressed)
schedule_work(...);
}
And mention that we are changing polling (no longer re-poll on read
errors) in addition to changing register/bits used.
Thanks.
--
Dmitry
^ permalink raw reply
* RE: [PATCH V1] da9052: ONKEY: use correct register bit for key status
From: Opensource [Anthony Olech] @ 2014-02-11 16:57 UTC (permalink / raw)
To: Dmitry Torokhov, Opensource [Anthony Olech]
Cc: Paul Gortmaker, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, David Dajun Chen
In-Reply-To: <20140211162857.GA12106@core.coreip.homeip.net>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 11 February 2014 16:29
..[snip]..
> > static void da9052_onkey_query(struct da9052_onkey *onkey) {
> > - int key_stat;
> > + int ret, key_stat;
> >
> > - key_stat = da9052_reg_read(onkey->da9052,
> DA9052_EVENT_B_REG);
> > - if (key_stat < 0) {
> > + ret = da9052_reg_read(onkey->da9052, DA9052_STATUS_A_REG);
> > + if (ret < 0) {
> > dev_err(onkey->da9052->dev,
> > - "Failed to read onkey event %d\n", key_stat);
> > + "Failed to read onkey event err=%d\n", ret);
> > + key_stat = false;
>
> Why do you need this assignment? Also, key_stat is integer, why are we
> using boolean values for it?
>
> > } else {
> > /*
> > * Since interrupt for deassertion of ONKEY pin is not
> > * generated, onkey event state determines the onkey
> > * button state.
> > */
> > - key_stat &= DA9052_EVENTB_ENONKEY;
> > + if (ret & DA9052_STATUSA_NONKEY)
> > + key_stat = false;
> > + else
> > + key_stat = true;
>
> It seems to me that the relevant changes are replacement of
> DA9052_EVENT_B_REG -> DA9052_STATUS_A_REG in da9052_reg_read()
> and doing:
>
> key_stat &= DA9052_STATUSA_NONKEY;
> input_report_key(onkey->input, KEY_POWER, !key_stat);
>
> Right?
Right as far as the register and the bit within it.
However, should the register read fail due to the PMIC not responding on the i2c bus then the previous code would just loop round the work queue.
By explicitly using key_stat as a boolean it makes the code very clear that in the case of failure a 'false' value is set. As it was the negative value of the error code would be treated as true, since -EFAULT being non-zero will be treated as true.
Tony Olech
> Thanks.
> Dmitry
^ permalink raw reply
* Re: [PATCH RFC] Input: Add Microchip AR1021 i2c touchscreen
From: Dmitry Torokhov @ 2014-02-11 16:34 UTC (permalink / raw)
To: Christian Gmeiner; +Cc: linux-input
In-Reply-To: <CAH9NwWcpttFN9=QKeCM_StJiLsvcYSapuDqcwtKQJTAxfQLGng@mail.gmail.com>
On Tue, Feb 11, 2014 at 02:10:50PM +0100, Christian Gmeiner wrote:
> Hi Dmitry
>
> 2014-01-31 18:16 GMT+01:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> > On Fri, Jan 31, 2014 at 09:15:21AM -0800, Dmitry Torokhov wrote:
> >> Hi Chrisitian,
> >>
> >> On Fri, Jan 31, 2014 at 12:40:19PM +0100, Christian Gmeiner wrote:
> >> > >> --- /dev/null
> >> > >> +++ b/drivers/input/touchscreen/ar1021_i2c.c
> >> > >> @@ -0,0 +1,201 @@
> >> > >> +/*
> >> > >> + * Microchip AR1021 driver for I2C
> >> > >> + *
> >> > >> + * Author: Christian Gmeiner <christian.gmeiner@gmail.com>
> >> > >> + *
> >> > >> + * License: GPL as published by the FSF.
> >>
> >> By the way, you probably do not want GPL v1 to apply... Maybe say GPL v2
> >> or GPL v2 and later (depending on your preference and the license of the
> >> code you used as a base)?
> >>
> >> > >> +
> >> > >> +static int ar1021_i2c_resume(struct device *dev)
> >> > >> +{
> >> > >> + struct i2c_client *client = to_i2c_client(dev);
> >> > >> +
> >> > >> + enable_irq(client->irq);
> >> > >
> >> > > You do not want to enable IRQ if there are no users (nobody opened
> >> > > device).
> >> > >
> >> >
> >> > Okay.. but then I also do not need the disable_irq(..) call in
> >> > ar1021_i2c_suspend
> >> > and can totally remove the PM stuff - or?
> >>
> >> No, I think you still need the PM methods, you just need to check if
> >> device is opened (take dev->mutex, check dev->users) and decide if you
> >> need to enable/disable IRQ or not.
> >
> > Hmm, on the other hand enable/disable does the counting for you so maybe
> > you should leave it all as it was.
>
> ok
>
> At the moment I am doing the final tests of the driver and it fails to
> load via device tree :(
> I changed the compatible string from "mc,ar1021-i2c" to "microchip,ar1021-i2c".
>
> and this is how my dts looks like on an imx6d board:
>
> &i2c2 {
> status = "okay";
> clock-frequency = <100000>;
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_i2c2_1>;
>
> ar1021@4d {
> compatible = "microchip,ar1021-i2c";
> reg = <0x4d>;
> interrupt-parent = <&gpio3>;
> interrupts = <26 0x2>;
> };
> };
>
>
> Any hints?
Not really. Does the driver bind to the device if you define I2C device
in the board code? Are there any errors reported by the kernel?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v4 RESEND] ims-pcu: Add commands supported by the new version of the FW
From: Andrey Smirnov @ 2014-02-11 16:31 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <20140211162943.GB12106@core.coreip.homeip.net>
On Tue, Feb 11, 2014 at 8:29 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Andrey,
>
> On Tue, Feb 11, 2014 at 07:28:40AM -0800, Andrey Smirnov wrote:
>> New version of the PCU firmware supports two new commands:
>> - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
>> registers of one finger navigation(OFN) chip present on the device
>> - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
>> registers of said chip.
>>
>> This commit adds two helper functions to use those commands and sysfs
>> attributes to use them. It also exposes some OFN configuration
>> parameters via sysfs.
>>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> I have it queued for the next merge window.
>
> Thanks.
Awesome, thank you so much!
>
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH v4 RESEND] ims-pcu: Add commands supported by the new version of the FW
From: Dmitry Torokhov @ 2014-02-11 16:29 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: linux-input, linux-kernel
In-Reply-To: <1392132520-2497-1-git-send-email-andrew.smirnov@gmail.com>
Hi Andrey,
On Tue, Feb 11, 2014 at 07:28:40AM -0800, Andrey Smirnov wrote:
> New version of the PCU firmware supports two new commands:
> - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
> registers of one finger navigation(OFN) chip present on the device
> - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
> registers of said chip.
>
> This commit adds two helper functions to use those commands and sysfs
> attributes to use them. It also exposes some OFN configuration
> parameters via sysfs.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
I have it queued for the next merge window.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH V1] da9052: ONKEY: use correct register bit for key status
From: Dmitry Torokhov @ 2014-02-11 16:28 UTC (permalink / raw)
To: Anthony Olech <anthony.olech.opensource@diasemi.com>
Cc: Paul Gortmaker, linux-input, linux-kernel, David Dajun Chen
In-Reply-To: <201402111610.s1BGAXJo056493@swsrvapps-02.lan>
Hi Anthony,
On Thu, Feb 06, 2014 at 03:19:49PM +0000, Anthony Olech <anthony.olech.opensource@diasemi.com> wrote:
> The wrong register bit of the DA9052/3 PMIC registers was
> used to determine the status on the ONKEY.
>
> Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
> Signed-off-by: David Dajun Chen <david.chen@diasemi.com>
> ---
>
> This patch is relative to linux-next repository tag next-20140206
>
> The bug that this patch fixes affects only the DA9052 ONKEY driver.
>
> The problem was detected whilst running a scripted set of functional
> regression tests whilst investigating a different problem.
>
> This patch has been test compiled on an amd64 server for both x86
> and arm targets.
>
> This patch has been spot verified using an SMDK6410 platform
> fly-wired to a Dialog da9053 EVB.
>
> drivers/input/misc/da9052_onkey.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/misc/da9052_onkey.c b/drivers/input/misc/da9052_onkey.c
> index 1f695f2..7e78334 100644
> --- a/drivers/input/misc/da9052_onkey.c
> +++ b/drivers/input/misc/da9052_onkey.c
> @@ -27,19 +27,23 @@ struct da9052_onkey {
>
> static void da9052_onkey_query(struct da9052_onkey *onkey)
> {
> - int key_stat;
> + int ret, key_stat;
>
> - key_stat = da9052_reg_read(onkey->da9052, DA9052_EVENT_B_REG);
> - if (key_stat < 0) {
> + ret = da9052_reg_read(onkey->da9052, DA9052_STATUS_A_REG);
> + if (ret < 0) {
> dev_err(onkey->da9052->dev,
> - "Failed to read onkey event %d\n", key_stat);
> + "Failed to read onkey event err=%d\n", ret);
> + key_stat = false;
Why do you need this assignment? Also, key_stat is integer, why are we
using boolean values for it?
> } else {
> /*
> * Since interrupt for deassertion of ONKEY pin is not
> * generated, onkey event state determines the onkey
> * button state.
> */
> - key_stat &= DA9052_EVENTB_ENONKEY;
> + if (ret & DA9052_STATUSA_NONKEY)
> + key_stat = false;
> + else
> + key_stat = true;
It seems to me that the relevant changes are replacement of
DA9052_EVENT_B_REG -> DA9052_STATUS_A_REG in da9052_reg_read() and doing:
key_stat &= DA9052_STATUSA_NONKEY;
input_report_key(onkey->input, KEY_POWER, !key_stat);
Right?
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH V1] da9052: ONKEY: use correct register bit for key status
From: Anthony Olech <anthony.olech.opensource@diasemi.com> @ 2014-02-06 15:19 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Paul Gortmaker, linux-input, linux-kernel, David Dajun Chen
The wrong register bit of the DA9052/3 PMIC registers was
used to determine the status on the ONKEY.
Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
Signed-off-by: David Dajun Chen <david.chen@diasemi.com>
---
This patch is relative to linux-next repository tag next-20140206
The bug that this patch fixes affects only the DA9052 ONKEY driver.
The problem was detected whilst running a scripted set of functional
regression tests whilst investigating a different problem.
This patch has been test compiled on an amd64 server for both x86
and arm targets.
This patch has been spot verified using an SMDK6410 platform
fly-wired to a Dialog da9053 EVB.
drivers/input/misc/da9052_onkey.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/input/misc/da9052_onkey.c b/drivers/input/misc/da9052_onkey.c
index 1f695f2..7e78334 100644
--- a/drivers/input/misc/da9052_onkey.c
+++ b/drivers/input/misc/da9052_onkey.c
@@ -27,19 +27,23 @@ struct da9052_onkey {
static void da9052_onkey_query(struct da9052_onkey *onkey)
{
- int key_stat;
+ int ret, key_stat;
- key_stat = da9052_reg_read(onkey->da9052, DA9052_EVENT_B_REG);
- if (key_stat < 0) {
+ ret = da9052_reg_read(onkey->da9052, DA9052_STATUS_A_REG);
+ if (ret < 0) {
dev_err(onkey->da9052->dev,
- "Failed to read onkey event %d\n", key_stat);
+ "Failed to read onkey event err=%d\n", ret);
+ key_stat = false;
} else {
/*
* Since interrupt for deassertion of ONKEY pin is not
* generated, onkey event state determines the onkey
* button state.
*/
- key_stat &= DA9052_EVENTB_ENONKEY;
+ if (ret & DA9052_STATUSA_NONKEY)
+ key_stat = false;
+ else
+ key_stat = true;
input_report_key(onkey->input, KEY_POWER, key_stat);
input_sync(onkey->input);
}
--
end-of-patch for da9052: ONKEY: use correct register bit for key status V1
^ permalink raw reply related
* [PATCH v4 RESEND] ims-pcu: Add commands supported by the new version of the FW
From: Andrey Smirnov @ 2014-02-11 15:28 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: linux-input, andrew.smirnov, linux-kernel
New version of the PCU firmware supports two new commands:
- IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
registers of one finger navigation(OFN) chip present on the device
- IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
registers of said chip.
This commit adds two helper functions to use those commands and sysfs
attributes to use them. It also exposes some OFN configuration
parameters via sysfs.
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/misc/ims-pcu.c | 260 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 251 insertions(+), 9 deletions(-)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index e204f26..0306e8d 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -51,6 +51,8 @@ struct ims_pcu_backlight {
#define IMS_PCU_BL_VERSION_LEN (9 + 1)
#define IMS_PCU_BL_RESET_REASON_LEN (2 + 1)
+#define IMS_PCU_PCU_B_DEVICE_ID 5
+
#define IMS_PCU_BUF_SIZE 128
struct ims_pcu {
@@ -68,6 +70,9 @@ struct ims_pcu {
char bl_version[IMS_PCU_BL_VERSION_LEN];
char reset_reason[IMS_PCU_BL_RESET_REASON_LEN];
int update_firmware_status;
+ u8 device_id;
+
+ u8 ofn_reg_addr;
struct usb_interface *ctrl_intf;
@@ -371,6 +376,8 @@ static void ims_pcu_destroy_gamepad(struct ims_pcu *pcu)
#define IMS_PCU_CMD_GET_DEVICE_ID 0xae
#define IMS_PCU_CMD_SPECIAL_INFO 0xb0
#define IMS_PCU_CMD_BOOTLOADER 0xb1 /* Pass data to bootloader */
+#define IMS_PCU_CMD_OFN_SET_CONFIG 0xb3
+#define IMS_PCU_CMD_OFN_GET_CONFIG 0xb4
/* PCU responses */
#define IMS_PCU_RSP_STATUS 0xc0
@@ -389,6 +396,9 @@ static void ims_pcu_destroy_gamepad(struct ims_pcu *pcu)
#define IMS_PCU_RSP_GET_DEVICE_ID 0xce
#define IMS_PCU_RSP_SPECIAL_INFO 0xd0
#define IMS_PCU_RSP_BOOTLOADER 0xd1 /* Bootloader response */
+#define IMS_PCU_RSP_OFN_SET_CONFIG 0xd2
+#define IMS_PCU_RSP_OFN_GET_CONFIG 0xd3
+
#define IMS_PCU_RSP_EVNT_BUTTONS 0xe0 /* Unsolicited, button state */
#define IMS_PCU_GAMEPAD_MASK 0x0001ff80UL /* Bits 7 through 16 */
@@ -1226,7 +1236,7 @@ static struct attribute *ims_pcu_attrs[] = {
&dev_attr_reset_device.attr,
&dev_attr_update_firmware.attr,
&dev_attr_update_firmware_status.attr,
- NULL
+ NULL,
};
static umode_t ims_pcu_is_attr_visible(struct kobject *kobj,
@@ -1256,6 +1266,225 @@ static struct attribute_group ims_pcu_attr_group = {
.attrs = ims_pcu_attrs,
};
+/* Support for a separate OFN attribute group */
+
+#define OFN_REG_RESULT_OFFSET 2
+
+static int ims_pcu_read_ofn_config(struct ims_pcu *pcu, u8 addr, u8 *data)
+{
+ int error;
+ s16 result;
+
+ error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
+ &addr, sizeof(addr));
+ if (error)
+ return error;
+
+ result = (s16)get_unaligned_le16(pcu->cmd_buf + OFN_REG_RESULT_OFFSET);
+ if (result < 0)
+ return -EIO;
+
+ /* We only need LSB */
+ *data = pcu->cmd_buf[OFN_REG_RESULT_OFFSET];
+ return 0;
+}
+
+static int ims_pcu_write_ofn_config(struct ims_pcu *pcu, u8 addr, u8 data)
+{
+ u8 buffer[] = { addr, data };
+ int error;
+ s16 result;
+
+ error = ims_pcu_execute_command(pcu, OFN_SET_CONFIG,
+ &buffer, sizeof(buffer));
+ if (error)
+ return error;
+
+ result = (s16)get_unaligned_le16(pcu->cmd_buf + OFN_REG_RESULT_OFFSET);
+ if (result < 0)
+ return -EIO;
+
+ return 0;
+}
+
+static ssize_t ims_pcu_ofn_reg_data_show(struct device *dev,
+ struct device_attribute *dattr,
+ char *buf)
+{
+ struct usb_interface *intf = to_usb_interface(dev);
+ struct ims_pcu *pcu = usb_get_intfdata(intf);
+ int error;
+ u8 data;
+
+ mutex_lock(&pcu->cmd_mutex);
+ error = ims_pcu_read_ofn_config(pcu, pcu->ofn_reg_addr, &data);
+ mutex_unlock(&pcu->cmd_mutex);
+
+ if (error)
+ return error;
+
+ return scnprintf(buf, PAGE_SIZE, "%x\n", data);
+}
+
+static ssize_t ims_pcu_ofn_reg_data_store(struct device *dev,
+ struct device_attribute *dattr,
+ const char *buf, size_t count)
+{
+ struct usb_interface *intf = to_usb_interface(dev);
+ struct ims_pcu *pcu = usb_get_intfdata(intf);
+ int error;
+ u8 value;
+
+ error = kstrtou8(buf, 0, &value);
+ if (error)
+ return error;
+
+ mutex_lock(&pcu->cmd_mutex);
+ error = ims_pcu_write_ofn_config(pcu, pcu->ofn_reg_addr, value);
+ mutex_unlock(&pcu->cmd_mutex);
+
+ return error ?: count;
+}
+
+static DEVICE_ATTR(reg_data, S_IRUGO | S_IWUSR,
+ ims_pcu_ofn_reg_data_show, ims_pcu_ofn_reg_data_store);
+
+static ssize_t ims_pcu_ofn_reg_addr_show(struct device *dev,
+ struct device_attribute *dattr,
+ char *buf)
+{
+ struct usb_interface *intf = to_usb_interface(dev);
+ struct ims_pcu *pcu = usb_get_intfdata(intf);
+ int error;
+
+ mutex_lock(&pcu->cmd_mutex);
+ error = scnprintf(buf, PAGE_SIZE, "%x\n", pcu->ofn_reg_addr);
+ mutex_unlock(&pcu->cmd_mutex);
+
+ return error;
+}
+
+static ssize_t ims_pcu_ofn_reg_addr_store(struct device *dev,
+ struct device_attribute *dattr,
+ const char *buf, size_t count)
+{
+ struct usb_interface *intf = to_usb_interface(dev);
+ struct ims_pcu *pcu = usb_get_intfdata(intf);
+ int error;
+ u8 value;
+
+ error = kstrtou8(buf, 0, &value);
+ if (error)
+ return error;
+
+ mutex_lock(&pcu->cmd_mutex);
+ pcu->ofn_reg_addr = value;
+ mutex_unlock(&pcu->cmd_mutex);
+
+ return error ?: count;
+}
+
+static DEVICE_ATTR(reg_addr, S_IRUGO | S_IWUSR,
+ ims_pcu_ofn_reg_addr_show, ims_pcu_ofn_reg_addr_store);
+
+struct ims_pcu_ofn_bit_attribute {
+ struct device_attribute dattr;
+ u8 addr;
+ u8 nr;
+};
+
+static ssize_t ims_pcu_ofn_bit_show(struct device *dev,
+ struct device_attribute *dattr,
+ char *buf)
+{
+ struct usb_interface *intf = to_usb_interface(dev);
+ struct ims_pcu *pcu = usb_get_intfdata(intf);
+ struct ims_pcu_ofn_bit_attribute *attr =
+ container_of(dattr, struct ims_pcu_ofn_bit_attribute, dattr);
+ int error;
+ u8 data;
+
+ mutex_lock(&pcu->cmd_mutex);
+ error = ims_pcu_read_ofn_config(pcu, attr->addr, &data);
+ mutex_unlock(&pcu->cmd_mutex);
+
+ if (error)
+ return error;
+
+ return scnprintf(buf, PAGE_SIZE, "%d\n", !!(data & (1 << attr->nr)));
+}
+
+static ssize_t ims_pcu_ofn_bit_store(struct device *dev,
+ struct device_attribute *dattr,
+ const char *buf, size_t count)
+{
+ struct usb_interface *intf = to_usb_interface(dev);
+ struct ims_pcu *pcu = usb_get_intfdata(intf);
+ struct ims_pcu_ofn_bit_attribute *attr =
+ container_of(dattr, struct ims_pcu_ofn_bit_attribute, dattr);
+ int error;
+ int value;
+ u8 data;
+
+ error = kstrtoint(buf, 0, &value);
+ if (error)
+ return error;
+
+ if (value > 1)
+ return -EINVAL;
+
+ mutex_lock(&pcu->cmd_mutex);
+
+ error = ims_pcu_read_ofn_config(pcu, attr->addr, &data);
+ if (!error) {
+ if (value)
+ data |= 1U << attr->nr;
+ else
+ data &= ~(1U << attr->nr);
+
+ error = ims_pcu_write_ofn_config(pcu, attr->addr, data);
+ }
+
+ mutex_unlock(&pcu->cmd_mutex);
+
+ return error ?: count;
+}
+
+#define IMS_PCU_OFN_BIT_ATTR(_field, _addr, _nr) \
+struct ims_pcu_ofn_bit_attribute ims_pcu_ofn_attr_##_field = { \
+ .dattr = __ATTR(_field, S_IWUSR | S_IRUGO, \
+ ims_pcu_ofn_bit_show, ims_pcu_ofn_bit_store), \
+ .addr = _addr, \
+ .nr = _nr, \
+}
+
+static IMS_PCU_OFN_BIT_ATTR(engine_enable, 0x60, 7);
+static IMS_PCU_OFN_BIT_ATTR(speed_enable, 0x60, 6);
+static IMS_PCU_OFN_BIT_ATTR(assert_enable, 0x60, 5);
+static IMS_PCU_OFN_BIT_ATTR(xyquant_enable, 0x60, 4);
+static IMS_PCU_OFN_BIT_ATTR(xyscale_enable, 0x60, 1);
+
+static IMS_PCU_OFN_BIT_ATTR(scale_x2, 0x63, 6);
+static IMS_PCU_OFN_BIT_ATTR(scale_y2, 0x63, 7);
+
+static struct attribute *ims_pcu_ofn_attrs[] = {
+ &dev_attr_reg_data.attr,
+ &dev_attr_reg_addr.attr,
+ &ims_pcu_ofn_attr_engine_enable.dattr.attr,
+ &ims_pcu_ofn_attr_speed_enable.dattr.attr,
+ &ims_pcu_ofn_attr_assert_enable.dattr.attr,
+ &ims_pcu_ofn_attr_xyquant_enable.dattr.attr,
+ &ims_pcu_ofn_attr_xyscale_enable.dattr.attr,
+ &ims_pcu_ofn_attr_scale_x2.dattr.attr,
+ &ims_pcu_ofn_attr_scale_y2.dattr.attr,
+ NULL
+};
+
+static struct attribute_group ims_pcu_ofn_attr_group = {
+ .name = "ofn",
+ .attrs = ims_pcu_ofn_attrs,
+};
+
static void ims_pcu_irq(struct urb *urb)
{
struct ims_pcu *pcu = urb->context;
@@ -1624,7 +1853,6 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
static atomic_t device_no = ATOMIC_INIT(0);
const struct ims_pcu_device_info *info;
- u8 device_id;
int error;
error = ims_pcu_get_device_info(pcu);
@@ -1633,7 +1861,7 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
return error;
}
- error = ims_pcu_identify_type(pcu, &device_id);
+ error = ims_pcu_identify_type(pcu, &pcu->device_id);
if (error) {
dev_err(pcu->dev,
"Failed to identify device, error: %d\n", error);
@@ -1645,9 +1873,9 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
return 0;
}
- if (device_id >= ARRAY_SIZE(ims_pcu_device_info) ||
- !ims_pcu_device_info[device_id].keymap) {
- dev_err(pcu->dev, "Device ID %d is not valid\n", device_id);
+ if (pcu->device_id >= ARRAY_SIZE(ims_pcu_device_info) ||
+ !ims_pcu_device_info[pcu->device_id].keymap) {
+ dev_err(pcu->dev, "Device ID %d is not valid\n", pcu->device_id);
/* Same as above, punt to userspace */
return 0;
}
@@ -1655,11 +1883,21 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
/* Device appears to be operable, complete initialization */
pcu->device_no = atomic_inc_return(&device_no) - 1;
+ /*
+ PCU-B devices, both GEN_1 and GEN_2 do not have OFN sensor
+ */
+ if (pcu->device_id != IMS_PCU_PCU_B_DEVICE_ID) {
+ error = sysfs_create_group(&pcu->dev->kobj,
+ &ims_pcu_ofn_attr_group);
+ if (error)
+ return error;
+ }
+
error = ims_pcu_setup_backlight(pcu);
if (error)
return error;
- info = &ims_pcu_device_info[device_id];
+ info = &ims_pcu_device_info[pcu->device_id];
error = ims_pcu_setup_buttons(pcu, info->keymap, info->keymap_len);
if (error)
goto err_destroy_backlight;
@@ -1674,10 +1912,10 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
return 0;
-err_destroy_backlight:
- ims_pcu_destroy_backlight(pcu);
err_destroy_buttons:
ims_pcu_destroy_buttons(pcu);
+err_destroy_backlight:
+ ims_pcu_destroy_backlight(pcu);
return error;
}
@@ -1691,6 +1929,10 @@ static void ims_pcu_destroy_application_mode(struct ims_pcu *pcu)
ims_pcu_destroy_gamepad(pcu);
ims_pcu_destroy_buttons(pcu);
ims_pcu_destroy_backlight(pcu);
+
+ if (pcu->device_id != IMS_PCU_PCU_B_DEVICE_ID)
+ sysfs_remove_group(&pcu->dev->kobj,
+ &ims_pcu_ofn_attr_group);
}
}
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH RFC] Input: Add Microchip AR1021 i2c touchscreen
From: Christian Gmeiner @ 2014-02-11 13:10 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
In-Reply-To: <20140131171647.GB20872@core.coreip.homeip.net>
Hi Dmitry
2014-01-31 18:16 GMT+01:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> On Fri, Jan 31, 2014 at 09:15:21AM -0800, Dmitry Torokhov wrote:
>> Hi Chrisitian,
>>
>> On Fri, Jan 31, 2014 at 12:40:19PM +0100, Christian Gmeiner wrote:
>> > >> --- /dev/null
>> > >> +++ b/drivers/input/touchscreen/ar1021_i2c.c
>> > >> @@ -0,0 +1,201 @@
>> > >> +/*
>> > >> + * Microchip AR1021 driver for I2C
>> > >> + *
>> > >> + * Author: Christian Gmeiner <christian.gmeiner@gmail.com>
>> > >> + *
>> > >> + * License: GPL as published by the FSF.
>>
>> By the way, you probably do not want GPL v1 to apply... Maybe say GPL v2
>> or GPL v2 and later (depending on your preference and the license of the
>> code you used as a base)?
>>
>> > >> +
>> > >> +static int ar1021_i2c_resume(struct device *dev)
>> > >> +{
>> > >> + struct i2c_client *client = to_i2c_client(dev);
>> > >> +
>> > >> + enable_irq(client->irq);
>> > >
>> > > You do not want to enable IRQ if there are no users (nobody opened
>> > > device).
>> > >
>> >
>> > Okay.. but then I also do not need the disable_irq(..) call in
>> > ar1021_i2c_suspend
>> > and can totally remove the PM stuff - or?
>>
>> No, I think you still need the PM methods, you just need to check if
>> device is opened (take dev->mutex, check dev->users) and decide if you
>> need to enable/disable IRQ or not.
>
> Hmm, on the other hand enable/disable does the counting for you so maybe
> you should leave it all as it was.
ok
At the moment I am doing the final tests of the driver and it fails to
load via device tree :(
I changed the compatible string from "mc,ar1021-i2c" to "microchip,ar1021-i2c".
and this is how my dts looks like on an imx6d board:
&i2c2 {
status = "okay";
clock-frequency = <100000>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_i2c2_1>;
ar1021@4d {
compatible = "microchip,ar1021-i2c";
reg = <0x4d>;
interrupt-parent = <&gpio3>;
interrupts = <26 0x2>;
};
};
Any hints?
--
Christian Gmeiner, MSc
https://soundcloud.com/christian-gmeiner
^ permalink raw reply
* Re: [PATCH 6/7] Input: pwm-beepe: remove HAVE_PWM dependencies
From: Jingoo Han @ 2014-02-11 12:18 UTC (permalink / raw)
To: 'Lars-Peter Clausen'
Cc: 'Dmitry Torokhov', linux-kernel, linux-input,
'Jingoo Han'
In-Reply-To: <52FA12D1.4020408@metafoo.de>
On Tuesday, February 11, 2014 9:09 PM, Lars-Peter Clausen wrote:
> On 02/10/2014 02:16 AM, Jingoo Han wrote:
> > The HAVE_PWM symbol is only for legacy platforms that provide
> > the PWM API without using the generic framework. However, legacy
> > PWM drivers were already moved to the generic PWM framework.
> > Thus, HAVE_PWM should be removed, because HAVE_PWM is not
> > required anymore.
> >
> > Signed-off-by: Jingoo Han <jg1.han@samsung.com>
>
> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
Hi Lars-Peter Clausen,
I really appreciate your Acked-by. However, the same patch was
already submitted by Sascha Hauer.[1] So, please ignore this patch.
Thank you.
[1] https://lkml.org/lkml/2014/1/16/262
Best regards,
Jingoo Han
>
> > ---
> > drivers/input/misc/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> > index ae74df7..762e6d2 100644
> > --- a/drivers/input/misc/Kconfig
> > +++ b/drivers/input/misc/Kconfig
> > @@ -470,7 +470,7 @@ config INPUT_PCF8574
> >
> > config INPUT_PWM_BEEPER
> > tristate "PWM beeper support"
> > - depends on PWM && HAVE_PWM
> > + depends on PWM
> > help
> > Say Y here to get support for PWM based beeper devices.
> >
> >
^ permalink raw reply
* Re: [PATCH 6/7] Input: pwm-beepe: remove HAVE_PWM dependencies
From: Lars-Peter Clausen @ 2014-02-11 12:08 UTC (permalink / raw)
To: Jingoo Han; +Cc: 'Dmitry Torokhov', linux-kernel, linux-input
In-Reply-To: <003f01cf25fd$b3e274b0$1ba75e10$%han@samsung.com>
On 02/10/2014 02:16 AM, Jingoo Han wrote:
> The HAVE_PWM symbol is only for legacy platforms that provide
> the PWM API without using the generic framework. However, legacy
> PWM drivers were already moved to the generic PWM framework.
> Thus, HAVE_PWM should be removed, because HAVE_PWM is not
> required anymore.
>
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
Acked-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
> drivers/input/misc/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index ae74df7..762e6d2 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -470,7 +470,7 @@ config INPUT_PCF8574
>
> config INPUT_PWM_BEEPER
> tristate "PWM beeper support"
> - depends on PWM && HAVE_PWM
> + depends on PWM
> help
> Say Y here to get support for PWM based beeper devices.
>
>
^ permalink raw reply
* Re: [PATCH 1/4] input: mc13783: Make mc13xxx_buttons_platform_data more flexible
From: Lee Jones @ 2014-02-11 10:07 UTC (permalink / raw)
To: Alexander Shiyan
Cc: linux-input, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
Samuel Ortiz
In-Reply-To: <1392111740.406850821@f89.i.mail.ru>
> > > > > Instead of define each button in separate variable, make an array
> > > > > for storing all buttons. This change will help to add support for
> > > > > other types of PMIC and add support for probing with devicetree
> > > > > in the future.
> > > > >
> > > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > > > ---
> > > > > arch/arm/mach-imx/mach-mx31moboard.c | 9 ++++--
> > > > > drivers/input/misc/mc13783-pwrbutton.c | 56
> > > > +++++++++++++++++-----------------
> > > > > include/linux/mfd/mc13xxx.h | 28 +++++++++--------
> > > >
> > > > Are all the changes in these files entangled? If there is any way to
> > > > make them orthogonal, then all the better.
> > >
> > > Can you say the same thing in other words, I didn't understand a bit
> > > of your comment.
> >
> > Do all of this changes in each of the files listed above depend on
> > each other?
>
> Yes, mc13783-pwrbutton.c & mc13xxx.h - changing the structure,
> mach-mx31moboard.c - updating current users.
>
> > > > > +struct mc13xxx_button {
> > > > > + u16 keycode;
> > > > > + unsigned int flags;
> > > > > +#define MC13XXX_BUTTON_DBNC_0MS 0
> > > > > +#define MC13XXX_BUTTON_DBNC_30MS 1
> > > > > +#define MC13XXX_BUTTON_DBNC_150MS 2
> > > > > +#define MC13XXX_BUTTON_DBNC_750MS 3
> > > > > +#define MC13XXX_BUTTON_ENABLE (1 << 2)
> > > > > +#define MC13XXX_BUTTON_POL_INVERT (1 << 3)
> > > > > +#define MC13XXX_BUTTON_RESET_EN (1 << 4)
> > > > > +};
> > > > > +
> > > >
> > > > Please take the opportunity to remove this slab list of #defines from
> > > > the centre of the struct definition. Just above will be fine.
> > >
> > > I would like to make it in a separate patch for the whole header file later.
> >
> > You may as well do it in this commit? Just add them outside of the
> > struct definition instead of inside.
>
> OK, I'll do it in version 2, but I meant that I could change it for the other
> structures in this header. As part of the input subsystem, I can not do that.
If changes can be subsystem orthogonal, it's better to keep them that
way. Fix this issue in this patch and fix any other issues within this
file in another patch which will go through the MFD tree.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/4] input: mc13783: Make mc13xxx_buttons_platform_data more flexible
From: Alexander Shiyan @ 2014-02-11 9:42 UTC (permalink / raw)
To: Lee Jones
Cc: linux-input, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
Samuel Ortiz
In-Reply-To: <20140211091735.GF32042@lee--X1>
Hello.
> > > > Instead of define each button in separate variable, make an array
> > > > for storing all buttons. This change will help to add support for
> > > > other types of PMIC and add support for probing with devicetree
> > > > in the future.
> > > >
> > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > > ---
> > > > arch/arm/mach-imx/mach-mx31moboard.c | 9 ++++--
> > > > drivers/input/misc/mc13783-pwrbutton.c | 56
> > > +++++++++++++++++-----------------
> > > > include/linux/mfd/mc13xxx.h | 28 +++++++++--------
> > >
> > > Are all the changes in these files entangled? If there is any way to
> > > make them orthogonal, then all the better.
> >
> > Can you say the same thing in other words, I didn't understand a bit
> > of your comment.
>
> Do all of this changes in each of the files listed above depend on
> each other?
Yes, mc13783-pwrbutton.c & mc13xxx.h - changing the structure,
mach-mx31moboard.c - updating current users.
> > > > +struct mc13xxx_button {
> > > > + u16 keycode;
> > > > + unsigned int flags;
> > > > +#define MC13XXX_BUTTON_DBNC_0MS 0
> > > > +#define MC13XXX_BUTTON_DBNC_30MS 1
> > > > +#define MC13XXX_BUTTON_DBNC_150MS 2
> > > > +#define MC13XXX_BUTTON_DBNC_750MS 3
> > > > +#define MC13XXX_BUTTON_ENABLE (1 << 2)
> > > > +#define MC13XXX_BUTTON_POL_INVERT (1 << 3)
> > > > +#define MC13XXX_BUTTON_RESET_EN (1 << 4)
> > > > +};
> > > > +
> > >
> > > Please take the opportunity to remove this slab list of #defines from
> > > the centre of the struct definition. Just above will be fine.
> >
> > I would like to make it in a separate patch for the whole header file later.
>
> You may as well do it in this commit? Just add them outside of the
> struct definition instead of inside.
OK, I'll do it in version 2, but I meant that I could change it for the other
structures in this header. As part of the input subsystem, I can not do that.
Thanks.
---
^ permalink raw reply
* Re: [PATCH 1/4] input: mc13783: Make mc13xxx_buttons_platform_data more flexible
From: Lee Jones @ 2014-02-11 9:17 UTC (permalink / raw)
To: Alexander Shiyan
Cc: linux-input, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
Samuel Ortiz
In-Reply-To: <1392101218.989322000@f169.i.mail.ru>
> > > Instead of define each button in separate variable, make an array
> > > for storing all buttons. This change will help to add support for
> > > other types of PMIC and add support for probing with devicetree
> > > in the future.
> > >
> > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > ---
> > > arch/arm/mach-imx/mach-mx31moboard.c | 9 ++++--
> > > drivers/input/misc/mc13783-pwrbutton.c | 56
> > +++++++++++++++++-----------------
> > > include/linux/mfd/mc13xxx.h | 28 +++++++++--------
> >
> > Are all the changes in these files entangled? If there is any way to
> > make them orthogonal, then all the better.
>
> Can you say the same thing in other words, I didn't understand a bit
> of your comment.
Do all of this changes in each of the files listed above depend on
each other?
> ...
> > > +struct mc13xxx_button {
> > > + u16 keycode;
> > > + unsigned int flags;
> > > +#define MC13XXX_BUTTON_DBNC_0MS 0
> > > +#define MC13XXX_BUTTON_DBNC_30MS 1
> > > +#define MC13XXX_BUTTON_DBNC_150MS 2
> > > +#define MC13XXX_BUTTON_DBNC_750MS 3
> > > +#define MC13XXX_BUTTON_ENABLE (1 << 2)
> > > +#define MC13XXX_BUTTON_POL_INVERT (1 << 3)
> > > +#define MC13XXX_BUTTON_RESET_EN (1 << 4)
> > > +};
> > > +
> >
> > Please take the opportunity to remove this slab list of #defines from
> > the centre of the struct definition. Just above will be fine.
>
> I would like to make it in a separate patch for the whole header file later.
You may as well do it in this commit? Just add them outside of the
struct definition instead of inside.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 2/3] input: appletouch: use better cursor movement smoothing algorithm
From: Clinton Sprain @ 2014-02-11 7:19 UTC (permalink / raw)
To: Henrik Rydberg, linux-input; +Cc: Dmitry Torokhov
In-Reply-To: <52F7714D.7040409@euromail.se>
Hi Henrik,
On 02/09/2014 06:15 AM, Henrik Rydberg wrote:
> Hi Clinton,
>
>> Use smoothed version of sensor array data to calculate movement and add weight
> to prior values when calculating average. This gives more granular and more
> predictable movement.
>>
>> Signed-off-by: Clinton Sprain <clintonsprain@gmail.com>
>> ---
>> drivers/input/mouse/appletouch.c | 60 ++++++++++++++++++++++++++------------
>> 1 file changed, 42 insertions(+), 18 deletions(-)
>
> I like this patch, but there are some technical glitches, comments below.
>
>> diff --git a/drivers/input/mouse/appletouch.c b/drivers/input/mouse/appletouch.c
>> index d48181b..edbdd95 100644
>> --- a/drivers/input/mouse/appletouch.c
>> +++ b/drivers/input/mouse/appletouch.c
>> @@ -338,21 +338,51 @@ static void atp_reinit(struct work_struct *work)
>> static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
>> int *z, int *fingers)
>> {
>> - int i;
>> + int i, k;
>> + int smooth[nb_sensors + 2];
>> + int smooth_tmp[nb_sensors + 2];
>> +
>> /* values to calculate mean */
>> int pcum = 0, psum = 0;
>> int is_increasing = 0;
>>
>> *fingers = 0;
>>
>> + /*
>> + * Use a smoothed version of sensor data for movement calculations, to
>> + * combat noise without needing to toss out values based on a threshold.
>> + * This improves tracking. Finger count is calculated separately based
>> + * on raw data.
>> + */
>> +
>> + for (i = 0; i < nb_sensors; i++) { /* Scale up to minimize */
>> + smooth[i] = xy_sensors[i] << 12; /* rounding/truncation. */
>> + }
>> +
>> + /* Mitigate some of the data loss from smoothing on the edge sensors. */
>> + smooth[-1] = smooth[0] >> 2;
>> + smooth[nb_sensors] = smooth[nb_sensors - 1] >> 2;
>
> Out of bounds array... also, the boundary condition seems odd. If you want to
> extrapolate the edge velocity, and assuming smooth[0] is the first sensor, you
> would get something like (N = nb_sensors)
>
> smooth_tmp[0] = 2 * smooth[1] - smooth[2];
> smooth_tmp[N-1] = 2 * smooth[N-2] - smooth[N-3];
>
This produces a big velocity increase when the finger crosses over from sensor 0 to sensor 1 or vice versa.
We might get by with something relatively simple:
smooth_tmp[0] = (3 * smooth[0] + smooth[1]) >> 2;
...and the same thing on the other edge. This produces predictable behavior while keeping values from bleeding off the board so to speak. (Less important now than if someone down the line ever decides to increase the number of passes.)
>> + for (k = 0; k < 4; k++) {
>> + for (i = 0; i < nb_sensors; i++) { /* Blend with neighbors. */
>
> And here would be for (i = 1; i < nb_sensors - 1; i++)
>
>> + smooth_tmp[i] = (smooth[i - 1] + smooth[i] * 2 + smooth[i + 1]) >> 2;
>> + }
>> +
>> + for (i = 0; i < nb_sensors; i++) {
>> + smooth[i] = smooth_tmp[i];
>> + if (k == 3) /* Last go-round, so scale back down. */
>> + smooth[i] = smooth[i] >> 12;
>
> The scale-up is done separately, so why not make the scale-down separately too?
>
>> + }
>> +
>> + smooth[-1] = smooth[0] >> 2;
>> + smooth[nb_sensors] = smooth[nb_sensors - 1] >> 2;
>
> And these would be dropped.
>
>> + }
>> +
>> for (i = 0; i < nb_sensors; i++) {
>> if (xy_sensors[i] < threshold) {
>> if (is_increasing)
>> is_increasing = 0;
>>
>> - continue;
>> - }
>> -
>> /*
>> * Makes the finger detection more versatile. For example,
>> * two fingers with no gap will be detected. Also, my
>> @@ -367,7 +397,8 @@ static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
>> *
>> * - Jason Parekh <jasonparekh@gmail.com>
>> */
>> - if (i < 1 ||
>> +
>> + } else if (i < 1 ||
>> (!is_increasing && xy_sensors[i - 1] < xy_sensors[i])) {
>> (*fingers)++;
>> is_increasing = 1;
>> @@ -375,15 +406,8 @@ static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
>> is_increasing = 0;
>> }
>>
>> - /*
>> - * Subtracts threshold so a high sensor that just passes the
>> - * threshold won't skew the calculated absolute coordinate.
>> - * Fixes an issue where slowly moving the mouse would
>> - * occasionally jump a number of pixels (slowly moving the
>> - * finger makes this issue most apparent.)
>> - */
>> - pcum += (xy_sensors[i] - threshold) * i;
>> - psum += (xy_sensors[i] - threshold);
>> + pcum += (smooth[i]) * i;
>> + psum += (smooth[i]);
>
> Great, this makes so much more sense.
>
>> }
>>
>> if (psum > 0) {
>> @@ -565,8 +589,8 @@ static void atp_complete_geyser_1_2(struct urb *urb)
>>
>> if (x && y) {
>> if (dev->x_old != -1) {
>> - x = (dev->x_old * 3 + x) >> 2;
>> - y = (dev->y_old * 3 + y) >> 2;
>> + x = (dev->x_old * 7 + x) >> 3;
>> + y = (dev->y_old * 7 + y) >> 3;
>> dev->x_old = x;
>> dev->y_old = y;
>>
>> @@ -677,8 +701,8 @@ static void atp_complete_geyser_3_4(struct urb *urb)
>>
>> if (x && y) {
>> if (dev->x_old != -1) {
>> - x = (dev->x_old * 3 + x) >> 2;
>> - y = (dev->y_old * 3 + y) >> 2;
>> + x = (dev->x_old * 7 + x) >> 3;
>> + y = (dev->y_old * 7 + y) >> 3;
>> dev->x_old = x;
>> dev->y_old = y;
>
> Would you say that the cursor slow-down here is noticeable, or are there enough
> samples per second that it does not matter?
Noticeable but not major. It's still possible to move the cursor across the screen in one movement with typical defaults, and the gain in smoothness is pretty big, so it seems like a reasonable trade-off. (The 'base' speed is also still quicker than the native OS driver, FWIW.)
>
> Thanks,
> Henrik
>
Thanks,
Clinton
^ permalink raw reply
* Re: [PATCH 1/4] input: mc13783: Make mc13xxx_buttons_platform_data more flexible
From: Alexander Shiyan @ 2014-02-11 6:46 UTC (permalink / raw)
To: Lee Jones
Cc: linux-input, Dmitry Torokhov, Shawn Guo, Sascha Hauer,
Samuel Ortiz
In-Reply-To: <20140210112917.GD21522@lee--X1>
Hello.
Понедельник, 10 февраля 2014, 11:29 UTC от Lee Jones <lee.jones@linaro.org>:
> > Instead of define each button in separate variable, make an array
> > for storing all buttons. This change will help to add support for
> > other types of PMIC and add support for probing with devicetree
> > in the future.
> >
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > ---
> > arch/arm/mach-imx/mach-mx31moboard.c | 9 ++++--
> > drivers/input/misc/mc13783-pwrbutton.c | 56
> +++++++++++++++++-----------------
> > include/linux/mfd/mc13xxx.h | 28 +++++++++--------
>
> Are all the changes in these files entangled? If there is any way to
> make them orthogonal, then all the better.
Can you say the same thing in other words, I didn't understand a bit
of your comment.
...
> > +struct mc13xxx_button {
> > + u16 keycode;
> > + unsigned int flags;
> > +#define MC13XXX_BUTTON_DBNC_0MS 0
> > +#define MC13XXX_BUTTON_DBNC_30MS 1
> > +#define MC13XXX_BUTTON_DBNC_150MS 2
> > +#define MC13XXX_BUTTON_DBNC_750MS 3
> > +#define MC13XXX_BUTTON_ENABLE (1 << 2)
> > +#define MC13XXX_BUTTON_POL_INVERT (1 << 3)
> > +#define MC13XXX_BUTTON_RESET_EN (1 << 4)
> > +};
> > +
>
> Please take the opportunity to remove this slab list of #defines from
> the centre of the struct definition. Just above will be fine.
I would like to make it in a separate patch for the whole header file later.
---
^ permalink raw reply
* Re: [PATCH v2 1/3] input: appletouch: parametrize and set saner defaults for fuzz and threshold
From: Clinton Sprain @ 2014-02-11 4:46 UTC (permalink / raw)
To: Henrik Rydberg, linux-input; +Cc: Dmitry Torokhov
In-Reply-To: <52F76E13.3060408@euromail.se>
Hi Herik,
On 02/09/2014 06:01 AM, Henrik Rydberg wrote:
> Hi Clinton,
>
> On 02/07/2014 01:40 AM, Clinton Sprain wrote:
>> Dials back the default fuzz and threshold settings for most devices using this driver, based on user input from forums and bug reports, increasing sensitivity. These two settings can also now both be overridden as module parameters in the event that users have hardware that does not respond well to the new defaults, or there is a desire to change the values for devices that do not have new defaults.
>>
>> Signed-off-by: Clinton Sprain <clintonsprain@gmail.com>
>> ---
>> drivers/input/mouse/appletouch.c | 48 ++++++++++++++++++++++++--------------
>> 1 file changed, 31 insertions(+), 17 deletions(-)
>
> Thanks for the patches, they seem like a good improvement. There are still some
> comments, though, you will find them inline.
>
>> diff --git a/drivers/input/mouse/appletouch.c b/drivers/input/mouse/appletouch.c
>> index 800ca7d..d48181b 100644
>> --- a/drivers/input/mouse/appletouch.c
>> +++ b/drivers/input/mouse/appletouch.c
>> @@ -48,6 +48,8 @@ struct atp_info {
>> int yfact; /* Y multiplication factor */
>> int datalen; /* size of USB transfers */
>> void (*callback)(struct urb *); /* callback function */
>> + int fuzz; /* fuzz touchpad generates */
>> + int threshold; /* sensitivity threshold */
>> };
>>
>> static void atp_complete_geyser_1_2(struct urb *urb);
>> @@ -61,6 +63,8 @@ static const struct atp_info fountain_info = {
>> .yfact = 43,
>> .datalen = 81,
>> .callback = atp_complete_geyser_1_2,
>> + .fuzz = 16,
>> + .threshold = 5,
>> };
>>
>> static const struct atp_info geyser1_info = {
>> @@ -71,6 +75,8 @@ static const struct atp_info geyser1_info = {
>> .yfact = 43,
>> .datalen = 81,
>> .callback = atp_complete_geyser_1_2,
>> + .fuzz = 16,
>> + .threshold = 5,
>> };
>>
>> static const struct atp_info geyser2_info = {
>> @@ -81,6 +87,8 @@ static const struct atp_info geyser2_info = {
>> .yfact = 43,
>> .datalen = 64,
>> .callback = atp_complete_geyser_1_2,
>> + .fuzz = 0,
>> + .threshold = 3,
>> };
>>
>> static const struct atp_info geyser3_info = {
>> @@ -90,6 +98,8 @@ static const struct atp_info geyser3_info = {
>> .yfact = 64,
>> .datalen = 64,
>> .callback = atp_complete_geyser_3_4,
>> + .fuzz = 0,
>> + .threshold = 3,
>> };
>>
>> static const struct atp_info geyser4_info = {
>> @@ -99,6 +109,8 @@ static const struct atp_info geyser4_info = {
>> .yfact = 64,
>> .datalen = 64,
>> .callback = atp_complete_geyser_3_4,
>> + .fuzz = 0,
>> + .threshold = 3,
>> };
>>
>> #define ATP_DEVICE(prod, info) \
>> @@ -155,18 +167,9 @@ MODULE_DEVICE_TABLE(usb, atp_table);
>> #define ATP_XSENSORS 26
>> #define ATP_YSENSORS 16
>>
>> -/* amount of fuzz this touchpad generates */
>> -#define ATP_FUZZ 16
>> -
>> /* maximum pressure this driver will report */
>> #define ATP_PRESSURE 300
>>
>> -/*
>> - * Threshold for the touchpad sensors. Any change less than ATP_THRESHOLD is
>> - * ignored.
>> - */
>> -#define ATP_THRESHOLD 5
>> -
>> /* Geyser initialization constants */
>> #define ATP_GEYSER_MODE_READ_REQUEST_ID 1
>> #define ATP_GEYSER_MODE_WRITE_REQUEST_ID 9
>> @@ -235,14 +238,20 @@ MODULE_AUTHOR("Sven Anders");
>> MODULE_DESCRIPTION("Apple PowerBook and MacBook USB touchpad driver");
>> MODULE_LICENSE("GPL");
>>
>> -/*
>> - * Make the threshold a module parameter
>> - */
>> -static int threshold = ATP_THRESHOLD;
>> +static int threshold = -1;
>> module_param(threshold, int, 0644);
>> MODULE_PARM_DESC(threshold, "Discard any change in data from a sensor"
>> " (the trackpad has many of these sensors)"
>> - " less than this value.");
>> + " less than this value. Devices have defaults;"
>> + " this parameter overrides them.");
>> +static int fuzz = -1;
>> +
>> +module_param(fuzz, int, 0644);
>> +MODULE_PARM_DESC(fuzz, "Fuzz the trackpad generates. The higher"
>> + " this is, the less sensitive the trackpad"
>> + " will feel, but values too low may generate"
>> + " random movement. Devices have defaults;"
>> + " this parameter overrides them.");
>
> Given that there already is a userland interface for the fuzz parameter - and it
> is indeed in frequent use - this one does not seem warranted. If we are unsure
> if the changes to the default are not all good, perhaps they should not be
> changed at all?
Forgive my ignorance - how would I interact with this parameter in
userland? I searched a bit but didn't find anything. If it's trivial to
fiddle with then I'm OK with dropping it as a module parameter.
I'm reasonably confident the defaults should change. They are based on
positive comments from multiple people with different hardware who
recompiled the driver with these values.
The majority of the benefit they saw was likely from the smaller
threshold value, due to its influence on the return value of
atp_calculate_abs. The second patch of this set takes it completely out
of the equation; however, there are still some small benefits to be
gained from changing these defaults. The threshold is still used for
finger count calculation and benefits from the increased sensitivity,
and the fuzz setting seems to inhibit more granular (1-2 px) cursor
movements.
>
>>
>> static int debug;
>> module_param(debug, int, 0644);
>> @@ -455,7 +464,7 @@ static void atp_detect_size(struct atp *dev)
>> input_set_abs_params(dev->input, ABS_X, 0,
>> (dev->info->xsensors_17 - 1) *
>> dev->info->xfact - 1,
>> - ATP_FUZZ, 0);
>> + fuzz, 0);
>> break;
>> }
>> }
>> @@ -808,6 +817,11 @@ static int atp_probe(struct usb_interface *iface,
>> dev->info = info;
>> dev->overflow_warned = false;
>>
>> + if (fuzz < 0)
>> + fuzz = dev->info->fuzz;
>> + if (threshold < 0)
>> + threshold = dev->info->threshold;
>> +
>> dev->urb = usb_alloc_urb(0, GFP_KERNEL);
>> if (!dev->urb)
>> goto err_free_devs;
>> @@ -843,10 +857,10 @@ static int atp_probe(struct usb_interface *iface,
>>
>> input_set_abs_params(input_dev, ABS_X, 0,
>> (dev->info->xsensors - 1) * dev->info->xfact - 1,
>> - ATP_FUZZ, 0);
>> + fuzz, 0);
>> input_set_abs_params(input_dev, ABS_Y, 0,
>> (dev->info->ysensors - 1) * dev->info->yfact - 1,
>> - ATP_FUZZ, 0);
>> + fuzz, 0);
>> input_set_abs_params(input_dev, ABS_PRESSURE, 0, ATP_PRESSURE, 0, 0);
>>
>> set_bit(EV_KEY, input_dev->evbit);
>>
>
> Thanks,
> Henrik
>
Thanks,
Clinton
^ permalink raw reply
* Re: [PATCH 0/7] Remove HAVE_PWM config option
From: Jingoo Han @ 2014-02-11 3:02 UTC (permalink / raw)
To: 'Arnd Bergmann', 'Ralf Baechle',
'Dmitry Torokhov', 'Thierry Reding'
Cc: 'Linus Walleij', 'Russell King - ARM Linux',
'Eric Miao', linux-kernel, linux-arm-kernel, linux-mips,
linux-input, linux-pwm, 'Jingoo Han',
'Sascha Hauer', 'Roland Stigge'
In-Reply-To: <003901cf25fc$73002790$590076b0$%han@samsung.com>
On Monday, February 10, 2014 10:07 AM, Jingoo Han wrote:
>
> The HAVE_PWM symbol is only for legacy platforms that provide
> the PWM API without using the generic framework, while PWM symbol
> is used for PWM drivers using the generic PWM framework.
>
> I looked at all HAVE_PWMs in the latest mainline kernel 3.14-rc1.
> Three platforms are still using HAVE_PWM as below:
>
> 1. ARM - PXA
> ./arch/arm/mach-pxa/Kconfig
>
> 2. ARM - NXP LPC32XX
> ./arch/arm/Kconfig
> config ARCH_LPC32XX
> select HAVE_PWM
>
> 3. MIPS - Ingenic JZ4740 based machines
> ./arch/mips/Kconfig
> config MACH_JZ4740
> select HAVE_PWM
>
> However, the legacy PWM drivers for PXA, LPC32XX, and JZ474 were
> already moved to the generic PWM framework.
> ./drivers/pwm/pwm-pxa.c
> ./drivers/pwm/pwm-lpc32xx.c
> ./drivers/pwm/pwm-jz4740.c
>
> In conclusion, HAVE_PWM should be removed, because HAVE_PWM is
> NOT required anymore.
>
> Jingoo Han (7):
> ARM: pxa: don't select HAVE_PWM
> ARM: lpc32xx: don't select HAVE_PWM
> ARM: remove HAVE_PWM config option
> MIPS: jz4740: don't select HAVE_PWM
> Input: max8997_haptic: remove HAVE_PWM dependencies
> Input: pwm-beepe: remove HAVE_PWM dependencies
> pwm: don't use IS_ENABLED(CONFIG_HAVE_PWM)
>
> arch/arm/Kconfig | 4 ----
> arch/arm/mach-pxa/Kconfig | 15 ---------------
> arch/mips/Kconfig | 1 -
> drivers/input/misc/Kconfig | 4 ++--
> include/linux/pwm.h | 2 +-
> 5 files changed, 3 insertions(+), 23 deletions(-)
(+cc Sascha Hauer, Roland Stigge)
The same patch was already submitted by Sascha Hauer. [1]
So, please ignore this patch. Thank you.
[1] https://lkml.org/lkml/2014/1/16/262
Best regards,
Jingoo Han
>
> I would like to merge these patches as below:
>
> 1. Through arm-soc tree
> [PATCH 1/7] ARM: pxa: don't select HAVE_PWM
> [PATCH 2/7] ARM: lpc32xx: don't select HAVE_PWM
> [PATCH 3/7] ARM: remove HAVE_PWM config option
>
> 2. Through MIPS tree
> [PATCH 4/7] MIPS: jz4740: don't select HAVE_PWM
>
> 3. Through Input tree
> [PATCH 5/7] Input: max8997_haptic: remove HAVE_PWM dependencies
> [PATCH 6/7] Input: pwm-beepe: remove HAVE_PWM dependencies
>
> 4. Through PWM tree
> [PATCH 7/7] pwm: don't use IS_ENABLED(CONFIG_HAVE_PWM)
>
> After merging these patches, all HAVE_PWM will be removed from
> the mainline kernel. Thank you. :-)
>
> Best regards,
> Jingoo Han
^ 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