Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Joonyoung Shim
On 6/22/2010 12:38 PM, Eric Miao wrote:
> On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
>  wrote:
>> On 6/22/2010 12:02 PM, Eric Miao wrote:
>>> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim  
>>> wrote:
 On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>  wrote:
>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
 On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim 
  wrote:
> +void __init samsung_keypad_set_platdata(struct 
> samsung_keypad_platdata *pd)
> +{
> + � � � struct samsung_keypad_platdata *npd;
> +
> + � � � if (!pd) {
> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
> + � � � � � � � return;
> + � � � }
> +
> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), 
> GFP_KERNEL);
> + � � � if (!npd)
> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n", 
> __func__);
 This part of the code is actually duplicated again and again and again
 for each device, PXA and other legacy platforms are bad references for
 this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
 major points:

 �1. A minimum 'struct pxa_device_desc' for a simple description of a
 � � device (more than 90% of the devices can be described that way),
 � � and avoid using a comparatively heavier weight platform_device,
 � � which can be generated at run-time

 �2. pxa_register_device() to allocate and register the platform_device
 � � at run-time, along with the platform data
>>> It's a bad idea to make platform data be run-time discardable like this:
>>>
> +struct samsung_keypad_platdata {
> + � � � const struct matrix_keymap_data *keymap_data;
>>> What you end up with is some platform data structures which must be kept
>>> (those which have pointers to them from the platform data), and others
>>> (the platform data itself) which can be discarded at runtime.
>>>
>>> We know that the __initdata attributations cause lots of problems -
>>> they're frequently wrong. �Just see the constant hastle with __devinit
>>> et.al. �The same issue happens with __initdata as well.
>>>
>>> So why make things more complicated by allowing some platform data
>>> structures to be discardable and others not to be? �Is their small
>>> size (maybe 6 words for this one) really worth the hastle of getting
>>> __initdata attributations wrong (eg, on the keymap data?)
>>>
>> Russell,
>>
>> The benefit I see is when multiple boards are compiled in, those
>> data not used can be automatically discarded.
> Yes, but only some of the data can be discarded.  Continuing with the
> example in hand, while you can discard the six words which represent
> samsung_keypad_platdata, but the keymap_data can't be because that won't
> be re-allocated, which is probably a much larger data structure.
>
 No. the keymap_data is possible too. The keypad driver allocates other
 keymap area of input device and it is assigned from datas based on this
 keymap_data.

>>> This is a generic issue. Even if in your example, you can avoid this by
>>> re-allocation and re-assignment (ignore the performance issue for such
>>> behavior), the real question is the difficult to track all these down. Since
>> Right, it can occur difficulty of maintain. I wanted just to inform the
>> current fact.
>>
>>> matrix_keypad_data is something out of your control (it was actually
>>> drafted by me and Dmitry if you are interested), and think about one day
>>> I changed it's definition, now you have to sync your driver and code every
>>> time to make sure the discarded data is not referenced.
>>>
>> if matrix_keypad_data is changed, i think the patchset should included
>> change of related other parts using it.
>>
> 
> That's reasonable but difficult in practice, every keypad driver using
> matrix_keypad_data could be doing things differently. That's what I'm

Just FYI, correct name is matrix_keymap_data and current all keypad 
drivers using matrix_keymap_data use it to same way.

> concerned about. Things will be much easier for driver writers if he
> knows the data passed in will always be reference-able.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Eric Miao
On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
 wrote:
> On 6/22/2010 12:02 PM, Eric Miao wrote:
>> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim  
>> wrote:
>>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
 On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>  wrote:
>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim 
>>>  wrote:
 +void __init samsung_keypad_set_platdata(struct 
 samsung_keypad_platdata *pd)
 +{
 + � � � struct samsung_keypad_platdata *npd;
 +
 + � � � if (!pd) {
 + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
 + � � � � � � � return;
 + � � � }
 +
 + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), 
 GFP_KERNEL);
 + � � � if (!npd)
 + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n", 
 __func__);
>>> This part of the code is actually duplicated again and again and again
>>> for each device, PXA and other legacy platforms are bad references for
>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>> major points:
>>>
>>> �1. A minimum 'struct pxa_device_desc' for a simple description of a
>>> � � device (more than 90% of the devices can be described that way),
>>> � � and avoid using a comparatively heavier weight platform_device,
>>> � � which can be generated at run-time
>>>
>>> �2. pxa_register_device() to allocate and register the platform_device
>>> � � at run-time, along with the platform data
>> It's a bad idea to make platform data be run-time discardable like this:
>>
 +struct samsung_keypad_platdata {
 + � � � const struct matrix_keymap_data *keymap_data;
>> What you end up with is some platform data structures which must be kept
>> (those which have pointers to them from the platform data), and others
>> (the platform data itself) which can be discarded at runtime.
>>
>> We know that the __initdata attributations cause lots of problems -
>> they're frequently wrong. �Just see the constant hastle with __devinit
>> et.al. �The same issue happens with __initdata as well.
>>
>> So why make things more complicated by allowing some platform data
>> structures to be discardable and others not to be? �Is their small
>> size (maybe 6 words for this one) really worth the hastle of getting
>> __initdata attributations wrong (eg, on the keymap data?)
>>
> Russell,
>
> The benefit I see is when multiple boards are compiled in, those
> data not used can be automatically discarded.
 Yes, but only some of the data can be discarded.  Continuing with the
 example in hand, while you can discard the six words which represent
 samsung_keypad_platdata, but the keymap_data can't be because that won't
 be re-allocated, which is probably a much larger data structure.

>>> No. the keymap_data is possible too. The keypad driver allocates other
>>> keymap area of input device and it is assigned from datas based on this
>>> keymap_data.
>>>
>>
>> This is a generic issue. Even if in your example, you can avoid this by
>> re-allocation and re-assignment (ignore the performance issue for such
>> behavior), the real question is the difficult to track all these down. Since
>
> Right, it can occur difficulty of maintain. I wanted just to inform the
> current fact.
>
>> matrix_keypad_data is something out of your control (it was actually
>> drafted by me and Dmitry if you are interested), and think about one day
>> I changed it's definition, now you have to sync your driver and code every
>> time to make sure the discarded data is not referenced.
>>
>
> if matrix_keypad_data is changed, i think the patchset should included
> change of related other parts using it.
>

That's reasonable but difficult in practice, every keypad driver using
matrix_keypad_data could be doing things differently. That's what I'm
concerned about. Things will be much easier for driver writers if he
knows the data passed in will always be reference-able.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Joonyoung Shim
On 6/22/2010 12:02 PM, Eric Miao wrote:
> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim  
> wrote:
>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
 On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
  wrote:
> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim 
>>  wrote:
>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata 
>>> *pd)
>>> +{
>>> + � � � struct samsung_keypad_platdata *npd;
>>> +
>>> + � � � if (!pd) {
>>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>>> + � � � � � � � return;
>>> + � � � }
>>> +
>>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), 
>>> GFP_KERNEL);
>>> + � � � if (!npd)
>>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n", 
>>> __func__);
>> This part of the code is actually duplicated again and again and again
>> for each device, PXA and other legacy platforms are bad references for
>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>> major points:
>>
>> �1. A minimum 'struct pxa_device_desc' for a simple description of a
>> � � device (more than 90% of the devices can be described that way),
>> � � and avoid using a comparatively heavier weight platform_device,
>> � � which can be generated at run-time
>>
>> �2. pxa_register_device() to allocate and register the platform_device
>> � � at run-time, along with the platform data
> It's a bad idea to make platform data be run-time discardable like this:
>
>>> +struct samsung_keypad_platdata {
>>> + � � � const struct matrix_keymap_data *keymap_data;
> What you end up with is some platform data structures which must be kept
> (those which have pointers to them from the platform data), and others
> (the platform data itself) which can be discarded at runtime.
>
> We know that the __initdata attributations cause lots of problems -
> they're frequently wrong. �Just see the constant hastle with __devinit
> et.al. �The same issue happens with __initdata as well.
>
> So why make things more complicated by allowing some platform data
> structures to be discardable and others not to be? �Is their small
> size (maybe 6 words for this one) really worth the hastle of getting
> __initdata attributations wrong (eg, on the keymap data?)
>
 Russell,

 The benefit I see is when multiple boards are compiled in, those
 data not used can be automatically discarded.
>>> Yes, but only some of the data can be discarded.  Continuing with the
>>> example in hand, while you can discard the six words which represent
>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>> be re-allocated, which is probably a much larger data structure.
>>>
>> No. the keymap_data is possible too. The keypad driver allocates other
>> keymap area of input device and it is assigned from datas based on this
>> keymap_data.
>>
> 
> This is a generic issue. Even if in your example, you can avoid this by
> re-allocation and re-assignment (ignore the performance issue for such
> behavior), the real question is the difficult to track all these down. Since

Right, it can occur difficulty of maintain. I wanted just to inform the
current fact.

> matrix_keypad_data is something out of your control (it was actually
> drafted by me and Dmitry if you are interested), and think about one day
> I changed it's definition, now you have to sync your driver and code every
> time to make sure the discarded data is not referenced.
> 

if matrix_keypad_data is changed, i think the patchset should included
change of related other parts using it.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Eric Miao
On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim  wrote:
> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>>  wrote:
 On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim  
> wrote:
>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata 
>> *pd)
>> +{
>> + � � � struct samsung_keypad_platdata *npd;
>> +
>> + � � � if (!pd) {
>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>> + � � � � � � � return;
>> + � � � }
>> +
>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), 
>> GFP_KERNEL);
>> + � � � if (!npd)
>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n", 
>> __func__);
> This part of the code is actually duplicated again and again and again
> for each device, PXA and other legacy platforms are bad references for
> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> major points:
>
> �1. A minimum 'struct pxa_device_desc' for a simple description of a
> � � device (more than 90% of the devices can be described that way),
> � � and avoid using a comparatively heavier weight platform_device,
> � � which can be generated at run-time
>
> �2. pxa_register_device() to allocate and register the platform_device
> � � at run-time, along with the platform data
 It's a bad idea to make platform data be run-time discardable like this:

>> +struct samsung_keypad_platdata {
>> + � � � const struct matrix_keymap_data *keymap_data;
 What you end up with is some platform data structures which must be kept
 (those which have pointers to them from the platform data), and others
 (the platform data itself) which can be discarded at runtime.

 We know that the __initdata attributations cause lots of problems -
 they're frequently wrong. �Just see the constant hastle with __devinit
 et.al. �The same issue happens with __initdata as well.

 So why make things more complicated by allowing some platform data
 structures to be discardable and others not to be? �Is their small
 size (maybe 6 words for this one) really worth the hastle of getting
 __initdata attributations wrong (eg, on the keymap data?)

>>> Russell,
>>>
>>> The benefit I see is when multiple boards are compiled in, those
>>> data not used can be automatically discarded.
>>
>> Yes, but only some of the data can be discarded.  Continuing with the
>> example in hand, while you can discard the six words which represent
>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>> be re-allocated, which is probably a much larger data structure.
>>
>
> No. the keymap_data is possible too. The keypad driver allocates other
> keymap area of input device and it is assigned from datas based on this
> keymap_data.
>

This is a generic issue. Even if in your example, you can avoid this by
re-allocation and re-assignment (ignore the performance issue for such
behavior), the real question is the difficult to track all these down. Since
matrix_keypad_data is something out of your control (it was actually
drafted by me and Dmitry if you are interested), and think about one day
I changed it's definition, now you have to sync your driver and code every
time to make sure the discarded data is not referenced.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] input: samsung-keypad - Add samsung keypad driver

2010-06-21 Thread Joonyoung Shim
On 6/22/2010 8:31 AM, Kukjin Kim wrote:
> From: Joonyoung Shim 
> 
> This patch adds support for keypad driver running on Samsung SoCs. This
> driver is tested on SMDK6410(S3C6410), SMDKC100(S5PC100), Aquila(S5PC110),
> GONI(S5PC110) and SMDKV210(S5PV210) boards.
> 
> Signed-off-by: Joonyoung Shim 
> Signed-off-by: Kyungmin Park 
> Tested-by: Naveen Krishna Ch 
> [kgene@samsung.com: added comments]
> Acked-by: Kukjin Kim 
> Signed-off-by: Kukjin Kim 

I think this is duplicated posting. Why don't you just reply the
tested-by or acked-by at previous my mail?
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH v4] libata: pata_samsung_cf: Add Samsung PATA controller driver

2010-06-21 Thread Kukjin Kim
Kukjin Kim wrote:
> 
> From: Abhilash Kesavan 
> 
> Adds support for the Samsung PATA controller. This driver is based on the
> Libata subsystem and references the earlier patches sent for IDE
subsystem.
> 
> Signed-off-by: Abhilash Kesavan 
> Signed-off-by: Kukjin Kim 
> ---

Hi,

Maybe missed? :-(

If no problems, please apply or kindly let me know how this should be
handled.

> 
> Changes since v3:
> 
> Addressed comments from
> 
> Sergei Shtylyov:
>   - Removed filename in header
>   - Used ata_timing_compute for obtaining timing parameters
>   - Removed valid PIO mode check
>   - Added softreset method and associated functions
>   - Used devm_request_mem_region function as had used devm_ioremap
> earlier
>   - Removed kree and release_mem_region calls because of managed
> kzalloc and request_mem_region fns being used
>   - Added return in clk_get function
>   - Added missing clk_put
>   - Corrected indentation issue
> 
> Ben Dooks:
>   - Added missing 'void __iomem' parameters
>   - Added check for trailing bytes in data_xfer function
>   - Removed assignment in data_xfer function
>   - Coding changes - e.g. in pata_s3c_enable, pata_s3c_dev_select,
> wait_for_host_ready
>   - Removed host checks in suspend/resume functions
> 
>  drivers/ata/Kconfig   |9 +
>  drivers/ata/Makefile  |1 +
>  drivers/ata/pata_samsung_cf.c |  735
> +
>  3 files changed, 745 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/ata/pata_samsung_cf.c
> 
(snip)


Thanks.

Best regards,
Kgene.
--
Kukjin Kim , Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] ARM: S5P6442: Fix PLL setting announce message.

2010-06-21 Thread Kukjin Kim
From: Thomas Abraham 

The S5P6442 PLL setting announce message incorrectly displays S5P6440
as the SoC. Change it to S5P6442.

Signed-off-by: Thomas Abraham 
Signed-off-by: Kukjin Kim 
---
 arch/arm/mach-s5p6442/clock.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-s5p6442/clock.c b/arch/arm/mach-s5p6442/clock.c
index 3aadbf4..087e57f 100644
--- a/arch/arm/mach-s5p6442/clock.c
+++ b/arch/arm/mach-s5p6442/clock.c
@@ -294,7 +294,7 @@ void __init_or_cpufreq s5p6442_setup_clocks(void)
mpll = s5p_get_pll45xx(xtal, __raw_readl(S5P_MPLL_CON), pll_4502);
epll = s5p_get_pll45xx(xtal, __raw_readl(S5P_EPLL_CON), pll_4500);
 
-   printk(KERN_INFO "S5P6440: PLL settings, A=%ld, M=%ld, E=%ld",
+   printk(KERN_INFO "S5P6442: PLL settings, A=%ld, M=%ld, E=%ld",
apll, mpll, epll);
 
clk_fout_apll.rate = apll;
-- 
1.6.6.rc2

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Joonyoung Shim
On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>  wrote:
>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
 On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim  
 wrote:
> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata 
> *pd)
> +{
> + � � � struct samsung_keypad_platdata *npd;
> +
> + � � � if (!pd) {
> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
> + � � � � � � � return;
> + � � � }
> +
> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), 
> GFP_KERNEL);
> + � � � if (!npd)
> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n", 
> __func__);
 This part of the code is actually duplicated again and again and again
 for each device, PXA and other legacy platforms are bad references for
 this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
 major points:

 �1. A minimum 'struct pxa_device_desc' for a simple description of a
 � � device (more than 90% of the devices can be described that way),
 � � and avoid using a comparatively heavier weight platform_device,
 � � which can be generated at run-time

 �2. pxa_register_device() to allocate and register the platform_device
 � � at run-time, along with the platform data
>>> It's a bad idea to make platform data be run-time discardable like this:
>>>
> +struct samsung_keypad_platdata {
> + � � � const struct matrix_keymap_data *keymap_data;
>>> What you end up with is some platform data structures which must be kept
>>> (those which have pointers to them from the platform data), and others
>>> (the platform data itself) which can be discarded at runtime.
>>>
>>> We know that the __initdata attributations cause lots of problems -
>>> they're frequently wrong. �Just see the constant hastle with __devinit
>>> et.al. �The same issue happens with __initdata as well.
>>>
>>> So why make things more complicated by allowing some platform data
>>> structures to be discardable and others not to be? �Is their small
>>> size (maybe 6 words for this one) really worth the hastle of getting
>>> __initdata attributations wrong (eg, on the keymap data?)
>>>
>> Russell,
>>
>> The benefit I see is when multiple boards are compiled in, those
>> data not used can be automatically discarded.
> 
> Yes, but only some of the data can be discarded.  Continuing with the
> example in hand, while you can discard the six words which represent
> samsung_keypad_platdata, but the keymap_data can't be because that won't
> be re-allocated, which is probably a much larger data structure.
> 

No. the keymap_data is possible too. The keypad driver allocates other
keymap area of input device and it is assigned from datas based on this
keymap_data.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Kukjin Kim
Eric Miao wrote:
> 
> >> and now in s5pc200 code, you can also register/re-use the s5pc100_keypad
> >> device/driver, and everyone knows - OK, the IP on S5PC100 is now re-used
> >> here.
> >>
> >
> > This is naming issue and i know it was not desided yet.
> > This keypad driver can support s3c64xx, s5pc100, s5pc110, s5pv210.
> >
> 
> I bet the first silicon this IP is deployed is s3c64xx, so my understanding
> 's3c64xx-keypad.c' should be an acceptable name.  And it's also reasonable,
> a s3c64xx-keypad driver, and is later expanded to support subsequent silicons
> without the file name being changed.

Hi,

Seems to be more suitable 'samsung-keypad', because can support S5PC100, 
S5PC110, and S5PV210 as well as S3C64XX.
Already tested on SMDK6410(S3C6410), SMDKC100(S5PC100), Aquila(S5PC110), 
GONI(S5PC110) and SMDKV210(S5PV210) boards.


Thanks.

Best regards,
Kgene.
--
Kukjin Kim , Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 2/4] ARM: S3C64XX: Add keypad device to the SMDK6410 board

2010-06-21 Thread Kukjin Kim
From: Naveen Krishna Ch 

This patch is to support keypad device to the SMDK6410 board.

Signed-off-by: Naveen Krishna Ch 
Signed-off-by: Kukjin Kim 
---
 arch/arm/mach-s3c64xx/Kconfig|7 ++
 arch/arm/mach-s3c64xx/Makefile   |1 +
 arch/arm/mach-s3c64xx/clock.c|6 +
 arch/arm/mach-s3c64xx/include/mach/map.h |2 +
 arch/arm/mach-s3c64xx/mach-smdk6410.c|   24 +
 arch/arm/mach-s3c64xx/setup-keypad.c |   34 ++
 6 files changed, 74 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-s3c64xx/setup-keypad.c

diff --git a/arch/arm/mach-s3c64xx/Kconfig b/arch/arm/mach-s3c64xx/Kconfig
index f5a5972..08fdb7f 100644
--- a/arch/arm/mach-s3c64xx/Kconfig
+++ b/arch/arm/mach-s3c64xx/Kconfig
@@ -62,6 +62,11 @@ config S3C64XX_SETUP_FB_24BPP
help
  Common setup code for S3C64XX with an 24bpp RGB display helper.
 
+config S3C64XX_SETUP_KEYPAD
+   bool
+   help
+ Common setup code for S3C64XX KEYPAD GPIO configurations
+
 config S3C64XX_SETUP_SDHCI_GPIO
bool
help
@@ -100,10 +105,12 @@ config MACH_SMDK6410
select S3C_DEV_USB_HOST
select S3C_DEV_USB_HSOTG
select S3C_DEV_WDT
+   select SAMSUNG_DEV_KEYPAD
select HAVE_S3C2410_WATCHDOG
select S3C64XX_SETUP_SDHCI
select S3C64XX_SETUP_I2C1
select S3C64XX_SETUP_FB_24BPP
+   select S3C64XX_SETUP_KEYPAD
help
  Machine support for the Samsung SMDK6410
 
diff --git a/arch/arm/mach-s3c64xx/Makefile b/arch/arm/mach-s3c64xx/Makefile
index 9d10069..b1b4d10 100644
--- a/arch/arm/mach-s3c64xx/Makefile
+++ b/arch/arm/mach-s3c64xx/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_S3C64XX_DMA) += dma.o
 
 obj-$(CONFIG_S3C64XX_SETUP_I2C0) += setup-i2c0.o
 obj-$(CONFIG_S3C64XX_SETUP_I2C1) += setup-i2c1.o
+obj-$(CONFIG_S3C64XX_SETUP_KEYPAD) += setup-keypad.o
 obj-$(CONFIG_S3C64XX_SETUP_SDHCI) += setup-sdhci.o
 obj-$(CONFIG_S3C64XX_SETUP_FB_24BPP) += setup-fb-24bpp.o
 obj-$(CONFIG_S3C64XX_SETUP_SDHCI_GPIO) += setup-sdhci-gpio.o
diff --git a/arch/arm/mach-s3c64xx/clock.c b/arch/arm/mach-s3c64xx/clock.c
index fbd85a9..ec8f097 100644
--- a/arch/arm/mach-s3c64xx/clock.c
+++ b/arch/arm/mach-s3c64xx/clock.c
@@ -165,6 +165,12 @@ static struct clk init_clocks_disable[] = {
.ctrlbit= S3C6410_CLKCON_PCLK_IIS2,
}, {
 #endif
+   .name   = "keypad",
+   .id = -1,
+   .parent = &clk_p,
+   .enable = s3c64xx_pclk_ctrl,
+   .ctrlbit= S3C_CLKCON_PCLK_KEYPAD,
+   }, {
.name   = "spi",
.id = 0,
.parent = &clk_p,
diff --git a/arch/arm/mach-s3c64xx/include/mach/map.h 
b/arch/arm/mach-s3c64xx/include/mach/map.h
index e1eab3c..f57ff08 100644
--- a/arch/arm/mach-s3c64xx/include/mach/map.h
+++ b/arch/arm/mach-s3c64xx/include/mach/map.h
@@ -67,6 +67,7 @@
 #define S3C64XX_PA_USB_HSOTG   (0x7C00)
 #define S3C64XX_PA_WATCHDOG(0x7E004000)
 #define S3C64XX_PA_RTC (0x7E005000)
+#define S3C64XX_PA_KEYPAD  (0x7E00A000)
 #define S3C64XX_PA_ADC (0x7E00B000)
 #define S3C64XX_PA_SYSCON  (0x7E00F000)
 #define S3C64XX_PA_AC97(0x7F001000)
@@ -120,5 +121,6 @@
 #define S3C_PA_WDT S3C64XX_PA_WATCHDOG
 
 #define SAMSUNG_PA_ADC S3C64XX_PA_ADC
+#define SAMSUNG_PA_KEYPAD  S3C64XX_PA_KEYPAD
 
 #endif /* __ASM_ARCH_6400_MAP_H */
diff --git a/arch/arm/mach-s3c64xx/mach-smdk6410.c 
b/arch/arm/mach-s3c64xx/mach-smdk6410.c
index d9a0355..52dd17e 100644
--- a/arch/arm/mach-s3c64xx/mach-smdk6410.c
+++ b/arch/arm/mach-s3c64xx/mach-smdk6410.c
@@ -17,6 +17,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -66,6 +67,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define UCON S3C2410_UCON_DEFAULT | S3C2410_UCON_UCLK
 #define ULCON S3C2410_LCON_CS8 | S3C2410_LCON_PNONE | S3C2410_LCON_STOPB
@@ -242,6 +244,25 @@ static struct platform_device smdk6410_b_pwr_5v = {
 };
 #endif
 
+static uint32_t smdk6410_keymap[] __initdata = {
+   /* KEY(row, col, keycode) */
+   KEY(0, 3, KEY_1), KEY(0, 4, KEY_2), KEY(0, 5, KEY_3),
+   KEY(0, 6, KEY_4), KEY(0, 7, KEY_5),
+   KEY(1, 3, KEY_A), KEY(1, 4, KEY_B), KEY(1, 5, KEY_C),
+   KEY(1, 6, KEY_D), KEY(1, 7, KEY_E)
+};
+
+static struct matrix_keymap_data smdk6410_keymap_data __initdata = {
+   .keymap = smdk6410_keymap,
+   .keymap_size= ARRAY_SIZE(smdk6410_keymap),
+};
+
+static struct samsung_keypad_platdata smdk6410_keypad_data __initdata = {
+   .keymap_data= &smdk6410_keymap_data,
+   .rows   = 2,
+   .cols   = 8,
+};
+
 static struct map_desc smdk6410_iodesc[] = {};
 
 static struct platform_device *smdk6410_devices[] __initdata = {
@@ -257,6 +278,7 @@ static struct platform_device *smdk6410_dev

[PATCH 3/4] ARM: S5PC100: Rename keypad clock for S5PC100.

2010-06-21 Thread Kukjin Kim
From: Naveen Krishna Ch 

This patch renames the keypad clock for S5PC100 from keyif to keypad.

Signed-off-by: Naveen Krishna Ch 
Signed-off-by: Kukjin Kim 
---
 arch/arm/mach-s5pc100/clock.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-s5pc100/clock.c b/arch/arm/mach-s5pc100/clock.c
index e3fed4c..e1136f4 100644
--- a/arch/arm/mach-s5pc100/clock.c
+++ b/arch/arm/mach-s5pc100/clock.c
@@ -737,7 +737,7 @@ static struct clk init_clocks_disable[] = {
.enable = s5pc100_d1_5_ctrl,
.ctrlbit= (1 << 7),
}, {
-   .name   = "keyif",
+   .name   = "keypad",
.id = -1,
.parent = &clk_div_d1_bus.clk,
.enable = s5pc100_d1_5_ctrl,
-- 
1.6.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 4/4] ARM: S5PC100: Add keypad device to the SMDKC100 board

2010-06-21 Thread Kukjin Kim
From: Naveen Krishna Ch 

This patch is to support keypad device to the SMDKC100 board.

Signed-off-by: Naveen Krishna Ch 
Signed-off-by: Kukjin Kim 
---
 arch/arm/mach-s5pc100/Kconfig|7 ++
 arch/arm/mach-s5pc100/Makefile   |1 +
 arch/arm/mach-s5pc100/include/mach/map.h |3 +-
 arch/arm/mach-s5pc100/mach-smdkc100.c|   24 +
 arch/arm/mach-s5pc100/setup-keypad.c |   34 ++
 5 files changed, 68 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-s5pc100/setup-keypad.c

diff --git a/arch/arm/mach-s5pc100/Kconfig b/arch/arm/mach-s5pc100/Kconfig
index b2a11df..a3f5695 100644
--- a/arch/arm/mach-s5pc100/Kconfig
+++ b/arch/arm/mach-s5pc100/Kconfig
@@ -25,6 +25,11 @@ config S5PC100_SETUP_I2C1
help
  Common setup code for i2c bus 1.
 
+config S5PC100_SETUP_KEYPAD
+   bool
+   help
+ Common setup code for KEYPAD GPIO configurations.
+
 config S5PC100_SETUP_SDHCI
bool
select S5PC100_SETUP_SDHCI_GPIO
@@ -44,8 +49,10 @@ config MACH_SMDKC100
select S3C_DEV_HSMMC
select S3C_DEV_HSMMC1
select S3C_DEV_HSMMC2
+   select SAMSUNG_DEV_KEYPAD
select S5PC100_SETUP_FB_24BPP
select S5PC100_SETUP_I2C1
+   select S5PC100_SETUP_KEYPAD
select S5PC100_SETUP_SDHCI
help
  Machine support for the Samsung SMDKC100
diff --git a/arch/arm/mach-s5pc100/Makefile b/arch/arm/mach-s5pc100/Makefile
index 543f3de..69cfd34 100644
--- a/arch/arm/mach-s5pc100/Makefile
+++ b/arch/arm/mach-s5pc100/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_CPU_S5PC100) += dma.o
 
 obj-$(CONFIG_S5PC100_SETUP_FB_24BPP)   += setup-fb-24bpp.o
 obj-$(CONFIG_S5PC100_SETUP_I2C1)   += setup-i2c1.o
+obj-$(CONFIG_S5PC100_SETUP_KEYPAD) += setup-keypad.o
 obj-$(CONFIG_S5PC100_SETUP_SDHCI)  += setup-sdhci.o
 obj-$(CONFIG_S5PC100_SETUP_SDHCI_GPIO) += setup-sdhci-gpio.o
 
diff --git a/arch/arm/mach-s5pc100/include/mach/map.h 
b/arch/arm/mach-s5pc100/include/mach/map.h
index cadae43..eb57d7f 100644
--- a/arch/arm/mach-s5pc100/include/mach/map.h
+++ b/arch/arm/mach-s5pc100/include/mach/map.h
@@ -129,10 +129,11 @@
 #define S3C_PA_HSMMC0  S5PC100_PA_HSMMC(0)
 #define S3C_PA_HSMMC1  S5PC100_PA_HSMMC(1)
 #define S3C_PA_HSMMC2  S5PC100_PA_HSMMC(2)
-#define S3C_PA_KEYPAD  S5PC100_PA_KEYPAD
 #define S3C_PA_TSADC   S5PC100_PA_TSADC
 #define S3C_PA_ONENAND S5PC100_PA_ONENAND
 #define S3C_PA_ONENAND_BUF S5PC100_PA_ONENAND_BUF
 #define S3C_SZ_ONENAND_BUF S5PC100_SZ_ONENAND_BUF
 
+#define SAMSUNG_PA_KEYPAD  S5PC100_PA_KEYPAD
+
 #endif /* __ASM_ARCH_C100_MAP_H */
diff --git a/arch/arm/mach-s5pc100/mach-smdkc100.c 
b/arch/arm/mach-s5pc100/mach-smdkc100.c
index af22f82..d006873 100644
--- a/arch/arm/mach-s5pc100/mach-smdkc100.c
+++ b/arch/arm/mach-s5pc100/mach-smdkc100.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -42,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Following are default values for UCON, ULCON and UFCON UART registers */
 #define S5PC100_UCON_DEFAULT   (S3C2410_UCON_TXILEVEL |\
@@ -149,6 +151,25 @@ static struct s3c_fb_platdata smdkc100_lcd_pdata 
__initdata = {
.setup_gpio = s5pc100_fb_gpio_setup_24bpp,
 };
 
+static uint32_t smdkc100_keymap[] __initdata = {
+   /* KEY(row, col, keycode) */
+   KEY(0, 3, KEY_1), KEY(0, 4, KEY_2), KEY(0, 5, KEY_3),
+   KEY(0, 6, KEY_4), KEY(0, 7, KEY_5),
+   KEY(1, 3, KEY_A), KEY(1, 4, KEY_B), KEY(1, 5, KEY_C),
+   KEY(1, 6, KEY_D), KEY(1, 7, KEY_E)
+};
+
+static struct matrix_keymap_data smdkc100_keymap_data __initdata = {
+   .keymap = smdkc100_keymap,
+   .keymap_size= ARRAY_SIZE(smdkc100_keymap),
+};
+
+static struct samsung_keypad_platdata smdkc100_keypad_data __initdata = {
+   .keymap_data= &smdkc100_keymap_data,
+   .rows   = 2,
+   .cols   = 8,
+};
+
 static struct platform_device *smdkc100_devices[] __initdata = {
&s3c_device_i2c0,
&s3c_device_i2c1,
@@ -158,6 +179,7 @@ static struct platform_device *smdkc100_devices[] 
__initdata = {
&s3c_device_hsmmc2,
&smdkc100_lcd_powerdev,
&s5pc100_device_iis0,
+   &samsung_device_keypad,
&s5pc100_device_ac97,
 };
 
@@ -178,6 +200,8 @@ static void __init smdkc100_machine_init(void)
 
s3c_fb_set_platdata(&smdkc100_lcd_pdata);
 
+   samsung_keypad_set_platdata(&smdkc100_keypad_data);
+
/* LCD init */
gpio_request(S5PC100_GPD(0), "GPD");
gpio_request(S5PC100_GPH0(6), "GPH0");
diff --git a/arch/arm/mach-s5pc100/setup-keypad.c 
b/arch/arm/mach-s5pc100/setup-keypad.c
new file mode 100644
index 000..d0837a7
--- /dev/null
+++ b/arch/arm/mach-s5pc100/setup-keypad.c
@@ -0,0 +1,34 @@
+/* linux/arch/arm/mach-s5pc100/setup-keypad.c
+ *
+ * Copyright (c) 2010 Samsung Electronics Co., Ltd.
+ *  

[PATCH 1/4] ARM: S5PV210: Add keypad device to the SMDKV210 board

2010-06-21 Thread Kukjin Kim
From: Naveen Krishna Ch 

This patch is to support keypad device to the SMDKV210 board.

Signed-off-by: Naveen Krishna Ch 
Signed-off-by: Kukjin Kim 
---
 arch/arm/mach-s5pv210/Kconfig |2 ++
 arch/arm/mach-s5pv210/mach-smdkv210.c |   23 +++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
index 692d01c..4ae365d 100644
--- a/arch/arm/mach-s5pv210/Kconfig
+++ b/arch/arm/mach-s5pv210/Kconfig
@@ -77,8 +77,10 @@ config MACH_SMDKV210
select CPU_S5PV210
select ARCH_SPARSEMEM_ENABLE
select SAMSUNG_DEV_ADC
+   select SAMSUNG_DEV_KEYPAD
select SAMSUNG_DEV_TS
select S3C_DEV_WDT
+   select S5PV210_SETUP_KEYPAD
select HAVE_S3C2410_WATCHDOG
help
  Machine support for Samsung SMDKV210
diff --git a/arch/arm/mach-s5pv210/mach-smdkv210.c 
b/arch/arm/mach-s5pv210/mach-smdkv210.c
index 0d46279..43bc10a 100644
--- a/arch/arm/mach-s5pv210/mach-smdkv210.c
+++ b/arch/arm/mach-s5pv210/mach-smdkv210.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Following are default values for UCON, ULCON and UFCON UART registers */
 #define S5PV210_UCON_DEFAULT   (S3C2410_UCON_TXILEVEL |\
@@ -73,10 +74,30 @@ static struct s3c2410_uartcfg smdkv210_uartcfgs[] 
__initdata = {
},
 };
 
+static uint32_t smdkv210_keymap[] __initdata = {
+   /* KEY(row, col, keycode) */
+   KEY(0, 3, KEY_1), KEY(0, 4, KEY_2), KEY(0, 5, KEY_3),
+   KEY(0, 6, KEY_4), KEY(0, 7, KEY_5),
+   KEY(1, 3, KEY_A), KEY(1, 4, KEY_B), KEY(1, 5, KEY_C),
+   KEY(1, 6, KEY_D), KEY(1, 7, KEY_E)
+};
+
+static struct matrix_keymap_data smdkv210_keymap_data __initdata = {
+   .keymap = smdkv210_keymap,
+   .keymap_size= ARRAY_SIZE(smdkv210_keymap),
+};
+
+static struct samsung_keypad_platdata smdkv210_keypad_data __initdata = {
+   .keymap_data= &smdkv210_keymap_data,
+   .rows   = 8,
+   .cols   = 8,
+};
+
 static struct platform_device *smdkv210_devices[] __initdata = {
&s5pv210_device_iis0,
&s5pv210_device_ac97,
&s3c_device_adc,
+   &samsung_device_keypad,
&s3c_device_ts,
&s3c_device_wdt,
 };
@@ -96,7 +117,9 @@ static void __init smdkv210_map_io(void)
 
 static void __init smdkv210_machine_init(void)
 {
+   samsung_keypad_set_platdata(&smdkv210_keypad_data);
s3c24xx_ts_set_platdata(&s3c_ts_platform);
+
platform_add_devices(smdkv210_devices, ARRAY_SIZE(smdkv210_devices));
 }
 
-- 
1.6.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 0/4] Add keypad support for SMDK6410, SMDKC100 and SMDKV210

2010-06-21 Thread Kukjin Kim
This patches add keypad support for SMDK6410, SMDKC100 and SMDKV210.

NOTE: depends on Joonyoung Shim's 'Add samsung keypad'

[PATCH 1/4] ARM: S5PV210: Add keypad device to the SMDKV210 board
[PATCH 2/4] ARM: S3C64XX: Add keypad device to the SMDK6410 board
[PATCH 3/4] ARM: S5PC100: Rename keypad clock for S5PC100.
[PATCH 4/4] ARM: S5PC100: Add keypad device to the SMDKC100 board
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] input: samsung-keypad - Add samsung keypad driver

2010-06-21 Thread Kukjin Kim
From: Joonyoung Shim 

This patch adds support for keypad driver running on Samsung SoCs. This
driver is tested on SMDK6410(S3C6410), SMDKC100(S5PC100), Aquila(S5PC110),
GONI(S5PC110) and SMDKV210(S5PV210) boards.

Signed-off-by: Joonyoung Shim 
Signed-off-by: Kyungmin Park 
Tested-by: Naveen Krishna Ch 
[kgene@samsung.com: added comments]
Acked-by: Kukjin Kim 
Signed-off-by: Kukjin Kim 
---
 drivers/input/keyboard/Kconfig  |9 +
 drivers/input/keyboard/Makefile |1 +
 drivers/input/keyboard/samsung-keypad.c |  400 +++
 3 files changed, 410 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/keyboard/samsung-keypad.c

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index d8fa5d7..bf6a50f 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -342,6 +342,15 @@ config KEYBOARD_PXA930_ROTARY
  To compile this driver as a module, choose M here: the
  module will be called pxa930_rotary.
 
+config KEYBOARD_SAMSUNG
+   tristate "Samsung keypad support"
+   depends on SAMSUNG_DEV_KEYPAD
+   help
+ Say Y here if you want to use the Samsung keypad.
+
+ To compile this driver as a module, choose M here: the
+ module will be called samsung-keypad.
+
 config KEYBOARD_STOWAWAY
tristate "Stowaway keyboard"
select SERIO
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 4596d0c..8f973ed 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_KEYBOARD_OPENCORES)  += opencores-kbd.o
 obj-$(CONFIG_KEYBOARD_PXA27x)  += pxa27x_keypad.o
 obj-$(CONFIG_KEYBOARD_PXA930_ROTARY)   += pxa930_rotary.o
 obj-$(CONFIG_KEYBOARD_QT2160)  += qt2160.o
+obj-$(CONFIG_KEYBOARD_SAMSUNG) += samsung-keypad.o
 obj-$(CONFIG_KEYBOARD_SH_KEYSC)+= sh_keysc.o
 obj-$(CONFIG_KEYBOARD_STOWAWAY)+= stowaway.o
 obj-$(CONFIG_KEYBOARD_SUNKBD)  += sunkbd.o
diff --git a/drivers/input/keyboard/samsung-keypad.c 
b/drivers/input/keyboard/samsung-keypad.c
new file mode 100644
index 000..4b56e6f
--- /dev/null
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -0,0 +1,400 @@
+/*
+ * samsung-keypad.c  --  Samsung keypad driver
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim 
+ * Author: Donghwa Lee 
+ *
+ *  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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+enum samsung_keypad_type {
+   KEYPAD_TYPE_SAMSUNG,
+   KEYPAD_TYPE_S5PV210,
+};
+
+struct samsung_keypad {
+   struct input_dev *input_dev;
+   struct clk *clk;
+   struct delayed_work work;
+   void __iomem *base;
+   unsigned short *keycodes;
+   unsigned int row_shift;
+   unsigned int rows;
+   unsigned int cols;
+   unsigned int row_state[SAMSUNG_MAX_COLS];
+   int irq;
+};
+
+static int samsung_keypad_is_s5pv210(struct device *dev)
+{
+   struct platform_device *pdev = to_platform_device(dev);
+   enum samsung_keypad_type type;
+
+   type = platform_get_device_id(pdev)->driver_data;
+   return type == KEYPAD_TYPE_S5PV210;
+}
+
+static void samsung_keypad_scan(struct samsung_keypad *keypad,
+   unsigned int *row_state)
+{
+   struct device *dev = keypad->input_dev->dev.parent;
+   unsigned int col;
+   unsigned int val;
+
+   for (col = 0; col < keypad->cols; col++) {
+   if (samsung_keypad_is_s5pv210(dev)) {
+   val = S5PV210_KEYIFCOLEN_MASK;
+   val &= ~(1 << col) << 8;
+   } else {
+   val = SAMSUNG_KEYIFCOL_MASK;
+   val &= ~(1 << col);
+   }
+
+   writel(val, keypad->base + SAMSUNG_KEYIFCOL);
+   mdelay(1);
+
+   val = readl(keypad->base + SAMSUNG_KEYIFROW);
+   row_state[col] = ~val & ((1 << keypad->rows) - 1);
+   }
+
+   /* KEYIFCOL reg clear */
+   writel(0, keypad->base + SAMSUNG_KEYIFCOL);
+}
+
+static void samsung_keypad_worker(struct work_struct *work)
+{
+   struct samsung_keypad *keypad = container_of(work,
+   struct samsung_keypad, work.work);
+   unsigned int row_state[SAMSUNG_MAX_COLS];
+   unsigned int val;
+   unsigned int changed;
+   unsigned int pressed;
+   unsigned int key_down = 0;
+   int col, row;
+
+   clk_enable(keypad->clk);
+
+   val = readl(keypad->base + SAMSUNG_KEYIFSTSCLR);
+
+   /* interrupt clear */
+   writel(~0x0, keyp

Add Samsung Keypad driver

2010-06-21 Thread Kukjin Kim
Following is for Samsung Keypad driver.
Tested on SMDK6410(S3C6410), SMDKC100(S5PC100), Aquila(S5PC110),
GONI(S5PC110) and SMDKV210(S5PV210) boards.

Please apply.

Thanks.

[PATCH] input: samsung-keypad - Add samsung keypad driver
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Eric Miao
>> and now in s5pc200 code, you can also register/re-use the s5pc100_keypad
>> device/driver, and everyone knows - OK, the IP on S5PC100 is now re-used
>> here.
>>
>
> This is naming issue and i know it was not desided yet.
> This keypad driver can support s3c64xx, s5pc100, s5pc110, s5pv210.
>

I bet the first silicon this IP is deployed is s3c64xx, so my understanding
's3c64xx-keypad.c' should be an acceptable name.  And it's also reasonable,
a s3c64xx-keypad driver, and is later expanded to support subsequent silicons
without the file name being changed.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Eric Miao
On Mon, Jun 21, 2010 at 7:16 PM, Russell King - ARM Linux
 wrote:
> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>  wrote:
>> > On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>> >> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim  
>> >> wrote:
>> >> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata 
>> >> > *pd)
>> >> > +{
>> >> > +       struct samsung_keypad_platdata *npd;
>> >> > +
>> >> > +       if (!pd) {
>> >> > +               printk(KERN_ERR "%s: no platform data\n", __func__);
>> >> > +               return;
>> >> > +       }
>> >> > +
>> >> > +       npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), 
>> >> > GFP_KERNEL);
>> >> > +       if (!npd)
>> >> > +               printk(KERN_ERR "%s: no memory for platform data\n", 
>> >> > __func__);
>> >>
>> >> This part of the code is actually duplicated again and again and again
>> >> for each device, PXA and other legacy platforms are bad references for
>> >> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>> >> major points:
>> >>
>> >>  1. A minimum 'struct pxa_device_desc' for a simple description of a
>> >>     device (more than 90% of the devices can be described that way),
>> >>     and avoid using a comparatively heavier weight platform_device,
>> >>     which can be generated at run-time
>> >>
>> >>  2. pxa_register_device() to allocate and register the platform_device
>> >>     at run-time, along with the platform data
>> >
>> > It's a bad idea to make platform data be run-time discardable like this:
>> >
>> >> > +struct samsung_keypad_platdata {
>> >> > +       const struct matrix_keymap_data *keymap_data;
>> >
>> > What you end up with is some platform data structures which must be kept
>> > (those which have pointers to them from the platform data), and others
>> > (the platform data itself) which can be discarded at runtime.
>> >
>> > We know that the __initdata attributations cause lots of problems -
>> > they're frequently wrong.  Just see the constant hastle with __devinit
>> > et.al.  The same issue happens with __initdata as well.
>> >
>> > So why make things more complicated by allowing some platform data
>> > structures to be discardable and others not to be?  Is their small
>> > size (maybe 6 words for this one) really worth the hastle of getting
>> > __initdata attributations wrong (eg, on the keymap data?)
>> >
>>
>> Russell,
>>
>> The benefit I see is when multiple boards are compiled in, those
>> data not used can be automatically discarded.
>
> Yes, but only some of the data can be discarded.  Continuing with the
> example in hand, while you can discard the six words which represent
> samsung_keypad_platdata, but the keymap_data can't be because that won't
> be re-allocated, which is probably a much larger data structure.
>
> So, samsung_keypad_platdata can be marked with __initdata, but the keymap
> data must not be - and this is where the confusion about what can be
> marked with __initdata starts - and eventually leads to the keymap data
> also being mistakenly marked __initdata.

Indeed. One ideal solution would be to make all the board code into
a module, yet since some of the code should be executed really
early and need to stay as built-in, looks like we need a way to
separate these two parts.

>
> Is saving six words (or so) worth the effort to track down stuff
> inappropriately marked with __initdata ?  I'm sure there's bigger savings
> to be had in other parts of the kernel (such as shrinking the size of
> tcp hash tables, reducing the kernel message ring buffer, etc) if size is
> really a concern.
>

That's true, however, I think the size will become a bit more significant
if more and more board code is compiled into a single kernel.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Joonyoung Shim
On 6/21/2010 6:05 PM, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim  
> wrote:
>> This patch adds samsung keypad device definition for samsung cpus.
>>
>> Signed-off-by: Joonyoung Shim 
>> Signed-off-by: Kyungmin Park 
>> ---
>>  arch/arm/plat-samsung/Kconfig|5 ++
>>  arch/arm/plat-samsung/Makefile   |1 +
>>  arch/arm/plat-samsung/dev-keypad.c   |   58 
>> +
> 
> Why need an individual file for a simple device?  In the end, these files
> will flood over the plat-samsung/ directory. And now think this - in your
> new SoC design ,the keypad IP is replaced with a completely new one, does
> that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
> 
> I personally prefer a single devices.c for all the devices, if you
> orgnize well, shouldn't take up much code size in that single file.
> 

I know, but will need more effort of other device as well as keypad to
do it.

>>  arch/arm/plat-samsung/include/plat/devs.h|2 +
>>  arch/arm/plat-samsung/include/plat/keypad.h  |   59 
>> ++
>>  arch/arm/plat-samsung/include/plat/regs-keypad.h |   49 ++
> 
> Will these registers be used elsewhere except within the keypad driver?
> If no, they might be better moved to the keypad driver itself, it makes
> the driver more self-contained and avoid the registers being accessed
> anywhere.
> 

Right, these registers are used only in the keypad driver. I think it's 
possible.

>>  6 files changed, 174 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/arm/plat-samsung/dev-keypad.c
>>  create mode 100644 arch/arm/plat-samsung/include/plat/keypad.h
>>  create mode 100644 arch/arm/plat-samsung/include/plat/regs-keypad.h
>>
>> diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
>> index 2753fb3..bd007e3 100644
>> --- a/arch/arm/plat-samsung/Kconfig
>> +++ b/arch/arm/plat-samsung/Kconfig
>> @@ -227,6 +227,11 @@ config SAMSUNG_DEV_TS
>>help
>>Common in platform device definitions for touchscreen device
>>
>> +config SAMSUNG_DEV_KEYPAD
>> +   bool
>> +   help
>> + Compile in platform device definitions for keypad
>> +
>>  # DMA
>>
>>  config S3C_DMA
>> diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
>> index b1d82cc..8269d80 100644
>> --- a/arch/arm/plat-samsung/Makefile
>> +++ b/arch/arm/plat-samsung/Makefile
>> @@ -48,6 +48,7 @@ obj-$(CONFIG_S3C_DEV_RTC) += dev-rtc.o
>>
>>  obj-$(CONFIG_SAMSUNG_DEV_ADC)  += dev-adc.o
>>  obj-$(CONFIG_SAMSUNG_DEV_TS)   += dev-ts.o
>> +obj-$(CONFIG_SAMSUNG_DEV_KEYPAD)   += dev-keypad.o
>>
>>  # DMA support
>>
>> diff --git a/arch/arm/plat-samsung/dev-keypad.c 
>> b/arch/arm/plat-samsung/dev-keypad.c
>> new file mode 100644
>> index 000..679b444
>> --- /dev/null
>> +++ b/arch/arm/plat-samsung/dev-keypad.c
>> @@ -0,0 +1,58 @@
>> +/*
>> + * linux/arch/arm/plat-samsung/dev-keypad.c
>> + *
>> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
>> + * Author: Joonyoung Shim 
>> + *
>> + *  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.
>> + *
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +static struct resource samsung_keypad_resources[] = {
>> +   [0] = {
>> +   .start  = SAMSUNG_PA_KEYPAD,
>> +   .end= SAMSUNG_PA_KEYPAD + 0x20 - 1,
>> +   .flags  = IORESOURCE_MEM,
>> +   },
>> +   [1] = {
>> +   .start  = IRQ_KEYPAD,
>> +   .end= IRQ_KEYPAD,
>> +   .flags  = IORESOURCE_IRQ,
>> +   },
>> +};
>> +
>> +struct platform_device samsung_device_keypad = {
>> +   .name   = "samsung-keypad",
>> +   .id = -1,
>> +   .num_resources  = ARRAY_SIZE(samsung_keypad_resources),
>> +   .resource   = samsung_keypad_resources,
>> +};
>> +
>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>> +{
>> +   struct samsung_keypad_platdata *npd;
>> +
>> +   if (!pd) {
>> +   printk(KERN_ERR "%s: no platform data\n", __func__);
>> +   return;
>> +   }
>> +
>> +   npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), 
>> GFP_KERNEL);
>> +   if (!npd)
>> +   printk(KERN_ERR "%s: no memory for platform data\n", 
>> __func__);
> 
> This part of the code is actually duplicated again and again and again
> for each device, PXA and other legacy platforms are bad references for
> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> major points:
> 

I know ben posted patches to remove duplicated codes such this.
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg01474.

Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Russell King - ARM Linux
On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>  wrote:
> > On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
> >> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim  
> >> wrote:
> >> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata 
> >> > *pd)
> >> > +{
> >> > +       struct samsung_keypad_platdata *npd;
> >> > +
> >> > +       if (!pd) {
> >> > +               printk(KERN_ERR "%s: no platform data\n", __func__);
> >> > +               return;
> >> > +       }
> >> > +
> >> > +       npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), 
> >> > GFP_KERNEL);
> >> > +       if (!npd)
> >> > +               printk(KERN_ERR "%s: no memory for platform data\n", 
> >> > __func__);
> >>
> >> This part of the code is actually duplicated again and again and again
> >> for each device, PXA and other legacy platforms are bad references for
> >> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> >> major points:
> >>
> >>  1. A minimum 'struct pxa_device_desc' for a simple description of a
> >>     device (more than 90% of the devices can be described that way),
> >>     and avoid using a comparatively heavier weight platform_device,
> >>     which can be generated at run-time
> >>
> >>  2. pxa_register_device() to allocate and register the platform_device
> >>     at run-time, along with the platform data
> >
> > It's a bad idea to make platform data be run-time discardable like this:
> >
> >> > +struct samsung_keypad_platdata {
> >> > +       const struct matrix_keymap_data *keymap_data;
> >
> > What you end up with is some platform data structures which must be kept
> > (those which have pointers to them from the platform data), and others
> > (the platform data itself) which can be discarded at runtime.
> >
> > We know that the __initdata attributations cause lots of problems -
> > they're frequently wrong.  Just see the constant hastle with __devinit
> > et.al.  The same issue happens with __initdata as well.
> >
> > So why make things more complicated by allowing some platform data
> > structures to be discardable and others not to be?  Is their small
> > size (maybe 6 words for this one) really worth the hastle of getting
> > __initdata attributations wrong (eg, on the keymap data?)
> >
> 
> Russell,
> 
> The benefit I see is when multiple boards are compiled in, those
> data not used can be automatically discarded.

Yes, but only some of the data can be discarded.  Continuing with the
example in hand, while you can discard the six words which represent
samsung_keypad_platdata, but the keymap_data can't be because that won't
be re-allocated, which is probably a much larger data structure.

So, samsung_keypad_platdata can be marked with __initdata, but the keymap
data must not be - and this is where the confusion about what can be
marked with __initdata starts - and eventually leads to the keymap data
also being mistakenly marked __initdata.

Is saving six words (or so) worth the effort to track down stuff
inappropriately marked with __initdata ?  I'm sure there's bigger savings
to be had in other parts of the kernel (such as shrinking the size of
tcp hash tables, reducing the kernel message ring buffer, etc) if size is
really a concern.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Eric Miao
On Mon, Jun 21, 2010 at 5:29 PM, Marek Szyprowski
 wrote:
> Hello,
>
> On Monday, June 21, 2010 11:06 AM Eric Miao wrote:
>
>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim 
>> wrote:
>> > This patch adds samsung keypad device definition for samsung cpus.
>> >
>> > Signed-off-by: Joonyoung Shim 
>> > Signed-off-by: Kyungmin Park 
>> > ---
>> >  arch/arm/plat-samsung/Kconfig                    |    5 ++
>> >  arch/arm/plat-samsung/Makefile                   |    1 +
>> >  arch/arm/plat-samsung/dev-keypad.c               |   58
>> +
>>
>> Why need an individual file for a simple device?  In the end, these files
>> will flood over the plat-samsung/ directory. And now think this - in your
>> new SoC design ,the keypad IP is replaced with a completely new one, does
>> that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
>>
>> I personally prefer a single devices.c for all the devices, if you
>> orgnize well, shouldn't take up much code size in that single file.
>
> A separate dev-abc.c files has been chosen by a platform maintainer. This
> has some advantages as some unused data can be easily not compiled into
> the kernel which is configured specially for a particular board.

This also can be done by something like below:

#if defined(CONFIG_S5P_KEYPAD) || defined(CONFIG_S5P_KEYPAD_MODULE)
struct platform_device s5p_device_keypad {

};
#endif

And if you want run-time discardable platform_device, you have to mark
this structure as __initdata, and duplicate that device when registering.

That's why in arch/arm/mach-mmp/, a light weight and descriptive
'struct pxa_device_desc' is introduced, so only those platform_devices
registered will be generated and registered.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Eric Miao
On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
 wrote:
> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim  
>> wrote:
>> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata 
>> > *pd)
>> > +{
>> > +       struct samsung_keypad_platdata *npd;
>> > +
>> > +       if (!pd) {
>> > +               printk(KERN_ERR "%s: no platform data\n", __func__);
>> > +               return;
>> > +       }
>> > +
>> > +       npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), 
>> > GFP_KERNEL);
>> > +       if (!npd)
>> > +               printk(KERN_ERR "%s: no memory for platform data\n", 
>> > __func__);
>>
>> This part of the code is actually duplicated again and again and again
>> for each device, PXA and other legacy platforms are bad references for
>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>> major points:
>>
>>  1. A minimum 'struct pxa_device_desc' for a simple description of a
>>     device (more than 90% of the devices can be described that way),
>>     and avoid using a comparatively heavier weight platform_device,
>>     which can be generated at run-time
>>
>>  2. pxa_register_device() to allocate and register the platform_device
>>     at run-time, along with the platform data
>
> It's a bad idea to make platform data be run-time discardable like this:
>
>> > +struct samsung_keypad_platdata {
>> > +       const struct matrix_keymap_data *keymap_data;
>
> What you end up with is some platform data structures which must be kept
> (those which have pointers to them from the platform data), and others
> (the platform data itself) which can be discarded at runtime.
>
> We know that the __initdata attributations cause lots of problems -
> they're frequently wrong.  Just see the constant hastle with __devinit
> et.al.  The same issue happens with __initdata as well.
>
> So why make things more complicated by allowing some platform data
> structures to be discardable and others not to be?  Is their small
> size (maybe 6 words for this one) really worth the hastle of getting
> __initdata attributations wrong (eg, on the keymap data?)
>

Russell,

The benefit I see is when multiple boards are compiled in, those
data not used can be automatically discarded.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Marek Szyprowski
Hello,

On Monday, June 21, 2010 11:06 AM Eric Miao wrote:

> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim 
> wrote:
> > This patch adds samsung keypad device definition for samsung cpus.
> >
> > Signed-off-by: Joonyoung Shim 
> > Signed-off-by: Kyungmin Park 
> > ---
> >  arch/arm/plat-samsung/Kconfig|5 ++
> >  arch/arm/plat-samsung/Makefile   |1 +
> >  arch/arm/plat-samsung/dev-keypad.c   |   58
> +
> 
> Why need an individual file for a simple device?  In the end, these files
> will flood over the plat-samsung/ directory. And now think this - in your
> new SoC design ,the keypad IP is replaced with a completely new one, does
> that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
> 
> I personally prefer a single devices.c for all the devices, if you
> orgnize well, shouldn't take up much code size in that single file.

A separate dev-abc.c files has been chosen by a platform maintainer. This
has some advantages as some unused data can be easily not compiled into
the kernel which is configured specially for a particular board.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center



--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Russell King - ARM Linux
On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim  
> wrote:
> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
> > +{
> > +       struct samsung_keypad_platdata *npd;
> > +
> > +       if (!pd) {
> > +               printk(KERN_ERR "%s: no platform data\n", __func__);
> > +               return;
> > +       }
> > +
> > +       npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), 
> > GFP_KERNEL);
> > +       if (!npd)
> > +               printk(KERN_ERR "%s: no memory for platform data\n", 
> > __func__);
> 
> This part of the code is actually duplicated again and again and again
> for each device, PXA and other legacy platforms are bad references for
> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> major points:
> 
>  1. A minimum 'struct pxa_device_desc' for a simple description of a
> device (more than 90% of the devices can be described that way),
> and avoid using a comparatively heavier weight platform_device,
> which can be generated at run-time
> 
>  2. pxa_register_device() to allocate and register the platform_device
> at run-time, along with the platform data

It's a bad idea to make platform data be run-time discardable like this:

> > +struct samsung_keypad_platdata {
> > +       const struct matrix_keymap_data *keymap_data;

What you end up with is some platform data structures which must be kept
(those which have pointers to them from the platform data), and others
(the platform data itself) which can be discarded at runtime.

We know that the __initdata attributations cause lots of problems -
they're frequently wrong.  Just see the constant hastle with __devinit
et.al.  The same issue happens with __initdata as well.

So why make things more complicated by allowing some platform data
structures to be discardable and others not to be?  Is their small
size (maybe 6 words for this one) really worth the hastle of getting
__initdata attributations wrong (eg, on the keymap data?)
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support

2010-06-21 Thread Eric Miao
On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim  wrote:
> This patch adds samsung keypad device definition for samsung cpus.
>
> Signed-off-by: Joonyoung Shim 
> Signed-off-by: Kyungmin Park 
> ---
>  arch/arm/plat-samsung/Kconfig                    |    5 ++
>  arch/arm/plat-samsung/Makefile                   |    1 +
>  arch/arm/plat-samsung/dev-keypad.c               |   58 +

Why need an individual file for a simple device?  In the end, these files
will flood over the plat-samsung/ directory. And now think this - in your
new SoC design ,the keypad IP is replaced with a completely new one, does
that mean a new dev-keypad-new1.c, dev-keypad-new2.c?

I personally prefer a single devices.c for all the devices, if you
orgnize well, shouldn't take up much code size in that single file.

>  arch/arm/plat-samsung/include/plat/devs.h        |    2 +
>  arch/arm/plat-samsung/include/plat/keypad.h      |   59 
> ++
>  arch/arm/plat-samsung/include/plat/regs-keypad.h |   49 ++

Will these registers be used elsewhere except within the keypad driver?
If no, they might be better moved to the keypad driver itself, it makes
the driver more self-contained and avoid the registers being accessed
anywhere.

>  6 files changed, 174 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/plat-samsung/dev-keypad.c
>  create mode 100644 arch/arm/plat-samsung/include/plat/keypad.h
>  create mode 100644 arch/arm/plat-samsung/include/plat/regs-keypad.h
>
> diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
> index 2753fb3..bd007e3 100644
> --- a/arch/arm/plat-samsung/Kconfig
> +++ b/arch/arm/plat-samsung/Kconfig
> @@ -227,6 +227,11 @@ config SAMSUNG_DEV_TS
>        help
>            Common in platform device definitions for touchscreen device
>
> +config SAMSUNG_DEV_KEYPAD
> +       bool
> +       help
> +         Compile in platform device definitions for keypad
> +
>  # DMA
>
>  config S3C_DMA
> diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
> index b1d82cc..8269d80 100644
> --- a/arch/arm/plat-samsung/Makefile
> +++ b/arch/arm/plat-samsung/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_S3C_DEV_RTC)     += dev-rtc.o
>
>  obj-$(CONFIG_SAMSUNG_DEV_ADC)  += dev-adc.o
>  obj-$(CONFIG_SAMSUNG_DEV_TS)   += dev-ts.o
> +obj-$(CONFIG_SAMSUNG_DEV_KEYPAD)       += dev-keypad.o
>
>  # DMA support
>
> diff --git a/arch/arm/plat-samsung/dev-keypad.c 
> b/arch/arm/plat-samsung/dev-keypad.c
> new file mode 100644
> index 000..679b444
> --- /dev/null
> +++ b/arch/arm/plat-samsung/dev-keypad.c
> @@ -0,0 +1,58 @@
> +/*
> + * linux/arch/arm/plat-samsung/dev-keypad.c
> + *
> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
> + * Author: Joonyoung Shim 
> + *
> + *  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.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static struct resource samsung_keypad_resources[] = {
> +       [0] = {
> +               .start  = SAMSUNG_PA_KEYPAD,
> +               .end    = SAMSUNG_PA_KEYPAD + 0x20 - 1,
> +               .flags  = IORESOURCE_MEM,
> +       },
> +       [1] = {
> +               .start  = IRQ_KEYPAD,
> +               .end    = IRQ_KEYPAD,
> +               .flags  = IORESOURCE_IRQ,
> +       },
> +};
> +
> +struct platform_device samsung_device_keypad = {
> +       .name           = "samsung-keypad",
> +       .id             = -1,
> +       .num_resources  = ARRAY_SIZE(samsung_keypad_resources),
> +       .resource       = samsung_keypad_resources,
> +};
> +
> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
> +{
> +       struct samsung_keypad_platdata *npd;
> +
> +       if (!pd) {
> +               printk(KERN_ERR "%s: no platform data\n", __func__);
> +               return;
> +       }
> +
> +       npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
> +       if (!npd)
> +               printk(KERN_ERR "%s: no memory for platform data\n", 
> __func__);

This part of the code is actually duplicated again and again and again
for each device, PXA and other legacy platforms are bad references for
this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
major points:

 1. A minimum 'struct pxa_device_desc' for a simple description of a
device (more than 90% of the devices can be described that way),
and avoid using a comparatively heavier weight platform_device,
which can be generated at run-time

 2. pxa_register_device() to allocate and register the platform_device
at run-time, along with the platform data

 3. pxa168_add_*() as a consistent/clearly named API, and with type
checking for the platform_data type (it's a bit difficult to che