Re: [PATCH] ASoC: Intel: add bytcr-wm5102 machine driver

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

On 06/05/2017 09:25 AM, Paulo Sergio wrote:
> Hi Pierre,how are you?
>
>>> +static int is_byt_cr(struct device *dev, struct sst_acpi_mach *mach, bool
>>> *bytcr)
>>>    {
>>>          int status = 0;
>>>    +     /* Lenovo Yoga2 exception - acpi_ipc_irq_index is the 1st index,
>>> +          but iosf_mbi_read (bios_status) returns 0 for bits 26:27 */
>>> +       if (!strcmp(mach->drv_name, "bytcr_wm5102")) {
>>> +               *bytcr = true;
>>> +               return status;
>>> +       }
>>
>> well no, this isn't the right way to do this. quirks need to be based on
>> real hardware information (e.g. DMI) and not a driver name.
> Thanks for the review. Could I use dmi_match function inside is_byt_cr?

Yes, however it's the first time I see a quirk based on 
DMI_CHASSIS_VERSION. we use PRODUCT or BOARD usually.

>
> git diff HEAD sound/soc/intel/atom/sst/sst_acpi.c
> diff --git a/sound/soc/intel/atom/sst/sst_acpi.c
> b/sound/soc/intel/atom/sst/sst_acpi.c
> index dd250b8..a99d5a5 100644
> --- a/sound/soc/intel/atom/sst/sst_acpi.c
> +++ b/sound/soc/intel/atom/sst/sst_acpi.c
> @@ -244,6 +244,13 @@ static int is_byt_cr(struct device *dev, bool *bytcr)
>   {
>          int status = 0;
>
> +       /* Lenovo Yoga2 exception - acpi_ipc_irq_index is the 1st index,
> +          but iosf_mbi_read (bios_status) returns 0 for bits 26:27 */
> +       if (dmi_match(DMI_SYS_VENDOR, "LENOVO") &&
> dmi_match(DMI_CHASSIS_VERSION, "1051F")) {
> +               *bytcr = true;
> +               return status;
> +       }
> +
>          if (IS_ENABLED(CONFIG_IOSF_MBI)) {
> ...
>
>
>> The rest looks more of less ok, it's not clear to me if all the quirks are
>> needed and make sense.
> Inside machine driver code, I can confirm these as necessary:
>
> - SSP0 usage
> - 25Mhz Clock
>
> About the other quirks: To be honest, as I started using bycr_rt5640
> as base, I avoided touching other parts of the code.
>
> I'll understand if this is not adequated to be pushed upstream.
> Besides, the codec side changes (arizona/wm5102 - not sent with this
> patch)
> are not ready to be pushed due some hardcoding & an issue with
> regulator part (arizona-code will only detected codec chip if
> arizona-ldo1 is marked as built-in; as a module it is not working)
>
> Regards
> Paulo Sérgio Travaglia (Pstglia)

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel