Message-ID: <20140310114819.GO14976@lee--X1>
Date:	Mon, 10 Mar 2014 11:48:19 +0000
From:	Lee Jones <lee.jones@...aro.org>
To:	Gabriel FERNANDEZ <gabriel.fernandez@...com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Rob Landley <rob@...dley.net>,
	Russell King <linux@....linux.org.uk>,
	Grant Likely <grant.likely@...aro.org>,
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-input@...r.kernel.org, kernel@...inux.com,
	Giuseppe Condorelli <giuseppe.condorelli@...com>
Subject: Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan
 driver

Hi Gabi,

Sorry for the delay. It was a hectic week last week.

As promised:

> This patch adds ST Keyscan driver to use the keypad hw a subset
> of ST boards provide. Specific board setup will be put in the
> given dt.
> 
> Signed-off-by: Giuseppe Condorelli <giuseppe.condorelli@...com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@...com>

Are you sure these are in the correct order?

What is the history of this commit?

> ---
>  .../devicetree/bindings/input/st-keypad.txt        |  50 ++++

This should be submitted as a seperate patch.

>  drivers/input/keyboard/Kconfig                     |  12 +
>  drivers/input/keyboard/Makefile                    |   1 +
>  drivers/input/keyboard/st-keyscan.c                | 323 +++++++++++++++++++++
>  4 files changed, 386 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/st-keypad.txt
>  create mode 100644 drivers/input/keyboard/st-keyscan.c
> 
> diff --git a/Documentation/devicetree/bindings/input/st-keypad.txt b/Documentation/devicetree/bindings/input/st-keypad.txt
> new file mode 100644
> index 0000000..63eb07a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/st-keypad.txt
> @@ -0,0 +1,50 @@
> +* ST Keypad controller device tree bindings

s/device tree/Device Tree

> +
> +The ST keypad controller device tree binding is based on the

As above.

> +matrix-keymap.
> +
> +Required properties:
> +
> +- compatible: "st,keypad"

I think there will be subsequent st,keypad drivers? Is there any way
we can make this compatible string more specific to _this_ device?

st,stih4xx-keypad?

> +- reg: Register base address of st-keypad controler.

s/address/address and size AND s/controler/controller

> +- interrupts: Interrupt numberof st-keypad controler.

s/numberof/number for the

> +- clocks: Must contain one entry, for the module clock.
> +  See ../clocks/clock-bindings.txt for details.

Great!

> +- pinctrl-0: Should specify pin control groups used for this controller.
> +
> +- pinctrl-names: Should contain only one value - "default".

Like to ../pinctrl/pinctrl-bindings.txt

> +- linux,keymap: The keymap for keys as described in the binding document
> +  devicetree/bindings/input/matrix-keymap.txt.
> +
> +- keypad,num-rows: Number of row lines connected to the keypad controller.
> +
> +- keypad,num-columns: Number of column lines connected to the keypad
> +  controller.
> +
> +- st,debounce_us: Debouncing interval time in microseconds

I'm sure there will be a shared binding for de-bounce.

If not, there certainly should be.

> +
> +

Superfluous new lines.

> +Example:
> +
> +keyscan: keyscan@...b0000 {
> +	compatible = "st,keypad";

Is there any way we can make this more specific to _this_ IP?

> +	reg = <0xfe4b0000 0x2000>;
> +	interrupts = <GIC_SPI 212 IRQ_TYPE_NONE>;
> +	clocks	= <&CLK_SYSIN>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_keyscan>;
> +
> +	keypad,num-rows = <4>;
> +	keypad,num-columns = <4>;
> +	st,debounce_us = <5000>;
> +	linux,keymap = < /* in0	in1	in2	in3 */

Do these line up in the file? I know Git can be a bit funny about this
sometimes.

> +		KEY_F13 KEY_F9  KEY_F5 KEY_F1		/* out0 */
> +		KEY_F14 KEY_F10 KEY_F6 KEY_F2		/* out1 */
> +		KEY_F15 KEY_F11 KEY_F7 KEY_F3		/* out2 */
> +		KEY_F16 KEY_F12 KEY_F8 KEY_F4 >;	/* out3 */
> +};
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index a673c9f..9e29125 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -512,6 +512,18 @@ config KEYBOARD_STOWAWAY
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called stowaway.
>  
> +config KEYBOARD_ST_KEYSCAN
> +	tristate "STMicroelectronics keyscan support"
> +	depends on ARCH_STI
> +	select INPUT_EVDEV
> +	select INPUT_MATRIXKMAP
> +	help
> +	  Say Y here if you want to use a keypad attached to the keyscan block
> +	  on some STMicroelectronics SoC devices.

Might be worth mentioning which ones.

> +	  To compile this driver as a module, choose M here: the
> +	  module will be called stm-keyscan.
> +
>  config KEYBOARD_SUNKBD
>  	tristate "Sun Type 4 and Type 5 keyboard"
>  	select SERIO
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index a699b61..5fd020a 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -50,6 +50,7 @@ obj-$(CONFIG_KEYBOARD_SH_KEYSC)		+= sh_keysc.o
>  obj-$(CONFIG_KEYBOARD_SPEAR)		+= spear-keyboard.o
>  obj-$(CONFIG_KEYBOARD_STMPE)		+= stmpe-keypad.o
>  obj-$(CONFIG_KEYBOARD_STOWAWAY)		+= stowaway.o
> +obj-$(CONFIG_KEYBOARD_ST_KEYSCAN)	+= st-keyscan.o
>  obj-$(CONFIG_KEYBOARD_SUNKBD)		+= sunkbd.o
>  obj-$(CONFIG_KEYBOARD_TC3589X)		+= tc3589x-keypad.o
>  obj-$(CONFIG_KEYBOARD_TEGRA)		+= tegra-kbc.o
> diff --git a/drivers/input/keyboard/st-keyscan.c b/drivers/input/keyboard/st-keyscan.c
> new file mode 100644
> index 0000000..606cc19
> --- /dev/null
> +++ b/drivers/input/keyboard/st-keyscan.c
> @@ -0,0 +1,323 @@
> +/*
> + * STMicroelectronics Key Scanning driver
> + *
> + * Copyright (c) 2014 STMicroelectonics Ltd.
> + * Author: Stuart Menefy <stuart.menefy@...com>
> + *
> + * Based on sh_keysc.c, copyright 2008 Magnus Damm

Did sh_keysc.c ever exist in Mainline?

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/input/matrix_keypad.h>
> +
> +#define ST_KEYSCAN_MAXKEYS 16
> +
> +#define KEYSCAN_CONFIG_OFF		0x000
> +#define KEYSCAN_CONFIG_ENABLE		1

0x001?

> +#define KEYSCAN_DEBOUNCE_TIME_OFF	0x004
> +#define KEYSCAN_MATRIX_STATE_OFF	0x008
> +#define KEYSCAN_MATRIX_DIM_OFF		0x00c

Odd that these are 3 digit padded? Is there a reason for that?

> +#define KEYSCAN_MATRIX_DIM_X_SHIFT	0
> +#define KEYSCAN_MATRIX_DIM_Y_SHIFT	2

For all 'ST_KEYSCAN_...'?

> +struct keypad_platform_data {
> +	const struct matrix_keymap_data *keymap_data;
> +	unsigned int num_out_pads;
> +	unsigned int num_in_pads;
> +	unsigned int debounce_us;
> +};
> +
> +struct keyscan_priv {
> +	void __iomem *base;
> +	int irq;
> +	struct clk *clk;
> +	struct input_dev *input_dev;
> +	struct keypad_platform_data *config;
> +	unsigned int last_state;
> +	u32 keycodes[ST_KEYSCAN_MAXKEYS];

Seems odd to limit this. Can't the information come from DT
i.e. keypad,num-rows and keypad,num-columns?

> +};
> +
> +static irqreturn_t keyscan_isr(int irq, void *dev_id)
> +{
> +	struct platform_device *pdev = dev_id;
> +	struct keyscan_priv *priv = platform_get_drvdata(pdev);
> +	int state;
> +	int change;
> +
> +	state = readl(priv->base + KEYSCAN_MATRIX_STATE_OFF) & 0xffff;
> +	change = priv->last_state ^ state;
> +
> +	while (change) {
> +		int scancode = __ffs(change);
> +		int down = state & (1 << scancode);

int down = state & BIT(scancode);

> +		input_report_key(priv->input_dev, priv->keycodes[scancode],
> +				 down);
> +		change ^= 1 << scancode;

change ^= BIT(scancode);

> +	};
> +
> +	input_sync(priv->input_dev);
> +
> +	priv->last_state = state;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void keyscan_start(struct keyscan_priv *priv)
> +{
> +	clk_enable(priv->clk);
> +
> +	writel(priv->config->debounce_us * (clk_get_rate(priv->clk) / 1000000),
> +	       priv->base + KEYSCAN_DEBOUNCE_TIME_OFF);
> +
> +	writel(((priv->config->num_in_pads - 1) << KEYSCAN_MATRIX_DIM_X_SHIFT) |
> +	       ((priv->config->num_out_pads - 1) << KEYSCAN_MATRIX_DIM_Y_SHIFT),
> +	       priv->base + KEYSCAN_MATRIX_DIM_OFF);
> +
> +	writel(KEYSCAN_CONFIG_ENABLE, priv->base + KEYSCAN_CONFIG_OFF);
> +}
> +
> +static void keyscan_stop(struct keyscan_priv *priv)
> +{
> +	writel(0, priv->base + KEYSCAN_CONFIG_OFF);
> +
> +	clk_disable(priv->clk);
> +}
> +
> +static int keypad_matrix_key_parse_dt(struct keyscan_priv *st_kp)
> +{
> +	struct device *dev = st_kp->input_dev->dev.parent;
> +	struct device_node *np = dev->of_node;
> +	struct keypad_platform_data *pdata;
> +	int error;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		dev_err(dev, "failed to allocate driver pdata\n");

If the system is OOM, this sort of error message will be pretty
redundant. Just return -ENOMEM instead.

> +		return -ENOMEM;
> +	}
> +
> +	error = matrix_keypad_parse_of_params(dev, &pdata->num_out_pads,
> +			&pdata->num_in_pads);
> +	if (error) {
> +		dev_err(dev, "failed to parse keypad params\n");
> +		return error;

Nit: It's pretty unusual to use this for a standard error handling
variable. Consider 'ret' or 'err' as a replacement.

> +	}
> +
> +	of_property_read_u32(np, "st,debounce_us", &pdata->debounce_us);

Isn't this a required property? Might be worth checking the return
value if so.

> +	st_kp->config = pdata;
> +
> +	dev_info(dev, "rows=%d col=%d debounce=%d\n",
> +			pdata->num_out_pads,
> +			pdata->num_in_pads,
> +			pdata->debounce_us);

Might be worth moving this down passed the final point of failure.

> +	error = of_property_read_u32_array(np, "linux,keymap",
> +					st_kp->keycodes, ST_KEYSCAN_MAXKEYS);
> +	if (error) {
> +		dev_err(dev, "failed to parse keymap\n");
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init keyscan_probe(struct platform_device *pdev)
> +{
> +	struct keypad_platform_data *pdata =
> +		dev_get_platdata(&pdev->dev);

Do we really support platform data still?

> +	struct keyscan_priv *st_kp;
> +	struct input_dev *input_dev;
> +	struct device *dev = &pdev->dev;

dev and pdev are used randomly in probe(). Just remove the dev
de-reference and use &pdev->dev exclusively.

It's pretty common to de-reference 'np = pdev->dev.of_node' though.

> +	struct resource *res;
> +	int len;
> +	int error;
> +	int i;
> +
> +	if (!pdata && !pdev->dev.of_node) {
> +		dev_err(&pdev->dev, "no keymap defined\n");
> +		return -EINVAL;
> +	}
> +
> +	st_kp = devm_kzalloc(dev, sizeof(*st_kp), GFP_KERNEL);
> +	if (!st_kp) {
> +		dev_err(dev, "failed to allocate driver data\n");
> +		return -ENOMEM;

I wouldn't print any messages on -ENOMEM personally. It's not a driver
failure per se, but a system one where the user will soon be notified.

> +	}
> +	st_kp->config = pdata;

You only want to do this in the !np case.

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "no I/O memory specified\n");
> +		return -ENXIO;
> +	}
> +
> +	len = resource_size(res);
> +	if (!devm_request_mem_region(dev, res->start, len, pdev->name)) {
> +		dev_err(dev, "failed to reserve I/O memory\n");
> +		return -EBUSY;
> +	}
> +
> +	st_kp->base = devm_ioremap_nocache(dev, res->start, len);
> +	if (st_kp->base == NULL) {
> +		dev_err(dev, "failed to remap I/O memory\n");
> +		return -ENXIO;
> +	}

Replace the two above with devm_ioremap_resource().

Make sure the IORESOURCE_CACHEABLE is set.

> +	st_kp->irq = platform_get_irq(pdev, 0);
> +	if (st_kp->irq < 0) {
> +		dev_err(dev, "no IRQ specified\n");
> +		return -ENXIO;

No such device or address, are you sure?

Perhaps -EINVAL would be better, as one should be specified.

> +	}
> +
> +	error = devm_request_irq(dev, st_kp->irq, keyscan_isr, 0,
> +				 pdev->name, pdev);
> +	if (error) {
> +		dev_err(dev, "failed to request IRQ\n");
> +		return error;
> +	}
> +
> +	input_dev = devm_input_allocate_device(&pdev->dev);
> +	if (!input_dev) {
> +		dev_err(&pdev->dev, "failed to allocate the input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	st_kp->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(st_kp->clk)) {
> +		dev_err(dev, "cannot get clock");
> +		return PTR_ERR(st_kp->clk);
> +	}
> +
> +	input_dev = input_allocate_device();
> +	if (!input_dev) {
> +		dev_err(dev, "failed to allocate input device\n");
> +		return -ENOMEM;
> +	}

Wasn't this done already?

> +	st_kp->input_dev = input_dev;
> +
> +	input_dev->name = pdev->name;
> +	input_dev->phys = "keyscan-keys/input0";
> +	input_dev->dev.parent = dev;
> +
> +	input_dev->id.bustype = BUS_HOST;
> +	input_dev->id.vendor = 0x0001;
> +	input_dev->id.product = 0x0001;
> +	input_dev->id.version = 0x0100;

Any chance we can #define these?

> +	if (!pdata) {
> +		error = keypad_matrix_key_parse_dt(st_kp);
> +		if (error)
> +			return error;
> +		pdata = st_kp->config;

Is pdata used after this?

> +	}
> +
> +	input_dev->keycode = st_kp->keycodes;
> +	input_dev->keycodesize = sizeof(st_kp->keycodes[0]);
> +	input_dev->keycodemax = ARRAY_SIZE(st_kp->keycodes);
> +
> +	for (i = 0; i < ST_KEYSCAN_MAXKEYS; i++)
> +		input_set_capability(input_dev, EV_KEY, st_kp->keycodes[i]);
> +	__clear_bit(KEY_RESERVED, input_dev->keybit);
> +
> +	error = input_register_device(input_dev);
> +	if (error) {
> +		dev_err(dev, "failed to register input device\n");
> +		input_free_device(input_dev);
> +		platform_set_drvdata(pdev, NULL);

You don't need to do this anymore. It's taken care of elsewhere.

> +		return error;
> +	}
> +
> +	platform_set_drvdata(pdev, st_kp);
> +
> +	keyscan_start(st_kp);
> +
> +	device_set_wakeup_capable(&pdev->dev, 1);
> +
> +	return 0;
> +}
> +
> +static int __exit keyscan_remove(struct platform_device *pdev)
> +{
> +	struct keyscan_priv *priv = platform_get_drvdata(pdev);
> +
> +	keyscan_stop(priv);
> +
> +	input_unregister_device(priv->input_dev);
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;

I already saw that Dmitry commented on the rest of the file.

<snip>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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