| Message ID | 20200217064250.15516-3-samuel@sholland.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sun8i-codec fixes and new features | expand |
On Mon, Feb 17, 2020 at 2:42 PM Samuel Holland <samuel@sholland.org> wrote: > > On the A64 (tested with the Pinephone), the current code causes the > left/right channels to be swapped during I2S playback from the CPU on > AIF1, and breaks DSP_A communication with the modem on AIF2. > > Trusting that the comment in the code is correct, the existing behavior > is kept for the A33. > > Cc: stable@kernel.org > Fixes: ec4a95409d5c ("arm64: dts: allwinner: a64: add nodes necessary for analog sound support") > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > sound/soc/sunxi/sun8i-codec.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c > index 55798bc8eae2..14cf31f5c535 100644 > --- a/sound/soc/sunxi/sun8i-codec.c > +++ b/sound/soc/sunxi/sun8i-codec.c > @@ -13,6 +13,7 @@ > #include <linux/delay.h> > #include <linux/clk.h> > #include <linux/io.h> > +#include <linux/of_device.h> > #include <linux/pm_runtime.h> > #include <linux/regmap.h> > #include <linux/log2.h> > @@ -89,6 +90,7 @@ struct sun8i_codec { > struct regmap *regmap; > struct clk *clk_module; > struct clk *clk_bus; > + bool inverted_lrck; > }; > > static int sun8i_codec_runtime_resume(struct device *dev) > @@ -209,18 +211,19 @@ static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) > value << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV); > > /* > - * It appears that the DAI and the codec don't share the same > - * polarity for the LRCK signal when they mean 'normal' and > - * 'inverted' in the datasheet. > + * It appears that the DAI and the codec in the A33 SoC don't > + * share the same polarity for the LRCK signal when they mean > + * 'normal' and 'inverted' in the datasheet. > * > * Since the DAI here is our regular i2s driver that have been > * tested with way more codecs than just this one, it means > * that the codec probably gets it backward, and we have to > * invert the value here. > */ > + value ^= scodec->inverted_lrck; > regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL, > BIT(SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV), > - !value << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV); > + value << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV); > > /* DAI format */ > switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { > @@ -568,6 +571,8 @@ static int sun8i_codec_probe(struct platform_device *pdev) > return PTR_ERR(scodec->regmap); > } > > + scodec->inverted_lrck = (uintptr_t)of_device_get_match_data(&pdev->dev); > + > platform_set_drvdata(pdev, scodec); > > pm_runtime_enable(&pdev->dev); > @@ -606,7 +611,14 @@ 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 = (void *)1, So depending on the answer to the previous patch, this might be enough, though somewhat an eyesore. Otherwise I suggest using a proper quirks structure. ChenYu > + }, > + { > + .compatible = "allwinner,sun50i-a64-codec", > + .data = (void *)0, > + }, > {} > }; > MODULE_DEVICE_TABLE(of, sun8i_codec_of_match); > -- > 2.24.1 >
On Mon, Feb 17, 2020 at 12:42:18AM -0600, Samuel Holland wrote: > + scodec->inverted_lrck = (uintptr_t)of_device_get_match_data(&pdev->dev); > + This is going to break the moment someone finds another quirk for some variant of this device, it's not scalable.
diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c index 55798bc8eae2..14cf31f5c535 100644 --- a/sound/soc/sunxi/sun8i-codec.c +++ b/sound/soc/sunxi/sun8i-codec.c @@ -13,6 +13,7 @@ #include <linux/delay.h> #include <linux/clk.h> #include <linux/io.h> +#include <linux/of_device.h> #include <linux/pm_runtime.h> #include <linux/regmap.h> #include <linux/log2.h> @@ -89,6 +90,7 @@ struct sun8i_codec { struct regmap *regmap; struct clk *clk_module; struct clk *clk_bus; + bool inverted_lrck; }; static int sun8i_codec_runtime_resume(struct device *dev) @@ -209,18 +211,19 @@ static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) value << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV); /* - * It appears that the DAI and the codec don't share the same - * polarity for the LRCK signal when they mean 'normal' and - * 'inverted' in the datasheet. + * It appears that the DAI and the codec in the A33 SoC don't + * share the same polarity for the LRCK signal when they mean + * 'normal' and 'inverted' in the datasheet. * * Since the DAI here is our regular i2s driver that have been * tested with way more codecs than just this one, it means * that the codec probably gets it backward, and we have to * invert the value here. */ + value ^= scodec->inverted_lrck; regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL, BIT(SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV), - !value << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV); + value << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV); /* DAI format */ switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { @@ -568,6 +571,8 @@ static int sun8i_codec_probe(struct platform_device *pdev) return PTR_ERR(scodec->regmap); } + scodec->inverted_lrck = (uintptr_t)of_device_get_match_data(&pdev->dev); + platform_set_drvdata(pdev, scodec); pm_runtime_enable(&pdev->dev); @@ -606,7 +611,14 @@ 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 = (void *)1, + }, + { + .compatible = "allwinner,sun50i-a64-codec", + .data = (void *)0, + }, {} }; MODULE_DEVICE_TABLE(of, sun8i_codec_of_match);
On the A64 (tested with the Pinephone), the current code causes the left/right channels to be swapped during I2S playback from the CPU on AIF1, and breaks DSP_A communication with the modem on AIF2. Trusting that the comment in the code is correct, the existing behavior is kept for the A33. Cc: stable@kernel.org Fixes: ec4a95409d5c ("arm64: dts: allwinner: a64: add nodes necessary for analog sound support") Signed-off-by: Samuel Holland <samuel@sholland.org> --- sound/soc/sunxi/sun8i-codec.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)