I've checked in lots of fixes for the driver into CVS. It should work better now. Would you please test it (either on 2.4, or merge the changes into your 2.6 port) thanks mds Gabriele Gorla wrote: > sure. > I did not know the existence of the testing script but > I did test it with a 2.6 kernel on my intel i815 > motherboard. One fan reads back something reasonable > one reads back 0 and the 2 pwm do not seem to do > anything (the registers read back correctly). > I assumed that some features do not work on my mb > because they are not connected. I do not have access > to the mb schematics to check. > > thanks, > GG > > --- Mark Studebaker <mds4 at verizon.net> wrote: > >>before you send us another patch, would you please >>test the driver either >>with the 2.4 version or the 2.6 version, to see if >>it works? >>thanks >> >>Gabriele Gorla wrote: >> >>>Hello, >>>wow! it has been a long time that I didn't get >> >>such >> >>>good code review, thanks guys! >>> >>>I agree... the PWM code seemed a bit strange to me >> >>but >> >>>I assumed it to be working in the original driver, >> >>so >> >>>I didn't really pay too much attention to it. >> >>(perhaps >> >>>I should have?) >>> >>>I'll work on the changes sometimes this week and >>>resubmit the file. >>> >>>thanks, >>>GG >>>--- Mark Studebaker <mds4 at verizon.net> wrote: >>> >>> >>>>Gabriele, I reviewed the code and the datasheet >> >>and >> >>>>I agree with Jean that the >>>>code looks totally broken. On the other hand you >>>>would think I tested the >>>>driver when I wrote it. Would you please test the >>>>PWM operation in the current driver, >>>>including the PWM enable? >>>>Running our pwmconfig script would be a good test. >>>>thanks >>>>mds >>>> >>>>Jean Delvare wrote: >>>> >>>> >>>>>Quoting myself: >>>>> >>>>> >>>>> >>>>> >>>>>>That's about all for this first check. I have >> >>yet >> >>>>to take a look at >>>> >>>> >>>>>>the sysfs callback functions. >>>>> >>>>> >>>>>OK, I just did this. Your code is correct in the >>>> >>>>sense that it does the >>>> >>>> >>>>>same as what the 2.4/CVS driver does. However, I >>>> >>>>have noticed a number >>>> >>>> >>>>>of strangenesses, and think this is the right >> >>time >> >>>>to discuss them. >>>> >>>> >>>>>First, there is no lock on read/write. There is a >>>> >>>>lock for the update >>>> >>>> >>>>>function, but that's about all. It's obviously >> >>not >> >>>>sufficent. Usually, >>>> >>>> >>>>>drivers have a second lock for read and write >>>> >>>>operations. >>>> >>>> >>>>>Second, I'm really confused by the operations >> >>made >> >>>>on the PWM registers. >>>> >>>> >>>>>When reading the code, it looks like bit 7 is for >>>> >>>>enabling PWM and bits >>>> >>>> >>>>>6-0 hold the duty cycle value. Except that only >>>> >>>>bits 5-0 can be set, >>>> >>>> >>>>>and the conversions macros are odd. When looking >>>> >>>>at the datasheet OTOH, >>>> >>>> >>>>>I read that PWM enable is bit 0, inverted (1 >>>> >>>>actually disables PWM), >>>> >>>> >>>>>and value is in bits 6-1. Bit 7 is a clock >>>> >>>>modifier, we don't care. >>>> >>>> >>>>>Conversions would be as simple as multiplying / >>>> >>>>dividing by 4 (with >>>> >>>> >>>>>some rounding if you want). >>>>> >>>>>Mark, am I missing something here or is PWM >>>> >>>>handling in the smsc47m1 >>>> >>>> >>>>>driver plain broken? I'd be surprised that nobody >>>> >>>>reported so far if it >>>> >>>> >>>>>is, but sometimes we get surprised. >>>>> >>>>>Another point that could be improved in the 2.6 >>>> >>>>driver is the fan >>>> >>>> >>>>>divisor setting. Instead of defaulting to 2 on >>>> >>>>invalid sets, you should >>>> >>>> >>>>>refuse them and return -EINVAL. This couldn't be >>>> >>>>done in the 2.4 >>>> >>>> >>>>>driver, but it can be in 2.6, so let's do it. A >>>> >>>>number of drivers >>>> >>>> >>>>>already do (fscher.c was the first IIRC). >>>>> >>>>>Thanks. >>>>> >>>> >>> >>> >>> >>> >>>__________________________________ >>>Do you Yahoo!? >>>Win a $20,000 Career Makeover at Yahoo! HotJobs >>> >> >>http://hotjobs.sweepstakes.yahoo.com/careermakeover >> >>> > > > > > __________________________________ > Do you Yahoo!? > Win a $20,000 Career Makeover at Yahoo! HotJobs > http://hotjobs.sweepstakes.yahoo.com/careermakeover >
