linux-kernel - Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6f2c7a0b-b68b-fc42-1a82-2b69c114823f@wanyeetech.com>
Date:   Wed, 13 Jul 2022 23:29:00 +0800
From:   Zhou Yanjie <zhouyu@...yeetech.com>
To:     Paul Cercueil <paul@...pouillou.net>
Cc:     Aidan MacDonald <aidanmacdonald.0x0@...il.com>,
        lgirdwood@...il.com, broonie@...nel.org, perex@...ex.cz,
        tiwai@...e.com, linux-mips@...r.kernel.org,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name
 SoC-specific

Hi Paul,

On 2022/7/13 下午11:07, Paul Cercueil wrote:
> Hi Zhou,
>
> Le mer., juil. 13 2022 at 22:33:44 +0800, Zhou Yanjie 
> <zhouyu@...yeetech.com> a écrit :
>> Hi Aidan,
>>
>> On 2022/7/9 上午12:02, Aidan MacDonald wrote:
>>> On some Ingenic SoCs, such as the X1000, there is a programmable
>>> divider used to generate the I2S system clock from a PLL, rather
>>> than a fixed PLL/2 clock. It doesn't make much sense to call the
>>> clock "pll half" on those SoCs, so the clock name should really be
>>> a SoC-dependent value.
>>>
>>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@...il.com>
>>> ---
>>>   sound/soc/jz4740/jz4740-i2s.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
>>> b/sound/soc/jz4740/jz4740-i2s.c
>>> index 0dcc658b3784..a41398c24d0e 100644
>>> --- a/sound/soc/jz4740/jz4740-i2s.c
>>> +++ b/sound/soc/jz4740/jz4740-i2s.c
>>> @@ -75,6 +75,8 @@ struct i2s_soc_info {
>>>       struct reg_field field_i2sdiv_capture;
>>>       struct reg_field field_i2sdiv_playback;
>>>   +    const char *pll_clk_name;
>>> +
>>>       bool shared_fifo_flush;
>>>   };
>>>   @@ -281,7 +283,7 @@ static int jz4740_i2s_set_sysclk(struct 
>>> snd_soc_dai *dai, int clk_id,
>>>           clk_set_parent(i2s->clk_i2s, parent);
>>>           break;
>>>       case JZ4740_I2S_CLKSRC_PLL:
>>> -        parent = clk_get(NULL, "pll half");
>>> +        parent = clk_get(NULL, i2s->soc_info->pll_clk_name);
>>>           if (IS_ERR(parent))
>>>               return PTR_ERR(parent);
>>>           clk_set_parent(i2s->clk_i2s, parent);
>>> @@ -400,6 +402,7 @@ static const struct i2s_soc_info 
>>> jz4740_i2s_soc_info = {
>>>       .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 8, 11),
>>>       .field_i2sdiv_capture    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>>       .field_i2sdiv_playback    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>> +    .pll_clk_name        = "pll half",
>>>       .shared_fifo_flush    = true,
>>>   };
>>>   @@ -409,6 +412,7 @@ static const struct i2s_soc_info 
>>> jz4760_i2s_soc_info = {
>>>       .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>>>       .field_i2sdiv_capture    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>>       .field_i2sdiv_playback    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>> +    .pll_clk_name        = "pll half",
>>>   };
>>
>>
>> Since JZ4760, according to the description of the I2SCDR register,
>> Ingenic SoCs no longer use PLL/2 clock, but directly use PLL clock,
>> so it seems also inappropriate to use "pll half" for these SoCs.
>
> The device tree passes the clock as "pll half". So the driver should 
> use this name as well...


I see...

It seems that the device tree of JZ4770 has used "pll half" already,
but there is no "pll half" used anywhere in the device tree of JZ4780,
maybe we can keep the pll_clk_name of JZ4770 as "pll half", and change
the pll_clk_name of JZ4780 to a more reasonable name.


Thanks and best regards!


>
> Cheers,
> -Paul
>
>>>     static struct snd_soc_dai_driver jz4770_i2s_dai = {
>>> @@ -435,6 +439,7 @@ static const struct i2s_soc_info 
>>> jz4770_i2s_soc_info = {
>>>       .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>>>       .field_i2sdiv_capture    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
>>>       .field_i2sdiv_playback    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>> +    .pll_clk_name        = "pll half",
>>>   };
>>
>>
>> Same here.
>>
>>
>>>     static const struct i2s_soc_info jz4780_i2s_soc_info = {
>>> @@ -443,6 +448,7 @@ static const struct i2s_soc_info 
>>> jz4780_i2s_soc_info = {
>>>       .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 16, 20),
>>>       .field_i2sdiv_capture    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 8, 11),
>>>       .field_i2sdiv_playback    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
>>> +    .pll_clk_name        = "pll half",
>>>   };
>>>
>>
>> Same here.
>>
>>
>> Thanks and best regards!
>>
>>
>>>   static const struct snd_soc_component_driver jz4740_i2s_component = {
>