* Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix
From: Courtney Cavin @ 2014-02-12 21:48 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Christopher Heiny, 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: <20140212064049.GA15855@core.coreip.homeip.net>
On Wed, Feb 12, 2014 at 07:40:49AM +0100, Dmitry Torokhov wrote:
> Hi Chris,
>
> On Tue, Feb 11, 2014 at 03:13:00PM -0800, 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");
> > 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);
>
> As Courtney mentioned if you are calling devm_kfree() you are most
> likely doing something wrong.
>
> How about the patch below? Please check the XXX comment, I have some
> concerns about lts vs doze_holdoff check mismatch in probe() and
> config().
>
> Thanks.
>
> --
> Dmitry
>
> Input: synaptics-rmi4 - F01 initialization cleanup
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> - rename data to f01 where appropriate;
> - switch to using rmi_read()/rmi_write() for single-byte data;
> - allocate interrupt mask together with the main structure;
> - do not kfree() memory allocated with devm;
> - do not write config data in probe(), we have config() for that;
> - drop unneeded rmi_f01_remove().
These seem like unrelated changes and make this patch hard to read, I
would prefer if we could separate these out. Perhaps like so?
[1] bug-fix
- do not kfree() memory allocated with devm
[2] simplify probe/remove logic
- allocate interrupt mask together with the main structure
- do not write config data in probe(), we have config() for that
- drop unneeded rmi_f01_remove()
[3] non-behavioral changes/cleanup
- switch to using rmi_read()/rmi_write() for single-byte data
- rename data to f01 where appropriate
Disregarding that, and the nitpick below, it looks good to me.
>
> Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/rmi4/rmi_f01.c | 397 ++++++++++++++++++------------------------
> 1 file changed, 172 insertions(+), 225 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> index 381ad60..8f7840e 100644
> --- a/drivers/input/rmi4/rmi_f01.c
> +++ b/drivers/input/rmi4/rmi_f01.c
[...]
> -static int rmi_f01_initialize(struct rmi_function *fn)
> +static int rmi_f01_probe(struct rmi_function *fn)
> {
> - u8 temp;
> - int error;
> - u16 ctrl_base_addr;
> struct rmi_device *rmi_dev = fn->rmi_dev;
> struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> - struct f01_data *data = fn->data;
> - struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
> + const struct rmi_device_platform_data *pdata =
> + to_rmi_platform_data(rmi_dev);
> + struct f01_data *f01;
> + size_t f01_size;
> + int error;
> + u16 ctrl_base_addr;
> + u8 device_status;
> + u8 temp;
> +
> + f01_size = sizeof(struct f01_data) +
> + sizeof(u8) * driver_data->num_of_irq_regs;
> + f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
> + if (!f01) {
> + dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
Nitpick: Can we drop this printout in the process? It's much less
useful than the error and backtrace coming from kmalloc on failure anyway.
> + return -ENOMEM;
> + }
[...]
> + /* XXX: why we check has_lts here but has_adjustable_doze in probe? */
Hrm. This register is poorly documented in the spec. All of these bits
are reserved. Chris, is there a newer version of the spec which
documents these bits?
-Courtney
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix
From: Christopher Heiny @ 2014-02-12 23:08 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, David Herrmann, Jiri Kosina, Courtney Cavin
In-Reply-To: <20140212064049.GA15855@core.coreip.homeip.net>
On 02/11/2014 10:40 PM, Dmitry Torokhov wrote:
> Hi Chris,
>
> On Tue, Feb 11, 2014 at 03:13:00PM -0800, 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");
>> > 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);
> As Courtney mentioned if you are calling devm_kfree() you are most
> likely doing something wrong.
>
> How about the patch below? Please check the XXX comment, I have some
> concerns about lts vs doze_holdoff check mismatch in probe() and
> config().
Thanks for calling attention to the has_lts vs doze_holdoff check.
There was actually a bug in two places relating to that. It looks like
a case of cut/paste dyslexia (or something like that). Anyway, here's a
version of your patch with those two bugs fixed. I've tested this on
Nexus 4.
Chris
Input: synaptics-rmi4 - F01 initialization cleanup
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
- rename data to f01 where appropriate;
- switch to using rmi_read()/rmi_write() for single-byte data;
- allocate interrupt mask together with the main structure;
- do not kfree() memory allocated with devm;
- do not write config data in probe(), we have config() for that;
- drop unneeded rmi_f01_remove().
- correctly calculate control register address based on has_lts
- correctly write adjustable doze control registers
Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
Reported-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
---
drivers/input/rmi4/rmi_f01.c | 400
+++++++++++++++++++------------------------
1 file changed, 173 insertions(+), 227 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 381ad60..9932548 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -123,47 +123,21 @@ struct f01_device_control {
struct f01_data {
struct f01_basic_properties properties;
-
struct f01_device_control device_control;
- struct mutex control_mutex;
-
- u8 device_status;
u16 interrupt_enable_addr;
u16 doze_interval_addr;
u16 wakeup_threshold_addr;
u16 doze_holdoff_addr;
- int irq_count;
- int num_of_irq_regs;
#ifdef CONFIG_PM_SLEEP
bool suspended;
bool old_nosleep;
#endif
-};
-
-static int rmi_f01_alloc_memory(struct rmi_function *fn,
- int num_of_irq_regs)
-{
- struct f01_data *f01;
-
- f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
- if (!f01) {
- dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
- return -ENOMEM;
- }
-
- f01->device_control.interrupt_enable = devm_kzalloc(&fn->dev,
- sizeof(u8) * (num_of_irq_regs),
- GFP_KERNEL);
- if (!f01->device_control.interrupt_enable) {
- dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
- return -ENOMEM;
- }
- fn->data = f01;
- return 0;
-}
+ unsigned int num_of_irq_regs;
+ u8 interrupt_enable[];
+};
static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
u16 query_base_addr,
@@ -174,8 +148,9 @@ static int rmi_f01_read_properties(struct rmi_device
*rmi_dev,
error = rmi_read_block(rmi_dev, query_base_addr,
basic_query, sizeof(basic_query));
- if (error < 0) {
- dev_err(&rmi_dev->dev, "Failed to read device query registers.\n");
+ if (error) {
+ dev_err(&rmi_dev->dev,
+ "Failed to read device query registers: %d\n", error);
return error;
}
@@ -204,38 +179,51 @@ static int rmi_f01_read_properties(struct
rmi_device *rmi_dev,
return 0;
}
-static int rmi_f01_initialize(struct rmi_function *fn)
+static int rmi_f01_probe(struct rmi_function *fn)
{
- u8 temp;
- int error;
- u16 ctrl_base_addr;
struct rmi_device *rmi_dev = fn->rmi_dev;
struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
- struct f01_data *data = fn->data;
- struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ const struct rmi_device_platform_data *pdata =
+ to_rmi_platform_data(rmi_dev);
+ struct f01_data *f01;
+ size_t f01_size;
+ int error;
+ u16 ctrl_base_addr;
+ u8 device_status;
+ u8 temp;
- mutex_init(&data->control_mutex);
+ f01_size = sizeof(struct f01_data) +
+ sizeof(u8) * driver_data->num_of_irq_regs;
+ f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
+ if (!f01) {
+ dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
+ return -ENOMEM;
+ }
+
+ f01->num_of_irq_regs = driver_data->num_of_irq_regs;
+ f01->device_control.interrupt_enable = f01->interrupt_enable;
/*
* Set the configured bit and (optionally) other important stuff
* in the device control register.
*/
ctrl_base_addr = fn->fd.control_base_addr;
- error = rmi_read_block(rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
- if (error < 0) {
- dev_err(&fn->dev, "Failed to read F01 control.\n");
+
+ error = rmi_read(rmi_dev, fn->fd.control_base_addr,
+ &f01->device_control.ctrl0);
+ if (error) {
+ dev_err(&fn->dev, "Failed to read F01 control: %d\n", error);
return error;
}
+
switch (pdata->power_management.nosleep) {
case RMI_F01_NOSLEEP_DEFAULT:
break;
case RMI_F01_NOSLEEP_OFF:
- data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
break;
case RMI_F01_NOSLEEP_ON:
- data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
break;
}
@@ -244,250 +232,212 @@ static int rmi_f01_initialize(struct
rmi_function *fn)
* reboot without power cycle. If so, clear it so the sensor
* is certain to function.
*/
- if ((data->device_control.ctrl0 & RMI_F01_CTRL0_SLEEP_MODE_MASK) !=
+ if ((f01->device_control.ctrl0 & RMI_F01_CTRL0_SLEEP_MODE_MASK) !=
RMI_SLEEP_MODE_NORMAL) {
dev_warn(&fn->dev,
"WARNING: Non-zero sleep mode found. Clearing...\n");
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
}
- data->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
- error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
- if (error < 0) {
- dev_err(&fn->dev, "Failed to write F01 control.\n");
+ error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+ f01->device_control.ctrl0);
+ if (error) {
+ dev_err(&fn->dev, "Failed to write F01 control: %d\n", error);
return error;
}
- data->irq_count = driver_data->irq_count;
- data->num_of_irq_regs = driver_data->num_of_irq_regs;
- ctrl_base_addr += sizeof(u8);
+ f01->num_of_irq_regs = driver_data->num_of_irq_regs;
+ ctrl_base_addr += sizeof(f01->device_control.ctrl0);
- data->interrupt_enable_addr = ctrl_base_addr;
+ f01->interrupt_enable_addr = ctrl_base_addr;
error = rmi_read_block(rmi_dev, ctrl_base_addr,
- data->device_control.interrupt_enable,
- sizeof(u8) * (data->num_of_irq_regs));
- if (error < 0) {
+ f01->device_control.interrupt_enable,
+ sizeof(u8) * (f01->num_of_irq_regs));
+ if (error) {
dev_err(&fn->dev,
- "Failed to read F01 control interrupt enable register.\n");
- goto error_exit;
+ "Failed to read F01 control interrupt enable register: %d\n",
+ error);
+ return error;
}
- ctrl_base_addr += data->num_of_irq_regs;
+ ctrl_base_addr += f01->num_of_irq_regs;
/* dummy read in order to clear irqs */
error = rmi_read(rmi_dev, fn->fd.data_base_addr + 1, &temp);
- if (error < 0) {
+ if (error) {
dev_err(&fn->dev, "Failed to read Interrupt Status.\n");
return error;
}
error = rmi_f01_read_properties(rmi_dev, fn->fd.query_base_addr,
- &data->properties);
- if (error < 0) {
+ &f01->properties);
+ if (error) {
dev_err(&fn->dev, "Failed to read F01 properties.\n");
return error;
}
+
dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s\n",
- data->properties.manufacturer_id == 1 ?
- "Synaptics" : "unknown",
- data->properties.product_id);
+ f01->properties.manufacturer_id == 1 ? "Synaptics" : "unknown",
+ f01->properties.product_id);
/* read control register */
- if (data->properties.has_adjustable_doze) {
- data->doze_interval_addr = ctrl_base_addr;
+ if (f01->properties.has_adjustable_doze) {
+ f01->doze_interval_addr = ctrl_base_addr;
ctrl_base_addr++;
if (pdata->power_management.doze_interval) {
- data->device_control.doze_interval =
+ f01->device_control.doze_interval =
pdata->power_management.doze_interval;
- error = rmi_write(rmi_dev, data->doze_interval_addr,
- data->device_control.doze_interval);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to configure F01 doze interval register.\n");
- goto error_exit;
- }
} else {
- error = rmi_read(rmi_dev, data->doze_interval_addr,
- &data->device_control.doze_interval);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to read F01 doze interval register.\n");
- goto error_exit;
+ error = rmi_read(rmi_dev, f01->doze_interval_addr,
+ &f01->device_control.doze_interval);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read F01 doze interval register: %d\n",
+ error);
+ return error;
}
}
- data->wakeup_threshold_addr = ctrl_base_addr;
+ f01->wakeup_threshold_addr = ctrl_base_addr;
ctrl_base_addr++;
if (pdata->power_management.wakeup_threshold) {
- data->device_control.wakeup_threshold =
+ f01->device_control.wakeup_threshold =
pdata->power_management.wakeup_threshold;
- error = rmi_write(rmi_dev, data->wakeup_threshold_addr,
- data->device_control.wakeup_threshold);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to configure F01 wakeup threshold
register.\n");
- goto error_exit;
- }
} else {
- error = rmi_read(rmi_dev, data->wakeup_threshold_addr,
- &data->device_control.wakeup_threshold);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to read F01 wakeup threshold register.\n");
- goto error_exit;
+ error = rmi_read(rmi_dev, f01->wakeup_threshold_addr,
+ &f01->device_control.wakeup_threshold);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read F01 wakeup threshold register: %d\n",
+ error);
+ return error;
}
}
}
- if (data->properties.has_adjustable_doze_holdoff) {
- data->doze_holdoff_addr = ctrl_base_addr;
+ if (f01->properties.has_lts)
+ ctrl_base_addr++;
+
+ if (f01->properties.has_adjustable_doze_holdoff) {
+ f01->doze_holdoff_addr = ctrl_base_addr;
ctrl_base_addr++;
if (pdata->power_management.doze_holdoff) {
- data->device_control.doze_holdoff =
+ f01->device_control.doze_holdoff =
pdata->power_management.doze_holdoff;
- error = rmi_write(rmi_dev, data->doze_holdoff_addr,
- data->device_control.doze_holdoff);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to configure F01 doze holdoff register.\n");
- goto error_exit;
- }
} else {
- error = rmi_read(rmi_dev, data->doze_holdoff_addr,
- &data->device_control.doze_holdoff);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to read F01 doze holdoff register.\n");
- goto error_exit;
+ error = rmi_read(rmi_dev, f01->doze_holdoff_addr,
+ &f01->device_control.doze_holdoff);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read F01 doze holdoff register: %d\n",
+ error);
+ return error;
}
}
}
- error = rmi_read_block(rmi_dev, fn->fd.data_base_addr,
- &data->device_status, sizeof(data->device_status));
- if (error < 0) {
- dev_err(&fn->dev, "Failed to read device status.\n");
- goto error_exit;
+ error = rmi_read(rmi_dev, fn->fd.data_base_addr, &device_status);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read device status: %d\n", error);
+ return error;
}
- if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
+ if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
dev_err(&fn->dev,
"Device was reset during configuration process, status: %#02x!\n",
- RMI_F01_STATUS_CODE(data->device_status));
- error = -EINVAL;
- goto error_exit;
+ RMI_F01_STATUS_CODE(device_status));
+ return -EINVAL;
}
- return 0;
+ fn->data = f01;
- error_exit:
- kfree(data);
- return error;
+ return 0;
}
static int rmi_f01_config(struct rmi_function *fn)
{
- struct f01_data *data = fn->data;
- int retval;
-
- retval = rmi_write_block(fn->rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write device_control.reg.\n");
- return retval;
- }
-
- retval = rmi_write_block(fn->rmi_dev, data->interrupt_enable_addr,
- data->device_control.interrupt_enable,
- sizeof(u8) * data->num_of_irq_regs);
+ struct f01_data *f01 = fn->data;
+ int error;
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write interrupt enable.\n");
- return retval;
+ error = rmi_write(fn->rmi_dev, fn->fd.control_base_addr,
+ f01->device_control.ctrl0);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write device_control reg: %d\n", error);
+ return error;
}
- if (data->properties.has_lts) {
- retval = rmi_write_block(fn->rmi_dev, data->doze_interval_addr,
- &data->device_control.doze_interval,
- sizeof(u8));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write doze interval.\n");
- return retval;
- }
+
+ error = rmi_write_block(fn->rmi_dev, f01->interrupt_enable_addr,
+ f01->interrupt_enable,
+ sizeof(u8) * f01->num_of_irq_regs);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write interrupt enable: %d\n", error);
+ return error;
}
- if (data->properties.has_adjustable_doze) {
- retval = rmi_write_block(fn->rmi_dev,
- data->wakeup_threshold_addr,
- &data->device_control.wakeup_threshold,
- sizeof(u8));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write wakeup threshold.\n");
- return retval;
+ if (f01->properties.has_adjustable_doze) {
+ error = rmi_write(fn->rmi_dev, f01->doze_interval_addr,
+ f01->device_control.doze_interval);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write doze interval: %d\n", error);
+ return error;
+ }
+ error = rmi_write(fn->rmi_dev, f01->wakeup_threshold_addr,
+ f01->device_control.wakeup_threshold);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write wakeup threshold: %d\n",
+ error);
+ return error;
}
}
- if (data->properties.has_adjustable_doze_holdoff) {
- retval = rmi_write_block(fn->rmi_dev, data->doze_holdoff_addr,
- &data->device_control.doze_holdoff,
- sizeof(u8));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write doze holdoff.\n");
- return retval;
+ if (f01->properties.has_adjustable_doze_holdoff) {
+ error = rmi_write(fn->rmi_dev, f01->doze_holdoff_addr,
+ f01->device_control.doze_holdoff);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write doze holdoff: %d\n", error);
+ return error;
}
}
- return 0;
-}
-
-static int rmi_f01_probe(struct rmi_function *fn)
-{
- struct rmi_driver_data *driver_data =
- dev_get_drvdata(&fn->rmi_dev->dev);
- int error;
-
- error = rmi_f01_alloc_memory(fn, driver_data->num_of_irq_regs);
- if (error)
- return error;
-
- error = rmi_f01_initialize(fn);
- if (error)
- return error;
return 0;
}
-static void rmi_f01_remove(struct rmi_function *fn)
-{
- /* Placeholder for now. */
-}
-
#ifdef CONFIG_PM_SLEEP
static int rmi_f01_suspend(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *data = fn->data;
+ struct f01_data *f01 = fn->data;
int error;
- data->old_nosleep = data->device_control.ctrl0 &
- RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->old_nosleep =
+ f01->device_control.ctrl0 & RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
- data->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
+ f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
- error = rmi_write_block(rmi_dev,
- fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
- if (error < 0) {
- dev_err(&fn->dev, "Failed to write sleep mode. Code: %d.\n",
- error);
- if (data->old_nosleep)
- data->device_control.ctrl0 |=
- RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
- data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
+
+ error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+ f01->device_control.ctrl0);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write sleep mode: %d.\n", error);
+ if (f01->old_nosleep)
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
return error;
}
@@ -498,22 +448,20 @@ static int rmi_f01_resume(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *data = fn->data;
+ struct f01_data *f01 = fn->data;
int error;
- if (data->old_nosleep)
- data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+ if (f01->old_nosleep)
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
- data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
- error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
- if (error < 0) {
+ error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+ f01->device_control.ctrl0);
+ if (error) {
dev_err(&fn->dev,
- "Failed to restore normal operation. Code: %d.\n",
- error);
+ "Failed to restore normal operation: %d.\n", error);
return error;
}
@@ -527,22 +475,21 @@ static int rmi_f01_attention(struct rmi_function *fn,
unsigned long *irq_bits)
{
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *data = fn->data;
- int retval;
-
- retval = rmi_read_block(rmi_dev, fn->fd.data_base_addr,
- &data->device_status, sizeof(data->device_status));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to read device status, code: %d.\n",
- retval);
- return retval;
+ int error;
+ u8 device_status;
+
+ error = rmi_read(rmi_dev, fn->fd.data_base_addr, &device_status);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read device status: %d.\n", error);
+ return error;
}
- if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
+ if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
dev_warn(&fn->dev, "Device reset detected.\n");
- retval = rmi_dev->driver->reset_handler(rmi_dev);
- if (retval < 0)
- return retval;
+ error = rmi_dev->driver->reset_handler(rmi_dev);
+ if (error < 0)
+ return error;
}
return 0;
}
@@ -559,7 +506,6 @@ static struct rmi_function_handler rmi_f01_handler = {
},
.func = 0x01,
.probe = rmi_f01_probe,
- .remove = rmi_f01_remove,
.config = rmi_f01_config,
.attention = rmi_f01_attention,
};
^ permalink raw reply related
* Re: [PATCH] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Dmitry Torokhov @ 2014-02-12 23:11 UTC (permalink / raw)
To: Barry Song
Cc: linux-input, linux-arm-kernel, workgroup.linux, Xianglong Du,
Rongjun Ying, Barry Song
In-Reply-To: <1392026859-4977-1-git-send-email-21cnbao@gmail.com>
Hi Barry,
On Mon, Feb 10, 2014 at 06:07:39PM +0800, Barry Song wrote:
>
> static int sirfsoc_pwrc_remove(struct platform_device *pdev)
> {
> + struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(&pdev->dev);
> +
> device_init_wakeup(&pdev->dev, 0);
>
> + cancel_delayed_work_sync(&pwrcdrv->work);
> +
This is racy: interrupt is freed later and can schedule work again.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix
From: Christopher Heiny @ 2014-02-12 23:21 UTC (permalink / raw)
To: Courtney Cavin, Dmitry Torokhov
Cc: 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: <20140212214819.GE1706@sonymobile.com>
On 02/12/2014 01:48 PM, Courtney Cavin wrote:
> On Wed, Feb 12, 2014 at 07:40:49AM +0100, Dmitry Torokhov wrote:
>> Hi Chris,
>>
>> On Tue, Feb 11, 2014 at 03:13:00PM -0800, 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");
>>> 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);
>>
>> As Courtney mentioned if you are calling devm_kfree() you are most
>> likely doing something wrong.
>>
>> How about the patch below? Please check the XXX comment, I have some
>> concerns about lts vs doze_holdoff check mismatch in probe() and
>> config().
>>
>> Thanks.
>>
>> --
>> Dmitry
>>
>> Input: synaptics-rmi4 - F01 initialization cleanup
>>
>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> - rename data to f01 where appropriate;
>> - switch to using rmi_read()/rmi_write() for single-byte data;
>> - allocate interrupt mask together with the main structure;
>> - do not kfree() memory allocated with devm;
>> - do not write config data in probe(), we have config() for that;
>> - drop unneeded rmi_f01_remove().
>
> These seem like unrelated changes and make this patch hard to read, I
> would prefer if we could separate these out. Perhaps like so?
> [1] bug-fix
> - do not kfree() memory allocated with devm
> [2] simplify probe/remove logic
> - allocate interrupt mask together with the main structure
> - do not write config data in probe(), we have config() for that
> - drop unneeded rmi_f01_remove()
> [3] non-behavioral changes/cleanup
> - switch to using rmi_read()/rmi_write() for single-byte data
> - rename data to f01 where appropriate
>
> Disregarding that, and the nitpick below, it looks good to me.
Hi,
This arrived a few seconds after I sent my reply. Looks like mail is
slow today.
I am OK to either split or lump the patch - Dmitry can make that call.
>
>>
>> Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>> drivers/input/rmi4/rmi_f01.c | 397 ++++++++++++++++++------------------------
>> 1 file changed, 172 insertions(+), 225 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
>> index 381ad60..8f7840e 100644
>> --- a/drivers/input/rmi4/rmi_f01.c
>> +++ b/drivers/input/rmi4/rmi_f01.c
> [...]
>> -static int rmi_f01_initialize(struct rmi_function *fn)
>> +static int rmi_f01_probe(struct rmi_function *fn)
>> {
>> - u8 temp;
>> - int error;
>> - u16 ctrl_base_addr;
>> struct rmi_device *rmi_dev = fn->rmi_dev;
>> struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
>> - struct f01_data *data = fn->data;
>> - struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
>> + const struct rmi_device_platform_data *pdata =
>> + to_rmi_platform_data(rmi_dev);
>> + struct f01_data *f01;
>> + size_t f01_size;
>> + int error;
>> + u16 ctrl_base_addr;
>> + u8 device_status;
>> + u8 temp;
>> +
>> + f01_size = sizeof(struct f01_data) +
>> + sizeof(u8) * driver_data->num_of_irq_regs;
>> + f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
>> + if (!f01) {
>> + dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
>
> Nitpick: Can we drop this printout in the process? It's much less
> useful than the error and backtrace coming from kmalloc on failure anyway.
We print messages like that in a lot of places. Based on your prior
comments, I figured to do a blanket up date that removes all of those at
once across the driver. Would that be an OK solution?
>
>> + return -ENOMEM;
>> + }
> [...]
>
>> + /* XXX: why we check has_lts here but has_adjustable_doze in probe? */
>
> Hrm. This register is poorly documented in the spec. All of these bits
> are reserved. Chris, is there a newer version of the spec which
> documents these bits?
Unfortunately, no. I've filed a bug on that. In the meantime, I've
found the following:
* It looks like there's a control register F01_RMI_Ctrl4 which is
present if the has_lts bit is set, but is not used in any shipped LTS
products.
* Both F01_RMI_Ctrl2 and F01_RMI_Ctrl3 (doze_interval and
wakeup_threshold) are controlled by the has_adjustable_doze bit.
The patch I sent a bit ago includes fixes based on this info.
Thanks,
Chris
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix
From: Dmitry Torokhov @ 2014-02-12 23:28 UTC (permalink / raw)
To: Courtney Cavin
Cc: Christopher Heiny, 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: <20140212214819.GE1706@sonymobile.com>
On Wed, Feb 12, 2014 at 01:48:19PM -0800, Courtney Cavin wrote:
> On Wed, Feb 12, 2014 at 07:40:49AM +0100, Dmitry Torokhov wrote:
> > Hi Chris,
> >
> > On Tue, Feb 11, 2014 at 03:13:00PM -0800, 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");
> > > 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);
> >
> > As Courtney mentioned if you are calling devm_kfree() you are most
> > likely doing something wrong.
> >
> > How about the patch below? Please check the XXX comment, I have some
> > concerns about lts vs doze_holdoff check mismatch in probe() and
> > config().
> >
> > Thanks.
> >
> > --
> > Dmitry
> >
> > Input: synaptics-rmi4 - F01 initialization cleanup
> >
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > - rename data to f01 where appropriate;
> > - switch to using rmi_read()/rmi_write() for single-byte data;
> > - allocate interrupt mask together with the main structure;
> > - do not kfree() memory allocated with devm;
> > - do not write config data in probe(), we have config() for that;
> > - drop unneeded rmi_f01_remove().
>
> These seem like unrelated changes and make this patch hard to read, I
> would prefer if we could separate these out. Perhaps like so?
> [1] bug-fix
> - do not kfree() memory allocated with devm
> [2] simplify probe/remove logic
> - allocate interrupt mask together with the main structure
> - do not write config data in probe(), we have config() for that
> - drop unneeded rmi_f01_remove()
> [3] non-behavioral changes/cleanup
> - switch to using rmi_read()/rmi_write() for single-byte data
> - rename data to f01 where appropriate
>
> Disregarding that, and the nitpick below, it looks good to me.
OK, I can do that...
>
> >
> > Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/input/rmi4/rmi_f01.c | 397 ++++++++++++++++++------------------------
> > 1 file changed, 172 insertions(+), 225 deletions(-)
> >
> > diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> > index 381ad60..8f7840e 100644
> > --- a/drivers/input/rmi4/rmi_f01.c
> > +++ b/drivers/input/rmi4/rmi_f01.c
> [...]
> > -static int rmi_f01_initialize(struct rmi_function *fn)
> > +static int rmi_f01_probe(struct rmi_function *fn)
> > {
> > - u8 temp;
> > - int error;
> > - u16 ctrl_base_addr;
> > struct rmi_device *rmi_dev = fn->rmi_dev;
> > struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> > - struct f01_data *data = fn->data;
> > - struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
> > + const struct rmi_device_platform_data *pdata =
> > + to_rmi_platform_data(rmi_dev);
> > + struct f01_data *f01;
> > + size_t f01_size;
> > + int error;
> > + u16 ctrl_base_addr;
> > + u8 device_status;
> > + u8 temp;
> > +
> > + f01_size = sizeof(struct f01_data) +
> > + sizeof(u8) * driver_data->num_of_irq_regs;
> > + f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
> > + if (!f01) {
> > + dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
>
> Nitpick: Can we drop this printout in the process? It's much less
> useful than the error and backtrace coming from kmalloc on failure anyway.
Actually I like messages: who knows when we decided to change kmalloc
behavior and it also helps when there are several allocations in the
same function to know which one failed.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix
From: Courtney Cavin @ 2014-02-12 23:35 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: <52FC0202.7020003@synaptics.com>
On Thu, Feb 13, 2014 at 12:21:38AM +0100, Christopher Heiny wrote:
> On 02/12/2014 01:48 PM, Courtney Cavin wrote:
> > On Wed, Feb 12, 2014 at 07:40:49AM +0100, Dmitry Torokhov wrote:
> >> Hi Chris,
> >>
> >> On Tue, Feb 11, 2014 at 03:13:00PM -0800, Christopher Heiny wrote:
> > [...]
> >> -static int rmi_f01_initialize(struct rmi_function *fn)
> >> +static int rmi_f01_probe(struct rmi_function *fn)
> >> {
> >> - u8 temp;
> >> - int error;
> >> - u16 ctrl_base_addr;
> >> struct rmi_device *rmi_dev = fn->rmi_dev;
> >> struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> >> - struct f01_data *data = fn->data;
> >> - struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
> >> + const struct rmi_device_platform_data *pdata =
> >> + to_rmi_platform_data(rmi_dev);
> >> + struct f01_data *f01;
> >> + size_t f01_size;
> >> + int error;
> >> + u16 ctrl_base_addr;
> >> + u8 device_status;
> >> + u8 temp;
> >> +
> >> + f01_size = sizeof(struct f01_data) +
> >> + sizeof(u8) * driver_data->num_of_irq_regs;
> >> + f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
> >> + if (!f01) {
> >> + dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
> >
> > Nitpick: Can we drop this printout in the process? It's much less
> > useful than the error and backtrace coming from kmalloc on failure anyway.
>
> We print messages like that in a lot of places. Based on your prior
> comments, I figured to do a blanket up date that removes all of those at
> once across the driver. Would that be an OK solution?
It's not really necessary to do a bulk cleanup of this, as it's not a
huge thing. I just thought that since we were changing the text anyway, we
might as well get rid of the unnecessary stuff. Dmitry's new email in this
thread settles the issue though, he prefers it, so it stays.
> >> + return -ENOMEM;
> >> + }
> > [...]
> >
> >> + /* XXX: why we check has_lts here but has_adjustable_doze in probe? */
> >
> > Hrm. This register is poorly documented in the spec. All of these bits
> > are reserved. Chris, is there a newer version of the spec which
> > documents these bits?
>
> Unfortunately, no. I've filed a bug on that. In the meantime, I've
> found the following:
>
> * It looks like there's a control register F01_RMI_Ctrl4 which is
> present if the has_lts bit is set, but is not used in any shipped LTS
> products.
>
> * Both F01_RMI_Ctrl2 and F01_RMI_Ctrl3 (doze_interval and
> wakeup_threshold) are controlled by the has_adjustable_doze bit.
>
> The patch I sent a bit ago includes fixes based on this info.
Ah, OK. Thanks for the info!
-Courtney
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix
From: Courtney Cavin @ 2014-02-13 0:04 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Christopher Heiny, 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: <20140212232800.GB29769@core.coreip.homeip.net>
On Thu, Feb 13, 2014 at 12:28:00AM +0100, Dmitry Torokhov wrote:
> On Wed, Feb 12, 2014 at 01:48:19PM -0800, Courtney Cavin wrote:
> > On Wed, Feb 12, 2014 at 07:40:49AM +0100, Dmitry Torokhov wrote:
> > > Hi Chris,
> > >
> > > On Tue, Feb 11, 2014 at 03:13:00PM -0800, 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");
> > > > 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);
> > >
> > > As Courtney mentioned if you are calling devm_kfree() you are most
> > > likely doing something wrong.
> > >
> > > How about the patch below? Please check the XXX comment, I have some
> > > concerns about lts vs doze_holdoff check mismatch in probe() and
> > > config().
> > >
> > > Thanks.
> > >
> > > --
> > > Dmitry
> > >
> > > Input: synaptics-rmi4 - F01 initialization cleanup
> > >
> > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > >
> > > - rename data to f01 where appropriate;
> > > - switch to using rmi_read()/rmi_write() for single-byte data;
> > > - allocate interrupt mask together with the main structure;
> > > - do not kfree() memory allocated with devm;
> > > - do not write config data in probe(), we have config() for that;
> > > - drop unneeded rmi_f01_remove().
> >
> > These seem like unrelated changes and make this patch hard to read, I
> > would prefer if we could separate these out. Perhaps like so?
> > [1] bug-fix
> > - do not kfree() memory allocated with devm
> > [2] simplify probe/remove logic
> > - allocate interrupt mask together with the main structure
> > - do not write config data in probe(), we have config() for that
> > - drop unneeded rmi_f01_remove()
> > [3] non-behavioral changes/cleanup
> > - switch to using rmi_read()/rmi_write() for single-byte data
> > - rename data to f01 where appropriate
> >
> > Disregarding that, and the nitpick below, it looks good to me.
>
> OK, I can do that...
>
> >
> > >
> > > Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > > drivers/input/rmi4/rmi_f01.c | 397 ++++++++++++++++++------------------------
> > > 1 file changed, 172 insertions(+), 225 deletions(-)
> > >
> > > diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> > > index 381ad60..8f7840e 100644
> > > --- a/drivers/input/rmi4/rmi_f01.c
> > > +++ b/drivers/input/rmi4/rmi_f01.c
> > [...]
> > > -static int rmi_f01_initialize(struct rmi_function *fn)
> > > +static int rmi_f01_probe(struct rmi_function *fn)
> > > {
> > > - u8 temp;
> > > - int error;
> > > - u16 ctrl_base_addr;
> > > struct rmi_device *rmi_dev = fn->rmi_dev;
> > > struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> > > - struct f01_data *data = fn->data;
> > > - struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
> > > + const struct rmi_device_platform_data *pdata =
> > > + to_rmi_platform_data(rmi_dev);
> > > + struct f01_data *f01;
> > > + size_t f01_size;
> > > + int error;
> > > + u16 ctrl_base_addr;
> > > + u8 device_status;
> > > + u8 temp;
> > > +
> > > + f01_size = sizeof(struct f01_data) +
> > > + sizeof(u8) * driver_data->num_of_irq_regs;
> > > + f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
> > > + if (!f01) {
> > > + dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
> >
> > Nitpick: Can we drop this printout in the process? It's much less
> > useful than the error and backtrace coming from kmalloc on failure anyway.
>
> Actually I like messages: who knows when we decided to change kmalloc
> behavior and it also helps when there are several allocations in the
> same function to know which one failed.
Of course it's your choice, but I find addr2line does a pretty good job
here with the backtrace. Maybe we'll get #define GFP_KERNEL __GFP_NOWARN,
but I think someone will go through with some cocci scripts at that point.
-Courtney
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.
From: Christopher Heiny @ 2014-02-13 2:31 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, David Herrmann, Jiri Kosina, Courtney Cavin
In-Reply-To: <20140212064925.GC15855@core.coreip.homeip.net>
On 02/11/2014 10:49 PM, Dmitry Torokhov wrote:
> On Tue, Feb 11, 2014 at 03:13:30PM -0800, 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>
>>
>> ---
>>
>> 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
[snip]
>> @@ -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);
>
> This is wrong, rmi_release_function should finish cleaning up resources,
> however unregistration part should happen much earlier. If someone takes
> reference to the device in debugfs the device may never go away as noone
> will kick the user out.
>
> Please put the calls to rmi_function_teardown_debugfs() back where they
> originally were.
OK.
>
>> + 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;
>
> Unless you create rmi_allocate_function() and do device initialization
> there (so that device and kobject are fully initialized and proper ktype
> is assigned so that ->release() is set up) you still need to free memory
> here if failure happens before calling rmi_register_function().
Hmmm. If we adopt the idea of allocating the irq_mask as an
array in the rmi_function structure, like you did in your
rmi_f01.c patch for the interrupt_enable mask, then there's no
point of failure bewteen allocating the storage and the call to
rmi_register_function(). rmi_register_function() doesn't have a
failure point prior to calling device_register(), and the
put_device() call in there should be able clean things up, like
Courtney proposed.
So making that change, along with some others you and Courtney
have suggested in other emails, we get the patch below.
Chris
--
Input: synaptics-rmi4 - Use put_device() and device_type.release() to
free storage.
From: Christopher Heiny <cheiny@synaptics.com>
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.
Allocate irq_mask as part of struct rmi_function.
Delete unused rmi_driver_irq_get_mask() function.
Suggested-by: Courtney Cavin <courtney.cavin@sonymobile.com>
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
---
drivers/input/rmi4/rmi_bus.c | 17 ++++++++-------
drivers/input/rmi4/rmi_bus.h | 2 +-
drivers/input/rmi4/rmi_driver.c | 46
+++++------------------------------------
3 files changed, 15 insertions(+), 50 deletions(-)
diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index 96a76e7..6342b45 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c
@@ -34,6 +34,7 @@ static struct dentry *rmi_debugfs_root;
static void rmi_release_device(struct device *dev)
{
struct rmi_device *rmi_dev = to_rmi_device(dev);
+
kfree(rmi_dev);
}
@@ -94,8 +95,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 +112,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));
@@ -142,6 +144,7 @@ EXPORT_SYMBOL(rmi_unregister_transport_device);
static void rmi_release_function(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
+
kfree(fn);
}
@@ -240,16 +243,14 @@ 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;
+ rmi_function_teardown_debugfs(fn);
+ 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)
diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index decb479..a544c9c 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h
@@ -47,13 +47,13 @@ struct rmi_function {
struct device dev;
int num_of_irqs;
int irq_pos;
- unsigned long *irq_mask;
void *data;
struct list_head node;
#ifdef CONFIG_RMI4_DEBUG
struct dentry *debugfs_root;
#endif
+ unsigned long irq_mask[];
};
#define to_rmi_function(d) container_of(d, struct rmi_function, dev)
diff --git a/drivers/input/rmi4/rmi_driver.c
b/drivers/input/rmi4/rmi_driver.c
index 34f19e9..a6a1aea 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -185,7 +185,6 @@ static void rmi_free_function_list(struct rmi_device
*rmi_dev)
list_for_each_entry_safe_reverse(fn, tmp,
&data->function_list, node) {
list_del(&fn->node);
- kfree(fn->irq_mask);
rmi_unregister_function(fn);
}
}
@@ -456,28 +455,6 @@ static int rmi_driver_reset_handler(struct
rmi_device *rmi_dev)
return 0;
}
-/*
- * Construct a function's IRQ mask. This should be called once and stored.
- */
-int rmi_driver_irq_get_mask(struct rmi_device *rmi_dev,
- struct rmi_function *fn)
-{
- struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
- int i;
-
- /* call devm_kcalloc when it will be defined in kernel in future */
- fn->irq_mask = devm_kzalloc(&rmi_dev->dev,
- BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
- GFP_KERNEL);
-
- if (fn->irq_mask) {
- for (i = 0; i < fn->num_of_irqs; i++)
- set_bit(fn->irq_pos+i, fn->irq_mask);
- return 0;
- } else
- return -ENOMEM;
-}
-
int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry
*entry,
u16 pdt_address)
{
@@ -652,7 +629,10 @@ static int rmi_create_function(struct rmi_device
*rmi_dev,
dev_dbg(dev, "Initializing F%02X for %s.\n",
pdt->function_number, pdata->sensor_name);
- fn = kzalloc(sizeof(struct rmi_function), GFP_KERNEL);
+ fn = kzalloc(sizeof(struct rmi_function) +
+ (BITS_TO_LONGS(data->irq_count) *
+ sizeof(unsigned long)),
+ GFP_KERNEL);
if (!fn) {
dev_err(dev, "Failed to allocate memory for F%02X\n",
pdt->function_number);
@@ -668,22 +648,12 @@ static int rmi_create_function(struct rmi_device
*rmi_dev,
fn->irq_pos = *current_irq_count;
*current_irq_count += fn->num_of_irqs;
- fn->irq_mask = kzalloc(
- BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
- GFP_KERNEL);
- 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;
- }
-
for (i = 0; i < fn->num_of_irqs; i++)
set_bit(fn->irq_pos + i, fn->irq_mask);
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 +661,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
* Re: [PATCH] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Barry Song @ 2014-02-13 2:32 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
DL-SHA-WorkGroupLinux, Xianglong Du, Rongjun Ying, Barry Song
In-Reply-To: <20140212231131.GA29769@core.coreip.homeip.net>
2014-02-13 7:11 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Barry,
>
> On Mon, Feb 10, 2014 at 06:07:39PM +0800, Barry Song wrote:
>>
>> static int sirfsoc_pwrc_remove(struct platform_device *pdev)
>> {
>> + struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(&pdev->dev);
>> +
>> device_init_wakeup(&pdev->dev, 0);
>>
>> + cancel_delayed_work_sync(&pwrcdrv->work);
>> +
>
> This is racy: interrupt is freed later and can schedule work again.
thanks, Dmitry. i will do a manual devm_free_irq() before cancelling
the work and before devres removes the resources.
>
> Thanks.
>
> --
> Dmitry
-barry
^ permalink raw reply
* Re: [PATCH] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Dmitry Torokhov @ 2014-02-13 3:41 UTC (permalink / raw)
To: Barry Song
Cc: linux-input@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
DL-SHA-WorkGroupLinux, Xianglong Du, Rongjun Ying, Barry Song
In-Reply-To: <CAGsJ_4wzQYmNkd421vzau5BiTyewRX=vzEiy=BiSnttb+rEC8Q@mail.gmail.com>
On February 12, 2014 6:32:03 PM PST, Barry Song <21cnbao@gmail.com> wrote:
>2014-02-13 7:11 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
>> Hi Barry,
>>
>> On Mon, Feb 10, 2014 at 06:07:39PM +0800, Barry Song wrote:
>>>
>>> static int sirfsoc_pwrc_remove(struct platform_device *pdev)
>>> {
>>> + struct sirfsoc_pwrc_drvdata *pwrcdrv =
>dev_get_drvdata(&pdev->dev);
>>> +
>>> device_init_wakeup(&pdev->dev, 0);
>>>
>>> + cancel_delayed_work_sync(&pwrcdrv->work);
>>> +
>>
>> This is racy: interrupt is freed later and can schedule work again.
>
>thanks, Dmitry. i will do a manual devm_free_irq() before cancelling
>the work and before devres removes the resources.
Another option would be to use devm custom action to ensure that work is canceled after freeing IRQ.
--
Dmitry
^ permalink raw reply
* [PATCH 01/11] Input: synaptics-rmi4 - do not kfree() managed memory in F01
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
Data that is allocated with devm_kzalloc() should not be freed with
kfree(). In fact, we should rely on the fact that memory is managed and let
devres core free it for us.
Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_f01.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 381ad60..92b90d1 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -272,7 +272,7 @@ static int rmi_f01_initialize(struct rmi_function *fn)
if (error < 0) {
dev_err(&fn->dev,
"Failed to read F01 control interrupt enable register.\n");
- goto error_exit;
+ return error;
}
ctrl_base_addr += data->num_of_irq_regs;
@@ -307,14 +307,14 @@ static int rmi_f01_initialize(struct rmi_function *fn)
data->device_control.doze_interval);
if (error < 0) {
dev_err(&fn->dev, "Failed to configure F01 doze interval register.\n");
- goto error_exit;
+ return error;
}
} else {
error = rmi_read(rmi_dev, data->doze_interval_addr,
&data->device_control.doze_interval);
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 doze interval register.\n");
- goto error_exit;
+ return error;
}
}
@@ -328,14 +328,14 @@ static int rmi_f01_initialize(struct rmi_function *fn)
data->device_control.wakeup_threshold);
if (error < 0) {
dev_err(&fn->dev, "Failed to configure F01 wakeup threshold register.\n");
- goto error_exit;
+ return error;
}
} else {
error = rmi_read(rmi_dev, data->wakeup_threshold_addr,
&data->device_control.wakeup_threshold);
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 wakeup threshold register.\n");
- goto error_exit;
+ return error;
}
}
}
@@ -351,14 +351,14 @@ static int rmi_f01_initialize(struct rmi_function *fn)
data->device_control.doze_holdoff);
if (error < 0) {
dev_err(&fn->dev, "Failed to configure F01 doze holdoff register.\n");
- goto error_exit;
+ return error;
}
} else {
error = rmi_read(rmi_dev, data->doze_holdoff_addr,
&data->device_control.doze_holdoff);
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 doze holdoff register.\n");
- goto error_exit;
+ return error;
}
}
}
@@ -367,22 +367,17 @@ static int rmi_f01_initialize(struct rmi_function *fn)
&data->device_status, sizeof(data->device_status));
if (error < 0) {
dev_err(&fn->dev, "Failed to read device status.\n");
- goto error_exit;
+ return error;
}
if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
dev_err(&fn->dev,
"Device was reset during configuration process, status: %#02x!\n",
RMI_F01_STATUS_CODE(data->device_status));
- error = -EINVAL;
- goto error_exit;
+ return -EINVAL;
}
return 0;
-
- error_exit:
- kfree(data);
- return error;
}
static int rmi_f01_config(struct rmi_function *fn)
--
1.8.5.3
^ permalink raw reply related
* [PATCH 03/11] Input: synaptics-rmi4 - do not update configuration in rmi_f01_probe()
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
Do not write configuration data in probe(), we have config() for that.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_f01.c | 18 ------------------
1 file changed, 18 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 897d8ac..976aba3 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -303,12 +303,6 @@ static int rmi_f01_initialize(struct rmi_function *fn)
if (pdata->power_management.doze_interval) {
data->device_control.doze_interval =
pdata->power_management.doze_interval;
- error = rmi_write(rmi_dev, data->doze_interval_addr,
- data->device_control.doze_interval);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to configure F01 doze interval register.\n");
- return error;
- }
} else {
error = rmi_read(rmi_dev, data->doze_interval_addr,
&data->device_control.doze_interval);
@@ -324,12 +318,6 @@ static int rmi_f01_initialize(struct rmi_function *fn)
if (pdata->power_management.wakeup_threshold) {
data->device_control.wakeup_threshold =
pdata->power_management.wakeup_threshold;
- error = rmi_write(rmi_dev, data->wakeup_threshold_addr,
- data->device_control.wakeup_threshold);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to configure F01 wakeup threshold register.\n");
- return error;
- }
} else {
error = rmi_read(rmi_dev, data->wakeup_threshold_addr,
&data->device_control.wakeup_threshold);
@@ -347,12 +335,6 @@ static int rmi_f01_initialize(struct rmi_function *fn)
if (pdata->power_management.doze_holdoff) {
data->device_control.doze_holdoff =
pdata->power_management.doze_holdoff;
- error = rmi_write(rmi_dev, data->doze_holdoff_addr,
- data->device_control.doze_holdoff);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to configure F01 doze holdoff register.\n");
- return error;
- }
} else {
error = rmi_read(rmi_dev, data->doze_holdoff_addr,
&data->device_control.doze_holdoff);
--
1.8.5.3
^ permalink raw reply related
* [PATCH 04/11] Input: synaptics-rmi4 - fix LTS handling in F01
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
From: Christopher Heiny <cheiny@synaptics.com>
Both F01_RMI_Ctrl2 and F01_RMI_Ctrl3 (doze_interval and wakeup_threshold)
are controlled by the has_adjustable_doze bit.
Signed-off-by: Christopher Heiny<cheiny@synaptics.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_f01.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 976aba3..30e0b50 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -328,6 +328,9 @@ static int rmi_f01_initialize(struct rmi_function *fn)
}
}
+ if (data->properties.has_lts)
+ ctrl_base_addr++;
+
if (data->properties.has_adjustable_doze_holdoff) {
data->doze_holdoff_addr = ctrl_base_addr;
ctrl_base_addr++;
@@ -383,7 +386,7 @@ static int rmi_f01_config(struct rmi_function *fn)
dev_err(&fn->dev, "Failed to write interrupt enable.\n");
return retval;
}
- if (data->properties.has_lts) {
+ if (data->properties.has_adjustable_doze) {
retval = rmi_write_block(fn->rmi_dev, data->doze_interval_addr,
&data->device_control.doze_interval,
sizeof(u8));
@@ -391,9 +394,7 @@ static int rmi_f01_config(struct rmi_function *fn)
dev_err(&fn->dev, "Failed to write doze interval.\n");
return retval;
}
- }
- if (data->properties.has_adjustable_doze) {
retval = rmi_write_block(fn->rmi_dev,
data->wakeup_threshold_addr,
&data->device_control.wakeup_threshold,
--
1.8.5.3
^ permalink raw reply related
* [PATCH 05/11] Input: synaptics-rmi4 - remove control_mutex from f01_data
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
It is not used by anyone.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_f01.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 30e0b50..6f90a6c 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -125,7 +125,6 @@ struct f01_data {
struct f01_basic_properties properties;
struct f01_device_control device_control;
- struct mutex control_mutex;
u8 device_status;
@@ -214,8 +213,6 @@ static int rmi_f01_initialize(struct rmi_function *fn)
struct f01_data *data = fn->data;
struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
- mutex_init(&data->control_mutex);
-
/*
* Set the configured bit and (optionally) other important stuff
* in the device control register.
--
1.8.5.3
^ permalink raw reply related
* [PATCH 06/11] Input: synaptics-rmi4 - remove device_status form f01_data
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
We do not need to persist it - we read it when signalled.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_f01.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 6f90a6c..1e49ab4 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -126,8 +126,6 @@ struct f01_data {
struct f01_device_control device_control;
- u8 device_status;
-
u16 interrupt_enable_addr;
u16 doze_interval_addr;
u16 wakeup_threshold_addr;
@@ -212,6 +210,7 @@ static int rmi_f01_initialize(struct rmi_function *fn)
struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
struct f01_data *data = fn->data;
struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ u8 device_status;
/*
* Set the configured bit and (optionally) other important stuff
@@ -346,16 +345,16 @@ static int rmi_f01_initialize(struct rmi_function *fn)
}
error = rmi_read_block(rmi_dev, fn->fd.data_base_addr,
- &data->device_status, sizeof(data->device_status));
+ &device_status, sizeof(device_status));
if (error < 0) {
dev_err(&fn->dev, "Failed to read device status.\n");
return error;
}
- if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
+ if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
dev_err(&fn->dev,
"Device was reset during configuration process, status: %#02x!\n",
- RMI_F01_STATUS_CODE(data->device_status));
+ RMI_F01_STATUS_CODE(device_status));
return -EINVAL;
}
@@ -497,18 +496,18 @@ static int rmi_f01_attention(struct rmi_function *fn,
unsigned long *irq_bits)
{
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *data = fn->data;
int retval;
+ u8 device_status;
retval = rmi_read_block(rmi_dev, fn->fd.data_base_addr,
- &data->device_status, sizeof(data->device_status));
+ &device_status, sizeof(device_status));
if (retval < 0) {
dev_err(&fn->dev, "Failed to read device status, code: %d.\n",
retval);
return retval;
}
- if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
+ if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
dev_warn(&fn->dev, "Device reset detected.\n");
retval = rmi_dev->driver->reset_handler(rmi_dev);
if (retval < 0)
--
1.8.5.3
^ permalink raw reply related
* [PATCH 08/11] Input: synaptics-rmi4 - use rmi_read/rmi_write in F01
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
Use rmi_read()/rmi_write() for reading/writing single-byte data. Also print
error code when IO fails.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_f01.c | 170 ++++++++++++++++++++++---------------------
1 file changed, 88 insertions(+), 82 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 1219e0c..a2f05bc 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -171,8 +171,9 @@ static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
error = rmi_read_block(rmi_dev, query_base_addr,
basic_query, sizeof(basic_query));
- if (error < 0) {
- dev_err(&rmi_dev->dev, "Failed to read device query registers.\n");
+ if (error) {
+ dev_err(&rmi_dev->dev,
+ "Failed to read device query registers: %d\n", error);
return error;
}
@@ -205,25 +206,25 @@ static int rmi_f01_initialize(struct rmi_function *fn)
{
u8 temp;
int error;
- u16 ctrl_base_addr;
struct rmi_device *rmi_dev = fn->rmi_dev;
struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
struct f01_data *f01 = fn->data;
- struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ const struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ u16 ctrl_base_addr = fn->fd.control_base_addr;
u8 device_status;
/*
* Set the configured bit and (optionally) other important stuff
* in the device control register.
*/
- ctrl_base_addr = fn->fd.control_base_addr;
- error = rmi_read_block(rmi_dev, fn->fd.control_base_addr,
- &f01->device_control.ctrl0,
- sizeof(f01->device_control.ctrl0));
- if (error < 0) {
- dev_err(&fn->dev, "Failed to read F01 control.\n");
+
+ error = rmi_read(rmi_dev, fn->fd.control_base_addr,
+ &f01->device_control.ctrl0);
+ if (error) {
+ dev_err(&fn->dev, "Failed to read F01 control: %d\n", error);
return error;
}
+
switch (pdata->power_management.nosleep) {
case RMI_F01_NOSLEEP_DEFAULT:
break;
@@ -249,11 +250,10 @@ static int rmi_f01_initialize(struct rmi_function *fn)
f01->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
- error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
- &f01->device_control.ctrl0,
- sizeof(f01->device_control.ctrl0));
- if (error < 0) {
- dev_err(&fn->dev, "Failed to write F01 control.\n");
+ error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+ f01->device_control.ctrl0);
+ if (error) {
+ dev_err(&fn->dev, "Failed to write F01 control: %d\n", error);
return error;
}
@@ -265,9 +265,10 @@ static int rmi_f01_initialize(struct rmi_function *fn)
error = rmi_read_block(rmi_dev, ctrl_base_addr,
f01->device_control.interrupt_enable,
sizeof(u8) * (f01->num_of_irq_regs));
- if (error < 0) {
+ if (error) {
dev_err(&fn->dev,
- "Failed to read F01 control interrupt enable register.\n");
+ "Failed to read F01 control interrupt enable register: %d\n",
+ error);
return error;
}
@@ -302,8 +303,10 @@ static int rmi_f01_initialize(struct rmi_function *fn)
} else {
error = rmi_read(rmi_dev, f01->doze_interval_addr,
&f01->device_control.doze_interval);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to read F01 doze interval register.\n");
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read F01 doze interval register: %d\n",
+ error);
return error;
}
}
@@ -318,7 +321,9 @@ static int rmi_f01_initialize(struct rmi_function *fn)
error = rmi_read(rmi_dev, f01->wakeup_threshold_addr,
&f01->device_control.wakeup_threshold);
if (error < 0) {
- dev_err(&fn->dev, "Failed to read F01 wakeup threshold register.\n");
+ dev_err(&fn->dev,
+ "Failed to read F01 wakeup threshold register: %d\n",
+ error);
return error;
}
}
@@ -337,17 +342,19 @@ static int rmi_f01_initialize(struct rmi_function *fn)
} else {
error = rmi_read(rmi_dev, f01->doze_holdoff_addr,
&f01->device_control.doze_holdoff);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to read F01 doze holdoff register.\n");
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read F01 doze holdoff register: %d\n",
+ error);
return error;
}
}
}
- error = rmi_read_block(rmi_dev, fn->fd.data_base_addr,
- &device_status, sizeof(device_status));
+ error = rmi_read(rmi_dev, fn->fd.data_base_addr, &device_status);
if (error < 0) {
- dev_err(&fn->dev, "Failed to read device status.\n");
+ dev_err(&fn->dev,
+ "Failed to read device status: %d\n", error);
return error;
}
@@ -364,50 +371,53 @@ static int rmi_f01_initialize(struct rmi_function *fn)
static int rmi_f01_config(struct rmi_function *fn)
{
struct f01_data *f01 = fn->data;
- int retval;
-
- retval = rmi_write_block(fn->rmi_dev, fn->fd.control_base_addr,
- &f01->device_control.ctrl0,
- sizeof(f01->device_control.ctrl0));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write device_control.reg.\n");
- return retval;
+ int error;
+
+ error = rmi_write(fn->rmi_dev, fn->fd.control_base_addr,
+ f01->device_control.ctrl0);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write device_control register: %d\n", error);
+ return error;
}
- retval = rmi_write_block(fn->rmi_dev, f01->interrupt_enable_addr,
- f01->device_control.interrupt_enable,
- sizeof(u8) * f01->num_of_irq_regs);
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write interrupt enable.\n");
- return retval;
+ error = rmi_write_block(fn->rmi_dev, f01->interrupt_enable_addr,
+ f01->device_control.interrupt_enable,
+ sizeof(u8) * f01->num_of_irq_regs);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write interrupt enable: %d\n", error);
+ return error;
}
if (f01->properties.has_adjustable_doze) {
- retval = rmi_write_block(fn->rmi_dev, f01->doze_interval_addr,
- &f01->device_control.doze_interval,
- sizeof(u8));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write doze interval.\n");
- return retval;
+ error = rmi_write(fn->rmi_dev, f01->doze_interval_addr,
+ f01->device_control.doze_interval);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write doze interval: %d\n", error);
+ return error;
}
- retval = rmi_write_block(fn->rmi_dev,
+ error = rmi_write_block(fn->rmi_dev,
f01->wakeup_threshold_addr,
&f01->device_control.wakeup_threshold,
sizeof(u8));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write wakeup threshold.\n");
- return retval;
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write wakeup threshold: %d\n",
+ error);
+ return error;
}
}
if (f01->properties.has_adjustable_doze_holdoff) {
- retval = rmi_write_block(fn->rmi_dev, f01->doze_holdoff_addr,
- &f01->device_control.doze_holdoff,
- sizeof(u8));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write doze holdoff.\n");
- return retval;
+ error = rmi_write(fn->rmi_dev, f01->doze_holdoff_addr,
+ f01->device_control.doze_holdoff);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write doze holdoff: %d\n", error);
+ return error;
}
}
@@ -439,23 +449,19 @@ static int rmi_f01_suspend(struct device *dev)
struct f01_data *f01 = fn->data;
int error;
- f01->old_nosleep = f01->device_control.ctrl0 &
- RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->old_nosleep =
+ f01->device_control.ctrl0 & RMI_F01_CRTL0_NOSLEEP_BIT;
f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
f01->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
- error = rmi_write_block(rmi_dev,
- fn->fd.control_base_addr,
- &f01->device_control.ctrl0,
- sizeof(f01->device_control.ctrl0));
- if (error < 0) {
- dev_err(&fn->dev, "Failed to write sleep mode. Code: %d.\n",
- error);
+ error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+ f01->device_control.ctrl0);
+ if (error) {
+ dev_err(&fn->dev, "Failed to write sleep mode: %d.\n", error);
if (f01->old_nosleep)
- f01->device_control.ctrl0 |=
- RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
return error;
@@ -477,13 +483,11 @@ static int rmi_f01_resume(struct device *dev)
f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
- error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
- &f01->device_control.ctrl0,
- sizeof(f01->device_control.ctrl0));
- if (error < 0) {
+ error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+ f01->device_control.ctrl0);
+ if (error) {
dev_err(&fn->dev,
- "Failed to restore normal operation. Code: %d.\n",
- error);
+ "Failed to restore normal operation: %d.\n", error);
return error;
}
@@ -497,23 +501,25 @@ static int rmi_f01_attention(struct rmi_function *fn,
unsigned long *irq_bits)
{
struct rmi_device *rmi_dev = fn->rmi_dev;
- int retval;
+ int error;
u8 device_status;
- retval = rmi_read_block(rmi_dev, fn->fd.data_base_addr,
- &device_status, sizeof(device_status));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to read device status, code: %d.\n",
- retval);
- return retval;
+ error = rmi_read(rmi_dev, fn->fd.data_base_addr, &device_status);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read device status: %d.\n", error);
+ return error;
}
if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
dev_warn(&fn->dev, "Device reset detected.\n");
- retval = rmi_dev->driver->reset_handler(rmi_dev);
- if (retval < 0)
- return retval;
+ error = rmi_dev->driver->reset_handler(rmi_dev);
+ if (error) {
+ dev_err(&fn->dev, "Device reset failed: %d\n", error);
+ return error;
+ }
}
+
return 0;
}
--
1.8.5.3
^ permalink raw reply related
* [PATCH 11/11] Input: synaptics-rmi4 - remove data pointer from RMI fucntion structure
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
Device core provides way of accessing driver-private data, we should
use it.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_bus.h | 1 -
drivers/input/rmi4/rmi_f01.c | 14 +++++------
drivers/input/rmi4/rmi_f11.c | 57 ++++++++++++++++++++------------------------
3 files changed, 32 insertions(+), 40 deletions(-)
diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index 02a2af8..5ad94c6 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h
@@ -48,7 +48,6 @@ struct rmi_function {
int num_of_irqs;
int irq_pos;
unsigned long *irq_mask;
- void *data;
struct list_head node;
#ifdef CONFIG_RMI4_DEBUG
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 36fcf8d..e646f60 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -354,14 +354,14 @@ static int rmi_f01_probe(struct rmi_function *fn)
return -EINVAL;
}
- fn->data = f01;
+ dev_set_drvdata(&fn->dev, f01);
return 0;
}
static int rmi_f01_config(struct rmi_function *fn)
{
- struct f01_data *f01 = fn->data;
+ struct f01_data *f01 = dev_get_drvdata(&fn->dev);
int error;
error = rmi_write(fn->rmi_dev, fn->fd.control_base_addr,
@@ -419,8 +419,7 @@ static int rmi_f01_config(struct rmi_function *fn)
static int rmi_f01_suspend(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
- struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *f01 = fn->data;
+ struct f01_data *f01 = dev_get_drvdata(&fn->dev);
int error;
f01->old_nosleep =
@@ -430,7 +429,7 @@ static int rmi_f01_suspend(struct device *dev)
f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
f01->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
- error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+ error = rmi_write(fn->rmi_dev, fn->fd.control_base_addr,
f01->device_control.ctrl0);
if (error) {
dev_err(&fn->dev, "Failed to write sleep mode: %d.\n", error);
@@ -447,8 +446,7 @@ static int rmi_f01_suspend(struct device *dev)
static int rmi_f01_resume(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
- struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *f01 = fn->data;
+ struct f01_data *f01 = dev_get_drvdata(&fn->dev);
int error;
if (f01->old_nosleep)
@@ -457,7 +455,7 @@ static int rmi_f01_resume(struct device *dev)
f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
- error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+ error = rmi_write(fn->rmi_dev, fn->fd.control_base_addr,
f01->device_control.ctrl0);
if (error) {
dev_err(&fn->dev,
diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.c
index 5072437..a26b944 100644
--- a/drivers/input/rmi4/rmi_f11.c
+++ b/drivers/input/rmi4/rmi_f11.c
@@ -1087,9 +1087,8 @@ static int rmi_f11_get_query_parameters(struct rmi_device *rmi_dev,
/* This operation is done in a number of places, so we have a handy routine
* for it.
*/
-static void f11_set_abs_params(struct rmi_function *fn)
+static void f11_set_abs_params(struct rmi_function *fn, struct f11_data *f11)
{
- struct f11_data *f11 = fn->data;
struct f11_2d_sensor *sensor = &f11->sensor;
struct input_dev *input = sensor->input;
/* These two lines are not doing what we want them to. So we use
@@ -1190,7 +1189,6 @@ static int rmi_f11_initialize(struct rmi_function *fn)
if (!f11)
return -ENOMEM;
- fn->data = f11;
f11->rezero_wait_ms = pdata->f11_rezero_wait;
query_base_addr = fn->fd.query_base_addr;
@@ -1280,13 +1278,16 @@ static int rmi_f11_initialize(struct rmi_function *fn)
}
mutex_init(&f11->dev_controls_mutex);
+
+ dev_set_drvdata(&fn->dev, f11);
+
return 0;
}
static int rmi_f11_register_devices(struct rmi_function *fn)
{
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f11_data *f11 = fn->data;
+ struct f11_data *f11 = dev_get_drvdata(&fn->dev);
struct input_dev *input_dev;
struct input_dev *input_dev_mouse;
struct rmi_driver *driver = rmi_dev->driver;
@@ -1304,8 +1305,8 @@ static int rmi_f11_register_devices(struct rmi_function *fn)
rc = driver->set_input_params(rmi_dev, input_dev);
if (rc < 0) {
dev_err(&fn->dev,
- "%s: Error in setting input device.\n",
- __func__);
+ "%s: Error in setting input device.\n",
+ __func__);
goto error_unregister;
}
}
@@ -1319,7 +1320,7 @@ static int rmi_f11_register_devices(struct rmi_function *fn)
set_bit(EV_ABS, input_dev->evbit);
input_set_capability(input_dev, EV_KEY, BTN_TOUCH);
- f11_set_abs_params(fn);
+ f11_set_abs_params(fn, f11);
if (sensor->sens_query.has_rel) {
set_bit(EV_REL, input_dev->evbit);
@@ -1327,7 +1328,7 @@ static int rmi_f11_register_devices(struct rmi_function *fn)
set_bit(REL_Y, input_dev->relbit);
}
rc = input_register_device(input_dev);
- if (rc < 0) {
+ if (rc) {
input_free_device(input_dev);
sensor->input = NULL;
goto error_unregister;
@@ -1347,8 +1348,8 @@ static int rmi_f11_register_devices(struct rmi_function *fn)
input_dev_mouse);
if (rc < 0) {
dev_err(&fn->dev,
- "%s: Error in setting input device.\n",
- __func__);
+ "%s: Error in setting input device.\n",
+ __func__);
goto error_unregister;
}
}
@@ -1366,7 +1367,7 @@ static int rmi_f11_register_devices(struct rmi_function *fn)
set_bit(BTN_RIGHT, input_dev_mouse->keybit);
rc = input_register_device(input_dev_mouse);
- if (rc < 0) {
+ if (rc) {
input_free_device(input_dev_mouse);
sensor->mouse_input = NULL;
goto error_unregister;
@@ -1390,19 +1391,9 @@ error_unregister:
return rc;
}
-static void rmi_f11_free_devices(struct rmi_function *fn)
-{
- struct f11_data *f11 = fn->data;
-
- if (f11->sensor.input)
- input_unregister_device(f11->sensor.input);
- if (f11->sensor.mouse_input)
- input_unregister_device(f11->sensor.mouse_input);
-}
-
static int rmi_f11_config(struct rmi_function *fn)
{
- struct f11_data *f11 = fn->data;
+ struct f11_data *f11 = dev_get_drvdata(&fn->dev);
int rc;
rc = f11_write_control_regs(fn, &f11->sensor.sens_query,
@@ -1416,7 +1407,7 @@ static int rmi_f11_config(struct rmi_function *fn)
static int rmi_f11_attention(struct rmi_function *fn, unsigned long *irq_bits)
{
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f11_data *f11 = fn->data;
+ struct f11_data *f11 = dev_get_drvdata(&fn->dev);
u16 data_base_addr = fn->fd.data_base_addr;
u16 data_base_addr_offset = 0;
int error;
@@ -1425,7 +1416,7 @@ static int rmi_f11_attention(struct rmi_function *fn, unsigned long *irq_bits)
data_base_addr + data_base_addr_offset,
f11->sensor.data_pkt,
f11->sensor.pkt_size);
- if (error < 0)
+ if (error)
return error;
rmi_f11_finger_handler(f11, &f11->sensor);
@@ -1438,18 +1429,17 @@ static int rmi_f11_attention(struct rmi_function *fn, unsigned long *irq_bits)
static int rmi_f11_resume(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
- struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f11_data *data = fn->data;
+ struct f11_data *f11 = dev_get_drvdata(&fn->dev);
int error;
dev_dbg(&fn->dev, "Resuming...\n");
- if (!data->rezero_wait_ms)
+ if (!f11->rezero_wait_ms)
return 0;
- mdelay(data->rezero_wait_ms);
+ mdelay(f11->rezero_wait_ms);
- error = rmi_write(rmi_dev, fn->fd.command_base_addr, RMI_F11_REZERO);
- if (error < 0) {
+ error = rmi_write(fn->rmi_dev, fn->fd.command_base_addr, RMI_F11_REZERO);
+ if (error) {
dev_err(&fn->dev,
"%s: failed to issue rezero command, error = %d.",
__func__, error);
@@ -1479,7 +1469,12 @@ static int rmi_f11_probe(struct rmi_function *fn)
static void rmi_f11_remove(struct rmi_function *fn)
{
- rmi_f11_free_devices(fn);
+ struct f11_data *f11 = dev_get_drvdata(&fn->dev);
+
+ if (f11->sensor.input)
+ input_unregister_device(f11->sensor.input);
+ if (f11->sensor.mouse_input)
+ input_unregister_device(f11->sensor.mouse_input);
}
static struct rmi_function_handler rmi_f11_handler = {
--
1.8.5.3
^ permalink raw reply related
* [PATCH 02/11] Input: synaptics-rmi4 - remove unused rmi_f01_remove()
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
It is an empty stub and is not needed.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_f01.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 92b90d1..897d8ac 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -451,11 +451,6 @@ static int rmi_f01_probe(struct rmi_function *fn)
return 0;
}
-static void rmi_f01_remove(struct rmi_function *fn)
-{
- /* Placeholder for now. */
-}
-
#ifdef CONFIG_PM_SLEEP
static int rmi_f01_suspend(struct device *dev)
{
@@ -554,7 +549,6 @@ static struct rmi_function_handler rmi_f01_handler = {
},
.func = 0x01,
.probe = rmi_f01_probe,
- .remove = rmi_f01_remove,
.config = rmi_f01_config,
.attention = rmi_f01_attention,
};
--
1.8.5.3
^ permalink raw reply related
* [PATCH 07/11] Input: synaptics-rmi4 - rename instances of f01_data from data to f01
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
We have too many "data"s: f01_data, driver_data, pdata, etc. Let's
untangle it a bit.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_f01.c | 135 ++++++++++++++++++++++---------------------
1 file changed, 68 insertions(+), 67 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 1e49ab4..1219e0c 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -208,7 +208,7 @@ static int rmi_f01_initialize(struct rmi_function *fn)
u16 ctrl_base_addr;
struct rmi_device *rmi_dev = fn->rmi_dev;
struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
- struct f01_data *data = fn->data;
+ struct f01_data *f01 = fn->data;
struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
u8 device_status;
@@ -218,8 +218,8 @@ static int rmi_f01_initialize(struct rmi_function *fn)
*/
ctrl_base_addr = fn->fd.control_base_addr;
error = rmi_read_block(rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
+ &f01->device_control.ctrl0,
+ sizeof(f01->device_control.ctrl0));
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 control.\n");
return error;
@@ -228,10 +228,10 @@ static int rmi_f01_initialize(struct rmi_function *fn)
case RMI_F01_NOSLEEP_DEFAULT:
break;
case RMI_F01_NOSLEEP_OFF:
- data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
break;
case RMI_F01_NOSLEEP_ON:
- data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
break;
}
@@ -240,38 +240,38 @@ static int rmi_f01_initialize(struct rmi_function *fn)
* reboot without power cycle. If so, clear it so the sensor
* is certain to function.
*/
- if ((data->device_control.ctrl0 & RMI_F01_CTRL0_SLEEP_MODE_MASK) !=
+ if ((f01->device_control.ctrl0 & RMI_F01_CTRL0_SLEEP_MODE_MASK) !=
RMI_SLEEP_MODE_NORMAL) {
dev_warn(&fn->dev,
"WARNING: Non-zero sleep mode found. Clearing...\n");
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
}
- data->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
+ &f01->device_control.ctrl0,
+ sizeof(f01->device_control.ctrl0));
if (error < 0) {
dev_err(&fn->dev, "Failed to write F01 control.\n");
return error;
}
- data->irq_count = driver_data->irq_count;
- data->num_of_irq_regs = driver_data->num_of_irq_regs;
+ f01->irq_count = driver_data->irq_count;
+ f01->num_of_irq_regs = driver_data->num_of_irq_regs;
ctrl_base_addr += sizeof(u8);
- data->interrupt_enable_addr = ctrl_base_addr;
+ f01->interrupt_enable_addr = ctrl_base_addr;
error = rmi_read_block(rmi_dev, ctrl_base_addr,
- data->device_control.interrupt_enable,
- sizeof(u8) * (data->num_of_irq_regs));
+ f01->device_control.interrupt_enable,
+ sizeof(u8) * (f01->num_of_irq_regs));
if (error < 0) {
dev_err(&fn->dev,
"Failed to read F01 control interrupt enable register.\n");
return error;
}
- ctrl_base_addr += data->num_of_irq_regs;
+ ctrl_base_addr += f01->num_of_irq_regs;
/* dummy read in order to clear irqs */
error = rmi_read(rmi_dev, fn->fd.data_base_addr + 1, &temp);
@@ -281,42 +281,42 @@ static int rmi_f01_initialize(struct rmi_function *fn)
}
error = rmi_f01_read_properties(rmi_dev, fn->fd.query_base_addr,
- &data->properties);
+ &f01->properties);
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 properties.\n");
return error;
}
dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s\n",
- data->properties.manufacturer_id == 1 ?
+ f01->properties.manufacturer_id == 1 ?
"Synaptics" : "unknown",
- data->properties.product_id);
+ f01->properties.product_id);
/* read control register */
- if (data->properties.has_adjustable_doze) {
- data->doze_interval_addr = ctrl_base_addr;
+ if (f01->properties.has_adjustable_doze) {
+ f01->doze_interval_addr = ctrl_base_addr;
ctrl_base_addr++;
if (pdata->power_management.doze_interval) {
- data->device_control.doze_interval =
+ f01->device_control.doze_interval =
pdata->power_management.doze_interval;
} else {
- error = rmi_read(rmi_dev, data->doze_interval_addr,
- &data->device_control.doze_interval);
+ error = rmi_read(rmi_dev, f01->doze_interval_addr,
+ &f01->device_control.doze_interval);
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 doze interval register.\n");
return error;
}
}
- data->wakeup_threshold_addr = ctrl_base_addr;
+ f01->wakeup_threshold_addr = ctrl_base_addr;
ctrl_base_addr++;
if (pdata->power_management.wakeup_threshold) {
- data->device_control.wakeup_threshold =
+ f01->device_control.wakeup_threshold =
pdata->power_management.wakeup_threshold;
} else {
- error = rmi_read(rmi_dev, data->wakeup_threshold_addr,
- &data->device_control.wakeup_threshold);
+ error = rmi_read(rmi_dev, f01->wakeup_threshold_addr,
+ &f01->device_control.wakeup_threshold);
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 wakeup threshold register.\n");
return error;
@@ -324,19 +324,19 @@ static int rmi_f01_initialize(struct rmi_function *fn)
}
}
- if (data->properties.has_lts)
+ if (f01->properties.has_lts)
ctrl_base_addr++;
- if (data->properties.has_adjustable_doze_holdoff) {
- data->doze_holdoff_addr = ctrl_base_addr;
+ if (f01->properties.has_adjustable_doze_holdoff) {
+ f01->doze_holdoff_addr = ctrl_base_addr;
ctrl_base_addr++;
if (pdata->power_management.doze_holdoff) {
- data->device_control.doze_holdoff =
+ f01->device_control.doze_holdoff =
pdata->power_management.doze_holdoff;
} else {
- error = rmi_read(rmi_dev, data->doze_holdoff_addr,
- &data->device_control.doze_holdoff);
+ error = rmi_read(rmi_dev, f01->doze_holdoff_addr,
+ &f01->device_control.doze_holdoff);
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 doze holdoff register.\n");
return error;
@@ -363,28 +363,28 @@ static int rmi_f01_initialize(struct rmi_function *fn)
static int rmi_f01_config(struct rmi_function *fn)
{
- struct f01_data *data = fn->data;
+ struct f01_data *f01 = fn->data;
int retval;
retval = rmi_write_block(fn->rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
+ &f01->device_control.ctrl0,
+ sizeof(f01->device_control.ctrl0));
if (retval < 0) {
dev_err(&fn->dev, "Failed to write device_control.reg.\n");
return retval;
}
- retval = rmi_write_block(fn->rmi_dev, data->interrupt_enable_addr,
- data->device_control.interrupt_enable,
- sizeof(u8) * data->num_of_irq_regs);
-
+ retval = rmi_write_block(fn->rmi_dev, f01->interrupt_enable_addr,
+ f01->device_control.interrupt_enable,
+ sizeof(u8) * f01->num_of_irq_regs);
if (retval < 0) {
dev_err(&fn->dev, "Failed to write interrupt enable.\n");
return retval;
}
- if (data->properties.has_adjustable_doze) {
- retval = rmi_write_block(fn->rmi_dev, data->doze_interval_addr,
- &data->device_control.doze_interval,
+
+ if (f01->properties.has_adjustable_doze) {
+ retval = rmi_write_block(fn->rmi_dev, f01->doze_interval_addr,
+ &f01->device_control.doze_interval,
sizeof(u8));
if (retval < 0) {
dev_err(&fn->dev, "Failed to write doze interval.\n");
@@ -392,8 +392,8 @@ static int rmi_f01_config(struct rmi_function *fn)
}
retval = rmi_write_block(fn->rmi_dev,
- data->wakeup_threshold_addr,
- &data->device_control.wakeup_threshold,
+ f01->wakeup_threshold_addr,
+ &f01->device_control.wakeup_threshold,
sizeof(u8));
if (retval < 0) {
dev_err(&fn->dev, "Failed to write wakeup threshold.\n");
@@ -401,15 +401,16 @@ static int rmi_f01_config(struct rmi_function *fn)
}
}
- if (data->properties.has_adjustable_doze_holdoff) {
- retval = rmi_write_block(fn->rmi_dev, data->doze_holdoff_addr,
- &data->device_control.doze_holdoff,
+ if (f01->properties.has_adjustable_doze_holdoff) {
+ retval = rmi_write_block(fn->rmi_dev, f01->doze_holdoff_addr,
+ &f01->device_control.doze_holdoff,
sizeof(u8));
if (retval < 0) {
dev_err(&fn->dev, "Failed to write doze holdoff.\n");
return retval;
}
}
+
return 0;
}
@@ -435,28 +436,28 @@ static int rmi_f01_suspend(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *data = fn->data;
+ struct f01_data *f01 = fn->data;
int error;
- data->old_nosleep = data->device_control.ctrl0 &
+ f01->old_nosleep = f01->device_control.ctrl0 &
RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
- data->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
error = rmi_write_block(rmi_dev,
fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
+ &f01->device_control.ctrl0,
+ sizeof(f01->device_control.ctrl0));
if (error < 0) {
dev_err(&fn->dev, "Failed to write sleep mode. Code: %d.\n",
error);
- if (data->old_nosleep)
- data->device_control.ctrl0 |=
+ if (f01->old_nosleep)
+ f01->device_control.ctrl0 |=
RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
- data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
return error;
}
@@ -467,18 +468,18 @@ static int rmi_f01_resume(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *data = fn->data;
+ struct f01_data *f01 = fn->data;
int error;
- if (data->old_nosleep)
- data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+ if (f01->old_nosleep)
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
- data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
+ &f01->device_control.ctrl0,
+ sizeof(f01->device_control.ctrl0));
if (error < 0) {
dev_err(&fn->dev,
"Failed to restore normal operation. Code: %d.\n",
--
1.8.5.3
^ permalink raw reply related
* [PATCH 09/11] Input: synaptics-rmi4 - consolidate memory allocations in F01
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
Let's allocate interrupt mask together with the main structure and combine
rmi_f01_alloc_memory, rmi_f01_initialize and rmi_f01_probe into single
function.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_f01.c | 86 ++++++++++++++++----------------------------
1 file changed, 30 insertions(+), 56 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index a2f05bc..0e21004 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -130,37 +130,15 @@ struct f01_data {
u16 doze_interval_addr;
u16 wakeup_threshold_addr;
u16 doze_holdoff_addr;
- int irq_count;
- int num_of_irq_regs;
#ifdef CONFIG_PM_SLEEP
bool suspended;
bool old_nosleep;
#endif
-};
-
-static int rmi_f01_alloc_memory(struct rmi_function *fn,
- int num_of_irq_regs)
-{
- struct f01_data *f01;
- f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
- if (!f01) {
- dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
- return -ENOMEM;
- }
-
- f01->device_control.interrupt_enable = devm_kzalloc(&fn->dev,
- sizeof(u8) * (num_of_irq_regs),
- GFP_KERNEL);
- if (!f01->device_control.interrupt_enable) {
- dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
- return -ENOMEM;
- }
- fn->data = f01;
-
- return 0;
-}
+ unsigned int num_of_irq_regs;
+ u8 interrupt_enable[];
+};
static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
u16 query_base_addr,
@@ -202,16 +180,28 @@ static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
return 0;
}
-static int rmi_f01_initialize(struct rmi_function *fn)
+static int rmi_f01_probe(struct rmi_function *fn)
{
- u8 temp;
- int error;
struct rmi_device *rmi_dev = fn->rmi_dev;
struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
- struct f01_data *f01 = fn->data;
const struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ struct f01_data *f01;
+ size_t f01_size;
+ int error;
u16 ctrl_base_addr = fn->fd.control_base_addr;
u8 device_status;
+ u8 temp;
+
+ f01_size = sizeof(struct f01_data) +
+ sizeof(u8) * driver_data->num_of_irq_regs;
+ f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
+ if (!f01) {
+ dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
+ return -ENOMEM;
+ }
+
+ f01->num_of_irq_regs = driver_data->num_of_irq_regs;
+ f01->device_control.interrupt_enable = f01->interrupt_enable;
/*
* Set the configured bit and (optionally) other important stuff
@@ -257,12 +247,11 @@ static int rmi_f01_initialize(struct rmi_function *fn)
return error;
}
- f01->irq_count = driver_data->irq_count;
- f01->num_of_irq_regs = driver_data->num_of_irq_regs;
- ctrl_base_addr += sizeof(u8);
-
+ /* Advance to interrupt control registers */
+ ctrl_base_addr++;
f01->interrupt_enable_addr = ctrl_base_addr;
- error = rmi_read_block(rmi_dev, ctrl_base_addr,
+
+ error = rmi_read_block(rmi_dev, f01->interrupt_enable_addr,
f01->device_control.interrupt_enable,
sizeof(u8) * (f01->num_of_irq_regs));
if (error) {
@@ -272,9 +261,7 @@ static int rmi_f01_initialize(struct rmi_function *fn)
return error;
}
- ctrl_base_addr += f01->num_of_irq_regs;
-
- /* dummy read in order to clear irqs */
+ /* Dummy read in order to clear irqs */
error = rmi_read(rmi_dev, fn->fd.data_base_addr + 1, &temp);
if (error < 0) {
dev_err(&fn->dev, "Failed to read Interrupt Status.\n");
@@ -287,11 +274,13 @@ static int rmi_f01_initialize(struct rmi_function *fn)
dev_err(&fn->dev, "Failed to read F01 properties.\n");
return error;
}
+
dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s\n",
- f01->properties.manufacturer_id == 1 ?
- "Synaptics" : "unknown",
+ f01->properties.manufacturer_id == 1 ? "Synaptics" : "unknown",
f01->properties.product_id);
+ ctrl_base_addr += f01->num_of_irq_regs;
+
/* read control register */
if (f01->properties.has_adjustable_doze) {
f01->doze_interval_addr = ctrl_base_addr;
@@ -365,6 +354,8 @@ static int rmi_f01_initialize(struct rmi_function *fn)
return -EINVAL;
}
+ fn->data = f01;
+
return 0;
}
@@ -424,23 +415,6 @@ static int rmi_f01_config(struct rmi_function *fn)
return 0;
}
-static int rmi_f01_probe(struct rmi_function *fn)
-{
- struct rmi_driver_data *driver_data =
- dev_get_drvdata(&fn->rmi_dev->dev);
- int error;
-
- error = rmi_f01_alloc_memory(fn, driver_data->num_of_irq_regs);
- if (error)
- return error;
-
- error = rmi_f01_initialize(fn);
- if (error)
- return error;
-
- return 0;
-}
-
#ifdef CONFIG_PM_SLEEP
static int rmi_f01_suspend(struct device *dev)
{
--
1.8.5.3
^ permalink raw reply related
* [PATCH 10/11] Input: synaptics-rmi4 - make accessor for platform data return const pointer
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
When accessing platform data of RMI device let's make sure we do not
accidentally change data that may be shared by returning const pointer.
Also switch to an inline function instead of macro to ensure type safety.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_bus.h | 7 ++++++-
drivers/input/rmi4/rmi_driver.c | 8 ++++----
drivers/input/rmi4/rmi_f01.c | 2 +-
drivers/input/rmi4/rmi_f11.c | 2 +-
4 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index 8d47f52..02a2af8 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h
@@ -224,7 +224,12 @@ struct rmi_device {
};
#define to_rmi_device(d) container_of(d, struct rmi_device, dev)
-#define to_rmi_platform_data(d) ((d)->xport->dev->platform_data)
+
+static inline const struct rmi_device_platform_data *
+rmi_get_platform_data(struct rmi_device *d)
+{
+ return dev_get_platdata(d->xport->dev);
+}
bool rmi_is_physical_device(struct device *dev);
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 736cfbd..473efbc 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -598,7 +598,7 @@ static int rmi_initial_reset(struct rmi_device *rmi_dev,
u16 cmd_addr = pdt->page_start + pdt->command_base_addr;
u8 cmd_buf = RMI_DEVICE_RESET_CMD;
const struct rmi_device_platform_data *pdata =
- to_rmi_platform_data(rmi_dev);
+ rmi_get_platform_data(rmi_dev);
error = rmi_write_block(rmi_dev, cmd_addr, &cmd_buf, 1);
if (error) {
@@ -621,7 +621,7 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
{
struct device *dev = &rmi_dev->dev;
struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
- struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ const struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
int *current_irq_count = ctx;
struct rmi_function *fn;
int i;
@@ -745,7 +745,7 @@ static int rmi_driver_remove(struct device *dev)
struct rmi_device *rmi_dev = to_rmi_device(dev);
struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
const struct rmi_device_platform_data *pdata =
- to_rmi_platform_data(rmi_dev);
+ rmi_get_platform_data(rmi_dev);
disable_sensor(rmi_dev);
rmi_free_function_list(rmi_dev);
@@ -781,7 +781,7 @@ static int rmi_driver_probe(struct device *dev)
rmi_driver = to_rmi_driver(dev->driver);
rmi_dev->driver = rmi_driver;
- pdata = to_rmi_platform_data(rmi_dev);
+ pdata = rmi_get_platform_data(rmi_dev);
data = devm_kzalloc(dev, sizeof(struct rmi_driver_data), GFP_KERNEL);
if (!data) {
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 0e21004..36fcf8d 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -184,7 +184,7 @@ static int rmi_f01_probe(struct rmi_function *fn)
{
struct rmi_device *rmi_dev = fn->rmi_dev;
struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
- const struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ const struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
struct f01_data *f01;
size_t f01_size;
int error;
diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.c
index 2aa7d17..5072437 100644
--- a/drivers/input/rmi4/rmi_f11.c
+++ b/drivers/input/rmi4/rmi_f11.c
@@ -1176,7 +1176,7 @@ static int rmi_f11_initialize(struct rmi_function *fn)
u16 control_base_addr;
u16 max_x_pos, max_y_pos, temp;
int rc;
- struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ const struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
struct f11_2d_sensor *sensor;
u8 buf;
--
1.8.5.3
^ permalink raw reply related
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.
From: Dmitry Torokhov @ 2014-02-13 6:15 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, David Herrmann, Jiri Kosina, Courtney Cavin
In-Reply-To: <52FC2E68.1060709@synaptics.com>
On Wed, Feb 12, 2014 at 06:31:04PM -0800, Christopher Heiny wrote:
> Input: synaptics-rmi4 - Use put_device() and device_type.release()
> to free storage.
>
> From: Christopher Heiny <cheiny@synaptics.com>
>
> 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.
>
> Allocate irq_mask as part of struct rmi_function.
>
> Delete unused rmi_driver_irq_get_mask() function.
>
> Suggested-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>
> ---
>
> drivers/input/rmi4/rmi_bus.c | 17 ++++++++-------
> drivers/input/rmi4/rmi_bus.h | 2 +-
> drivers/input/rmi4/rmi_driver.c | 46
> +++++------------------------------------
> 3 files changed, 15 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> index 96a76e7..6342b45 100644
> --- a/drivers/input/rmi4/rmi_bus.c
> +++ b/drivers/input/rmi4/rmi_bus.c
> @@ -34,6 +34,7 @@ static struct dentry *rmi_debugfs_root;
> static void rmi_release_device(struct device *dev)
> {
> struct rmi_device *rmi_dev = to_rmi_device(dev);
> +
> kfree(rmi_dev);
> }
>
> @@ -94,8 +95,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 +112,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);
We also need to tear down debugfs here.
> return error;
> + }
>
> dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
> pdata->sensor_name, dev_name(&rmi_dev->dev));
> @@ -142,6 +144,7 @@ EXPORT_SYMBOL(rmi_unregister_transport_device);
> static void rmi_release_function(struct device *dev)
> {
> struct rmi_function *fn = to_rmi_function(dev);
> +
> kfree(fn);
> }
>
> @@ -240,16 +243,14 @@ 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;
> + rmi_function_teardown_debugfs(fn);
> + put_device(&fn->dev);
This is not proper place to free structures since
rmi_register_function() was not the one that allocated them.
OK, I split the rmi_driver_irq_get_mask() removal into a separate patch
and applied it, this leaves us with the patch below (BTW, your mailed
damaged - line wrapped - the patch so I had to reconstruct it).
I switched from using device_register/device_unregister to
device_initialize/device_add/device_del/put_device as it allows better
control over error unwinding and destroying the objects which is
important if you want to delete debugfs entries once all fucntion
handler have been unregister but before our memory is gone.
Thanks.
--
Dmitry
Input: synaptics-rmi4 - use put_device() to free devices
From: Christopher Heiny <cheiny@synaptics.com>
For rmi_sensor and rmi_function device_types, use put_device() and
the associated device_type->release() function to clean up related
structures and storage in the correct and safe order.
Allocate irq_mask as part of struct rmi_function.
Suggested-by: Courtney Cavin <courtney.cavin@sonymobile.com>
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_bus.c | 30 +++++++++++++++++++++---------
drivers/input/rmi4/rmi_bus.h | 7 ++++---
drivers/input/rmi4/rmi_driver.c | 25 +++++++------------------
3 files changed, 32 insertions(+), 30 deletions(-)
diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index 96a76e7..7efe7ed 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c
@@ -34,6 +34,7 @@ static struct dentry *rmi_debugfs_root;
static void rmi_release_device(struct device *dev)
{
struct rmi_device *rmi_dev = to_rmi_device(dev);
+
kfree(rmi_dev);
}
@@ -94,11 +95,12 @@ 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;
+ device_initialize(&rmi_dev->dev);
+
rmi_dev->xport = xport;
rmi_dev->number = atomic_inc_return(&transport_device_count) - 1;
@@ -111,14 +113,19 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
rmi_physical_setup_debugfs(rmi_dev);
- error = device_register(&rmi_dev->dev);
+ error = device_add(&rmi_dev->dev);
if (error)
- return error;
+ goto err_put_device;
dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
pdata->sensor_name, dev_name(&rmi_dev->dev));
return 0;
+
+err_put_device:
+ rmi_physical_teardown_debugfs(rmi_dev);
+ put_device(&rmi_dev->dev);
+ return error;
}
EXPORT_SYMBOL_GPL(rmi_register_transport_device);
@@ -131,8 +138,9 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
{
struct rmi_device *rmi_dev = xport->rmi_dev;
+ device_del(&rmi_dev->dev);
rmi_physical_teardown_debugfs(rmi_dev);
- device_unregister(&rmi_dev->dev);
+ put_device(&rmi_dev->dev);
}
EXPORT_SYMBOL(rmi_unregister_transport_device);
@@ -142,6 +150,7 @@ EXPORT_SYMBOL(rmi_unregister_transport_device);
static void rmi_release_function(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
+
kfree(fn);
}
@@ -226,6 +235,8 @@ int rmi_register_function(struct rmi_function *fn)
struct rmi_device *rmi_dev = fn->rmi_dev;
int error;
+ device_initialize(&fn->dev);
+
dev_set_name(&fn->dev, "%s.fn%02x",
dev_name(&rmi_dev->dev), fn->fd.function_number);
@@ -235,27 +246,28 @@ int rmi_register_function(struct rmi_function *fn)
rmi_function_setup_debugfs(fn);
- error = device_register(&fn->dev);
+ error = device_add(&fn->dev);
if (error) {
dev_err(&rmi_dev->dev,
"Failed device_register function device %s\n",
dev_name(&fn->dev));
- goto error_exit;
+ goto err_teardown_debugfs;
}
dev_dbg(&rmi_dev->dev, "Registered F%02X.\n", fn->fd.function_number);
return 0;
-error_exit:
+err_teardown_debugfs:
rmi_function_teardown_debugfs(fn);
return error;
}
void rmi_unregister_function(struct rmi_function *fn)
{
+ device_del(&fn->dev);
rmi_function_teardown_debugfs(fn);
- device_unregister(&fn->dev);
+ put_device(&fn->dev);
}
/**
diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index 5ad94c6..1672b05 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h
@@ -45,14 +45,15 @@ struct rmi_function {
struct rmi_function_descriptor fd;
struct rmi_device *rmi_dev;
struct device dev;
- int num_of_irqs;
- int irq_pos;
- unsigned long *irq_mask;
struct list_head node;
#ifdef CONFIG_RMI4_DEBUG
struct dentry *debugfs_root;
#endif
+
+ unsigned int num_of_irqs;
+ unsigned int irq_pos;
+ unsigned long irq_mask[];
};
#define to_rmi_function(d) container_of(d, struct rmi_function, dev)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 473efbc..788343a 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -185,7 +185,6 @@ static void rmi_free_function_list(struct rmi_device *rmi_dev)
list_for_each_entry_safe_reverse(fn, tmp,
&data->function_list, node) {
list_del(&fn->node);
- kfree(fn->irq_mask);
rmi_unregister_function(fn);
}
}
@@ -617,7 +616,7 @@ static int rmi_initial_reset(struct rmi_device *rmi_dev,
}
static int rmi_create_function(struct rmi_device *rmi_dev,
- void *ctx, const struct pdt_entry *pdt)
+ void *ctx, const struct pdt_entry *pdt)
{
struct device *dev = &rmi_dev->dev;
struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
@@ -630,7 +629,9 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
dev_dbg(dev, "Initializing F%02X for %s.\n",
pdt->function_number, pdata->sensor_name);
- fn = kzalloc(sizeof(struct rmi_function), GFP_KERNEL);
+ fn = kzalloc(sizeof(struct rmi_function) +
+ BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
+ GFP_KERNEL);
if (!fn) {
dev_err(dev, "Failed to allocate memory for F%02X\n",
pdt->function_number);
@@ -646,22 +647,12 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
fn->irq_pos = *current_irq_count;
*current_irq_count += fn->num_of_irqs;
- fn->irq_mask = kzalloc(
- BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
- GFP_KERNEL);
- 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;
- }
-
for (i = 0; i < fn->num_of_irqs; i++)
set_bit(fn->irq_pos + i, fn->irq_mask);
error = rmi_register_function(fn);
if (error)
- goto err_free_irq_mask;
+ goto err_put_fn;
if (pdt->function_number == 0x01)
data->f01_container = fn;
@@ -670,10 +661,8 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
return RMI_SCAN_CONTINUE;
-err_free_irq_mask:
- kfree(fn->irq_mask);
-err_free_mem:
- kfree(fn);
+err_put_fn:
+ put_device(&fn->dev);
return error;
}
^ permalink raw reply related
* Re: [PATCH 01/15] Input: synaptics-rmi4 - fix checkpatch.pl, sparse and GCC warnings
From: Dmitry Torokhov @ 2014-02-13 6:36 UTC (permalink / raw)
To: Christopher Heiny; +Cc: Courtney Cavin, linux-input
In-Reply-To: <52F2E709.5060007@synaptics.com>
On Wed, Feb 05, 2014 at 05:36:09PM -0800, Christopher Heiny wrote:
> On 02/05/2014 05:09 PM, Dmitry Torokhov wrote:
> >On Tue, Feb 04, 2014 at 03:08:12PM -0800, Christopher Heiny wrote:
> >>>On 01/23/2014 04:00 PM, Courtney Cavin wrote:
> >>>> >Cc: Christopher Heiny<cheiny@synaptics.com>
> >>>> >Cc: Dmitry Torokhov<dmitry.torokhov@gmail.com>
> >>>> >Signed-off-by: Courtney Cavin<courtney.cavin@sonymobile.com>
> >>>> >---
> >>>> > drivers/input/rmi4/rmi_bus.c | 4 ++--
> >>>> > drivers/input/rmi4/rmi_bus.h | 2 +-
> >>>> > drivers/input/rmi4/rmi_driver.c | 17 ++++++++++++-----
> >>>> > drivers/input/rmi4/rmi_f11.c | 4 +++-
> >>>> > 4 files changed, 18 insertions(+), 9 deletions(-)
> >>>> >
> >>>> >diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> >>>> >index 96a76e7..8a939f3 100644
> >>>> >--- a/drivers/input/rmi4/rmi_bus.c
> >>>> >+++ b/drivers/input/rmi4/rmi_bus.c
> >>>> >@@ -37,7 +37,7 @@ static void rmi_release_device(struct device *dev)
> >>>> > kfree(rmi_dev);
> >>>> > }
> >>>> >
> >>>> >-struct device_type rmi_device_type = {
> >>>> >+static struct device_type rmi_device_type = {
> >>>> > .name = "rmi_sensor",
> >>>> > .release = rmi_release_device,
> >>>> > };
> >>>
> >>>This struct is used by diagnostic modules to identify sensor
> >>>devices, so it cannot be static.
> >
> >Then we need to declare it somewhere or provide an accessor function.
>
> Currently it's in a header not included in the patches. We'll move
> it to rmi_bus.h.
Hmm, we do have rmi_is_physical_device() to identify whether it is a
sensor or a function, so I believe we should mark all structures static
to avoid anyone poking at them.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH v2] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Barry Song @ 2014-02-13 6:38 UTC (permalink / raw)
To: dmitry.torokhov, linux-input
Cc: workgroup.linux, Xianglong Du, Rongjun Ying, Barry Song
From: Xianglong Du <Xianglong.Du@csr.com>
this patch adds a delayed_work to detect the untouch of onkey since HW will
not generate interrupt for it.
at the same time, we move the KEY event to POWER instead of SUSPEND, which
will be suitable for both Android and Linux. Userspace PowerManager Daemon
will decide to suspend or shutdown based on how long we have touched onkey
Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
-v2:
avoid the race of reschedule the work in remove;
fix the typo about reporting KEY_POWER;
drivers/input/misc/sirfsoc-onkey.c | 60 +++++++++++++++++++++++++++---------
1 files changed, 45 insertions(+), 15 deletions(-)
diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
index e8897c3..4d13903 100644
--- a/drivers/input/misc/sirfsoc-onkey.c
+++ b/drivers/input/misc/sirfsoc-onkey.c
@@ -13,16 +13,45 @@
#include <linux/input.h>
#include <linux/rtc/sirfsoc_rtciobrg.h>
#include <linux/of.h>
+#include <linux/workqueue.h>
struct sirfsoc_pwrc_drvdata {
u32 pwrc_base;
struct input_dev *input;
+ int irq;
+ struct delayed_work work;
};
#define PWRC_ON_KEY_BIT (1 << 0)
#define PWRC_INT_STATUS 0xc
#define PWRC_INT_MASK 0x10
+#define PWRC_PIN_STATUS 0x14
+#define PWRC_KEY_DETECT_UP_TIME 20 /* ms*/
+
+static inline int sirfsoc_pwrc_is_on_key_down(
+ struct sirfsoc_pwrc_drvdata *pwrcdrv)
+{
+ int state = sirfsoc_rtc_iobrg_readl(
+ pwrcdrv->pwrc_base + PWRC_PIN_STATUS)
+ & PWRC_ON_KEY_BIT;
+ return !state; /* ON_KEY is active low */
+}
+
+static void sirfsoc_pwrc_report_event(struct work_struct *work)
+{
+ struct sirfsoc_pwrc_drvdata *pwrcdrv =
+ container_of((struct delayed_work *)work,
+ struct sirfsoc_pwrc_drvdata, work);
+
+ if (!sirfsoc_pwrc_is_on_key_down(pwrcdrv)) {
+ input_event(pwrcdrv->input, EV_KEY, KEY_POWER, 0);
+ input_sync(pwrcdrv->input);
+ } else {
+ schedule_delayed_work(&pwrcdrv->work,
+ msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME));
+ }
+}
static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
{
@@ -34,17 +63,11 @@ static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
sirfsoc_rtc_iobrg_writel(int_status & ~PWRC_ON_KEY_BIT,
pwrcdrv->pwrc_base + PWRC_INT_STATUS);
- /*
- * For a typical Linux system, we report KEY_SUSPEND to trigger apm-power.c
- * to queue a SUSPEND APM event
- */
- input_event(pwrcdrv->input, EV_PWR, KEY_SUSPEND, 1);
- input_sync(pwrcdrv->input);
- /*
- * Todo: report KEY_POWER event for Android platforms, Android PowerManager
- * will handle the suspend and powerdown/hibernation
- */
+ input_event(pwrcdrv->input, EV_KEY, KEY_POWER, 1);
+ input_sync(pwrcdrv->input);
+ schedule_delayed_work(&pwrcdrv->work,
+ msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME));
return IRQ_HANDLED;
}
@@ -59,7 +82,6 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct sirfsoc_pwrc_drvdata *pwrcdrv;
- int irq;
int error;
pwrcdrv = devm_kzalloc(&pdev->dev, sizeof(struct sirfsoc_pwrc_drvdata),
@@ -86,15 +108,17 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
pwrcdrv->input->name = "sirfsoc pwrckey";
pwrcdrv->input->phys = "pwrc/input0";
- pwrcdrv->input->evbit[0] = BIT_MASK(EV_PWR);
+ pwrcdrv->input->evbit[0] = BIT_MASK(EV_KEY);
- irq = platform_get_irq(pdev, 0);
- error = devm_request_irq(&pdev->dev, irq,
+ INIT_DELAYED_WORK(&pwrcdrv->work, sirfsoc_pwrc_report_event);
+
+ pwrcdrv->irq = platform_get_irq(pdev, 0);
+ error = devm_request_irq(&pdev->dev, pwrcdrv->irq,
sirfsoc_pwrc_isr, IRQF_SHARED,
"sirfsoc_pwrc_int", pwrcdrv);
if (error) {
dev_err(&pdev->dev, "unable to claim irq %d, error: %d\n",
- irq, error);
+ pwrcdrv->irq, error);
return error;
}
@@ -119,8 +143,14 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
static int sirfsoc_pwrc_remove(struct platform_device *pdev)
{
+ struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(&pdev->dev);
+
device_init_wakeup(&pdev->dev, 0);
+ devm_free_irq(&pdev->dev, pwrcdrv->irq, pwrcdrv);
+
+ cancel_delayed_work_sync(&pwrcdrv->work);
+
return 0;
}
--
1.7.5.4
^ permalink raw reply related
* [PATCH 0/4] input: sirfsoc-onkey - a bundle of minor clean or fix
From: Barry Song @ 2014-02-13 6:40 UTC (permalink / raw)
To: dmitry.torokhov, linux-input; +Cc: workgroup.linux, Barry Song
From: Barry Song <Baohua.Song@csr.com>
Hi Dmitry,
this patchset depends on Xianglong's
"[PATCH v2] input: sirfsoc-onkey - report onkey untouch event by
detecting pin status"
Barry Song (2):
input: sirfsoc-onkey - drop the IRQF_SHARED flag
input: sirfsoc-onkey - update copyright years to 2014
Xianglong Du (2):
input: sirfsoc-onkey - namespace pwrc_resume function
input: sirfsoc-onkey - use dev_get_drvdata instead of
platform_get_drvdata
drivers/input/misc/sirfsoc-onkey.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
--
1.7.5.4
^ 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