[PATCH v2 1/3] dt-bindings: tlv320adcx140: Add GPIO config and drive config

[PATCH v2 1/3] dt-bindings: tlv320adcx140: Add GPIO config and drive config

[Camel Guo]

From: Camel Guo <camelg@axis.com>

Add properties for configuring the General Purpose Input Outputs (GPIO).
There are 2 settings for GPIO, configuration and the output drive type.

Signed-off-by: Camel Guo <camelg@axis.com>
---
 .../bindings/sound/tlv320adcx140.yaml         | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/tlv320adcx140.yaml b/Documentation/devicetree/bindings/sound/tlv320adcx140.yaml
index f578f17f3e04..7b0b4554da59 100644
--- a/Documentation/devicetree/bindings/sound/tlv320adcx140.yaml
+++ b/Documentation/devicetree/bindings/sound/tlv320adcx140.yaml
@@ -134,6 +134,49 @@ patternProperties:
        4d - Drive weak low and active high
        5d - Drive Hi-Z and active high
 
+  ti,gpio-config:
+    description: |
+       Defines the configuration and output driver for the general purpose
+       input and output pin (GPIO1). Its value is a pair, the first value is for
+       the configuration type and the second value is for the output drive
+       type. The array is defined as <GPIO1_CFG GPIO1_DRV>
+
+       configuration for the GPIO pin can be one of the following:
+       0 - disabled
+       1 - GPIO1 is configured as a general-purpose output (GPO)
+       2 - (default) GPIO1 is configured as a device interrupt output (IRQ)
+       3 - GPIO1 is configured as a secondary ASI output (SDOUT2)
+       4 - GPIO1 is configured as a PDM clock output (PDMCLK)
+       8 - GPIO1 is configured as an input to control when MICBIAS turns on or
+           off (MICBIAS_EN)
+       9 - GPIO1 is configured as a general-purpose input (GPI)
+       10 - GPIO1 is configured as a master clock input (MCLK)
+       11 - GPIO1 is configured as an ASI input for daisy-chain (SDIN)
+       12 - GPIO1 is configured as a PDM data input for channel 1 and channel 2
+            (PDMDIN1)
+       13 - GPIO1 is configured as a PDM data input for channel 3 and channel 4
+            (PDMDIN2)
+       14 - GPIO1 is configured as a PDM data input for channel 5 and channel 6
+            (PDMDIN3)
+       15 - GPIO1 is configured as a PDM data input for channel 7 and channel 8
+            (PDMDIN4)
+
+       output drive type for the GPIO pin can be one of the following:
+       0 - Hi-Z output
+       1 - Drive active low and active high
+       2 - (default) Drive active low and weak high
+       3 - Drive active low and Hi-Z
+       4 - Drive weak low and active high
+       5 - Drive Hi-Z and active high
+
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32-array
+      - minItems: 2
+        maxItems: 2
+        items:
+          maximum: 15
+        default: [2, 2]
+
 required:
   - compatible
   - reg
@@ -150,6 +193,7 @@ examples:
         ti,mic-bias-source = <6>;
         ti,pdm-edge-select = <0 1 0 1>;
         ti,gpi-config = <4 5 6 7>;
+        ti,gpio-config = <10 2>;
         ti,gpo-config-1 = <0 0>;
         ti,gpo-config-2 = <0 0>;
         reset-gpios = <&gpio0 14 GPIO_ACTIVE_HIGH>;
-- 
2.20.1

[PATCH v2 2/3] ASoC: tlv320adcx140: Add support for configuring GPIO pin

[Camel Guo]

From: Camel Guo <camelg@axis.com>

Add support to configure the GPIO pin to the specific configuration.
The GPIO pin can be configured as GPO, IRQ, SDOUT2, PDMCLK, MICBASE_EN,
GPI, MCLK, SDIN, PDMDIN1, PDMDIN2, PDMDIN3 or PDMDIN4 and the output
drive can be configured with various configuration.

Signed-off-by: Camel Guo <camelg@axis.com>
---
 sound/soc/codecs/tlv320adcx140.c | 44 ++++++++++++++++++++++++++++++++
 sound/soc/codecs/tlv320adcx140.h |  4 +++
 2 files changed, 48 insertions(+)

diff --git a/sound/soc/codecs/tlv320adcx140.c b/sound/soc/codecs/tlv320adcx140.c
index f33ee604ee78..97f16fbba441 100644
--- a/sound/soc/codecs/tlv320adcx140.c
+++ b/sound/soc/codecs/tlv320adcx140.c
@@ -837,6 +837,46 @@ static int adcx140_configure_gpo(struct adcx140_priv *adcx140)
 
 }
 
+static int adcx140_configure_gpio(struct adcx140_priv *adcx140)
+{
+	int gpio_count = 0;
+	u32 gpio_outputs[2];
+	u32 gpio_output_val = 0;
+	int ret;
+
+	gpio_count = device_property_count_u32(adcx140->dev,
+			"ti,gpio-config");
+	if (gpio_count == 0)
+		return 0;
+
+	if (gpio_count != 2)
+		return -EINVAL;
+
+	ret = device_property_read_u32_array(adcx140->dev, "ti,gpio-config",
+			gpio_outputs, gpio_count);
+	if (ret)
+		return ret;
+
+	if (gpio_outputs[0] > ADCX140_GPIO_CFG_MAX) {
+		dev_err(adcx140->dev, "GPIO config out of range\n");
+		return -EINVAL;
+	}
+
+	if (gpio_outputs[1] > ADCX140_GPIO_DRV_MAX) {
+		dev_err(adcx140->dev, "GPIO drive out of range\n");
+		return -EINVAL;
+	}
+
+	gpio_output_val = gpio_outputs[0] << ADCX140_GPIO_SHIFT
+		| gpio_outputs[1];
+
+	ret = regmap_write(adcx140->regmap, ADCX140_GPIO_CFG0, gpio_output_val);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int adcx140_codec_probe(struct snd_soc_component *component)
 {
 	struct adcx140_priv *adcx140 = snd_soc_component_get_drvdata(component);
@@ -934,6 +974,10 @@ static int adcx140_codec_probe(struct snd_soc_component *component)
 			return ret;
 	}
 
+	ret = adcx140_configure_gpio(adcx140);
+	if (ret)
+		return ret;
+
 	ret = adcx140_configure_gpo(adcx140);
 	if (ret)
 		goto out;
diff --git a/sound/soc/codecs/tlv320adcx140.h b/sound/soc/codecs/tlv320adcx140.h
index eedbc1d7221f..96f067e65e2a 100644
--- a/sound/soc/codecs/tlv320adcx140.h
+++ b/sound/soc/codecs/tlv320adcx140.h
@@ -145,4 +145,8 @@
 #define ADCX140_GPO_CFG_MAX		4
 #define ADCX140_GPO_DRV_MAX		5
 
+#define ADCX140_GPIO_SHIFT		4
+#define ADCX140_GPIO_CFG_MAX		15
+#define ADCX140_GPIO_DRV_MAX		5
+
 #endif /* _TLV320ADCX140_ */
-- 
2.20.1

[PATCH v2 3/3] ASoC: tlv320adcx140: Add proper support for master mode

[Camel Guo]

From: Camel Guo <camelg@axis.com>

Add setup of bclk-to-ws ratio and sample rate when in master mode,
as well as MCLK input pin setup.

Signed-off-by: Camel Guo <camelg@axis.com>
---
 v2:
  - Move GPIO setting into devicetree
  - Move master config register setting into a new function

 sound/soc/codecs/tlv320adcx140.c | 139 ++++++++++++++++++++++++++++++-
 sound/soc/codecs/tlv320adcx140.h |  27 ++++++
 2 files changed, 162 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/tlv320adcx140.c b/sound/soc/codecs/tlv320adcx140.c
index 97f16fbba441..685f5fd8b537 100644
--- a/sound/soc/codecs/tlv320adcx140.c
+++ b/sound/soc/codecs/tlv320adcx140.c
@@ -35,6 +35,7 @@ struct adcx140_priv {
 	unsigned int dai_fmt;
 	unsigned int tdm_delay;
 	unsigned int slot_width;
+	bool master;
 };
 
 static const char * const gpo_config_names[] = {
@@ -651,11 +652,136 @@ static int adcx140_reset(struct adcx140_priv *adcx140)
 	return ret;
 }
 
+static int adcx140_fs_bclk_ratio(unsigned int bclk_ratio)
+{
+	switch (bclk_ratio) {
+	case 16:
+		return ADCX140_RATIO_16;
+	case 24:
+		return ADCX140_RATIO_24;
+	case 32:
+		return ADCX140_RATIO_32;
+	case 48:
+		return ADCX140_RATIO_48;
+	case 64:
+		return ADCX140_RATIO_64;
+	case 96:
+		return ADCX140_RATIO_96;
+	case 128:
+		return ADCX140_RATIO_128;
+	case 192:
+		return ADCX140_RATIO_192;
+	case 256:
+		return ADCX140_RATIO_256;
+	case 384:
+		return ADCX140_RATIO_384;
+	case 512:
+		return ADCX140_RATIO_512;
+	case 1024:
+		return ADCX140_RATIO_1024;
+	case 2048:
+		return ADCX140_RATIO_2048;
+	default:
+		break;
+	}
+	return -EINVAL;
+}
+
+static int adcx140_fs_rate(unsigned int rate)
+{
+	switch (rate) {
+	case 7350:
+	case 8000:
+		return ADCX140_8_OR_7_35KHZ;
+	case 14700:
+	case 16000:
+		return ADCX140_16_OR_14_7KHZ;
+	case 22050:
+	case 24000:
+		return ADCX140_24_OR_22_05KHZ;
+	case 29400:
+	case 32000:
+		return ADCX140_32_OR_29_4KHZ;
+	case 44100:
+	case 48000:
+		return ADCX140_48_OR_44_1KHZ;
+	case 88200:
+	case 96000:
+		return ADCX140_96_OR_88_2KHZ;
+	case 176400:
+	case 192000:
+		return ADCX140_192_OR_176_4KHZ;
+	case 352800:
+	case 384000:
+		return ADCX140_384_OR_352_8KHZ;
+	case 705600:
+	case 768000:
+		return ADCX140_768_OR_705_6KHZ;
+	default:
+		break;
+	}
+	return -EINVAL;
+}
+
+static int adcx140_setup_master_config(struct snd_soc_component *component,
+				       struct snd_pcm_hw_params *params)
+{
+	int ret = 0;
+	struct adcx140_priv *adcx140 = snd_soc_component_get_drvdata(component);
+
+	if (adcx140->master) {
+		u8 mst_cfg1 = 0;
+		u8 mst_cfg0 = 0;
+		unsigned int bclk_ratio;
+
+		mst_cfg0 = ADCX140_BCLK_FSYNC_MASTER;
+		if (params_rate(params) % 1000)
+			mst_cfg0 |= ADCX140_FSYNCINV_BIT; /* 44.1 kHz et al */
+
+		ret = adcx140_fs_rate(params_rate(params));
+		if (ret < 0) {
+			dev_err(adcx140->dev, "%s: Unsupported rate %d\n",
+					__func__, params_rate(params));
+			return ret;
+		}
+		mst_cfg1 |= ret;
+
+		/* In slave mode when using automatic clock configuration,
+		 * the codec figures out the BCLK to FSYNC ratio itself. But
+		 * here in master mode, we need to tell it.
+		 */
+
+		bclk_ratio = snd_soc_params_to_frame_size(params);
+		ret = adcx140_fs_bclk_ratio(bclk_ratio);
+		if (ret < 0) {
+			dev_err(adcx140->dev, "%s: Unsupported bclk_ratio %d\n",
+					__func__, bclk_ratio);
+			return ret;
+		}
+		mst_cfg1 |= ret;
+
+		snd_soc_component_update_bits(component, ADCX140_MST_CFG1,
+				ADCX140_FS_RATE_MSK |
+				ADCX140_RATIO_MSK,
+				mst_cfg1);
+
+		snd_soc_component_update_bits(component, ADCX140_MST_CFG0,
+				ADCX140_FSYNCINV_BIT |
+				ADCX140_BCLK_FSYNC_MASTER,
+				mst_cfg0);
+
+	}
+
+	return ret;
+}
+
+
 static int adcx140_hw_params(struct snd_pcm_substream *substream,
 			     struct snd_pcm_hw_params *params,
 			     struct snd_soc_dai *dai)
 {
 	struct snd_soc_component *component = dai->component;
+	int ret = 0;
 	u8 data = 0;
 
 	switch (params_width(params)) {
@@ -677,6 +803,13 @@ static int adcx140_hw_params(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
+	ret = adcx140_setup_master_config(component, params);
+	if (ret < 0) {
+		dev_err(component->dev, "%s: Failed to set up master config\n",
+			__func__);
+		return ret;
+	}
+
 	snd_soc_component_update_bits(component, ADCX140_ASI_CFG0,
 			    ADCX140_WORD_LEN_MSK, data);
 
@@ -689,16 +822,16 @@ static int adcx140_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	struct snd_soc_component *component = codec_dai->component;
 	struct adcx140_priv *adcx140 = snd_soc_component_get_drvdata(component);
 	u8 iface_reg1 = 0;
-	u8 iface_reg2 = 0;
 	int offset = 0;
 	int width = adcx140->slot_width;
 
 	/* set master/slave audio interface */
 	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
 	case SND_SOC_DAIFMT_CBM_CFM:
-		iface_reg2 |= ADCX140_BCLK_FSYNC_MASTER;
+		adcx140->master = true;
 		break;
 	case SND_SOC_DAIFMT_CBS_CFS:
+		adcx140->master = false;
 		break;
 	case SND_SOC_DAIFMT_CBS_CFM:
 	case SND_SOC_DAIFMT_CBM_CFS:
@@ -751,8 +884,6 @@ static int adcx140_set_dai_fmt(struct snd_soc_dai *codec_dai,
 				      ADCX140_BCLKINV_BIT |
 				      ADCX140_ASI_FORMAT_MSK,
 				      iface_reg1);
-	snd_soc_component_update_bits(component, ADCX140_MST_CFG0,
-				      ADCX140_BCLK_FSYNC_MASTER, iface_reg2);
 
 	/* Configure data offset */
 	snd_soc_component_update_bits(component, ADCX140_ASI_CFG1,
diff --git a/sound/soc/codecs/tlv320adcx140.h b/sound/soc/codecs/tlv320adcx140.h
index 96f067e65e2a..1fbb7fa3c73d 100644
--- a/sound/soc/codecs/tlv320adcx140.h
+++ b/sound/soc/codecs/tlv320adcx140.h
@@ -149,4 +149,31 @@
 #define ADCX140_GPIO_CFG_MAX		15
 #define ADCX140_GPIO_DRV_MAX		5
 
+/* MST_CFG1 */
+#define ADCX140_8_OR_7_35KHZ	(0 << 4)
+#define ADCX140_16_OR_14_7KHZ	(1 << 4)
+#define ADCX140_24_OR_22_05KHZ	(2 << 4)
+#define ADCX140_32_OR_29_4KHZ	(3 << 4)
+#define ADCX140_48_OR_44_1KHZ	(4 << 4)
+#define ADCX140_96_OR_88_2KHZ	(5 << 4)
+#define ADCX140_192_OR_176_4KHZ	(6 << 4)
+#define ADCX140_384_OR_352_8KHZ	(7 << 4)
+#define ADCX140_768_OR_705_6KHZ	(8 << 4)
+#define ADCX140_FS_RATE_MSK	GENMASK(7, 4)
+
+#define ADCX140_RATIO_16	0
+#define ADCX140_RATIO_24	1
+#define ADCX140_RATIO_32	2
+#define ADCX140_RATIO_48	3
+#define ADCX140_RATIO_64	4
+#define ADCX140_RATIO_96	5
+#define ADCX140_RATIO_128	6
+#define ADCX140_RATIO_192	7
+#define ADCX140_RATIO_256	8
+#define ADCX140_RATIO_384	9
+#define ADCX140_RATIO_512	10
+#define ADCX140_RATIO_1024	11
+#define ADCX140_RATIO_2048	12
+#define ADCX140_RATIO_MSK	GENMASK(3, 0)
+
 #endif /* _TLV320ADCX140_ */
-- 
2.20.1

Re: [PATCH v2 1/3] dt-bindings: tlv320adcx140: Add GPIO config and drive config

[Dan Murphy]

Camel

On 9/11/20 3:07 AM, Camel Guo wrote:
> From: Camel Guo <camelg@axis.com>
>
> Add properties for configuring the General Purpose Input Outputs (GPIO).

s/Outputs/Output

There is only one for the x140

> There are 2 settings for GPIO, configuration and the output drive type.
>
> Signed-off-by: Camel Guo <camelg@axis.com>
> ---
>   .../bindings/sound/tlv320adcx140.yaml         | 44 +++++++++++++++++++
>   1 file changed, 44 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/sound/tlv320adcx140.yaml b/Documentation/devicetree/bindings/sound/tlv320adcx140.yaml
> index f578f17f3e04..7b0b4554da59 100644
> --- a/Documentation/devicetree/bindings/sound/tlv320adcx140.yaml
> +++ b/Documentation/devicetree/bindings/sound/tlv320adcx140.yaml
> @@ -134,6 +134,49 @@ patternProperties:
>          4d - Drive weak low and active high
>          5d - Drive Hi-Z and active high
>   
> +  ti,gpio-config:
> +    description: |
> +       Defines the configuration and output driver for the general purpose

s/driver/drive

You capitalized the General Purpose Input and Output in the commit 
message but kept it lower case here.

Beyond these

Acked-by: Dan Murphy <dmurphy@ti.com>

Re: [PATCH v2 2/3] ASoC: tlv320adcx140: Add support for configuring GPIO pin

[Dan Murphy]

Camel

On 9/11/20 3:07 AM, Camel Guo wrote:
> From: Camel Guo <camelg@axis.com>
>
> Add support to configure the GPIO pin to the specific configuration.
> The GPIO pin can be configured as GPO, IRQ, SDOUT2, PDMCLK, MICBASE_EN,
> GPI, MCLK, SDIN, PDMDIN1, PDMDIN2, PDMDIN3 or PDMDIN4 and the output
> drive can be configured with various configuration.
>
> Signed-off-by: Camel Guo <camelg@axis.com>
> ---
>   sound/soc/codecs/tlv320adcx140.c | 44 ++++++++++++++++++++++++++++++++
>   sound/soc/codecs/tlv320adcx140.h |  4 +++
>   2 files changed, 48 insertions(+)
>
> diff --git a/sound/soc/codecs/tlv320adcx140.c b/sound/soc/codecs/tlv320adcx140.c
> index f33ee604ee78..97f16fbba441 100644
> --- a/sound/soc/codecs/tlv320adcx140.c
> +++ b/sound/soc/codecs/tlv320adcx140.c
> @@ -837,6 +837,46 @@ static int adcx140_configure_gpo(struct adcx140_priv *adcx140)
>   
>   }
>   
> +static int adcx140_configure_gpio(struct adcx140_priv *adcx140)
> +{
> +	int gpio_count = 0;
> +	u32 gpio_outputs[2];

This is #defined in configure_gpo and configure_gpi would like to see 
the consistency here.

> +	u32 gpio_output_val = 0;
> +	int ret;
> +
> +	gpio_count = device_property_count_u32(adcx140->dev,
> +			"ti,gpio-config");
> +	if (gpio_count == 0)
> +		return 0;
> +
> +	if (gpio_count != 2)
Same comment as above.
> +		return -EINVAL;
> +
> +	ret = device_property_read_u32_array(adcx140->dev, "ti,gpio-config",
> +			gpio_outputs, gpio_count);
> +	if (ret)
> +		return ret;
> +
> +	if (gpio_outputs[0] > ADCX140_GPIO_CFG_MAX) {
> +		dev_err(adcx140->dev, "GPIO config out of range\n");
> +		return -EINVAL;
> +	}
> +
> +	if (gpio_outputs[1] > ADCX140_GPIO_DRV_MAX) {
> +		dev_err(adcx140->dev, "GPIO drive out of range\n");
> +		return -EINVAL;
> +	}
> +
> +	gpio_output_val = gpio_outputs[0] << ADCX140_GPIO_SHIFT
> +		| gpio_outputs[1];
> +
> +	ret = regmap_write(adcx140->regmap, ADCX140_GPIO_CFG0, gpio_output_val);
> +	if (ret)
> +		return ret;

Just do return regmap_write no reason to check it. It is checked by the 
caller.


Dan

Re: [PATCH v2 3/3] ASoC: tlv320adcx140: Add proper support for master mode

[Dan Murphy]

Camel

On 9/11/20 3:07 AM, Camel Guo wrote:
> From: Camel Guo <camelg@axis.com>
>
> Add setup of bclk-to-ws ratio and sample rate when in master mode,
> as well as MCLK input pin setup.
>
> Signed-off-by: Camel Guo <camelg@axis.com>
> ---
>   v2:
>    - Move GPIO setting into devicetree
>    - Move master config register setting into a new function
>
>   sound/soc/codecs/tlv320adcx140.c | 139 ++++++++++++++++++++++++++++++-
>   sound/soc/codecs/tlv320adcx140.h |  27 ++++++
>   2 files changed, 162 insertions(+), 4 deletions(-)
>
> diff --git a/sound/soc/codecs/tlv320adcx140.c b/sound/soc/codecs/tlv320adcx140.c
> index 97f16fbba441..685f5fd8b537 100644
> --- a/sound/soc/codecs/tlv320adcx140.c
> +++ b/sound/soc/codecs/tlv320adcx140.c
> @@ -35,6 +35,7 @@ struct adcx140_priv {
>   	unsigned int dai_fmt;
>   	unsigned int tdm_delay;
>   	unsigned int slot_width;
> +	bool master;
>   };
>   
>   static const char * const gpo_config_names[] = {
> @@ -651,11 +652,136 @@ static int adcx140_reset(struct adcx140_priv *adcx140)
>   	return ret;
>   }
>   
> +static int adcx140_fs_bclk_ratio(unsigned int bclk_ratio)
> +{
> +	switch (bclk_ratio) {
> +	case 16:
> +		return ADCX140_RATIO_16;
> +	case 24:
> +		return ADCX140_RATIO_24;
> +	case 32:
> +		return ADCX140_RATIO_32;
> +	case 48:
> +		return ADCX140_RATIO_48;
> +	case 64:
> +		return ADCX140_RATIO_64;
> +	case 96:
> +		return ADCX140_RATIO_96;
> +	case 128:
> +		return ADCX140_RATIO_128;
> +	case 192:
> +		return ADCX140_RATIO_192;
> +	case 256:
> +		return ADCX140_RATIO_256;
> +	case 384:
> +		return ADCX140_RATIO_384;
> +	case 512:
> +		return ADCX140_RATIO_512;
> +	case 1024:
> +		return ADCX140_RATIO_1024;
> +	case 2048:
> +		return ADCX140_RATIO_2048;
> +	default:
> +		break;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int adcx140_fs_rate(unsigned int rate)
> +{
> +	switch (rate) {
> +	case 7350:
> +	case 8000:
> +		return ADCX140_8_OR_7_35KHZ;
> +	case 14700:
> +	case 16000:
> +		return ADCX140_16_OR_14_7KHZ;
> +	case 22050:
> +	case 24000:
> +		return ADCX140_24_OR_22_05KHZ;
> +	case 29400:
> +	case 32000:
> +		return ADCX140_32_OR_29_4KHZ;
> +	case 44100:
> +	case 48000:
> +		return ADCX140_48_OR_44_1KHZ;
> +	case 88200:
> +	case 96000:
> +		return ADCX140_96_OR_88_2KHZ;
> +	case 176400:
> +	case 192000:
> +		return ADCX140_192_OR_176_4KHZ;
> +	case 352800:
> +	case 384000:
> +		return ADCX140_384_OR_352_8KHZ;
> +	case 705600:
> +	case 768000:
> +		return ADCX140_768_OR_705_6KHZ;
> +	default:
> +		break;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int adcx140_setup_master_config(struct snd_soc_component *component,
> +				       struct snd_pcm_hw_params *params)
> +{
> +	int ret = 0;
> +	struct adcx140_priv *adcx140 = snd_soc_component_get_drvdata(component);
> +
> +	if (adcx140->master) {

Move this out to hw_params.  No reason to jump here just to jump back.

Check for master and if master then configure

This will eliminate the mixed code and variable declaration below which 
is what I wanted to avoid in v1.

This will also allow you to remove some of the variable initialization.

> +		u8 mst_cfg1 = 0;
> +		u8 mst_cfg0 = 0;
This can be init to mst_cfg0 = ADCX140_BCLK_FSYNC_MASTER no reason to 
set it here and then change it immediately.
> +		unsigned int bclk_ratio;
> +
> +		mst_cfg0 = ADCX140_BCLK_FSYNC_MASTER;
> +		if (params_rate(params) % 1000)
> +			mst_cfg0 |= ADCX140_FSYNCINV_BIT; /* 44.1 kHz et al */
> +
> +		ret = adcx140_fs_rate(params_rate(params));
> +		if (ret < 0) {
> +			dev_err(adcx140->dev, "%s: Unsupported rate %d\n",
> +					__func__, params_rate(params));
> +			return ret;
> +		}
> +		mst_cfg1 |= ret;
Why the | here?  This is initialized to 0 so mst_cfg1 = ret. And why 
even use ret just return into mst_cfg1 and check that variable
> +
> +		/* In slave mode when using automatic clock configuration,
> +		 * the codec figures out the BCLK to FSYNC ratio itself. But
> +		 * here in master mode, we need to tell it.
> +		 */
> +
> +		bclk_ratio = snd_soc_params_to_frame_size(params);
> +		ret = adcx140_fs_bclk_ratio(bclk_ratio);
> +		if (ret < 0) {
> +			dev_err(adcx140->dev, "%s: Unsupported bclk_ratio %d\n",
> +					__func__, bclk_ratio);
> +			return ret;
> +		}
> +		mst_cfg1 |= ret;
> +
> +		snd_soc_component_update_bits(component, ADCX140_MST_CFG1,
> +				ADCX140_FS_RATE_MSK |
> +				ADCX140_RATIO_MSK,
> +				mst_cfg1);

I don't understand the update_bits since you have calcualted both the 
Ratio and rate you can just write the register with the 
snd_soc_component_write.

> +
> +		snd_soc_component_update_bits(component, ADCX140_MST_CFG0,
> +				ADCX140_FSYNCINV_BIT |
> +				ADCX140_BCLK_FSYNC_MASTER,
> +				mst_cfg0);
> +

But this is ok.  I actually have other changes I am posting which move 
this to the set_dai_format.  So I am not sure if this will be needed 
after that patch is applied.

I will CC you on those patches.

Dan

Re: [PATCH v2 2/3] ASoC: tlv320adcx140: Add support for configuring GPIO pin

[Dan Murphy]

Camel

On 9/15/20 1:41 PM, Dan Murphy wrote:
> Camel
>
> On 9/11/20 3:07 AM, Camel Guo wrote:
>> From: Camel Guo <camelg@axis.com>


One other thing for the device tree you need to add Rob Herring and 
devicetree@vger.kernel.org

Re: [PATCH v2 3/3] ASoC: tlv320adcx140: Add proper support for master mode

[Camel Guo]

Please forget about this patch since Dan will upload a similar one.

@Dan, see my comment below.

On 9/15/20 8:50 PM, Dan Murphy wrote:
> Camel
> 
> On 9/11/20 3:07 AM, Camel Guo wrote:
>> From: Camel Guo <camelg@axis.com>
>>
>> Add setup of bclk-to-ws ratio and sample rate when in master mode,
>> as well as MCLK input pin setup.
>>
>> Signed-off-by: Camel Guo <camelg@axis.com>
>> ---
>>   v2:
>>    - Move GPIO setting into devicetree
>>    - Move master config register setting into a new function
>>
>>   sound/soc/codecs/tlv320adcx140.c | 139 ++++++++++++++++++++++++++++++-
>>   sound/soc/codecs/tlv320adcx140.h |  27 ++++++
>>   2 files changed, 162 insertions(+), 4 deletions(-)
>>
>> diff --git a/sound/soc/codecs/tlv320adcx140.c b/sound/soc/codecs/tlv320adcx140.c
>> index 97f16fbba441..685f5fd8b537 100644
>> --- a/sound/soc/codecs/tlv320adcx140.c
>> +++ b/sound/soc/codecs/tlv320adcx140.c
>> @@ -35,6 +35,7 @@ struct adcx140_priv {
>>        unsigned int dai_fmt;
>>        unsigned int tdm_delay;
>>        unsigned int slot_width;
>> +     bool master;
>>   };
>>   
>>   static const char * const gpo_config_names[] = {
>> @@ -651,11 +652,136 @@ static int adcx140_reset(struct adcx140_priv *adcx140)
>>        return ret;
>>   }
>>   
>> +static int adcx140_fs_bclk_ratio(unsigned int bclk_ratio)
>> +{
>> +     switch (bclk_ratio) {
>> +     case 16:
>> +             return ADCX140_RATIO_16;
>> +     case 24:
>> +             return ADCX140_RATIO_24;
>> +     case 32:
>> +             return ADCX140_RATIO_32;
>> +     case 48:
>> +             return ADCX140_RATIO_48;
>> +     case 64:
>> +             return ADCX140_RATIO_64;
>> +     case 96:
>> +             return ADCX140_RATIO_96;
>> +     case 128:
>> +             return ADCX140_RATIO_128;
>> +     case 192:
>> +             return ADCX140_RATIO_192;
>> +     case 256:
>> +             return ADCX140_RATIO_256;
>> +     case 384:
>> +             return ADCX140_RATIO_384;
>> +     case 512:
>> +             return ADCX140_RATIO_512;
>> +     case 1024:
>> +             return ADCX140_RATIO_1024;
>> +     case 2048:
>> +             return ADCX140_RATIO_2048;
>> +     default:
>> +             break;
>> +     }
>> +     return -EINVAL;
>> +}
>> +
>> +static int adcx140_fs_rate(unsigned int rate)
>> +{
>> +     switch (rate) {
>> +     case 7350:
>> +     case 8000:
>> +             return ADCX140_8_OR_7_35KHZ;
>> +     case 14700:
>> +     case 16000:
>> +             return ADCX140_16_OR_14_7KHZ;
>> +     case 22050:
>> +     case 24000:
>> +             return ADCX140_24_OR_22_05KHZ;
>> +     case 29400:
>> +     case 32000:
>> +             return ADCX140_32_OR_29_4KHZ;
>> +     case 44100:
>> +     case 48000:
>> +             return ADCX140_48_OR_44_1KHZ;
>> +     case 88200:
>> +     case 96000:
>> +             return ADCX140_96_OR_88_2KHZ;
>> +     case 176400:
>> +     case 192000:
>> +             return ADCX140_192_OR_176_4KHZ;
>> +     case 352800:
>> +     case 384000:
>> +             return ADCX140_384_OR_352_8KHZ;
>> +     case 705600:
>> +     case 768000:
>> +             return ADCX140_768_OR_705_6KHZ;
>> +     default:
>> +             break;
>> +     }
>> +     return -EINVAL;
>> +}
>> +
>> +static int adcx140_setup_master_config(struct snd_soc_component *component,
>> +                                    struct snd_pcm_hw_params *params)
>> +{
>> +     int ret = 0;
>> +     struct adcx140_priv *adcx140 = snd_soc_component_get_drvdata(component);
>> +
>> +     if (adcx140->master) {
> 
> Move this out to hw_params.  No reason to jump here just to jump back.
> 
> Check for master and if master then configure
> 
> This will eliminate the mixed code and variable declaration below which
> is what I wanted to avoid in v1.
> 
> This will also allow you to remove some of the variable initialization.
> 
>> +             u8 mst_cfg1 = 0;
>> +             u8 mst_cfg0 = 0;
> This can be init to mst_cfg0 = ADCX140_BCLK_FSYNC_MASTER no reason to
> set it here and then change it immediately.
>> +             unsigned int bclk_ratio;
>> +
>> +             mst_cfg0 = ADCX140_BCLK_FSYNC_MASTER;
>> +             if (params_rate(params) % 1000)
>> +                     mst_cfg0 |= ADCX140_FSYNCINV_BIT; /* 44.1 kHz et al */
>> +
>> +             ret = adcx140_fs_rate(params_rate(params));
>> +             if (ret < 0) {
>> +                     dev_err(adcx140->dev, "%s: Unsupported rate %d\n",
>> +                                     __func__, params_rate(params));
>> +                     return ret;
>> +             }
>> +             mst_cfg1 |= ret;
> Why the | here?  This is initialized to 0 so mst_cfg1 = ret. And why
> even use ret just return into mst_cfg1 and check that variable
>> +
>> +             /* In slave mode when using automatic clock configuration,
>> +              * the codec figures out the BCLK to FSYNC ratio itself. But
>> +              * here in master mode, we need to tell it.
>> +              */
>> +
>> +             bclk_ratio = snd_soc_params_to_frame_size(params);
>> +             ret = adcx140_fs_bclk_ratio(bclk_ratio);
>> +             if (ret < 0) {
>> +                     dev_err(adcx140->dev, "%s: Unsupported bclk_ratio %d\n",
>> +                                     __func__, bclk_ratio);
>> +                     return ret;
>> +             }
>> +             mst_cfg1 |= ret;
>> +
>> +             snd_soc_component_update_bits(component, ADCX140_MST_CFG1,
>> +                             ADCX140_FS_RATE_MSK |
>> +                             ADCX140_RATIO_MSK,
>> +                             mst_cfg1);
> 
> I don't understand the update_bits since you have calcualted both the
> Ratio and rate you can just write the register with the
> snd_soc_component_write.
> 
>> +
>> +             snd_soc_component_update_bits(component, ADCX140_MST_CFG0,
>> +                             ADCX140_FSYNCINV_BIT |
>> +                             ADCX140_BCLK_FSYNC_MASTER,
>> +                             mst_cfg0);
>> +
> 
> But this is ok.  I actually have other changes I am posting which move
> this to the set_dai_format.  So I am not sure if this will be needed
> after that patch is applied.

Maybe I can just wait for your patch. If it can make master mode work, 
there is no need for me to work on this one.

> 
> I will CC you on those patches.
> 
> Dan

Re: [PATCH v2 3/3] ASoC: tlv320adcx140: Add proper support for master mode

[Dan Murphy]

Re: [PATCH v2 3/3] ASoC: tlv320adcx140: Add proper support for master mode

[Dan Murphy]

Camel

On 9/16/20 2:52 AM, Camel Guo wrote:
> Please forget about this patch since Dan will upload a similar one.
>
> @Dan, see my comment below.
>
I have cc'd you on my patchset and as you can see I did not add any of 
the master mode programming only setting of the master mode bits.

So this patch is still viable it's just the master mode bit programming 
that needs to be looked at.

Dan