Home » Mailing list archives » linux-kernel » 2008 » March » 25

Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers

Previous thread: [PATCH 6/6] [NETNS]: Lookup in FIB semantic hashes taking into account the namespace. by Denis V. Lunev on Thursday, January 31, 2008 - 5:00 am. (1 message)

Next thread: Why do I have a /sys/devices/system/cpu/cpuidle under kernel 2.6.24 with a Pentium M processor ? by Toralf on Thursday, January 31, 2008 - 6:00 am. (3 messages)
[view original message]
From: Jochen Friedrich
Subject: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers
Date: Thursday, January 31, 2008 - 5:54 am

Using the port of 2.4 code from Vitaly Bordug <vitb@kernel.crashing.org>
and the actual algorithm used by the i2c driver of the DBox code on
cvs.tuxboc.org from Tmbinc, Gillem (htoa@gmx.net). Renamed i2c-rpx.c and
i2c-algo-8xx.c to i2c-cpm.c and converted the driver to an
of_platform_driver.

Signed-off-by: Jochen Friedrich <jochen@scram.de>
---
 drivers/i2c/busses/Kconfig   |   10 +
 drivers/i2c/busses/Makefile  |    1 +
 drivers/i2c/busses/i2c-cpm.c |  759 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 770 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-cpm.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index b61f56b..1d18db7 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -114,6 +114,16 @@ config I2C_BLACKFIN_TWI_CLK_KHZ
 	help
 	  The unit of the TWI clock is kHz.
 
+config I2C_CPM
+	tristate "Freescale CPM1 or CPM2 (MPC8xx/826x)"
+	depends on (CPM1 || CPM2) && I2C && PPC_OF
+	help
+	  This supports the use of the I2C interface on Freescale
+	  processors with CPM1 or CPM2.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-cpm.
+
 config I2C_DAVINCI
 	tristate "DaVinci I2C driver"
 	depends on ARCH_DAVINCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index ea7068f..1291e2b 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_I2C_AMD8111)	+= i2c-amd8111.o
 obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
+obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
 obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
 obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
 obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
new file mode 100644
index 0000000..7191427
--- /dev/null
+++ b/drivers/i2c/busses/i2c-cpm.c
@@ -0,0 +1,759 @@
+/*
+ * Freescale ...
[ message continues ]
[view original message]
From: Jean Delvare
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers
Date: Thursday, February 21, 2008 - 5:05 am

Hallo Jochen,


The dependency on I2C is now handled at menu level, you no longer need

Maybe this block comment can be removed entirely? It adds more



Please group all the copyright statements together, having some before

It's not obvious to me why you need to include <linux/sched.h>, please



"I2C CPM" isn't very descriptive. Best would be to list the structure


Nack. New-style i2c drivers don't require this probing mechanism,
that's what you want to use if your I2C master doesn't support
zero-byte messages. Well, that's most probably what you want to use on
embedded platforms anyway.

So, please no hack for zero-byte messages. If they aren't supported by

Are there known I2C chips on this platform which need this flag? Its
implementation is purely optional, only a few broken I2C chips need it,

Many of the dev_dbg() calls in the function above could be replaced


Actually I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK)

Not defined anywhere, so this won't build. This ID is optional, so if
you don't need it, just don't set it. With the advent of new-style i2c








Better give more explicit names to your labels (for example



-- 
Jean Delvare
--


[ message continues ]
[view original message]
From: Jochen Friedrich
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers
Date: Friday, February 22, 2008 - 4:16 am

I know you don't like the patch from Jon Smirl and you also explained your reasons.
Fortunately, I2c no longer uses numeric device IDs but names. So what are the alternatives?

1. modify the I2c subsystem to accept OF names additionally to I2c names (proposed by Jon smirl).
2. record the I2c name in the dts tree, either as seperate tag (like linux,i2c-name="<i2c-name>")
   or as additional compatible entry (like compatible="...", "linux,<i2c-name>").
3. use a glue layer with a translation map.

What is the preferred way to do this?

Thanks,
Jochen
--


[ message continues ]
[view original message]
From: Jean Delvare
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers
Date: Saturday, February 23, 2008 - 5:43 am

Hi Jochen,


The problem I have with this is that it breaks compatibility. The chip
name is not only used for device/driver matching, it is also exported
to userspace as a sysfs attribute ("name"). Applications might rely on
it. At least libsensors does.

One way to work around the problem would be to use separate fields for
device/driver matching and the "name" attribute. However, this will add
some complexity to the i2c-core code and cost some memory as well, so

This would work, and it would require almost no change to the current
i2c-core code, but I thought that these dts files were not under our
control?

The problem I see with this approach is that the name translation would
have to be done for each dts file, rather than just once for each device
type. This means more work, but maybe this can be done if at least part
of it is automated. I admit that I never considered this option because
I considered the dts files as read-only. If this assumption was
incorrect, then maybe this is the best solution. Can you please

This is what we do at the moment (see i2c_devices in
arch/powerpc/sysdev/fsl_soc.c). Jon Smirl is worried that it won't

I still have to think about it. I didn't have much time to work on this
during the last 2 weeks, hopefully I will fine some time to experiment
again soon. As I underlined before, my patch set affects no less than 5
subsystems with different needs and expectations, it's no trivial
change.

-- 
Jean Delvare
--


[ message continues ]
[view original message]
From: Jochen Friedrich
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers
Date: Tuesday, March 25, 2008 - 11:46 am

Sorry for the late response, but I was pretty busy the last couple of weeks.

I'm thinking about something like this (based on http://patchwork.ozlabs.org/linuxppc/patch?id=16282 to 16284):

This patch implements various helpers to support OF bindings for
the i2c API.

Signed-off-by: Jochen Friedrich <jochen@scram.de>
---
 drivers/of/Kconfig     |    6 +++
 drivers/of/Makefile    |    1 +
 drivers/of/i2c.c       |  113 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_i2c.h |   24 ++++++++++
 4 files changed, 144 insertions(+), 0 deletions(-)
 create mode 100644 drivers/of/i2c.c
 create mode 100644 include/linux/of_i2c.h

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index c03072b..3cff449 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -1,3 +1,9 @@
 config OF_DEVICE
 	def_bool y
 	depends on OF && (SPARC || PPC_OF)
+
+config OF_I2C
+	def_bool y
+	depends on OF && PPC_OF && I2C
+	help
+	  OpenFirmware I2C accessors
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index ab9be5d..655b982 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -1,2 +1,3 @@
 obj-y = base.o
 obj-$(CONFIG_OF_DEVICE) += device.o platform.o
+obj-$(CONFIG_OF_I2C)	+= i2c.o
diff --git a/drivers/of/i2c.c b/drivers/of/i2c.c
new file mode 100644
index 0000000..d3d1e9e
--- /dev/null
+++ b/drivers/of/i2c.c
@@ -0,0 +1,113 @@
+/*
+ * OF helpers for the I2C API
+ *
+ * Copyright (c) 2008 Jochen Friedrich <jochen@scram.de>
+ *
+ * Based on a previous patch from Jon Smirl <jonsmirl@gmail.com>
+ *
+ * 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 <asm/prom.h>
+
+struct i2c_driver_device {
+	char    *of_device;
+	char    *i2c_type;
+};
+
+static struct i2c_driver_device i2c_devices[] = ...
[ message continues ]
[view original message]
From: Stephen Rothwell
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers
Date: Tuesday, March 25, 2008 - 5:41 pm

Hi Jochen,

Firstly, you should probably cc Dave Miller <davem@davemloft.net> on
anything in drivers/of as Sparc is the other user of this stuff (just in
case they are interested).


                                      ^
You don't need the comma before the brace (unless you are anticipating
that struct i2c_driver_device will change.  Even then, its not a big

	for_each_child_of_node(adap_node, node) {

Do you need to clean up after the irq_of_parse_and_map() above?

--=20
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[ message continues ]
[view original message]
From: Olof Johansson
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers
Date: Saturday, February 23, 2008 - 2:28 pm

I have to say no on this one. The device tree is not supposed to know
about how linux uses devices, there are firmwares out there that don't

In my opinion this is an OK solution since the same information has to
be added somewhere already anyway -- eiither to the drivers or to this
translation table. It should of course be an abstacted shared table,
preferrably contained under the i2c source directories since several
platforms and architectures might share them.


-Olof
--


[ message continues ]
[view original message]
From: Jochen Friedrich
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers
Date: Sunday, February 24, 2008 - 8:16 am

I still believe this this could be done for embedded devices which are usually booted
via wrapper or U-Boot as those devices will most probably use the most exotic I2c devices

I could think of a mixture between 2. and 3.:

Using the compatible attribute with the manufacturer stripped off as I2c name by default
and using an exception table. For now, the struct i2c_driver_device would currently only
need one entry ("dallas,ds1374", "rtc-ds1374").

Thanks,
Jochen
--


[ message continues ]
[view original message]
From: Olof Johansson
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers
Date: Sunday, February 24, 2008 - 11:15 am

Hi,


Sorry, but you're wrong in your assumptions. Not all embedded devices
use U-boot, and not all use the wrapper. It's just a bad idea to encode
linux-specific things in the device tree, period.

And even if you DO decide to go that route, guess what? You need a

You still need the translation table, you're just flattening the
namespace to one string instead of two, the same information still has
to be encoded. I can't see what the benefit of this approach compared to
the other one is. "dallas,ds1374" already only has one translation entry
in the table?


-Olof

--


[ message continues ]
[view original message]
From: Jochen Friedrich
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers
Date: Monday, February 25, 2008 - 9:48 am

As soon as http://lists.lm-sensors.org/pipermail/i2c/2008-January/002752.html has been
applied, one could get rid of all entries where the I2c (alias) name can be obtained from
the OF name just by stripping the manufacturer.

Thanks,
Jochen
--


[ message continues ]
[view original message]
From: Jon Smirl
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers
Date: Sunday, February 24, 2008 - 9:19 am

The correct statement is: modify the i2c subsystem to support the
standard kernel driver aliasing mechanism. Leaving powerpc and OF out
of the argument for the moment, i2c has a custom aliasing scheme on
the x86 too.

So as a first step, can we remove the custom i2c aliasing scheme and
change i2c to use standard module aliases on the x86? Patches for this
already exist.


I think there is some confusion here. The OF aliases are only used by
the kernel to load the correct driver. Would doing something like this
help?

static struct i2c_device_id pcf8563_id[] = {
	{"pcf8563", 0, "sysfs_legacy_name"},
	{"rtc8564", 0, "sysfs_legacy_name"},
	OF_ID("phillips,pcf8563", &pcf8563_id[0], 0)
	OF_ID("epson,rtc8564", &pcf8563_id[1], 0)
	{},
};
MODULE_DEVICE_TABLE(i2c, pcf8563_id);

Then in the probe function you can use the pointer to find the base id

Not really practical for the millions of machines (all PowerPC Macs)

Audio codecs have exactly the same problem. There are probably other
devices that also need mapping.

This mapping table will need to contain a map from the OF names to the
kernel driver names.  It will need to stored permanently in RAM and
contain all possible mappings. This table will only grow in size.

The kernel has a widely used mechanism for mapping -- aliases in the
device drivers. Why do we want to build a new, parallel one?

What we are doing now is option 4.

4. Use kconfig to build custom kernels for each target system. Don't
load drivers automatically based on what the BIOS tells us.

-- 
Jon Smirl
jonsmirl@gmail.com
--


[ message continues ]
Previous thread: [PATCH 6/6] [NETNS]: Lookup in FIB semantic hashes taking into account the namespace. by Denis V. Lunev on Thursday, January 31, 2008 - 5:00 am. (1 message)

Next thread: Why do I have a /sys/devices/system/cpu/cpuidle under kernel 2.6.24 with a Pentium M processor ? by Toralf on Thursday, January 31, 2008 - 6:00 am. (3 messages)