From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [PATCH v3 03/10] of: Add PWM support. Date: Fri, 24 Feb 2012 16:58:31 +0000 Message-ID: <201202241658.32062.arnd@arndb.de> References: <1329923841-32017-1-git-send-email-thierry.reding@avionic-design.de> <201202231403.01614.arnd@arndb.de> <20120224064749.GB18786@avionic-0098.mockup.avionic-design.de> Mime-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20120224064749.GB18786-RM9K5IK7kjIQXX3q8xo1gnVAuStQJXxyR5q1nwbD4aMs9pC9oP6+/A@public.gmane.org> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Thierry Reding Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Sascha Hauer , Matthias Kaehlcke , Kurt Van Dijck , Rob Herring , Grant Likely , Colin Cross , Olof Johansson , Richard Purdie , Mark Brown , Mitch Bradley , Mike Frysinger , Eric Miao , Lars-Peter Clausen , Ryan Mallon List-Id: linux-tegra@vger.kernel.org On Friday 24 February 2012, Thierry Reding wrote: > * Arnd Bergmann wrote: > > On Thursday 23 February 2012, Thierry Reding wrote: > > > * Arnd Bergmann wrote: > [...] > > > > * Why not include the pwm_request() call in this and return the > > > > pwm_device directly? You said that you want to get rid of the > > > > pwm_id eventually, which is a good idea, but this interface still > > > > forces one to use it. > > > > > > Okay, that sounds sensible. I propose to rename the function to something like > > > of_request_pwm(). > > > > Sounds good. On second thought, I would actually prefer starting the name with pwm_ and making it independent of device tree. There might be other ways how to find the pwm_device from a struct device in the future, but it should always be possible using a device together with a string and/or numeric identifier, much in the same way that we can get a resource from a platform_device. Ideally, there would be a common theme behind finding a memory region, irq, gpio pin, clock, regulator, dma-channel and pwm or anything else that requires a link between two device nodes. > > > It would of course need an additional parameter (name) to > > > forward to pwm_request(). > > > > Not necessarily, it could use the dev_name(device) or the name > > of the property, or a combination of the two. > > The problem with that is that usually the device would be named something > generic like "pwm", while in case where the PWM is used for the backlight > it makes sense to label the PWM device "backlight". > > Looking at debugfs and seeing an entry "backlight" is much more straight- > forward than "pwm.0". I mean "pwm.0" doesn't carry any useful information > really, does it? But the device name would be from the device using the pwm, not the pwm controller, so it should be something more helpful, no? > > > > > +EXPORT_SYMBOL(of_get_named_pwm); > > > > > > > > EXPORT_SYMBOL_GPL? > > > > > > It was brought up at some point that it might be nice to allow non-GPL > > > drivers to use the PWM framework as well. I don't remember any discussion > > > resulting from the comment. Perhaps we should have that discussion now and > > > decide whether or not we want to keep it GPL-only or not. > > > > I would definitely use EXPORT_SYMBOL_GPL for all new code unless it > > replaces an earlier interface that was available as EXPORT_SYMBOL. > > I just grepped the code and noticed this: > > $ $ git grep -n 'EXPORT_SYMBOL.*(pwm_request)' > arch/arm/mach-vt8500/pwm.c:139:EXPORT_SYMBOL(pwm_request); > arch/arm/plat-mxc/pwm.c:183:EXPORT_SYMBOL(pwm_request); > arch/arm/plat-samsung/pwm.c:83:EXPORT_SYMBOL(pwm_request); > arch/unicore32/kernel/pwm.c:132:EXPORT_SYMBOL(pwm_request); > drivers/mfd/twl6030-pwm.c:156:EXPORT_SYMBOL(pwm_request); > drivers/misc/ab8500-pwm.c:108:EXPORT_SYMBOL(pwm_request); > drivers/pwm/core.c:262:EXPORT_SYMBOL_GPL(pwm_request); > > It seems like the legacy PWM API used to be non-GPL. Should I switch it back? > Also does it make sense to have something like of_request_pwm() GPL when the > rest of the API isn't? I guess the choice is to make between you and Sascha. The implementation is new, so you could pick EXPORT_SYMBOL_GPL, but you could also try to keep to the current API. Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Fri, 24 Feb 2012 16:58:31 +0000 Subject: [PATCH v3 03/10] of: Add PWM support. In-Reply-To: <20120224064749.GB18786@avionic-0098.mockup.avionic-design.de> References: <1329923841-32017-1-git-send-email-thierry.reding@avionic-design.de> <201202231403.01614.arnd@arndb.de> <20120224064749.GB18786@avionic-0098.mockup.avionic-design.de> Message-ID: <201202241658.32062.arnd@arndb.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Friday 24 February 2012, Thierry Reding wrote: > * Arnd Bergmann wrote: > > On Thursday 23 February 2012, Thierry Reding wrote: > > > * Arnd Bergmann wrote: > [...] > > > > * Why not include the pwm_request() call in this and return the > > > > pwm_device directly? You said that you want to get rid of the > > > > pwm_id eventually, which is a good idea, but this interface still > > > > forces one to use it. > > > > > > Okay, that sounds sensible. I propose to rename the function to something like > > > of_request_pwm(). > > > > Sounds good. On second thought, I would actually prefer starting the name with pwm_ and making it independent of device tree. There might be other ways how to find the pwm_device from a struct device in the future, but it should always be possible using a device together with a string and/or numeric identifier, much in the same way that we can get a resource from a platform_device. Ideally, there would be a common theme behind finding a memory region, irq, gpio pin, clock, regulator, dma-channel and pwm or anything else that requires a link between two device nodes. > > > It would of course need an additional parameter (name) to > > > forward to pwm_request(). > > > > Not necessarily, it could use the dev_name(device) or the name > > of the property, or a combination of the two. > > The problem with that is that usually the device would be named something > generic like "pwm", while in case where the PWM is used for the backlight > it makes sense to label the PWM device "backlight". > > Looking at debugfs and seeing an entry "backlight" is much more straight- > forward than "pwm.0". I mean "pwm.0" doesn't carry any useful information > really, does it? But the device name would be from the device using the pwm, not the pwm controller, so it should be something more helpful, no? > > > > > +EXPORT_SYMBOL(of_get_named_pwm); > > > > > > > > EXPORT_SYMBOL_GPL? > > > > > > It was brought up at some point that it might be nice to allow non-GPL > > > drivers to use the PWM framework as well. I don't remember any discussion > > > resulting from the comment. Perhaps we should have that discussion now and > > > decide whether or not we want to keep it GPL-only or not. > > > > I would definitely use EXPORT_SYMBOL_GPL for all new code unless it > > replaces an earlier interface that was available as EXPORT_SYMBOL. > > I just grepped the code and noticed this: > > $ $ git grep -n 'EXPORT_SYMBOL.*(pwm_request)' > arch/arm/mach-vt8500/pwm.c:139:EXPORT_SYMBOL(pwm_request); > arch/arm/plat-mxc/pwm.c:183:EXPORT_SYMBOL(pwm_request); > arch/arm/plat-samsung/pwm.c:83:EXPORT_SYMBOL(pwm_request); > arch/unicore32/kernel/pwm.c:132:EXPORT_SYMBOL(pwm_request); > drivers/mfd/twl6030-pwm.c:156:EXPORT_SYMBOL(pwm_request); > drivers/misc/ab8500-pwm.c:108:EXPORT_SYMBOL(pwm_request); > drivers/pwm/core.c:262:EXPORT_SYMBOL_GPL(pwm_request); > > It seems like the legacy PWM API used to be non-GPL. Should I switch it back? > Also does it make sense to have something like of_request_pwm() GPL when the > rest of the API isn't? I guess the choice is to make between you and Sascha. The implementation is new, so you could pick EXPORT_SYMBOL_GPL, but you could also try to keep to the current API. Arnd