Messages in this thread | | | From | Christopher Heiny <> | Subject | RE: [RFC PATCH 03/06] input/rmi4: I2C physical interface | Date | Thu, 11 Oct 2012 04:21:12 +0000 |
| |
Linus Walleij wrote: > On Sat, Oct 6, 2012 at 6:10 AM, Christopher Heiny <cheiny@synaptics.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.
Good point. I was describing the patch, but not from the correct point of view. :-)
[snip some items covered in a previous email]
> > > +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.
> > > +struct i2c_debugfs_data { > > + bool done; > > Done with what? ... needs some doc.
OK.
> > > + 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?
Hmmmmmm. That looks like a merge regression - I'm pretty sure we implemented devm_kzalloc there.
> > (...) > > > +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.
| |