Hi Rudolf, Jean
I finished the modification the 792 driver for linux-2.6 with your
suggestions
after your review, please refer to the contents below for more details.
The attachment is the 792 driver for linux-2.6, if you have time, would
you
please check my codes again before other guys start the test? Especially
the modified codes. After your confirmation and test, please send it to
the
kernel group for me, then give me a response, thanks.
On your request, the "sign-off" is:
Signed-off-by: Chunhao Huang <huang0 at winbond.com.tw>
> >(1) We misunderstand the meaning of temperature low limit, this bug
also
> >exists in the 792 driver for linux-2.4 and our 792 Windows driver.
>
> Please can you fix this ASAP so we can send it to kernel folks?
Fixed, need the up to date lm_sensors in the CVS, because the
lm_sensors-2.9.1
does NOT work.
> >(2) I have not implemented the 0.5 degree to temperature2 and
temperature3,
> >while the driver for linux-2.4 implemented this.
>
> That is the reason temp1 has separate handling and variable? If so,
please add
> the code.
> or better would be to fix existing code so it does not look so
strange.
>
> From Jean:
> No, better would be to actually add the handling of the additional
> resolution bit.
Fixed, it took me much time to fix this bug :-(
I don't think the chip drivers for linux-2.6 are of good
maintainability,
and readability because of those many macros in the driver.
> >(3) The 792 driver for linux-2.4 can adjust the fan divisor
automatically,
> >while I have not implemented it in this 2.6 one.
>
> This would be nice but better to put there some driver instead of
none...
> I think this can wait but it is good to have it soon.
NOT fixed, because I do NOT think the method in 792 driver for linux-2.4
is good enough. I will keep it until finding a good method to realize
the fan divisor's auto modification
> > #include <linux/config.h>
> > #include <linux/module.h>
> > #include <linux/init.h>
> > #include <linux/slab.h>
> > #include <linux/i2c.h>
> > #include <linux/i2c-sensor.h>
> > #include <linux/i2c-vid.h>
> > #include <asm/io.h>
> Do you really need this header?
Fixed
> > /* #include "lm75.h" */
> Please remove this.
Fixed
> > /* Addresses to scan */
> > static unsigned short normal_i2c[] = { 0x2c, 0x2f, I2C_CLIENT_END };
> > static unsigned int normal_isa[] = { I2C_CLIENT_ISA_END };
> >
> > /* Insmod parameters */
> > SENSORS_INSMOD_1(w83792d);
> > I2C_CLIENT_MODULE_PARM(force_subclients, "List of subclient
addresses: "
> > "{bus, clientaddr, subclientaddr1,
subclientaddr2}");
> >
> > static int init;
> > module_param(init, bool, 0);
> > MODULE_PARM_DESC(init, "Set to one for chip initialization");
>
> Maybe better "Set to one to force chip initialization"
Fixed
> > /* show/display the debug message */
> > #define W83792D_DEBUG 1
>
> There is some global #define for debugging called DEBUG it seems, so
maybe it
> would be good
> to remove this and use DEBUG later.
Fiexed
> > #define W83792D_REG_CHASSIS 0x42 /* Bit 5: Case Open status bit
*/
> > #define W83792D_REG_CHASSIS_CLR 0x44 /* Bit 7: Case Open
CLR_CHS/Reset
> bit */
> >
> > /* ctroll in0/in1 's limit modifiability */
> perhap a typo ?
Fixed
> > u8 in[9]; /* Register value */
> > u8 in_max[9]; /* Register value */
> > u8 in_min[9]; /* Register value */
> > u8 low_bits[2]; /* Register value */
>
> Please comment that lowbits belong to IN, or choose better name for
the variable
Fixed
> > u8 fan[7]; /* Register value */
> > u8 fan_min[7]; /* Register value */
> > u8 temp;
> > u8 temp_max; /* Register value */
> > u8 temp_max_hyst; /* Register value */
> > u8 temp_add[2]; /* Register value */
>
> Please align comment with tab to same col as above.
Fixed
> Also why is temp1 separated? why there is not temps[3] instead?
temp1 is separated because temp2 and temp3 need addtional resolution bit
to get/set the 0.5 degree, while temp1 need NOT.
> > u8 temp_max_add[2]; /* Register value */
> > u8 temp_max_hyst_add[2]; /* Register value */
>
> Big I and align too please, with tab to same col as above.
Fixed
> > #ifdef W83792D_DEBUG
> > static void w83792d_print_debug(struct w83792d_data *data);
> > #endif
>
> I think you can change it to: #ifdef DEBUG
Fixed
> > static ssize_t
> > store_fan_min(struct device *dev, const char *buf, size_t count, int
nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> >
> > val = simple_strtoul(buf, NULL, 10);
> > data->fan_min[nr - 1] =
> > FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr - 1]));
>
> Please use second tab instead of 4 spaces
Fixed
> > #define show_temp_reg(reg) \
> > static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
> > { \
> > struct w83792d_data *data = w83792d_update_device(dev); \
> > if (nr >= 2) { /* TEMP2 and TEMP3 */ \
> > return sprintf(buf,"%ld\n", \
> > (long)TEMP_FROM_REG(data->reg##_add[nr-2])); \
> > } else { /* TEMP1 */ \
> > return sprintf(buf,"%ld\n",
(long)TEMP_FROM_REG(data->reg)); \
> > } \
> > }
>
> Same question related here? Why temp1 separate?
Fixed, please refer to one reason above:
temp2 and temp3 need addtional resolution
> > show_temp_reg(temp);
> > show_temp_reg(temp_max);
> > show_temp_reg(temp_max_hyst);
> >
> > #define store_temp_reg(REG, reg) \
> > static ssize_t store_temp_##reg (struct device *dev, const char
*buf, size_t
> count, int nr) \
> > { \
> > struct i2c_client *client = to_i2c_client(dev); \
> > struct w83792d_data *data = i2c_get_clientdata(client); \
> > s32 val; \
> > \
> > val = simple_strtol(buf, NULL, 10); \
> > \
> > if (nr >= 2) { \
> > data->temp_##reg##_add[nr-2] = TEMP_TO_REG(val); \
> > w83792d_write_value(client,
W83792D_REG_TEMP_##REG[nr-1], \
> > data->temp_##reg##_add[nr-2]); \
> > } else { \
> > data->temp_##reg = TEMP_TO_REG(val); \
> > w83792d_write_value(client,
W83792D_REG_TEMP_##REG[nr-1], \
> > data->temp_##reg); \
> > } \
> > \
> > return count; \
> > }
>
> Same question related here? Why temp1 separate?
Same as above
> > store_fan_div_reg(struct device *dev, const char *buf, size_t count,
int nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > unsigned long min;
> > /*u8 reg;*/
>
> Please remove
>
> > u8 fan_div_reg=0; u8 tmp_fan_div;
>
> Please separate them on two lines.
Fixed
> > /* Save fan_min */
> > min = FAN_FROM_REG(data->fan_min[nr],
> > DIV_FROM_REG(data->fan_div[nr]));
> >
> > data->fan_div[nr] = DIV_TO_REG(simple_strtoul(buf, NULL, 10));
> >
> > fan_div_reg = w83792d_read_value(client,
W83792D_REG_FAN_DIV[nr/2]);
>
> nr/2 could be [nr >> 1]
>
> > fan_div_reg &= (nr%2 == 0) ? 0xf8 : 0x8f;
>
> and here
>
> fan_div_reg &= (nr & 1) ? 0x8f : 0xf8;
Fixed
> > tmp_fan_div = (nr%2 == 0) ? ((data->fan_div[nr])&0x07)
> > :
(((data->fan_div[nr])<<4)&0x70);
>
> This needs definetly spaces between operators but it can be also done
this way:
>
> tmp_fan_div = (nr & 1) ? ((data->fan_div[nr] << 4) & 0x70)
> : data->fan_div[nr] & 0x07;
Fixed
> > w83792d_write_value(client, W83792D_REG_FAN_DIV[nr/2],
>
> nr >> 1
Fixed
> > static ssize_t
> > show_pwmenable_reg(struct device *dev, char *buf, int nr)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > long pwm_enable_tmp = 1;
>
> Should be u32 instead?
>
> From Jean:
> u32 or long doesn't make much difference when you only need to store
> values from 0 to 2. Even an u8 would do.
> Note that this function could be made more readable/compact/efficient
by
> using either a switch/case construct, or a PWM_ENABLE_FROM_REG macro
or
> inline function, or even a lookup table.
Not fixed, original codes is kept.
> > cfg1_tmp = data->pwmenable[0];
> > cfg2_tmp = (data->pwmenable[1]) << 2;
> > cfg3_tmp = (data->pwmenable[2]) << 4;
> > cfg4_tmp = w83792d_read_value(client,W83792D_REG_FAN_CFG) &
0xc0;
> > fan_cfg_tmp = ((cfg4_tmp|cfg3_tmp)|cfg2_tmp)|cfg1_tmp;
>
> Missing spaces
> fan_cfg_tmp = ((cfg4_tmp | cfg3_tmp) | cfg2_tmp) | cfg1_tmp;
>
> From Jean:
> And useless parentheses ;)
Fixed, spaces have been added, but parentheses is kept because of my
own coding style :-)
> > static ssize_t
> > store_chassis_clear_reg(struct device *dev, const char *buf, size_t
count)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 temp1 = 0, temp2 = 0;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > data->chassis_clear = SENSORS_LIMIT(val, 0 ,1);
> > temp1 = ((data->chassis_clear) << 7) & 0x80;
> > temp2 = w83792d_read_value(client,
> > W83792D_REG_CHASSIS_CLR) & 0x7f;
> > w83792d_write_value(client, W83792D_REG_CHASSIS_CLR,
temp1|temp2);
>
> Missing spaces between |
Fixed
> > static ssize_t
> > store_thermal_cruise_reg(struct device *dev, const char *buf, size_t
count,
> int nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 target_tmp=0, target_mask=0;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > target_tmp = val;
> > target_tmp = target_tmp & 0x7f;
> > target_mask = w83792d_read_value(client,
W83792D_REG_THERMAL[nr-1]) &
> > 0x80;
> > data->thermal_cruise[nr-1] = SENSORS_LIMIT(target_tmp, 0, 255);
> > w83792d_write_value(client, W83792D_REG_THERMAL[nr-1],
> > (data->thermal_cruise[nr-1])|target_mask);
>
> Missing spaces between |
Fixed
> > static ssize_t
> > store_tolerance_reg(struct device *dev, const char *buf, size_t
count, int
> nr)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 tol_tmp, tol_mask;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > tol_mask = w83792d_read_value(client,
> > W83792D_REG_TOLERANCE[nr-1]) & ((nr==2)?0x0f:0xf0);
>
> Missing spaces: ((nr == 2) ? 0x0f : 0xf0);
Fixed
> > tol_tmp = SENSORS_LIMIT(val, 0, 15);
> > tol_tmp &= 0x0f;
> > data->tolerance[nr-1] = tol_tmp;
> > if (nr==2) {
> > tol_tmp <<= 4;
> > }
> > w83792d_write_value(client, W83792D_REG_TOLERANCE[nr-1],
> > tol_mask|tol_tmp);
>
> Missing spaces between |
Fixed
> > static ssize_t
> > show_sf2_level_reg(struct device *dev, char *buf, int nr, int index)
> > {
> > struct w83792d_data *data = w83792d_update_device(dev);
> > return sprintf(buf, "%ld\n",
> (long)(((data->sf2_levels[index-1][nr])*100)/15));
>
> Missing spaces
>
> From Jean:
> Also note that the (long) cast could (and should) be avoided.
>
Fixed
> > static ssize_t
> > store_sf2_level_reg(struct device *dev, const char *buf, size_t
count, int
> nr, int index)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83792d_data *data = i2c_get_clientdata(client);
> > u32 val;
> > u8 mask_tmp=0, level_tmp=0;
> >
> > val = simple_strtoul(buf, NULL, 10);
> >
> > data->sf2_levels[index-1][nr] =
> > SENSORS_LIMIT((val*15)/100, 0, 15);
>
> And here too it seems
Fixed
> > /* This function is called when:
> > * w83792d_driver is inserted (when this module is loaded), for
each
> > available adapter
> > * when a new adapter is inserted (and w83792d_driver is still
present)
> */
> > static int
> > w83792d_attach_adapter(struct i2c_adapter *adapter)
>
> I think static int and rest belong to one line
NOT Fixed, code is kept accroding Jean's comments.
> > {
> > if (!(adapter->class & I2C_CLASS_HWMON))
> > return 0;
> > return i2c_detect(adapter, &addr_data, w83792d_detect);
> > }
> >
> >
> > static int
> > w83792d_detect_subclients(struct i2c_adapter *adapter, int address,
int
> kind,
> > struct i2c_client *new_client)
>
> and here too
Not Fixed, same as above
> > if (force_subclients[0] == id && force_subclients[1] == address)
{
> > for (i = 2; i <= 3; i++) {
> > if (force_subclients[i] < 0x48 ||
> > force_subclients[i] > 0x4f) {
> > dev_err(&new_client->dev, "invalid
subclient "
> > "address %d; must be
0x48-0x4f\n",
> > force_subclients[i]);
> > err = -ENODEV;
> > goto ERROR_SC_2;
> > }
> > }
> > w83792d_write_value(new_client, W83792D_REG_I2C_SUBADDR,
> > (force_subclients[2] & 0x03) |
> > ((force_subclients[3] & 0x03)
<<4));
> > data->lm75[0]->addr = force_subclients[2];
> > data->lm75[1]->addr = force_subclients[3];
> > } else {
> > int val = w83792d_read_value(new_client,
W83792D_REG_I2C_SUBADDR);
>
> This should be u8 val =
Fixed
> > static int
> > w83792d_detect(struct i2c_adapter *adapter, int address, int kind)
> > {
>
> Same line?
NOT Fixed, same as above
> > /* Now, we do the remaining detection. */
> >
> > /* The w8378?d may be stuck in some other bank than bank 0. This
may
>
> Bad chip name in comment
Fixed
> > if (kind < 0) {
> > if (w83792d_read_value(new_client, W83792D_REG_CONFIG) &
0x80) {
> > dev_warn(&new_client->dev, "Detection failed at
step "
> > "3\n");
> > goto ERROR1;
> > }
> > val1 = w83792d_read_value(new_client, W83792D_REG_BANK);
> > val2 = w83792d_read_value(new_client,
W83792D_REG_CHIPMAN);
> > /* Check for Winbond ID if in bank 0 */
> > if (!(val1 & 0x07)) { /* is Bank0 */
> > if (((!(val1 & 0x80)) && (val2 != 0xa3)) ||
> > ((val1 & 0x80) && (val2 != 0x5c))) {
> > goto ERROR1;
> > }
> > }
> > /* If Winbond SMBus, check address at 0x48 */
>
> better comment would be:
> If Winbond chip, address of chip and W83792D_REG_I2C_ADDR should
match.
Fixed
> > /* We have either had a force parameter, or we have already
detected the
> > Winbond. Put it now into bank 0 and Vendor ID High Byte */
>
> > w83792d_write_value(new_client, W83792D_REG_BANK,
> > (w83792d_read_value(new_client,
> > W83792D_REG_BANK) &
0x78) |
> > 0x80);
> >
>
> Maybe better indent?
Fixed
> > static int
> > w83792d_detach_client(struct i2c_client *client)
>
> I think one line too.
Not Fixed, same as above
> > static int
> > w83792d_read_value(struct i2c_client *client, u8 reg)
>
> I think one line too.
Not Fixed, same as above
> > static int
> > w83792d_write_value(struct i2c_client *client, u8 reg, u8 value)
>
> I think one line too.
Not Fixed, same as above
> > /* Called when we have found a new W83792D. It should set limits,
etc. */
> > static void
> > w83792d_init_client(struct i2c_client *client)
> > {
> > int temp2_cfg, temp3_cfg, i=0;
>
> should not be u8 ? (except of i)
Fixed
> > #ifdef W83792D_DEBUG
> > w83792d_print_debug(data);
> > #endif
>
> Do not forget to change this too.
Fixed
> > return data;
> > }
> >
> > #ifdef W83792D_DEBUG
>
> and here :)
Fixed
> > MODULE_AUTHOR("Chunhao Huang @ Winbond");
>
> No email?
Fixed
Thanks
Best Regards
Chunhao
===========================================================================================The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Winbond is strictly prohibited; and any information in this email irrelevant 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.It essentially repeats the statement in English given 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??.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83792d.c
Type: application/octet-stream
Size: 54327 bytes
Desc: w83792d.c
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050531/c8f57679/attachment.obj
