From mboxrd@z Thu Jan 1 00:00:00 1970 From: rmallon@gmail.com (Ryan Mallon) Date: Thu, 12 Apr 2012 06:59:42 +1000 Subject: [PATCH] arm: ep93xx: use gpio_led_register_device In-Reply-To: References: <201204041042.22674.hartleys@visionengravers.com> <4F84E93C.1070004@gmail.com> Message-ID: <4F85F0BE.3060302@gmail.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 12/04/12 03:16, H Hartley Sweeten wrote: > On Tuesday, April 10, 2012 7:15 PM, Ryan Mallon wrote: >> On 05/04/12 03:42, H Hartley Sweeten wrote: >> >>> Use gpio_led_register_device to register the two leds connected to >>> the ep93xx. >>> >>> Add a SOC_EP93XX Kconfig option for common options needed by ep93xx >>> and use that option to select LEDS_GPIO_REGISTER. >>> >>> Signed-off-by: Hartley Sweeten >>> Cc: Ryan Mallon >> >> Hi Hartley, >> >> Just a couple of comments below. >> >> ~Ryan >> >>> --- >>> >>> arch/arm/mach-ep93xx/Kconfig | 12 ++++++++++++ >>> arch/arm/mach-ep93xx/core.c | 16 ++++------------ >>> 2 files changed, 16 insertions(+), 12 deletions(-) >>> >>> diff --git a/arch/arm/mach-ep93xx/Kconfig b/arch/arm/mach-ep93xx/Kconfig >>> index 97a2493..b27a8ad 100644 >>> --- a/arch/arm/mach-ep93xx/Kconfig >>> +++ b/arch/arm/mach-ep93xx/Kconfig >>> @@ -2,6 +2,10 @@ if ARCH_EP93XX >>> >>> menu "Cirrus EP93xx Implementation Options" >>> >>> +config SOC_EP93XX >>> + bool >>> + select LEDS_GPIO_REGISTER >>> + >> >> >> So, this option is currently just used to indirectly select >> LEDS_GPIO_REGISTER. Do you have plans for it to select other things? >>> Otherwise, its just a bunch of extra Kconfig lines for not much benefit. > > Yes, this option will be used to indirectly select common "generic" options for > the ep93xx. I think this is cleaner than putting them directly under ARCH_EP93XX > in arch/arm/Kconfig. > > EP93XX specific options are already handled in the various subsystems with the > "depends on ARCH_EP93XX", but for generic stuff we would need to either > update the defconfig or rely on the user to select the options. > > Currently, with the single option being selected, it is a bit of overkill. But as more > options (hopefully) get added it should be a benefit. Okay, if all of the boards select it then we can do what omap does for CONFIG_OMAP2PLUS_TYPICAL and make the option default y so that we don't need to select it individually for each board. >>> config CRUNCH >>> bool "Support for MaverickCrunch" >>> help >>> @@ -48,12 +52,14 @@ endchoice >>> config MACH_ADSSPHERE >>> bool "Support ADS Sphere" >>> depends on EP93XX_SDCE3_SYNC_PHYS_OFFSET >>> + select SOC_EP93XX >>> help >>> Say 'Y' here if you want your kernel to support the ADS >>> Sphere board. >>> >>> config MACH_EDB93XX >>> bool >>> + select SOC_EP93XX >>> >>> config MACH_EDB9301 >>> bool "Support Cirrus Logic EDB9301" >>> @@ -122,12 +128,14 @@ config MACH_EDB9315A >>> config MACH_GESBC9312 >>> depends on EP93XX_SDCE3_SYNC_PHYS_OFFSET >>> bool "Support Glomation GESBC-9312-sx" >>> + select SOC_EP93XX >>> help >>> Say 'Y' here if you want your kernel to support the Glomation >>> GESBC-9312-sx board. >>> >>> config MACH_MICRO9 >>> bool >>> + select SOC_EP93XX >>> >>> config MACH_MICRO9H >>> bool "Support Contec Micro9-High" >>> @@ -164,6 +172,7 @@ config MACH_MICRO9S >>> config MACH_SIM_ONE >>> bool "Support Simplemachines Sim.One board" >>> depends on EP93XX_SDCE0_PHYS_OFFSET >>> + select SOC_EP93XX >> >> >> The existing whitespace here is using spaces instead of tabs. If the >> result looks terrible (not aligned) then we should maybe do a separate >> patch to clean up the crappy whitespace. > > I noticed that also... Hmm... who used the spaces ;-) > > I agree, a separate patch should clean up the shitespace. I'm only really bothered if the mixed whitespace makes things look horrible. If you make the CONFIG_SOC_EP93XX option default y then this is no longer an issue anyway. >>> help >>> Say 'Y' here if you want your kernel to support the >>> Simplemachines Sim.One board. >>> @@ -171,6 +180,7 @@ config MACH_SIM_ONE >>> config MACH_SNAPPER_CL15 >>> bool "Support Bluewater Systems Snapper CL15 Module" >>> depends on EP93XX_SDCE0_PHYS_OFFSET >>> + select SOC_EP93XX >>> help >>> Say 'Y' here if you want your kernel to support the Bluewater >>> Systems Snapper CL15 Module. >>> @@ -178,6 +188,7 @@ config MACH_SNAPPER_CL15 >>> config MACH_TS72XX >>> bool "Support Technologic Systems TS-72xx SBC" >>> depends on EP93XX_SDCE3_SYNC_PHYS_OFFSET >>> + select SOC_EP93XX >>> help >>> Say 'Y' here if you want your kernel to support the >>> Technologic Systems TS-72xx board. >>> @@ -185,6 +196,7 @@ config MACH_TS72XX >>> config MACH_VISION_EP9307 >>> bool "Support Vision Engraving Systems EP9307 SoM" >>> depends on EP93XX_SDCE0_PHYS_OFFSET >>> + select SOC_EP93XX >>> help >>> Say 'Y' here if you want your kernel to support the >>> Vision Engraving Systems EP9307 SoM. >>> diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c >>> index 8d25895..257a124 100644 >>> --- a/arch/arm/mach-ep93xx/core.c >>> +++ b/arch/arm/mach-ep93xx/core.c >>> @@ -513,7 +513,7 @@ void __init ep93xx_register_spi(struct ep93xx_spi_info *info, >>> /************************************************************************* >>> * EP93xx LEDs >>> *************************************************************************/ >>> -static struct gpio_led ep93xx_led_pins[] = { >>> +static const struct gpio_led ep93xx_led_pins[] __initconst = { >> >> >> This fix, and related changes are not mentioned in the changelog. > > Sorry about that. From include/Linux/leds.h: > > struct platform_device *gpio_led_register_device( > int id, const struct gpio_led_platform_data *pdata); > > Since pdata needs to be const I changed the two relevant static variables to const. > And, since nothing should modify them I also made them __initconst. If you > feel this needs to be mentioned in the changelog I will resubmit the patch. Yeah, please mention it in the changelog. Thanks, ~Ryan