Message-Id: <20080227230928.993d424e.akpm@linux-foundation.org>
Date:	Wed, 27 Feb 2008 23:09:28 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	Dmitry Torokhov <dtor@...l.ru>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Liam Girdwood <liam.girdwood@...fsonmicro.com>,
	Graeme Gregory <gg@...nsource.wolfsonmicro.com>,
	Mike Arthur <mike.arthur@...fsonmicro.com>,
	Dmitry Baryshkov <dbaryshkov@...il.com>,
	Stanley Cai <stanley.cai@...el.com>,
	Rodolfo Giometti <giometti@...eenne.com>,
	Russell King <rmk@....linux.org.uk>,
	Pete MacKay <linux01@...hitechnical.net>,
	Marc Kleine-Budde <mkl@...gutronix.de>,
	Ian Molton <spyro@....com>,
	Vincent Sanders <vince@...likki.org>,
	Andrew Zabolotny <zap@...elink.ru>
Subject: Re: [PATCH 1/6] Core driver for WM97xx touchscreens

On Tue, 26 Feb 2008 13:40:13 +0000 Mark Brown <broonie@...nsource.wolfsonmicro.com> wrote:

> This patch series adds support for the touchscreen controllers provided
> by Wolfson Microelectronics WM97xx series chips in both polled and
> streaming modes.
> 
> These drivers have been maintained out of tree since 2003.  During that
> time the driver the primary maintainer was Liam Girdwood and a number of
> people have made contributions including Dmitry Baryshkov, Stanley Cai,
> Rodolfo Giometti, Russell King, Marc Kleine-Budde, Ian Molton, Vincent
> Sanders, Andrew Zabolotny, Graeme Gregory, Mike Arthur and myself.
> Apologies to anyone I have omitted.
> 

Nice looking drivers.

> ...
>
> +/*
> + * Handle a pen down interrupt.
> + */
> +static void wm97xx_pen_irq_worker(struct work_struct *work)
> +{
> +	struct wm97xx *wm = container_of(work, struct wm97xx, pen_event_work);
> +
> +	/* do we need to enable the touch panel reader */
> +	if (wm->id == WM9705_ID2) {
> +		if (wm97xx_reg_read(wm, AC97_WM97XX_DIGITISER_RD) &
> +					WM97XX_PEN_DOWN)
> +			wm->pen_is_down = 1;
> +		else
> +			wm->pen_is_down = 0;
> +	} else {
> +		u16 status, pol;
> +		mutex_lock(&wm->codec_mutex);
> +		status = wm97xx_reg_read(wm, AC97_GPIO_STATUS);
> +		pol = wm97xx_reg_read(wm, AC97_GPIO_POLARITY);
> +
> +		if (WM97XX_GPIO_13 & pol & status) {
> +			wm->pen_is_down = 1;
> +			wm97xx_reg_write(wm, AC97_GPIO_POLARITY, pol &
> +						~WM97XX_GPIO_13);
> +		} else {
> +			wm->pen_is_down = 0;
> +			wm97xx_reg_write(wm, AC97_GPIO_POLARITY, pol |
> +					 WM97XX_GPIO_13);
> +		}
> +
> +		if (wm->id == WM9712_ID2)
> +			wm97xx_reg_write(wm, AC97_GPIO_STATUS, (status &
> +						~WM97XX_GPIO_13) << 1);
> +		else
> +			wm97xx_reg_write(wm, AC97_GPIO_STATUS, status &
> +						~WM97XX_GPIO_13);
> +		mutex_unlock(&wm->codec_mutex);
> +	}
> +
> +	queue_delayed_work(wm->ts_workq, &wm->ts_reader, 0);

This is equvaltnet to queue_work().

Why queue the work instead of just callng it synchronously right here?

> +	if (!wm->pen_is_down && wm->mach_ops && wm->mach_ops->acc_enabled)
> +		wm->mach_ops->acc_pen_up(wm);
> +	wm->mach_ops->irq_enable(wm, 1);
> +}
> +
> +/*
> + * Codec PENDOWN irq handler
> + *
> + * We have to disable the codec interrupt in the handler because it can
> + * take upto 1ms to clear the interrupt source. The interrupt is then enabled
> + * again in the slow handler when the source has been cleared.
> + */
> +static irqreturn_t wm97xx_pen_interrupt(int irq, void *dev_id)
> +{
> +	struct wm97xx *wm = dev_id;
> +	wm->mach_ops->irq_enable(wm, 0);
> +	queue_work(wm->ts_workq, &wm->pen_event_work);
> +	return IRQ_HANDLED;
> +}

We can lose events, can't we?  We only have one work to queue, and if it is
already queued, the queue_work() here is a no-op.

otoh there's no payload so there's actually no data to lose.

Still, this stands out a bit and perhaps a comment which describes the
overall interrupt handling design would help here.

> +/*
> + * initialise pen IRQ handler and workqueue
> + */
> +static int wm97xx_init_pen_irq(struct wm97xx *wm)
> +{
> +	u16 reg;
> +
> +	/* If an interrupt is supplied an IRQ enable operation must also be
> +	 * provided. */
> +	BUG_ON(!wm->mach_ops->irq_enable);
> +
> +	INIT_WORK(&wm->pen_event_work, wm97xx_pen_irq_worker);
> +
> +	if (request_irq(wm->pen_irq, wm97xx_pen_interrupt, IRQF_SHARED,
> +			"wm97xx-pen", wm)) {
> +		dev_err(wm->dev,
> +			"Failed to register pen down interrupt, polling");
> +		wm->pen_irq = 0;
> +		return -EINVAL;
> +	}
> +
> +	/* enable PEN down on wm9712/13 */
> +	if (wm->id != WM9705_ID2) {
> +		reg = wm97xx_reg_read(wm, AC97_MISC_AFE);
> +		wm97xx_reg_write(wm, AC97_MISC_AFE, reg & 0xfffb);
> +		reg = wm97xx_reg_read(wm, 0x5a);
> +		wm97xx_reg_write(wm, 0x5a, reg & ~0x0001);
> +	}
> +
> +	return 0;
> +}
> +
> +static int wm97xx_read_samples(struct wm97xx *wm)
> +{
> +	struct wm97xx_data data;
> +	int rc;
> +
> +	mutex_lock(&wm->codec_mutex);
> +
> +	if (wm->mach_ops && wm->mach_ops->acc_enabled)
> +		rc = wm->mach_ops->acc_pen_down(wm);
> +	else
> +		rc = wm->codec->poll_touch(wm, &data);
> +
> +	if (rc & RC_PENUP) {
> +		if (wm->pen_is_down) {
> +			wm->pen_is_down = 0;
> +			dev_dbg(wm->dev, "pen up\n");
> +			input_report_abs(wm->input_dev, ABS_PRESSURE, 0);
> +			input_sync(wm->input_dev);
> +		} else if (!(rc & RC_AGAIN)) {
> +			/* We need high frequency updates only while
> +			* pen is down, the user never will be able to
> +			* touch screen faster than a few times per
> +			* second... On the other hand, when the user
> +			* is actively working with the touchscreen we
> +			* don't want to lose the quick response. So we
> +			* will slowly increase sleep time after the
> +			* pen is up and quicky restore it to ~one task
> +			* switch when pen is down again.
> +			*/
> +			if (wm->ts_reader_interval < HZ / 10)
> +				wm->ts_reader_interval++;
> +		}
> +
> +	} else if (rc & RC_VALID) {
> +		dev_dbg(wm->dev,
> +			"pen down: x=%x:%d, y=%x:%d, pressure=%x:%d\n",
> +			data.x >> 12, data.x & 0xfff, data.y >> 12,
> +			data.y & 0xfff, data.p >> 12, data.p & 0xfff);
> +		input_report_abs(wm->input_dev, ABS_X, data.x & 0xfff);
> +		input_report_abs(wm->input_dev, ABS_Y, data.y & 0xfff);
> +		input_report_abs(wm->input_dev, ABS_PRESSURE, data.p & 0xfff);
> +		input_sync(wm->input_dev);
> +		wm->pen_is_down = 1;
> +		wm->ts_reader_interval = wm->ts_reader_min_interval;
> +	} else if (rc & RC_PENDOWN) {
> +		dev_dbg(wm->dev, "pen down\n");
> +		wm->pen_is_down = 1;
> +		wm->ts_reader_interval = wm->ts_reader_min_interval;
> +	}
> +
> +	mutex_unlock(&wm->codec_mutex);
> +	return rc;
> +}
> +
> +/*
> +* The touchscreen sample reader.
> +*/
> +static void wm97xx_ts_reader(struct work_struct *work)
> +{
> +	int rc;
> +	struct wm97xx *wm = container_of(work, struct wm97xx, ts_reader.work);
> +
> +	BUG_ON(!wm->codec);
> +
> +	do {
> +		rc = wm97xx_read_samples(wm);
> +	} while (rc & RC_AGAIN);
> +
> +	if (wm->pen_is_down || !wm->pen_irq)
> +		queue_delayed_work(wm->ts_workq, &wm->ts_reader,
> +				   wm->ts_reader_interval);
> +}

again, if this work is already scheduled, your queue_delayed_work() here is
a no-op.  Will things all work correctly?

> +/**
> + *	wm97xx_ts_input_open - Open the touch screen input device.
> + *	@idev:	Input device to be opened.
> + *
> + *	Called by the input sub system to open a wm97xx touchscreen device.
> + *  Starts the touchscreen thread and touch digitiser.

A bit of whitecpace inconsistency there.

I don't understand why people use tabs to indent kerneldoc - all it does is
waste the space where you want to put text.  Shrug.

> + */
> +static int wm97xx_ts_input_open(struct input_dev *idev)
> +{
> +	struct wm97xx *wm = input_get_drvdata(idev);
> +
> +	wm->ts_workq = create_singlethread_workqueue("kwm97xx");
> +	if (wm->ts_workq == NULL) {
> +		dev_err(wm->dev,
> +			"Failed to create workqueue\n");
> +		return -EINVAL;
> +	}
> +
> +	/* start digitiser */
> +	if (wm->mach_ops && wm->mach_ops->acc_enabled)
> +		wm->codec->acc_enable(wm, 1);
> +	wm->codec->dig_enable(wm, 1);
> +
> +	INIT_DELAYED_WORK(&wm->ts_reader, wm97xx_ts_reader);
> +
> +	wm->ts_reader_min_interval = HZ >= 100 ? HZ / 100 : 1;
> +	if (wm->ts_reader_min_interval < 1)
> +		wm->ts_reader_min_interval = 1;
> +	wm->ts_reader_interval = wm->ts_reader_min_interval;
> +
> +	wm->pen_is_down = 0;
> +	if (wm->pen_irq)
> +		wm97xx_init_pen_irq(wm);
> +	else
> +		dev_err(wm->dev, "No IRQ specified\n");
> +
> +	/* If we either don't have an interrupt for pen down events or
> +	 * failed to acquire it then we need to poll.
> +	 */
> +	if (wm->pen_irq == 0)
> +		queue_delayed_work(wm->ts_workq, &wm->ts_reader,
> +				   wm->ts_reader_interval);
> +
> +	return 0;
> +}

So...  this driver continuously polls the hardware at 100HZ?  If so,
powertop users will come after you with pitchforks.

> +/**
> + *	wm97xx_ts_input_close - Close the touch screen input device.
> + *	@idev:	Input device to be closed.
> + *
> + *	Called by the input sub system to close a wm97xx touchscreen device.
> + *  Kills the touchscreen thread and stops the touch digitiser.
> + */
> +
> +static void wm97xx_ts_input_close(struct input_dev *idev)
> +{
> +	struct wm97xx *wm = input_get_drvdata(idev);
> +
> +	if (wm->pen_irq)
> +		free_irq(wm->pen_irq, wm);
> +
> +	wm->pen_is_down = 0;
> +
> +	/* ts_reader rearms itself so we need to explicitly stop it
> +	 * before we destroy the workqueue.
> +	 */
> +	cancel_delayed_work_sync(&wm->ts_reader);
> +	destroy_workqueue(wm->ts_workq);
> +
> +	/* stop digitiser */
> +	wm->codec->dig_enable(wm, 0);
> +	if (wm->mach_ops && wm->mach_ops->acc_enabled)
> +		wm->codec->acc_enable(wm, 0);
> +}
> +
> +static int wm97xx_probe(struct device *dev)
> +{
> +	struct wm97xx *wm;
> +	int ret = 0, id = 0;
> +
> +	wm = kzalloc(sizeof(struct wm97xx), GFP_KERNEL);
> +	if (!wm)
> +		return -ENOMEM;
> +	mutex_init(&wm->codec_mutex);
> +
> +	wm->dev = dev;
> +	dev->driver_data = wm;
> +	wm->ac97 = to_ac97_t(dev);
> +
> +	/* check that we have a supported codec */
> +	id = wm97xx_reg_read(wm, AC97_VENDOR_ID1);
> +	if (id != WM97XX_ID1) {
> +		dev_err(dev, "Device with vendor %04x is not a wm97xx\n", id);
> +		ret = -ENODEV;
> +		goto alloc_err;
> +	}
> +
> +	wm->id = wm97xx_reg_read(wm, AC97_VENDOR_ID2);
> +
> +	dev_info(wm->dev, "detected a wm97%02x codec\n", wm->id & 0xff);
> +
> +	switch (wm->id & 0xff) {
> +#ifdef CONFIG_TOUCHSCREEN_WM9705
> +	case 0x05:
> +		wm->codec = &wm9705_codec;
> +		break;
> +#endif
> +#ifdef CONFIG_TOUCHSCREEN_WM9712
> +	case 0x12:
> +		wm->codec = &wm9712_codec;
> +		break;
> +#endif
> +#ifdef CONFIG_TOUCHSCREEN_WM9713
> +	case 0x13:
> +		wm->codec = &wm9713_codec;
> +		break;
> +#endif

That's a wee bit combersome.  Really a "core" should not know about its
specific clients.  A better and more typical design would have the clients
registering themselves with the core via some registration interface.

> +	default:
> +		dev_err(wm->dev, "Support for wm97%02x not compiled in.\n",
> +			wm->id & 0xff);
> +		ret = -ENODEV;
> +		goto alloc_err;
> +	}
> +
> +	wm->input_dev = input_allocate_device();
> +	if (wm->input_dev == NULL) {
> +		ret = -ENOMEM;
> +		goto alloc_err;
> +	}
> +
> +	/* set up touch configuration */
> +	wm->input_dev->name = "wm97xx touchscreen";

spaces in names are OK here?  They can make proc files basically
unparseable.

> +	wm->input_dev->open = wm97xx_ts_input_open;
> +	wm->input_dev->close = wm97xx_ts_input_close;
> +	set_bit(EV_ABS, wm->input_dev->evbit);
> +	set_bit(ABS_X, wm->input_dev->absbit);
> +	set_bit(ABS_Y, wm->input_dev->absbit);
> +	set_bit(ABS_PRESSURE, wm->input_dev->absbit);
> +	input_set_abs_params(wm->input_dev, ABS_X, abs_x[0], abs_x[1],
> +			     abs_x[2], 0);
> +	input_set_abs_params(wm->input_dev, ABS_Y, abs_y[0], abs_y[1],
> +			     abs_y[2], 0);
> +	input_set_abs_params(wm->input_dev, ABS_PRESSURE, abs_p[0], abs_p[1],
> +			     abs_p[2], 0);
> +	input_set_drvdata(wm->input_dev, wm);
> +	wm->input_dev->dev.parent = dev;
> +	ret = input_register_device(wm->input_dev);
> +	if (ret < 0)
> +		goto dev_alloc_err;
> +
> +	/* set up physical characteristics */
> +	wm->codec->phy_init(wm);
> +
> +	/* load gpio cache */
> +	wm->gpio[0] = wm97xx_reg_read(wm, AC97_GPIO_CFG);
> +	wm->gpio[1] = wm97xx_reg_read(wm, AC97_GPIO_POLARITY);
> +	wm->gpio[2] = wm97xx_reg_read(wm, AC97_GPIO_STICKY);
> +	wm->gpio[3] = wm97xx_reg_read(wm, AC97_GPIO_WAKEUP);
> +	wm->gpio[4] = wm97xx_reg_read(wm, AC97_GPIO_STATUS);
> +	wm->gpio[5] = wm97xx_reg_read(wm, AC97_MISC_AFE);
> +
> +	/* register our battery device */
> +	wm->battery_dev = platform_device_alloc("wm97xx-battery", -1);
> +	if (!wm->battery_dev) {
> +		ret = -ENOMEM;
> +		goto batt_err;
> +	}
> +	platform_set_drvdata(wm->battery_dev, wm);
> +	wm->battery_dev->dev.parent = dev;
> +	ret = platform_device_add(wm->battery_dev);
> +	if (ret < 0)
> +		goto batt_reg_err;
> +
> +	/* register our extended touch device (for machine specific
> +	 * extensions) */
> +	wm->touch_dev = platform_device_alloc("wm97xx-touch", -1);
> +	if (!wm->touch_dev) {
> +		ret = -ENOMEM;
> +		goto touch_err;
> +	}
> +	platform_set_drvdata(wm->touch_dev, wm);
> +	wm->touch_dev->dev.parent = dev;
> +	ret = platform_device_add(wm->touch_dev);
> +	if (ret < 0)
> +		goto touch_reg_err;
> +
> +	return ret;
> +
> + touch_reg_err:
> +	platform_device_put(wm->touch_dev);
> + touch_err:
> +	platform_device_unregister(wm->battery_dev);
> +	wm->battery_dev = NULL;
> + batt_reg_err:
> +	platform_device_put(wm->battery_dev);
> + batt_err:
> +	input_unregister_device(wm->input_dev);
> +	wm->input_dev = NULL;
> + dev_alloc_err:
> +	input_free_device(wm->input_dev);
> + alloc_err:
> +	kfree(wm);
> +
> +	return ret;
> +}

The kernel->userspace interface looks fairly complex.  Is it documented
anywhere?

> +static int wm97xx_remove(struct device *dev)
> +{
> +	struct wm97xx *wm = dev_get_drvdata(dev);
> +
> +	platform_device_unregister(wm->battery_dev);
> +	platform_device_unregister(wm->touch_dev);
> +	input_unregister_device(wm->input_dev);
> +
> +	kfree(wm);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int wm97xx_resume(struct device *dev)
> +{
> +	struct wm97xx *wm = dev_get_drvdata(dev);
> +
> +	/* restore digitiser and gpios */
> +	if (wm->id == WM9713_ID2) {
> +		wm97xx_reg_write(wm, AC97_WM9713_DIG1, wm->dig[0]);
> +		wm97xx_reg_write(wm, 0x5a, wm->misc);
> +		if (wm->input_dev->users) {
> +			u16 reg;
> +			reg = wm97xx_reg_read(wm, AC97_EXTENDED_MID) & 0x7fff;
> +			wm97xx_reg_write(wm, AC97_EXTENDED_MID, reg);
> +		}
> +	}
> +
> +	wm97xx_reg_write(wm, AC97_WM9713_DIG2, wm->dig[1]);
> +	wm97xx_reg_write(wm, AC97_WM9713_DIG3, wm->dig[2]);
> +
> +	wm97xx_reg_write(wm, AC97_GPIO_CFG, wm->gpio[0]);
> +	wm97xx_reg_write(wm, AC97_GPIO_POLARITY, wm->gpio[1]);
> +	wm97xx_reg_write(wm, AC97_GPIO_STICKY, wm->gpio[2]);
> +	wm97xx_reg_write(wm, AC97_GPIO_WAKEUP, wm->gpio[3]);
> +	wm97xx_reg_write(wm, AC97_GPIO_STATUS, wm->gpio[4]);
> +	wm97xx_reg_write(wm, AC97_MISC_AFE, wm->gpio[5]);
> +
> +	return 0;
> +}
> +
> +#else
> +#define wm97xx_resume		NULL
> +#endif

It's strange to see a .resume but no .suspend.

> +/*
> + * Machine specific operations
> + */
> +int wm97xx_register_mach_ops(struct wm97xx *wm,
> +			     struct wm97xx_mach_ops *mach_ops)
> +{
> +	mutex_lock(&wm->codec_mutex);
> +	if (wm->mach_ops) {
> +		mutex_unlock(&wm->codec_mutex);
> +		return -EINVAL;
> +	}
> +	wm->mach_ops = mach_ops;
> +	mutex_unlock(&wm->codec_mutex);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(wm97xx_register_mach_ops);
> +
> +void wm97xx_unregister_mach_ops(struct wm97xx *wm)
> +{
> +	mutex_lock(&wm->codec_mutex);
> +	wm->mach_ops = NULL;
> +	mutex_unlock(&wm->codec_mutex);
> +}
> +EXPORT_SYMBOL_GPL(wm97xx_unregister_mach_ops);
> +
> +static struct device_driver wm97xx_driver = {
> +	.name = 	"ac97",
> +	.bus = 		&ac97_bus_type,
> +	.owner = 	THIS_MODULE,
> +	.probe = 	wm97xx_probe,
> +	.remove = 	wm97xx_remove,
> +	.resume =	wm97xx_resume,
> +};
> +
> +static int __init wm97xx_init(void)
> +{
> +	return driver_register(&wm97xx_driver);
> +}
> +
> +static void __exit wm97xx_exit(void)
> +{
> +	driver_unregister(&wm97xx_driver);
> +}
> +
> +module_init(wm97xx_init);
> +module_exit(wm97xx_exit);
> +
> +/* Module information */
> +MODULE_AUTHOR("Liam Girdwood <liam.girdwood@...fsonmicro.com>");
> +MODULE_DESCRIPTION("WM97xx Core - Touch Screen / AUX ADC / GPIO Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/wm97xx.h b/include/linux/wm97xx.h
> new file mode 100644
> index 0000000..f0d9fc0
> --- /dev/null
> +++ b/include/linux/wm97xx.h
> @@ -0,0 +1,308 @@
> +
> +/*
> + * Register bits and API for Wolfson WM97xx series of codecs
> + */
> +
> +#ifndef _LINUX_WM97XX_H
> +#define _LINUX_WM97XX_H
> +
> +#include <sound/core.h>
> +#include <sound/pcm.h>
> +#include <sound/ac97_codec.h>
> +#include <sound/initval.h>
> +#include <linux/types.h>
> +#include <linux/list.h>
> +#include <linux/input.h>	/* Input device layer */
> +#include <linux/platform_device.h>
> +
> +/*
> + * WM97xx AC97 Touchscreen registers
> + */

ac97 stuff?  But I saw no appropriate dependencies for this in the Kconfig
patch?

> +/*
> + * Codec GPIO status
> + */
> +enum wm97xx_gpio_status {
> +    WM97XX_GPIO_HIGH,
> +    WM97XX_GPIO_LOW
> +};
> +
> +/*
> + * Codec GPIO direction
> + */
> +enum wm97xx_gpio_dir {
> +    WM97XX_GPIO_IN,
> +    WM97XX_GPIO_OUT
> +};
> +
> +/*
> + * Codec GPIO polarity
> + */
> +enum wm97xx_gpio_pol {
> +    WM97XX_GPIO_POL_HIGH,
> +    WM97XX_GPIO_POL_LOW
> +};
> +
> +/*
> + * Codec GPIO sticky
> + */
> +enum wm97xx_gpio_sticky {
> +    WM97XX_GPIO_STICKY,
> +    WM97XX_GPIO_NOTSTICKY
> +};
> +
> +/*
> + * Codec GPIO wake
> + */
> +enum wm97xx_gpio_wake {
> +    WM97XX_GPIO_WAKE,
> +    WM97XX_GPIO_NOWAKE
> +};

Are these gpio's really gp?  If so, should this driver be using the
drivers/gpio/ infrastructure?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists