The driver model, I2C and gpio provision on Sharp SL-C1000 (Akita)

This code is the final link is getting akita working but I'm not sure its the right approach. I'm posting this in the hope someone might see a better way to achieve this driver's objectives. I'd like to get akita support into mainline and this is the only barrier.

Background:

The Sharp SL-C1000 (Akita) has an 8 port Maxim 7310 IO Expander connected via I2C. In this specific case, 5 outputs are used from a variety of linux subsystems such as the backlight, IR port and sound.

Driver Design Criteria:

* GPIOs need to be available fairly early in the boot procedure as we can't guarantee when the subsystems will use them
* GPIOs can be changed from interrupt context
* GPIO providers need to be one of the last devices suspended/first resumed

A Solution:

Firstly, I wrote an I2C driver for the MAX7310 which is included below. This shouldn't be too controversial. It does export some functions and I'm not sure what to do about a header file for them. Creating a specific header file for them seems a little over the top. I realise I need to sort out the I2C_DRIVERID properly. I'm also aware it has a lot of different ids and no method to detect whether the chip is really present (Akita uses 0x18 as the slave id).

I2C drivers appear relatively late in the boot procedure and changing that isn't practical. I therefore ended up writing akita-ioexp which acts as an interface between the rest of the kernel drivers and the max7310 i2c driver.

When akita-ioexp loads, the i2c driver isn't likely to be present. I've used the init levels to ensure it comes up after the i2c-core but that's about all we can hope for. It therefore searches for the max7310 and if not found, schedules a repeat of the search for later, repeating until it finds the device (using a workqueue). The workqueue is also used to get around gpio requests from interrupt context.

I had to turn akita-ioexp into a platform device to get the suspend signal which is used to flush the workqueue. With a platform device available at machine init time, I can insert it as a parent of the corgi device chain which means its one of the last devices to be suspended. I couldn't do this with an i2c device as it won't be around at early init. I do remember reading about the idea of being able to dynamically alter the device parent chain which would help with this but I don't think this exists (yet?).

In this case, we only have output gpios and the timing isn't critical. We can therefore queue the output until the max7310 becomes available. This wouldn't work for inputs or devices that would have an issue with delayed output.

Alternatives:

I can see a case for ditching the generic max7310 driver and putting its code directly into akita-ioexp. If I'd tried this the other way around, I know what I would have been told :). Anyhow, this solves some issues and creates others. The main one would be how to arrange for a very late suspend call as the device wouldn't be available to set as a parent for the corgi device tree at early boot. Obviously any such driver would then be totally akita specific.

There is a fundamental problem with the lack of a proper gpio interface in Linux. Every driver does something different with them (be it pxa specific gpios, SCOOP gpios, those on a IO expander, those on a video chip (w100fb springs to mind) to name just the Zaurus specific ones.

Is this patch going to be acceptable and/or is there anything we can do to improve this driver and/or Linux in general to support these kind of needs?

Richard

[The full Akita machine patch is available as
http://www.rpsys.net/openzaurus/patches/spitz_base_extras-r4.patch ]

Index: linux-2.6.13/arch/arm/mach-pxa/akita-ioexp.c
===================================================================
@@ -0,0 +1,118 @@
+/*
+ * Support for the IO Expander on the Sharp SL-C1000 (Akita)
+ *
+ * Copyright 2005 Openedhand Ltd.
+ *
+ * Author: Richard Purdie <[email protected]>
+ *
+ * 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
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/workqueue.h>
+#include <asm/arch/akita.h>
+
+/* These should be somewhere else */
+int max7310_config(struct device *dev, int iomode, int polarity);
+int max7310_set_ouputs(struct device *dev, int outputs);
+
+static void akita_ioexp_work(void *private_);
+
+static struct device *akita_ioexp_device;
+static unsigned char ioexp_output_value = AKITA_IOEXP_IO_OUT;
+DECLARE_WORK(akita_ioexp, akita_ioexp_work, NULL);
+
+void akita_set_ioexp(struct device *dev, unsigned char bit)
+{
+	ioexp_output_value |= bit;
+
+	if (akita_ioexp_device)
+		schedule_work(&akita_ioexp);
+	return;
+}
+
+void akita_reset_ioexp(struct device *dev, unsigned char bit)
+{
+	ioexp_output_value &= ~bit;
+
+	if (akita_ioexp_device)
+		schedule_work(&akita_ioexp);
+	return;
+}
+
+EXPORT_SYMBOL(akita_set_ioexp);
+EXPORT_SYMBOL(akita_reset_ioexp);
+
+static int is_akita_ioexp_device(struct device *dev, void *data)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	return (strncmp(client->name, "max7310", 7) == 0);
+}
+
+static void akita_ioexp_work(void *private_)
+{
+	if (!akita_ioexp_device) {
+		akita_ioexp_device = bus_find_device(&i2c_bus_type, NULL, NULL, is_akita_ioexp_device);
+
+		if (!akita_ioexp_device) {
+			/* if we can't find the device, wait and retry */
+			schedule_delayed_work(&akita_ioexp, msecs_to_jiffies(500));
+			return;
+		}
+		
+		max7310_config(akita_ioexp_device, AKITA_IOEXP_IO_DIR, 0);
+	}
+
+	max7310_set_ouputs(akita_ioexp_device, ioexp_output_value);
+}
+
+
+#ifdef CONFIG_PM
+static int akita_ioexp_suspend(struct device *dev, pm_message_t state, uint32_t level)
+{
+	if (level == SUSPEND_POWER_DOWN) {
+		flush_scheduled_work();
+	}
+	return 0;
+}	
+		
+static int akita_ioexp_resume(struct device *dev, uint32_t level)
+{
+	if (level == RESUME_POWER_ON) {
+		schedule_work(&akita_ioexp);
+	}
+	return 0;
+}
+#else
+#define akita_ioexp_suspend NULL
+#define akita_ioexp_resume NULL
+#endif
+
+
+static int __init akita_ioexp_probe(struct device *dev)
+{
+	schedule_work(&akita_ioexp);
+	return 0;
+}
+
+static struct device_driver akita_ioexp_driver = {
+	.name		= "akita-ioexp",
+	.bus		= &platform_bus_type,
+	.probe		= akita_ioexp_probe,
+	.suspend	= akita_ioexp_suspend,
+	.resume		= akita_ioexp_resume,
+};
+
+static int __devinit akita_ioexp_init(void)
+{
+	return driver_register(&akita_ioexp_driver);
+}
+
+module_init(akita_ioexp_init);
+

Index: linux-2.6.13/drivers/i2c/chips/Kconfig
===================================================================
@@ -126,4 +126,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called max6875.
 
+config MAX7310
+	tristate "Maxim MAX7310 8 Port IO Exander"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the Maxim MAX7310
+	  8 Port IO Exander. 
+
+	  This chip is found in certain devices such as the Sharp Zaurus
+	  SL-C1000 (Akita).
+	  
+	  This driver can also be built as a module.  If so, the module
+	  will be called max7310.
+
 endmenu

Index: linux-2.6.13/drivers/i2c/chips/Makefile
===================================================================
@@ -12,6 +12,7 @@
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
 obj-$(CONFIG_SENSORS_RTC8564)	+= rtc8564.o
 obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
+obj-$(CONFIG_MAX7310)		+= max7310.o
 obj-$(CONFIG_TPS65010)		+= tps65010.o
 
 ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)

You're not the only one with this problem, and this is one reason I've never submitted the SA11x0 audio drivers. The L3 bus subsystem inter- links on some platforms with the I2C subsystem (it physically shares the same signals but it isn't I2C compatible as such.)

Hence, in order to lock I2C off the bus, we have to take the same driver-based locks as I2C. This can only really happen if I2C is initialised first.

Below is the complete patch for the L3 support, which includes moving the I2C initialisation early.

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git -N linus/Documentation/l3/structure linux/Documentation/l3/structure
@@ -0,0 +1,23 @@
+L3 Bus Driver
+-------------
+
+The structure of the L3 subsystem is as follows:
+
+       ^              ^            ^
+       |              |            |
+       |        +-----v----+  +----v-----+
+       |        | device   |  | device   |
+       |        | driver 1 |  | driver 2 |
+  +----v-----+  +-----^----+  +----^-----+
+  |   core   |        |            |
+  | services <--------+------------+
+  +----^-----+
+       |
+   +---v----+   +-----------+
+   |  bus   <---> algorithm |
+   | driver |   |  driver   |
+   +--------+   +-----------+
+
+Applications talk to the core to obtain bus adapters.  Applications talk
+to device drivers to perform actions.  Device drivers in turn talk to the
+core to perform L3 bus transactions via the bus and algorithm drivers.

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -r orig/arch/arm/Kconfig linux/./arch/arm/Kconfig
@@ -736,7 +736,7 @@ source "drivers/char/Kconfig"
 
 source "drivers/hwmon/Kconfig"
 
-#source "drivers/l3/Kconfig"
+source "drivers/l3/Kconfig"
 
 source "drivers/misc/Kconfig"
 

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -r orig/drivers/Makefile linux/./drivers/Makefile
@@ -12,6 +12,8 @@ obj-$(CONFIG_ACPI_BOOT)		+= acpi/
 # PnP must come after ACPI since it will eventually need to check if acpi
 # was used and do nothing if so
 obj-$(CONFIG_PNP)		+= pnp/
+obj-$(CONFIG_I2C)		+= i2c/
+obj-$(CONFIG_L3)		+= l3/
 
 # char/ comes before serial/ etc so that the VT console is the boot-time
 # default.

@@ -52,7 +54,6 @@ obj-$(CONFIG_SBUS)		+= sbus/
 obj-$(CONFIG_GAMEPORT)		+= input/gameport/
 obj-$(CONFIG_INPUT)		+= input/
 obj-$(CONFIG_I2O)		+= message/
-obj-$(CONFIG_I2C)		+= i2c/
 obj-$(CONFIG_W1)		+= w1/
 obj-$(CONFIG_HWMON)		+= hwmon/
 obj-$(CONFIG_PHONE)		+= telephony/

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x *.orig -x *.rej -x .git linus/drivers/i2c/busses/Kconfig linux/drivers/i2c/busses/Kconfig
@@ -107,6 +107,17 @@ config I2C_HYDRA
 	  This support is also available as a module.  If so, the module
 	  will be called i2c-hydra.
 
+config I2C_BIT_SA1100_GPIO
+	tristate "SA1100 I2C GPIO adapter"
+	depends on I2C && ARCH_SA1100
+	select BIT_SA1100_GPIO
+	help
+	  This supports I2C on the SA11x0 processor GPIO pins.  This
+	  shares support with the L3 driver.
+
+	  This support is also available as a module.  If so, the module
+	  will be called l3-bit-sa1100.
+
 config I2C_I801
 	tristate "Intel 82801 (ICH)"
 	depends on I2C && PCI

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git -N orig/drivers/l3/Kconfig linux/drivers/l3/Kconfig
@@ -0,0 +1,23 @@
+#
+# L3 bus configuration
+#
+
+menu "L3 serial bus support"
+
+config L3
+	tristate "L3 support"
+
+config L3_BIT_SA1100_GPIO
+	tristate "SA11x0 L3 GPIO adapter"
+	depends on L3 && ARCH_SA1100
+	select BIT_SA1100_GPIO
+
+config BIT_SA1100_GPIO
+	tristate
+	select L3_ALGOBIT if L3
+	select I2C_ALGOBIT if I2C
+
+config L3_ALGOBIT
+	bool
+
+endmenu

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git -N orig/drivers/l3/Makefile linux/drivers/l3/Makefile
@@ -0,0 +1,8 @@
+#
+# Makefile for the L3 bus driver.
+#
+
+obj-$(CONFIG_L3)		+= l3-core.o
+obj-$(CONFIG_L3_ALGOBIT)	+= l3-algo-bit.o
+obj-$(CONFIG_BIT_SA1100_GPIO)	+= l3-bit-sa11x0.o
+

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -x .git -N orig/drivers/l3/l3-algo-bit.h linux/drivers/l3/l3-algo-bit.h
@@ -0,0 +1,37 @@
+/*
+ *  linux/include/linux/l3/algo-bit.h
+ *
+ *  Copyright (C) 2001 Russell King, All Rights Reserved.
+ *
+ * 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.
+ *
+ * L3 Bus bit-banging algorithm.  Derived from i2c-algo-bit.h by
+ * Simon G. Vogl.
+ */
+#ifndef L3_ALGO_BIT_H
+#define L3_ALGO_BIT_H 1
+
+#include <linux/l3.h>
+
+struct l3_algo_bit_data {
+	void (*setdat) (struct l3_adapter *, int state);
+	void (*setclk) (struct l3_adapter *, int state);
+	void (*setmode)(struct l3_adapter *, int state);
+	void (*setdir) (struct l3_adapter *, int in);	/* set data direction */
+	int  (*getdat) (struct l3_adapter *);
+
+	/* bus timings (us) */
+	int	data_hold;
+	int	data_setup;
+	int	clock_high;
+	int	mode_hold;
+	int	mode_setup;
+	int	mode;
+};
+
+ssize_t l3_algo_bit_read(struct l3_adapter *, struct l3_algo_bit_data *, u8 addr, u8 *buf, size_t len);
+ssize_t l3_algo_bit_write(struct l3_adapter *, struct l3_algo_bit_data *, u8 addr, const u8 *buf, size_t len);
+
+#endif

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Hi!

Well, what you describe is not too nice, but I do not see nicer solutions :-(.

It seems that making i2c init early is only sane choice. I realize PC people will hate it... but apart from that, why is it impractical?

Yup. GPIOs are not problem on i386, so noone solved this one :-(.

Pavel

64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

FWIW, I have also run into this "I need I2C early in boot, but it's not inited until late" on SiByte (arch/mips/sibyte/{sb1250,bcm1480}/setup.c). For the time being in the linux-mips tree we simply have two drivers talking to the I2C interface - sibyte/swarm/rtc_* and i2c-sibyte.c, and they are currently lacking even any trivial locking. We haven't seen any problems yet but that's due to limited exercise - the default config doesn't hook up any drivers for the other chips on I2C.

How do other arches that have I2C RTCs deal with this problem? Or is there something wrong with how arch/mips/kernel/time.c:time_init deals with the rtc?

I would also be overjoyed to have a GPIO infrastructure to plug into.

(And I would say "GPIOs are not used on PCs"; I am confident the Geode driving the seat-back TV on this Song flight has GPIOs...)

-andy

On ARM, where we have I2C RTCs, I tend to leave xtime well alone in time_init and just setup the timer. When i2c is initialised, and the bus and RTC have been detected, I set the time from them at that point.

I haven't seen any problems with this approach. In fact, I'd rather time_init() just setup the timer, and we set the time of day later during the kernels initialisation.

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core