Re: tfp410 and i2c_bus_num
(dropping Tony and stable) On 2012-11-18 13:34, Felipe Balbi wrote: > how are you toggling the gpio ? You said tfp410 isn't controlled at all > except for the power down gpio. Who provides the gpio ? It's a normal OMAP GPIO, going to TFP410. I use gpio_set_value() to set/unset it. >> Well, it's not that simple. TFP410 manages hot plug detection of the >> HDMI cable (although not implemented currently), so the we'd need to >> convey that information to the i2c-edid somehow. Which, of course, is >> doable, but increases complexity. > > I'd say it would decrease it since you would have a single copy of > i2c-edid.c for DRM or DSS or any other panel/display framework. Well. Reading EDID via i2c is one or two i2c reads. There's nothing else to it. If we had a separate, independent i2c-edid driver, we'd have to somehow link the i2c-edid driver and tfp410 (or whatever driver would handle the video link in that particular case) so that the i2c-edid driver would know about hotplug events. We would also need to link the drivers so that the edid can be associated with the video pipeline the tfp410 chip is part of, and to the particular slot in the pipeline where the tfp410 is. I haven't tried writing such a driver, but it sounds much more complex to me than doing one or two i2c reads in the tfp410 driver. >> Another thing is that even if in this case the i2c and EDID reading are >> separate from the actual video path, that may not be the case for other >> display solutions. We could have a DSI panel which reports its >> resolution via DSI bus, or we could have an external DSI-to-HDMI chip, >> which reads the EDID via i2c from the monitor, but reports them back to >> the SoC via DSI bus. > > In this last case we would see just the DSI, not the I2C bus, so it's > like I2C bus doesn't exist, so in fact we would have two possibilities: > > a) read EDID via I2C bus (standard HDMI) > b) read EDID via DSI. Right. And currently reading edid is done via read_edid call with omapdss, and the caller doesn't care if read_edid uses i2c or DSI, so it should just work. > b) doesn't exist today so we need to care about it until something like > what you say shows up in the market. Until then, we care for EDID over > I2C IMHO. There are chips such as I described, although not supported in the mainline. >> So we need a generic way to get the resolution information, and it makes >> sense to have this in the components of the video path, not in a >> separate driver which is not part of the video path as such. > > But the I2C bus isn't part of the video path anyway. It's a completely > separate path which is dedicated to reading EDID. If you need a generic While reading EDID is totally separate from the video data itself, it is not separate from the hotplug status of the cable. And the EDID needs to be associated with the video path in question, of course. > way for that case you wanna cope with other methods for reading EDID, > then you need a generic layer which doesn't care if it's I2C or DSI. > Currently implementation is hardcoded to I2C anyway. No, reading edid is done via generic read_edid call. TFP410 uses i2c to read edid, but it could do it in any way it wants. > In fact current implementation is far worse than having an extra > i2c-edid.c driver because it doesn't encapsulate EDID implementation > from tfp410. > > Then moment you need to read EDID via DSI, you will likely have to pass > a DSI handle to e.g. TFP410 so it can read EDID via DSI. TFP410 is a parallel RGB to DVI chip. It's not related to DSI. The DSI case would be with some other chip. > What I'm suggesting, however, is that you have a layer hiding all that. > Make your i2c-edid.c driver read EDID and report it to this generic > layer, if some other driver in kernel needs the information, you should > be calling into this generic edid layer to get the cached values. > > If you don't need that data in kernel, then just expose it through a set > of standard sysfs files and teach Xorg about those. We need the data in the kernel. >> And while the above issues could perhaps be solved, all we do is read >> one or two 128 byte datablocks from the i2c device. I'm not sure if the >> extra complexity is worth it. At least it's not on the top of my todo > > When you have EDID over DSI, what will you do ? Write the code to read the EDID =). How that is done in practice depends on the chip in question, using chip specific DSI commands. >> list as long as the current solution works =). > > that's your choice. > >>> If you have a generic i2c-edid.c simple driver, I guess X could be >>> changes to read those values from sysfs and take actions based on those. >> >> We handle the EDID inside the kernel, although I'm not sure if drm also >> exposes the EDID to userspace. > > I suppose it does, but haven't looked through the code. > >>> Looks like even drm_edid.c should change, btw. >> >> Yes, DRM handles it in somewhat similar way. > > another reason
[PATCH v4 1/4] i2c: introduce i2c-cbus-gpio driver
Add i2c driver to enable access to devices behind CBUS on Nokia Internet
Tablets.
The patch also adds CBUS I2C configuration for N8x0 which is one of the
users of this driver.
Cc: linux-i2c@vger.kernel.org
Acked-by: Felipe Balbi
Acked-by: Tony Lindgren
Signed-off-by: Aaro Koskinen
Cc: Wolfram Sang
---
.../devicetree/bindings/i2c/i2c-cbus-gpio.txt | 27 ++
arch/arm/mach-omap2/board-n8x0.c | 42 +++
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile|1 +
drivers/i2c/busses/i2c-cbus-gpio.c | 300
include/linux/platform_data/i2c-cbus-gpio.h| 27 ++
6 files changed, 407 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-cbus-gpio.txt
create mode 100644 drivers/i2c/busses/i2c-cbus-gpio.c
create mode 100644 include/linux/platform_data/i2c-cbus-gpio.h
diff --git a/Documentation/devicetree/bindings/i2c/i2c-cbus-gpio.txt
b/Documentation/devicetree/bindings/i2c/i2c-cbus-gpio.txt
new file mode 100644
index 000..8ce9cd2
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-cbus-gpio.txt
@@ -0,0 +1,27 @@
+Device tree bindings for i2c-cbus-gpio driver
+
+Required properties:
+ - compatible = "i2c-cbus-gpio";
+ - gpios: clk, dat, sel
+ - #address-cells = <1>;
+ - #size-cells = <0>;
+
+Optional properties:
+ - child nodes conforming to i2c bus binding
+
+Example:
+
+i2c@0 {
+ compatible = "i2c-cbus-gpio";
+ gpios = <&gpio 66 0 /* clk */
+&gpio 65 0 /* dat */
+&gpio 64 0 /* sel */
+ >;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ retu-mfd: retu@1 {
+ compatible = "retu-mfd";
+ reg = <0x1>;
+ };
+};
diff --git a/arch/arm/mach-omap2/board-n8x0.c b/arch/arm/mach-omap2/board-n8x0.c
index d95f727..bbfd742 100644
--- a/arch/arm/mach-omap2/board-n8x0.c
+++ b/arch/arm/mach-omap2/board-n8x0.c
@@ -16,10 +16,12 @@
#include
#include
#include
+#include
#include
#include
#include
#include
+#include
#include
#include
#include
@@ -39,6 +41,45 @@
#define TUSB6010_GPIO_ENABLE 0
#define TUSB6010_DMACHAN 0x3f
+#if defined(CONFIG_I2C_CBUS_GPIO) || defined(CONFIG_I2C_CBUS_GPIO_MODULE)
+static struct i2c_cbus_platform_data n8x0_cbus_data = {
+ .clk_gpio = 66,
+ .dat_gpio = 65,
+ .sel_gpio = 64,
+};
+
+static struct platform_device n8x0_cbus_device = {
+ .name = "i2c-cbus-gpio",
+ .id = 3,
+ .dev= {
+ .platform_data = &n8x0_cbus_data,
+ },
+};
+
+static struct i2c_board_info n8x0_i2c_board_info_3[] __initdata = {
+ {
+ I2C_BOARD_INFO("retu-mfd", 0x01),
+ },
+};
+
+static void __init n8x0_cbus_init(void)
+{
+ const int retu_irq_gpio = 108;
+
+ if (gpio_request_one(retu_irq_gpio, GPIOF_IN, "Retu IRQ"))
+ return;
+ irq_set_irq_type(gpio_to_irq(retu_irq_gpio), IRQ_TYPE_EDGE_RISING);
+ n8x0_i2c_board_info_3[0].irq = gpio_to_irq(retu_irq_gpio);
+ i2c_register_board_info(3, n8x0_i2c_board_info_3,
+ ARRAY_SIZE(n8x0_i2c_board_info_3));
+ platform_device_register(&n8x0_cbus_device);
+}
+#else /* CONFIG_I2C_CBUS_GPIO */
+static void __init n8x0_cbus_init(void)
+{
+}
+#endif /* CONFIG_I2C_CBUS_GPIO */
+
#if defined(CONFIG_USB_MUSB_TUSB6010) ||
defined(CONFIG_USB_MUSB_TUSB6010_MODULE)
/*
* Enable or disable power to TUSB6010. When enabling, turn on 3.3 V and
@@ -677,6 +718,7 @@ static void __init n8x0_init_machine(void)
gpmc_onenand_init(board_onenand_data);
n8x0_mmc_init();
n8x0_usb_init();
+ n8x0_cbus_init();
}
MACHINE_START(NOKIA_N800, "Nokia N800")
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e9df461..e949edf 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -337,6 +337,16 @@ config I2C_BLACKFIN_TWI_CLK_KHZ
help
The unit of the TWI clock is kHz.
+config I2C_CBUS_GPIO
+ tristate "CBUS I2C driver"
+ depends on GENERIC_GPIO
+ help
+ Support for CBUS access using I2C API. Mostly relevant for Nokia
+ Internet Tablets (770, N800 and N810).
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-cbus-gpio.
+
config I2C_CPM
tristate "Freescale CPM1 or CPM2 (MPC8xx/826x)"
depends on (CPM1 || CPM2) && OF_I2C
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 395b516..f9e3e0b 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_I2C_POWERMAC)+= i2c-powermac.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_CBUS_GPIO)+= i2c-cbus-gpio.
Re: tfp410 and i2c_bus_num
Hi, On Sat, Nov 17, 2012 at 07:41:36AM +0200, Tomi Valkeinen wrote: > On 2012-11-16 20:21, Felipe Balbi wrote: > > Hi, > > > > On Fri, Nov 16, 2012 at 05:39:44PM +0200, Tomi Valkeinen wrote: > > To be fair, the whole i2c_bus_num looks like a big hackery introduced by > > the way panel drivers are written for OMAP DSS. > > > > TFP410 is an I2C client, not an OMAPDSS client. After a quick look at > > the driver, there's is no such thing as a DSS bus, so looks like you > > should have an I2C driver for TFP410 and the whole DSS stuff should be > > just a list of clients, but not a struct bus at all. > > > > The fact that you have to pass the I2C bus number down to the panel > > driver is already a big indication of how wrong this is, IMHO. > > Without going deeper in the dss device model problems, I would agree > with you if this was about i2c panel, but this is not quite like that. > > A panel controlled via i2c would be an i2c device. But TFP410 is not > controlled via i2c. It's not really controlled at all except via > power-down gpio. TFP410 doesn't need the i2c to be functional at all. > >>> > >>> then why does it need the i2c adapter ? What is this power-down gpio ? > >>> Should that be hidden under gpiolib instead ? > >> > >> For the i2c, see below. Power-down GPIO is used to power down and up the > >> tfp410 chip. > > > > that much I guessed ;-) Should it be hidden under gpiolib ? > > I have to say I have no idea what you mean... Hidden how? how are you toggling the gpio ? You said tfp410 isn't controlled at all except for the power down gpio. Who provides the gpio ? > The i2c lines do not even touch TFP410 chip, so to be precise, the i2c > lines should not be TFP410's concern. The i2c lines come from the > monitor and go to OMAP's i2c pins. But TFP410 driver is a convenient > place to manage them. > >>> > >>> fair enough... but who's actually using those i2c lines ? OMAP is the > >>> I2C master, who's the slave ? It's something in the monitor, I assume... > >>> > >>> IIUC, this I2C bus goes over the HDMI wire ? > >> > >> Yes, the i2c goes over HDMI wire. OMAP is the master, monitor is the > >> slave. You can see some more info from > >> http://en.wikipedia.org/wiki/Display_Data_Channel under DDC2 section. > >> > >> It is used to read the EDID > >> (http://en.wikipedia.org/wiki/Extended_display_identification_data) > >> information from the monitor, which tells things like supported video > >> timings etc. > >> > >> As for why the tfp410 driver handles the i2c... We don't have a better > >> place. There's no driver for the monitor. Although in the future with > > > > than that's wrong :-) If TFP410 isn't really using I2C it shouldn't need > > the whole i2c_bus_num logic. I'm far from fully understanding dss > > architecture but it looks like what you want is a generic 'i2c-edid.c' > > which just reads the edid structure during probe and caches the values > > and exposes them via sysfs ?!? (perhaps you also need a kernel API to > > read those values... I don't know; but that's also doable). > > Well, it's not that simple. TFP410 manages hot plug detection of the > HDMI cable (although not implemented currently), so the we'd need to > convey that information to the i2c-edid somehow. Which, of course, is > doable, but increases complexity. I'd say it would decrease it since you would have a single copy of i2c-edid.c for DRM or DSS or any other panel/display framework. > Another thing is that even if in this case the i2c and EDID reading are > separate from the actual video path, that may not be the case for other > display solutions. We could have a DSI panel which reports its > resolution via DSI bus, or we could have an external DSI-to-HDMI chip, > which reads the EDID via i2c from the monitor, but reports them back to > the SoC via DSI bus. In this last case we would see just the DSI, not the I2C bus, so it's like I2C bus doesn't exist, so in fact we would have two possibilities: a) read EDID via I2C bus (standard HDMI) b) read EDID via DSI. b) doesn't exist today so we need to care about it until something like what you say shows up in the market. Until then, we care for EDID over I2C IMHO. > So we need a generic way to get the resolution information, and it makes > sense to have this in the components of the video path, not in a > separate driver which is not part of the video path as such. But the I2C bus isn't part of the video path anyway. It's a completely separate path which is dedicated to reading EDID. If you need a generic way for that case you wanna cope with other methods for reading EDID, then you need a generic layer which doesn't care if it's I2C or DSI. Currently implementation is hardcoded to I2C anyway. In fact current implementation is far worse than having an extra i2c-edid.c driver because it doesn't encapsulate EDID implementation from tfp410. Then moment you need to rea
