i2c driver for TPS6501x

Attached is a patch against a GIT snapshot from yesterday, adding support for Texas Instruments TPS6501x power management chips(*).

These chips are used with a variety of OMAP boards including cell phones, cameras, and more. I don't think anyone's submitted drivers for any similar chip yet; like the ones from FreeScale (for working with Motorola based phones) or Dialog (for Intel PXA based ones).

This particular driver has been used in the Linux-OMAP tree for most of the last year, accumulating updates as needed to support various boards, and it seems now might be a good time to push it upstream. (The current kernel already includes a header file with part of the API exposed by this driver, FWIW.)

So -- please let me know about any problems you notice which you think may need to be fixed before this merges. (And I'm not on the "sensors" list, so please CC me.) I already know some boards will need a hook to the report VBUS IRQ to non-OTG USB transceiver drivers, but that can wait for a while. :)

- Dave

(*) http://focus.ti.com/docs/prod/folders/print/tps65010.html
for more info. Hmm, where did that tps65014 thing come from? It wasn't there last week, I swear!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: tps65010.patch
Type: text/x-diff
Size: 34681 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050505/53cba30d/tps65010.bin

OK, so no feedback ... seems like there are no issues. In that case, please merge this into the next set of I2C driver patches.

The official theory is after all to move all drivers into the main kernel tree ... :)

- Dave

Hello,

Sorry not this way. The driver is too complicated for me and Jean. Others seems to be more busy these days so nobody who could be qualified for review responded.

I suggest you to make sure that coding style is OK. Check the Documentation/CodingStyle file please. Then you can continue with SubmittingPatches.

I2C subsystem handles Greg Kroah [email protected]

So I'm suggesting to ask people for review on LKML Dont forget to CC greg too in that case.

Again I'm very sorry I cant help you right now.

regards

Rudolf

Complicated? Heh! What's complicated about it? It's common for drivers to use IRQs and workqueues, and expose APIs to other drivers. Not for "sensors" drivers, true; most of them don't expose APIs to other parts of the system (except by sysfs).

From an I2C list I'd expect feedback about I2C issues more than about how it relates to system booting and power management.

Or maybe about how much simpler such drivers can be when I2C finally gets an async message based API ... though this is not one of the drivers that'd _really_ benefit from that, IMO. ;)

Well, the folk working various boards that rely on this driver to boot haven't been complaining. In fact they've found it straightforward to add new capabilities as needed. Those are the folk I'd normally call "most qualified" to comment, on anything except maybe I2C-specific issues.

In general this would seem to be a "new type" of I2C driver, because of the system role it serves. I brought it up on the linux-pm list a month or two ago in response to a question about how Linux should handle such chips ... there are a few other chips like it, used in cell phones and other battery powered devices, but this seems to be the first such driver that's generally available. (And there's a new CEA standard for such chips, combining battery and voltage management with USB OTG transceiver. Also using I2C ... the OTG parts really crave an async I2C API, it's unnatural to force that into the current synchronous model. If you were to call the OTG stuff complicated, I'd agree!)

Did you have any specific issue to raise? Otherwise I'm not going to bother; it's quite conformant already, and any minor differences are well within the accepted level of variability for kernel code.

Hmm, Greg already said he'd merge it, though I see he's not had time to do it yet.

Greg -- do you think this needs review on LKML?
That is, _before_ merging.

- Dave

Hi Dave,

Nothing is fundamentally complicated. How much complicated we think a thing is mostly depends on what we are used to deal with. It happens that the driver you submitted deals we an architecture we don't know much about, is for a kind of chip we never met before, and uses many kernel features that are never found in hardware monitoring drivers. This means that we couldn't properly review your code without investing a significant amount of our time in learning about this new context. Other folks on other lists are likely to be more skilled that us for the job. This is the reason why I did not review your driver.

That's a different issue then. If all you expect from us is a review of the i2c part of the code, that's something I may do. Maybe this is how we should have understood your request in the first place. Maybe you could have made it a bit clearer in the first place (or we need to become more clever). That being said, it ain't easy to read *only* a part of a driver. You have to understand how things work, and this require to read the whole code at some point.

No idea on this, see with Corey Minyard, he's the one working on this. This is a big change he is proposing, and I do not have the time to take a look at it for now. I would rather move all non-i2c hardware monitoring drivers out of the i2c subsystem before changing it significantly.

Here are a few random comments on your code, as I finally have some time to review it. Per your own request, I skipped the parts dealing with things I am not skilled in.

Not that it really matters, but usual name for unused variables is "dummy".

You are limiting yourself to a single device by doing so. This is against the design of the i2c subsystem. What would it take to allow an unlimited number of devices?

That would rather be = NULL.

This function, as well as the rest of the interface, solely relies on the fact that only chip can be handled by the driver. This may show its limit at some point, causing an interface change. You could make it so that the users of the interface have a pointer to the i2c client and pass it to the function. That way, the interface would not need to be changed for multiple device support.

Why not simply #ifdef?

What if the driver is compiled as a module? (This is really a question, I don't know and am willing to learn.)

And one additional note about debugging. That part of the driver is rather big, and the implementation isn't exactly elegant (100 byte local buffers... iirk). It seems to be the consequence of you trying to make debugging available through either the logs or debugfs, and having common code for both methods. This may be overkill for a simple device driver. I would suggest that you either drop one of the two debugging methods, or make the non-debugfs way more simple (you don't really need to decode all the individual bits of each register, this can easily be done in userspace.) Doing so would get rid of a few unelegant constructs too. Just my humble opinion anyway, it's only debugging after all.

--
Jean Delvare

Hi Jean, thanks for the comments. I'll provide some responses now, and the attached patch addresses some of your feedback.

Yes. Also interrelationships with things we don't deal with, which was my suspicion about what the biggest issue was.

Suspicion confirmed! Though there's not a lot that's ARM-specific there except the IRQ trigger type stuff, and machine_is_XXX() checks for board-specific logic.

The "isp1301_omap" driver uses even more kernel features, FWIW, but it's not exactly a "hardware monitoring" driver either. I2C being used in both cases as a low-speed system control bus, which happens to have the nice feature of needing only two signals routed. That is, in addition to the chip's IRQ. :)

True enough.

I looked at it when he first proposed it, and applauded. Haven't had reason to look at it again since then, but I hope it's ready next time I need to write an I2C driver tht processes hardware IRQs!

I hope you're not saying what I think you're saying ... why remove everything from the I2C stack except hardware monitoring support?? I2C is used for more than that, and it's the "more" which most needs features like being able to issue requests in IRQ handlers (and then get async completion notifications).

As part of a port to a new board, someone removed the TPS_BASE + 1 there without updating the comment. I think the story is I2C scanning doesn't work right on some of the systems ... and in any case, no board with this chip currently uses Linux and address 0x49.

True enough; it's gone.

Yes, thanks!

I suppose snprintf() is "printf-like" ... ;)

The "hibit" case just started out really evil as an inline expression, though by now it's simplified. As you say about complexity, this is just a case of what you're used to; I guess I'm used to having one-shot things like that in-line. In context, it's unambiguous; and the whole thing started out as a macro way back when. Before the tps65011/13 support produced any need for "hibit".

Actually that just makes the diagnostics annoyingly verbose; there's no point to having more than one of these chips in a system. There's a bit of inconsistency here since several folk have morphed this driver over time, but it's improving. (E.g. now that tps65013 routine could probably be merged into its sibling ... by someone who can test against both types of hardware.)

The compiler does indeed inline those as NOPs, generating no code. That's the whole point of that idiom ... there's just one #ifdef, and the main body of the code doesn't get cluttered with others.

Why does one cast a value into the void? To make it clear that one doesn't want it. Some tools give annoying warnings about unused function return values ... this shuts them up. That particular value is useless here; we don't care if some work has already been scheduled or not.

First and foremost: having more than one of them make sense from a hardware perspective ... :)

Goals for these chips include supporting highly integrated systems with very low chip count. There's no real need for multiple Vcore and Vmain regulators or battery chargers; and sharing for example one VBUS power source between two chips makes no sense. If a system needs more power, it'll use a different chip, not several of these.

The same response applies to the points you made about debug messages and the APIs not accepting some sort of chip ID as a parameter. If multiple chips made sense in some experimental board, the folk working with that would first have to sort out a lot of other issues before their solutions could be reusable.

Huh, I could have sworn I ran "sparse" over this recently to catch such style issues; I guess not. Fixed.

But memory shortages can improve ... I'd have to go back and look at the innards of I2C again, but I think this was mostly to be sure that a failure here couldn't cause a surprise failure elsewhere. I recall thinking this was a place where the driver model and I2C don't quite see eye-to-eye.

OK, simple enough ...

I don't follow. It's an ARM-specific API ... admittedly it's a huge hole in the standard IRQ infrastructure (Linux should have a standard way for drivers to say how their hardware triggers its IRQs), but that also means there's no better #ifdef to use.

Again: the general policy is to avoid scattering #ifdefs throughout code; this avoids exactly such an #ifdef.

What's it even for? The docs show it used, but don't say how a driver author could know if it's needed. Sounds like maybe the docs should stop recommending it be set...

It's a standard idiom used in drivers to wrap pointers to __exit functions, which often vanish when drivers are statically linked. The "__exit_p" evaluates to NULL in that case rather than garbage, preventing oopses.

Most drivers use it for their remove() methods; that's basically
the role of the I2C detach_client() method. See <linux/init.h>
and most hotpluggable PCI drivers.

Drivers that don't much care about small kernel size aren't going to use that for very much; drivers on embedded hardware need to care. The other I2C drivers don't much use __exit markings.

I'll look; contributions from other people weren't necessarily eyeballed by me before they got merged. (I just pulled the latest from GIT.)

You can see it's useless, as can I, but some tools don't seem to understand that if "tries" starts as "3" then this statement is always going to get executed. The initialation avoids annoying warnings.

That's what this code does. :(

The problem seemed to be that on certain boards the I2C controller has startup glitches. Since those boards don't boot Linux properly without this chip running -- as in, essential devices unavailable without the TPS chip being available to manage them -- it's critical that this driver initialize. Usually one re-probe suffices.

Yes, that's annoying, and quite possibly the I2C controller driver is a better place to fix this ... if not the hardware. But until such a fix exists, this is the best workaround we have.

No good reason. It just grew that way. :)

Then <linux/init.h> makes this be the same as module_init(), and various board features will be unusable. Now that the driver is working, I wonder if it even makes sense to support compiling it as a module ... it was of course rather handy during initial development. Now the module option seems to cause nothing but trouble.

(Note that the I2C driver on those systems is also a subsys_initcall, to ensure the TPS chip is available before the drivers relying on it start to initialize ...)

Got a better solution? But 100 bytes doesn't seem worrisome.

Maybe, but it works and has been useful. At this point it'd be more work to remove it than keep it.

And it normally compiles away to nothing, too. :)

Thing is, it's actually handy when debugging to have two different kinds of view of the hardware. One being "right now" during some sort of event sequence (e.g. logging during IRQ handling) and the other being a more static overview of the whole state (debugfs). The two views turn up different kinds of information, and it's easier to use them if they both provide the same decoding.

I expect that when someone implements the "fast battery charge" both kinds of debug info will again be useful. Right now this only uses the hardware default charge rate.

- Dave

-------------- next part --------------
A non-text attachment was scrubbed...
Name: tps.tweaks
Type: text/x-diff
Size: 5952 bytes
Desc: not available
Url : http://lists.atrpms.net/pipermail/lm-sensors/attachments/20050527/3024f956/tps-0001.bin

Hi Dave,

And as a matter of fact it was not posted for review on the lm-sensors list.

I am saying what I am saying, but it looks like you are not understanding what I am saying ;)

Non-i2c hardware monitoring drivers != non-hardware-monitoring i2c drivers. Not all hardware monitoring chips are I2C/SMBus based, and I would like to have these which are not moved to their respective subsystem rather than cluttering the i2c subsystem. We're far from that though.

Funny thing to read when you see how verbose the debugging messages themselves are in this driver ;)

But there is a point in having consistent messages among drivers. You should expect Greg to tell you the same when he finally reviews your driver for inclusion. If he is happy with the way you did, I'll admit you were right and will try to remember for my next review.

I guess you assume very small values of 2 here, right? ;)

You are correct. This needs a full rewrite actually, which will happen someday. I wish I had a little less things to track simultaneously, or more time to work on them.

My point was as follow. Given A and B two unrelated instructions, if A needs to be done under a condition and B needs to done under the opposite condition, I would rather write:

#if condition
	A;

#endif

#if !condition
	B;

#endif

than:

#ifdef condition
	A;
#else
	B;

#endif

I find the first approach less error-prone. If the condition for A changes at some point in time, the second approach is likely to result in a wrong condition being used for B.

Actually everyone is wondering ;)

You know it's needed if you use it yourself. The i2c core doesn't use this ID as far as I remember. I saw some uses of it in some architecture specific drivers, and possibly in some media/video drivers. Ultimately this field may be removed entirely, but as the many things I have to improve on the i2c subsystem, such a change affects several dozen drivers so I try to be careful and do them one at a time.

Patches are welcome :)

I know what __exit_p() is for. What I would like you to explain is why tps65010_detach_client was tagged __exit in the first place. There are two cases where an i2c client can be removed: when its i2c chip driver itself is removed, or when the i2c adapter driver it sits on is removed. I can easily understand that the first case cannot happen if that __exit_p() resolved to NULL. The second case could happen however, unless you prove otherwise. My point is that an i2c_driver.detach_client method can only be tagged __exit_p if you ensure that clients will only attach to i2c adapter drivers which cannot be removed themselves. This is quite a bold statement to hold, if you want my opinion. You would need to do much more tests that you are doing right now on the driver/adapter connection.

Please tag it as such in this case. Alternatively you could probably switch to a do {} while() statement, which such code analysis tools would enjoy more. (This is a pretty simple case BTW, and such code analysis tools should be improved to support it. Also, making the code worse isn't the goal of code analysis tools, yes this is what happens here. Sigh.)

If the I2C controller is faulty, the workaround should be moved to its driver, as you seem to agree upon yourself. See, "such a fix exists" could be made a condition for your tps65010 driver to be accepted in the main kernel tree. It's really easy ;)

And finally some minor comments on your incremental patch:

Jean Delvare

It was posted a few times on the USB and OMAP lists. There are some patches lurking to split it into two parts (now that there's finally some point to doing that, meaning a component that needs swapping), which if you like I'll post to the sensors list for review. (That's still the only Linux I2C list, AFAIK...)

I'll not disagree with that. Good -- you weren't saying what I was afraid you were saying!

It's a question of signal-to-noise ratio, that's all. Interpreting bits doesn't bother most folk (heck, the BSDs even have kernel printf utilities to print bitfields!), but repeating the same string over and over, redundantly, many times again and again, is overkill. :)

Well, he merged it and didn't tell me. I concluded he's either happy, content to not argue, way too busy, or all the above. :)

Given I've been a big contributor to switching for example USB over to use the driver model messages, I suspect he's accepted that I may have good reasons for what I do. For singleton drivers, there really is no point to driver model messages. For things like USB, it really does make a huge difference to be able to tell for example WHICH hub or disk is making trouble.

VERY small, yes. :)

The reason the hardware supports two addreses is more likely IMO to be a "just in case someone else needs to use the main address" than anything else ... I've seen a bunch of boards using them, and in all cases there's only one TPS chip managing one battery's output/input.

Yes. Or a few more apprentice hackers you'd trust to rewrite core stuff!

My point was that "!A" is today the only portable test for B. So in that sense they are NOT unrelated.

Attached. Glad to know I wasn't the only one being puzzled!

I understand your point, but systems that use these chips rely on them. rather fundamentally. Normally both the I2C bus driver and the TPS driver are _both_ statically linked ... you'd not remove the I2C bus driver any more than you'd remove the PCI driver from a x86 box.

I said "possibly". It's not clear. If that were the case, why was the startup glitch only seen with the TPS driver? And in any case, nobody seems to have time to debug that particular board's specific hardware problem lately. These sorts of things are hard to spy on even with a decent hardware debug setup, since the main chips are surface mount BGA and there's no place to stick any kind of logic analyser in those multilayer boards... :(

Yes, I realized that. I've been getting rid of redundant messages incrementally. Ditto bogus whitespace/tabbing, as with the next one:

Mixed whitespace and tabs at line starts?? Positively revolting! :)

- Dave

-------------- next part --------------
A non-text attachment was scrubbed...
Name: i2c.doc
Type: text/x-diff
Size: 1496 bytes
Desc: not available
Url : http://lists.atrpms.net/pipermail/lm-sensors/attachments/20050531/02d238e2/i2c.bin