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 <[email protected]>
> > Cc: Dmitry Torokhov <[email protected]>
> > Cc: Benjamin Tissoires <[email protected]>
> > Cc: Linux Walleij <[email protected]>
> > Cc: David Herrmann <[email protected]>
> > Cc: Jiri Kosina <[email protected]>
> >
> > ---
> >
> > 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 <[email protected]>
>
> - 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 <[email protected]>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
