Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44507 )
Change subject: soc/intel/apl: Add ACPI for SMBus control ......................................................................
Patch Set 9:
(10 comments)
Oh no
https://review.coreboot.org/c/coreboot/+/44507/9/src/soc/intel/apollolake/ac... File src/soc/intel/apollolake/acpi/smbus.asl:
https://review.coreboot.org/c/coreboot/+/44507/9/src/soc/intel/apollolake/ac... PS9, Line 8: #if CONFIG(SOC_INTEL_COMMON_BLOCK_SMBUS_ACPI_DRIVER) Why not put this inside a smbus_methods.asl in common code and include it if desired? You can place the first part inside a `Scope (SBUS)` block for simplicity
https://review.coreboot.org/c/coreboot/+/44507/9/src/soc/intel/apollolake/ac... PS9, Line 48: // 0x00 : OK : // 0x07 : Unknown Failure : // 0x10 : Address Not Acknowledged : // 0x11 : Device Error : // 0x12 : Command Access Denied : // 0x13 : Unknown Error : // 0x17 : Device Access Denied : // 0x18 : Timeout : // 0x19 : Unsupported Protocol : // 0x1A : Bus Busy : // 0x1F : PEC (CRC-8) Error Use #defines for these?
https://review.coreboot.org/c/coreboot/+/44507/9/src/soc/intel/apollolake/ac... PS9, Line 65: One I would:
s/Zero/0/g s/One/1/g
https://review.coreboot.org/c/coreboot/+/44507/9/src/soc/intel/apollolake/ac... PS9, Line 82: // Transaction isn't complete. Check for timeout, and if not, sleep 10ms and line over 96 characters. Please try to split this into two parts of about the same length.
https://review.coreboot.org/c/coreboot/+/44507/9/src/soc/intel/apollolake/ac... PS9, Line 92: 0x0A Does using hex make this easier to read?
https://review.coreboot.org/c/coreboot/+/44507/9/src/soc/intel/apollolake/ac... PS9, Line 145: If ((Arg0 != 0x03)) // Read Quick Command codes should really be #defined somewhere. Also, this would be better done within the first switch-case like block inside the mutex acquired section.
https://review.coreboot.org/c/coreboot/+/44507/9/src/soc/intel/apollolake/ac... PS9, Line 168: it they (command code and data length)
https://review.coreboot.org/c/coreboot/+/44507/9/src/soc/intel/apollolake/ac... PS9, Line 208: 0x03E8 huh?
https://review.coreboot.org/c/coreboot/+/44507/9/src/soc/intel/apollolake/ac... PS9, Line 219: { Read quick?
https://review.coreboot.org/c/coreboot/+/44507/9/src/soc/intel/apollolake/ac... PS9, Line 334: If ((Arg0 != 0x02)) // Write Quick : { : If ((Arg0 != 0x04)) // Send Byte : { : If ((Arg0 != 0x06)) // Write Byte : { : If ((Arg0 != 0x08)) // Write Word : { : If ((Arg0 != 0x0A)) // Write Block : { : Local0 [Zero] = 0x19 : Return (Local0) : } : } : } : } : } Why not check this within the mutex section?