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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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;
>