On Mon, Feb 28, 2011 at 12:50:45PM -0500, Lucas Stach wrote: > Hello all, > > let me revive this discussion a bit. After reading some things about the > matter I think the way to go is to use Intels thermal framework. It > should be easy to add the needed temp_get and fan_set functions to the > affected hwmon drivers. > > But then one problem still persists: when the hwmon driver probes a > device it automatically creates the sysfs entries. This is unintended > behaviour from nouveau's point of view. The sensor value is not the real > temperature value; it has to be scaled with some factor and an offset > hard-coded in the video bios tables. This scaling could only be done on > nouveau's side, so the hwmon driver is exposing an interface with wrong > values to the userspace. > > We need a way to use the hwmon i2c driver without exposing this > interface. Do you have any preferred way how we could achieve this? > The whole point of the hwmon subsystem is to expose hardware monitoring attributes to userspace. A hwmon driver without sysfs attributes does not make sense. Guenter > I see two ways to do this: > 1. Sort out if we need the sysfs entries in the probe function. This > could be done with some name postfix in the board_info. Downside: adds > some noise to this function. > > 2. Creating a new module which only exposes the thermal framework API. > This leads to code duplication and we end up with two drivers for the > same i2c monitoring chip. On the other hand this _could_ lead to cleaner > code and we could move this part to drivers/thermal instead of > drivers/hwmon. I don't like the code duplication, do you see any chance > to avoid this in case this is the way to go? > > Please comment on my thoughts, we need your feedback to bring up an API > everyone could agree on. I'm willing to hack together a prototype over > the next week, but want to avoid running in a completely wrong > direction. > > -- Lucas > > Am Sonntag, den 13.02.2011, 23:08 +0100 schrieb Jean Delvare: > > On Sun, 13 Feb 2011 09:16:40 -0800, Guenter Roeck wrote: > > > On Sun, Feb 13, 2011 at 07:18:44AM -0500, Martin Peres wrote: > > > > Hi, > > > > > > > > I am working on power management on the nouveau driver and I need a way > > > > to get data out of and send commands to the i2c drivers from the kernel > > > > space. > > > > Why? You already have a way to get data out of and send commands to the > > I2C devices themselves (Using the i2c_smbus_* functions). Why do you > > insist on going through the I2C device drivers? > > > > > > We can already change the clocks of the card, but we need a way to > > > > How is this relevant to the discussion? Are the clock chips connected > > to the I2C bus too? > > > > > > monitor the temperature and bump the fan speed if needed. > > > > Hardware is very badly designed if the driver actually has to take care > > of this. Thermal management should be handled by the hardware directly. > > And as a matter of fact, the Analog Devices ADT7473 and the Fintek > > F75375S support this. > > > > > > Another problem with letting users mess with the i2c driver by > > > > themselves is that some cards use the i2c driver for fan management > > > > while others don't. This is why I would like to introduce nouveau as an > > > > I guess you mean I2C device, not i2c driver. You will have to be > > precise in your wording if you want others to understand where you are > > going. > > > > When the I2C device isn't used for fan management, how is it done? > > > > > > hwmon driver, exporting the temperature, fan management and clock speeds > > > > so as we can use the thermal zone to monitor the temperature and react > > > > when needed. > > > > It only makes sense to instantiate a hwmon device from nouveau directly > > if the temperature sensor or the fan management is _not_ done by an I2C > > device. So it seems unrelated with your patch. Why are you mentioning > > it then? > > > > And clock speeds don't have anything to do with hwmon, BTW. > > > > > > So far, we use: > > > > - w83l785ts > > > > - w83781d > > > > - adt7473 (most common one) > > > > - f75375 > > > > - lm99 > > > > > > > > With the help of Matthew Garret, I updated his previous proposal for an > > > > in-kernel API for hwmon. The patch should apply cleanly on Linux > > > > I can't remember this proposal. A link would be appreciated. > > > > > > 2.6.38-rc4. This patch only provides the API, no modification to the > > > > drivers has been completed yet. > > > > Do you mean that you don't have the code at all yet, or that you did > > not include it in this patch set? Either way, this is wrong. There is > > no point in asking for a review of only half of your solution. We can't > > comment on the relevance of your proposal without seeing how you intend > > to use it. > > > > > > Looking forward to your review and feedback. > > > > I have no plan to review your patch, sorry. You did not provide a > > proper description of your problem, and you didn't explain why it can't > > be solved with the current kernel infrastructures. Worse, you propose a > > brand new hwmon subsystem, but you don't even provide a description of > > its design, let alone an explanation of why you think this design is > > appropriate. And you don't show us code using it either. > > > > All I can say after a quick look at the patch, is that you are > > overengineering a lot. You have enumerated all sensor properties which > > exist, and are trying to handle all sensor types. You have a specific > > need (thermal management of graphics cards), but you are already trying > > to provide a generic access to all hwmon attributes which exist, > > regardless of the needs or relevance. Do you really think you'll need > > information about the case intrusion status in nouveau? Seriously? > > > > > > From 059b647b7b8bd98c04cf48b4062048b8ae963593 Mon Sep 17 00:00:00 2001 > > > > From: Martin Peres <martin.peres@xxxxxxxxxxxxxxx> > > > > Date: Sun, 13 Feb 2011 11:35:17 +0100 > > > > Subject: [PATCH] hwmon API update > > > > > > > > Original creator: Matthew Garrett <mjg@xxxxxxxxxx> > > > > > > > > Signed-off-by: Martin Peres <martin.peres@xxxxxxxxxxxxxxx> > > > > > > This is an extremely complex change just for the benefit of one driver, > > > with a huge potential of misuse. The changes required in each driver > > > to actually implement the API are substantial, and pretty much only add > > > complexity to each hwmon driver with no real benefit. > > > > I would be very curious to know how comes that the radeon driver > > apparently works just fine without this change, if the nouveau driver > > can't do without it. > > > > My main concern is that the code you will have to add to every new > > hwmon driver you will want nouveau to be able to use, is likely to be > > larger than the code needed to access the device registers directly > > from nouveau. Getting a temperature value from a hwmon device is > > typically done with a single call to i2c_smbus_read_byte_data(). > > > > > The cost gets even larger if one has to consider that some may want or > > > have to to backport drivers to earlier kernel versions. This patchset > > > would result in significant efforts to do such backports. > > > > This will never be a good reason to reject a change, sorry. Just look > > at the many changes the i2c subsystem went through in the past 2 years. > > They make it difficult to backport i2c device drivers to older kernels, > > but they still happened, because they were needed. When backporting a > > driver, you have to deal with the history of the kernel at large, > > that's life. > > > > > For the API itself, there are lots of functions with similar parameters, > > > and those parameters are needed in the drivers to determine which attribute > > > is affected. A single function would have accomplished the same, as the drivers > > > will need case statements anyway to identify the actual attribute to be read > > > or written. What we end up here with is a large number of functions to be > > > supported by each driver, all with pretty much the same set of arguments. > > > > This is the kind of thing which would show up immediately if we could > > see a few actual implementations of the hwmon driver side of the API. > > In general, I would tend to agree with Guenter that exporting a dozen > > functions from the hwmon core driver seems just wrong, especially given > > the specific problem you claim you are trying to solve. > > > > > I don't know what current thinking is about kernel size increases, but it > > > looks like this patch will result in quite significant kernel size increase > > > (some 18*8 = 144 bytes per driver for all the pointers, plus the actual > > > functions, adds up to a lot). Again this would be with no benefit for most > > > of the users of the hwmon subsystem. Sure, one can argue that the size increases > > > will only occur if the drivers are actually loaded, but that is a pretty weak > > > argument since the code size increase will still show up in each driver. > > > > > > In summary I am not in favor for this change. Maybe Jean thinks differently, > > > but for my part I don't plan to approve it. > > > > I don't plan to approve it either, at least not in its current state. > > As I said above already, I want a complete description of the problem > > first, an explanation of why the change is needed, why a more > > lightweight solution wouldn't do, and why nouveau needs it when radeon > > doesn't. And I want to see actual implementations of the API on both > > sides. > > > > Sorry but you can't push for a new API affecting 100 drivers without > > justifying everything you do. An API with no implementers and no users > > is not how you'll convince me. > > > > _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
