Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys
From: Greg KH <gregkh@suse.de>
Date: 2011-07-26 04:44:21
On Tue, Jul 26, 2011 at 09:29:59AM +0800, Wanlong Gao wrote:
On 07/26/2011 02:37 AM, Dmitry Torokhov wrote:quoted
On Mon, Jul 25, 2011 at 07:29:28PM +0100, Mark Brown wrote:quoted
On Mon, Jul 25, 2011 at 11:19:16AM -0700, Dmitry Torokhov wrote:quoted
Right, like i2c bus we could just have platform core clean up platform drvdata pointer after calling ->remove() and also if ->probe() errors out.I2C doesn't do this (at least not any more).Sure does. See drivers/i2c/i2c-core.c::i2c_device_probe() and i2c_device_remove().Yeah, I see it. Sure i2c does. Let the Core to do the pointer's clean up is very good idea.
No it isn't.
quoted hunk ↗ jump to hunk
So, Dmitry, do you means this ? Signed-off-by: Wanlong Gao <redacted> --- drivers/base/platform.c | 30 ++++++++++++++++++++++++++++-- 1 files changed, 28 insertions(+), 2 deletions(-)diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 6040717..349e71b 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c@@ -405,8 +405,21 @@ static int platform_drv_probe(struct device *_dev) { struct platform_driver *drv = to_platform_driver(_dev->driver); struct platform_device *dev = to_platform_device(_dev); + int status; - return drv->probe(dev); + if (!dev) + return 0;
When would dev ever be NULL? Wouldn't that be an error?
+ + if (!drv->probe) + return -ENODEV;
That's not a valid error for this. And when would probe ever be NULL?
+ + dev_dbg(_dev, "probe\n");
Why add this noise?
+ + status = drv->probe(dev); + if (status) + platform_set_drvdata(dev, NULL);
No, not ok.
quoted hunk ↗ jump to hunk
+ + return status; } static int platform_drv_probe_fail(struct device *_dev)@@ -418,8 +431,21 @@ static int platform_drv_remove(struct device *_dev) { struct platform_driver *drv = to_platform_driver(_dev->driver); struct platform_device *dev = to_platform_device(_dev); + int status; + + if (!dev) + return 0;
Again, when would that ever happen?
+
+ if (drv->remove) {
+ dev_dbg(_dev, "remove\n");
+ status = drv->remove(dev);
+ } else {
+ status = 0;
+ }Again, why would remove ever be NULL? This whole thing isn't needed at all. greg k-h