Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
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
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
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
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
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
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.
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
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
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
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.
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
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
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
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
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
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
>> 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
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
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
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
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
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
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
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
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