Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
References: <20210505072607.3815442-1-narmstrong@baylibre.com>
 <20210505072607.3815442-4-narmstrong@baylibre.com>
 <a2d7df9e-5fb7-0c86-b0b0-2a5cdfab8823@baylibre.com>
User-agent: mu4e 1.4.15; emacs 27.1
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Neil Armstrong <narmstrong@baylibre.com>, broonie@kernel.org
Cc:     alsa-devel@alsa-project.org, linux-amlogic@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] sound: meson: g12a-toacodec: add support for SM1
 TOACODEC
In-reply-to: <a2d7df9e-5fb7-0c86-b0b0-2a5cdfab8823@baylibre.com>
Message-ID: <1jlf8rngc3.fsf@starbuckisacylon.baylibre.com>
Date:   Fri, 07 May 2021 09:38:52 +0200
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


On Wed 05 May 2021 at 09:27, Neil Armstrong <narmstrong@baylibre.com> wrote:

> On 05/05/2021 09:26, Neil Armstrong wrote:
>> This adds support for the TOACODEC found in Amlogic SM1 SoCs.
>> 
>> The bits are shifted for more selection of clock sources, so this only
>> maps the same support for G12A to the SM1 bits.
>> 
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  sound/soc/meson/g12a-toacodec.c | 64 ++++++++++++++++++++++++++++++++-
>>  1 file changed, 63 insertions(+), 1 deletion(-)
>> 
>> diff --git a/sound/soc/meson/g12a-toacodec.c b/sound/soc/meson/g12a-toacodec.c
>> index 2084c9542c9c..5437ecba9c47 100644
>> --- a/sound/soc/meson/g12a-toacodec.c
>> +++ b/sound/soc/meson/g12a-toacodec.c
>> @@ -21,13 +21,22 @@
>>  
>>  #define TOACODEC_CTRL0			0x0
>>  #define  CTRL0_ENABLE_SHIFT		31
>> +#define  CTRL0_DAT_SEL_SM1_MSB		19
>> +#define  CTRL0_DAT_SEL_SM1_LSB		18
>>  #define  CTRL0_DAT_SEL_MSB		15
>>  #define  CTRL0_DAT_SEL_LSB		14
>> +#define  CTRL0_LANE_SEL_SM1		16
>>  #define  CTRL0_LANE_SEL			12
>> +#define  CTRL0_LRCLK_SEL_SM1_MSB	14
>> +#define  CTRL0_LRCLK_SEL_SM1_LSB	12
>>  #define  CTRL0_LRCLK_SEL_MSB		9
>>  #define  CTRL0_LRCLK_SEL_LSB		8
>> +#define  CTRL0_LRCLK_INV_SM1		BIT(10)
>> +#define  CTRL0_BLK_CAP_INV_SM1		BIT(9)
>>  #define  CTRL0_BLK_CAP_INV		BIT(7)
>> +#define  CTRL0_BCLK_O_INV_SM1		BIT(8)
>>  #define  CTRL0_BCLK_O_INV		BIT(6)
>> +#define  CTRL0_BCLK_SEL_SM1_MSB		6
>>  #define  CTRL0_BCLK_SEL_MSB		5
>>  #define  CTRL0_BCLK_SEL_LSB		4
>>  #define  CTRL0_MCLK_SEL			GENMASK(2, 0)
>> @@ -41,6 +50,7 @@ struct g12a_toacodec {
>>  };
>>  
>>  struct g12a_toacodec_match_data {
>> +	const struct snd_soc_component_driver *component_drv;
>>  	struct reg_field field_dat_sel;
>>  	struct reg_field field_lrclk_sel;
>>  	struct reg_field field_bclk_sel;
>> @@ -98,11 +108,20 @@ static SOC_ENUM_SINGLE_DECL(g12a_toacodec_mux_enum, TOACODEC_CTRL0,
>>  			    CTRL0_DAT_SEL_LSB,
>>  			    g12a_toacodec_mux_texts);
>>  
>> +static SOC_ENUM_SINGLE_DECL(sm1_toacodec_mux_enum, TOACODEC_CTRL0,
>> +			    CTRL0_DAT_SEL_SM1_LSB,
>> +			    g12a_toacodec_mux_texts);
>> +
>>  static const struct snd_kcontrol_new g12a_toacodec_mux =
>>  	SOC_DAPM_ENUM_EXT("Source", g12a_toacodec_mux_enum,
>>  			  snd_soc_dapm_get_enum_double,
>>  			  g12a_toacodec_mux_put_enum);
>>  
>> +static const struct snd_kcontrol_new sm1_toacodec_mux =
>> +	SOC_DAPM_ENUM_EXT("Source", sm1_toacodec_mux_enum,
>> +			  snd_soc_dapm_get_enum_double,
>> +			  g12a_toacodec_mux_put_enum);
>> +
>>  static const struct snd_kcontrol_new g12a_toacodec_out_enable =
>>  	SOC_DAPM_SINGLE_AUTODISABLE("Switch", TOACODEC_CTRL0,
>>  				    CTRL0_ENABLE_SHIFT, 1, 0);
>> @@ -114,6 +133,13 @@ static const struct snd_soc_dapm_widget g12a_toacodec_widgets[] = {
>>  			    &g12a_toacodec_out_enable),
>>  };
>>  
>> +static const struct snd_soc_dapm_widget sm1_toacodec_widgets[] = {
>> +	SND_SOC_DAPM_MUX("SRC", SND_SOC_NOPM, 0, 0,
>> +			 &sm1_toacodec_mux),
>> +	SND_SOC_DAPM_SWITCH("OUT EN", SND_SOC_NOPM, 0, 0,
>> +			    &g12a_toacodec_out_enable),
>> +};
>> +
>>  static int g12a_toacodec_input_hw_params(struct snd_pcm_substream *substream,
>>  					 struct snd_pcm_hw_params *params,
>>  					 struct snd_soc_dai *dai)
>> @@ -184,6 +210,13 @@ static int g12a_toacodec_component_probe(struct snd_soc_component *c)
>>  				       CTRL0_BLK_CAP_INV);
>>  }
>>  
>> +static int sm1_toacodec_component_probe(struct snd_soc_component *c)
>> +{
>> +	/* Initialize the static clock parameters */
>> +	return snd_soc_component_write(c, TOACODEC_CTRL0,
>> +				       CTRL0_BLK_CAP_INV_SM1);
>> +}
>> +
>>  static const struct snd_soc_dapm_route g12a_toacodec_routes[] = {
>>  	{ "SRC", "I2S A", "IN A Playback" },
>>  	{ "SRC", "I2S B", "IN B Playback" },
>> @@ -196,6 +229,10 @@ static const struct snd_kcontrol_new g12a_toacodec_controls[] = {
>>  	SOC_SINGLE("Lane Select", TOACODEC_CTRL0, CTRL0_LANE_SEL, 3, 0),
>>  };
>>  
>> +static const struct snd_kcontrol_new sm1_toacodec_controls[] = {
>> +	SOC_SINGLE("Lane Select", TOACODEC_CTRL0, CTRL0_LANE_SEL_SM1, 3, 0),
>> +};
>> +
>>  static const struct snd_soc_component_driver g12a_toacodec_component_drv = {
>>  	.probe			= g12a_toacodec_component_probe,
>>  	.controls		= g12a_toacodec_controls,
>> @@ -208,6 +245,18 @@ static const struct snd_soc_component_driver g12a_toacodec_component_drv = {
>>  	.non_legacy_dai_naming	= 1,
>>  };
>>  
>> +static const struct snd_soc_component_driver sm1_toacodec_component_drv = {
>> +	.probe			= sm1_toacodec_component_probe,
>> +	.controls		= sm1_toacodec_controls,
>> +	.num_controls		= ARRAY_SIZE(sm1_toacodec_controls),
>> +	.dapm_widgets		= sm1_toacodec_widgets,
>> +	.num_dapm_widgets	= ARRAY_SIZE(sm1_toacodec_widgets),
>> +	.dapm_routes		= g12a_toacodec_routes,
>> +	.num_dapm_routes	= ARRAY_SIZE(g12a_toacodec_routes),
>> +	.endianness		= 1,
>> +	.non_legacy_dai_naming	= 1,
>> +};
>> +
>>  static const struct regmap_config g12a_toacodec_regmap_cfg = {
>>  	.reg_bits	= 32,
>>  	.val_bits	= 32,
>> @@ -215,16 +264,29 @@ static const struct regmap_config g12a_toacodec_regmap_cfg = {
>>  };
>>  
>>  static const struct g12a_toacodec_match_data g12a_toacodec_match_data = {
>> +	.component_drv	= &g12a_toacodec_component_drv,
>>  	.field_dat_sel	= REG_FIELD(TOACODEC_CTRL0, CTRL0_DAT_SEL_LSB, CTRL0_DAT_SEL_MSB),
>>  	.field_lrclk_sel = REG_FIELD(TOACODEC_CTRL0, CTRL0_LRCLK_SEL_LSB, CTRL0_LRCLK_SEL_MSB),
>>  	.field_bclk_sel	= REG_FIELD(TOACODEC_CTRL0, CTRL0_BCLK_SEL_LSB, CTRL0_BCLK_SEL_MSB),
>>  };
>>  
>> +static const struct g12a_toacodec_match_data sm1_toacodec_match_data = {
>> +	.component_drv	= &sm1_toacodec_component_drv,
>> +	.field_dat_sel	= REG_FIELD(TOACODEC_CTRL0, CTRL0_DAT_SEL_SM1_LSB, CTRL0_DAT_SEL_SM1_MSB),
>> +	.field_lrclk_sel = REG_FIELD(TOACODEC_CTRL0, CTRL0_LRCLK_SEL_SM1_LSB,
>> +				     CTRL0_LRCLK_SEL_SM1_MSB),
>> +	.field_bclk_sel	= REG_FIELD(TOACODEC_CTRL0, CTRL0_BCLK_SEL_LSB, CTRL0_BCLK_SEL_SM1_MSB),
>> +};

If you make another round, the LSB/MSB defines don't really help
readability here (and in the other patch)

Feel free to put the values directly.

>> +
>>  static const struct of_device_id g12a_toacodec_of_match[] = {
>>  	{
>>  		.compatible = "amlogic,g12a-toacodec",
>>  		.data = &g12a_toacodec_match_data,
>>  	},
>> +	{
>> +		.compatible = "amlogic,sm1-toacodec",
>> +		.data = &sm1_toacodec_match_data,
>> +	},
>>  	{}
>>  };
>>  MODULE_DEVICE_TABLE(of, g12a_toacodec_of_match);
>> @@ -278,7 +340,7 @@ static int g12a_toacodec_probe(struct platform_device *pdev)
>>  		return PTR_ERR(priv->field_bclk_sel);
>>  
>>  	return devm_snd_soc_register_component(dev,
>> -			&g12a_toacodec_component_drv, g12a_toacodec_dai_drv,
>> +			data->component_drv, g12a_toacodec_dai_drv,
>>  			ARRAY_SIZE(g12a_toacodec_dai_drv));
>>  }
>>  
>> 
>
> Oops, spurious one, please ignore it.
>
> Neil