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 31 Jul 2010 19:40
On Wed, Mar 24, 2010 at 11:31:40 +0100, Hans de Goede wrote:
> On 03/24/2010 10:23, Giel van Schijndel wrote:
>> On Wed, Mar 24, 2010 at 09:25:08 +0100, Hans de Goede wrote:
>>> See comments inline.
>>>
>>> On 03/24/2010 12:12 AM, Giel van Schijndel wrote:
>>>> Allow device probing to recognise the Fintek F71808E.
>>>>
>>>> Sysfs interface:
>>>> * Fan/pwm control is the same as for F71889FG
>>>> * Temperature and voltage sensor handling is largely the same as for
>>>> the F71889FG
>>>> - Has one temperature sensor less (doesn't have temp3)
>>>> - Misses one voltage sensor (doesn't have V6, thus in6_input refers to
>>>> what in7_input refers for F71889FG)
>>>>
>>>> For the purpose of the sysfs interface fxxxx_in_temp_attr[] is split up
>>>> such that it can largely be reused.
>>>>---
>>>> Documentation/hwmon/f71882fg | 4 ++
>>>> drivers/hwmon/Kconfig | 6 ++--
>>>> drivers/hwmon/f71882fg.c | 80 +++++++++++++++++++++++++++++++++++++----
>>>> 3 files changed, 79 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
>>>> index 25e1cad..b290b87 100644
>>>> --- a/drivers/hwmon/f71882fg.c
>>>> +++ b/drivers/hwmon/f71882fg.c
>>>> @@ -1974,8 +2002,27 @@ static int __devinit f71882fg_probe(struct platform_device *pdev)
>>>> ... snip ...
>>>> + /* fall through! */
>>>
>>> Ugh, please don't fall through, and then have an if below to only do
>>> some parts of the case falling through. This is quite confusing at
>>> first I thought your code was buggy I had to read it twice to notice
>>> the if. Instead just duplicate the following lines:
>>>> + err = f71882fg_create_sysfs_files(pdev,
>>>> + fxxxx_temp_attr,
>>>> + ARRAY_SIZE(fxxxx_temp_attr));
>>> In the f71862fg case, end the f71862fg case with a break and remove
>>> the if test from the f71808fg case.
>>>
>>>>+ case f71808fg:
>>>>+ if (data->type == f71808fg) {
>>>>+ err = f71882fg_create_sysfs_files(pdev,
>>>>+ f71808_in_attr,
>>>>+ ARRAY_SIZE(f71808_in_attr));
>>>>+ if (err)
>>>>+ goto exit_unregister_sysfs;
>>>>+ }
>>>>+ err = f71882fg_create_sysfs_files(pdev,
>>>>+ fxxxx_temp_attr,
>>>>+ ARRAY_SIZE(fxxxx_temp_attr));
>>>> ... snip ...
>>
>> Ack. New and improved patch follows this line.
>> =====================================================================
>> hwmon: f71882fg: Add support for the Fintek F71808E
>
> This new version looks good to me:
> Acked-by: Hans de Goede <hdegoede(a)redhat.com>

Thanks. Anyone else I need to poke in order to set this on track to
mainline?

--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
From: Hans de Goede on 1 Aug 2010 02:20
Hi,

On 08/01/2010 01:31 AM, Giel van Schijndel wrote:
> On Wed, Mar 24, 2010 at 11:31:40 +0100, Hans de Goede wrote:
>> On 03/24/2010 10:23, Giel van Schijndel wrote:
>>> On Wed, Mar 24, 2010 at 09:25:08 +0100, Hans de Goede wrote:
>>>> See comments inline.
>>>>
>>>> On 03/24/2010 12:12 AM, Giel van Schijndel wrote:
>>>>> Allow device probing to recognise the Fintek F71808E.
>>>>>
>>>>> Sysfs interface:
>>>>> * Fan/pwm control is the same as for F71889FG
>>>>> * Temperature and voltage sensor handling is largely the same as for
>>>>> the F71889FG
>>>>> - Has one temperature sensor less (doesn't have temp3)
>>>>> - Misses one voltage sensor (doesn't have V6, thus in6_input refers to
>>>>> what in7_input refers for F71889FG)
>>>>>
>>>>> For the purpose of the sysfs interface fxxxx_in_temp_attr[] is split up
>>>>> such that it can largely be reused.
>>>>> ---
>>>>> Documentation/hwmon/f71882fg | 4 ++
>>>>> drivers/hwmon/Kconfig | 6 ++--
>>>>> drivers/hwmon/f71882fg.c | 80 +++++++++++++++++++++++++++++++++++++----
>>>>> 3 files changed, 79 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
>>>>> index 25e1cad..b290b87 100644
>>>>> --- a/drivers/hwmon/f71882fg.c
>>>>> +++ b/drivers/hwmon/f71882fg.c
>>>>> @@ -1974,8 +2002,27 @@ static int __devinit f71882fg_probe(struct platform_device *pdev)
>>>>> ... snip ...
>>>>> + /* fall through! */
>>>>
>>>> Ugh, please don't fall through, and then have an if below to only do
>>>> some parts of the case falling through. This is quite confusing at
>>>> first I thought your code was buggy I had to read it twice to notice
>>>> the if. Instead just duplicate the following lines:
>>>>> + err = f71882fg_create_sysfs_files(pdev,
>>>>> + fxxxx_temp_attr,
>>>>> + ARRAY_SIZE(fxxxx_temp_attr));
>>>> In the f71862fg case, end the f71862fg case with a break and remove
>>>> the if test from the f71808fg case.
>>>>
>>>>> + case f71808fg:
>>>>> + if (data->type == f71808fg) {
>>>>> + err = f71882fg_create_sysfs_files(pdev,
>>>>> + f71808_in_attr,
>>>>> + ARRAY_SIZE(f71808_in_attr));
>>>>> + if (err)
>>>>> + goto exit_unregister_sysfs;
>>>>> + }
>>>>> + err = f71882fg_create_sysfs_files(pdev,
>>>>> + fxxxx_temp_attr,
>>>>> + ARRAY_SIZE(fxxxx_temp_attr));
>>>>> ... snip ...
>>>
>>> Ack. New and improved patch follows this line.
>>> =====================================================================
>>> hwmon: f71882fg: Add support for the Fintek F71808E
>>
>> This new version looks good to me:
>> Acked-by: Hans de Goede<hdegoede(a)redhat.com>
>
> Thanks. Anyone else I need to poke in order to set this on track to
> mainline?
>

No,

Jean should have picked this up, I guess it has fallen through the cracks.

I think it is probably best if you resend this patch and the watchdog patches
re-based against the latest upstream rc, then I'll re-review them and ack
them and Jean can then put them into his tree.

Thanks & 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: Giel van Schijndel on 1 Aug 2010 09:30
On Sun, Aug 01, 2010 at 08:12:40 +0200, Hans de Goede wrote:
> On 08/01/2010 01:31 AM, Giel van Schijndel wrote:
>> On Wed, Mar 24, 2010 at 11:31:40 +0100, Hans de Goede wrote:
>>> On 03/24/2010 10:23, Giel van Schijndel wrote:
>>>> Ack. New and improved patch follows this line.
>>>> ===================================================================
>>>> hwmon: f71882fg: Add support for the Fintek F71808E
>>>
>>> This new version looks good to me:
>>> Acked-by: Hans de Goede<hdegoede(a)redhat.com>
>>
>> Thanks. Anyone else I need to poke in order to set this on track to
>> mainline?
>
> No,
>
> Jean should have picked this up, I guess it has fallen through the cracks.
>
> I think it is probably best if you resend this patch and the watchdog
> patches re-based against the latest upstream rc, then I'll re-review
> them and ack them and Jean can then put them into his tree.

Ack. I'll resend them as replies to this mail.

--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
From: Hans de Goede on 4 Aug 2010 07:40
Hi,

I know I've reviewed this patch before, but now I have a datasheet
so this time I've been a bit more thorough and I've found 2 small
issues and 1 bigger one.

Andrew can you please drop this patch from -mm until this is resolved?

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 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%.

So it looks like you need to create a new f71808e_auto_pwm_attr array
esp. for this model, as well as a special case for reading the
auto pwm attr in f71882fg_update_device.

Also the auto pwm of the F71808E allows following of digital temps
read to peci / amdsi / ibex rather then following a directly connected
temp diode like the F71889FG, which the driver does not support, so
you should check if this is enabled and if so disable the auto pwm
attr entirely. Code for this is already in place for the F71889FG,
you simply need to make it trigger when the chip is a F71808E too.

> * Temperature and voltage sensor handling is largely the same as for
> the F71889FG
> - Has one temperature sensor less (doesn't have temp3)
> - Misses one voltage sensor (doesn't have V6, thus in6_input refers to
> what in7_input refers for F71889FG)
>
> For the purpose of the sysfs interface fxxxx_in_temp_attr[] is split up
> such that it can largely be reused.

There is a problem here though, the new fxxxx_temp_attr contains
attributes for temp#_max_beep and temp#_crit_beep, but the F71808E
lacks that function. So I think that the new fxxxx_temp_attr
need to be split into fxxxx_temp_attr and fxxxx_temp_beep_attr,
like is already done with fxxxx_fan_attr.

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.

> 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).

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.

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: Giel van Schijndel on 4 Aug 2010 11:50
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?

Because in the datasheet I have for the F71889 only 3 fan controls are
mentioned, not 5, it does however have 4 boundary temps:
> 7.5. Hardware Monitor
> ... snip ...
> ... page 46-47:
> Device registers, the following is a register map order which shows a
> summary of all registers. Please refer each one register if you want
> more detail information.
> Register CR01 ~ CR03 -> Configuration Registers
> Register CR0A ~ CR0F -> PECI/SST/TSI Control Register
> Register CR10 ~ CR4F -> Voltage Setting Register
> Register CR60 ~ CR8E -> Temperature Setting Register
> Register CR90 ~ CRDF -> Fan Control Setting Register
> -> Fan1 Detail Setting CRA0 ~ CRAF
> -> Fan2 Detail Setting CRB0 ~ CRBF
> -> Fan3 Detail Setting CRC0 ~ CRCF
> Register CR5A ~ CR5D -> HW Chip ID and Vender ID Register

> So it looks like you need to create a new f71808e_auto_pwm_attr array
> esp. for this model, as well as a special case for reading the
> auto pwm attr in f71882fg_update_device.

Ack.

> Also the auto pwm of the F71808E allows following of digital temps
> read to peci / amdsi / ibex rather then following a directly connected
> temp diode like the F71889FG, which the driver does not support, so
> you should check if this is enabled and if so disable the auto pwm
> attr entirely. Code for this is already in place for the F71889FG,
> you simply need to make it trigger when the chip is a F71808E too.

Do you mean the checking of the FANx_TEMP_SEL_DIG flag in the fan's
"Temperature Mapping Select" registers currently done for the F71889 in
the second switch-statement inside f71882fg_probe? As that code is
already used for the F71808E as well.

>> * Temperature and voltage sensor handling is largely the same as for
>> the F71889FG
>> - Has one temperature sensor less (doesn't have temp3)
>> - Misses one voltage sensor (doesn't have V6, thus in6_input refers to
>> what in7_input refers for F71889FG)
>>
>> For the purpose of the sysfs interface fxxxx_in_temp_attr[] is split up
>> such that it can largely be reused.
>
> There is a problem here though, the new fxxxx_temp_attr contains
> attributes for temp#_max_beep and temp#_crit_beep, but the F71808E
> lacks that function. So I think that the new fxxxx_temp_attr
> need to be split into fxxxx_temp_attr and fxxxx_temp_beep_attr,
> like is already done with fxxxx_fan_attr.

Ack.

> 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.

>> 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).

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).

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

> 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. E.g. something similar to this patch:
========================================================================
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index b891b65..bfb2b4c 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -2113,7 +2113,8 @@ static int __devinit f71882fg_probe(struct platform_device *pdev)
break;
}
/* fall through */
- default: /* f71858fg / f71882fg */
+ case f71858fg:
+ case f71882fg:
err = f71882fg_create_sysfs_files(pdev,
&fxxxx_auto_pwm_attr[0][0],
ARRAY_SIZE(fxxxx_auto_pwm_attr[0]) * nr_fans);
@@ -2224,7 +2225,10 @@ static int f71882fg_remove(struct platform_device *pdev)
f8000_auto_pwm_attr,
ARRAY_SIZE(f8000_auto_pwm_attr));
break;
- default: /* f71808e / f71858fg / f71882fg / f71889fg */
+ case f71808e:
+ case f71858fg:
+ case f71882fg:
+ case f71889fg:
f71882fg_remove_sysfs_files(pdev,
&fxxxx_auto_pwm_attr[0][0],
ARRAY_SIZE(fxxxx_auto_pwm_attr[0]) * nr_fans);
========================================================================

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.

--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
 |  Next  |  Last
Pages: 1 2
Prev: [PATCH 3/3 v2] cyber2000fb: add I2C support
Next: act_nat: the checksum of ICMP doesn't have pseudo header