[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BFABBC009FBA1C43B207B15FD485ABD922C22C1F@USW-MAIL1.synaptics-inc.local>
Date: Thu, 11 Oct 2012 04:21:12 +0000
From: Christopher Heiny <Cheiny@...aptics.com>
To: Linus Walleij <linus.walleij@...aro.org>
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>,
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>,
Alexandra Chin <alexandra.chin@...synaptics.com>
Subject: RE: [RFC PATCH 03/06] input/rmi4: I2C physical interface
Linus Walleij wrote:
> 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.
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.--
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/
Powered by blists - more mailing lists