From: Chen-Yu Tsai <wens@csie.org> To: Chen-Yu Tsai <wens@csie.org> Cc: Linux-ALSA <alsa-devel@alsa-project.org>, "Liam Girdwood" <lgirdwood@gmail.com>, "Vasily Khoruzhick" <anarsoul@gmail.com>, "Marcus Cooper" <codekipper@gmail.com>, "Mark Brown" <broonie@kernel.org>, "Maxime Ripard" <maxime.ripard@free-electrons.com>, "Mylène Josserand" <mylene.josserand@free-electrons.com>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH 3/9] ASoC: sun8i-codec: Add quirk to specify aif1_lrck_div value Date: Mon, 4 Dec 2017 16:26:30 +0800 [thread overview] Message-ID: <CAGb2v65BCkF9Tf1uBMAybtmn_zZhU4pH7+bsZVUwrbZDgciaGg@mail.gmail.com> (raw) In-Reply-To: <CAGb2v65KMosNgkCa5Ddmf_T--8-BeLLjdqrdX3wb4H+kbudLjA@mail.gmail.com> On Mon, Dec 4, 2017 at 3:38 PM, Chen-Yu Tsai <wens@csie.org> wrote: > On Mon, Dec 4, 2017 at 4:41 AM, Vasily Khoruzhick <anarsoul@gmail.com> wrote: >> LRCK divider for A64 differs from A33, so add a quirk to support that. >> >> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> >> --- >> sound/soc/sunxi/sun8i-codec.c | 38 ++++++++++++++++++++++++++++++++------ >> 1 file changed, 32 insertions(+), 6 deletions(-) >> >> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c >> index 3dd183be08a4..054201d1de03 100644 >> --- a/sound/soc/sunxi/sun8i-codec.c >> +++ b/sound/soc/sunxi/sun8i-codec.c >> @@ -24,6 +24,7 @@ >> #include <linux/io.h> >> #include <linux/pm_runtime.h> >> #include <linux/regmap.h> >> +#include <linux/of_device.h> > > Please keep them sorted in alphabetical order. > >> >> #include <sound/pcm_params.h> >> #include <sound/soc.h> >> @@ -75,11 +76,22 @@ >> #define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK GENMASK(8, 6) >> #define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK GENMASK(12, 9) >> >> +/* >> + * struct sun8i_codec_quirks - Differences between SoC variants. >> + * >> + * @aif1_lrck_div: LRCK divider >> + */ >> +struct sun8i_codec_quirks { >> + unsigned int aif1_lrck_div; >> +}; >> + >> + >> struct sun8i_codec { >> - struct device *dev; >> - struct regmap *regmap; >> - struct clk *clk_module; >> - struct clk *clk_bus; >> + struct device *dev; >> + struct regmap *regmap; >> + struct clk *clk_module; >> + struct clk *clk_bus; >> + const struct sun8i_codec_quirks *variant; >> }; >> >> static int sun8i_codec_runtime_resume(struct device *dev) >> @@ -305,7 +317,7 @@ static int sun8i_codec_hw_params(struct snd_pcm_substream *substream, >> >> regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL, >> SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK, >> - SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_16); >> + scodec->variant->aif1_lrck_div); > > Hard coding a fixed divider is generally a bad idea. > You adding another fixed value for this is evidence of this. > > The BCK:LRCK divider is equal to sample width * 2. You should > have that instead. > > Also I see no evidence that either the I2S controller (see my > comment on patch 1) or the codec is limited to a certain sample > width in hardware, other than your claims, which are vague at > best. > > The user manual for both the A33 and A64 list many possible > widths and dividers. If the user manual is wrong, please provide > solid evidence proving it wrong. > > Either way this series will not be accepted in its current form. So following up on the discussion on IRC, please add the relevant information to the commit messages: - Why is this hard-coded value needed? i.e. does it not work with standard values generated by the framework from the format value? This is the most important part. This might also include: * What you have done to test the non-working values Other things worth mentioning: - Where the hard-coded values came from? - Why a special quirk is needed, instead of say, limiting the supported format to SNDRV_PCM_FMTBIT_S32_LE, which produces the correct wss and lrck_div values - Anything else you think is relevant to the patch or hardware You should also leave a comment explaining why the quirk is needed just before the code where you handle the quirks. Otherwise it makes little sense to people that don't have the context about this. Regards ChenYu > > ChenYu > >> >> sample_rate = sun8i_codec_get_hw_rate(params); >> if (sample_rate < 0) >> @@ -440,6 +452,10 @@ static const struct regmap_config sun8i_codec_regmap_config = { >> .cache_type = REGCACHE_FLAT, >> }; >> >> +static const struct sun8i_codec_quirks sun8i_a33_codec_quirks = { >> + .aif1_lrck_div = SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_16, >> +}; >> + >> static int sun8i_codec_probe(struct platform_device *pdev) >> { >> struct resource *res_base; >> @@ -453,6 +469,13 @@ static int sun8i_codec_probe(struct platform_device *pdev) >> >> scodec->dev = &pdev->dev; >> >> + scodec->variant = of_device_get_match_data(&pdev->dev); >> + if (!scodec->variant) { >> + dev_err(&pdev->dev, "Failed to determine the quirks to use\n"); >> + return -ENODEV; >> + } >> + >> + >> scodec->clk_module = devm_clk_get(&pdev->dev, "mod"); >> if (IS_ERR(scodec->clk_module)) { >> dev_err(&pdev->dev, "Failed to get the module clock\n"); >> @@ -524,7 +547,10 @@ static int sun8i_codec_remove(struct platform_device *pdev) >> } >> >> static const struct of_device_id sun8i_codec_of_match[] = { >> - { .compatible = "allwinner,sun8i-a33-codec" }, >> + { >> + .compatible = "allwinner,sun8i-a33-codec", >> + .data = &sun8i_a33_codec_quirks, >> + }, >> {} >> }; >> MODULE_DEVICE_TABLE(of, sun8i_codec_of_match); >> -- >> 2.15.0 >>
WARNING: multiple messages have this Message-ID (diff)
From: wens@csie.org (Chen-Yu Tsai) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 3/9] ASoC: sun8i-codec: Add quirk to specify aif1_lrck_div value Date: Mon, 4 Dec 2017 16:26:30 +0800 [thread overview] Message-ID: <CAGb2v65BCkF9Tf1uBMAybtmn_zZhU4pH7+bsZVUwrbZDgciaGg@mail.gmail.com> (raw) In-Reply-To: <CAGb2v65KMosNgkCa5Ddmf_T--8-BeLLjdqrdX3wb4H+kbudLjA@mail.gmail.com> On Mon, Dec 4, 2017 at 3:38 PM, Chen-Yu Tsai <wens@csie.org> wrote: > On Mon, Dec 4, 2017 at 4:41 AM, Vasily Khoruzhick <anarsoul@gmail.com> wrote: >> LRCK divider for A64 differs from A33, so add a quirk to support that. >> >> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> >> --- >> sound/soc/sunxi/sun8i-codec.c | 38 ++++++++++++++++++++++++++++++++------ >> 1 file changed, 32 insertions(+), 6 deletions(-) >> >> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c >> index 3dd183be08a4..054201d1de03 100644 >> --- a/sound/soc/sunxi/sun8i-codec.c >> +++ b/sound/soc/sunxi/sun8i-codec.c >> @@ -24,6 +24,7 @@ >> #include <linux/io.h> >> #include <linux/pm_runtime.h> >> #include <linux/regmap.h> >> +#include <linux/of_device.h> > > Please keep them sorted in alphabetical order. > >> >> #include <sound/pcm_params.h> >> #include <sound/soc.h> >> @@ -75,11 +76,22 @@ >> #define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK GENMASK(8, 6) >> #define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK GENMASK(12, 9) >> >> +/* >> + * struct sun8i_codec_quirks - Differences between SoC variants. >> + * >> + * @aif1_lrck_div: LRCK divider >> + */ >> +struct sun8i_codec_quirks { >> + unsigned int aif1_lrck_div; >> +}; >> + >> + >> struct sun8i_codec { >> - struct device *dev; >> - struct regmap *regmap; >> - struct clk *clk_module; >> - struct clk *clk_bus; >> + struct device *dev; >> + struct regmap *regmap; >> + struct clk *clk_module; >> + struct clk *clk_bus; >> + const struct sun8i_codec_quirks *variant; >> }; >> >> static int sun8i_codec_runtime_resume(struct device *dev) >> @@ -305,7 +317,7 @@ static int sun8i_codec_hw_params(struct snd_pcm_substream *substream, >> >> regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL, >> SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK, >> - SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_16); >> + scodec->variant->aif1_lrck_div); > > Hard coding a fixed divider is generally a bad idea. > You adding another fixed value for this is evidence of this. > > The BCK:LRCK divider is equal to sample width * 2. You should > have that instead. > > Also I see no evidence that either the I2S controller (see my > comment on patch 1) or the codec is limited to a certain sample > width in hardware, other than your claims, which are vague at > best. > > The user manual for both the A33 and A64 list many possible > widths and dividers. If the user manual is wrong, please provide > solid evidence proving it wrong. > > Either way this series will not be accepted in its current form. So following up on the discussion on IRC, please add the relevant information to the commit messages: - Why is this hard-coded value needed? i.e. does it not work with standard values generated by the framework from the format value? This is the most important part. This might also include: * What you have done to test the non-working values Other things worth mentioning: - Where the hard-coded values came from? - Why a special quirk is needed, instead of say, limiting the supported format to SNDRV_PCM_FMTBIT_S32_LE, which produces the correct wss and lrck_div values - Anything else you think is relevant to the patch or hardware You should also leave a comment explaining why the quirk is needed just before the code where you handle the quirks. Otherwise it makes little sense to people that don't have the context about this. Regards ChenYu > > ChenYu > >> >> sample_rate = sun8i_codec_get_hw_rate(params); >> if (sample_rate < 0) >> @@ -440,6 +452,10 @@ static const struct regmap_config sun8i_codec_regmap_config = { >> .cache_type = REGCACHE_FLAT, >> }; >> >> +static const struct sun8i_codec_quirks sun8i_a33_codec_quirks = { >> + .aif1_lrck_div = SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_16, >> +}; >> + >> static int sun8i_codec_probe(struct platform_device *pdev) >> { >> struct resource *res_base; >> @@ -453,6 +469,13 @@ static int sun8i_codec_probe(struct platform_device *pdev) >> >> scodec->dev = &pdev->dev; >> >> + scodec->variant = of_device_get_match_data(&pdev->dev); >> + if (!scodec->variant) { >> + dev_err(&pdev->dev, "Failed to determine the quirks to use\n"); >> + return -ENODEV; >> + } >> + >> + >> scodec->clk_module = devm_clk_get(&pdev->dev, "mod"); >> if (IS_ERR(scodec->clk_module)) { >> dev_err(&pdev->dev, "Failed to get the module clock\n"); >> @@ -524,7 +547,10 @@ static int sun8i_codec_remove(struct platform_device *pdev) >> } >> >> static const struct of_device_id sun8i_codec_of_match[] = { >> - { .compatible = "allwinner,sun8i-a33-codec" }, >> + { >> + .compatible = "allwinner,sun8i-a33-codec", >> + .data = &sun8i_a33_codec_quirks, >> + }, >> {} >> }; >> MODULE_DEVICE_TABLE(of, sun8i_codec_of_match); >> -- >> 2.15.0 >>
next prev parent reply other threads:[~2017-12-04 8:26 UTC|newest] Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-12-03 20:41 [PATCH 0/9] Add audiocodec support for A64 SoC Vasily Khoruzhick 2017-12-03 20:41 ` Vasily Khoruzhick 2017-12-03 20:41 ` [PATCH 1/9] ASoC: sun4i-i2s: Add quirk to handle fixed WSS Vasily Khoruzhick 2017-12-03 20:41 ` Vasily Khoruzhick 2017-12-04 0:43 ` Chen-Yu Tsai 2017-12-04 0:43 ` Chen-Yu Tsai 2017-12-03 20:41 ` [PATCH 2/9] ASoC: sun4i-i2s: Add compatibility with A64 codec I2S Vasily Khoruzhick 2017-12-03 20:41 ` Vasily Khoruzhick 2017-12-04 6:42 ` Code Kipper 2017-12-04 6:42 ` Code Kipper 2017-12-04 7:34 ` Vasily Khoruzhick 2017-12-04 7:34 ` Vasily Khoruzhick 2017-12-05 8:01 ` Maxime Ripard 2017-12-05 8:01 ` Maxime Ripard 2017-12-05 23:04 ` Vasily Khoruzhick 2017-12-05 23:04 ` Vasily Khoruzhick 2017-12-06 15:27 ` Maxime Ripard 2017-12-06 15:27 ` Maxime Ripard 2017-12-07 9:21 ` Code Kipper 2017-12-07 9:21 ` Code Kipper 2017-12-07 9:30 ` Chen-Yu Tsai 2017-12-07 9:30 ` Chen-Yu Tsai 2017-12-07 22:48 ` Vasily Khoruzhick 2017-12-07 22:48 ` Vasily Khoruzhick 2017-12-08 6:40 ` Code Kipper 2017-12-08 6:40 ` Code Kipper 2017-12-08 22:16 ` Vasily Khoruzhick 2017-12-08 22:16 ` Vasily Khoruzhick 2017-12-03 20:41 ` [PATCH 3/9] ASoC: sun8i-codec: Add quirk to specify aif1_lrck_div value Vasily Khoruzhick 2017-12-03 20:41 ` Vasily Khoruzhick 2017-12-04 7:23 ` Code Kipper 2017-12-04 7:23 ` Code Kipper 2017-12-04 7:38 ` Chen-Yu Tsai 2017-12-04 7:38 ` Chen-Yu Tsai 2017-12-04 8:26 ` Chen-Yu Tsai [this message] 2017-12-04 8:26 ` Chen-Yu Tsai 2017-12-03 20:41 ` [PATCH 4/9] ASoC: sun8i-codec: Add support for A64 SoC Vasily Khoruzhick 2017-12-03 20:41 ` Vasily Khoruzhick 2017-12-05 8:04 ` Maxime Ripard 2017-12-05 8:04 ` Maxime Ripard 2017-12-05 23:17 ` Vasily Khoruzhick 2017-12-05 23:17 ` Vasily Khoruzhick 2017-12-06 15:32 ` Maxime Ripard 2017-12-06 15:32 ` Maxime Ripard 2017-12-06 15:48 ` Mark Brown 2017-12-06 15:48 ` Mark Brown 2017-12-06 18:53 ` Maxime Ripard 2017-12-06 18:53 ` Maxime Ripard 2017-12-06 19:13 ` Mark Brown 2017-12-06 19:13 ` Mark Brown 2017-12-03 20:41 ` [PATCH 5/9] ASoC: sun8i-codec-analog: Use callbacks to add headphones and lineout outputs Vasily Khoruzhick 2017-12-03 20:41 ` Vasily Khoruzhick 2017-12-03 20:41 ` [PATCH 6/9] ASoC: sun8i-codec-analog: Add component driver field to quirks structure Vasily Khoruzhick 2017-12-03 20:41 ` Vasily Khoruzhick 2017-12-03 20:41 ` [PATCH 7/9] ASoC: sun8i-codec-analog: Add support for A64 SoC Vasily Khoruzhick 2017-12-03 20:41 ` Vasily Khoruzhick 2017-12-03 20:41 ` [PATCH 8/9] arm64: dts: allwinner: a64: Add nodes necessary for analog sound support Vasily Khoruzhick 2017-12-03 20:41 ` Vasily Khoruzhick 2017-12-03 20:41 ` [PATCH 9/9] arm64: dts: allwinner: a64: Enable sound on Pine64 and SoPine Vasily Khoruzhick 2017-12-03 20:41 ` Vasily Khoruzhick 2017-12-05 3:24 ` Chen-Yu Tsai 2017-12-05 3:24 ` Chen-Yu Tsai 2017-12-09 19:54 ` Vasily Khoruzhick 2017-12-09 19:54 ` Vasily Khoruzhick
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAGb2v65BCkF9Tf1uBMAybtmn_zZhU4pH7+bsZVUwrbZDgciaGg@mail.gmail.com \ --to=wens@csie.org \ --cc=alsa-devel@alsa-project.org \ --cc=anarsoul@gmail.com \ --cc=broonie@kernel.org \ --cc=codekipper@gmail.com \ --cc=lgirdwood@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=maxime.ripard@free-electrons.com \ --cc=mylene.josserand@free-electrons.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.