[lm-sensors] Address configuration for scx200_acb

[lm-sensors] Address configuration for scx200_acb

[Jean Delvare]

Ben, Jordan,

Can you please comment on this?
http://bugzilla.kernel.org/show_bug.cgi?idd45#c15

This is a propsal to make the scx200_acb driver's default base
addresses configurable through Kconfig. What do you think?

Another possibility would be to detect them at runtime depending on
some platform data. Sounds more complex, if possible at all.

Or we can stick to the current state (default addresses, can be
overriden with module parameters).

What makes the current default addresses (0x820 and 0x840) more
legitimate than what Alexander has (0x810 and 0x820)?

Thanks,
-- 
Jean Delvare

[lm-sensors] Address configuration for scx200_acb

[Thomas Andrews]

On Thu, Apr 27, 2006 at 11:41:14PM +0200, Jean Delvare wrote:

> Can you please comment on this?
> http://bugzilla.kernel.org/show_bug.cgi?idd45#c15
> 
> This is a propsal to make the scx200_acb driver's default base
> addresses configurable through Kconfig. What do you think?
> 
> Another possibility would be to detect them at runtime depending on
> some platform data. Sounds more complex, if possible at all.
> 
> Or we can stick to the current state (default addresses, can be
> overriden with module parameters).
> 
> What makes the current default addresses (0x820 and 0x840) more
> legitimate than what Alexander has (0x810 and 0x820)?

I've been wondering about this myself. I also use the wrap boards, and
I've found that the address can be automatically set using something
like this:

static void detect_acb_base_addresses(int *acb1, int* acb2)
{
    unsigned char h1 = 0;
    unsigned char l1 = 0;
    unsigned char h2 = 0;
    unsigned char l2 = 0;

    /* Select ACB1  (LDN 5) */
    outb(0x07,CFG_INDEX);
    outb(0x05,CFG_DATA);
    outb(0x60,CFG_INDEX);
    h1 = inb(CFG_DATA);
    outb(0x61,CFG_INDEX);
    l1 = inb(CFG_DATA);

    /* ACB2 (LDN 6) */
    outb(0x07,CFG_INDEX);
    outb(0x06,CFG_DATA);
    outb(0x60,CFG_INDEX);
    h2 = inb(CFG_DATA);
    outb(0x61,CFG_INDEX);
    l2 = inb(CFG_DATA);
    *acb1 = (h1 << 8) | l1;
    *acb2 = (h2 << 8) | l2;    
    DEBUG(1,printk("ACB1 at %x, ACB2 at %x",*acb1,*acb2));
}

Would this not work for the whole family ?

Regards,
Thomas

[lm-sensors] Address configuration for scx200_acb

[Jean Delvare]

Hi Thomas,

[Jean Delvare]
> > This is a propsal to make the scx200_acb driver's default base
> > addresses configurable through Kconfig. What do you think?
> > 
> > Another possibility would be to detect them at runtime depending on
> > some platform data. Sounds more complex, if possible at all.
> > 
> > Or we can stick to the current state (default addresses, can be
> > overriden with module parameters).
> > 
> > What makes the current default addresses (0x820 and 0x840) more
> > legitimate than what Alexander has (0x810 and 0x820)?

[Thomas Andrews]
> I've been wondering about this myself. I also use the wrap boards, and
> I've found that the address can be automatically set using something
> like this:
> 
> static void detect_acb_base_addresses(int *acb1, int* acb2)
> {
>     unsigned char h1 = 0;
>     unsigned char l1 = 0;
>     unsigned char h2 = 0;
>     unsigned char l2 = 0;
> 
>     /* Select ACB1  (LDN 5) */
>     outb(0x07,CFG_INDEX);
>     outb(0x05,CFG_DATA);
>     outb(0x60,CFG_INDEX);
>     h1 = inb(CFG_DATA);
>     outb(0x61,CFG_INDEX);
>     l1 = inb(CFG_DATA);
> 
>     /* ACB2 (LDN 6) */
>     outb(0x07,CFG_INDEX);
>     outb(0x06,CFG_DATA);
>     outb(0x60,CFG_INDEX);
>     h2 = inb(CFG_DATA);
>     outb(0x61,CFG_INDEX);
>     l2 = inb(CFG_DATA);
>     *acb1 = (h1 << 8) | l1;
>     *acb2 = (h2 << 8) | l2;    
>     DEBUG(1,printk("ACB1 at %x, ACB2 at %x",*acb1,*acb2));
> }
> 
> Would this not work for the whole family ?

Looks like standard LPC/Super-IO register mapping. Are the SC1100 and
SCx200 LPC chips? What are the values of CFG_INDEX and CFG_DATA in the
code snippet above? I'd guess/hope 0x2e and 0x2f respectively (or 0x4e
and 0x4f.)

I don't have much time to work on that myself, especially when I have
no hardware to test this on, but if you or anyone else could prepare a
patch implementing the idea above in the scx200_acb driver, so that
other users can give it some testing, this would be great.

Thanks,
-- 
Jean Delvare

[lm-sensors] Address configuration for scx200_acb

[Thomas Andrews]

Hi Jean,

On Sun, Apr 30, 2006 at 09:43:33AM +0200, Jean Delvare wrote:

[Jean Delvare]
> > > This is a propsal to make the scx200_acb driver's default base
> > > addresses configurable through Kconfig. What do you think?
> > > 
> > > Another possibility would be to detect them at runtime depending on
> > > some platform data. Sounds more complex, if possible at all.
> > > 
> > > Or we can stick to the current state (default addresses, can be
> > > overriden with module parameters).
> > > 
> > > What makes the current default addresses (0x820 and 0x840) more
> > > legitimate than what Alexander has (0x810 and 0x820)?
> 
[Thomas Andrews]
> > I've been wondering about this myself. I also use the wrap boards, and
> > I've found that the address can be automatically set using something
> > like this:
> > 
> > static void detect_acb_base_addresses(int *acb1, int* acb2)
> > {
> >     unsigned char h1 = 0;
> >     unsigned char l1 = 0;
> >     unsigned char h2 = 0;
> >     unsigned char l2 = 0;
> > 
> >     /* Select ACB1  (LDN 5) */
> >     outb(0x07,CFG_INDEX);
> >     outb(0x05,CFG_DATA);
> >     outb(0x60,CFG_INDEX);
> >     h1 = inb(CFG_DATA);
> >     outb(0x61,CFG_INDEX);
> >     l1 = inb(CFG_DATA);
> > 
> >     /* ACB2 (LDN 6) */
> >     outb(0x07,CFG_INDEX);
> >     outb(0x06,CFG_DATA);
> >     outb(0x60,CFG_INDEX);
> >     h2 = inb(CFG_DATA);
> >     outb(0x61,CFG_INDEX);
> >     l2 = inb(CFG_DATA);
> >     *acb1 = (h1 << 8) | l1;
> >     *acb2 = (h2 << 8) | l2;    
> >     DEBUG(1,printk("ACB1 at %x, ACB2 at %x",*acb1,*acb2));
> > }
> > 
> > Would this not work for the whole family ?
> 
[Jean Delvare]
> Looks like standard LPC/Super-IO register mapping. Are the SC1100 and
> SCx200 LPC chips? What are the values of CFG_INDEX and CFG_DATA in the
> code snippet above? I'd guess/hope 0x2e and 0x2f respectively (or 0x4e
> and 0x4f.)
> 
> I don't have much time to work on that myself, especially when I have
> no hardware to test this on, but if you or anyone else could prepare a
> patch implementing the idea above in the scx200_acb driver, so that
> other users can give it some testing, this would be great.

I don't actually know what "LPC/Super-IO" means, but the data-sheet for
the SC1100 seems to indicate that it is so:

    "The LPC interface of the Core Logic module is based on the Intel Low
    Pin Count (LPC) Interface specification, revision 1.0."

and also:

    "LPC SuperI/O Addressing. SuperI/O control addresses I/O Ports 2Eh-2Fh.
    See bit 16 for decode"

And as you suggested, there is another address pair that can be used:

    "LPC Alternate SuperI/O Addressing. Alternate SuperI/O control addresses
    4Eh-4Fh. See bit 16 for decode."

In the code snippet above, I used 0x2e & x2f. I don't know how you
determine which pair to use. I just tried the first one. Is there a
"standard" way to select it?

I will submit a patch once I have a better understanding.

Regards,
Thomas

[lm-sensors] Address configuration for scx200_acb

[Jean Delvare]

Hi Thomas,

[Jean Delvare]
> > Looks like standard LPC/Super-IO register mapping. Are the SC1100 and
> > SCx200 LPC chips? What are the values of CFG_INDEX and CFG_DATA in the
> > code snippet above? I'd guess/hope 0x2e and 0x2f respectively (or 0x4e
> > and 0x4f.)
> > 
> > I don't have much time to work on that myself, especially when I have
> > no hardware to test this on, but if you or anyone else could prepare a
> > patch implementing the idea above in the scx200_acb driver, so that
> > other users can give it some testing, this would be great.

[Thomas Andrews]
> I don't actually know what "LPC/Super-IO" means, but the data-sheet for
> the SC1100 seems to indicate that it is so:
> 
>     "The LPC interface of the Core Logic module is based on the Intel Low
>     Pin Count (LPC) Interface specification, revision 1.0."
> 
> and also:
> 
>     "LPC SuperI/O Addressing. SuperI/O control addresses I/O Ports 2Eh-2Fh.
>     See bit 16 for decode"
> 
> And as you suggested, there is another address pair that can be used:
> 
>     "LPC Alternate SuperI/O Addressing. Alternate SuperI/O control addresses
>     4Eh-4Fh. See bit 16 for decode."
> 
> In the code snippet above, I used 0x2e & x2f. I don't know how you
> determine which pair to use. I just tried the first one. Is there a
> "standard" way to select it?

Usually the address is selected by hardware, by latching the state of
one pin of the chip at power-up. So, most of the time you just have to
try both address pairs. See function w83627hf_find in
drivers/hwmon/w83627hf.c for an example. 0x2e/0x2f is almost always
the right address pair.

> I will submit a patch once I have a better understanding.

Great, thanks.

-- 
Jean Delvare

[lm-sensors] Address configuration for scx200_acb

[Jim Cromie]

Thomas Andrews wrote:
> On Thu, Apr 27, 2006 at 11:41:14PM +0200, Jean Delvare wrote:
>
>   
>> Can you please comment on this?
>> http://bugzilla.kernel.org/show_bug.cgi?idd45#c15
>>
>> This is a propsal to make the scx200_acb driver's default base
>> addresses configurable through Kconfig. What do you think?
>>
>> Another possibility would be to detect them at runtime depending on
>> some platform data. Sounds more complex, if possible at all.
>>
>> Or we can stick to the current state (default addresses, can be
>> overriden with module parameters).
>>
>> What makes the current default addresses (0x820 and 0x840) more
>> legitimate than what Alexander has (0x810 and 0x820)?
>>     
>
> I've been wondering about this myself. I also use the wrap boards, and
> I've found that the address can be automatically set using something
> like this:
>
> static void detect_acb_base_addresses(int *acb1, int* acb2)
> {
>     unsigned char h1 = 0;
>     unsigned char l1 = 0;
>     unsigned char h2 = 0;
>     unsigned char l2 = 0;
>
>     /* Select ACB1  (LDN 5) */
>     outb(0x07,CFG_INDEX);
>     outb(0x05,CFG_DATA);
>     outb(0x60,CFG_INDEX);
>     h1 = inb(CFG_DATA);
>     outb(0x61,CFG_INDEX);
>     l1 = inb(CFG_DATA);
>
>     /* ACB2 (LDN 6) */
>     outb(0x07,CFG_INDEX);
>     outb(0x06,CFG_DATA);
>     outb(0x60,CFG_INDEX);
>     h2 = inb(CFG_DATA);
>     outb(0x61,CFG_INDEX);
>     l2 = inb(CFG_DATA);
>     *acb1 = (h1 << 8) | l1;
>     *acb2 = (h2 << 8) | l2;    
>     DEBUG(1,printk("ACB1 at %x, ACB2 at %x",*acb1,*acb2));
> }
>
> Would this not work for the whole family ?
>
> Regards,
> Thomas
>   

I have a soekris net4801 which has the same sc-1100.
On my board, ISA addys 2e, 2f talk to a separate Super-IO chip, pc87360.
which IIRC has an acb unit also.
Back when I got the board, the presence of 2 superio devices was somewhat
confusing, until Id read both datasheets - 2x.

FWIW, the pc87360 has these (from a pre-proto-patch I still have)

+char* funcblocks[] = {
+       "Floppy Disk Controller (FDC)",
+       "Parallel Port (PP)",
+       "Serial Port 2 with IR (SP2)",
+       "Serial Port 1 (SP1)",
+       "System Wake-Up Control (SWC)",
+       "Keyboard and Mouse Controller (KBC) - Mouse interface",
+       "Keyboard and Mouse Controller (KBC) - Keyboard interface",
+       "General-Purpose I/O (GPIO) Ports",
+       "ACCESS.bus Interface (ACB)",
+       "Fan Speed Control and Monitor (FSCM)",
+       "WATCHDOG Timer (WDT)",
+       "Game Port (GMP)",
+       "Musical Instrument Digital Interface (MIDI) Port",
+       "Voltage Level Monitor (VLM)",
+       "Temperature Sensor (TMS)",
+       NULL
+};
+
+
+struct pc87366_unit pc87366_units[] = {
+       { 8,    "Floppy Disk Controller (FDC)" },
+       { 0x10, "Parallel Port (PP)" },
+       { 8, "Serial Port 2 with IR (SP2)" },
+       { 8, "Serial Port 1 (SP1)" },
+       { 0x10, "System Wake-Up Control (SWC)" },
+       { 0x10, "Keyboard and Mouse Controller (KBC) - Mouse interface" },
+       { 0x10, "Keyboard and Mouse Controller (KBC) - Keyboard 
interface" },
+       { 0x0c, "General-Purpose I/O (GPIO) Ports" },
+       { 6,    "ACCESS.bus Interface (ACB)" },
+       { 0,    "Fan Speed Control and Monitor (FSCM)" },
+       { 4,    "WATCHDOG Timer (WDT)" },
+       { 0x0d, "Game Port (GMP)" },
+       { 3,    "Musical Instrument Digital Interface (MIDI) Port" },
+       { 0x10, "Voltage Level Monitor (VLM)" },
+       { 0x0f, "Temperature Sensor (TMS)" },
+       { 0,    NULL }
+};


the sc-1100 has fewer functional units; I dimly recall poking at them 
this same way,
but I cant dig up any old bones.

[lm-sensors] Address configuration for scx200_acb

[Jean Delvare]

Hi Jim,

> I have a soekris net4801 which has the same sc-1100.
> On my board, ISA addys 2e, 2f talk to a separate Super-IO chip, pc87360.
> which IIRC has an acb unit also.

True, we don't have a driver for it at the moment though.

> Back when I got the board, the presence of 2 superio devices was somewhat
> confusing, until Id read both datasheets - 2x.

Does the SC1100 appears at 0x4e/0x4f on your Soekris board? If so,
Thomas' code should still work, as long as we probe both possible
address pairs.

-- 
Jean Delvare

[lm-sensors] Address configuration for scx200_acb

[Thomas Andrews]

On Mon, May 01, 2006 at 02:22:52PM +0200, Jean Delvare wrote:

> > I have a soekris net4801 which has the same sc-1100.
> > On my board, ISA addys 2e, 2f talk to a separate Super-IO chip, pc87360.
> > which IIRC has an acb unit also.
> 
> True, we don't have a driver for it at the moment though.
> 
> > Back when I got the board, the presence of 2 superio devices was somewhat
> > confusing, until Id read both datasheets - 2x.
> 
> Does the SC1100 appears at 0x4e/0x4f on your Soekris board? If so,
> Thomas' code should still work, as long as we probe both possible
> address pairs.

At the moment I'm probing 2e, 4e, and 15c for the base addresses. The
addition of the last one was as a result of two things:

    (a) The old i2c-nscacb.c driver had this option
    (b) The SC1100 data-sheet indicates that it could be located there.

I have also made this a module parameter, so you could actually probe
anywhere for the base addresses, if you needed to.

I've retained the base addresses as a module parameter, so if you specify
those, then it will skip the probing at 2e, 4e, and 15c, and just use
the addresses you give.

Will that satisfy everyone's needs?

Regards,
Thomas

[lm-sensors] Address configuration for scx200_acb

[Jim Cromie]

Thomas Andrews wrote:
> On Mon, May 01, 2006 at 02:22:52PM +0200, Jean Delvare wrote:
>
>   
>>> I have a soekris net4801 which has the same sc-1100.
>>> On my board, ISA addys 2e, 2f talk to a separate Super-IO chip, pc87360.
>>> which IIRC has an acb unit also.
>>>       
>> True, we don't have a driver for it at the moment though.
>>
>>     
>>> Back when I got the board, the presence of 2 superio devices was somewhat
>>> confusing, until Id read both datasheets - 2x.
>>>       
>> Does the SC1100 appears at 0x4e/0x4f on your Soekris board? If so,
>> Thomas' code should still work, as long as we probe both possible
>> address pairs.
>>     
>
> At the moment I'm probing 2e, 4e, and 15c for the base addresses. The
> addition of the last one was as a result of two things:
>
>     (a) The old i2c-nscacb.c driver had this option
>     (b) The SC1100 data-sheet indicates that it could be located there.
>
> I have also made this a module parameter, so you could actually probe
> anywhere for the base addresses, if you needed to.
>
> I've retained the base addresses as a module parameter, so if you specify
> those, then it will skip the probing at 2e, 4e, and 15c, and just use
> the addresses you give.
>
> Will that satisfy everyone's needs?
>   

If theres a device-id, it would be good to check.
I dont know the 1st thing about the ACB (on either chip), but I will 
(eventually)
try your code, to see if it works on either or both devices.
At this point, I dunno how Ill know about 'works'.

Jean,

> Does the SC1100 appears at 0x4e/0x4f on your Soekris board? 


dunno, Ill try that too, when I can.

I suppose I should think some more about sio_lockmgr I talked about once.
http://thread.gmane.org/gmane.linux.drivers.sensors/8710/focus‘22

> Regards,
> Thomas
>
>   

thanks

[lm-sensors] Address configuration for scx200_acb

[Thomas Andrews]

On Mon, May 01, 2006 at 11:12:13AM -0400, Jim Cromie wrote:

> Thomas Andrews wrote:
> >At the moment I'm probing 2e, 4e, and 15c for the base addresses. The
> >addition of the last one was as a result of two things:
> >
> >    (a) The old i2c-nscacb.c driver had this option
> >    (b) The SC1100 data-sheet indicates that it could be located there.
> >
> >I have also made this a module parameter, so you could actually probe
> >anywhere for the base addresses, if you needed to.
> >
> >I've retained the base addresses as a module parameter, so if you specify
> >those, then it will skip the probing at 2e, 4e, and 15c, and just use
> >the addresses you give.
> >
> >Will that satisfy everyone's needs?
> >  
> 
> If theres a device-id, it would be good to check.

Yes, there is: at address 0x20 there is SIO ID. I was wondering if it
was of any use to check this because I rely on probing to find out if
the ACB devices are there, but now I am inclined to think that I should
check this byte before doing any probing.

> I dont know the 1st
> thing about the ACB (on either chip), but I will (eventually) try your
> code, to see if it works on either or both devices.

Thanks!

[lm-sensors] Address configuration for scx200_acb

[Alexander Krause]

Hi,

>static void detect_acb_base_addresses(int *acb1, int* acb2)
>{
>    unsigned char h1 = 0;
>    unsigned char l1 = 0;
>    unsigned char h2 = 0;
>    unsigned char l2 = 0;
>
>    /* Select ACB1  (LDN 5) */
>    outb(0x07,CFG_INDEX);
>    outb(0x05,CFG_DATA);
>    outb(0x60,CFG_INDEX);
>    h1 = inb(CFG_DATA);
>    outb(0x61,CFG_INDEX);
>    l1 = inb(CFG_DATA);
>
>    /* ACB2 (LDN 6) */
>    outb(0x07,CFG_INDEX);
>    outb(0x06,CFG_DATA);
>    outb(0x60,CFG_INDEX);
>    h2 = inb(CFG_DATA);
>    outb(0x61,CFG_INDEX);
>    l2 = inb(CFG_DATA);
>    *acb1 = (h1 << 8) | l1;
>    *acb2 = (h2 << 8) | l2;    
>    DEBUG(1,printk("ACB1 at %x, ACB2 at %x",*acb1,*acb2));
>}
is there any patch against the last kernel containing this function?
Well, i would write one if not and i could test it on my WRAP board (sc1100 
processor).

>Would this not work for the whole family ?
I'm not sure which devices uses scx200_acb and i can't imagine autodetection 
works on each one.
Maybe the module should try autodetection at first and uses static base 
addresses if it failed. 
And autodetection should be dissabled if there is any kernel parameter like 
scx200_acb.base=0x???,0x???

...just some thoughts.

cheers Alex
-- 
Alexander Krause
Amtmann-K?stner-Platz 11
99091 Erfurt

Mail alexander.krause at erazor-zone.de
Tel. +49 (179) 4725826

[lm-sensors] Address configuration for scx200_acb

[Thomas Andrews]

Hi Alex,

On Tue, May 02, 2006 at 04:08:36PM +0200, Alexander Krause wrote:

> >static void detect_acb_base_addresses(int *acb1, int* acb2)
> >{
> >    unsigned char h1 = 0;
> >    unsigned char l1 = 0;
> >    unsigned char h2 = 0;
> >    unsigned char l2 = 0;
> >
> >    /* Select ACB1  (LDN 5) */
> >    outb(0x07,CFG_INDEX);
> >    outb(0x05,CFG_DATA);
> >    outb(0x60,CFG_INDEX);
> >    h1 = inb(CFG_DATA);
> >    outb(0x61,CFG_INDEX);
> >    l1 = inb(CFG_DATA);
> >
> >    /* ACB2 (LDN 6) */
> >    outb(0x07,CFG_INDEX);
> >    outb(0x06,CFG_DATA);
> >    outb(0x60,CFG_INDEX);
> >    h2 = inb(CFG_DATA);
> >    outb(0x61,CFG_INDEX);
> >    l2 = inb(CFG_DATA);
> >    *acb1 = (h1 << 8) | l1;
> >    *acb2 = (h2 << 8) | l2;    
> >    DEBUG(1,printk("ACB1 at %x, ACB2 at %x",*acb1,*acb2));
> >}
> is there any patch against the last kernel containing this function?
> Well, i would write one if not and i could test it on my WRAP board (sc1100 
> processor).

Not currently, but I will have one ready in roughly a week. The delay is
because I am actually modifying an older version suitable for the 2.4
kernel. On my WRAP platform I don't yet have a suitable 2.6 kernel for
testing it. If you want a patch urgently to try out, I can perhaps make
one, but I'll have to rely on you to test it.

Regards,
Thomas

[lm-sensors] Address configuration for scx200_acb

[Alexander Krause]

Hi
> Not currently, but I will have one ready in roughly a week. The delay is
> because I am actually modifying an older version suitable for the 2.4
> kernel. On my WRAP platform I don't yet have a suitable 2.6 kernel for
> testing it. If you want a patch urgently to try out, I can perhaps make
> one, but I'll have to rely on you to test it.
i could simply add your function to the lastest 2.6.17 and implement that auto 
mode.

I'm not quite sure if it's helpfull to change the io-base as described in the 
other posts but i could try that stuff later too.

And I'm open for tests ;-)

cheers Alex
-- 
Alexander Krause
Amtmann-K?stner-Platz 11
99091 Erfurt

Mail alexander.krause at erazor-zone.de
Tel. +49 (179) 4725826

[lm-sensors] Address configuration for scx200_acb

[Thomas Andrews]

On Wed, May 03, 2006 at 01:39:45PM +0200, Alexander Krause wrote:

> > Not currently, but I will have one ready in roughly a week. The delay is
> > because I am actually modifying an older version suitable for the 2.4
> > kernel. On my WRAP platform I don't yet have a suitable 2.6 kernel for
> > testing it. If you want a patch urgently to try out, I can perhaps make
> > one, but I'll have to rely on you to test it.
> i could simply add your function to the lastest 2.6.17 and implement that auto 
> mode.

You can, but (a) I know that the test works on the WRAP card, and (b) my
patch will make a cleaner job of it I believe.
 
> I'm not quite sure if it's helpfull to change the io-base as described in the 
> other posts but i could try that stuff later too.

I've found out the 'proper' way to read the base address. Depending on
various bit settings in the F0 and F5 configuration spaces, the base
address can be either 2e, 4e, or 15c. I have prototype code that can
read all of these settings so that it is no longer required to 'guess'.
There is one last issue outstanding, but I should have that sorted out
soon. I'm going to have to break my changes up into several patches,
because I'm messing with interrupts, base addresses, and speed settings
all at the same time.
 
> And I'm open for tests ;-)

Thanks, I appreciate it!

[lm-sensors] Address configuration for scx200_acb

[Thomas Andrews]

Hi Alex,

On Wed, May 03, 2006 at 01:39:45PM +0200, Alexander Krause wrote:

> And I'm open for tests ;-)

OK, I'm attaching a patch based on:

/usr/src/linux-2.6.15/drivers/i2c/busses/scx200_acb.c

This patch is to set up the base addresses automatically without
probing (or guessing.)

As I mentioned earlier the patch is untested because I am actually
making the changes on a 2.4 kernel. Please let me know if *anything*
goes wrong on 2.6. If it breaks, I will only be able to get a 2.6 dev
system up next week to debug it :(

8<----------8<----------8<----------8<----------8<----------8<----------

Usage notes:

The driver now finds out by looking at the PCI configuration registers
where exactly the SIO is, so it no longer has to guess 2e, 4e, 15c...
If you don't want this automatic look-up of SIO, you can just specify it
as a module parameter: sioaddr

sioaddr can take up to 4 parameters. Use it to force the driver to use a
specific value of SIO. 0x2e is common, but 0x4e and 0x15c are also
possible.

The base address is now also looked up using the PCI configuration
registers. There are two of these addresses eg 0x810 and 0x820. Once the
driver has a value for sioaddr, it looks up the base address. If you
don't want this automatic lookup of the base addresses, you can still
specify them as module parameters as before. If you do this, it does not
bother to find out what sioaddr is.

As before, specify it as a list of one to four comma-delimited values
eg: base=0x810,0x820,0x830. It will try everything in the list.

8<----------8<----------8<----------8<----------8<----------8<----------

Regards,
Thomas
-------------- next part --------------
diff -Naur orig/scx200_acb.c latest/scx200_acb.c
--- orig/scx200_acb.c	2006-05-03 21:02:04.000000000 +0200
+++ latest/scx200_acb.c	2006-05-03 21:15:57.000000000 +0200
@@ -43,9 +43,15 @@
 MODULE_LICENSE("GPL");
 
 #define MAX_DEVICES 4
-static int base[MAX_DEVICES] = { 0x820, 0x840 };
-module_param_array(base, int, NULL, 0);
+#define MAX_SIO_ADDRS 4
+static int base[MAX_DEVICES] = { };
+static int sioaddr[MAX_SIO_ADDRS] = { };
+MODULE_PARM(base, "1-4i");
+MODULE_PARM(sioaddr, "1-4i");
 MODULE_PARM_DESC(base, "Base addresses for the ACCESS.bus controllers");
+MODULE_PARM_DESC(sioaddr, "SIO addresses for the ACCESS.bus controllers");
+module_param_array(base, int, NULL, 0);
+module_param_array(sioaddr, int, NULL, 0);
 
 #ifdef DEBUG
 #define DBG(x...) printk(KERN_DEBUG NAME ": " x)
@@ -116,8 +122,116 @@
 #define ACBCTL2		(iface->base + 5)
 #define    ACBCTL2_ENABLE	0x01
 
+/* Static port addresses in the PCI configuration space */
+#define CAR 0xCF8	/* Configuration Address Register */
+#define CDR 0xCFC	/* Configuration Data Register */
+
+static int deviceno;
+
 /************************************************************************/
 
+static unsigned long scx200_get_pci_deviceno(void)
+{
+	unsigned long reg;
+
+	/* Depending on the setting of IO_STRAP_IDSEL_SELECT (F5BAR0+I/O Offset 04h[0])
+	 * the PCI "device number" (used to read anything in the F0 - F5 config space)
+	 * can be either 1001b or 1000b for IO_STRAP_IDSEL_SELECT=0/1 respectively
+	 * This is the only thing we have to probe for. */
+
+	// Try with the default device no of 1001b to read the Vendor & Device ID
+	outl((unsigned)0x80000000 | 0x9000, CAR);
+	reg = inl(CDR);
+	if (((reg >> 16) & 0xFFFF) = 0x0510)
+		return (0x9000);
+
+	// Try with the alternative device no of 1000b to read the Vendor & Device ID
+	outl((unsigned)0x80000000 | 0x8000, CAR);
+	reg = inl(CDR);
+	if (((reg >> 16) & 0xFFFF) = 0x0510)
+		return (0x8000);
+
+	return 0; // Couldn't find it
+}
+
+static unsigned long scx200_get_baseaddr(unsigned long addr)
+{
+	// Retrieve FnBARx
+	outl(addr, CAR);
+	return ((inl(CDR)) & 0xFFFFFFC0);
+}
+
+static int scx200_acb_detect_sioaddr(void)
+{
+	int base;
+	unsigned long siocfg_in, lad_en;
+
+	deviceno = scx200_get_pci_deviceno();
+	if (!deviceno)
+		return 0;
+	DBG("deviceno is %#x\n",deviceno);
+
+	// Read IO_SIOCFG_IN 
+	base = scx200_get_baseaddr(0x80000500 | deviceno | 0x10); // Retrueve F5BAR0
+	siocfg_in = ((inl(base))>>25)&3;
+	DBG("IO_SIOCFG_IN is %x\n", (unsigned int) siocfg_in);
+	switch (siocfg_in) {
+	case 0:		// SIO disabled!
+	case 1:		// SIO configuration access denied!
+		return 0;
+	case 2:		// Base address 1 (2e/2f) selected (or perhaps 4e/4f)
+		base = scx200_get_baseaddr(0x80000000 | deviceno | 0x14); // Retrieve F0BAR1
+		// Get LAD_EN - LPC address enable register: bit 14 is "LPC alternate SIO addressing"
+		lad_en = ((inl(base+0x10))>>14)&1;
+		DBG("LPC alternate SIO addressing is %x\n", (unsigned int) lad_en);
+		return (lad_en ? 0x4e : 0x2e);
+	case 3:		// Base address 2 (15c/15d) selected
+		return 0x15c;
+	}
+
+	return 0;
+}
+
+static int scx200_acb_detect_baseaddr(int sio, int *acb1, int* acb2)
+{
+	unsigned char h1,l1,h2,l2;
+	int val = sio+1;
+
+	// First let's check for the ID of the SIO
+	printk(KERN_DEBUG NAME ": Probing for SIO at address %#x\n",sio);
+	outb(0x20,sio); // SIO ID register
+	if (inb(val) != 0xF5) {
+		*acb1 = 0;
+		*acb2 = 0;
+		printk(KERN_DEBUG NAME ": SIO not found at %x\n",sio);
+		return -1;
+	}
+
+	// Select ACB0 (LDN 5)
+	outb(0x07,sio); // LDN select register
+	outb(0x05,val); // Choose LDN 5 ie ACCESS.bus 1
+	outb(0x60,sio); // Select I/O port 0 base addr
+	h1 = inb(val);
+	outb(0x61,sio);
+	l1 = inb(val);
+
+	// Select ACB1 (LDN 6)
+	outb(0x07,sio); // LDN select register
+	outb(0x06,val); // Choose LDN 6 ie ACCESS.bus 2
+	outb(0x60,sio); // Select I/O port 0 base addr
+	h2 = inb(val);
+	outb(0x61,sio);
+	l2 = inb(val);
+	*acb1 = (h1 << 8) | l1;
+	*acb2 = (h2 << 8) | l2;    
+	if (*acb1 = 0xffff)   // Not a valid address
+		*acb1 = 0;
+	if (*acb2 = 0xffff)   // Not a valid address
+		*acb2 = 0;
+	printk(KERN_DEBUG NAME ": Found ACB0 at %#x, ACB1 at %#x\n",*acb1,*acb2);
+	return 0;
+}
+
 static void scx200_acb_machine(struct scx200_acb_iface *iface, u8 status)
 {
 	const char *errmsg;
@@ -404,6 +518,11 @@
 static int scx200_acb_probe(struct scx200_acb_iface *iface)
 {
 	u8 val;
+	u8 acbctl1,acbctl2;
+
+	// In case probing fails
+	acbctl1 = inb(ACBCTL1);
+	acbctl2 = inb(ACBCTL2);
 
 	/* Disable the ACCESS.bus device and Configure the SCL
            frequency: 16 clock cycles */
@@ -411,7 +530,7 @@
 
 	if (inb(ACBCTL2) != 0x70) {
 		DBG("ACBCTL2 readback failed\n");
-		return -ENXIO;
+		goto errout;
 	}
 
 	outb(inb(ACBCTL1) | ACBCTL1_NMINTE, ACBCTL1);
@@ -419,7 +538,7 @@
 	val = inb(ACBCTL1);
 	if (val) {
 		DBG("disabled, but ACBCTL1=0x%02x\n", val);
-		return -ENXIO;
+		goto errout;
 	}
 
 	outb(inb(ACBCTL2) | ACBCTL2_ENABLE, ACBCTL2);
@@ -429,10 +548,15 @@
 	val = inb(ACBCTL1);
 	if ((val & ACBCTL1_NMINTE) != ACBCTL1_NMINTE) {
 		DBG("enabled, but NMINTE won't be set, ACBCTL1=0x%02x\n", val);
-		return -ENXIO;
+		goto errout;
 	}
 
 	return 0;
+
+ errout:	// Try to restore clobbered registers
+	outb(acbctl1, ACBCTL1);
+	outb(acbctl2, ACBCTL2);
+	return -ENXIO;
 }
 
 static int  __init scx200_acb_create(int base, int index)
@@ -508,6 +632,10 @@
 {
 	int i;
 	int rc;
+	int probe = 1;
+	int acb1_base = 0;
+	int acb2_base = 0;
+	int sio = 0;
 
 	pr_debug(NAME ": NatSemi SCx200 ACCESS.bus Driver\n");
 
@@ -517,11 +645,42 @@
 
 	rc = -ENXIO;
 	for (i = 0; i < MAX_DEVICES; ++i) {
-		if (base[i] > 0)
+		if (base[i] > 0) {
+			probe = 0;	// A base address was specified, so turn off probing for base
 			rc = scx200_acb_create(base[i], i);
 	}
-	if (scx200_acb_list)
+	}
+	// You need an sioaddr to be able to read the base addresses, so do that first
+	if (probe) {		// No static base addresses specified, so let's probe it
+		for (i=0; i< MAX_SIO_ADDRS; i++) {
+			if (sioaddr[i]) {
+				probe = 0;	// An sio address was specified, so turn off probing for sio
+				if(scx200_acb_detect_baseaddr(sioaddr[i], &acb1_base, &acb2_base) = 0) {
+					if (acb1_base)
+						rc = scx200_acb_create(acb1_base, 0);
+					if (acb2_base)
+						rc = scx200_acb_create(acb2_base, 1);
+					sio = sioaddr[i];
+					break; // No point looking further if the SIO was found
+				}
+			}
+		}
+		if (probe) {					// sioaddr was not specified
+			if (!(sio = scx200_acb_detect_sioaddr()))
+				return -ENODEV;			// couldn't find the sio
+			if(scx200_acb_detect_baseaddr(sio, &acb1_base, &acb2_base) = 0) {
+				if (acb1_base)
+					rc = scx200_acb_create(acb1_base, 0);
+				if (acb2_base)
+					rc = scx200_acb_create(acb2_base, 1);
+			} else {
+				return -ENODEV;			// couldn't read the base addresses
+			}
+		}
+	}
+	if (scx200_acb_list) {
 		return 0;
+	}
 	return rc;
 }
 

[lm-sensors] Address configuration for scx200_acb

[Jim Cromie]

Thomas Andrews wrote:
> Hi Alex,
>
> On Wed, May 03, 2006 at 01:39:45PM +0200, Alexander Krause wrote:
>
>   
>> And I'm open for tests ;-)
>>     
>
> OK, I'm attaching a patch based on:
>
> /usr/src/linux-2.6.15/drivers/i2c/busses/scx200_acb.c
>
> This patch is to set up the base addresses automatically without
> probing (or guessing.)
>
> As I mentioned earlier the patch is untested because I am actually
> making the changes on a 2.4 kernel. Please let me know if *anything*
> goes wrong on 2.6. If it breaks, I will only be able to get a 2.6 dev
> system up next week to debug it :(
>
>   
This is premature, and hugely incomplete - (basically one observation)

Ill try to give it a go RSN   (FLW)

> +
> +	// Select ACB0 (LDN 5)
> +	outb(0x07,sio); // LDN select register
> +	outb(0x05,val); // Choose LDN 5 ie ACCESS.bus 1
> +	outb(0x60,sio); // Select I/O port 0 base addr
> +	h1 = inb(val);
> +	outb(0x61,sio);
> +	l1 = inb(val);
>   


please use these helper functions - lifted verbatim from another patch.
These show up in various superio drivers in lm-sensors, consistency is good,
and will help later if we need to mediate access to the SIO port,
to share it amongst multiple drivers.

+
+static inline void superio_outb(int reg, int val)
+{
+	outb(reg, SIO_REG_CIP);
+	outb(val, SIO_REG_DIP);
+}
+
+static inline int superio_inb(int reg)
+{
+	outb(reg, SIO_REG_CIP);
+	return inb(SIO_REG_DIP);
+}
+
+static inline void superio_select(int ldn)
+{
+	outb(SIO_VT1211_LDN, SIO_REG_CIP);
+	outb(ldn, SIO_REG_DIP);
+}
+
+static inline void superio_enter(void)
+{
+	outb(0x87, SIO_REG_CIP);
+	outb(0x87, SIO_REG_CIP);
+}
+
+static inline void superio_exit(void)
+{
+	outb(0xaa, SIO_REG_CIP);
+}


BTW - can anyone clarify when / why to use the _p variants,
and how they relate to barriers (mem & otherwize)


>  
>  static int  __init scx200_acb_create(int base, int index)
> @@ -508,6 +632,10 @@
>  {
>  	int i;
>   

At 1st, this looked like recursion.
I havent actually checked, but it would be nice if you used the -p option
   -p     Show which C function each change is in.

> +					if (acb1_base)
> +						rc = scx200_acb_create(acb1_base, 0);
> +					if (acb2_base)
> +						rc = scx200_acb_create(acb2_base, 1);
> +					sio = sioaddr[i];
> +					break; // No point looking further if the SIO was found
> +				}
> +			}
>   

and eventually, you'll probly need to do something about the nesting level.
// and the c++ comments. :-(

[lm-sensors] Address configuration for scx200_acb

[Thomas Andrews]

Hi Jim,

On Wed, May 03, 2006 at 11:24:23PM -0400, Jim Cromie wrote:

> This is premature, and hugely incomplete - (basically one observation)

Even so I really appreciate it! :)
 
> >+	// Select ACB0 (LDN 5)
> >+	outb(0x07,sio); // LDN select register
> >+	outb(0x05,val); // Choose LDN 5 ie ACCESS.bus 1
> >+	outb(0x60,sio); // Select I/O port 0 base addr
> >+	h1 = inb(val);
> >+	outb(0x61,sio);
> >+	l1 = inb(val);
> >  
> 
> please use these helper functions - lifted verbatim from another patch.
> These show up in various superio drivers in lm-sensors, consistency is good,
> and will help later if we need to mediate access to the SIO port,
> to share it amongst multiple drivers.
> 
> +
> +static inline void superio_outb(int reg, int val)
> +{
> +	outb(reg, SIO_REG_CIP);
> +	outb(val, SIO_REG_DIP);
> +}
> +
> +static inline int superio_inb(int reg)
> +{
> +	outb(reg, SIO_REG_CIP);
> +	return inb(SIO_REG_DIP);
> +}
> +
> +static inline void superio_select(int ldn)
> +{
> +	outb(SIO_VT1211_LDN, SIO_REG_CIP);
> +	outb(ldn, SIO_REG_DIP);
> +}
> +
> +static inline void superio_enter(void)
> +{
> +	outb(0x87, SIO_REG_CIP);
> +	outb(0x87, SIO_REG_CIP);
> +}
> +
> +static inline void superio_exit(void)
> +{
> +	outb(0xaa, SIO_REG_CIP);
> +}

I can't use those helper functions as is, because the whole purpose of
my patch is so that you can move away from using a #define for
SIO_REG_CIP. I'll write some equivalents for this module which use a
parameter instead.

> I havent actually checked, but it would be nice if you used the -p option
>   -p     Show which C function each change is in.

Thanks, I will use -p in future.

> and eventually, you'll probly need to do something about the nesting level.
> // and the c++ comments. :-(

I've changed all the comments to /* */ style.

Regards,
Thomas

[lm-sensors] Address configuration for scx200_acb

[Thomas Andrews]

Hi Jim,

On Fri, May 05, 2006 at 10:39:50AM -0400, Jim Cromie wrote:

[...]

> Im looking for the right way to coordinate use of Super-IO control
> ports amongst the drivers that would use them.  While you and I dont
> actually compete for the same port (mines at 2E,2F), you clearly have
> a clue about these things, and could either; - provide me with one (if
> you see that I need it ;-) - add it into your thinking on your driver
> (so that its easy to fit in a port-lock-coordinator later) - tell me
> that Ive more or less thought of everything that seems relevant.  - or
> some combo of them..

I've had a look at the links you gave, and it made a lot of sense to me
because I've been worrying about collisions in usage of the SIO. I'm
pretty sure I can make good use of your module.

One thing that I must mention though, is that in order to discover the
actual SIO address, I have had to access other regions, and these are
not always the same; the actual address depends on a) the bootloader,
and b) how pins are strapped.

The long and short of this is that I will need to make use of your
locking mechanism on all of the addresses, not just on the SIO address.
Looking at your code, that doesn't seem to be a problem though. I only
need to lock 3 or 4 addresses briefly.

If my explanation is too vague, I'll send you my module, and you can
have a look to see how I do it, but it's not very readable unless you
have the sc1100 data-sheet close at hand. Just shout if you want to see
it.
 
[...]

> Im still on the hook for giving your code a compile, load, try.  I
> might need some guidance on the try part, I'll email on that if/when I
> do.

Thanks. But you could also hang on until I submit a more final version,
because I've drastically improved the discovery of the SIO address now,
plus got interrupt-driven mode working nicely.

> btw, wheres za ?

South Africa - Down at the very bottom in a town called Cape Town

Cheers,
Thomas

[lm-sensors] Address configuration for scx200_acb

[Jim Cromie]

Thomas Andrews wrote:
> Hi Jim,
>
> On Fri, May 05, 2006 at 10:39:50AM -0400, Jim Cromie wrote:
>
> [...]
>
>   
>> Im looking for the right way to coordinate use of Super-IO control
>> ports amongst the drivers that would use them.  While you and I dont
>> actually compete for the same port (mines at 2E,2F), you clearly have
>> a clue about these things, and could either; - provide me with one (if
>> you see that I need it ;-) - add it into your thinking on your driver
>> (so that its easy to fit in a port-lock-coordinator later) - tell me
>> that Ive more or less thought of everything that seems relevant.  - or
>> some combo of them..
>>     
>
> I've had a look at the links you gave, and it made a lot of sense to me
> because I've been worrying about collisions in usage of the SIO. I'm
> pretty sure I can make good use of your module.
>
>   
I should follow up:  Heres a newer-cleaner version of the locks coordinator,
plus a 2nd patch that uses it, during init.  I had it running against 
rc5-mm3,
quite unspectacular . ie good.  Comments welcome.


> One thing that I must mention though, is that in order to discover the
> actual SIO address, I have had to access other regions, and these are
> not always the same; the actual address depends on a) the bootloader,
> and b) how pins are strapped.
>   
Please look at find_superio() - its a more active wrapper on get_superio().
Here it is, as used in the 2nd patch:

+/* 0-terminated search vectors for find_superio() */
+static u8 cmd_addrs[] = { 0x2e, 0x4e, 0 };
+static u8 devids[] = { 0xE1, 0xE8, 0xE4, 0xE5, 0xE9, 0 };
+static struct superio* gate;
+

...

-	int i;
+	int i, status = -ENODEV;
+
+	gate = find_superio(cmd_addrs, DEVID, devids);
+	if (!gate) {
+		printk(KERN_WARNING "pc87360: superio port not detected, "
+		       "module not intalled.\n");
+		goto unlock;
+	}
+	/* this driver only uses superio port during init */
+	printk(KERN_WARNING "pc87360: locking superio \n");
+	mutex_lock(&gate->lock);


Code has had only cursory testing, but it feels tighter 
now with the 2nd version of the patch


> The long and short of this is that I will need to make use of your
> locking mechanism on all of the addresses, not just on the SIO address.
> Looking at your code, that doesn't seem to be a problem though. I only
> need to lock 3 or 4 addresses briefly.
>   
Id want to see the code to understand what youre talking about,
 *and* Im way far from a lock-meister, (as Ive probably just admitted in 
the patches ;)
but ..

- maybe I need a try-to-allocate-lock.
- im hoping your issue vanishes when you see find-superio
- whats protected by any lock is by agreement of users coordinating via it.
    disregarding agreements is B&E (breakin & entry)

Whats -all- the addresses ?  do you mean those in regular ISA space ?
Im not sure you need the 'shared' lock for that,  does  in*_p()  work 
correctly ?
( I fret from ignorance)

All that squawking aside, if you have agreement amongst the users of that
lock, there shouldnt be a problem.  This is independent of whether or not
the locks-coordinator ever knew about them -

You could share the lock addr amongst any group that you want,
and coordinate that way, the lockmanager has ;
-    forgotten you asked for a superio-port,
-   knows only that someone asked,
         it found one, and allocated a lock for future sharing

changing terms midstream:
    the locks-provider/vendor   sells the lock, and customer goes away...

      
it knows someone asked


> If my explanation is too vague, I'll send you my module, and you can
> have a look to see how I do it, but it's not very readable unless you
> have the sc1100 data-sheet close at hand. Just shout if you want to see
> it.
>  
>   
yes

> [...]
>
>   
>> Im still on the hook for giving your code a compile, load, try.  I
>> might need some guidance on the try part, I'll email on that if/when I
>> do.
>>     
>
>   
I have something to report :-(

soekris:~/pinlab# modprobe scx200_acb
[ 4455.721842] scx200_acb: probe failed
[ 4455.731069] scx200_acb: probe failed
FATAL: Error inserting scx200_acb 
(/lib/modules/2.6.17-rc6-gpio-sk/kernel/drivers/i2c/busses/scx200_acb.ko): 
No such device or address

This is a hacked kernel, in modules closely related to yours (sharing 
the chip),
so I could easily have trashed that symbol somehow.  This was several
days ago when I found this, so I figured I better hit send - in whatever 
form I could muster.

IIRC I also tried something recent of yours, against vanilla rc6, with 
rejects.
I'll repeat shortly so I can talk specifics, or I'll see a proto-patch 
from you ;-)

> Thanks. But you could also hang on until I submit a more final version,
> because I've drastically improved the discovery of the SIO address now,
> plus got interrupt-driven mode working nicely.
>
>   
That sounds cool - I very much want to take this chance to test for you,
and in so doing get the 'guided tour' :-)



So youre also on an SC-1100 ?   What board ? 
Im on a soekris net-4801

soekris:~/pinlab# cat /proc/cpuinfo
processor       : 0
vendor_id       : Geode by NSC
cpu family      : 5
model           : 9
model name      : Unknown
stepping        : 1
cpu MHz         : 266.694
fdiv_bug        : no
hlt_bug         : no
f00f_bug        : no
coma_bug        : no
fpu             : yes
fpu_exception   : yes
cpuid level     : 2
wp              : yes
flags           : fpu tsc msr cx8 cmov mmx cxmmx up
bogomips        : 534.87

>> btw, wheres za ?
>>     
>
> South Africa - Down at the very bottom in a town called Cape Town
>
> Cheers,
> Thomas
>
>   
have a great day
-jimc
-------------- next part --------------
A non-text attachment was scrubbed...
Name: superio-locks-add-module.patch
Type: text/x-patch
Size: 6257 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060609/1d9e208f/attachment-0002.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: superio-locks-usein-pc87360.patch
Type: text/x-patch
Size: 2038 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060609/1d9e208f/attachment-0003.bin 

Re: [lm-sensors] Address configuration for scx200_acb

[Jean Delvare]

Hi Thomas,

A long time ago...

On Thu, 4 May 2006 08:32:05 +0200, Thomas Andrews wrote:
> Hi Jim,
> 
> On Wed, May 03, 2006 at 11:24:23PM -0400, Jim Cromie wrote:
> 
> > This is premature, and hugely incomplete - (basically one observation)
> 
> Even so I really appreciate it! :)
>  
> > >+	// Select ACB0 (LDN 5)
> > >+	outb(0x07,sio); // LDN select register
> > >+	outb(0x05,val); // Choose LDN 5 ie ACCESS.bus 1
> > >+	outb(0x60,sio); // Select I/O port 0 base addr
> > >+	h1 = inb(val);
> > >+	outb(0x61,sio);
> > >+	l1 = inb(val);
> > >  
> > 
> > please use these helper functions - lifted verbatim from another patch.
> > These show up in various superio drivers in lm-sensors, consistency is good,
> > and will help later if we need to mediate access to the SIO port,
> > to share it amongst multiple drivers.
> > 
> > +
> > +static inline void superio_outb(int reg, int val)
> > +{
> > +	outb(reg, SIO_REG_CIP);
> > +	outb(val, SIO_REG_DIP);
> > +}
> > +
> > +static inline int superio_inb(int reg)
> > +{
> > +	outb(reg, SIO_REG_CIP);
> > +	return inb(SIO_REG_DIP);
> > +}
> > +
> > +static inline void superio_select(int ldn)
> > +{
> > +	outb(SIO_VT1211_LDN, SIO_REG_CIP);
> > +	outb(ldn, SIO_REG_DIP);
> > +}
> > +
> > +static inline void superio_enter(void)
> > +{
> > +	outb(0x87, SIO_REG_CIP);
> > +	outb(0x87, SIO_REG_CIP);
> > +}
> > +
> > +static inline void superio_exit(void)
> > +{
> > +	outb(0xaa, SIO_REG_CIP);
> > +}
> 
> I can't use those helper functions as is, because the whole purpose of
> my patch is so that you can move away from using a #define for
> SIO_REG_CIP. I'll write some equivalents for this module which use a
> parameter instead.
> 
> > I havent actually checked, but it would be nice if you used the -p option
> >   -p     Show which C function each change is in.
> 
> Thanks, I will use -p in future.
> 
> > and eventually, you'll probly need to do something about the nesting level.
> > // and the c++ comments. :-(
> 
> I've changed all the comments to /* */ style.

Any news about that patch? It seems that you had something almost ready
and it was lost?

It would be great if you could update this patch so that it applies on
top of kernel 2.6.21 or later, and resubmit it.

Thanks,
-- 
Jean Delvare

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