Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753193AbZJYJje (ORCPT ); Sun, 25 Oct 2009 05:39:34 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753158AbZJYJjd (ORCPT ); Sun, 25 Oct 2009 05:39:33 -0400 Received: from poutre.nerim.net ([62.4.16.124]:51889 "EHLO poutre.nerim.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753157AbZJYJjb (ORCPT ); Sun, 25 Oct 2009 05:39:31 -0400 Date: Sun, 25 Oct 2009 10:39:32 +0100 From: Jean Delvare To: djwong@us.ibm.com Cc: Crane Cai , Bjorn Helgaas , lenb@kernel.org, linux-kernel , linux-i2c@vger.kernel.org, linux-acpi@vger.kernel.org Subject: Re: [PATCH 1/2] i2c-scmi: support IBM SMBus CMI devices Message-ID: <20091025103932.31ce9a6d@hyperion.delvare> In-Reply-To: <20091023170306.GP26149@tux1.beaverton.ibm.com> References: <20091020231149.GM26149@tux1.beaverton.ibm.com> <20091021023016.GC32413@crane-desktop> <200910210857.13978.bjorn.helgaas@hp.com> <20091021173733.GN26149@tux1.beaverton.ibm.com> <20091022071748.GA17917@crane-desktop> <20091022174304.GO26149@tux1.beaverton.ibm.com> <20091023044451.GB1367@crane-desktop> <20091023170306.GP26149@tux1.beaverton.ibm.com> X-Mailer: Claws Mail 3.5.0 (GTK+ 2.14.4; i586-suse-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4613 Lines: 138 Hi Darrick, On Fri, 23 Oct 2009 10:03:07 -0700, Darrick J. Wong wrote: > *) add a new HID for IBM SMBus CMI devices > *) add methods for IBM SMBus CMI devices > *) hook different HID with different control methods set > > Signed-off-by: Crane Cai When sending patches not written by you, please add as the first line of the comment a "From:" line with the author. Please also add your own Signed-off-by line. > --- > > drivers/i2c/busses/i2c-scmi.c | 31 +++++++++++++++++++++++-------- > 1 files changed, 23 insertions(+), 8 deletions(-) > > > diff --git a/drivers/i2c/busses/i2c-scmi.c b/drivers/i2c/busses/i2c-scmi.c > index b4a55d4..a15aaf4 100644 > --- a/drivers/i2c/busses/i2c-scmi.c > +++ b/drivers/i2c/busses/i2c-scmi.c > @@ -33,6 +33,7 @@ struct acpi_smbus_cmi { > u8 cap_info:1; > u8 cap_read:1; > u8 cap_write:1; > + struct smbus_methods_t *methods; > }; > > static const struct smbus_methods_t smbus_methods = { > @@ -41,8 +42,16 @@ static const struct smbus_methods_t smbus_methods = { > .mt_sbw = "_SBW", > }; > > +/* Some IBM BIOSes omit the leading underscore */ > +static const struct smbus_methods_t ibm_smbus_methods = { > + .mt_info = "SBI_", > + .mt_sbr = "SBR_", > + .mt_sbw = "SBW_", > +}; > + > static const struct acpi_device_id acpi_smbus_cmi_ids[] = { > - {"SMBUS01", 0}, > + {"SMBUS01", (kernel_ulong_t)&smbus_methods}, > + {"SMBUSIBM", (kernel_ulong_t)&ibm_smbus_methods}, > {"", 0} > }; I don't like this method, but as I am neither the author of the driver not its maintainer... so be it. > > @@ -150,11 +159,11 @@ acpi_smbus_cmi_access(struct i2c_adapter *adap, u16 addr, unsigned short flags, > > if (read_write == I2C_SMBUS_READ) { > protocol |= ACPI_SMBUS_PRTCL_READ; > - method = smbus_methods.mt_sbr; > + method = smbus_cmi->methods->mt_sbr; > input.count = 3; > } else { > protocol |= ACPI_SMBUS_PRTCL_WRITE; > - method = smbus_methods.mt_sbw; > + method = smbus_cmi->methods->mt_sbw; > input.count = 5; > } > > @@ -290,13 +299,13 @@ static int acpi_smbus_cmi_add_cap(struct acpi_smbus_cmi *smbus_cmi, > union acpi_object *obj; > acpi_status status; > > - if (!strcmp(name, smbus_methods.mt_info)) { > + if (!strcmp(name, smbus_cmi->methods->mt_info)) { > status = acpi_evaluate_object(smbus_cmi->handle, > - smbus_methods.mt_info, > + smbus_cmi->methods->mt_info, > NULL, &buffer); > if (ACPI_FAILURE(status)) { > ACPI_ERROR((AE_INFO, "Evaluating %s: %i", > - smbus_methods.mt_info, status)); > + smbus_cmi->methods->mt_info, status)); > return -EIO; > } > > @@ -319,9 +328,9 @@ static int acpi_smbus_cmi_add_cap(struct acpi_smbus_cmi *smbus_cmi, > > kfree(buffer.pointer); > smbus_cmi->cap_info = 1; > - } else if (!strcmp(name, smbus_methods.mt_sbr)) > + } else if (!strcmp(name, smbus_cmi->methods->mt_sbr)) > smbus_cmi->cap_read = 1; > - else if (!strcmp(name, smbus_methods.mt_sbw)) > + else if (!strcmp(name, smbus_cmi->methods->mt_sbw)) > smbus_cmi->cap_write = 1; > else > ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Unsupported CMI method: %s\n", > @@ -349,6 +358,7 @@ static acpi_status acpi_smbus_cmi_query_methods(acpi_handle handle, u32 level, > static int acpi_smbus_cmi_add(struct acpi_device *device) > { > struct acpi_smbus_cmi *smbus_cmi; > + struct acpi_device_id *id; > > smbus_cmi = kzalloc(sizeof(struct acpi_smbus_cmi), GFP_KERNEL); > if (!smbus_cmi) > @@ -362,6 +372,11 @@ static int acpi_smbus_cmi_add(struct acpi_device *device) > smbus_cmi->cap_read = 0; > smbus_cmi->cap_write = 0; > > + for (id = (struct acpi_device_id *)acpi_smbus_cmi_ids; id->id[0]; id++) This cast shouldn't be needed. The problem is that you should have declared "id" const in the first place. > + if (!strcmp((char *) id->id, acpi_device_hid(device))) This cast doesn't seem needed either. At least I don't get any warning without it, do you? > + smbus_cmi->methods = > + (struct smbus_methods_t *) id->driver_data; > + As a side note it's really too bad that the acpi subsystem doesn't provide the id as a parameter as all other subsystems do. This would save individual drivers from having to look it up again. > acpi_walk_namespace(ACPI_TYPE_METHOD, smbus_cmi->handle, 1, > acpi_smbus_cmi_query_methods, smbus_cmi, NULL); > -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/