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?