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

Message ID 20180104164709.64387-2-andriy.shevchenko@linux.intel.com
State New
Headers show
Series
  • ACPI, ASoC, gpio: Introduce and use acpi_dev_get_dev_name()
Related show

Commit Message

Andy Shevchenko Jan. 4, 2018, 4:47 p.m.
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(-)

Comments

Pierre-Louis Bossart Jan. 4, 2018, 5:31 p.m. | #1
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;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko Jan. 4, 2018, 5:40 p.m. | #2
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.
Pierre-Louis Bossart Jan. 5, 2018, 12:47 a.m. | #3
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;

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Jan. 5, 2018, 12:05 p.m. | #4
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...
Rafael J. Wysocki Jan. 5, 2018, 12:06 p.m. | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko Jan. 5, 2018, 12:22 p.m. | #6
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?
Rafael J. Wysocki Jan. 5, 2018, 12:34 p.m. | #7
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko Jan. 5, 2018, 12:39 p.m. | #8
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 Jan. 5, 2018, 12:43 p.m. | #9
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?
Pierre-Louis Bossart Jan. 5, 2018, 3:46 p.m. | #10
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?
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pierre-Louis Bossart Jan. 5, 2018, 3:55 p.m. | #11
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.
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;