diff mbox series

[23/25] ASoC: sun8i-codec: Generalize AIF clock control

Message ID 20201001021148.15852-24-samuel@sholland.org (mailing list archive)
State New, archived
Headers show
Series ASoC: sun8i-codec: support for AIF2 and AIF3 | expand

Commit Message

Samuel Holland Oct. 1, 2020, 2:11 a.m. UTC
The AIF clock control register has the same layout for all three AIFs.
The only difference between them is that AIF3 is missing some fields. We
can reuse the same register field definitions for all three registers,
and use the DAI ID to select the correct register address.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 sound/soc/sunxi/sun8i-codec.c | 64 +++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 30 deletions(-)

Comments

Maxime Ripard Oct. 5, 2020, 12:04 p.m. UTC | #1
Hi,

On Wed, Sep 30, 2020 at 09:11:46PM -0500, Samuel Holland wrote:
> The AIF clock control register has the same layout for all three AIFs.
> The only difference between them is that AIF3 is missing some fields. We
> can reuse the same register field definitions for all three registers,
> and use the DAI ID to select the correct register address.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  sound/soc/sunxi/sun8i-codec.c | 64 +++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 30 deletions(-)
> 
> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
> index 032a3f714dbb..1c34502ac47a 100644
> --- a/sound/soc/sunxi/sun8i-codec.c
> +++ b/sound/soc/sunxi/sun8i-codec.c
> @@ -37,23 +37,23 @@
>  #define SUN8I_MOD_CLK_ENA_DAC				2
>  #define SUN8I_MOD_RST_CTL				0x014
>  #define SUN8I_MOD_RST_CTL_AIF1				15
>  #define SUN8I_MOD_RST_CTL_ADC				3
>  #define SUN8I_MOD_RST_CTL_DAC				2
>  #define SUN8I_SYS_SR_CTRL				0x018
>  #define SUN8I_SYS_SR_CTRL_AIF1_FS			12
>  #define SUN8I_SYS_SR_CTRL_AIF2_FS			8
> -#define SUN8I_AIF1CLK_CTRL				0x040
> -#define SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD		15
> -#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_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_AIF_CLK_CTRL(n)				(0x040 * (1 + (n)))
> +#define SUN8I_AIF_CLK_CTRL_MSTR_MOD			15
> +#define SUN8I_AIF_CLK_CTRL_CLK_INV			13
> +#define SUN8I_AIF_CLK_CTRL_BCLK_DIV			9
> +#define SUN8I_AIF_CLK_CTRL_LRCK_DIV			6
> +#define SUN8I_AIF_CLK_CTRL_WORD_SIZ			4
> +#define SUN8I_AIF_CLK_CTRL_DATA_FMT			2
>  #define SUN8I_AIF1_ADCDAT_CTRL				0x044
>  #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0L_ENA		15
>  #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0R_ENA		14
>  #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0L_SRC		10
>  #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0R_SRC		8
>  #define SUN8I_AIF1_DACDAT_CTRL				0x048
>  #define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0L_ENA		15
>  #define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA		14
> @@ -83,21 +83,21 @@
>  #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
>  
>  #define SUN8I_SYSCLK_CTL_AIF1CLK_SRC_MASK	GENMASK(9, 8)
>  #define SUN8I_SYSCLK_CTL_AIF2CLK_SRC_MASK	GENMASK(5, 4)
>  #define SUN8I_SYS_SR_CTRL_AIF1_FS_MASK		GENMASK(15, 12)
>  #define SUN8I_SYS_SR_CTRL_AIF2_FS_MASK		GENMASK(11, 8)
> -#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_INV_MASK	GENMASK(14, 13)
> -#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK	GENMASK(12, 9)
> -#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK	GENMASK(8, 6)
> -#define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASK	GENMASK(5, 4)
> -#define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT_MASK	GENMASK(3, 2)
> +#define SUN8I_AIF_CLK_CTRL_CLK_INV_MASK		GENMASK(14, 13)
> +#define SUN8I_AIF_CLK_CTRL_BCLK_DIV_MASK	GENMASK(12, 9)
> +#define SUN8I_AIF_CLK_CTRL_LRCK_DIV_MASK	GENMASK(8, 6)
> +#define SUN8I_AIF_CLK_CTRL_WORD_SIZ_MASK	GENMASK(5, 4)
> +#define SUN8I_AIF_CLK_CTRL_DATA_FMT_MASK	GENMASK(3, 2)
>  
>  #define SUN8I_CODEC_PASSTHROUGH_SAMPLE_RATE 48000
>  
>  #define SUN8I_CODEC_PCM_FORMATS	(SNDRV_PCM_FMTBIT_S8     |\
>  				 SNDRV_PCM_FMTBIT_S16_LE |\
>  				 SNDRV_PCM_FMTBIT_S20_LE |\
>  				 SNDRV_PCM_FMTBIT_S24_LE |\
>  				 SNDRV_PCM_FMTBIT_S20_3LE|\
> @@ -223,32 +223,34 @@ static int sun8i_codec_update_sample_rate(struct sun8i_codec *scodec)
>  			   hw_rate << SUN8I_SYS_SR_CTRL_AIF1_FS);
>  
>  	return 0;
>  }
>  
>  static int sun8i_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>  {
>  	struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
> +	u32 reg = SUN8I_AIF_CLK_CTRL(dai->id);
>  	u32 format, invert, value;
>  
>  	/* clock masters */
>  	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>  	case SND_SOC_DAIFMT_CBS_CFS: /* Codec slave, DAI master */
>  		value = 0x1;
>  		break;
>  	case SND_SOC_DAIFMT_CBM_CFM: /* Codec Master, DAI slave */
>  		value = 0x0;
>  		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);
> +
> +	regmap_update_bits(scodec->regmap, reg,
> +			   BIT(SUN8I_AIF_CLK_CTRL_MSTR_MOD),
> +			   value << SUN8I_AIF_CLK_CTRL_MSTR_MOD);

I guess it would be more readable without the intermediate variable to
store the register.

With that fixed,
Acked-by: Maxime Ripard <mripard@kernel.org>

Maxime
Samuel Holland Oct. 6, 2020, 4:51 a.m. UTC | #2
On 10/5/20 7:04 AM, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Sep 30, 2020 at 09:11:46PM -0500, Samuel Holland wrote:
>> The AIF clock control register has the same layout for all three AIFs.
>> The only difference between them is that AIF3 is missing some fields. We
>> can reuse the same register field definitions for all three registers,
>> and use the DAI ID to select the correct register address.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  sound/soc/sunxi/sun8i-codec.c | 64 +++++++++++++++++++----------------
>>  1 file changed, 34 insertions(+), 30 deletions(-)
>>
>> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
>> index 032a3f714dbb..1c34502ac47a 100644
>> --- a/sound/soc/sunxi/sun8i-codec.c
>> +++ b/sound/soc/sunxi/sun8i-codec.c
>> @@ -37,23 +37,23 @@
>>  #define SUN8I_MOD_CLK_ENA_DAC				2
>>  #define SUN8I_MOD_RST_CTL				0x014
>>  #define SUN8I_MOD_RST_CTL_AIF1				15
>>  #define SUN8I_MOD_RST_CTL_ADC				3
>>  #define SUN8I_MOD_RST_CTL_DAC				2
>>  #define SUN8I_SYS_SR_CTRL				0x018
>>  #define SUN8I_SYS_SR_CTRL_AIF1_FS			12
>>  #define SUN8I_SYS_SR_CTRL_AIF2_FS			8
>> -#define SUN8I_AIF1CLK_CTRL				0x040
>> -#define SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD		15
>> -#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_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_AIF_CLK_CTRL(n)				(0x040 * (1 + (n)))
>> +#define SUN8I_AIF_CLK_CTRL_MSTR_MOD			15
>> +#define SUN8I_AIF_CLK_CTRL_CLK_INV			13
>> +#define SUN8I_AIF_CLK_CTRL_BCLK_DIV			9
>> +#define SUN8I_AIF_CLK_CTRL_LRCK_DIV			6
>> +#define SUN8I_AIF_CLK_CTRL_WORD_SIZ			4
>> +#define SUN8I_AIF_CLK_CTRL_DATA_FMT			2
>>  #define SUN8I_AIF1_ADCDAT_CTRL				0x044
>>  #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0L_ENA		15
>>  #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0R_ENA		14
>>  #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0L_SRC		10
>>  #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0R_SRC		8
>>  #define SUN8I_AIF1_DACDAT_CTRL				0x048
>>  #define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0L_ENA		15
>>  #define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA		14
>> @@ -83,21 +83,21 @@
>>  #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
>>  
>>  #define SUN8I_SYSCLK_CTL_AIF1CLK_SRC_MASK	GENMASK(9, 8)
>>  #define SUN8I_SYSCLK_CTL_AIF2CLK_SRC_MASK	GENMASK(5, 4)
>>  #define SUN8I_SYS_SR_CTRL_AIF1_FS_MASK		GENMASK(15, 12)
>>  #define SUN8I_SYS_SR_CTRL_AIF2_FS_MASK		GENMASK(11, 8)
>> -#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_INV_MASK	GENMASK(14, 13)
>> -#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK	GENMASK(12, 9)
>> -#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK	GENMASK(8, 6)
>> -#define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASK	GENMASK(5, 4)
>> -#define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT_MASK	GENMASK(3, 2)
>> +#define SUN8I_AIF_CLK_CTRL_CLK_INV_MASK		GENMASK(14, 13)
>> +#define SUN8I_AIF_CLK_CTRL_BCLK_DIV_MASK	GENMASK(12, 9)
>> +#define SUN8I_AIF_CLK_CTRL_LRCK_DIV_MASK	GENMASK(8, 6)
>> +#define SUN8I_AIF_CLK_CTRL_WORD_SIZ_MASK	GENMASK(5, 4)
>> +#define SUN8I_AIF_CLK_CTRL_DATA_FMT_MASK	GENMASK(3, 2)
>>  
>>  #define SUN8I_CODEC_PASSTHROUGH_SAMPLE_RATE 48000
>>  
>>  #define SUN8I_CODEC_PCM_FORMATS	(SNDRV_PCM_FMTBIT_S8     |\
>>  				 SNDRV_PCM_FMTBIT_S16_LE |\
>>  				 SNDRV_PCM_FMTBIT_S20_LE |\
>>  				 SNDRV_PCM_FMTBIT_S24_LE |\
>>  				 SNDRV_PCM_FMTBIT_S20_3LE|\
>> @@ -223,32 +223,34 @@ static int sun8i_codec_update_sample_rate(struct sun8i_codec *scodec)
>>  			   hw_rate << SUN8I_SYS_SR_CTRL_AIF1_FS);
>>  
>>  	return 0;
>>  }
>>  
>>  static int sun8i_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>>  {
>>  	struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
>> +	u32 reg = SUN8I_AIF_CLK_CTRL(dai->id);
>>  	u32 format, invert, value;
>>  
>>  	/* clock masters */
>>  	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>>  	case SND_SOC_DAIFMT_CBS_CFS: /* Codec slave, DAI master */
>>  		value = 0x1;
>>  		break;
>>  	case SND_SOC_DAIFMT_CBM_CFM: /* Codec Master, DAI slave */
>>  		value = 0x0;
>>  		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);
>> +
>> +	regmap_update_bits(scodec->regmap, reg,
>> +			   BIT(SUN8I_AIF_CLK_CTRL_MSTR_MOD),
>> +			   value << SUN8I_AIF_CLK_CTRL_MSTR_MOD);
> 
> I guess it would be more readable without the intermediate variable to
> store the register.

You mean you want me to call `SUN8I_AIF_CLK_CTRL(dai->id)` in each
regmap_update_bits call? I don't know that the compiler could optimize
that due to the pointer read. And how would you suggest handling patch
25, where sometimes AIF3 needs to write to AIF2's clock register?

Cheers,
Samuel

> With that fixed,
> Acked-by: Maxime Ripard <mripard@kernel.org>
> 
> Maxime
>
Maxime Ripard Oct. 12, 2020, 11:17 a.m. UTC | #3
On Mon, Oct 05, 2020 at 11:51:08PM -0500, Samuel Holland wrote:
> On 10/5/20 7:04 AM, Maxime Ripard wrote:
> > Hi,
> > 
> > On Wed, Sep 30, 2020 at 09:11:46PM -0500, Samuel Holland wrote:
> >> The AIF clock control register has the same layout for all three AIFs.
> >> The only difference between them is that AIF3 is missing some fields. We
> >> can reuse the same register field definitions for all three registers,
> >> and use the DAI ID to select the correct register address.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>  sound/soc/sunxi/sun8i-codec.c | 64 +++++++++++++++++++----------------
> >>  1 file changed, 34 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
> >> index 032a3f714dbb..1c34502ac47a 100644
> >> --- a/sound/soc/sunxi/sun8i-codec.c
> >> +++ b/sound/soc/sunxi/sun8i-codec.c
> >> @@ -37,23 +37,23 @@
> >>  #define SUN8I_MOD_CLK_ENA_DAC				2
> >>  #define SUN8I_MOD_RST_CTL				0x014
> >>  #define SUN8I_MOD_RST_CTL_AIF1				15
> >>  #define SUN8I_MOD_RST_CTL_ADC				3
> >>  #define SUN8I_MOD_RST_CTL_DAC				2
> >>  #define SUN8I_SYS_SR_CTRL				0x018
> >>  #define SUN8I_SYS_SR_CTRL_AIF1_FS			12
> >>  #define SUN8I_SYS_SR_CTRL_AIF2_FS			8
> >> -#define SUN8I_AIF1CLK_CTRL				0x040
> >> -#define SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD		15
> >> -#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_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_AIF_CLK_CTRL(n)				(0x040 * (1 + (n)))
> >> +#define SUN8I_AIF_CLK_CTRL_MSTR_MOD			15
> >> +#define SUN8I_AIF_CLK_CTRL_CLK_INV			13
> >> +#define SUN8I_AIF_CLK_CTRL_BCLK_DIV			9
> >> +#define SUN8I_AIF_CLK_CTRL_LRCK_DIV			6
> >> +#define SUN8I_AIF_CLK_CTRL_WORD_SIZ			4
> >> +#define SUN8I_AIF_CLK_CTRL_DATA_FMT			2
> >>  #define SUN8I_AIF1_ADCDAT_CTRL				0x044
> >>  #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0L_ENA		15
> >>  #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0R_ENA		14
> >>  #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0L_SRC		10
> >>  #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0R_SRC		8
> >>  #define SUN8I_AIF1_DACDAT_CTRL				0x048
> >>  #define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0L_ENA		15
> >>  #define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA		14
> >> @@ -83,21 +83,21 @@
> >>  #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
> >>  
> >>  #define SUN8I_SYSCLK_CTL_AIF1CLK_SRC_MASK	GENMASK(9, 8)
> >>  #define SUN8I_SYSCLK_CTL_AIF2CLK_SRC_MASK	GENMASK(5, 4)
> >>  #define SUN8I_SYS_SR_CTRL_AIF1_FS_MASK		GENMASK(15, 12)
> >>  #define SUN8I_SYS_SR_CTRL_AIF2_FS_MASK		GENMASK(11, 8)
> >> -#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_INV_MASK	GENMASK(14, 13)
> >> -#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK	GENMASK(12, 9)
> >> -#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK	GENMASK(8, 6)
> >> -#define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASK	GENMASK(5, 4)
> >> -#define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT_MASK	GENMASK(3, 2)
> >> +#define SUN8I_AIF_CLK_CTRL_CLK_INV_MASK		GENMASK(14, 13)
> >> +#define SUN8I_AIF_CLK_CTRL_BCLK_DIV_MASK	GENMASK(12, 9)
> >> +#define SUN8I_AIF_CLK_CTRL_LRCK_DIV_MASK	GENMASK(8, 6)
> >> +#define SUN8I_AIF_CLK_CTRL_WORD_SIZ_MASK	GENMASK(5, 4)
> >> +#define SUN8I_AIF_CLK_CTRL_DATA_FMT_MASK	GENMASK(3, 2)
> >>  
> >>  #define SUN8I_CODEC_PASSTHROUGH_SAMPLE_RATE 48000
> >>  
> >>  #define SUN8I_CODEC_PCM_FORMATS	(SNDRV_PCM_FMTBIT_S8     |\
> >>  				 SNDRV_PCM_FMTBIT_S16_LE |\
> >>  				 SNDRV_PCM_FMTBIT_S20_LE |\
> >>  				 SNDRV_PCM_FMTBIT_S24_LE |\
> >>  				 SNDRV_PCM_FMTBIT_S20_3LE|\
> >> @@ -223,32 +223,34 @@ static int sun8i_codec_update_sample_rate(struct sun8i_codec *scodec)
> >>  			   hw_rate << SUN8I_SYS_SR_CTRL_AIF1_FS);
> >>  
> >>  	return 0;
> >>  }
> >>  
> >>  static int sun8i_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> >>  {
> >>  	struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
> >> +	u32 reg = SUN8I_AIF_CLK_CTRL(dai->id);
> >>  	u32 format, invert, value;
> >>  
> >>  	/* clock masters */
> >>  	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> >>  	case SND_SOC_DAIFMT_CBS_CFS: /* Codec slave, DAI master */
> >>  		value = 0x1;
> >>  		break;
> >>  	case SND_SOC_DAIFMT_CBM_CFM: /* Codec Master, DAI slave */
> >>  		value = 0x0;
> >>  		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);
> >> +
> >> +	regmap_update_bits(scodec->regmap, reg,
> >> +			   BIT(SUN8I_AIF_CLK_CTRL_MSTR_MOD),
> >> +			   value << SUN8I_AIF_CLK_CTRL_MSTR_MOD);
> > 
> > I guess it would be more readable without the intermediate variable to
> > store the register.
> 
> You mean you want me to call `SUN8I_AIF_CLK_CTRL(dai->id)` in each
> regmap_update_bits call?

Yep

> I don't know that the compiler could optimize that due to the pointer
> read. And how would you suggest handling patch 25, where sometimes
> AIF3 needs to write to AIF2's clock register?

I'm not sure the compiler optimisation really matters anyway. It's not
really a fast path, and assuming it does the multiplication every time,
it will still be completely negligible compared to other operations done
in that function (starting with regmap_update_bits).

Maxime
diff mbox series

Patch

diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
index 032a3f714dbb..1c34502ac47a 100644
--- a/sound/soc/sunxi/sun8i-codec.c
+++ b/sound/soc/sunxi/sun8i-codec.c
@@ -37,23 +37,23 @@ 
 #define SUN8I_MOD_CLK_ENA_DAC				2
 #define SUN8I_MOD_RST_CTL				0x014
 #define SUN8I_MOD_RST_CTL_AIF1				15
 #define SUN8I_MOD_RST_CTL_ADC				3
 #define SUN8I_MOD_RST_CTL_DAC				2
 #define SUN8I_SYS_SR_CTRL				0x018
 #define SUN8I_SYS_SR_CTRL_AIF1_FS			12
 #define SUN8I_SYS_SR_CTRL_AIF2_FS			8
-#define SUN8I_AIF1CLK_CTRL				0x040
-#define SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD		15
-#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_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_AIF_CLK_CTRL(n)				(0x040 * (1 + (n)))
+#define SUN8I_AIF_CLK_CTRL_MSTR_MOD			15
+#define SUN8I_AIF_CLK_CTRL_CLK_INV			13
+#define SUN8I_AIF_CLK_CTRL_BCLK_DIV			9
+#define SUN8I_AIF_CLK_CTRL_LRCK_DIV			6
+#define SUN8I_AIF_CLK_CTRL_WORD_SIZ			4
+#define SUN8I_AIF_CLK_CTRL_DATA_FMT			2
 #define SUN8I_AIF1_ADCDAT_CTRL				0x044
 #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0L_ENA		15
 #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0R_ENA		14
 #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0L_SRC		10
 #define SUN8I_AIF1_ADCDAT_CTRL_AIF1_AD0R_SRC		8
 #define SUN8I_AIF1_DACDAT_CTRL				0x048
 #define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0L_ENA		15
 #define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA		14
@@ -83,21 +83,21 @@ 
 #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
 
 #define SUN8I_SYSCLK_CTL_AIF1CLK_SRC_MASK	GENMASK(9, 8)
 #define SUN8I_SYSCLK_CTL_AIF2CLK_SRC_MASK	GENMASK(5, 4)
 #define SUN8I_SYS_SR_CTRL_AIF1_FS_MASK		GENMASK(15, 12)
 #define SUN8I_SYS_SR_CTRL_AIF2_FS_MASK		GENMASK(11, 8)
-#define SUN8I_AIF1CLK_CTRL_AIF1_CLK_INV_MASK	GENMASK(14, 13)
-#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK	GENMASK(12, 9)
-#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK	GENMASK(8, 6)
-#define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASK	GENMASK(5, 4)
-#define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT_MASK	GENMASK(3, 2)
+#define SUN8I_AIF_CLK_CTRL_CLK_INV_MASK		GENMASK(14, 13)
+#define SUN8I_AIF_CLK_CTRL_BCLK_DIV_MASK	GENMASK(12, 9)
+#define SUN8I_AIF_CLK_CTRL_LRCK_DIV_MASK	GENMASK(8, 6)
+#define SUN8I_AIF_CLK_CTRL_WORD_SIZ_MASK	GENMASK(5, 4)
+#define SUN8I_AIF_CLK_CTRL_DATA_FMT_MASK	GENMASK(3, 2)
 
 #define SUN8I_CODEC_PASSTHROUGH_SAMPLE_RATE 48000
 
 #define SUN8I_CODEC_PCM_FORMATS	(SNDRV_PCM_FMTBIT_S8     |\
 				 SNDRV_PCM_FMTBIT_S16_LE |\
 				 SNDRV_PCM_FMTBIT_S20_LE |\
 				 SNDRV_PCM_FMTBIT_S24_LE |\
 				 SNDRV_PCM_FMTBIT_S20_3LE|\
@@ -223,32 +223,34 @@  static int sun8i_codec_update_sample_rate(struct sun8i_codec *scodec)
 			   hw_rate << SUN8I_SYS_SR_CTRL_AIF1_FS);
 
 	return 0;
 }
 
 static int sun8i_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 {
 	struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
+	u32 reg = SUN8I_AIF_CLK_CTRL(dai->id);
 	u32 format, invert, value;
 
 	/* clock masters */
 	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
 	case SND_SOC_DAIFMT_CBS_CFS: /* Codec slave, DAI master */
 		value = 0x1;
 		break;
 	case SND_SOC_DAIFMT_CBM_CFM: /* Codec Master, DAI slave */
 		value = 0x0;
 		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);
+
+	regmap_update_bits(scodec->regmap, reg,
+			   BIT(SUN8I_AIF_CLK_CTRL_MSTR_MOD),
+			   value << SUN8I_AIF_CLK_CTRL_MSTR_MOD);
 
 	/* DAI format */
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_I2S:
 		format = 0x0;
 		break;
 	case SND_SOC_DAIFMT_LEFT_J:
 		format = 0x1;
@@ -262,19 +264,20 @@  static int sun8i_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		break;
 	case SND_SOC_DAIFMT_DSP_B:
 		format = 0x3;
 		invert = 0x1; /* Set LRCK_INV to 1 */
 		break;
 	default:
 		return -EINVAL;
 	}
-	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
-			   SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT_MASK,
-			   format << SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT);
+
+	regmap_update_bits(scodec->regmap, reg,
+			   SUN8I_AIF_CLK_CTRL_DATA_FMT_MASK,
+			   format << SUN8I_AIF_CLK_CTRL_DATA_FMT);
 
 	/* clock inversion */
 	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
 	case SND_SOC_DAIFMT_NB_NF: /* Normal */
 		value = 0x0;
 		break;
 	case SND_SOC_DAIFMT_NB_IF: /* Inverted LRCK */
 		value = 0x1;
@@ -305,19 +308,19 @@  static int sun8i_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		 * 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->quirks->lrck_inversion;
 	}
 
-	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
-			   SUN8I_AIF1CLK_CTRL_AIF1_CLK_INV_MASK,
-			   value << SUN8I_AIF1CLK_CTRL_AIF1_CLK_INV);
+	regmap_update_bits(scodec->regmap, reg,
+			   SUN8I_AIF_CLK_CTRL_CLK_INV_MASK,
+			   value << SUN8I_AIF_CLK_CTRL_CLK_INV);
 
 	return 0;
 }
 
 static int sun8i_codec_set_tdm_slot(struct snd_soc_dai *dai,
 				    unsigned int tx_mask, unsigned int rx_mask,
 				    int slots, int slot_width)
 {
@@ -435,16 +438,17 @@  static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
 {
 	struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
 	struct sun8i_codec_aif *aif = &scodec->aifs[dai->id];
 	unsigned int sample_rate = params_rate(params);
 	unsigned int slots = aif->slots ?: params_channels(params);
 	unsigned int slot_width = aif->slot_width ?: params_width(params);
 	unsigned int sysclk_rate = sun8i_codec_get_sysclk_rate(sample_rate);
 	int bclk_div, lrck_div_order, ret, word_size;
+	u32 reg = SUN8I_AIF_CLK_CTRL(dai->id);
 
 	/* word size */
 	switch (params_width(params)) {
 	case 8:
 		word_size = 0x0;
 		break;
 	case 16:
 		word_size = 0x1;
@@ -454,37 +458,37 @@  static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
 		break;
 	case 24:
 		word_size = 0x3;
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
-			   SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_MASK,
-			   word_size << SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ);
+	regmap_update_bits(scodec->regmap, reg,
+			   SUN8I_AIF_CLK_CTRL_WORD_SIZ_MASK,
+			   word_size << SUN8I_AIF_CLK_CTRL_WORD_SIZ);
 
 	/* LRCK divider (BCLK/LRCK ratio) */
 	lrck_div_order = sun8i_codec_get_lrck_div_order(slots, slot_width);
 	if (lrck_div_order < 0)
 		return lrck_div_order;
 
-	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
-			   SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV_MASK,
-			   (lrck_div_order - 4) << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV);
+	regmap_update_bits(scodec->regmap, reg,
+			   SUN8I_AIF_CLK_CTRL_LRCK_DIV_MASK,
+			   (lrck_div_order - 4) << SUN8I_AIF_CLK_CTRL_LRCK_DIV);
 
 	/* BCLK divider (SYSCLK/BCLK ratio) */
 	bclk_div = sun8i_codec_get_bclk_div(sysclk_rate, lrck_div_order, sample_rate);
 	if (bclk_div < 0)
 		return bclk_div;
 
-	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
-			   SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK,
-			   bclk_div << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV);
+	regmap_update_bits(scodec->regmap, reg,
+			   SUN8I_AIF_CLK_CTRL_BCLK_DIV_MASK,
+			   bclk_div << SUN8I_AIF_CLK_CTRL_BCLK_DIV);
 
 	/* SYSCLK rate */
 	if (aif->open_streams) {
 		ret = clk_set_rate(scodec->clk_module, sysclk_rate);
 		if (ret < 0)
 			return ret;
 	} else {
 		ret = clk_set_rate_exclusive(scodec->clk_module, sysclk_rate);