[RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Maciej W. Rozycki]

 The standard rtc-m41t80.c driver cannot be used with the SWARM as it is,
because the board does not provide setup information for the I2C core.  
As a result the bus and the address to probe for the M41T80 chip is not
known.

 Here is a set of changes that fix the problem:

1. i2c-swarm.c -- SWARM I2C board setup, currently for the M41T80 chip on 
   the bus #1 only.

2. The i2c-sibyte.c BCM1250A SMBus controller driver now registers its 
   buses as numbered so that board setup is correctly applied.  Plus minor 
   corrections.

3. SWARM platform library is now built as an object to include i2c-swarm.c 
   which is only used as an initcall and does not provide any symbols that 
   would pull the file from an archive.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
patch-2.6.26-rc1-20080505-swarm-i2c-15
diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/arch/mips/Makefile linux-2.6.26-rc1-20080505/arch/mips/Makefile
--- linux-2.6.26-rc1-20080505.macro/arch/mips/Makefile	2008-05-05 02:55:23.000000000 +0000
+++ linux-2.6.26-rc1-20080505/arch/mips/Makefile	2008-05-05 21:10:50.000000000 +0000
@@ -538,19 +538,19 @@ cflags-$(CONFIG_SIBYTE_BCM1x80)	+= -Iinc
 # Sibyte SWARM board
 # Sibyte BCM91x80 (BigSur) board
 #
-libs-$(CONFIG_SIBYTE_CARMEL)	+= arch/mips/sibyte/swarm/
+core-$(CONFIG_SIBYTE_CARMEL)	+= arch/mips/sibyte/swarm/
 load-$(CONFIG_SIBYTE_CARMEL)	:= 0xffffffff80100000
-libs-$(CONFIG_SIBYTE_CRHINE)	+= arch/mips/sibyte/swarm/
+core-$(CONFIG_SIBYTE_CRHINE)	+= arch/mips/sibyte/swarm/
 load-$(CONFIG_SIBYTE_CRHINE)	:= 0xffffffff80100000
-libs-$(CONFIG_SIBYTE_CRHONE)	+= arch/mips/sibyte/swarm/
+core-$(CONFIG_SIBYTE_CRHONE)	+= arch/mips/sibyte/swarm/
 load-$(CONFIG_SIBYTE_CRHONE)	:= 0xffffffff80100000
-libs-$(CONFIG_SIBYTE_RHONE)	+= arch/mips/sibyte/swarm/
+core-$(CONFIG_SIBYTE_RHONE)	+= arch/mips/sibyte/swarm/
 load-$(CONFIG_SIBYTE_RHONE)	:= 0xffffffff80100000
-libs-$(CONFIG_SIBYTE_SENTOSA)	+= arch/mips/sibyte/swarm/
+core-$(CONFIG_SIBYTE_SENTOSA)	+= arch/mips/sibyte/swarm/
 load-$(CONFIG_SIBYTE_SENTOSA)	:= 0xffffffff80100000
-libs-$(CONFIG_SIBYTE_SWARM)	+= arch/mips/sibyte/swarm/
+core-$(CONFIG_SIBYTE_SWARM)	+= arch/mips/sibyte/swarm/
 load-$(CONFIG_SIBYTE_SWARM)	:= 0xffffffff80100000
-libs-$(CONFIG_SIBYTE_BIGSUR)	+= arch/mips/sibyte/swarm/
+core-$(CONFIG_SIBYTE_BIGSUR)	+= arch/mips/sibyte/swarm/
 load-$(CONFIG_SIBYTE_BIGSUR)	:= 0xffffffff80100000
 
 #
diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/arch/mips/sibyte/swarm/Makefile linux-2.6.26-rc1-20080505/arch/mips/sibyte/swarm/Makefile
--- linux-2.6.26-rc1-20080505.macro/arch/mips/sibyte/swarm/Makefile	2004-01-29 04:57:05.000000000 +0000
+++ linux-2.6.26-rc1-20080505/arch/mips/sibyte/swarm/Makefile	2008-05-06 01:18:21.000000000 +0000
@@ -1,3 +1,4 @@
-lib-y				= setup.o rtc_xicor1241.o rtc_m41t81.o
+obj-y				:= setup.o rtc_xicor1241.o rtc_m41t81.o
 
-lib-$(CONFIG_KGDB)		+= dbg_io.o
+obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-swarm.o
+obj-$(CONFIG_KGDB)		+= dbg_io.o
diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/arch/mips/sibyte/swarm/i2c-swarm.c linux-2.6.26-rc1-20080505/arch/mips/sibyte/swarm/i2c-swarm.c
--- linux-2.6.26-rc1-20080505.macro/arch/mips/sibyte/swarm/i2c-swarm.c	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.26-rc1-20080505/arch/mips/sibyte/swarm/i2c-swarm.c	2008-05-06 23:51:34.000000000 +0000
@@ -0,0 +1,37 @@
+/*
+ *	arch/mips/sibyte/swarm/i2c-swarm.c
+ *
+ *	Broadcom BCM91250A (SWARM), etc. I2C platform setup.
+ *
+ *	Copyright (c) 2008  Maciej W. Rozycki
+ *
+ *	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.
+ */
+
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+
+
+static struct i2c_board_info swarm_i2c_info1[] __initdata = {
+	{
+		I2C_BOARD_INFO("m41t81", 0x68),
+	},
+};
+
+static int __init swarm_i2c_init(void)
+{
+	int err;
+
+	err = i2c_register_board_info(1, swarm_i2c_info1,
+				      ARRAY_SIZE(swarm_i2c_info1));
+	if (err < 0)
+		printk(KERN_ERR
+		       "i2c-swarm: cannot register board I2C devices\n");
+	return err;
+}
+
+arch_initcall(swarm_i2c_init);
diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/drivers/i2c/busses/i2c-sibyte.c linux-2.6.26-rc1-20080505/drivers/i2c/busses/i2c-sibyte.c
--- linux-2.6.26-rc1-20080505.macro/drivers/i2c/busses/i2c-sibyte.c	2008-05-05 02:55:25.000000000 +0000
+++ linux-2.6.26-rc1-20080505/drivers/i2c/busses/i2c-sibyte.c	2008-05-06 23:45:32.000000000 +0000
@@ -2,6 +2,7 @@
  * Copyright (C) 2004 Steven J. Hill
  * Copyright (C) 2001,2002,2003 Broadcom Corporation
  * Copyright (C) 1995-2000 Simon G. Vogl
+ * Copyright (C) 2008  Maciej W. Rozycki
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -132,18 +133,18 @@ static const struct i2c_algorithm i2c_si
 /*
  * registering functions to load algorithms at runtime
  */
-int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
+static int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
 {
 	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
 
-	/* register new adapter to i2c module... */
+	/* Register new adapter to i2c module...  */
 	i2c_adap->algo = &i2c_sibyte_algo;
 
-	/* Set the frequency to 100 kHz */
+	/* Set the requested frequency.  */
 	csr_out32(speed, SMB_CSR(adap,R_SMB_FREQ));
 	csr_out32(0, SMB_CSR(adap,R_SMB_CONTROL));
 
-	return i2c_add_adapter(i2c_adap);
+	return i2c_add_numbered_adapter(i2c_adap);
 }
 
 
@@ -159,6 +160,7 @@ static struct i2c_adapter sibyte_board_a
 		.class		= I2C_CLASS_HWMON,
 		.algo		= NULL,
 		.algo_data	= &sibyte_board_data[0],
+		.nr		= 0,
 		.name		= "SiByte SMBus 0",
 	},
 	{
@@ -167,6 +169,7 @@ static struct i2c_adapter sibyte_board_a
 		.class		= I2C_CLASS_HWMON,
 		.algo		= NULL,
 		.algo_data	= &sibyte_board_data[1],
+		.nr		= 1,
 		.name		= "SiByte SMBus 1",
 	},
 };

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Jean Delvare]

Hi Maciej,

On Wed, 7 May 2008 01:40:27 +0100 (BST), Maciej W. Rozycki wrote:
> (...)
> 2. The i2c-sibyte.c BCM1250A SMBus controller driver now registers its 
>    buses as numbered so that board setup is correctly applied.  Plus minor 
>    corrections.

Minor corrections which would ideally belong to a separate patch
(there's a whole lot more cleanups that could be done in that driver,
BTW...)

> (...)
> --- linux-2.6.26-rc1-20080505.macro/drivers/i2c/busses/i2c-sibyte.c	2008-05-05 02:55:25.000000000 +0000
> +++ linux-2.6.26-rc1-20080505/drivers/i2c/busses/i2c-sibyte.c	2008-05-06 23:45:32.000000000 +0000
> @@ -2,6 +2,7 @@
>   * Copyright (C) 2004 Steven J. Hill
>   * Copyright (C) 2001,2002,2003 Broadcom Corporation
>   * Copyright (C) 1995-2000 Simon G. Vogl
> + * Copyright (C) 2008  Maciej W. Rozycki

I don't think that the minor changes below are enough for you to claim
copyright on that driver.

>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License
> @@ -132,18 +133,18 @@ static const struct i2c_algorithm i2c_si
>  /*
>   * registering functions to load algorithms at runtime
>   */
> -int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
> +static int __init i2c_sibyte_add_bus(struct i2c_adapter *i2c_adap, int speed)
>  {
>  	struct i2c_algo_sibyte_data *adap = i2c_adap->algo_data;
>  
> -	/* register new adapter to i2c module... */
> +	/* Register new adapter to i2c module...  */
>  	i2c_adap->algo = &i2c_sibyte_algo;
>  
> -	/* Set the frequency to 100 kHz */
> +	/* Set the requested frequency.  */

Why do you double the space and the end of comments? Never seen that
before, and I can't see the idea.

>  	csr_out32(speed, SMB_CSR(adap,R_SMB_FREQ));
>  	csr_out32(0, SMB_CSR(adap,R_SMB_CONTROL));
>  
> -	return i2c_add_adapter(i2c_adap);
> +	return i2c_add_numbered_adapter(i2c_adap);
>  }
>  
>  
> @@ -159,6 +160,7 @@ static struct i2c_adapter sibyte_board_a
>  		.class		= I2C_CLASS_HWMON,
>  		.algo		= NULL,
>  		.algo_data	= &sibyte_board_data[0],
> +		.nr		= 0,
>  		.name		= "SiByte SMBus 0",
>  	},
>  	{
> @@ -167,6 +169,7 @@ static struct i2c_adapter sibyte_board_a
>  		.class		= I2C_CLASS_HWMON,
>  		.algo		= NULL,
>  		.algo_data	= &sibyte_board_data[1],
> +		.nr		= 1,
>  		.name		= "SiByte SMBus 1",
>  	},
>  };

I'm not sure how you intend to push these changes upstream. I would
take a patch only touching drivers/i2c/busses/i2c-sibyte.c in my i2c
tree, however a patch also touching arch code, must be handled be the
maintainer for that architecture or platform.

-- 
Jean Delvare

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Jean Delvare]

Oh, BTW...

On Wed, 7 May 2008 01:40:27 +0100 (BST), Maciej W. Rozycki wrote:
> (...)
> 1. i2c-swarm.c -- SWARM I2C board setup, currently for the M41T80 chip on 
>    the bus #1 only.
> (...)
> --- linux-2.6.26-rc1-20080505.macro/arch/mips/sibyte/swarm/Makefile	2004-01-29 04:57:05.000000000 +0000
> +++ linux-2.6.26-rc1-20080505/arch/mips/sibyte/swarm/Makefile	2008-05-06 01:18:21.000000000 +0000
> @@ -1,3 +1,4 @@
> -lib-y				= setup.o rtc_xicor1241.o rtc_m41t81.o
> +obj-y				:= setup.o rtc_xicor1241.o rtc_m41t81.o
>  
> -lib-$(CONFIG_KGDB)		+= dbg_io.o
> +obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-swarm.o
> +obj-$(CONFIG_KGDB)		+= dbg_io.o
> (...)
> --- linux-2.6.26-rc1-20080505.macro/arch/mips/sibyte/swarm/i2c-swarm.c	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.26-rc1-20080505/arch/mips/sibyte/swarm/i2c-swarm.c	2008-05-06 23:51:34.000000000 +0000

i2c-foo.c is consistently used for i2c bus driver themselves so far.
It's somewhat confusing to see you name platform code that way. It's
also redundant, given that the file lives in the swarm platform
directory. May I suggest naming this file just
arch/mips/sibyte/swarm/i2c.c? Other architectures (cris, arm) are doing
this already.

-- 
Jean Delvare

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Andrew Morton]

On Wed, 7 May 2008 08:59:53 +0200 Jean Delvare <khali@linux-fr.org> wrote:

> I'm not sure how you intend to push these changes upstream. I would
> take a patch only touching drivers/i2c/busses/i2c-sibyte.c in my i2c
> tree, however a patch also touching arch code, must be handled be the
> maintainer for that architecture or platform.

Not "must".  The arch maintainer could ask you to merge it or you could ask
the arch maintainer to merge it.

It's some little one-line change like this one appeared to be, it's
fair to assume the arch maintainer won't care much about it.  View it as an
i2c patch?

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Geert Uytterhoeven]

On Wed, 7 May 2008, Jean Delvare wrote:
> Oh, BTW...
> 
> On Wed, 7 May 2008 01:40:27 +0100 (BST), Maciej W. Rozycki wrote:
> > (...)
> > 1. i2c-swarm.c -- SWARM I2C board setup, currently for the M41T80 chip on 
> >    the bus #1 only.
> > (...)
> > --- linux-2.6.26-rc1-20080505.macro/arch/mips/sibyte/swarm/Makefile	2004-01-29 04:57:05.000000000 +0000
> > +++ linux-2.6.26-rc1-20080505/arch/mips/sibyte/swarm/Makefile	2008-05-06 01:18:21.000000000 +0000
> > @@ -1,3 +1,4 @@
> > -lib-y				= setup.o rtc_xicor1241.o rtc_m41t81.o
> > +obj-y				:= setup.o rtc_xicor1241.o rtc_m41t81.o
> >  
> > -lib-$(CONFIG_KGDB)		+= dbg_io.o
> > +obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-swarm.o
> > +obj-$(CONFIG_KGDB)		+= dbg_io.o
> > (...)
> > --- linux-2.6.26-rc1-20080505.macro/arch/mips/sibyte/swarm/i2c-swarm.c	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6.26-rc1-20080505/arch/mips/sibyte/swarm/i2c-swarm.c	2008-05-06 23:51:34.000000000 +0000
> 
> i2c-foo.c is consistently used for i2c bus driver themselves so far.
> It's somewhat confusing to see you name platform code that way. It's
> also redundant, given that the file lives in the swarm platform
> directory. May I suggest naming this file just
> arch/mips/sibyte/swarm/i2c.c? Other architectures (cris, arm) are doing
> this already.

Is there any chance CONFIG_I2C_BOARDINFO could become tristate?
If yes, it's problematic if you have multiple modules called i2c.ko.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Jean Delvare]

Hi Geert,

On Wed, 7 May 2008 09:37:01 +0200 (CEST), Geert Uytterhoeven wrote:
> On Wed, 7 May 2008, Jean Delvare wrote:
> > Oh, BTW...
> > 
> > On Wed, 7 May 2008 01:40:27 +0100 (BST), Maciej W. Rozycki wrote:
> > > (...)
> > > 1. i2c-swarm.c -- SWARM I2C board setup, currently for the M41T80 chip on 
> > >    the bus #1 only.
> > > (...)
> > > --- linux-2.6.26-rc1-20080505.macro/arch/mips/sibyte/swarm/Makefile	2004-01-29 04:57:05.000000000 +0000
> > > +++ linux-2.6.26-rc1-20080505/arch/mips/sibyte/swarm/Makefile	2008-05-06 01:18:21.000000000 +0000
> > > @@ -1,3 +1,4 @@
> > > -lib-y				= setup.o rtc_xicor1241.o rtc_m41t81.o
> > > +obj-y				:= setup.o rtc_xicor1241.o rtc_m41t81.o
> > >  
> > > -lib-$(CONFIG_KGDB)		+= dbg_io.o
> > > +obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-swarm.o
> > > +obj-$(CONFIG_KGDB)		+= dbg_io.o
> > > (...)
> > > --- linux-2.6.26-rc1-20080505.macro/arch/mips/sibyte/swarm/i2c-swarm.c	1970-01-01 00:00:00.000000000 +0000
> > > +++ linux-2.6.26-rc1-20080505/arch/mips/sibyte/swarm/i2c-swarm.c	2008-05-06 23:51:34.000000000 +0000
> > 
> > i2c-foo.c is consistently used for i2c bus driver themselves so far.
> > It's somewhat confusing to see you name platform code that way. It's
> > also redundant, given that the file lives in the swarm platform
> > directory. May I suggest naming this file just
> > arch/mips/sibyte/swarm/i2c.c? Other architectures (cris, arm) are doing
> > this already.
> 
> Is there any chance CONFIG_I2C_BOARDINFO could become tristate?
> If yes, it's problematic if you have multiple modules called i2c.ko.

No, CONFIG_I2C_BOARDINFO is boolean by nature, it will never become
tristate.

-- 
Jean Delvare

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Maciej W. Rozycki]

Hi Jean,

> Minor corrections which would ideally belong to a separate patch
> (there's a whole lot more cleanups that could be done in that driver,
> BTW...)

 Not suprising, as usually with most pieces of code for the SWARM and the 
SiByte SOC.  That can be done gradually, but mixing a driver overhaul with 
functional changes usually only results in confusion later on.

> I don't think that the minor changes below are enough for you to claim
> copyright on that driver.

 Well, I decide whether or not to add one based on how important changes
are from the piece's of code point of view.  In this case the change is
essential for new-style client drivers to work at all, which I think is
more important than e.g. a lot of cosmetical changes throughout would be.  
But I do not insist on keeping it -- if you think I misjudged on this
occasion, I see no problem with discarding it.

> Why do you double the space and the end of comments? Never seen that
> before, and I can't see the idea.

 This is mostly habitual -- this is what the GNU Coding Standard specifies
for comments and which is enforced for GNU software which I have dealt a
lot with.  I think the idea is it improves readability and I tend to
agree.  The same goes for using a capital at the beginning and a full stop
at the end of sentences in comments -- it improves readability and
(together with a good style of code itself) makes the result look more
professional.  Certainly well-formatted code is easier to comprehend for 
someone looking at it for the first time.

 I do not insist on the extraneous space if you have a strong opinion 
against though.

> I'm not sure how you intend to push these changes upstream. I would
> take a patch only touching drivers/i2c/busses/i2c-sibyte.c in my i2c
> tree, however a patch also touching arch code, must be handled be the
> maintainer for that architecture or platform.

 Andrew has spoken (thank you, Andrew) and I would only like to add an
explanation why I have not split this change further.  Certainly it is
functionally consistent.  Then adding i2c-swarm.c only breaks things as
the onchip buses suddenly get the numbers 2 and 3.  On the other hand, if
adding the i2c-sibyte.c change only, it will take a while until it
propagates back to the MIPS tree and without that as it is there is no
single way to use the whole set of changes as the clock device will not be
seen.

 If you are scared off by the MIPS-specific Makefile (lib vs obj) changes,
then I think they should be reasonably easy to sort out separately in a
couple of days as functionally not changing anything.  The only other file
in the affected subdirectory that depends on a config option uses
CONFIG_KGDB which does not seem to rely on being pulled implicitly by the
linker.  But such a mechanical change by itself would make little sense 
(don't fix what isn't broken), so I have not pushed it without a 
reasonable justification.

 Ralf -- what do you think about the Makefile changes?  I can send you a 
separate patch which will reduce the span of this one.

  Maciej

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Maciej W. Rozycki]

Hi Jean,

> > > i2c-foo.c is consistently used for i2c bus driver themselves so far.
> > > It's somewhat confusing to see you name platform code that way. It's
> > > also redundant, given that the file lives in the swarm platform
> > > directory. May I suggest naming this file just
> > > arch/mips/sibyte/swarm/i2c.c? Other architectures (cris, arm) are doing
> > > this already.
> > 
> > Is there any chance CONFIG_I2C_BOARDINFO could become tristate?
> > If yes, it's problematic if you have multiple modules called i2c.ko.
> 
> No, CONFIG_I2C_BOARDINFO is boolean by nature, it will never become
> tristate.

 I can do that and I have considered it while preparing the change.  What
convinced me not to use a name that is already present elsewhere in the
tree is the confusion that it sometimes causes.  For example during a
debugging session GDB only reports the file name and not the leading
pathname (and some people do run GDB over the kernel).  Of course the
actual file can still be chased with some `find' and `grep' scriptery, but
why to create a problem in the first place?

 I consider repeated file names throughout a tree of a single program a 
namespace pollution similar to one with repeated static symbol names.  
While syntactically valid and working, it asks for unnecessary confusion.

 This is my point of view, but I can see others may not necessarily follow
it.  I am fine with changing the name to i2c.c as it is unlikely I will
run GDB over it. ;-)

  Maciej

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Ralf Baechle]

On Wed, May 07, 2008 at 10:13:23PM +0100, Maciej W. Rozycki wrote:

>  Ralf -- what do you think about the Makefile changes?  I can send you a 
> separate patch which will reduce the span of this one.

I like it; we maybe should consider getting rid of most of the libs-* stuff
in arch/mips/Makefile.  Some of it might be causing subtle bugs such as
routines exported to modules not getting linked into the kernel proper.

  Ralf

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Ralf Baechle]

On Wed, May 07, 2008 at 10:25:08PM +0100, Maciej W. Rozycki wrote:

> > > > i2c-foo.c is consistently used for i2c bus driver themselves so far.
> > > > It's somewhat confusing to see you name platform code that way. It's
> > > > also redundant, given that the file lives in the swarm platform
> > > > directory. May I suggest naming this file just
> > > > arch/mips/sibyte/swarm/i2c.c? Other architectures (cris, arm) are doing
> > > > this already.

>  I can do that and I have considered it while preparing the change.  What
> convinced me not to use a name that is already present elsewhere in the
> tree is the confusion that it sometimes causes.  For example during a
> debugging session GDB only reports the file name and not the leading
> pathname (and some people do run GDB over the kernel).  Of course the
> actual file can still be chased with some `find' and `grep' scriptery, but
> why to create a problem in the first place?
> 
>  I consider repeated file names throughout a tree of a single program a 
> namespace pollution similar to one with repeated static symbol names.  
> While syntactically valid and working, it asks for unnecessary confusion.
> 
>  This is my point of view, but I can see others may not necessarily follow
> it.  I am fine with changing the name to i2c.c as it is unlikely I will
> run GDB over it. ;-)

I've started using unique prefixes such as ip22- or ip27- a while ago.
And why not following that example with arch/mips/sibyte/swarm/swarm-i2c.c
or similar?

  Ralf

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Jean Delvare]

Hi Maciej,

On Wed, 7 May 2008 22:25:08 +0100 (BST), Maciej W. Rozycki wrote:
> > i2c-foo.c is consistently used for i2c bus driver themselves so far.
> > It's somewhat confusing to see you name platform code that way. It's
> > also redundant, given that the file lives in the swarm platform
> > directory. May I suggest naming this file just
> > arch/mips/sibyte/swarm/i2c.c? Other architectures (cris, arm) are doing
> > this already.
>
>  I can do that and I have considered it while preparing the change.  What
> convinced me not to use a name that is already present elsewhere in the
> tree is the confusion that it sometimes causes.  For example during a
> debugging session GDB only reports the file name and not the leading
> pathname (and some people do run GDB over the kernel).  Of course the
> actual file can still be chased with some `find' and `grep' scriptery, but
> why to create a problem in the first place?
>
>  I consider repeated file names throughout a tree of a single program a 
> namespace pollution similar to one with repeated static symbol names.  
> While syntactically valid and working, it asks for unnecessary confusion.

$ find linux-2.6.26-rc1 -name Kconfig | wc -l
455
$ find linux-2.6.26-rc1 -name Makefile | wc -l
1030
$

Not to mention the 102 setup.c, 87 irq.c, 62 time.c... It is very
common to have duplicated file names in the kernel tree because it
supports so many architectures and platforms. In general, when you work
on a given architecture or platform, names become unique again. Taking
GDB as an example again, you definitely know what architecture you are
debugging, so there should be relatively little ambiguity on what files
are involved.

(On top of that, I'd argue that we _should_ be able to display relative
paths to file names when debugging.)

Your point about the "single program namespace" is certainly valid for
small to medium-size programs, but in the case of something as big as
the kernel, it probably no longer holds.

>  This is my point of view, but I can see others may not necessarily follow
> it.  I am fine with changing the name to i2c.c as it is unlikely I will
> run GDB over it. ;-)

I don't have a strong opinion on this either, it is very unlikely that
I'll ever have to deal with this file personally. I'm only telling you
what the common practice is in the kernel tree.

-- 
Jean Delvare

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Jean Delvare]

Hi Maciej,

On Wed, 7 May 2008 22:13:23 +0100 (BST), Maciej W. Rozycki wrote:
> Hi Jean,
> 
> > Minor corrections which would ideally belong to a separate patch
> > (there's a whole lot more cleanups that could be done in that driver,
> > BTW...)
> 
>  Not suprising, as usually with most pieces of code for the SWARM and the 
> SiByte SOC.  That can be done gradually, but mixing a driver overhaul with 
> functional changes usually only results in confusion later on.

I fully agree.

> > I don't think that the minor changes below are enough for you to claim
> > copyright on that driver.
> 
>  Well, I decide whether or not to add one based on how important changes
> are from the piece's of code point of view.  In this case the change is
> essential for new-style client drivers to work at all, which I think is
> more important than e.g. a lot of cosmetical changes throughout would be.  
> But I do not insist on keeping it -- if you think I misjudged on this
> occasion, I see no problem with discarding it.

If you had to add a missing semicolon to a source file to get it to
build again, it would be an "essential" change (without it nothing
works) but still, you can't claim you added any intellectual value to
the source file. So, no copyright. The copyright is about how much
value you add, not how important the change is in the big picture.

> > Why do you double the space and the end of comments? Never seen that
> > before, and I can't see the idea.
> 
>  This is mostly habitual -- this is what the GNU Coding Standard specifies
> for comments and which is enforced for GNU software which I have dealt a
> lot with.

>From Documentation/CodingStyle:

"First off, I'd suggest printing out a copy of the GNU coding standards,
and NOT read it.  Burn them, it's a great symbolic gesture."

I'm not going to tell how bad I think the GNU coding standards are, the
point here is that we don't follow them at all, so whatever they say is
totally irrelevant. Read Documentation/CodingStyle, it describes what
we do. Also make sure that you run your patches through
scripts/checkpatch.pl. The rest is up to you, but in general, when
modifying existing code, you want to stick to what the surrounding code
looks like.

>  I think the idea is it improves readability and I tend to
> agree.  The same goes for using a capital at the beginning and a full stop
> at the end of sentences in comments -- it improves readability and
> (together with a good style of code itself) makes the result look more
> professional.  Certainly well-formatted code is easier to comprehend for 
> someone looking at it for the first time.
> 
>  I do not insist on the extraneous space if you have a strong opinion 
> against though.

I do insist ;) Admittedly, double spaces at end of comments are used in
some places in the kernel tree (I had never seen that before), but they
are still outnumbered by single-space ending comments, 50 to 1. Do
what you want in the drivers your create or maintain, but please don't
change existing comments, especially not in the middle of functional
changes.

> > I'm not sure how you intend to push these changes upstream. I would
> > take a patch only touching drivers/i2c/busses/i2c-sibyte.c in my i2c
> > tree, however a patch also touching arch code, must be handled be the
> > maintainer for that architecture or platform.
> 
>  Andrew has spoken (thank you, Andrew) and I would only like to add an
> explanation why I have not split this change further.  Certainly it is
> functionally consistent.  Then adding i2c-swarm.c only breaks things as
> the onchip buses suddenly get the numbers 2 and 3.  On the other hand, if
> adding the i2c-sibyte.c change only, it will take a while until it
> propagates back to the MIPS tree and without that as it is there is no
> single way to use the whole set of changes as the clock device will not be
> seen.
> 
>  If you are scared off by the MIPS-specific Makefile (lib vs obj) changes,
> then I think they should be reasonably easy to sort out separately in a
> couple of days as functionally not changing anything.  The only other file
> in the affected subdirectory that depends on a config option uses
> CONFIG_KGDB which does not seem to rely on being pulled implicitly by the
> linker.  But such a mechanical change by itself would make little sense 
> (don't fix what isn't broken), so I have not pushed it without a 
> reasonable justification.
> 
>  Ralf -- what do you think about the Makefile changes?  I can send you a 
> separate patch which will reduce the span of this one.

That's not a matter of being scared, and I was also _not_ asking you to
split the patch. That's a matter of synchronizing merges between me and
the architecture maintainer. If I take a patch in my i2c tree which
touches architecture-specific files, and I only push it to Linus in 2
months, then chances are that the architecture-specific files in
question will change several times meanwhile, resulting in conflicts in
-next and -mm. I am only trying to prevent this from happening. I
simply think that it is easier to synchronize patches if all
architecture-specific patches go through the relevant architecture tree.

BTW, SWARM seems to be only one of the 4 SiByte platforms we support.
What about the other ones? Your changes to the i2c-sibyte driver could
cause the i2c bus registration to fail, as the other platforms do not
declare I2C devices, the bus numbers 0 and 1 won't be reserved by
i2c-core. Care to comment on this?

Thanks,
-- 
Jean Delvare

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Maciej W. Rozycki]

On Thu, 8 May 2008, Ralf Baechle wrote:

> I like it; we maybe should consider getting rid of most of the libs-* stuff
> in arch/mips/Makefile.  Some of it might be causing subtle bugs such as
> routines exported to modules not getting linked into the kernel proper.

 Well, I think it should be reasonable to convert everything but lib/, the
latter being used for various implicit bits like strcpy() or whatever some
version of GCC may come up with.  Plus possibly generic code overridable
by boards for optimisation; I am not sure if we have got left any at the
moment though.

 With the much better configuration language of 2.6 and more and more 
stuff being done by magic sections bits built as libraries cause more 
hassle than convenience.

  Maciej

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Maciej W. Rozycki]

Hi Jean,

> If you had to add a missing semicolon to a source file to get it to
> build again, it would be an "essential" change (without it nothing
> works) but still, you can't claim you added any intellectual value to

 Agreed about this example because the change is mechanical and can almost
be done by an automaton.

> the source file. So, no copyright. The copyright is about how much
> value you add, not how important the change is in the big picture.

 As I wrote, no big concern about it from my side and it looks I will be 
changing more of the file anyway. ;-)

> I'm not going to tell how bad I think the GNU coding standards are, the
> point here is that we don't follow them at all, so whatever they say is
> totally irrelevant. Read Documentation/CodingStyle, it describes what

 Oh come on -- that's just common sense.  If something is good, there is
no point in discarding it without thinking, just because it is a part of a
bigger entity that we consider bad.  I consider it good not because it is
a part of the GNU standard, but because I have concluded that it is and it
is pure coincidence ;-) I have taken it from the said standard.  But as I
said, this is a minor nit here and I can resist from adding extraneous
spaces in pieces of code you are interested in as long as I am able to
track which ones they actually are.

> we do. Also make sure that you run your patches through
> scripts/checkpatch.pl. The rest is up to you, but in general, when
> modifying existing code, you want to stick to what the surrounding code
> looks like.

 I had no problems with writing code checkpatch.pl would swallow without a
blink even before it existed.  It does not mean I should follow the
surrounding mess if this is what the state of code is.

> I do insist ;) Admittedly, double spaces at end of comments are used in
> some places in the kernel tree (I had never seen that before), but they
> are still outnumbered by single-space ending comments, 50 to 1. Do
> what you want in the drivers your create or maintain, but please don't
> change existing comments, especially not in the middle of functional
> changes.

 Fine with me, no problem.

> That's not a matter of being scared, and I was also _not_ asking you to
> split the patch. That's a matter of synchronizing merges between me and
> the architecture maintainer. If I take a patch in my i2c tree which
> touches architecture-specific files, and I only push it to Linus in 2
> months, then chances are that the architecture-specific files in
> question will change several times meanwhile, resulting in conflicts in
> -next and -mm. I am only trying to prevent this from happening. I
> simply think that it is easier to synchronize patches if all
> architecture-specific patches go through the relevant architecture tree.

 Your concern is valid and this is why I proposed the split.  My point
being, unlike the rest of the MIPS arch tree these days the Broadcom bits
are only touched from time to time by two or three people (myself
included), so it is much easier to coordinate changes if they are limited 
to this subarch.  Which means stripping out changes needed elsewhere from 
the i2c patch itself can only improve things.

> BTW, SWARM seems to be only one of the 4 SiByte platforms we support.

 I think nobody really knows for sure how many Broadcom/SiByte platforms 
we support at the moment. ;-)  I am fairly sure there is some interest in 
the BigSur, SWARM and Sentosa only.

> What about the other ones? Your changes to the i2c-sibyte driver could
> cause the i2c bus registration to fail, as the other platforms do not
> declare I2C devices, the bus numbers 0 and 1 won't be reserved by
> i2c-core. Care to comment on this?

 Well, arch/mips/sibyte/swarm/ is included for all the three above as well 
as a couple of other I may not necessarily be sure what they are even.  So 
this should be of no concern.

 BTW, do you mean i2c_add_numbered_adapter() will fail if no devices have
been declared to exist on the given bus with i2c_register_board_info()?  
That sounds strange...  Note in all cases there are EEPROMs (onboard ones
as well as optionally SPD ones) on both buses on Broadcom/SiByte boards
and they are handled by a legacy client driver.  The Broadcom SOC is
actually capable to bootstrap from one of these EEPROMs (rather than form
the usual system parallel Flash ROM).

  Maciej

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Jean Delvare]

Hi Maciej,

On Fri, 9 May 2008 00:10:47 +0100 (BST), Maciej W. Rozycki wrote:
> > I'm not going to tell how bad I think the GNU coding standards are, the
> > point here is that we don't follow them at all, so whatever they say is
> > totally irrelevant. Read Documentation/CodingStyle, it describes what
> 
>  Oh come on -- that's just common sense.  If something is good, there is
> no point in discarding it without thinking, just because it is a part of a
> bigger entity that we consider bad.  I consider it good not because it is
> a part of the GNU standard, but because I have concluded that it is and it
> is pure coincidence ;-) I have taken it from the said standard.

Let me just quote you:

"This is mostly habitual -- this is what the GNU Coding Standard specifies
for comments and which is enforced for GNU software which I have dealt a
lot with."

You didn't say it was common sense. You did say that it was what the
GNU Coding Standard specified, and as a consequence, what you were used
to. So please keep your "oh come on" for yourself, you pointed the
discussion in this direction yourself.

>                                                                   But as I
> said, this is a minor nit here and I can resist from adding extraneous
> spaces in pieces of code you are interested in as long as I am able to
> track which ones they actually are.

What matters is not "the pieces of code I am interested in", but the
pieces of code _you_ are the master of, or not. As explained somewhere
else in this thread, you are free to use whatever style you like (as
long as it complies with Documentation/CodingStyle, that is) in new
code you write and in code you maintain. For all the rest, you should
stick to the surrounding style. This is common sense, as you'd say.

> (...)
>  Well, arch/mips/sibyte/swarm/ is included for all the three above as well 
> as a couple of other I may not necessarily be sure what they are even.  So 
> this should be of no concern.
> 
>  BTW, do you mean i2c_add_numbered_adapter() will fail if no devices have
> been declared to exist on the given bus with i2c_register_board_info()?  
> That sounds strange...

i2c_add_numbered_adapter() _may_ fail if no I2C devices have been
declared _and_ other i2c adapters are registered using
i2c_add_adapter(). When you declare I2C devices, i2c-core reserves the
bus numbers in question for i2c_add_numbered_adapter() and they cannot
be used for i2c_add_adapter(). This is what guarantees that the calls
to i2c_add_numbered_adapter() (in i2c-sibyte for example) will succeed.
If no I2C devices are declared, bus numbers are not reserved, so if it
happens that another I2C bus driver registers itself before i2c-sibyte
does, when i2c-sibyte calls i2c_add_numbered_adapter(), the bus number
is already in use and the call fails.

I admit that this is unlikely to happen, but depending on what exact
hardware there is on the system and what drivers are built-in, it could
happen. I think it is a weakness of i2c-core, it should allow platform
code to reserve i2c bus numbers regardless of what devices are
registered. I seem to remember I said that when the code was added to
the kernel already. I guess we'll have to fix it the day it actually
breaks.

BTW, i2c-sibyte should be converted to a proper platform driver, so
that only platforms with such a device instantiate it.

>                         Note in all cases there are EEPROMs (onboard ones
> as well as optionally SPD ones) on both buses on Broadcom/SiByte boards
> and they are handled by a legacy client driver.  The Broadcom SOC is
> actually capable to bootstrap from one of these EEPROMs (rather than form
> the usual system parallel Flash ROM).

Which legacy driver, "eeprom"? You should probably look into David
Brownell's at24c driver:
http://lists.lm-sensors.org/pipermail/i2c/2008-April/003307.html
If it gets enough attention and testing, it could go upstream quickly.

-- 
Jean Delvare

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Maciej W. Rozycki]

Hi Jean,

> $ find linux-2.6.26-rc1 -name Kconfig | wc -l
> 455
> $ find linux-2.6.26-rc1 -name Makefile | wc -l
> 1030
> $

 Well, these are not pieces of code and serve a different purpose, don't
they?

> Not to mention the 102 setup.c, 87 irq.c, 62 time.c... It is very
> common to have duplicated file names in the kernel tree because it
> supports so many architectures and platforms. In general, when you work

 Well, that is not a technical argument.  It is a fact of life, sure, but
it does not necessarily mean it is right, but perhaps that nobody has
really thought about it.

> on a given architecture or platform, names become unique again. Taking
> GDB as an example again, you definitely know what architecture you are
> debugging, so there should be relatively little ambiguity on what files
> are involved.

 Hmm, why to have little ambiguity, when you can have none?  We do not
rely on crippled filesystems, so we do not have to save characters in file
names -- we keep them reasonably short anyway.  I say there is no
technical advantage in having duplicate file names throughout the tree
(please name one if I am wrong) and there are advantages -- however small,
but still -- in keeping file names unique.  Therefore the gain from
converting the existing file names may not justify the effort required,
but it does not mean new additions may not take the gain into account?

> (On top of that, I'd argue that we _should_ be able to display relative
> paths to file names when debugging.)

 Human's perception is limited -- GDB's `info frame' is probably already
overloaded with information, so adding the path to the source file in
question will not make it any better.

> Your point about the "single program namespace" is certainly valid for
> small to medium-size programs, but in the case of something as big as
> the kernel, it probably no longer holds.

 I think it is actually the reverse -- the bigger a project is, the easier
to get lost within. ;-)  With small programs it is easier to maintain,
while with bigger ones it is really where it pays off.

> I don't have a strong opinion on this either, it is very unlikely that
> I'll ever have to deal with this file personally. I'm only telling you
> what the common practice is in the kernel tree.

 I don't think this practice has been architected and see above for
justification why it may not necessarily be the cleverest idea. :-)

  Maciej

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Maciej W. Rozycki]

Hi Jean,

> Let me just quote you:
> 
> "This is mostly habitual -- this is what the GNU Coding Standard specifies
> for comments and which is enforced for GNU software which I have dealt a
> lot with."
> 
> You didn't say it was common sense. You did say that it was what the
> GNU Coding Standard specified, and as a consequence, what you were used
> to. So please keep your "oh come on" for yourself, you pointed the
> discussion in this direction yourself.

 Well, I take no habits that make no sense in the first place.  And I have
gone into great lengths to explain and justify what drives me in this case
-- I got it from the GNU standard and got convinced it is good, so I got
to using it.  I can write comments according to a different style, no
problem (as long as there is any defined style for a given case), but I
have to put some explicit effort into it.

 Similarly, habitually I write code in the Linux indentation style because
I like it, but I can use your hated GNU style (or any other that follows
any recognisable rules) as well, except I have to put some brainpower into
it.

> What matters is not "the pieces of code I am interested in", but the
> pieces of code _you_ are the master of, or not. As explained somewhere
> else in this thread, you are free to use whatever style you like (as
> long as it complies with Documentation/CodingStyle, that is) in new
> code you write and in code you maintain. For all the rest, you should
> stick to the surrounding style. This is common sense, as you'd say.

 Well, sorry, but I could only sense the lack of style in this piece of
code, which is why I tried to apply some.  You are free to disagree and as
you have undertaken maintenance of this area I am going to respect it.

> BTW, i2c-sibyte should be converted to a proper platform driver, so
> that only platforms with such a device instantiate it.

 The whole of SiByte support should eventually get converted to implement
platform initialisation.  I started some of this with changes to the
sb1250-mac.c Ethernet driver sometime in 2006, but no further progress has 
been made since.  I have other priorities higher on the list, but I have 
not forgotten about it and will come back at some point unless someone 
else does it first.

> Which legacy driver, "eeprom"? You should probably look into David
> Brownell's at24c driver:
> http://lists.lm-sensors.org/pipermail/i2c/2008-April/003307.html
> If it gets enough attention and testing, it could go upstream quickly.

 I can see if I can find a couple of cycles to spare and give this piece
of code a shot with my SWARM.  There is a pair of 128kB EEPROM chips
onboard (one as a bootstrap option and one to store configuration) and I
have two SDRAM modules installed providing another pair of a smaller size.

  Maciej

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Jean Delvare]

Hi Maciej,

On Fri, 9 May 2008 21:27:04 +0100 (BST), Maciej W. Rozycki wrote:
>  Well, sorry, but I could only sense the lack of style in this piece of
> code, which is why I tried to apply some.

I agree, i2c-sibyte is very old code, and unmaintained, coding style is
horrible. That's one more reason to not include random style cleanups
in a patch doing functional changes, the improvement will hardly be
visible. If you care about cleaning up the code of this driver - and I
would appreciate that - please make a separate patch.

>                                            You are free to disagree and as
> you have undertaken maintenance of this area I am going to respect it.

And I thank you for that.

-- 
Jean Delvare

Re: [RFC][PATCH 2/4] RTC: SWARM I2C board initialization

[Maciej W. Rozycki]

Hi Jean,

> I agree, i2c-sibyte is very old code, and unmaintained, coding style is
> horrible. That's one more reason to not include random style cleanups
> in a patch doing functional changes, the improvement will hardly be
> visible. If you care about cleaning up the code of this driver - and I
> would appreciate that - please make a separate patch.

 While I have not officially undertaken maintenance of code to support any
of the pieces of hardware relevant to the SWARM board, I have done a
considerable number of changes throughout all the bits and if you have any
concern related to this piece of code (or any other I am referring to
here), then you are certainly welcome to contact me directly, cc-ing
linux-mips, perhaps.  I do SWARM development solely in my free time and I
simply cannot afford chasing all the bits around, especially ones I do not
actively use.  I do try to care about bits other people have concerns
about though.

  Maciej