diff mbox

[06/14] ASoC: Add sun8i digital audio codec

Message ID 85cbd9926e52d0aa03f6bbfd8794373d8db491e0.1475571575.git.mylene.josserand@free-electrons.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Mylene JOSSERAND Oct. 4, 2016, 9:46 a.m. UTC
Add the digital sun8i audio codec which handles the base register
(without DAI).
The driver handles only the basic playback of the A33 codec, from
the DAC to the headphones. All other features (microphone, capture,
etc) will be added later.

Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
---
 sound/soc/sunxi/Kconfig       |   9 +
 sound/soc/sunxi/Makefile      |   1 +
 sound/soc/sunxi/sun8i-codec.c | 492 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 502 insertions(+)
 create mode 100644 sound/soc/sunxi/sun8i-codec.c

Comments

Thomas Petazzoni Oct. 4, 2016, 12:40 p.m. UTC | #1
Hello,

On Tue,  4 Oct 2016 11:46:19 +0200, Mylène Josserand wrote:
> Add the digital sun8i audio codec which handles the base register
> (without DAI).

I'm not sure what you mean by "which handles the base register".

> diff --git a/sound/soc/sunxi/Kconfig b/sound/soc/sunxi/Kconfig
> index 7aee95a..9e287b0 100644
> --- a/sound/soc/sunxi/Kconfig
> +++ b/sound/soc/sunxi/Kconfig
> @@ -27,6 +27,15 @@ config SND_SUN4I_SPDIF
>  	  Say Y or M to add support for the S/PDIF audio block in the Allwinner
>  	  A10 and affiliated SoCs.
>  
> +config SND_SUN8I_CODEC
> +	tristate "Allwinner SUN8I audio codec"
> +	select REGMAP_MMIO
> +        help

Indentation issue here, it should be intended with one tab, not spaces.

You probably also want a "depends on OF" here.

> +/* CODEC_OFFSET represents the offset of the codec registers
> + * and not all the DAI registers
> + */

This is not the proper comment style I believe for audio code, it
should be:

/*
 * ...
 */

> +#define CODEC_OFFSET				0x200

Do you really need this CODEC_OFFSET macro? Why not simply use directly
the right offsets? I.e instead of:

  #define SUN8I_SYSCLK_CTL			(0x20c - CODEC_OFFSET)

use:

  #define SUN8I_SYSCLK_CTL			0xc

> +#define CODEC_BASSADDRESS			0x01c22c00

This define is not used anywhere.

> +#define SUN8I_SYSCLK_CTL			(0x20c - CODEC_OFFSET)
> +#define SUN8I_SYSCLK_CTL_AIF1CLK_ENA		(11)
> +#define SUN8I_SYSCLK_CTL_SYSCLK_ENA		(3)
> +#define SUN8I_SYSCLK_CTL_SYSCLK_SRC		(0)

Parenthesis around single values are not really useful.

> +#define SUN8I_MOD_CLK_ENA			(0x210 - CODEC_OFFSET)
> +#define SUN8I_MOD_CLK_ENA_AIF1			(15)
> +#define SUN8I_MOD_CLK_ENA_DAC			(2)
> +#define SUN8I_MOD_RST_CTL			(0x214 - CODEC_OFFSET)
> +#define SUN8I_MOD_RST_CTL_AIF1			(15)
> +#define SUN8I_MOD_RST_CTL_DAC			(2)
> +#define SUN8I_SYS_SR_CTRL			(0x218 - CODEC_OFFSET)
> +#define SUN8I_SYS_SR_CTRL_AIF1_FS		(12)
> +#define SUN8I_SYS_SR_CTRL_AIF2_FS		(8)
> +#define SUN8I_AIF1CLK_CTRL			(0x240 - CODEC_OFFSET)
> +#define SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD	(15)
> +#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV	(14)
> +#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV	(13)
> +#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV	(9)
> +#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV	(6)
> +#define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ	(4)
> +#define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT	(2)
> +#define SUN8I_AIF1_DACDAT_CTRL			(0x248 - CODEC_OFFSET)
> +#define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0L_ENA	(15)
> +#define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA	(14)
> +#define SUN8I_DAC_DIG_CTRL			(0x320 - CODEC_OFFSET)
> +#define SUN8I_DAC_DIG_CTRL_ENDA		(15)
> +#define SUN8I_DAC_MXR_SRC			(0x330 - CODEC_OFFSET)
> +#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF1DA0L (15)
> +#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF1DA1L (14)
> +#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF2DACL (13)
> +#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_ADCL	(12)
> +#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA0R (11)
> +#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA1R (10)
> +#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF2DACR (9)
> +#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_ADCR	(8)

Indentation of the value is not very clean for those last defines.

> +static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> +{
> +	struct sun8i_codec *scodec = snd_soc_codec_get_drvdata(dai->codec);
> +	unsigned long value;

I'm not sure "unsigned long" is a very good choice here, it's going to
be a 64 bits integer on 64 bits platform. I'd suggest to use "u32",
which also seems to be what's used in _set_fmt() function of the
sun4i-i2s.c driver.


> +static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params,
> +				 struct snd_soc_dai *dai)
> +{
> +	int rs_value  = 0;

Two spaces before the = sign, not needed. Is the initialization to 0
really needed? Also, this should be a u32.

> +	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
> +			   0x3 << SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ,

Maybe a #define value to replace the hardcoded 0x3 ?

> +			   rs_value << SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ);
> +
> +	/* calculate bclk_lrck_div Ratio */
> +	bclk_lrck_div = sample_resolution * 2;
> +	switch (bclk_lrck_div) {
> +	case 16:
> +		bclk_lrck_div = 0;
> +		break;
> +	case 32:
> +		bclk_lrck_div = 1;
> +		break;
> +	case 64:
> +		bclk_lrck_div = 2;
> +		break;
> +	case 128:
> +		bclk_lrck_div = 3;
> +		break;
> +	case 256:
> +		bclk_lrck_div = 4;
> +		break;

This could quite easily be replaced by a formula, if you don't care
about error checking:

	bclk_lrck_div = log2(bclk_lrck_div) - 4;

Of course, if you care about error checking, this switch is nicer.

> +	default:

So there's no error checking if the value is not supported?

> +		break;
> +	}
> +	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
> +			   0x7 << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV,

#define to replace the hard-coded 0x7 ?

> +			   bclk_lrck_div << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV);
> +
> +	sample_rate = sun8i_codec_get_hw_rate(params);
> +	if (sample_rate < 0)
> +		return sample_rate;
> +
> +	regmap_update_bits(scodec->regmap, SUN8I_SYS_SR_CTRL,
> +			   0xf << SUN8I_SYS_SR_CTRL_AIF1_FS,

Ditto 0xf

> +			   sample_rate << SUN8I_SYS_SR_CTRL_AIF1_FS);
> +	regmap_update_bits(scodec->regmap, SUN8I_SYS_SR_CTRL,
> +			   0xf << SUN8I_SYS_SR_CTRL_AIF2_FS,

Ditto 0xf.


> +static struct snd_soc_dai_driver sun8i_codec_dai = {
> +	.name = "sun8i",
> +	/* playback capabilities */
> +	.playback = {
> +		.stream_name = "Playback",
> +		.channels_min = 1,
> +		.channels_max = 2,
> +		.rates = SNDRV_PCM_RATE_8000_192000 |
> +			SNDRV_PCM_RATE_KNOT,
> +		.formats = SNDRV_PCM_FMTBIT_S8 |
> +			SNDRV_PCM_FMTBIT_S16_LE |
> +			SNDRV_PCM_FMTBIT_S18_3LE |
> +			SNDRV_PCM_FMTBIT_S20_3LE |
> +			SNDRV_PCM_FMTBIT_S24_LE |
> +			SNDRV_PCM_FMTBIT_S32_LE,
> +	},
> +	/* pcm operations */
> +	.ops = &sun8i_codec_dai_ops,
> +};
> +EXPORT_SYMBOL(sun8i_codec_dai);

This EXPORT_SYMBOL looks wrong. First because it doesn't seem to be
used outside of this module. And second because using EXPORT_SYMBOL on
a function defined as static doesn't make much sense, as the "static"
qualifier limits the visibility of the symbol to the current
compilation unit.

> +
> +static int sun8i_soc_probe(struct snd_soc_codec *codec)
> +{
> +	return 0;
> +}
> +
> +/* power down chip */
> +static int sun8i_soc_remove(struct snd_soc_codec *codec)
> +{
> +	return 0;
> +}

I believe you can remove those stub functions.

> +
> +static struct snd_soc_codec_driver sun8i_soc_codec = {
> +	.probe			= sun8i_soc_probe,
> +	.remove		= sun8i_soc_remove,

And remove these.

> +	.component_driver = {
> +		.dapm_widgets		= sun8i_codec_dapm_widgets,
> +		.num_dapm_widgets	= ARRAY_SIZE(sun8i_codec_dapm_widgets),
> +		.dapm_routes		= sun8i_codec_dapm_routes,
> +		.num_dapm_routes	= ARRAY_SIZE(sun8i_codec_dapm_routes),

I'm probably missing something, but in the sun4i-codec.c driver, those
fields are initialized directly in the snd_soc_codec_driver structure,
not in the .component_driver sub-structure.


> +static int sun8i_codec_probe(struct platform_device *pdev)
> +{
> +	struct resource *res_base;
> +	struct sun8i_codec *scodec;
> +	void __iomem *base;
> +
> +	scodec = devm_kzalloc(&pdev->dev, sizeof(*scodec), GFP_KERNEL);
> +	if (!scodec)
> +		return -ENOMEM;
> +
> +	scodec->dev = &pdev->dev;
> +
> +	/* Get the clocks from the DT */
> +	scodec->clk_module = devm_clk_get(&pdev->dev, "codec");
> +	if (IS_ERR(scodec->clk_module)) {
> +		dev_err(&pdev->dev, "Failed to get the module clock\n");
> +		return PTR_ERR(scodec->clk_module);
> +	}
> +	if (clk_prepare_enable(scodec->clk_module))
> +		pr_err("err:open failed;\n");

Grr, pr_err, not good. Plus you want to return with an error from the
probe() function.

> +
> +	scodec->clk_apb = devm_clk_get(&pdev->dev, "apb");
> +	if (IS_ERR(scodec->clk_apb)) {
> +		dev_err(&pdev->dev, "Failed to get the apb clock\n");
> +		return PTR_ERR(scodec->clk_apb);
> +	}
> +	if (clk_prepare_enable(scodec->clk_apb))
> +		pr_err("err:open failed;\n");

Ditto. + unprepare/disable the previous clock.

> +
> +	/* Get base resources, registers and regmap */
> +	res_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "audio");
> +	base = devm_ioremap_resource(&pdev->dev, res_base);
> +	if (IS_ERR(base)) {
> +		dev_err(&pdev->dev, "Failed to map the registers\n");
> +		return PTR_ERR(base);
> +	}
> +
> +	scodec->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +					       &sun8i_codec_regmap_config);
> +	if (IS_ERR(scodec->regmap)) {
> +		dev_err(&pdev->dev, "Failed to create our regmap\n");
> +		return PTR_ERR(scodec->regmap);
> +	}
> +
> +	/* Set the codec data as driver data */
> +	dev_set_drvdata(&pdev->dev, scodec);

Use:

	platform_set_drvdata(pdev, scodec)

> +
> +	snd_soc_register_codec(&pdev->dev, &sun8i_soc_codec, &sun8i_codec_dai,
> +			       1);

That's a matter of taste, but I find the "1" alone on its own line a
bit weird. Maybe move &sun8i_codec_dai on the second line as well. But
again, it's mainly a matter of taste, so Mark might disagree here.

> +
> +	return 0;
> +}
> +
> +static int sun8i_codec_remove(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card = platform_get_drvdata(pdev);
> +	struct sun8i_codec *scodec = snd_soc_card_get_drvdata(card);
> +
> +	snd_soc_unregister_codec(&pdev->dev);
> +	clk_disable_unprepare(scodec->clk_module);
> +	clk_disable_unprepare(scodec->clk_apb);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sun8i_codec_of_match[] = {
> +	{ .compatible = "allwinner,sun8i-a33-codec" },
> +	{ .compatible = "allwinner,sun8i-a23-codec" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sun8i_codec_of_match);
> +
> +static struct platform_driver sun8i_codec_driver = {
> +	.driver = {
> +		.name = "sun8i-codec",
> +		.owner = THIS_MODULE,
> +		.of_match_table = sun8i_codec_of_match,
> +	},
> +	.probe = sun8i_codec_probe,
> +	.remove = sun8i_codec_remove,
> +};
> +module_platform_driver(sun8i_codec_driver);
> +
> +MODULE_DESCRIPTION("Allwinner A33 (sun8i) codec driver");
> +MODULE_AUTHOR("huanxin<huanxin@reuuimllatech.com>");

Space between the name and the e-mail address.

> +MODULE_AUTHOR("Mylène Josserand <mylene.josserand@free-electrons.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:sun8i-codec");

Thanks,

Thomas
Mark Brown Oct. 4, 2016, 1:07 p.m. UTC | #2
On Tue, Oct 04, 2016 at 02:40:08PM +0200, Thomas Petazzoni wrote:

> > +/* CODEC_OFFSET represents the offset of the codec registers
> > + * and not all the DAI registers
> > + */

> This is not the proper comment style I believe for audio code, it
> should be:

> /*
>  * ...
>  */

I don't care, IIRC that's something from CodingStyle which checkpatch
moans about.

> > +	/* pcm operations */
> > +	.ops = &sun8i_codec_dai_ops,
> > +};
> > +EXPORT_SYMBOL(sun8i_codec_dai);

> This EXPORT_SYMBOL looks wrong. First because it doesn't seem to be
> used outside of this module. And second because using EXPORT_SYMBOL on
> a function defined as static doesn't make much sense, as the "static"
> qualifier limits the visibility of the symbol to the current
> compilation unit.

Also all the ASoC code is _GPL so a non-GPL export is an issue.

> > +	.component_driver = {
> > +		.dapm_widgets		= sun8i_codec_dapm_widgets,
> > +		.num_dapm_widgets	= ARRAY_SIZE(sun8i_codec_dapm_widgets),
> > +		.dapm_routes		= sun8i_codec_dapm_routes,
> > +		.num_dapm_routes	= ARRAY_SIZE(sun8i_codec_dapm_routes),

> I'm probably missing something, but in the sun4i-codec.c driver, those
> fields are initialized directly in the snd_soc_codec_driver structure,
> not in the .component_driver sub-structure.

We're in the process of pushing everything out to component level, this
update should be made in the old code if it's not happened already.

> > +	if (clk_prepare_enable(scodec->clk_module))
> > +		pr_err("err:open failed;\n");

> Grr, pr_err, not good. Plus you want to return with an error from the
> probe() function.

Also when printing an error message use dev_err().
Thomas Petazzoni Oct. 4, 2016, 1:16 p.m. UTC | #3
Hello,

On Tue, 4 Oct 2016 15:07:27 +0200, Mark Brown wrote:

> > /*
> >  * ...
> >  */  
> 
> I don't care, IIRC that's something from CodingStyle which checkpatch
> moans about.

Correct. The

/* ..
 * ..
 */

style is mandatory for net/ and crypto code, but not in the rest of the
kernel.

> > I'm probably missing something, but in the sun4i-codec.c driver, those
> > fields are initialized directly in the snd_soc_codec_driver structure,
> > not in the .component_driver sub-structure.  
> 
> We're in the process of pushing everything out to component level, this
> update should be made in the old code if it's not happened already.

OK.

> > > +	if (clk_prepare_enable(scodec->clk_module))
> > > +		pr_err("err:open failed;\n");  
> 
> > Grr, pr_err, not good. Plus you want to return with an error from the
> > probe() function.  
> 
> Also when printing an error message use dev_err().

That's why I said "Grr, pr_err, not good" :)

Thomas
Maxime Ripard Oct. 4, 2016, 4:09 p.m. UTC | #4
Hi,

On Tue, Oct 04, 2016 at 02:40:08PM +0200, Thomas Petazzoni wrote:
> > +	scodec->clk_apb = devm_clk_get(&pdev->dev, "apb");
> > +	if (IS_ERR(scodec->clk_apb)) {
> > +		dev_err(&pdev->dev, "Failed to get the apb clock\n");
> > +		return PTR_ERR(scodec->clk_apb);
> > +	}
> > +	if (clk_prepare_enable(scodec->clk_apb))
> > +		pr_err("err:open failed;\n");
> 
> Ditto. + unprepare/disable the previous clock.

Ideally, that would be even not be part of the runtime_pm
hooks. Ideally, that would be great if that driver supports it.

We'll have to go through all the drivers to support it, that would be
one less to do (and ASoC makes it very easy, you can have a look at
the sun4i-i2s driver).

Thanks!
Maxime
Maxime Ripard Oct. 4, 2016, 4:15 p.m. UTC | #5
Hi,

> +static const struct of_device_id sun8i_codec_of_match[] = {
> +	{ .compatible = "allwinner,sun8i-a33-codec" },
> +	{ .compatible = "allwinner,sun8i-a23-codec" },

I thought that the A23 and A33 had different codecs? In that case, it
wouldn't be a good assumption to make


> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sun8i_codec_of_match);
> +
> +static struct platform_driver sun8i_codec_driver = {
> +	.driver = {
> +		.name = "sun8i-codec",
> +		.owner = THIS_MODULE,
> +		.of_match_table = sun8i_codec_of_match,
> +	},
> +	.probe = sun8i_codec_probe,
> +	.remove = sun8i_codec_remove,
> +};
> +module_platform_driver(sun8i_codec_driver);
> +
> +MODULE_DESCRIPTION("Allwinner A33 (sun8i) codec driver");
> +MODULE_AUTHOR("huanxin<huanxin@reuuimllatech.com>");

Those obfuscated email adresses are not really helpful :)

Thanks,
Maxime
Mylene JOSSERAND Oct. 5, 2016, 11:54 a.m. UTC | #6
Hello,


On 04/10/2016 14:40, Thomas Petazzoni wrote:
> Hello,
>
> On Tue,  4 Oct 2016 11:46:19 +0200, Mylène Josserand wrote:
>> Add the digital sun8i audio codec which handles the base register
>> (without DAI).
>
> I'm not sure what you mean by "which handles the base register".

I wanted to explain that it is registers for audio codec and not PRCM 
ones. This is, maybe, unclear (and useless ?).

>
>> diff --git a/sound/soc/sunxi/Kconfig b/sound/soc/sunxi/Kconfig
>> index 7aee95a..9e287b0 100644
>> --- a/sound/soc/sunxi/Kconfig
>> +++ b/sound/soc/sunxi/Kconfig
>> @@ -27,6 +27,15 @@ config SND_SUN4I_SPDIF
>>  	  Say Y or M to add support for the S/PDIF audio block in the Allwinner
>>  	  A10 and affiliated SoCs.
>>
>> +config SND_SUN8I_CODEC
>> +	tristate "Allwinner SUN8I audio codec"
>> +	select REGMAP_MMIO
>> +        help
>
> Indentation issue here, it should be intended with one tab, not spaces.
>
> You probably also want a "depends on OF" here.

Yes, thanks !

>
>> +/* CODEC_OFFSET represents the offset of the codec registers
>> + * and not all the DAI registers
>> + */
>
> This is not the proper comment style I believe for audio code, it
> should be:
>
> /*
>  * ...
>  */
>
>> +#define CODEC_OFFSET				0x200
>
> Do you really need this CODEC_OFFSET macro? Why not simply use directly
> the right offsets? I.e instead of:
>
>   #define SUN8I_SYSCLK_CTL			(0x20c - CODEC_OFFSET)
>
> use:
>
>   #define SUN8I_SYSCLK_CTL			0xc

I thought it could be easier to find registers using offset but I guess 
that register's names are enough.

>
>> +#define CODEC_BASSADDRESS			0x01c22c00
>
> This define is not used anywhere.

Yes, sorry, I forgot to remove it.

>
>> +#define SUN8I_SYSCLK_CTL			(0x20c - CODEC_OFFSET)
>> +#define SUN8I_SYSCLK_CTL_AIF1CLK_ENA		(11)
>> +#define SUN8I_SYSCLK_CTL_SYSCLK_ENA		(3)
>> +#define SUN8I_SYSCLK_CTL_SYSCLK_SRC		(0)
>
> Parenthesis around single values are not really useful.
>
>> +#define SUN8I_MOD_CLK_ENA			(0x210 - CODEC_OFFSET)
>> +#define SUN8I_MOD_CLK_ENA_AIF1			(15)
>> +#define SUN8I_MOD_CLK_ENA_DAC			(2)
>> +#define SUN8I_MOD_RST_CTL			(0x214 - CODEC_OFFSET)
>> +#define SUN8I_MOD_RST_CTL_AIF1			(15)
>> +#define SUN8I_MOD_RST_CTL_DAC			(2)
>> +#define SUN8I_SYS_SR_CTRL			(0x218 - CODEC_OFFSET)
>> +#define SUN8I_SYS_SR_CTRL_AIF1_FS		(12)
>> +#define SUN8I_SYS_SR_CTRL_AIF2_FS		(8)
>> +#define SUN8I_AIF1CLK_CTRL			(0x240 - CODEC_OFFSET)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD	(15)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV	(14)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV	(13)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV	(9)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV	(6)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ	(4)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT	(2)
>> +#define SUN8I_AIF1_DACDAT_CTRL			(0x248 - CODEC_OFFSET)
>> +#define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0L_ENA	(15)
>> +#define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA	(14)
>> +#define SUN8I_DAC_DIG_CTRL			(0x320 - CODEC_OFFSET)
>> +#define SUN8I_DAC_DIG_CTRL_ENDA		(15)
>> +#define SUN8I_DAC_MXR_SRC			(0x330 - CODEC_OFFSET)
>> +#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF1DA0L (15)
>> +#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF1DA1L (14)
>> +#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF2DACL (13)
>> +#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_ADCL	(12)
>> +#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA0R (11)
>> +#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA1R (10)
>> +#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF2DACR (9)
>> +#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_ADCR	(8)
>
> Indentation of the value is not very clean for those last defines.
>
>> +static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>> +{
>> +	struct sun8i_codec *scodec = snd_soc_codec_get_drvdata(dai->codec);
>> +	unsigned long value;
>
> I'm not sure "unsigned long" is a very good choice here, it's going to
> be a 64 bits integer on 64 bits platform. I'd suggest to use "u32",
> which also seems to be what's used in _set_fmt() function of the
> sun4i-i2s.c driver.

Agreed, thanks !

>
>
>> +static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
>> +				 struct snd_pcm_hw_params *params,
>> +				 struct snd_soc_dai *dai)
>> +{
>> +	int rs_value  = 0;
>
> Two spaces before the = sign, not needed. Is the initialization to 0
> really needed? Also, this should be a u32.

ditto

>
>> +	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
>> +			   0x3 << SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ,
>
> Maybe a #define value to replace the hardcoded 0x3 ?
>
>> +			   rs_value << SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ);
>> +
>> +	/* calculate bclk_lrck_div Ratio */
>> +	bclk_lrck_div = sample_resolution * 2;
>> +	switch (bclk_lrck_div) {
>> +	case 16:
>> +		bclk_lrck_div = 0;
>> +		break;
>> +	case 32:
>> +		bclk_lrck_div = 1;
>> +		break;
>> +	case 64:
>> +		bclk_lrck_div = 2;
>> +		break;
>> +	case 128:
>> +		bclk_lrck_div = 3;
>> +		break;
>> +	case 256:
>> +		bclk_lrck_div = 4;
>> +		break;
>
> This could quite easily be replaced by a formula, if you don't care
> about error checking:
>
> 	bclk_lrck_div = log2(bclk_lrck_div) - 4;
>
> Of course, if you care about error checking, this switch is nicer.
>
>> +	default:
>
> So there's no error checking if the value is not supported?

You are right. I guess it should return -EINVAL.

[snip]

>
>
>> +static struct snd_soc_dai_driver sun8i_codec_dai = {
>> +	.name = "sun8i",
>> +	/* playback capabilities */
>> +	.playback = {
>> +		.stream_name = "Playback",
>> +		.channels_min = 1,
>> +		.channels_max = 2,
>> +		.rates = SNDRV_PCM_RATE_8000_192000 |
>> +			SNDRV_PCM_RATE_KNOT,
>> +		.formats = SNDRV_PCM_FMTBIT_S8 |
>> +			SNDRV_PCM_FMTBIT_S16_LE |
>> +			SNDRV_PCM_FMTBIT_S18_3LE |
>> +			SNDRV_PCM_FMTBIT_S20_3LE |
>> +			SNDRV_PCM_FMTBIT_S24_LE |
>> +			SNDRV_PCM_FMTBIT_S32_LE,
>> +	},
>> +	/* pcm operations */
>> +	.ops = &sun8i_codec_dai_ops,
>> +};
>> +EXPORT_SYMBOL(sun8i_codec_dai);
>
> This EXPORT_SYMBOL looks wrong. First because it doesn't seem to be
> used outside of this module. And second because using EXPORT_SYMBOL on
> a function defined as static doesn't make much sense, as the "static"
> qualifier limits the visibility of the symbol to the current
> compilation unit.
>

Yes, sorry, I missed it from the clean-up of the original driver.

[snip]

>> +static int sun8i_codec_probe(struct platform_device *pdev)
>> +{
>> +	struct resource *res_base;
>> +	struct sun8i_codec *scodec;
>> +	void __iomem *base;
>> +
>> +	scodec = devm_kzalloc(&pdev->dev, sizeof(*scodec), GFP_KERNEL);
>> +	if (!scodec)
>> +		return -ENOMEM;
>> +
>> +	scodec->dev = &pdev->dev;
>> +
>> +	/* Get the clocks from the DT */
>> +	scodec->clk_module = devm_clk_get(&pdev->dev, "codec");
>> +	if (IS_ERR(scodec->clk_module)) {
>> +		dev_err(&pdev->dev, "Failed to get the module clock\n");
>> +		return PTR_ERR(scodec->clk_module);
>> +	}
>> +	if (clk_prepare_enable(scodec->clk_module))
>> +		pr_err("err:open failed;\n");
>
> Grr, pr_err, not good. Plus you want to return with an error from the
> probe() function.

Oh, sorry for that ugly use :(

>
>> +
>> +	scodec->clk_apb = devm_clk_get(&pdev->dev, "apb");
>> +	if (IS_ERR(scodec->clk_apb)) {
>> +		dev_err(&pdev->dev, "Failed to get the apb clock\n");
>> +		return PTR_ERR(scodec->clk_apb);
>> +	}
>> +	if (clk_prepare_enable(scodec->clk_apb))
>> +		pr_err("err:open failed;\n");
>
> Ditto. + unprepare/disable the previous clock.

[snip]

ack, thank you for the review!
Alexandre Belloni Oct. 6, 2016, 6:23 p.m. UTC | #7
On 07/10/2016 at 00:06:57 +0800, Icenowy Zheng wrote :
> 05.10.2016, 00:20, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> > Hi,
> >
> >>  +static const struct of_device_id sun8i_codec_of_match[] = {
> >>  + { .compatible = "allwinner,sun8i-a33-codec" },
> >>  + { .compatible = "allwinner,sun8i-a23-codec" },
> >
> > I thought that the A23 and A33 had different codecs? In that case, it
> > wouldn't be a good assumption to make
> 
> Yes.
> 
> >
> >>  + {}
> >>  +};
> >>  +MODULE_DEVICE_TABLE(of, sun8i_codec_of_match);
> >>  +
> >>  +static struct platform_driver sun8i_codec_driver = {
> >>  + .driver = {
> >>  + .name = "sun8i-codec",
> >>  + .owner = THIS_MODULE,
> >>  + .of_match_table = sun8i_codec_of_match,
> >>  + },
> >>  + .probe = sun8i_codec_probe,
> >>  + .remove = sun8i_codec_remove,
> >>  +};
> >>  +module_platform_driver(sun8i_codec_driver);
> >>  +
> >>  +MODULE_DESCRIPTION("Allwinner A33 (sun8i) codec driver");
> >>  +MODULE_AUTHOR("huanxin<huanxin@reuuimllatech.com>");
> >
> > Those obfuscated email adresses are not really helpful :)
> 
> This kind of email addresses  are kept in many places in mainline kernel.
> 
> e.g. drivers/mmc/host/sunxi-mmc.c have 'Aaron Maoye <leafy.myeh@reuuimllatech.com>'
> 

Well, that is only one place and it is a comment, not in the
MODULE_AUTHOR macro. I would agree that it is not useful to have a stale
email address in MODULE_AUTHOR.
diff mbox

Patch

diff --git a/sound/soc/sunxi/Kconfig b/sound/soc/sunxi/Kconfig
index 7aee95a..9e287b0 100644
--- a/sound/soc/sunxi/Kconfig
+++ b/sound/soc/sunxi/Kconfig
@@ -27,6 +27,15 @@  config SND_SUN4I_SPDIF
 	  Say Y or M to add support for the S/PDIF audio block in the Allwinner
 	  A10 and affiliated SoCs.
 
+config SND_SUN8I_CODEC
+	tristate "Allwinner SUN8I audio codec"
+	select REGMAP_MMIO
+        help
+	  This option enables the digital part of the internal audio codec for
+	  Allwinner sun8i SoC.
+
+	  Say Y or M if you want to add sun8i digital audio codec support.
+
 config SND_SUN8I_CODEC_ANALOG
 	tristate "Allwinner SUN8I analog codec"
 	select REGMAP_MMIO
diff --git a/sound/soc/sunxi/Makefile b/sound/soc/sunxi/Makefile
index 241c0df..1da63d3 100644
--- a/sound/soc/sunxi/Makefile
+++ b/sound/soc/sunxi/Makefile
@@ -1,4 +1,5 @@ 
 obj-$(CONFIG_SND_SUN4I_CODEC) += sun4i-codec.o
 obj-$(CONFIG_SND_SUN4I_I2S) += sun4i-i2s.o
 obj-$(CONFIG_SND_SUN4I_SPDIF) += sun4i-spdif.o
+obj-$(CONFIG_SND_SUN8I_CODEC) += sun8i-codec.o
 obj-$(CONFIG_SND_SUN8I_CODEC_ANALOG) += sun8i-codec-analog.o
diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
new file mode 100644
index 0000000..e157f89
--- /dev/null
+++ b/sound/soc/sunxi/sun8i-codec.c
@@ -0,0 +1,492 @@ 
+/*
+ * This driver supports the digital controls for the internal codec
+ * found in Allwinner's A33 and A23 SoCs.
+ *
+ * (C) Copyright 2010-2016
+ * Reuuimlla Technology Co., Ltd. <www.reuuimllatech.com>
+ * huangxin <huangxin@Reuuimllatech.com>
+ * Mylène Josserand <mylene.josserand@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/regmap.h>
+
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/soc-dapm.h>
+
+/* CODEC_OFFSET represents the offset of the codec registers
+ * and not all the DAI registers
+ */
+#define CODEC_OFFSET				0x200
+#define CODEC_BASSADDRESS			0x01c22c00
+#define SUN8I_SYSCLK_CTL			(0x20c - CODEC_OFFSET)
+#define SUN8I_SYSCLK_CTL_AIF1CLK_ENA		(11)
+#define SUN8I_SYSCLK_CTL_SYSCLK_ENA		(3)
+#define SUN8I_SYSCLK_CTL_SYSCLK_SRC		(0)
+#define SUN8I_MOD_CLK_ENA			(0x210 - CODEC_OFFSET)
+#define SUN8I_MOD_CLK_ENA_AIF1			(15)
+#define SUN8I_MOD_CLK_ENA_DAC			(2)
+#define SUN8I_MOD_RST_CTL			(0x214 - CODEC_OFFSET)
+#define SUN8I_MOD_RST_CTL_AIF1			(15)
+#define SUN8I_MOD_RST_CTL_DAC			(2)
+#define SUN8I_SYS_SR_CTRL			(0x218 - CODEC_OFFSET)
+#define SUN8I_SYS_SR_CTRL_AIF1_FS		(12)
+#define SUN8I_SYS_SR_CTRL_AIF2_FS		(8)
+#define SUN8I_AIF1CLK_CTRL			(0x240 - CODEC_OFFSET)
+#define SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD	(15)
+#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV	(14)
+#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV	(13)
+#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV	(9)
+#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV	(6)
+#define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ	(4)
+#define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT	(2)
+#define SUN8I_AIF1_DACDAT_CTRL			(0x248 - CODEC_OFFSET)
+#define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0L_ENA	(15)
+#define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA	(14)
+#define SUN8I_DAC_DIG_CTRL			(0x320 - CODEC_OFFSET)
+#define SUN8I_DAC_DIG_CTRL_ENDA		(15)
+#define SUN8I_DAC_MXR_SRC			(0x330 - CODEC_OFFSET)
+#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF1DA0L (15)
+#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF1DA1L (14)
+#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF2DACL (13)
+#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_ADCL	(12)
+#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA0R (11)
+#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA1R (10)
+#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF2DACR (9)
+#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_ADCR	(8)
+
+struct sun8i_codec {
+	struct device	*dev;
+	struct regmap	*regmap;
+	struct clk	*clk_module;
+	struct clk	*clk_apb;
+};
+
+static int sun8i_codec_get_hw_rate(struct snd_pcm_hw_params *params)
+{
+	unsigned int rate = params_rate(params);
+
+	switch (rate) {
+	case 8000:
+	case 7350:
+		return 0x0;
+	case 11025:
+		return 0x1;
+	case 12000:
+		return 0x2;
+	case 16000:
+		return 0x3;
+	case 22050:
+		return 0x4;
+	case 24000:
+		return 0x5;
+	case 32000:
+		return 0x6;
+	case 44100:
+		return 0x7;
+	case 48000:
+		return 0x8;
+	case 96000:
+		return 0x9;
+	case 192000:
+		return 0xa;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+	struct sun8i_codec *scodec = snd_soc_codec_get_drvdata(dai->codec);
+	unsigned long value;
+
+	/* clock masters */
+	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
+	case SND_SOC_DAIFMT_CBS_CFS: /* DAI Slave */
+		value = 0x0; /* Codec Master */
+		break;
+	case SND_SOC_DAIFMT_CBM_CFM: /* DAI Master */
+		value = 0x1; /* Codec Slave */
+		break;
+	default:
+		return -EINVAL;
+	}
+	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
+			   BIT(SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD),
+			   value << SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD);
+
+	/* clock inversion */
+	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+	case SND_SOC_DAIFMT_NB_NF: /* Normal */
+		value = 0x0;
+		break;
+	case SND_SOC_DAIFMT_IB_IF: /* Inversion */
+		value = 0x1;
+		break;
+	default:
+		return -EINVAL;
+	}
+	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
+			   BIT(SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV),
+			   value << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV);
+	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
+			   BIT(SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV),
+			   value << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV);
+
+	/* DAI format */
+	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+		value = 0x0;
+		break;
+	case SND_SOC_DAIFMT_LEFT_J:
+		value = 0x1;
+		break;
+	case SND_SOC_DAIFMT_RIGHT_J:
+		value = 0x2;
+		break;
+	case SND_SOC_DAIFMT_DSP_A:
+	case SND_SOC_DAIFMT_DSP_B:
+		value = 0x3;
+		break;
+	default:
+		return -EINVAL;
+	}
+	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
+			   BIT(SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT),
+			   value << SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT);
+
+	return 0;
+}
+
+static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
+				 struct snd_pcm_hw_params *params,
+				 struct snd_soc_dai *dai)
+{
+	int rs_value  = 0;
+	u32 bclk_lrck_div = 0, sample_resolution;
+	int sample_rate = 0;
+	struct sun8i_codec *scodec = snd_soc_codec_get_drvdata(dai->codec);
+
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		sample_resolution = 16;
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+	case SNDRV_PCM_FORMAT_S24_LE:
+	case SNDRV_PCM_FORMAT_S32_LE:
+		sample_resolution = 24;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/*calculate word select bit*/
+	switch (sample_resolution) {
+	case 8:
+		rs_value = 0x0;
+		break;
+	case 16:
+		rs_value = 0x1;
+		break;
+	case 20:
+		rs_value = 0x2;
+		break;
+	case 24:
+		rs_value = 0x3;
+		break;
+	default:
+		break;
+	}
+	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
+			   0x3 << SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ,
+			   rs_value << SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ);
+
+	/* calculate bclk_lrck_div Ratio */
+	bclk_lrck_div = sample_resolution * 2;
+	switch (bclk_lrck_div) {
+	case 16:
+		bclk_lrck_div = 0;
+		break;
+	case 32:
+		bclk_lrck_div = 1;
+		break;
+	case 64:
+		bclk_lrck_div = 2;
+		break;
+	case 128:
+		bclk_lrck_div = 3;
+		break;
+	case 256:
+		bclk_lrck_div = 4;
+		break;
+	default:
+		break;
+	}
+	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
+			   0x7 << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV,
+			   bclk_lrck_div << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV);
+
+	sample_rate = sun8i_codec_get_hw_rate(params);
+	if (sample_rate < 0)
+		return sample_rate;
+
+	regmap_update_bits(scodec->regmap, SUN8I_SYS_SR_CTRL,
+			   0xf << SUN8I_SYS_SR_CTRL_AIF1_FS,
+			   sample_rate << SUN8I_SYS_SR_CTRL_AIF1_FS);
+	regmap_update_bits(scodec->regmap, SUN8I_SYS_SR_CTRL,
+			   0xf << SUN8I_SYS_SR_CTRL_AIF2_FS,
+			   sample_rate << SUN8I_SYS_SR_CTRL_AIF2_FS);
+
+	return 0;
+}
+
+static const struct snd_kcontrol_new sun8i_output_left_mixer_controls[] = {
+	SOC_DAPM_SINGLE("LSlot 0", SUN8I_DAC_MXR_SRC,
+			SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF1DA0L, 1, 0),
+	SOC_DAPM_SINGLE("LSlot 1", SUN8I_DAC_MXR_SRC,
+			SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF1DA1L, 1, 0),
+	SOC_DAPM_SINGLE("DACL", SUN8I_DAC_MXR_SRC,
+			SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF2DACL, 1, 0),
+	SOC_DAPM_SINGLE("ADCL", SUN8I_DAC_MXR_SRC,
+			SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_ADCL, 1, 0),
+};
+
+static const struct snd_kcontrol_new sun8i_output_right_mixer_controls[] = {
+	SOC_DAPM_SINGLE("RSlot 0", SUN8I_DAC_MXR_SRC,
+			SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA0R, 1, 0),
+	SOC_DAPM_SINGLE("RSlot 1", SUN8I_DAC_MXR_SRC,
+			SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA1R, 1, 0),
+	SOC_DAPM_SINGLE("DACR", SUN8I_DAC_MXR_SRC,
+			SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF2DACR, 1, 0),
+	SOC_DAPM_SINGLE("ADCR", SUN8I_DAC_MXR_SRC,
+			SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_ADCR, 1, 0),
+};
+
+static const struct snd_soc_dapm_widget sun8i_codec_dapm_widgets[] = {
+	/* Digital parts of the DACs */
+	SND_SOC_DAPM_SUPPLY("DAC", SUN8I_DAC_DIG_CTRL, SUN8I_DAC_DIG_CTRL_ENDA,
+			    0, NULL, 0),
+
+	/* Analog DAC */
+	SND_SOC_DAPM_DAC("Left DAC", "Playback", SUN8I_AIF1_DACDAT_CTRL,
+			 SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0L_ENA, 0),
+	SND_SOC_DAPM_DAC("Right DAC", "Playback", SUN8I_AIF1_DACDAT_CTRL,
+			 SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA, 0),
+
+	/* DAC Mixers */
+	SND_SOC_DAPM_MIXER("Left DAC Mixer", SND_SOC_NOPM, 0, 0,
+			   sun8i_output_left_mixer_controls,
+			   ARRAY_SIZE(sun8i_output_left_mixer_controls)),
+	SND_SOC_DAPM_MIXER("Right DAC Mixer", SND_SOC_NOPM, 0, 0,
+			   sun8i_output_right_mixer_controls,
+			   ARRAY_SIZE(sun8i_output_right_mixer_controls)),
+
+	/* Clocks */
+	SND_SOC_DAPM_SUPPLY("MODCLK AFI1", SUN8I_MOD_CLK_ENA,
+			    SUN8I_MOD_CLK_ENA_AIF1, 0, NULL, 0),
+	SND_SOC_DAPM_SUPPLY("MODCLK DAC", SUN8I_MOD_CLK_ENA,
+			    SUN8I_MOD_CLK_ENA_DAC, 0, NULL, 0),
+	SND_SOC_DAPM_SUPPLY("AIF1", SUN8I_SYSCLK_CTL,
+			    SUN8I_SYSCLK_CTL_AIF1CLK_ENA, 0, NULL, 0),
+	SND_SOC_DAPM_SUPPLY("SYSCLK", SUN8I_SYSCLK_CTL,
+			    SUN8I_SYSCLK_CTL_SYSCLK_ENA, 0, NULL, 0),
+
+	SND_SOC_DAPM_SUPPLY("AIF1 PLL", SUN8I_SYSCLK_CTL, 0x9, 0, NULL, 0),
+	/* Inversion as 0=AIF1, 1=AIF2 */
+	SND_SOC_DAPM_SUPPLY("SYSCLK AIF1", SUN8I_SYSCLK_CTL,
+			    SUN8I_SYSCLK_CTL_SYSCLK_SRC, 1, NULL, 0),
+
+	/* Module reset */
+	SND_SOC_DAPM_SUPPLY("RST AIF1", SUN8I_MOD_RST_CTL,
+			    SUN8I_MOD_RST_CTL_AIF1, 0, NULL, 0),
+	SND_SOC_DAPM_SUPPLY("RST DAC", SUN8I_MOD_RST_CTL,
+			    SUN8I_MOD_RST_CTL_DAC, 0, NULL, 0),
+
+	/* Headphone outputs */
+	SND_SOC_DAPM_OUTPUT("HPOUTL"),
+	SND_SOC_DAPM_OUTPUT("HPOUTR"),
+
+};
+
+static const struct snd_soc_dapm_route sun8i_codec_dapm_routes[] = {
+	/* Clock Routes */
+	{ "AIF1", NULL, "SYSCLK AIF1" },
+	{ "AIF1 PLL", NULL, "AIF1" },
+	{ "RST AIF1", NULL, "AIF1 PLL" },
+	{ "MODCLK AFI1", NULL, "RST AIF1" },
+	{ "DAC", NULL, "MODCLK AFI1" },
+
+	{ "RST DAC", NULL, "SYSCLK" },
+	{ "MODCLK DAC", NULL, "RST DAC" },
+	{ "DAC", NULL, "MODCLK DAC" },
+
+	/* DAC Routes */
+	{ "Left DAC", NULL, "DAC" },
+	{ "Right DAC", NULL, "DAC" },
+
+	/* DAC Mixer Routes */
+	{ "Left DAC Mixer", "LSlot 0", "Left DAC"},
+	{ "Right DAC Mixer", "RSlot 0", "Right DAC"},
+
+	/* End of route : HP out */
+	{ "HPOUTL", NULL, "Left DAC Mixer" },
+	{ "HPOUTR", NULL, "Right DAC Mixer" },
+};
+
+static struct snd_soc_dai_ops sun8i_codec_dai_ops = {
+	.hw_params = sun8i_codec_hw_params,
+	.set_fmt = sun8i_set_fmt,
+};
+
+static struct snd_soc_dai_driver sun8i_codec_dai = {
+	.name = "sun8i",
+	/* playback capabilities */
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_8000_192000 |
+			SNDRV_PCM_RATE_KNOT,
+		.formats = SNDRV_PCM_FMTBIT_S8 |
+			SNDRV_PCM_FMTBIT_S16_LE |
+			SNDRV_PCM_FMTBIT_S18_3LE |
+			SNDRV_PCM_FMTBIT_S20_3LE |
+			SNDRV_PCM_FMTBIT_S24_LE |
+			SNDRV_PCM_FMTBIT_S32_LE,
+	},
+	/* pcm operations */
+	.ops = &sun8i_codec_dai_ops,
+};
+EXPORT_SYMBOL(sun8i_codec_dai);
+
+static int sun8i_soc_probe(struct snd_soc_codec *codec)
+{
+	return 0;
+}
+
+/* power down chip */
+static int sun8i_soc_remove(struct snd_soc_codec *codec)
+{
+	return 0;
+}
+
+static struct snd_soc_codec_driver sun8i_soc_codec = {
+	.probe			= sun8i_soc_probe,
+	.remove		= sun8i_soc_remove,
+	.component_driver = {
+		.dapm_widgets		= sun8i_codec_dapm_widgets,
+		.num_dapm_widgets	= ARRAY_SIZE(sun8i_codec_dapm_widgets),
+		.dapm_routes		= sun8i_codec_dapm_routes,
+		.num_dapm_routes	= ARRAY_SIZE(sun8i_codec_dapm_routes),
+	},
+};
+
+static const struct regmap_config sun8i_codec_regmap_config = {
+	.reg_bits	= 32,
+	.reg_stride	= 4,
+	.val_bits	= 32,
+	.max_register	= SUN8I_DAC_MXR_SRC,
+};
+
+static int sun8i_codec_probe(struct platform_device *pdev)
+{
+	struct resource *res_base;
+	struct sun8i_codec *scodec;
+	void __iomem *base;
+
+	scodec = devm_kzalloc(&pdev->dev, sizeof(*scodec), GFP_KERNEL);
+	if (!scodec)
+		return -ENOMEM;
+
+	scodec->dev = &pdev->dev;
+
+	/* Get the clocks from the DT */
+	scodec->clk_module = devm_clk_get(&pdev->dev, "codec");
+	if (IS_ERR(scodec->clk_module)) {
+		dev_err(&pdev->dev, "Failed to get the module clock\n");
+		return PTR_ERR(scodec->clk_module);
+	}
+	if (clk_prepare_enable(scodec->clk_module))
+		pr_err("err:open failed;\n");
+
+	scodec->clk_apb = devm_clk_get(&pdev->dev, "apb");
+	if (IS_ERR(scodec->clk_apb)) {
+		dev_err(&pdev->dev, "Failed to get the apb clock\n");
+		return PTR_ERR(scodec->clk_apb);
+	}
+	if (clk_prepare_enable(scodec->clk_apb))
+		pr_err("err:open failed;\n");
+
+	/* Get base resources, registers and regmap */
+	res_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "audio");
+	base = devm_ioremap_resource(&pdev->dev, res_base);
+	if (IS_ERR(base)) {
+		dev_err(&pdev->dev, "Failed to map the registers\n");
+		return PTR_ERR(base);
+	}
+
+	scodec->regmap = devm_regmap_init_mmio(&pdev->dev, base,
+					       &sun8i_codec_regmap_config);
+	if (IS_ERR(scodec->regmap)) {
+		dev_err(&pdev->dev, "Failed to create our regmap\n");
+		return PTR_ERR(scodec->regmap);
+	}
+
+	/* Set the codec data as driver data */
+	dev_set_drvdata(&pdev->dev, scodec);
+
+	snd_soc_register_codec(&pdev->dev, &sun8i_soc_codec, &sun8i_codec_dai,
+			       1);
+
+	return 0;
+}
+
+static int sun8i_codec_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = platform_get_drvdata(pdev);
+	struct sun8i_codec *scodec = snd_soc_card_get_drvdata(card);
+
+	snd_soc_unregister_codec(&pdev->dev);
+	clk_disable_unprepare(scodec->clk_module);
+	clk_disable_unprepare(scodec->clk_apb);
+
+	return 0;
+}
+
+static const struct of_device_id sun8i_codec_of_match[] = {
+	{ .compatible = "allwinner,sun8i-a33-codec" },
+	{ .compatible = "allwinner,sun8i-a23-codec" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sun8i_codec_of_match);
+
+static struct platform_driver sun8i_codec_driver = {
+	.driver = {
+		.name = "sun8i-codec",
+		.owner = THIS_MODULE,
+		.of_match_table = sun8i_codec_of_match,
+	},
+	.probe = sun8i_codec_probe,
+	.remove = sun8i_codec_remove,
+};
+module_platform_driver(sun8i_codec_driver);
+
+MODULE_DESCRIPTION("Allwinner A33 (sun8i) codec driver");
+MODULE_AUTHOR("huanxin<huanxin@reuuimllatech.com>");
+MODULE_AUTHOR("Mylène Josserand <mylene.josserand@free-electrons.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:sun8i-codec");