Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver

[Gabriel Fernandez <gabriel.fernandez@st.com>]

Hi Lee,

On 03/10/2014 12:48 PM, Lee Jones wrote:
> 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@st.com>
>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> Are you sure these are in the correct order?
ok i change the order

>> +- 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.

you want to refer to "debounce-interval" ?

>
> +Example:
> +
> +keyscan: keyscan@fe4b0000 {
> +	compatible = "st,keypad";
> Is there any way we can make this more specific to _this_ IP?
for my knowledge this IP is the same for stixxxx platform.


>
>> +	  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@st.com>
>> + *
>> + * Based on sh_keysc.c, copyright 2008 Magnus Damm
> Did sh_keysc.c ever exist in Mainline?
i think no, i 'll suppress "Based on sh_keysc.c, copyright 2008 Magnus Damm"

>
>> + * 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?
no reason for the padded,  i will change that.

>> +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?
>
i 'll rename 'num_out_pads' into 'n_rows' and 'num_in_pads' into 'n_cols'



> 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.
no mandatory property, i will update the  dt binding.

>
>> +	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?
no, i will remove that.

> +	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.
ok, we are in no cacheable use case.

>
>> +	}
>> +
>> +	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?
yes, crap these lines.
>> +	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?
i will follow Dmitry remark (omit these information)

>
>> +	if (!pdata) {
>> +		error = keypad_matrix_key_parse_dt(st_kp);
>> +		if (error)
>> +			return error;
>> +		pdata = st_kp->config;
> Is pdata used after this?
no, i will remove platform data support


Thanks