Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[David Brownell]

And I **really** hope this gets merged into 2.6.18 since virtually
no OMAP board is very usable without it.  I2C is one of the main
missing pieces(*) ... can whoever's managing I2C merges please
expedite this?

I just tried building an OSK config against RC3 and found at least
five will-not-build errors in the kernel.org tree.  The reason for
this is basically that folk have no option except the linux-omap
tree, since there's no point in trying to use the kernel.org version
until the I2C driver finally gets merged ... so such bugs won't get
fixed.  Needless to say, this is not the desired development process.

- Dave

(*) I submitted the then-current I2C driver over a year ago, but
    after a few months of inaction I found that it was dropped
    (or rejected?) by the I2C list software.  Of course at that
    point I no longer had time to resubmit the current code ...

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[Jean Delvare]

Hi David,

> And I **really** hope this gets merged into 2.6.18 since virtually
> no OMAP board is very usable without it.  I2C is one of the main
> missing pieces(*) ... can whoever's managing I2C merges please
> expedite this?

It doesn't work like this, sorry. The merge window for 2.6.18 is
closed. The driver needs to be reviewed before it is merged. So the
best you can hope for is -mm soon, and 2.6.19.

> I just tried building an OSK config against RC3 and found at least
> five will-not-build errors in the kernel.org tree.  The reason for
> this is basically that folk have no option except the linux-omap
> tree, since there's no point in trying to use the kernel.org version
> until the I2C driver finally gets merged ... so such bugs won't get
> fixed.  Needless to say, this is not the desired development process.

Indeed, this is no good. If you want things to improve, please help by
reviewing Komal's driver. I think I understand you already commented on
it, but I'd like you to really review it, and add a formal approval to
it (e.g. Signed-off-by or Acked-by). Then I'll review it for merge.

> (*) I submitted the then-current I2C driver over a year ago, but
>     after a few months of inaction I found that it was dropped
>     (or rejected?) by the I2C list software.  Of course at that
>     point I no longer had time to resubmit the current code ...

Neither dropped nor rejected, as I received it and it shows in the
archive as well:
  http://lists.lm-sensors.org/pipermail/lm-sensors/2005-August/013216.html
The reason why it was "ignored" is more likely a lack of time and/or
interest.

What is the relation between your "old" driver and the new one Komal is
submitting now? Evolution, or rewrite?

-- 
Jean Delvare

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[David Brownell]

On Monday 31 July 2006 9:13 am, Jean Delvare wrote:
> Hi David,
> 
> > And I **really** hope this gets merged into 2.6.18 since virtually
> > no OMAP board is very usable without it.  I2C is one of the main
> > missing pieces(*) ... can whoever's managing I2C merges please
> > expedite this?
> 
> It doesn't work like this, sorry. The merge window for 2.6.18 is
> closed. The driver needs to be reviewed before it is merged. So the
> best you can hope for is -mm soon, and 2.6.19.

So there's been a change from the "new drivers can be merged late"
policy?  News to me if so.  Not that this is a particularly new
driver of course, or at all unstable.


> > I just tried building an OSK config against RC3 and found at least
> > five will-not-build errors in the kernel.org tree.  The reason for
> > this is basically that folk have no option except the linux-omap
> > tree, since there's no point in trying to use the kernel.org version
> > until the I2C driver finally gets merged ... so such bugs won't get
> > fixed.  Needless to say, this is not the desired development process.
> 
> Indeed, this is no good. If you want things to improve, please help by
> reviewing Komal's driver. I think I understand you already commented on
> it, but I'd like you to really review it, and add a formal approval to
> it (e.g. Signed-off-by or Acked-by). Then I'll review it for merge.

Review it again?  I'll try to make some time to help there, but
it's unlikely I'll notice significant issues that seem to me
worth holding up the upstream merge.  Certainly none that make
up for the problems caused by having the kernel.org tree be all
but unusable for OMAP work, and couldn't be patched later.


> > (*) I submitted the then-current I2C driver over a year ago, but
> >     after a few months of inaction I found that it was dropped
> >     (or rejected?) by the I2C list software.  Of course at that
> >     point I no longer had time to resubmit the current code ...
> 
> Neither dropped nor rejected, as I received it and it shows in the
> archive as well:
>   http://lists.lm-sensors.org/pipermail/lm-sensors/2005-August/013216.html
> The reason why it was "ignored" is more likely a lack of time and/or
> interest.
> 
> What is the relation between your "old" driver

Not "my" driver at all; I'm not even on the copyright list, and
maybe the biggest change I did was the platform_driver conversion.
(If that; I don't recall, but I converted a lot of drivers around
that time and I think I probably did that one too.)


> and the new one Komal is 
> submitting now? Evolution, or rewrite?

Evolution.  I see support for the new OMAP2 parts (ARMv6) and maybe
the clock framework stuff is new; plus as you know the I2C framework
has collected simplifications and updates.

- Dave

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[Russell King]

On Mon, Jul 31, 2006 at 09:41:00AM -0700, David Brownell wrote:
> On Monday 31 July 2006 9:13 am, Jean Delvare wrote:
> > Hi David,
> > 
> > > And I **really** hope this gets merged into 2.6.18 since virtually
> > > no OMAP board is very usable without it.  I2C is one of the main
> > > missing pieces(*) ... can whoever's managing I2C merges please
> > > expedite this?

Slightly off-topic, and probably not your area, but it would probably
help your case if omap were better looked after in mainline.  Most OMAP
platforms build fine, except for one long standing one - the H2 1610
defconfig, which hasn't built since 2.6.17-git11.

So, rather than shoveling new stuff in there, can the maintainence of
the stuff already merged please be improved.

Build results vs kernel version for H2 1610:

http://armlinux.simtec.co.uk/kautobuild/omap_h2_1610_defconfig.html

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[Jean Delvare]

> > It doesn't work like this, sorry. The merge window for 2.6.18 is
> > closed. The driver needs to be reviewed before it is merged. So the
> > best you can hope for is -mm soon, and 2.6.19.
> 
> So there's been a change from the "new drivers can be merged late"
> policy?  News to me if so.  Not that this is a particularly new
> driver of course, or at all unstable.

This is my own policy for i2c drivers as the maintainer of the i2c
subsystem. I simply observed that new drivers cause much more late-rc
trouble than all other changes. So in order to let things stabilize
quickly, I don't take new drivers after -rc1. I see no reason why new
drivers would be allowed to bypass the -mm staging anyway.

If people want their drivers merged, they have to send them early. And
if they think I don't review them quickly enough (which is true) they
have to find other reviewers. There is no reason why I would have to
review every new i2c driver. As a matter of fact, I just don't have the
time to do so.

> > Indeed, this is no good. If you want things to improve, please help by
> > reviewing Komal's driver. I think I understand you already commented on
> > it, but I'd like you to really review it, and add a formal approval to
> > it (e.g. Signed-off-by or Acked-by). Then I'll review it for merge.
> 
> Review it again?  I'll try to make some time to help there, but
> it's unlikely I'll notice significant issues that seem to me
> worth holding up the upstream merge.  Certainly none that make
> up for the problems caused by having the kernel.org tree be all
> but unusable for OMAP work, and couldn't be patched later.

The "and could be patched later" way has been tried before, and I'm not
going there again. Later happened to be "never" or at least "too late"
more often than not.

And again maybe other subsystem maintainers have other policies, I
don't really care. I do the things the way I think works better. Anyone
not happy with that will have to help me a lot with the i2c patches
before I listen to him/her.

Thanks,
-- 
Jean Delvare

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[David Brownell]

On Monday 31 July 2006 9:13 am, Jean Delvare wrote:
>	 If you want things to improve, please help by
> reviewing Komal's driver. I think I understand you already commented on
> it, but I'd like you to really review it, and add a formal approval to
> it (e.g. Signed-off-by or Acked-by). Then I'll review it for merge.

The issues noted in the code are still almost all low priority
(non-blocking).

 - The FIXME about choosing the address is very low priority,
   and would affect only multi-master systems.  The fix would
   involve defining a new i2c-specific struct for platform_data,
   updating various boards to use it (e.g. OSK can use 400 MHz),
   and wouldn't change behavior for any board I've ever seen.

 - Likewise with the REVISIT for the bus speed to use.  They'd
   be fixed with the same patch.

 - The REVISIT about maybe a better way to probe is also low
   priority; someone with a board that needs better probing
   could address it at that time.  (Then restest any changes
   on multiple generations of silicion ... which IMO is the
   role the linux-omap tree should play.)

 - The revisit about adap->retries is still up in the air,
   and was a question in my submission from last year.
   How exactly is that supposed to be used?  Right now
   it's neither initialized (except to zero) nor tested.

Re coding style issues, I didn't give it a detailed nitpick
but I did easily notice two things worth fixing:

 - Some lines are more than 80 characters, so they'll wrap
   on standard editor windows.

 - There are a couple instances of hidden whitespace to
   remove:  at end of line, or space-before-tab.

This doesn't include the drivers/Makefile change to push i2c
linkage up near the beginning with other "system" busses,
but that can be a separate patch in any case (assuming that
it's still needed).

Assuming those two coding style things get resolved first,

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

- Dave

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[David Brownell]

On Monday 31 July 2006 12:10 pm, Russell King wrote:
> On Mon, Jul 31, 2006 at 09:41:00AM -0700, David Brownell wrote:
> > On Monday 31 July 2006 9:13 am, Jean Delvare wrote:
> > > Hi David,
> > > 
> > > > And I **really** hope this gets merged into 2.6.18 since virtually
> > > > no OMAP board is very usable without it.  I2C is one of the main
> > > > missing pieces(*) ... can whoever's managing I2C merges please
> > > > expedite this?
> 
> Slightly off-topic, and probably not your area, but it would probably
> help your case if omap were better looked after in mainline. 

Actually that _is_ part of my case.  It can't be looked after in any
reasonable way until the I2C driver gets merged, because significant
chunks of the OMAP driver stack require I2C.  (And notably for me,
USB always requires I2C to handle VBUS switches ...)

So for example once the OMAP I2C gets merged, then it'll finallly
become practical for folk to try _using_ mainline kernels.  Which
is a prerequisite for getting the bugs there fixed with any level
of promptness, since they can't be fixed until they're noticed.


> Most OMAP 
> platforms build fine, except for one long standing one - the H2 1610
> defconfig, which hasn't built since 2.6.17-git11.

Yet oddly enough, that's the only OMAP defconfig present.  :( 

Once I2C gets merged, the OSK defconfig could be merged too; and
that's a much handier board to work with and test.  H2 and OSK use
basically the same OMAP chip (5912 ~= 1610b), but 5912 doesn't need
NDAs; and the OSK board is is 10x smaller in size and price, plus
it's available on the open market.


> So, rather than shoveling new stuff in there, can the maintainence of
> the stuff already merged please be improved.
> 
> Build results vs kernel version for H2 1610:
> 
> http://armlinux.simtec.co.uk/kautobuild/omap_h2_1610_defconfig.html

You'll observe that I recently posted four build fixes (tps65010,
ohci-omap, omap-rng, smc91x) ... all of which would affect H2... the
fifth build fix will go to your armlinux patch database soon, it
addresses the fatal error in that build log.  I've already submitted
patches for the three Kconfig complaints.  (By the way, those build
logs could become more informative by using "make -k" ...)

Plus H2 is another of the OMAP platforms that can't be fully
initialized without using I2C.  :)

- Dave

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[Tony Lindgren]

* David Brownell <david-b@pacbell.net> [060801 02:58]:
> On Monday 31 July 2006 12:10 pm, Russell King wrote:
> > On Mon, Jul 31, 2006 at 09:41:00AM -0700, David Brownell wrote:
> > > On Monday 31 July 2006 9:13 am, Jean Delvare wrote:
> > > > Hi David,
> > > > 
> > > > > And I **really** hope this gets merged into 2.6.18 since virtually
> > > > > no OMAP board is very usable without it.  I2C is one of the main
> > > > > missing pieces(*) ... can whoever's managing I2C merges please
> > > > > expedite this?
> > 
> > Slightly off-topic, and probably not your area, but it would probably
> > help your case if omap were better looked after in mainline. 
> 
> Actually that _is_ part of my case.  It can't be looked after in any
> reasonable way until the I2C driver gets merged, because significant
> chunks of the OMAP driver stack require I2C.  (And notably for me,
> USB always requires I2C to handle VBUS switches ...)
> 
> So for example once the OMAP I2C gets merged, then it'll finallly
> become practical for folk to try _using_ mainline kernels.  Which
> is a prerequisite for getting the bugs there fixed with any level
> of promptness, since they can't be fixed until they're noticed.

I agree, we want to use the mainline kernel for omap. Most of the core
omap code is already integrated, now we just need to get few more
omap device drivers integrated to have mainline kernel usable for
most omap boards.
 
> > Most OMAP 
> > platforms build fine, except for one long standing one - the H2 1610
> > defconfig, which hasn't built since 2.6.17-git11.
> 
> Yet oddly enough, that's the only OMAP defconfig present.  :( 
> 
> Once I2C gets merged, the OSK defconfig could be merged too; and
> that's a much handier board to work with and test.  H2 and OSK use
> basically the same OMAP chip (5912 ~= 1610b), but 5912 doesn't need
> NDAs; and the OSK board is is 10x smaller in size and price, plus
> it's available on the open market.

I agree. After the omap I2C driver has been integrated, we should
provide patches for few more omap defconfigs.
 
> > So, rather than shoveling new stuff in there, can the maintainence of
> > the stuff already merged please be improved.

Sorry, my fault, I've been on vacation for most of July.
 
> > Build results vs kernel version for H2 1610:
> > 
> > http://armlinux.simtec.co.uk/kautobuild/omap_h2_1610_defconfig.html
> 
> You'll observe that I recently posted four build fixes (tps65010,
> ohci-omap, omap-rng, smc91x) ... all of which would affect H2... the
> fifth build fix will go to your armlinux patch database soon, it
> addresses the fatal error in that build log.  I've already submitted
> patches for the three Kconfig complaints.  (By the way, those build
> logs could become more informative by using "make -k" ...)
> 
> Plus H2 is another of the OMAP platforms that can't be fully
> initialized without using I2C.  :)

Good to hear the patches mentioned above fix the compile problem :)

Tony

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[Jean Delvare]

Hi David,

> > The "and could be patched later" way has been tried before, and I'm not
> > going there again. Later happened to be "never" or at least "too late"
> > more often than not.
> 
> Well, you've seen my basic review.  As for "later", it's quite routine;
> that's what "submit early and often" means.  That whole ext4 devel
> cycle depends on using "later" intelligently.

Releasing "early and often" was never an excuse to release buggy code.
It is (to me, at least) about adding functionalities one at a time,
rather than waiting to be full-featured before releasing first.

As for the "later", there are areas where it is known not to work, e.g.
documentation - or more generally, everything which is optional from a
functional point of view. I do expect functional bugs to be fixed
later when they bite the users, but I am more skeptical about cleanups
and documentation.

-- 
Jean Delvare

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[Jean Delvare]

Hi Komal,

> Updated driver patch based on the review comments from David Brownell.
> 
> >That #define of MODULE_NAME should be removed; useless/unused.
> >And the #ifdefs for 15xx should be cpu-is-15xx type tests.

Here comes my review.

> This patch adds I2C bus driver for various Texas
> Instruments (TI) OMAP1/2 (http://www.ti.com/omap) series
> based boards like OMAP1510/1610/1710/242x.
> 
> Signed-off-by: Komal Shah <komal_shah802003@yahoo.com>
> 
> ---
> 
>  drivers/i2c/busses/Kconfig    |    7 
>  drivers/i2c/busses/Makefile   |    1 
>  drivers/i2c/busses/i2c-omap.c |  712 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 720 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-omap.c
> 
> bfd0caa110e5912e38a113983119d96c173fa083
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 884320e..bf49c97 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -537,4 +537,11 @@ config I2C_MV64XXX
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-mv64xxx.
>  
> +config I2C_OMAP
> +	tristate "OMAP I2C adapter"
> +	depends on I2C && ARCH_OMAP
> +	default y if MACH_OMAP_H3 || MACH_OMAP_OSK
> +	help
> +	  Support for TI OMAP I2C driver. Say yes if you want to use the OMAP
> +	  I2C interface.
>  endmenu

In alphabetical order, please. I see that some recent entries broke the
rule... This needs to be fixed.

"Support for TI OMAP I2C driver" doesn't make much sense.

> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index ac56df5..2fd82e4 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_I2C_VIAPRO)	+= i2c-viapro.o
>  obj-$(CONFIG_I2C_VOODOO3)	+= i2c-voodoo3.o
>  obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
>  obj-$(CONFIG_SCx200_I2C)	+= scx200_i2c.o
> +obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
>  
>  ifeq ($(CONFIG_I2C_DEBUG_BUS),y)
>  EXTRA_CFLAGS += -DDEBUG

Alphabetical order here too, please.

> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> new file mode 100644
> index 0000000..2cdecd5
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -0,0 +1,712 @@
> +/*
> + * linux/drivers/i2c/busses/i2c-omap.c

Don't include the full path here. At best it is redundant, at worst in
tends to become wrong over time as we move and/or rename driver.

> + *
> + * TI OMAP I2C master mode driver
> + *
> + * Copyright (C) 2003 MontaVista Software, Inc.
> + * Copyright (C) 2004 Texas Instruments.
> + *
> + * Updated to work with multiple I2C interfaces on 24xx by
> + * Tony Lindgren <tony@atomide.com> and Imre Deak <imre.deak@nokia.com>
> + * Copyright (C) 2005 Nokia Corporation
> + *
> + * Cleaned up by Juha Yrjölä <juha.yrjola@nokia.com>
> + *
> + * ----------------------------------------------------------------------------
> + * This file was highly leveraged from i2c-elektor.c:
> + *
> + * Copyright 1995-97 Simon G. Vogl
> + *           1998-99 Hans Berglund
> + *
> + * With some changes from Kyösti Mälkki <kmalkki@cc.hut.fi> and even
> + * Frodo Looijaard <frodol@dds.nl>

Please drop this highly generic and irrelevant paragraph.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +// #define DEBUG

Drop (debugging can be enabled with Kconfig).

> +
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +
> +#include <asm/io.h>
> +
> +/* ----- global defines ----------------------------------------------- */
> +static const char driver_name[] = "i2c_omap";
> +
> +#define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000)) /* timeout waiting for the controller to respond */

Line too long, please move the comment on its own line.

> +
> +#define DEFAULT_OWN		1	/* default own I2C address */

Bad choice, 1 is a reserved address for I2C (CBUS address).

> +#define MAX_MESSAGES		65536	/* max number of messages */

This is a unreasonably high limit. I don't see the point in having a
limit anyway.

> +
> +#define OMAP_I2C_REV_REG		0x00
> +#define OMAP_I2C_IE_REG			0x04
> +#define OMAP_I2C_STAT_REG		0x08
> +#define OMAP_I2C_IV_REG			0x0c
> +#define OMAP_I2C_SYSS_REG		0x10
> +#define OMAP_I2C_BUF_REG		0x14
> +#define OMAP_I2C_CNT_REG		0x18
> +#define OMAP_I2C_DATA_REG		0x1c
> +#define OMAP_I2C_SYSC_REG		0x20
> +#define OMAP_I2C_CON_REG		0x24
> +#define OMAP_I2C_OA_REG			0x28
> +#define OMAP_I2C_SA_REG			0x2c
> +#define OMAP_I2C_PSC_REG		0x30
> +#define OMAP_I2C_SCLL_REG		0x34
> +#define OMAP_I2C_SCLH_REG		0x38
> +#define OMAP_I2C_SYSTEST_REG		0x3c
> +
> +/* I2C Interrupt Enable Register (OMAP_I2C_IE): */
> +#define OMAP_I2C_IE_XRDY	(1 << 4)	/* TX data ready int enable */
> +#define OMAP_I2C_IE_RRDY	(1 << 3)	/* RX data ready int enable */
> +#define OMAP_I2C_IE_ARDY	(1 << 2)	/* Access ready int enable */
> +#define OMAP_I2C_IE_NACK	(1 << 1)	/* No ack interrupt enable */
> +#define OMAP_I2C_IE_AL		(1 << 0)	/* Arbitration lost int ena */
> +
> +/* I2C Status Register (OMAP_I2C_STAT): */
> +#define OMAP_I2C_STAT_SBD	(1 << 15)	/* Single byte data */
> +#define OMAP_I2C_STAT_BB	(1 << 12)	/* Bus busy */
> +#define OMAP_I2C_STAT_ROVR	(1 << 11)	/* Receive overrun */
> +#define OMAP_I2C_STAT_XUDF	(1 << 10)	/* Transmit underflow */
> +#define OMAP_I2C_STAT_AAS	(1 << 9)	/* Address as slave */
> +#define OMAP_I2C_STAT_AD0	(1 << 8)	/* Address zero */
> +#define OMAP_I2C_STAT_XRDY	(1 << 4)	/* Transmit data ready */
> +#define OMAP_I2C_STAT_RRDY	(1 << 3)	/* Receive data ready */
> +#define OMAP_I2C_STAT_ARDY	(1 << 2)	/* Register access ready */
> +#define OMAP_I2C_STAT_NACK	(1 << 1)	/* No ack interrupt enable */
> +#define OMAP_I2C_STAT_AL	(1 << 0)	/* Arbitration lost int ena */
> +
> +/* I2C Buffer Configuration Register (OMAP_I2C_BUF): */
> +#define OMAP_I2C_BUF_RDMA_EN	(1 << 15)	/* RX DMA channel enable */
> +#define OMAP_I2C_BUF_XDMA_EN	(1 << 7)	/* TX DMA channel enable */
> +
> +/* I2C Configuration Register (OMAP_I2C_CON): */
> +#define OMAP_I2C_CON_EN		(1 << 15)	/* I2C module enable */
> +#define OMAP_I2C_CON_BE		(1 << 14)	/* Big endian mode */
> +#define OMAP_I2C_CON_STB	(1 << 11)	/* Start byte mode (master) */
> +#define OMAP_I2C_CON_MST	(1 << 10)	/* Master/slave mode */
> +#define OMAP_I2C_CON_TRX	(1 << 9)	/* TX/RX mode (master only) */
> +#define OMAP_I2C_CON_XA		(1 << 8)	/* Expand address */
> +#define OMAP_I2C_CON_RM		(1 << 2)	/* Repeat mode (master only) */
> +#define OMAP_I2C_CON_STP	(1 << 1)	/* Stop cond (master only) */
> +#define OMAP_I2C_CON_STT	(1 << 0)	/* Start condition (master) */
> +
> +/* I2C System Test Register (OMAP_I2C_SYSTEST): */
> +#define OMAP_I2C_SYSTEST_ST_EN		(1 << 15)	/* System test enable */
> +#define OMAP_I2C_SYSTEST_FREE		(1 << 14)	/* Free running mode */
> +#define OMAP_I2C_SYSTEST_TMODE_MASK	(3 << 12)	/* Test mode select */
> +#define OMAP_I2C_SYSTEST_TMODE_SHIFT	(12)		/* Test mode select */
> +#define OMAP_I2C_SYSTEST_SCL_I		(1 << 3)	/* SCL line sense in */
> +#define OMAP_I2C_SYSTEST_SCL_O		(1 << 2)	/* SCL line drive out */
> +#define OMAP_I2C_SYSTEST_SDA_I		(1 << 1)	/* SDA line sense in */
> +#define OMAP_I2C_SYSTEST_SDA_O		(1 << 0)	/* SDA line drive out */

You don't use the system test register anywhere, so these bit
definitions are useless, as is the definition of the register itself.

> +
> +/* I2C System Status register (OMAP_I2C_SYSS): */
> +#define OMAP_I2C_SYSS_RDONE		1		/* Reset Done */

Not used anywhere?

> +
> +/* I2C System Configuration Register (OMAP_I2C_SYSC): */
> +#define OMAP_I2C_SYSC_SRST		(1 << 1)	/* Soft Reset */
> +
> +/* REVISIT: Use platform_data instead of module parameters */
> +static int clock = 100;	/* Default: Fast Mode = 400 KHz, Standard = 100 KHz */

This comment is confusing, it sounds like 400 is the default.

Also, the proper unit symbol is kHz, not KHz.

> +module_param(clock, int, 0);
> +MODULE_PARM_DESC(clock, "Set I2C clock in kHz: 100 or 400 (Fast Mode)");

Might be worth mentioning here that 100 is the default.

> +
> +static int own;
> +module_param(own, int, 0);
> +MODULE_PARM_DESC(own, "Address of OMAP I2C master (0 for default == 1)");

The comment is confusing, as this address is usually known as the
"slave address" in the I2C world. Masters don't need no address on an
I2C bus. "own" is not a very explicit parameter name, what about
"slave_addr"? Ideally this should be retrieved from platform_data too,
else you can't be sure you won't collide with a device on the bus.

"0 for default" doesn't make sense, as the default is, by definition,
when the user doesn't speficiy anything. That this is internally coded
as 0 is an implementation detail user-space doesn't need to know.

This could be made an unsigned short, too.

> +
> +struct omap_i2c_dev {
> +	struct device		*dev;
> +	void __iomem		*base;		/* virtual */
> +	int			irq;
> +	struct clk		*iclk;		/* Interface clock */
> +	struct clk		*fclk;		/* Functional clock */
> +	struct completion	cmd_complete;
> +	u16			cmd_err;
> +	u8			*buf;
> +	size_t			buf_len;
> +	struct i2c_adapter	adapter;
> +	unsigned		rev1:1;
> +	u8			own_address;
> +};
> +
> +static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
> +				      int reg, u16 val)
> +{
> +	__raw_writew(val, i2c_dev->base + reg);
> +}
> +
> +static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
> +{
> +	return __raw_readw(i2c_dev->base + reg);
> +}
> +
> +static int omap_i2c_get_clocks(struct omap_i2c_dev *dev)
> +{
> +	if (cpu_is_omap16xx() || cpu_is_omap24xx()) {
> +		dev->iclk = clk_get(dev->dev, "i2c_ick");
> +		if (IS_ERR(dev->iclk))
> +			return -ENODEV;
> +	}
> +
> +	dev->fclk = clk_get(dev->dev, "i2c_fck");
> +	if (IS_ERR(dev->fclk)) {
> +		if (dev->iclk != NULL)
> +			clk_put(dev->iclk);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static void omap_i2c_put_clocks(struct omap_i2c_dev *dev)
> +{
> +	clk_put(dev->fclk);
> +	dev->fclk = NULL;
> +	if (dev->iclk != NULL) {
> +		clk_put(dev->iclk);
> +		dev->iclk = NULL;
> +	}
> +}

There is a minor inconsistency here, omap_i2c_put_clocks sets the
pointers to NULL, but the error paths of omap_i2c_get_clocks don't?

> +
> +static void omap_i2c_enable_clocks(struct omap_i2c_dev *dev)
> +{
> +	if (dev->iclk != NULL)
> +		clk_enable(dev->iclk);
> +	clk_enable(dev->fclk);
> +}
> +
> +static void omap_i2c_disable_clocks(struct omap_i2c_dev *dev)
> +{
> +	if (dev->iclk != NULL)
> +		clk_disable(dev->iclk);
> +	clk_disable(dev->fclk);
> +}
> +
> +static void omap_i2c_reset(struct omap_i2c_dev *dev)

This function is misnamed, it seems to fully initialize the device, not
just reset it.

> +{
> +	u16 psc;
> +	unsigned long fclk_rate;
> +
> +	if (!dev->rev1) {

Would be nice to explain why/how this revision is different in a
comment.

> +		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, OMAP_I2C_SYSC_SRST);
> +		/* For some reason we need to set the EN bit before the
> +		 * reset done bit gets set. */
> +		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
> +		while (!(omap_i2c_read_reg(dev, OMAP_I2C_SYSS_REG) & 0x01));

No way. This is calling for a deadlock sooner or later. You can't just
expect that the hardware will ever set that bit you are waiting for, so
you need some timeout mechanism.

Also, shouldn't this 0x01 actually be OMAP_I2C_SYSS_RDONE?

> +	}
> +	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
> +
> +	if (cpu_class_is_omap1()) {
> +		struct clk *armxor_ck;
> +		unsigned long armxor_rate;
> +
> +		armxor_ck = clk_get(NULL, "armxor_ck");
> +		if (IS_ERR(armxor_ck)) {
> +			printk(KERN_WARNING "i2c: Could not get armxor_ck\n");

Please use dev_warn() instead.

> +			armxor_rate = 12000000;
> +		} else {
> +			armxor_rate = clk_get_rate(armxor_ck);
> +			clk_put(armxor_ck);
> +		}
> +
> +		if (armxor_rate > 16000000)
> +			psc = (armxor_rate + 8000000) / 12000000;
> +		else
> +			psc = 0;

Can you please explain this formula?

> +
> +		fclk_rate = armxor_rate;

I fail to see why you have been introducing this armxor_rate local
variable, instead of setting fclk_rate directly.

> +	} else if (cpu_class_is_omap2()) {
> +		fclk_rate = 12000000;
> +		psc = 0;
> +	}

Else? fclk_rate and psc are undefined, and the following code will do
random writes.

You really shouldn't ignore compiler warnings, you know? ;)

Given that these seem to be the defaults in several cases, it might
make sense to initialize them to 1200000 and 0, respectively, and kill
some code.

> +
> +	/* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
> +	omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, psc);
> +
> +	/* Program desired operating rate */
> +	fclk_rate /= (psc + 1) * 1000;
> +	if (psc > 2)
> +		psc = 2;
> +
> +	omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG,
> +			   fclk_rate / (clock * 2) - 7 + psc);
> +	omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG,
> +			   fclk_rate / (clock * 2) - 7 + psc);
> +
> +	/* Set Own Address: */
> +	omap_i2c_write_reg(dev, OMAP_I2C_OA_REG, dev->own_address);
> +
> +	/* Take the I2C module out of reset: */
> +	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
> +
> +	/* Enable interrupts */
> +	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG,
> +			   (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
> +			    OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
> +			    OMAP_I2C_IE_AL));
> +}
> +
> +/*
> + * Waiting on Bus Busy
> + */
> +static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
> +{
> +	unsigned long timeout;
> +
> +	timeout = jiffies + OMAP_I2C_TIMEOUT;
> +	while (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG) & OMAP_I2C_STAT_BB) {
> +		if (time_after(jiffies, timeout)) {
> +			dev_warn(dev->dev, "timeout waiting for bus ready\n");
> +			return -ETIMEDOUT;
> +		}
> +		msleep(1);
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Low level master read/write transaction.
> + */
> +static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> +			     struct i2c_msg *msg, int stop)
> +{
> +	struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
> +	int r;
> +	u16 w;
> +	u8 zero_byte = 0;
> +
> +	dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
> +		msg->addr, msg->len, msg->flags, stop);
> +
> +	omap_i2c_write_reg(dev, OMAP_I2C_SA_REG, msg->addr);
> +
> +	/* Sigh, seems we can't do zero length transactions. Thus, we
> +	 * can't probe for devices w/o actually sending/receiving at least
> +	 * a single byte. So we'll set count to 1 for the zero length
> +	 * transaction case and hope we don't cause grief for some
> +	 * arbitrary device due to random byte write/read during
> +	 * probes.
> +	 */

That is, you lie on your capabilities. This is no good. You should not
advertise something the device can't actually do, and you should fail
if still requested to do it. You just can't write a byte to a chip when
you were asked to write none, sorry.

> +	/* REVISIT: Could the STB bit of I2C_CON be used with probing? */
> +	if (msg->len == 0) {
> +		dev->buf = &zero_byte;
> +		dev->buf_len = 1;
> +	} else {
> +		dev->buf = msg->buf;
> +		dev->buf_len = msg->len;
> +	}
> +	omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev->buf_len);
> +
> +	init_completion(&dev->cmd_complete);
> +	dev->cmd_err = 0;
> +
> +	w = OMAP_I2C_CON_EN | OMAP_I2C_CON_MST | OMAP_I2C_CON_STT;
> +	if (msg->flags & I2C_M_TEN)
> +		w |= OMAP_I2C_CON_XA;
> +	if (!(msg->flags & I2C_M_RD))
> +		w |= OMAP_I2C_CON_TRX;
> +	if (stop)
> +		w |= OMAP_I2C_CON_STP;
> +	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
> +
> +	r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
> +						      OMAP_I2C_TIMEOUT);
> +	dev->buf_len = 0;
> +	if (r < 0)
> +		return r;
> +	if (r == 0) {
> +		dev_err(dev->dev, "controller timed out\n");
> +		omap_i2c_reset(dev);
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (likely(!dev->cmd_err))
> +		return 0;
> +
> +	/* We have an error */
> +	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
> +		if (msg->flags & I2C_M_IGNORE_NAK)
> +			return 0;

Couldn't you have other error bits set as well? I2C_M_IGNORE_NAK means
you can ignore OMAP_I2C_STAT_NACK, not other errors.

> +		if (stop) {
> +			u16 w;

You already have an u16 w available, why define a new one?

> +
> +			w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
> +			w |= OMAP_I2C_CON_STP;
> +			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
> +		}
> +		return -EREMOTEIO;
> +	}
> +	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
> +			    OMAP_I2C_STAT_XUDF))
> +		omap_i2c_reset(dev);
> +	return -EIO;
> +}
> +
> +
> +/*
> + * Prepare controller for a transaction and call omap_i2c_xfer_msg
> + * to do the work during IRQ processing.
> + */
> +static int
> +omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> +{
> +	struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
> +	int i;
> +	int r = 0;

Needless initialization.

> +
> +	if (num < 1 || num > MAX_MESSAGES)
> +		return -EINVAL;

This check doesn't appear to be necessary.

> +
> +	/* Check for valid parameters in messages */
> +	for (i = 0; i < num; i++)
> +		if (msgs[i].buf == NULL)
> +			return -EINVAL;

Nor does this one - no other i2c bus drivers checks this. If we wanted
to check this, we would do it at the i2c-core level.

> +
> +	omap_i2c_enable_clocks(dev);
> +
> +	/* REVISIT: initialize and use adap->retries */
> +	if ((r = omap_i2c_wait_for_bb(dev)) < 0)
> +		goto out;
> +
> +	for (i = 0; i < num; i++) {
> +		dev_dbg(dev->dev, "msg: %d, addr: 0x%04x, len: %d, flags: 0x%x\n",
> +			i, msgs[i].addr, msgs[i].len, msgs[i].flags);

You have the same debug message at the beginning of omap_i2c_xfer_msg,
this is redundant.

> +		r = omap_i2c_xfer_msg(adap, &msgs[i], (i == (num - 1)));
> +		if (r != 0)
> +			break;
> +	}
> +
> +	if (r == 0)
> +		r = num;
> +out:
> +	omap_i2c_disable_clocks(dev);
> +	return r;
> +}
> +
> +static u32
> +omap_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}

As said above, you shouldn't advertise I2C_FUNC_SMBUS_EMUL, as you
can't do I2C_FUNC_SMBUS_QUICK.

> +
> +static inline void
> +omap_i2c_complete_cmd(struct omap_i2c_dev *dev, u16 err)
> +{
> +	dev->cmd_err |= err;
> +	complete(&dev->cmd_complete);
> +}
> +
> +static inline void
> +omap_i2c_ack_stat(struct omap_i2c_dev *dev, u16 stat)
> +{
> +	omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
> +}
> +
> +static irqreturn_t
> +omap_i2c_rev1_isr(int this_irq, void *dev_id, struct pt_regs *regs)
> +{
> +	struct omap_i2c_dev *dev = dev_id;
> +	u16 iv, w;
> +
> +	iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
> +	switch (iv) {
> +	case 0x00:	/* None */
> +		break;
> +	case 0x01:	/* Arbitration lost */
> +		dev_err(dev->dev, "Arbitration lost\n");
> +		omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_AL);
> +		break;
> +	case 0x02:	/* No acknowledgement */
> +		omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_NACK);
> +		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_STP);
> +		break;
> +	case 0x03:	/* Register access ready */
> +		omap_i2c_complete_cmd(dev, 0);
> +		break;
> +	case 0x04:	/* Receive data ready */
> +		if (dev->buf_len) {
> +			w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
> +			*dev->buf++ = w;

Shouldn't this be w & 0xff? (I agree it doesn't change anything in
practice, but...)

> +			dev->buf_len--;
> +			if (dev->buf_len) {
> +				*dev->buf++ = w >> 8;
> +				dev->buf_len--;
> +			}
> +		} else
> +			dev_err(dev->dev, "RRDY IRQ while no data requested\n");
> +		break;
> +	case 0x05:	/* Transmit data ready */
> +		if (dev->buf_len) {
> +			w = *dev->buf++;
> +			dev->buf_len--;
> +			if (dev->buf_len) {
> +				w |= *dev->buf++ << 8;
> +				dev->buf_len--;
> +			}
> +			omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
> +		} else
> +			dev_err(dev->dev, "XRDY IRQ while no data to send\n");
> +		break;
> +	default:
> +		return IRQ_NONE;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t
> +omap_i2c_isr(int this_irq, void *dev_id, struct pt_regs *regs)
> +{
> +	struct omap_i2c_dev *dev = dev_id;
> +	u16 bits;
> +	u16 stat, w;
> +	int count = 0;
> +
> +	bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
> +	while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
> +		dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
> +		if (count++ == 100) {
> +			dev_warn(dev->dev, "Too much work in one IRQ\n");
> +			break;
> +		}
> +
> +		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
> +
> +		if (stat & OMAP_I2C_STAT_ARDY) {
> +			omap_i2c_complete_cmd(dev, 0);
> +			continue;
> +		}
> +		if (stat & OMAP_I2C_STAT_RRDY) {
> +			w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
> +			if (dev->buf_len) {
> +				*dev->buf++ = w;

I'd add & 0xff here as well.

> +				dev->buf_len--;
> +				if (dev->buf_len) {
> +					*dev->buf++ = w >> 8;
> +					dev->buf_len--;
> +				}
> +			} else
> +				dev_err(dev->dev, "RRDY IRQ while no data requested\n");
> +			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RRDY);
> +			continue;
> +		}
> +		if (stat & OMAP_I2C_STAT_XRDY) {
> +			int bail_out = 0;
> +
> +			w = 0;
> +			if (dev->buf_len) {
> +				w = *dev->buf++;
> +				dev->buf_len--;
> +				if (dev->buf_len) {
> +					w |= *dev->buf++ << 8;
> +					dev->buf_len--;
> +				}
> +			} else
> +				dev_err(dev->dev, "XRDY IRQ while no data to send\n");
> +#if 0
> +			if (!(stat & OMAP_I2C_STAT_BB)) {
> +				dev_warn(dev->dev, "XRDY while bus not busy\n");
> +				bail_out = 1;
> +			}
> +#endif

Why is this if'd out? If it's not needed, please drop.

> +			omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
> +			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XRDY);
> +			if (bail_out)
> +				omap_i2c_complete_cmd(dev, 1 << 15);

Where does this 1 << 15 come from?

> +			continue;
> +		}
> +		if (stat & OMAP_I2C_STAT_ROVR) {
> +			dev_err(dev->dev, "Receive overrun\n");
> +			dev->cmd_err |= OMAP_I2C_STAT_ROVR;
> +		}
> +		if (stat & OMAP_I2C_STAT_XUDF) {
> +			dev_err(dev->dev, "Transmit overflow\n");
> +			dev->cmd_err |= OMAP_I2C_STAT_XUDF;
> +		}
> +		if (stat & OMAP_I2C_STAT_NACK) {
> +			omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_NACK);
> +			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_STP);
> +		}
> +		if (stat & OMAP_I2C_STAT_AL) {
> +			dev_err(dev->dev, "Arbitration lost\n");
> +			omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_AL);
> +		}
> +	}
> +
> +	return count ? IRQ_HANDLED : IRQ_NONE;
> +}

Several lines larger than 80 columns in the above function, please fix.

> +
> +static struct i2c_algorithm omap_i2c_algo = {
> +	.master_xfer	= omap_i2c_xfer,
> +	.functionality	= omap_i2c_func,
> +};
> +
> +static int
> +omap_i2c_probe(struct platform_device *pdev)
> +{
> +	struct omap_i2c_dev	*dev;
> +	struct i2c_adapter	*adap;
> +	struct resource		*mem, *irq;
> +	int r;
> +
> +	/* NOTE: driver uses the static register mapping */
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!mem) {
> +		dev_err(&pdev->dev, "no mem resource?\n");
> +		return -ENODEV;
> +	}
> +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!irq) {
> +		dev_err(&pdev->dev, "no irq resource?\n");
> +		return -ENODEV;
> +	}
> +
> +	r = (int) request_mem_region(mem->start, (mem->end - mem->start) + 1,
> +			driver_name);

Ugly cast, you can do without it.

> +	if (!r) {
> +		dev_err(&pdev->dev, "I2C region already claimed\n");
> +		return -EBUSY;
> +	}
> +
> +	if (clock > 200)
> +		clock = 400;	/* Fast mode */
> +	else
> +		clock = 100;	/* Standard mode */
> +
> +	dev = kzalloc(sizeof(struct omap_i2c_dev), GFP_KERNEL);
> +	if (!dev) {
> +		r = -ENOMEM;
> +		goto do_release_region;
> +	}
> +
> +	/* FIXME: Get own address from platform_data */
> +	if (own >= 1 && own < 0x7f)
> +		dev->own_address = own;

Your range is slightly too wide actually. You should only accept
addresses from 0x03 to 0x77. Others are reserved for future use or for
the 10-bit addressing.

Anyway, given that Linux doesn't support this at the moment, I wonder
why you bother setting it.

> +	else
> +		own = DEFAULT_OWN;
> +
> +	dev->dev = &pdev->dev;
> +	dev->irq = irq->start;
> +	dev->base = (void __iomem *) IO_ADDRESS(mem->start);
> +	platform_set_drvdata(pdev, dev);
> +
> +	if ((r = omap_i2c_get_clocks(dev)) != 0)
> +		goto do_free_mem;
> +
> +	omap_i2c_enable_clocks(dev);
> +
> +	if (cpu_is_omap15xx())
> +		dev->rev1 = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) < 0x20;
> +
> +	/* reset ASAP, clearing any IRQs */
> +	omap_i2c_reset(dev);
> +
> +	if (cpu_is_omap15xx())
> +		r = request_irq(dev->irq, dev->rev1 ? omap_i2c_rev1_isr : omap_i2c_isr,

Line too long.

> +				0, driver_name, dev);
> +	else
> +		r = request_irq(dev->irq, omap_i2c_isr, 0, driver_name, dev);

This could be simplified. The test on cpu_is_omap15xx() is not needed,
dev->rev1 can only be set if it is true. So the "if" part works in all
cases.

> +		
> +	if (r) {
> +		dev_err(dev->dev, "failure requesting irq %i\n", dev->irq);
> +		goto do_unuse_clocks;
> +	}
> +	r = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
> +	dev_info(dev->dev, "bus %d rev%d.%d at %d kHz\n",
> +		 pdev->id - 1, r >> 4, r & 0xf, clock);

This "- 1" is error prone IMHO.

> +
> +	adap = &dev->adapter;
> +	i2c_set_adapdata(adap, dev);
> +	adap->owner = THIS_MODULE;
> +	adap->class = I2C_CLASS_HWMON;
> +	strncpy(adap->name, "OMAP I2C adapter", sizeof(adap->name));
> +	adap->algo = &omap_i2c_algo;
> +	adap->dev.parent = &pdev->dev;
> +
> +	/* i2c device drivers may be active on return from add_adapter() */
> +	r = i2c_add_adapter(adap);
> +	if (r) {
> +		dev_err(dev->dev, "failure adding adapter\n");
> +		goto do_free_irq;
> +	}
> +
> +	omap_i2c_disable_clocks(dev);
> +
> +	return 0;
> +
> +do_free_irq:
> +	free_irq(dev->irq, dev);
> +do_unuse_clocks:
> +	omap_i2c_disable_clocks(dev);
> +	omap_i2c_put_clocks(dev);
> +do_free_mem:

Missing platform_set_drvdata(pdev, NULL).

> +	kfree(dev);
> +do_release_region:
> +	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
> +	release_mem_region(mem->start, (mem->end - mem->start) + 1);
> +
> +	return r;
> +}

These do_ prefixes on your labels are bad looking (probably a matter of
taste...) What about err_ or exit_ instead?

> +
> +static int
> +omap_i2c_remove(struct platform_device *pdev)
> +{
> +	struct omap_i2c_dev	*dev = platform_get_drvdata(pdev);
> +	struct resource		*mem;
> +

I would suggest platform_set_drvdata(pdev, NULL) before anything else,
else you leave a dangling pointed behind you.

> +	free_irq(dev->irq, dev);
> +	i2c_del_adapter(&dev->adapter);
> +	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
> +	omap_i2c_put_clocks(dev);
> +	kfree(dev);
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	release_mem_region(mem->start, (mem->end - mem->start) + 1);
> +	return 0;
> +}
> +
> +static struct platform_driver omap_i2c_driver = {
> +	.probe		= omap_i2c_probe,
> +	.remove		= omap_i2c_remove,
> +	.driver 	= {

Space before tab (after "driver").

> +		.name	= (char *)driver_name,

This case shouldn't be necessary?

Also, missing .owner = THIS_MODULE.

> +	},
> +};
> +
> +/* I2C may be needed to bring up other drivers */
> +static int __init
> +omap_i2c_init_driver(void)
> +{
> +	return platform_driver_register(&omap_i2c_driver);
> +}
> +subsys_initcall(omap_i2c_init_driver);
> +
> +static void __exit omap_i2c_exit_driver(void)
> +{
> +	platform_driver_unregister(&omap_i2c_driver);
> +}
> +module_exit(omap_i2c_exit_driver);
> +
> +MODULE_AUTHOR("MontaVista Software, Inc. (and others)");
> +MODULE_DESCRIPTION("TI OMAP I2C bus adapter");
> +MODULE_LICENSE("GPL");

Here you are. Please address theses issues, then you can resubmit you
driver.

-- 
Jean Delvare

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[Jean Delvare]

Hi David,

> On Monday 31 July 2006 9:13 am, Jean Delvare wrote:
> >	 If you want things to improve, please help by
> > reviewing Komal's driver. I think I understand you already commented on
> > it, but I'd like you to really review it, and add a formal approval to
> > it (e.g. Signed-off-by or Acked-by). Then I'll review it for merge.
> 
> The issues noted in the code are still almost all low priority
> (non-blocking).
> 
>  - The FIXME about choosing the address is very low priority,
>    and would affect only multi-master systems.  The fix would
>    involve defining a new i2c-specific struct for platform_data,
>    updating various boards to use it (e.g. OSK can use 400 MHz),
>    and wouldn't change behavior for any board I've ever seen.

Given that the slave mode isn't supported by Linux at the moment, I
don't even see how this is relevant. (So I agree with you that this is
very low priority - I would even kill that part of the code.)

> 
>  - Likewise with the REVISIT for the bus speed to use.  They'd
>    be fixed with the same patch.

I don't see any relation with the slave address mechanism. The bus
speed is a simple parameter, internal to the device (i2c-core doesn't
care) so it should be very easy to move it to platform data. Not that I
really care, though.

>  - The REVISIT about maybe a better way to probe is also low
>    priority; someone with a board that needs better probing
>    could address it at that time.  (Then restest any changes
>    on multiple generations of silicion ... which IMO is the
>    role the linux-omap tree should play.)

No, it's not low priority, it's a blocker. I can't let that code go in.
The driver must do what it is told to do. If it can't, it must fail.
Yes, this means no (in-kernel) probing on this bus at the moment. Blame
it on the hardware manufacturer (if the chip actually can't do it.) In
user-space, i2cdetect can be told to use byte reads for probing.

>  - The revisit about adap->retries is still up in the air,
>    and was a question in my submission from last year.
>    How exactly is that supposed to be used?  Right now
>    it's neither initialized (except to zero) nor tested.

This is an optional feature, most i2c adapter drivers don't implement
it. I'm fine without it, this can always be implemented later if needed.

I just sent my own review of the code. As you can see, I had quite a
few comments. Hopefully you now agree that pushing that code to Linus
right away wouldn't have been wise.

Thanks,
-- 
Jean Delvare

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[David Brownell]

On Wednesday 02 August 2006 8:50 am, Jean Delvare wrote:

> >  - The FIXME about choosing the address is very low priority,
> >    ...
> 
> Given that the slave mode isn't supported by Linux at the moment, I
> don't even see how this is relevant. (So I agree with you that this is
> very low priority - I would even kill that part of the code.)

Killing it might be the right thing to do, but someone should double
check against omap 1510 errata ... on first principles that should be
either a workaround for a very old erratum, or else a bug that's been
sitting around for a long time.  (Possibly making one of your other
points!)  The earliest versions of that driver were pretty crappy,
and I could imagine that mechanism was left unchanged since, unlike
a lot of stuf you didn't see, this wasn't actively buggy.


> >  - Likewise with the REVISIT for the bus speed to use.  They'd
> >    be fixed with the same patch.
> 
> I don't see any relation with the slave address mechanism. The bus
> speed is a simple parameter, internal to the device (i2c-core doesn't
> care) so it should be very easy to move it to platform data. Not that I
> really care, though.

The relation is that they'd both be per-board mechanisms that
belong better with platform data.  (Unless of course setting that
one address is not really needed.)


> >  - The REVISIT about maybe a better way to probe is also low
> >    priority; someone with a board that needs better probing
> >    could address it at that time.  (Then restest any changes
> >    on multiple generations of silicion ... which IMO is the
> >    role the linux-omap tree should play.)
> 
> No, it's not low priority, it's a blocker. I can't let that code go in.
> The driver must do what it is told to do. If it can't, it must fail.

Well for the record that's not been reported as a problem ... that is,
nobody's reported a board where that matters.  We'd have to see if the
driver is even usable without that, since the I2C stack overall seemed
to require probing to work.

(There's a separate issue about how the I2C stack doesn't just have a
mechanism to just declare "this board has these chips, these addresses",
so I2C drivers have needless reliance on probing...)


> Yes, this means no (in-kernel) probing on this bus at the moment. Blame
> it on the hardware manufacturer (if the chip actually can't do it.) In
> user-space, i2cdetect can be told to use byte reads for probing.

The spec does say it's tested for zero after decrementing the count,
so a value of zero gives a 65536 byte transfer.

 
> >  - The revisit about adap->retries is still up in the air,
> >    and was a question in my submission from last year.
> >    How exactly is that supposed to be used?  Right now
> >    it's neither initialized (except to zero) nor tested.
> 
> This is an optional feature, most i2c adapter drivers don't implement
> it. I'm fine without it, this can always be implemented later if needed.

Good.  Komal, please update the comment accordingly ... it'd be
handy if kerneldoc or Documentation/i2c/* said that adap->retries
is optional, and said what it's supposed to do.


> I just sent my own review of the code. As you can see, I had quite a
> few comments. Hopefully you now agree that pushing that code to Linus
> right away wouldn't have been wise.

My main concern was that the process had stalled, for an essential
driver.  It's not stalled any more, assuming Komal makes time to
address those comments!

- Dave

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[Jean Delvare]

Hi David,

> > >  - The REVISIT about maybe a better way to probe is also low
> > >    priority; someone with a board that needs better probing
> > >    could address it at that time.  (Then restest any changes
> > >    on multiple generations of silicion ... which IMO is the
> > >    role the linux-omap tree should play.)
> > 
> > No, it's not low priority, it's a blocker. I can't let that code go in.
> > The driver must do what it is told to do. If it can't, it must fail.
> 
> Well for the record that's not been reported as a problem ... that is,
> nobody's reported a board where that matters.  We'd have to see if the
> driver is even usable without that, since the I2C stack overall seemed
> to require probing to work.

The i2c core provides a mechanism to bypass the probing when you know
for sure what device is at a given address. For an embedded system, that
should work.

> (There's a separate issue about how the I2C stack doesn't just have a
> mechanism to just declare "this board has these chips, these addresses",
> so I2C drivers have needless reliance on probing...)

This is being (slowly) addressed by Nathan Lutchansky and Mark M.
Hoffman. The best solution implies converting the i2c subsystem to the
device driver model - a non-trivial task.

> > Yes, this means no (in-kernel) probing on this bus at the moment. Blame
> > it on the hardware manufacturer (if the chip actually can't do it.) In
> > user-space, i2cdetect can be told to use byte reads for probing.
> 
> The spec does say it's tested for zero after decrementing the count,
> so a value of zero gives a 65536 byte transfer.

Did you try setting the OMAP_I2C_CON_STP bit before writing the address
to OMAP_I2C_SA_REG for zero-byte transfactions? And then don't write to
the OMAP_I2C_CNT_REG register at all, as the transaction is already
finished.

BTW, it looks like the driver attempts to handle 10-bit addresses, but
the way it is done, I very much doubt it would work. So it would be
better no to implement it at all (I've never seen a 10-bit address chip
anyway.)

> > >  - The revisit about adap->retries is still up in the air,
> > >    and was a question in my submission from last year.
> > >    How exactly is that supposed to be used?  Right now
> > >    it's neither initialized (except to zero) nor tested.
> > 
> > This is an optional feature, most i2c adapter drivers don't implement
> > it. I'm fine without it, this can always be implemented later if needed.
> 
> Good.  Komal, please update the comment accordingly ... it'd be
> handy if kerneldoc or Documentation/i2c/* said that adap->retries
> is optional, and said what it's supposed to do.

I take patches :) I don't see in which file it would fit under
Documentation/i2c (there's no i2c bus driver writing howto), maybe it
would be more useful to add a comment to include/linux/i2c.h instead.

-- 
Jean Delvare

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[David Brownell]

On Thursday 03 August 2006 2:19 am, Jean Delvare wrote:
> The i2c core provides a mechanism to bypass the probing when you know
> for sure what device is at a given address. For an embedded system, that
> should work.

Unfortunately the mechanisms I'm aware of require either error-prone
kernel command line parameters, or (not error prone, but inelegant)
board-specific logic in the drivers, before driver registration, to
do equivalent stuff.


> > (There's a separate issue about how the I2C stack doesn't just have a
> > mechanism to just declare "this board has these chips, these addresses",
> > so I2C drivers have needless reliance on probing...)
> 
> This is being (slowly) addressed by Nathan Lutchansky and Mark M.
> Hoffman. The best solution implies converting the i2c subsystem to the
> device driver model - a non-trivial task.

Glad to hear that fixes are in the works.  That's the same conclusion
I reached:  that I2C needed those non-trivial changes.

It may help to see how the SPI core solves that problem.  Unlike I2C,
SPI actually _can't_ probe (except in rare specialized cases), and when
I did the SPI stuff I was thinking about models that could apply easily
to help I2C avoid probing.  (Though not, at this point, code.)

That model of a table of board-specific declarations (with things like
"I2C chip type X at address A, using interrupt I and platform_data P")
should work for I2C too.

- Dave

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[Jean Delvare]

David,

> On Thursday 03 August 2006 2:19 am, Jean Delvare wrote:
> > The i2c core provides a mechanism to bypass the probing when you know
> > for sure what device is at a given address. For an embedded system, that
> > should work.
> 
> Unfortunately the mechanisms I'm aware of require either error-prone
> kernel command line parameters, or (not error prone, but inelegant)
> board-specific logic in the drivers, before driver registration, to
> do equivalent stuff.

I said it was possible, not that it was nice and elegant ;) I know it's
ugly at the moment - which is exactly why people have been asking for
an i2c-core conversion.

> It may help to see how the SPI core solves that problem.  Unlike I2C,
> SPI actually _can't_ probe (except in rare specialized cases), and when
> I did the SPI stuff I was thinking about models that could apply easily
> to help I2C avoid probing.  (Though not, at this point, code.)

I2C can't really probe either. We abuse a transaction type we known most
chips will reply to but otherwise ignore to achieve chip presence
detection. It did damage in the past (killing Thinkpad laptops) and
could do again in the future. So, having board-specific lists of chips
to avoid probing would help.

> That model of a table of board-specific declarations (with things like
> "I2C chip type X at address A, using interrupt I and platform_data P")
> should work for I2C too.

What we have in the works is "I2C chip type X at address A". No
interrupt nor platform data at this point. But once the conversion to
the device driver model is done, I guess it'll come naturally if needed.

-- 
Jean Delvare