[patch 2.6.12-rc3+] i2c driver for TPS6501x

[patch 2.6.12-rc3+] i2c driver for TPS6501x

[David Brownell]

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

[patch 2.6.12-rc3+] i2c driver for TPS6501x

[David Brownell]

On Thursday 05 May 2005 4:18 pm, David Brownell wrote:
> Attached is a patch against a GIT snapshot from yesterday, adding
> support for Texas Instruments TPS6501x power management chips(*).

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

[lm-sensors] Re: [patch 2.6.12-rc3+] i2c driver for TPS6501x

[Rudolf Marek]

Hello,

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

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  greg@kroah.com

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

[lm-sensors] Re: [patch 2.6.12-rc3+] i2c driver for TPS6501x

[David Brownell]

On Thursday 26 May 2005 12:15 pm, Rudolf Marek wrote:
> 
> Sorry not this way. The driver is too complicated for me and Jean.

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


> Others seems to be more busy these days so nobody who could
> be qualified for review responded.

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


> I suggest you to make sure that coding style is OK.
> Check the Documentation/CodingStyle file please.

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.


> Then you can continue with SubmittingPatches.
> 
> I2C subsystem handles Greg Kroah  greg@kroah.com
> 
> So I'm suggesting to ask people for review on LKML

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

[lm-sensors] Re: [patch 2.6.12-rc3+] i2c driver for TPS6501x

[Jean Delvare]

Hi Dave,

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

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.

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

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.

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

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.

> +	  This driver can also be built as a module.  If so, the module
> +	  will be called tps65010.
> +
> +

Single empty line here please.

> --- linux-2.6/drivers/i2c/chips/tps65010.c
> +++ omap-2.6/drivers/i2c/chips/tps65010.c
> (...)
> +#undef	DEBUG

No. Debugging is controlled through Kconfig, you're breaking that
mechanism by doing this.

> +/* only two addresses possible */
> +#define	TPS_BASE	0x48
> +static unsigned short normal_i2c[] = {
> +	TPS_BASE,
> +	I2C_CLIENT_END };

This is only one address as far as I can see. So either the comment is
wrong, or there is one address missing in the list.

Also note that "TPS_BASE" is a poor name. Most I2C chips, including
this one, have a single address, nothing like an I/O port range, so the
term "base" doesn't make much sense. I see very little benefit in the
#define anyway, the value is really only used here and it's quite
explicit what it is, so the preprocessing overhead is mostly unjustified
IMHO.

> +struct tps65010 {
> (...)
> +	/* plus four GPIOs, probably used to switch power */

I don't get this comment.

> +static void dbg_regstat(char *buf, size_t len, u8 regstatus)
> +{
> (...)
> +		(regstatus & TPS_REG_PG_LD02) ? " ld01_bad" : "",

Hmm, typo?

> +static void dbg_chgconf(int por, char *buf, size_t len, u8 chgconfig)
> +{
> +	char *hibit;
> +
> +	if (por)
> +		hibit = (chgconfig & TPS_CHARGE_POR)
> +				? "PORims" : "POR=1sec";
> +	else
> +		hibit = (chgconfig & TPS65013_AUA) ? "AUA" : "";

hibit could be made a const char *, methinks.

> +		({int p; switch ((chgconfig >> 3) & 3) {
> +		case 3:		p = 100; break;
> +		case 2:		p = 75; break;
> +		case 1:		p = 50; break;
> +		default:	p = 25; break;
> +		}; p; }),

Putting this inside the printf-like construct doesn't help readability.
Why don't you move it outside, like you did for hibit?

> +static void show_chgstatus(const char *label, u8 chgstatus)
> +{
> (...)
> +	pr_debug("%s: %s %s", DRIVER_NAME, label, buf);

You should be using dev_dbg() instead. Ditto in show_regstatus() and
show_chgconfig().

> +#else
> +
> +static inline void show_chgstatus(const char *label, u8 chgstatus) { }
> +static inline void show_regstatus(const char *label, u8 chgstatus) { }
> +static inline void show_chgconfig(int por, const char *label, u8
> +                                  chgconfig) { }
> +
> +#endif

Doesn't look good to me, as the driver will include useless function
calls in non-debug mode (unless the compiler can automatically skip
these?) I would rather skip the calls themselves (but see below).

> +static int dbg_show(struct seq_file *s, void *_)

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

> +	(void) schedule_delayed_work(&tps->work, POWER_POLL_DELAY);

Excuse my ignorance, but what is the (void) for?

> +		if (value & (1 << (4 +i)))

Coding style.

> +static struct tps65010 *the_tps;

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?

> +static int __exit tps65010_detach_client(struct i2c_client *client)
> +{
> (...)
> the_tps = 0;

That would rather be = NULL.

> +/* no error returns, they'd just make bus scanning stop */
> +static int __init
> +tps65010_probe(struct i2c_adapter *bus, int address, int kind)
> (...)
> +	tps = kmalloc(sizeof *tps, GFP_KERNEL);
> +	if (!tps)
> +		return 0;

In some cases, you actually want the bus scanning to stop on error. This
is the case for memory shortage, as well as the "only one device
supported" case.

> +fail1:
> +		kfree(tps);
> +		return 0;
> +	}

Please move this common error path at the end of the function, like the
other drivers do.

> +#else
> +#define set_irq_type(num,trigger)	do{}while(0)
> +#endif

Ugly trick if you want my opinion. First because that #else statement is
not the counterpart of the #ifdef statement at all (they just *happen*
to have an opposite condition). Second because you could just as easily
surround the only call to set_irq_type() with #ifdef CONFIG_ARM/#endif
for more clarity.

> +		printk(KERN_WARNING "%s: IRQ not configured!\n",
> +				DRIVER_NAME);

Why not dev_warn()? Same for many, many printk/pr_debug all around.

You should probably split the tps65010_probe() into parts, it's too
long. E.g. the chip initialization should be a separate function.

> +static struct i2c_driver tps65010_driver = {
> +	.owner		= THIS_MODULE,
> +	.name		= "tps65010",
> +	.id		= 888,		/* FIXME assign "official" value */

You don't really need an ID, do you? Just omit it.

+	.flags		= I2C_DF_NOTIFY,
+	.attach_adapter	= tps65010_scan_bus,
+	.detach_client	= __exit_p(tps65010_detach_client),
+};

The __exit_p() looks suspicious to me. Can you justify it? None of the
other i2c client drivers have it.

> +int tps65010_set_vbus_draw(unsigned mA)
> (...)
> EXPORT_SYMBOL(tps65010_set_vbus_draw);

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.

> +int tps65010_set_led(unsigned led, unsigned mode)
> (...)
> +	dev_dbg (&the_tps->client.dev, "led%i_on   0x%02x\n", led,

Coding style (several more in this function).

> +	if (status != 0)
> +		printk(KERN_ERR "%s: Failed to write vdcdc1 register\n",
> +	 DRIVER_NAME);
> +	else

Indentation.

> +static int __init tps_init(void)
> +{
> +	u32	tries = 3;
> +	int	status = -ENODEV;

Useless initialization of status as far as I can see.

> +	/* some boards have startup glitches */
> +	while (tries--) {
> +		status = i2c_add_driver(&tps65010_driver);
> +		if (the_tps)
> +			break;
> +		i2c_del_driver(&tps65010_driver);
> +		if (!tries) {
> +			printk(KERN_ERR "%s: no chip?\n", DRIVER_NAME);
> +			return -ENODEV;
> +		}
> +		pr_debug("%s: re-probe ...\n", DRIVER_NAME);
> +		msleep(10);
> +	}

This is very, very ugly. Cycling a driver because a device may fail to
register sucks. Please isolate the one part which may fail, and restart
only this one. If the driver ever supports more that one device (and it
really should), then cycling the driver will unregister possibly working
clients and have them register again, giving them one more chance to
fail - let alone the waste of time and power. The retries should really
be done at device level, not driver level.

> +#if defined(CONFIG_ARM)

Why not simply #ifdef?

> +subsys_initcall(tps_init);

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

[lm-sensors] Re: [patch 2.6.12-rc3+] i2c driver for TPS6501x

[David Brownell]

On Friday 27 May 2005 4:09 am, Jean Delvare wrote:
> 
> Hi Dave,

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


> > 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).
> 
> Nothing is fundamentally complicated. How much complicated we think a
> thing is mostly depends on what we are used to deal with.

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


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

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


> > From an I2C list I'd expect feedback about I2C issues more than
> > about how it relates to system booting and power management.
> 
> 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.

True enough.


> > 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.  ;)
> 
> 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 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 would rather move all non-i2c hardware 
> monitoring drivers out of the i2c subsystem before changing it
> significantly.

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


> > +/* only two addresses possible */
> > +#define	TPS_BASE	0x48
> > +static unsigned short normal_i2c[] = {
> > +	TPS_BASE,
> > +	I2C_CLIENT_END };
> 
> This is only one address as far as I can see. So either the comment is
> wrong, or there is one address missing in the list.

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.


>	I see very little benefit in the
> #define anyway, 

True enough; it's gone.

> > +		(regstatus & TPS_REG_PG_LD02) ? " ld01_bad" : "",
> 
> Hmm, typo?

Yes, thanks!

> > +		({int p; switch ((chgconfig >> 3) & 3) {
> > +		case 3:		p = 100; break;
> > +		case 2:		p = 75; break;
> > +		case 1:		p = 50; break;
> > +		default:	p = 25; break;
> > +		}; p; }),
> 
> Putting this inside the printf-like construct

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

> doesn't help readability. 
> Why don't you move it outside, like you did for hibit?

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


> > +static void show_chgstatus(const char *label, u8 chgstatus)
> > +{
> > (...)
> > +	pr_debug("%s: %s %s", DRIVER_NAME, label, buf);
> 
> You should be using dev_dbg() instead. Ditto in show_regstatus() and
> show_chgconfig().

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


> > +#else
> > +
> > +static inline void show_chgstatus(const char *label, u8 chgstatus) { }
> > +static inline void show_regstatus(const char *label, u8 chgstatus) { }
> > +static inline void show_chgconfig(int por, const char *label, u8
> > +                                  chgconfig) { }
> > +
> > +#endif
> 
> Doesn't look good to me, as the driver will include useless function
> calls in non-debug mode (unless the compiler can automatically skip
> these?) I would rather skip the calls themselves (but see below).

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.


> > +	(void) schedule_delayed_work(&tps->work, POWER_POLL_DELAY);
> 
> Excuse my ignorance, but what is the (void) for?

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.


> > +static struct tps65010 *the_tps;
> 
> 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?

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.


> > +static int __exit tps65010_detach_client(struct i2c_client *client)
> > +{
> > (...)
> > the_tps = 0;
> 
> That would rather be = NULL.

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


> > +/* no error returns, they'd just make bus scanning stop */
> > +static int __init
> > +tps65010_probe(struct i2c_adapter *bus, int address, int kind)
> > (...)
> > +	tps = kmalloc(sizeof *tps, GFP_KERNEL);
> > +	if (!tps)
> > +		return 0;
> 
> In some cases, you actually want the bus scanning to stop on error. This
> is the case for memory shortage, as well as the "only one device
> supported" case.

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.


> > +fail1:
> > +		kfree(tps);
> > +		return 0;
> > +	}
> 
> Please move this common error path at the end of the function, like the
> other drivers do.

OK, simple enough ...


> > +#else
> > +#define set_irq_type(num,trigger)	do{}while(0)
> > +#endif
> 
> Ugly trick if you want my opinion. First because that #else statement is
> not the counterpart of the #ifdef statement at all (they just *happen*
> to have an opposite condition).

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.


> Second because you could just as easily 
> surround the only call to set_irq_type() with #ifdef CONFIG_ARM/#endif
> for more clarity.

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


> > +static struct i2c_driver tps65010_driver = {
> > +	.owner		= THIS_MODULE,
> > +	.name		= "tps65010",
> > +	.id		= 888,		/* FIXME assign "official" value */
> 
> You don't really need an ID, do you? Just omit it.

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


> +	.flags		= I2C_DF_NOTIFY,
> +	.attach_adapter	= tps65010_scan_bus,
> +	.detach_client	= __exit_p(tps65010_detach_client),
> +};
> 
> The __exit_p() looks suspicious to me. Can you justify it? None of the
> other i2c client drivers have it.

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.


> > +int tps65010_set_led(unsigned led, unsigned mode)
> > (...)
> > +	dev_dbg (&the_tps->client.dev, "led%i_on   0x%02x\n", led,
> 
> Coding style (several more in this function).

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


> > +static int __init tps_init(void)
> > +{
> > +	u32	tries = 3;
> > +	int	status = -ENODEV;
> 
> Useless initialization of status as far as I can see.
> 
> > +	/* some boards have startup glitches */
> > +	while (tries--) {
> > +		status = i2c_add_driver(&tps65010_driver);

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.


> > +		if (the_tps)
> > +			break;
> > +		i2c_del_driver(&tps65010_driver);
> > +		if (!tries) {
> > +			printk(KERN_ERR "%s: no chip?\n", DRIVER_NAME);
> > +			return -ENODEV;
> > +		}
> > +		pr_debug("%s: re-probe ...\n", DRIVER_NAME);
> > +		msleep(10);
> > +	}
> 
> This is very, very ugly. Cycling a driver because a device may fail to
> register sucks. Please isolate the one part which may fail, and restart
> only this one. 

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.


> > +#if defined(CONFIG_ARM)
> 
> Why not simply #ifdef?

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


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

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


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

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


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

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


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

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



> 
> --
> Jean Delvare
> 
-------------- 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

[lm-sensors] Re: [patch 2.6.12-rc3+] i2c driver for TPS6501x

[Jean Delvare]

Hi Dave,

> The "isp1301_omap" driver uses even more kernel features, FWIW, but
> it's not exactly a "hardware monitoring" driver either.

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

> > I would rather move all non-i2c hardware 
> > monitoring drivers out of the i2c subsystem before changing it
> > significantly.
> 
> I hope you're not saying what I think you're saying ...

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

> (...) why
> remove everything from the I2C stack except hardware monitoring
> support??

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.

> > You should be using dev_dbg() instead. Ditto in show_regstatus() and
> > show_chgconfig().
> 
> Actually that just makes the diagnostics annoyingly verbose;

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

> there's no point to having more than one of these chips in a system.

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.

> > 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?
> 
> First and foremost:  having more than one of them make sense from a
> hardware perspective ... :)

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

> > In some cases, you actually want the bus scanning to stop on error.
> > This is the case for memory shortage, as well as the "only one
> > device supported" case.
> 
> 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.

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.

> > > +#else
> > > +#define set_irq_type(num,trigger)	do{}while(0)
> > > +#endif
> > 
> > Ugly trick if you want my opinion. First because that #else
> > statement is not the counterpart of the #ifdef statement at all
> > (they just *happen* to have an opposite condition).
> 
> 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.

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.

> > You don't really need an ID, do you? Just omit it.
> 
> What's it even for?

Actually everyone is wondering ;)

> (...) The docs show it used, but don't say how
> a driver author could know if it's needed.

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.

> (...) Sounds like maybe
> the docs should stop recommending it be set...

Patches are welcome :)

> > The __exit_p() looks suspicious to me. Can you justify it? None of
> > the other i2c client drivers have it.
> 
> 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 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.

> > > +static int __init tps_init(void)
> > > +{
> > > +	u32	tries = 3;
> > > +	int	status = -ENODEV;
> > 
> > Useless initialization of status as far as I can see.
> > 
> > > +	/* some boards have startup glitches */
> > > +	while (tries--) {
> > > +		status = i2c_add_driver(&tps65010_driver);
> 
> 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.

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

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

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:

> -	dev_dbg (&the_tps->client.dev, "led%i_on   0x%02x\n", led,
> -		i2c_smbus_read_byte_data(&the_tps->client, TPS_LED1_ON + offs));
> (...)
> +	pr_debug("%s: led%i_on   0x%02x\n", DRIVER_NAME, led,
> +		i2c_smbus_read_byte_data(&the_tps->client,
> +				TPS_LED1_ON + offs));

Let it be clear that my complaint about the coding style was about the
additional space between dev_dbg and the opening parenthesis. Nothing
more. The rest of the changes are up to you.

>  	if (status != 0)
>  		printk(KERN_ERR "%s: Failed to write vdcdc1 register\n", 
> -		       DRIVER_NAME);
> +			DRIVER_NAME);
>  	else

For some reason I wasn't seeing the correct indentations when reviewing
your original patch this morning, thus my complaint. However the
indentation was actually just fine, so this change is not needed at all
(I personally prefer it the original way).

-- 
Jean Delvare

[lm-sensors] Re: [patch 2.6.12-rc3+] i2c driver for TPS6501x

[David Brownell]

On Friday 27 May 2005 2:54 pm, Jean Delvare wrote:
> Hi Dave,
> 
> > The "isp1301_omap" driver uses even more kernel features, FWIW, but
> > it's not exactly a "hardware monitoring" driver either.
> 
> And as a matter of fact it was not posted for review on the lm-sensors
> list.

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


> > (...) why
> > remove everything from the I2C stack except hardware monitoring
> > support??
> 
> 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.

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


> > > You should be using dev_dbg() instead. Ditto in show_regstatus() and
> > > show_chgconfig().
> > 
> > Actually that just makes the diagnostics annoyingly verbose;
> 
> Funny thing to read when you see how verbose the debugging messages
> themselves are in this driver ;)

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


> > there's no point to having more than one of these chips in a system.
> 
> 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.

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.


> > > 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?
> > 
> > First and foremost:  having more than one of them make sense from a
> > hardware perspective ... :)
> 
> I guess you assume very small values of 2 here, right? ;)

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.


> > > In some cases, you actually want the bus scanning to stop on error.
> > > This is the case for memory shortage, as well as the "only one
> > > device supported" case.
> > 
> > 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.
> 
> 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.

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


> > > > +#else
> > > > +#define set_irq_type(num,trigger)	do{}while(0)
> > > > +#endif
> > > 
> > > Ugly trick if you want my opinion. First because that #else
> > > statement is not the counterpart of the #ifdef statement at all
> > > (they just *happen* to have an opposite condition).
> > 
> > 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.
> 
> My point was as follow. Given A and B two unrelated instructions...

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

> 
> > > You don't really need an ID, do you? Just omit it.
> > 
> > What's it even for?
> 
> Actually everyone is wondering ;)
> 
> ...
> 
> > (...) Sounds like maybe
> > the docs should stop recommending it be set...
> 
> Patches are welcome :)

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


> > > The __exit_p() looks suspicious to me. Can you justify it? None of
> > > the other i2c client drivers have it.
> > 
> > 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.
> > 
> > ...
> 
> 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.

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.


> > 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.
> 
> If the I2C controller is faulty, the workaround should be moved to its
> driver, as you seem to agree upon yourself.

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


> And finally some minor comments on your incremental patch:
> 
> > -	dev_dbg (&the_tps->client.dev, "led%i_on   0x%02x\n", led,
> > -		i2c_smbus_read_byte_data(&the_tps->client, TPS_LED1_ON + offs));
> > (...)
> > +	pr_debug("%s: led%i_on   0x%02x\n", DRIVER_NAME, led,
> > +		i2c_smbus_read_byte_data(&the_tps->client,
> > +				TPS_LED1_ON + offs));
> 
> Let it be clear that my complaint about the coding style was about the
> additional space between dev_dbg and the opening parenthesis. Nothing
> more. The rest of the changes are up to you.

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


> >  	if (status != 0)
> >  		printk(KERN_ERR "%s: Failed to write vdcdc1 register\n", 
> > -		       DRIVER_NAME);
> > +			DRIVER_NAME);
> >  	else
> 
> For some reason I wasn't seeing the correct indentations when reviewing
> your original patch this morning, thus my complaint. However the
> indentation was actually just fine, so this change is not needed at all
> (I personally prefer it the original way).

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