Re: [PATCH 1/2] Input: DaVinci Keypad Driver

[Miguel Aguilar <miguel.aguilar@ridgerun.com>]

Dmitry,

I addressed your comments but I still have a couple of questions.


>> +CONFIG_KEYBOARD_DAVINCI_DM365=m
>>  # CONFIG_INPUT_MOUSE is not set
>>  # CONFIG_INPUT_JOYSTICK is not set
>>  # CONFIG_INPUT_TABLET is not set
> 
> 
> If you want to merge the driver through input tree the defconfig chunk
> has to go elsewhere.
[MA] Ok, independent patch.


>>  
>> +config KEYBOARD_DAVINCI
>> +	tristate "TI DaVinci Keypad"
>> +	depends on ARCH_DAVINCI_DM365
>> +	help
>> +	  Say Y to enable keypad module support for the TI DaVinci
>> +	  platforms (DM365)
>> +
> 
> "To compile this driver as a module..."
[MA] Comment added.


>> +		keycode = keymap[position];
>> +		if((new_status >> position) & 0x1) {
> 
> bool release = (new_status >> position) & 0x1;
> input_report_key(davinci_kp->input, keycode, !release);
> dev_dbg(dev, "davinci_keypad: key %d %s\n",
> 	keycode, release ? "released" : "pressed");
> 
> is shorter.
> 
>> +			/* Report release */
>> +			dev_dbg(dev, "davinci_keypad: key %d released\n",
>> +				    keycode);
>> +			input_report_key(davinci_kp->input, keycode, 0);
>> +		} else {
>> +			/* Report press */
>> +			dev_dbg(dev, "davinci_keypad: key %d pressed\n",
>> +				    keycode);
>> +			input_report_key(davinci_kp->input, keycode, 1);
>> +		}
>> +		input_sync(davinci_kp->input);
>> +		ret = IRQ_HANDLED;
>> +	}
>> +
>> +	/* Clearing interrupt */
>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR);
> 
> You return IRQ_HANDLED only if keypad state changed but clear interrupt
> regardless. This is suspicious.
> 
>> +
>> +	/* Enable interrupts */
>> +	davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA);
>> +
>> +	return ret;
>> +}

[MA] This is the current irq function:
static irqreturn_t davinci_ks_interrupt(int irq, void *dev_id)
{
	struct davinci_ks *davinci_ks = dev_id;
	struct device *dev = &davinci_ks->input->dev;
	unsigned short *keymap = davinci_ks->keymap;
	u32 prev_status, new_status, changed, position;
	bool release;
	int keycode = KEY_UNKNOWN;
	int ret = IRQ_NONE;

	/* Disable interrupt */
	davinci_ks_write(davinci_ks, 0x0, DAVINCI_KEYSCAN_INTENA);

	/* Reading previous and new status of the key scan */
	prev_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_PREVSTATE);
	new_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_CURRENTST);

	changed = prev_status ^ new_status;
	position = ffs(changed) - 1;

	if (changed) {
		keycode = keymap[position];
		release = (new_status >> position) & 0x1;
		dev_dbg(dev, "davinci_keyscan: key %d %s\n",
		    keycode, release ? "released" : "pressed");

		input_report_key(davinci_ks->input, keycode, !release);
		input_sync(davinci_ks->input);

		/* Clearing interrupt */
		davinci_ks_write(davinci_ks, DAVINCI_KEYSCAN_INT_ALL,
				    DAVINCI_KEYSCAN_INTCLR);

		ret = IRQ_HANDLED;
	}

	/* Enable interrupts */
	davinci_ks_write(davinci_ks, 0x1, DAVINCI_KEYSCAN_INTENA);

	return ret;
}


>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		dev_err(dev, "no mem resource\n");
>> +		ret = -ENODEV;
> 
> -EINVAL I'd say.
[MA] Ok. -EINVAL


>> +	key_dev->id.vendor = 0x0001;
>> +	key_dev->id.product = 0x0001;
>> +	key_dev->id.version = 0x0001;
>> +	key_dev->keycode = davinci_kp->pdata->keymap;
> 
> Please kopy keymap into the davinci_kp stucture and use it so that
> platform data is never changed and can be declared const.
> 
[MA] keymap copied to private data.

>> +	key_dev->keycodesize = sizeof(unsigned int);
> 
> sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short.
> 
[MA] Now it uses sizeof(davinci_kp->keymap[0])

>> +}
>> +
>> +static int __exit davinci_kp_remove(struct platform_device *pdev)
> 
> __devexit?
>

[MA] So, this will be __devexit


>> +static struct platform_driver davinci_kp_driver = {
>> +	.driver = {
>> +			.name = "davinci_keypad",
>> +			.owner = THIS_MODULE,
>> +		},
>> +	.remove = __exit_p(davinci_kp_remove),
> 
> __devexit_p(). I think you can still unbind the device even if you use
> platform_driver_probe.
> 
[MA] ... and __devexit.


>> +static int __init davinci_kp_init(void)
>> +{
>> +	return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe);
>> +}
>> +module_init(davinci_kp_init);
[MA] Should I use platform_driver_probe?


>> +static void __exit davinci_kp_exit(void)
>> +{
>> +	platform_driver_unregister(&davinci_kp_driver);
>> +}
>> +module_exit(davinci_kp_exit);
[MA] Is the module exit function __exit or __devexit

Thanks,
Miguel Aguilar