Hi Jim, > Im hoping (against experience) that by doing these: > > - identify drivers using macro repeated function defns > - show how to find the lines to look at (the above grep) > - discuss the technique ( 'dynamic' doesnt explain it, at least for me) Agreed, a better name would probably have been "parametrized" or similar, but for some reason we came up with "dynamic" originally. > .. that I/we can lower the barrier to participation. > The task is still somewhat harder than the average janitorial patch, > (which may attract would-be hackers), and of course there are the > testing / validation issues (must have hardware). Sure, that'd be nice. Maybe you can really make it a kernel janitors project? You might be more successful that way than by assuming that people on the lm-sensors list will pick your plan and implement it. As you said, experience suggests this won't happen, as much as we'd both like it to change. At any rate, this should only happen after we're done with the unchecked return status changes, and preferably after the individual alarm files are added, because these two changes have higher priority. > <aside> Testing by personal invitation has worked for me, at least once. > Yuan Mu from Winbond took a compile-tested patch from me, and finished it. > I aint too proud to beg ;-) .. (cues up some Rolling Stones ..) Yep, sometimes it works, and that was nice. > > As I mentioned earlier during the vt1211 review, I'm no big fan of this > > approach. You end up aggregating many features which don't share much > > code. It's still reasonable in your example above, but I think it has > > gone to an extreme in one case in the vt1211 driver. Let's not jump > > from one extreme to the other. > > As with everything else, its a judgement call - Juerg used it for > show_in, set_in, show_temp, set_temp, show_fan, set_fan, show|set_pwm > He did not for many others. Well the list above covers almost all the callbacks, only missing are the automatic fan speed regulation and misc ones. > In most places he did so, the combo-callback > was still ~30 lines, a pretty good indicator of clarity. True, the only one I think was not so nice was set_pwm, as it reaches 90 lines. > Also, the 'assignment' of callbacks (by SENSOR_ATTR*) is hugely flexible; > one could use a combo-callback to handle show-ops, but stick with > dedicated callbacks > for store-ops. This is perhaps subtle at 1st, but a 2 line comment > could address that. I agree, that makes sense, as the show-ops are usually very simple, and the store-ops usually more complex. > > I do agree that having a single callback for displaying fanN_input and > > fanN_min makes full sense, and this suggests a second level of > > indexing. But beyond that point I think we have to be reasonable. I > > don't mind for new drivers, authors should stay relatively free, > > however I strongly object to converting all our perfectly working > > drivers to use that approach. > > Uhm .. "strongly object" ? > Is it your intent to discourage patches of this nature ? Yes. > (and by inference, other minor improvements ?) No. The change proposed above is not a "minor improvement" by any mean, it is a complete rewrite of the callback functions, for no technical benefit that I can see. To summarize my idea, I do encourage people to get rid of all the ugly macros the older hardware monitoring drivers have, assuming they have the hardware to test their change (or can find someone who does). I do encourage them to use 1-index "dynamic" callbacks because they are straightforward and do only require minimum changes. Going beyong that requires larger changes, design decisions, and is more risky; not something for newcomers. > I know you've got bigger fish to fry ( conversion to platform, etc - > todo list forthcoming ? ;-) Indeed. i2c core conversion to the driver model, unchecked return values, i2c-isa/platform conversion, individual alarm files, all these are big and have higher priority. People could help with some of these, by the way. > and that review of combo-callback conversions is a non-zero cost upon you, > but surely you want to give objective incentives for folks to get involved. If people really want to get involved, they will. If they are looking for easy little changes to start with and learn how the kernel development works, there are the kernel janitors and kernel mentors projects for that, and I don't plan to duplicate their effort. I couldn't afford it anyway. > For my part, your acceptance of my previous work is a big factor in my > hanging around here, and Id hope you'd regard those reviews as a > successful investment, Sure I do, and I know that my reviews might sound harsh sometimes and might discourage the newcomers. I try to explain my reasons everytime so that it doesn't sound too arbitrary or personal. But well, we want high quality code in the kernel, and we want code that follows guidelines, some written, some unfortunately not. I am also aware that my reviews are often delayed beyond what contributors would like (see the vt1211 driver for an extreme example), but my time and energy is limited. The only two ways around it are: * I get paid for the reviews. Now that I am a Suse employee, it is partly true, but my main task there is L3 support, not kernel development, so I only have a few hours every month that I can use for kernel development. This month I used this time for the vt1211 review. * Others help me with the reviews. Ideally both would happen. > beyond the X kB trimmed from 1 driver. Even half-baked reviews (such as > the ones Ive offered) can result in brush-clearing (something our > president is good at ;-) Absolutely. I encourage everyone here to review other's patches. You don't have to be me to do it, and every contribution in that area can help lower my workload, which I think can only help the project. > > I have been thinking of a "standard hwmon attribute" which would > > combine our current sensor and sensor_2 attributes. I'll think again > > about it after we are done with the unchecked return values. > > interesting. I played briefly with enums, but couldnt figure out how to > do it as a specialization/derivation of struct sensor_device_attribute. > maybe anonymous unions have a place here.. enums don't have anything to do here, unions could. But my original intent was just one simple structure which would fit all needs. The idea came to me as I realized that the struct sensor_device_attribute_2 is _smaller_ than the struct sensor_device_attribute. -- Jean Delvare
