From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751992AbbIQNK0 (ORCPT ); Thu, 17 Sep 2015 09:10:26 -0400 Received: from down.free-electrons.com ([37.187.137.238]:60392 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751059AbbIQNKY (ORCPT ); Thu, 17 Sep 2015 09:10:24 -0400 Date: Thu, 17 Sep 2015 15:05:20 +0200 From: Maxime Ripard To: yassinjaffer@gmail.com Cc: linux-sunxi@googlegroups.com, dmitry.torokhov@gmail.com, linux-input@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/4] input: Add new sun4i-keypad driver Message-ID: <20150917130520.GD4684@lukather> References: <1442325957-10102-1-git-send-email-yassinjaffer@gmail.com> <1442325957-10102-4-git-send-email-yassinjaffer@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="27KoNqt0fmcl1zj/" Content-Disposition: inline In-Reply-To: <1442325957-10102-4-git-send-email-yassinjaffer@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --27KoNqt0fmcl1zj/ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Wed, Sep 16, 2015 at 12:05:56AM +1000, yassinjaffer@gmail.com wrote: > From: Yassin Jaffer >=20 > Allwinnner SUN4i Keypad controller is used to interface a SoC > with a matrix-typekeypad device. > The keypad controller supports multiple row and column lines. > A key can be placed at each intersection of a unique > row and a unique column. > The keypad controller can sense a key-press and key-release and report the > event using a interrupt to the cpu. > This patch adds a driver support to this. > The keypad controller driver does not give proper information > if more that two keys are selected. >=20 > Signed-off-by: Yassin Jaffer > --- > drivers/input/keyboard/Kconfig | 11 ++ > drivers/input/keyboard/Makefile | 1 + > drivers/input/keyboard/sun4i-keypad.c | 361 ++++++++++++++++++++++++++++= ++++++ > 3 files changed, 373 insertions(+) > create mode 100644 drivers/input/keyboard/sun4i-keypad.c >=20 > diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kcon= fig > index 2e80107..4f2f3f8 100644 > --- a/drivers/input/keyboard/Kconfig > +++ b/drivers/input/keyboard/Kconfig > @@ -590,6 +590,17 @@ config KEYBOARD_SUN4I_LRADC > To compile this driver as a module, choose M here: the > module will be called sun4i-lradc-keys. > =20 > +config KEYBOARD_SUN4I_KEYPAD > + tristate "Allwinner sun4i keypad support" > + depends on ARCH_SUNXI Is this IP found on all the know SoCs, or just a subset of them? You probably want to add || COMPILE_TEST in that depends on too. > + select INPUT_MATRIXKMAP > + help > + This selects support for the Allwinner keypad > + on Allwinner sunxi SoCs. > + > + To compile this driver as a module, choose M here: the > + module will be called sun4i-keypad. > + > config KEYBOARD_DAVINCI > tristate "TI DaVinci Key Scan" > depends on ARCH_DAVINCI_DM365 > diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Mak= efile > index 1d416dd..d9f54b4 100644 > --- a/drivers/input/keyboard/Makefile > +++ b/drivers/input/keyboard/Makefile > @@ -57,6 +57,7 @@ obj-$(CONFIG_KEYBOARD_STMPE) +=3D stmpe-keypad.o > obj-$(CONFIG_KEYBOARD_STOWAWAY) +=3D stowaway.o > obj-$(CONFIG_KEYBOARD_ST_KEYSCAN) +=3D st-keyscan.o > obj-$(CONFIG_KEYBOARD_SUN4I_LRADC) +=3D sun4i-lradc-keys.o > +obj-$(CONFIG_KEYBOARD_SUN4I_KEYPAD) +=3D sun4i-keypad.o > obj-$(CONFIG_KEYBOARD_SUNKBD) +=3D sunkbd.o > obj-$(CONFIG_KEYBOARD_TC3589X) +=3D tc3589x-keypad.o > obj-$(CONFIG_KEYBOARD_TEGRA) +=3D tegra-kbc.o > diff --git a/drivers/input/keyboard/sun4i-keypad.c b/drivers/input/keyboa= rd/sun4i-keypad.c > new file mode 100644 > index 0000000..995f9665 > --- /dev/null > +++ b/drivers/input/keyboard/sun4i-keypad.c > @@ -0,0 +1,361 @@ > +/* > + * Allwinner sun4i keypad Controller driver > + * > + * Copyright (C) 2015 Yassin Jaffer > + * > + * Parts of this software are based on (derived from): > + * Copyright (C) 2013-2015 liming@allwinnertech.com, > + * qys > + * > + * 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. > + * > + * 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. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* > + * Keypad Controller registers > + */ > +#define KP_CTL 0x00 /* Keypad Control register */ > +#define KP_TIMING 0x04 /* Keypad Timing register */ > +#define KP_INT_CFG 0x08 /* Keypad interrupt Config register */ > +#define KP_INT_STA 0x0c /* Keypad interrupt Status register */ > + > +#define KP_IN_OFFSET 0x10 /* Keypad Input Data register 0 */ > +#define KP_INX_OFFSET(reg_n) (KP_IN_OFFSET + 4 * (reg_n)) > + > +/* KP_CTL bits */ > +#define ENABLE(x) ((x) << 0) > +#define COLMASK(x) ((~x & 0xff) << 8) > +#define ROWMASK(x) ((~x & 0xff) << 16) Having the name of the register in that define would be great, to spot more easily issues (writing a value to another register, for example). Something like KP_CTL_ENABLE(x) in this case. > +/* KP_TIMING bits */ > +#define SCAN_CYCLE(x) ((x) << 0) > +#define DBC_CYCLE(x) ((x) << 16) > + > +/* KP_INT_CFG bits */ > +#define KP_IRQ_FEDGE BIT(0) > +#define KP_IRQ_REDGE BIT(1) > + > +/* KP_INT_STA bits */ > +#define KP_STA_FEDGE BIT(0) > +#define KP_STA_REDGE BIT(1) > + > +#define KP_MAX_ROWS 8 > +#define KP_MAX_COLS 8 > +#define N_ROWS_REG 2 > +#define KP_ROW_SHIFT 3 > +#define KP_BIT_SHIFT 32 > + > +#define MAX_MATRIX_KEY_NUM (KP_MAX_ROWS * KP_MAX_COLS) > + > +#define KP_BASE_CLK 1000000 > +#define MIN_CYCLE 0x10 > +#define MIN_SCAN_CYCLE 0x100 > +#define MIN_DBC_CYCLE 0x200 > + > +/* > + * keypad Controller structure: stores sunxi keypad controller informati= on > + * > + * @dev: parent device > + * @input: pointer to input device object > + * @apb_clk: keypad Controller APB clock > + * @clk: keypad Controller mod clock > + * @base: keypad controller registers > + * @irq: interrupt > + * @rows_en_mask: Masks for enabled rows > + * @cols_en_mask: Masks for enabled cols > + * @keymap: matrix scan code table for keycodes > + * @key_press_state: cached keys press state > + * @debounce_cycl: keypad specific debounce cycle > + * @scan_cycl: keypad specific scan cycle > + * @autorepeat: flag for auto repetition > + */ > +struct sun4i_keypad_data { > + struct device *dev; > + struct input_dev *input; > + > + struct clk *apb_clk; > + struct clk *clk; > + void __iomem *base; > + int irq; > + > + unsigned short rows_en_mask; > + unsigned short cols_en_mask; > + > + unsigned short keymap[MAX_MATRIX_KEY_NUM]; > + > + unsigned int press_state[N_ROWS_REG]; > + > + unsigned int debounce_cycl; > + unsigned int scan_cycl; > + bool autorepeat; > +}; > + > +static void sun4i_keypad_scan(struct sun4i_keypad_data *keypad, bool edg= e) > +{ > + struct input_dev *input_dev =3D keypad->input; > + u32 key_scan[N_ROWS_REG]; > + unsigned long change; > + unsigned short code; > + int reg_nr, bit_nr, key_up; > + > + for (reg_nr =3D 0; reg_nr < N_ROWS_REG; reg_nr++) { > + key_scan[reg_nr] =3D ~(readl(keypad->base + > + KP_INX_OFFSET(reg_nr))); > + > + change =3D edge ? key_scan[reg_nr] : > + keypad->press_state[reg_nr] ^ key_scan[reg_nr]; > + > + key_up =3D edge || (change & keypad->press_state[reg_nr]); > + > + for_each_set_bit(bit_nr, &change, BITS_PER_LONG) { > + code =3D keypad->keymap[KP_BIT_SHIFT * reg_nr + bit_nr]; > + input_report_key(input_dev, code, key_up); > + input_sync(input_dev); I don't think you need to do an input_sync call each time for eah input_report_key. > + } > + > + keypad->press_state[reg_nr] =3D edge ? 0 : key_scan[reg_nr]; > + } > +} > + > +static irqreturn_t sun4i_keypad_irq(int irq, void *dev_id) > +{ > + struct sun4i_keypad_data *keypad =3D dev_id; > + u32 intr_status; > + > + intr_status =3D readl(keypad->base + KP_INT_STA); > + > + /* release only gives a valid information for a single key release */ > + if (intr_status & KP_STA_REDGE) > + sun4i_keypad_scan(keypad, true); > + > + /* press does not give valid information when > + * multiple rows are selected > + */ /* * This is the multi-line comment style */ > + if (intr_status & KP_STA_FEDGE) > + sun4i_keypad_scan(keypad, false); > + > + writel(intr_status, keypad->base + KP_INT_STA); > + > + return IRQ_HANDLED; > +} > + > +static int sun4i_keypad_open(struct input_dev *dev) > +{ > + struct sun4i_keypad_data *keypad =3D input_get_drvdata(dev); > + int reg_nr; > + > + if (clk_prepare_enable(keypad->apb_clk)) > + return -EINVAL; > + > + if (clk_prepare_enable(keypad->clk)) { > + clk_disable_unprepare(keypad->apb_clk); > + return -EINVAL; > + } > + > + for (reg_nr =3D 0; reg_nr < N_ROWS_REG; reg_nr++) > + keypad->press_state[reg_nr] =3D ~(readl(keypad->base + > + KP_INX_OFFSET(reg_nr))); > + > + writel(KP_IRQ_FEDGE | KP_IRQ_REDGE, keypad->base + KP_INT_CFG); > + > + writel(SCAN_CYCLE(keypad->scan_cycl) | > + DBC_CYCLE(keypad->debounce_cycl), keypad->base + KP_TIMING); > + > + writel(ENABLE(1) | ROWMASK(keypad->rows_en_mask) | > + COLMASK(keypad->cols_en_mask), keypad->base + KP_CTL); > + > + return 0; > +} > + > +static void sun4i_keypad_close(struct input_dev *dev) > +{ > + struct sun4i_keypad_data *keypad =3D input_get_drvdata(dev); > + > + writel(ENABLE(0) | ROWMASK(keypad->rows_en_mask) | > + COLMASK(keypad->cols_en_mask), keypad->base + KP_CTL); > + > + clk_disable_unprepare(keypad->clk); > + clk_disable_unprepare(keypad->apb_clk); > +} > + > +static int sun4i_keypad_parse_dt(struct device *dev, > + struct sun4i_keypad_data *keypad) > +{ > + struct device_node *np; > + > + np =3D dev->of_node; > + if (!np) > + return -EINVAL; > + > + of_property_read_u32(np, "debounce-cycle", &keypad->debounce_cycl); > + if (keypad->debounce_cycl < MIN_CYCLE) > + keypad->debounce_cycl =3D MIN_DBC_CYCLE; > + > + of_property_read_u32(np, "scan-cycle", &keypad->scan_cycl); > + if (keypad->scan_cycl < MIN_CYCLE) > + keypad->scan_cycl =3D MIN_SCAN_CYCLE; > + > + if (of_property_read_bool(np, "autorepeat")) > + keypad->autorepeat =3D true; > + > + return 0; > +} > + > +static int sun4i_keypad_probe(struct platform_device *pdev) > +{ > + struct sun4i_keypad_data *keypad; > + struct device *dev =3D &pdev->dev; > + int row, col, code; > + int ret =3D 0; > + > + keypad =3D devm_kzalloc(dev, sizeof(struct sun4i_keypad_data), > + GFP_KERNEL); > + if (!keypad) > + return -ENOMEM; > + > + ret =3D sun4i_keypad_parse_dt(dev, keypad); > + if (ret) > + return ret; > + > + keypad->base =3D devm_ioremap_resource(dev, > + platform_get_resource(pdev, IORESOURCE_MEM, 0)); Usually, it gets called on two lines to make it clearer. > + if (IS_ERR(keypad->base)) > + return PTR_ERR(keypad->base); > + > + keypad->dev =3D dev; > + keypad->input =3D devm_input_allocate_device(dev); > + if (!keypad->input) > + return -ENOMEM; > + > + keypad->input->name =3D pdev->name; > + keypad->input->phys =3D "sun4i_keypad/input0"; > + > + keypad->input->id.bustype =3D BUS_HOST; > + keypad->input->id.vendor =3D 0x0001; > + keypad->input->id.product =3D 0x0001; > + keypad->input->id.version =3D 0x0100; > + > + keypad->input->open =3D sun4i_keypad_open; > + keypad->input->close =3D sun4i_keypad_close; > + > + /* matrix keypad keymap as: > + * row << 24 | column << 16 | key-code > + */ > + ret =3D matrix_keypad_build_keymap(NULL, NULL, > + KP_MAX_ROWS, > + KP_MAX_COLS, > + keypad->keymap, > + keypad->input); > + if (ret) { > + dev_err(dev, "failed to build keymap\n"); > + return ret; > + } > + > + /* Search for enabled rows and cols */ > + for (row =3D 0; row < KP_MAX_ROWS; row++) { > + for (col =3D 0; col < KP_MAX_COLS; col++) { > + code =3D MATRIX_SCAN_CODE(row, col, KP_ROW_SHIFT); > + if (keypad->keymap[code] !=3D KEY_RESERVED) { > + keypad->rows_en_mask |=3D 1 << row; > + keypad->cols_en_mask |=3D 1 << col; > + } > + } > + } > + > + if (keypad->autorepeat) > + __set_bit(EV_REP, keypad->input->evbit); > + > + input_set_capability(keypad->input, EV_MSC, MSC_SCAN); > + input_set_drvdata(keypad->input, keypad); > + > + keypad->irq =3D platform_get_irq(pdev, 0); > + if (keypad->irq < 0) { > + dev_err(dev, "failed to get keypad IRQ\n"); > + return -ENXIO; > + } > + > + ret =3D devm_request_irq(dev, keypad->irq, > + sun4i_keypad_irq, 0, > + "sun4i-a10-keypad", keypad); > + if (ret) { > + dev_err(dev, "failed to request IRQ\n"); > + return ret; > + } > + > + keypad->apb_clk =3D devm_clk_get(dev, "apb"); > + if (IS_ERR(keypad->apb_clk)) { > + dev_err(dev, "failed to get a apb clock.\n"); > + return PTR_ERR(keypad->apb_clk); > + } > + > + keypad->clk =3D devm_clk_get(dev, "keypad"); > + if (IS_ERR(keypad->clk)) { > + dev_err(dev, "failed to get a keypad clock.\n"); > + return PTR_ERR(keypad->clk); > + } > + > + ret =3D clk_set_rate(keypad->clk, KP_BASE_CLK); > + if (ret) { > + dev_err(dev, "set keypad base clock failed!\n"); > + return ret; > + } Do you need that rate to be enforced, or is it some leftover from the allwinner BSP? > + ret =3D input_register_device(keypad->input); > + if (ret) > + return ret; > + > + platform_set_drvdata(pdev, keypad); > + > + return 0; > +} > + > +static int sun4i_keypad_remove(struct platform_device *pdev) > +{ > + struct sun4i_keypad_data *keypad =3D platform_get_drvdata(pdev); > + > + free_irq(keypad->irq, keypad); > + input_unregister_device(keypad->input); > + kfree(keypad); The point of using devm_ functions is precisely to remove the removal code from both the probe error path (and you did so there), and from the remove. > + > + return 0; > +} > + > +static const struct of_device_id sun4i_keypad_of_match[] =3D { > + { .compatible =3D "allwinner,sun4i-a10-keypad", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, sun4i_keypad_of_match); > + > +static struct platform_driver sun4i_keypad_driver =3D { > + .driver =3D { > + .name =3D "sun4i-a10-keypad", > + .of_match_table =3D of_match_ptr(sun4i_keypad_of_match), > + }, > + .probe =3D sun4i_keypad_probe, > + .remove =3D sun4i_keypad_remove, > +}; > + > +module_platform_driver(sun4i_keypad_driver); > + > +MODULE_DESCRIPTION("Allwinner sun4i keypad controller driver"); > +MODULE_AUTHOR("Yassin Jaffer "); > +MODULE_LICENSE("GPL"); > + It looks good otherwise, thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --27KoNqt0fmcl1zj/ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJV+rqQAAoJEBx+YmzsjxAgPe0P/2YMhX/R17NqSKj38IGndOHj WWicVlvFQ0fxL+pOvV8OvDSsJXj9+o3+VE2OU3F3fnwKdhFXMV1E5/tdHFNkAZmg 1xy/5fn5uqzXut2E8jx2e6ovL2ZqEtN5/fsUmoSV/rjp8vmTcaqilSfv0HpEF6F6 Tq7yHRJOI7U3qmXqx1h3hRR8mh1l19hUwW+yn/oYwxCfzpiICCNv+68XoDaRYquA 0qIzd4RJhTZn9h32AOBPAeImObgytrtxx3sodt740sQv0NPCbLqB7vD/LVAkimld 6wmufrgrP+5iqkZLOqbz8LlJEyKZT84XxftZIGus5iWAYPbLsCWf4xNIF979V+sj NrMA5F7OI2vmmT6N3lVbVcLa9ZEL6vrxsGfBzN0TMYnK6a+ceLeBgZ9V1f9/pj1q svDVeDqwGGMe59XkWwIh1o4k4KRJ6unyhEgW79Rgpr523tNgCStr0vzqgUbI4ezW Odk9LtRMlUIFhvFil2iv58GvQBve7Chgb1wQtDmnSSYpxldx3389Lmglupx6jqCA JYUfGvTOEv5cS+lPy2I5TkdK6l2pzGq0sT7NgoaiCDA5zlfY8hSRoIclJjKlIoE3 jHJZ7QGOjw3Qir+rFJtlqLDdqcij1jbi7RyqgOdFg99wEtmnkbb/l7u9aAdglqCA iRDOkIOcDqboqrrKi0s5 =P3xp -----END PGP SIGNATURE----- --27KoNqt0fmcl1zj/--