[i2c] [patch 2/5] i2c can remove()

[Jean Delvare]

David,

On Fri, 2 Mar 2007 10:42:06 -0800, David Brownell wrote:
> On Friday 02 March 2007 9:09 am, Jean Delvare wrote:
> > On Thu, 15 Feb 2007 00:41:59 -0800, David Brownell wrote:
> > > +static void i2c_unregister_device(struct i2c_client *client)
> > > +{
> > > +	struct i2c_adapter	*adap = client->adapter;
> > > +	struct i2c_driver	*driver = client->driver;
> > > +	int			status;
> > > +
> > > +	if (driver && !driver->remove) {
> > > +		dev_err(&client->dev, "can't unregister devices "
> > > +			"with legacy drivers\n");
> > > +		WARN_ON(1);
> > > +		return;
> > > +	}
> > 
> > I'm not sure I agree with this. Isn't it valid for a (device driver
> > model compliant) driver not to have a remove() method? Also, I don't
> > quite see how this method would ever be called for a legacy driver, as
> > you check explicitely against that at registration time.
> 
> A later patch in this series makes this method public, so those
> cases could happen that way.  The overall patch series is simpler
> (and smaller) if this routine doesn't need to change twice.

OTOH, moving this addition to the patch in question would make it
clearer why the test was needed.

> > > +
> > > +	/* unbind driver */
> > > +	if (driver) {
> > > +		/* REVISIT driver model core would handle this for us, and
> > > +		 * once i2c_detach_client() stops updating lists, it should.
> > > +		 */
> > 
> > Actually my current i2c stack no longer has the faulty i2c_adapter
> > driver, so it should be possible to remove the duplicate list
> > management. Could you provide a patch doing this? Would it let you
> > make this one patch cleaner?
> 
> I'd much rather get this patch series merged first, then clean up those
> superfluous lists later.  Removing those lists first implies respinning
> most patches in this series (not just this one), extra testing time, and
> further magnifies the delays involved.  Simpler to remove them later.

OK, fine with me. But don't wait too much after this series is reviewed
and pushed to -mm before sending the one cleaning up the list,
otherwise I fear it'll never happen.

> > > @@ -444,12 +508,13 @@ int i2c_del_driver(struct i2c_driver *dr
> > >  			}
> > >  		}
> > >  	}
> > > +out_unlock:
> > > +	mutex_unlock(&core_lists);
> > >  
> > > +unregister:
> > >  	driver_unregister(&driver->driver);
> > >  	pr_debug("i2c-core: driver [%s] unregistered\n", driver->driver.name);
> > >  
> > > - out_unlock:
> > > -	mutex_unlock(&core_lists);
> > >  	return 0;
> > >  }
> > 
> > It's not correct for legacy drivers, I think. The original code did
> > _not_ call driver_unregister on error, and now it does. I don't think
> > your patch is supposed to change anything for legacy drivers, is it?
> 
> Hmm, you have a point.  But if you think for a moment about what the
> code would look like if it preserves that original behavior:
> 
> 	if (new-style driver)
> 		driver_unregister();
> 	else
> 		current code;
> 
> Maybe it would be better if i2c_del_driver() were legacy-only, and
> new-style drivers used a new i2c_unregister_driver() call.  (And if
> we go that route, maybe similarly on the registration side.)  What
> would you think of that approach?

Yes, after thinking about it some more, if there is that little
common code, having separate functions makes sense. And this will
pretty much guarantee that your patches don't affect the current
drivers.

> > When the patches are ready, I think I'll fold this patch into the first
> > one (i2c stack can probe()). Each doesn't make much sense without the
> > other.
> 
> If you want to merge the patches, that's your call.  I will point out
> that it's common to split out sequences of patches this way ... and in
> fact, neither of these make sense without the later patches either.
> So in a sense it's a tradeoff between One Big Patch (that's not very
> digestible/reviewable) and a series of bite-sized ones.

Good point. I admit I am happy to have smaller drivers to review, and
as long as nothing breaks between patches, it's acceptable to merge
them upstream that way.

-- 
Jean Delvare