Status of Andigilog asc7621 driver submitted by George Joseph on 2008-05-29

Status of Andigilog asc7621 driver submitted by George Joseph on 2008-05-29

[Jaswinder Singh Rajput]

Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
This chip is used by various Intel Motherboards.

Is it still under review and testing :

http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html

http://www.spinics.net/lists/lm-sensors/msg26915.html

http://www.spinics.net/lists/lm-sensors/msg26916.html

http://www.lm-sensors.org/wiki/Devices

Thank you,
--
JSR

[lm-sensors] Status of Andigilog asc7621 driver submitted by George

[Jaswinder Singh Rajput]

Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
This chip is used by various Intel Motherboards.

Is it still under review and testing :

http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html

http://www.spinics.net/lists/lm-sensors/msg26915.html

http://www.spinics.net/lists/lm-sensors/msg26916.html

http://www.lm-sensors.org/wiki/Devices

Thank you,
--
JSR

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[Hans de Goede]

Hi,

On 01/23/2010 06:52 PM, Jaswinder Singh Rajput wrote:
> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
> This chip is used by various Intel Motherboards.
>
> Is it still under review and testing :
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
>
> http://www.spinics.net/lists/lm-sensors/msg26915.html
>
> http://www.spinics.net/lists/lm-sensors/msg26916.html
>
> http://www.lm-sensors.org/wiki/Devices
>

I guess this is partly my fault, I started a review but never finished
it. One of the big problems is that George did many things completely
different to how every other single hwmon driver does things. Which made
the review harder then necessary, and more over made me wonder if we
should accept the driver in that incarnation at all.

Then a lot of things happened and I never got around to doing anything
with it at all.

I just checked my Drafts folder, and I still have my unfinished review
in there. So if there is interest I can send that, note that it is
not a complete review though (there is a note in there which part
of the code is reviewed and which still needs to be reviewed).

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by George Joseph on 2008-05-29

[Hans de Goede]

Hi,

On 01/23/2010 06:52 PM, Jaswinder Singh Rajput wrote:
> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
> This chip is used by various Intel Motherboards.
>
> Is it still under review and testing :
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
>
> http://www.spinics.net/lists/lm-sensors/msg26915.html
>
> http://www.spinics.net/lists/lm-sensors/msg26916.html
>
> http://www.lm-sensors.org/wiki/Devices
>

I guess this is partly my fault, I started a review but never finished
it. One of the big problems is that George did many things completely
different to how every other single hwmon driver does things. Which made
the review harder then necessary, and more over made me wonder if we
should accept the driver in that incarnation at all.

Then a lot of things happened and I never got around to doing anything
with it at all.

I just checked my Drafts folder, and I still have my unfinished review
in there. So if there is interest I can send that, note that it is
not a complete review though (there is a note in there which part
of the code is reviewed and which still needs to be reviewed).

Regards,

Hans

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by George Joseph on 2008-05-29

[Jaswinder Singh Rajput]

Hello Hans,

On Sat, Jan 23, 2010 at 11:28 PM, Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> Hi,
>
> On 01/23/2010 06:52 PM, Jaswinder Singh Rajput wrote:
>>
>> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
>> This chip is used by various Intel Motherboards.
>>
>> Is it still under review and testing :
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
>>
>> http://www.spinics.net/lists/lm-sensors/msg26915.html
>>
>> http://www.spinics.net/lists/lm-sensors/msg26916.html
>>
>> http://www.lm-sensors.org/wiki/Devices
>>
>
> I guess this is partly my fault, I started a review but never finished
> it. One of the big problems is that George did many things completely
> different to how every other single hwmon driver does things. Which made
> the review harder then necessary, and more over made me wonder if we
> should accept the driver in that incarnation at all.
>

George Joseph submitted the driver on 2008-05-29 and then he updated
the driver on 2009-10-15 for 2.6.30 and many developers are able to
test it and used it.

Are you able to compile the driver with latest kernel git.

> Then a lot of things happened and I never got around to doing anything
> with it at all.
>

Many new Intel Motherboards are coming with this chip, if we complete
this driver then it will be great.

> I just checked my Drafts folder, and I still have my unfinished review
> in there. So if there is interest I can send that, note that it is
> not a complete review though (there is a note in there which part
> of the code is reviewed and which still needs to be reviewed).
>

Please provide your review so that we can discuss about this driver
and make relevant changes to accept it.

Thank you,
--
Jaswinder Singh.

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[Jaswinder Singh Rajput]

Hello Hans,

On Sat, Jan 23, 2010 at 11:28 PM, Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> Hi,
>
> On 01/23/2010 06:52 PM, Jaswinder Singh Rajput wrote:
>>
>> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
>> This chip is used by various Intel Motherboards.
>>
>> Is it still under review and testing :
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
>>
>> http://www.spinics.net/lists/lm-sensors/msg26915.html
>>
>> http://www.spinics.net/lists/lm-sensors/msg26916.html
>>
>> http://www.lm-sensors.org/wiki/Devices
>>
>
> I guess this is partly my fault, I started a review but never finished
> it. One of the big problems is that George did many things completely
> different to how every other single hwmon driver does things. Which made
> the review harder then necessary, and more over made me wonder if we
> should accept the driver in that incarnation at all.
>

George Joseph submitted the driver on 2008-05-29 and then he updated
the driver on 2009-10-15 for 2.6.30 and many developers are able to
test it and used it.

Are you able to compile the driver with latest kernel git.

> Then a lot of things happened and I never got around to doing anything
> with it at all.
>

Many new Intel Motherboards are coming with this chip, if we complete
this driver then it will be great.

> I just checked my Drafts folder, and I still have my unfinished review
> in there. So if there is interest I can send that, note that it is
> not a complete review though (there is a note in there which part
> of the code is reviewed and which still needs to be reviewed).
>

Please provide your review so that we can discuss about this driver
and make relevant changes to accept it.

Thank you,
--
Jaswinder Singh.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[Constantine A. Murenin]

On 24/01/2010, Jaswinder Singh Rajput <jaswinderlinux@gmail.com> wrote:
> Many new Intel Motherboards are coming with this chip, if we complete
>  this driver then it will be great.

Really?  My understanding was that the company, Andigilog, no longer
seem to exist since at least a couple of months ago (I don't recall
the exact dates their web-site was taken down without any notice, but
currently the domain has no information regarding semiconductors
whatsoever [0]).  It would therefore seem somewhat strange if Intel
was still using these chips in their recent designs — do you have some
concrete examples?  Anyone has any more details regarding Andigilog
and their products?

C.

[0] http://translate.google.com/translate?hl=en&sl=ja&tl=en&u=http%3A%2F%2Fwww.andigilog.com%2F

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by George Joseph on 2008-05-29

[Constantine A. Murenin]

On 24/01/2010, Jaswinder Singh Rajput <jaswinderlinux@gmail.com> wrote:
> Many new Intel Motherboards are coming with this chip, if we complete
>  this driver then it will be great.

Really?  My understanding was that the company, Andigilog, no longer
seem to exist since at least a couple of months ago (I don't recall
the exact dates their web-site was taken down without any notice, but
currently the domain has no information regarding semiconductors
whatsoever [0]).  It would therefore seem somewhat strange if Intel
was still using these chips in their recent designs — do you have some
concrete examples?  Anyone has any more details regarding Andigilog
and their products?

C.

[0] http://translate.google.com/translate?hl=en&sl=ja&tl=en&u=http%3A%2F%2Fwww.andigilog.com%2F

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by George Joseph on 2008-05-29

[Jaswinder Singh Rajput]

Hello,

On Sun, Jan 24, 2010 at 11:14 AM, Constantine A. Murenin
<mureninc@gmail.com> wrote:
> On 24/01/2010, Jaswinder Singh Rajput <jaswinderlinux@gmail.com> wrote:
>> Many new Intel Motherboards are coming with this chip, if we complete
>>  this driver then it will be great.
>
> Really?

Yes, http://www.intel.com/products/server/motherboard/index.htm?iid=mbd_body+sv_all


I have checked with Intel® Workstation Board WX58BP :

Now follows a summary of the probes I have just done.
Just press ENTER to continue:

Driver `to-be-written':
  * Bus `SMBus I801 adapter at 2000'
    Busdriver `i2c_i801', I2C address 0x2e
    Chip `Andigilog aSC7621' (confidence: 5)

Driver `coretemp':
  * Chip `Intel Core family thermal sensor' (confidence: 9)

Note: there is no driver for Andigilog aSC7621 yet.
Check http://www.lm-sensors.org/wiki/Devices for updates.

Do you want to overwrite /etc/sysconfig/lm_sensors? (YES/no): YES
Starting lm_sensors: loading module coretemp               [  OK  ]
Unloading i2c-dev... OK

You can also check with other intel motherboards.

> My understanding was that the company, Andigilog, no longer
> seem to exist since at least a couple of months ago (I don't recall
> the exact dates their web-site was taken down without any notice, but
> currently the domain has no information regarding semiconductors
> whatsoever [0]).  It would therefore seem somewhat strange if Intel
> was still using these chips in their recent designs — do you have some
> concrete examples?  Anyone has any more details regarding Andigilog
> and their products?
>

http://who.is/whois/andigilog.com/

These motherboards are build in Taiwan, China. I have no idea that how
they are getting these chips but you can see them in your latest Intel
motherboards ;-)

Thanks,
--
Jaswinder Singh.

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[Jaswinder Singh Rajput]

Hello,

On Sun, Jan 24, 2010 at 11:14 AM, Constantine A. Murenin
<mureninc@gmail.com> wrote:
> On 24/01/2010, Jaswinder Singh Rajput <jaswinderlinux@gmail.com> wrote:
>> Many new Intel Motherboards are coming with this chip, if we complete
>>  this driver then it will be great.
>
> Really?

Yes, http://www.intel.com/products/server/motherboard/index.htm?iid=mbd_body+sv_all


I have checked with Intel® Workstation Board WX58BP :

Now follows a summary of the probes I have just done.
Just press ENTER to continue:

Driver `to-be-written':
  * Bus `SMBus I801 adapter at 2000'
    Busdriver `i2c_i801', I2C address 0x2e
    Chip `Andigilog aSC7621' (confidence: 5)

Driver `coretemp':
  * Chip `Intel Core family thermal sensor' (confidence: 9)

Note: there is no driver for Andigilog aSC7621 yet.
Check http://www.lm-sensors.org/wiki/Devices for updates.

Do you want to overwrite /etc/sysconfig/lm_sensors? (YES/no): YES
Starting lm_sensors: loading module coretemp               [  OK  ]
Unloading i2c-dev... OK

You can also check with other intel motherboards.

> My understanding was that the company, Andigilog, no longer
> seem to exist since at least a couple of months ago (I don't recall
> the exact dates their web-site was taken down without any notice, but
> currently the domain has no information regarding semiconductors
> whatsoever [0]).  It would therefore seem somewhat strange if Intel
> was still using these chips in their recent designs — do you have some
> concrete examples?  Anyone has any more details regarding Andigilog
> and their products?
>

http://who.is/whois/andigilog.com/

These motherboards are build in Taiwan, China. I have no idea that how
they are getting these chips but you can see them in your latest Intel
motherboards ;-)

Thanks,
--
Jaswinder Singh.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[Constantine A. Murenin]

2010/1/24 Jaswinder Singh Rajput <jaswinderlinux@gmail.com>:
> Hello,
>
> On Sun, Jan 24, 2010 at 11:14 AM, Constantine A. Murenin
> <mureninc@gmail.com> wrote:
>> On 24/01/2010, Jaswinder Singh Rajput <jaswinderlinux@gmail.com> wrote:
>>> Many new Intel Motherboards are coming with this chip, if we complete
>>>  this driver then it will be great.
>>
>> Really?
>
> Yes, http://www.intel.com/products/server/motherboard/index.htm?iid=mbd_body+sv_all
>
>
> I have checked with Intel® Workstation Board WX58BP :

http://www.intel.com/support/motherboards/server/wx58bp/sb/CS-030316.htm
«
Spares, Parts List and Configuration Guide [PDF]
File Name: WX58BP Config Guide 1.0.pdf
Size: 76,709 bytes
Date: April 2009
File Revision: 1.0
»

The board seems old, or, at least, old enough to be designed when
Andigilog still existed. :-)


> Now follows a summary of the probes I have just done.
> Just press ENTER to continue:
>
> Driver `to-be-written':
>  * Bus `SMBus I801 adapter at 2000'
>    Busdriver `i2c_i801', I2C address 0x2e
>    Chip `Andigilog aSC7621' (confidence: 5)
>
> Driver `coretemp':
>  * Chip `Intel Core family thermal sensor' (confidence: 9)
>
> Note: there is no driver for Andigilog aSC7621 yet.
> Check http://www.lm-sensors.org/wiki/Devices for updates.
>
> Do you want to overwrite /etc/sysconfig/lm_sensors? (YES/no): YES
> Starting lm_sensors: loading module coretemp               [  OK  ]
> Unloading i2c-dev... OK
>
> You can also check with other intel motherboards.
>
>> My understanding was that the company, Andigilog, no longer
>> seem to exist since at least a couple of months ago (I don't recall
>> the exact dates their web-site was taken down without any notice, but
>> currently the domain has no information regarding semiconductors
>> whatsoever [0]).  It would therefore seem somewhat strange if Intel
>> was still using these chips in their recent designs — do you have some
>> concrete examples?  Anyone has any more details regarding Andigilog
>> and their products?
>>
>
> http://who.is/whois/andigilog.com/
>
> These motherboards are build in Taiwan, China. I have no idea that how
> they are getting these chips but you can see them in your latest Intel
> motherboards ;-)

Well, a better link would have been:

http://web.archive.org/web/*/http://www.andigilog.com/

:-)

C.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by George Joseph on 2008-05-29

[Constantine A. Murenin]

2010/1/24 Jaswinder Singh Rajput <jaswinderlinux@gmail.com>:
> Hello,
>
> On Sun, Jan 24, 2010 at 11:14 AM, Constantine A. Murenin
> <mureninc@gmail.com> wrote:
>> On 24/01/2010, Jaswinder Singh Rajput <jaswinderlinux@gmail.com> wrote:
>>> Many new Intel Motherboards are coming with this chip, if we complete
>>>  this driver then it will be great.
>>
>> Really?
>
> Yes, http://www.intel.com/products/server/motherboard/index.htm?iid=mbd_body+sv_all
>
>
> I have checked with Intel® Workstation Board WX58BP :

http://www.intel.com/support/motherboards/server/wx58bp/sb/CS-030316.htm
«
Spares, Parts List and Configuration Guide [PDF]
File Name: WX58BP Config Guide 1.0.pdf
Size: 76,709 bytes
Date: April 2009
File Revision: 1.0
»

The board seems old, or, at least, old enough to be designed when
Andigilog still existed. :-)


> Now follows a summary of the probes I have just done.
> Just press ENTER to continue:
>
> Driver `to-be-written':
>  * Bus `SMBus I801 adapter at 2000'
>    Busdriver `i2c_i801', I2C address 0x2e
>    Chip `Andigilog aSC7621' (confidence: 5)
>
> Driver `coretemp':
>  * Chip `Intel Core family thermal sensor' (confidence: 9)
>
> Note: there is no driver for Andigilog aSC7621 yet.
> Check http://www.lm-sensors.org/wiki/Devices for updates.
>
> Do you want to overwrite /etc/sysconfig/lm_sensors? (YES/no): YES
> Starting lm_sensors: loading module coretemp               [  OK  ]
> Unloading i2c-dev... OK
>
> You can also check with other intel motherboards.
>
>> My understanding was that the company, Andigilog, no longer
>> seem to exist since at least a couple of months ago (I don't recall
>> the exact dates their web-site was taken down without any notice, but
>> currently the domain has no information regarding semiconductors
>> whatsoever [0]).  It would therefore seem somewhat strange if Intel
>> was still using these chips in their recent designs — do you have some
>> concrete examples?  Anyone has any more details regarding Andigilog
>> and their products?
>>
>
> http://who.is/whois/andigilog.com/
>
> These motherboards are build in Taiwan, China. I have no idea that how
> they are getting these chips but you can see them in your latest Intel
> motherboards ;-)

Well, a better link would have been:

http://web.archive.org/web/*/http://www.andigilog.com/

:-)

C.

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[Hans de Goede]

On 01/24/2010 06:05 AM, Jaswinder Singh Rajput wrote:
> Hello Hans,
>
> On Sat, Jan 23, 2010 at 11:28 PM, Hans de Goede<j.w.r.degoede@hhs.nl>  wrote:
>> Hi,
>>
>> I just checked my Drafts folder, and I still have my unfinished review
>> in there. So if there is interest I can send that, note that it is
>> not a complete review though (there is a note in there which part
>> of the code is reviewed and which still needs to be reviewed).
>>
>
> Please provide your review so that we can discuss about this driver
> and make relevant changes to accept it.
>

Ok, here is the partly review I've done:

---begin---
Hi Joseph,

george.joseph@peakin.com wrote:
 > Here's the c file by itself.
 >
 > Hans, will you have any time to review the driver in the near future?
 >

I believe I said I would make time for this some time ago. Given that the 2.6.29 merge window has already opened, I've now done so (made time). So that this hopefully / maybe can make 2.6.29's merge window.

I must say that this driver deviates a lot from the standard way all other hwmon drivers are written making the review somewhat cumbersome and this might also be a problem if you step down and someone else becomes the maintainer.

I've split me review in 2 parts, first some comments about how you've implemented the sysfs API:

I notice that you have added foo_label sysfs entries, while you have nothing usefull to put in them, please do not _label entries are only for when the driver knows / can provide a label in the prefered format for a human reader of the sensors output, so not "in1", but something like "ATX 12V", so please remove all _label sysfs entries and remove the corresponding "char *label" member from the asc7621_param struct.

Likewise you've added tempX_type entries, but these always return the same value, in this case they can (and should be omitted) there is only the need to know (and more importantly be able to set) the type of the sensor, if it can be of different types, as in this case the BIOS might have set it wrong, when you remove these sysfs entries, you can also remove the corresponding "value" member from the asc7621_param struct.



And second, some feedback on the code itself.

First of all in *ALL* your store functions you need to store the result of strtol in a long and strtoul in an unsigned long, storing in smaller types can cause an overflow before you do any checking / clamping when the user gives a really large value as input.

Also in *ALL* store functions please use strict_strtol (and strtoul) instead of simple.

Last in some functions you need to clamp the input from the user to the valid input range before doing further calculations to avoid overflows during the calculation. Often clamping after calculations can be too late.

Example:

Instead of:

static ssize_t store_u8(struct device *dev, struct device_attribute *attr,
                         const char *buf, size_t count)
{
         SETUP_STORE_data_param(dev, attr);

         u8 reqval = simple_strtoul(buf, NULL, 10);

         mutex_lock(&data->update_lock);
         data->reg[param->msb[0]] = reqval;
         write_byte(param->msb[0], reqval);
         mutex_unlock(&data->update_lock);
         return count;
}

Write:

static ssize_t store_u8(struct device *dev, struct device_attribute *attr,
                         const char *buf, size_t count)
{
     SETUP_STORE_data_param(dev, attr);
     long reqval;

     if (strict_strtoul(buf, 10, &reqval))
         return -EINVAL;

     reqval = SENSORS_LIMIT(v, 0, 255);

         mutex_lock(&data->update_lock);
         data->reg[param->msb[0]] = reqval;
         write_byte(param->msb[0], reqval);
         mutex_unlock(&data->update_lock);
         return count;
}

Note that in this example all 3 described changes were made (use long, use strict_strtol, clamp before further use). *ALL* your store functions need the first 2 changes (use long, use strict_strtol), so I'm not going to make that comment for each and every store functions in the detailed comments below.

Where a store function needs earlier / different clamping I will make a comment in the detailed feedback given below.

Below are pieces of code from the driver with detailed feedback below them.

###

  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; version 2 of the License.
  *

I notice there is no "or at your option any later version" language here, this
is fine, but I wonder if this is on purpose, if not please add "or at your
option any later version", so that if the kernel ever (not likely) wants to
move to GPL v3 this is one less file to worry about.

###

/*
  * The "chips" enum created by I2C_CLIENT_INSMOD_2 has "any_chip" as
  * the first element in the enum, so it must also be first in our array.
  */
static struct asc7621_chip asc7621_chips[] = {
         {
          .name = "any",.chip_type = any_chip,
          },
         {
          .name = "asc7621",.chip_type = asc7621,
          .company_reg = 0x3e,.company_id = 0x61,
          .verstep_reg = 0x3f,.verstep_id = 0x6c,
          .addresses = normal_i2c,
          },
         {
          .name = "asc7621a",.chip_type = asc7621a,
          .company_reg = 0x3e,.company_id = 0x61,
          .verstep_reg = 0x3f,.verstep_id = 0x6d,
          .addresses = normal_i2c,
          },
};

Your driver still uses old style i2c driver binding, please update it too the
new style. See for example:
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-July/023773.html

You will probably want to remove using this when you do this, as this overlaps
with the i2c_device_id array you need to declare then.

###

static ssize_t store_u8(struct device *dev, struct device_attribute *attr,
                         const char *buf, size_t count)
{
         SETUP_STORE_data_param(dev, attr);

         u8 reqval = simple_strtoul(buf, NULL, 10);

         mutex_lock(&data->update_lock);
         data->reg[param->msb[0]] = reqval;
         write_byte(param->msb[0], reqval);
         mutex_unlock(&data->update_lock);
         return count;
}

Clamp large input values instead of allowing large values to cause overflows, see above for how this function should look (IMHO)

###

static ssize_t store_bitmask(struct device *dev,
                              struct device_attribute *attr,
                              const char *buf, size_t count)
{
         SETUP_STORE_data_param(dev, attr);

         u8 reqval = simple_strtoul(buf, NULL, 10);

Same as for store_u8, except the values between which to clamp depend on the mask.

###

static ssize_t show_fan16(struct device *dev,
                           struct device_attribute *attr, char *buf)
{
         SETUP_SHOW_data_param(dev, attr);

         u16 regval = (data->reg[param->msb[0]] << 8) | data->reg[param->lsb[0]]


You are using multiple values from data here, this might race with device update resulting in using an old msb with a new lsb or vica versa, so you need locking around this.

###

static ssize_t show_in10(struct device *dev, struct device_attribute *attr,
                          char *buf)
{

You are using multiple values from data here, you need locking around this.

###

static ssize_t store_in8(struct device *dev, struct device_attribute *attr,
                          const char *buf, size_t count)
{

         u8 nr;
         s32 reqval;

         SETUP_STORE_data_param(dev, attr);

         nr = sda->index;
         reqval = simple_strtoul(buf, NULL, 10);
         reqval *= asc7621_in_scaling_div[nr];
         reqval /= asc7621_in_scaling_mul[nr];
         reqval = SENSORS_LIMIT(reqval, 0, 255);

do SENSORS_LIMIT before calculations, the multiple might cause an overflow
otherwise (and ofcourse, reqval should be a long, use strict_strtol).

###
static ssize_t show_temp10(struct device *dev,
                            struct device_attribute *attr, char *buf)
{

You are using multiple values from data here, you need locking around this.

###

static ssize_t show_ap2_temp(struct device *dev,
                              struct device_attribute *attr, char *buf)
{

You are using multiple values from data here, you need locking around this.

###

static ssize_t store_ap2_temp(struct device *dev,
                               struct device_attribute *attr,
                               const char *buf, size_t count)
{

you need to take the lock earlier, before reading auto_point1 from the cached
register array (and ofcourse, reqval should be a long, use strict_strtol).

###

static ssize_t show_pwm_enable(struct device *dev,
                                struct device_attribute *attr, char *buf)
{

You are using multiple values from data here, you need locking around this.

###

In store store_pwm_freq() / store_pwm_ast() / store_temp_st() the following:
         if (newval = 255)
                 return count;

Should return -EINVAL, as you are rejecting the value (not making any changes)

###

Note to self all store / show functions are reviewed

---end---

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by George Joseph on 2008-05-29

[Hans de Goede]

On 01/24/2010 06:05 AM, Jaswinder Singh Rajput wrote:
> Hello Hans,
>
> On Sat, Jan 23, 2010 at 11:28 PM, Hans de Goede<j.w.r.degoede@hhs.nl>  wrote:
>> Hi,
>>
>> I just checked my Drafts folder, and I still have my unfinished review
>> in there. So if there is interest I can send that, note that it is
>> not a complete review though (there is a note in there which part
>> of the code is reviewed and which still needs to be reviewed).
>>
>
> Please provide your review so that we can discuss about this driver
> and make relevant changes to accept it.
>

Ok, here is the partly review I've done:

---begin---
Hi Joseph,

george.joseph@peakin.com wrote:
 > Here's the c file by itself.
 >
 > Hans, will you have any time to review the driver in the near future?
 >

I believe I said I would make time for this some time ago. Given that the 2.6.29 merge window has already opened, I've now done so (made time). So that this hopefully / maybe can make 2.6.29's merge window.

I must say that this driver deviates a lot from the standard way all other hwmon drivers are written making the review somewhat cumbersome and this might also be a problem if you step down and someone else becomes the maintainer.

I've split me review in 2 parts, first some comments about how you've implemented the sysfs API:

I notice that you have added foo_label sysfs entries, while you have nothing usefull to put in them, please do not _label entries are only for when the driver knows / can provide a label in the prefered format for a human reader of the sensors output, so not "in1", but something like "ATX 12V", so please remove all _label sysfs entries and remove the corresponding "char *label" member from the asc7621_param struct.

Likewise you've added tempX_type entries, but these always return the same value, in this case they can (and should be omitted) there is only the need to know (and more importantly be able to set) the type of the sensor, if it can be of different types, as in this case the BIOS might have set it wrong, when you remove these sysfs entries, you can also remove the corresponding "value" member from the asc7621_param struct.



And second, some feedback on the code itself.

First of all in *ALL* your store functions you need to store the result of strtol in a long and strtoul in an unsigned long, storing in smaller types can cause an overflow before you do any checking / clamping when the user gives a really large value as input.

Also in *ALL* store functions please use strict_strtol (and strtoul) instead of simple.

Last in some functions you need to clamp the input from the user to the valid input range before doing further calculations to avoid overflows during the calculation. Often clamping after calculations can be too late.

Example:

Instead of:

static ssize_t store_u8(struct device *dev, struct device_attribute *attr,
                         const char *buf, size_t count)
{
         SETUP_STORE_data_param(dev, attr);

         u8 reqval = simple_strtoul(buf, NULL, 10);

         mutex_lock(&data->update_lock);
         data->reg[param->msb[0]] = reqval;
         write_byte(param->msb[0], reqval);
         mutex_unlock(&data->update_lock);
         return count;
}

Write:

static ssize_t store_u8(struct device *dev, struct device_attribute *attr,
                         const char *buf, size_t count)
{
     SETUP_STORE_data_param(dev, attr);
     long reqval;

     if (strict_strtoul(buf, 10, &reqval))
         return -EINVAL;

     reqval = SENSORS_LIMIT(v, 0, 255);

         mutex_lock(&data->update_lock);
         data->reg[param->msb[0]] = reqval;
         write_byte(param->msb[0], reqval);
         mutex_unlock(&data->update_lock);
         return count;
}

Note that in this example all 3 described changes were made (use long, use strict_strtol, clamp before further use). *ALL* your store functions need the first 2 changes (use long, use strict_strtol), so I'm not going to make that comment for each and every store functions in the detailed comments below.

Where a store function needs earlier / different clamping I will make a comment in the detailed feedback given below.

Below are pieces of code from the driver with detailed feedback below them.

###

  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; version 2 of the License.
  *

I notice there is no "or at your option any later version" language here, this
is fine, but I wonder if this is on purpose, if not please add "or at your
option any later version", so that if the kernel ever (not likely) wants to
move to GPL v3 this is one less file to worry about.

###

/*
  * The "chips" enum created by I2C_CLIENT_INSMOD_2 has "any_chip" as
  * the first element in the enum, so it must also be first in our array.
  */
static struct asc7621_chip asc7621_chips[] = {
         {
          .name = "any",.chip_type = any_chip,
          },
         {
          .name = "asc7621",.chip_type = asc7621,
          .company_reg = 0x3e,.company_id = 0x61,
          .verstep_reg = 0x3f,.verstep_id = 0x6c,
          .addresses = normal_i2c,
          },
         {
          .name = "asc7621a",.chip_type = asc7621a,
          .company_reg = 0x3e,.company_id = 0x61,
          .verstep_reg = 0x3f,.verstep_id = 0x6d,
          .addresses = normal_i2c,
          },
};

Your driver still uses old style i2c driver binding, please update it too the
new style. See for example:
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-July/023773.html

You will probably want to remove using this when you do this, as this overlaps
with the i2c_device_id array you need to declare then.

###

static ssize_t store_u8(struct device *dev, struct device_attribute *attr,
                         const char *buf, size_t count)
{
         SETUP_STORE_data_param(dev, attr);

         u8 reqval = simple_strtoul(buf, NULL, 10);

         mutex_lock(&data->update_lock);
         data->reg[param->msb[0]] = reqval;
         write_byte(param->msb[0], reqval);
         mutex_unlock(&data->update_lock);
         return count;
}

Clamp large input values instead of allowing large values to cause overflows, see above for how this function should look (IMHO)

###

static ssize_t store_bitmask(struct device *dev,
                              struct device_attribute *attr,
                              const char *buf, size_t count)
{
         SETUP_STORE_data_param(dev, attr);

         u8 reqval = simple_strtoul(buf, NULL, 10);

Same as for store_u8, except the values between which to clamp depend on the mask.

###

static ssize_t show_fan16(struct device *dev,
                           struct device_attribute *attr, char *buf)
{
         SETUP_SHOW_data_param(dev, attr);

         u16 regval = (data->reg[param->msb[0]] << 8) | data->reg[param->lsb[0]]


You are using multiple values from data here, this might race with device update resulting in using an old msb with a new lsb or vica versa, so you need locking around this.

###

static ssize_t show_in10(struct device *dev, struct device_attribute *attr,
                          char *buf)
{

You are using multiple values from data here, you need locking around this.

###

static ssize_t store_in8(struct device *dev, struct device_attribute *attr,
                          const char *buf, size_t count)
{

         u8 nr;
         s32 reqval;

         SETUP_STORE_data_param(dev, attr);

         nr = sda->index;
         reqval = simple_strtoul(buf, NULL, 10);
         reqval *= asc7621_in_scaling_div[nr];
         reqval /= asc7621_in_scaling_mul[nr];
         reqval = SENSORS_LIMIT(reqval, 0, 255);

do SENSORS_LIMIT before calculations, the multiple might cause an overflow
otherwise (and ofcourse, reqval should be a long, use strict_strtol).

###
static ssize_t show_temp10(struct device *dev,
                            struct device_attribute *attr, char *buf)
{

You are using multiple values from data here, you need locking around this.

###

static ssize_t show_ap2_temp(struct device *dev,
                              struct device_attribute *attr, char *buf)
{

You are using multiple values from data here, you need locking around this.

###

static ssize_t store_ap2_temp(struct device *dev,
                               struct device_attribute *attr,
                               const char *buf, size_t count)
{

you need to take the lock earlier, before reading auto_point1 from the cached
register array (and ofcourse, reqval should be a long, use strict_strtol).

###

static ssize_t show_pwm_enable(struct device *dev,
                                struct device_attribute *attr, char *buf)
{

You are using multiple values from data here, you need locking around this.

###

In store store_pwm_freq() / store_pwm_ast() / store_temp_st() the following:
         if (newval == 255)
                 return count;

Should return -EINVAL, as you are rejecting the value (not making any changes)

###

Note to self all store / show functions are reviewed

---end---

Regards,

Hans

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[Jean Delvare]

On Sat, 23 Jan 2010 23:22:29 +0530, Jaswinder Singh Rajput wrote:
> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
> This chip is used by various Intel Motherboards.
> 
> Is it still under review and testing :
> 
> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
> 
> http://www.spinics.net/lists/lm-sensors/msg26915.html
> 
> http://www.spinics.net/lists/lm-sensors/msg26916.html
> 
> http://www.lm-sensors.org/wiki/Devices

I've updated the wiki entry to point to the latest version of the
driver. I've also included a complete list of known requests so far,
which can be used to get in touch with potential testers.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: Status of Andigilog asc7621 driver submitted by George Joseph on 2008-05-29

[Jean Delvare]

On Sat, 23 Jan 2010 23:22:29 +0530, Jaswinder Singh Rajput wrote:
> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
> This chip is used by various Intel Motherboards.
> 
> Is it still under review and testing :
> 
> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
> 
> http://www.spinics.net/lists/lm-sensors/msg26915.html
> 
> http://www.spinics.net/lists/lm-sensors/msg26916.html
> 
> http://www.lm-sensors.org/wiki/Devices

I've updated the wiki entry to point to the latest version of the
driver. I've also included a complete list of known requests so far,
which can be used to get in touch with potential testers.

-- 
Jean Delvare

Re: Status of Andigilog asc7621 driver submitted by George Joseph on 2008-05-29

[Jaswinder Singh Rajput]

Hello Jean,

On Sun, Jan 24, 2010 at 3:00 PM, Jean Delvare <khali@linux-fr.org> wrote:
> On Sat, 23 Jan 2010 23:22:29 +0530, Jaswinder Singh Rajput wrote:
>> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
>> This chip is used by various Intel Motherboards.
>>
>> Is it still under review and testing :
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
>>
>> http://www.spinics.net/lists/lm-sensors/msg26915.html
>>
>> http://www.spinics.net/lists/lm-sensors/msg26916.html
>>
>> http://www.lm-sensors.org/wiki/Devices
>
> I've updated the wiki entry to point to the latest version of the
> driver. I've also included a complete list of known requests so far,
> which can be used to get in touch with potential testers.
>

Thanks for updating the wiki.

Latest asc7621 driver is giving compilation errors with latest kernel
git. Do you also also have plans to update the driver so that we can
use the asc7621 driver.

I am ready for testing and debugging it.

Thank you,
--
Jaswinder Singh.

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[Jaswinder Singh Rajput]

Hello Jean,

On Sun, Jan 24, 2010 at 3:00 PM, Jean Delvare <khali@linux-fr.org> wrote:
> On Sat, 23 Jan 2010 23:22:29 +0530, Jaswinder Singh Rajput wrote:
>> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
>> This chip is used by various Intel Motherboards.
>>
>> Is it still under review and testing :
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
>>
>> http://www.spinics.net/lists/lm-sensors/msg26915.html
>>
>> http://www.spinics.net/lists/lm-sensors/msg26916.html
>>
>> http://www.lm-sensors.org/wiki/Devices
>
> I've updated the wiki entry to point to the latest version of the
> driver. I've also included a complete list of known requests so far,
> which can be used to get in touch with potential testers.
>

Thanks for updating the wiki.

Latest asc7621 driver is giving compilation errors with latest kernel
git. Do you also also have plans to update the driver so that we can
use the asc7621 driver.

I am ready for testing and debugging it.

Thank you,
--
Jaswinder Singh.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[Jean Delvare]

On Sun, 24 Jan 2010 17:44:11 +0530, Jaswinder Singh Rajput wrote:
> Hello Jean,
> 
> On Sun, Jan 24, 2010 at 3:00 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > On Sat, 23 Jan 2010 23:22:29 +0530, Jaswinder Singh Rajput wrote:
> >> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
> >> This chip is used by various Intel Motherboards.
> >>
> >> Is it still under review and testing :
> >>
> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
> >>
> >> http://www.spinics.net/lists/lm-sensors/msg26915.html
> >>
> >> http://www.spinics.net/lists/lm-sensors/msg26916.html
> >>
> >> http://www.lm-sensors.org/wiki/Devices
> >
> > I've updated the wiki entry to point to the latest version of the
> > driver. I've also included a complete list of known requests so far,
> > which can be used to get in touch with potential testers.
> >
> 
> Thanks for updating the wiki.
> 
> Latest asc7621 driver is giving compilation errors with latest kernel
> git. Do you also also have plans to update the driver so that we can
> use the asc7621 driver.
> 
> I am ready for testing and debugging it.

No, I don't have any plan. I already have too much on my plate, adding
anything would be unreasonable.

The only way I would start working on this is if someone would offer me
one of these motherboards which use the chip in question.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: Status of Andigilog asc7621 driver submitted by George Joseph on 2008-05-29

[Jean Delvare]

On Sun, 24 Jan 2010 17:44:11 +0530, Jaswinder Singh Rajput wrote:
> Hello Jean,
> 
> On Sun, Jan 24, 2010 at 3:00 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > On Sat, 23 Jan 2010 23:22:29 +0530, Jaswinder Singh Rajput wrote:
> >> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
> >> This chip is used by various Intel Motherboards.
> >>
> >> Is it still under review and testing :
> >>
> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html
> >>
> >> http://www.spinics.net/lists/lm-sensors/msg26915.html
> >>
> >> http://www.spinics.net/lists/lm-sensors/msg26916.html
> >>
> >> http://www.lm-sensors.org/wiki/Devices
> >
> > I've updated the wiki entry to point to the latest version of the
> > driver. I've also included a complete list of known requests so far,
> > which can be used to get in touch with potential testers.
> >
> 
> Thanks for updating the wiki.
> 
> Latest asc7621 driver is giving compilation errors with latest kernel
> git. Do you also also have plans to update the driver so that we can
> use the asc7621 driver.
> 
> I am ready for testing and debugging it.

No, I don't have any plan. I already have too much on my plate, adding
anything would be unreasonable.

The only way I would start working on this is if someone would offer me
one of these motherboards which use the chip in question.

-- 
Jean Delvare

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[George Joseph]

> -----Original Message-----
> From: Jaswinder Singh Rajput [mailto:jaswinderlinux@gmail.com]
> Sent: Sunday, January 24, 2010 5:14 AM
> To: Jean Delvare
> Cc: George Joseph; lm-sensors@lm-sensors.org; Linux Kernel Mailing
> List; Hans de Goede
> Subject: Re: Status of Andigilog asc7621 driver submitted by George
> Joseph on 2008-05-29
> 
> Hello Jean,
> 
> On Sun, Jan 24, 2010 at 3:00 PM, Jean Delvare <khali@linux-fr.org>
> wrote:
> > On Sat, 23 Jan 2010 23:22:29 +0530, Jaswinder Singh Rajput wrote:
> >> Andigilog asc7621 driver submitted by George Joseph on 2008-05-29,
> >> This chip is used by various Intel Motherboards.
> >>
> >> Is it still under review and testing :
> >>
> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-
> May/023257.html
> >>
> >> http://www.spinics.net/lists/lm-sensors/msg26915.html
> >>
> >> http://www.spinics.net/lists/lm-sensors/msg26916.html
> >>
> >> http://www.lm-sensors.org/wiki/Devices
> >
> > I've updated the wiki entry to point to the latest version of the
> > driver. I've also included a complete list of known requests so far,
> > which can be used to get in touch with potential testers.
> >
> 
> Thanks for updating the wiki.
> 
> Latest asc7621 driver is giving compilation errors with latest kernel
> git. Do you also also have plans to update the driver so that we can
> use the asc7621 driver.
> 
> I am ready for testing and debugging it.

I'll update the driver for 2.6.33.  Give me a day or two.

> 
> Thank you,
> --
> Jaswinder Singh.


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[George Joseph]

All items incorporated.  Updated patch to follow.

> -----Original Message-----
> From: Hans de Goede [mailto:j.w.r.degoede@hhs.nl]
> Sent: Tuesday, January 26, 2010 4:56 AM
> To: Jaswinder Singh Rajput
> Cc: George Joseph; lm-sensors@lm-sensors.org; Jean Delvare; Linux
> Kernel Mailing List
> Subject: Re: [lm-sensors] Status of Andigilog asc7621 driver submitted
> by George Joseph on 2008-05-29
> 
> On 01/24/2010 06:05 AM, Jaswinder Singh Rajput wrote:
> > Hello Hans,
> >
> > On Sat, Jan 23, 2010 at 11:28 PM, Hans de Goede<j.w.r.degoede@hhs.nl>
> wrote:
> >> Hi,
> >>
> >> I just checked my Drafts folder, and I still have my unfinished
> review
> >> in there. So if there is interest I can send that, note that it is
> >> not a complete review though (there is a note in there which part
> >> of the code is reviewed and which still needs to be reviewed).
> >>
> >
> > Please provide your review so that we can discuss about this driver
> > and make relevant changes to accept it.
> >
> 
> Ok, here is the partly review I've done:
> 
> ---begin---
> Hi Joseph,
> 
> george.joseph@peakin.com wrote:
>  > Here's the c file by itself.
>  >
>  > Hans, will you have any time to review the driver in the near
> future?
>  >
> 
> I believe I said I would make time for this some time ago. Given that
> the 2.6.29 merge window has already opened, I've now done so (made
> time). So that this hopefully / maybe can make 2.6.29's merge window.
> 
> I must say that this driver deviates a lot from the standard way all
> other hwmon drivers are written making the review somewhat cumbersome
> and this might also be a problem if you step down and someone else
> becomes the maintainer.
> 
> I've split me review in 2 parts, first some comments about how you've
> implemented the sysfs API:
> 
> I notice that you have added foo_label sysfs entries, while you have
> nothing usefull to put in them, please do not _label entries are only
> for when the driver knows / can provide a label in the prefered format
> for a human reader of the sensors output, so not "in1", but something
> like "ATX 12V", so please remove all _label sysfs entries and remove
> the corresponding "char *label" member from the asc7621_param struct.
> 
> Likewise you've added tempX_type entries, but these always return the
> same value, in this case they can (and should be omitted) there is only
> the need to know (and more importantly be able to set) the type of the
> sensor, if it can be of different types, as in this case the BIOS might
> have set it wrong, when you remove these sysfs entries, you can also
> remove the corresponding "value" member from the asc7621_param struct.
> 
> 
> 
> And second, some feedback on the code itself.
> 
> First of all in *ALL* your store functions you need to store the result
> of strtol in a long and strtoul in an unsigned long, storing in smaller
> types can cause an overflow before you do any checking / clamping when
> the user gives a really large value as input.
> 
> Also in *ALL* store functions please use strict_strtol (and strtoul)
> instead of simple.
> 
> Last in some functions you need to clamp the input from the user to the
> valid input range before doing further calculations to avoid overflows
> during the calculation. Often clamping after calculations can be too
> late.
> 
> Example:
> 
> Instead of:
> 
> static ssize_t store_u8(struct device *dev, struct device_attribute
> *attr,
>                          const char *buf, size_t count)
> {
>          SETUP_STORE_data_param(dev, attr);
> 
>          u8 reqval = simple_strtoul(buf, NULL, 10);
> 
>          mutex_lock(&data->update_lock);
>          data->reg[param->msb[0]] = reqval;
>          write_byte(param->msb[0], reqval);
>          mutex_unlock(&data->update_lock);
>          return count;
> }
> 
> Write:
> 
> static ssize_t store_u8(struct device *dev, struct device_attribute
> *attr,
>                          const char *buf, size_t count)
> {
>      SETUP_STORE_data_param(dev, attr);
>      long reqval;
> 
>      if (strict_strtoul(buf, 10, &reqval))
>          return -EINVAL;
> 
>      reqval = SENSORS_LIMIT(v, 0, 255);
> 
>          mutex_lock(&data->update_lock);
>          data->reg[param->msb[0]] = reqval;
>          write_byte(param->msb[0], reqval);
>          mutex_unlock(&data->update_lock);
>          return count;
> }
> 
> Note that in this example all 3 described changes were made (use long,
> use strict_strtol, clamp before further use). *ALL* your store
> functions need the first 2 changes (use long, use strict_strtol), so
> I'm not going to make that comment for each and every store functions
> in the detailed comments below.
> 
> Where a store function needs earlier / different clamping I will make a
> comment in the detailed feedback given below.
> 
> Below are pieces of code from the driver with detailed feedback below
> them.
> 
> ###
> 
>   * This program is free software; you can redistribute it and/or
> modify
>   * it under the terms of the GNU General Public License as published
> by
>   * the Free Software Foundation; version 2 of the License.
>   *
> 
> I notice there is no "or at your option any later version" language
> here, this
> is fine, but I wonder if this is on purpose, if not please add "or at
> your
> option any later version", so that if the kernel ever (not likely)
> wants to
> move to GPL v3 this is one less file to worry about.
> 
> ###
> 
> /*
>   * The "chips" enum created by I2C_CLIENT_INSMOD_2 has "any_chip" as
>   * the first element in the enum, so it must also be first in our
> array.
>   */
> static struct asc7621_chip asc7621_chips[] = {
>          {
>           .name = "any",.chip_type = any_chip,
>           },
>          {
>           .name = "asc7621",.chip_type = asc7621,
>           .company_reg = 0x3e,.company_id = 0x61,
>           .verstep_reg = 0x3f,.verstep_id = 0x6c,
>           .addresses = normal_i2c,
>           },
>          {
>           .name = "asc7621a",.chip_type = asc7621a,
>           .company_reg = 0x3e,.company_id = 0x61,
>           .verstep_reg = 0x3f,.verstep_id = 0x6d,
>           .addresses = normal_i2c,
>           },
> };
> 
> Your driver still uses old style i2c driver binding, please update it
> too the
> new style. See for example:
> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-July/023773.html
> 
> You will probably want to remove using this when you do this, as this
> overlaps
> with the i2c_device_id array you need to declare then.
> 
> ###
> 
> static ssize_t store_u8(struct device *dev, struct device_attribute
> *attr,
>                          const char *buf, size_t count)
> {
>          SETUP_STORE_data_param(dev, attr);
> 
>          u8 reqval = simple_strtoul(buf, NULL, 10);
> 
>          mutex_lock(&data->update_lock);
>          data->reg[param->msb[0]] = reqval;
>          write_byte(param->msb[0], reqval);
>          mutex_unlock(&data->update_lock);
>          return count;
> }
> 
> Clamp large input values instead of allowing large values to cause
> overflows, see above for how this function should look (IMHO)
> 
> ###
> 
> static ssize_t store_bitmask(struct device *dev,
>                               struct device_attribute *attr,
>                               const char *buf, size_t count)
> {
>          SETUP_STORE_data_param(dev, attr);
> 
>          u8 reqval = simple_strtoul(buf, NULL, 10);
> 
> Same as for store_u8, except the values between which to clamp depend
> on the mask.
> 
> ###
> 
> static ssize_t show_fan16(struct device *dev,
>                            struct device_attribute *attr, char *buf)
> {
>          SETUP_SHOW_data_param(dev, attr);
> 
>          u16 regval = (data->reg[param->msb[0]] << 8) | data-
> >reg[param->lsb[0]]
> 
> 
> You are using multiple values from data here, this might race with
> device update resulting in using an old msb with a new lsb or vica
> versa, so you need locking around this.
> 
> ###
> 
> static ssize_t show_in10(struct device *dev, struct device_attribute
> *attr,
>                           char *buf)
> {
> 
> You are using multiple values from data here, you need locking around
> this.
> 
> ###
> 
> static ssize_t store_in8(struct device *dev, struct device_attribute
> *attr,
>                           const char *buf, size_t count)
> {
> 
>          u8 nr;
>          s32 reqval;
> 
>          SETUP_STORE_data_param(dev, attr);
> 
>          nr = sda->index;
>          reqval = simple_strtoul(buf, NULL, 10);
>          reqval *= asc7621_in_scaling_div[nr];
>          reqval /= asc7621_in_scaling_mul[nr];
>          reqval = SENSORS_LIMIT(reqval, 0, 255);
> 
> do SENSORS_LIMIT before calculations, the multiple might cause an
> overflow
> otherwise (and ofcourse, reqval should be a long, use strict_strtol).
> 
> ###
> static ssize_t show_temp10(struct device *dev,
>                             struct device_attribute *attr, char *buf)
> {
> 
> You are using multiple values from data here, you need locking around
> this.
> 
> ###
> 
> static ssize_t show_ap2_temp(struct device *dev,
>                               struct device_attribute *attr, char *buf)
> {
> 
> You are using multiple values from data here, you need locking around
> this.
> 
> ###
> 
> static ssize_t store_ap2_temp(struct device *dev,
>                                struct device_attribute *attr,
>                                const char *buf, size_t count)
> {
> 
> you need to take the lock earlier, before reading auto_point1 from the
> cached
> register array (and ofcourse, reqval should be a long, use
> strict_strtol).
> 
> ###
> 
> static ssize_t show_pwm_enable(struct device *dev,
>                                 struct device_attribute *attr, char
> *buf)
> {
> 
> You are using multiple values from data here, you need locking around
> this.
> 
> ###
> 
> In store store_pwm_freq() / store_pwm_ast() / store_temp_st() the
> following:
>          if (newval = 255)
>                  return count;
> 
> Should return -EINVAL, as you are rejecting the value (not making any
> changes)
> 
> ###
> 
> Note to self all store / show functions are reviewed
> 
> ---end---
> 
> Regards,
> 
> Hans


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[Jaswinder Singh Rajput]

Hello George,

On Sun, Jan 31, 2010 at 3:38 AM, George Joseph
<george.joseph@fairview5.com> wrote:
> All items incorporated.  Updated patch to follow.
>

If you need any kind of help, please let me know. I am ready to test
and debug the patch.

Thanks,
--
Jaswinder Singh.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[George Joseph]

On 2/19/2010 7:56 AM, Jaswinder Singh Rajput wrote:
> Hello George,
>
> On Sun, Jan 31, 2010 at 3:38 AM, George Joseph
> <george.joseph@fairview5.com>  wrote:
>    
>> All items incorporated.  Updated patch to follow.
>>
>>      
> If you need any kind of help, please let me know. I am ready to test
> and debug the patch.
>    
I posted the updated patch to the list on 1/30.
http://lists.lm-sensors.org/pipermail/lm-sensors/2010-January/027739.html

Give it a try and let me know how it works.  I've got it running on the 
Intel XBX2, BT2, and DP45SG boards all on one version or another of 2.6.33.

> Thanks,
> --
> Jaswinder Singh.
>    


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[Jaswinder Singh Rajput]

Hello George,

On Fri, Feb 19, 2010 at 8:48 PM, George Joseph
<george.joseph@fairview5.com> wrote:
> On 2/19/2010 7:56 AM, Jaswinder Singh Rajput wrote:
>>
>> Hello George,
>>
>> On Sun, Jan 31, 2010 at 3:38 AM, George Joseph
>> <george.joseph@fairview5.com>  wrote:
>>
>>>
>>> All items incorporated.  Updated patch to follow.
>>>
>>>
>>
>> If you need any kind of help, please let me know. I am ready to test
>> and debug the patch.
>>
>
> I posted the updated patch to the list on 1/30.
> http://lists.lm-sensors.org/pipermail/lm-sensors/2010-January/027739.html
>
> Give it a try and let me know how it works.  I've got it running on the
> Intel XBX2, BT2, and DP45SG boards all on one version or another of 2.6.33.
>

I tested this patch on Intel WX58BP with Core i7, so far it seems
good, I am doing further investigations regarding temp1, temp5, temp6,
temp7 and temp8.

Can we make above temp more readable.

[jaswinder@i7 ~]$ sensors
coretemp-isa-0000
Adapter: ISA adapter
Core 0:      +40.0°C  (high = +80.0°C, crit = +100.0°C)

coretemp-isa-0001
Adapter: ISA adapter
Core 1:      +38.0°C  (high = +80.0°C, crit = +100.0°C)

coretemp-isa-0002
Adapter: ISA adapter
Core 2:      +43.0°C  (high = +80.0°C, crit = +100.0°C)

coretemp-isa-0003
Adapter: ISA adapter
Core 3:      +36.0°C  (high = +80.0°C, crit = +100.0°C)

coretemp-isa-0004
Adapter: ISA adapter
Core 4:      +41.0°C  (high = +80.0°C, crit = +100.0°C)

coretemp-isa-0005
Adapter: ISA adapter
Core 5:      +38.0°C  (high = +80.0°C, crit = +100.0°C)

coretemp-isa-0006
Adapter: ISA adapter
Core 6:      +43.0°C  (high = +80.0°C, crit = +100.0°C)

coretemp-isa-0007
Adapter: ISA adapter
Core 7:      +36.0°C  (high = +80.0°C, crit = +100.0°C)

asc7621a-i2c-0-2e
Adapter: SMBus I801 adapter at 2000
in0:         +1.08 V  (min =  +0.00 V, max =  +3.31 V)
in1:         +0.92 V  (min =  +0.00 V, max =  +2.99 V)
in2:         +3.19 V  (min =  +0.00 V, max =  +4.36 V)
in3:         +4.99 V  (min =  +0.00 V, max =  +6.61 V)
in4:        +12.00 V  (min =  +0.00 V, max = +15.94 V)
fan1:        947 RPM  (min =    0 RPM)
fan2:       2425 RPM  (min =    0 RPM)
fan3:          0 RPM  (min =    0 RPM)
fan4:        625 RPM  (min =    0 RPM)
temp1:       -57.8°C  (low  = -127.0°C, high =  +0.0°C)
                      (crit = -17.0°C)
temp2:       +35.5°C  (low  = -127.0°C, high = +127.0°C)
                      (crit = +65.0°C)
temp3:       +42.0°C  (low  = -127.0°C, high = +127.0°C)
                      (crit = +65.0°C)
temp4:       +52.0°C  (low  = -127.0°C, high = +127.0°C)
                      (crit = +103.0°C)
temp5:       -58.0°C
temp6:       -84.5°C
temp7:       +96.2°C
temp8:      +118.5°C

[jaswinder@i7 ~]$

Thanks,
--
Jaswinder Singh

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[Jaswinder Singh Rajput]

Hello George,

On Fri, Feb 19, 2010 at 11:24 PM, Jaswinder Singh Rajput
<jaswinderlinux@gmail.com> wrote:
>
> asc7621a-i2c-0-2e
> Adapter: SMBus I801 adapter at 2000
> in0:         +1.08 V  (min =  +0.00 V, max =  +3.31 V)
> in1:         +0.92 V  (min =  +0.00 V, max =  +2.99 V)
> in2:         +3.19 V  (min =  +0.00 V, max =  +4.36 V)
> in3:         +4.99 V  (min =  +0.00 V, max =  +6.61 V)
> in4:        +12.00 V  (min =  +0.00 V, max = +15.94 V)
> fan1:        947 RPM  (min =    0 RPM)
> fan2:       2425 RPM  (min =    0 RPM)
> fan3:          0 RPM  (min =    0 RPM)
> fan4:        625 RPM  (min =    0 RPM)
> temp1:       -57.8°C  (low  = -127.0°C, high =  +0.0°C)
>                      (crit = -17.0°C)
> temp2:       +35.5°C  (low  = -127.0°C, high = +127.0°C)
>                      (crit = +65.0°C)
> temp3:       +42.0°C  (low  = -127.0°C, high = +127.0°C)
>                      (crit = +65.0°C)
> temp4:       +52.0°C  (low  = -127.0°C, high = +127.0°C)
>                      (crit = +103.0°C)
> temp5:       -58.0°C
> temp6:       -84.5°C
> temp7:       +96.2°C
> temp8:      +118.5°C
>

I did further investigations and noticed that in my case : temp1 temp5 and temp6, temp7 and temp8 are false readings which never
changes :

temp1:       -33.0°C  (low  = -127.0°C, high =  +0.0°C)
                      (crit = -17.0°C)
temp2:       +38.2°C  (low  = -127.0°C, high = +127.0°C)
                      (crit = +65.0°C)
temp3:       +51.8°C  (low  = -127.0°C, high = +127.0°C)
                      (crit = +65.0°C)
temp4:       +55.5°C  (low  = -127.0°C, high = +127.0°C)
                      (crit = +103.0°C)
temp5:       -33.0°C
temp6:       -84.5°C
temp7:       +96.2°C
temp8:      +118.5°C

Can you give me some hint why temp6, temp7 and temp8 are showing wrong
value for Intel WX58BP with Core i7.

Can you share your temp6, temp7 and temp8 readings.

Thanks,
--
Jaswinder Singh.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Status of Andigilog asc7621 driver submitted by

[George Joseph]

On 2/19/2010 11:50 AM, Jaswinder Singh Rajput wrote:
> Hello George,
>
> On Fri, Feb 19, 2010 at 11:24 PM, Jaswinder Singh Rajput
> <jaswinderlinux@gmail.com>  wrote:
>    
>> asc7621a-i2c-0-2e
>> Adapter: SMBus I801 adapter at 2000
>> in0:         +1.08 V  (min =  +0.00 V, max =  +3.31 V)
>> in1:         +0.92 V  (min =  +0.00 V, max =  +2.99 V)
>> in2:         +3.19 V  (min =  +0.00 V, max =  +4.36 V)
>> in3:         +4.99 V  (min =  +0.00 V, max =  +6.61 V)
>> in4:        +12.00 V  (min =  +0.00 V, max = +15.94 V)
>> fan1:        947 RPM  (min =    0 RPM)
>> fan2:       2425 RPM  (min =    0 RPM)
>> fan3:          0 RPM  (min =    0 RPM)
>> fan4:        625 RPM  (min =    0 RPM)
>> temp1:       -57.8°C  (low  = -127.0°C, high =  +0.0°C)
>>                       (crit = -17.0°C)
>> temp2:       +35.5°C  (low  = -127.0°C, high = +127.0°C)
>>                       (crit = +65.0°C)
>> temp3:       +42.0°C  (low  = -127.0°C, high = +127.0°C)
>>                       (crit = +65.0°C)
>> temp4:       +52.0°C  (low  = -127.0°C, high = +127.0°C)
>>                       (crit = +103.0°C)
>> temp5:       -58.0°C
>> temp6:       -84.5°C
>> temp7:       +96.2°C
>> temp8:      +118.5°C
>>
>>      
> I did further investigations and noticed that in my case : temp1 > temp5 and temp6, temp7 and temp8 are false readings which never
> changes :
>
> temp1:       -33.0°C  (low  = -127.0°C, high =  +0.0°C)
>                        (crit = -17.0°C)
> temp2:       +38.2°C  (low  = -127.0°C, high = +127.0°C)
>                        (crit = +65.0°C)
> temp3:       +51.8°C  (low  = -127.0°C, high = +127.0°C)
>                        (crit = +65.0°C)
> temp4:       +55.5°C  (low  = -127.0°C, high = +127.0°C)
>                        (crit = +103.0°C)
> temp5:       -33.0°C
> temp6:       -84.5°C
> temp7:       +96.2°C
> temp8:      +118.5°C
>
> Can you give me some hint why temp6, temp7 and temp8 are showing wrong
> value for Intel WX58BP with Core i7.
>    

The mapping of the temps is controlled by the motherboard design and the 
BIOS.  On most Intel motherboards...

temp1 = PECI CPU temp.  I.E.  Degrees below thermal throttling.  Similar 
to what coretemp reports.
temp2 = aSC7621 chip itself.  I.E.  Motherboard temp.
temp3 = Sensor between the DIMM sockets.
temp4 = CPU diode temp.
temp5 = temp1
temp6, 7, 8 = Documented by Andigilog as additional PECI temps but not 
documented by Intel.

That's all the info I have.
> Can you share your temp6, temp7 and temp8 readings.
>    
Pretty much same as yours.
> Thanks,
> --
> Jaswinder Singh.
>    


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors