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 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?


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