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

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

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.

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