Port of w83627hf to kernel 2.6

PATCH New Bernhard C. Schrenk 3 participants 10 messages May 19, 2005

lore

Share & tools

lore

Bernhard C. Schrenk

author

2005-05-19 06:24 UTC #0 lore raw

PATCH 1/1 Port of w83627hf to kernel 2.6

Hi,

I have ported the w83627hf module to the 2.6 kernel. I hope it's useful for you. The patch is against linux kernel 2.6.3-rc4. I have tested it with a w83627thf chip on an Asus P4P800 motherboard.

Regards,
Bernhard
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627hf.patch.bz2
Type: application/octet-stream
Size: 9873 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040217/b1ab0ec8/w83627hf.patch.obj

Bernhard Schrenk

author

2005-05-19 06:24 UTC #1 lore raw

PATCH 1/1 Port of w83627hf to kernel 2.6

Hi Mark,

Thanks for your suggestions for improvement. I have incorporated them into the patch and merged the modifications from the latest CVS driver version. The new patch is attached to this mail.

Regards,
Bernhard
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627hf.patch2.bz2
Type: application/octet-stream
Size: 9410 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040218/5cadf334/w83627hf.patch2.obj

Bernhard Schrenk

author

2005-05-19 06:24 UTC #2 lore raw

PATCH 1/1 Port of w83627hf to kernel 2.6

In reply to Bernhard C. Schrenk

Hi Mark,

From: "Mark M. Hoffman" <[email protected]>

Quoted 7 lines

But, there's still this (which is my fault for not being clear earlier):
Quoted 1 line
#define IN_FROM_REG(val) (((val) * 16 + 5) / 10)
And then later...
Quoted 1 line
        return sprintf(buf,"%ld\n", (long)IN_FROM_REG(data->reg[nr] *

10)); \

I have corrected this and attached the new patch to the mail.

Regards,
Bernhard
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627hf.patch3.bz2
Type: application/octet-stream
Size: 9405 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040219/68476067/w83627hf.patch3.obj

Mark M. Hoffman

2005-05-19 06:24 UTC #3 lore raw

PATCH 1/1 Port of w83627hf to kernel 2.6

In reply to Bernhard C. Schrenk

Hi Bernhard:

Quoted 3 lines · Bernhard C. Schrenk

I have ported the w83627hf module to the 2.6 kernel. I hope it's useful for you. The patch is against linux kernel 2.6.3-rc4. I have tested it with a w83627thf chip on an Asus P4P800 motherboard.

Great, this is something I wanted to do but I've been wicked busy...

The 2.4 driver in CVS was updated to include support for w83637hf. You should consider including that. You should consider the other changes from r1.12 to r1.13 as well (pwm, etc).

Quoted 11 lines

#define TEMP_MIN (-1280)
#define TEMP_MAX ( 1270)
/* TEMP: 1/10 degrees C (-128C to +127C)
REG: 1C/bit, two's complement */
static u8 TEMP_TO_REG(int temp)
{
        int ntemp = SENSORS_LIMIT(temp, TEMP_MIN, TEMP_MAX);
        ntemp += (ntemp<0 ? -5 : 5);
        return (u8)(ntemp / 10);
}

The 1/10 degrees C representation makes no sense for a 2.6 driver. Let's try to keep all the conversions in one place. Take a look at the 2.6 version of asb100.c for a better example.

Quoted 3 lines

/* TEMP: 0.1C/bit (-55C to +125C)
REG: (0.5C/bit, two's complement) << 7 */
static inline u16 LM75_TEMP_TO_REG(int temp)

Definitely don't do this. The correct definition is in lm75.h. Again, an example is asb100.c.

Quoted 1 line

/* There are some complications in a module like this. First off, W83781D chips

Can you please just strike this whole comment... it's obviously cut and paste from another driver and doesn't even make much sense for this one... all w836xxxx are accessed by ISA only! I'll yank it out of our CVS too.

There are many lines which are more than 80 columns, please fix.

Quoted 2 lines

sysfs_fan_offset(1);
sysfs_fan_min_offset(1);

These macro invocations which define functions should not get semicolons at the end...

Quoted 2 lines

#define device_create_file_vid(client) \
device_create_file(&client->dev, &dev_attr_vid);

.. neither should these. I know it's pedantic, and most of the other 2.6 drivers violate it too.

Thanks and regards,

Signature · 2 lines

Mark M. Hoffman
[email protected]

Mark M. Hoffman

2005-05-19 06:24 UTC #4 lore raw

PATCH 1/1 Port of w83627hf to kernel 2.6

In reply to Bernhard C. Schrenk

Hi Bernhard:

Quoted 3 lines · Bernhard Schrenk

Thanks for your suggestions for improvement. I have incorporated them into the patch and merged the modifications from the latest CVS driver version. The new patch is attached to this mail.

Nice work.

But, there's still this (which is my fault for not being clear earlier):

Quoted 1 line

#define IN_FROM_REG(val) (((val) * 16 + 5) / 10)

And then later...

Quoted 1 line

        return sprintf(buf,"%ld\n", (long)IN_FROM_REG(data->reg[nr] * 10)); \

The '/10' and '*10' are artifacts of the 2.4 representation. They're not needed here. This is what I mean by keeping the conversions in one place... there's no reason to carry around an intermediate representation which differs by a factor of ten (or whatever).

I know... the obvious example (w83781d.c) gets this wrong as well.

In the mean time, I'm off to actually load the code and try it. ;)

Thanks and regards,

Signature · 2 lines

Mark M. Hoffman
[email protected]

Bernhard Schrenk

author

2005-05-19 06:24 UTC #5 lore raw

PATCH 1/1 Port of w83627hf to kernel 2.6

In reply to Bernhard C. Schrenk

Hi,

Please excuse my late response. I have attached the new patch including the changes in latest cvs revision (1.15) of the driver and the suggestion from Jean to change the descriptive text. The patch is against kernel version 2.6.3.

Quoted 3 lines · Jean Delvare

Quoted 2 lines
MMH, can you take care about this port (review, test if possible, and send to Greg)?

I can take care about this, but its my first linux kernel patch, so I do not really know the required review and test process.
I am testing this on w83627thf. It is now running for some days without reboot and without any problems (used by fancontrol).

Regards,
Bernhard
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627hf.patch4.bz2
Type: application/octet-stream
Size: 9405 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040225/19c2ffc7/w83627hf.patch4.obj

Jean Delvare

2005-05-19 06:24 UTC #6 lore raw

PATCH 1/1 Port of w83627hf to kernel 2.6

In reply to Bernhard C. Schrenk

Quoting "Mark M. Hoffman" <[email protected]>:

Quoted 5 lines

I was going to resubmit your most recent patch to Greg, but you may as well do that yourself... put [PATCH 2.6] in the subject; put a short description of what the patch does in the body; attach the patch itself to the message as *plain text* or even inline with the message body. Be sure to CC the sensors mailing list.

I think that Greg only takes patches inline (which supposes that you know how to configure your e-mail client so that it doesn't trash long lines in them).

Also, Bernhard, I invite you to mention the fact that Mark Hoffman reviewed your code in your comment to Greg. This should help him decide how confident he can be with your code.

Signature · 2 lines

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Jean Delvare

2005-05-19 06:24 UTC #7 lore raw

PATCH 1/1 Port of w83627hf to kernel 2.6

In reply to Bernhard C. Schrenk

Quoted 1 line

I have corrected this and attached the new patch to the mail.

I just commited a new fix to the w83627hf driver (CVS). Could you incorporate it in your driver? (Sorry ;))

Also, please consider changing this:
+ If you say yes here you get support for the Winbond W836X series
to:
+ If you say yes here you get support for the Winbond W836X7 series

Not very important, I admit, but I think it designates the family better.

MMH, can you take care about this port (review, test if possible, and send to Greg)?

Thanks (both of you).

Signature · 2 lines

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Mark M. Hoffman

2005-05-19 06:24 UTC #8 lore raw

PATCH 1/1 Port of w83627hf to kernel 2.6

In reply to Bernhard C. Schrenk

Quoted 10 lines · Jean Delvare

Quoted 1 line
I have corrected this and attached the new patch to the mail.
I just commited a new fix to the w83627hf driver (CVS). Could you incorporate it in your driver? (Sorry ;))
Also, please consider changing this:
+ If you say yes here you get support for the Winbond W836X series
to:
+ If you say yes here you get support for the Winbond W836X7 series

Quoted 4 lines

Not very important, I admit, but I think it designates the family better.
MMH, can you take care about this port (review, test if possible, and send to Greg)?

I have reviewed and tested it, btw, on w83627thf.

I can take over and make the changes you (Jean) mentioned. But, what about the sysfs names? Do we want this driver to go through -mm tree first? If so, I'll make those changes as well and prepare a patch.

Better still, Greg, please apply Bernhard's most recent patch as-is for -mm; then I will send another patch within a couple days for the rest.

(This way there's a clear attribution for Bernhard's good work before I take over and foul it up. ;)

Regards,

Signature · 2 lines

Mark M. Hoffman
[email protected]

Mark M. Hoffman

2005-05-19 06:24 UTC #9 lore raw

PATCH 1/1 Port of w83627hf to kernel 2.6

In reply to Bernhard C. Schrenk

Quoted 6 lines · Bernhard Schrenk

Hi,
Please excuse my late response. I have attached the new patch including the changes in latest cvs revision (1.15) of the driver and the suggestion from Jean to change the descriptive text. The patch is against kernel version 2.6.3.

Thanks again!

Quoted 6 lines

Quoted 3 lines · Jean Delvare
Quoted 2 lines
MMH, can you take care about this port (review, test if possible, and send to Greg)?
I can take care about this, but its my first linux kernel patch, so I do not really know the required review and test process.

Nothing so formal, really. Basically...

There's a small group of people that have CVS commit privileges for the kernel 2.4 and userland stuff. These same people (but mostly Jean lately) review any driver work destined for kernel 2.6. After review, a patch is sent to Greg KH who is the gatekeeper for I2C and sensors kernel 2.6.

I was going to resubmit your most recent patch to Greg, but you may as well do that yourself... put [PATCH 2.6] in the subject; put a short description of what the patch does in the body; attach the patch itself to the message as *plain text* or even inline with the message body. Be sure to CC the sensors mailing list.

Some time later, Greg will apply it to his tree; then he'll send a batch of I2C/sensors patches to LKML. Later still, your driver will show up in Linus' tree.

Of course, others may review and comment at every step... however most of the review for these drivers happens here on this mailing list.

Regards,

Signature · 2 lines

Mark M. Hoffman
[email protected]