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

[Jean Delvare]

On Fri, 2 Mar 2007 09:27:25 -0800, David Brownell wrote:
> 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.

Good. I will want to test it myself as well on hardware monitoring
chips and/or video devices. And I will ask Alessandro Zummo for
feedback and testing too.

> On Friday 02 March 2007 8:28 am, Jean Delvare wrote:
> > On Thu, 15 Feb 2007 00:41:37 -0800, David Brownell wrote:

[Snipping everything we agree on.]

> > > +/* 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;

You really mean if (!dev->driver)?

> > > +	/* 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().

In general I would tend to agree with you, but during the transition
period where the two models will be supported and drivers will be
migrated from one to the other... I think we're doing a favor to
developers by having an explicit error message printed if they mess up.
We can degrade it to a debug message after some months or years. 

> > >  	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.)

Having debug messages in success cases isn't unusual as far as I know
and can prove useful. I think that the error message is missing here,
and I don't mind you adding it.

> > >  	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.

I don't think so. Just split this patch in two parts, first the locking
fix, second all the rest. The combination of both should result in
exactly what you have here (modulo the other changes you want to make)
so this should not have any effect on the rest of the series.

> > Looks good overall, thanks.
> 
> Thanks.  I'm still hoping to see this patch series land in -mm soon ... :)

Workin' on it :')

-- 
Jean Delvare