Fwd: PCF8583 not detected on RiscPC

Fwd: PCF8583 not detected on RiscPC

[Russell King - ARM Linux]

Previous mail sent just to Jean:

| Any ideas why the RTC isn't being detected on the ARM RiscPC platform?
| 
| Could it be because someone's decided to regress stuff because of having
| an i2c_boardinfo structure without actually first making sure everything
| is converted over?
| 
| If yes, what do I need to do to make it work?  (IOW, please supply a
| patch to fix the regression or explain in detail what's required to make
| it work.)

To summarise the situation, the bus driver is:

drivers/i2c/busses/i2c-acorn.c

which is not a platform driver.  It is not initialised by the platform
code.  It does not provide i2c_boarddata or whatever the funky name for
that feature is.

However, PCF8583 got converted to require i2c_boarddata without first
fixing up the platform which uses it, thereby making the driver utterly
useless.

Putting i2c_boarddata into i2c-acorn.c feels wrong.

So, please either revert 02bb584f3b1cfc8188522a4d2c8881b65073a4f1 so
that the regression can be fixed (no RTC on Acorn RiscPC machines) or
provide a patch which fixes this mess that the above change causes.

Re: Fwd: PCF8583 not detected on RiscPC

[Russell King - ARM Linux]

On Sat, Feb 21, 2009 at 07:48:14PM +0000, Russell King - ARM Linux wrote:
> Previous mail sent just to Jean:
> 
> | Any ideas why the RTC isn't being detected on the ARM RiscPC platform?
> | 
> | Could it be because someone's decided to regress stuff because of having
> | an i2c_boardinfo structure without actually first making sure everything
> | is converted over?
> | 
> | If yes, what do I need to do to make it work?  (IOW, please supply a
> | patch to fix the regression or explain in detail what's required to make
> | it work.)
> 
> To summarise the situation, the bus driver is:
> 
> drivers/i2c/busses/i2c-acorn.c
> 
> which is not a platform driver.  It is not initialised by the platform
> code.  It does not provide i2c_boarddata or whatever the funky name for
> that feature is.
> 
> However, PCF8583 got converted to require i2c_boarddata without first
> fixing up the platform which uses it, thereby making the driver utterly
> useless.
> 
> Putting i2c_boarddata into i2c-acorn.c feels wrong.
> 
> So, please either revert 02bb584f3b1cfc8188522a4d2c8881b65073a4f1 so
> that the regression can be fixed (no RTC on Acorn RiscPC machines) or
> provide a patch which fixes this mess that the above change causes.

Confirmation: reverting 02bb584f3b1cfc8188522a4d2c8881b65073a4f1 and
removing the reference to I2C_DRIVERID_PCF8583 results in the regression
being fixed and normal behaviour being restored.

Patch below.

diff --git a/drivers/rtc/rtc-pcf8583.c b/drivers/rtc/rtc-pcf8583.c
index 7d33cda..d93da76 100644
--- a/drivers/rtc/rtc-pcf8583.c
+++ b/drivers/rtc/rtc-pcf8583.c
@@ -2,7 +2,6 @@
  *  drivers/rtc/rtc-pcf8583.c
  *
  *  Copyright (C) 2000 Russell King
- *  Copyright (C) 2008 Wolfram Sang & Juergen Beisert, Pengutronix
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -15,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 #include <linux/rtc.h>
 #include <linux/init.h>
 #include <linux/errno.h>
@@ -27,6 +27,7 @@ struct rtc_mem {
 };
 
 struct pcf8583 {
+	struct i2c_client client;
 	struct rtc_device *rtc;
 	unsigned char ctrl;
 };
@@ -39,6 +40,10 @@ struct pcf8583 {
 #define CTRL_ALARM	0x02
 #define CTRL_TIMER	0x01
 
+static const unsigned short normal_i2c[] = { 0x50, I2C_CLIENT_END };
+
+/* Module parameters */
+I2C_CLIENT_INSMOD;
 
 static struct i2c_driver pcf8583_driver;
 
@@ -264,60 +269,105 @@ static const struct rtc_class_ops pcf8583_rtc_ops = {
 	.set_time	= pcf8583_rtc_set_time,
 };
 
-static int pcf8583_probe(struct i2c_client *client,
-				const struct i2c_device_id *id)
+static int pcf8583_probe(struct i2c_adapter *adap, int addr, int kind);
+
+static int pcf8583_attach(struct i2c_adapter *adap)
+{
+	return i2c_probe(adap, &addr_data, pcf8583_probe);
+}
+
+static int pcf8583_detach(struct i2c_client *client)
+{
+	int err;
+	struct pcf8583 *pcf = i2c_get_clientdata(client);
+	struct rtc_device *rtc = pcf->rtc;
+
+	if (rtc)
+		rtc_device_unregister(rtc);
+
+	if ((err = i2c_detach_client(client)))
+		return err;
+
+	kfree(pcf);
+	return 0;
+}
+
+static struct i2c_driver pcf8583_driver = {
+	.driver = {
+		.name	= "pcf8583",
+	},
+	.attach_adapter	= pcf8583_attach,
+	.detach_client	= pcf8583_detach,
+};
+
+static int pcf8583_probe(struct i2c_adapter *adap, int addr, int kind)
 {
-	struct pcf8583 *pcf8583;
+	struct pcf8583 *pcf;
+	struct i2c_client *client;
+	struct rtc_device *rtc;
+	unsigned char buf[1], ad[1] = { 0 };
 	int err;
+	struct i2c_msg msgs[2] = {
+		{
+			.addr = addr,
+			.flags = 0,
+			.len = 1,
+			.buf = ad,
+		}, {
+			.addr = addr,
+			.flags = I2C_M_RD,
+			.len = 1,
+			.buf = buf,
+		}
+	};
 
-	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
-		return -ENODEV;
+	if (!i2c_check_functionality(adap, I2C_FUNC_I2C))
+		return 0;
 
-	pcf8583 = kzalloc(sizeof(struct pcf8583), GFP_KERNEL);
-	if (!pcf8583)
+	pcf = kzalloc(sizeof(*pcf), GFP_KERNEL);
+	if (!pcf)
 		return -ENOMEM;
 
-	pcf8583->rtc = rtc_device_register(pcf8583_driver.driver.name,
-			&client->dev, &pcf8583_rtc_ops, THIS_MODULE);
+	client = &pcf->client;
 
-	if (IS_ERR(pcf8583->rtc)) {
-		err = PTR_ERR(pcf8583->rtc);
+	client->addr		= addr;
+	client->adapter	= adap;
+	client->driver	= &pcf8583_driver;
+
+	strlcpy(client->name, pcf8583_driver.driver.name, I2C_NAME_SIZE);
+
+	if (i2c_transfer(client->adapter, msgs, 2) != 2) {
+		err = -EIO;
 		goto exit_kfree;
 	}
 
-	i2c_set_clientdata(client, pcf8583);
-	return 0;
+	err = i2c_attach_client(client);
 
-exit_kfree:
-	kfree(pcf8583);
-	return err;
-}
+	if (err)
+		goto exit_kfree;
 
-static int __devexit pcf8583_remove(struct i2c_client *client)
-{
-	struct pcf8583 *pcf8583 = i2c_get_clientdata(client);
+	rtc = rtc_device_register(pcf8583_driver.driver.name, &client->dev,
+				  &pcf8583_rtc_ops, THIS_MODULE);
+
+	if (IS_ERR(rtc)) {
+		err = PTR_ERR(rtc);
+		goto exit_detach;
+	}
+
+	pcf->rtc = rtc;
+	i2c_set_clientdata(client, pcf);
+	set_ctrl(client, buf[0]);
 
-	if (pcf8583->rtc)
-		rtc_device_unregister(pcf8583->rtc);
-	kfree(pcf8583);
 	return 0;
-}
 
-static const struct i2c_device_id pcf8583_id[] = {
-	{ "pcf8583", 0 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, pcf8583_id);
+exit_detach:
+	i2c_detach_client(client);
 
-static struct i2c_driver pcf8583_driver = {
-	.driver = {
-		.name	= "pcf8583",
-		.owner	= THIS_MODULE,
-	},
-	.probe		= pcf8583_probe,
-	.remove		= __devexit_p(pcf8583_remove),
-	.id_table	= pcf8583_id,
-};
+exit_kfree:
+	kfree(pcf);
+
+	return err;
+}
 
 static __init int pcf8583_init(void)
 {

Re: Fwd: PCF8583 not detected on RiscPC

[Alessandro Zummo]

On Sat, 21 Feb 2009 20:41:47 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> Confirmation: reverting 02bb584f3b1cfc8188522a4d2c8881b65073a4f1 and
> removing the reference to I2C_DRIVERID_PCF8583 results in the regression
> being fixed and normal behaviour being restored.
> 
> Patch below.

 this is a nack on my side. the whole i2c has been converted to the
 new style model. everyone is following and this is
 the wrong way to fix the issue.

 the device must be instantiated in the appropriate
 platform code. 

 for an example of what needs to be done, you can grep for
 i2c_board_info in arch/arm .

 if you need help, just tell me where's the acorn related
 platform setup code and I'll have a look.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: Fwd: PCF8583 not detected on RiscPC

[Russell King - ARM Linux]

On Sun, Feb 22, 2009 at 01:19:36AM +0100, Alessandro Zummo wrote:
> On Sat, 21 Feb 2009 20:41:47 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > Confirmation: reverting 02bb584f3b1cfc8188522a4d2c8881b65073a4f1 and
> > removing the reference to I2C_DRIVERID_PCF8583 results in the regression
> > being fixed and normal behaviour being restored.
> > 
> > Patch below.
> 
>  this is a nack on my side. the whole i2c has been converted to the
>  new style model. everyone is following and this is
>  the wrong way to fix the issue.

While you may not desire the patch, the fact of the matter is that the
conversion caused a regression.  Regressions trump forward progress and
fixing stuff to work on new platforms.

So, really, I'm not listening to NACKs from anyone for this.  The only
thing I'll listen to is something _constructive_ to make it work again.
I'm sure Andrew Morton will back me up on this.

>  the device must be instantiated in the appropriate
>  platform code. 
> 
>  for an example of what needs to be done, you can grep for
>  i2c_board_info in arch/arm .
> 
>  if you need help, just tell me where's the acorn related
>  platform setup code and I'll have a look.

drivers/i2c/busses/i2c-acorn.c

That's where the problem is - it's totally unclear how to convert
this to the board_info method, because it doesn't live in the kernel
as a platform thing.  Putting the board_info stuff into i2c-acorn
just seems completely wrong.

Now, we can either totally reorganize i2c-acorn, but that won't be
acceptable for 2.6.29-rc.

The problem is that this *is* a regression, and therefore must be fixed
in 2.6.29-rc.  As I see it, the only sane way to do that is to revert
the conversion until a proper fix can be done.

So, please provide constructive suggestions on how to add boardinfo to
this in a sane way, or we revert PCF8583 back to something which works.

Re: Fwd: PCF8583 not detected on RiscPC

[Alessandro Zummo]

On Sun, 22 Feb 2009 08:28:29 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

 
> So, really, I'm not listening to NACKs from anyone for this.  The only
> thing I'll listen to is something _constructive_ to make it work again.
> I'm sure Andrew Morton will back me up on this.

 You never listened to anyone, as far as I can remember. The constructive
 part was where I said I'd be happy to give help.


> Now, we can either totally reorganize i2c-acorn, but that won't be
> acceptable for 2.6.29-rc.

  i2c-acorn is just fine as is, while you might want to change
 it to a platform driver at a later time.


> The problem is that this *is* a regression, and therefore must be fixed
> in 2.6.29-rc.  As I see it, the only sane way to do that is to revert
> the conversion until a proper fix can be done.
> 
> So, please provide constructive suggestions on how to add boardinfo to
> this in a sane way, or we revert PCF8583 back to something which works.

  That's very simple, even if acorn is a bit unstructured. You just
 need to choice a place under arch/arm that you like
 (arch/arm/plat-acorn/ ? ) and place a bit of code called
 by an appropriate initcall. 

  There you write something like that, changing
 the values to match your rtc name and i2c address:

static struct i2c_board_info __initdata nslu2_i2c_board_info [] = {
        {
                I2C_BOARD_INFO("x1205", 0x6f),
        },
};
 
...

   i2c_register_board_info(0, nslu2_i2c_board_info,
                                ARRAY_SIZE(nslu2_i2c_board_info));

 
 Unless I missed something that should be all you need.
    

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: Fwd: PCF8583 not detected on RiscPC

[Russell King - ARM Linux]

On Sun, Feb 22, 2009 at 10:42:05AM +0100, Alessandro Zummo wrote:
> On Sun, 22 Feb 2009 08:28:29 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
>  
> > So, really, I'm not listening to NACKs from anyone for this.  The only
> > thing I'll listen to is something _constructive_ to make it work again.
> > I'm sure Andrew Morton will back me up on this.
> 
>  You never listened to anyone, as far as I can remember. The constructive
>  part was where I said I'd be happy to give help.

Ditto your listening ability over the fact that the PCF8583 driver is
platform specific.

> > Now, we can either totally reorganize i2c-acorn, but that won't be
> > acceptable for 2.6.29-rc.
> 
>   i2c-acorn is just fine as is, while you might want to change
>  it to a platform driver at a later time.
> 
> 
> > The problem is that this *is* a regression, and therefore must be fixed
> > in 2.6.29-rc.  As I see it, the only sane way to do that is to revert
> > the conversion until a proper fix can be done.
> > 
> > So, please provide constructive suggestions on how to add boardinfo to
> > this in a sane way, or we revert PCF8583 back to something which works.
> 
>   That's very simple, even if acorn is a bit unstructured. You just
>  need to choice a place under arch/arm that you like
>  (arch/arm/plat-acorn/ ? ) and place a bit of code called
>  by an appropriate initcall. 

Thanks.

If it is as simple as you are suggesting, why wasn't it done _before_
breaking the RTC support?

After all, you know damned well that PCF8583 is used on ARM and you
know the config symbol for the platform, especially as you pointedly
remove the dependencies on CONFIG_ARCH_RPC from the driver inspite of
it being full of platform specifics (location of year byte in CMOS
and the checksum algorithm and checksum location.)

The only reason I can think is pure and simple spite.

Re: Fwd: PCF8583 not detected on RiscPC

[Jean Delvare]

Russell,

On Sun, 22 Feb 2009 08:28:29 +0000, Russell King - ARM Linux wrote:
> On Sun, Feb 22, 2009 at 01:19:36AM +0100, Alessandro Zummo wrote:
> > On Sat, 21 Feb 2009 20:41:47 +0000
> > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > 
> > > Confirmation: reverting 02bb584f3b1cfc8188522a4d2c8881b65073a4f1 and
> > > removing the reference to I2C_DRIVERID_PCF8583 results in the regression
> > > being fixed and normal behaviour being restored.
> > > 
> > > Patch below.
> > 
> >  this is a nack on my side. the whole i2c has been converted to the
> >  new style model. everyone is following and this is
> >  the wrong way to fix the issue.

+1

> While you may not desire the patch, the fact of the matter is that the
> conversion caused a regression.  Regressions trump forward progress and
> fixing stuff to work on new platforms.

I am sorry that we caused a regression. OTOH this change has been
publicly advertised several times, and the patch in question was
applied over 6 months ago...

> So, really, I'm not listening to NACKs from anyone for this.  The only
> thing I'll listen to is something _constructive_ to make it work again.
> I'm sure Andrew Morton will back me up on this.

... and now you say we should fix it in the day and then threaten us?
Come on.

> >  the device must be instantiated in the appropriate
> >  platform code. 
> > 
> >  for an example of what needs to be done, you can grep for
> >  i2c_board_info in arch/arm .
> > 
> >  if you need help, just tell me where's the acorn related
> >  platform setup code and I'll have a look.
> 
> drivers/i2c/busses/i2c-acorn.c
> 
> That's where the problem is - it's totally unclear how to convert
> this to the board_info method, because it doesn't live in the kernel
> as a platform thing.  Putting the board_info stuff into i2c-acorn
> just seems completely wrong.
>
> Now, we can either totally reorganize i2c-acorn, but that won't be
> acceptable for 2.6.29-rc.

It doesn't matter at all that i2c-acorn isn't a platform driver.
There's no need to reorganize anything. No need to sound so dramatic.

> The problem is that this *is* a regression, and therefore must be fixed
> in 2.6.29-rc.  As I see it, the only sane way to do that is to revert
> the conversion until a proper fix can be done.

You can't claim it is a regression that must be fixed in 2.6.29,
because it is already broken in 2.6.28, and even 2.6.27, and nobody
complained.

> So, please provide constructive suggestions on how to add boardinfo to
> this in a sane way, or we revert PCF8583 back to something which works.

Alessandro nicely proposed to solve your problem the right way if you
provided the required technical information, which, as far as I can
see, you didn't. I asked for hardware details and you didn't provide
them either.

So let me ask more precise questions, and hopefully we will get
somewhere.

1* How many different system types is i2c-acorn used on?

2* Do all these systems have a PCF8583 RTC chip?

3* At which address does the PCF8583 chip live on these systems? I
   guess 0x50.

4* Is there any acorn-specific piece of code which is run when the
   Linux kernel boots?

Depending on the answer to these questions, I can think of 3 different
nice ways to fix the regression. Reverting
02bb584f3b1cfc8188522a4d2c8881b65073a4f1 is not one of them, because 1*
it would potentially cause regressions on other systems and 2* it would
make the rtc-pcf8583 driver use an API which is marked as deprecated.

-- 
Jean Delvare

Re: Fwd: PCF8583 not detected on RiscPC

[Russell King - ARM Linux]

On Sun, Feb 22, 2009 at 10:52:05AM +0100, Jean Delvare wrote:
> Russell,
> 
> On Sun, 22 Feb 2009 08:28:29 +0000, Russell King - ARM Linux wrote:
> > So, really, I'm not listening to NACKs from anyone for this.  The only
> > thing I'll listen to is something _constructive_ to make it work again.
> > I'm sure Andrew Morton will back me up on this.
> 
> ... and now you say we should fix it in the day and then threaten us?
> Come on.

I'm serious.  It's a regression, it needs fixing.

> > The problem is that this *is* a regression, and therefore must be fixed
> > in 2.6.29-rc.  As I see it, the only sane way to do that is to revert
> > the conversion until a proper fix can be done.
> 
> You can't claim it is a regression that must be fixed in 2.6.29,
> because it is already broken in 2.6.28, and even 2.6.27, and nobody
> complained.

Sorry, our definitions of what's a regression are different, and you are
wrong.  As I've already said to you, it worked in 2.6.25-rc and it worked
last time I tested which was a 2.6.27-rc kernel.

If every single kernel has to be tested on every single machine to find
"regressions" in your terminology, that's all I'd be doing.  Get real.
It's not practical to do that.

So, yes, I'm going to continue claiming that this is a regression and
needs to be fixed.

> > So, please provide constructive suggestions on how to add boardinfo to
> > this in a sane way, or we revert PCF8583 back to something which works.
> 
> Alessandro nicely proposed to solve your problem the right way if you
> provided the required technical information, which, as far as I can
> see, you didn't. I asked for hardware details and you didn't provide
> them either.

I'm sorry, what hardware details are you wanting that I didn't give.
I've said it's the PCF8583 driver.  I've said that the bus driver is
i2c-acorn.c, even giving its location in the kernel tree.

It's a bit banged I2C bus.  It's on the main board.  It's connected to
expansion cards.  It uses 5V signalling with pull up resistors.

I don't think I've missed anything out.

> So let me ask more precise questions, and hopefully we will get
> somewhere.
> 
> 1* How many different system types is i2c-acorn used on?

It used to be used on four, two of which have been long since removed,
and one was removed in the last merge window.  So it's now only used on
one platform.

> 2* Do all these systems have a PCF8583 RTC chip?

Yes.

> 3* At which address does the PCF8583 chip live on these systems? I
>    guess 0x50.

Looking back in the history of the driver, yes, it's 0x50.

> 4* Is there any acorn-specific piece of code which is run when the
>    Linux kernel boots?

For the riscpc, arch/arm/mach-rpc

> Depending on the answer to these questions, I can think of 3 different
> nice ways to fix the regression. Reverting
> 02bb584f3b1cfc8188522a4d2c8881b65073a4f1 is not one of them,

Well, I've tried it, and unfortunately it doesn't work.  The result is
that with the board_info in place, the i2c-acorn bus seems to no longer
be registered as bus 0, but becomes bus 1.

> because 1*
> it would potentially cause regressions on other systems and 2* it would
> make the rtc-pcf8583 driver use an API which is marked as deprecated.

That's bullshit in light of the fact that this used to work, but then
a patch was merged which stopped it working.  That's a regression on
the main platform which PCF8583 was written for.  Plain and simple.

Re: Fwd: PCF8583 not detected on RiscPC

[Alessandro Zummo]

On Sun, 22 Feb 2009 09:51:50 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> >   That's very simple, even if acorn is a bit unstructured. You just
> >  need to choice a place under arch/arm that you like
> >  (arch/arm/plat-acorn/ ? ) and place a bit of code called
> >  by an appropriate initcall. 
> 
> Thanks.
> 
> If it is as simple as you are suggesting, why wasn't it done _before_
> breaking the RTC support?

 I guess because who did the modification didn't know
 it was used on acorn and nor did I.

 
> After all, you know damned well that PCF8583 is used on ARM and you
> know the config symbol for the platform, especially as you pointedly
> remove the dependencies on CONFIG_ARCH_RPC from the driver inspite of
 
 ????


> it being full of platform specifics (location of year byte in CMOS
> and the checksum algorithm and checksum location.)
> 
> The only reason I can think is pure and simple spite.

 what??? The driver has been introduced in 9c0c570576d02000063e28faadcce8c07396755d
 without any platform specific ifdef or depend in Kconfig and I never saw it before that
 commit. 

 If any change was ever proposed to make it platform specific (so that it would
 have worked only on a single platform) at any later time I'm pretty sure I
 wouldn't have acked it.

 It was described as:

 "A port of the driver for the pcf8583 i2c rtc controller to the generic RTC
 framework by Alessandro Zummo. Based on
 drivers/acorn/char/{pcf8583.[hc],i2c.c}. Hopefully, acorn can be converted
 too to use this driver in the future."

 and you were on the Cc list. I don't know who converted the acorn platform to
 use it and I can't care less. Who did should have checked the driver 
 for compatibility with his own platform before migrating from the driver under
 drivers/char/ .

 As far as I can see with git log, you applied changes to the driver
 multiple times, without having me in Cc nor G. Liakhovetski (who did the port).

 So, before accusing people, do you homework with a simple
 git log drivers/rtc/rtc-pcf8583.c 

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: Fwd: PCF8583 not detected on RiscPC

[Jean Delvare]

On Sun, 22 Feb 2009 10:22:17 +0000, Russell King - ARM Linux wrote:
> On Sun, Feb 22, 2009 at 10:52:05AM +0100, Jean Delvare wrote:
> > Russell,
> > 
> > On Sun, 22 Feb 2009 08:28:29 +0000, Russell King - ARM Linux wrote:
> > > So, really, I'm not listening to NACKs from anyone for this.  The only
> > > thing I'll listen to is something _constructive_ to make it work again.
> > > I'm sure Andrew Morton will back me up on this.
> > 
> > ... and now you say we should fix it in the day and then threaten us?
> > Come on.
> 
> I'm serious.  It's a regression, it needs fixing.
> 
> > > The problem is that this *is* a regression, and therefore must be fixed
> > > in 2.6.29-rc.  As I see it, the only sane way to do that is to revert
> > > the conversion until a proper fix can be done.
> > 
> > You can't claim it is a regression that must be fixed in 2.6.29,
> > because it is already broken in 2.6.28, and even 2.6.27, and nobody
> > complained.
> 
> Sorry, our definitions of what's a regression are different, and you are
> wrong.  As I've already said to you, it worked in 2.6.25-rc and it worked
> last time I tested which was a 2.6.27-rc kernel.

Maybe I'm wrong but at least I am polite.

And I pretty much doubt it worked with a 2.6.27-rc kernel, given that
the patch which broke it went into 2.6.27-rc1. So you are just as wrong
as I am.

> If every single kernel has to be tested on every single machine to find
> "regressions" in your terminology, that's all I'd be doing.  Get real.
> It's not practical to do that.
> 
> So, yes, I'm going to continue claiming that this is a regression and
> needs to be fixed.
> 
> > > So, please provide constructive suggestions on how to add boardinfo to
> > > this in a sane way, or we revert PCF8583 back to something which works.
> > 
> > Alessandro nicely proposed to solve your problem the right way if you
> > provided the required technical information, which, as far as I can
> > see, you didn't. I asked for hardware details and you didn't provide
> > them either.
> 
> I'm sorry, what hardware details are you wanting that I didn't give.
> I've said it's the PCF8583 driver.  I've said that the bus driver is
> i2c-acorn.c, even giving its location in the kernel tree.
> 
> It's a bit banged I2C bus.  It's on the main board.  It's connected to
> expansion cards.  It uses 5V signalling with pull up resistors.
> 
> I don't think I've missed anything out.
> 
> > So let me ask more precise questions, and hopefully we will get
> > somewhere.
> > 
> > 1* How many different system types is i2c-acorn used on?
> 
> It used to be used on four, two of which have been long since removed,
> and one was removed in the last merge window.  So it's now only used on
> one platform.
> 
> > 2* Do all these systems have a PCF8583 RTC chip?
> 
> Yes.
> 
> > 3* At which address does the PCF8583 chip live on these systems? I
> >    guess 0x50.
> 
> Looking back in the history of the driver, yes, it's 0x50.
> 
> > 4* Is there any acorn-specific piece of code which is run when the
> >    Linux kernel boots?
> 
> For the riscpc, arch/arm/mach-rpc
> 
> > Depending on the answer to these questions, I can think of 3 different
> > nice ways to fix the regression. Reverting
> > 02bb584f3b1cfc8188522a4d2c8881b65073a4f1 is not one of them,
> 
> Well, I've tried it, and unfortunately it doesn't work.  The result is
> that with the board_info in place, the i2c-acorn bus seems to no longer
> be registered as bus 0, but becomes bus 1.

The following patch should fix it:

---
 drivers/i2c/busses/i2c-acorn.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- linux-2.6.29-rc5.orig/drivers/i2c/busses/i2c-acorn.c	2009-02-21 14:33:57.000000000 +0100
+++ linux-2.6.29-rc5/drivers/i2c/busses/i2c-acorn.c	2009-02-22 11:33:10.000000000 +0100
@@ -83,6 +83,7 @@ static struct i2c_algo_bit_data ioc_data
 };
 
 static struct i2c_adapter ioc_ops = {
+	.nr			= 0,
 	.algo_data		= &ioc_data,
 };
 
@@ -90,7 +91,7 @@ static int __init i2c_ioc_init(void)
 {
 	force_ones = FORCE_ONES | SCL | SDA;
 
-	return i2c_bit_add_bus(&ioc_ops);
+	return i2c_bit_add_numbered_bus(&ioc_ops);
 }
 
 module_init(i2c_ioc_init);

> > because 1*
> > it would potentially cause regressions on other systems and 2* it would
> > make the rtc-pcf8583 driver use an API which is marked as deprecated.
> 
> That's bullshit in light of the fact that this used to work, but then
> a patch was merged which stopped it working.  That's a regression on
> the main platform which PCF8583 was written for.  Plain and simple.


-- 
Jean Delvare

Re: Fwd: PCF8583 not detected on RiscPC

[Russell King - ARM Linux]

On Sun, Feb 22, 2009 at 11:35:44AM +0100, Alessandro Zummo wrote:
> On Sun, 22 Feb 2009 09:51:50 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > >   That's very simple, even if acorn is a bit unstructured. You just
> > >  need to choice a place under arch/arm that you like
> > >  (arch/arm/plat-acorn/ ? ) and place a bit of code called
> > >  by an appropriate initcall. 
> > 
> > Thanks.
> > 
> > If it is as simple as you are suggesting, why wasn't it done _before_
> > breaking the RTC support?
> 
>  I guess because who did the modification didn't know
>  it was used on acorn and nor did I.
> 
>  
> > After all, you know damned well that PCF8583 is used on ARM and you
> > know the config symbol for the platform, especially as you pointedly
> > remove the dependencies on CONFIG_ARCH_RPC from the driver inspite of
>  
>  ????

I refer you to bb71f99f8daefb4a2c2441298bc127aaff9af947 and the
discussion resulting from that commit, and changed in your commit
09a21e56dc3767ce444e21c1383d587b261af13c.

>  I don't know who converted the acorn platform to
>  use it and I can't care less. Who did should have checked the driver 
>  for compatibility with his own platform before migrating from the driver under
>  drivers/char/ .

Which bit of "it used to work" did you miss?  At the time of converesion,
it was checked and after some initial trivial bug fixing and it was
working, and continued to work up until this recent breakage.

>  As far as I can see with git log, you applied changes to the driver
>  multiple times, without having me in Cc nor G. Liakhovetski (who did
>  the port).

I don't add CC entries to commits, so you can't make that assumption.
However, I did talk to Guennadi around the time of those changes about
some of the issues therein, in particular adding back the I2C address of
the PCF8583.

Most of those other changes were trivial bug fixes, and I do apologise
for not copying you with those.

Re: Fwd: PCF8583 not detected on RiscPC

[Russell King - ARM Linux]

On Sun, Feb 22, 2009 at 11:40:00AM +0100, Jean Delvare wrote:
> On Sun, 22 Feb 2009 10:22:17 +0000, Russell King - ARM Linux wrote:
> > On Sun, Feb 22, 2009 at 10:52:05AM +0100, Jean Delvare wrote:
> > > Russell,
> > > 
> > > On Sun, 22 Feb 2009 08:28:29 +0000, Russell King - ARM Linux wrote:
> > > > So, really, I'm not listening to NACKs from anyone for this.  The only
> > > > thing I'll listen to is something _constructive_ to make it work again.
> > > > I'm sure Andrew Morton will back me up on this.
> > > 
> > > ... and now you say we should fix it in the day and then threaten us?
> > > Come on.
> > 
> > I'm serious.  It's a regression, it needs fixing.
> > 
> > > > The problem is that this *is* a regression, and therefore must be fixed
> > > > in 2.6.29-rc.  As I see it, the only sane way to do that is to revert
> > > > the conversion until a proper fix can be done.
> > > 
> > > You can't claim it is a regression that must be fixed in 2.6.29,
> > > because it is already broken in 2.6.28, and even 2.6.27, and nobody
> > > complained.
> > 
> > Sorry, our definitions of what's a regression are different, and you are
> > wrong.  As I've already said to you, it worked in 2.6.25-rc and it worked
> > last time I tested which was a 2.6.27-rc kernel.
> 
> Maybe I'm wrong but at least I am polite.
> 
> And I pretty much doubt it worked with a 2.6.27-rc kernel, given that
> the patch which broke it went into 2.6.27-rc1. So you are just as wrong
> as I am.
> 
> > If every single kernel has to be tested on every single machine to find
> > "regressions" in your terminology, that's all I'd be doing.  Get real.
> > It's not practical to do that.
> > 
> > So, yes, I'm going to continue claiming that this is a regression and
> > needs to be fixed.
> > 
> > > > So, please provide constructive suggestions on how to add boardinfo to
> > > > this in a sane way, or we revert PCF8583 back to something which works.
> > > 
> > > Alessandro nicely proposed to solve your problem the right way if you
> > > provided the required technical information, which, as far as I can
> > > see, you didn't. I asked for hardware details and you didn't provide
> > > them either.
> > 
> > I'm sorry, what hardware details are you wanting that I didn't give.
> > I've said it's the PCF8583 driver.  I've said that the bus driver is
> > i2c-acorn.c, even giving its location in the kernel tree.
> > 
> > It's a bit banged I2C bus.  It's on the main board.  It's connected to
> > expansion cards.  It uses 5V signalling with pull up resistors.
> > 
> > I don't think I've missed anything out.
> > 
> > > So let me ask more precise questions, and hopefully we will get
> > > somewhere.
> > > 
> > > 1* How many different system types is i2c-acorn used on?
> > 
> > It used to be used on four, two of which have been long since removed,
> > and one was removed in the last merge window.  So it's now only used on
> > one platform.
> > 
> > > 2* Do all these systems have a PCF8583 RTC chip?
> > 
> > Yes.
> > 
> > > 3* At which address does the PCF8583 chip live on these systems? I
> > >    guess 0x50.
> > 
> > Looking back in the history of the driver, yes, it's 0x50.
> > 
> > > 4* Is there any acorn-specific piece of code which is run when the
> > >    Linux kernel boots?
> > 
> > For the riscpc, arch/arm/mach-rpc
> > 
> > > Depending on the answer to these questions, I can think of 3 different
> > > nice ways to fix the regression. Reverting
> > > 02bb584f3b1cfc8188522a4d2c8881b65073a4f1 is not one of them,
> > 
> > Well, I've tried it, and unfortunately it doesn't work.  The result is
> > that with the board_info in place, the i2c-acorn bus seems to no longer
> > be registered as bus 0, but becomes bus 1.
> 
> The following patch should fix it:
> 
> ---
>  drivers/i2c/busses/i2c-acorn.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- linux-2.6.29-rc5.orig/drivers/i2c/busses/i2c-acorn.c	2009-02-21 14:33:57.000000000 +0100
> +++ linux-2.6.29-rc5/drivers/i2c/busses/i2c-acorn.c	2009-02-22 11:33:10.000000000 +0100
> @@ -83,6 +83,7 @@ static struct i2c_algo_bit_data ioc_data
>  };
>  
>  static struct i2c_adapter ioc_ops = {
> +	.nr			= 0,
>  	.algo_data		= &ioc_data,
>  };
>  
> @@ -90,7 +91,7 @@ static int __init i2c_ioc_init(void)
>  {
>  	force_ones = FORCE_ONES | SCL | SDA;
>  
> -	return i2c_bit_add_bus(&ioc_ops);
> +	return i2c_bit_add_numbered_bus(&ioc_ops);
>  }
>  
>  module_init(i2c_ioc_init);
> 

Thanks, that does fix it.  Here's the addition of the boardinfo.  I don't
have any issue with combining this with your patch above.  Please apply
for the current -rc series.

Subject: [ARM] Add i2c_board_info for RiscPC PCF8583

Add the necessary i2c_board_info structure to fix the lack of PCF8583
RTC on RiscPC.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

---
 arch/arm/mach-rpc/riscpc.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/mach-rpc/riscpc.c b/arch/arm/mach-rpc/riscpc.c
index e88d417..c7fc01e 100644
--- a/arch/arm/mach-rpc/riscpc.c
+++ b/arch/arm/mach-rpc/riscpc.c
@@ -19,6 +19,7 @@
 #include <linux/serial_8250.h>
 #include <linux/ata_platform.h>
 #include <linux/io.h>
+#include <linux/i2c.h>
 
 #include <asm/elf.h>
 #include <asm/mach-types.h>
@@ -201,8 +202,13 @@ static struct platform_device *devs[] __initdata = {
 	&pata_device,
 };
 
+static struct i2c_board_info i2c_rtc = {
+	I2C_BOARD_INFO("pcf8583", 0x50)
+};
+
 static int __init rpc_init(void)
 {
+	i2c_register_board_info(0, &i2c_rtc, 1);
 	return platform_add_devices(devs, ARRAY_SIZE(devs));
 }
 

Re: Fwd: PCF8583 not detected on RiscPC

[Alessandro Zummo]

On Sun, 22 Feb 2009 11:26:54 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:


> I refer you to bb71f99f8daefb4a2c2441298bc127aaff9af947 and the
> discussion resulting from that commit, and changed in your commit
> 09a21e56dc3767ce444e21c1383d587b261af13c.

 As I said, the driver was born without the additional
 depend. 

 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=9c0c570576d02000063e28faadcce8c07396755d

 You then added it in

 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=bb71f99f8daefb4a2c2441298bc127aaff9af947
 
 crating an arch specific driver from a driver that
 was mostly generic. I wasn't in Cc and I didn't noticed at that time.

  If you had it sent to me I would have told you that it
 was plain wrong. 

 When I did the Kconfig cleanup I noticed an additional
 dependency and removed it. This was not done because I don't
 like acorn, the arm stuff (to which I have too contributed)
 or anyone else.


> >  I don't know who converted the acorn platform to
> >  use it and I can't care less. Who did should have checked the driver 
> >  for compatibility with his own platform before migrating from the driver under
> >  drivers/char/ .
> 
> Which bit of "it used to work" did you miss?  At the time of converesion,
> it was checked and after some initial trivial bug fixing and it was
> working, and continued to work up until this recent breakage.

 I was discussing about the non-platform-specific nature of the driver.

 The breakage happened because the whole i2c stack has been revamped.
 It has been discussed for a long time and we have spent much effort
 to ensure that everything was correct. However we can all fail.
 I have checked all the files in the arch directory searching for
 i2c users. acorn was not there and I missed it.

 That said, the kernel is evolving continuously and once you reported
 the problem Jean and I actively worked to fix it.

 I'm not stating that we had not a problem or that the there wasn't
 a breakage. Only that the fault is not in the driver itself but
 on how the platform uses it. And that's why I refuse a revert-everything
 approach.

 If a better communication approach had been established in the past
 this probably wouldn't have happened.


> >  As far as I can see with git log, you applied changes to the driver
> >  multiple times, without having me in Cc nor G. Liakhovetski (who did
> >  the port).
> 
> I don't add CC entries to commits, so you can't make that assumption.

 Please check your archive, but I'm pretty sure I would have noticed
 an email with a subject. I don't see it in my archives nor in the 
 mailing list.

 [ARM] rtc-pcf8583: Final fixes for this RTC on RiscPC

 If you did sent it to me or to the list and I hadn't noticed, I beg your pardon.

> However, I did talk to Guennadi around the time of those changes about
> some of the issues therein, in particular adding back the I2C address of
> the PCF8583.

 So you and Guennadi knew but not everyone else.
 
> Most of those other changes were trivial bug fixes, and I do apologise
> for not copying you with those.

 I accept the apologies but you don't need to copy me for those, we
 have trivial@ for those, which usually Ccs me. And there's Andrew who
 kindly gives a look at everything.

 But when things are important I do appreciate to see it at the proper time.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it