Re: [PATCH v6] input: keyboard: Add keys driver for the LPC32xx SoC
flat view
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2012-06-25 07:06:24
Also in:
linux-arm-kernel, linux-devicetree, lkml
HI Roland, On Sun, Jun 24, 2012 at 04:44:38PM +0200, Roland Stigge wrote:
This patch adds a driver for the key scan interface of the LPC32xx SoC Signed-off-by: Roland Stigge <redacted>
This looks _very_ good, I just have a couple more comments:
quoted hunk ↗ jump to hunk
+ +static int lpc32xx_kscan_open(struct input_dev *dev) +{ + struct lpc32xx_kscan_drv *kscandat = input_get_drvdata(dev); + + return clk_prepare_enable(kscandat->clk);
Don't you need to clear IRQ here, the same way you do it in resume?
quoted hunk ↗ jump to hunk
+} + +static void lpc32xx_kscan_close(struct input_dev *dev) +{ + struct lpc32xx_kscan_drv *kscandat = input_get_drvdata(dev); + + clk_disable_unprepare(kscandat->clk);
And same here.
quoted hunk ↗ jump to hunk
+} + +int lpc32xx_parse_dt(struct platform_device *pdev)
Static? Also why don't you pass kscandat instead of platform device?
quoted hunk ↗ jump to hunk
+{ + struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev); + struct device_node *np = pdev->dev.of_node; + u32 rows, columns; + + of_property_read_u32(np, "keypad,num-rows", &rows); + of_property_read_u32(np, "keypad,num-columns", &columns); + if (!rows || rows != columns) { + dev_err(&pdev->dev, + "rows and columns must be specified and be equal!\n"); + return -EINVAL; + } + + kscandat->matrix_sz = rows; + kscandat->row_shift = get_count_order(columns); + + of_property_read_u32(np, "nxp,debounce-delay-ms", &kscandat->deb_clks); + of_property_read_u32(np, "nxp,scan-delay-ms", &kscandat->scan_delay); + if (!kscandat->deb_clks || !kscandat->scan_delay) { + dev_err(&pdev->dev, "debounce or scan delay not specified\n"); + return -ENXIO; + } + + kscandat->keymap = kzalloc(sizeof(kscandat->keymap[0]) * + (rows << kscandat->row_shift), GFP_KERNEL); + if (!kscandat->keymap) { + dev_err(&pdev->dev, "could not allocate memory for keymap\n"); + return -ENOMEM; + }
I think this allocation belongs to the probe().
quoted hunk ↗ jump to hunk
+ + return 0; +} + +static int __devinit lpc32xx_kscan_probe(struct platform_device *pdev) +{ + struct lpc32xx_kscan_drv *kscandat; + struct resource *res; + int error; + int irq; + + kscandat = kzalloc(sizeof(struct lpc32xx_kscan_drv), GFP_KERNEL); + if (!kscandat) { + dev_err(&pdev->dev, "failed to allocate memory\n"); + return -ENOMEM; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + dev_err(&pdev->dev, "failed to get platform I/O memory\n"); + error = -EBUSY; + goto out1; + } + + res = request_mem_region(res->start, resource_size(res), pdev->name); + if (!res) { + dev_err(&pdev->dev, "failed to request I/O memory\n"); + error = -EBUSY; + goto out1; + } + kscandat->io_p_start = res->start; + kscandat->io_p_size = resource_size(res); + + kscandat->kscan_base = ioremap(res->start, resource_size(res)); + if (!kscandat->kscan_base) { + dev_err(&pdev->dev, "failed to remap I/O memory\n"); + error = -EBUSY; + goto out2; + } + + /* Get the key scanner clock */ + kscandat->clk = clk_get(&pdev->dev, NULL); + if (IS_ERR(kscandat->clk)) { + dev_err(&pdev->dev, "failed to get clock\n"); + error = PTR_ERR(kscandat->clk); + goto out3; + } + clk_enable(kscandat->clk);
Why not clk_prepare_enable()? Also I think you need to move it down, to where you actually write to the device's registers.
quoted hunk ↗ jump to hunk
+ + irq = platform_get_irq(pdev, 0); + if ((irq < 0) || (irq >= NR_IRQS)) { + dev_err(&pdev->dev, "failed to get platform irq\n"); + error = -EINVAL; + goto out4; + } + error = request_irq(irq, lpc32xx_kscan_irq, 0, pdev->name, kscandat); + if (error) { + dev_err(&pdev->dev, "failed to request irq\n"); + goto out4; + } + + kscandat->input = input_allocate_device(); + if (kscandat->input == NULL) { + dev_err(&pdev->dev, "failed to allocate device\n"); + error = -ENOMEM; + goto out5; + }
This might be too late - IRQ is alreday registered and clock is enabled - what is to stop interrupts to be delivered? I'd allocate the input device together with kscandat structure.
quoted hunk ↗ jump to hunk
+ + platform_set_drvdata(pdev, kscandat); + + error = lpc32xx_parse_dt(pdev); + if (error) { + dev_err(&pdev->dev, "failed to parse device tree\n"); + goto out6; + } + + /* Setup key input */ + kscandat->input->evbit[0] = BIT_MASK(EV_KEY); + kscandat->input->name = pdev->name; + kscandat->input->phys = "matrix-keys/input0"; + kscandat->input->id.vendor = 0x0001; + kscandat->input->id.product = 0x0001; + kscandat->input->id.version = 0x0100; + kscandat->input->open = lpc32xx_kscan_open; + kscandat->input->close = lpc32xx_kscan_close; + kscandat->input->dev.parent = &pdev->dev; + + error = matrix_keypad_build_keymap(NULL, NULL, kscandat->matrix_sz, + kscandat->matrix_sz, + kscandat->keymap, kscandat->input); + if (error) { + dev_err(&pdev->dev, "failed to build keymap\n"); + goto out6; + } + + input_set_drvdata(kscandat->input, kscandat); + input_set_capability(kscandat->input, EV_MSC, MSC_SCAN); + + error = input_register_device(kscandat->input); + if (error) { + dev_err(&pdev->dev, "failed to register input device\n"); + goto out6; + } +
I think this needs to be done before you register input device as as soon as it is registered it may be used.
quoted hunk ↗ jump to hunk
+ /* Configure the key scanner */ + writel(kscandat->deb_clks, LPC32XX_KS_DEB(kscandat->kscan_base)); + writel(kscandat->scan_delay, LPC32XX_KS_SCAN_CTL(kscandat->kscan_base)); + writel(LPC32XX_KSCAN_FTST_USE32K_CLK, + LPC32XX_KS_FAST_TST(kscandat->kscan_base)); + writel(kscandat->matrix_sz, + LPC32XX_KS_MATRIX_DIM(kscandat->kscan_base)); + writel(1, LPC32XX_KS_IRQ(kscandat->kscan_base)); + clk_disable(kscandat->clk);
clk_disable_unprepare()?
quoted hunk ↗ jump to hunk
+ return 0; + +out6: + input_free_device(kscandat->input); +out5: + free_irq(irq, pdev); +out4: + clk_disable(kscandat->clk); + clk_put(kscandat->clk); +out3: + iounmap(kscandat->kscan_base); +out2: + release_mem_region(kscandat->io_p_start, kscandat->io_p_size); +out1: + kfree(kscandat); + + return error; +} + +static int __devexit lpc32xx_kscan_remove(struct platform_device *pdev) +{ + struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev); + + input_unregister_device(kscandat->input);
Are we certain it is safe to free input device before freeing IRQ? I see we have close(), still I'd feel safer if you swapper unregister and free_irq().
quoted hunk ↗ jump to hunk
+ free_irq(platform_get_irq(pdev, 0), pdev); + clk_put(kscandat->clk); + iounmap(kscandat->kscan_base); + release_mem_region(kscandat->io_p_start, kscandat->io_p_size); + kfree(kscandat); + + return 0; +} + +#ifdef CONFIG_PM_SLEEP +static int lpc32xx_kscan_suspend(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev); + + /* Clear IRQ and disable clock */ + writel(1, LPC32XX_KS_IRQ(kscandat->kscan_base)); + clk_disable_unprepare(kscandat->clk); +
This may race with open()/close(). You need to take input_dev->mutex and avoid touching the device if it sis not opened (users == 0).
quoted hunk ↗ jump to hunk
+ return 0; +} + +static int lpc32xx_kscan_resume(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev); + + /* Enable clock and clear IRQ */ + clk_prepare_enable(kscandat->clk); + writel(1, LPC32XX_KS_IRQ(kscandat->kscan_base)); +
Same here. Thanks! -- Dmitry