[i2c] [patch 1/5] i2c stack can probe()

[David Brownell]

Hi Jean,

FYI I thought I'd point out that these patches ran OK when I tested on
one platform, with both new-style and legacy versions of rtc-ds1307.
Pretty much as expected, but it's always nice to confirm that respinning
patches didn't break anything!  I'll send those platform patches along.


On Friday 02 March 2007 8:28 am, Jean Delvare wrote:
> Hi David,
> 
> On Thu, 15 Feb 2007 00:41:37 -0800, David Brownell wrote:
> > One of a series of I2C infrastructure updates to support enumeration using
> > the standard Linux driver model.
> > 
> > This patch updates probe() and associated hotplug/coldplug support, but
> > not remove(). ...
> > 

> >  static int i2c_device_match(struct device *dev, struct device_driver *drv)
> >  {
> > -	return 1;
> > +	struct i2c_client	*client = to_i2c_client(dev);
> > +	struct i2c_driver	*driver = to_i2c_driver(drv);
> > +
> > +	/* legacy i2c drivers bypass driver model probing entirely;
> > +	 * such drivers scan each i2c adapter/bus themselves.
> > +	 */
> > +	if (!driver->probe)
> > +		return 0;
> 
> Just to clarify things... This function will never actually be called
> for legacy drivers, will it?

I expect it could be called for all drivers; the normal sequence of
events on device registration involves scanning the list of drivers
for one that both match()es and probe()s suceessfully.  The issue is
that legacy drivers can't probe() normally; so this fails them ASAP.


> > +/* uevent helps with hotplug: modprobe -q $(MODALIAS) */
> > +static int i2c_device_uevent(struct device *dev, char **envp, int num_envp,
> > +		      char *buffer, int buffer_size)
> > +{
> > +	struct i2c_client	*client = to_i2c_client(dev);
> > +	int			i = 0, length = 0;
> > +
> > +	if (!dev)
> > +		return -ENODEV;
> 
> Can this realistically happen? I fail to see how.

No, and in fact that's the only change I made in my local copy of
these patches:  replacing that test with one to return early for legacy
drivers, since they can't hotplug:

	if (dev->driver)
		return 0;

In general, legacy drivers and hotplugging mix poorly.


> >  static int i2c_device_probe(struct device *dev)
> >  {
> > -	return -ENODEV;
> > +	struct i2c_client	*client = to_i2c_client(dev);
> > +	struct i2c_driver	*driver;
> > +
> > +	if (!dev->driver)
> > +		return -ENODEV;
> 
> Same question here, can this realistically happen?

That one's puzzling.  No, can't happen.  It'd be a bug.  I think that
must be a curious cut'n'paste artifact.  I'll remove that from my copy.


> > +static /*const*/ struct device_attribute i2c_dev_attrs[] = {
> 
> What's this commented out "const"?

A reminder of some driver model stupidity; it *should* be const.
I'll remove it, as I've tried to do elsewhere.


> > +	__ATTR(name, S_IRUGO, show_client_name, NULL),
> > +	/* modalias helps coldplug:  modprobe $(cat .../modalias) */
> > +	__ATTR(modalias, S_IRUGO, show_client_name, NULL),
> > +	{ },
> > +};
> 
> This means we will create the modalias file for both legacy and new
> style i2c clients. Is it OK?

It's harmless there, yes.  Much simpler to handle the issue that way.


> >  struct bus_type i2c_bus_type = {
> >  	.name		= "i2c",
> > +	.dev_attrs	= i2c_dev_attrs,
> 
> Nice cleanup :)

Simpler device setup code is an all-around win.  :)



> >  int i2c_register_driver(struct module *owner, struct i2c_driver *driver)
> >  {
> > -	struct list_head   *item;
> > -	struct i2c_adapter *adapter;
> >  	int res;
> >  
> > +	/* new style driver methods can't mix with legacy ones */
> > +	if (driver->probe) {
> > +		if (driver->attach_adapter || driver->detach_adapter
> > +				|| driver->detach_client) {
> > +			pr_debug("i2c-core: driver [%s] is confused\n",
> > +					driver->driver.name);
> > +			return -EINVAL;
> > +		}
> > +	}
> 
> Very good idea, I like it. But I believe this deserves at the very
> least a KERN_NOTICE-level message, not just debugging. KERN_WARN or
> KERN_ERR might even be more suitable.

How about pr_info()?  My general policy is to avoid kernel messages
which will only be triggered in setup paths for buggy drivers.  I
figure that if a developer tests at all, they WILL sort that out;
and there's no point in bloating the kernel image for strings that
can't appear except with a driver that the developer obviously never
did basic sanity testing with.  That's why I used pr_debug().


> > +
> >  	/* add the driver to the list of i2c drivers in the driver core */
> >  	driver->driver.owner = owner;
> >  	driver->driver.bus = &i2c_bus_type;
> >  
> >  	res = driver_register(&driver->driver);
> > +	pr_debug("i2c-core: register driver [%s] --> %d\n",
> > +			driver->driver.name, res);
> >  	if (res)
> >  		return res;
> >  
> > -	mutex_lock(&core_lists);
> > -
> > -	pr_debug("i2c-core: driver [%s] registered\n", driver->driver.name);
> 
> I don't like this change. I understand that you want errors to be
> reported, but having a single string for success and error is somewhat
> confusing. I propose that you leave the current debug message in place
> for the success case, and add a separate KERN_ERR-level message for the
> error case. OK?

Again, I did it this way to avoid image bloat.  There's no need for
two messages.  And in fact, if there were to be only one message it
would be on the fault path, not the normal/success path!  But if you
don't like having one unified message, I'll just move the message back
to where it was.  (And then fix up later patches accordingly.)

> 
> > -
> > -	/* now look for instances of driver on our adapters */
> > +	/* legacy drivers scan i2c busses directly */
> >  	if (driver->attach_adapter) {
> > +		struct list_head   *item;
> > +		struct i2c_adapter *adapter;
> > +
> > +		mutex_lock(&core_lists);
> >  		list_for_each(item,&adapters) {
> >  			adapter = list_entry(item, struct i2c_adapter, list);
> >  			driver->attach_adapter(adapter);
> >  		}
> > +		mutex_unlock(&core_lists);
> >  	}
> 
> I like this locking and local variables cleanup, however it really
> doesn't belong to that patch. Can you please provide a separate patch
> instead?

If I must.   It probably involves respinning the whole patch series though,
so I'll hold off until I get your comments on the later patches.


> Looks good overall, thanks.

Thanks.  I'm still hoping to see this patch series land in -mm soon ... :)

- Dave