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
