Re: [PATCH 1/4] ASoC: Intel: Haswell: Adjust machine device private context

From: Cezary Rojewski <cezary.rojewski@intel.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	alsa-devel@alsa-project.org
Cc: broonie@kernel.org, tiwai@suse.com, lgirdwood@gmail.com
Subject: Re: [PATCH 1/4] ASoC: Intel: Haswell: Adjust machine device private context
Date: Thu, 22 Aug 2019 21:02:32 +0200	[thread overview]
Message-ID: <3ff82f6b-647f-5f9c-09c7-be42a00c8bd4@intel.com> (raw)
In-Reply-To: <90925296-5cb4-fa87-9c35-a7008f5e8df5@linux.intel.com>

On 2019-08-22 20:44, Pierre-Louis Bossart wrote:
> 
> 
> On 8/22/19 12:14 PM, Cezary Rojewski wrote:
>> On 2019-08-22 18:42, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 8/22/19 11:05 AM, Cezary Rojewski wrote:
>>>> On 2019-08-22 17:58, Pierre-Louis Bossart wrote:
>>>>>
>>>>>
>>>>> On 8/22/19 6:36 AM, Cezary Rojewski wrote:
>>>>>> Apart from Haswell machines, all other devices have their private 
>>>>>> data
>>>>>> set to snd_soc_acpi_mach instance.
>>>>>>
>>>>>> Changes for HSW/ BDW boards introduced with series:
>>>>>> https://patchwork.kernel.org/cover/10782035/
>>>>>>
>>>>>> added support for dai_link platform_name adjustments within card 
>>>>>> probe
>>>>>> routines. These take for granted private_data points to
>>>>>> snd_soc_acpi_mach whereas for Haswell, it's sst_pdata instead. Change
>>>>>> private context of platform_device - representing machine board - to
>>>>>> address this.
>>>>>>
>>>>>> Fixes: e87055d732e3 ("ASoC: Intel: haswell: platform name fixup 
>>>>>> support")
>>>>>> Fixes: 7e40ddcf974a ("ASoC: Intel: bdw-rt5677: platform name fixup 
>>>>>> support")
>>>>>> Fixes: 2d067b2807f9 ("ASoC: Intel: broadwell: platform name fixup 
>>>>>> support")
>>>>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>>>>> ---
>>>>>>   sound/soc/intel/common/sst-acpi.c | 3 ++-
>>>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/sound/soc/intel/common/sst-acpi.c 
>>>>>> b/sound/soc/intel/common/sst-acpi.c
>>>>>> index 15f2b27e643f..c34f628c7987 100644
>>>>>> --- a/sound/soc/intel/common/sst-acpi.c
>>>>>> +++ b/sound/soc/intel/common/sst-acpi.c
>>>>>> @@ -109,11 +109,12 @@ int sst_acpi_probe(struct platform_device 
>>>>>> *pdev)
>>>>>>       }
>>>>>>       platform_set_drvdata(pdev, sst_acpi);
>>>>>> +    mach->pdata = sst_pdata;
>>>>>>       /* register machine driver */
>>>>>>       sst_acpi->pdev_mach =
>>>>>>           platform_device_register_data(dev, mach->drv_name, -1,
>>>>>> -                          sst_pdata, sizeof(*sst_pdata));
>>>>>> +                          mach, sizeof(*mach));
>>>>>
>>>>> I now agree that the code I added is incorrect and probably 
>>>>> accesses memory offsets that aren't right. I have absolutely no 
>>>>> idea why I added this comment that 'legacy does not pass 
>>>>> parameters' when it most definitively does. Good catch on your side.
>>>>>
>>>>> That said, doesn't the proposed fix introduce another issue?
>>>>>
>>>>> In the machine drivers, you still get pdata directly, so aren't you 
>>>>> missing an indirection to get back to pdata from mach?
>>>>>
>>>>> static int bdw_rt5677_rtd_init(struct snd_soc_pcm_runtime *rtd)
>>>>> {
>>>>>      struct snd_soc_component *component = 
>>>>> snd_soc_rtdcom_lookup(rtd, DRV_NAME);
>>>>>      struct sst_pdata *pdata = dev_get_platdata(component->dev);
>>>>>      struct sst_hsw *broadwell = pdata->dsp;
>>>>>
>>>>> <<< so here you took the wrong pointer, no?
>>>>
>>>> Both Baytrail and Haswell are enumerated in a bit different fashion 
>>>> than SKL equivalents.
>>>>
>>>> There is an in-place registration for machine device - whose 
>>>> private_data gets used in machine probe - and pcm device which 
>>>> happens on firmware load callback 
>>>> (/sound/soc/intel/common/sst-acpi:63). _rtd_init makes use of the 
>>>> latter of two.
>>>
>>> I don't get your explanations. can you elaborate on what this does 
>>> now that pdata is no longer passed as an argument to the machine driver:
>>>
>>> struct snd_soc_component *component = snd_soc_rtdcom_lookup(rtd, 
>>> DRV_NAME);
>>> struct sst_pdata *pdata = dev_get_platdata(component->dev);
>>>
>>> the 'component' here is not the PCM one, is it?
>>>
>>>
>>
>> Sure thing.
>>
>> Code:
>>      /* register machine driver */
>>      sst_acpi->pdev_mach =
>>          platform_device_register_data(dev, mach->drv_name, -1,
>>                            sst_pdata, sizeof(*sst_pdata));
>>
>> Found in sst_acpi_probe (/sound/soc/intel/common/sst-acpi.c:145) 
>> generates new platform_device - which represents machine board - with 
>> its private data set to pointer to instance of struct sst_pdata type. 
>> This data gets used on machine board probe, e.g.: 
>> broadwell_audio_probe (/sound/soc/intel/boards/broadwell.c:270).
>> Involved platform is called: broadwell-audio. Requested private data 
>> type by broadwell_audio_probe: struct snd_soc_acpi_mach *. MISMATCH.
>>
>>
>> Code:
>>
>>      /* register PCM and DAI driver */
>>      sst_acpi->pdev_pcm =
>>          platform_device_register_data(dev, desc->drv_name, -1,
>>                            sst_pdata, sizeof(*sst_pdata));
>>
>> Found in sst_acpi_fw_cb (/sound/soc/intel/common/sst_acpi_fw_cb:47) 
>> generates new platform_device - which represents Haswell PCM, you may 
>> treat it as Skylake equivalent - with its private data set to pointer 
>> to instance of struct sst_pdata type. This data gets used on dai .init 
>> - broadwell_rtd_init - invocation when card is instantiated by ASoC 
>> code. As you can see on (/sound/soc/intel/boards/broadwell.c:162), 
>> platform tied with it is: haswell-pcm-audio. Requested private data 
>> type by broadwell_rtd_init - struct sst_pdata *. MATCH.
> 
> 
> the machine drivers uses snd_soc_rtdcom_lookup(rtd, DRV_NAME);
> 
> How is DRV_NAME connected to haswell-pcm-audio?
> 
> I must be missing something in your logic.
> 

Please checkout sst-acpi.c file and see declaration of legacy platform 
descriptors. See the names of PCM devices (platform devices) being declared.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel