[lm-sensors] vt1211 driver (was hwmon/pc87360 as a platform

[lm-sensors] vt1211 driver (was hwmon/pc87360 as a platform

[Jim Cromie]

Juerg Haefliger wrote:
>> The SHOW_SET_*_*  constants are just a bit off-putting at first read,
>> but I know what you mean, and I dont have a better idea.
>> maybe SHOW_SETTNG_*_* ?
>> or SHOW_CURR_*_*,   thats confuse-able with the current reading
>>     
>
> Well SHOW_SET_xxx_yyy is supposed to hint that the constant is used in
> both the show_xxx and set_xxx callbacks. It's the best I could come up
> with although I have to admit I didn't spend too much time thinking
> about this....
>
>   
>> you can improve your printks :
>>
>> s/(printk\(KERN_DEBUG)/dev_dbg\(&pdev->dev/;
>> s/(printk\(KERN_ERR)/dev_err\(&pdev->dev/
>> s/(printk\(KERN_INFO)/dev_info\(&pdev->dev/
>>
>> at least where pdev has already been initd
>>     
>
> Hmm... I thought I used printks only in places where pdev is not initialized.
>
>   
I didnt look that carefully.
>> Ill try to read thru the enire patch this weekend sometime.
>>     
>
> I certainly appreciate the review :-)
>
>   
good thing that *this* is so flexible ;-)

> ...juerg
>
>   

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: hwmon-vt1211-driver-review
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060417/2985cd2c/hwmon-vt1211-driver-review.pl

[lm-sensors] vt1211 driver (was hwmon/pc87360 as a platform

[Juerg Haefliger]

Hi Jim,

Thanks for the review!

> + * Reading 1                      temp3       Intel thermal diode
> + * Reading 3                      temp1       internal thermal diode
>
> Intel or internal ??

Intel *and* internal, just as the comment says.

> + * ------------------------------------------------------------------------ */
>
> Im not sure how Jean feels about 'code-fences' like --------------,
> but Id like them to not exceed 79 columns.
> my editor, emacs, has 80 column window by default,
> and its sticking in a line-wrap.
> Just one less char, and the wrap goes away.

I *like* them :-) I'll fix the length issue.

> +static struct vt1211_data *vt1211_update_device(struct device *dev)
> +{
> +       struct vt1211_data *data = dev_get_drvdata(dev);
> +       int ix, val;
> +
> +       mutex_lock(&data->update_lock);
> +
> +       /* registers cache is refreshed after 1 second */
> +       if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> +
> +               /* voltage (in) registers */
> +               for (ix = 0; ix <= 6; ix++) {
>
>
> here and elsewhere, ARRAY_SIZE() instead of numeric constants

OK.

> +               /* temp registers */
> +               for (ix = 1; ix <= 7; ix++) {
>
> why start at 1 ?
> you could instead build the offset into your SENSOR_ATTR_{TEMP,FAN}
> initializations, then you dont need runtime -1

I think it's more clear this way. I like the callback numbers to match
the sysfs numbers. In fact when I first looked at the driver, I had a
hard time figuring out what was going on because of the offsetting.

> +#define SHOW_IN_INPUT  0
> +#define SHOW_SET_IN_MIN        1
> +#define SHOW_SET_IN_MAX        2
> +#define SHOW_IN_ALARM  3
>
> FWIW, I like these combo callbacks

Me too. It shrinks the source down and makes it more readable.

> +       case SHOW_IN_ALARM:
> +               return sprintf(buf, "%d\n", ISVOLT(ix, data->uch_config) &
> +                              (ix = 0 ? data->alarms >> 11 :
> +                               ix = 1 ? data->alarms >>  0 :
> +                               ix = 2 ? data->alarms >>  1 :
> +                               ix = 3 ? data->alarms >>  3 :
> +                               ix = 4 ? data->alarms >>  8 :
> +                               ix = 5 ? data->alarms >>  2 :
>
> maybe a const lookup table is better here,
> esp for doc'g the mapping.

Yep, good idea. Will fix.

> +       case SHOW_SET_PWM_FREQUENCY:
> +               tmp = data->pwm_clk & 7;
> +               return sprintf(buf, "%d\n",
> +                              (tmp = 1 ? 45000 : tmp = 2 ? 22500 :
> +                               tmp = 3 ? 11250 : tmp = 4 ?  5630 :
> +                               tmp = 5 ?  2800 : tmp = 6 ?  1400 :
> +                               tmp = 7 ?   700 : 90000));
>
>
> another map, this time with very magic numbers.

Not very magic, just copied from the datasheet.

> +static ssize_t set_pwm_auto_point_pwm(struct device *dev,
> +                                     struct device_attribute *attr,
> +                                     const char *buf, size_t count)
> +{
> +       struct vt1211_data *data = dev_get_drvdata(dev);
> +       struct sensor_device_attribute_2 *sensor_attr_2 > +                                               to_sensor_dev_attr_2(attr);
> +       int ix = sensor_attr_2->index;
> +       int ap = sensor_attr_2->nr;
> +       long val = simple_strtol(buf, NULL, 10);
> +       if (ap = 2 || ap = 3) {
>
> comment why only 2,3 ??

0 and 4 are hard-wired in the chip and not programmable.

> +static int __init vt1211_device_add(unsigned short address)
> +{
> +       struct resource res = {
> +               .start  = address,
> +               .end    = address + 0x7f,
> +               .flags  = IORESOURCE_IO,
> +       };
> +       int err;
> +
>
>
> Im curious, how many functional units does your SIO device have ?
> are you mapping them all in 1 block ?

The vt1211 has 12 logical units (which map to different base
addresses) but the driver only deals with one of them (HW monitor).
The size of the HW monitor register space is 0x80 and it's mapped to
0xec00 (default).


...juerg