diff for duplicates of <20100907020053.GB5332@ericsson.com> diff --git a/a/1.txt b/N1/1.txt index facb6dc..5f0b654 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -121,14 +121,14 @@ Ok. > > + && data->kind != max6695) > > + > > +#define lm90_have_local_temp_ext(data) \ -> > + (data->kind = max6657 || data->kind = max6646 \ -> > + || data->kind = max6695) +> > + (data->kind == max6657 || data->kind == max6646 \ +> > + || data->kind == max6695) > > + > > +#define lm90_have_remote_limit_ext(data) \ > > + (data->kind != max6657 && data->kind != max6646 \ > > + && data->kind != max6680 && data->kind != max6695) > > + -> > +#define lm90_have_emergency(data) (data->kind = max6695) +> > +#define lm90_have_emergency(data) (data->kind == max6695) > > Makes the code more readable, I agree, but OTOH it hides complexity. > Such tests are OK during probe or remove, as they happen only once, but @@ -242,7 +242,7 @@ Correct. > > else > > data->temp8[nr] = temp_to_s8(val); > > + -> > + if (data->kind = max6695 && nr >= 6) { +> > + if (data->kind == max6695 && nr >= 6) { > > + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config); > > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, > > + config | 0x08); @@ -250,7 +250,7 @@ Correct. > > + > > i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]); > > + -> > + if (data->kind = max6695 && nr >= 6) +> > + if (data->kind == max6695 && nr >= 6) > > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); > > + > > mutex_unlock(&data->update_lock); @@ -267,11 +267,11 @@ Correct. > > if (err < 0) > > @@ -462,19 +519,31 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > > mutex_lock(&data->update_lock); -> > if (data->kind = adt7461) +> > if (data->kind == adt7461) > > data->temp11[nr] = temp_to_u16_adt7461(data, val); -> > - else if (data->kind = max6657 || data->kind = max6680) +> > - else if (data->kind == max6657 || data->kind == max6680) > > - data->temp11[nr] = temp_to_s8(val) << 8; -> > else if (data->kind = max6646) +> > else if (data->kind == max6646) > > data->temp11[nr] = temp_to_u8(val) << 8; > > + else if (!lm90_have_remote_limit_ext(data)) > > + data->temp11[nr] = temp_to_s8(val) << 8; @@ -279,7 +279,7 @@ Correct. > > data->temp11[nr] = temp_to_s16(val); > > > > - i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2], -> > + if (data->kind = max6695 && nr >= 6) { +> > + if (data->kind == max6695 && nr >= 6) { > > + /* select output channel 2 */ > > + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config); > > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, @@ -303,7 +303,7 @@ how to improve it, though. I'll think about it. > > + i2c_smbus_write_byte_data(client, reg[(nr - offset) * 2 + 1], > > data->temp11[nr] & 0xff); > > + -> > + if (data->kind = max6695 && nr >= 6) +> > + if (data->kind == max6695 && nr >= 6) > > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, > > + config); > > + @@ -408,8 +408,8 @@ Ok, makes sense. Basic idea was that we read chip_id all the time even if the register doesn't exist, and react to it the same way. But you are right, it should only be read for maxim chips. -> > if ((address = 0x4C || address = 0x4D) -> > && man_id = 0x01) { /* National Semiconductor */ +> > if ((address == 0x4C || address == 0x4D) +> > && man_id == 0x01) { /* National Semiconductor */ > > int reg_config2; > > @@ -770,6 +900,22 @@ static int lm90_detect(struct i2c_client *new_client, > > name = "max6657"; @@ -433,8 +433,8 @@ Unfortunately, yes. I thought the read would return man_id, but it doesn't. > > + * one of those registers exists (and thus returns a value > > + * different to the previous reading). > > + */ -> > + if (chip_id = 0x01 -> > + && (reg_config1 & 0x10) = 0x00 +> > + if (chip_id == 0x01 +> > + && (reg_config1 & 0x10) == 0x00 > > + && reg_emerg != reg_convrate > > Note that there is a remote chance that both values are equal even @@ -495,7 +495,7 @@ Yes. > > + goto exit_remove_base; > > + } > > + -> > + if (data->kind = max6695) { +> > + if (data->kind == max6695) { > > Don't we want lm90_have_temp3(data) or similar for this? > @@ -526,7 +526,7 @@ Sure. > > return 0; > > > > exit_remove_files: -> > + if (data->kind = max6695) +> > + if (data->kind == max6695) > > + sysfs_remove_group(&new_client->dev.kobj, &lm90_temp3_group); > > +exit_remove_emergency: > > + if (lm90_have_emergency(data)) @@ -545,13 +545,13 @@ Ok. > > device_remove_file(&new_client->dev, &dev_attr_pec); > > exit_free: > > @@ -913,6 +1084,12 @@ static void lm90_init_client(struct i2c_client *client) -> > if (data->kind = max6680) +> > if (data->kind == max6680) > > config |= 0x18; > > > > + /* > > + * Select external channel 1 for max6695 > > + */ -> > + if (data->kind = max6695) +> > + if (data->kind == max6695) > > + config &= ~0x08; > > + > > config &= 0xBF; /* run */ @@ -563,7 +563,7 @@ Ok. > > hwmon_device_unregister(data->hwmon_dev); > > + if (lm90_have_emergency(data)) > > + sysfs_remove_group(&client->dev.kobj, &lm90_emergency_group); -> > + if (data->kind = max6695) +> > + if (data->kind == max6695) > > + sysfs_remove_group(&client->dev.kobj, &lm90_temp3_group); > > sysfs_remove_group(&client->dev.kobj, &lm90_group); > > device_remove_file(&client->dev, &dev_attr_pec); @@ -587,12 +587,12 @@ Makes sense. I'll check if it also makes sense to put that into a separate patch > > + u8 config, alarms, alarms2 = 0; > > > > lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); -> > - if ((alarms & 0x7f) = 0) { +> > - if ((alarms & 0x7f) == 0) { > > + -> > + if (data->kind = max6695) +> > + if (data->kind == max6695) > > + lm90_read_reg(client, LM90_REG_R_STATUS2, &alarms2); > > + -> > + if ((alarms & 0x7f) = 0 && (alarms2 & 0xfe) = 0) { +> > + if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) { > > dev_info(&client->dev, "Everything OK\n"); > > } else { > > if (alarms & 0x61) @@ -625,43 +625,43 @@ You are right. I'll move alarms2 into the max6695 path and keep it local there. > > lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]); > > lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst); > > -> > - if (data->kind = max6657 || data->kind = max6646) { +> > - if (data->kind == max6657 || data->kind == max6646) { > > + if (lm90_have_local_temp_ext(data)) { > > lm90_read16(client, LM90_REG_R_LOCAL_TEMP, > > MAX6657_REG_R_LOCAL_TEMPL, > > &data->temp11[4]); > > @@ -1033,29 +1223,63 @@ static struct lm90_data *lm90_update_device(struct device *dev) > > -> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) = 0) { +> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) { > > data->temp11[1] = h << 8; > > - if (data->kind != max6657 && data->kind != max6680 > > - && data->kind != max6646 > > + if (lm90_have_remote_limit_ext(data) > > && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL, -> > &l) = 0) +> > &l) == 0) > > data->temp11[1] |= l; > > } -> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) = 0) { +> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) { > > data->temp11[2] = h << 8; > > - if (data->kind != max6657 && data->kind != max6680 > > - && data->kind != max6646 > > + if (lm90_have_remote_limit_ext(data) > > && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL, -> > &l) = 0) +> > &l) == 0) > > data->temp11[2] |= l; > > } > > > > - if (data->kind != max6657 && data->kind != max6646) { > > + if (lm90_have_offset(data)) { > > if (lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSH, -> > &h) = 0 +> > &h) == 0 > > && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL, -> > &l) = 0) +> > &l) == 0) > > data->temp11[3] = (h << 8) | l; > > } > > - lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms); > > + -> > + if (data->kind = max6695) { +> > + if (data->kind == max6695) { > > + u8 config; > > + > > + lm90_read_reg(client, LM90_REG_R_LOCAL_EMERG, @@ -729,8 +729,3 @@ I'll see if I can do some more cleanup. Thanks a lot for the review! Guenter - -_______________________________________________ -lm-sensors mailing list -lm-sensors@lm-sensors.org -http://lists.lm-sensors.org/mailman/listinfo/lm-sensors diff --git a/a/content_digest b/N1/content_digest index 8fce251..cde9f90 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,8 +1,8 @@ "ref\01283639675-31714-1-git-send-email-guenter.roeck@ericsson.com\0" "ref\020100906181229.2e25db07@hyperion.delvare\0" "From\0Guenter Roeck <guenter.roeck@ericsson.com>\0" - "Subject\0Re: [lm-sensors] [PATCH] hwmon: Add support for max6695 and max6696\0" - "Date\0Tue, 07 Sep 2010 02:00:53 +0000\0" + "Subject\0Re: [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver\0" + "Date\0Mon, 6 Sep 2010 19:00:53 -0700\0" "To\0Jean Delvare <khali@linux-fr.org>\0" "Cc\0Andrew Morton <akpm@linux-foundation.org>" lm-sensors@lm-sensors.org <lm-sensors@lm-sensors.org> @@ -132,14 +132,14 @@ "> > + && data->kind != max6695)\n" "> > +\n" "> > +#define lm90_have_local_temp_ext(data) \\\n" - "> > + (data->kind = max6657 || data->kind = max6646 \\\n" - "> > + || data->kind = max6695)\n" + "> > + (data->kind == max6657 || data->kind == max6646 \\\n" + "> > + || data->kind == max6695)\n" "> > +\n" "> > +#define lm90_have_remote_limit_ext(data) \\\n" "> > + (data->kind != max6657 && data->kind != max6646 \\\n" "> > + && data->kind != max6680 && data->kind != max6695)\n" "> > +\n" - "> > +#define lm90_have_emergency(data) (data->kind = max6695)\n" + "> > +#define lm90_have_emergency(data) (data->kind == max6695)\n" "> \n" "> Makes the code more readable, I agree, but OTOH it hides complexity.\n" "> Such tests are OK during probe or remove, as they happen only once, but\n" @@ -253,7 +253,7 @@ "> > else\n" "> > data->temp8[nr] = temp_to_s8(val);\n" "> > +\n" - "> > + if (data->kind = max6695 && nr >= 6) {\n" + "> > + if (data->kind == max6695 && nr >= 6) {\n" "> > + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);\n" "> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,\n" "> > + config | 0x08);\n" @@ -261,7 +261,7 @@ "> > +\n" "> > i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);\n" "> > +\n" - "> > + if (data->kind = max6695 && nr >= 6)\n" + "> > + if (data->kind == max6695 && nr >= 6)\n" "> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);\n" "> > +\n" "> > mutex_unlock(&data->update_lock);\n" @@ -278,11 +278,11 @@ "> > if (err < 0)\n" "> > @@ -462,19 +519,31 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,\n" "> > mutex_lock(&data->update_lock);\n" - "> > if (data->kind = adt7461)\n" + "> > if (data->kind == adt7461)\n" "> > data->temp11[nr] = temp_to_u16_adt7461(data, val);\n" - "> > - else if (data->kind = max6657 || data->kind = max6680)\n" + "> > - else if (data->kind == max6657 || data->kind == max6680)\n" "> > - data->temp11[nr] = temp_to_s8(val) << 8;\n" - "> > else if (data->kind = max6646)\n" + "> > else if (data->kind == max6646)\n" "> > data->temp11[nr] = temp_to_u8(val) << 8;\n" "> > + else if (!lm90_have_remote_limit_ext(data))\n" "> > + data->temp11[nr] = temp_to_s8(val) << 8;\n" @@ -290,7 +290,7 @@ "> > data->temp11[nr] = temp_to_s16(val);\n" "> >\n" "> > - i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2],\n" - "> > + if (data->kind = max6695 && nr >= 6) {\n" + "> > + if (data->kind == max6695 && nr >= 6) {\n" "> > + /* select output channel 2 */\n" "> > + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);\n" "> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,\n" @@ -314,7 +314,7 @@ "> > + i2c_smbus_write_byte_data(client, reg[(nr - offset) * 2 + 1],\n" "> > data->temp11[nr] & 0xff);\n" "> > +\n" - "> > + if (data->kind = max6695 && nr >= 6)\n" + "> > + if (data->kind == max6695 && nr >= 6)\n" "> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,\n" "> > + config);\n" "> > +\n" @@ -419,8 +419,8 @@ "the register doesn't exist, and react to it the same way. But you are right,\n" "it should only be read for maxim chips.\n" "\n" - "> > if ((address = 0x4C || address = 0x4D)\n" - "> > && man_id = 0x01) { /* National Semiconductor */\n" + "> > if ((address == 0x4C || address == 0x4D)\n" + "> > && man_id == 0x01) { /* National Semiconductor */\n" "> > int reg_config2;\n" "> > @@ -770,6 +900,22 @@ static int lm90_detect(struct i2c_client *new_client,\n" "> > name = \"max6657\";\n" @@ -444,8 +444,8 @@ "> > + * one of those registers exists (and thus returns a value\n" "> > + * different to the previous reading).\n" "> > + */\n" - "> > + if (chip_id = 0x01\n" - "> > + && (reg_config1 & 0x10) = 0x00\n" + "> > + if (chip_id == 0x01\n" + "> > + && (reg_config1 & 0x10) == 0x00\n" "> > + && reg_emerg != reg_convrate\n" "> \n" "> Note that there is a remote chance that both values are equal even\n" @@ -506,7 +506,7 @@ "> > + goto exit_remove_base;\n" "> > + }\n" "> > +\n" - "> > + if (data->kind = max6695) {\n" + "> > + if (data->kind == max6695) {\n" "> \n" "> Don't we want lm90_have_temp3(data) or similar for this?\n" "> \n" @@ -537,7 +537,7 @@ "> > return 0;\n" "> >\n" "> > exit_remove_files:\n" - "> > + if (data->kind = max6695)\n" + "> > + if (data->kind == max6695)\n" "> > + sysfs_remove_group(&new_client->dev.kobj, &lm90_temp3_group);\n" "> > +exit_remove_emergency:\n" "> > + if (lm90_have_emergency(data))\n" @@ -556,13 +556,13 @@ "> > device_remove_file(&new_client->dev, &dev_attr_pec);\n" "> > exit_free:\n" "> > @@ -913,6 +1084,12 @@ static void lm90_init_client(struct i2c_client *client)\n" - "> > if (data->kind = max6680)\n" + "> > if (data->kind == max6680)\n" "> > config |= 0x18;\n" "> >\n" "> > + /*\n" "> > + * Select external channel 1 for max6695\n" "> > + */\n" - "> > + if (data->kind = max6695)\n" + "> > + if (data->kind == max6695)\n" "> > + config &= ~0x08;\n" "> > +\n" "> > config &= 0xBF; /* run */\n" @@ -574,7 +574,7 @@ "> > hwmon_device_unregister(data->hwmon_dev);\n" "> > + if (lm90_have_emergency(data))\n" "> > + sysfs_remove_group(&client->dev.kobj, &lm90_emergency_group);\n" - "> > + if (data->kind = max6695)\n" + "> > + if (data->kind == max6695)\n" "> > + sysfs_remove_group(&client->dev.kobj, &lm90_temp3_group);\n" "> > sysfs_remove_group(&client->dev.kobj, &lm90_group);\n" "> > device_remove_file(&client->dev, &dev_attr_pec);\n" @@ -598,12 +598,12 @@ "> > + u8 config, alarms, alarms2 = 0;\n" "> >\n" "> > lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);\n" - "> > - if ((alarms & 0x7f) = 0) {\n" + "> > - if ((alarms & 0x7f) == 0) {\n" "> > +\n" - "> > + if (data->kind = max6695)\n" + "> > + if (data->kind == max6695)\n" "> > + lm90_read_reg(client, LM90_REG_R_STATUS2, &alarms2);\n" "> > +\n" - "> > + if ((alarms & 0x7f) = 0 && (alarms2 & 0xfe) = 0) {\n" + "> > + if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {\n" "> > dev_info(&client->dev, \"Everything OK\\n\");\n" "> > } else {\n" "> > if (alarms & 0x61)\n" @@ -636,43 +636,43 @@ "> > lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);\n" "> > lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);\n" "> >\n" - "> > - if (data->kind = max6657 || data->kind = max6646) {\n" + "> > - if (data->kind == max6657 || data->kind == max6646) {\n" "> > + if (lm90_have_local_temp_ext(data)) {\n" "> > lm90_read16(client, LM90_REG_R_LOCAL_TEMP,\n" "> > MAX6657_REG_R_LOCAL_TEMPL,\n" "> > &data->temp11[4]);\n" "> > @@ -1033,29 +1223,63 @@ static struct lm90_data *lm90_update_device(struct device *dev)\n" "> >\n" - "> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) = 0) {\n" + "> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {\n" "> > data->temp11[1] = h << 8;\n" "> > - if (data->kind != max6657 && data->kind != max6680\n" "> > - && data->kind != max6646\n" "> > + if (lm90_have_remote_limit_ext(data)\n" "> > && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,\n" - "> > &l) = 0)\n" + "> > &l) == 0)\n" "> > data->temp11[1] |= l;\n" "> > }\n" - "> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) = 0) {\n" + "> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {\n" "> > data->temp11[2] = h << 8;\n" "> > - if (data->kind != max6657 && data->kind != max6680\n" "> > - && data->kind != max6646\n" "> > + if (lm90_have_remote_limit_ext(data)\n" "> > && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,\n" - "> > &l) = 0)\n" + "> > &l) == 0)\n" "> > data->temp11[2] |= l;\n" "> > }\n" "> >\n" "> > - if (data->kind != max6657 && data->kind != max6646) {\n" "> > + if (lm90_have_offset(data)) {\n" "> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSH,\n" - "> > &h) = 0\n" + "> > &h) == 0\n" "> > && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,\n" - "> > &l) = 0)\n" + "> > &l) == 0)\n" "> > data->temp11[3] = (h << 8) | l;\n" "> > }\n" "> > - lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms);\n" "> > +\n" - "> > + if (data->kind = max6695) {\n" + "> > + if (data->kind == max6695) {\n" "> > + u8 config;\n" "> > +\n" "> > + lm90_read_reg(client, LM90_REG_R_LOCAL_EMERG,\n" @@ -739,11 +739,6 @@ "\n" "Thanks a lot for the review!\n" "\n" - "Guenter\n" - "\n" - "_______________________________________________\n" - "lm-sensors mailing list\n" - "lm-sensors@lm-sensors.org\n" - http://lists.lm-sensors.org/mailman/listinfo/lm-sensors + Guenter -a5af20c19c12294fbbc395253001ed53f9c037f62104edfd9ca6ccdde028fd82 +6bda4da1afcea55f03ef2f632e07159fe0a6105a9f0a90db1f49eebde5bb1112
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.