Hi Jean This is Chunhao. Quoting Jean: > No register read should happen outside of the update function > (except in the init function when loading the driver, granted). But in kernel/chips/w83781d.c I find that there are some "w83781d_read_value" in the following functions: w83781d_beep() w83781d_fan_div() w83781d_pwm() w83781d_sens() when these functions do the "write" operations, which are outside the update function. So my questions are: (1) Is it improper for w83781d to use "w83781d_read_value" in that way? (2) In w83792d.c, there are also many places where call the w83792d_read_value() function outside w83792d_update_client() when dealing with the "write" operation, which is similar with the above situation in w83781d.c Need I modify the source into using the member of struct w83792d_data? Thanks Best Regards Chunhao -----Original Message----- From: Jean Delvare [mailto:khali at linux-fr.org] Sent: 2004??12??5?? 19:17 To: PI14 HUANG0 Cc: LM Sensors Subject: RE: New chip driver development plan to Lm_sensors Hi Chunhao, Sorry for the delay, I have been encountering severe mail problems and lost 6 days worth of data, including your latest request for information. I finally found it while browsing the mailing-list archive, so here are my answers. > I have finished some of the functions in w83792d.c, and can produce > some device files in directory /proc/sys/dev/sensors/w83792d-i2c-0-2f/ > for example in0. Great :) > But each time(except within the interval "HZ + HZ/2") I read the in0 file > by "cat" command, I find that the delay is comparatively large, which is > about 1-2 seconds. That's to say it takes me about 1-2 seconds to show me > the content of "in0", which also leads to the delay of the program "sensors". This is expected. All reads are grouped in a single function by design, so updating one value or the whole chip takes the same time. > I have debugged about it, find that the function "w83792d_update_client" > is the reason of the delay. Because in the course of the delay, this > function is executing and reading the registers' value one by one. True, you got it. > I also tested another Winbond chip W83627THF with the driver w83627hf.c > find that this phenomena does NOT exist on W83627THF. That's to say all > the device files of W83627THF can be read very quickly almost without any > delay, just like regular text file. This is due to the speed difference between I2C (SMBus) and ISA. ISA is really fast, I2C/SMbus is comparatively very slow. > So my questions are: > (1) Does this phenomena appears in other I2C chip drivers? such as > w83781d.c. Because W83627THF use ISA bus instead of I2C. Yes, that's exactlty it. > (2) Does it have something to do with the register numbers be read in > the function " w83792d_update_client()"? > Because w83792d_update_client read much more registers than w83627hf. Yes, the time required for one update is naturally proportional to the number of registers, for a given type of bus (ISA vs I2C). It makes no sense to compare a N1-register chip on I2C with a N2-register on ISA though. > (3) Or does it have something to do with the "w83792d_data->last_updated" > in the last mail? but there is also "w83627hf_data->last_updated" in > w83627hf.c, and the interval is same: HZ + HZ/2 This isn't. HZ + HZ/2 defines the minimal delay between two updates (1.5 second in this case). However, it makes sense to define a larger delay if the chip is slow updating, both for security/stability and user experience reasons. > (4) I also have tried to split the codes of w83792d_update_client() > into indivial function, for example: I move the following codes from > w83792d_update_client() into the function w83792d_in(), and comment > out the " w83792d_update_client()" in w83792d_in(), but it seems > useless, because the delay becomes more larger.:-( > > ************ codes be moved ****************** > data->in[0] = w83792d_read_value(client, W83792D_REG_IN0); > data->in[1] = w83792d_read_value(client, W83792D_REG_IN1); > data->in[2] = w83792d_read_value(client, W83792D_REG_IN2); > data->in[3] = w83792d_read_value(client, W83792D_REG_IN3); > data->in[4] = w83792d_read_value(client, W83792D_REG_IN4); > data->in[5] = w83792d_read_value(client, W83792D_REG_IN5); > data->in[6] = w83792d_read_value(client, W83792D_REG_IN6); > data->in[7] = w83792d_read_value(client, W83792D_REG_IN7); > data->in[8] = w83792d_read_value(client, W83792D_REG_IN8); By doing that you remove the effect of caching for these registers. This won't speed up the single-register reads, but will significantly slow "sensors" down. Also, it breaks our security model. Don't do that. No register read should happen outside of the update function (except in the init function when loading the driver, granted). The SMBus is slow, we have to live with it ;) One possibility is to have two different minimal delays between updates, one for measured values and one (longer) for values that supposedly don't change (such as limits and config). I think that only the lm85 driver is doing it this way so far, take a look there. For a chip with a very high number of registers, that may make sense. Another possibility is to use I2C/SMBus block reads. However: 1* The W83792D would need to support it, I don't know if it is the case. 2* Not all SMBus master chips support it, so the w83792d driver would still have to support the byte/word read method for those who don't. 3* Block reads have not been much tested so far so it might not be a good idea for you to rely on them. You would better get the driver working with regular byte/word reads first. Block reads can be added later (providing the W83792D supports them, that is). A third possibility would be to use SMBus direct byte reads instead of data byte reads, but this requires that the W83792D has autoincrement registers. Without a datasheet I of course cannot tell whether this is the case or not. Please do not hesitate to share your code with us as soon as you get something working, so that we can comment on it and users can start testing it too. Thanks, -- Jean Delvare http://khali.linux-fr.org/ ===========================================================================================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??.