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

On Sat, Oct 6, 2012 at 6:10 AM, Christopher Heiny [off-list ref] 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