Add support for ACPI ig4 device and fix i2c write/read sequence

[gonzo]

Summary

• Add ACPI attach/detach methods for ig4 devices
• Direction change in RDWR sequence is valid case for I2C operation, so do not generate error in this case

Test Plan

Tested on Minnowboard Turbot with TMP102 attached using following program:
https://people.freebsd.org/~gonzo/tmp102.c

[gonzo]

Fix build for architectures without ACPI support

[avg]

sys/dev/ichiic/ig4_iic.c

423 ↗

(On Diff #22792)

This change contradicts my understanding of I2C protocol. Which is that a slave always obeys the read/write bit in the address sequence.
So, I do not see how the slave is supposed to know that the direction of transfer changes if there is no start / repeated start sequence with a new value of the read/write bit.

Could you please provide some diagram or a bit sequence to help me understand how this kind of transfer can work?
E.g., let's consider this:

struct iic_msg msgs[] = {
     { slave, IIC_M_WR | IIC_M_NOSTOP, 1, &byte_to_send },
     { slave, IIC_M_RD | IIC_M_NOSTART, 1, &byte_to_read },
};

How would that look on the wire?

[avg]

sys/dev/ichiic/ig4_iic.c

423 ↗

(On Diff #22792)

By the way, I am confident that if you restore the check and remove IIC_M_NOSTART from your test program, then the program will work just the same. I think that having IIC_M_NOSTART is a mistake (explained in my previous comment). And the repeated start is automatically generated by the hardware anyway:

RESTART (RESTART): This bit controls whether a RESTART is issued before the
byte is sent or received.

1: a RESTART is issued before the data is sent/received (according to the value of
CMD), regardless of whether or not the transfer direction is changing from the
previous command.

0: a RESTART is issued only if the transfer direction is changing from the previous
command

As you can see, RESTART is issued in any case if the direction changes.

[gonzo]

sys/dev/ichiic/ig4_iic.c

423 ↗

(On Diff #22792)

Yes, you're right. It was misunderstanding of i2c protocol on my side. I checked TMP102 datasheet and there is stop/start sequence in timing diagram that I somehow managed to ignore when I was looking at it previous time. With this restriction back and without NOSTOP/NOSTART in app it works just fine.

[gonzo]

Bring back sanity check for stop/start sequence on direction change

[avg]

I am just curious what is the motivation to add the ACPI attachment?
Are PCI devices hidden somehow on some platforms?

If they are still visible as regular PCI devices, then maybe it would be simpler to just add more PCI IDs?
I do not see a big difference between a table of PCI IDs and a table of non-standardized ACPI IDs.

sys/dev/ichiic/ig4_acpi.c

86 ↗

(On Diff #22806)

Redundant. newbus code zeroes out softc before a device is probed.

89 ↗

(On Diff #22806)

I think that lock initialization should go to the common ig4iic_attach.
And likewise for the destruction below.

147 ↗

(On Diff #22806)

One return is enough :-)

sys/dev/ichiic/ig4_iic.c

423 ↗

(On Diff #22792)

I think that you could keep NOSTOP, it seems that the repeated start should be supported both by the controller and the slave device. But that's a very minor detail.

[gonzo]

Fix according to comments

sys/dev/ichiic/ig4_acpi.c

86 ↗

(On Diff #22806)

Yes, it's just copy-paste from PCI part of the module. I also removed it there

[gonzo]

Is there anything else that should be fixed in this version of the patch?

[avg]

No specific requests, but I am still curious about the question I asked earlier:

I am just curious what is the motivation to add the ACPI attachment?
Are PCI devices hidden somehow on some platforms?

If they are still visible as regular PCI devices, then maybe it would be simpler to just add more PCI IDs?
I do not see a big difference between a table of PCI IDs and a table of non-standardized ACPI IDs.

[gonzo]

In D8742#184308, @avg wrote:

No specific requests, but I am still curious about the question I asked earlier:

I am just curious what is the motivation to add the ACPI attachment?
Are PCI devices hidden somehow on some platforms?

If they are still visible as regular PCI devices, then maybe it would be simpler to just add more PCI IDs?
I do not see a big difference between a table of PCI IDs and a table of non-standardized ACPI IDs.

Oops. Sorry, I overlooked the question somehow. I have Minnowboard Turbot device and GPIO and SPI are both ACPI-only. It never occurred to me to check PCI devices for I2C. Apparently there is PCI device for SMBus. I am not sure if it's ig4-compatible. IO and memory windows are only 32 bytes and outside this window register reads return 0xffffffff. pciconf output for the device:

none2@pci0:0:31:3:      class=0x0c0500 card=0x72708086 chip=0x0f128086 rev=0x11 hdr=0x00
    vendor     = 'Intel Corporation'
    device     = 'Atom Processor E3800 Series SMBus Controller'
    class      = serial bus
    subclass   = SMBus
    bar   [10] = type Memory, range 32, base 0x9081c000, size 32, enabled
    bar   [20] = type I/O Port, range 32, base 0x2000, size 32, enabled

ACPI device on the other hand works fine.

[avg]

Most likely the SMBus device is a different device as the platform has both SMBus and (multiple) I2C controllers.
I now understand your change better. Thanks!