Rudolf Marek wrote:
> Hello again,
>
> This is the first time after a week or so I have relatively free evening...
> I'm sorry for the delay, but again, too much stuff around here lately.
>
> I was speaking with Jean to become a co-maintainer so I could accept the patches
> too, but I'm bit worried if I know enough to do that...
>
> Please check my comments. Generally the -1 stuff should be fixed, the ----
> lines should be a bit shorter and ARRAY_SIZE macro can be used. And alarm
> stuff should use the mapping arrrays...
>
> I'm not sure if I like the "combo" callbacks, but I'm not strictly aginst it.
>
> Regards
> Rudolf
>
>
>
>> + /* temp registers */
>> + for (ix = 1; ix <= 7; ix++) {
>>
>
> And Jims proposal for the "get rid of -1" is a good one too. And of course on
> other places too.
>
>
Its cartainly your call, but a single comment where its 1st used (in the
SENSOR_ATTR_2 initialization)
will clear it up for everyone - and yourself after its actually coded
that way ;-)
Its faster too, though unmeasureably..
IOW, your code could become
/* the -1 in ix-1 adjusts the 1 based attr enumeration under /sys/*
to match 0-based arrays in C */
+#define SENSOR_ATTR_IN(ix) \
> + SENSOR_ATTR_2(in##ix##_input, S_IRUGO, \
> + show_in, NULL, SHOW_IN_INPUT, ix-1), \
> + SENSOR_ATTR_2(in##ix##_min, S_IRUGO | S_IWUSR, \
> + show_in, set_in, SHOW_SET_IN_MIN, ix-1), \
WRT combo callbacks,
> Jim wrote that he likes the idea, well I'm myself still not convinced.
>
>
Preferences aside, it saved me about 10% of the module's object code.
'It'
http://lists.lm-sensors.org/pipermail/lm-sensors/2006-January/015126.html
IE, it combined :
4 FAN_FNs into 1 show_fan() accessor
2 PWM_FNs,
4 IN_FNs into 1 show_in() accessor
2 IN_FNs in 1 set_in() mutator
5 THERM_FNs into 1 show_therm() accessor
5 TEMP_FNs into 1 show_temp()
5 TEMP_FNs into 1 set_temp()
IOW, lots of functions disappeared by use of combining.
One could reasonably expect 10% bloat if Juerg were to de-combine them.
> The classic 2D array grouping is more elegant in term of function, but bit
> strange on the hand of array mapping/dimension.
>
>
Yes, I too like 1D for array of identical sensors, 2nd D for FN-specifier.
Seems like what SENSOR_ATTR_2 was invented for.
But then, I already had the separate arrays of identical sensors. ;-)
IIRC, I asked Juerg about that, and he answered in a sense. (or maybe Im
remembering someone else :-/)
I didnt quite get the argument ( I think he gains other advantages by
his grouping strategy)
but I hadnt reviewed that diligently.
(to misquote a marvel-ous Spiderman saying: with no power comes no
responsibility :-D
BTW, theres also real attribute_group's, IIRC Jean wrote some code to
demonstrate its use,
but I cant find it now.
> This case is vice-versa data structures are nice function is bit more
> complicated. I'm not telling you to remove it, but please at least change
> the default case to error and nr to some better name.
>
Well, he did take the name from the sensor-attr-2 field,
but 'func' or 'funcidx' does seem better.
IIRC the nr name was chosen to deliberately not imply a specific use.
We're using it, we must know what for ;-)
thanks
jimc
