[V5 PATCH 0/2] Introduce ACPI support for ahci_platform driver

[V5 PATCH 0/2] Introduce ACPI support for ahci_platform driver

[Suravee Suthikulpanit]

This patch series introduce ACPI support for AHCI platform driver.
Existing ACPI support for AHCI assumes the device controller is a PCI device.
Since there is no ACPI _CID for generic AHCI controller, the driver
could not use it for matching devices. Therefore, this patch introduces
a mechanism for drivers to match devices using ACPI _CLS method.
_CLS contains PCI-defined class-code.

This patch series also modifies ACPI modalias to add class-code to the
exisiting format, which currently only uses _HID and _CIDs. This is required
to support loadable modules w/ _CLS.

This patch series is rebased from and tested with:

    http://git.linaro.org/leg/acpi/acpi.git acpi-5.1-v9

This topic was discussed earlier here (as part of introducing support for
AMD Seattle SATA controller):

    http://marc.info/?l=linux-arm-kernel&m=141083492521584&w=2

Changes from V4 (https://lkml.org/lkml/2015/3/2/56)
	* [1/2] Bug fixed: Reorder the declaration of
	  struct acpi_pnp_device_id cls in the struct acpi_device_info
	  (include/acpi/actypes.h) since compatible_id_list must be last one.
	* [2/2] Added Acked-by: Tejun Heo <tj@kernel.org>

Changes from V3 (https://lkml.org/lkml/2015/2/8/106)
	* Instead of introducing new structure acpi_device_cls, add cls into
	  the acpi_device_id, and modify the __acpi_match_device
	  to also match for cls. (per Mika suggestion.)
	* Add loadable module support, which requires changes in ACPI
	  modalias. (per Mika suggestion.)
	* Rebased and tested with acpi-5.1-v9

Changes from V2 (https://lkml.org/lkml/2015/1/5/662)
	* Update with review comment from Rafael in patch 1/2
	* Rebased and tested with acpi-5.1-v8

Changes from V1 (https://lkml.org/lkml/2014/12/19/345)
	* Rebased to 3.19.0-rc2
	* Change from acpi_cls in device_driver to acpi_match_cls (Hanjun comment)
	* Change the matching logic in acpi_driver_match_device() due to the new
	  special PRP0001 _HID.
	* Simplify the return type of acpi_match_device_cls() to boolean.

Changes from RFC (https://lkml.org/lkml/2014/12/17/446)
	* Remove #ifdef and make non-ACPI version of the acpi_match_device_cls
	  as inline. (per Arnd)
	* Simplify logic to retrieve and evaluate _CLS handle. (per Hanjun)

Suravee Suthikulpanit (2):
  ACPI / scan: Add support for ACPI _CLS device matching
  ata: ahci_platform: Add ACPI _CLS matching

 drivers/acpi/acpica/acutils.h     |  3 ++
 drivers/acpi/acpica/nsxfname.c    | 21 ++++++++++--
 drivers/acpi/acpica/utids.c       | 71 +++++++++++++++++++++++++++++++++++++++
 drivers/acpi/scan.c               | 17 ++++++++--
 drivers/ata/Kconfig               |  2 +-
 drivers/ata/ahci_platform.c       |  9 +++++
 include/acpi/acnames.h            |  1 +
 include/acpi/actypes.h            |  4 ++-
 include/linux/mod_devicetable.h   |  1 +
 scripts/mod/devicetable-offsets.c |  1 +
 scripts/mod/file2alias.c          | 13 +++++--
 11 files changed, 134 insertions(+), 9 deletions(-)

-- 
2.1.0

[V5 PATCH 1/2] ACPI / scan: Add support for ACPI _CLS device matching

[Suravee Suthikulpanit]

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[] = {
        { "", 0, PCI_CLASS_STORAGE_SATA_AHCI },
        {},
    };

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/acpica/acutils.h     |  3 ++
 drivers/acpi/acpica/nsxfname.c    | 21 ++++++++++--
 drivers/acpi/acpica/utids.c       | 71 +++++++++++++++++++++++++++++++++++++++
 drivers/acpi/scan.c               | 17 ++++++++--
 include/acpi/acnames.h            |  1 +
 include/acpi/actypes.h            |  4 ++-
 include/linux/mod_devicetable.h   |  1 +
 scripts/mod/devicetable-offsets.c |  1 +
 scripts/mod/file2alias.c          | 13 +++++--
 9 files changed, 124 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/acpica/acutils.h b/drivers/acpi/acpica/acutils.h
index c2f03e8..2aef850 100644
--- a/drivers/acpi/acpica/acutils.h
+++ b/drivers/acpi/acpica/acutils.h
@@ -430,6 +430,9 @@ acpi_status
 acpi_ut_execute_CID(struct acpi_namespace_node *device_node,
 		    struct acpi_pnp_device_id_list ** return_cid_list);
 
+acpi_status
+acpi_ut_execute_CLS(struct acpi_namespace_node *device_node,
+		    struct acpi_pnp_device_id **return_id);
 /*
  * utlock - reader/writer locks
  */
diff --git a/drivers/acpi/acpica/nsxfname.c b/drivers/acpi/acpica/nsxfname.c
index d66c326..590ef06 100644
--- a/drivers/acpi/acpica/nsxfname.c
+++ b/drivers/acpi/acpica/nsxfname.c
@@ -276,11 +276,12 @@ acpi_get_object_info(acpi_handle handle,
 	struct acpi_pnp_device_id *hid = NULL;
 	struct acpi_pnp_device_id *uid = NULL;
 	struct acpi_pnp_device_id *sub = NULL;
+	struct acpi_pnp_device_id *cls = NULL;
 	char *next_id_string;
 	acpi_object_type type;
 	acpi_name name;
 	u8 param_count = 0;
-	u8 valid = 0;
+	u16 valid = 0;
 	u32 info_size;
 	u32 i;
 	acpi_status status;
@@ -320,7 +321,7 @@ acpi_get_object_info(acpi_handle handle,
 	if ((type == ACPI_TYPE_DEVICE) || (type == ACPI_TYPE_PROCESSOR)) {
 		/*
 		 * Get extra info for ACPI Device/Processor objects only:
-		 * Run the Device _HID, _UID, _SUB, and _CID methods.
+		 * Run the Device _HID, _UID, _SUB, _CID and _CLS methods.
 		 *
 		 * Note: none of these methods are required, so they may or may
 		 * not be present for this device. The Info->Valid bitfield is used
@@ -351,6 +352,14 @@ acpi_get_object_info(acpi_handle handle,
 			valid |= ACPI_VALID_SUB;
 		}
 
+		/* Execute the Device._CLS method */
+
+		status = acpi_ut_execute_CLS(node, &cls);
+		if (ACPI_SUCCESS(status)) {
+			info_size += cls->length;
+			valid |= ACPI_VALID_CLS;
+		}
+
 		/* Execute the Device._CID method */
 
 		status = acpi_ut_execute_CID(node, &cid_list);
@@ -468,6 +477,11 @@ acpi_get_object_info(acpi_handle handle,
 							sub, next_id_string);
 	}
 
+	if (cls) {
+		next_id_string = acpi_ns_copy_device_id(&info->cls,
+							cls, next_id_string);
+	}
+
 	if (cid_list) {
 		info->compatible_id_list.count = cid_list->count;
 		info->compatible_id_list.list_size = cid_list->list_size;
@@ -507,6 +521,9 @@ cleanup:
 	if (sub) {
 		ACPI_FREE(sub);
 	}
+	if (cls) {
+		ACPI_FREE(cls);
+	}
 	if (cid_list) {
 		ACPI_FREE(cid_list);
 	}
diff --git a/drivers/acpi/acpica/utids.c b/drivers/acpi/acpica/utids.c
index 27431cf..a64b5d1 100644
--- a/drivers/acpi/acpica/utids.c
+++ b/drivers/acpi/acpica/utids.c
@@ -416,3 +416,74 @@ cleanup:
 	acpi_ut_remove_reference(obj_desc);
 	return_ACPI_STATUS(status);
 }
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_ut_execute_CLS
+ *
+ * PARAMETERS:  device_node         - Node for the device
+ *              return_id           - Where the string UID is returned
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Executes the _CLS control method that returns PCI-defined
+ *              class code of the device. The ACPI spec define _CLS as a
+ *              package with three integers. The returned string has format:
+ *
+ *                      "bbsspp"
+ *              where:
+ *                  bb = Base-class code
+ *                  ss = Sub-class code
+ *                  pp = Programming Interface code
+ *
+ ******************************************************************************/
+
+acpi_status
+acpi_ut_execute_CLS(struct acpi_namespace_node *device_node,
+		    struct acpi_pnp_device_id **return_id)
+{
+	struct acpi_pnp_device_id *cls;
+	union acpi_operand_object *obj_desc;
+	union acpi_operand_object **cls_objects;
+	acpi_status status;
+
+	ACPI_FUNCTION_TRACE(ut_execute_CLS);
+	status = acpi_ut_evaluate_object(device_node, METHOD_NAME__CLS,
+					 ACPI_BTYPE_PACKAGE, &obj_desc);
+	if (ACPI_FAILURE(status))
+		return_ACPI_STATUS(status);
+
+	cls_objects = obj_desc->package.elements;
+
+	if (obj_desc->common.type == ACPI_TYPE_PACKAGE &&
+	    obj_desc->package.count == 3 &&
+	    cls_objects[0]->common.type == ACPI_TYPE_INTEGER &&
+	    cls_objects[1]->common.type == ACPI_TYPE_INTEGER &&
+	    cls_objects[2]->common.type == ACPI_TYPE_INTEGER) {
+
+		/* Allocate a buffer for the CLS */
+		cls = ACPI_ALLOCATE_ZEROED(sizeof(struct acpi_pnp_device_id) +
+					 (acpi_size) 7);
+		if (!cls) {
+			status = AE_NO_MEMORY;
+			goto cleanup;
+		}
+
+		cls->string =
+		    ACPI_ADD_PTR(char, cls, sizeof(struct acpi_pnp_device_id));
+
+		sprintf(cls->string, "%02x%02x%02x",
+			(u8)ACPI_TO_INTEGER(cls_objects[0]->integer.value),
+			(u8)ACPI_TO_INTEGER(cls_objects[1]->integer.value),
+			(u8)ACPI_TO_INTEGER(cls_objects[2]->integer.value));
+		cls->length = 7;
+		*return_id = cls;
+	}
+
+cleanup:
+
+	/* On exit, we must delete the return object */
+
+	acpi_ut_remove_reference(obj_desc);
+	return_ACPI_STATUS(status);
+}
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index bbca783..f6ecbd1 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -907,10 +907,19 @@ 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 (id->cls) {
+				char buf[7];
+
+				sprintf(buf, "%06x", id->cls);
+				if (!strcmp(buf, hwid->id))
+					return id;
+			}
+		}
+	}
 
 	return NULL;
 }
@@ -1974,6 +1983,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..9a42522 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -189,6 +189,7 @@ struct css_device_id {
 struct acpi_device_id {
 	__u8 id[ACPI_ID_LEN];
 	kernel_ulong_t driver_data;
+	__u32 cls;
 };
 
 #define PNP_ID_LEN	8
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index f282516..7f68268 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -63,6 +63,7 @@ int main(void)
 
 	DEVID(acpi_device_id);
 	DEVID_FIELD(acpi_device_id, id);
+	DEVID_FIELD(acpi_device_id, cls);
 
 	DEVID(pnp_device_id);
 	DEVID_FIELD(pnp_device_id, id);
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index e614ef6..ba5998c 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -511,12 +511,21 @@ static int do_serio_entry(const char *filename,
 }
 ADD_TO_DEVTABLE("serio", serio_device_id, do_serio_entry);
 
-/* looks like: "acpi:ACPI0003 or acpi:PNP0C0B" or "acpi:LNXVIDEO" */
+/* looks like: "acpi:ACPI0003" or "acpi:PNP0C0B"  or "acpi:LNXVIDEO" or
+ *             "acpi:bbsspp" (bb=base-class, ss=sub-class, pp=prog-if)
+ *
+ * NOTE: * Each driver should use one of the following : _HID, _CIDs or _CLS.
+ */
 static int do_acpi_entry(const char *filename,
 			void *symval, char *alias)
 {
 	DEF_FIELD_ADDR(symval, acpi_device_id, id);
-	sprintf(alias, "acpi*:%s:*", *id);
+	DEF_FIELD_ADDR(symval, acpi_device_id, cls);
+
+	if (id && strlen((const char *)*id))
+		sprintf(alias, "acpi*:%s:*", *id);
+	else if (cls)
+		sprintf(alias, "acpi*:%06x:*", *cls);
 	return 1;
 }
 ADD_TO_DEVTABLE("acpi", acpi_device_id, do_acpi_entry);
-- 
2.1.0

Re: [V5 PATCH 1/2] ACPI / scan: Add support for ACPI _CLS device matching

[Mika Westerberg]

On Fri, Mar 06, 2015 at 12:11:41AM -0600, 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[] = {
>         { "", 0, PCI_CLASS_STORAGE_SATA_AHCI },

How about introducing macro like PCI already has and then do it like:

	{ ACPI_DEVICE_CLASS(PCI_CLASS_STORAGE_SATA_AHCI) },

Also should we allow mask here as well? This would allow partial match
if, for example we are only interested in class and subclass.

>         {},
>     };
> 
> 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/acpica/acutils.h     |  3 ++
>  drivers/acpi/acpica/nsxfname.c    | 21 ++++++++++--
>  drivers/acpi/acpica/utids.c       | 71 +++++++++++++++++++++++++++++++++++++++

I think you need to split the ACPICA part to be a separate patch. Those
are merged through ACPICA.

>  drivers/acpi/scan.c               | 17 ++++++++--
>  include/acpi/acnames.h            |  1 +
>  include/acpi/actypes.h            |  4 ++-
>  include/linux/mod_devicetable.h   |  1 +
>  scripts/mod/devicetable-offsets.c |  1 +
>  scripts/mod/file2alias.c          | 13 +++++--
>  9 files changed, 124 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/acutils.h b/drivers/acpi/acpica/acutils.h
> index c2f03e8..2aef850 100644
> --- a/drivers/acpi/acpica/acutils.h
> +++ b/drivers/acpi/acpica/acutils.h
> @@ -430,6 +430,9 @@ acpi_status
>  acpi_ut_execute_CID(struct acpi_namespace_node *device_node,
>  		    struct acpi_pnp_device_id_list ** return_cid_list);
>  
> +acpi_status
> +acpi_ut_execute_CLS(struct acpi_namespace_node *device_node,
> +		    struct acpi_pnp_device_id **return_id);
>  /*
>   * utlock - reader/writer locks
>   */
> diff --git a/drivers/acpi/acpica/nsxfname.c b/drivers/acpi/acpica/nsxfname.c
> index d66c326..590ef06 100644
> --- a/drivers/acpi/acpica/nsxfname.c
> +++ b/drivers/acpi/acpica/nsxfname.c
> @@ -276,11 +276,12 @@ acpi_get_object_info(acpi_handle handle,
>  	struct acpi_pnp_device_id *hid = NULL;
>  	struct acpi_pnp_device_id *uid = NULL;
>  	struct acpi_pnp_device_id *sub = NULL;
> +	struct acpi_pnp_device_id *cls = NULL;
>  	char *next_id_string;
>  	acpi_object_type type;
>  	acpi_name name;
>  	u8 param_count = 0;
> -	u8 valid = 0;
> +	u16 valid = 0;
>  	u32 info_size;
>  	u32 i;
>  	acpi_status status;
> @@ -320,7 +321,7 @@ acpi_get_object_info(acpi_handle handle,
>  	if ((type == ACPI_TYPE_DEVICE) || (type == ACPI_TYPE_PROCESSOR)) {
>  		/*
>  		 * Get extra info for ACPI Device/Processor objects only:
> -		 * Run the Device _HID, _UID, _SUB, and _CID methods.
> +		 * Run the Device _HID, _UID, _SUB, _CID and _CLS methods.
>  		 *
>  		 * Note: none of these methods are required, so they may or may
>  		 * not be present for this device. The Info->Valid bitfield is used
> @@ -351,6 +352,14 @@ acpi_get_object_info(acpi_handle handle,
>  			valid |= ACPI_VALID_SUB;
>  		}
>  
> +		/* Execute the Device._CLS method */
> +
> +		status = acpi_ut_execute_CLS(node, &cls);
> +		if (ACPI_SUCCESS(status)) {
> +			info_size += cls->length;
> +			valid |= ACPI_VALID_CLS;
> +		}
> +
>  		/* Execute the Device._CID method */
>  
>  		status = acpi_ut_execute_CID(node, &cid_list);
> @@ -468,6 +477,11 @@ acpi_get_object_info(acpi_handle handle,
>  							sub, next_id_string);
>  	}
>  
> +	if (cls) {
> +		next_id_string = acpi_ns_copy_device_id(&info->cls,
> +							cls, next_id_string);
> +	}
> +
>  	if (cid_list) {
>  		info->compatible_id_list.count = cid_list->count;
>  		info->compatible_id_list.list_size = cid_list->list_size;
> @@ -507,6 +521,9 @@ cleanup:
>  	if (sub) {
>  		ACPI_FREE(sub);
>  	}
> +	if (cls) {
> +		ACPI_FREE(cls);
> +	}
>  	if (cid_list) {
>  		ACPI_FREE(cid_list);
>  	}
> diff --git a/drivers/acpi/acpica/utids.c b/drivers/acpi/acpica/utids.c
> index 27431cf..a64b5d1 100644
> --- a/drivers/acpi/acpica/utids.c
> +++ b/drivers/acpi/acpica/utids.c
> @@ -416,3 +416,74 @@ cleanup:
>  	acpi_ut_remove_reference(obj_desc);
>  	return_ACPI_STATUS(status);
>  }
> +
> +/*******************************************************************************
> + *
> + * FUNCTION:    acpi_ut_execute_CLS
> + *
> + * PARAMETERS:  device_node         - Node for the device
> + *              return_id           - Where the string UID is returned
> + *
> + * RETURN:      Status
> + *
> + * DESCRIPTION: Executes the _CLS control method that returns PCI-defined
> + *              class code of the device. The ACPI spec define _CLS as a
> + *              package with three integers. The returned string has format:
> + *
> + *                      "bbsspp"
> + *              where:
> + *                  bb = Base-class code
> + *                  ss = Sub-class code
> + *                  pp = Programming Interface code
> + *
> + ******************************************************************************/
> +
> +acpi_status
> +acpi_ut_execute_CLS(struct acpi_namespace_node *device_node,
> +		    struct acpi_pnp_device_id **return_id)
> +{
> +	struct acpi_pnp_device_id *cls;
> +	union acpi_operand_object *obj_desc;
> +	union acpi_operand_object **cls_objects;
> +	acpi_status status;
> +
> +	ACPI_FUNCTION_TRACE(ut_execute_CLS);
> +	status = acpi_ut_evaluate_object(device_node, METHOD_NAME__CLS,
> +					 ACPI_BTYPE_PACKAGE, &obj_desc);
> +	if (ACPI_FAILURE(status))
> +		return_ACPI_STATUS(status);
> +
> +	cls_objects = obj_desc->package.elements;
> +
> +	if (obj_desc->common.type == ACPI_TYPE_PACKAGE &&
> +	    obj_desc->package.count == 3 &&
> +	    cls_objects[0]->common.type == ACPI_TYPE_INTEGER &&
> +	    cls_objects[1]->common.type == ACPI_TYPE_INTEGER &&
> +	    cls_objects[2]->common.type == ACPI_TYPE_INTEGER) {
> +
> +		/* Allocate a buffer for the CLS */
> +		cls = ACPI_ALLOCATE_ZEROED(sizeof(struct acpi_pnp_device_id) +
> +					 (acpi_size) 7);
> +		if (!cls) {
> +			status = AE_NO_MEMORY;
> +			goto cleanup;
> +		}
> +
> +		cls->string =
> +		    ACPI_ADD_PTR(char, cls, sizeof(struct acpi_pnp_device_id));
> +
> +		sprintf(cls->string, "%02x%02x%02x",
> +			(u8)ACPI_TO_INTEGER(cls_objects[0]->integer.value),
> +			(u8)ACPI_TO_INTEGER(cls_objects[1]->integer.value),
> +			(u8)ACPI_TO_INTEGER(cls_objects[2]->integer.value));
> +		cls->length = 7;
> +		*return_id = cls;
> +	}
> +
> +cleanup:
> +
> +	/* On exit, we must delete the return object */
> +
> +	acpi_ut_remove_reference(obj_desc);
> +	return_ACPI_STATUS(status);
> +}
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index bbca783..f6ecbd1 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -907,10 +907,19 @@ 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 (id->cls) {
> +				char buf[7];
> +
> +				sprintf(buf, "%06x", id->cls);
> +				if (!strcmp(buf, hwid->id))
> +					return id;
> +			}
> +		}
> +	}
>  
>  	return NULL;
>  }
> @@ -1974,6 +1983,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..9a42522 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -189,6 +189,7 @@ struct css_device_id {
>  struct acpi_device_id {
>  	__u8 id[ACPI_ID_LEN];
>  	kernel_ulong_t driver_data;
> +	__u32 cls;

It would be nice if we could change ordering here but I understand that
it breaks quite many drivers. Perhaps we should consider creating
ACPI_DEVICE() macro and convert existing drivers to that at some point.

>  };
>  
>  #define PNP_ID_LEN	8
> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
> index f282516..7f68268 100644
> --- a/scripts/mod/devicetable-offsets.c
> +++ b/scripts/mod/devicetable-offsets.c
> @@ -63,6 +63,7 @@ int main(void)
>  
>  	DEVID(acpi_device_id);
>  	DEVID_FIELD(acpi_device_id, id);
> +	DEVID_FIELD(acpi_device_id, cls);
>  
>  	DEVID(pnp_device_id);
>  	DEVID_FIELD(pnp_device_id, id);
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index e614ef6..ba5998c 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -511,12 +511,21 @@ static int do_serio_entry(const char *filename,
>  }
>  ADD_TO_DEVTABLE("serio", serio_device_id, do_serio_entry);
>  
> -/* looks like: "acpi:ACPI0003 or acpi:PNP0C0B" or "acpi:LNXVIDEO" */
> +/* looks like: "acpi:ACPI0003" or "acpi:PNP0C0B"  or "acpi:LNXVIDEO" or
> + *             "acpi:bbsspp" (bb=base-class, ss=sub-class, pp=prog-if)
> + *
> + * NOTE: * Each driver should use one of the following : _HID, _CIDs or _CLS.
> + */
>  static int do_acpi_entry(const char *filename,
>  			void *symval, char *alias)
>  {
>  	DEF_FIELD_ADDR(symval, acpi_device_id, id);
> -	sprintf(alias, "acpi*:%s:*", *id);
> +	DEF_FIELD_ADDR(symval, acpi_device_id, cls);
> +
> +	if (id && strlen((const char *)*id))
> +		sprintf(alias, "acpi*:%s:*", *id);
> +	else if (cls)
> +		sprintf(alias, "acpi*:%06x:*", *cls);
>  	return 1;
>  }
>  ADD_TO_DEVTABLE("acpi", acpi_device_id, do_acpi_entry);
> -- 
> 2.1.0

Re: [V5 PATCH 1/2] ACPI / scan: Add support for ACPI _CLS device matching

[Suravee Suthikulpanit]

Not sure if this email went out originally. So, I am resending this.

On 3/9/15 10:20, Mika Westerberg wrote:
> On Fri, Mar 06, 2015 at 12:11:41AM -0600, 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[] = {
>>          { "", 0, PCI_CLASS_STORAGE_SATA_AHCI },
>
> How about introducing macro like PCI already has and then do it like:
>
> 	{ ACPI_DEVICE_CLASS(PCI_CLASS_STORAGE_SATA_AHCI) },

This is good.  I'll update this.
>
> Also should we allow mask here as well? This would allow partial match
> if, for example we are only interested in class and subclass.

Hm, I'm not familiar with how other drivers might be benefit from this. 
Could you please give an example of a drivers that only deals with just 
the class and/or subclass of devices?

If so, could we use 0xFF to specify the don't care field?

>
>>          {},
>>      };
>>
>> 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/acpica/acutils.h     |  3 ++
>>   drivers/acpi/acpica/nsxfname.c    | 21 ++++++++++--
>>   drivers/acpi/acpica/utids.c       | 71 +++++++++++++++++++++++++++++++++++++++
>
> I think you need to split the ACPICA part to be a separate patch. Those
> are merged through ACPICA.

I will update this in the next patch set.

>> [....]
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>> index e530533..9a42522 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
>> @@ -189,6 +189,7 @@ struct css_device_id {
>>   struct acpi_device_id {
>>   	__u8 id[ACPI_ID_LEN];
>>   	kernel_ulong_t driver_data;
>> +	__u32 cls;
>
> It would be nice if we could change ordering here but I understand that
> it breaks quite many drivers. Perhaps we should consider creating
> ACPI_DEVICE() macro and convert existing drivers to that at some point.

Yes, a roughly grep in the drivers directory showing about 112 files at 
the moment. If you think this is the right approach going forward, we 
can work on cleaning this up on a separate patch series. Please let me 
know what you think.

Thanks,

Suravee

>>   };
>>
>>   #define PNP_ID_LEN	8
>> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
>> index f282516..7f68268 100644
>> --- a/scripts/mod/devicetable-offsets.c
>> +++ b/scripts/mod/devicetable-offsets.c
>> @@ -63,6 +63,7 @@ int main(void)
>>
>>   	DEVID(acpi_device_id);
>>   	DEVID_FIELD(acpi_device_id, id);
>> +	DEVID_FIELD(acpi_device_id, cls);
>>
>>   	DEVID(pnp_device_id);
>>   	DEVID_FIELD(pnp_device_id, id);
>> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
>> index e614ef6..ba5998c 100644
>> --- a/scripts/mod/file2alias.c
>> +++ b/scripts/mod/file2alias.c
>> @@ -511,12 +511,21 @@ static int do_serio_entry(const char *filename,
>>   }
>>   ADD_TO_DEVTABLE("serio", serio_device_id, do_serio_entry);
>>
>> -/* looks like: "acpi:ACPI0003 or acpi:PNP0C0B" or "acpi:LNXVIDEO" */
>> +/* looks like: "acpi:ACPI0003" or "acpi:PNP0C0B"  or "acpi:LNXVIDEO" or
>> + *             "acpi:bbsspp" (bb=base-class, ss=sub-class, pp=prog-if)
>> + *
>> + * NOTE: * Each driver should use one of the following : _HID, _CIDs or _CLS.
>> + */
>>   static int do_acpi_entry(const char *filename,
>>   			void *symval, char *alias)
>>   {
>>   	DEF_FIELD_ADDR(symval, acpi_device_id, id);
>> -	sprintf(alias, "acpi*:%s:*", *id);
>> +	DEF_FIELD_ADDR(symval, acpi_device_id, cls);
>> +
>> +	if (id && strlen((const char *)*id))
>> +		sprintf(alias, "acpi*:%s:*", *id);
>> +	else if (cls)
>> +		sprintf(alias, "acpi*:%06x:*", *cls);
>>   	return 1;
>>   }
>>   ADD_TO_DEVTABLE("acpi", acpi_device_id, do_acpi_entry);
>> --
>> 2.1.0
> --
> 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
>

Re: [V5 PATCH 1/2] ACPI / scan: Add support for ACPI _CLS device matching

[Mika Westerberg]

On Tue, Mar 24, 2015 at 09:23:47AM -0500, Suravee Suthikulpanit wrote:
> Not sure if this email went out originally. So, I am resending this.
> 
> On 3/9/15 10:20, Mika Westerberg wrote:
> >On Fri, Mar 06, 2015 at 12:11:41AM -0600, 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[] = {
> >>         { "", 0, PCI_CLASS_STORAGE_SATA_AHCI },
> >
> >How about introducing macro like PCI already has and then do it like:
> >
> >	{ ACPI_DEVICE_CLASS(PCI_CLASS_STORAGE_SATA_AHCI) },
> 
> This is good.  I'll update this.
> >
> >Also should we allow mask here as well? This would allow partial match
> >if, for example we are only interested in class and subclass.
> 
> Hm, I'm not familiar with how other drivers might be benefit from this.
> Could you please give an example of a drivers that only deals with just the
> class and/or subclass of devices?

If you check for example drivers/ata/ata_generic.c it has this:

        { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_IDE << 8, 0xFFFFFF00UL),
	          .driver_data = ATA_GEN_CLASS_MATCH },

That seems to ignore programming interface.

> If so, could we use 0xFF to specify the don't care field?

In the above it seems that 0 is the don't care.

> >
> >>         {},
> >>     };
> >>
> >>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/acpica/acutils.h     |  3 ++
> >>  drivers/acpi/acpica/nsxfname.c    | 21 ++++++++++--
> >>  drivers/acpi/acpica/utids.c       | 71 +++++++++++++++++++++++++++++++++++++++
> >
> >I think you need to split the ACPICA part to be a separate patch. Those
> >are merged through ACPICA.
> 
> I will update this in the next patch set.
> 
> >>[....]
> >>diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> >>index e530533..9a42522 100644
> >>--- a/include/linux/mod_devicetable.h
> >>+++ b/include/linux/mod_devicetable.h
> >>@@ -189,6 +189,7 @@ struct css_device_id {
> >>  struct acpi_device_id {
> >>  	__u8 id[ACPI_ID_LEN];
> >>  	kernel_ulong_t driver_data;
> >>+	__u32 cls;
> >
> >It would be nice if we could change ordering here but I understand that
> >it breaks quite many drivers. Perhaps we should consider creating
> >ACPI_DEVICE() macro and convert existing drivers to that at some point.
> 
> Yes, a roughly grep in the drivers directory showing about 112 files at the
> moment. If you think this is the right approach going forward, we can work
> on cleaning this up on a separate patch series. Please let me know what you
> think.

I think having ACPI_DEVICE() macro would be pretty useful and it avoids
things like this if we need to add new fields in the future. Rafael has
the last word, though :-)

Re: [V5 PATCH 1/2] ACPI / scan: Add support for ACPI _CLS device matching

[Rafael J. Wysocki]

On Tuesday, March 24, 2015 04:43:46 PM Mika Westerberg wrote:
> On Tue, Mar 24, 2015 at 09:23:47AM -0500, Suravee Suthikulpanit wrote:
> > Not sure if this email went out originally. So, I am resending this.
> > 
> > On 3/9/15 10:20, Mika Westerberg wrote:
> > >On Fri, Mar 06, 2015 at 12:11:41AM -0600, 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[] = {
> > >>         { "", 0, PCI_CLASS_STORAGE_SATA_AHCI },
> > >
> > >How about introducing macro like PCI already has and then do it like:
> > >
> > >	{ ACPI_DEVICE_CLASS(PCI_CLASS_STORAGE_SATA_AHCI) },
> > 
> > This is good.  I'll update this.
> > >
> > >Also should we allow mask here as well? This would allow partial match
> > >if, for example we are only interested in class and subclass.
> > 
> > Hm, I'm not familiar with how other drivers might be benefit from this.
> > Could you please give an example of a drivers that only deals with just the
> > class and/or subclass of devices?
> 
> If you check for example drivers/ata/ata_generic.c it has this:
> 
>         { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_IDE << 8, 0xFFFFFF00UL),
> 	          .driver_data = ATA_GEN_CLASS_MATCH },
> 
> That seems to ignore programming interface.
> 
> > If so, could we use 0xFF to specify the don't care field?
> 
> In the above it seems that 0 is the don't care.
> 
> > >
> > >>         {},
> > >>     };
> > >>
> > >>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/acpica/acutils.h     |  3 ++
> > >>  drivers/acpi/acpica/nsxfname.c    | 21 ++++++++++--
> > >>  drivers/acpi/acpica/utids.c       | 71 +++++++++++++++++++++++++++++++++++++++
> > >
> > >I think you need to split the ACPICA part to be a separate patch. Those
> > >are merged through ACPICA.
> > 
> > I will update this in the next patch set.
> > 
> > >>[....]
> > >>diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > >>index e530533..9a42522 100644
> > >>--- a/include/linux/mod_devicetable.h
> > >>+++ b/include/linux/mod_devicetable.h
> > >>@@ -189,6 +189,7 @@ struct css_device_id {
> > >>  struct acpi_device_id {
> > >>  	__u8 id[ACPI_ID_LEN];
> > >>  	kernel_ulong_t driver_data;
> > >>+	__u32 cls;
> > >
> > >It would be nice if we could change ordering here but I understand that
> > >it breaks quite many drivers. Perhaps we should consider creating
> > >ACPI_DEVICE() macro and convert existing drivers to that at some point.
> > 
> > Yes, a roughly grep in the drivers directory showing about 112 files at the
> > moment. If you think this is the right approach going forward, we can work
> > on cleaning this up on a separate patch series. Please let me know what you
> > think.
> 
> I think having ACPI_DEVICE() macro would be pretty useful and it avoids
> things like this if we need to add new fields in the future. Rafael has
> the last word, though :-)

I agree.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [V5 PATCH 1/2] ACPI / scan: Add support for ACPI _CLS device matching

[Suravee Suthikulpanit]

On 3/24/15 15:20, Rafael J. Wysocki wrote:
> On Tuesday, March 24, 2015 04:43:46 PM Mika Westerberg wrote:
>> On Tue, Mar 24, 2015 at 09:23:47AM -0500, Suravee Suthikulpanit wrote:
>>> On 3/9/15 10:20, Mika Westerberg wrote:
>>>>> [....]
>>>>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>>>>> index e530533..9a42522 100644
>>>>> --- a/include/linux/mod_devicetable.h
>>>>> +++ b/include/linux/mod_devicetable.h
>>>>> @@ -189,6 +189,7 @@ struct css_device_id {
>>>>>   struct acpi_device_id {
>>>>>   	__u8 id[ACPI_ID_LEN];
>>>>>   	kernel_ulong_t driver_data;
>>>>> +	__u32 cls;
>>>>
>>>> It would be nice if we could change ordering here but I understand that
>>>> it breaks quite many drivers. Perhaps we should consider creating
>>>> ACPI_DEVICE() macro and convert existing drivers to that at some point.
>>>
>>> Yes, a roughly grep in the drivers directory showing about 112 files at the
>>> moment. If you think this is the right approach going forward, we can work
>>> on cleaning this up on a separate patch series. Please let me know what you
>>> think.
>>
>> I think having ACPI_DEVICE() macro would be pretty useful and it avoids
>> things like this if we need to add new fields in the future. Rafael has
>> the last word, though :-)
>
> I agree.

Okay, how should I organize this big change? Can we do this as a 
separate patch series?

Thanks,
Suravee

Re: [V5 PATCH 1/2] ACPI / scan: Add support for ACPI _CLS device matching

[Mika Westerberg]

On Wed, Mar 25, 2015 at 10:09:42AM -0500, Suravee Suthikulpanit wrote:
> On 3/24/15 15:20, Rafael J. Wysocki wrote:
> >On Tuesday, March 24, 2015 04:43:46 PM Mika Westerberg wrote:
> >>On Tue, Mar 24, 2015 at 09:23:47AM -0500, Suravee Suthikulpanit wrote:
> >>>On 3/9/15 10:20, Mika Westerberg wrote:
> >>>>>[....]
> >>>>>diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> >>>>>index e530533..9a42522 100644
> >>>>>--- a/include/linux/mod_devicetable.h
> >>>>>+++ b/include/linux/mod_devicetable.h
> >>>>>@@ -189,6 +189,7 @@ struct css_device_id {
> >>>>>  struct acpi_device_id {
> >>>>>  	__u8 id[ACPI_ID_LEN];
> >>>>>  	kernel_ulong_t driver_data;
> >>>>>+	__u32 cls;
> >>>>
> >>>>It would be nice if we could change ordering here but I understand that
> >>>>it breaks quite many drivers. Perhaps we should consider creating
> >>>>ACPI_DEVICE() macro and convert existing drivers to that at some point.
> >>>
> >>>Yes, a roughly grep in the drivers directory showing about 112 files at the
> >>>moment. If you think this is the right approach going forward, we can work
> >>>on cleaning this up on a separate patch series. Please let me know what you
> >>>think.
> >>
> >>I think having ACPI_DEVICE() macro would be pretty useful and it avoids
> >>things like this if we need to add new fields in the future. Rafael has
> >>the last word, though :-)
> >
> >I agree.
> 
> Okay, how should I organize this big change? Can we do this as a separate
> patch series?

Separate patch series is fine.

Re: [V5 PATCH 1/2] ACPI / scan: Add support for ACPI _CLS device matching

[Suravee Suthikulpanit]

On 3/24/15 15:20, Rafael J. Wysocki wrote:
> On Tuesday, March 24, 2015 04:43:46 PM Mika Westerberg wrote:
>> On Tue, Mar 24, 2015 at 09:23:47AM -0500, Suravee Suthikulpanit wrote:
>>> On 3/9/15 10:20, Mika Westerberg wrote:
>>>>> [....]
>>>>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>>>>> index e530533..9a42522 100644
>>>>> --- a/include/linux/mod_devicetable.h
>>>>> +++ b/include/linux/mod_devicetable.h
>>>>> @@ -189,6 +189,7 @@ struct css_device_id {
>>>>>   struct acpi_device_id {
>>>>>   	__u8 id[ACPI_ID_LEN];
>>>>>   	kernel_ulong_t driver_data;
>>>>> +	__u32 cls;
>>>>
>>>> It would be nice if we could change ordering here but I understand that
>>>> it breaks quite many drivers. Perhaps we should consider creating
>>>> ACPI_DEVICE() macro and convert existing drivers to that at some point.
>>>
>>> Yes, a roughly grep in the drivers directory showing about 112 files at the
>>> moment. If you think this is the right approach going forward, we can work
>>> on cleaning this up on a separate patch series. Please let me know what you
>>> think.
>>
>> I think having ACPI_DEVICE() macro would be pretty useful and it avoids
>> things like this if we need to add new fields in the future. Rafael has
>> the last word, though :-)
>
> I agree.

Okay, how should I organize this big change? Can we do this as a 
separate patch series?

Thanks,
Suravee

[V5 PATCH 2/2] ata: ahci_platform: Add ACPI _CLS matching

[Suravee Suthikulpanit]

This patch adds ACPI supports for AHCI platform driver, which uses _CLS
method to match the device.

The following is an example of ASL structure in DSDT for a SATA controller,
which contains _CLS package to be matched by the ahci_platform driver:

  Device (AHC0) // AHCI Controller
  {
    Name(_HID, "AMDI0600")
    Name (_CCA, 1)
    Name (_CLS, Package (3)
    {
      0x01, // Base Class: Mass Storage
      0x06, // Sub-Class: serial ATA
      0x01, // Interface: AHCI
    })
    Name (_CRS, ResourceTemplate ()
    {
      Memory32Fixed (ReadWrite, 0xE0300000, 0x00010000)
      Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive,,,) { 387 }
    })
  }

Also, since ATA driver should not require PCI support for ATA_ACPI,
this patch removes dependency in the driver/ata/Kconfig.

Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 drivers/ata/Kconfig         | 2 +-
 drivers/ata/ahci_platform.c | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 5f60155..50305e3 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -48,7 +48,7 @@ config ATA_VERBOSE_ERROR
 
 config ATA_ACPI
 	bool "ATA ACPI Support"
-	depends on ACPI && PCI
+	depends on ACPI
 	default y
 	help
 	  This option adds support for ATA-related ACPI objects.
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 78d6ae0..842cd13 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -20,6 +20,8 @@
 #include <linux/platform_device.h>
 #include <linux/libata.h>
 #include <linux/ahci_platform.h>
+#include <linux/acpi.h>
+#include <linux/pci_ids.h>
 #include "ahci.h"
 
 #define DRV_NAME "ahci"
@@ -78,12 +80,19 @@ static const struct of_device_id ahci_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, ahci_of_match);
 
+static const struct acpi_device_id ahci_acpi_match[] = {
+	{ "", 0, PCI_CLASS_STORAGE_SATA_AHCI },
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, ahci_acpi_match);
+
 static struct platform_driver ahci_driver = {
 	.probe = ahci_probe,
 	.remove = ata_platform_remove_one,
 	.driver = {
 		.name = DRV_NAME,
 		.of_match_table = ahci_of_match,
+		.acpi_match_table = ahci_acpi_match,
 		.pm = &ahci_pm_ops,
 	},
 };
-- 
2.1.0