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
