Hi Miguel,
On Sat, Oct 10, 2009 at 12:30 AM, Miguel Aguilar
<[email protected]> wrote:
> Hi Trilok
> Trilok Soni wrote:
>>
>> Hi Miguel,
>>
>> To me it seems that scripts/checkpatch.pl will show few errors. Please
>> run it through checkpatch.pl.
>
> This patch was made with git format-patch.
>>
As I said below it will definitely show few errors.
>>> +
>>> +#ifndef DAVINCI_KEYSCAN_H
>>> +#define DAVINCI_KEYSCAN_H
>>> +
>>> +#include <linux/io.h>
>>> +
>>> +/* Base of key scan register bank */
>>> +#define DM365_KEYSCAN_BASE (0x01C69400)
>>
>> Why? You don't need this if you do ioremap and define this base
>> address through resources[].
>
> Actually this macro is used to define the base address through resources in
> the dm365.c file, so it does exactly what you mean. DaVinci platforms handle
> the base address in this way.
I think there is no need of putting this #define in this file at all.
Just use this base address directly in resource in devices.c or put
this #define at top of the resources itself. It serves no purpose in
this file.
>>
>>> +
>>> +enum davinci_matrix_types {
>>> + DAVINCI_KEYSCAN_MATRIX_4X4,
>>> + DAVINCI_KEYSCAN_MATRIX_5X3,
>>> +};
>>> +
>>> +struct davinci_ks_platform_data {
>>> + unsigned short *keymap;
>>> + u32 keymapsize;
>>> + u8 rep:1;
>>> + u8 strobe;
>>> + u8 interval;
>>> + u8 matrix_type;
>>> +};
>>> +
>>> +#endif
>>> +
>>> diff --git a/drivers/input/keyboard/Kconfig
>>> b/drivers/input/keyboard/Kconfig
>>> index ee98b1b..b7668ed 100644
>>> --- a/drivers/input/keyboard/Kconfig
>>> +++ b/drivers/input/keyboard/Kconfig
>>> @@ -423,4 +423,14 @@ config KEYBOARD_W90P910
>>> To compile this driver as a module, choose M here: the
>>> module will be called w90p910_keypad.
>>>
>>> +config KEYBOARD_DAVINCI
>>> + tristate "TI DaVinci Key Scan"
>>> + 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, choose M here: the
>>> + module will be called davinci_keyscan.
>>> +
>>> endif
>>> +static void davinci_ks_write(struct davinci_ks *davinci_ks, u32 val, u32
>>> addr)
>>> +{
>>> + u32 base = (u32)davinci_ks->base;
>>
>> Why this casting? Why don't we simply use readl and writel?
>
> Ok unneeded casting.
>>
>>> +
>>> + __raw_writel(val,(u32 *)(base + addr));
>>> +}
>>> +
>>> +static u32 davinci_ks_read(struct davinci_ks *davinci_ks, u32 addr)
>>> +{
>>> + u32 base = (u32)davinci_ks->base;
>>> +
>>> + return __raw_readl((u32 *)(base + addr));
>>> +}
>>> +
>>> +
>>> + if(changed) {
>>
>> space after "if" is required. checkpatch will show you lot's of errors
>> like this.
>
> ok.
>>
>>> +
>>> +MODULE_AUTHOR("Miguel Aguilar");
>>> +MODULE_DESCRIPTION("Texas Instruments DaVinci Key Scan Driver");
>>> +MODULE_LICENSE("GPL");
>>
>> MODULE_ALIAS ?
>
> Is this really needed or it just used the name of the platform driver by
> default?
Not much strict on this though.
--
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
