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? > > +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? Again thanks for your answers and patience! :) Cheers, Thilo
<<attachment: smime.p7s>>