Re: [PATCH v4] pwm: add sysfs interface

[Thierry Reding]

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: pgp00000.pgp
Description: PGP signature