linux-kernel - Re: [RFC PATCH 03/06] input/rmi4: I2C physical interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdY+hK=zbZz6K-ONyAckfpZ-n+TS5c25AxRETVk1M9weQw@mail.gmail.com>
Date:	Tue, 9 Oct 2012 11:05:42 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Christopher Heiny <cheiny@...aptics.com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Jean Delvare <khali@...ux-fr.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Linux Input <linux-input@...r.kernel.org>,
	Allie Xiong <axiong@...aptics.com>,
	Vivian Ly <vly@...aptics.com>,
	Daniel Rosenberg <daniel.rosenberg@...aptics.com>,
	Alexandra Chen <alexandra.chen@...synaptics.com>,
	Joerie de Gram <j.de.gram@...il.com>,
	Wolfram Sang <w.sang@...gutronix.de>,
	Mathieu Poirier <mathieu.poirier@...aro.org>,
	Linus Walleij <linus.walleij@...ricsson.com>,
	Naveen Kumar Gaddipati <naveen.gaddipati@...ricsson.com>
Subject: Re: [RFC PATCH 03/06] input/rmi4: I2C physical interface

On Sat, Oct 6, 2012 at 6:10 AM, Christopher Heiny <cheiny@...aptics.com> wrote:

> The I2C physical driver is not extensively changed in terms of functionality
> since the previous patch.  Management of the attention GPIO has been moved to
> rmi_driver.c (see previous email), and most of the debug related interfaces
> have been moved from sysfs to debugfs.  Control of the debug features has been
> moved from compile-time to runtime switches available via debugfs.
>
> The core I2C functionality was previously ACKed by Jean Delvare.  I don't
> believe that portion of the code has changed much since then, but we'd
> appreciate a second glance at this.

The above commit blurb looks more like a changelog than a description
of the actual patch. Nothing wrong with that but begin by describing
the patch first.

(...)
> +#ifdef CONFIG_RMI4_DEBUG
> +
> +#include <linux/debugfs.h>
> +#include <linux/uaccess.h>

Just move these up to the common includes. It doesn't matter
that they get included even when debug is not enabled.

> +static int setup_debugfs(struct rmi_device *rmi_dev, struct rmi_i2c_data *data);
> +static void teardown_debugfs(struct rmi_i2c_data *data);

Why do you need to forward-declare these? Can't you just move them
up above the functions using them?

> +struct i2c_debugfs_data {
> +       bool done;

Done with what? ... needs some doc.

> +       struct rmi_i2c_data *i2c_data;
> +};

(...)
> +static int __devinit rmi_i2c_probe(struct i2c_client *client,
> +                                 const struct i2c_device_id *id)
(...)
> +       rmi_phys = kzalloc(sizeof(struct rmi_phys_device), GFP_KERNEL);
(...)
> +       data = kzalloc(sizeof(struct rmi_i2c_data), GFP_KERNEL);

Can you use devm_kzalloc(&client->dev, ...) for these so you don't
need to free() them explicitly?

(...)
> +static int __devexit rmi_i2c_remove(struct i2c_client *client)
> +{
> +       struct rmi_phys_device *phys = i2c_get_clientdata(client);
> +       struct rmi_device_platform_data *pd = client->dev.platform_data;
> +
> +       /* Can I remove this disable_device */
> +       /*disable_device(phys); */

So just delete these two lines then?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/