Hi,
On 09/16/2011 12:31 AM, Lucas Stach wrote:
v3: now checkpatch clean (doh!)
Many of the errors were caused by my attempt to make all these
definitions readable. Now I think I have a good compromise between
readability and making checkpatch happy.
This adds a new hwmon-core interface to centralize sysfs handling
and enable cooperation with other in-kernel drivers.
First of all a big thanks for working on this, having a way for other in-kernel
drivers to access hwmon devices is a much needed feature for proper gpu power
management.
However I'm afraid that the current implementation needs some work. My biggest
reason to nack this at this time is that it is not flexible enough.
My biggest concern wrt flexibility is that the current code assumes that
all temperature / voltage / fan sensors of a hwmon ic have the same capabilities
(and thus the same sysfs attributes) that is simply not true. On for example
the f71882fg (and other models supported by that driver), only one of the
3 - 9 voltage inputs has an alarm capability, and thus alarm and max attributes.
Another concern is that it always assumes 1 dimensional indexing, but if you
look at automatic fan control related sysfs attributes (which your code currently
does not seem to support), then there is 2 dimensional indexing going on there,
for each fan or temperature (depending on if the thresholds one can configure are
bound to a fan input/output, or to a temp input) one can set multiple thresholds
(and per threshold a corresponding pwm duty cycle). One could simply see this
as 1 dimensional with a lot of attributes, but it would be much better (and easier
from the driver pov) to implement this 2 dimensional.
For a good idea of how complicated a hwmon driver can grow when it comes to
having a wild mix of sysfs attributes, where the type of attributes can
vary from one channel to the next take a look at the f71882fg driver, if your
code can handle converting that to the new hwmon-core interface (and preferably
clean up the driver while doing so), then it has the needed flexibility.
An other issue you need to tackle is non continuous indexes. Some drivers have
the ability to handle large amounts of channels, for example the sch5636 has
upto 5 voltage inputs, 16 temperatures and 8 fans. Almost no motherboard
will use these all, and the BIOS should program the chip to disable unused
channels, we read this information back and only add sysfs attributes for
enabled channels, which can lead to having fan1 and fan6 but nothing in between.
Note that when you've per sensor rather then per channel caps, you can essentially
handle this for free, since an unused index can simply be a channel without any
caps.
And one last nitpick wrt the current implementation:
+static ssize_t get_num(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ struct hwmon_device_attribute *hw_attr = to_hwmon_device_attr(attr);
+ struct hwmon_device_instance *hw_dev = hw_attr->hw_dev_inst;
+ int value, ret;
+ ret = hw_dev->get_numeric_attr(hw_dev->inst_data,
+ hw_attr->attr_enum, attr->index,&value);
+ if (ret)
+ return ret;
+ ret = snprintf(buf, PAGE_SIZE, "%u\n", value);
value is a signed int, so that should be %d, and we really want that as
things like negative temperatures are possible.
Regards,
Hans
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
