[lm-sensors] [PATCH] Add LM93 support

[Mark M. Hoffman]

Hello:

* Hans-Jürgen Koch <hjk at linutronix.de> [2007-07-01 00:35:20 +0200]:
> Mark,
> sorry for not responding to your last review for so long. I hoped all the
> time I might be able to add LM94 support as well. But unfortunately, the
> LM94 datasheet is available under NDA only (hard to believe, but true).
> National Semiconductor Germany said it were no problem to get an NDA that
> still allows me to write an open source driver, but that National in USA
> must decide that. Now I'm waiting, still haven't got that NDA, let alone
> the datasheet.
> 
> Good news is that I will get an i2c-tiny-usb interface soon, together
> with an LM94 test board. This means I'll have an LM94 always available
> and can perform all sorts of tests with it. I can therefore offer to
> become maintainer of this driver.
> 
> Find attached my current version of the LM93 patch. I addressed all 
> the issues you mentioned in your last review. There are a few things
> I'd be glad if you could check if I got it right:
> 
> 1.) I replaced pwmN_override with pwmN_enable.

1825         case 0:
1826                 ctl2 |= 0x01; /* enable manual override */
1827                 ctl2 &= 0x0F; /* set PWM to zero */
1828                 break;

No... the value 0 must turn the fan full-on, not off.  Sorry if I was not
clear about that in my review comments.

See also: Documentation/hwmon/sysfs_interface.

> 2.) IMO, show_pwm() was actually buggy as you suspected. I had a look 
>     at the datasheet and fixed it.

Yep, thanks.

> 3.) IMO, this code is also not correct:
> 
> > +/* PWM: 0-255 per sensors documentation
> > +   REG: 0-13 as mapped below... right justified */
> > +typedef enum { LM93_PWM_MAP_HI_FREQ, LM93_PWM_MAP_LO_FREQ } pwm_freq_t;
> > +static int lm93_pwm_map[2][14] = {
> > +     {
> > +             0x00, /*   0.00% */ 0x40, /*  25.00% */
> > +             0x50, /*  31.25% */ 0x60, /*  37.50% */
> > +             0x70, /*  43.75% */ 0x80, /*  50.00% */
> > +             0x90, /*  56.25% */ 0xa0, /*  62.50% */
> > +             0xb0, /*  68.75% */ 0xc0, /*  75.00% */
> > +             0xd0, /*  81.25% */ 0xe0, /*  87.50% */
> > +             0xf0, /*  93.75% */ 0xff, /* 100.00% */
> > +     },
> > +     {
> > +             0x00, /*   0.00% */ 0x40, /*  25.00% */
> > +             0x49, /*  28.57% */ 0x52, /*  32.14% */
> > +             0x5b, /*  35.71% */ 0x64, /*  39.29% */
> > +             0x6d, /*  42.86% */ 0x76, /*  46.43% */
> > +             0x80, /*  50.00% */ 0x89, /*  53.57% */
> > +             0x92, /*  57.14% */ 0xb6, /*  71.43% */
> > +             0xdb, /*  85.71% */ 0xff, /* 100.00% */
> > +     },
> > +};
> > +
> > +static int LM93_PWM_FROM_REG(u8 reg, pwm_freq_t freq)
> > +{
> > +     return lm93_pwm_map[freq][reg & 0x0f];
> > +}
> 
> OK, the values 14 and 15 for reg are reserved and should not occur.
> But if for some reason you read 0xff from the chip, the above
> code would access bytes beyond the end of the array. I added two
> more 0xff bytes so that all 16 possible values are valid.

Makes sense.

> I didn't have the time to test that patch until now. In fact, I
> cannot test everything with my current equipment. I'll test as
> good as I can until Monday evening, perhaps you or somebody else
> can help testing, too.
> 
> As it might take some more weeks until I can add LM94 support,
> I think we should take LM93 as it is now and patch in LM94
> later. I'd really like to see this going mainline in the next
> merge window.

Agreed.

> Please have a look at my patch and let me know if there are
> still issues I need to fix.

Just the pwm_enable thing.

Thanks & regards,

-- 
Mark M. Hoffman
mhoffman at lightlink.com