Re: [RFC PATCH 4/8] ASoC: Intel: move all ACPI match tables to common module

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

On 9/27/17 4:45 AM, Vinod Koul wrote:
> On Tue, Sep 26, 2017 at 02:13:23PM -0500, Pierre-Louis Bossart wrote:
>> On 9/25/17 11:23 PM, Vinod Koul wrote:
>>> On Fri, Sep 08, 2017 at 03:56:58PM -0500, Pierre-Louis Bossart wrote:
>>>> First step of cleaning, move all tables to soc-acpi-intel-match module
>>>>
>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>> ---
>>>>   include/sound/soc-acpi-intel-match.h          |  32 +++
>>>>   sound/soc/intel/Kconfig                       |  24 +-
>>>>   sound/soc/intel/Makefile                      |   2 +-
>>>>   sound/soc/intel/atom/sst/sst_acpi.c           | 311 ++-----------------------
>>>>   sound/soc/intel/common/Makefile               |   2 +
>>>>   sound/soc/intel/common/soc-acpi-intel-match.c | 323 ++++++++++++++++++++++++++
>>>>   sound/soc/intel/common/sst-acpi.c             |  44 +---
>>>>   7 files changed, 395 insertions(+), 343 deletions(-)
>>>>   create mode 100644 include/sound/soc-acpi-intel-match.h
>>>>   create mode 100644 sound/soc/intel/common/soc-acpi-intel-match.c
>>>>
>>>> diff --git a/include/sound/soc-acpi-intel-match.h b/include/sound/soc-acpi-intel-match.h
>>>> new file mode 100644
>>>> index 0000000..1a9191c
>>>> --- /dev/null
>>>> +++ b/include/sound/soc-acpi-intel-match.h
>>>> @@ -0,0 +1,32 @@
>>>> +
>>>> +/*
>>>> + * Copyright (C) 2017, Intel Corporation. All rights reserved.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU General Public License version
>>>> + * 2 as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + *
>>>> + */
>>>> +
>>>> +#ifndef __LINUX_SND_SOC_ACPI_INTEL_MATCH_H
>>>> +#define __LINUX_SND_SOC_ACPI_INTEL_MATCH_H
>>>
>>> do we need LINUX on this one, it already too long :)
>>
>> I checked and all include/sound/soc* files have this LINUX_SND_SOC prefix. I
>> don't mind removing it but then someone will object it's not consistent.
> 
> I didnt realize we had that in soc files :)
> 
> $ grep __LINUX include/sound/* |wc -l
> 56
> $ grep -L __LINUX include/sound/* |wc -l
> 118
> 
> Take your pick :D
> 
>>
>>>
>>>> +
>>>> +#include <linux/stddef.h>
>>>> +#include <linux/acpi.h>
>>>> +
>>>> +/*
>>>> + * these tables are not constants, some fields can be used for
>>>> + * pdata or machine ops
>>>> + */
>>>> +extern struct snd_soc_acpi_mach snd_soc_acpi_intel_haswell_machines[];
>>>> +extern struct snd_soc_acpi_mach snd_soc_acpi_intel_broadwell_machines[];
>>>> +extern struct snd_soc_acpi_mach snd_soc_acpi_intel_baytrail_legacy_machines[];
>>>> +extern struct snd_soc_acpi_mach snd_soc_acpi_intel_baytrail_machines[];
>>>> +extern struct snd_soc_acpi_mach snd_soc_acpi_intel_cherrytrail_machines[];
>>>
>>> so the header is just for externs, not a pretty thing, can we avoid these
>>> somehow. Do they need to be in common file, why not keep then in respective
>>> byt/hsw file.
>>
>> Because they will be shared between drivers, that's the whole point.
>> I can't put a common table in either of sound/soc/sof or
>> sound/soc/intel/atom. I didn't find a better solution than a module with
>> just tables + matching functions in it.
> 
> yes but shared between byt family or hsw family, maybe a common byt-tables.c
> hsw-tables.c and we can move skl ones out to skl-tables.c

oh, if you are talking about splitting the tables in different files yes 
this is no issue. I thought you objected to the declaration of the 
tables themselves.

> 
>>
>>>
>>>
>>>> +config SND_SOC_INTEL_COMMON
>>>> +	tristate
>>>> +
>>>>   config SND_SOC_INTEL_SST
>>>>   	tristate
>>>> +	select SND_SOC_INTEL_COMMON
>>>>   	select SND_SOC_INTEL_SST_ACPI if ACPI
>>>> -	select SND_SOC_INTEL_SST_MATCH if ACPI
>>>> +	select SND_SOC_ACPI_INTEL_MATCH if ACPI
>>>> -config SND_SOC_INTEL_SST_MATCH
>>>> +config SND_SOC_ACPI_INTEL_MATCH
>>>>   	tristate
>>>>   	select SND_SOC_ACPI if ACPI
>>>> @@ -145,7 +149,7 @@ config SND_SOC_INTEL_BYTCR_RT5640_MACH
>>>>   	select SND_SOC_RT5640
>>>>   	select SND_SST_ATOM_HIFI2_PLATFORM
>>>>   	select SND_SST_IPC_ACPI
>>>> -	select SND_SOC_INTEL_SST_MATCH if ACPI
>>>> +	select SND_SOC_ACPI_INTEL_MATCH if ACPI
>>>
>>> why do you need this change, SND_SOC_INTEL_SST selects
>>> SND_SOC_ACPI_INTEL_MATCH, so we should select top symbol which is
>>> SND_SOC_INTEL_SST
>>
>> The idea is also to share machine drivers between SOF and closed-source
>> based platform drivers, so if we keep the semantic of INTEL_SST as is then
>> it needs to go away from the machine parts.
> 
> ok
>