On Tue, Jun 11, 2013 at 01:45:37PM -0500, H Hartley Sweeten wrote: > On Tuesday, June 11, 2013 11:35 AM, Thierry Reding wrote: > > On Tue, Jun 11, 2013 at 11:47:23AM -0500, H Hartley Sweeten wrote: > >> On Tuesday, June 11, 2013 9:09 AM, H Hartley Sweeten wrote: > >>> On Tuesday, June 11, 2013 4:29 AM, Ryan Mallon wrote: > >>>> On 11/06/13 20:14, Thierry Reding wrote: > >>>>> On Mon, Jun 10, 2013 at 04:12:07PM -0700, H Hartley Sweeten wrote: > >>>>>> +config PWM_SYSFS > >>>>>> + bool "/sys/class/pwm/... (sysfs interface)" > >>>>>> + depends on SYSFS > >>>>>> + help > >>>>>> + Say Y here to provide a sysfs interface to control PWMs. > >>>>>> + > >>>>>> + For every instance of a PWM device there is a pwmchipN directory > >>>>>> + created in /sys/class/pwm. Use the export attribute to request > >>>>>> + a PWM to be accessible from userspace and the unexport attribute > >>>>>> + to return the PWM to the kernel. Each exported PWM will have a > >>>>>> + pwmX directory in the pwmchipN it is associated with. > >>>>> > >>>>> I have a small quibble with this. Introducing options like this make it > >>>>> increasingly difficult to compile-test all the various combinations, so > >>>>> I'd like to see this converted to a form that will play well with the > >>>>> IS_ENABLED() macro. We already have the same issue with DEBUG_FS, only > >>>>> to a lesser degree because it doesn't have an additional PWM-specific > >>>>> Kconfig option. > >>> > >>> How about removing the Kconfig option and just doing: > >>> > >>> obj-$(CONFIG_SYSFS) += sysfs.o > >>> > >>> This way the PWM sysfs interface is always compiled and included in the build > >>> as long as CONFIG_SYSFS is enabled. The check in the header would change to > >> > >> That didn't work. As Ryan pointed out we get undefined references due to > >> sysfs.c being compiled but not core.c when CONFIG_PWM is not enabled. > > > > Why not add dummies for the missing functions? It was my impression that > > we had dummies for all of them already, but if not they should certainly > > be added to match what other subsystems do. > > I just looked it over and the undefined reference was for pwm_set_polarity(). > The dummy for that function is missing. > > The other problem with this approach is the sysfs.c file gets compiled whenever > CONFIG_SYSFS is enabled even if CONFIG_PWM is not enabled. Perhaps add something like this to the Makefile: ifeq ($(CONFIG_PWM),y) obj-$(CONFIG_SYSFS) += sysfs.o endif That should solve the problem of building sysfs.c if SYSFS && !PWM. I'm tempted to take your v5 patch and make a note to clean that up separately at some point (along with similar changes for the DEBUG_FS support). Thierry
Attachment:
pgpCoTU0WWULT.pgp
Description: PGP signature
