Re: [lm-sensors] superio lock coordinator

Re: [lm-sensors] superio lock coordinator

[Jim Cromie]

hi Hans,

thanks for the personal invite to this thread. 
I messed up a laptop update last nov, and lost my mail folders
(several gig had piled up so the purge was needed anyway)

hi everyone else too,
apologies for being out of it for so long, and for things left half done.

> David Hubbard wrote:
>> Hi Hans,
>>
>> I can pick it up again and push it forward. IIRC we wanted to know
>> what was the best way to encode the enter and exit sequences for a
>> SuperIO because the w83627ehf is different than pretty much any other.
>> That was the tricky part.
>>
>
> If you look at the latest version posted by Jim:
> http://lists.lm-sensors.org/pipermail/lm-sensors/2007-October/021562.html
>
> You will see that it already has configurable enter / exit sequences. 
> AFAIK the winbond chips are not unique in having differen enter / exit 
> sequences as other superio's have, so having to specify the enter / 
> exit sequence in the driver using the super io coordinator seems correct.
>

I have recently been revisiting the patches, but havent been entirely 
happy with them,
and didnt really like them enough to propose again.

This thread certainly ups it from a background muse.
Happily, I got my soekris back, and have a box that can actually
use 2 superio-client-drivers (ghod this needs a shorter name)


these are on my current todo / partly done

- struct superio_common - stuff thats copied from struct superio-search 
to struct superio when reservation is taken

- add a u8 flags to struct superio
-- carries bit for tracking enter/exit state -
    should support warnings on situations like enter-on-entered, 
exit-on-exited
 - could carry a toggle bit for a union superio_exit { fnptr; 
exit_seq[X] } or somesuch.

-- following another thread - we'd
--- reserve region when reservation taken, after devid is validated,
    might help keep other semi-rogue drivers out.

= sort out enter / exit

As David noted in this thread, the ehf driver has an odd exit sequence,

The combined combined superio_exit() function I have coded currently is 
really quite hideous,
and has me thinking towards  David's function-pointer idea.

- the other alternative is that David's driver should include its own 
custom exit-fn, which
he would use instead of the regular superio_exit().  It could use the 
superio_enter routine as-is iirc.

my stall points - (fishing for answers;)

- enter / exit should be tracked by superio-locks, in flags bit.   
(recent idea, seems sound)
- enter / exit should be vaguely discouraged in kernel-doc - due to 
expectations of isadump etc.


I guess I gotta stop overcomplicating things, and get busy on the patch-set.


> More interesting questions IMHO are:
> 1) is hwmon the correct subsystem to put the .c file in (somewhat 
> superfical
>   really, but we need a place to put the lock coordinator (and enable 
> / disable
>   its building).

I put it here for proximity to the center-of-mass of the drivers I found.

> 2) non hwmon drivers need to be edited for superio register use too, 
> and then
>   modified to use the lock coordinator if they touch the superio 
> registers.
>   This will also be a good exercise to see if our API is generic enough.
>
I guess there are drivers I missed ?


> Regards,
>
> Hans
>

thanks again Hans for pulling me back into this, I'll commit to staying 
with it now.
Should I infer from your comments that the code at the link incorporates 
your feedback ?
They seem to have aquired some momentum recently 8-)

Jean, you raised the idea of a sysfs interface.
Does the October patchset look to fit with what youre thinking ?
- the most simple I can think of :
--expose the kzalloced array of superio-fields as readonly attributes.
--values go non-zero when superio-port is registered by some user-driver
--it would be nice to see client-drivers list on each slot, but that 
involves more code and mem

David,
do you have a preference wrt fn-ptr ? and if so, a single fn with 1 arg 
:  0,1,2 for enter/exit/query ?
do you have any use for a flag bit ?  is there generality in it ?

does anyone think s/superio/sio/g is appropriate ?
anything else ?


thanks
Jim


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] superio lock coordinator

[Jean Delvare]

Hi Jim,

On Tue, 15 Jul 2008 01:56:00 -0600, Jim Cromie wrote:
> these are on my current todo / partly done
> 
> - struct superio_common - stuff thats copied from struct superio-search 
> to struct superio when reservation is taken
> 
> - add a u8 flags to struct superio
> -- carries bit for tracking enter/exit state -
>     should support warnings on situations like enter-on-entered, 
> exit-on-exited
>  - could carry a toggle bit for a union superio_exit { fnptr; 
> exit_seq[X] } or somesuch.
> 
> -- following another thread - we'd
> --- reserve region when reservation taken, after devid is validated,
>     might help keep other semi-rogue drivers out.

Actually, _before_ devid is validated. You shouldn't make _any_ I/O
access without first requesting the region. You can always release the
region afterwards if you didn't find anything interesting.

> 
> = sort out enter / exit
> 
> As David noted in this thread, the ehf driver has an odd exit sequence,
> 
> The combined combined superio_exit() function I have coded currently is 
> really quite hideous,
> and has me thinking towards  David's function-pointer idea.
> 
> - the other alternative is that David's driver should include its own 
> custom exit-fn, which
> he would use instead of the regular superio_exit().  It could use the 
> superio_enter routine as-is iirc.

Please read my reply to David's concern, and why having a per-chip exit
sequence doesn't make that much sense. I would use the same exit
function as isadump and sensors-detect have been using for months, as
this appears to work fine.

> my stall points - (fishing for answers;)
> 
> - enter / exit should be tracked by superio-locks, in flags bit.   
> (recent idea, seems sound)
> - enter / exit should be vaguely discouraged in kernel-doc - due to 
> expectations of isadump etc.

Which expectations? User-space should stay away from I/O ports the
kernel has requested. There's little point in trying to sort out
concurrent accesses which simply shouldn't happen in the first place.

> 
> 
> I guess I gotta stop overcomplicating things, and get busy on the patch-set.
> 
> 
> > More interesting questions IMHO are:
> > 1) is hwmon the correct subsystem to put the .c file in (somewhat 
> > superfical
> >   really, but we need a place to put the lock coordinator (and enable 
> > / disable
> >   its building).
> 
> I put it here for proximity to the center-of-mass of the drivers I found.

That's not necessarily a bad initial location. We can have superio live
in drivers/hwmon at first, and once all hwmon drivers use it, move it
to a neutral ground and convert the other drivers in the kernel tree
(mainly watchdog drivers I think.)

> > 2) non hwmon drivers need to be edited for superio register use too, 
> > and then
> >   modified to use the lock coordinator if they touch the superio 
> > registers.
> >   This will also be a good exercise to see if our API is generic enough.
> >
> I guess there are drivers I missed ?

Best way I think is to grep for each super-I/O chip name.

> thanks again Hans for pulling me back into this, I'll commit to staying 
> with it now.
> Should I infer from your comments that the code at the link incorporates 
> your feedback ?
> They seem to have aquired some momentum recently 8-)
> 
> Jean, you raised the idea of a sysfs interface.
> Does the October patchset look to fit with what youre thinking ?

I don't think your patch offered a sysfs interface? I'm not really
thinking of anything in particular anyway. We just want a way for
sensors-detect to identify supported Super-I/O chips without actually
accessing the I/O ports if a kernel driver already requested them.

> - the most simple I can think of :
> --expose the kzalloced array of superio-fields as readonly attributes.
> --values go non-zero when superio-port is registered by some user-driver
> --it would be nice to see client-drivers list on each slot, but that 
> involves more code and mem

What we really need for sensors-detect is simply the value of registers
0x20 and 0x21, and values of registers 0x30, 0x60 and 0x61 for a
selected logical device. In what form we export these, needs to be
discussed.

> David,
> do you have a preference wrt fn-ptr ? and if so, a single fn with 1 arg 
> :  0,1,2 for enter/exit/query ?
> do you have any use for a flag bit ?  is there generality in it ?
> 
> does anyone think s/superio/sio/g is appropriate ?

No. This was discussed before and I have always objected to the name
"sio". It's too short and could mean about anything (starting with
"synchronous I/O", as opposed to "asynchronous I/O" which is very often
spelled AIO.)

> anything else ?

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] superio lock coordinator

[David Hubbard]

Hi Jim,

Glad to see you again on lm-sensors!

On Tue, Jul 15, 2008 at 4:40 AM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Jim,
>
> On Tue, 15 Jul 2008 01:56:00 -0600, Jim Cromie wrote:
>> these are on my current todo / partly done
>>
>> - struct superio_common - stuff thats copied from struct superio-search
>> to struct superio when reservation is taken
>>
>> - add a u8 flags to struct superio
>> -- carries bit for tracking enter/exit state -
>>     should support warnings on situations like enter-on-entered,
>> exit-on-exited
>>  - could carry a toggle bit for a union superio_exit { fnptr;
>> exit_seq[X] } or somesuch.
>>
>> -- following another thread - we'd
>> --- reserve region when reservation taken, after devid is validated,
>>     might help keep other semi-rogue drivers out.
>
> Actually, _before_ devid is validated. You shouldn't make _any_ I/O
> access without first requesting the region. You can always release the
> region afterwards if you didn't find anything interesting.
>
>>
>> = sort out enter / exit
>>
>> As David noted in this thread, the ehf driver has an odd exit sequence,
>>
>> The combined combined superio_exit() function I have coded currently is
>> really quite hideous,
>> and has me thinking towards  David's function-pointer idea.
>>
>> - the other alternative is that David's driver should include its own
>> custom exit-fn, which
>> he would use instead of the regular superio_exit().  It could use the
>> superio_enter routine as-is iirc.
>
> Please read my reply to David's concern, and why having a per-chip exit
> sequence doesn't make that much sense. I would use the same exit
> function as isadump and sensors-detect have been using for months, as
> this appears to work fine.

Here it is:
On Mon, Jul 14, 2008 at 2:12 AM, Jean Delvare <khali@linux-fr.org> wrote:
> sensors-detect doesn't pay much attention to custom exist sequences,
> and neither does isadump:
>
> void superio_reset(int addrreg, int datareg)
> {
>        /* Some chips (SMSC, Winbond) want this */
>        outb(0xaa, addrreg);
>
>        /* Return to "Wait For Key" state (PNP-ISA spec) */
>        outb(0x02, addrreg);
>        outb(0x02, datareg);
> }

That will certainly work for w83627ehf, so superio-locks would not
even need to store the enter and exit sequences. Every hwmon device
should work like this.

>> my stall points - (fishing for answers;)
>>
>> - enter / exit should be tracked by superio-locks, in flags bit.
>> (recent idea, seems sound)
>> - enter / exit should be vaguely discouraged in kernel-doc - due to
>> expectations of isadump etc.
>
> Which expectations? User-space should stay away from I/O ports the
> kernel has requested. There's little point in trying to sort out
> concurrent accesses which simply shouldn't happen in the first place.
>
>>
>>
>> I guess I gotta stop overcomplicating things, and get busy on the patch-set.
>>
>>
>> > More interesting questions IMHO are:
>> > 1) is hwmon the correct subsystem to put the .c file in (somewhat
>> > superfical
>> >   really, but we need a place to put the lock coordinator (and enable
>> > / disable
>> >   its building).
>>
>> I put it here for proximity to the center-of-mass of the drivers I found.
>
> That's not necessarily a bad initial location. We can have superio live
> in drivers/hwmon at first, and once all hwmon drivers use it, move it
> to a neutral ground and convert the other drivers in the kernel tree
> (mainly watchdog drivers I think.)

Sounds like a good plan. drivers/hwmon/superio-locks.

>> > 2) non hwmon drivers need to be edited for superio register use too,
>> > and then
>> >   modified to use the lock coordinator if they touch the superio
>> > registers.
>> >   This will also be a good exercise to see if our API is generic enough.
>> >
>> I guess there are drivers I missed ?
>
> Best way I think is to grep for each super-I/O chip name.
>
>> thanks again Hans for pulling me back into this, I'll commit to staying
>> with it now.
>> Should I infer from your comments that the code at the link incorporates
>> your feedback ?
>> They seem to have aquired some momentum recently 8-)
>>
>> Jean, you raised the idea of a sysfs interface.
>> Does the October patchset look to fit with what youre thinking ?
>
> I don't think your patch offered a sysfs interface? I'm not really
> thinking of anything in particular anyway. We just want a way for
> sensors-detect to identify supported Super-I/O chips without actually
> accessing the I/O ports if a kernel driver already requested them.
>
>> - the most simple I can think of :
>> --expose the kzalloced array of superio-fields as readonly attributes.
>> --values go non-zero when superio-port is registered by some user-driver
>> --it would be nice to see client-drivers list on each slot, but that
>> involves more code and mem
>
> What we really need for sensors-detect is simply the value of registers
> 0x20 and 0x21, and values of registers 0x30, 0x60 and 0x61 for a
> selected logical device. In what form we export these, needs to be
> discussed.

Should superio-locks enumerate the available logical device ID's,
perhaps as a sysfs interface?

>> David,
>> do you have a preference wrt fn-ptr ? and if so, a single fn with 1 arg
>> :  0,1,2 for enter/exit/query ?
>> do you have any use for a flag bit ?  is there generality in it ?

I think we could just use the same enter and exit sequences for
everything, and not make it customizable. Does that sound OK?

>> does anyone think s/superio/sio/g is appropriate ?
>
> No. This was discussed before and I have always objected to the name
> "sio". It's too short and could mean about anything (starting with
> "synchronous I/O", as opposed to "asynchronous I/O" which is very often
> spelled AIO.)

David

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] superio lock coordinator

[Jim Cromie]

On 7/15/08, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Jim,
>
>
>  On Tue, 15 Jul 2008 01:56:00 -0600, Jim Cromie wrote:

>  > -- following another thread - we'd
>  > --- reserve region when reservation taken, after devid is validated,
>  >     might help keep other semi-rogue drivers out.
>
>
> Actually, _before_ devid is validated. You shouldn't make _any_ I/O
>  access without first requesting the region. You can always release the
>  region afterwards if you didn't find anything interesting.

ACK.

>
>
>  >
>  > = sort out enter / exit
>  >
>  > As David noted in this thread, the ehf driver has an odd exit sequence,
>  >
>  > The combined combined superio_exit() function I have coded currently is
>  > really quite hideous,
>  > and has me thinking towards  David's function-pointer idea.
>  >
>  > - the other alternative is that David's driver should include its own
>  > custom exit-fn, which
>  > he would use instead of the regular superio_exit().  It could use the
>  > superio_enter routine as-is iirc.
>
>
> Please read my reply to David's concern, and why having a per-chip exit
>  sequence doesn't make that much sense. I would use the same exit
>  function as isadump and sensors-detect have been using for months, as
>  this appears to work fine.

OK, quoting

As I recall, the rationale for the code above is that you do not
necessarily recognize a known Super-I/O chip after the init sequence.
As different chips have different exit sequences, there are definitely
cases where you just don't know what to do. So we need a universal exit
sequence, which is the code above. And once you have that and it
appears to work with all known chips, there's simply no reason to not
use it for all chips even when we know their exit sequence.

I dont understand what you mean by :
"not necessarily recognize a known Super-I/O chip after the init sequence"

recognize =? known device id ?
init sequence =? superio-enter sequence ?

>
>
>  > my stall points - (fishing for answers;)
>  >
>  > - enter / exit should be tracked by superio-locks, in flags bit.
>  > (recent idea, seems sound)
>  > - enter / exit should be vaguely discouraged in kernel-doc - due to
>  > expectations of isadump etc.
>

What I meant here was that we should discourage use of superio-exit(),
leaving the port enabled for normal operations.  ISTM that this is the same
effect as the "universal exit sequence" - ie a harmless sequence of bytes
that does NOT disable the port, leaving it working for anyone, and
not interfering with drivers which resend the superio_enter() sequence.

IOW - if we send enter_sequence when registering port, and never send
exit-sequence,
we should get the current working behavior.



>
> Which expectations?

Mostly that isadump continues to work - if drivers are aggressive with
enter/exit sequences, we're more likely to leave the port in a
disabled state, and isadump
and friends would need correct enter-sequences to re-enable.

I'll take a peek at coreboot RSN.  thanks for that link.

User-space should stay away from I/O ports the
>  kernel has requested. There's little point in trying to sort out
>  concurrent accesses which simply shouldn't happen in the first place.
>
>






>  > I put it here for proximity to the center-of-mass of the drivers I found.
>
>
> That's not necessarily a bad initial location. We can have superio live
>  in drivers/hwmon at first, and once all hwmon drivers use it, move it
>  to a neutral ground and convert the other drivers in the kernel tree
>  (mainly watchdog drivers I think.)
>

Good - Ill stick with it for now, and move if/when appropriate.


>  > Jean, you raised the idea of a sysfs interface.
>  > Does the October patchset look to fit with what youre thinking ?
>
>
> I don't think your patch offered a sysfs interface? I'm not really
>  thinking of anything in particular anyway.

Thats correct - it did not.    Im just trying to use the idea as
a probe to see what the patchset might have missed.
Following comments are intended for consideration in an additional patch..


 We just want a way for
>  sensors-detect to identify supported Super-I/O chips without actually
>  accessing the I/O ports if a kernel driver already requested them.
>
>
>  > - the most simple I can think of :
>  > --expose the kzalloced array of superio-fields as readonly attributes.
>  > --values go non-zero when superio-port is registered by some user-driver
>  > --it would be nice to see client-drivers list on each slot, but that
>  > involves more code and mem
>
>
> What we really need for sensors-detect is simply the value of registers
>  0x20 and 0x21, and values of registers 0x30, 0x60 and 0x61 for a
>  selected logical device. In what form we export these, needs to be
>  discussed.
>

Ok - those are familiar addys - I'll come up with candidate names for
these attrs.

I note you left out DevID, is that an accident ?
ISTM that it would be needed for a user-space sensor-module loader,
which would know device ids supported by drivers.

Also, this implies that Locks-Coordinator should actively scan for
common superio-ports (based upon superio_locks.scan_on_load=1), an



>  > does anyone think s/superio/sio/g is appropriate ?
>
>
> No. This was discussed before and I have always objected to the name
>  "sio". It's too short and could mean about anything (starting with
>  "synchronous I/O", as opposed to "asynchronous I/O" which is very often
>  spelled AIO.)
>

ACK.  I'll not ask again (I dont recall that I did, but thats irrelevant)


>
> Jean Delvare
>

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] superio lock coordinator

[Jean Delvare]

Hi Jim,

On Tue, 15 Jul 2008 17:04:57 -0600, Jim Cromie wrote:
> On 7/15/08, Jean Delvare <khali@linux-fr.org> wrote:
> > Please read my reply to David's concern, and why having a per-chip exit
> >  sequence doesn't make that much sense. I would use the same exit
> >  function as isadump and sensors-detect have been using for months, as
> >  this appears to work fine.
> 
> OK, quoting
> 
> As I recall, the rationale for the code above is that you do not
> necessarily recognize a known Super-I/O chip after the init sequence.
> As different chips have different exit sequences, there are definitely
> cases where you just don't know what to do. So we need a universal exit
> sequence, which is the code above. And once you have that and it
> appears to work with all known chips, there's simply no reason to not
> use it for all chips even when we know their exit sequence.
> 
> I dont understand what you mean by :
> "not necessarily recognize a known Super-I/O chip after the init sequence"
> 
> recognize =? known device id ?

Yes.

> init sequence =? superio-enter sequence ?

Yes.

To take a concrete example: say that you issue the Winbond Super-I/O
enter sequence and it works. You know that you probably have some
Winbond chip (but you're not even sure). You check the device ID and it
doesn't match anything you know. Now you want to issue the exit
sequence. You can use the one of the W83627HF, or you can use the one
of the W83627EHF. Which one will you use, given that you have no idea
what chip you actually have?

This is the reason why sensors-detect etc. don't use per-chip exit
sequenced.

> (...)
> What I meant here was that we should discourage use of superio-exit(),
> leaving the port enabled for normal operations.  ISTM that this is the same
> effect as the "universal exit sequence" - ie a harmless sequence of bytes
> that does NOT disable the port, leaving it working for anyone, and
> not interfering with drivers which resend the superio_enter() sequence.
>
> IOW - if we send enter_sequence when registering port, and never send
> exit-sequence, we should get the current working behavior.

That's not OK. For one thing, some chips stop working while in
configuration mode. For another (probably related), some chips leave
configuration mode on their own after a number of configuration
register reads and/or period of time (that's why isadump reissues the
enter sequence every 16 reads or so). So you don't want to leave the
chip in configuration mode, and in some cases you couldn't even if you
wanted to.

In general, I strongly suggest that you look at what sensors-detect and
isadump are doing. These tools have been in use for years, we have
improved them over time, and I can't remember any problem report
related to Super-I/O.

> > Which expectations?
> 
> Mostly that isadump continues to work - if drivers are aggressive with
> enter/exit sequences, we're more likely to leave the port in a
> disabled state, and isadump
> and friends would need correct enter-sequences to re-enable.

isadump always had to issue the correct enter-sequence, we have an
option for that (-k). There's no reason for this to change (except for
the fact that it is considered bad practice to access I/O ports from
user-space that have been requested by a kernel driver.)

> > (...)
> > What we really need for sensors-detect is simply the value of registers
> >  0x20 and 0x21, and values of registers 0x30, 0x60 and 0x61 for a
> >  selected logical device. In what form we export these, needs to be
> >  discussed.
> 
> Ok - those are familiar addys - I'll come up with candidate names for
> these attrs.
> 
> I note you left out DevID, is that an accident ?
> ISTM that it would be needed for a user-space sensor-module loader,
> which would know device ids supported by drivers.

DevID is 0x20, I didn't leave it out. Or do you mean something else?

> Also, this implies that Locks-Coordinator should actively scan for
> common superio-ports (based upon superio_locks.scan_on_load=1), an

If we want hwmon drivers to load automatically, it will have to anyway.
I would even make it the default.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] superio lock coordinator

[Jean Delvare]

Hi David,

On Tue, 15 Jul 2008 09:23:23 -0600, David Hubbard wrote:
> On Tue, Jul 15, 2008 at 4:40 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > What we really need for sensors-detect is simply the value of registers
> > 0x20 and 0x21, and values of registers 0x30, 0x60 and 0x61 for a
> > selected logical device. In what form we export these, needs to be
> > discussed.
> 
> Should superio-locks enumerate the available logical device ID's,
> perhaps as a sysfs interface?

I don't have anything precise in mind, I admit. All I can say is that
it needs to be extensible, in case we need to export more register
values in the future.

> >> David,
> >> do you have a preference wrt fn-ptr ? and if so, a single fn with 1 arg
> >> :  0,1,2 for enter/exit/query ?
> >> do you have any use for a flag bit ?  is there generality in it ?
> 
> I think we could just use the same enter and exit sequences for
> everything, and not make it customizable. Does that sound OK?

No. We can have a universal exit sequence, but enter sequences are
per-chip (or actually per chip family). That's even one part of the
device identification (lets us differentiate between two chips with the
same device ID register value.)

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] superio lock coordinator

[David Hubbard]

Hi Jim,

On Wed, Jul 16, 2008 at 1:25 AM, Jean Delvare <khali@linux-fr.org> wrote:
>> >> David,
>> >> do you have a preference wrt fn-ptr ? and if so, a single fn with 1 arg
>> >> :  0,1,2 for enter/exit/query ?
>> >> do you have any use for a flag bit ?  is there generality in it ?
>>
>> I think we could just use the same enter and exit sequences for
>> everything, and not make it customizable. Does that sound OK?
>
> No. We can have a universal exit sequence, but enter sequences are
> per-chip (or actually per chip family). That's even one part of the
> device identification (lets us differentiate between two chips with the
> same device ID register value.)

I think a fn-ptr would be fine. The function could take as an arg
enter/exit/query (0, 1, 2) and a pointer to a byte array with offset,
value, offset, value pairs. So I see the function having 3 args:

void (* fn-ptr)(int which, char * sequence, int sequence_max_length);

David

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] superio lock coordinator

[Jean Delvare]

On Wed, 16 Jul 2008 09:44:02 -0600, David Hubbard wrote:
> Hi Jim,
> 
> On Wed, Jul 16, 2008 at 1:25 AM, Jean Delvare <khali@linux-fr.org> wrote:
> >> >> David,
> >> >> do you have a preference wrt fn-ptr ? and if so, a single fn with 1 arg
> >> >> :  0,1,2 for enter/exit/query ?
> >> >> do you have any use for a flag bit ?  is there generality in it ?
> >>
> >> I think we could just use the same enter and exit sequences for
> >> everything, and not make it customizable. Does that sound OK?
> >
> > No. We can have a universal exit sequence, but enter sequences are
> > per-chip (or actually per chip family). That's even one part of the
> > device identification (lets us differentiate between two chips with the
> > same device ID register value.)
> 
> I think a fn-ptr would be fine. The function could take as an arg
> enter/exit/query (0, 1, 2) and a pointer to a byte array with offset,
> value, offset, value pairs. So I see the function having 3 args:
> 
> void (* fn-ptr)(int which, char * sequence, int sequence_max_length);

I'm lost. I thought that we had just agreed that the exit sequence
would be universal? Also, what is the "query" sequence supposed to be?

Then I don't get the prototype of the function. Who would be calling
it? And who would provide the parameters? The enter sequence is a set
of number being written to a port. It is a per-chip thing (or
per-family) thing. So there are 2 possibilities:

* For each known chip or family, you store the enter sequence in an
  array, and the superio driver iterates over it. This is the approach
  taken in both isadump and sensors-detect.

OR

* For each known chip or family, you store a function, which the
  superio driver calls. The prototype of such a function would be
  something like:
  void (*enter)(int index_port, int data_port);
  This is more flexible, but at this point I have no reason to
  believe that this is needed.

Hmmm, I thought I had said I would let someone else work on this...

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] superio lock coordinator

[David Hubbard]

Hi Jean,

On Wed, Jul 16, 2008 at 10:51 AM, Jean Delvare <khali@linux-fr.org> wrote:
>> I think a fn-ptr would be fine. The function could take as an arg
>> enter/exit/query (0, 1, 2) and a pointer to a byte array with offset,
>> value, offset, value pairs. So I see the function having 3 args:
>>
>> void (* fn-ptr)(int which, char * sequence, int sequence_max_length);
>
> I'm lost. I thought that we had just agreed that the exit sequence
> would be universal? Also, what is the "query" sequence supposed to be?
>
> Then I don't get the prototype of the function. Who would be calling
> it? And who would provide the parameters? The enter sequence is a set
> of number being written to a port. It is a per-chip thing (or
> per-family) thing. So there are 2 possibilities:
>
> * For each known chip or family, you store the enter sequence in an
>  array, and the superio driver iterates over it. This is the approach
>  taken in both isadump and sensors-detect.
>
> OR
>
> * For each known chip or family, you store a function, which the
>  superio driver calls. The prototype of such a function would be
>  something like:
>  void (*enter)(int index_port, int data_port);
>  This is more flexible, but at this point I have no reason to
>  believe that this is needed.
>
> Hmmm, I thought I had said I would let someone else work on this...

Sorry, I'm just being slow. I don't know what the "query" sequence is
supposed to be either, but just merged it straight from Jim's email.

I agree, just an enter sequence in an array which the superio driver
iterates over - that is all we need.

David

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] superio lock coordinator

[David Hubbard]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

Hi Jim, Jean,

Attached is a patch (I'm renaming it to .txt to get the right
mime-type). It's only compile-tested. Main questions I have are:

- the code in superio.c is supposed to lead up to
static int __devinit w83627ehf_probe(struct platform_device *pdev)

Did I do that right? Any obvious errors?

- I'm using a fixed-size array devlist[] in superio.c. I would use a
LIST_HEAD() but I haven't had time yet.

- It's only compile-tested. Assuming there aren't blatant design
mistakes, I might even try running it. ;-)

I appreciate any (brief) review you can afford to do...

Cheers,
David

[-- Attachment #2: w83627ehf-use-superio-v01.patch.txt --]
[-- Type: text/plain, Size: 19676 bytes --]

diff -Naur old/superio.c new/superio.c
--- old/superio.c	1969-12-31 17:00:00.000000000 -0700
+++ new/superio.c	2008-07-18 15:10:54.000000000 -0600
@@ -0,0 +1,251 @@
+/*
+	superio - Driver for Super-I/O chips
+
+	Copyright (C) 2008  David Hubbard <david.c.hubbard@gmail.com>
+
+	This driver is part of lm-sensors. http://www.lm-sensors.org
+
+	This program is free software; you can redistribute it and/or modify
+	it under the terms of the GNU General Public License as published by
+	the Free Software Foundation; either version 2 of the License, or
+	(at your option) any later version.
+
+	This program is distributed in the hope that it will be useful,
+	but WITHOUT ANY WARRANTY; without even the implied warranty of
+	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+	GNU General Public License for more details.
+
+	You should have received a copy of the GNU General Public License
+	along with this program; if not, write to the Free Software
+	Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+
+*/
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <asm/io.h>	/* inb() outb() */
+#include "superio.h"
+
+/* DRVNAME is used in the driver core and sysfs to identify this driver */
+#define DRVNAME "superio"
+
+/* Table of recognized devices */
+struct superio_recognize_device
+{
+	u16		devid, devid_mask;
+	const char *	driver_name;
+};
+
+static struct superio_recognize_device recognize[] = {
+	{ 0x8850, 0xfff0, "w83627ehf" },	/* w83627ehf */
+	{ 0x8860, 0xfff0, "w83627ehf" },	/* w83627ehg */
+	{ 0xa020, 0xfff0, "w83627ehf" },	/* w83627dhg */
+};
+
+
+/* superio I/O port access */
+#define SUPERIO_IOREGION_LENGTH	2
+
+void superio_outb(int ioreg, int reg, int val)
+{
+	outb(reg, ioreg);
+	outb(val, ioreg + 1);
+}
+
+int superio_inb(int ioreg, int reg)
+{
+	outb(reg, ioreg);
+	return inb(ioreg + 1);
+}
+
+void superio_select(int ioreg, int ld)
+{
+	outb(SUPERIO_REG_LDSEL, ioreg);
+	outb(ld, ioreg + 1);
+}
+
+void superio_enter(int ioreg)
+{
+	outb(0x87, ioreg);
+	outb(0x87, ioreg);
+}
+
+void superio_exit(int ioreg)
+{
+	outb(0x02, ioreg);
+	outb(0x02, ioreg + 1);
+}
+
+
+#if 0
+static int __devinit superio_probe(struct platform_device * pdev)
+{
+	/* the driver that knows more about this Super-I/O can now probe() */
+	return 0;
+}
+
+static int __devexit superio_remove(struct platform_device * pdev)
+{
+	return 0;
+}
+#endif
+
+static struct platform_driver superio_driver = {
+	/*.probe	= superio_probe,*/
+	/*.remove	= __devexit_p(superio_remove),*/
+	.driver	= {
+		.name	= DRVNAME,
+		.owner	= THIS_MODULE,
+	},
+};
+
+#define SUPERIO_MAXDEV 2
+static struct platform_device * devlist[SUPERIO_MAXDEV];
+static unsigned devlist_num = 0;
+
+/* module_param force: useful for an unrecognized device, because generic
+ * registers will be available in sysfs for inspection
+ * (e.g. for isadump and sensors-detect) */
+static ushort force;
+module_param(force, ushort, 0);
+MODULE_PARM_DESC(force, "Force to load with unrecognized device ID");
+
+/* module init: request region and see if a Super-I/O device lives here */
+static int __init superio_find_at(int addr)
+{
+	u16 val;
+	struct resource res;
+	struct platform_device * pdev;
+	const char * recognized_name = 0;
+	int err, i;
+
+	if (!request_region(addr, SUPERIO_IOREGION_LENGTH, DRVNAME)) {
+		printk(KERN_ERR DRVNAME ": region request 0x%x-0x%x failed\n",
+			addr, addr + SUPERIO_IOREGION_LENGTH - 1);
+		err = -EBUSY;
+		goto exit;
+	}
+
+	/* see if a Super-I/O device lives here */
+	superio_enter(addr);
+	val = (superio_inb(addr, SUPERIO_REG_DEVID) << 8)
+		| superio_inb(addr, SUPERIO_REG_DEVID + 1);
+
+	for (i = 0; i < ARRAY_SIZE(recognize); i++) {
+		if ((val & recognize[i].devid_mask) == recognize[i].devid) {
+			recognized_name = recognize[i].driver_name;
+			pr_info(DRVNAME ": Found %s\n", recognized_name);
+			break;
+		}
+	}
+
+	superio_exit(addr);
+
+	if (!recognized_name) {
+		if (val != 0xffff) {
+			pr_debug(DRVNAME ": unsupported superio ID: 0x%04x\n",
+				val);
+			if (/*module_param*/ force) recognized_name = DRVNAME;
+		}
+		else pr_info(DRVNAME ": no superio found (ffff)\n");
+
+		if (!recognized_name) {
+			err = -ENODEV;
+			goto exit_release;
+		}
+	}
+
+	if (devlist_num >= SUPERIO_MAXDEV) {
+		printk(KERN_ERR DRVNAME ": too many superio devices (%d)\n",
+			devlist_num + 1);
+		err = -ENOMEM;
+		goto exit_release;
+	}
+
+	/* create/register platform device for the new Super-I/O device */
+	memset(&res, 0, sizeof(res));
+	res.name = recognized_name;
+	res.start = addr;
+	res.end = addr + SUPERIO_IOREGION_LENGTH - 1;
+	res.flags = IORESOURCE_IO;
+	pdev = platform_device_register_simple(recognized_name, addr, &res, 1);
+	if (IS_ERR(pdev)) {
+		printk(KERN_ERR DRVNAME ": device registration failed (%ld)\n",
+			PTR_ERR(pdev));
+		err = (int) (PTR_ERR(pdev));
+		goto exit_release;
+	}
+
+	devlist[devlist_num] = pdev;
+	devlist_num++;
+	return 0;
+
+exit_release:
+	release_region(addr, SUPERIO_IOREGION_LENGTH);
+exit:
+	return err;
+}
+
+/* module init function */
+static int __init superio_module_init(void)
+{
+	int err;
+
+	err = platform_driver_register(&superio_driver);
+	if (err)
+		goto exit;
+
+	/* request region for each of the possible Super-I/O ports, and
+	 * probe the port to see if a Super-I/O device lives there.
+	 * If it fails with -ENODEV, look elsewhere. If it fails with
+	 * -EBUSY, look elsewhere (the region is reserved, that's all). */
+
+	err = superio_find_at(0x2e);
+	if (err != 0 && err != -ENODEV && err != -EBUSY)
+		goto exit_unregister;
+
+	if (err != 0) {
+		err = superio_find_at(0x4e);
+		if (err != 0)
+			goto exit_unregister;
+
+		/* superio_find_at(0x4e) == 0, so we are OK */
+
+	} else {
+		err = superio_find_at(0x4e);
+		if (err != 0 && err != -ENODEV && err != -EBUSY)
+			goto exit_unregister;
+
+		/* superio_find_at(0x2e) == 0, so we are OK */
+
+	}
+
+	return 0;
+
+exit_unregister:
+	platform_driver_unregister(&superio_driver);
+exit:
+	return err;
+}
+
+/* module exit function */
+static void __exit superio_module_exit(void)
+{
+	unsigned i;
+
+	for (i = 0; i < devlist_num; i++) {
+		platform_device_unregister(devlist[i]);
+	}
+
+	platform_driver_unregister(&superio_driver);
+}
+
+MODULE_AUTHOR("David Hubbard <david.c.hubbard@gmail.com>");
+MODULE_DESCRIPTION("Super I/O driver");
+MODULE_LICENSE("GPL");
+
+module_init(superio_module_init);
+module_exit(superio_module_exit);
diff -Naur old/superio.h new/superio.h
--- old/superio.h	1969-12-31 17:00:00.000000000 -0700
+++ new/superio.h	2008-07-18 15:10:54.000000000 -0600
@@ -0,0 +1,13 @@
+/* These superio registers are exported for all superio devices */
+/* The device will have more superio registers that these */
+
+#define SUPERIO_REG_LDSEL	0x07	/* Logical device select */
+#define SUPERIO_REG_DEVID	0x20	/* Device ID (2 bytes) */
+#define SUPERIO_REG_ENABLE	0x30	/* Logical device enable */
+#define SUPERIO_REG_ADDR	0x60	/* Logical device address (2 bytes) */
+
+void superio_outb(int ioreg, int reg, int val);
+int superio_inb(int ioreg, int reg);
+void superio_select(int ioreg, int ld);
+void superio_enter(int ioreg);
+void superio_exit(int ioreg);
diff -Naur old/w83627ehf.c new/w83627ehf.c
--- old/w83627ehf.c	2008-07-18 11:04:15.000000000 -0600
+++ new/w83627ehf.c	2008-07-18 15:10:54.000000000 -0600
@@ -48,17 +48,12 @@
 #include <linux/hwmon-vid.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
-#include <asm/io.h>
-#include "lm75.h"
+#include <asm/io.h>	/* inb() outb() */
+#include "lm75.h"	/* for LM75_TEMP_TO_REG / FROM_REG */
+#include "superio.h"
 
 enum kinds { w83627ehf, w83627dhg };
 
-/* used to set data->name = w83627ehf_device_names[data->sio_kind] */
-static const char * w83627ehf_device_names[] = {
-	"w83627ehf",
-	"w83627dhg",
-};
-
 static unsigned short force_id;
 module_param(force_id, ushort, 0);
 MODULE_PARM_DESC(force_id, "Override the detected device ID");
@@ -71,54 +66,15 @@
 
 #define W83627EHF_LD_HWM	0x0b
 
-#define SIO_REG_LDSEL		0x07	/* Logical device select */
-#define SIO_REG_DEVID		0x20	/* Device ID (2 bytes) */
-#define SIO_REG_EN_VRM10	0x2C	/* GPIO3, GPIO4 selection */
-#define SIO_REG_ENABLE		0x30	/* Logical device enable */
-#define SIO_REG_ADDR		0x60	/* Logical device address (2 bytes) */
-#define SIO_REG_VID_CTRL	0xF0	/* VID control */
-#define SIO_REG_VID_DATA	0xF1	/* VID data */
+#define W83627EHF_SIOREG_EN_VRM10	0x2C	/* GPIO3, GPIO4 selection */
+#define W83627EHF_SIOREG_VID_CTRL	0xF0	/* VID control */
+#define W83627EHF_SIOREG_VID_DATA	0xF1	/* VID data */
 
 #define SIO_W83627EHF_ID	0x8850
 #define SIO_W83627EHG_ID	0x8860
 #define SIO_W83627DHG_ID	0xa020
 #define SIO_ID_MASK		0xFFF0
 
-static inline void
-superio_outb(int ioreg, int reg, int val)
-{
-	outb(reg, ioreg);
-	outb(val, ioreg + 1);
-}
-
-static inline int
-superio_inb(int ioreg, int reg)
-{
-	outb(reg, ioreg);
-	return inb(ioreg + 1);
-}
-
-static inline void
-superio_select(int ioreg, int ld)
-{
-	outb(SIO_REG_LDSEL, ioreg);
-	outb(ld, ioreg + 1);
-}
-
-static inline void
-superio_enter(int ioreg)
-{
-	outb(0x87, ioreg);
-	outb(0x87, ioreg);
-}
-
-static inline void
-superio_exit(int ioreg)
-{
-	outb(0x02, ioreg);
-	outb(0x02, ioreg + 1);
-}
-
 /*
  * ISA constants
  */
@@ -299,11 +255,6 @@
 	u8 vrm;
 };
 
-struct w83627ehf_sio_data {
-	int sioreg;
-	enum kinds kind;
-};
-
 static inline int is_word_sized(u16 reg)
 {
 	return (((reg & 0xff00) == 0x100
@@ -1245,70 +1196,139 @@
 static int __devinit w83627ehf_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct w83627ehf_sio_data *sio_data = dev->platform_data;
 	struct w83627ehf_data *data;
-	struct resource *res;
+	struct resource * siores, w_res;
 	u8 fan4pin, fan5pin, en_vrm10;
-	int i, err = 0;
+	int i, sioaddr, err = 0;
+	enum kinds kind;
 
-	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	if (!request_region(res->start, IOREGION_LENGTH, DRVNAME)) {
-		err = -EBUSY;
-		dev_err(dev, "Failed to request region 0x%lx-0x%lx\n",
-			(unsigned long)res->start,
-			(unsigned long)res->start + IOREGION_LENGTH - 1);
+	siores = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!siores) {
+		err = -EINVAL;
+		dev_err(dev, "missing superio address\n");
 		goto exit;
 	}
+	sioaddr = siores->start;
+
+	/* We have a known chip, find the HWM I/O address */
 
 	if (!(data = kzalloc(sizeof(struct w83627ehf_data), GFP_KERNEL))) {
 		err = -ENOMEM;
+		goto exit;
+	}
+
+	superio_enter(sioaddr);
+	superio_select(sioaddr, W83627EHF_LD_HWM);
+	data->addr = (superio_inb(sioaddr, SUPERIO_REG_ADDR) << 8)
+	    | superio_inb(sioaddr, SUPERIO_REG_ADDR + 1);
+	data->addr &= IOREGION_ALIGNMENT;
+	if (data->addr == 0) {
+		dev_err(dev, " Refusing to enable a Super-I/O "
+		       "device with a base I/O port 0.\n");
+		superio_exit(sioaddr);
+		err = -ENODEV;
+		goto exit_free;
+	}
+
+	/* Activate logical device if needed */
+	i = superio_inb(sioaddr, SUPERIO_REG_ENABLE);
+	if (!(i & 0x01)) {
+		dev_warn(dev, "Forcibly enabling Super-I/O. "
+		       "Sensor is probably unusable.\n");
+		superio_outb(sioaddr, SUPERIO_REG_ENABLE, i | 0x01);
+	}
+
+	/* add another resource at the now-found w83627ehf addr */
+	w_res.name = DRVNAME;
+	w_res.start = data->addr + IOREGION_OFFSET;
+	w_res.end = data->addr + IOREGION_OFFSET + IOREGION_LENGTH - 1;
+	w_res.flags = IORESOURCE_IO;
+
+	if (!request_region(w_res.start, IOREGION_LENGTH, DRVNAME)) {
+		err = -EBUSY;
+		superio_exit(sioaddr);
+		dev_err(dev, "Failed to request region 0x%lx-0x%lx\n",
+			(unsigned long) w_res.start,
+			(unsigned long) w_res.start + IOREGION_LENGTH - 1);
+		goto exit_free;
+	}
+
+	err = platform_device_add_resources(pdev, &w_res, 1);
+	if (err) {
+		superio_exit(sioaddr);
+		dev_err(dev, "add resource to device failed (%d)\n", err);
 		goto exit_release;
 	}
 
-	data->addr = res->start;
+	/* auto-detect the type of chip (ehf, ehg, or dhg) - unless forced */
+	if (force_id)
+		i = force_id;
+	else
+		i = (superio_inb(sioaddr, SUPERIO_REG_DEVID) << 8)
+		    | superio_inb(sioaddr, SUPERIO_REG_DEVID + 1);
+	switch (i & SIO_ID_MASK) {
+	case SIO_W83627EHF_ID:
+		kind = w83627ehf;
+		data->name = "w83627ehf";
+		break;
+	case SIO_W83627EHG_ID:
+		kind = w83627ehf;
+		data->name = "w83627ehg";
+		break;
+	case SIO_W83627DHG_ID:
+		kind = w83627dhg;
+		data->name = "w83627dhg";
+		break;
+	default:
+		err = -ENODEV;
+		superio_exit(sioaddr);
+		dev_err(dev, "unsupported chip ID: 0x%04x\n", i);
+		goto exit_release;
+	}
+
+	pr_info(DRVNAME ": found a %s at %#x\n", data->name, data->addr);
 	mutex_init(&data->lock);
 	mutex_init(&data->update_lock);
-	data->name = w83627ehf_device_names[sio_data->kind];
 	platform_set_drvdata(pdev, data);
 
 	/* 627EHG and 627EHF have 10 voltage inputs; DHG has 9 */
-	data->in_num = (sio_data->kind == w83627dhg) ? 9 : 10;
+	data->in_num = (kind == w83627dhg) ? 9 : 10;
 
 	/* Initialize the chip */
 	w83627ehf_init_device(data);
 
 	data->vrm = vid_which_vrm();
-	superio_enter(sio_data->sioreg);
 	/* Read VID value */
-	superio_select(sio_data->sioreg, W83627EHF_LD_HWM);
-	if (superio_inb(sio_data->sioreg, SIO_REG_VID_CTRL) & 0x80) {
+	if (superio_inb(sioaddr, W83627EHF_SIOREG_VID_CTRL) & 0x80) {
 		/* Set VID input sensibility if needed. In theory the BIOS
 		   should have set it, but in practice it's not always the
 		   case. We only do it for the W83627EHF/EHG because the
 		   W83627DHG is more complex in this respect. */
-		if (sio_data->kind == w83627ehf) {
-			en_vrm10 = superio_inb(sio_data->sioreg,
-					       SIO_REG_EN_VRM10);
+		if (kind == w83627ehf) {
+			en_vrm10 = superio_inb(sioaddr,
+				W83627EHF_SIOREG_EN_VRM10);
 			if ((en_vrm10 & 0x08) && data->vrm == 90) {
 				dev_warn(dev, "Setting VID input voltage to "
 					 "TTL\n");
-				superio_outb(sio_data->sioreg, SIO_REG_EN_VRM10,
+				superio_outb(sioaddr, W83627EHF_SIOREG_EN_VRM10,
 					     en_vrm10 & ~0x08);
 			} else if (!(en_vrm10 & 0x08) && data->vrm == 100) {
 				dev_warn(dev, "Setting VID input voltage to "
 					 "VRM10\n");
-				superio_outb(sio_data->sioreg, SIO_REG_EN_VRM10,
+				superio_outb(sioaddr, W83627EHF_SIOREG_EN_VRM10,
 					     en_vrm10 | 0x08);
 			}
 		}
 
-		data->vid = superio_inb(sio_data->sioreg, SIO_REG_VID_DATA);
-		if (sio_data->kind == w83627ehf) /* 6 VID pins only */
+		data->vid = superio_inb(sioaddr, W83627EHF_SIOREG_VID_DATA);
+		if (kind == w83627ehf) /* 6 VID pins only */
 			data->vid &= 0x3f;
 
 		err = device_create_file(dev, &dev_attr_cpu0_vid);
-		if (err)
+		if (err) {
+			superio_exit(sioaddr);
 			goto exit_release;
+		}
 	} else {
 		dev_info(dev, "VID pins in output mode, CPU VID not "
 			 "available\n");
@@ -1316,9 +1336,9 @@
 
 	/* fan4 and fan5 share some pins with the GPIO and serial flash */
 
-	fan5pin = superio_inb(sio_data->sioreg, 0x24) & 0x2;
-	fan4pin = superio_inb(sio_data->sioreg, 0x29) & 0x6;
-	superio_exit(sio_data->sioreg);
+	fan5pin = superio_inb(sioaddr, 0x24) & 0x2;
+	fan4pin = superio_inb(sioaddr, 0x29) & 0x6;
+	superio_exit(sioaddr);
 
 	/* It looks like fan4 and fan5 pins can be alternatively used
 	   as fan on/off switches, but fan5 control is write only :/
@@ -1339,14 +1359,14 @@
 	/* Register sysfs hooks */
   	for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays); i++)
 		if ((err = device_create_file(dev,
-			&sda_sf3_arrays[i].dev_attr)))
+				&sda_sf3_arrays[i].dev_attr)))
 			goto exit_remove;
 
 	/* if fan4 is enabled create the sf3 files for it */
 	if (data->has_fan & (1 << 3))
 		for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++) {
 			if ((err = device_create_file(dev,
-				&sda_sf3_arrays_fan4[i].dev_attr)))
+					&sda_sf3_arrays_fan4[i].dev_attr)))
 				goto exit_remove;
 		}
 
@@ -1404,10 +1424,11 @@
 
 exit_remove:
 	w83627ehf_device_remove_files(dev);
-	kfree(data);
 	platform_set_drvdata(pdev, NULL);
 exit_release:
-	release_region(res->start, IOREGION_LENGTH);
+	release_region(w_res.start, IOREGION_LENGTH);
+exit_free:
+	kfree(data);
 exit:
 	return err;
 }
@@ -1434,6 +1455,7 @@
 	.remove		= __devexit_p(w83627ehf_remove),
 };
 
+#if 0
 /* w83627ehf_find() looks for a '627 in the Super-I/O config space */
 static int __init w83627ehf_find(int sioaddr, unsigned short *addr,
 				 struct w83627ehf_sio_data *sio_data)
@@ -1447,54 +1469,9 @@
 
 	superio_enter(sioaddr);
 
-	if (force_id)
-		val = force_id;
-	else
-		val = (superio_inb(sioaddr, SIO_REG_DEVID) << 8)
-		    | superio_inb(sioaddr, SIO_REG_DEVID + 1);
-	switch (val & SIO_ID_MASK) {
-	case SIO_W83627EHF_ID:
-		sio_data->kind = w83627ehf;
-		sio_name = sio_name_W83627EHF;
-		break;
-	case SIO_W83627EHG_ID:
-		sio_data->kind = w83627ehf;
-		sio_name = sio_name_W83627EHG;
-		break;
-	case SIO_W83627DHG_ID:
-		sio_data->kind = w83627dhg;
-		sio_name = sio_name_W83627DHG;
-		break;
-	default:
-		if (val != 0xffff)
-			pr_debug(DRVNAME ": unsupported chip ID: 0x%04x\n",
-				 val);
-		superio_exit(sioaddr);
-		return -ENODEV;
-	}
-
-	/* We have a known chip, find the HWM I/O address */
-	superio_select(sioaddr, W83627EHF_LD_HWM);
-	val = (superio_inb(sioaddr, SIO_REG_ADDR) << 8)
-	    | superio_inb(sioaddr, SIO_REG_ADDR + 1);
-	*addr = val & IOREGION_ALIGNMENT;
-	if (*addr == 0) {
-		printk(KERN_ERR DRVNAME ": Refusing to enable a Super-I/O "
-		       "device with a base I/O port 0.\n");
-		superio_exit(sioaddr);
-		return -ENODEV;
-	}
-
-	/* Activate logical device if needed */
-	val = superio_inb(sioaddr, SIO_REG_ENABLE);
-	if (!(val & 0x01)) {
-		printk(KERN_WARNING DRVNAME ": Forcibly enabling Super-I/O. "
-		       "Sensor is probably unusable.\n");
-		superio_outb(sioaddr, SIO_REG_ENABLE, val | 0x01);
-	}
 
 	superio_exit(sioaddr);
-	pr_info(DRVNAME ": Found %s chip at %#x\n", sio_name, *addr);
+
 	sio_data->sioreg = sioaddr;
 
 	return 0;
@@ -1505,33 +1482,25 @@
  * track of the w83627ehf driver. But since we platform_device_alloc(), we
  * must keep track of the device */
 static struct platform_device *pdev;
+#endif
 
 static int __init sensors_w83627ehf_init(void)
 {
-	int err;
-	unsigned short address;
-	struct resource res;
-	struct w83627ehf_sio_data sio_data;
-
-	/* initialize sio_data->kind and sio_data->sioreg.
-	 *
-	 * when Super-I/O functions move to a separate file, the Super-I/O
-	 * driver will probe 0x2e and 0x4e and auto-detect the presence of a
-	 * w83627ehf hardware monitor, and call probe() */
-	if (w83627ehf_find(0x2e, &address, &sio_data) &&
-	    w83627ehf_find(0x4e, &address, &sio_data))
-		return -ENODEV;
-
-	err = platform_driver_register(&w83627ehf_driver);
-	if (err)
-		goto exit;
+	return platform_driver_register(&w83627ehf_driver);
 
+#if 0
 	if (!(pdev = platform_device_alloc(DRVNAME, address))) {
 		err = -ENOMEM;
 		printk(KERN_ERR DRVNAME ": Device allocation failed\n");
 		goto exit_unregister;
 	}
 
+	/* initialize sio_data->kind and sio_data->sioreg.
+	 * struct w83627ehf_sio_data sio_data;
+	 *
+	 * when Super-I/O functions move to a separate file, the Super-I/O
+	 * driver will probe 0x2e and 0x4e and auto-detect the presence of a
+	 * w83627ehf hardware monitor, and call probe()
 	err = platform_device_add_data(pdev, &sio_data,
 				       sizeof(struct w83627ehf_sio_data));
 	if (err) {
@@ -1550,6 +1519,7 @@
 		       "(%d)\n", err);
 		goto exit_device_put;
 	}
+	*/
 
 	/* platform_device_add calls probe() */
 	err = platform_device_add(pdev);
@@ -1567,11 +1537,13 @@
 	platform_driver_unregister(&w83627ehf_driver);
 exit:
 	return err;
+#endif
+
 }
 
 static void __exit sensors_w83627ehf_exit(void)
 {
-	platform_device_unregister(pdev);
+	/*platform_device_unregister(pdev);*/
 	platform_driver_unregister(&w83627ehf_driver);
 }
 

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] superio lock coordinator

[David Hubbard]

[-- Attachment #1: Type: text/plain, Size: 2397 bytes --]

Hi Jim, Hans,

> Hi David,
>
> heres my git-log -p  for your patch, and several others layering on
> trivial fixes.
>
> Maybe its useful  ( maybe it saves you a tiny bit of time )
> Or maybe not.
>
> have fun
> -jimc

At last, I think I've made it to a point where I can ask for your help.

Attached is a patch that works (sort of!) on my w83627ehf. (.txt is so
the content-type is right.)

I *think* this is what is happening:

Before, w83627ehf would call platform_device_alloc when it probed the
superio registers. That's a primitive approach (discussed in
Documentation/driver-model/platform.txt).

Well, I've pulled the two steps apart. In superio.c, it looks at
superio registers, finds the superio ID, and then creates a
platform_device with a driver name that pulls in w83627ehf. There's a
device table in superio.c, so Jim can add pc87360 and it should
automatically get modprobed also. (I'd be happy to port drivers, but
it could take a while.)

The driver core sees the platform device, autoloads w83627ehf.ko and
calls the probe() function in it. This part works. :-)

[ 6383.126757] superio: Found w83627ehf (0x8863) at 0x2e
[ 6383.126842] w83627ehf: found a w83627ehg at 0x290
[ 6383.127443] superio: no superio found at 0x4e

So something I'm not too sure about, but I think I did it right, is
superio.c:153
platform_device_put(pdev); /* now the device can exist on its own */

In theory, superio.c doesn't store the pdev pointer at all. It
releases its hold on it and leaves the rest to the driver core. But
when I remove the superio module:

[ 6475.442046] Trying to free nonexistent resource <0290-0291>
[ 6506.698974] superio: region request 0x2e-0x2f failed
[ 6506.698996] superio: no superio found at 0x4e

So Jim, Hans, can you take a look at how I'm doing platform device
code and make suggestions?

Jim, I'm fine with your suggestion, if (err) instead of if (err != 0).
I chose superio_in2b instead of superio_inw, because it is possible to
do a hardware 16-bit read with inw() which could have a different
result. After fumbling around a little, I added #ifdef/#define to
superio.h and then defined the functions superio_enter / superio_exit
etc. with "static inline". That should work OK for now.

Things TODO:
- Add mutexes (the whole point in the first place)
- Don't print out which superio ports had nothing, as long as at least
one port has something

Cheers!
David

[-- Attachment #2: w83627ehf-use-superio-v02.patch.txt --]
[-- Type: text/plain, Size: 21714 bytes --]

diff -Nuar old/Makefile new/Makefile
--- old/Makefile	2008-11-14 03:06:42.000000000 -0700
+++ new/Makefile	2008-11-14 03:06:31.000000000 -0700
@@ -73,6 +73,7 @@
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
 obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
 obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o
+obj-$(CONFIG_SENSORS_W83627EHF)	+= superio.o
 obj-$(CONFIG_SENSORS_W83627EHF)	+= w83627ehf.o
 obj-$(CONFIG_SENSORS_W83L785TS)	+= w83l785ts.o
 obj-$(CONFIG_SENSORS_W83L786NG)	+= w83l786ng.o
diff -Nuar old/superio.c new/superio.c
--- old/superio.c	1969-12-31 17:00:00.000000000 -0700
+++ new/superio.c	2008-11-14 03:17:13.000000000 -0700
@@ -0,0 +1,197 @@
+/*
+	superio - Driver for Super-I/O chips
+
+	Copyright (C) 2008  David Hubbard <david.c.hubbard@gmail.com>
+
+	This driver is part of lm-sensors. http://www.lm-sensors.org
+
+	This program is free software; you can redistribute it and/or modify
+	it under the terms of the GNU General Public License as published by
+	the Free Software Foundation; either version 2 of the License, or
+	(at your option) any later version.
+
+	This program is distributed in the hope that it will be useful,
+	but WITHOUT ANY WARRANTY; without even the implied warranty of
+	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+	GNU General Public License for more details.
+
+	You should have received a copy of the GNU General Public License
+	along with this program; if not, write to the Free Software
+	Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+
+*/
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <asm/io.h>	/* inb() outb() */
+#include "superio.h"
+
+/* DRVNAME is used in the driver core and sysfs to identify this driver */
+#define DRVNAME "superio"
+
+struct superio_recognize_device
+{
+	u16			devid, devid_mask;
+	const char *		driver_name;
+};
+
+static struct superio_recognize_device recognize[] = {
+	{ 0x8850, 0xfff0, /*w83627ehf*/ "w83627ehf", },
+	{ 0x8860, 0xfff0, /*w83627ehg*/ "w83627ehf", },
+	{ 0xa020, 0xfff0, /*w83627dhg*/ "w83627ehf", },
+};
+
+static struct platform_driver superio_driver = {
+	/*.probe	= superio_probe,*/
+	/*.remove	= __devexit_p(superio_remove),*/
+	.driver	= {
+		.name	= DRVNAME,
+		.owner	= THIS_MODULE,
+	},
+};
+
+/* superio I/O port region has 2 ports (write addr to 1st, r/w data from 2nd) */
+#define SUPERIO_IOREGION_LENGTH	2
+
+/* module_param force: useful for an unrecognized device, because generic
+ * registers will be available in sysfs for inspection
+ * (e.g. for isadump and sensors-detect) */
+static ushort force;
+module_param(force, ushort, 0);
+MODULE_PARM_DESC(force, "Force to load with unrecognized device ID");
+
+/* module_param superio_port: instead of probing the normal ports, just probe
+ * this one port */
+static ushort superio_port;
+module_param(superio_port, ushort, 0);
+MODULE_PARM_DESC(superio_port, "Force probe at superio_port, and not defaults");
+
+/* module init: request region and see if a Super-I/O device lives here */
+static int __init superio_find_at(int addr)
+{
+	u16 val;
+	struct resource res;
+	struct platform_device * pdev;
+	const char * recognized_name = 0;
+	int err, i;
+
+	if (!request_region(addr, SUPERIO_IOREGION_LENGTH, DRVNAME)) {
+		printk(KERN_ERR DRVNAME ": region request 0x%x-0x%x failed\n",
+			addr, addr + SUPERIO_IOREGION_LENGTH - 1);
+		err = -EBUSY;
+		goto exit;
+	}
+
+	/* see if a Super-I/O device lives here */
+	superio_enter(addr);
+	val = superio_in2b(addr, SUPERIO_REG_DEVID);
+
+	for (i = 0; i < ARRAY_SIZE(recognize); i++) {
+		if ((val & recognize[i].devid_mask) == recognize[i].devid) {
+			recognized_name = recognize[i].driver_name;
+			pr_debug(DRVNAME ": Found %s (0x%04x) at 0x%x\n",
+				recognized_name, val, addr);
+			break;
+		}
+	}
+
+	superio_exit(addr);
+
+	if (!recognized_name) {
+		if (val != 0xffff) {
+			pr_debug(DRVNAME ": 0x%x: unsupported superio ID: "
+				"0x%04x\n", addr, val);
+			if (/*module_param*/ force) recognized_name = DRVNAME;
+		}
+		else
+			pr_info(DRVNAME ": no superio found at 0x%x\n", addr);
+
+		if (!recognized_name) {
+			err = -ENODEV;
+			goto exit_release;
+		}
+	}
+
+	/* create/register platform device for the new Super-I/O device
+	 * its bus_id will be "name.addr", e.g. w83627ehf.46 or w83627ehf.78
+	 * by using platform_device_register_simple(), you can leave the
+	 * hwmon module loaded and unload this "superio" module */
+	memset(&res, 0, sizeof(res));
+	res.name = recognized_name;
+	res.start = addr;
+	res.end = addr + SUPERIO_IOREGION_LENGTH - 1;
+	res.flags = IORESOURCE_IO;
+	pdev = platform_device_register_simple(recognized_name, addr, &res, 1);
+	if (IS_ERR(pdev)) {
+		printk(KERN_ERR DRVNAME ": device registration failed (%ld)\n",
+			PTR_ERR(pdev));
+		err = (int) (PTR_ERR(pdev));
+		goto exit_release;
+	}
+
+	platform_device_put(pdev); /* now the device can exist on its own */
+	return 0;
+
+exit_release:
+	release_region(addr, SUPERIO_IOREGION_LENGTH);
+exit:
+	return err;
+}
+
+/* module init function */
+static int __init superio_module_init(void)
+{
+	int superio_ports[] = { 0x2e, 0x4e };
+	int err;
+	unsigned num_ports, i;
+
+	err = platform_driver_register(&superio_driver);
+	if (err)
+		goto exit;
+
+	/* request region for each of the possible Super-I/O ports, and
+	 * probe the port to see if a Super-I/O device lives there.
+	 * If it fails with -ENODEV, look elsewhere. If it fails with
+	 * -EBUSY, look elsewhere (the region is reserved, that's all). */
+
+	num_ports = ARRAY_SIZE(superio_ports);
+	if (superio_port) {	/* user-specified port: do not try defaults */
+		superio_ports[0] = superio_port;
+		num_ports = 1;
+	}
+	superio_port = 0;	/* count how many we find */
+	for (i = 0; i < num_ports; i++) {
+		err = superio_find_at(superio_ports[i]);
+		if (err == 0)
+			superio_port++;	/* count how many we find */
+		else if (err != -ENODEV && err != -EBUSY)
+			goto exit_unregister;	/* got an error we don't want */
+	}
+	if (superio_port == 0) {	/* error -ENODEV: no devices found */
+		err = -ENODEV;	/* just to be sure err != -EBUSY */
+		goto exit_unregister;
+	}
+
+	return 0;	/* found at least 1, so we are ok */
+
+exit_unregister:
+	platform_driver_unregister(&superio_driver);
+exit:
+	return err;
+}
+
+/* module exit function */
+static void __exit superio_module_exit(void)
+{
+	platform_driver_unregister(&superio_driver);
+}
+
+MODULE_AUTHOR("David Hubbard <david.c.hubbard@gmail.com>");
+MODULE_DESCRIPTION("Super I/O driver");
+MODULE_LICENSE("GPL");
+
+module_init(superio_module_init);
+module_exit(superio_module_exit);
diff -Nuar old/superio.h new/superio.h
--- old/superio.h	1969-12-31 17:00:00.000000000 -0700
+++ new/superio.h	2008-11-14 03:07:37.000000000 -0700
@@ -0,0 +1,52 @@
+#ifndef _SUPERIO_H_
+#define _SUPERIO_H_
+
+/* These superio registers are exported for all superio devices */
+/* The device will have more superio registers that these */
+
+#define SUPERIO_REG_LDSEL	0x07	/* Logical device select */
+#define SUPERIO_REG_DEVID	0x20	/* Device ID (2 bytes) */
+#define SUPERIO_REG_ENABLE	0x30	/* Logical device enable */
+#define SUPERIO_REG_ADDR	0x60	/* Logical device address (2 bytes) */
+
+static inline void superio_outb(int ioreg, int reg, int val)
+{
+	outb(reg, ioreg);
+	outb(val, ioreg + 1);
+}
+
+static inline int superio_inb(int ioreg, int reg)
+{
+	outb(reg, ioreg);
+	return inb(ioreg + 1);
+}
+
+static inline int superio_in2b(int ioreg, int reg)
+{
+	return (superio_inb(ioreg, reg) << 8) | superio_inb(ioreg, reg + 1);
+}
+
+static inline void superio_select(int ioreg, int ld)
+{
+	outb(SUPERIO_REG_LDSEL, ioreg);
+	outb(ld, ioreg + 1);
+}
+
+static inline void superio_enter(int ioreg)
+{
+	outb(0x87, ioreg);
+	outb(0x87, ioreg);
+}
+
+static inline void superio_exit(int ioreg)
+{
+	/* Thanks to Jean Delvare <khali@linux-fr.org>, and sensors-detect:
+	 * There are two ways to do it. Some chips (SMSC, Winbond) want this: */
+	outb(0xaa, ioreg);
+
+	/* And some want return to "Wait For Key" state (PNP-ISA spec): */
+	superio_outb(ioreg, 0x02, 0x02);
+}
+
+
+#endif /* _SUPERIO_H_ */
diff -Nuar old/w83627ehf.c new/w83627ehf.c
--- old/w83627ehf.c	2008-11-14 00:23:29.000000000 -0700
+++ new/w83627ehf.c	2008-11-14 03:06:25.000000000 -0700
@@ -4,7 +4,7 @@
     Copyright (C) 2005  Jean Delvare <khali@linux-fr.org>
     Copyright (C) 2006  Yuan Mu (Winbond),
                         Rudolf Marek <r.marek@assembler.cz>
-                        David Hubbard <david.c.hubbard@gmail.com>
+    Copyright (C) 2008  David Hubbard <david.c.hubbard@gmail.com>
 
     Shamelessly ripped from the w83627hf driver
     Copyright (C) 2003  Mark Studebaker
@@ -48,21 +48,12 @@
 #include <linux/hwmon-vid.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
-#include <asm/io.h>
-#include "lm75.h"
+#include <asm/io.h>	/* inb() outb() */
+#include "lm75.h"	/* for LM75_TEMP_TO_REG / FROM_REG */
+#include "superio.h"
 
 enum kinds { w83627ehf, w83627dhg };
 
-/* used to set data->name = w83627ehf_device_names[data->sio_kind] */
-static const char * w83627ehf_device_names[] = {
-	"w83627ehf",
-	"w83627dhg",
-};
-
-static unsigned short force_id;
-module_param(force_id, ushort, 0);
-MODULE_PARM_DESC(force_id, "Override the detected device ID");
-
 #define DRVNAME "w83627ehf"
 
 /*
@@ -71,54 +62,15 @@
 
 #define W83627EHF_LD_HWM	0x0b
 
-#define SIO_REG_LDSEL		0x07	/* Logical device select */
-#define SIO_REG_DEVID		0x20	/* Device ID (2 bytes) */
-#define SIO_REG_EN_VRM10	0x2C	/* GPIO3, GPIO4 selection */
-#define SIO_REG_ENABLE		0x30	/* Logical device enable */
-#define SIO_REG_ADDR		0x60	/* Logical device address (2 bytes) */
-#define SIO_REG_VID_CTRL	0xF0	/* VID control */
-#define SIO_REG_VID_DATA	0xF1	/* VID data */
+#define W83627EHF_SIOREG_EN_VRM10	0x2C	/* GPIO3, GPIO4 selection */
+#define W83627EHF_SIOREG_VID_CTRL	0xF0	/* VID control */
+#define W83627EHF_SIOREG_VID_DATA	0xF1	/* VID data */
 
 #define SIO_W83627EHF_ID	0x8850
 #define SIO_W83627EHG_ID	0x8860
 #define SIO_W83627DHG_ID	0xa020
 #define SIO_ID_MASK		0xFFF0
 
-static inline void
-superio_outb(int ioreg, int reg, int val)
-{
-	outb(reg, ioreg);
-	outb(val, ioreg + 1);
-}
-
-static inline int
-superio_inb(int ioreg, int reg)
-{
-	outb(reg, ioreg);
-	return inb(ioreg + 1);
-}
-
-static inline void
-superio_select(int ioreg, int ld)
-{
-	outb(SIO_REG_LDSEL, ioreg);
-	outb(ld, ioreg + 1);
-}
-
-static inline void
-superio_enter(int ioreg)
-{
-	outb(0x87, ioreg);
-	outb(0x87, ioreg);
-}
-
-static inline void
-superio_exit(int ioreg)
-{
-	outb(0x02, ioreg);
-	outb(0x02, ioreg + 1);
-}
-
 /*
  * ISA constants
  */
@@ -299,11 +251,6 @@
 	u8 vrm;
 };
 
-struct w83627ehf_sio_data {
-	int sioreg;
-	enum kinds kind;
-};
-
 static inline int is_word_sized(u16 reg)
 {
 	return (((reg & 0xff00) == 0x100
@@ -1242,73 +1189,141 @@
 	}
 }
 
+/* When the "superio" module thinks a chip is w83627ehf-compatible, it creates
+ * a platform_device and names it with our DRVNAME, so drivers/base/platform.c
+ * calls us here. */
 static int __devinit w83627ehf_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct w83627ehf_sio_data *sio_data = dev->platform_data;
 	struct w83627ehf_data *data;
-	struct resource *res;
+	struct resource * siores, w_res;
 	u8 fan4pin, fan5pin, en_vrm10;
-	int i, err = 0;
+	int i, sioaddr, err = 0;
+	enum kinds kind;
 
-	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	if (!request_region(res->start, IOREGION_LENGTH, DRVNAME)) {
-		err = -EBUSY;
-		dev_err(dev, "Failed to request region 0x%lx-0x%lx\n",
-			(unsigned long)res->start,
-			(unsigned long)res->start + IOREGION_LENGTH - 1);
+	siores = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!siores) {
+		err = -EINVAL;
+		dev_err(dev, "missing superio address\n");
 		goto exit;
 	}
+	sioaddr = siores->start;
 
 	if (!(data = kzalloc(sizeof(struct w83627ehf_data), GFP_KERNEL))) {
 		err = -ENOMEM;
+		goto exit;
+	}
+
+	/* The device should have an I/O port for direct inb() outb() access */
+	superio_enter(sioaddr);
+	superio_select(sioaddr, W83627EHF_LD_HWM);
+	data->addr = (superio_inb(sioaddr, SUPERIO_REG_ADDR) << 8)
+	    | superio_inb(sioaddr, SUPERIO_REG_ADDR + 1);
+	data->addr &= IOREGION_ALIGNMENT;
+	if (data->addr == 0) {
+		dev_err(dev, " Refusing to enable a Super-I/O "
+		       "device with a base I/O port 0.\n");
+		superio_exit(sioaddr);
+		err = -ENODEV;
+		goto exit_free;
+	}
+
+	/* Activate logical device if needed */
+	i = superio_inb(sioaddr, SUPERIO_REG_ENABLE);
+	if (!(i & 0x01)) {
+		dev_warn(dev, "Forcibly enabling Super-I/O. "
+		       "Sensor is probably unusable.\n");
+		superio_outb(sioaddr, SUPERIO_REG_ENABLE, i | 0x01);
+	}
+
+	/* add another resource at the now-found w83627ehf addr
+	 * to the platform_device we are working on */
+	w_res.name = DRVNAME;
+	w_res.start = data->addr + IOREGION_OFFSET;
+	w_res.end = data->addr + IOREGION_OFFSET + IOREGION_LENGTH - 1;
+	w_res.flags = IORESOURCE_IO;
+
+	if (!request_region(w_res.start, IOREGION_LENGTH, DRVNAME)) {
+		err = -EBUSY;
+		superio_exit(sioaddr);
+		dev_err(dev, "Failed to request region 0x%lx-0x%lx\n",
+			(unsigned long) w_res.start,
+			(unsigned long) w_res.start + IOREGION_LENGTH - 1);
+		goto exit_free;
+	}
+
+	err = platform_device_add_resources(pdev, &w_res, 1);
+	if (err) {
+		superio_exit(sioaddr);
+		dev_err(dev, "add resource to device failed (%d)\n", err);
+		goto exit_release;
+	}
+
+	/* auto-detect the type of chip (ehf, ehg, or dhg) */
+	i = superio_in2b(sioaddr, SUPERIO_REG_DEVID);
+	switch (i & SIO_ID_MASK) {
+	case SIO_W83627EHF_ID:
+		kind = w83627ehf;
+		data->name = "w83627ehf";
+		break;
+	case SIO_W83627EHG_ID:
+		kind = w83627ehf;
+		data->name = "w83627ehg";
+		break;
+	case SIO_W83627DHG_ID:
+		kind = w83627dhg;
+		data->name = "w83627dhg";
+		break;
+	default:
+		err = -ENODEV;
+		superio_exit(sioaddr);
+		dev_err(dev, "unsupported chip ID: 0x%04x\n", i);
 		goto exit_release;
 	}
 
-	data->addr = res->start;
+	pr_info(DRVNAME ": found a %s at %#x\n", data->name, data->addr);
 	mutex_init(&data->lock);
 	mutex_init(&data->update_lock);
-	data->name = w83627ehf_device_names[sio_data->kind];
 	platform_set_drvdata(pdev, data);
 
 	/* 627EHG and 627EHF have 10 voltage inputs; DHG has 9 */
-	data->in_num = (sio_data->kind == w83627dhg) ? 9 : 10;
+	data->in_num = (kind == w83627dhg) ? 9 : 10;
 
 	/* Initialize the chip */
 	w83627ehf_init_device(data);
 
 	data->vrm = vid_which_vrm();
-	superio_enter(sio_data->sioreg);
 	/* Read VID value */
-	superio_select(sio_data->sioreg, W83627EHF_LD_HWM);
-	if (superio_inb(sio_data->sioreg, SIO_REG_VID_CTRL) & 0x80) {
+	if (superio_inb(sioaddr, W83627EHF_SIOREG_VID_CTRL) & 0x80) {
 		/* Set VID input sensibility if needed. In theory the BIOS
 		   should have set it, but in practice it's not always the
 		   case. We only do it for the W83627EHF/EHG because the
 		   W83627DHG is more complex in this respect. */
-		if (sio_data->kind == w83627ehf) {
-			en_vrm10 = superio_inb(sio_data->sioreg,
-					       SIO_REG_EN_VRM10);
+		if (kind == w83627ehf) {
+			en_vrm10 = superio_inb(sioaddr,
+				W83627EHF_SIOREG_EN_VRM10);
 			if ((en_vrm10 & 0x08) && data->vrm == 90) {
 				dev_warn(dev, "Setting VID input voltage to "
 					 "TTL\n");
-				superio_outb(sio_data->sioreg, SIO_REG_EN_VRM10,
+				superio_outb(sioaddr, W83627EHF_SIOREG_EN_VRM10,
 					     en_vrm10 & ~0x08);
 			} else if (!(en_vrm10 & 0x08) && data->vrm == 100) {
 				dev_warn(dev, "Setting VID input voltage to "
 					 "VRM10\n");
-				superio_outb(sio_data->sioreg, SIO_REG_EN_VRM10,
+				superio_outb(sioaddr, W83627EHF_SIOREG_EN_VRM10,
 					     en_vrm10 | 0x08);
 			}
 		}
 
-		data->vid = superio_inb(sio_data->sioreg, SIO_REG_VID_DATA);
-		if (sio_data->kind == w83627ehf) /* 6 VID pins only */
+		data->vid = superio_inb(sioaddr, W83627EHF_SIOREG_VID_DATA);
+		if (kind == w83627ehf) /* 6 VID pins only */
 			data->vid &= 0x3f;
 
 		err = device_create_file(dev, &dev_attr_cpu0_vid);
-		if (err)
+		if (err) {
+			superio_exit(sioaddr);
 			goto exit_release;
+		}
 	} else {
 		dev_info(dev, "VID pins in output mode, CPU VID not "
 			 "available\n");
@@ -1316,9 +1331,9 @@
 
 	/* fan4 and fan5 share some pins with the GPIO and serial flash */
 
-	fan5pin = superio_inb(sio_data->sioreg, 0x24) & 0x2;
-	fan4pin = superio_inb(sio_data->sioreg, 0x29) & 0x6;
-	superio_exit(sio_data->sioreg);
+	fan5pin = superio_inb(sioaddr, 0x24) & 0x2;
+	fan4pin = superio_inb(sioaddr, 0x29) & 0x6;
+	superio_exit(sioaddr);
 
 	/* It looks like fan4 and fan5 pins can be alternatively used
 	   as fan on/off switches, but fan5 control is write only :/
@@ -1339,14 +1354,14 @@
 	/* Register sysfs hooks */
   	for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays); i++)
 		if ((err = device_create_file(dev,
-			&sda_sf3_arrays[i].dev_attr)))
+				&sda_sf3_arrays[i].dev_attr)))
 			goto exit_remove;
 
 	/* if fan4 is enabled create the sf3 files for it */
 	if (data->has_fan & (1 << 3))
 		for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++) {
 			if ((err = device_create_file(dev,
-				&sda_sf3_arrays_fan4[i].dev_attr)))
+					&sda_sf3_arrays_fan4[i].dev_attr)))
 				goto exit_remove;
 		}
 
@@ -1404,10 +1419,11 @@
 
 exit_remove:
 	w83627ehf_device_remove_files(dev);
-	kfree(data);
 	platform_set_drvdata(pdev, NULL);
 exit_release:
-	release_region(res->start, IOREGION_LENGTH);
+	release_region(w_res.start, IOREGION_LENGTH);
+exit_free:
+	kfree(data);
 exit:
 	return err;
 }
@@ -1434,144 +1450,13 @@
 	.remove		= __devexit_p(w83627ehf_remove),
 };
 
-/* w83627ehf_find() looks for a '627 in the Super-I/O config space */
-static int __init w83627ehf_find(int sioaddr, unsigned short *addr,
-				 struct w83627ehf_sio_data *sio_data)
-{
-	static const char __initdata sio_name_W83627EHF[] = "W83627EHF";
-	static const char __initdata sio_name_W83627EHG[] = "W83627EHG";
-	static const char __initdata sio_name_W83627DHG[] = "W83627DHG";
-
-	u16 val;
-	const char *sio_name;
-
-	superio_enter(sioaddr);
-
-	if (force_id)
-		val = force_id;
-	else
-		val = (superio_inb(sioaddr, SIO_REG_DEVID) << 8)
-		    | superio_inb(sioaddr, SIO_REG_DEVID + 1);
-	switch (val & SIO_ID_MASK) {
-	case SIO_W83627EHF_ID:
-		sio_data->kind = w83627ehf;
-		sio_name = sio_name_W83627EHF;
-		break;
-	case SIO_W83627EHG_ID:
-		sio_data->kind = w83627ehf;
-		sio_name = sio_name_W83627EHG;
-		break;
-	case SIO_W83627DHG_ID:
-		sio_data->kind = w83627dhg;
-		sio_name = sio_name_W83627DHG;
-		break;
-	default:
-		if (val != 0xffff)
-			pr_debug(DRVNAME ": unsupported chip ID: 0x%04x\n",
-				 val);
-		superio_exit(sioaddr);
-		return -ENODEV;
-	}
-
-	/* We have a known chip, find the HWM I/O address */
-	superio_select(sioaddr, W83627EHF_LD_HWM);
-	val = (superio_inb(sioaddr, SIO_REG_ADDR) << 8)
-	    | superio_inb(sioaddr, SIO_REG_ADDR + 1);
-	*addr = val & IOREGION_ALIGNMENT;
-	if (*addr == 0) {
-		printk(KERN_ERR DRVNAME ": Refusing to enable a Super-I/O "
-		       "device with a base I/O port 0.\n");
-		superio_exit(sioaddr);
-		return -ENODEV;
-	}
-
-	/* Activate logical device if needed */
-	val = superio_inb(sioaddr, SIO_REG_ENABLE);
-	if (!(val & 0x01)) {
-		printk(KERN_WARNING DRVNAME ": Forcibly enabling Super-I/O. "
-		       "Sensor is probably unusable.\n");
-		superio_outb(sioaddr, SIO_REG_ENABLE, val | 0x01);
-	}
-
-	superio_exit(sioaddr);
-	pr_info(DRVNAME ": Found %s chip at %#x\n", sio_name, *addr);
-	sio_data->sioreg = sioaddr;
-
-	return 0;
-}
-
-/* when Super-I/O functions move to a separate file, the Super-I/O
- * bus will manage the lifetime of the device and this module will only keep
- * track of the w83627ehf driver. But since we platform_device_alloc(), we
- * must keep track of the device */
-static struct platform_device *pdev;
-
 static int __init sensors_w83627ehf_init(void)
 {
-	int err;
-	unsigned short address;
-	struct resource res;
-	struct w83627ehf_sio_data sio_data;
-
-	/* initialize sio_data->kind and sio_data->sioreg.
-	 *
-	 * when Super-I/O functions move to a separate file, the Super-I/O
-	 * driver will probe 0x2e and 0x4e and auto-detect the presence of a
-	 * w83627ehf hardware monitor, and call probe() */
-	if (w83627ehf_find(0x2e, &address, &sio_data) &&
-	    w83627ehf_find(0x4e, &address, &sio_data))
-		return -ENODEV;
-
-	err = platform_driver_register(&w83627ehf_driver);
-	if (err)
-		goto exit;
-
-	if (!(pdev = platform_device_alloc(DRVNAME, address))) {
-		err = -ENOMEM;
-		printk(KERN_ERR DRVNAME ": Device allocation failed\n");
-		goto exit_unregister;
-	}
-
-	err = platform_device_add_data(pdev, &sio_data,
-				       sizeof(struct w83627ehf_sio_data));
-	if (err) {
-		printk(KERN_ERR DRVNAME ": Platform data allocation failed\n");
-		goto exit_device_put;
-	}
-
-	memset(&res, 0, sizeof(res));
-	res.name = DRVNAME;
-	res.start = address + IOREGION_OFFSET;
-	res.end = address + IOREGION_OFFSET + IOREGION_LENGTH - 1;
-	res.flags = IORESOURCE_IO;
-	err = platform_device_add_resources(pdev, &res, 1);
-	if (err) {
-		printk(KERN_ERR DRVNAME ": Device resource addition failed "
-		       "(%d)\n", err);
-		goto exit_device_put;
-	}
-
-	/* platform_device_add calls probe() */
-	err = platform_device_add(pdev);
-	if (err) {
-		printk(KERN_ERR DRVNAME ": Device addition failed (%d)\n",
-		       err);
-		goto exit_device_put;
-	}
-
-	return 0;
-
-exit_device_put:
-	platform_device_put(pdev);
-exit_unregister:
-	platform_driver_unregister(&w83627ehf_driver);
-exit:
-	return err;
+	return platform_driver_register(&w83627ehf_driver);
 }
 
 static void __exit sensors_w83627ehf_exit(void)
 {
-	platform_device_unregister(pdev);
 	platform_driver_unregister(&w83627ehf_driver);
 }
 

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors