Patchworkβ [v2,1/2] acpi: support IBM SMBus CMI devices

On some old IBM workstations and desktop computers, the BIOS presents in the
DSDT an SMBus object that is missing the HID identifier that the i2c-scmi
driver looks for.  Modify the ACPI device scan code to insert the missing HID
if it finds an IBM system with such an object.  This patch needs Crane Cai's
update to i2c-scmi to work around incorrectly named methods within the SMBus
control object.

Affected machines: IntelliStation Z20/Z30.  Note that the i2c-i801 driver no
longer works on these machines because of ACPI resource conflicts.

v2 contains minor tweaks suggested by Jean Delvare.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 drivers/acpi/scan.c         |   38 ++++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_drivers.h |    2 ++
 2 files changed, 40 insertions(+), 0 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 26 Oct 2009 13:58:19 -0700, Darrick J. Wong wrote:
> On some old IBM workstations and desktop computers, the BIOS presents in the
> DSDT an SMBus object that is missing the HID identifier that the i2c-scmi
> driver looks for.  Modify the ACPI device scan code to insert the missing HID
> if it finds an IBM system with such an object.  This patch needs Crane Cai's
> update to i2c-scmi to work around incorrectly named methods within the SMBus
> control object.
> 
> Affected machines: IntelliStation Z20/Z30.  Note that the i2c-i801 driver no
> longer works on these machines because of ACPI resource conflicts.
> 
> v2 contains minor tweaks suggested by Jean Delvare.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> ---
> 
>  drivers/acpi/scan.c         |   38 ++++++++++++++++++++++++++++++++++++++
>  include/acpi/acpi_drivers.h |    2 ++
>  2 files changed, 40 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 14a7481..5a24429 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -8,6 +8,7 @@
>  #include <linux/acpi.h>
>  #include <linux/signal.h>
>  #include <linux/kthread.h>
> +#include <linux/dmi.h>
>  
>  #include <acpi/acpi_drivers.h>
>  
> @@ -1014,6 +1015,41 @@ static void acpi_add_id(struct acpi_device *device, const char *dev_id)
>  	list_add_tail(&id->list, &device->pnp.ids);
>  }
>  
> +/*
> + * Old IBM workstations have a DSDT bug wherein the SMBus object
> + * lacks the SMBUS01 HID and the methods do not have the necessary "_"
> + * prefix.  Work around this.
> + */
> +static int acpi_ibm_smbus_match(struct acpi_device *device)
> +{
> +	acpi_handle h_dummy;
> +	struct acpi_buffer path = {ACPI_ALLOCATE_BUFFER, NULL};
> +	int result;
> +
> +	if (!dmi_name_in_vendors("IBM"))
> +		return -ENODEV;
> +
> +	/* Look for SMBS object */
> +	result = acpi_get_name(device->handle, ACPI_SINGLE_NAME, &path);
> +	if (result)
> +		return result;
> +
> +	if (strcmp("SMBS", path.pointer)) {
> +		result = -ENODEV;
> +		goto out;
> +	}
> +
> +	/* Does it have the necessary (but misnamed) methods? */
> +	result = -ENODEV;
> +	if (ACPI_SUCCESS(acpi_get_handle(device->handle, "SBI", &h_dummy)) &&
> +	    ACPI_SUCCESS(acpi_get_handle(device->handle, "SBR", &h_dummy)) &&
> +	    ACPI_SUCCESS(acpi_get_handle(device->handle, "SBW", &h_dummy)))
> +		result = 0;
> +out:
> +	kfree(path.pointer);
> +	return result;
> +}

I'm only half please with this. You change the function named, but it
doesn't follow the calling convention of acpi_dock_match(), which is a
little confusing.

Anyway, I will need an ack from the ACPI people before I can pick this
patch. Or maybe they should even push it upstream themselves.

> +
>  static void acpi_device_set_id(struct acpi_device *device)
>  {
>  	acpi_status status;
> @@ -1064,6 +1100,8 @@ static void acpi_device_set_id(struct acpi_device *device)
>  			acpi_add_id(device, ACPI_BAY_HID);
>  		else if (ACPI_SUCCESS(acpi_dock_match(device)))
>  			acpi_add_id(device, ACPI_DOCK_HID);
> +		else if (!acpi_ibm_smbus_match(device))
> +			acpi_add_id(device, ACPI_SMBUS_IBM_HID);
>  
>  		break;
>  	case ACPI_BUS_TYPE_POWER:
> diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
> index f4906f6..83a2960 100644
> --- a/include/acpi/acpi_drivers.h
> +++ b/include/acpi/acpi_drivers.h
> @@ -65,6 +65,8 @@
>  #define ACPI_VIDEO_HID			"LNXVIDEO"
>  #define ACPI_BAY_HID			"LNXIOBAY"
>  #define ACPI_DOCK_HID			"LNXDOCK"
> +/* Quirk for broken IBM BIOSes */
> +#define ACPI_SMBUS_IBM_HID		"SMBUSIBM"
>  
>  /*
>   * For fixed hardware buttons, we fabricate acpi_devices with HID

On Tue, Oct 27, 2009 at 06:03:32PM +0100, Jean Delvare wrote:

> I'm only half please with this. You change the function named, but it
> doesn't follow the calling convention of acpi_dock_match(), which is a
> little confusing.
> 
> Anyway, I will need an ack from the ACPI people before I can pick this
> patch. Or maybe they should even push it upstream themselves.

I am confused.  Looking at that bunch of ifs, acpi_is_video_device returns 1
for a match and 0 for no match.  acpi_bay_match returns 0 for a match and
-ENODEV for no match, which just happens to work with the ACPI_SUCCESS macro.
acpi_dock_match returns ACPI error codes.  Each of the three existing tests has
different return value semantics, so it is not clear to me which one I should
use.

I didn't think it was correct for my probe function to use the ACPI_STATUS
macro unless it returned ACPI error codes... which it does not.  -ENODEV seemed
appropriate for "no device found".

Is it desirable to clean them all up to follow the same convention?

--D
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 27 Oct 2009 10:30:01 -0700, Darrick J. Wong wrote:
> On Tue, Oct 27, 2009 at 06:03:32PM +0100, Jean Delvare wrote:
> 
> > I'm only half please with this. You change the function named, but it
> > doesn't follow the calling convention of acpi_dock_match(), which is a
> > little confusing.
> > 
> > Anyway, I will need an ack from the ACPI people before I can pick this
> > patch. Or maybe they should even push it upstream themselves.
> 
> I am confused.  Looking at that bunch of ifs, acpi_is_video_device returns 1
> for a match and 0 for no match.  acpi_bay_match returns 0 for a match and
> -ENODEV for no match, which just happens to work with the ACPI_SUCCESS macro.
> acpi_dock_match returns ACPI error codes.  Each of the three existing tests has
> different return value semantics, so it is not clear to me which one I should
> use.

Ah, sorry, I looked at one (don't remember which one) and assumed the
other ones followed the same convention.

> I didn't think it was correct for my probe function to use the ACPI_STATUS
> macro unless it returned ACPI error codes... which it does not.  -ENODEV seemed
> appropriate for "no device found".
> 
> Is it desirable to clean them all up to follow the same convention?

Ideally, yes, but that would be a separate task from what you're up to
at the moment. And anyway this is not in my realm an I am not going to
work on it myself, so my opinion probably doesn't matter that much.
Sorry for bothering you with this in the first place.