pwm: add sysfs interface

drivers/pwm/sysfs.c +0 -0

On Mon, Jun 10, 2013 at 04:12:07PM -0700, H Hartley Sweeten wrote:
[...]

Documentation/ABI/testing/sysfs-class-pwm line 62

Quoted 6 lines

Documentation/ABI/testing/sysfs-class-pwm new file +6 -0

+What:		/sys/class/pwm/pwmchipN/pwmX/duty
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <[email protected]>
+Description:
+		Sets the PWM duty cycle in nanoseconds.

Sorry, I should've been more specific before. I'd like this to be named duty_cycle. We now have the pwm_{set,get}_duty_cycle() accessors and I'd like to eventually use the spelled-out form consistently.

drivers/pwm/Kconfig menuconfig PWM · line 41

Quoted 11 lines

drivers/pwm/Kconfig +11 -0

+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.

In order not to burden you with too much work, one option for now would be to unconditionally build the sysfs.c file and use something along these lines in pwmchip_sysfs_{,un}export():

	if (!IS_ENABLED(CONFIG_PWM_SYSFS))
		return;

Which should allow the compiler to throw away all PWM_SYSFS-related code in that file, leaving an empty function.

It's not the optimal
solution, which would require the sysfs code to go into core.c and be conditionalized there, but it's good enough. I can always go and clean up that code later (maybe doing the same for DEBUG_FS while at it).

The big advantage of this is that we get full compile coverage of the sysfs interface, whether it's enabled or not. Calling an empty function once when the chip is registered is an acceptable overhead.

Quoted 12 lines · Thierry Reding

On Mon, Jun 10, 2013 at 04:12:07PM -0700, H Hartley Sweeten wrote:
[...]

Documentation/ABI/testing/sysfs-class-pwm line 62

Quoted 6 lines

Documentation/ABI/testing/sysfs-class-pwm new file +6 -0

+What:		/sys/class/pwm/pwmchipN/pwmX/duty
+Date:		May 2013
+KernelVersion:	3.11
+Contact:	H Hartley Sweeten <[email protected]>
+Description:
+		Sets the PWM duty cycle in nanoseconds.

Sorry, I should've been more specific before. I'd like this to be named duty_cycle. We now have the pwm_{set,get}_duty_cycle() accessors and I'd like to eventually use the spelled-out form consistently.

Quoted 23 lines

drivers/pwm/Kconfig menuconfig PWM · line 41

Quoted 11 lines

drivers/pwm/Kconfig +11 -0

+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.

In order not to burden you with too much work, one option for now would be to unconditionally build the sysfs.c file and use something along these lines in pwmchip_sysfs_{,un}export():

Quoted 5 lines

	if (!IS_ENABLED(CONFIG_PWM_SYSFS))
		return;

Which should allow the compiler to throw away all PWM_SYSFS-related code in that file, leaving an empty function.

Won't that also cause the compiler to spew a bunch of warnings about unreachable code in the !CONFIG_PWM_SYSFS case? We have the unreachable() macro, but that isn't supported nicely by all compilers.

If CONFIG_SYSFS is not enabled and sysfs.c is using functions that now do not exist, that will cause compile errors, since the compiler will still attempt to compile all of the code, even though it will remove most of it after doing so.

Also, any functions that are extern will also end up generating empty functions in the kernel binary (static linkage functions should disappear completely). This is obviously very negligible in size, but using a proper Kconfig option results in zero size if the option is compiled out.

Quoted 8 lines

It's not the optimal
solution, which would require the sysfs code to go into core.c and be conditionalized there, but it's good enough. I can always go and clean up that code later (maybe doing the same for DEBUG_FS while at it).

The big advantage of this is that we get full compile coverage of the sysfs interface, whether it's enabled or not. Calling an empty function once when the chip is registered is an acceptable overhead.

Why not just make CONFIG_PWM_SYSFS default y, so that if CONFIG_SYSFS is enabled (which should be true for the vast majority of test configs) that pwm sysfs is also enabled?

The IS_ENABLED method just seems very messy for a very small gain.

Quoted 12 lines · Ryan Mallon

Quoted 11 lines · Thierry Reding

Quoted 10 lines · H Hartley Sweeten

drivers/pwm/Kconfig +10 -0

+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

Quoted 8 lines

Quoted 8 lines

drivers/pwm/Kconfig menuconfig PWM · line 41

Quoted 1 line

drivers/pwm/Kconfig +1 -0

+	  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

Quoted 24 lines · H Hartley Sweeten

Quoted 12 lines · Ryan Mallon

Quoted 11 lines · Thierry Reding

Quoted 10 lines · H Hartley Sweeten

drivers/pwm/Kconfig +10 -0

+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

Quoted 8 lines

Quoted 8 lines

drivers/pwm/Kconfig menuconfig PWM · line 41

Quoted 1 line

drivers/pwm/Kconfig +1 -0

+	  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:

Quoted 4 lines

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.

Quoted 30 lines · H Hartley Sweeten

Quoted 28 lines · H Hartley Sweeten

Quoted 12 lines · Ryan Mallon

Quoted 11 lines · Thierry Reding

Quoted 10 lines · H Hartley Sweeten

drivers/pwm/Kconfig +10 -0

+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

Quoted 8 lines

Quoted 8 lines

drivers/pwm/Kconfig menuconfig PWM · line 41

Quoted 1 line

drivers/pwm/Kconfig +1 -0

+	  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

Quoted 2 lines

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.

Quoted 31 lines · Thierry Reding

Quoted 30 lines · H Hartley Sweeten

Quoted 28 lines · H Hartley Sweeten

Quoted 12 lines · Ryan Mallon

Quoted 11 lines · Thierry Reding

Quoted 10 lines · H Hartley Sweeten

(quoted diff) +10 -0

+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

Quoted 8 lines

Quoted 8 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	  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

Quoted 6 lines

Quoted 2 lines

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.

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).

Quoted 4 lines · Thierry Reding

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,

So you want a v6 of this patch or are you ok with v5?

Quoted 1 line · Thierry Reding

On Mon, Jun 10, 2013 at 04:12:07PM -0700, H Hartley Sweeten wrote:

Quoted 26 lines

drivers/pwm/Kconfig menuconfig PWM · line 41

Quoted 11 lines

drivers/pwm/Kconfig +11 -0

+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.

In order not to burden you with too much work, one option for now would be to unconditionally build the sysfs.c file and use something along these lines in pwmchip_sysfs_{,un}export():

	if (!IS_ENABLED(CONFIG_PWM_SYSFS))
		return;

Quoted 2 lines

Which should allow the compiler to throw away all PWM_SYSFS-related code in that file, leaving an empty function.

Won't that also cause the compiler to spew a bunch of warnings about unreachable code in the !CONFIG_PWM_SYSFS case? We have the unreachable() macro, but that isn't supported nicely by all compilers.

If CONFIG_SYSFS is not enabled and sysfs.c is using functions that now do not exist, that will cause compile errors, since the compiler will still attempt to compile all of the code, even though it will remove most of it after doing so.

Also, any functions that are extern will also end up generating empty functions in the kernel binary (static linkage functions should disappear completely). This is obviously very negligible in size, but using a proper Kconfig option results in zero size if the option is compiled out.

Quoted 6 lines

It's not the optimal
solution, which would require the sysfs code to go into core.c and be conditionalized there, but it's good enough. I can always go and clean up that code later (maybe doing the same for DEBUG_FS while at it).

Quoted 3 lines

The big advantage of this is that we get full compile coverage of the sysfs interface, whether it's enabled or not. Calling an empty function once when the chip is registered is an acceptable overhead.

Why not just make CONFIG_PWM_SYSFS default y, so that if CONFIG_SYSFS is enabled (which should be true for the vast majority of test configs) that pwm sysfs is also enabled?

The IS_ENABLED method just seems very messy for a very small gain.