[PATCH v2 0/3] ACPI, ASoC, gpio: Introduce and use acpi_dev_get_dev_name()

[PATCH v2 0/3] ACPI, ASoC, gpio: Introduce and use acpi_dev_get_dev_name()

[Andy Shevchenko]

It appears that at least one current user (patch 2) and upcoming one
(patch 3) need to get device name by ACPI HID.

Here we introduce acpi_dev_get_dev_name() based on code done for
acpi_dev_present() and reuse it where appropriate.

The series has been tested on Intel Edison with ACPI enabled U-Boot,
while patch 2 wasn't tested anyhow (Pierre, can you help with it?).

Since patch 1 and cross subsystem nature of the series I think the best
way is to push this via Rafael's linux-pm tree.

In worst case, if, Rafael, you have no objection, push at least first
patch for this cycle to allow utilization in the future.

Andy Shevchenko (3):
  ACPI / utils: Introduce acpi_dev_get_dev_name()
  ASoC: Intel - Convert users to use acpi_dev_get_dev_name()
  gpio: merrifield: Add support of ACPI enabled platforms

 drivers/acpi/utils.c                    | 40 +++++++++++++++++++++++++++------
 drivers/gpio/gpio-merrifield.c          | 11 ++++++++-
 include/acpi/acpi_bus.h                 |  1 +
 include/linux/acpi.h                    |  6 +++++
 include/sound/soc-acpi.h                |  7 ------
 sound/soc/intel/boards/bytcht_da7213.c  |  2 +-
 sound/soc/intel/boards/bytcr_rt5640.c   |  2 +-
 sound/soc/intel/boards/bytcr_rt5651.c   |  2 +-
 sound/soc/intel/boards/cht_bsw_rt5645.c |  2 +-
 sound/soc/intel/boards/cht_bsw_rt5672.c |  2 +-
 sound/soc/soc-acpi.c                    | 33 ---------------------------
 11 files changed, 55 insertions(+), 53 deletions(-)

-- 
2.15.1

[PATCH v2 1/3] ACPI / utils: Introduce acpi_dev_get_dev_name()

[Andy Shevchenko]

Sometimes the user want to have device name of the match rather than
just checking if device present or not. To make life easier for such
users introduce acpi_dev_get_dev_name() helper based on code for
acpi_dev_present().

To be more consistent with the purpose rename

  struct acpi_dev_present_info  -> struct acpi_dev_match_info
  acpi_dev_present_cb()         -> acpi_dev_match_cb()

in the utils.c file.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/utils.c    | 40 +++++++++++++++++++++++++++++++++-------
 include/acpi/acpi_bus.h |  1 +
 include/linux/acpi.h    |  6 ++++++
 3 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 9d49a1acebe3..1da9e986d510 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -737,16 +737,17 @@ bool acpi_dev_found(const char *hid)
 }
 EXPORT_SYMBOL(acpi_dev_found);
 
-struct acpi_dev_present_info {
+struct acpi_dev_match_info {
+	const char *dev_name;
 	struct acpi_device_id hid[2];
 	const char *uid;
 	s64 hrv;
 };
 
-static int acpi_dev_present_cb(struct device *dev, void *data)
+static int acpi_dev_match_cb(struct device *dev, void *data)
 {
 	struct acpi_device *adev = to_acpi_device(dev);
-	struct acpi_dev_present_info *match = data;
+	struct acpi_dev_match_info *match = data;
 	unsigned long long hrv;
 	acpi_status status;
 
@@ -757,6 +758,8 @@ static int acpi_dev_present_cb(struct device *dev, void *data)
 	    strcmp(adev->pnp.unique_id, match->uid)))
 		return 0;
 
+	match->dev_name = acpi_dev_name(adev);
+
 	if (match->hrv == -1)
 		return 1;
 
@@ -789,20 +792,43 @@ static int acpi_dev_present_cb(struct device *dev, void *data)
  */
 bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
 {
-	struct acpi_dev_present_info match = {};
+	struct acpi_dev_match_info match = {};
 	struct device *dev;
 
 	strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
 	match.uid = uid;
 	match.hrv = hrv;
 
-	dev = bus_find_device(&acpi_bus_type, NULL, &match,
-			      acpi_dev_present_cb);
-
+	dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb);
 	return !!dev;
 }
 EXPORT_SYMBOL(acpi_dev_present);
 
+/**
+ * acpi_dev_get_dev_name - Return device name of first match of ACPI device
+ * @hid: Hardware ID of the device.
+ * @uid: Unique ID of the device, pass NULL to not check _UID
+ * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
+ *
+ * Return device name if a matching device was present
+ * at the moment of invocation, or NULL otherwise.
+ *
+ * See additional information in acpi_dev_present() as well.
+ */
+const char *acpi_dev_get_dev_name(const char *hid, const char *uid, s64 hrv)
+{
+	struct acpi_dev_match_info match = {};
+	struct device *dev;
+
+	strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
+	match.uid = uid;
+	match.hrv = hrv;
+
+	dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb);
+	return dev ? match.dev_name : NULL;
+}
+EXPORT_SYMBOL(acpi_dev_get_dev_name);
+
 /*
  * acpi_backlight= handling, this is done here rather then in video_detect.c
  * because __setup cannot be used in modules.
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 79287629c888..8883f5ebb6ce 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -90,6 +90,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const guid_t *guid, u64 rev,
 
 bool acpi_dev_found(const char *hid);
 bool acpi_dev_present(const char *hid, const char *uid, s64 hrv);
+const char *acpi_dev_get_dev_name(const char *hid, const char *uid, s64 hrv);
 
 #ifdef CONFIG_ACPI
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 1922063f6894..d6576eec45d1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -644,6 +644,12 @@ static inline bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
 	return false;
 }
 
+static inline
+const char *acpi_dev_get_dev_name(const char *hid, const char *uid, s64 hrv)
+{
+	return NULL;
+}
+
 static inline bool is_acpi_node(struct fwnode_handle *fwnode)
 {
 	return false;
-- 
2.15.1

[PATCH v2 2/3] ASoC: Intel - Convert users to use acpi_dev_get_dev_name()

[Andy Shevchenko]

Instead of home grown snd_soc_acpi_find_name_from_hid() use
acpi_dev_get_dev_name().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/sound/soc-acpi.h                |  7 -------
 sound/soc/intel/boards/bytcht_da7213.c  |  2 +-
 sound/soc/intel/boards/bytcr_rt5640.c   |  2 +-
 sound/soc/intel/boards/bytcr_rt5651.c   |  2 +-
 sound/soc/intel/boards/cht_bsw_rt5645.c |  2 +-
 sound/soc/intel/boards/cht_bsw_rt5672.c |  2 +-
 sound/soc/soc-acpi.c                    | 33 ---------------------------------
 7 files changed, 5 insertions(+), 45 deletions(-)

diff --git a/include/sound/soc-acpi.h b/include/sound/soc-acpi.h
index a7d8d335b043..a71a935b5bc8 100644
--- a/include/sound/soc-acpi.h
+++ b/include/sound/soc-acpi.h
@@ -27,16 +27,9 @@ struct snd_soc_acpi_package_context {
 };
 
 #if IS_ENABLED(CONFIG_ACPI)
-/* translation fron HID to I2C name, needed for DAI codec_name */
-const char *snd_soc_acpi_find_name_from_hid(const u8 hid[ACPI_ID_LEN]);
 bool snd_soc_acpi_find_package_from_hid(const u8 hid[ACPI_ID_LEN],
 				    struct snd_soc_acpi_package_context *ctx);
 #else
-static inline const char *
-snd_soc_acpi_find_name_from_hid(const u8 hid[ACPI_ID_LEN])
-{
-	return NULL;
-}
 static inline bool
 snd_soc_acpi_find_package_from_hid(const u8 hid[ACPI_ID_LEN],
 				   struct snd_soc_acpi_package_context *ctx)
diff --git a/sound/soc/intel/boards/bytcht_da7213.c b/sound/soc/intel/boards/bytcht_da7213.c
index c4d82ad41bd7..828f839d0701 100644
--- a/sound/soc/intel/boards/bytcht_da7213.c
+++ b/sound/soc/intel/boards/bytcht_da7213.c
@@ -243,7 +243,7 @@ static int bytcht_da7213_probe(struct platform_device *pdev)
 	}
 
 	/* fixup codec name based on HID */
-	i2c_name = snd_soc_acpi_find_name_from_hid(mach->id);
+	i2c_name = acpi_dev_get_dev_name(mach->id, NULL, -1);
 	if (i2c_name) {
 		snprintf(codec_name, sizeof(codec_name),
 			"%s%s", "i2c-", i2c_name);
diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index f2c0fc415e52..849344baf233 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -762,7 +762,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 	}
 
 	/* fixup codec name based on HID */
-	i2c_name = snd_soc_acpi_find_name_from_hid(mach->id);
+	i2c_name = acpi_dev_get_dev_name(mach->id, NULL, -1);
 	if (i2c_name) {
 		snprintf(byt_rt5640_codec_name, sizeof(byt_rt5640_codec_name),
 			"%s%s", "i2c-", i2c_name);
diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 488ec48f296a..87bab84c13a7 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -511,7 +511,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	}
 
 	/* fixup codec name based on HID */
-	i2c_name = snd_soc_acpi_find_name_from_hid(mach->id);
+	i2c_name = acpi_dev_get_dev_name(mach->id, NULL, -1);
 	if (i2c_name) {
 		snprintf(byt_rt5651_codec_name, sizeof(byt_rt5651_codec_name),
 			"%s%s", "i2c-", i2c_name);
diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c
index f898ee140cdc..9443b4d4f8b0 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5645.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
@@ -573,7 +573,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 		}
 
 	/* fixup codec name based on HID */
-	i2c_name = snd_soc_acpi_find_name_from_hid(mach->id);
+	i2c_name = acpi_dev_get_dev_name(mach->id, NULL, -1);
 	if (i2c_name) {
 		snprintf(cht_rt5645_codec_name, sizeof(cht_rt5645_codec_name),
 			"%s%s", "i2c-", i2c_name);
diff --git a/sound/soc/intel/boards/cht_bsw_rt5672.c b/sound/soc/intel/boards/cht_bsw_rt5672.c
index f8f21eee9b2d..029670048c17 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5672.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5672.c
@@ -396,7 +396,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 
 	/* fixup codec name based on HID */
 	if (mach) {
-		i2c_name = snd_soc_acpi_find_name_from_hid(mach->id);
+		i2c_name = acpi_dev_get_dev_name(mach->id, NULL, -1);
 		if (i2c_name) {
 			snprintf(drv->codec_name, sizeof(drv->codec_name),
 				 "i2c-%s", i2c_name);
diff --git a/sound/soc/soc-acpi.c b/sound/soc/soc-acpi.c
index f21df28bc28e..ed2e7f1a77ea 100644
--- a/sound/soc/soc-acpi.c
+++ b/sound/soc/soc-acpi.c
@@ -16,39 +16,6 @@
 
 #include <sound/soc-acpi.h>
 
-static acpi_status snd_soc_acpi_find_name(acpi_handle handle, u32 level,
-				      void *context, void **ret)
-{
-	struct acpi_device *adev;
-	const char *name = NULL;
-
-	if (acpi_bus_get_device(handle, &adev))
-		return AE_OK;
-
-	if (adev->status.present && adev->status.functional) {
-		name = acpi_dev_name(adev);
-		*(const char **)ret = name;
-		return AE_CTRL_TERMINATE;
-	}
-
-	return AE_OK;
-}
-
-const char *snd_soc_acpi_find_name_from_hid(const u8 hid[ACPI_ID_LEN])
-{
-	const char *name = NULL;
-	acpi_status status;
-
-	status = acpi_get_devices(hid, snd_soc_acpi_find_name, NULL,
-				  (void **)&name);
-
-	if (ACPI_FAILURE(status) || name[0] == '\0')
-		return NULL;
-
-	return name;
-}
-EXPORT_SYMBOL_GPL(snd_soc_acpi_find_name_from_hid);
-
 static acpi_status snd_soc_acpi_mach_match(acpi_handle handle, u32 level,
 				       void *context, void **ret)
 {
-- 
2.15.1

[PATCH v2 3/3] gpio: merrifield: Add support of ACPI enabled platforms

[Andy Shevchenko]

The driver needs the pin control device name for ACPI.

We are looking through ACPI namespace and return first found device
based on ACPI HID for Intel Merrifield FLIS.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-merrifield.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-merrifield.c b/drivers/gpio/gpio-merrifield.c
index dd67a31ac337..68320d57d5f0 100644
--- a/drivers/gpio/gpio-merrifield.c
+++ b/drivers/gpio/gpio-merrifield.c
@@ -9,6 +9,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/acpi.h>
 #include <linux/bitops.h>
 #include <linux/gpio/driver.h>
 #include <linux/init.h>
@@ -380,9 +381,16 @@ static void mrfld_irq_init_hw(struct mrfld_gpio *priv)
 	}
 }
 
+static const char *mrfld_gpio_get_pinctrl_dev_name(const char *fallback)
+{
+	const char *dev_name = acpi_dev_get_dev_name("INTC1002", NULL, -1);
+	return dev_name ? dev_name : fallback;
+}
+
 static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	const struct mrfld_gpio_pinrange *range;
+	const char *pinctrl_dev_name;
 	struct mrfld_gpio *priv;
 	u32 gpio_base, irq_base;
 	void __iomem *base;
@@ -439,10 +447,11 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
 		return retval;
 	}
 
+	pinctrl_dev_name = mrfld_gpio_get_pinctrl_dev_name("pinctrl-merrifield");
 	for (i = 0; i < ARRAY_SIZE(mrfld_gpio_ranges); i++) {
 		range = &mrfld_gpio_ranges[i];
 		retval = gpiochip_add_pin_range(&priv->chip,
-						"pinctrl-merrifield",
+						pinctrl_dev_name,
 						range->gpio_base,
 						range->pin_base,
 						range->npins);
-- 
2.15.1

Re: [PATCH v2 1/3] ACPI / utils: Introduce acpi_dev_get_dev_name()

[Pierre-Louis Bossart]

On 1/4/18 10:47 AM, Andy Shevchenko wrote:
> Sometimes the user want to have device name of the match rather than
> just checking if device present or not. To make life easier for such
> users introduce acpi_dev_get_dev_name() helper based on code for
> acpi_dev_present().
> 
> To be more consistent with the purpose rename
> 
>    struct acpi_dev_present_info  -> struct acpi_dev_match_info
>    acpi_dev_present_cb()         -> acpi_dev_match_cb()
> 
> in the utils.c file.

I would have done this differently. You have two routines 
(acpi_dev_present and acpi_dev_get_dev_name) which do the same thing 
except for what they return (bool and const char *) respectively.

This could be factored with

static struct device *dev _acpi_dev_present(const char *hid, const char 
*uid, s64 hrv) {

	struct acpi_dev_present_info match = {};
	struct device *dev;

	strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
	match.uid = uid;
	match.hrv = hrv;

	dev = bus_find_device(&acpi_bus_type, NULL, &match,
		acpi_dev_present_cb);
	
	return dev;
}

bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
{
	return !!_acpi_dev_present(const char *hid, const char *uid, s64 hrv);
}

const char *acpi_dev_get_dev_name(const char *hid, const char *uid, s64 hrv)
{
	struct device *dev;

	dev = _acpi_dev_present(const char *hid, const char *uid, s64 hrv);

	return dev ? match.dev_name : NULL;
}

That said you are a much better programmer than me so the other code is 
fine with me...

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/acpi/utils.c    | 40 +++++++++++++++++++++++++++++++++-------
>   include/acpi/acpi_bus.h |  1 +
>   include/linux/acpi.h    |  6 ++++++
>   3 files changed, 40 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 9d49a1acebe3..1da9e986d510 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -737,16 +737,17 @@ bool acpi_dev_found(const char *hid)
>   }
>   EXPORT_SYMBOL(acpi_dev_found);
>   
> -struct acpi_dev_present_info {
> +struct acpi_dev_match_info {
> +	const char *dev_name;
>   	struct acpi_device_id hid[2];
>   	const char *uid;
>   	s64 hrv;
>   };
>   
> -static int acpi_dev_present_cb(struct device *dev, void *data)
> +static int acpi_dev_match_cb(struct device *dev, void *data)
>   {
>   	struct acpi_device *adev = to_acpi_device(dev);
> -	struct acpi_dev_present_info *match = data;
> +	struct acpi_dev_match_info *match = data;
>   	unsigned long long hrv;
>   	acpi_status status;
>   
> @@ -757,6 +758,8 @@ static int acpi_dev_present_cb(struct device *dev, void *data)
>   	    strcmp(adev->pnp.unique_id, match->uid)))
>   		return 0;
>   
> +	match->dev_name = acpi_dev_name(adev);
> +
>   	if (match->hrv == -1)
>   		return 1;
>   
> @@ -789,20 +792,43 @@ static int acpi_dev_present_cb(struct device *dev, void *data)
>    */
>   bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
>   {
> -	struct acpi_dev_present_info match = {};
> +	struct acpi_dev_match_info match = {};
>   	struct device *dev;
>   
>   	strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
>   	match.uid = uid;
>   	match.hrv = hrv;
>   
> -	dev = bus_find_device(&acpi_bus_type, NULL, &match,
> -			      acpi_dev_present_cb);
> -
> +	dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb);
>   	return !!dev;
>   }
>   EXPORT_SYMBOL(acpi_dev_present);
>   
> +/**
> + * acpi_dev_get_dev_name - Return device name of first match of ACPI device
> + * @hid: Hardware ID of the device.
> + * @uid: Unique ID of the device, pass NULL to not check _UID
> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
> + *
> + * Return device name if a matching device was present
> + * at the moment of invocation, or NULL otherwise.
> + *
> + * See additional information in acpi_dev_present() as well.
> + */
> +const char *acpi_dev_get_dev_name(const char *hid, const char *uid, s64 hrv)
> +{
> +	struct acpi_dev_match_info match = {};
> +	struct device *dev;
> +
> +	strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
> +	match.uid = uid;
> +	match.hrv = hrv;
> +
> +	dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb);
> +	return dev ? match.dev_name : NULL;
> +}
> +EXPORT_SYMBOL(acpi_dev_get_dev_name);
> +
>   /*
>    * acpi_backlight= handling, this is done here rather then in video_detect.c
>    * because __setup cannot be used in modules.
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 79287629c888..8883f5ebb6ce 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -90,6 +90,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const guid_t *guid, u64 rev,
>   
>   bool acpi_dev_found(const char *hid);
>   bool acpi_dev_present(const char *hid, const char *uid, s64 hrv);
> +const char *acpi_dev_get_dev_name(const char *hid, const char *uid, s64 hrv);
>   
>   #ifdef CONFIG_ACPI
>   
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 1922063f6894..d6576eec45d1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -644,6 +644,12 @@ static inline bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
>   	return false;
>   }
>   
> +static inline
> +const char *acpi_dev_get_dev_name(const char *hid, const char *uid, s64 hrv)
> +{
> +	return NULL;
> +}
> +
>   static inline bool is_acpi_node(struct fwnode_handle *fwnode)
>   {
>   	return false;
> 

Re: [PATCH v2 0/3] ACPI, ASoC, gpio: Introduce and use acpi_dev_get_dev_name()

[Pierre-Louis Bossart]

On 1/4/18 10:47 AM, Andy Shevchenko wrote:
> It appears that at least one current user (patch 2) and upcoming one
> (patch 3) need to get device name by ACPI HID.
> 
> Here we introduce acpi_dev_get_dev_name() based on code done for
> acpi_dev_present() and reuse it where appropriate.
> 
> The series has been tested on Intel Edison with ACPI enabled U-Boot,
> while patch 2 wasn't tested anyhow (Pierre, can you help with it?).

Yes I'll try to test later this week. I just have to figure out which of 
my devices needs this (been a while since I worked on this).

> 
> Since patch 1 and cross subsystem nature of the series I think the best
> way is to push this via Rafael's linux-pm tree.
> 
> In worst case, if, Rafael, you have no objection, push at least first
> patch for this cycle to allow utilization in the future.
> 
> Andy Shevchenko (3):
>    ACPI / utils: Introduce acpi_dev_get_dev_name()
>    ASoC: Intel - Convert users to use acpi_dev_get_dev_name()
>    gpio: merrifield: Add support of ACPI enabled platforms
> 
>   drivers/acpi/utils.c                    | 40 +++++++++++++++++++++++++++------
>   drivers/gpio/gpio-merrifield.c          | 11 ++++++++-
>   include/acpi/acpi_bus.h                 |  1 +
>   include/linux/acpi.h                    |  6 +++++
>   include/sound/soc-acpi.h                |  7 ------
>   sound/soc/intel/boards/bytcht_da7213.c  |  2 +-
>   sound/soc/intel/boards/bytcr_rt5640.c   |  2 +-
>   sound/soc/intel/boards/bytcr_rt5651.c   |  2 +-
>   sound/soc/intel/boards/cht_bsw_rt5645.c |  2 +-
>   sound/soc/intel/boards/cht_bsw_rt5672.c |  2 +-
>   sound/soc/soc-acpi.c                    | 33 ---------------------------
>   11 files changed, 55 insertions(+), 53 deletions(-)
> 

Re: [alsa-devel] [PATCH v2 1/3] ACPI / utils: Introduce acpi_dev_get_dev_name()

[Andy Shevchenko]

On Thu, 2018-01-04 at 11:31 -0600, Pierre-Louis Bossart wrote:
> On 1/4/18 10:47 AM, Andy Shevchenko wrote:
> > Sometimes the user want to have device name of the match rather than
> > just checking if device present or not. To make life easier for such
> > users introduce acpi_dev_get_dev_name() helper based on code for
> > acpi_dev_present().
> > 
> > To be more consistent with the purpose rename
> > 
> >    struct acpi_dev_present_info  -> struct acpi_dev_match_info
> >    acpi_dev_present_cb()         -> acpi_dev_match_cb()
> > 
> > in the utils.c file.
> 
> I would have done this differently. You have two routines 
> (acpi_dev_present and acpi_dev_get_dev_name) which do the same thing 
> except for what they return (bool and const char *) respectively.

I have thought about it :-)

I decide that for two current users there is a little benefit over
effort and readability.

See also below.

> This could be factored with
> 
> static struct device *dev _acpi_dev_present(const char *hid, const
> char 
> *uid, s64 hrv) {
> 
> 	struct acpi_dev_present_info match = {};
> 	struct device *dev;
> 
> 	strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
> 	match.uid = uid;
> 	match.hrv = hrv;
> 
> 	dev = bus_find_device(&acpi_bus_type, NULL, &match,
> 		acpi_dev_present_cb);
> 	
> 	return dev;
> }
> 
> bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
> {
> 	return !!_acpi_dev_present(const char *hid, const char *uid, s64
> hrv);
> }
> 
> const char *acpi_dev_get_dev_name(const char *hid, const char *uid,
> s64 hrv)
> {
> 	struct device *dev;
> 
> 	dev = _acpi_dev_present(const char *hid, const char *uid, s64
> hrv);
> 
> 	return dev ? match.dev_name : NULL;

And where is the match declaration?

Yes, needs to be duplicated. That's why I stopped going this direction.

It still might make sense when 3+ variant will appear.

> }
> 
> That said you are a much better programmer than me so the other code
> is 
> fine with me...

Thanks.

Would be nice to get your Tested-by at some point.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 1/3] ACPI / utils: Introduce acpi_dev_get_dev_name()

[Pierre-Louis Bossart]

On 01/04/2018 10:47 AM, Andy Shevchenko wrote:
> Sometimes the user want to have device name of the match rather than
> just checking if device present or not. To make life easier for such
> users introduce acpi_dev_get_dev_name() helper based on code for
> acpi_dev_present().
>
> To be more consistent with the purpose rename
>
>    struct acpi_dev_present_info  -> struct acpi_dev_match_info
>    acpi_dev_present_cb()         -> acpi_dev_match_cb()
>
> in the utils.c file.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
This works fine on a Dell 5585 where the default codec dai name needs to 
be updated based on the actual HID information, so
Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

the next patch does not apply directly however, and needs additional 
changes for the ES8316 machine driver. see the changes here: 
https://github.com/plbossart/sound/tree/topic/bytcht-acpi-fixes

It's probably best to let this patch go through the acpi tree, and the 
next one through Mark's tree once the first is merged and all the other 
Kconfig/acpi stuff is also in -next?

> ---
>   drivers/acpi/utils.c    | 40 +++++++++++++++++++++++++++++++++-------
>   include/acpi/acpi_bus.h |  1 +
>   include/linux/acpi.h    |  6 ++++++
>   3 files changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 9d49a1acebe3..1da9e986d510 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -737,16 +737,17 @@ bool acpi_dev_found(const char *hid)
>   }
>   EXPORT_SYMBOL(acpi_dev_found);
>   
> -struct acpi_dev_present_info {
> +struct acpi_dev_match_info {
> +	const char *dev_name;
>   	struct acpi_device_id hid[2];
>   	const char *uid;
>   	s64 hrv;
>   };
>   
> -static int acpi_dev_present_cb(struct device *dev, void *data)
> +static int acpi_dev_match_cb(struct device *dev, void *data)
>   {
>   	struct acpi_device *adev = to_acpi_device(dev);
> -	struct acpi_dev_present_info *match = data;
> +	struct acpi_dev_match_info *match = data;
>   	unsigned long long hrv;
>   	acpi_status status;
>   
> @@ -757,6 +758,8 @@ static int acpi_dev_present_cb(struct device *dev, void *data)
>   	    strcmp(adev->pnp.unique_id, match->uid)))
>   		return 0;
>   
> +	match->dev_name = acpi_dev_name(adev);
> +
>   	if (match->hrv == -1)
>   		return 1;
>   
> @@ -789,20 +792,43 @@ static int acpi_dev_present_cb(struct device *dev, void *data)
>    */
>   bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
>   {
> -	struct acpi_dev_present_info match = {};
> +	struct acpi_dev_match_info match = {};
>   	struct device *dev;
>   
>   	strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
>   	match.uid = uid;
>   	match.hrv = hrv;
>   
> -	dev = bus_find_device(&acpi_bus_type, NULL, &match,
> -			      acpi_dev_present_cb);
> -
> +	dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb);
>   	return !!dev;
>   }
>   EXPORT_SYMBOL(acpi_dev_present);
>   
> +/**
> + * acpi_dev_get_dev_name - Return device name of first match of ACPI device
> + * @hid: Hardware ID of the device.
> + * @uid: Unique ID of the device, pass NULL to not check _UID
> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
> + *
> + * Return device name if a matching device was present
> + * at the moment of invocation, or NULL otherwise.
> + *
> + * See additional information in acpi_dev_present() as well.
> + */
> +const char *acpi_dev_get_dev_name(const char *hid, const char *uid, s64 hrv)
> +{
> +	struct acpi_dev_match_info match = {};
> +	struct device *dev;
> +
> +	strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
> +	match.uid = uid;
> +	match.hrv = hrv;
> +
> +	dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb);
> +	return dev ? match.dev_name : NULL;
> +}
> +EXPORT_SYMBOL(acpi_dev_get_dev_name);
> +
>   /*
>    * acpi_backlight= handling, this is done here rather then in video_detect.c
>    * because __setup cannot be used in modules.
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 79287629c888..8883f5ebb6ce 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -90,6 +90,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const guid_t *guid, u64 rev,
>   
>   bool acpi_dev_found(const char *hid);
>   bool acpi_dev_present(const char *hid, const char *uid, s64 hrv);
> +const char *acpi_dev_get_dev_name(const char *hid, const char *uid, s64 hrv);
>   
>   #ifdef CONFIG_ACPI
>   
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 1922063f6894..d6576eec45d1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -644,6 +644,12 @@ static inline bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
>   	return false;
>   }
>   
> +static inline
> +const char *acpi_dev_get_dev_name(const char *hid, const char *uid, s64 hrv)
> +{
> +	return NULL;
> +}
> +
>   static inline bool is_acpi_node(struct fwnode_handle *fwnode)
>   {
>   	return false;

Re: [PATCH v2 1/3] ACPI / utils: Introduce acpi_dev_get_dev_name()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 247 bytes --]

On Thu, Jan 04, 2018 at 06:47:56PM -0600, Pierre-Louis Bossart wrote:

> one through Mark's tree once the first is merged and all the other
> Kconfig/acpi stuff is also in -next?

If we could get this in during the merge window that'd be ideal...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] ACPI / utils: Introduce acpi_dev_get_dev_name()

[Rafael J. Wysocki]

On Thu, Jan 4, 2018 at 5:47 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Sometimes the user want to have device name of the match rather than
> just checking if device present or not.

I would give an example here.

> To make life easier for such
> users introduce acpi_dev_get_dev_name()

What about acpi_dev_get_match_name() or just acpi_dev_match_name()?

> helper based on code for
> acpi_dev_present().
>
> To be more consistent with the purpose rename
>
>   struct acpi_dev_present_info  -> struct acpi_dev_match_info
>   acpi_dev_present_cb()         -> acpi_dev_match_cb()
>
> in the utils.c file.

OK

Thanks,
Rafael

Re: [PATCH v2 1/3] ACPI / utils: Introduce acpi_dev_get_dev_name()

[Andy Shevchenko]

On Fri, 2018-01-05 at 13:06 +0100, Rafael J. Wysocki wrote:
> On Thu, Jan 4, 2018 at 5:47 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Sometimes the user want to have device name of the match rather than
> > just checking if device present or not.
> 
> I would give an example here.

You mean to mention what is in patch 2?
I can do that.

"Subset of Intel ASoC drivers is an existing user of this API when they
need to find an actual instance of the codec device based on its ACPI
HID."

> > To make life easier for such
> > users introduce acpi_dev_get_dev_name()
> 
> What about acpi_dev_get_match_name() or just acpi_dev_match_name()?

It's "get", not "match".

So, acpi_dev_get_name() then?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 1/3] ACPI / utils: Introduce acpi_dev_get_dev_name()

[Rafael J. Wysocki]

On Fri, Jan 5, 2018 at 1:22 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, 2018-01-05 at 13:06 +0100, Rafael J. Wysocki wrote:
>> On Thu, Jan 4, 2018 at 5:47 PM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>> > Sometimes the user want to have device name of the match rather than
>> > just checking if device present or not.
>>
>> I would give an example here.
>
> You mean to mention what is in patch 2?
> I can do that.
>
> "Subset of Intel ASoC drivers is an existing user of this API when they
> need to find an actual instance of the codec device based on its ACPI
> HID."

And why do they need the name (and not something else)?

>> > To make life easier for such
>> > users introduce acpi_dev_get_dev_name()
>>
>> What about acpi_dev_get_match_name() or just acpi_dev_match_name()?
>
> It's "get", not "match".

OK

> So, acpi_dev_get_name() then?

It would be somewhat clearer to call it acpi_dev_get_first_match_name() IMO.

Otherwise it may not be clear what name this is going to return.

Re: [PATCH v2 1/3] ACPI / utils: Introduce acpi_dev_get_dev_name()

[Andy Shevchenko]

On Fri, 2018-01-05 at 13:34 +0100, Rafael J. Wysocki wrote:
> On Fri, Jan 5, 2018 at 1:22 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, 2018-01-05 at 13:06 +0100, Rafael J. Wysocki wrote:
> > > On Thu, Jan 4, 2018 at 5:47 PM, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

> > "Subset of Intel ASoC drivers is an existing user of this API when
> > they
> > need to find an actual instance of the codec device based on its
> > ACPI
> > HID."
> 
> And why do they need the name (and not something else)?

I don't know the guts of Intel ASoC implementation, so, can't answer
this without investigation.

Pierre, can you elaborate this piece?

> > So, acpi_dev_get_name() then?
> 
> It would be somewhat clearer to call it
> acpi_dev_get_first_match_name() IMO.
> 
> Otherwise it may not be clear what name this is going to return.

Works for me.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 1/3] ACPI / utils: Introduce acpi_dev_get_dev_name()

[Andy Shevchenko]

On Thu, 2018-01-04 at 18:47 -0600, Pierre-Louis Bossart wrote:
> 
> On 01/04/2018 10:47 AM, Andy Shevchenko wrote:
> > Sometimes the user want to have device name of the match rather than
> > just checking if device present or not. To make life easier for such
> > users introduce acpi_dev_get_dev_name() helper based on code for
> > acpi_dev_present().
> > 
> > To be more consistent with the purpose rename
> > 
> >    struct acpi_dev_present_info  -> struct acpi_dev_match_info
> >    acpi_dev_present_cb()         -> acpi_dev_match_cb()
> > 
> > in the utils.c file.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> This works fine on a Dell 5585 where the default codec dai name needs
> to 
> be updated based on the actual HID information, so
> Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Thanks!

Is this only for patch 1, or for both 1 and 2 ?

> the next patch does not apply directly however, and needs additional 
> changes for the ES8316 machine driver. see the changes here: 
> https://github.com/plbossart/sound/tree/topic/bytcht-acpi-fixes

You mean because of this
https://github.com/plbossart/sound/commit/2416827fa6d221b27edd6397f17daa
a1f3cd5fb0
?

I.e. I need to rebase on top of your series.

> It's probably best to let this patch go through the acpi tree, and
> the 
> next one through Mark's tree once the first is merged and all the
> other 
> Kconfig/acpi stuff is also in -next?

It wouldn't prevent to push patch 3 as well via linux-pm at the same
time as patch 1.

Linus, do you have any objection on that?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 1/3] ACPI / utils: Introduce acpi_dev_get_dev_name()

[Pierre-Louis Bossart]

On 1/5/18 6:43 AM, Andy Shevchenko wrote:
> On Thu, 2018-01-04 at 18:47 -0600, Pierre-Louis Bossart wrote:
>>
>> On 01/04/2018 10:47 AM, Andy Shevchenko wrote:
>>> Sometimes the user want to have device name of the match rather than
>>> just checking if device present or not. To make life easier for such
>>> users introduce acpi_dev_get_dev_name() helper based on code for
>>> acpi_dev_present().
>>>
>>> To be more consistent with the purpose rename
>>>
>>>     struct acpi_dev_present_info  -> struct acpi_dev_match_info
>>>     acpi_dev_present_cb()         -> acpi_dev_match_cb()
>>>
>>> in the utils.c file.
>>>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>
>> This works fine on a Dell 5585 where the default codec dai name needs
>> to
>> be updated based on the actual HID information, so
>> Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> Thanks!
> 
> Is this only for patch 1, or for both 1 and 2 ?
> 
>> the next patch does not apply directly however, and needs additional
>> changes for the ES8316 machine driver. see the changes here:
>> https://github.com/plbossart/sound/tree/topic/bytcht-acpi-fixes
> 
> You mean because of this
> https://github.com/plbossart/sound/commit/2416827fa6d221b27edd6397f17daa
> a1f3cd5fb0
> ?

No, not only, there is also a conflict in soc-acpi.c
> 
> I.e. I need to rebase on top of your series.

The code I use contains all the Kconfig cleanups (v3 sent yesterday), so 
there is another dependency here.

> 
>> It's probably best to let this patch go through the acpi tree, and
>> the
>> next one through Mark's tree once the first is merged and all the
>> other
>> Kconfig/acpi stuff is also in -next?
> 
> It wouldn't prevent to push patch 3 as well via linux-pm at the same
> time as patch 1.
> 
> Linus, do you have any objection on that?
> 

Re: [PATCH v2 1/3] ACPI / utils: Introduce acpi_dev_get_dev_name()

[Pierre-Louis Bossart]

On 1/5/18 6:39 AM, Andy Shevchenko wrote:
> On Fri, 2018-01-05 at 13:34 +0100, Rafael J. Wysocki wrote:
>> On Fri, Jan 5, 2018 at 1:22 PM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>> On Fri, 2018-01-05 at 13:06 +0100, Rafael J. Wysocki wrote:
>>>> On Thu, Jan 4, 2018 at 5:47 PM, Andy Shevchenko
>>>> <andriy.shevchenko@linux.intel.com> wrote:
> 
>>> "Subset of Intel ASoC drivers is an existing user of this API when
>>> they
>>> need to find an actual instance of the codec device based on its
>>> ACPI
>>> HID."
>>
>> And why do they need the name (and not something else)?
> 
> I don't know the guts of Intel ASoC implementation, so, can't answer
> this without investigation.
> 
> Pierre, can you elaborate this piece?

Because the ASoC DAI structures use strings to represent the codec name. 
See e.g.

http://elixir.free-electrons.com/linux/latest/source/sound/soc/intel/boards/cht_bsw_rt5645.c#L480
http://elixir.free-electrons.com/linux/latest/source/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c#L37

In most cases a hard-coded name is fine, except when the BIOS uses a 
different HID or the ACPI subsystem changes the :00 ending to :01

So what the code does is that you look for a set of known HIDs, create 
the machine driver and pass the HID as pdata, then in the machine driver 
once you query the string that lets ASoC do the rest of the card creation.

You can argue that ASoC should not use strings, but that's not really a 
debate at this point: all I am trying to do is to make devices work when 
their BIOS is slightly different from the norm.

> 
>>> So, acpi_dev_get_name() then?
>>
>> It would be somewhat clearer to call it
>> acpi_dev_get_first_match_name() IMO.
>>
>> Otherwise it may not be clear what name this is going to return.
> 
> Works for me.
> 
>