On 04/09/2014 12:37 PM, Jean Delvare wrote: > Hi Prarit, > > On Wed, 9 Apr 2014 12:22:43 -0400, Prarit Bhargava wrote: >> RFC and a work in progress ... I need to go through and do a bunch of error >> condition checking, more testing, etc. I'm just throwing this out there to see >> if anyone has any major concerns about doing something like this. >> >> TODO: >> >> - decipher error returns from ACPI AML operations (and implement those too) >> - additional error checking from i2c and acpi function calls (this code is >> not robust enough) >> - testing >> >> P. >> >> ----8<---- >> >> Some ACPI tables on new systems implement an SBUS device in ACPI. This results >> in a conflict with the ACPI tables and the i2c-i801 driver over the address >> space. As a result the i2c-i801 driver will not load. To workaround this, we >> have users use the kernel parameter "acpi_enforce_resources=lax". This, >> isn't a good solution as I've seen other conflicts on some systems that are >> also overriden. I thought about implementing an i2c-core kernel parameter and >> a wrapper function for acpi_check_resource_conflict() but that seems rather >> clunky and doesn't resolve the issue of the shared resource between the ACPI >> and the device. >> >> There isn't any documentaton on Intel's website about the SBUS device but from >> reading the acpidump it looks like the operations are straightforward. >> >> SSXB >> transmit one byte >> SRXB >> receive one byte >> SWRB >> write one byte >> SRDB >> read one byte >> SWRW >> write one word >> SRDW >> read one word >> SBLW >> write block >> SBRW >> read block >> >> I've implemented these as an i2c algorithm so that users who cannot load the >> regular i801 i2c driver can at least get the functionality they are looking >> for. > > Writing a driver for the ACPI interface to the SMBus is IMHO a very > good idea, thanks for doing that. > > However I have two technical concerns with your approach: > > 1* What you wrote is NOT an "i2c algo". i2c "algorithms" are abstracted > I2C implementations, not correlated with any hardware or BIOS specific > interfaces. If you check other drivers under i2c/algos you'll see they > do NOT call i2c_add_adapter (although they may implement helper > wrappers around that function.) Hardware-specific drivers which build > on top of the algo driver are responsible for that. > > What you wrote is much more like i2c/busses/i2c-scmi.c, so it is > actually an i2c _bus_ driver. > > 2* You are creating a link between i2c-i801 and your driver, where IMHO > there should not be any. Both drivers are independent, and from the > kernel's perspective, they are not even for the same hardware. In your > case the ACPI interface is backed by an i801-like chip, but the same > interface could totally be implemented on top of any other chip. > > So I invite you to make your driver an independent i2c bus driver much > like i2c-scmi. Hey Jean -- thanks for the valuable feedback. This is exactly why I RFC'd it :) before I got in too deep :) I'll rework the patch with your suggestions. I may have (possibly dumb) questions and I hope that's okay .. P. > > Thanks, > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
