* [lm-sensors] hwmon-sensor-attr-array-2.patch
2005-12-28 21:43 [lm-sensors] hwmon-sensor-attr-array-2.patch Jim Cromie
@ 2006-01-02 10:01 ` Jean Delvare
2006-01-02 10:51 ` Hans de Goede
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2006-01-02 10:01 UTC (permalink / raw)
To: lm-sensors
Hi Jim,
Jim Cromie wrote:
> This patch refactors SENSOR_DEVICE_ATTR_2 macro, following pattern set by
> SENSOR_ATTR. First it creates a new macro SENSOR_ATTR_2() which expands
> to an initialization expression, then it uses that in SENSOR_DEVICE_ATTR_2,
> which declares and initializes a struct sensor_device_attribute_2.
Looks OK to me, consider it applied. I won't push it until I am also
able to push a driver which uses it though (but that should happen soon
enough.)
We should probably think of this part of the code and see what can be
done to improve it. struct sensor_device_attribute is currently
*larger* than struct sensor_device_attribute_2, which makes one wonder
if we shouldn't use sensor_device_attribute_2 everywhere even when we
don't actually need the second parameter. Also, the field names "index"
and "nr" are too generic IMHO and this might get confusing in the long
run. I was thinking of "channel" instead of "index". No idea for "nr"
though. Or maybe we are generic on purpose for reusability, but then
"index" and "nr" should be "nr0" and "nr1" or something equally
symmetrical. Thoughts?
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread* [lm-sensors] hwmon-sensor-attr-array-2.patch
2005-12-28 21:43 [lm-sensors] hwmon-sensor-attr-array-2.patch Jim Cromie
2006-01-02 10:01 ` Jean Delvare
@ 2006-01-02 10:51 ` Hans de Goede
2006-01-02 19:20 ` Jim Cromie
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2006-01-02 10:51 UTC (permalink / raw)
To: lm-sensors
Jean Delvare wrote:
> Hi Jim,
>
> Jim Cromie wrote:
>> This patch refactors SENSOR_DEVICE_ATTR_2 macro, following pattern set by
>> SENSOR_ATTR. First it creates a new macro SENSOR_ATTR_2() which expands
>> to an initialization expression, then it uses that in SENSOR_DEVICE_ATTR_2,
>> which declares and initializes a struct sensor_device_attribute_2.
>
> Looks OK to me, consider it applied. I won't push it until I am also
> able to push a driver which uses it though (but that should happen soon
> enough.)
>
> We should probably think of this part of the code and see what can be
> done to improve it. struct sensor_device_attribute is currently
> *larger* than struct sensor_device_attribute_2, which makes one wonder
> if we shouldn't use sensor_device_attribute_2 everywhere even when we
> don't actually need the second parameter. Also, the field names "index"
> and "nr" are too generic IMHO and this might get confusing in the long
> run. I was thinking of "channel" instead of "index". No idea for "nr"
> though. Or maybe we are generic on purpose for reusability, but then
> "index" and "nr" should be "nr0" and "nr1" or something equally
> symmetrical. Thoughts?
>
> Thanks,
I use the 2 fields in a number of different ways (table use 8-width tabs):
nr selects index selects
values NA sensor
pwm_auto/min/ma min/max (offset) sensor/pwm
xxx_mask_xxx beep/shutdown/alarm (bitmask) sensor_type (volt/temp)*
*) NA for bank2 as bank2 only contains fan sensors
I've attached a new version of the uGuru driver which adds the above
table as a comment for future reference.
Anyways considering the wide number of uses I've found for the 2 params
in 1 driver I think we should keep the names as generic as possible,
maybe index and param?, which would suggest swapping the order of the 2
in the MACRO arg lists.
Regards,
Hans
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: abituguru.c.txt
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060102/29f17edb/abituguru.c-0001.txt
^ permalink raw reply [flat|nested] 6+ messages in thread* [lm-sensors] hwmon-sensor-attr-array-2.patch
2005-12-28 21:43 [lm-sensors] hwmon-sensor-attr-array-2.patch Jim Cromie
2006-01-02 10:01 ` Jean Delvare
2006-01-02 10:51 ` Hans de Goede
@ 2006-01-02 19:20 ` Jim Cromie
2006-01-05 4:52 ` Jim Cromie
2006-01-05 8:12 ` Hans de Goede
4 siblings, 0 replies; 6+ messages in thread
From: Jim Cromie @ 2006-01-02 19:20 UTC (permalink / raw)
To: lm-sensors
Hans de Goede wrote:
>
>
> Jean Delvare wrote:
>
>> Hi Jim,
>>
>> Jim Cromie wrote:
>>
>>> This patch refactors SENSOR_DEVICE_ATTR_2 macro, following pattern
>>> set by
>>> SENSOR_ATTR. First it creates a new macro SENSOR_ATTR_2() which
>>> expands to an initialization expression, then it uses that in
>>> SENSOR_DEVICE_ATTR_2, which declares and initializes a struct
>>> sensor_device_attribute_2.
>>
>>
>> Looks OK to me, consider it applied. I won't push it until I am also
>> able to push a driver which uses it though (but that should happen soon
>> enough.)
>>
>> We should probably think of this part of the code and see what can be
>> done to improve it. struct sensor_device_attribute is currently
>> *larger* than struct sensor_device_attribute_2, which makes one wonder
>> if we shouldn't use sensor_device_attribute_2 everywhere even when we
>> don't actually need the second parameter.
>
I cant help but suspect that alignment issues will eat the space savings,
particularly at non-word boundaries. But perhaps not on some archs (x86).
>> Also, the field names "index"
>> and "nr" are too generic IMHO and this might get confusing in the long
>> run. I was thinking of "channel" instead of "index". No idea for "nr"
>> though. Or maybe we are generic on purpose for reusability, but then
>> "index" and "nr" should be "nr0" and "nr1" or something equally
>> symmetrical. Thoughts?
>>
Im thinking of a union, something like the following,
struct sensor_device_attribute_2 {
struct device_attribute dev_attr;
union {
u8 info[4];
struct {
u8 index, nr;
} u;
};
but I cant declare the union without giving the element a name 'u',
so derefs to its members would have to change, defeating the point of it.
But maybe Im missing something.
And perhaps a one-time churn isnt too far OOB,
though Im not sufficiently fire-proof to propose it myself ;-)
I agree that the names and (particularly) order are unfortunate.
Normally, when an interface extends another (ie SENSOR_DEVICE_ATTR_2) ,
new params are added to the end of the sig, not inserted in the middle.
(In contrast, .nr was added to end of the struct).
Lastly, it sounds like there may be opporunity to coordinate
any churn of the field names with plans inimated here:
http://lwn.net/Articles/161236/
in Future Driver core changes section.
Hans, I noticed a few doc-nits. (havent read for anything else)
>/* Const names for use in the dynamicly genererated in and temp sysfs attr,
> 16 is a bit over kill since there are know known mainboards with 16 in
>
>
s/know known/no known/
s/over kill/overkill/
<aside>
I thank the Germans (really) for the English willingness to create new
words out of
smaller ones. Why say 'pomme de terre' when you can say kartoffel ?
With apologies to the nation of France :-D
jimc
^ permalink raw reply [flat|nested] 6+ messages in thread* [lm-sensors] hwmon-sensor-attr-array-2.patch
2005-12-28 21:43 [lm-sensors] hwmon-sensor-attr-array-2.patch Jim Cromie
` (2 preceding siblings ...)
2006-01-02 19:20 ` Jim Cromie
@ 2006-01-05 4:52 ` Jim Cromie
2006-01-05 8:12 ` Hans de Goede
4 siblings, 0 replies; 6+ messages in thread
From: Jim Cromie @ 2006-01-05 4:52 UTC (permalink / raw)
To: lm-sensors
Hans de Goede wrote:
what the hey, Ill give it a cursory look.
dont feel the need to ack whatever I say here (I havent started yet),
unless you want to disagree.
I assume Jean will eventually get to it, but perhaps I can save him a
few words.
Hopefully Im not so far off base that he spends them correcting me ;-)
1. strip trailing whitespace :-O
Im just saying it preemptively.
strip macro eventually.
>/* This is needed untill this gets merged upstream */
>#ifndef SENSOR_ATTR_2
>#define SENSOR_ATTR_2(_name, _mode, _show, _store, _nr, _index) \
> { .dev_attr = __ATTR(_name, _mode, _show, _store), \
> .index = _index, \
> .nr = _nr }
>#endif
>
>
>
>/* Debugging output level:
> 0 no debug output
> 1 log errors with printk
> 2 also log sensors type probing info
> 3 log retryable errors
> This is at 2 for now, since this driver is still in the testing fase */
>#define ABIT_UGURU_DEBUG_LEVEL 2
>
>
make this a module-param ?
how much object code does it add ?
maybe a param, which is also gated by ifdefs
(or a debug_() macro to hide ifdefs) ?
see dev_dbg, for a start, though I think youll want to
wrap it so that you can pass in a level 1,2,3,
while keeping 1 ifdef to remove the debug-code entirely.
$ grep dev_dbg pc87360.c
dev_dbg(&new_client->dev, "Using %s reference voltage\n",
dev_dbg(&client->dev, "Forcibly "
dev_dbg(&client->dev, "Forcibly "
dev_dbg(&client->dev, "Skipping "
dev_dbg(&client->dev, "Forcibly "
>/* For the Abit uGuru, we need to keep some data in memory.
> The structure is dynamically allocated, at the same time when a new
> abituguru client is allocated. */
>
>
do you foresee / have you tried multiple chips ?
>/* Put the uguru in ready for input state, this code assumes that
> the uguru is not already in this state.
> It is the callers responsibility to make sure it isn't! */
>
>
what do you save by making this assumption ?
yes its static, so less ripe for abuse, but if its 1 flag ..
> /* The first try the uguru normally will be ready for the first
> input and thus in ABIT_UGURU_STATUS_INPUT state. If however
> something went wrong previously it might not be, so then we
> try to force it into ready state.
>
>
is this forcing something the user should have to explicitly enable ?
>/* Following are the sysfs callback functions. These functions use
> sensor_device_attribute_2->nr and sensor_device_attribute_2->index
> in the following ways:
> nr selects index selects
>_value/pwm_* NA sensor/pwm *
>_setting auto/min/max (offset) sensor/pwm
>_alarms/_mask beep/shutdown/flag (bitmask) sensor_type (volt/temp) **
>
> *) Except for pwm_setting which uses nr and index as _setting
> **) NA for bank2 as bank2 only contains fan sensors */
>
>
following are nearly identical
>static ssize_t show_bank1_value(struct device *dev,
> struct device_attribute *devattr, char *buf)
>
>static ssize_t show_bank1_setting(struct device *dev,
> struct device_attribute *devattr, char *buf)
>
>static ssize_t show_bank2_value(struct device *dev,
> struct device_attribute *devattr, char *buf)
>
>
>
+ others below I think.
it looks like attr->index, and bank 1,2 could drive a constants & formula
lookup to provide many calcs reachable by 1/few handers.
Im beginning to think you could make good use of SENSOR_ATTR_4,
which Jean blue-sky'd, but you could make concrete easily.
with it, you could squeeze these many handlers down to a few,
doing scale and offset lookups and calcs, forex.
All right, Im fresh out of insights
hth
jimc
PS. what boxes have this chip in them ?
/me thinks it might be in comments.
/me should just look myself, instead of asking :-O
^ permalink raw reply [flat|nested] 6+ messages in thread* [lm-sensors] hwmon-sensor-attr-array-2.patch
2005-12-28 21:43 [lm-sensors] hwmon-sensor-attr-array-2.patch Jim Cromie
` (3 preceding siblings ...)
2006-01-05 4:52 ` Jim Cromie
@ 2006-01-05 8:12 ` Hans de Goede
4 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2006-01-05 8:12 UTC (permalink / raw)
To: lm-sensors
Jim Cromie wrote:
> Hans de Goede wrote:
>
>
> what the hey, Ill give it a cursory look.
>
Thanks
> dont feel the need to ack whatever I say here (I havent started yet),
> unless you want to disagree.
>
> I assume Jean will eventually get to it, but perhaps I can save him a
> few words.
> Hopefully Im not so far off base that he spends them correcting me ;-)
>
>
> 1. strip trailing whitespace :-O
> Im just saying it preemptively.
>
There shouldn't be any, I myself dislike it too.
>
>
>
> strip macro eventually.
>
>> /* This is needed untill this gets merged upstream */
>> #ifndef SENSOR_ATTR_2
>> #define SENSOR_ATTR_2(_name, _mode, _show, _store, _nr, _index) \
>> { .dev_attr = __ATTR(_name, _mode, _show, _store), \
>> .index = _index, \
>> .nr = _nr }
>> #endif
>>
>>
>>
>
Once a mainstream kernel is released with this macro I will.
>> /* Debugging output level:
>> 0 no debug output
>> 1 log errors with printk
>> 2 also log sensors type probing info
>> 3 log retryable errors
>> This is at 2 for now, since this driver is still in the testing fase */
>> #define ABIT_UGURU_DEBUG_LEVEL 2
>>
>>
> make this a module-param ?
> how much object code does it add ?
> maybe a param, which is also gated by ifdefs
> (or a debug_() macro to hide ifdefs) ?
>
A macro to hide all the ifdefs is a good idea, making it a param then is
easy, the added objectcode will mostly be all the strings, there is a
definite win in using to preprocessor to strip all these. Dunno if the
win is worth it though, I don't know how important object size is these
days.
> see dev_dbg, for a start, though I think youll want to
> wrap it so that you can pass in a level 1,2,3,
> while keeping 1 ifdef to remove the debug-code entirely.
>
> $ grep dev_dbg pc87360.c
> dev_dbg(&new_client->dev, "Using %s reference voltage\n",
> dev_dbg(&client->dev, "Forcibly "
> dev_dbg(&client->dev, "Forcibly "
> dev_dbg(&client->dev, "Skipping "
> dev_dbg(&client->dev, "Forcibly "
>
>
>
>
I think using dev_dbg is an idea, wrapped with the debug level thingie I
use.
>
>
>> /* For the Abit uGuru, we need to keep some data in memory.
>> The structure is dynamically allocated, at the same time when a new
>> abituguru client is allocated. */
>>
>>
> do you foresee / have you tried multiple chips ?
>
No and no, but I wanted to:
-do things identical to other drivers for readability
-want to keep the option of multiple chips open for the future
>
>> /* Put the uguru in ready for input state, this code assumes that
>> the uguru is not already in this state.
>> It is the callers responsibility to make sure it isn't! */
>>
>>
>
> what do you save by making this assumption ?
> yes its static, so less ripe for abuse, but if its 1 flag ..
>
The problem is there is no way to ask the uGuru if its ready, I keep a
ready flag which gets cleared when we're talking to it and set when
we've successfully put it back in ready state, this function could check
the flag, but it currently is never called when that flag is set.
Besides the flag there is no way to know thus the caller should know
what it is doing, just like other functions should clear the ready flag
as soon as they do something with the uGuru. The comment is there in an
attempt to make people aware of this.
Also doing a ready cycle when the uGuru already is ready can't hurt.
(AFAIK remember this is a reverse engineered driver).
>
>
>
>
>
>> /* The first try the uguru normally will be ready for the first
>> input and thus in ABIT_UGURU_STATUS_INPUT state. If however
>> something went wrong previously it might not be, so then we
>> try to force it into ready state.
>>
>>
>
> is this forcing something the user should have to explicitly enable ?
>
No, normally after a successfull data exchange (read or write) with the
uGuru the uGuru gets put back in ready state, so the first try to start
a data transfer (which starts with sending the bank and sensor address)
the ready flag will be one and the code below the comment:
if (!data->uguru_ready && (abituguru_ready(client) != 0))
return -EIO;
Will do nothing, if sending the address fails and the caller of
abituguru_send_address has specified that he wants to retry the second
and later tries the ready flag will be 0 and the uguru will be put back
into ready state before retrying.
>
>
>> /* Following are the sysfs callback functions. These functions use
>> sensor_device_attribute_2->nr and sensor_device_attribute_2->index
>> in the following ways:
>> nr selects index selects
>> _value/pwm_* NA sensor/pwm *
>> _setting auto/min/max (offset) sensor/pwm
>> _alarms/_mask beep/shutdown/flag (bitmask) sensor_type
>> (volt/temp) **
>>
>> *) Except for pwm_setting which uses nr and index as _setting
>> **) NA for bank2 as bank2 only contains fan sensors */
>>
>>
>
> following are nearly identical
>
>> static ssize_t show_bank1_value(struct device *dev,
>> struct device_attribute *devattr, char *buf)
>>
>> static ssize_t show_bank1_setting(struct device *dev,
>> struct device_attribute *devattr, char *buf)
>>
>> static ssize_t show_bank2_value(struct device *dev,
>> struct device_attribute *devattr, char *buf)
>>
>>
>>
I've thought about merging these too, but there actually a number of
subtile differences which when all added together make it easier to just
keep 2 versions:
-bank1 and bank2 have a different address
-bank1 settings contain 3 bytes, bank2 settings 2
-bank1 contains both temp and volt sensors, when showing or storing a
mask like beep_mask_in_low / shutdown_mask_temp, etc. We don't apply
bit 0 of the mask to sensor 0 etc, but we do this indirectly addressed
through data->bank1_address[attr->index][i] where attr->index selects
the sensortype (in or temp) and i selects the i-th sensor, this makes
bit 0 of shutdown_mask_temp correlate to temp1, bit 1 to temp2 etc,
instead of the alarm mask is chip specific, use the chip specific
defines hell other drivers have, also see sysfs-interface-proposal,
which is all about this. Not having the define hell is the only way
for the uGuru as what kinda sensor (in or temp) is attached to which
address is determined on driver load and can change from mb to mb.
bank2 otoh is straight, so bit 0 is fan1, bit1 fan2 etc, to use the
same code for bank2 I would need to create an 1x6 2 dimensional array
with bank2_address and just fill this with 0,1,2,3,4,5 . Which IMHO is
ugly.
-bank1 has 1 alarm bit per sensor, and extra bits in the sensor settings
bank which can be used to determine if its a in low or in high alarm,
bank2 does not have these extra bits in its sensor settings bank
So they are not quite the same, some functions differ more between banks
then others but for readability I've choosen to keep them all seperate.
> + others below I think. it looks like attr->index, and bank 1,2 could
> drive a constants & formula
> lookup to provide many calcs reachable by 1/few handers.
>
>
> Im beginning to think you could make good use of SENSOR_ATTR_4,
> which Jean blue-sky'd, but you could make concrete easily.
>
> with it, you could squeeze these many handlers down to a few,
> doing scale and offset lookups and calcs, forex.
>
I might be able todo that, but the objectsize would probably only grow
since the remaining functions stand a good chance of getting more then 2
times bigger, and the code would get much more complicated. The bank1
code currently already is complicated with the dynamicly detecting
senortype stuff and indirect addressing for masks, and I for one am all
for KIS .
>
> PS. what boxes have this chip in them ?
> /me thinks it might be in comments.
> /me should just look myself, instead of asking :-O
>
All rescent Abit motherboards, for an unannounced driver it actually
already has quite a few users who all found it be actively looking for
it and finding it on the mailinglist.
Thanks & Regards,
Hans
^ permalink raw reply [flat|nested] 6+ messages in thread