On 06/06/2016 02:21 AM, thilo.cestonaro@xxxxxxxxxxxxxx wrote:
Hey Guenter!
Thanks for your patience and very detailed review!
I will change the code to address all you hints where no discussion is needed.
The others follow down here.
On Fr, 2016-06-03 at 22:51 -0700, Guenter Roeck wrote:
+#define FTSTEUTATES_WATCHDOG_RESOLUTION 60
+
60 seconds (minimum) resolution ? Really ? This is very unusual.
Will double check and talk to the hw guy.
+static ssize_t show_fan_fault(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ int index = to_sensor_dev_attr(devattr)->index;
+ struct ftsteutates_data *data = ftsteutates_update_device(dev);
+
+ return sprintf(buf, "%d\n", data->fan_present[index] == 1 ? 0 : 1);
A non-present fan does not indicate a fan fault.
For me a fan fault was a not existing fan, as the alarm is handled via fan_alarm.
What is the understanding of fan_fault and fan_alarm?
Some fan controllers can detect if a fan is faulty, for example if it consumes
power but does not turn. Unless you know for sure that a fan is faulty, don't
report that it is. A non-existing fan is definitely not faulty.
+static ssize_t show_fan_source(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ int index = to_sensor_dev_attr(devattr)->index;
+ struct ftsteutates_data *data = ftsteutates_update_device(dev);
+
+ if (data->fan_present[index]) {
+ if (data->fan_source[index] == 0xFF)
+ return sprintf(buf, "none\n");
+ else
+ return sprintf(buf, "%d\n", data->fan_source[index]);
+ } else
+ return sprintf(buf, "-\n");
is_visible() is your friend here. If the fan is not present, don't display
the attributes. If fan presence can change at runtime, just display 0.
I just tried to change to presence at runtime (pull the fan cable),
and it looks like I don't change. So I'll use the is_visible function of the
sysfs entries.
In general though I don't understand what fan presence has to do with
the the fan source. What is the fan source anyway ? This is a non-standard
attribute, so you will need to explain why it is needed and what data
it provides in detail.
The fan source represents the sensor number which actually defines the fan-speed.
And the present or not present check is just to show another value if not present.
Which will be unnecessary when the module uses is_visible.
+SENSOR_DEVICE_ATTR(overall_uptime, S_IRUGO, show_overall_uptime, NULL, 0);
+
I am not inclined to accept those (non-standard) attributes unless you make
a really good case why they are needed.
The chip has this functionality, so I wanted to provide access to it.
If this non-standard "export" of functionality disturbs anyone,
I have no problem with removing it.
+ err = watchdog_register_device(&ftsteutates_wdt);
+ if (err) {
+ dev_err(&client->dev,
+ "Registering watchdog device failed: %d\n", err);
+ goto exit_detach;
+ }
I don't really like combining those drivers. There should really be a means
(other than mfd) to write two separate drivers for a chip like this.
I'll have to research that a bit.
So export the watchdog part into a separate module?
Is the i2c client forwarded to another driver if a driver which uses it was found?
An mfd driver would handle that. As I said, I'll have to research if there is an
easier way to do it.
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
