[lm-sensors] [PATCH 1/2] hwmon: new vt1211 driver

[lm-sensors] [PATCH 1/2] hwmon: new vt1211 driver

[Juerg Haefliger]

[PATCH 1/2] hwmon: new vt1211 driver

This is a new driver for the VIA vt1211 Super-IO chip. It is a rewrite of the
existing vt1211 driver (by Mark D. Studebaker and Lars Ekman) that has been
around for a while but never made it into the main kernel tree. It is
implemented as a platform driver and therefore requires the latest CVS version
of lm_sensors to function properly.

Signed-off-by: Juerg Haefliger <juergh at gmail.com>

---
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hwmon-vt1211-new-driver.patch
Type: text/x-patch
Size: 40592 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060225/20104c1f/hwmon-vt1211-new-driver-0001.bin

[lm-sensors] [PATCH 1/2] hwmon: new vt1211 driver

[Juerg Haefliger]

Hi,

I don't mean to be pushy but did anybody have a chance to look at my
vt1211 patch?

Thanks
...juerg


> [PATCH 1/2] hwmon: new vt1211 driver
>
> This is a new driver for the VIA vt1211 Super-IO chip. It is a rewrite of the
> existing vt1211 driver (by Mark D. Studebaker and Lars Ekman) that has been
> around for a while but never made it into the main kernel tree. It is
> implemented as a platform driver and therefore requires the latest CVS version
> of lm_sensors to function properly.
>
> Signed-off-by: Juerg Haefliger <juergh at gmail.com>
>
> ---

[lm-sensors] [PATCH 1/2] hwmon: new vt1211 driver

[Rudolf Marek]

Juerg Haefliger wrote:
> Hi,
> 
> I don't mean to be pushy but did anybody have a chance to look at my
> vt1211 patch?

I have it in queue. I already did the 3more last week and still w83791d is pending...
(older) so please be patient. I will try to do that next week.

Regards
Rudolf

[lm-sensors] [PATCH 1/2] hwmon: new vt1211 driver

[Rudolf Marek]

Hi again,

I'm attaching commented version in file.

Sometimes there are places with spaces and not tabs (especially in #define ) plus some operators like + dont have spaces around too.
I'm proposing that some of funcs can be done via macro or 2D array. This might help to shrink the source code a bit.
I'm not user about the runtime UCONFIG configuration. Perhaps cleanest solution would be to create/remove sysfs nodes
runtime instead of exporting them with zero.
Also I think several printks can be replaced with dev_err stuff.
Last thing to discuss is about the forced interrupt mode. I'm not sure if it is OK because it may affect the chip
pins.
I'm not too familiar with the platform stuff yet so I'm asking Jean to look into this.

Overall the code looks very good. I tried to read all the stuff carefuly but it is quite difficult not to miss anything.
(and even I spent like 3 hours just with reading and I'm not even sure :( )

Regards
Rudolf

-------------- next part --------------
A non-text attachment was scrubbed...
Name: hwmon-vt1211-new-driver.patch
Type: text/x-patch
Size: 41217 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060324/46c3f4b5/hwmon-vt1211-new-driver-0001.bin

[lm-sensors] [PATCH 1/2] hwmon: new vt1211 driver

[Juerg Haefliger]

Hi,

> Sometimes there are places with spaces and not tabs (especially in #define
> ) plus some operators like + dont have spaces around too.

Ok, will fix.

> I'm proposing that some of funcs can be done via macro or 2D array. This
> might help to shrink the source code a bit.

I had it this way in the begining but threw it out for some reasons I
can't remember anymore. I'll take a look at it again.

> I'm not user about the runtime UCONFIG configuration. Perhaps cleanest
> solution would be to create/remove sysfs nodes
> runtime instead of exporting them with zero.

I don't understand. Exporting the UCH config register is a left-over
from the 2.4 driver. I think the reason why it's in the 2.4 driver is
because some BIOSes don't set it correctly. Maybe this is not true
anymore and we can now fully rely on the BIOS doing it right and don't
need to export the register anymore. Comments anyone?

> Also I think several printks can be replaced with dev_err stuff.

OK, will do.

> Last thing to discuss is about the forced interrupt mode. I'm not sure if
> it is OK because it may affect the chip
> pins.

Well the vt1211 implements 3 different interrupt modes (or more
precisely 3 different modes of how interrupts get cleared):

1) Generate INT when temp exceeds max limit. Clear INT when temp falls
below max limit.
2) Generate INT when temp exceeds max limit. Clear INT when status
register is read. Regenerate INT as long as temp stays above
hysteresis limit.
3) Generate INT when temp exceeds max limit. Clear INT when status
register is read. DON'T regenerate INT until temp falls below
hysteresis limit and exceeds hot limit again.

The default is mode 3 (on my EPIA M10000) and the driver sets it to
mode 2 upon initialization. This is also a left-over from 2.4 and it
has been around for some time. I'm not aware of any problems with this
so I'm reluctant to change it unless somebody fells strongly about
it...

> I'm not too familiar with the platform stuff yet so I'm asking Jean to
> look into this.
>
> Overall the code looks very good. I tried to read all the stuff carefuly
> but it is quite difficult not to miss anything.
> (and even I spent like 3 hours just with reading and I'm not even sure :(
> )

Thanks for taking the time to look at the patch. I really appreciate
it and keep up the good work! You guys do an outstanding job!

...juerg


> Regards
> Rudolf
>
>

[lm-sensors] [PATCH 1/2] hwmon: new vt1211 driver

[Rudolf Marek]

Juerg Haefliger wrote:
> Hi,
> 
> 
>>Sometimes there are places with spaces and not tabs (especially in #define
>>) plus some operators like + dont have spaces around too.
> 
> 
> Ok, will fix.

Good.

> 
> 
>>I'm proposing that some of funcs can be done via macro or 2D array. This
>>might help to shrink the source code a bit.
> 
> 
> I had it this way in the begining but threw it out for some reasons I
> can't remember anymore. I'll take a look at it again.

Maybe Jean have some idea about it too? Or others?


>>I'm not user about the runtime UCONFIG configuration. Perhaps cleanest
>>solution would be to create/remove sysfs nodes
>>runtime instead of exporting them with zero.
> 
> 
> I don't understand. Exporting the UCH config register is a left-over
> from the 2.4 driver. I think the reason why it's in the 2.4 driver is
> because some BIOSes don't set it correctly. Maybe this is not true
> anymore and we can now fully rely on the BIOS doing it right and don't
> need to export the register anymore. Comments anyone?

Well me. I think we can drop this interface and read the UHC conf only at the
driver start. User with bad BIOS config can do i2cset/isaset commands if neccessary
to change the config and then load the driver.


>>Also I think several printks can be replaced with dev_err stuff.
> 
> 
> OK, will do.
> 

Good.

>>Last thing to discuss is about the forced interrupt mode. I'm not sure if
>>it is OK because it may affect the chip
>>pins.
> 
> 
> Well the vt1211 implements 3 different interrupt modes (or more
> precisely 3 different modes of how interrupts get cleared):
> 
> 1) Generate INT when temp exceeds max limit. Clear INT when temp falls
> below max limit.
> 2) Generate INT when temp exceeds max limit. Clear INT when status
> register is read. Regenerate INT as long as temp stays above
> hysteresis limit.
> 3) Generate INT when temp exceeds max limit. Clear INT when status
> register is read. DON'T regenerate INT until temp falls below
> hysteresis limit and exceeds hot limit again.
> 
> The default is mode 3 (on my EPIA M10000) and the driver sets it to
> mode 2 upon initialization. This is also a left-over from 2.4 and it
> has been around for some time. I'm not aware of any problems with this
> so I'm reluctant to change it unless somebody fells strongly about
> it...

Ok I think we can drop it later if it causes some troubles.

> 
> 
>>I'm not too familiar with the platform stuff yet so I'm asking Jean to
>>look into this.
>>
>>Overall the code looks very good. I tried to read all the stuff carefuly
>>but it is quite difficult not to miss anything.
>>(and even I spent like 3 hours just with reading and I'm not even sure :(
>>)
> 
> 
> Thanks for taking the time to look at the patch. I really appreciate
> it and keep up the good work! You guys do an outstanding job!

Well this work is done mostly by me and Jean and sometimes Mark... People willing to help with this would be welcomed.

Regards,
Rudolf

[lm-sensors] [PATCH 1/2] hwmon: new vt1211 driver

[Juerg Haefliger]

>>> I'm proposing that some of funcs can be done via macro or 2D array.
>>> This might help to shrink the source code a bit.
>>
>> I had it this way in the begining but threw it out for some reasons I
>> can't remember anymore. I'll take a look at it again.
>
> Maybe Jean have some idea about it too? Or others?

I fixed it already, don't bother.

>>> I'm not user about the runtime UCONFIG configuration. Perhaps cleanest
>>> solution would be to create/remove sysfs nodes runtime instead of
>>> exporting them with zero.
>>
>> I don't understand. Exporting the UCH config register is a left-over
>> from the 2.4 driver. I think the reason why it's in the 2.4 driver is
>> because some BIOSes don't set it correctly. Maybe this is not true
>> anymore and we can now fully rely on the BIOS doing it right and don't
>> need to export the register anymore. Comments anyone?
>
> Well me. I think we can drop this interface and read the UHC
> conf only at the driver start. User with bad BIOS config can
> do i2cset/isaset commands if neccessary to change the config
> and then load the driver.
>

hmm... libsensor currently supports this interface. Is it deprecated?
Using i2c to change it would be OK I guess, except that the driver
itself doesn't need i2c anymore (it's platform) and the user would
have to compile i2c just for this (if required, that is).
How about using a module load parameter instead?

...juerg

[lm-sensors] [PATCH 1/2] hwmon: new vt1211 driver

[Jim Cromie]

Rudolf Marek wrote:
> Juerg Haefliger wrote:
>   
>
>>> I'm proposing that some of funcs can be done via macro or 2D array. This
>>> might help to shrink the source code a bit.
>>>       
>> I had it this way in the begining but threw it out for some reasons I
>> can't remember anymore. I'll take a look at it again.
>>     
>
> Maybe Jean have some idea about it too? Or others?
>
>
>   


you might want to initialize 1-D arrays with a series of SENSOR_ATTR_2's

+static struct sensor_device_attribute_2 fan_input[] = {
+       SENSOR_ATTR_2(fan1_input, S_IRUGO, show_fan_input, NULL, 
fn_fan_input, 0),
+       SENSOR_ATTR_2(fan2_input, S_IRUGO, show_fan_input, NULL, 
fn_fan_input, 1),
+       SENSOR_ATTR_2(fan3_input, S_IRUGO, show_fan_input, NULL, 
fn_fan_input, 2),
 };

there are 2 members that you can use to specify indexes;   (paste from 
macro defn)
+         .index = _index,                                      \
+         .nr = _nr

in your callback routine, you obtain those values, and decide what to do 
on that basis.

static ssize_t show_fan_input(struct device *dev, struct 
device_attribute *devattr, char *buf)
 {
       struct sensor_device_attribute_2 *attr = 
to_sensor_dev_attr_2(devattr);
       int idx = attr->index;
       int nr = attr->nr;
    /* how you decide here */
}

By convention, index tells your driver which hardware unit to read.

is offset into the array of sensors as seen in userspace
IE how it appears in sysfs:  /sys/bus/i2c/devices/9191-6620/* on my box.
Drivers typically number their sensors starting from 0 or 1, (I think 
this is to
match the numbering used in the tech-docs, which simplifies driver 
maintenance).
Anyway, if your driver numbers, say FANs, starting at 1, you'd put that 
offset
in the fan-input declaration pasted above. 

The .nr field really has no convention yet - you can encode whatever you 
like
into the value, and use it in your callback.  Ive used nr in a recently 
proposed patch
( hwmon-pc87360-sysfs-combo-callbacks.patch ),  to tell the callback what
attribute of the indexed sensor was requested, so my /* how you decide 
here */
looks like this:

+       switch(nr) {
+       case fn_fan_input:
+               res = FAN_FROM_REG(data->fan[idx],
+                                  FAN_DIV_FROM_REG(data->fan_status[idx]));
+               break;
+       case fn_fan_min:
+               res = FAN_FROM_REG(data->fan_min[idx],
+                                  FAN_DIV_FROM_REG(data->fan_status[idx]));
+               break;
+       case fn_fan_status:
+               res = FAN_STATUS_FROM_REG(data->fan_status[idx]);
+               break;
+       case fn_fan_div:
+               res = FAN_DIV_FROM_REG(data->fan_status[idx]);
+               break;
+       default:
+               printk(KERN_ERR "unknown attr fetch\n");


IOW, decoding nr lets your callback do almost anything (2**8 choices),
and you can reduce the number of discrete callbacks accordingly.
We rely on programmers good judgment to not try combining
floor-wax with dessert-topping ;-)


Is this along the lines of what you were contemplating ?

[lm-sensors] [PATCH 1/2] hwmon: new vt1211 driver

[Rudolf Marek]

Hello,

Sorry for delay.

> hmm... libsensor currently supports this interface. Is it deprecated?
> Using i2c to change it would be OK I guess, except that the driver
> itself doesn't need i2c anymore (it's platform) and the user would
> have to compile i2c just for this (if required, that is).

I had isaset in mind. Sorry for confusion.

> How about using a module load parameter instead?

Yes that would be cleanest solution.

Regards
Rudolf