This series is a preparatory cleanup of the jz4740-i2s driver before
adding support for a new SoC. The two improvements are lifting
unnecessary restrictions on sample rates and formats -- the existing
ones appear to be derived from the limitations of the JZ4740's internal
codec and don't reflect the actual capabilities of the I2S controller.
I'm unable to test the series on any JZ47xx SoCs, but I have tested
on an X1000 (which is the SoC I'll be adding in a followup series).
Changes in v2:
* Drop two patches already in sound for-next.
* Squash two removal patches into the regmap fields patch.
* Remove the unused 'mem' resource in the driver private struct.
* Use regmap_set_bits() and regmap_clear_bits() to improve readability.
* Add fix for SoCs with independent FIFO flush bits (ie. most of them).
* Update sample formats patch with a more informative commit message.
* Add two new patches to refactor DAI/component probing.
Changes in v3:
* Fix missing 'ret' in patch 11 (yes, that was pretty silly of me)
Changes in v4:
* Refactor FIFO flush bits fix so it doesn't depend on regmap conversion.
Aidan MacDonald (11):
ASoC: jz4740-i2s: Handle independent FIFO flush bits
ASoC: jz4740-i2s: Remove unused 'mem' resource
ASoC: jz4740-i2s: Convert to regmap API
ASoC: jz4740-i2s: Simplify using regmap fields
ASoC: jz4740-i2s: Use FIELD_PREP() macros in hw_params callback
ASoC: jz4740-i2s: Align macro values and sort includes
ASoC: jz4740-i2s: Make the PLL clock name SoC-specific
ASoC: jz4740-i2s: Support S20_LE and S24_LE sample formats
ASoC: jz4740-i2s: Support continuous sample rate
ASoC: jz4740-i2s: Move component functions near the component driver
ASoC: jz4740-i2s: Refactor DAI probe/remove ops as component ops
sound/soc/jz4740/Kconfig | 1 +
sound/soc/jz4740/jz4740-i2s.c | 461 ++++++++++++++++++----------------
2 files changed, 248 insertions(+), 214 deletions(-)
--
2.35.1
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 <[email protected]>
---
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",
};
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",
};
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",
};
static const struct snd_soc_component_driver jz4740_i2s_component = {
--
2.35.1
Move the component suspend/resume functions near the definition
of the component driver to emphasize that they're unrelated to
the DAI functions.
Signed-off-by: Aidan MacDonald <[email protected]>
---
sound/soc/jz4740/jz4740-i2s.c | 72 +++++++++++++++++------------------
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index 70b9d28a40ce..5db73f12efcf 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -303,42 +303,6 @@ static int jz4740_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
return ret;
}
-static int jz4740_i2s_suspend(struct snd_soc_component *component)
-{
- struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
-
- if (snd_soc_component_active(component)) {
- regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
- clk_disable_unprepare(i2s->clk_i2s);
- }
-
- clk_disable_unprepare(i2s->clk_aic);
-
- return 0;
-}
-
-static int jz4740_i2s_resume(struct snd_soc_component *component)
-{
- struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
- int ret;
-
- ret = clk_prepare_enable(i2s->clk_aic);
- if (ret)
- return ret;
-
- if (snd_soc_component_active(component)) {
- ret = clk_prepare_enable(i2s->clk_i2s);
- if (ret) {
- clk_disable_unprepare(i2s->clk_aic);
- return ret;
- }
-
- regmap_set_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
- }
-
- return 0;
-}
-
static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
{
struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
@@ -459,6 +423,42 @@ static const struct i2s_soc_info jz4780_i2s_soc_info = {
.pll_clk_name = "pll half",
};
+static int jz4740_i2s_suspend(struct snd_soc_component *component)
+{
+ struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
+
+ if (snd_soc_component_active(component)) {
+ regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
+ clk_disable_unprepare(i2s->clk_i2s);
+ }
+
+ clk_disable_unprepare(i2s->clk_aic);
+
+ return 0;
+}
+
+static int jz4740_i2s_resume(struct snd_soc_component *component)
+{
+ struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
+ int ret;
+
+ ret = clk_prepare_enable(i2s->clk_aic);
+ if (ret)
+ return ret;
+
+ if (snd_soc_component_active(component)) {
+ ret = clk_prepare_enable(i2s->clk_i2s);
+ if (ret) {
+ clk_disable_unprepare(i2s->clk_aic);
+ return ret;
+ }
+
+ regmap_set_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
+ }
+
+ return 0;
+}
+
static const struct snd_soc_component_driver jz4740_i2s_component = {
.name = "jz4740-i2s",
.suspend = jz4740_i2s_suspend,
--
2.35.1
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 <[email protected]>
> ---
> 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.
>
> 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 = {
Hi Zhou,
Le mer., juil. 13 2022 at 22:33:44 +0800, Zhou Yanjie
<[email protected]> 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 <[email protected]>
>> ---
>> 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...
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
>> = {
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
> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 = {
>
Hi Aidan,
Le ven., juil. 8 2022 at 17:02:43 +0100, Aidan MacDonald
<[email protected]> a ?crit :
> Move the component suspend/resume functions near the definition
> of the component driver to emphasize that they're unrelated to
> the DAI functions.
>
> Signed-off-by: Aidan MacDonald <[email protected]>
I'm not really fond of moving code like that, so I'll leave Mark with
the liberty to take or not this patch.
Acked-by: Paul Cercueil <[email protected]>
Cheers,
-Paul
> ---
> sound/soc/jz4740/jz4740-i2s.c | 72
> +++++++++++++++++------------------
> 1 file changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/sound/soc/jz4740/jz4740-i2s.c
> b/sound/soc/jz4740/jz4740-i2s.c
> index 70b9d28a40ce..5db73f12efcf 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -303,42 +303,6 @@ static int jz4740_i2s_set_sysclk(struct
> snd_soc_dai *dai, int clk_id,
> return ret;
> }
>
> -static int jz4740_i2s_suspend(struct snd_soc_component *component)
> -{
> - struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
> -
> - if (snd_soc_component_active(component)) {
> - regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CONF,
> JZ_AIC_CONF_ENABLE);
> - clk_disable_unprepare(i2s->clk_i2s);
> - }
> -
> - clk_disable_unprepare(i2s->clk_aic);
> -
> - return 0;
> -}
> -
> -static int jz4740_i2s_resume(struct snd_soc_component *component)
> -{
> - struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
> - int ret;
> -
> - ret = clk_prepare_enable(i2s->clk_aic);
> - if (ret)
> - return ret;
> -
> - if (snd_soc_component_active(component)) {
> - ret = clk_prepare_enable(i2s->clk_i2s);
> - if (ret) {
> - clk_disable_unprepare(i2s->clk_aic);
> - return ret;
> - }
> -
> - regmap_set_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
> - }
> -
> - return 0;
> -}
> -
> static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
> {
> struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> @@ -459,6 +423,42 @@ static const struct i2s_soc_info
> jz4780_i2s_soc_info = {
> .pll_clk_name = "pll half",
> };
>
> +static int jz4740_i2s_suspend(struct snd_soc_component *component)
> +{
> + struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
> +
> + if (snd_soc_component_active(component)) {
> + regmap_clear_bits(i2s->regmap, JZ_REG_AIC_CONF,
> JZ_AIC_CONF_ENABLE);
> + clk_disable_unprepare(i2s->clk_i2s);
> + }
> +
> + clk_disable_unprepare(i2s->clk_aic);
> +
> + return 0;
> +}
> +
> +static int jz4740_i2s_resume(struct snd_soc_component *component)
> +{
> + struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
> + int ret;
> +
> + ret = clk_prepare_enable(i2s->clk_aic);
> + if (ret)
> + return ret;
> +
> + if (snd_soc_component_active(component)) {
> + ret = clk_prepare_enable(i2s->clk_i2s);
> + if (ret) {
> + clk_disable_unprepare(i2s->clk_aic);
> + return ret;
> + }
> +
> + regmap_set_bits(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_ENABLE);
> + }
> +
> + return 0;
> +}
> +
> static const struct snd_soc_component_driver jz4740_i2s_component = {
> .name = "jz4740-i2s",
> .suspend = jz4740_i2s_suspend,
> --
> 2.35.1
>
Hi Aidan,
Le ven., juil. 8 2022 at 17:02:40 +0100, Aidan MacDonald
<[email protected]> a ?crit :
> 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 <[email protected]>
Reviewed-by: Paul Cercueil <[email protected]>
Cheers,
-Paul
> ---
> 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",
> };
>
> 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",
> };
>
> 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",
> };
>
> static const struct snd_soc_component_driver jz4740_i2s_component = {
> --
> 2.35.1
>
On Fri, 8 Jul 2022 17:02:33 +0100, Aidan MacDonald wrote:
> This series is a preparatory cleanup of the jz4740-i2s driver before
> adding support for a new SoC. The two improvements are lifting
> unnecessary restrictions on sample rates and formats -- the existing
> ones appear to be derived from the limitations of the JZ4740's internal
> codec and don't reflect the actual capabilities of the I2S controller.
>
> I'm unable to test the series on any JZ47xx SoCs, but I have tested
> on an X1000 (which is the SoC I'll be adding in a followup series).
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[02/11] ASoC: jz4740-i2s: Remove unused 'mem' resource
commit: cd57272c4e686d4ad2d2e775a40a3eac9f96ec7c
[04/11] ASoC: jz4740-i2s: Simplify using regmap fields
(no commit info)
[05/11] ASoC: jz4740-i2s: Use FIELD_PREP() macros in hw_params callback
(no commit info)
[06/11] ASoC: jz4740-i2s: Align macro values and sort includes
(no commit info)
[07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific
(no commit info)
[08/11] ASoC: jz4740-i2s: Support S20_LE and S24_LE sample formats
(no commit info)
[09/11] ASoC: jz4740-i2s: Support continuous sample rate
(no commit info)
[10/11] ASoC: jz4740-i2s: Move component functions near the component driver
(no commit info)
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
Zhou Yanjie <[email protected]> writes:
> 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 <[email protected]>
>> a écrit :
>>> Hi Aidan,
>>>
>>> On 2022/7/9 上午12:02, Aidan MacDonald wrote:
>>>> @@ -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,
>>>> };
>>>
>>>
>>> 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!
Actually, the clock names in the DT are meaningless. The clk_get() call
matches only the clock's name in the CGU driver. So in fact the driver
is "broken" for jz4780. It seems jz4770 doesn't work correctly either,
it has no "pll half", and three possible parents for its "i2s" clock.
Since the driver only supports the internal codec, which requires the
"ext" clock, there isn't a problem in practice.
I'm just going to drop this patch and leave .set_sysclk() alone for now.
I think a better approach is to have the DT define an array of parent
clocks for .set_sysclk()'s use, instead of hardcoding parents in the
driver. If the parent array is missing the driver can default to using
"ext" so existing DTs will work.
Regards,
Aidan
Hi Aidan,
Le sam. 22 oct. 2022 à 18:15:05 +0100, Aidan MacDonald
<[email protected]> a écrit :
>
> Zhou Yanjie <[email protected]> writes:
>
>> 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
>>> <[email protected]>
>>> a écrit :
>>>> Hi Aidan,
>>>>
>>>> On 2022/7/9 上午12:02, Aidan MacDonald wrote:
>>>>> @@ -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,
>>>>> };
>>>>
>>>>
>>>> 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!
>
> Actually, the clock names in the DT are meaningless. The clk_get()
> call
> matches only the clock's name in the CGU driver. So in fact the driver
> is "broken" for jz4780. It seems jz4770 doesn't work correctly either,
> it has no "pll half", and three possible parents for its "i2s" clock.
That's not true. The clock names are matched via DT.
Only in the case where a corresponding clock cannot be found via DT
will it search for the clock name among the clock providers. I believe
this is a legacy mechanism and you absolutely shouldn't rely on it.
-Paul
> Since the driver only supports the internal codec, which requires the
> "ext" clock, there isn't a problem in practice.
>
> I'm just going to drop this patch and leave .set_sysclk() alone for
> now.
> I think a better approach is to have the DT define an array of parent
> clocks for .set_sysclk()'s use, instead of hardcoding parents in the
> driver. If the parent array is missing the driver can default to using
> "ext" so existing DTs will work.
>
> Regards,
> Aidan
Paul Cercueil <[email protected]> writes:
> Hi Aidan,
>
> Le sam. 22 oct. 2022 à 18:15:05 +0100, Aidan MacDonald
> <[email protected]> a écrit :
>> Zhou Yanjie <[email protected]> writes:
>>
>>> 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
>>>> <[email protected]>
>>>> a écrit :
>>>>> Hi Aidan,
>>>>> On 2022/7/9 上午12:02, Aidan MacDonald wrote:
>>>>>> @@ -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,
>>>>>> };
>>>>> 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!
>> Actually, the clock names in the DT are meaningless. The clk_get() call
>> matches only the clock's name in the CGU driver. So in fact the driver
>> is "broken" for jz4780. It seems jz4770 doesn't work correctly either,
>> it has no "pll half", and three possible parents for its "i2s" clock.
>
> That's not true. The clock names are matched via DT.
>
> Only in the case where a corresponding clock cannot be found via DT will it
> search for the clock name among the clock providers. I believe this is a legacy
> mechanism and you absolutely shouldn't rely on it.
>
> -Paul
>
What you say is only true for clk_get() with a device argument. When the
device argument is NULL -- which is the case in .set_sysclk() -- then
the DT name is not matched. Check drivers/clk/clkdev.c, in clk_find().
When the dev_id is NULL, it will not match any lookup entries with a
non-null dev_id, and I believe dev_id is the mechanism that implements
DT clock lookup. Only the wildcard entries from the CGU driver will be
matched if dev_id is NULL, so the DT is being ignored.
If you don't believe me, try changing "pll half" in the device tree and
the I2S driver to something else. I have done this, and it doesn't work.
That proves the name in the device tree is not being used.
I agree we shouldn't rely on this, it's a legacy behavior, but the fact
is that's how the driver already works. I'm dropping this patch because
the driver is wrong and needs a different fix...
>> I think a better approach is to have the DT define an array of parent
>> clocks for .set_sysclk()'s use, instead of hardcoding parents in the
>> driver. If the parent array is missing the driver can default to using
>> "ext" so existing DTs will work.
As much as I like this idea there doesn't seem to be a mechanism for
handling a free-floating array of clocks in the DT. Everything has
to be put in the main "clocks" array. That makes it pretty hard to
figure out which ones are meant to be the parent clocks.
Do you know of any way to do this generically from the DT? If there's
no way to get away from a hardcoded array of names in the driver, I can
at least add a device argument to clk_get() so it'll use the DT names.
Regards,
Aidan
Hi Aidan,
Le dim. 23 oct. 2022 à 14:29:24 +0100, Aidan MacDonald
<[email protected]> a écrit :
>
> Paul Cercueil <[email protected]> writes:
>
>> Hi Aidan,
>>
>> Le sam. 22 oct. 2022 à 18:15:05 +0100, Aidan MacDonald
>> <[email protected]> a écrit :
>>> Zhou Yanjie <[email protected]> writes:
>>>
>>>> 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
>>>>> <[email protected]>
>>>>> a écrit :
>>>>>> Hi Aidan,
>>>>>> On 2022/7/9 上午12:02, Aidan MacDonald wrote:
>>>>>>> @@ -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,
>>>>>>> };
>>>>>> 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!
>>> Actually, the clock names in the DT are meaningless. The clk_get()
>>> call
>>> matches only the clock's name in the CGU driver. So in fact the
>>> driver
>>> is "broken" for jz4780. It seems jz4770 doesn't work correctly
>>> either,
>>> it has no "pll half", and three possible parents for its "i2s"
>>> clock.
>>
>> That's not true. The clock names are matched via DT.
>>
>> Only in the case where a corresponding clock cannot be found via DT
>> will it
>> search for the clock name among the clock providers. I believe this
>> is a legacy
>> mechanism and you absolutely shouldn't rely on it.
>>
>> -Paul
>>
>
> What you say is only true for clk_get() with a device argument. When
> the
> device argument is NULL -- which is the case in .set_sysclk() -- then
> the DT name is not matched. Check drivers/clk/clkdev.c, in clk_find().
> When the dev_id is NULL, it will not match any lookup entries with a
> non-null dev_id, and I believe dev_id is the mechanism that implements
> DT clock lookup. Only the wildcard entries from the CGU driver will be
> matched if dev_id is NULL, so the DT is being ignored.
>
> If you don't believe me, try changing "pll half" in the device tree
> and
> the I2S driver to something else. I have done this, and it doesn't
> work.
> That proves the name in the device tree is not being used.
Well, let's pass them a device pointer then.
> I agree we shouldn't rely on this, it's a legacy behavior, but the
> fact
> is that's how the driver already works. I'm dropping this patch
> because
> the driver is wrong and needs a different fix...
"How the driver already works" is a bit misleading, I never saw this
.set_sysclk() callback being called, so I can't really say that it
works.
>>> I think a better approach is to have the DT define an array of
>>> parent
>>> clocks for .set_sysclk()'s use, instead of hardcoding parents in
>>> the
>>> driver. If the parent array is missing the driver can default to
>>> using
>>> "ext" so existing DTs will work.
>
> As much as I like this idea there doesn't seem to be a mechanism for
> handling a free-floating array of clocks in the DT. Everything has
> to be put in the main "clocks" array. That makes it pretty hard to
> figure out which ones are meant to be the parent clocks.
>
> Do you know of any way to do this generically from the DT? If there's
> no way to get away from a hardcoded array of names in the driver, I
> can
> at least add a device argument to clk_get() so it'll use the DT names.
In jz4740_i2s_set_sysclk():
#define JZ4740_I2S_FIRST_PARENT_CLK 2
parent = of_clk_get(dev->of_node, JZ4740_I2S_FIRST_PARENT_CLK + clk_id);
is how I'd do it.
The DTs all have "aic", "i2s" as the first two clocks. It is even
enforced in the DT schemas.
Cheers,
-Paul
Paul Cercueil <[email protected]> writes:
> Hi Aidan,
>
> Le dim. 23 oct. 2022 à 14:29:24 +0100, Aidan MacDonald
> <[email protected]> a écrit :
>> Paul Cercueil <[email protected]> writes:
>>
>>> Hi Aidan,
>>> Le sam. 22 oct. 2022 à 18:15:05 +0100, Aidan MacDonald
>>> <[email protected]> a écrit :
>>>> Actually, the clock names in the DT are meaningless. The clk_get() call
>>>> matches only the clock's name in the CGU driver. So in fact the driver
>>>> is "broken" for jz4780. It seems jz4770 doesn't work correctly either,
>>>> it has no "pll half", and three possible parents for its "i2s" clock.
>>> That's not true. The clock names are matched via DT.
>>> Only in the case where a corresponding clock cannot be found via DT will it
>>> search for the clock name among the clock providers. I believe this is a
>>> legacy
>>> mechanism and you absolutely shouldn't rely on it.
>>> -Paul
>>>
>> What you say is only true for clk_get() with a device argument. When the
>> device argument is NULL -- which is the case in .set_sysclk() -- then
>> the DT name is not matched. Check drivers/clk/clkdev.c, in clk_find().
>> When the dev_id is NULL, it will not match any lookup entries with a
>> non-null dev_id, and I believe dev_id is the mechanism that implements
>> DT clock lookup. Only the wildcard entries from the CGU driver will be
>> matched if dev_id is NULL, so the DT is being ignored.
>> If you don't believe me, try changing "pll half" in the device tree and
>> the I2S driver to something else. I have done this, and it doesn't work.
>> That proves the name in the device tree is not being used.
>
> Well, let's pass them a device pointer then.
>
Yes, I'll do that when I revise the patch.
>> I agree we shouldn't rely on this, it's a legacy behavior, but the fact
>> is that's how the driver already works. I'm dropping this patch because
>> the driver is wrong and needs a different fix...
>
> "How the driver already works" is a bit misleading, I never saw this
> .set_sysclk() callback being called, so I can't really say that it works.
>
>>>> I think a better approach is to have the DT define an array of parent
>>>> clocks for .set_sysclk()'s use, instead of hardcoding parents in the
>>>> driver. If the parent array is missing the driver can default to using
>>>> "ext" so existing DTs will work.
>> As much as I like this idea there doesn't seem to be a mechanism for
>> handling a free-floating array of clocks in the DT. Everything has
>> to be put in the main "clocks" array. That makes it pretty hard to
>> figure out which ones are meant to be the parent clocks.
>> Do you know of any way to do this generically from the DT? If there's
>> no way to get away from a hardcoded array of names in the driver, I can
>> at least add a device argument to clk_get() so it'll use the DT names.
>
> In jz4740_i2s_set_sysclk():
>
> #define JZ4740_I2S_FIRST_PARENT_CLK 2
> parent = of_clk_get(dev->of_node, JZ4740_I2S_FIRST_PARENT_CLK + clk_id);
>
> is how I'd do it.
>
> The DTs all have "aic", "i2s" as the first two clocks. It is even enforced in
> the DT schemas.
>
> Cheers,
> -Paul
Sounds like a plan. I was hoping to avoid adding CONFIG_OF back
considering I removed it in an earlier patch since it was unused. :)
Guess it doesn't really matter for this driver since the Ingenic
SoCs need CONFIG_OF anyway.
Regards,
Aidan