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. -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
