Hi Jean, MDS Thank you for your suggestion, I will modify the source with your help. > 6* The code does *not* properly handle the case where subclients are > disabled. If subclients addresses are forced, they will be forcibly > enabled (this is OK). If they are not forced, then we might register > clients which do not exist (this is NOT OK). Then how should I handle the case where subclients are disabled? > [I'd have a side question, about the subclients in general... > Why do we register subclients addresses at all? We could instead > forcibly disable them. This is easier and just as efficient (disabling > the addresses doesn't disable the temperature channels themselves).] Yes, maybe you are right, but I think your this question is to the other members of lm_sensors group, Am I right? You need a discussion with them, because I'm not very familiar with the subclients. Thanks Best Regards Chunhao > -----Original Message----- > From: Jean Delvare [mailto:khali at linux-fr.org] > Sent: 2005??4??10?? 17:53 > To: Mark Studebaker; PI14 HUANG0 > Cc: LM Sensors > Subject: Re: Winbond W83792D driver > > Hi Chunhao, hi Mark, > > > I'm sorry I'm not really familiar with how subclients were done in > > 2.6, or 2.6 drivers in general. I did a diff between your w83792d.c > > and w83781d.c in the 2.6.11 kernel. I saw some differences in > > xxx_detect_subclients(). These may or may not make a difference. Be > > sure you don't have any compile warnings about wrong pointer types. > > One of the other guys on the list may be able to help you better. good > > luck. mds > > I've read Chunhao's code and found the problem: > > /* memset(data->lm75[0], 0x00, sizeof (struct i2c_client)); */ > > This is commented out in your code. The memset is NOT optional, it is a > requirement. The correct memset would be: > memset(data->lm75, 0x00, 2 * sizeof(struct i2c_client)); > Add this and this will solve your problem (at least it did for me). > > Additional notes: > > 1* You have put a comment: > struct i2c_client *lm75; /* for secondary I2C addresses */ > /* array of 2 pointers to subclients */ > > This is *not* an array of 2 pointers to subclient, this is a pointer to > an array of 2 subclients, which is quite different. > > 2* You include "lm75.h" but never use it. This header file contains > temperature converters for LM75-like chips. This doesn't seem to be the > case of the W83792D (according to your code at least). > > 3* Your code does *not* free the memory you allocated for subclients > when unloading the driver. Please add the necessary code. > > 4* You do *not* need to do casts like this one: > ((struct w83792d_data *) data) > It was needed in linux 2.4 but in 2.6 things are cleaner so the casts > are not needed. > > 5* In the case we do *not* force the subclient addresses, the code is: > > val1 = w83792d_read_value(new_client, > W83792D_REG_I2C_SUBADDR); > data->lm75[0].addr = 0x48 + (val1 & 0x03); > data->lm75[1].addr = 0x4c + ((val1 >> 4) & 0x03); > > This is *not* correct IMHO, because it assumes that the first subclient > is in 0x48-0x4b and the second it in 0x4c-0x4f. While this is true at > power-up for the W83792D, the BIOS may have changed this. So we should > not assume anything and simply do: > > data->lm75[0].addr = 0x48 + (val1 & 0x07); > data->lm75[1].addr = 0x48 + ((val1 >> 4) & 0x07); > > This should be fixed in the 2.4 driver as well. > > 6* The code does *not* properly handle the case where subclients are > disabled. If subclients addresses are forced, they will be forcibly > enabled (this is OK). If they are not forced, then we might register > clients which do not exist (this is NOT OK). > > [I'd have a side question, about the subclients in general... > Why do we register subclients addresses at all? We could instead > forcibly disable them. This is easier and just as efficient (disabling > the addresses doesn't disable the temperature channels themselves).] > > Thanks, > -- > Jean Delvare ===========================================================================================The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original author of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such person, please kindly reply the sender indicating accordingly and delete all copies of it from your computer and network server immediately. We thank you for your cooperation. It is advisable that any unauthorized use of confidential information of Winbond is strictly prohibited; and any information in this email that does not relate to the official business of Winbond shall be deemed as neither given nor endorsed by Winbond.===========================================================================================If your computer is unable to decode Chinese font, please ignore the following message. They essentially repea! t the English statement above.???H???????t?????q?l???]???????K?????T, ?????v???o?H?H???w?????H?H???\????. ?????z???D?Q???w?????H?H???]???????]?b???g???v?????????U???????H??, ???z?i?????o?H?H?????Y?N?H???q?q???P???????A???????H????. ?????z???X?@, ?????????P??. ?S??????, ???????g???v?????????????q?l?????K???T???????O?Q?Y???T????. ?H???P?????q?l???~?L???????e,???o?????????q?l?????????N??.
