Am 23.06.2017 um 19:05 schrieb Alex Deucher:
> On Fri, Jun 23, 2017 at 12:43 PM, Christian König
> <deathsimple at vodafone.de> wrote:
>> Am 23.06.2017 um 18:34 schrieb Alex Deucher:
>>> From: Vijendar Mukunda <Vijendar.Mukunda at amd.com>
>>>
>>> asic_type information is passed to ACP DMA Driver as platform data.
>>> We need this to determine whether the asic is Carrizo (CZ) or
>>> Stoney (ST) in the acp sound driver.
>>>
>>> Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
>>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda at amd.com>
>>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 4 ++++
>>> sound/soc/amd/acp-pcm-dma.c | 8 ++------
>>> sound/soc/amd/acp.h | 7 +++++++
>>> 3 files changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> index 06879d1..7a2a765 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> @@ -262,6 +262,7 @@ static int acp_hw_init(void *handle)
>>> uint64_t acp_base;
>>> struct device *dev;
>>> struct i2s_platform_data *i2s_pdata;
>>> + u32 asic_type;
>>> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>> @@ -271,6 +272,7 @@ static int acp_hw_init(void *handle)
>>> if (!ip_block)
>>> return -EINVAL;
>>> + asic_type = adev->asic_type;
>>> r = amd_acp_hw_init(adev->acp.cgs_device,
>>> ip_block->version->major,
>>> ip_block->version->minor);
>>> /* -ENODEV means board uses AZ rather than ACP */
>>> @@ -355,6 +357,8 @@ static int acp_hw_init(void *handle)
>>> adev->acp.acp_cell[0].name = "acp_audio_dma";
>>> adev->acp.acp_cell[0].num_resources = 4;
>>> adev->acp.acp_cell[0].resources = &adev->acp.acp_res[0];
>>> + adev->acp.acp_cell[0].platform_data = &asic_type;
>>> + adev->acp.acp_cell[0].pdata_size = sizeof(asic_type);
>>
>> Have the painkillers jellyfied my brain or are you setting a pointer in the
>> amdgpu device structure to some variable on the stack?
>>
>> If it's just for initialization then that's probably ok, but I would reset
>> the pointer at the end of the function.
>>
>> Otherwise somebody might have the idea to dereference it later on and that
>> can only lead to trouble.
> I think it's ok because the hotplug of the audio device is triggered
> from this function, so the audio device should be probed by the time
> this variable goes out of scope. That said, it would probably be
> better to use the asic_type from the GPU driver instance directly
> rather than a stack variable.
Yeah, agree. But keeping a stale reference in to a stack variable in the
driver struct is still ugly like hell.
At bare minimum set this to NULL at the end of the function, or even
better use a intptr_t to encode the asic type into the pointer.
Christian.
>
> Alex
>
>
>> Christian.
>>
>>
>>> adev->acp.acp_cell[1].name = "designware-i2s";
>>> adev->acp.acp_cell[1].num_resources = 1;
>>> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
>>> index 08b1399..dcbf997 100644
>>> --- a/sound/soc/amd/acp-pcm-dma.c
>>> +++ b/sound/soc/amd/acp-pcm-dma.c
>>> @@ -73,12 +73,6 @@ static const struct snd_pcm_hardware
>>> acp_pcm_hardware_capture = {
>>> .periods_max = CAPTURE_MAX_NUM_PERIODS,
>>> };
>>> -struct audio_drv_data {
>>> - struct snd_pcm_substream *play_stream;
>>> - struct snd_pcm_substream *capture_stream;
>>> - void __iomem *acp_mmio;
>>> -};
>>> -
>>> static u32 acp_reg_read(void __iomem *acp_mmio, u32 reg)
>>> {
>>> return readl(acp_mmio + (reg * 4));
>>> @@ -916,6 +910,7 @@ static int acp_audio_probe(struct platform_device
>>> *pdev)
>>> int status;
>>> struct audio_drv_data *audio_drv_data;
>>> struct resource *res;
>>> + const u32 *pdata = pdev->dev.platform_data;
>>> audio_drv_data = devm_kzalloc(&pdev->dev, sizeof(struct
>>> audio_drv_data),
>>> GFP_KERNEL);
>>> @@ -932,6 +927,7 @@ static int acp_audio_probe(struct platform_device
>>> *pdev)
>>> audio_drv_data->play_stream = NULL;
>>> audio_drv_data->capture_stream = NULL;
>>> + audio_drv_data->asic_type = *pdata;
>>> res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>> if (!res) {
>>> diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
>>> index 330832e..28cf914 100644
>>> --- a/sound/soc/amd/acp.h
>>> +++ b/sound/soc/amd/acp.h
>>> @@ -84,6 +84,13 @@ struct audio_substream_data {
>>> void __iomem *acp_mmio;
>>> };
>>> +struct audio_drv_data {
>>> + struct snd_pcm_substream *play_stream;
>>> + struct snd_pcm_substream *capture_stream;
>>> + void __iomem *acp_mmio;
>>> + u32 asic_type;
>>> +};
>>> +
>>> enum {
>>> ACP_TILE_P1 = 0,
>>> ACP_TILE_P2,
>>
>>
