On Sun, Mar 17, 2013 at 06:19:33AM -0700, Guenter Roeck wrote: > On Sun, Mar 17, 2013 at 01:39:20PM +0100, Jean Delvare wrote: > > On Sat, 16 Mar 2013 14:25:40 -0700, Guenter Roeck wrote: > > > On Sat, Mar 16, 2013 at 12:50:02PM -0700, Greg Kroah-Hartman wrote: > > > > On Sat, Mar 16, 2013 at 11:12:53AM -0700, Guenter Roeck wrote: > > > > > My use case is primarily for hwmon drivers. > > > > > > > > > > hwmon has a separate API call to register a driver with the hwmon subsystem, > > > > > which creates a separate hwmon device and provides the user space notification. > > > > > As the created attribute files are often conditional on device variant and device > > > > > configuration, I don't see how this could be done through a default attribute > > > > > list (even though it might be worthwhile exploring if it can be used for some > > > > > of the simpler drivers). > > > > > > > > The default attribute list functionality offers you the ability to have > > > > callbacks to your driver to validate if you really want this sysfs file > > > > to be created or not. Just use that like other subsystems do, then you > > > > will never have to be making these create and remove calls at all. > > > > > > I thought about it, but right now I have no idea how to make it work. > > > Initialization sequence in hwmon drivers is > > > probe(): > > > allocate and initialize local driver data structures > > > detect configuration and initialize hardware if necessary > > > create attribute files > > > register with hwmon subsystem > > > sometimes do additional work, such as enable interrupts > > > > > > If I use attribute_group of the device_driver structure to create the attribute > > > files, my understanding is that those would be created prior to calling > > > the probe function. > > > > > > This would be too early, since local data structures do not yet exist, and > > > the chip configuration is unknown and uninitialized. > > > > > > On the other side, attribute files must exist before hwmon_device_register() > > > is called, since otherwise userspace would get confused. > > > > I'd like to add something at this point. > > > > We have historically created the hwmon attributes in the hardware (i2c, > > platform...) device, and then created an empty hwmon class device on > > top of it so that libsensors etc. can locate all hardware monitoring > > chips on the system. This is probably wrong and this may explain the > > difference of views between Greg and Guenter. > > > > I suspect that ideally all hwmon-related attributes should belong to the > > hwmon-class device and not the physical device. Would doing so solve > > the problem of is_visible() needing chip-specific information that can > > only be gathered during probe()? Sure this is an interface change, but > > a few hwmon drivers already do it that way (the ones without an actual > > hardware device, e.g. ACPI thermal zones) and libsensors supports this > > since version 3.0.3, which was released in September 2008 - 4.5 years > > ago. > > > > This would require creating the attributes after calling > > hwmon_device_register() rather than before, but from the ongoing > > discussion I seem to understand that the driver core supports creating > > the attributes for us, possibly at the same time as the class device > > will be created. Would this solve the userspace timing issue? > > > This is what I had in mind as ultimate possibility when I created > the second API mentioned in my other e-mail. > > struct device *devm_hwmon_device_register(struct device *dev, > const struct attribute_group **groups) > > The attributes are still attached to dev (ie to the hardware device) > in my current code, but it should be possible to attach them to the > hwmon class device instead. > > Problem with that approach is that it makes drivers larger, not smaller, > at least if is_visible is needed. So it kind of defeats the purpose. > > We can go along that route anyway if people think it is the right or a better > approach, but I am not sure if it is worth it. I can send out the patches if > there is interest. I found an alternate way of converting drivers with optional attributes - don't use is_visible, but collect the attribute groups dynamically. With this, conversion results look much better. For lm90: drivers/hwmon/lm90.c | 91 +++++++++++++++++++++----------------------------- 1 file changed, 38 insertions(+), 53 deletions(-) Code size is reduced by ~550 bytes. With that, this approach is much more feasible. I'll figure out how to attach the attributes to the hwmon device and send out a new set of patches after I get it working. Thanks, Guenter _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
