rtc-ds1307 platform update, powerpc mpc834x_itx and mpc8313_rdb

PATCH In review David Brownell 5 participants 10 messages May 14, 2007 – May 15, 2007

Subsystems (4)

PowerPC I2C subsystem host drivers Linux for PowerPC embedded PPC8XX and PPC83XX Freescale mpc I2C driver

lore

Share & tools

lore

David Brownell

author

2007-05-14 17:15 UTC #0 lore raw

PATCH 4/4 rtc-ds1307 platform update, powerpc mpc834x_itx and mpc8313_rdb

Update powerpc platforms to support new-style rtc-ds1307 driver:

- Make i2c-mpc driver use numbered adapter;
- Update boards identified by defconfig to list a ds1339 device:
* mpc834x_itx
* mpc8313_rdb

Didn't update the pasemi reference board; public info doesn't say which chip it uses.

UNTESTED

Signed-off-by David Brownell <[email protected]>

---
NOTE: cc'd the relevant powerpc maintainers lists; patches 1-3 will be archived in the rtc and i2c lists. I'd like to see these get tested/fixed so that the rtc-ds1307 driver can swich completely to use the new-style I2c driver model.

 arch/powerpc/platforms/83xx/mpc8313_rdb.c |   11 +++++++++++
 arch/powerpc/platforms/83xx/mpc834x_itx.c |   12 ++++++++++++
 drivers/i2c/busses/i2c-mpc.c              |    3 ++-
 3 files changed, 25 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-mpc.c +2 -1

@@ -280,6 +280,7 @@ static struct i2c_adapter mpc_ops = {
 	.owner = THIS_MODULE,
 	.name = "MPC adapter",
 	.id = I2C_HW_MPC107,
+	/* .nr = 0, */
 	.algo = &mpc_algo,
 	.class = I2C_CLASS_HWMON,
 	.timeout = 1,

@@ -329,7 +330,7 @@ static int fsl_i2c_probe(struct platform
 	i2c->adap = mpc_ops;
 	i2c_set_adapdata(&i2c->adap, i2c);
 	i2c->adap.dev.parent = &pdev->dev;
-	if ((result = i2c_add_adapter(&i2c->adap)) < 0) {
+	if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
 		printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
 		goto fail_add;
 	}

arch/powerpc/platforms/83xx/mpc834x_itx.c +12 -0

@@ -23,6 +23,7 @@
 #include <linux/delay.h>
 #include <linux/seq_file.h>
 #include <linux/root_dev.h>
+#include <linux/i2c.h>
 
 #include <asm/system.h>
 #include <asm/atomic.h>

@@ -48,6 +49,15 @@ unsigned long isa_mem_base = 0;
  * Setup the architecture
  *
  */
+
+static struct i2c_board_info __initdata mpc834x_itx_i2c_devices[] = {
+	{ I2C_BOARD_INFO("rtc-ds1307", 0x68),
+	  .type = "ds1339",
+	  /* REVISIT .irq = IRQ2 ... this RTC has an alarm */
+	},
+	/* REVISIT: also has an eeprom */
+};
+
 static void __init mpc834x_itx_setup_arch(void)
 {
 #ifdef CONFIG_PCI

@@ -63,6 +73,8 @@ static void __init mpc834x_itx_setup_arc
 
 	ppc_md.pci_exclude_device = mpc83xx_exclude_device;
 #endif
+	i2c_register_board_info(0, mpc834x_itx_i2c_devices,
+			ARRAY_SIZE(mpc834x_itx_i2c_devices));
 }
 
 static void __init mpc834x_itx_init_IRQ(void)

arch/powerpc/platforms/83xx/mpc8313_rdb.c +11 -0

@@ -14,6 +14,7 @@
  */
 
 #include <linux/pci.h>
+#include <linux/i2c.h>
 
 #include <asm/time.h>
 #include <asm/ipic.h>

@@ -38,6 +39,14 @@ unsigned long isa_mem_base = 0;
  * Setup the architecture
  *
  */
+static struct i2c_board_info __initdata mpc8313_rdb_i2c_devices[] = {
+	{ I2C_BOARD_INFO("rtc-ds1307", 0x68),
+	  .type = "ds1339",
+	  /* REVISIT .irq = IRQ4 ... this RTC has an alarm */
+	},
+	/* REVISIT: also has an eeprom and lm75 */
+};
+
 static void __init mpc8313_rdb_setup_arch(void)
 {
 #ifdef CONFIG_PCI

@@ -53,6 +62,8 @@ static void __init mpc8313_rdb_setup_arc
 
 	ppc_md.pci_exclude_device = mpc83xx_exclude_device;
 #endif
+	i2c_register_board_info(0, mpc8313_i2c_devices,
+			ARRAY_SIZE(mpc8313_i2c_devices));
 }
 
 void __init mpc8313_rdb_init_IRQ(void)

Grant Likely

2007-05-14 17:44 UTC #1 lore raw

Quoted 35 lines · David Brownell

Update powerpc platforms to support new-style rtc-ds1307 driver:
- Make i2c-mpc driver use numbered adapter;
- Update boards identified by defconfig to list a ds1339 device:
* mpc834x_itx
* mpc8313_rdb
Didn't update the pasemi reference board; public info doesn't say which chip it uses.
UNTESTED
Signed-off-by David Brownell <[email protected]>
---
NOTE: cc'd the relevant powerpc maintainers lists; patches 1-3 will be archived in the rtc and i2c lists. I'd like to see these get tested/fixed so that the rtc-ds1307 driver can swich completely to use the new-style I2c driver model.
 arch/powerpc/platforms/83xx/mpc8313_rdb.c |   11 +++++++++++
 arch/powerpc/platforms/83xx/mpc834x_itx.c |   12 ++++++++++++
 drivers/i2c/busses/i2c-mpc.c              |    3 ++-
 3 files changed, 25 insertions(+), 1 deletion(-)
drivers/i2c/busses/i2c-mpc.c 2007-05-13 10:56:52.000000000 -0700 renamed from drivers/i2c/busses/i2c-mpc.c 2007-05-10 00:48:22.000000000 -0700 +1 -0
@@ -280,6 +280,7 @@ static struct i2c_adapter mpc_ops = {
        .owner = THIS_MODULE,
        .name = "MPC adapter",
        .id = I2C_HW_MPC107,
+       /* .nr = 0, */
        .algo = &mpc_algo,
        .class = I2C_CLASS_HWMON,
        .timeout = 1,

drivers/i2c/busses/i2c-mpc.c 2007-05-13 10:56:52.000000000 -0700 static int fsl_i2c_probe(struct platform · line 336

Quoted 8 lines

drivers/i2c/busses/i2c-mpc.c 2007-05-13 10:56:52.000000000 -0700 renamed from drivers/i2c/busses/i2c-mpc.c 2007-05-10 00:48:22.000000000 -0700 +1 -1

@@ -329,7 +330,7 @@ static int fsl_i2c_probe(struct platform
        i2c->adap = mpc_ops;
        i2c_set_adapdata(&i2c->adap, i2c);
        i2c->adap.dev.parent = &pdev->dev;
-       if ((result = i2c_add_adapter(&i2c->adap)) < 0) {
+       if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
                printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
                goto fail_add;
        }

I think this will break many boards as more than one i2c adapter is common on these chips. If I read the code correctly, the first adapter will be registered with adap->nr == 0. The second adapter will also be registered with adap->nr == 0; but i2c_add_numbered_adapter will return -EBUSY because the id is already used.

I think you need to add this line before the call to i2c_add_numbered_adapter:

i2c->adap.nr = pdev->id;

Cheers,
g.

Signature · 4 lines

Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
[email protected]
(403) 399-0195

David Brownell

author

2007-05-14 18:26 UTC #2 lore raw

Quoted 14 lines · Grant Likely

Quoted 14 lines

drivers/i2c/busses/i2c-mpc.c 2007-05-13 10:56:52.000000000 -0700 renamed from drivers/i2c/busses/i2c-mpc.c 2007-05-10 00:48:22.000000000 -0700 +1 -0

@@ -280,6 +280,7 @@ static struct i2c_adapter mpc_ops = {
        .owner = THIS_MODULE,
        .name = "MPC adapter",
        .id = I2C_HW_MPC107,
+       /* .nr = 0, */
        .algo = &mpc_algo,
        .class = I2C_CLASS_HWMON,
        .timeout = 1,

@@ -329,7 +330,7 @@ static int fsl_i2c_probe(struct platform
        i2c->adap = mpc_ops;
        i2c_set_adapdata(&i2c->adap, i2c);
        i2c->adap.dev.parent = &pdev->dev;

Quoted 8 lines

drivers/i2c/busses/i2c-mpc.c 2007-05-13 10:56:52.000000000 -0700 static int fsl_i2c_probe(struct platform · line 336
Quoted 5 lines
drivers/i2c/busses/i2c-mpc.c 2007-05-13 10:56:52.000000000 -0700 renamed from drivers/i2c/busses/i2c-mpc.c 2007-05-10 00:48:22.000000000 -0700 +1 -1
-       if ((result = i2c_add_adapter(&i2c->adap)) < 0) {
+       if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
                printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
                goto fail_add;
        }
I think this will break many boards as more than one i2c adapter is common on these chips.

It didn't look like that from my quick glance at the sources, but I could have been deceived. I certainly don't know powerpc hardware.

Quoted 9 lines

If I read the code correctly, the first
adapter will be registered with adap->nr == 0. The second adapter will also be registered with adap->nr == 0; but i2c_add_numbered_adapter will return -EBUSY because the id is already used.
I think you need to add this line before the call to i2c_add_numbered_adapter:
i2c->adap.nr = pdev->id;

If these chips do have multiple instances of that adapter logic, that would be an appropriate change. The only "gotcha" might be that some platforms use the "pdev->id = -1" trick when there's only a single instance ... that would probably map to zero.

I'll hope that whoever makes all this work will resolve those particular questions ...

- Dave

David Brownell

author

2007-05-14 18:49 UTC #3 lore raw

Quoted 17 lines

Quoted 13 lines

Quoted 10 lines

drivers/i2c/busses/i2c-mpc.c +1 -1

@@ -329,7 +330,7 @@ static int fsl_i2c_probe(struct platform
        i2c->adap = mpc_ops;
        i2c_set_adapdata(&i2c->adap, i2c);
        i2c->adap.dev.parent = &pdev->dev;
-       if ((result = i2c_add_adapter(&i2c->adap)) < 0) {
+       if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
                printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
                goto fail_add;
        }

...

Quoted 3 lines

I think you need to add this line before the call to i2c_add_numbered_adapter:
i2c->adap.nr = pdev->id;

Quoted 7 lines

If these chips do have multiple instances of that adapter logic, that would be an appropriate change. The only "gotcha" might be that some platforms use the "pdev->id = -1" trick when there's only a single instance ... that would probably map to zero.
I'll hope that whoever makes all this work will resolve those particular questions ...

Oh, and by the way: patches updating I2C adapter drivers to work with new-style drivers look to be OK to get into 2.6.22 with just an Ack from Jean, via the MM tree. (Right, Jean?)

In this case I'll hope someone else does the real submission, after testing. I just wanted to nudge the process along.

- Dave

Grant Likely

2007-05-14 19:02 UTC #4 lore raw

Quoted 25 lines · David Brownell

Quoted 17 lines
Quoted 13 lines
Quoted 10 lines
drivers/i2c/busses/i2c-mpc.c +1 -1
@@ -329,7 +330,7 @@ static int fsl_i2c_probe(struct platform
        i2c->adap = mpc_ops;
        i2c_set_adapdata(&i2c->adap, i2c);
        i2c->adap.dev.parent = &pdev->dev;
-       if ((result = i2c_add_adapter(&i2c->adap)) < 0) {
+       if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
                printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
                goto fail_add;
        }
...
Quoted 3 lines
I think you need to add this line before the call to i2c_add_numbered_adapter:
i2c->adap.nr = pdev->id;
Quoted 7 lines
If these chips do have multiple instances of that adapter logic, that would be an appropriate change. The only "gotcha" might be that some platforms use the "pdev->id = -1" trick when there's only a single instance ... that would probably map to zero.
I'll hope that whoever makes all this work will resolve those particular questions ...

Quoted 6 lines

Oh, and by the way: patches updating I2C adapter drivers to work with new-style drivers look to be OK to get into 2.6.22 with just an Ack from Jean, via the MM tree. (Right, Jean?)
In this case I'll hope someone else does the real submission, after testing. I just wanted to nudge the process along

I'll submit this one (at least the i2c-mpc.c bits). It's already working on my mpc5200 board. I don't have any of the other boards, so those fixups should come from others.

Cheers,
g.

Signature · 4 lines

Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
[email protected]
(403) 399-0195

Jean Delvare

2007-05-15 13:23 UTC #5 lore raw

In reply to David Brownell

Quoted 22 lines · David Brownell

Quoted 22 lines
Quoted 13 lines
Quoted 10 lines
drivers/i2c/busses/i2c-mpc.c +1 -1
@@ -329,7 +330,7 @@ static int fsl_i2c_probe(struct platform
        i2c->adap = mpc_ops;
        i2c_set_adapdata(&i2c->adap, i2c);
        i2c->adap.dev.parent = &pdev->dev;
-       if ((result = i2c_add_adapter(&i2c->adap)) < 0) {
+       if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
                printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
                goto fail_add;
        }
...
Quoted 3 lines
I think you need to add this line before the call to i2c_add_numbered_adapter:
i2c->adap.nr = pdev->id;
If these chips do have multiple instances of that adapter logic, that would be an appropriate change. The only "gotcha" might be that some platforms use the "pdev->id = -1" trick when there's only a single instance ... that would probably map to zero.

Quoted 6 lines

Quoted 2 lines
I'll hope that whoever makes all this work will resolve those particular questions ...
Oh, and by the way: patches updating I2C adapter drivers to work with new-style drivers look to be OK to get into 2.6.22 with just an Ack from Jean, via the MM tree. (Right, Jean?)

Correct. I will happily ack such patches, but I won't be pushing them upstream - unless waiting until 2.6.23 is OK for you.

Signature · 1 line

Jean Delvare

Olof Johansson

2007-05-14 18:02 UTC #6 lore raw

In reply to David Brownell

Quoted 9 lines · David Brownell

Update powerpc platforms to support new-style rtc-ds1307 driver:
- Make i2c-mpc driver use numbered adapter;
- Update boards identified by defconfig to list a ds1339 device:
* mpc834x_itx
* mpc8313_rdb
Didn't update the pasemi reference board; public info doesn't say which chip it uses.

ds1338.

Quoted 4 lines

NOTE: cc'd the relevant powerpc maintainers lists; patches 1-3 will be archived in the rtc and i2c lists. I'd like to see these get tested/fixed so that the rtc-ds1307 driver can swich completely to use the new-style I2c driver model.

Specific URLs can sometimes be useful, so we don't have to go digging through the archives. But thanks for the heads up, I'll make the corresponding changes to my code and test it out.

(I'm guessing long-term, we will want to pull the chip info out of the device tree instad of have it in a table in the board code, but that's extra credit at the moment.)

-Olof

David Brownell

author

2007-05-14 18:37 UTC #7 lore raw

Quoted 6 lines · Olof Johansson

Quoted 2 lines · David Brownell
Didn't update the pasemi reference board; public info doesn't say which chip it uses.
ds1338.

OK. The relevant updates should be evident from this patch.

Mr Grep tells me that if those powerpc platforms update along with csb337, all current rtc-ds1307 users will be upgraded.

Then there's a separate issue of removing the old SENSORS_DS1337 driver. Simpler IMO for those drivers to have one transition, to new-style rtc-ds1307, not two (to old-style, then new-style).

Quoted 8 lines

Quoted 4 lines
NOTE: cc'd the relevant powerpc maintainers lists; patches 1-3 will be archived in the rtc and i2c lists. I'd like to see these get tested/fixed so that the rtc-ds1307 driver can switch completely to use the new-style I2c driver model.
Specific URLs can sometimes be useful, so we don't have to go digging through the archives. But thanks for the heads up, I'll make the corresponding changes to my code and test it out.

The URLS don't exist until after the email's gotten to the list servers ... sort of a chicken/egg issue.

Quoted 3 lines

(I'm guessing long-term, we will want to pull the chip info out of the device tree instad of have it in a table in the board code, but that's extra credit at the moment.)

For some definition of "long term". ISTR seeing patches from Scott Wood doing exactly that ... on the I2C list last November or so. ;)

Right now my concern is how to migrate the rtc-ds1307 driver. I'd prefer to avoid needing to have a period where the legacy mode must co-exist with the new-style driver support, or needing to entangle with such tree conversions.

- Dave

Alessandro Zummo

2007-05-14 19:02 UTC #8 lore raw

Quoted 7 lines · On Mon, 14 May 2007 11:37:39 -0700 David Brownell <[email protected]> wrote:

Quoted 4 lines
(I'm guessing long-term, we will want to pull the chip info out of the device tree instad of have it in a table in the board code, but that's extra credit at the moment.)
For some definition of "long term". ISTR seeing patches from Scott Wood doing exactly that ... on the I2C list last November or so. ;)

I saw them too. pretty nice set of patches. I would agree
on them fully.

Signature · 6 lines

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Olof Johansson

2007-05-14 19:27 UTC #9 lore raw

In reply to David Brownell

Quoted 8 lines · David Brownell

Quoted 6 lines · Olof Johansson
Quoted 2 lines · David Brownell
Didn't update the pasemi reference board; public info doesn't say which chip it uses.
ds1338.

Quoted 8 lines

OK. The relevant updates should be evident from this patch.
Mr Grep tells me that if those powerpc platforms update along with csb337, all current rtc-ds1307 users will be upgraded.
Then there's a separate issue of removing the old SENSORS_DS1337 driver. Simpler IMO for those drivers to have one transition, to new-style rtc-ds1307, not two (to old-style, then new-style).

Sounds good.

Quoted 11 lines

Quoted 8 lines
Quoted 4 lines
NOTE: cc'd the relevant powerpc maintainers lists; patches 1-3 will be archived in the rtc and i2c lists. I'd like to see these get tested/fixed so that the rtc-ds1307 driver can switch completely to use the new-style I2c driver model.
Specific URLs can sometimes be useful, so we don't have to go digging through the archives. But thanks for the heads up, I'll make the corresponding changes to my code and test it out.
The URLS don't exist until after the email's gotten to the list servers ... sort of a chicken/egg issue.

Ah yes, good point.

Quoted 6 lines

Quoted 3 lines
(I'm guessing long-term, we will want to pull the chip info out of the device tree instad of have it in a table in the board code, but that's extra credit at the moment.)
For some definition of "long term". ISTR seeing patches from Scott Wood doing exactly that ... on the I2C list last November or so. ;)

Yep, I asked on IRC and got a similar answer. I had forgotten about them.

Quoted 4 lines

Right now my concern is how to migrate the rtc-ds1307 driver. I'd prefer to avoid needing to have a period where the legacy mode must co-exist with the new-style driver support, or needing to entangle with such tree conversions.

Which is why I said "long term", as in "after this is done". :)

-Olof