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

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2019-08-22 22:44, Pierre-Louis Bossart wrote:
> 
> 
> On 8/22/19 2:02 PM, Cezary Rojewski wrote:
>> 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.
> 
> what happens in sst-acpi.c stays in sst-acpi.c
> I don't get how you retrieve the pdata in the machine driver from 
> *another* driver. Different devices, different platform data.

DAI is tied with platform device called "haswell-pcm-audio" whereas 
machine board is represented by "broadwell-audio" platform deivce. Which 
part is still unclear?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel