diff mbox series

[v3,3/3] drm/mediatek: dp: Add the audio divider to mtk_dp_data struct

Message ID 20230720082604.18618-4-shuijing.li@mediatek.com
State New
Headers
Series Add compatible to increase MT8188 audio control |

Commit Message

Shuijing Li July 20, 2023, 8:26 a.m. UTC 
  Due to the difference of HW, different dividers need to be set.

Signed-off-by: Shuijing Li <shuijing.li@mediatek.com>
Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
---
Changes in v3:
Separate these two things into two different patches.
per suggestion from the previous thread:
https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.camel@mediatek.com/
Changes in v2:
- change the variables' name to be more descriptive
- add a comment that describes the function of mtk_dp_audio_sample_arrange
- reduce indentation by doing the inverse check
- add a definition of some bits
- add support for mediatek, mt8188-edp-tx
per suggestion from the previous thread:
https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7babe9c@collabora.com/
---
 drivers/gpu/drm/mediatek/mtk_dp.c     | 7 ++++++-
 drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

AngeloGioacchino Del Regno July 20, 2023, 9:28 a.m. UTC | #1 
Il 20/07/23 10:26, Shuijing Li ha scritto:
> Due to the difference of HW, different dividers need to be set.
> 
> Signed-off-by: Shuijing Li <shuijing.li@mediatek.com>
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Alexandre Mergnat July 20, 2023, 10:14 a.m. UTC | #2 
On 20/07/2023 10:26, Shuijing Li wrote:
> Due to the difference of HW, different dividers need to be set.
> 
> Signed-off-by: Shuijing Li <shuijing.li@mediatek.com>
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> ---
> Changes in v3:
> Separate these two things into two different patches.
> per suggestion from the previous thread:
> https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.camel@mediatek.com/
> Changes in v2:
> - change the variables' name to be more descriptive
> - add a comment that describes the function of mtk_dp_audio_sample_arrange
> - reduce indentation by doing the inverse check
> - add a definition of some bits
> - add support for mediatek, mt8188-edp-tx
> per suggestion from the previous thread:
> https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7babe9c@collabora.com/
> ---
>   drivers/gpu/drm/mediatek/mtk_dp.c     | 7 ++++++-
>   drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 +
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index d8cda83d6fef..8e1a13ab2ba2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -140,6 +140,7 @@ struct mtk_dp_data {
>   	const struct mtk_dp_efuse_fmt *efuse_fmt;
>   	bool audio_supported;
>   	bool audio_pkt_in_hblank_area;
> +	u16 audio_m_div2_bit;
>   };
>   
>   static const struct mtk_dp_efuse_fmt mt8195_edp_efuse_fmt[MTK_DP_CAL_MAX] = {
> @@ -648,7 +649,7 @@ static void mtk_dp_audio_sdp_asp_set_channels(struct mtk_dp *mtk_dp,
>   static void mtk_dp_audio_set_divider(struct mtk_dp *mtk_dp)
>   {
>   	mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_30BC,
> -			   AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
> +			   mtk_dp->data->audio_m_div2_bit,
>   			   AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MASK);
>   }
>   
> @@ -2636,6 +2637,7 @@ static const struct mtk_dp_data mt8188_edp_data = {
>   	.efuse_fmt = mt8195_edp_efuse_fmt,
>   	.audio_supported = false,
>   	.audio_pkt_in_hblank_area = false,
> +	.audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>   };
>   
>   static const struct mtk_dp_data mt8188_dp_data = {
> @@ -2644,6 +2646,7 @@ static const struct mtk_dp_data mt8188_dp_data = {
>   	.efuse_fmt = mt8195_dp_efuse_fmt,
>   	.audio_supported = true,
>   	.audio_pkt_in_hblank_area = true,
> +	.audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>   };
>   
>   static const struct mtk_dp_data mt8195_edp_data = {
> @@ -2652,6 +2655,7 @@ static const struct mtk_dp_data mt8195_edp_data = {
>   	.efuse_fmt = mt8195_edp_efuse_fmt,
>   	.audio_supported = false,
>   	.audio_pkt_in_hblank_area = false,
> +	.audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>   };
>   
>   static const struct mtk_dp_data mt8195_dp_data = {
> @@ -2660,6 +2664,7 @@ static const struct mtk_dp_data mt8195_dp_data = {
>   	.efuse_fmt = mt8195_dp_efuse_fmt,
>   	.audio_supported = true,
>   	.audio_pkt_in_hblank_area = false,
> +	.audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>   };
>   
>   static const struct of_device_id mtk_dp_of_match[] = {
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> index f38d6ff12afe..6d7f0405867e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> @@ -162,6 +162,7 @@
>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2	(1 << 8)
>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4	(2 << 8)
>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8	(3 << 8)
> +#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2	(4 << 8)

IMO, it's a bit weird to have SoC specific define in the generic header.
Are you sure this bit is only available for MT8188 ?

>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2	(5 << 8)
>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4	(6 << 8)
>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8	(7 << 8)

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
AngeloGioacchino Del Regno July 20, 2023, 11:54 a.m. UTC | #3 
Il 20/07/23 12:14, Alexandre Mergnat ha scritto:
> 
> 
> On 20/07/2023 10:26, Shuijing Li wrote:
>> Due to the difference of HW, different dividers need to be set.
>>
>> Signed-off-by: Shuijing Li <shuijing.li@mediatek.com>
>> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
>> ---
>> Changes in v3:
>> Separate these two things into two different patches.
>> per suggestion from the previous thread:
>> https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.camel@mediatek.com/
>> Changes in v2:
>> - change the variables' name to be more descriptive
>> - add a comment that describes the function of mtk_dp_audio_sample_arrange
>> - reduce indentation by doing the inverse check
>> - add a definition of some bits
>> - add support for mediatek, mt8188-edp-tx
>> per suggestion from the previous thread:
>> https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7babe9c@collabora.com/
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dp.c     | 7 ++++++-
>>   drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 +
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
>> index d8cda83d6fef..8e1a13ab2ba2 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>> @@ -140,6 +140,7 @@ struct mtk_dp_data {
>>       const struct mtk_dp_efuse_fmt *efuse_fmt;
>>       bool audio_supported;
>>       bool audio_pkt_in_hblank_area;
>> +    u16 audio_m_div2_bit;
>>   };
>>   static const struct mtk_dp_efuse_fmt mt8195_edp_efuse_fmt[MTK_DP_CAL_MAX] = {
>> @@ -648,7 +649,7 @@ static void mtk_dp_audio_sdp_asp_set_channels(struct mtk_dp 
>> *mtk_dp,
>>   static void mtk_dp_audio_set_divider(struct mtk_dp *mtk_dp)
>>   {
>>       mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_30BC,
>> -               AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>> +               mtk_dp->data->audio_m_div2_bit,
>>                  AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MASK);
>>   }
>> @@ -2636,6 +2637,7 @@ static const struct mtk_dp_data mt8188_edp_data = {
>>       .efuse_fmt = mt8195_edp_efuse_fmt,
>>       .audio_supported = false,
>>       .audio_pkt_in_hblank_area = false,
>> +    .audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>>   };
>>   static const struct mtk_dp_data mt8188_dp_data = {
>> @@ -2644,6 +2646,7 @@ static const struct mtk_dp_data mt8188_dp_data = {
>>       .efuse_fmt = mt8195_dp_efuse_fmt,
>>       .audio_supported = true,
>>       .audio_pkt_in_hblank_area = true,
>> +    .audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>>   };
>>   static const struct mtk_dp_data mt8195_edp_data = {
>> @@ -2652,6 +2655,7 @@ static const struct mtk_dp_data mt8195_edp_data = {
>>       .efuse_fmt = mt8195_edp_efuse_fmt,
>>       .audio_supported = false,
>>       .audio_pkt_in_hblank_area = false,
>> +    .audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>>   };
>>   static const struct mtk_dp_data mt8195_dp_data = {
>> @@ -2660,6 +2664,7 @@ static const struct mtk_dp_data mt8195_dp_data = {
>>       .efuse_fmt = mt8195_dp_efuse_fmt,
>>       .audio_supported = true,
>>       .audio_pkt_in_hblank_area = false,
>> +    .audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>>   };
>>   static const struct of_device_id mtk_dp_of_match[] = {
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h 
>> b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>> index f38d6ff12afe..6d7f0405867e 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>> +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>> @@ -162,6 +162,7 @@
>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2    (1 << 8)
>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4    (2 << 8)
>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8    (3 << 8)
>> +#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2    (4 << 8)
> 
> IMO, it's a bit weird to have SoC specific define in the generic header.
> Are you sure this bit is only available for MT8188 ?
> 

Eh, the P0_DIV2 bit is 5<<8 for MT8195, while for 8188 it's 4<<8, clearly :-)

>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2    (5 << 8)
>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4    (6 << 8)
>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8    (7 << 8)
> 
> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
>
Alexandre Mergnat July 20, 2023, 12:07 p.m. UTC | #4 
On 20/07/2023 13:54, AngeloGioacchino Del Regno wrote:
> Il 20/07/23 12:14, Alexandre Mergnat ha scritto:
>>
>>
>> On 20/07/2023 10:26, Shuijing Li wrote:
>>> Due to the difference of HW, different dividers need to be set.
>>>
>>> Signed-off-by: Shuijing Li <shuijing.li@mediatek.com>
>>> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
>>> ---
>>> Changes in v3:
>>> Separate these two things into two different patches.
>>> per suggestion from the previous thread:
>>> https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.camel@mediatek.com/
>>> Changes in v2:
>>> - change the variables' name to be more descriptive
>>> - add a comment that describes the function of 
>>> mtk_dp_audio_sample_arrange
>>> - reduce indentation by doing the inverse check
>>> - add a definition of some bits
>>> - add support for mediatek, mt8188-edp-tx
>>> per suggestion from the previous thread:
>>> https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7babe9c@collabora.com/
>>> ---
>>>   drivers/gpu/drm/mediatek/mtk_dp.c     | 7 ++++++-
>>>   drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 +
>>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>>
...
>>> b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>> index f38d6ff12afe..6d7f0405867e 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>> +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>> @@ -162,6 +162,7 @@
>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2    (1 << 8)
>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4    (2 << 8)
>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8    (3 << 8)
>>> +#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2    (4 << 8)
>>
>> IMO, it's a bit weird to have SoC specific define in the generic header.
>> Are you sure this bit is only available for MT8188 ?
>>
> 
> Eh, the P0_DIV2 bit is 5<<8 for MT8195, while for 8188 it's 4<<8, 
> clearly :-)
> 

Ok then, to avoid this kind of issue for other SoCs in the future, is 
that make sense for you to do a SoC specific header file beside the 
generic one?

>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2    (5 << 8)
>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4    (6 << 8)
>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8    (7 << 8)
>>
>> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
>>
> 
>
AngeloGioacchino Del Regno July 20, 2023, 12:08 p.m. UTC | #5 
Il 20/07/23 14:07, Alexandre Mergnat ha scritto:
> 
> 
> On 20/07/2023 13:54, AngeloGioacchino Del Regno wrote:
>> Il 20/07/23 12:14, Alexandre Mergnat ha scritto:
>>>
>>>
>>> On 20/07/2023 10:26, Shuijing Li wrote:
>>>> Due to the difference of HW, different dividers need to be set.
>>>>
>>>> Signed-off-by: Shuijing Li <shuijing.li@mediatek.com>
>>>> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
>>>> ---
>>>> Changes in v3:
>>>> Separate these two things into two different patches.
>>>> per suggestion from the previous thread:
>>>> https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.camel@mediatek.com/
>>>> Changes in v2:
>>>> - change the variables' name to be more descriptive
>>>> - add a comment that describes the function of mtk_dp_audio_sample_arrange
>>>> - reduce indentation by doing the inverse check
>>>> - add a definition of some bits
>>>> - add support for mediatek, mt8188-edp-tx
>>>> per suggestion from the previous thread:
>>>> https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7babe9c@collabora.com/
>>>> ---
>>>>   drivers/gpu/drm/mediatek/mtk_dp.c     | 7 ++++++-
>>>>   drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 +
>>>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>>>
> ...
>>>> b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>>> index f38d6ff12afe..6d7f0405867e 100644
>>>> --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>>> +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>>> @@ -162,6 +162,7 @@
>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2    (1 << 8)
>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4    (2 << 8)
>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8    (3 << 8)
>>>> +#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2    (4 << 8)
>>>
>>> IMO, it's a bit weird to have SoC specific define in the generic header.
>>> Are you sure this bit is only available for MT8188 ?
>>>
>>
>> Eh, the P0_DIV2 bit is 5<<8 for MT8195, while for 8188 it's 4<<8, clearly :-)
>>
> 
> Ok then, to avoid this kind of issue for other SoCs in the future, is that make 
> sense for you to do a SoC specific header file beside the generic one?
> 

For just one definition? That's a bit overkill :-)

>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2    (5 << 8)
>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4    (6 << 8)
>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8    (7 << 8)
>>>
>>> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>
>>
>>
>
Alexandre Mergnat July 20, 2023, 12:29 p.m. UTC | #6 
On 20/07/2023 14:08, AngeloGioacchino Del Regno wrote:
> Il 20/07/23 14:07, Alexandre Mergnat ha scritto:
>>
>>
>> On 20/07/2023 13:54, AngeloGioacchino Del Regno wrote:
>>> Il 20/07/23 12:14, Alexandre Mergnat ha scritto:
>>>>
>>>>
>>>> On 20/07/2023 10:26, Shuijing Li wrote:
>>>>> Due to the difference of HW, different dividers need to be set.
>>>>>
>>>>> Signed-off-by: Shuijing Li <shuijing.li@mediatek.com>
>>>>> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
>>>>> ---
>>>>> Changes in v3:
>>>>> Separate these two things into two different patches.
>>>>> per suggestion from the previous thread:
>>>>> https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.camel@mediatek.com/
>>>>> Changes in v2:
>>>>> - change the variables' name to be more descriptive
>>>>> - add a comment that describes the function of 
>>>>> mtk_dp_audio_sample_arrange
>>>>> - reduce indentation by doing the inverse check
>>>>> - add a definition of some bits
>>>>> - add support for mediatek, mt8188-edp-tx
>>>>> per suggestion from the previous thread:
>>>>> https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7babe9c@collabora.com/
>>>>> ---
>>>>>   drivers/gpu/drm/mediatek/mtk_dp.c     | 7 ++++++-
>>>>>   drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 +
>>>>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>>>>
>> ...
>>>>> b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>>>> index f38d6ff12afe..6d7f0405867e 100644
>>>>> --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>>>> +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>>>> @@ -162,6 +162,7 @@
>>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2    (1 << 8)
>>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4    (2 << 8)
>>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8    (3 << 8)
>>>>> +#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2    (4 << 8)
>>>>
>>>> IMO, it's a bit weird to have SoC specific define in the generic 
>>>> header.
>>>> Are you sure this bit is only available for MT8188 ?
>>>>
>>>
>>> Eh, the P0_DIV2 bit is 5<<8 for MT8195, while for 8188 it's 4<<8, 
>>> clearly :-)
>>>
>>
>> Ok then, to avoid this kind of issue for other SoCs in the future, is 
>> that make sense for you to do a SoC specific header file beside the 
>> generic one?
>>
> 
> For just one definition? That's a bit overkill :-)
> 

You're right, but we must start somewhere ^^, and show the proper way 
for future patches. Actually, I gave my Reviewed-by because it's only 
one definition. This will be fixed later (I hope).

>>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2    (5 << 8)
>>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4    (6 << 8)
>>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8    (7 << 8)
>>>>
>>>> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>>
>>>
>>>
>>
> 
>
AngeloGioacchino Del Regno July 20, 2023, 12:49 p.m. UTC | #7 
Il 20/07/23 14:29, Alexandre Mergnat ha scritto:
> 
> 
> On 20/07/2023 14:08, AngeloGioacchino Del Regno wrote:
>> Il 20/07/23 14:07, Alexandre Mergnat ha scritto:
>>>
>>>
>>> On 20/07/2023 13:54, AngeloGioacchino Del Regno wrote:
>>>> Il 20/07/23 12:14, Alexandre Mergnat ha scritto:
>>>>>
>>>>>
>>>>> On 20/07/2023 10:26, Shuijing Li wrote:
>>>>>> Due to the difference of HW, different dividers need to be set.
>>>>>>
>>>>>> Signed-off-by: Shuijing Li <shuijing.li@mediatek.com>
>>>>>> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
>>>>>> ---
>>>>>> Changes in v3:
>>>>>> Separate these two things into two different patches.
>>>>>> per suggestion from the previous thread:
>>>>>> https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.camel@mediatek.com/
>>>>>> Changes in v2:
>>>>>> - change the variables' name to be more descriptive
>>>>>> - add a comment that describes the function of mtk_dp_audio_sample_arrange
>>>>>> - reduce indentation by doing the inverse check
>>>>>> - add a definition of some bits
>>>>>> - add support for mediatek, mt8188-edp-tx
>>>>>> per suggestion from the previous thread:
>>>>>> https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7babe9c@collabora.com/
>>>>>> ---
>>>>>>   drivers/gpu/drm/mediatek/mtk_dp.c     | 7 ++++++-
>>>>>>   drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 +
>>>>>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>>>>>
>>> ...
>>>>>> b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>>>>> index f38d6ff12afe..6d7f0405867e 100644
>>>>>> --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>>>>> +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>>>>> @@ -162,6 +162,7 @@
>>>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2    (1 << 8)
>>>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4    (2 << 8)
>>>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8    (3 << 8)
>>>>>> +#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2    (4 << 8)
>>>>>
>>>>> IMO, it's a bit weird to have SoC specific define in the generic header.
>>>>> Are you sure this bit is only available for MT8188 ?
>>>>>
>>>>
>>>> Eh, the P0_DIV2 bit is 5<<8 for MT8195, while for 8188 it's 4<<8, clearly :-)
>>>>
>>>
>>> Ok then, to avoid this kind of issue for other SoCs in the future, is that make 
>>> sense for you to do a SoC specific header file beside the generic one?
>>>
>>
>> For just one definition? That's a bit overkill :-)
>>
> 
> You're right, but we must start somewhere ^^, and show the proper way for future 
> patches. Actually, I gave my Reviewed-by because it's only one definition. This 
> will be fixed later (I hope).
> 

I'm confident that *if* and when "a bunch" of SoC-specific definitions will
appear, those will be splitted in different headers. :-)

>>>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2    (5 << 8)
>>>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4    (6 << 8)
>>>>>>   #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8    (7 << 8)
>>>>>
>>>>> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>>>
>>>>
>>>>
>>>
>>
>>
>
CK Hu (胡俊光) July 26, 2023, 3:35 a.m. UTC | #8 
Hi, Shuijing:

On Thu, 2023-07-20 at 16:26 +0800, Shuijing Li wrote:
> Due to the difference of HW, different dividers need to be set.
> 
> Signed-off-by: Shuijing Li <shuijing.li@mediatek.com>
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> ---
> Changes in v3:
> Separate these two things into two different patches.
> per suggestion from the previous thread:
> 
https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.camel@mediatek.com/
> Changes in v2:
> - change the variables' name to be more descriptive
> - add a comment that describes the function of
> mtk_dp_audio_sample_arrange
> - reduce indentation by doing the inverse check
> - add a definition of some bits
> - add support for mediatek, mt8188-edp-tx
> per suggestion from the previous thread:
> 
https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7babe9c@collabora.com/
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c     | 7 ++++++-
>  drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c
> b/drivers/gpu/drm/mediatek/mtk_dp.c
> index d8cda83d6fef..8e1a13ab2ba2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -140,6 +140,7 @@ struct mtk_dp_data {
>  	const struct mtk_dp_efuse_fmt *efuse_fmt;
>  	bool audio_supported;
>  	bool audio_pkt_in_hblank_area;
> +	u16 audio_m_div2_bit;
>  };
>  
>  static const struct mtk_dp_efuse_fmt
> mt8195_edp_efuse_fmt[MTK_DP_CAL_MAX] = {
> @@ -648,7 +649,7 @@ static void
> mtk_dp_audio_sdp_asp_set_channels(struct mtk_dp *mtk_dp,
>  static void mtk_dp_audio_set_divider(struct mtk_dp *mtk_dp)
>  {
>  	mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_30BC,
> -			   AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
> +			   mtk_dp->data->audio_m_div2_bit,
>  			   AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MASK);
>  }
>  
> @@ -2636,6 +2637,7 @@ static const struct mtk_dp_data mt8188_edp_data
> = {
>  	.efuse_fmt = mt8195_edp_efuse_fmt,
>  	.audio_supported = false,
>  	.audio_pkt_in_hblank_area = false,
> +	.audio_m_div2_bit =
> MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>  };
>  
>  static const struct mtk_dp_data mt8188_dp_data = {
> @@ -2644,6 +2646,7 @@ static const struct mtk_dp_data mt8188_dp_data
> = {
>  	.efuse_fmt = mt8195_dp_efuse_fmt,
>  	.audio_supported = true,
>  	.audio_pkt_in_hblank_area = true,
> +	.audio_m_div2_bit =
> MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>  };
>  
>  static const struct mtk_dp_data mt8195_edp_data = {
> @@ -2652,6 +2655,7 @@ static const struct mtk_dp_data mt8195_edp_data
> = {
>  	.efuse_fmt = mt8195_edp_efuse_fmt,
>  	.audio_supported = false,
>  	.audio_pkt_in_hblank_area = false,
> +	.audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>  };
>  
>  static const struct mtk_dp_data mt8195_dp_data = {
> @@ -2660,6 +2664,7 @@ static const struct mtk_dp_data mt8195_dp_data
> = {
>  	.efuse_fmt = mt8195_dp_efuse_fmt,
>  	.audio_supported = true,
>  	.audio_pkt_in_hblank_area = false,
> +	.audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
>  };
>  
>  static const struct of_device_id mtk_dp_of_match[] = {
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> index f38d6ff12afe..6d7f0405867e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> @@ -162,6 +162,7 @@
>  #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2	(1 << 8)
>  #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4	(2 << 8)
>  #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8	(3 << 8)
> +#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2	(4 <<
> 8)

For AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_*, add prefix 'MT8195_'.
And list all configuration for MT8188.

Regards,
CK

>  #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2	(5 << 8)
>  #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4	(6 << 8)
>  #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8	(7 << 8)
diff mbox series

Patch

diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
index d8cda83d6fef..8e1a13ab2ba2 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -140,6 +140,7 @@  struct mtk_dp_data {
 	const struct mtk_dp_efuse_fmt *efuse_fmt;
 	bool audio_supported;
 	bool audio_pkt_in_hblank_area;
+	u16 audio_m_div2_bit;
 };
 
 static const struct mtk_dp_efuse_fmt mt8195_edp_efuse_fmt[MTK_DP_CAL_MAX] = {
@@ -648,7 +649,7 @@  static void mtk_dp_audio_sdp_asp_set_channels(struct mtk_dp *mtk_dp,
 static void mtk_dp_audio_set_divider(struct mtk_dp *mtk_dp)
 {
 	mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_30BC,
-			   AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
+			   mtk_dp->data->audio_m_div2_bit,
 			   AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MASK);
 }
 
@@ -2636,6 +2637,7 @@  static const struct mtk_dp_data mt8188_edp_data = {
 	.efuse_fmt = mt8195_edp_efuse_fmt,
 	.audio_supported = false,
 	.audio_pkt_in_hblank_area = false,
+	.audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
 };
 
 static const struct mtk_dp_data mt8188_dp_data = {
@@ -2644,6 +2646,7 @@  static const struct mtk_dp_data mt8188_dp_data = {
 	.efuse_fmt = mt8195_dp_efuse_fmt,
 	.audio_supported = true,
 	.audio_pkt_in_hblank_area = true,
+	.audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
 };
 
 static const struct mtk_dp_data mt8195_edp_data = {
@@ -2652,6 +2655,7 @@  static const struct mtk_dp_data mt8195_edp_data = {
 	.efuse_fmt = mt8195_edp_efuse_fmt,
 	.audio_supported = false,
 	.audio_pkt_in_hblank_area = false,
+	.audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
 };
 
 static const struct mtk_dp_data mt8195_dp_data = {
@@ -2660,6 +2664,7 @@  static const struct mtk_dp_data mt8195_dp_data = {
 	.efuse_fmt = mt8195_dp_efuse_fmt,
 	.audio_supported = true,
 	.audio_pkt_in_hblank_area = false,
+	.audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
 };
 
 static const struct of_device_id mtk_dp_of_match[] = {
diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
index f38d6ff12afe..6d7f0405867e 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
+++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
@@ -162,6 +162,7 @@ 
 #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2	(1 << 8)
 #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4	(2 << 8)
 #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8	(3 << 8)
+#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2	(4 << 8)
 #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2	(5 << 8)
 #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4	(6 << 8)
 #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8	(7 << 8)