| Message ID | 1389669956-2304-3-git-send-email-voice.shen@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 14, 2014 at 11:25:55AM +0800, Bo Shen wrote: > Make it available to choose the clock from TK pin or RK pin. This > is hardware design decided. > --- a/sound/soc/atmel/atmel_wm8904.c > +++ b/sound/soc/atmel/atmel_wm8904.c > @@ -108,6 +108,7 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev) > struct device_node *codec_np, *cpu_np; > struct snd_soc_card *card = &atmel_asoc_wm8904_card; > struct snd_soc_dai_link *dailink = &atmel_asoc_wm8904_dailink; > + struct atmel_ssc_info *ssc_info; > int ret; > > if (!np) { > @@ -115,6 +116,15 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev) > return -EINVAL; > } > > + ssc_info = devm_kzalloc(&pdev->dev, sizeof(*ssc_info), GFP_KERNEL); > + if (!ssc_info) > + return -ENOMEM; > + > + ssc_info->clk_from_rk_pin = > + of_property_read_bool(np, "clk_from_rk_pin"); > + > + card->drvdata = (void *)ssc_info; Shouldn't this code be in the DAI driver? Otherwise this series looks fine to me, though the DT folks might have something to say I guess.
On 14/01/2014 04:25, Bo Shen : > Make it available to choose the clock from TK pin or RK pin. This > is hardware design decided. > > Signed-off-by: Bo Shen <voice.shen@atmel.com> > --- > > sound/soc/atmel/atmel_wm8904.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/sound/soc/atmel/atmel_wm8904.c b/sound/soc/atmel/atmel_wm8904.c > index b4e3690..b85088d 100644 > --- a/sound/soc/atmel/atmel_wm8904.c > +++ b/sound/soc/atmel/atmel_wm8904.c > @@ -108,6 +108,7 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev) > struct device_node *codec_np, *cpu_np; > struct snd_soc_card *card = &atmel_asoc_wm8904_card; > struct snd_soc_dai_link *dailink = &atmel_asoc_wm8904_dailink; > + struct atmel_ssc_info *ssc_info; > int ret; > > if (!np) { > @@ -115,6 +116,15 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev) > return -EINVAL; > } > > + ssc_info = devm_kzalloc(&pdev->dev, sizeof(*ssc_info), GFP_KERNEL); Isn't an atmel_ssc_info structure table already instantiated in sound/soc/atmel/atmel_ssc_dai.c ... I see, you copy the information contained in this field in the proper ssc_info of the DAI in the previous patch... Well, isn't it a better way to pass parameters to the DAI than this one? > + if (!ssc_info) > + return -ENOMEM; > + > + ssc_info->clk_from_rk_pin = > + of_property_read_bool(np, "clk_from_rk_pin"); > + > + card->drvdata = (void *)ssc_info; > + > ret = snd_soc_of_parse_card_name(card, "atmel,model"); > if (ret) { > dev_err(&pdev->dev, "failed to parse card name\n"); >
Hi Mark, On 01/15/2014 04:36 AM, Mark Brown wrote: > On Tue, Jan 14, 2014 at 11:25:55AM +0800, Bo Shen wrote: >> Make it available to choose the clock from TK pin or RK pin. This >> is hardware design decided. > >> --- a/sound/soc/atmel/atmel_wm8904.c >> +++ b/sound/soc/atmel/atmel_wm8904.c >> @@ -108,6 +108,7 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev) >> struct device_node *codec_np, *cpu_np; >> struct snd_soc_card *card = &atmel_asoc_wm8904_card; >> struct snd_soc_dai_link *dailink = &atmel_asoc_wm8904_dailink; >> + struct atmel_ssc_info *ssc_info; >> int ret; >> >> if (!np) { >> @@ -115,6 +116,15 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev) >> return -EINVAL; >> } >> >> + ssc_info = devm_kzalloc(&pdev->dev, sizeof(*ssc_info), GFP_KERNEL); >> + if (!ssc_info) >> + return -ENOMEM; >> + >> + ssc_info->clk_from_rk_pin = >> + of_property_read_bool(np, "clk_from_rk_pin"); >> + >> + card->drvdata = (void *)ssc_info; > > Shouldn't this code be in the DAI driver? Otherwise this series looks > fine to me, though the DT folks might have something to say I guess. For audio on Atmel SoC, it depends on three device nodes, one is SSC node, one is the codec node and the sound node. The sound node will parse by machine driver, and machine driver is mainly for hardware connection. As the "clk_from_rk_pin" is decided by hardware, so, I put it here. If I move the code to dai driver, it will parse the sound node in dai driver, I think it will make the code a little bit not explicit. What do you think? Best Regards, Bo Shen
On Thu, Jan 16, 2014 at 09:31:23AM +0800, Bo Shen wrote: > >Shouldn't this code be in the DAI driver? Otherwise this series looks > >fine to me, though the DT folks might have something to say I guess. > For audio on Atmel SoC, it depends on three device nodes, one is > SSC node, one is the codec node and the sound node. > The sound node will parse by machine driver, and machine driver is > mainly for hardware connection. As the "clk_from_rk_pin" is decided > by hardware, so, I put it here. > If I move the code to dai driver, it will parse the sound node in > dai driver, I think it will make the code a little bit not explicit. > What do you think? I think it should just be a property of the DAI device. It's true that the card defines the connections but if something is going to be an option that's there for most if not all systems then putting it in the device driver means less effort on each integration.
diff --git a/sound/soc/atmel/atmel_wm8904.c b/sound/soc/atmel/atmel_wm8904.c index b4e3690..b85088d 100644 --- a/sound/soc/atmel/atmel_wm8904.c +++ b/sound/soc/atmel/atmel_wm8904.c @@ -108,6 +108,7 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev) struct device_node *codec_np, *cpu_np; struct snd_soc_card *card = &atmel_asoc_wm8904_card; struct snd_soc_dai_link *dailink = &atmel_asoc_wm8904_dailink; + struct atmel_ssc_info *ssc_info; int ret; if (!np) { @@ -115,6 +116,15 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev) return -EINVAL; } + ssc_info = devm_kzalloc(&pdev->dev, sizeof(*ssc_info), GFP_KERNEL); + if (!ssc_info) + return -ENOMEM; + + ssc_info->clk_from_rk_pin = + of_property_read_bool(np, "clk_from_rk_pin"); + + card->drvdata = (void *)ssc_info; + ret = snd_soc_of_parse_card_name(card, "atmel,model"); if (ret) { dev_err(&pdev->dev, "failed to parse card name\n");
Make it available to choose the clock from TK pin or RK pin. This is hardware design decided. Signed-off-by: Bo Shen <voice.shen@atmel.com> --- sound/soc/atmel/atmel_wm8904.c | 10 ++++++++++ 1 file changed, 10 insertions(+)