Prev: [PATCH 3/3 v2] cyber2000fb: add I2C support
Next: act_nat: the checksum of ICMP doesn't have pseudo header
From: Giel van Schijndel on 10 Aug 2010 15:20
On Wed, Aug 04, 2010 at 01:36:22PM +0200, Hans de Goede wrote:
> On 08/01/2010 03:30 PM, 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 ...

I just noticed this patch was applied to mainline anyway. Regardless, I
will (try to) address these issues you raised.

Right now however, I am prioritising personal stuff above this
driver---bachelor's thesis and graduation presentation. When finished
with that (september) I'll allocate time to these issues again.

PS Those datasheets are written very poorly, although I have seen worse.
And as a reader I tend to become more "lazy" when I discover the
writer was "lazy" (not exactly by choice, more out of habit).
Unfortunately that doesn't work very well as reading style with
technical documentation, so this probably explains some of the errors
in this current patch.

PPS I do have a patch ready that addresses some of the issues you
raised. Those are only the cosmetic changes though (e.g. change
naming of chip, comment updates, etc.). Would you suggest me to
send it right now already or wait until I have time to address the
other issues as well?

--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
From: Hans de Goede on 13 Aug 2010 06:00
Hi,

On 08/10/2010 09:11 PM, Giel van Schijndel wrote:
> On Wed, Aug 04, 2010 at 01:36:22PM +0200, Hans de Goede wrote:
>> On 08/01/2010 03:30 PM, 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 ...
>
> I just noticed this patch was applied to mainline anyway. Regardless, I
> will (try to) address these issues you raised.
>
> Right now however, I am prioritising personal stuff above this
> driver---bachelor's thesis and graduation presentation. When finished
> with that (september) I'll allocate time to these issues again.
>

I've send a mail directly to akpm asking for this to be removed, hopefully
this will help.

> PPS I do have a patch ready that addresses some of the issues you
> raised. Those are only the cosmetic changes though (e.g. change
> naming of chip, comment updates, etc.). Would you suggest me to
> send it right now already or wait until I have time to address the
> other issues as well?

My goal for 2.6.36 is to pull the patch, and then we can wait til all
issues are fixed properly before re-introducing it.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Hans de Goede on 13 Aug 2010 07:00
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(a)mortis.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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  | 
Pages: 1 2
Prev: [PATCH 3/3 v2] cyber2000fb: add I2C support
Next: act_nat: the checksum of ICMP doesn't have pseudo header