[i2c] [patch 2.6.20-rc1 6/6] i2c driver suspend()/resume()/shutdown() support

Jean Delvare khali at linux-fr.org
Fri Jan 5 15:10:12 CET 2007


Hi David,

On Thu, 04 Jan 2007 17:16:52 -0800, David Brownell wrote:
> > >  - Add new suspend(), resume(), and shutdown() methods.  Use them in the
> > >    standard driver model style; document them.
> >
> > Is there any driver using this? Usually we want at least one driver to
> > use a feature before we add that feature to the kernel. Although here I
> > understand that what you're adding is pretty standard.
> 
> More ... it's *MISSING BASIC FUNCTIONALITY* for the driver model.

Well, as I understand it, the functionality was there, just using an
older, now deprecated model?

> Because of the chicken/egg scenario, drivers won't be updated
> (and get system-wide integration retesting) until after the
> mechanism is first put in place.

Looks like we have a first candidate here:
http://lists.lm-sensors.org/pipermail/i2c/2007-January/000701.html
It is indeed using the old model, and will need to be updated when your
patch #6 is applied.

> Over the past couple years there are probably at least half a dozen
> I2C drivers I've touched that should be using suspend()/resume()
> calls, but that never got updated since I2C doesn't support that.
> I don't have access to most of that hardware any more.  Not all
> those drivers are upstream, either.  Realistically, none of them
> can start using i2c suspend/resume until it arrives via "git pull". 
> 
> And the hardware _I've_ got which needs I2C suspend/resume support
> and has been merged upstream can't possibly kick it in until after
> those "Phase II" patches merge...

Yeah, I understand that.

> > > -* [Copyright] Use (C), not (c), for copyright.
> > > +* [Copyright] Use (C), not (c), for copyright.  Provide dates and names:
> > > +  "Copyright (C) 2006 you", "Copyright (C) 1998,2004-2006 company", etc
> >
> > Please back this one out, it's really not specific to i2c drivers and
> > at any rate doesn't belong to this patch.
> 
> In that case the comment about (C) vs (c) doesn't belong in that
> file either ... maybe you should just remove it entirely?  That
> file is full of advice not specific to I2C, for that matter.

I have indeed considered removing this point about the copyright symbol.
But the fact is that most i2c drivers are ported by now, and this
documentation file is no longer actively maintained.

The non-i2c-related parts are explained by the history of this
document, which was originally written for people porting drivers from
the lm_sensors project to Linux 2.6. As the lm_sensors project wasn't
exactly state-of-the-art coding-style-wise, and I didn't want to repeat
the same things over and over again, I mentioned the more frequent
problems here.

> > >  static int i2c_device_match(struct device *dev, struct device_driver *drv)
> > >  {
> > > -	return 1;
> > > +	/* (for now) bypass driver model probing entirely; drivers
> > > +	 * scan each i2c adapter/bus themselves.
> > > +	 */
> > > +	return 0;
> > >  }
> >
> > I'm a bit confused by this one, the match was always succeeding, now
> > it's never succeeding and... it doesn't make any difference?
> 
> Exactly.  The previous comment was wrong.  This particular patch is
> about implementing core driver model mechanisms correctly.

The subject line and header comment say differently; they don't mention
that change, which is why I was asking.

I'd rather have that change moved to the patch where the rest of the
related changes are, so that it is obvious why the change is made.

> And the reason to have it in this patch was so that these trivial
> fixes could be merged into 2.6.20 ... it was originally submitted
> before the 2.6.20 merge window even opened.  I was hoping to see
> I2C drivers start to obey the driver model, at least in these small
> but often-important ways, before next summer. :(

I am the one who decides what i2c-related patches go in what kernel.
And that's not for my own fun, and I'm not rolling a dice. There are
_many_ things to take into consideration. User-space compatibility,
possible unexpected side effects, schedule of impacted subsystems,
etc. Unfortunately, the moment at which you submitted the patches
doesn't quite matter here, what matters is the moment the patches are
ready to be merged, and it is mostly unpredictable.

> >	This
> > change doesn't appear to belong to this patch, if it makes any sense at
> > all.
> 
> I suppose I can reissue this patch.  For the sixth (?) time.
> Then this particular change would be the first part of "Part II"
> 
> Redoing all those "Part II" patches (again) will be a real PITA.

Your first series of patches, not including this one, touches 95
drivers in 7 different subsystems, this deserved extra attention, which
I've put in reviewing the patches several times and testing on several
systems. I found quite a few bugs in your patches, so it wasn't in
vain; it was needed.

You'll redo your patches as many times as needed. And I'll review them
as many times as needed. This is how the Linux kernel development
works, you should be used to it by now. Things get merged when they are
ready, not before.

Don't get me wrong, BTW: I'm really happy that you're working on these
large i2c-core changes with me (well, the other way around actually).
And I'm sorry it's taking so long. I'm sorry I am only spending 20
hours a week or so working of the i2c subsystem, and that I don't
receive much help (besides yours), and that there are a number of
things I still have to learn. I'm doing my best, you know.

> > What about moving i2c_device_suspend() and i2c_device_resume() down
> > after i2c_device_shutdown() to group related functions together?
> 
> ISTR that doing that doubled the size of this patch, and made it
> all but unreadable, for some reason known only to "diff".
> 
> The shutdown() isn't actually related to suspend()/resume(), except
> that this patch adds all three missing driver model mechanisms at
> the same time.

Well, shutdown() is more related to suspend() and resume() than to
probe() and match(), if you want my opinion. All three are about power
state changes, while the rest is about devices being added or removed.
No big deal anyway, I can make that change myself before pushing the
patch.

All the other patches from your series 1 are in my public tree by now,
and will be in the next -mm kernel. This one is the only one missing;
when it's done, I can start reviewing series 2 (not immediately though,
I must take care of the lm_sensors 2.10.2 release first.

http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/

Thanks,
-- 
Jean Delvare



More information about the i2c mailing list