On Wed, Feb 01, 2012 at 03:01:38AM -0500, Jean Delvare wrote: > On Tue, 31 Jan 2012 15:31:17 -0800, Guenter Roeck wrote: > > On Tue, 2012-01-31 at 16:38 -0500, Jean Delvare wrote: > > > I don't see the point of preserving the API. We usually don't do that. > > > The API was wrong, you're fixing it, let's adjust all drivers > > > accordingly and be done with it. > > > > I started looking into this. > > > > Problem is that some callers pass a fixed vrm value to vid_from_reg(). > > See lm87.c or lm93.c. Can we safely ignore this, or do we have to retain > > the parameter and use it if provided (ie if it is != 0) ? > > I don't see that in lm87.c, I presume you meant lm78.c. For lm78 this > is clearly a bug, the driver was written before multiple VRM versions > existed, and was never adapter to support other VRM versions. So you > can ignore it. > > For lm93 the hard-coded value is because the chip support relative > Vcore voltage limits (which adjust themselves as the VID code changes) > and that can only work if the chip itself knows how to interpret the > codes. As the LM93 only knows of VRM 10.x, This strongly suggested that > the chip will never be used in combinations with other CPUs, thus the > hard-coded VRM. I don't think it was so smart though, as the LM93 could > still be used with other CPUs, simply the dynamic Vcore limit feature > can't be used. Also note that the driver now supports the LM94 which > _does_ support VRD 10 and VRD 11, so the hard-coded value is really > incorrect. > > In general, if a driver wants to only support a specific VRM value, it > should now call vid_witch_vrm() and fail loading or limit its features > if the value isn't what it wanted. > Ok, makes sense. > > On the plus side, if we retain the parameter and use it to override the > > configured/calculated value, we can retain the API, and I can submit a > > series of patches to remove vrm usage from the various drivers, instead > > of a single large patch which does it all in one go. So this would have > > some benefits. > > I think we'll end up removing the extra parameter. But if you want to > keep it for the time being because you feel more comfortable that way > or you believe it offers a smoother update path, that's your call. > > > > > +} > > > > EXPORT_SYMBOL(vid_which_vrm); > > > > > > There should be no user left for this function if you don't attempt to > > > preserve the API. > > > > Actually, turns out there is at least one user - the value is used in > > w83627ehf.c to configure the chip. Not sure how to do that if the driver > > can not retrieve the vrm value. > > Good point, vid_which_vrm must stay. > > > Then there is vid_to_reg(), which is currently an inline function. Guess > > I'll have to move that to hwmon-vid.c and export it. > > Correct. > Another key question: Can I remove the vrm attribute entirely from all drivers, and/or can I even make it read-only, without going through the feature removal process ? It is documented in the ABI, and libsensors supports it. Maybe I should only apply the patch to hwmon-vid.c now, and schedule vrm removal for something like 2013 ? Thanks, Guenter _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
