Re: [V6 PATCH 2/3] ACPI / scan: Add support for ACPI _CLS device matching

[Hanjun Guo <guohanjun@huawei.com>]

On 2015/3/26 18:29, Mika Westerberg wrote:
> On Wed, Mar 25, 2015 at 03:46:07PM -0500, Suravee Suthikulpanit wrote:
>> Device drivers typically use ACPI _HIDs/_CIDs listed in struct device_driver
>> acpi_match_table to match devices. However, for generic drivers, we do not
>> want to list _HID for all supported devices. Also, certain classes of devices
>> do not have _CID (e.g. SATA, USB). Instead, we can leverage ACPI _CLS,
>> which specifies PCI-defined class code (i.e. base-class, subclass and
>> programming interface). This patch adds support for matching ACPI devices using
>> the _CLS method.
>>
>> To support loadable module, current design uses _HID or _CID to match device's
>> modalias. With the new way of matching with _CLS this would requires modification
>> to the current ACPI modalias key to include _CLS. This patch appends PCI-defined
>> class-code to the existing ACPI modalias as following.
>>
>>     acpi:<HID>:<CID1>:<CID2>:..:<CIDn>:<bbsspp>:
>> E.g:
>>     # cat /sys/devices/platform/AMDI0600:00/modalias
>>     acpi:AMDI0600:010601:
>>
>> where bb is th base-class code, ss is te sub-class code, and pp is the
>> programming interface code
>>
>> Since there would not be _HID/_CID in the ACPI matching table of the driver,
>> this patch adds a field to acpi_device_id to specify the matching _CLS.
>>
>>     static const struct acpi_device_id ahci_acpi_match[] = {
>>         { ACPI_DEVICE_CLASS(PCI_CLASS_STORAGE_SATA_AHCI, 0xFFFFFF) },
>>         {},
>>     };
>>
>> In this case, the corresponded entry in modules.alias file would be:
>>
>>     alias acpi*:010601:* ahci_platform
>>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> ---
>>  drivers/acpi/scan.c               | 39 +++++++++++++++++++++++++++++++++++----
>>  include/acpi/acnames.h            |  1 +
>>  include/acpi/actypes.h            |  4 +++-
>>  include/linux/mod_devicetable.h   |  4 ++++
>>  scripts/mod/devicetable-offsets.c |  2 ++
>>  scripts/mod/file2alias.c          | 32 ++++++++++++++++++++++++++++++--
>>  6 files changed, 75 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 52a62aa..f26bf80 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -895,6 +895,32 @@ static void acpi_device_remove_files(struct acpi_device *dev)
>>  			ACPI Bus operations
>>     -------------------------------------------------------------------------- */
>>  
>> +static bool __acpi_match_device_cls(const struct acpi_device_id *id,
>> +				    struct acpi_hardware_id *hwid)
>> +{
>> +	int i, msk, byte_shift;
>> +	bool found = true;
>> +	char buf[3];
>> +
>> +	if (!id->cls)
>> +		return false;
>> +
>> +	/* Apply class-code bitmask, before checking each class-code byte */
>> +	for (i = 1; i <= 3; i++) {
>> +		byte_shift = 8 * (3 - i);
>> +		msk = (id->cls_msk >> byte_shift) & 0xFF;
>> +		if (!msk)
>> +			continue;
>> +
>> +		sprintf(buf, "%02x", (id->cls >> byte_shift) & msk);
>> +		if (strncmp(buf, &hwid->id[(i - 1) * 2], 2)) {
>> +			found = false;
>> +			break;
> You can return false directly here.
>
>> +		}
>> +	}
>> +	return found;
>> +}
>> +
>>  static const struct acpi_device_id *__acpi_match_device(
>>  	struct acpi_device *device, const struct acpi_device_id *ids)
>>  {
>> @@ -908,11 +934,14 @@ static const struct acpi_device_id *__acpi_match_device(
>>  	if (!device->status.present)
>>  		return NULL;
>>  
>> -	for (id = ids; id->id[0]; id++)
>> -		list_for_each_entry(hwid, &device->pnp.ids, list)
>> -			if (!strcmp((char *) id->id, hwid->id))
>> +	for (id = ids; id->id[0] || id->cls; id++) {
>> +		list_for_each_entry(hwid, &device->pnp.ids, list) {
>> +			if (id->id[0] && !strcmp((char *) id->id, hwid->id))
>>  				return id;
>> -
>> +			else if (__acpi_match_device_cls(id, hwid))
>> +				return id;
>> +		}
>> +	}
>>  	return NULL;
>>  }
>>  
>> @@ -2005,6 +2034,8 @@ static void acpi_set_pnp_ids(acpi_handle handle, struct acpi_device_pnp *pnp,
>>  		if (info->valid & ACPI_VALID_UID)
>>  			pnp->unique_id = kstrdup(info->unique_id.string,
>>  							GFP_KERNEL);
>> +		if (info->valid & ACPI_VALID_CLS)
>> +			acpi_add_id(pnp, info->cls.string);
>>  
>>  		kfree(info);
>>  
>> diff --git a/include/acpi/acnames.h b/include/acpi/acnames.h
>> index 273de70..b52c0dc 100644
>> --- a/include/acpi/acnames.h
>> +++ b/include/acpi/acnames.h
>> @@ -51,6 +51,7 @@
>>  #define METHOD_NAME__BBN        "_BBN"
>>  #define METHOD_NAME__CBA        "_CBA"
>>  #define METHOD_NAME__CID        "_CID"
>> +#define METHOD_NAME__CLS        "_CLS"
>>  #define METHOD_NAME__CRS        "_CRS"
>>  #define METHOD_NAME__DDN        "_DDN"
>>  #define METHOD_NAME__HID        "_HID"
>> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
>> index b034f10..ab3dac8 100644
>> --- a/include/acpi/actypes.h
>> +++ b/include/acpi/actypes.h
>> @@ -1148,7 +1148,7 @@ struct acpi_device_info {
>>  	u32 name;		/* ACPI object Name */
>>  	acpi_object_type type;	/* ACPI object Type */
>>  	u8 param_count;		/* If a method, required parameter count */
>> -	u8 valid;		/* Indicates which optional fields are valid */
>> +	u16 valid;		/* Indicates which optional fields are valid */
>>  	u8 flags;		/* Miscellaneous info */
>>  	u8 highest_dstates[4];	/* _sx_d values: 0xFF indicates not valid */
>>  	u8 lowest_dstates[5];	/* _sx_w values: 0xFF indicates not valid */
>> @@ -1157,6 +1157,7 @@ struct acpi_device_info {
>>  	struct acpi_pnp_device_id hardware_id;	/* _HID value */
>>  	struct acpi_pnp_device_id unique_id;	/* _UID value */
>>  	struct acpi_pnp_device_id subsystem_id;	/* _SUB value */
>> +	struct acpi_pnp_device_id cls;		/* _CLS value */
>>  	struct acpi_pnp_device_id_list compatible_id_list;	/* _CID list <must be last> */
>>  };
>>  
>> @@ -1174,6 +1175,7 @@ struct acpi_device_info {
>>  #define ACPI_VALID_CID                  0x20
>>  #define ACPI_VALID_SXDS                 0x40
>>  #define ACPI_VALID_SXWS                 0x80
>> +#define ACPI_VALID_CLS                  0x100
>>  
>>  /* Flags for _STA return value (current_status above) */
>>  
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>> index e530533..9563abe 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
>> @@ -189,8 +189,12 @@ struct css_device_id {
>>  struct acpi_device_id {
>>  	__u8 id[ACPI_ID_LEN];
>>  	kernel_ulong_t driver_data;
>> +	__u32 cls;
>> +	__u32 cls_msk;
>>  };
>>  
>> +#define ACPI_DEVICE_CLASS(cls, msk)		"", 0, cls, msk
> Consider moving this to <linux/acpi.h>, just like PCI_DEVICE_CLASS() is
> defined in <linux/pci.h>.
>
> Also please use designated initializers here, eg:
>
> #define ACPI_DEVICE_CLASS(cls, msk)	.cls = (cls), .cls_mask = (msk)

I agree.

>
> Once done you can add my,
>
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Please add my
Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org> too.

Thanks
Hanjun