[i2c] [RFC] Lifebook apanel driver

Jean Delvare khali at linux-fr.org
Thu Jan 4 09:17:20 CET 2007


Hi Stephen,

On Wed, 3 Jan 2007 15:23:51 -0800, Stephen Hemminger wrote:
> This is a driver to support the applications buttons on the Fujitsu Lifebook laptops.
> It is based on the earlier apanel driver done by Jochen Eisenger, but with many changes.
> The original driver used ioctl's and a separate user space program, this version hooks
> into the input subsystem so that the normal Gnome/KDE shortcuts work without any
> userspace changes. In order to run all that is need is to load the i2c interface (i2c-i801)
> and the apanel module.
> 
> It has been tested (a little) on the two laptop's listed in the DMI table. 
> It probably works on more but may require some changes.
> 
> Signed-off-by: Stephen Hemminger <shemminger at osdl.org>
> 
> ---
>  drivers/input/misc/Kconfig  |   11 +
>  drivers/input/misc/Makefile |    1 
>  drivers/input/misc/apanel.c |  384 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c-id.h      |    1 
>  4 files changed, 397 insertions(+)
> 
> --- lifebook.orig/drivers/input/misc/Kconfig	2007-01-03 14:44:55.000000000 -0800
> +++ lifebook/drivers/input/misc/Kconfig	2007-01-03 14:44:57.000000000 -0800
> @@ -50,6 +50,17 @@
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called wistron_btns.
>  
> +config INPUT_APANEL
> +       tristate "Fujitsu Lifebook Application Panel buttons"
> +       depends on X86 && !X86_64
> +       select I2C
> +       help

Broken indentation, you should be using tabs not spaces. Also, drivers
should depend on I2C, not select it. Mixing both tends to be confusing.

> +	 Say Y here for support of the Application Panel buttons, used on
> +	 Fujitsu Lifebook.
> +
> +	 To compile this driver as a module, choose M here: the module will
> +	 be called apanel.
> +
>  config INPUT_IXP4XX_BEEPER
>  	tristate "IXP4XX Beeper support"
>  	depends on ARCH_IXP4XX
> --- lifebook.orig/drivers/input/misc/Makefile	2007-01-03 14:44:55.000000000 -0800
> +++ lifebook/drivers/input/misc/Makefile	2007-01-03 14:44:57.000000000 -0800
> @@ -9,5 +9,6 @@
>  obj-$(CONFIG_INPUT_M68K_BEEP)		+= m68kspkr.o
>  obj-$(CONFIG_INPUT_UINPUT)		+= uinput.o
>  obj-$(CONFIG_INPUT_WISTRON_BTNS)	+= wistron_btns.o
> +obj-$(CONFIG_INPUT_APANEL)		+= apanel.o
>  obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
>  obj-$(CONFIG_INPUT_IXP4XX_BEEPER)	+= ixp4xx-beeper.o
> --- lifebook.orig/include/linux/i2c-id.h	2007-01-03 14:44:55.000000000 -0800
> +++ lifebook/include/linux/i2c-id.h	2007-01-03 14:44:57.000000000 -0800
> @@ -115,6 +115,7 @@
>  #define I2C_DRIVERID_KS0127	86	/* Samsung ks0127 video decoder */
>  #define I2C_DRIVERID_TLV320AIC23B 87	/* TI TLV320AIC23B audio codec  */
>  #define I2C_DRIVERID_ISL1208	88	/* Intersil ISL1208 RTC		*/
> +#define I2C_DRIVERID_APANEL	89	/* Lifebook Application Panel   */

Probably not needed, see below.

>  #define I2C_DRIVERID_I2CDEV	900
>  #define I2C_DRIVERID_ARP        902    /* SMBus ARP Client              */
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ lifebook/drivers/input/misc/apanel.c	2007-01-03 15:07:04.000000000 -0800
> @@ -0,0 +1,384 @@
> +/*
> +    SMBus client for the Fujitsu Lifebook Application Panel
> +
> +    Copyright (C) 2007 Stephen Hemminger <shemminger at osdl.org>
> +    Copyright (C) 2001-2003 Jochen Eisinger <jochen at penguin-breeder.org>
> +
> +    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.

Actually not, the kernel is GPLv2 only.

> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> +    You should have received a copy of the GNU General Public License
> +    along with this program; if not, write to the Free Software
> +    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA

This address is no longer valid. This is now:
51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.

> +*/
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/ioport.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/dmi.h>
> +#include <linux/input.h>
> +#include <linux/i2c.h>
> +#include <linux/workqueue.h>
> +
> +/* List of systems known to work */
> +static struct dmi_system_id apanel_dmi_table[] __initdata = {
> +	{
> +		.ident = "Lifebook S",
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "LifeBook S Series"),
> +		},
> +	},
> +	{
> +		.ident = "Lifebook B6210",
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Lifebook B6210"),
> +		},
> +	},
> +	{ }
> +};

One entry has a capital B and the other doesn't... Typo?

> +
> +enum apanel_devid {
> +	APANEL_DEV_NONE = 0,
> +	APANEL_DEV_APPBTN=1,
> +	APANEL_DEV_CDBTN= 2,
> +	APANEL_DEV_LCD	= 3,
> +	APANEL_DEV_LED	= 4,

One more tab would align numbers better.

> +
> +	APANEL_DEV_MAX,
> +};
> +
> +static enum apanel_chip {
> +	CHIP_NONE   =0,
> +	CHIP_OZ992C =1,
> +	CHIP_OZ163T =2,
> +	CHIP_OZ711M3=4,
> +} devdata[APANEL_DEV_MAX];

Same here, and one space after "=".

> +
> +static const char *device_names[APANEL_DEV_MAX] = {
> +	[APANEL_DEV_APPBTN] = "Application Buttons",
> +	[APANEL_DEV_LCD]    = "LCD",
> +	[APANEL_DEV_LED]    = "LED",
> +	[APANEL_DEV_CDBTN]  = "CD Buttons",
> +};

Align with a tab instead of spaces.

> +
> +struct apanel {
> +	struct input_dev *input;
> +	struct i2c_client client;
> +	struct delayed_work poll_timer;
> +	struct work_struct led_work;
> +};
> +
> +#define POLL_FREQUENCY 10 /* Number of polls per second */
> +
> +#if POLL_FREQUENCY > HZ
> +#error "POLL_FREQUENCY too high"
> +#endif
> +
> +MODULE_AUTHOR("Stephen Hemminger <shemminger at osdl.org>, Jochen Eisinger <jochen at penguin-breeder.org>");

Line too long, please split.

> +MODULE_DESCRIPTION("Fujitsu Lifebook Application Panel driver");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("0.2");
> +
> +/* NB: can't call this 'force' because of I2C_INSMOD macro foolishness. */

This is being worked on, please be patient...

> +static int dmi_force;
> +module_param(dmi_force, bool, 0);
> +MODULE_PARM_DESC(dmi_force, "Load even if computer is not in database");
> +
> +/* definitions for i2c (smbus) interface */
> +static int apanel_detach_client(struct i2c_client *client);
> +static int apanel_attach_adapter(struct i2c_adapter *adap);
> +
> +static struct i2c_driver apanel_driver = {
> +	.driver = {
> +		.name = "apanel",
> +	},
> +	.id = I2C_DRIVERID_APANEL,

You don't use this value anywhere, do you? Given it's optional, I'd
rather not define it unless you really need it. This field is likely to
be dropped in a future i2c-core cleanup.

> +	.attach_adapter = &apanel_attach_adapter,
> +	.detach_client = &apanel_detach_client,
> +};
> +
> +/* for now, we only support one address */
> +static unsigned short normal_i2c[] = {0, I2C_CLIENT_END};
> +
> +/* generate some stupid additional structures.  */
> +I2C_CLIENT_INSMOD;
> +
> +static void report_key(struct input_dev *input, u8 key)
> +{
> +	input_report_key(input, key, 1);
> +	input_sync(input);
> +	input_report_key(input, key, 0);
> +	input_sync(input);
> +}
> +
> +/* Poll for key changes every 100ms
> + *
> + * Read Application keys via SMI
> + *  A (0x4), B (0x8), Internet (0x2), Email (0x1).
> + */
> +static void apanel_poll(struct work_struct *work)
> +{
> +	struct apanel *ap = container_of(work, struct apanel, poll_timer.work);
> +
> +	u8 cmd = devdata[APANEL_DEV_APPBTN] == CHIP_OZ992C ? 0 : 8;
> +	s32 data;
> +
> +	data = i2c_smbus_read_word_data(&ap->client, cmd);
> +	i2c_smbus_write_word_data(&ap->client, cmd, 0);
> +
> +	if (!data)
> +		return;
> +
> +	pr_debug("apanel: app key data=%#x\n", data);
> +	if (data & 0x1)
> +		report_key(ap->input, KEY_EMAIL);
> +	if (data & 0x2)
> +		report_key(ap->input, KEY_WWW);
> +	if (data & 0x4)
> +		report_key(ap->input, KEY_PROG1);
> +	if (data & 0x8)
> +		report_key(ap->input, KEY_PROG2);
> +	if (data & 0x400)
> +		report_key(ap->input, KEY_STOPCD);
> +	if (data & 0x800)
> +		report_key(ap->input, KEY_PLAYPAUSE);
> +	if (data & 0x200)
> +		report_key(ap->input, KEY_REWIND);
> +	if (data & 0x100)
> +		report_key(ap->input, KEY_FORWARD);
> +
> +	schedule_delayed_work(&ap->poll_timer, POLL_FREQUENCY);
> +}
> +
> +/* Track state changes of LED */
> +static void apanel_led(struct work_struct *work)
> +{
> +	struct apanel *ap = container_of(work, struct apanel, led_work);
> +	int onoff = test_bit(LED_MISC, ap->input->led);
> +
> +	pr_debug("apanel: led %s\n", onoff ? "on" : "off");
> +	i2c_smbus_write_word_data(&ap->client, 0x10, onoff ? 0x8000 : 0);
> +}
> +
> +/* Callback from input layer when state change happens.
> + * Used to handle LED control.
> + */
> +static int apanel_event(struct input_dev *dev, unsigned int type,
> +			unsigned int code, int value)
> +{
> +	struct apanel *ap = dev->private;
> +
> +	if (devdata[APANEL_DEV_LED] == CHIP_NONE || type != EV_LED)
> +		return -1;
> +
> +	schedule_work(&ap->led_work);
> +	return 0;
> +}
> +
> +/*
> +  basically this function should probe the i2c client, but we know that it has
> +  to be the one we're looking for - and I have no idea how I should probe for
> +  it, so we just register...
> + */

In fact, no, you don't know for sure if it is the one you're looking
for. You know the I2C address, which is only unique on a given bus.
apanel_attach_adapter() will be called for every i2c bus on the system,
and if another bus happens to have a device at the same address, your
driver will attach to it too, with possibly bad effects.

Right now the i2c-core isn't very friendly with undetectable I2C chips,
I have to admit. The best solution is to have the attach_adapter
function explicitely check the adapter's ID. It should be
I2C_HW_SMBUS_I801 for the i2c-i801 adapter.

(I just noticed that the i2c-i801 driver doesn't actually declare its
ID, although it was reserved in i2c-id.h, so you'll have to fix this
first.)

> +static int struct i2c_adapter *adap, int addr, int kind)
> +{
> +	struct apanel *ap;
> +	struct input_dev *input;
> +	int err = -ENOMEM;
> +
> +	ap = kzalloc(sizeof(*ap), GFP_KERNEL);
> +	if (!ap)
> +		goto out1;
> +
> +	input = input_allocate_device();
> +	if (!input)
> +		goto out1;
> +
> +	ap->input = input;
> +	strcpy(ap->client.name, "apanel");

Please use strlcpy instead, with size = I2C_NAME_SIZE.

> +	ap->client.driver = &apanel_driver;
> +	ap->client.adapter = adap;
> +	ap->client.addr = addr;
> +	INIT_DELAYED_WORK(&ap->poll_timer, apanel_poll);
> +	INIT_WORK(&ap->led_work, apanel_led);
> +	i2c_set_clientdata(&ap->client, ap);
> +
> +	input->name = "Lifebook Panel buttons";
> +	input->phys = "apanel/input0";
> +	input->id.bustype = BUS_HOST;
> +	input->cdev.dev = &ap->client.dev;
> +	input->private = &ap;
> +
> +	if (devdata[APANEL_DEV_APPBTN] != CHIP_NONE) {
> +		input->evbit[LONG(EV_KEY)] = BIT(EV_KEY);
> +		set_bit(KEY_PROG1, input->keybit);
> +		set_bit(KEY_PROG2, input->keybit);
> +		set_bit(KEY_EMAIL, input->keybit);
> +		set_bit(KEY_WWW, input->keybit);
> +	}
> +
> +	if (devdata[APANEL_DEV_CDBTN] != CHIP_NONE) {
> +		input->evbit[LONG(EV_KEY)] = BIT(EV_KEY);
> +		set_bit(KEY_STOPCD, input->keybit);
> +		set_bit(KEY_PLAYPAUSE, input->keybit);
> +		set_bit(KEY_REWIND, input->keybit);
> +		set_bit(KEY_FORWARD, input->keybit);
> +	}
> +
> +	if (devdata[APANEL_DEV_LED] != CHIP_NONE) {
> +		input->event = apanel_event;
> +		input->evbit[0] |= BIT(EV_LED);
> +		input->ledbit[0] = BIT(LED_MISC);
> +	}
> +
> +	err = i2c_attach_client(&ap->client);
> +	if (err)
> +		goto out2;
> +
> +	err = input_register_device(input);
> +	if (err)
> +		goto out3;
> +
> +	schedule_delayed_work(&ap->poll_timer, POLL_FREQUENCY);
> +	return 0;
> +out3:
> +	i2c_detach_client(&ap->client);
> +out2:
> +	kfree(input);
> +out1:

I'd suggest to give these labels names rather than numbers. It's easier
when you later need to add or remove one, and it's less error-prone as
well.

> +	kfree(ap);
> +	return err;
> +}
> +
> +static int apanel_detach_client(struct i2c_client *client)
> +{
> +	struct apanel *ap = i2c_get_clientdata(client);
> +
> +	cancel_rearming_delayed_work(&ap->poll_timer);
> +	input_unregister_device(ap->input);
> +	input_free_device(ap->input);
> +
> +	i2c_detach_client(&ap->client);
> +	kfree(ap);
> +	return 0;
> +}
> +
> +/* this function is invoked for every i2c adapter, that has a device at the
> + * address we've specified
> + */

Not true. This function is invoked for just every i2c adapter. The
address maching is done later by i2c_probe().

> +static int apanel_attach_adapter(struct i2c_adapter *adap)
> +{
> +	/* try to attach on any smbus adapter */
> +	if (adap->algo->smbus_xfer)
> +		return i2c_probe(adap, &addr_data, apanel_probe);

This test is not correct. It will succeed for every i2c_adapter driver
which implements smbus_xfer, which is not relevant at all. From a
technical point of view, what you need to check is that the adapter
supports the transaction types used by the driver, i.e. SMBus Read Word
and SMBus Write Word. This may not be the case even if the adapter
driver implements smbus_xfer, and this may be the case even if the
adapter does not implement smbus_xfer (it can be emulated). A proper
check would be:

	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WORD_DATA))
		return 0;

Then, as suggested above, you should check the adapter ID if you really
only want to attach to a given adapter:

	if (adap->id != I2C_HW_SMBUS_I801)
		return -ENODEV;

However this second check would better be done in apanel_probe(), so
that you can skip the check for forced probes (kind >= 0). If you do
that check unconditionally, the interest of using I2C_CLIENT_INSMOD is
rater limited.

> +
> +	return -ENODEV;
> +}
> +
> +/* scan the system rom for the signature "FJKEYINF" */
> +static __init void __iomem *bios_signature(void)
> +{
> +	void __iomem *bios;
> +	ssize_t offset;
> +	const unsigned char signature[] = "FJKEYINF";
> +
> +	bios = ioremap(0xF0000, 0x10000); /* Can't fail */
> +
> +	for (offset = 0; offset < 0x10000; offset += 0x10) {
> +		if (check_signature(bios + offset, signature,
> +				    sizeof(signature)-1))
> +			return bios + offset;
> +	}
> +
> +	printk(KERN_DEBUG
> +	       "apanel: Fujitsu BIOS signature '%s' not found...\n", signature);

Please use pr_debug() instead.

> +	iounmap(bios);
> +	return NULL;
> +}
> +
> +static int __init apanel_init(void)
> +{
> +	void __iomem *bios;
> +	u8 devno;
> +	int found = 0;
> +
> +	if (!dmi_check_system(apanel_dmi_table)) {
> +		if (dmi_force)
> +			printk(KERN_INFO "apanel: system not found in database forced\n");

Line too long, please split. This can also probably be reworded to
sound better (just adding a comma after "database" may be enough.)
KERN_NOTICE could probably be used here.

> +		else
> +			return -ENODEV;
> +	}
> +
> +	bios = bios_signature();
> +	if (!bios)
> +		return -ENODEV;
> +
> +	bios += 8;
> +
> +	/* just use the first address */
> +	normal_i2c[0] = readb(bios+3) >> 1;
> +
> +	for ( ; (devno = readb(bios)) & 0x7f; bios += 4) {
> +		unsigned char method, slave, chip;
> +
> +		method = readb(bios + 1);
> +		chip = readb(bios + 2);
> +		slave = readb(bios + 3) >> 1;
> +
> +		if (slave != normal_i2c[0]) {
> +			printk(KERN_INFO "apanel: only one SMBus slave "
> +				 "address supported, skiping device...\n");
> +			continue;
> +		}
> +
> +		/* translate alternative device numbers */
> +		switch (devno) {
> +		case 6:
> +			devno = APANEL_DEV_APPBTN;
> +			break;
> +		case 7:
> +			devno = APANEL_DEV_LED;
> +			break;
> +		}
> +
> +		if (devno >= APANEL_DEV_MAX)
> +			printk(KERN_INFO "apanel: unknown device %d found\n", devno);
> +		else if (devdata[devno] != CHIP_NONE)
> +			printk(KERN_INFO "apanel: duplicate entry for device %s\n",
> +			       device_names[devno]);
> +
> +		else if (method != 1 && method != 2 && method != 4) {
> +			printk(KERN_INFO "apanel: unknown access method %u for %s\n",
> +			       method, device_names[devno]);

3 lines too long here, please wrap them to make them fit in 80 columns.

> +		} else {
> +			pr_debug(KERN_DEBUG "apanel: %s found, chip=%d\n",
> +				 device_names[devno], chip);

Drop "KERN_DEBUG", it's already added by pr_debug().

> +
> +			devdata[devno] = (enum apanel_chip) chip;
> +			++found;
> +		}
> +	}
> +	iounmap(bios);
> +
> +	if (found == 0) {
> +		printk(KERN_INFO "apanel: no input devices reported by bios\n");

Is this expected to happen? If not, KERN_NOTICE or even KERN_WARN would
be better.

> +		return -ENODEV;
> +	}
> +	return i2c_add_driver(&apanel_driver);
> +}
> +module_init(apanel_init);
> +
> +static void __exit apanel_cleanup(void)
> +{
> +	i2c_del_driver(&apanel_driver);
> +}
> +module_exit(apanel_cleanup);


-- 
Jean Delvare



More information about the i2c mailing list