Enhance VoodooI2CDeviceNub #468

williambj1 · 2021-08-11T11:18:54Z

Hi developers! This PR includes enhancements for VoodooI2CDeviceNub.

Support reading I2C Serial Bus declaration from _DSM
Previously, _DSM will not be evaluated if SBFB (aka "I2C Serial Bus declaration") was not returned in _CRS, here is an example from an Acer Laptop

Method (_CRS, 0, Serialized)  // _CRS: Current Resource Settings
{
    Local0 = Zero
    Local1 = Zero
    Local1 = DerefOf (DerefOf (TPID [Local0]) [Zero])
    While (((Local1 != 0xFE) && (Local1 != TPDF)))
    {
        Local0++
        If ((Local0 >= SizeOf (TPID)))
        {
            Break
        }

        Local1 = DerefOf (DerefOf (TPID [Local0]) [Zero])
    }

    ADR0 = DerefOf (DerefOf (TPID [Local0]) [One])
    HID2 = DerefOf (DerefOf (TPID [Local0]) [0x02])
    If ((OSYS < 0x07DC))
    {
        Return (SBFI) // Returned here
    }

    If (Ones)
    {
        Return (ConcatenateResTemplate (SBFB, SBFG))
    }

    Return (ConcatenateResTemplate (SBFB, SBFI))
}

Current implementation in Repo:

VoodooI2C/VoodooI2C/VoodooI2C/VoodooI2CDevice/VoodooI2CDeviceNub.cpp

Lines 192 to 203 in 1671524

    
           if (crs_parser.found_i2c) { 
        
               use_10bit_addressing = crs_parser.i2c_info.address_mode_10Bit; 
        
               setProperty("addrWidth", use_10bit_addressing ? 10 : 7, 8); 
        
               i2c_address = crs_parser.i2c_info.address; 
        
               setProperty("i2cAddress", i2c_address, 16); 
        
               setProperty("sclHz", crs_parser.i2c_info.bus_speed, 32); 
        
           } else { 
        
               IOLog("%s::%s Could not find an I2C Serial Bus declaration\n", getName(), acpi_device->getName()); 
        
               return kIOReturnNotFound; 
        
           }

Refactor VoodooI2CDeviceNub::getDeviceResources()
Clarify skipping APIC Interrupt mode
This PR does not change the behavior of skipping interrupts (skips setting gpio values). AFAIK there is no way to avoid APIC interrupts in VoodooI2C (and it doesn't make sense to do so, although it can be accomplished by modifying ACPI), please correct me if I'm wrong.
Slightly adjust logs and macro names
- Data returned from _DSM using the TP7G index doesn't only contain GPIO resources The macro name was adjusted based on docs from Microsoft; the third argument (Arg2) is always called as the "function index".
- The log that complains about unsupported index is adjusted since it prints whenever _DSM returns useless data. Calls with any uuid or function indexes will end up printing the line if the evaluation above is not satisfied.

CC @zhen-zen

williambj1 · 2021-08-11T11:36:42Z

~~I just realized this PR would break the fallback of choosing _DSM if only SBFB was returned from _CRS. I will fix that ASAP.~~

Fixed in the latest commit

GZXiaoBai · 2021-08-14T13:58:11Z

That's great! I've tried this! Everything is working! I hope this can be adopted.

kprinssu · 2021-08-14T22:08:16Z

@williambj1 Please give me some time to review this. I am hoping to do so by Monday evening.

@mat121212 Your comment was deleted due to you asking for compiled kexts and not contributing to the development of this PR. Please keep the discussion to the PR at hand.

williambj1 · 2021-08-15T05:42:27Z

Sample DSDT that benefits from this patch:

HuaweiMateBook14-origin.zip

Device path: _SB.PCI0.I2C1.TPL1

kprinssu

Good work! Minor comments and will be good to merge if we can add some documentation on the changes.

By documentation, we should add the sample DSDT code to the Github.io pages.

VoodooI2C/VoodooI2C/VoodooI2CDevice/VoodooI2CDeviceNub.cpp

williambj1 · 2021-08-16T13:47:10Z

Thank you for taking your precious time to review my changes!

Good work! Minor comments and will be good to merge if we can add some documentation on the changes.

By documentation, we should add the sample DSDT code to the Github.io pages.

I'm not quite sure how to handle this properly to be honest, as the main goal of this PR is to allow more devices to operate properly without ACPI modifications/hacks.

kprinssu · 2021-08-16T17:52:55Z

@ben9923 Can you please take a look? Otherwise, this looks to be good to me and I will merge it in on Thursday.

- Support reading I2C Serial Bus declaration from `_DSM` - Refactor `VoodooI2CDeviceNub::getDeviceResources()` - Clarify skipping APIC Interrupt mode - Slightly adjust logs

zhen-zen · 2021-08-22T01:29:28Z

@williambj1 Just revisited this issue and I'm not in favor of the rename. Function index is everything you put into the Arg2. It will then call the function at specified index. So there's no official definition other than 0, which is a bitmap indicating available functions. Check 9.1.1 _DSM in ACPI spec.

ben9923

Thanks for the PR, I like the idea!

Added just a couple of notes, sorry it took me a while to review :)

VoodooI2C/VoodooI2C/VoodooI2CDevice/VoodooI2CDeviceNub.cpp

VoodooI2C/VoodooI2C/VoodooI2CDevice/VoodooI2CDeviceNub.hpp

kprinssu · 2021-09-02T17:01:55Z

@williambj1 I would love to merge this in but I would like you to address @ben9923's comments before I do so.

williambj1 · 2021-09-05T00:47:09Z

@williambj1 I would love to merge this in but I would like you to address @ben9923's comments before I do so.

I apologize for not being able to respond in the past two weeks. I will adopt these suggestions on Sep 6.

williambj1 · 2021-09-06T14:19:00Z

@ben9923 I apologize again for not being able to resolve those issues earlier. I think I've covered all of the comments, please have a look :)

kprinssu · 2021-09-07T21:13:13Z

Thanks @williambj1. This looks good and merging it in.

VoodooI2C deleted a comment from mat121212 Aug 14, 2021

kprinssu requested changes Aug 16, 2021

View reviewed changes

Enhance VoodooI2CDeviceNub

50f9085

- Support reading I2C Serial Bus declaration from `_DSM` - Refactor `VoodooI2CDeviceNub::getDeviceResources()` - Clarify skipping APIC Interrupt mode - Slightly adjust logs

kprinssu approved these changes Aug 19, 2021

View reviewed changes

Rename TP7G index macro

50f2df9

ben9923 reviewed Aug 25, 2021

View reviewed changes

Rename VoodooI2CACPICRSParser to VoodooI2CACPIResourcesParser

2075d2f

williambj1 added 3 commits September 6, 2021 23:55

Rename TP7G index macro again

4edc88b

Improve parser assignments

2de1baf

Print logs for valid APIC interrupt pins

19f3c08

kprinssu merged commit 385c068 into VoodooI2C:master Sep 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance VoodooI2CDeviceNub #468

Enhance VoodooI2CDeviceNub #468

williambj1 commented Aug 11, 2021 •

edited

Loading

williambj1 commented Aug 11, 2021 •

edited

Loading

GZXiaoBai commented Aug 14, 2021

kprinssu commented Aug 14, 2021 •

edited

Loading

williambj1 commented Aug 15, 2021 •

edited

Loading

kprinssu left a comment •

edited

Loading

williambj1 commented Aug 16, 2021

kprinssu commented Aug 16, 2021

zhen-zen commented Aug 22, 2021 •

edited

Loading

ben9923 left a comment

kprinssu commented Sep 2, 2021

williambj1 commented Sep 5, 2021

williambj1 commented Sep 6, 2021

kprinssu commented Sep 7, 2021

	if (crs_parser.found_i2c) {
	use_10bit_addressing = crs_parser.i2c_info.address_mode_10Bit;
	setProperty("addrWidth", use_10bit_addressing ? 10 : 7, 8);

	i2c_address = crs_parser.i2c_info.address;
	setProperty("i2cAddress", i2c_address, 16);

	setProperty("sclHz", crs_parser.i2c_info.bus_speed, 32);
	} else {
	IOLog("%s::%s Could not find an I2C Serial Bus declaration\n", getName(), acpi_device->getName());
	return kIOReturnNotFound;
	}

Enhance VoodooI2CDeviceNub #468

Enhance VoodooI2CDeviceNub #468

Conversation

williambj1 commented Aug 11, 2021 • edited Loading

williambj1 commented Aug 11, 2021 • edited Loading

GZXiaoBai commented Aug 14, 2021

kprinssu commented Aug 14, 2021 • edited Loading

williambj1 commented Aug 15, 2021 • edited Loading

kprinssu left a comment • edited Loading

Choose a reason for hiding this comment

williambj1 commented Aug 16, 2021

kprinssu commented Aug 16, 2021

zhen-zen commented Aug 22, 2021 • edited Loading

ben9923 left a comment

Choose a reason for hiding this comment

kprinssu commented Sep 2, 2021

williambj1 commented Sep 5, 2021

williambj1 commented Sep 6, 2021

kprinssu commented Sep 7, 2021

williambj1 commented Aug 11, 2021 •

edited

Loading

williambj1 commented Aug 11, 2021 •

edited

Loading

kprinssu commented Aug 14, 2021 •

edited

Loading

williambj1 commented Aug 15, 2021 •

edited

Loading

kprinssu left a comment •

edited

Loading

zhen-zen commented Aug 22, 2021 •

edited

Loading