Message-ID: <4C6524EC.3020200@redhat.com>
Date:	Fri, 13 Aug 2010 12:56:44 +0200
From:	Hans de Goede <hdegoede@...hat.com>
To:	Giel van Schijndel <me@...tis.eu>
CC:	Laurens Leemans <laurens@...nips.com>,
	Jonathan Cameron <jic23@....ac.uk>,
	Randy Dunlap <rdunlap@...otime.net>,
	Jean Delvare <khali@...ux-fr.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	lm-sensors@...sensors.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH] hwmon: f71882fg: Add support for the Fintek F71808E

Hi,

On 08/04/2010 05:44 PM, Giel van Schijndel wrote:
> On Wed, Aug 04, 2010 at 13:36:22 +0200, Hans de Goede wrote:
>> On 08/01/2010 03:30, Giel van Schijndel wrote:
>>> Allow device probing to recognise the Fintek F71808E.
>>>
>>> Sysfs interface:
>>>    * Fan/pwm control is the same as for F71889FG
>>
>> My datasheet strongly disagrees with this the F71889FG has 5 pwm zones
>> each with their own speed divided by 4 boundary temps, where as
>> the F71808E has 3 pwm zones divided by 2 boundary temps. So it is much
>> more like the F71862FG, which also has 2 boundary temps, and 3 pwm zones,
>> *but* the F71862FG has one pwm zone hardwired to 100%.
>
> I'm assuming that by "pwm zone" you mean a separate PWM output channel?
> I.e. each "pwm zone" controls a single fan?
>

With pwm zone I mean the number of different speeds which can be programmed
for one output channel, the temps divide the entire temp range into zones
(number of zones == number of temps + 1) and for each zone one can then
tell at what speed / pwm setting the fan should operate when the temperature
is in that zone.


<snip>

>> Also while making changes, I must say I don't like the splitting
>> of fxxxx_temp_attr into fxxxx_temp_attr and f71862_temp_attr just because
>> the number of sensors differs. I think it would be better to instead
>> make fxxxx_temp_attr a 2 dimensional array like fxxxx_fan_attr and like
>> with fxxxx_fan_attr register as many sensor attr blocks as the specific
>> model has.
>
> Right, that's probably a nicer way of going about it, I think that might
> be easier done in a separate patch (most likely preceding the addition
> of F71808E support), though I'll look at that.
>

Yes first splitting the attr in a separate patch would be very good.

>>> Signed-off-by: Giel van Schijndel<me@...tis.eu>
>>> ---
>>>    Documentation/hwmon/f71882fg |    4 ++
>>>    drivers/hwmon/Kconfig        |    6 ++--
>>>    drivers/hwmon/f71882fg.c     |   83 ++++++++++++++++++++++++++++++++++++++----
>>>    3 files changed, 82 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/Documentation/hwmon/f71882fg b/Documentation/hwmon/f71882fg
>>> index a7952c2..1a07fd6 100644
>>> --- a/Documentation/hwmon/f71882fg
>>> +++ b/Documentation/hwmon/f71882fg
>>> @@ -2,6 +2,10 @@ Kernel driver f71882fg
>>>    ======================
>>>
>>>    Supported chips:
>>> +  * Fintek F71808E
>>> +    Prefix: 'f71808fg'
>>
>> This is wrong, as you already indicate and the datasheet as well this
>> chip in question is an f71808e not an f71808fg, also note that there is
>> an f71808a model as well which is different (and has a different super io
>> chip id).
>
> Ah yes, I think I, wrongly, assumed that 'fg' was just some suffix used
> in this driver.  For example, I cannot find F71889FG in the datasheet I
> have, only 'F71889' along with 'F71889F' in the section "Ordering
> Information" (for the F71889 I've got datasheet version V0.17P released
> December 2008).
>

I have a V0.27P datasheet for the 71889, but yes the fg suffix does not
seem to be mentioned anywhere in the datasheet not sure where it comes from.
I do know however that there are now new chips coming out with different
a and e suffixes so I suggest that we stay with fg for the old chips and
use a and e to distuingish the new ones.

> At the same time the F71808E datasheet I have clearly marks the chip as
> F71808E all over the entire datasheet (version V0.17P released October
> 2009).
>

Right.

> Either way I changed that ^^ portion of documentation while changing the
> enumeration value 'f71808fg' ->  'f71808e' in the code itself as well.
>

Good.

>> One last request in the second switch case in f71882fg_remove()
>> there is a default label which contains a comment which models it applies
>> to, please add the f71808e to that comment.
>
> Wouldn't it be better, to instead replace that 'default' label with a
> serie of case labels that code applies to?  Along with providing the
> same documentation effect (expressed in C instead of English) it would
> cause GCC to warn whenever one of the chips was forgotten in a switch
> statement.

Ack, if you could do that that would be great! Please do that
in a preparation patch though and not in the main patch.

<snip>

> PS For comparison, which datasheet versions do you have?
> All Fintek datasheets I have access to:
>   * F71808E - V0.17P, October 2009
>   * F71858  - V0.26P, July 2007
>   * F71862  - V0.28P, July 2008
>   * F71882  - V0.24P, November 2006
>   * F71889  - V0.17P, December 2008
>
> Those most interesting are of course the F71808E, F71862 and F71889---as
> you refer to those in your text.  This because I have already had
> experience with a hardware vendor giving me the wrong datasheets and
> would like to prevent any such mistakes from causing similar
> communication problems here.

Here is my list:
F71612A_V020P.pdf
F71808A_V0.15P.pdf
F71808E_V0.20P.pdf
F71858_V026P.pdf
F71862_V028P.pdf
F71869E_V0.19P.pdf
F71869_V1.1.pdf
F71882_V0.24P.pdf
F71889E_V0.19P.pdf
F71889_V0.27P.pdf
F8000_REG.pdf

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists