Linus Walleij wrote:
On Sat, Oct 6, 2012 at 6:10 AM, Christopher Heiny [off-list ref] wrote:
quoted
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.
Good point. I was describing the patch, but not from the correct point of view. :-)
[snip some items covered in a previous email]
quoted
+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?
Probably. We'll do that if possible.
quoted
+struct i2c_debugfs_data {
+ bool done;
Done with what? ... needs some doc.
OK.
quoted
+ struct rmi_i2c_data *i2c_data;
+};
(...)
quoted
+static int __devinit rmi_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
(...)
quoted
+ rmi_phys = kzalloc(sizeof(struct rmi_phys_device), GFP_KERNEL);
(...)
quoted
+ 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?
Hmmmmmm. That looks like a merge regression - I'm pretty sure we implemented devm_kzalloc there.
(...)
quoted
+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?
Yes.