Hi Juerg, On Fri, 9 Apr 2010 10:20:40 -0700, Juerg Haefliger wrote: > Sorry for the slow response. The past few months have been quite hectic.. I can understand that... > >> @@ -2329,10 +2431,10 @@ > >> dme1737_sio_enter(sio_cip); > >> > >> /* Check device ID > >> - * We currently know about SCH3112 (0x7c), SCH3114 (0x7d), and > >> - * SCH3116 (0x7f). */ > >> + * We currently know about SCH3112, SCH3114, SCH3116, and SCH5127 */ > >> reg = force_id ? force_id : dme1737_sio_inb(sio_cip, 0x20); > >> - if (!(reg == 0x7c || reg == 0x7d || reg == 0x7f)) { > >> + if (!(reg == SCH3112_ID || reg == SCH3114_ID || reg == SCH3116_ID || > >> + reg == SCH5127_ID)) { > >> err = -ENODEV; > >> goto exit; > >> } > >> @@ -2428,19 +2530,31 @@ > >> company = dme1737_read(data, DME1737_REG_COMPANY); > >> device = dme1737_read(data, DME1737_REG_DEVICE); > >> > >> - if (!((company == DME1737_COMPANY_SMSC) && > >> - (device == SCH311X_DEVICE))) { > >> + if ((company == DME1737_COMPANY_SMSC) && > >> + (device == SCH311X_DEVICE)) { > >> + force_id = SCH3112_ID; > >> + } else if ((company == DME1737_COMPANY_SMSC) && > >> + (device == SCH5127_DEVICE)) { > >> + force_id = SCH5127_ID; > >> + } else { > > > > I'm not fond of abusing force_id that way. I admit that it should work > > in practice, however it could break in theory, for example if more than > > one chip were present on a given system. I don't expect to see this any > > time soon, but this shows the danger of writing to a global variable > > that way. I'd rather do force_id ==> data->type ==> data->name, this is > > cleaner and shouldn't require much mode code. > > I don't understand your proposal. Could you elaborate? I propose that you don't change the value of force_id in the driver. This value can be set by the user and should be considered read-only by the driver itself. The resulting code might be slightly larger, but it is also more correct: /* Skip chip detection if module is loaded with force_id parameter */ switch (force_id) { case SCH5127_ID: data->type = sch5127; break; case SCH3112_ID: data->type = sch311x; break; default: company = dme1737_read(data, DME1737_REG_COMPANY); device = dme1737_read(data, DME1737_REG_DEVICE); if ((company == DME1737_COMPANY_SMSC) && (device == SCH311X_DEVICE)) { data->type = sch5127; } else if ((company == DME1737_COMPANY_SMSC) && (device == SCH5127_DEVICE)) { data->type = sch311x; } else { err = -ENODEV; goto exit_kfree; } } data->name = data->type == sch5127 ? "sch5127" : "sch311x"; > >> err = -ENODEV; > >> goto exit_kfree; > >> } > >> } > >> - data->type = sch311x; > >> > >> - /* Fill in the remaining client fields and initialize the mutex */ > >> - data->name = "sch311x"; > >> + if (force_id == SCH5127_ID) { > >> + data->type = sch5127; > >> + data->name = "sch5127"; > >> + } else { > >> + data->type = sch311x; > >> + data->name = "sch311x"; > >> + } > > > > This assumes that force_id MUST be either SCH5127_ID or any of > > SCH311x_ID. This is true, but this is enforced in a different function > > (dme1737_isa_detect), so it's hard to guarantee that it will always be > > the case in the future. > > Is this just a comment or do you want me to make changes? What would > be your suggestion? This is related to the issue above, so the problem will go away if you follow my advice above. -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors