Re: [PATCH] added kernel module for FTS sensor chip "Teutates"

[Guenter Roeck <linux@roeck-us.net>]

On 06/06/2016 02:21 AM, thilo.cestonaro@ts.fujitsu.com 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