Date:   Fri, 10 Nov 2017 08:39:37 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Jeremy Cline <jeremy@...ine.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>
Cc:     linux-kernel@...r.kernel.org, alsa-devel@...a-project.org,
        Takashi Iwai <tiwai@...e.com>
Subject: Re: [alsa-devel] [PATCH] ASoC: Intel: common: Replace
 sst_acpi_check_hid with acpi_dev_present



On 11/10/2017 05:33 AM, Hans de Goede wrote:
> Hi,
>
> On 09-11-17 22:07, Jeremy Cline wrote:
>> Replace the sst_acpi_check_hid helper function added in
>> commit 915ae2b9f0fe ("ASoC: Intel: Create a helper to search for
>> matching machine") with the generic acpi_dev_present helper function
>> and remove the now unused sst_acpi_check_hid function. This should have
>> no functional change.
>>
>> Signed-off-by: Jeremy Cline <jeremy@...ine.org>
>
> I've just successfully tested this on an Assu T100HA:
>
> Tested-by: Hans de Goede <hdegoede@...hat.com>
> Reviewed-by: Hans de Goede <hdegoede@...hat.com>

I don't mind this change, but I'd also like to see a bit more testing 
with weird devices to check that the code is indeed equivalent (not 
obvious from a quick code inspection). Surface 3 was broken recently due 
to lack of testing of a change that 'should have been functionally 
equivalent'.

Also this patch will not apply, the code was changed to remove any 
reference to the sst driver, you need to rebase against broonie/for-next.

>
> Regards,
>
> Hans
>
>
>
>> ---
>>   sound/soc/intel/common/sst-acpi.h       |  3 ---
>>   sound/soc/intel/common/sst-match-acpi.c | 32 
>> ++------------------------------
>>   2 files changed, 2 insertions(+), 33 deletions(-)
>>
>> diff --git a/sound/soc/intel/common/sst-acpi.h 
>> b/sound/soc/intel/common/sst-acpi.h
>> index afe9b87b8bd5..d7facac83b7e 100644
>> --- a/sound/soc/intel/common/sst-acpi.h
>> +++ b/sound/soc/intel/common/sst-acpi.h
>> @@ -43,9 +43,6 @@ static inline bool 
>> sst_acpi_find_package_from_hid(const u8 hid[ACPI_ID_LEN],
>>   /* acpi match */
>>   struct sst_acpi_mach *sst_acpi_find_machine(struct sst_acpi_mach 
>> *machines);
>>   -/* acpi check hid */
>> -bool sst_acpi_check_hid(const u8 hid[ACPI_ID_LEN]);
>> -
>>   /* Descriptor for SST ASoC machine driver */
>>   struct sst_acpi_mach {
>>       /* ACPI ID for the matching machine driver. Audio codec for 
>> instance */
>> diff --git a/sound/soc/intel/common/sst-match-acpi.c 
>> b/sound/soc/intel/common/sst-match-acpi.c
>> index 56d26f36a3cb..f8e1b92de79a 100644
>> --- a/sound/soc/intel/common/sst-match-acpi.c
>> +++ b/sound/soc/intel/common/sst-match-acpi.c
>> @@ -49,40 +49,12 @@ const char *sst_acpi_find_name_from_hid(const u8 
>> hid[ACPI_ID_LEN])
>>   }
>>   EXPORT_SYMBOL_GPL(sst_acpi_find_name_from_hid);
>>   -static acpi_status sst_acpi_mach_match(acpi_handle handle, u32 level,
>> -                       void *context, void **ret)
>> -{
>> -    unsigned long long sta;
>> -    acpi_status status;
>> -
>> -    *(bool *)context = true;
>> -    status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
>> -    if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
>> -        *(bool *)context = false;
>> -
>> -    return AE_OK;
>> -}
>> -
>> -bool sst_acpi_check_hid(const u8 hid[ACPI_ID_LEN])
>> -{
>> -    acpi_status status;
>> -    bool found = false;
>> -
>> -    status = acpi_get_devices(hid, sst_acpi_mach_match, &found, NULL);
>> -
>> -    if (ACPI_FAILURE(status))
>> -        return false;
>> -
>> -    return found;
>> -}
>> -EXPORT_SYMBOL_GPL(sst_acpi_check_hid);
>> -
>>   struct sst_acpi_mach *sst_acpi_find_machine(struct sst_acpi_mach 
>> *machines)
>>   {
>>       struct sst_acpi_mach *mach;
>>         for (mach = machines; mach->id[0]; mach++) {
>> -        if (sst_acpi_check_hid(mach->id) == true) {
>> +        if (acpi_dev_present(mach->id, NULL, -1) == true) {
>>               if (mach->machine_quirk == NULL)
>>                   return mach;
>>   @@ -161,7 +133,7 @@ struct sst_acpi_mach *sst_acpi_codec_list(void 
>> *arg)
>>           return mach;
>>         for (i = 0; i < codec_list->num_codecs; i++) {
>> -        if (sst_acpi_check_hid(codec_list->codecs[i]) != true)
>> +        if (acpi_dev_present(codec_list->codecs[i], NULL, -1) != true)
>>               return NULL;
>>       }
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@...a-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Powered by blists - more mailing lists