Hi Hans,
On Fri, Sep 16, 2011 at 04:37:34AM -0400, Hans de Goede wrote:
> 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.
>
Excellent summary. Exactly my concerns as well. Another example for high complexity
is pmbus drivers, thoigh that is probably a bit extreme.
To get this patchset approved, I think we'll have to have a couple of the drivers
converted to the new interface, to see if it works and is flexible enough
to be useful, and if it really reduces complexity on the driver level.
> 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.
>
Not just temperatures. Oddly enough, some DC-DC converters report negative
currents under low load.
Another nitpick: The set function does not check for errors.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
