[PATCH v2 1/2] ASoC: adau17x1: Unused exported functions changed to internal

[PATCH v2 1/2] ASoC: adau17x1: Unused exported functions changed to internal

[Robert Rosengren]

adau17x1_setup_firmware and adau17x1_has_dsp is only used internally, so
making them static instead of exported.

Change-Id: I4882513ab457966b0371ac2c1024550ca6edbe0c
Signed-off-by: Robert Rosengren <robertr@axis.com>
---
 sound/soc/codecs/adau17x1.c | 9 +++++----
 sound/soc/codecs/adau17x1.h | 4 ----
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/adau17x1.c b/sound/soc/codecs/adau17x1.c
index 57169b8ff14e..2f2afb4c0c88 100644
--- a/sound/soc/codecs/adau17x1.c
+++ b/sound/soc/codecs/adau17x1.c
@@ -60,6 +60,9 @@ static const struct snd_kcontrol_new adau17x1_controls[] = {
 	SOC_ENUM("Mic Bias Mode", adau17x1_mic_bias_mode_enum),
 };
 
+static int adau17x1_setup_firmware(struct snd_soc_component *component,
+	unsigned int rate);
+
 static int adau17x1_pll_event(struct snd_soc_dapm_widget *w,
 	struct snd_kcontrol *kcontrol, int event)
 {
@@ -313,7 +316,7 @@ static const struct snd_soc_dapm_route adau17x1_no_dsp_dapm_routes[] = {
 	{ "Capture", NULL, "Right Decimator" },
 };
 
-bool adau17x1_has_dsp(struct adau *adau)
+static bool adau17x1_has_dsp(struct adau *adau)
 {
 	switch (adau->type) {
 	case ADAU1761:
@@ -324,7 +327,6 @@ bool adau17x1_has_dsp(struct adau *adau)
 		return false;
 	}
 }
-EXPORT_SYMBOL_GPL(adau17x1_has_dsp);
 
 static int adau17x1_set_dai_pll(struct snd_soc_dai *dai, int pll_id,
 	int source, unsigned int freq_in, unsigned int freq_out)
@@ -836,7 +838,7 @@ bool adau17x1_volatile_register(struct device *dev, unsigned int reg)
 }
 EXPORT_SYMBOL_GPL(adau17x1_volatile_register);
 
-int adau17x1_setup_firmware(struct snd_soc_component *component,
+static int adau17x1_setup_firmware(struct snd_soc_component *component,
 	unsigned int rate)
 {
 	int ret;
@@ -880,7 +882,6 @@ int adau17x1_setup_firmware(struct snd_soc_component *component,
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(adau17x1_setup_firmware);
 
 int adau17x1_add_widgets(struct snd_soc_component *component)
 {
diff --git a/sound/soc/codecs/adau17x1.h b/sound/soc/codecs/adau17x1.h
index e6fe87beec07..98a3b6f5bc96 100644
--- a/sound/soc/codecs/adau17x1.h
+++ b/sound/soc/codecs/adau17x1.h
@@ -68,10 +68,6 @@ int adau17x1_resume(struct snd_soc_component *component);
 
 extern const struct snd_soc_dai_ops adau17x1_dai_ops;
 
-int adau17x1_setup_firmware(struct snd_soc_component *component,
-	unsigned int rate);
-bool adau17x1_has_dsp(struct adau *adau);
-
 #define ADAU17X1_CLOCK_CONTROL			0x4000
 #define ADAU17X1_PLL_CONTROL			0x4002
 #define ADAU17X1_REC_POWER_MGMT			0x4009
-- 
2.11.0

[PATCH v2 2/2] ASoC: adau17x1: Implemented safeload support

[Robert Rosengren]

From: Danny Smith <dannys@axis.com>

Safeload support has been implemented which is used
when updating for instance filter parameters using
alsa controls. Without safeload support audio can
become distorted during update.

Signed-off-by: Danny Smith <dannys@axis.com>
Signed-off-by: Robert Rosengren <robertr@axis.com>
---
 sound/soc/codecs/adau17x1.c | 68 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/adau17x1.c b/sound/soc/codecs/adau17x1.c
index 2f2afb4c0c88..6d0b2261c959 100644
--- a/sound/soc/codecs/adau17x1.c
+++ b/sound/soc/codecs/adau17x1.c
@@ -26,6 +26,10 @@
 #include "adau17x1.h"
 #include "adau-utils.h"
 
+#define ADAU17X1_SAFELOAD_TARGET_ADDRESS 0x0006
+#define ADAU17X1_SAFELOAD_TRIGGER 0x0007
+#define ADAU17X1_SAFELOAD_DATA(i) (0x0001 + (i))
+
 static const char * const adau17x1_capture_mixer_boost_text[] = {
 	"Normal operation", "Boost Level 1", "Boost Level 2", "Boost Level 3",
 };
@@ -328,6 +332,17 @@ static bool adau17x1_has_dsp(struct adau *adau)
 	}
 }
 
+static bool adau17x1_has_safeload(struct adau *adau)
+{
+	switch (adau->type) {
+	case ADAU1761:
+	case ADAU1781:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int adau17x1_set_dai_pll(struct snd_soc_dai *dai, int pll_id,
 	int source, unsigned int freq_in, unsigned int freq_out)
 {
@@ -958,6 +973,50 @@ int adau17x1_resume(struct snd_soc_component *component)
 }
 EXPORT_SYMBOL_GPL(adau17x1_resume);
 
+static int adau17x1_safeload(struct sigmadsp *sigmadsp, unsigned int addr,
+	const uint8_t bytes[], size_t len)
+{
+	uint8_t buf[4];
+	unsigned int i;
+	int ret;
+
+	/* target address is offset by 1 */
+	unsigned int addr_offset = addr - 1;
+
+	/* write safeload addresses */
+	for (i = 0; i < len / 4; i++) {
+		memcpy(buf, bytes + i * 4, 4);
+		ret = regmap_raw_write(sigmadsp->control_data,
+			ADAU17X1_SAFELOAD_DATA(i), buf, 4);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* write target address */
+	buf[0] = (addr_offset >> 24) & 0xff;
+	buf[1] = (addr_offset >> 16) & 0xff;
+	buf[2] = (addr_offset >> 8) & 0xff;
+	buf[3] = (addr_offset) & 0xff;
+	ret = regmap_raw_write(sigmadsp->control_data,
+		ADAU17X1_SAFELOAD_TARGET_ADDRESS, buf, 4);
+	if (ret < 0)
+		return ret;
+
+	/* write nbr of words to trigger address */
+	buf[0] = 0x00;
+	buf[1] = 0x00;
+	buf[2] = 0x00;
+	buf[3] = i & 0xff;
+	ret = regmap_raw_write(sigmadsp->control_data,
+		ADAU17X1_SAFELOAD_TRIGGER, buf, 4);
+
+	return ret;
+}
+
+static const struct sigmadsp_ops adau17x1_sigmadsp_ops = {
+	.safeload = adau17x1_safeload,
+};
+
 int adau17x1_probe(struct device *dev, struct regmap *regmap,
 	enum adau17x1_type type, void (*switch_mode)(struct device *dev),
 	const char *firmware_name)
@@ -1003,8 +1062,13 @@ int adau17x1_probe(struct device *dev, struct regmap *regmap,
 	dev_set_drvdata(dev, adau);
 
 	if (firmware_name) {
-		adau->sigmadsp = devm_sigmadsp_init_regmap(dev, regmap, NULL,
-			firmware_name);
+		if (adau17x1_has_safeload(adau)) {
+			adau->sigmadsp = devm_sigmadsp_init_regmap(dev, regmap,
+				&adau17x1_sigmadsp_ops, firmware_name);
+		} else {
+			adau->sigmadsp = devm_sigmadsp_init_regmap(dev, regmap,
+				NULL, firmware_name);
+		}
 		if (IS_ERR(adau->sigmadsp)) {
 			dev_warn(dev, "Could not find firmware file: %ld\n",
 				PTR_ERR(adau->sigmadsp));
-- 
2.11.0

Re: [PATCH v2 2/2] ASoC: adau17x1: Implemented safeload support

[Lars-Peter Clausen]

On 08/13/2018 09:33 AM, Robert Rosengren wrote:
> From: Danny Smith <dannys@axis.com>
> 
> Safeload support has been implemented which is used
> when updating for instance filter parameters using
> alsa controls. Without safeload support audio can
> become distorted during update.
> 
> Signed-off-by: Danny Smith <dannys@axis.com>
> Signed-off-by: Robert Rosengren <robertr@axis.com>

Hi,

Thanks for implementing this. Some comments inline.

[...]
> +static int adau17x1_safeload(struct sigmadsp *sigmadsp, unsigned int addr,
> +	const uint8_t bytes[], size_t len)
> +{
> +	uint8_t buf[4];
> +	unsigned int i;
> +	int ret;
> +
> +	/* target address is offset by 1 */
> +	unsigned int addr_offset = addr - 1;
> +
> +	/* write safeload addresses */
> +	for (i = 0; i < len / 4; i++) {
> +		memcpy(buf, bytes + i * 4, 4);

Is this memcpy really required or could we just write bytes directly in a
single regmap_raw_write()? I believe the adau17x1 will auto increment the
register addresses.

And I suppose we also need to handle the case when len is not a multiple of 4.

> +		ret = regmap_raw_write(sigmadsp->control_data,
> +			ADAU17X1_SAFELOAD_DATA(i), buf, 4);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/* write target address */
> +	buf[0] = (addr_offset >> 24) & 0xff;
> +	buf[1] = (addr_offset >> 16) & 0xff;
> +	buf[2] = (addr_offset >> 8) & 0xff;
> +	buf[3] = (addr_offset) & 0xff;

This could be slightly simplified using

	put_unaligend_be32(addr_offset, buf);

> +	ret = regmap_raw_write(sigmadsp->control_data,
> +		ADAU17X1_SAFELOAD_TARGET_ADDRESS, buf, 4);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* write nbr of words to trigger address */
> +	buf[0] = 0x00;
> +	buf[1] = 0x00;
> +	buf[2] = 0x00;
> +	buf[3] = i & 0xff;
Same here I guess

> +	ret = regmap_raw_write(sigmadsp->control_data,
> +		ADAU17X1_SAFELOAD_TRIGGER, buf, 4);
> +
> +	return ret;
> +}

Re: [PATCH v2 1/2] ASoC: adau17x1: Unused exported functions changed to internal

[Lars-Peter Clausen]

On 08/13/2018 09:33 AM, Robert Rosengren wrote:
> adau17x1_setup_firmware and adau17x1_has_dsp is only used internally, so
> making them static instead of exported.
> 
> Change-Id: I4882513ab457966b0371ac2c1024550ca6edbe0c
> Signed-off-by: Robert Rosengren <robertr@axis.com>

Thanks for cleaning this up.

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

Applied "ASoC: adau17x1: Unused exported functions changed to internal" to the asoc tree

[Mark Brown]

The patch

   ASoC: adau17x1: Unused exported functions changed to internal

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 164c0f6454186d1b0e22c27dfe3d3d59fc80f979 Mon Sep 17 00:00:00 2001
From: Robert Rosengren <robert.rosengren@axis.com>
Date: Mon, 13 Aug 2018 09:33:58 +0200
Subject: [PATCH] ASoC: adau17x1: Unused exported functions changed to internal

adau17x1_setup_firmware and adau17x1_has_dsp is only used internally, so
making them static instead of exported.

Change-Id: I4882513ab457966b0371ac2c1024550ca6edbe0c
Signed-off-by: Robert Rosengren <robertr@axis.com>
Acked-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/adau17x1.c | 9 +++++----
 sound/soc/codecs/adau17x1.h | 4 ----
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/adau17x1.c b/sound/soc/codecs/adau17x1.c
index 57169b8ff14e..2f2afb4c0c88 100644
--- a/sound/soc/codecs/adau17x1.c
+++ b/sound/soc/codecs/adau17x1.c
@@ -60,6 +60,9 @@ static const struct snd_kcontrol_new adau17x1_controls[] = {
 	SOC_ENUM("Mic Bias Mode", adau17x1_mic_bias_mode_enum),
 };
 
+static int adau17x1_setup_firmware(struct snd_soc_component *component,
+	unsigned int rate);
+
 static int adau17x1_pll_event(struct snd_soc_dapm_widget *w,
 	struct snd_kcontrol *kcontrol, int event)
 {
@@ -313,7 +316,7 @@ static const struct snd_soc_dapm_route adau17x1_no_dsp_dapm_routes[] = {
 	{ "Capture", NULL, "Right Decimator" },
 };
 
-bool adau17x1_has_dsp(struct adau *adau)
+static bool adau17x1_has_dsp(struct adau *adau)
 {
 	switch (adau->type) {
 	case ADAU1761:
@@ -324,7 +327,6 @@ bool adau17x1_has_dsp(struct adau *adau)
 		return false;
 	}
 }
-EXPORT_SYMBOL_GPL(adau17x1_has_dsp);
 
 static int adau17x1_set_dai_pll(struct snd_soc_dai *dai, int pll_id,
 	int source, unsigned int freq_in, unsigned int freq_out)
@@ -836,7 +838,7 @@ bool adau17x1_volatile_register(struct device *dev, unsigned int reg)
 }
 EXPORT_SYMBOL_GPL(adau17x1_volatile_register);
 
-int adau17x1_setup_firmware(struct snd_soc_component *component,
+static int adau17x1_setup_firmware(struct snd_soc_component *component,
 	unsigned int rate)
 {
 	int ret;
@@ -880,7 +882,6 @@ int adau17x1_setup_firmware(struct snd_soc_component *component,
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(adau17x1_setup_firmware);
 
 int adau17x1_add_widgets(struct snd_soc_component *component)
 {
diff --git a/sound/soc/codecs/adau17x1.h b/sound/soc/codecs/adau17x1.h
index e6fe87beec07..98a3b6f5bc96 100644
--- a/sound/soc/codecs/adau17x1.h
+++ b/sound/soc/codecs/adau17x1.h
@@ -68,10 +68,6 @@ int adau17x1_resume(struct snd_soc_component *component);
 
 extern const struct snd_soc_dai_ops adau17x1_dai_ops;
 
-int adau17x1_setup_firmware(struct snd_soc_component *component,
-	unsigned int rate);
-bool adau17x1_has_dsp(struct adau *adau);
-
 #define ADAU17X1_CLOCK_CONTROL			0x4000
 #define ADAU17X1_PLL_CONTROL			0x4002
 #define ADAU17X1_REC_POWER_MGMT			0x4009
-- 
2.18.0

Re: [PATCH v2 2/2] ASoC: adau17x1: Implemented safeload support

[Danny Smith]

On 08/13/2018 12:44 PM, Lars-Peter Clausen wrote:
> On 08/13/2018 09:33 AM, Robert Rosengren wrote:
>> From: Danny Smith <dannys@axis.com>
>>
>> Safeload support has been implemented which is used
>> when updating for instance filter parameters using
>> alsa controls. Without safeload support audio can
>> become distorted during update.
>>
>> Signed-off-by: Danny Smith <dannys@axis.com>
>> Signed-off-by: Robert Rosengren <robertr@axis.com>
> Hi,
>
> Thanks for implementing this. Some comments inline.
>
> [...]
>> +static int adau17x1_safeload(struct sigmadsp *sigmadsp, unsigned int addr,
>> +	const uint8_t bytes[], size_t len)
>> +{
>> +	uint8_t buf[4];
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	/* target address is offset by 1 */
>> +	unsigned int addr_offset = addr - 1;
>> +
>> +	/* write safeload addresses */
>> +	for (i = 0; i < len / 4; i++) {
>> +		memcpy(buf, bytes + i * 4, 4);
> Is this memcpy really required or could we just write bytes directly in a
> single regmap_raw_write()? I believe the adau17x1 will auto increment the
> register addresses.
>
> And I suppose we also need to handle the case when len is not a multiple of 4.
A single regmap_raw_write should work, forgot about the addresses auto 
incrementing.
What would be the proper way of handling the case where len is not a 
multiple of 4?
>
>> +		ret = regmap_raw_write(sigmadsp->control_data,
>> +			ADAU17X1_SAFELOAD_DATA(i), buf, 4);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	/* write target address */
>> +	buf[0] = (addr_offset >> 24) & 0xff;
>> +	buf[1] = (addr_offset >> 16) & 0xff;
>> +	buf[2] = (addr_offset >> 8) & 0xff;
>> +	buf[3] = (addr_offset) & 0xff;
> This could be slightly simplified using
>
> 	put_unaligend_be32(addr_offset, buf);
>
>> +	ret = regmap_raw_write(sigmadsp->control_data,
>> +		ADAU17X1_SAFELOAD_TARGET_ADDRESS, buf, 4);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* write nbr of words to trigger address */
>> +	buf[0] = 0x00;
>> +	buf[1] = 0x00;
>> +	buf[2] = 0x00;
>> +	buf[3] = i & 0xff;
> Same here I guess
>
>> +	ret = regmap_raw_write(sigmadsp->control_data,
>> +		ADAU17X1_SAFELOAD_TRIGGER, buf, 4);
>> +
>> +	return ret;
>> +}