Re: [PATCH 1/2] i2c-scmi: support IBM SMBus CMI devices

Jean Delvare
2009-10-25T09:39:34Z

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 <crane.cai@amd.com>

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