Re: [PATCH v3] input: tegra-kbc - Add tegra keyboard driver

[Trilok Soni]

Hi Rakesh,

On 1/7/2011 11:15 PM, riyer@xxxxxxxxxx wrote:
> From: Rakesh Iyer <riyer@xxxxxxxxxx>
> 
> This patch adds support for the internal matrix keyboard controller for
> Nvidia Tegra platforms.
> 
> Signed-off-by: Rakesh Iyer <riyer@xxxxxxxxxx>
> ---
> Removed NULL check before input_free_device as suggested by Jesper Juhl.

Sorry for the delay. Few comments.

> +
> +#include <linux/module.h>
> +#include <linux/input.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/clk.h>
> +#include <linux/slab.h>
> +#include <mach/clk.h>

You may not need this.

>
> +
> +static void tegra_kbc_keypress_timer(unsigned long data)
> +{
> +	struct tegra_kbc *kbc = (struct tegra_kbc *)data;
> +	unsigned long flags;
> +	u32 val;
> +	int i;
> +

...

> +	}
> +	return;

No need.

> +}
> +
> +
> +
> +
> +static int tegra_kbc_open(struct input_dev *dev)
> +{
> +	struct tegra_kbc *kbc = input_get_drvdata(dev);
> +	unsigned long flags;
> +	unsigned int debounce_cnt;
> +	u32 val = 0;
> +
> +	clk_enable(kbc->clk);
> +
> +	/* Reset the KBC controller to clear all previous status.*/
> +	tegra_periph_reset_assert(kbc->clk);
> +	udelay(100);
> +	tegra_periph_reset_deassert(kbc->clk);
> +	udelay(100);
> +
> +	tegra_kbc_config_pins(kbc);
> +	tegra_kbc_setup_wakekeys(kbc, false);
> +
> +	writel(kbc->pdata->repeat_cnt, kbc->mmio + KBC_RPT_DLY_0);
> +
> +	debounce_cnt = min_t(unsigned int, kbc->pdata->debounce_cnt, 0x3fful);
> +	val = debounce_cnt << 4;
> +	val |= 1<<14; /* fifo interrupt threshold = 1 entry */
> +	val |= 1<<3;  /* interrupt on FIFO threshold reached */

As mentioned below, try to get driver out of these magic nos.

> +	val |= 1;     /* enable */
> +	writel(val, kbc->mmio + KBC_CONTROL_0);
> +
> +	/* Compute the delay(ns) from interrupt mode to continuous polling mode
> +	 * so the timer routine is scheduled appropriately. */
> +	val = readl(kbc->mmio + KBC_INIT_DLY_0);
> +	kbc->cp_dly_jiffies = usecs_to_jiffies((val & 0xfffff) * 32);
> +
> +	/* atomically clear out any remaining entries in the key FIFO
> +	 * and enable keyboard interrupts */
> +	spin_lock_irqsave(&kbc->lock, flags);
> +	while (1) {
> +		val = readl(kbc->mmio + KBC_INT_0);
> +		val >>= 4;
> +		if (val) {
> +			val = readl(kbc->mmio + KBC_KP_ENT0_0);
> +			val = readl(kbc->mmio + KBC_KP_ENT1_0);
> +		} else {
> +			break;
> +		}
> +	}
> +	writel(0x7, kbc->mmio + KBC_INT_0);
> +	spin_unlock_irqrestore(&kbc->lock, flags);
> +
> +	return 0;
> +}
> +
> +

> +
> +static irqreturn_t tegra_kbc_isr(int irq, void *args)
> +{
> +	struct tegra_kbc *kbc = args;
> +	u32 val, ctl;
> +
> +	/* until all keys are released, defer further processing to
> +	 * the polling loop in tegra_kbc_keypress_timer */
> +	ctl = readl(kbc->mmio + KBC_CONTROL_0);
> +	ctl &= ~(1<<3);

No magic nos. please. Add relevant #defines for it.

> +	writel(ctl, kbc->mmio + KBC_CONTROL_0);
> +
> +	/* quickly bail out & reenable interrupts if the interrupt source
> +	 * wasn't fifo count threshold */

"wasn't equal to fifo count threshold" ?

> +	val = readl(kbc->mmio + KBC_INT_0);
> +	writel(val, kbc->mmio + KBC_INT_0);
> +
> +	if (!(val & (1<<2))) {
> +		ctl |= 1<<3;

As mentioned above, no magic nos. please.

> +		writel(ctl, kbc->mmio + KBC_CONTROL_0);
> +		return IRQ_HANDLED;
> +	}
> +
> +	/* Schedule timer to run when hardware is in continuous polling mode. */
> +	mod_timer(&kbc->timer, jiffies + kbc->cp_dly_jiffies);
> +	return IRQ_HANDLED;
> +}
> +
> +static int __devinit tegra_kbc_probe(struct platform_device *pdev)
> +{
> +	struct tegra_kbc *kbc;
> +	const struct tegra_kbc_platform_data *pdata = pdev->dev.platform_data;
> +	struct resource *res;
> +	int irq;
> +	int err;
> +	int rows[KBC_MAX_ROW];
> +	int cols[KBC_MAX_COL];
> +	int i, j;
> +	int nr = 0;
> +	unsigned int debounce_cnt;
> +
> +	if (!pdata)
> +		return -EINVAL;
> +
> +	kbc = kzalloc(sizeof(*kbc), GFP_KERNEL);
> +	if (!kbc)
> +		return -ENOMEM;
> +
> +	kbc->pdata = pdata;
> +	kbc->irq = -EINVAL;

You don't need such assignments for irq.

> +
> +	memset(rows, 0, sizeof(rows));
> +	memset(cols, 0, sizeof(cols));
> +
> +	kbc->idev = input_allocate_device();
> +	if (!kbc->idev) {
> +		err = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	debounce_cnt = min_t(unsigned int, pdata->debounce_cnt, 0x3fful);
> +	kbc->repoll_time = 5 + (16 + debounce_cnt) * nr + pdata->repeat_cnt;
> +	kbc->repoll_time = (kbc->repoll_time + 31) / 32;

Care to add note on this equation? Lot's of magic nos. used in this, so they better
get documented. 

> +
> +	kbc->idev->evbit[0] = BIT_MASK(EV_KEY);
> +
> +	/* Override the default keycodes with the board supplied ones. */
> +	if (pdata->plain_keycode) {
> +		kbc->plain_keycode = pdata->plain_keycode;
> +		kbc->fn_keycode = pdata->fn_keycode;
> +	} else {
> +		kbc->plain_keycode = plain_kbd_keycode;
> +		kbc->fn_keycode = fn_kbd_keycode;
> +	}
> +
> +	for (i = 0; i < KBC_MAX_COL; i++) {
> +		if (!cols[i])
> +			continue;
> +		for (j = 0; j < KBC_MAX_ROW; j++) {
> +			int keycode;
> +
> +			if (!rows[j])
> +				continue;
> +
> +			/* enable all the mapped keys. */
> +			keycode = tegra_kbc_keycode(kbc, j, i, false);
> +			if (keycode != -1)
> +				set_bit(keycode, kbc->idev->keybit);
> +
> +			keycode = tegra_kbc_keycode(kbc, j, i, true);
> +			if (keycode != -1)
> +				set_bit(keycode, kbc->idev->keybit);
> +		}
> +	}
> +
> +	setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc);
> +	/* Initialize the FIFO to invalid entries */
> +	for (i = 0; i < ARRAY_SIZE(kbc->fifo); i++)
> +		kbc->fifo[i] = -1;
> +
> +	/* keycode FIFO needs to be read atomically; leave local
> +	 * interrupts disabled when handling KBC interrupt */
> +	err = request_irq(irq, tegra_kbc_isr, IRQF_TRIGGER_HIGH,
> +		pdev->name, kbc);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to request keypad IRQ\n");
> +		goto fail;
> +	}
> +	kbc->irq = irq;
> +
> +	err = input_register_device(kbc->idev);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to register input device\n");
> +		goto fail;
> +	}
> +
> +	device_init_wakeup(&pdev->dev, pdata->wakeup);
> +	return 0;
> +
> +fail:

It is better if you use multiple labels with differing names, as it will
really improve your error patch and you don't need such if (...) checks against
each of these resource release operations then. Please check other drivers..

> +	input_free_device(kbc->idev);
> +	if (kbc->irq >= 0)
> +		free_irq(kbc->irq, pdev);
> +	if (kbc->clk)
> +		clk_put(kbc->clk);
> +	if (kbc->mmio)
> +		iounmap(kbc->mmio);



> +	kfree(kbc);
> +	return err;
> +}
> +
> +#ifdef CONFIG_PM
> +static int tegra_kbc_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct tegra_kbc *kbc = platform_get_drvdata(pdev);
> +
> +	if (device_may_wakeup(&pdev->dev)) {
> +		tegra_kbc_setup_wakekeys(kbc, true);
> +		enable_irq_wake(kbc->irq);
> +		/* Forcefully clear the interrupt status */
> +		writel(0x7, kbc->mmio + KBC_INT_0);
> +		msleep(30);
> +	} else {
> +		mutex_lock(&kbc->idev->mutex);
> +		tegra_kbc_close(kbc->idev);

Only if there are any users. Please add that check.

> +		mutex_unlock(&kbc->idev->mutex);
> +	}
> +
> +	return 0;
> +}
> +
> +static int tegra_kbc_resume(struct platform_device *pdev)
> +{
> +	struct tegra_kbc *kbc = platform_get_drvdata(pdev);
> +	int err = 0;
> +
> +	if (device_may_wakeup(&pdev->dev)) {
> +		disable_irq_wake(kbc->irq);
> +		tegra_kbc_setup_wakekeys(kbc, false);
> +	} else if (kbc->idev->users) {
> +		mutex_lock(&kbc->idev->mutex);
> +		err = tegra_kbc_open(kbc->idev);

As mentioned above.

> +		mutex_unlock(&kbc->idev->mutex);
> +	}
> +
> +	return err;
> +}
> +#endif
> +


-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/