| Message ID | 4E2E1897.9020808@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 26, 2011 at 09:29:59AM +0800, Wanlong Gao wrote: > On 07/26/2011 02:37 AM, Dmitry Torokhov wrote: > >On Mon, Jul 25, 2011 at 07:29:28PM +0100, Mark Brown wrote: > >>On Mon, Jul 25, 2011 at 11:19:16AM -0700, Dmitry Torokhov wrote: > >> > >>>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. > So, Dmitry, do you means this ? > > Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com> > --- > 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. > + > + 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/26/2011 12:39 PM, Greg KH wrote: >> + >> + 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 Yeah, I see. But Greg, why does i2c-core do this? like:drivers/i2c/i2c-core.c: static int i2c_device_remove(struct device *dev) { struct i2c_client *client = i2c_verify_client(dev); struct i2c_driver *driver; int status; if (!client || !dev->driver) return 0; driver = to_i2c_driver(dev->driver); if (driver->remove) { dev_dbg(dev, "remove\n"); status = driver->remove(client); } else { dev->driver = NULL; status = 0; } if (status == 0) { client->driver = NULL; i2c_set_clientdata(client, NULL); } return status; } And now, I'm in a fog, can you clear me/us ?
On Tue, Jul 26, 2011 at 01:36:00PM +0800, Wanlong Gao wrote: > On 07/26/2011 12:39 PM, Greg KH wrote: > > >>+ > >>+ 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 > Yeah, I see. > > But Greg, why does i2c-core do this? > like:drivers/i2c/i2c-core.c: The i2c core has different requirements than the driver core does, right? They are two totally different things, please don't assume that the rules for one are the same for the other. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/26/2011 02:02 PM, Greg KH wrote: > On Tue, Jul 26, 2011 at 01:36:00PM +0800, Wanlong Gao wrote: >> On 07/26/2011 12:39 PM, Greg KH wrote: >> >>>> + >>>> + 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 >> Yeah, I see. >> >> But Greg, why does i2c-core do this? >> like:drivers/i2c/i2c-core.c: > > The i2c core has different requirements than the driver core does, > right? They are two totally different things, please don't assume that > the rules for one are the same for the other. > > greg k-h > Hmm...They are totally different things, maybe I see..
On Tue, Jul 26, 2011 at 02:25:48PM +0800, Wanlong Gao wrote: > On 07/26/2011 02:02 PM, Greg KH wrote: > >On Tue, Jul 26, 2011 at 01:36:00PM +0800, Wanlong Gao wrote: > >>On 07/26/2011 12:39 PM, Greg KH wrote: > >> > >>>>+ > >>>>+ 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 > >>Yeah, I see. > >> > >>But Greg, why does i2c-core do this? > >>like:drivers/i2c/i2c-core.c: > > > >The i2c core has different requirements than the driver core does, > >right? They are two totally different things, please don't assume that > >the rules for one are the same for the other. > > > >greg k-h > > > Hmm...They are totally different things, maybe I see.. Still, it would make sense to clean up platform device's drvdata pointer, so that every platform driver out there does not have to do it on its own.
On Mon, Jul 25, 2011 at 11:45:20PM -0700, Dmitry Torokhov wrote: > On Tue, Jul 26, 2011 at 02:25:48PM +0800, Wanlong Gao wrote: > > On 07/26/2011 02:02 PM, Greg KH wrote: > > >On Tue, Jul 26, 2011 at 01:36:00PM +0800, Wanlong Gao wrote: > > >>On 07/26/2011 12:39 PM, Greg KH wrote: > > >> > > >>>>+ > > >>>>+ 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 > > >>Yeah, I see. > > >> > > >>But Greg, why does i2c-core do this? > > >>like:drivers/i2c/i2c-core.c: > > > > > >The i2c core has different requirements than the driver core does, > > >right? They are two totally different things, please don't assume that > > >the rules for one are the same for the other. > > > > > >greg k-h > > > > > Hmm...They are totally different things, maybe I see.. > > Still, it would make sense to clean up platform device's drvdata > pointer, so that every platform driver out there does not have to do it > on its own. Again, it shouldn't need to be "cleaned" up, as no one relies on it being there. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 26, 2011 at 09:41:06AM -0700, Greg KH wrote: > On Mon, Jul 25, 2011 at 11:45:20PM -0700, Dmitry Torokhov wrote: > > On Tue, Jul 26, 2011 at 02:25:48PM +0800, Wanlong Gao wrote: > > > On 07/26/2011 02:02 PM, Greg KH wrote: > > > >On Tue, Jul 26, 2011 at 01:36:00PM +0800, Wanlong Gao wrote: > > > >>On 07/26/2011 12:39 PM, Greg KH wrote: > > > >> > > > >>>>+ > > > >>>>+ 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 > > > >>Yeah, I see. > > > >> > > > >>But Greg, why does i2c-core do this? > > > >>like:drivers/i2c/i2c-core.c: > > > > > > > >The i2c core has different requirements than the driver core does, > > > >right? They are two totally different things, please don't assume that > > > >the rules for one are the same for the other. > > > > > > > >greg k-h > > > > > > > Hmm...They are totally different things, maybe I see.. > > > > Still, it would make sense to clean up platform device's drvdata > > pointer, so that every platform driver out there does not have to do it > > on its own. > > Again, it shouldn't need to be "cleaned" up, as no one relies on it > being there. No one should rely on it being there however I came across quite a few MFD patches that tried passing parent's data in that pointer. Hopefully they are all cleaned now but we getting new drivers all the time... If we had it in the platform bus code such uses would break even before we get such drivers and we won't have to deal with them. Thanks.
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; + + if (!drv->probe) + return -ENODEV; + + dev_dbg(_dev, "probe\n"); + + status = drv->probe(dev); + if (status) + platform_set_drvdata(dev, NULL); + + 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; + + if (drv->remove) { + dev_dbg(_dev, "remove\n"); + status = drv->remove(dev); + } else { + status = 0; + } + if (status == 0) + platform_set_drvdata(dev, NULL); - return drv->remove(dev); + return status; } static void platform_drv_shutdown(struct device *_dev)