Re: [PATCH] hwmon: Add sch5127 support to dme1737

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Kernel]     [Linux Hardware Monitoring]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux