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

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

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-5C7GfCeVMHo@public.gmane.org>

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