[PATCH 0/2] ASoC: tegra210: simplify byte map handling in ADX and AMX

[PATCH 0/2] ASoC: tegra210: simplify byte map handling in ADX and AMX

[Piyush Patle]

The Tegra210 ADX and AMX drivers both keep their "Byte Map N" ALSA
control state as a byte-packed u32 map[] array along with a separate
byte_mask[] bitmap. This is because the control range exposed to
userspace is [0, 256], where 256 is the "disabled" sentinel and
does not fit in a byte, so the two arrays have to be cross-checked
on every get()/put().

This series stores each slot as a u16 holding the user-visible
value directly, turning get_byte_map() into a direct return and
put_byte_map() into a compare-and-store. The hardware-facing packed
RAM word and the IN_BYTE_EN / OUT_BYTE_EN enable masks are computed
on the fly inside each write_map_ram() callback, which is the only
place that needs to know the hardware layout. The byte_mask[] field
is dropped from both driver state structs.

There is no userspace-visible ABI change. Control declarations,
ranges, initial values and handling of out-of-range writes is
preserved by treating values outside [0, 255] as disabled (256),
matching previous behavior. As a side effect each patch also fixes
a latent bug in put_byte_map() where an enabled-to-enabled value
change was not persisted.

The packed RAM word construction is also updated to ensure the shift
operates on a u32 value, avoiding potential undefined behavior due
to signed integer promotion.

Addresses TODO comments left in tegra210_{adx,amx}_get_byte_map().

Patch 1/2: ASoC: tegra210_adx: simplify byte map get/put logic
Patch 2/2: ASoC: tegra210_amx: simplify byte map get/put logic

-- 
2.34.1

[PATCH 1/2] ASoC: tegra210_adx: simplify byte map get/put logic

[Piyush Patle]

The byte-map controls ("Byte Map N") already expose a value range of
[0, 256] to userspace via SOC_SINGLE_EXT(), where 256 is the
"disabled" sentinel. The driver stored this state as a byte-packed
u32 map[] array plus a separate byte_mask[] bitmap tracking which
slots were enabled, because 256 does not fit in a byte. As a result
get_byte_map() had to consult byte_mask[] to decide whether to
report the stored byte or 256, and put_byte_map() had to keep the
two arrays in sync on every write.

Store each slot as a u16 holding the control value directly
(0..255 enabled, 256 disabled). This is the native representation
for what userspace already sees, so get_byte_map() becomes a direct
return and put_byte_map() becomes a compare-and-store. The
hardware-facing packed RAM word and the IN_BYTE_EN mask are now
derived on the fly inside tegra210_adx_write_map_ram() from the
slot array, which is the only place that needs to know about the
hardware layout. This also lets us drop the byte_mask field from
struct tegra210_adx.

Slots are initialised to 256 in probe() so the default reported
value stays "disabled", matching previous behaviour. Values written
from userspace that fall outside [0, 255] are clamped to 256
("disabled") exactly as before -- no userspace-visible change.

As a side effect this also fixes a latent bug in the previous
put_byte_map(): because it compared the enable mask rather than the
stored byte, changing a slot from one enabled value to another
enabled value (e.g. 42 -> 99) would early-return without persisting
the new value.

Also fix a potential undefined behavior when constructing the packed
RAM word by ensuring the shift operates on a u32 value.

Addresses TODO left in tegra210_adx_get_byte_map().

Signed-off-by: Piyush Patle <piyushpatle228@gmail.com>
---
 sound/soc/tegra/tegra210_adx.c | 80 ++++++++++++++++------------------
 sound/soc/tegra/tegra210_adx.h |  5 ++-
 2 files changed, 40 insertions(+), 45 deletions(-)

diff --git a/sound/soc/tegra/tegra210_adx.c b/sound/soc/tegra/tegra210_adx.c
index 95875c75ddf8..67948459e884 100644
--- a/sound/soc/tegra/tegra210_adx.c
+++ b/sound/soc/tegra/tegra210_adx.c
@@ -47,6 +47,7 @@ static const struct reg_default tegra264_adx_reg_defaults[] = {
 
 static void tegra210_adx_write_map_ram(struct tegra210_adx *adx)
 {
+	unsigned int byte_mask[TEGRA264_ADX_BYTE_MASK_COUNT] = { 0 };
 	int i;
 
 	regmap_write(adx->regmap, TEGRA210_ADX_CFG_RAM_CTRL +
@@ -55,15 +56,28 @@ static void tegra210_adx_write_map_ram(struct tegra210_adx *adx)
 		     TEGRA210_ADX_CFG_RAM_CTRL_ADDR_INIT_EN |
 		     TEGRA210_ADX_CFG_RAM_CTRL_RW_WRITE);
 
-	for (i = 0; i < adx->soc_data->ram_depth; i++)
+	for (i = 0; i < adx->soc_data->ram_depth; i++) {
+		u32 word = 0;
+		int b;
+
+		for (b = 0; b < 4; b++) {
+			unsigned int slot = i * 4 + b;
+			u16 val = adx->map[slot];
+
+			if (val >= 256)
+				continue;
+
+			word |= (u32)val << (b * 8);
+			byte_mask[slot / 32] |= 1U << (slot % 32);
+		}
 		regmap_write(adx->regmap, TEGRA210_ADX_CFG_RAM_DATA +
-				adx->soc_data->cya_offset,
-			     adx->map[i]);
+				adx->soc_data->cya_offset, word);
+	}
 
 	for (i = 0; i < adx->soc_data->byte_mask_size; i++)
 		regmap_write(adx->regmap,
 			     TEGRA210_ADX_IN_BYTE_EN0 + (i * TEGRA210_ADX_AUDIOCIF_CH_STRIDE),
-			     adx->byte_mask[i]);
+			     byte_mask[i]);
 }
 
 static int tegra210_adx_startup(struct snd_pcm_substream *substream,
@@ -188,27 +202,10 @@ static int tegra210_adx_get_byte_map(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_component *cmpnt = snd_kcontrol_chip(kcontrol);
 	struct tegra210_adx *adx = snd_soc_component_get_drvdata(cmpnt);
-	struct soc_mixer_control *mc;
-	unsigned char *bytes_map = (unsigned char *)adx->map;
-	int enabled;
+	struct soc_mixer_control *mc =
+		(struct soc_mixer_control *)kcontrol->private_value;
 
-	mc = (struct soc_mixer_control *)kcontrol->private_value;
-	enabled = adx->byte_mask[mc->reg / 32] & (1 << (mc->reg % 32));
-
-	/*
-	 * TODO: Simplify this logic to just return from bytes_map[]
-	 *
-	 * Presently below is required since bytes_map[] is
-	 * tightly packed and cannot store the control value of 256.
-	 * Byte mask state is used to know if 256 needs to be returned.
-	 * Note that for control value of 256, the put() call stores 0
-	 * in the bytes_map[] and disables the corresponding bit in
-	 * byte_mask[].
-	 */
-	if (enabled)
-		ucontrol->value.integer.value[0] = bytes_map[mc->reg];
-	else
-		ucontrol->value.integer.value[0] = 256;
+	ucontrol->value.integer.value[0] = adx->map[mc->reg];
 
 	return 0;
 }
@@ -218,23 +215,22 @@ static int tegra210_adx_put_byte_map(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_component *cmpnt = snd_kcontrol_chip(kcontrol);
 	struct tegra210_adx *adx = snd_soc_component_get_drvdata(cmpnt);
-	unsigned char *bytes_map = (unsigned char *)adx->map;
-	int value = ucontrol->value.integer.value[0];
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	unsigned int mask_val = adx->byte_mask[mc->reg / 32];
+	unsigned int value = ucontrol->value.integer.value[0];
 
-	if (value >= 0 && value <= 255)
-		mask_val |= (1 << (mc->reg % 32));
-	else
-		mask_val &= ~(1 << (mc->reg % 32));
+	/*
+	 * Match the previous behaviour: any value outside [0, 255] is
+	 * treated as the "disabled" sentinel (256). Negative values from
+	 * userspace fold in through the unsigned cast and are caught here.
+	 */
+	if (value > 255)
+		value = 256;
 
-	if (mask_val == adx->byte_mask[mc->reg / 32])
+	if (adx->map[mc->reg] == value)
 		return 0;
 
-	/* Update byte map and slot */
-	bytes_map[mc->reg] = value % 256;
-	adx->byte_mask[mc->reg / 32] = mask_val;
+	adx->map[mc->reg] = value;
 
 	return 1;
 }
@@ -675,7 +671,7 @@ static int tegra210_adx_platform_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	struct tegra210_adx_soc_data *soc_data;
 	void __iomem *regs;
-	int err;
+	int err, i;
 
 	adx = devm_kzalloc(dev, sizeof(*adx), GFP_KERNEL);
 	if (!adx)
@@ -700,16 +696,14 @@ static int tegra210_adx_platform_probe(struct platform_device *pdev)
 
 	regcache_cache_only(adx->regmap, true);
 
-	adx->map = devm_kzalloc(dev, soc_data->ram_depth * sizeof(*adx->map),
-				GFP_KERNEL);
+	adx->map = devm_kcalloc(dev, soc_data->ram_depth * 4,
+				sizeof(*adx->map), GFP_KERNEL);
 	if (!adx->map)
 		return -ENOMEM;
 
-	adx->byte_mask = devm_kzalloc(dev,
-				      soc_data->byte_mask_size * sizeof(*adx->byte_mask),
-				      GFP_KERNEL);
-	if (!adx->byte_mask)
-		return -ENOMEM;
+	/* Initialize all byte map slots as disabled (value 256). */
+	for (i = 0; i < soc_data->ram_depth * 4; i++)
+		adx->map[i] = 256;
 
 	tegra210_adx_dais[TEGRA_ADX_IN_DAI_ID].playback.channels_max =
 			adx->soc_data->max_ch;
diff --git a/sound/soc/tegra/tegra210_adx.h b/sound/soc/tegra/tegra210_adx.h
index 176a4e40de0a..afe95e45458f 100644
--- a/sound/soc/tegra/tegra210_adx.h
+++ b/sound/soc/tegra/tegra210_adx.h
@@ -8,6 +8,8 @@
 #ifndef __TEGRA210_ADX_H__
 #define __TEGRA210_ADX_H__
 
+#include <linux/types.h>
+
 /* Register offsets from TEGRA210_ADX*_BASE */
 #define TEGRA210_ADX_RX_STATUS		0x0c
 #define TEGRA210_ADX_RX_INT_STATUS	0x10
@@ -88,8 +90,7 @@ struct tegra210_adx_soc_data {
 
 struct tegra210_adx {
 	struct regmap *regmap;
-	unsigned int *map;
-	unsigned int *byte_mask;
+	u16 *map;
 	const struct tegra210_adx_soc_data *soc_data;
 };
 
-- 
2.34.1

[PATCH 2/2] ASoC: tegra210_amx: simplify byte map get/put logic

[Piyush Patle]

The byte-map controls ("Byte Map N") already expose a value range of
[0, 256] to userspace via SOC_SINGLE_EXT(), where 256 is the
"disabled" sentinel. The driver stored this state as a byte-packed
u32 map[] array plus a separate byte_mask[] bitmap tracking which
slots were enabled, because 256 does not fit in a byte. As a result
get_byte_map() had to consult byte_mask[] to decide whether to
report the stored byte or 256, and put_byte_map() had to keep the
two arrays in sync on every write.

Store each slot as a u16 holding the control value directly
(0..255 enabled, 256 disabled). This is the native representation
for what userspace already sees, so get_byte_map() becomes a direct
return and put_byte_map() becomes a compare-and-store. The
hardware-facing packed RAM word and the OUT_BYTE_EN mask are now
derived on the fly inside tegra210_amx_write_map_ram() from the
slot array, which is the only place that needs to know about the
hardware layout. This also lets us drop the byte_mask field from
struct tegra210_amx.

Slots are initialised to 256 in probe() so the default reported
value stays "disabled", matching previous behaviour. Values written
from userspace that fall outside [0, 255] are clamped to 256
("disabled") exactly as before -- no userspace-visible change.

As a side effect this also fixes a latent bug in the previous
put_byte_map(): because it compared the enable mask rather than the
stored byte, changing a slot from one enabled value to another
enabled value (e.g. 42 -> 99) would early-return without persisting
the new value.

Also fix a potential undefined behavior when constructing the packed
RAM word by ensuring the shift operates on a u32 value.

Addresses TODO left in tegra210_amx_get_byte_map().

Signed-off-by: Piyush Patle <piyushpatle228@gmail.com>
---
 sound/soc/tegra/tegra210_amx.c | 77 ++++++++++++++++------------------
 sound/soc/tegra/tegra210_amx.h |  5 ++-
 2 files changed, 38 insertions(+), 44 deletions(-)

diff --git a/sound/soc/tegra/tegra210_amx.c b/sound/soc/tegra/tegra210_amx.c
index bfda82505298..4dd158e6e974 100644
--- a/sound/soc/tegra/tegra210_amx.c
+++ b/sound/soc/tegra/tegra210_amx.c
@@ -60,6 +60,7 @@ static const struct reg_default tegra264_amx_reg_defaults[] = {
 
 static void tegra210_amx_write_map_ram(struct tegra210_amx *amx)
 {
+	unsigned int byte_mask[TEGRA264_AMX_BYTE_MASK_COUNT] = { 0 };
 	int i;
 
 	regmap_write(amx->regmap, TEGRA210_AMX_CFG_RAM_CTRL + amx->soc_data->reg_offset,
@@ -67,14 +68,28 @@ static void tegra210_amx_write_map_ram(struct tegra210_amx *amx)
 		     TEGRA210_AMX_CFG_RAM_CTRL_ADDR_INIT_EN |
 		     TEGRA210_AMX_CFG_RAM_CTRL_RW_WRITE);
 
-	for (i = 0; i < amx->soc_data->ram_depth; i++)
+	for (i = 0; i < amx->soc_data->ram_depth; i++) {
+		u32 word = 0;
+		int b;
+
+		for (b = 0; b < 4; b++) {
+			unsigned int slot = i * 4 + b;
+			u16 val = amx->map[slot];
+
+			if (val >= 256)
+				continue;
+
+			word |= (u32)val << (b * 8);
+			byte_mask[slot / 32] |= 1U << (slot % 32);
+		}
 		regmap_write(amx->regmap, TEGRA210_AMX_CFG_RAM_DATA + amx->soc_data->reg_offset,
-			     amx->map[i]);
+			     word);
+	}
 
 	for (i = 0; i < amx->soc_data->byte_mask_size; i++)
 		regmap_write(amx->regmap,
 			     TEGRA210_AMX_OUT_BYTE_EN0 + (i * TEGRA210_AMX_AUDIOCIF_CH_STRIDE),
-			     amx->byte_mask[i]);
+			     byte_mask[i]);
 }
 
 static int tegra210_amx_startup(struct snd_pcm_substream *substream,
@@ -212,26 +227,8 @@ static int tegra210_amx_get_byte_map(struct snd_kcontrol *kcontrol,
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
 	struct tegra210_amx *amx = snd_soc_component_get_drvdata(cmpnt);
-	unsigned char *bytes_map = (unsigned char *)amx->map;
-	int reg = mc->reg;
-	int enabled;
 
-	enabled = amx->byte_mask[reg / 32] & (1 << (reg % 32));
-
-	/*
-	 * TODO: Simplify this logic to just return from bytes_map[]
-	 *
-	 * Presently below is required since bytes_map[] is
-	 * tightly packed and cannot store the control value of 256.
-	 * Byte mask state is used to know if 256 needs to be returned.
-	 * Note that for control value of 256, the put() call stores 0
-	 * in the bytes_map[] and disables the corresponding bit in
-	 * byte_mask[].
-	 */
-	if (enabled)
-		ucontrol->value.integer.value[0] = bytes_map[reg];
-	else
-		ucontrol->value.integer.value[0] = 256;
+	ucontrol->value.integer.value[0] = amx->map[mc->reg];
 
 	return 0;
 }
@@ -243,22 +240,20 @@ static int tegra210_amx_put_byte_map(struct snd_kcontrol *kcontrol,
 		(struct soc_mixer_control *)kcontrol->private_value;
 	struct snd_soc_component *cmpnt = snd_kcontrol_chip(kcontrol);
 	struct tegra210_amx *amx = snd_soc_component_get_drvdata(cmpnt);
-	unsigned char *bytes_map = (unsigned char *)amx->map;
-	int reg = mc->reg;
-	int value = ucontrol->value.integer.value[0];
-	unsigned int mask_val = amx->byte_mask[reg / 32];
+	unsigned int value = ucontrol->value.integer.value[0];
 
-	if (value >= 0 && value <= 255)
-		mask_val |= (1 << (reg % 32));
-	else
-		mask_val &= ~(1 << (reg % 32));
+	/*
+	 * Match the previous behaviour: any value outside [0, 255] is
+	 * treated as the "disabled" sentinel (256). Negative values from
+	 * userspace fold in through the unsigned cast and are caught here.
+	 */
+	if (value > 255)
+		value = 256;
 
-	if (mask_val == amx->byte_mask[reg / 32])
+	if (amx->map[mc->reg] == value)
 		return 0;
 
-	/* Update byte map and slot */
-	bytes_map[reg] = value % 256;
-	amx->byte_mask[reg / 32] = mask_val;
+	amx->map[mc->reg] = value;
 
 	return 1;
 }
@@ -727,7 +722,7 @@ static int tegra210_amx_platform_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct tegra210_amx *amx;
 	void __iomem *regs;
-	int err;
+	int err, i;
 
 	amx = devm_kzalloc(dev, sizeof(*amx), GFP_KERNEL);
 	if (!amx)
@@ -750,16 +745,14 @@ static int tegra210_amx_platform_probe(struct platform_device *pdev)
 
 	regcache_cache_only(amx->regmap, true);
 
-	amx->map = devm_kzalloc(dev, amx->soc_data->ram_depth * sizeof(*amx->map),
-				GFP_KERNEL);
+	amx->map = devm_kcalloc(dev, amx->soc_data->ram_depth * 4,
+				sizeof(*amx->map), GFP_KERNEL);
 	if (!amx->map)
 		return -ENOMEM;
 
-	amx->byte_mask = devm_kzalloc(dev,
-				      amx->soc_data->byte_mask_size * sizeof(*amx->byte_mask),
-				      GFP_KERNEL);
-	if (!amx->byte_mask)
-		return -ENOMEM;
+	/* Initialize all byte map slots as disabled (value 256). */
+	for (i = 0; i < amx->soc_data->ram_depth * 4; i++)
+		amx->map[i] = 256;
 
 	tegra210_amx_dais[TEGRA_AMX_OUT_DAI_ID].capture.channels_max =
 			amx->soc_data->max_ch;
diff --git a/sound/soc/tegra/tegra210_amx.h b/sound/soc/tegra/tegra210_amx.h
index 50a237b197ba..6df9ab0fe220 100644
--- a/sound/soc/tegra/tegra210_amx.h
+++ b/sound/soc/tegra/tegra210_amx.h
@@ -8,6 +8,8 @@
 #ifndef __TEGRA210_AMX_H__
 #define __TEGRA210_AMX_H__
 
+#include <linux/types.h>
+
 /* Register offsets from TEGRA210_AMX*_BASE */
 #define TEGRA210_AMX_RX_STATUS			0x0c
 #define TEGRA210_AMX_RX_INT_STATUS		0x10
@@ -105,8 +107,7 @@ struct tegra210_amx_soc_data {
 
 struct tegra210_amx {
 	const struct tegra210_amx_soc_data *soc_data;
-	unsigned int *map;
-	unsigned int *byte_mask;
+	u16 *map;
 	struct regmap *regmap;
 };
 
-- 
2.34.1

Re: [PATCH 2/2] ASoC: tegra210_amx: simplify byte map get/put logic

[Sheetal .]

On 07-04-2026 22:33, Piyush Patle wrote:
> External email: Use caution opening links or attachments
> 
> 
> The byte-map controls ("Byte Map N") already expose a value range of
> [0, 256] to userspace via SOC_SINGLE_EXT(), where 256 is the
> "disabled" sentinel. The driver stored this state as a byte-packed
> u32 map[] array plus a separate byte_mask[] bitmap tracking which
> slots were enabled, because 256 does not fit in a byte. As a result
> get_byte_map() had to consult byte_mask[] to decide whether to
> report the stored byte or 256, and put_byte_map() had to keep the
> two arrays in sync on every write.
> 
> Store each slot as a u16 holding the control value directly
> (0..255 enabled, 256 disabled). This is the native representation
> for what userspace already sees, so get_byte_map() becomes a direct
> return and put_byte_map() becomes a compare-and-store. The
> hardware-facing packed RAM word and the OUT_BYTE_EN mask are now
> derived on the fly inside tegra210_amx_write_map_ram() from the
> slot array, which is the only place that needs to know about the
> hardware layout. This also lets us drop the byte_mask field from
> struct tegra210_amx.
> 
> Slots are initialised to 256 in probe() so the default reported
> value stays "disabled", matching previous behaviour. Values written
> from userspace that fall outside [0, 255] are clamped to 256
> ("disabled") exactly as before -- no userspace-visible change.
> 
> As a side effect this also fixes a latent bug in the previous
> put_byte_map(): because it compared the enable mask rather than the
> stored byte, changing a slot from one enabled value to another
> enabled value (e.g. 42 -> 99) would early-return without persisting
> the new value.
> 
> Also fix a potential undefined behavior when constructing the packed
> RAM word by ensuring the shift operates on a u32 value.
> 
> Addresses TODO left in tegra210_amx_get_byte_map().
> 
> Signed-off-by: Piyush Patle <piyushpatle228@gmail.com>
> ---
>   sound/soc/tegra/tegra210_amx.c | 77 ++++++++++++++++------------------
>   sound/soc/tegra/tegra210_amx.h |  5 ++-
>   2 files changed, 38 insertions(+), 44 deletions(-)
> 
> diff --git a/sound/soc/tegra/tegra210_amx.c b/sound/soc/tegra/tegra210_amx.c
> index bfda82505298..4dd158e6e974 100644
> --- a/sound/soc/tegra/tegra210_amx.c
> +++ b/sound/soc/tegra/tegra210_amx.c
> @@ -60,6 +60,7 @@ static const struct reg_default tegra264_amx_reg_defaults[] = {
> 
>   static void tegra210_amx_write_map_ram(struct tegra210_amx *amx)
>   {
> +       unsigned int byte_mask[TEGRA264_AMX_BYTE_MASK_COUNT] = { 0 };


byte_mask[] is sized to the chip-specific TEGRA264_AMX_BYTE_MASK_COUNT,
but the map array in probe() is already dynamically sized from
soc_data. Since soc_data->byte_mask_size is available here, kcalloc() 
would be consistent and avoid coupling to a specific SoC variant's constant.


>          int i;
> 
>          regmap_write(amx->regmap, TEGRA210_AMX_CFG_RAM_CTRL + amx->soc_data->reg_offset,
> @@ -67,14 +68,28 @@ static void tegra210_amx_write_map_ram(struct tegra210_amx *amx)
>                       TEGRA210_AMX_CFG_RAM_CTRL_ADDR_INIT_EN |
>                       TEGRA210_AMX_CFG_RAM_CTRL_RW_WRITE);
> 
> -       for (i = 0; i < amx->soc_data->ram_depth; i++)
> +       for (i = 0; i < amx->soc_data->ram_depth; i++) {
> +               u32 word = 0;
> +               int b;
> +
> +               for (b = 0; b < 4; b++) {
> +                       unsigned int slot = i * 4 + b;
> +                       u16 val = amx->map[slot];
> +
> +                       if (val >= 256)
> +                               continue;
> +
> +                       word |= (u32)val << (b * 8);


The literal '4' (bytes per RAM word) and '8' (bits per byte) are magic
numbers scattered through the code here and in probe function. Please 
consider defining:
   #define TEGRA_AMX_SLOTS_PER_WORD    4
and using BITS_PER_BYTE from <linux/bits.h> for the shift.


> +                       byte_mask[slot / 32] |= 1U << (slot % 32);
> +               }
>                  regmap_write(amx->regmap, TEGRA210_AMX_CFG_RAM_DATA + amx->soc_data->reg_offset,
> -                            amx->map[i]);
> +                            word);
> +       }
> 
>          for (i = 0; i < amx->soc_data->byte_mask_size; i++)
>                  regmap_write(amx->regmap,
>                               TEGRA210_AMX_OUT_BYTE_EN0 + (i * TEGRA210_AMX_AUDIOCIF_CH_STRIDE),
> -                            amx->byte_mask[i]);
> +                            byte_mask[i]);
>   }
> 
>   static int tegra210_amx_startup(struct snd_pcm_substream *substream,
> @@ -212,26 +227,8 @@ static int tegra210_amx_get_byte_map(struct snd_kcontrol *kcontrol,
>          struct soc_mixer_control *mc =
>                  (struct soc_mixer_control *)kcontrol->private_value;
>          struct tegra210_amx *amx = snd_soc_component_get_drvdata(cmpnt);
> -       unsigned char *bytes_map = (unsigned char *)amx->map;
> -       int reg = mc->reg;
> -       int enabled;
> 
> -       enabled = amx->byte_mask[reg / 32] & (1 << (reg % 32));
> -
> -       /*
> -        * TODO: Simplify this logic to just return from bytes_map[]
> -        *
> -        * Presently below is required since bytes_map[] is
> -        * tightly packed and cannot store the control value of 256.
> -        * Byte mask state is used to know if 256 needs to be returned.
> -        * Note that for control value of 256, the put() call stores 0
> -        * in the bytes_map[] and disables the corresponding bit in
> -        * byte_mask[].
> -        */
> -       if (enabled)
> -               ucontrol->value.integer.value[0] = bytes_map[reg];
> -       else
> -               ucontrol->value.integer.value[0] = 256;
> +       ucontrol->value.integer.value[0] = amx->map[mc->reg];
> 
>          return 0;
>   }
> @@ -243,22 +240,20 @@ static int tegra210_amx_put_byte_map(struct snd_kcontrol *kcontrol,
>                  (struct soc_mixer_control *)kcontrol->private_value;
>          struct snd_soc_component *cmpnt = snd_kcontrol_chip(kcontrol);
>          struct tegra210_amx *amx = snd_soc_component_get_drvdata(cmpnt);
> -       unsigned char *bytes_map = (unsigned char *)amx->map;
> -       int reg = mc->reg;
> -       int value = ucontrol->value.integer.value[0];
> -       unsigned int mask_val = amx->byte_mask[reg / 32];
> +       unsigned int value = ucontrol->value.integer.value[0];
> 
> -       if (value >= 0 && value <= 255)
> -               mask_val |= (1 << (reg % 32));
> -       else
> -               mask_val &= ~(1 << (reg % 32));
> +       /*
> +        * Match the previous behaviour: any value outside [0, 255] is
> +        * treated as the "disabled" sentinel (256). Negative values from
> +        * userspace fold in through the unsigned cast and are caught here.
> +        */
> +       if (value > 255)
> +               value = 256;
> 
> -       if (mask_val == amx->byte_mask[reg / 32])
> +       if (amx->map[mc->reg] == value)
>                  return 0;
> 
> -       /* Update byte map and slot */
> -       bytes_map[reg] = value % 256;
> -       amx->byte_mask[reg / 32] = mask_val;
> +       amx->map[mc->reg] = value;
> 
>          return 1;
>   }
> @@ -727,7 +722,7 @@ static int tegra210_amx_platform_probe(struct platform_device *pdev)
>          struct device *dev = &pdev->dev;
>          struct tegra210_amx *amx;
>          void __iomem *regs;
> -       int err;
> +       int err, i;
> 
>          amx = devm_kzalloc(dev, sizeof(*amx), GFP_KERNEL);
>          if (!amx)
> @@ -750,16 +745,14 @@ static int tegra210_amx_platform_probe(struct platform_device *pdev)
> 
>          regcache_cache_only(amx->regmap, true);
> 
> -       amx->map = devm_kzalloc(dev, amx->soc_data->ram_depth * sizeof(*amx->map),
> -                               GFP_KERNEL);
> +       amx->map = devm_kcalloc(dev, amx->soc_data->ram_depth * 4,
> +                               sizeof(*amx->map), GFP_KERNEL);
>          if (!amx->map)
>                  return -ENOMEM;
> 
> -       amx->byte_mask = devm_kzalloc(dev,
> -                                     amx->soc_data->byte_mask_size * sizeof(*amx->byte_mask),
> -                                     GFP_KERNEL);
> -       if (!amx->byte_mask)
> -               return -ENOMEM;
> +       /* Initialize all byte map slots as disabled (value 256). */
> +       for (i = 0; i < amx->soc_data->ram_depth * 4; i++)
> +               amx->map[i] = 256;
> 
>          tegra210_amx_dais[TEGRA_AMX_OUT_DAI_ID].capture.channels_max =
>                          amx->soc_data->max_ch;
> diff --git a/sound/soc/tegra/tegra210_amx.h b/sound/soc/tegra/tegra210_amx.h
> index 50a237b197ba..6df9ab0fe220 100644
> --- a/sound/soc/tegra/tegra210_amx.h
> +++ b/sound/soc/tegra/tegra210_amx.h
> @@ -8,6 +8,8 @@
>   #ifndef __TEGRA210_AMX_H__
>   #define __TEGRA210_AMX_H__
> 
> +#include <linux/types.h>
> +
>   /* Register offsets from TEGRA210_AMX*_BASE */
>   #define TEGRA210_AMX_RX_STATUS                 0x0c
>   #define TEGRA210_AMX_RX_INT_STATUS             0x10
> @@ -105,8 +107,7 @@ struct tegra210_amx_soc_data {
> 
>   struct tegra210_amx {
>          const struct tegra210_amx_soc_data *soc_data;
> -       unsigned int *map;
> -       unsigned int *byte_mask;
> +       u16 *map;
>          struct regmap *regmap;
>   };
> 
> --
> 2.34.1
> 


Same comments apply to the ADX patch (patch 1/2) as well.

Thanks,
Sheetal

[PATCH v2 0/2] ASoC: tegra210: simplify byte map handling in ADX and AMX

[Piyush Patle]

The Tegra210 ADX and AMX drivers both keep their "Byte Map N" ALSA
control state as a byte-packed u32 map[] array along with a separate
byte_mask[] bitmap. This is because the control range exposed to
userspace is [0, 256], where 256 is the "disabled" sentinel and
does not fit in a byte, so the two arrays have to be cross-checked
on every get()/put().

This series stores each slot as a u16 holding the user-visible
value directly, turning get_byte_map() into a direct return and
put_byte_map() into a compare-and-store. The hardware-facing packed
RAM word and the IN_BYTE_EN / OUT_BYTE_EN enable masks are computed
on the fly inside each write_map_ram() callback, which is the only
place that needs to know the hardware layout. The byte_mask[] field
is dropped from both driver state structs.

There is no userspace-visible ABI change. Control declarations,
ranges, initial values and handling of out-of-range writes is
preserved by treating values outside [0, 255] as disabled (256),
matching previous behavior. As a side effect each patch also fixes
a latent bug in put_byte_map() where an enabled-to-enabled value
change was not persisted.

The packed RAM word construction is also updated to ensure the shift
operates on a u32 value, avoiding potential undefined behavior due
to signed integer promotion.

Addresses TODO comments left in tegra210_{adx,amx}_get_byte_map().

Changes since v1:
 - Allocate byte_mask[] dynamically using kcalloc() based on
   soc_data->byte_mask_size instead of fixed-size arrays
 - Propagate -ENOMEM from write_map_ram() to callers
 - Replace magic numbers with TEGRA_{ADX,AMX}_SLOTS_PER_WORD
 - Use BITS_PER_BYTE and BITS_PER_TYPE() instead of literal shifts
 - Add <linux/bits.h> and <linux/slab.h> includes

Patch 1/2: ASoC: tegra210_adx: simplify byte map get/put logic
Patch 2/2: ASoC: tegra210_amx: simplify byte map get/put logic

-- 
2.34.1

[PATCH v2 1/2] ASoC: tegra210_adx: simplify byte map get/put logic

[Piyush Patle]

The byte-map controls ("Byte Map N") already expose a value range of
[0, 256] to userspace via SOC_SINGLE_EXT(), where 256 is the
"disabled" sentinel. The driver stored this state as a byte-packed
u32 map[] array plus a separate byte_mask[] bitmap tracking which
slots were enabled, because 256 does not fit in a byte. As a result
get_byte_map() had to consult byte_mask[] to decide whether to
report the stored byte or 256, and put_byte_map() had to keep the
two arrays in sync on every write.

Store each slot as a u16 holding the control value directly
(0..255 enabled, 256 disabled). This is the native representation
for what userspace already sees, so get_byte_map() becomes a direct
return and put_byte_map() becomes a compare-and-store. The
hardware-facing packed RAM word and the IN_BYTE_EN mask are now
derived on the fly inside tegra210_adx_write_map_ram() from the
slot array, which is the only place that needs to know about the
hardware layout.

The byte_mask scratch buffer is allocated dynamically using
kcalloc() based on soc_data->byte_mask_size, removing dependency
on SoC-specific constants. The byte_mask field is dropped from
struct tegra210_adx.

A new TEGRA_ADX_SLOTS_PER_WORD constant replaces the literal '4'
used for byte slots per RAM word, and BITS_PER_BYTE /
BITS_PER_TYPE() from <linux/bits.h> replace the literal '8' and
'32' shifts.

Slots are initialised to 256 in probe() so the default reported
value stays "disabled", matching previous behaviour. Values written
from userspace that fall outside [0, 255] are clamped to 256
("disabled") exactly as before -- no userspace-visible change.

As a side effect this also fixes a latent bug in the previous
put_byte_map(): because it compared the enable mask rather than the
stored byte, changing a slot from one enabled value to another
enabled value (e.g. 42 -> 99) would early-return without persisting
the new value, and the next CFG_RAM flush would still program the
old byte. The new implementation compares the stored value itself,
so this case is now handled correctly.

Addresses TODO left in tegra210_adx_get_byte_map().

Signed-off-by: Piyush Patle <piyushpatle228@gmail.com>
---
Changes since v1:
 - Allocate byte_mask[] dynamically using kcalloc()
 - Propagate -ENOMEM from write_map_ram()
 - Replace magic numbers with TEGRA_ADX_SLOTS_PER_WORD
 - Use BITS_PER_BYTE and BITS_PER_TYPE()
 - Add <linux/bits.h> and <linux/slab.h>

 sound/soc/tegra/tegra210_adx.c | 99 ++++++++++++++++++----------------
 sound/soc/tegra/tegra210_adx.h |  6 ++-
 2 files changed, 56 insertions(+), 49 deletions(-)

diff --git a/sound/soc/tegra/tegra210_adx.c b/sound/soc/tegra/tegra210_adx.c
index 95875c75ddf8..2a2f7e9e90ca 100644
--- a/sound/soc/tegra/tegra210_adx.c
+++ b/sound/soc/tegra/tegra210_adx.c
@@ -4,6 +4,7 @@
 //
 // tegra210_adx.c - Tegra210 ADX driver
 
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/io.h>
@@ -13,6 +14,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
+#include <linux/slab.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -45,25 +47,49 @@ static const struct reg_default tegra264_adx_reg_defaults[] = {
 	{ TEGRA264_ADX_CFG_RAM_CTRL, 0x00004000},
 };
 
-static void tegra210_adx_write_map_ram(struct tegra210_adx *adx)
+static int tegra210_adx_write_map_ram(struct tegra210_adx *adx)
 {
+	const unsigned int bits_per_mask = BITS_PER_TYPE(*adx->map) * BITS_PER_BYTE;
+	unsigned int *byte_mask;
 	int i;
 
+	byte_mask = kcalloc(adx->soc_data->byte_mask_size, sizeof(*byte_mask),
+			    GFP_KERNEL);
+	if (!byte_mask)
+		return -ENOMEM;
+
 	regmap_write(adx->regmap, TEGRA210_ADX_CFG_RAM_CTRL +
 			adx->soc_data->cya_offset,
 		     TEGRA210_ADX_CFG_RAM_CTRL_SEQ_ACCESS_EN |
 		     TEGRA210_ADX_CFG_RAM_CTRL_ADDR_INIT_EN |
 		     TEGRA210_ADX_CFG_RAM_CTRL_RW_WRITE);
 
-	for (i = 0; i < adx->soc_data->ram_depth; i++)
+	for (i = 0; i < adx->soc_data->ram_depth; i++) {
+		u32 word = 0;
+		int b;
+
+		for (b = 0; b < TEGRA_ADX_SLOTS_PER_WORD; b++) {
+			unsigned int slot = i * TEGRA_ADX_SLOTS_PER_WORD + b;
+			u16 val = adx->map[slot];
+
+			if (val >= 256)
+				continue;
+
+			word |= (u32)val << (b * BITS_PER_BYTE);
+			byte_mask[slot / bits_per_mask] |= 1U << (slot % bits_per_mask);
+		}
 		regmap_write(adx->regmap, TEGRA210_ADX_CFG_RAM_DATA +
-				adx->soc_data->cya_offset,
-			     adx->map[i]);
+				adx->soc_data->cya_offset, word);
+	}
 
 	for (i = 0; i < adx->soc_data->byte_mask_size; i++)
 		regmap_write(adx->regmap,
 			     TEGRA210_ADX_IN_BYTE_EN0 + (i * TEGRA210_ADX_AUDIOCIF_CH_STRIDE),
-			     adx->byte_mask[i]);
+			     byte_mask[i]);
+
+	kfree(byte_mask);
+
+	return 0;
 }
 
 static int tegra210_adx_startup(struct snd_pcm_substream *substream,
@@ -118,9 +144,7 @@ static int tegra210_adx_runtime_resume(struct device *dev)
 	regcache_cache_only(adx->regmap, false);
 	regcache_sync(adx->regmap);
 
-	tegra210_adx_write_map_ram(adx);
-
-	return 0;
+	return tegra210_adx_write_map_ram(adx);
 }
 
 static int tegra210_adx_set_audio_cif(struct snd_soc_dai *dai,
@@ -188,27 +212,10 @@ static int tegra210_adx_get_byte_map(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_component *cmpnt = snd_kcontrol_chip(kcontrol);
 	struct tegra210_adx *adx = snd_soc_component_get_drvdata(cmpnt);
-	struct soc_mixer_control *mc;
-	unsigned char *bytes_map = (unsigned char *)adx->map;
-	int enabled;
-
-	mc = (struct soc_mixer_control *)kcontrol->private_value;
-	enabled = adx->byte_mask[mc->reg / 32] & (1 << (mc->reg % 32));
+	struct soc_mixer_control *mc =
+		(struct soc_mixer_control *)kcontrol->private_value;
 
-	/*
-	 * TODO: Simplify this logic to just return from bytes_map[]
-	 *
-	 * Presently below is required since bytes_map[] is
-	 * tightly packed and cannot store the control value of 256.
-	 * Byte mask state is used to know if 256 needs to be returned.
-	 * Note that for control value of 256, the put() call stores 0
-	 * in the bytes_map[] and disables the corresponding bit in
-	 * byte_mask[].
-	 */
-	if (enabled)
-		ucontrol->value.integer.value[0] = bytes_map[mc->reg];
-	else
-		ucontrol->value.integer.value[0] = 256;
+	ucontrol->value.integer.value[0] = adx->map[mc->reg];
 
 	return 0;
 }
@@ -218,23 +225,22 @@ static int tegra210_adx_put_byte_map(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_component *cmpnt = snd_kcontrol_chip(kcontrol);
 	struct tegra210_adx *adx = snd_soc_component_get_drvdata(cmpnt);
-	unsigned char *bytes_map = (unsigned char *)adx->map;
-	int value = ucontrol->value.integer.value[0];
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	unsigned int mask_val = adx->byte_mask[mc->reg / 32];
+	unsigned int value = ucontrol->value.integer.value[0];
 
-	if (value >= 0 && value <= 255)
-		mask_val |= (1 << (mc->reg % 32));
-	else
-		mask_val &= ~(1 << (mc->reg % 32));
+	/*
+	 * Match the previous behaviour: any value outside [0, 255] is
+	 * treated as the "disabled" sentinel (256). Negative values from
+	 * userspace fold in through the unsigned cast and are caught here.
+	 */
+	if (value > 255)
+		value = 256;
 
-	if (mask_val == adx->byte_mask[mc->reg / 32])
+	if (adx->map[mc->reg] == value)
 		return 0;
 
-	/* Update byte map and slot */
-	bytes_map[mc->reg] = value % 256;
-	adx->byte_mask[mc->reg / 32] = mask_val;
+	adx->map[mc->reg] = value;
 
 	return 1;
 }
@@ -675,7 +681,7 @@ static int tegra210_adx_platform_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	struct tegra210_adx_soc_data *soc_data;
 	void __iomem *regs;
-	int err;
+	int err, i;
 
 	adx = devm_kzalloc(dev, sizeof(*adx), GFP_KERNEL);
 	if (!adx)
@@ -700,16 +706,15 @@ static int tegra210_adx_platform_probe(struct platform_device *pdev)
 
 	regcache_cache_only(adx->regmap, true);
 
-	adx->map = devm_kzalloc(dev, soc_data->ram_depth * sizeof(*adx->map),
-				GFP_KERNEL);
+	adx->map = devm_kcalloc(dev,
+				soc_data->ram_depth * TEGRA_ADX_SLOTS_PER_WORD,
+				sizeof(*adx->map), GFP_KERNEL);
 	if (!adx->map)
 		return -ENOMEM;
 
-	adx->byte_mask = devm_kzalloc(dev,
-				      soc_data->byte_mask_size * sizeof(*adx->byte_mask),
-				      GFP_KERNEL);
-	if (!adx->byte_mask)
-		return -ENOMEM;
+	/* Initialise all byte map slots as disabled (value 256). */
+	for (i = 0; i < soc_data->ram_depth * TEGRA_ADX_SLOTS_PER_WORD; i++)
+		adx->map[i] = 256;
 
 	tegra210_adx_dais[TEGRA_ADX_IN_DAI_ID].playback.channels_max =
 			adx->soc_data->max_ch;
diff --git a/sound/soc/tegra/tegra210_adx.h b/sound/soc/tegra/tegra210_adx.h
index 176a4e40de0a..7ea623b4183b 100644
--- a/sound/soc/tegra/tegra210_adx.h
+++ b/sound/soc/tegra/tegra210_adx.h
@@ -8,6 +8,8 @@
 #ifndef __TEGRA210_ADX_H__
 #define __TEGRA210_ADX_H__
 
+#include <linux/types.h>
+
 /* Register offsets from TEGRA210_ADX*_BASE */
 #define TEGRA210_ADX_RX_STATUS		0x0c
 #define TEGRA210_ADX_RX_INT_STATUS	0x10
@@ -61,6 +63,7 @@
 #define TEGRA210_ADX_SOFT_RESET_SOFT_DEFAULT		(0 << TEGRA210_ADX_SOFT_RESET_SOFT_RESET_SHIFT)
 
 #define TEGRA210_ADX_AUDIOCIF_CH_STRIDE		4
+#define TEGRA_ADX_SLOTS_PER_WORD		4
 #define TEGRA210_ADX_RAM_DEPTH			16
 #define TEGRA210_ADX_MAP_STREAM_NUMBER_SHIFT	6
 #define TEGRA210_ADX_MAP_WORD_NUMBER_SHIFT	2
@@ -88,8 +91,7 @@ struct tegra210_adx_soc_data {
 
 struct tegra210_adx {
 	struct regmap *regmap;
-	unsigned int *map;
-	unsigned int *byte_mask;
+	u16 *map;
 	const struct tegra210_adx_soc_data *soc_data;
 };
 
-- 
2.34.1

Re: [PATCH v2 1/2] ASoC: tegra210_adx: simplify byte map get/put logic

[Jon Hunter]

On 08/04/2026 18:08, Piyush Patle wrote:
> The byte-map controls ("Byte Map N") already expose a value range of
> [0, 256] to userspace via SOC_SINGLE_EXT(), where 256 is the
> "disabled" sentinel. The driver stored this state as a byte-packed
> u32 map[] array plus a separate byte_mask[] bitmap tracking which
> slots were enabled, because 256 does not fit in a byte. As a result
> get_byte_map() had to consult byte_mask[] to decide whether to
> report the stored byte or 256, and put_byte_map() had to keep the
> two arrays in sync on every write.
> 
> Store each slot as a u16 holding the control value directly
> (0..255 enabled, 256 disabled). This is the native representation
> for what userspace already sees, so get_byte_map() becomes a direct
> return and put_byte_map() becomes a compare-and-store. The
> hardware-facing packed RAM word and the IN_BYTE_EN mask are now
> derived on the fly inside tegra210_adx_write_map_ram() from the
> slot array, which is the only place that needs to know about the
> hardware layout.
> 
> The byte_mask scratch buffer is allocated dynamically using
> kcalloc() based on soc_data->byte_mask_size, removing dependency
> on SoC-specific constants. The byte_mask field is dropped from
> struct tegra210_adx.

So this was already the case. However ...


> -static void tegra210_adx_write_map_ram(struct tegra210_adx *adx)
> +static int tegra210_adx_write_map_ram(struct tegra210_adx *adx)
>   {
> +	const unsigned int bits_per_mask = BITS_PER_TYPE(*adx->map) * BITS_PER_BYTE;
> +	unsigned int *byte_mask;
>   	int i;
>   
> +	byte_mask = kcalloc(adx->soc_data->byte_mask_size, sizeof(*byte_mask),
> +			    GFP_KERNEL);
> +	if (!byte_mask)
> +		return -ENOMEM;
> +

Now you are allocating this everytime this function is called (which 
happens on RPM resume) instead of ...

> @@ -700,16 +706,15 @@ static int tegra210_adx_platform_probe(struct platform_device *pdev)
>   
>   	regcache_cache_only(adx->regmap, true);
>   
> -	adx->map = devm_kzalloc(dev, soc_data->ram_depth * sizeof(*adx->map),
> -				GFP_KERNEL);
> +	adx->map = devm_kcalloc(dev,
> +				soc_data->ram_depth * TEGRA_ADX_SLOTS_PER_WORD,
> +				sizeof(*adx->map), GFP_KERNEL);
>   	if (!adx->map)
>   		return -ENOMEM;
>   
> -	adx->byte_mask = devm_kzalloc(dev,
> -				      soc_data->byte_mask_size * sizeof(*adx->byte_mask),
> -				      GFP_KERNEL);
> -	if (!adx->byte_mask)
> -		return -ENOMEM;

... here in the probe function, which makes more sense. IOW I am not 
sure why you have changed this.

Jon

-- 
nvpublic

Re: [PATCH v2 1/2] ASoC: tegra210_adx: simplify byte map get/put logic

[Piyush Patle]

On Wed, Apr 8, 2026 at 11:09 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 08/04/2026 18:08, Piyush Patle wrote:
> > The byte-map controls ("Byte Map N") already expose a value range of
> > [0, 256] to userspace via SOC_SINGLE_EXT(), where 256 is the
> > "disabled" sentinel. The driver stored this state as a byte-packed
> > u32 map[] array plus a separate byte_mask[] bitmap tracking which
> > slots were enabled, because 256 does not fit in a byte. As a result
> > get_byte_map() had to consult byte_mask[] to decide whether to
> > report the stored byte or 256, and put_byte_map() had to keep the
> > two arrays in sync on every write.
> >
> > Store each slot as a u16 holding the control value directly
> > (0..255 enabled, 256 disabled). This is the native representation
> > for what userspace already sees, so get_byte_map() becomes a direct
> > return and put_byte_map() becomes a compare-and-store. The
> > hardware-facing packed RAM word and the IN_BYTE_EN mask are now
> > derived on the fly inside tegra210_adx_write_map_ram() from the
> > slot array, which is the only place that needs to know about the
> > hardware layout.
> >
> > The byte_mask scratch buffer is allocated dynamically using
> > kcalloc() based on soc_data->byte_mask_size, removing dependency
> > on SoC-specific constants. The byte_mask field is dropped from
> > struct tegra210_adx.
>
> So this was already the case. However ...
>
>
> > -static void tegra210_adx_write_map_ram(struct tegra210_adx *adx)
> > +static int tegra210_adx_write_map_ram(struct tegra210_adx *adx)
> >   {
> > +     const unsigned int bits_per_mask = BITS_PER_TYPE(*adx->map) * BITS_PER_BYTE;
> > +     unsigned int *byte_mask;
> >       int i;
> >
> > +     byte_mask = kcalloc(adx->soc_data->byte_mask_size, sizeof(*byte_mask),
> > +                         GFP_KERNEL);
> > +     if (!byte_mask)
> > +             return -ENOMEM;
> > +
>
> Now you are allocating this everytime this function is called (which
> happens on RPM resume) instead of ...

You're right, I was wrong here. I read Sheetal's v1comment as
"allocate it dynamically" and over-corrected by moving the
allocation into write_map_ram() itself.

>
> > @@ -700,16 +706,15 @@ static int tegra210_adx_platform_probe(struct platform_device *pdev)
> >
> >       regcache_cache_only(adx->regmap, true);
> >
> > -     adx->map = devm_kzalloc(dev, soc_data->ram_depth * sizeof(*adx->map),
> > -                             GFP_KERNEL);
> > +     adx->map = devm_kcalloc(dev,
> > +                             soc_data->ram_depth * TEGRA_ADX_SLOTS_PER_WORD,
> > +                             sizeof(*adx->map), GFP_KERNEL);
> >       if (!adx->map)
> >               return -ENOMEM;
> >
> > -     adx->byte_mask = devm_kzalloc(dev,
> > -                                   soc_data->byte_mask_size * sizeof(*adx->byte_mask),
> > -                                   GFP_KERNEL);
> > -     if (!adx->byte_mask)
> > -             return -ENOMEM;
>
> ... here in the probe function, which makes more sense. IOW I am not
> sure why you have changed this.
>
>
The intent was only to drop the dependency on
the SoC-specific TEGRA264_*_BYTE_MASK_COUNT
constant; the lifetime should still be probe-scoped

For v3 I'll keep byte_mask as a member of struct tegra210_adx and
allocate it once in probe() with:

  adx->byte_mask = devm_kcalloc(dev, soc_data->byte_mask_size,
                               sizeof(*adx->byte_mask),
                               GFP_KERNEL);

That keeps Sheetal's "no chip-specific constant" requirement (size
is still soc_data->byte_mask_size) while avoiding the per-RPM-resume
allocation.
I think that would be better, Right?

Same change in tegra210_amx.c.

Re: [PATCH v2 1/2] ASoC: tegra210_adx: simplify byte map get/put logic

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

On Wed, Apr 08, 2026 at 10:38:17PM +0530, Piyush Patle wrote:

> +static int tegra210_adx_write_map_ram(struct tegra210_adx *adx)
>  {
> +	const unsigned int bits_per_mask = BITS_PER_TYPE(*adx->map) * BITS_PER_BYTE;

Why are we multiplying by BITS_PER_BYTE here?  We've got a number of
bits already from BITS_PER_TYPE().

> +	for (i = 0; i < adx->soc_data->ram_depth; i++) {
> +		u32 word = 0;
> +		int b;
> +
> +		for (b = 0; b < TEGRA_ADX_SLOTS_PER_WORD; b++) {
> +			unsigned int slot = i * TEGRA_ADX_SLOTS_PER_WORD + b;
> +			u16 val = adx->map[slot];
> +
> +			if (val >= 256)
> +				continue;
> +
> +			word |= (u32)val << (b * BITS_PER_BYTE);
> +			byte_mask[slot / bits_per_mask] |= 1U << (slot % bits_per_mask);

How big can bits_per_mask get?

> @@ -118,9 +144,7 @@ static int tegra210_adx_runtime_resume(struct device *dev)
>  	regcache_cache_only(adx->regmap, false);
>  	regcache_sync(adx->regmap);
>  
> -	tegra210_adx_write_map_ram(adx);
> -
> -	return 0;
> +	return tegra210_adx_write_map_ram(adx);

We need to unwind at least the regcache_cache_only() above if resume
fails.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] ASoC: tegra210_adx: simplify byte map get/put logic

[Piyush Patle]

> > +static int tegra210_adx_write_map_ram(struct tegra210_adx *adx)
> >  {
> > +     const unsigned int bits_per_mask = BITS_PER_TYPE(*adx->map) * BITS_PER_BYTE;
>
> Why are we multiplying by BITS_PER_BYTE here?  We've got a number of
> bits already from BITS_PER_TYPE().

Okay correct, that's a bug BITS_PER_TYPE() already returns bits,
so the extra * BITS_PER_BYTE is a unit error. It also references
the wrong type: the bitmap word is unsigned int (which is what the
original code's hard-coded 32 came from), not the map element type.

For v3 I'll change it to:
    const unsigned int bits_per_mask = BITS_PER_TYPE(*adx->byte_mask);

>
> > +     for (i = 0; i < adx->soc_data->ram_depth; i++) {
> > +             u32 word = 0;
> > +             int b;
> > +
> > +             for (b = 0; b < TEGRA_ADX_SLOTS_PER_WORD; b++) {
> > +                     unsigned int slot = i * TEGRA_ADX_SLOTS_PER_WORD + b;
> > +                     u16 val = adx->map[slot];
> > +
> > +                     if (val >= 256)
> > +                             continue;
> > +
> > +                     word |= (u32)val << (b * BITS_PER_BYTE);
> > +                     byte_mask[slot / bits_per_mask] |= 1U << (slot % bits_per_mask);
>
> How big can bits_per_mask get?

With the fix above, bits_per_mask == BITS_PER_TYPE(unsigned int) ==
32,
matching the original "slot / 32" / "slot % 32" expressions exactly.

>
> > @@ -118,9 +144,7 @@ static int tegra210_adx_runtime_resume(struct device *dev)
> >       regcache_cache_only(adx->regmap, false);
> >       regcache_sync(adx->regmap);
> >
> > -     tegra210_adx_write_map_ram(adx);
> > -
> > -     return 0;
> > +     return tegra210_adx_write_map_ram(adx);
>
> We need to unwind at least the regcache_cache_only() above if resume
> fails.

As per Jon's comment on the same patch, I'm moving the
byte_mask buffer back to a probe-time devm_kcalloc() in v3, so
write_map_ram() no longer has a failure path. runtime_resume() will
go back to returning 0 unconditionally and the regcache_cache_only()
unwind won't be needed. If write_map_ram() ever grows another failure
mode in the future, the unwind would have to be added then.

Thanks
Piyush Patle

[PATCH v2 2/2] ASoC: tegra210_amx: simplify byte map get/put logic

[Piyush Patle]

The byte-map controls ("Byte Map N") already expose a value range of
[0, 256] to userspace via SOC_SINGLE_EXT(), where 256 is the
"disabled" sentinel. The driver stored this state as a byte-packed
u32 map[] array plus a separate byte_mask[] bitmap tracking which
slots were enabled, because 256 does not fit in a byte. As a result
get_byte_map() had to consult byte_mask[] to decide whether to
report the stored byte or 256, and put_byte_map() had to keep the
two arrays in sync on every write.

Store each slot as a u16 holding the control value directly
(0..255 enabled, 256 disabled). This is the native representation
for what userspace already sees, so get_byte_map() becomes a direct
return and put_byte_map() becomes a compare-and-store. The
hardware-facing packed RAM word and the OUT_BYTE_EN mask are now
derived on the fly inside tegra210_amx_write_map_ram() from the
slot array, which is the only place that needs to know about the
hardware layout.

The byte_mask scratch buffer is allocated dynamically using
kcalloc() based on soc_data->byte_mask_size, removing dependency
on SoC-specific constants. The byte_mask field is dropped from
struct tegra210_amx.

A new TEGRA_AMX_SLOTS_PER_WORD constant replaces the literal '4'
used for byte slots per RAM word, and BITS_PER_BYTE /
BITS_PER_TYPE() from <linux/bits.h> replace the literal '8' and
'32' shifts.

Slots are initialised to 256 in probe() so the default reported
value stays "disabled", matching previous behaviour. Values written
from userspace that fall outside [0, 255] are clamped to 256
("disabled") exactly as before -- no userspace-visible change.

As a side effect this also fixes a latent bug in the previous
put_byte_map(): because it compared the enable mask rather than the
stored byte, changing a slot from one enabled value to another
enabled value (e.g. 42 -> 99) would early-return without persisting
the new value, and the next CFG_RAM flush would still program the
old byte. The new implementation compares the stored value itself,
so this case is now handled correctly.

Addresses TODO left in tegra210_amx_get_byte_map().

Signed-off-by: Piyush Patle <piyushpatle228@gmail.com>
---
Changes since v1:
 - Allocate byte_mask[] dynamically using kcalloc()
 - Propagate -ENOMEM from write_map_ram()
 - Replace magic numbers with TEGRA_AMX_SLOTS_PER_WORD
 - Use BITS_PER_BYTE and BITS_PER_TYPE()
 - Add <linux/bits.h> and <linux/slab.h>

 sound/soc/tegra/tegra210_amx.c | 96 ++++++++++++++++++----------------
 sound/soc/tegra/tegra210_amx.h |  6 ++-
 2 files changed, 54 insertions(+), 48 deletions(-)

diff --git a/sound/soc/tegra/tegra210_amx.c b/sound/soc/tegra/tegra210_amx.c
index bfda82505298..bc808fabe171 100644
--- a/sound/soc/tegra/tegra210_amx.c
+++ b/sound/soc/tegra/tegra210_amx.c
@@ -4,6 +4,7 @@
 //
 // tegra210_amx.c - Tegra210 AMX driver
 
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/io.h>
@@ -12,6 +13,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
+#include <linux/slab.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -58,23 +60,48 @@ static const struct reg_default tegra264_amx_reg_defaults[] = {
 	{ TEGRA264_AMX_CFG_RAM_CTRL, 0x00004000},
 };
 
-static void tegra210_amx_write_map_ram(struct tegra210_amx *amx)
+static int tegra210_amx_write_map_ram(struct tegra210_amx *amx)
 {
+	const unsigned int bits_per_mask = BITS_PER_TYPE(*amx->map) * BITS_PER_BYTE;
+	unsigned int *byte_mask;
 	int i;
 
+	byte_mask = kcalloc(amx->soc_data->byte_mask_size, sizeof(*byte_mask),
+			    GFP_KERNEL);
+	if (!byte_mask)
+		return -ENOMEM;
+
 	regmap_write(amx->regmap, TEGRA210_AMX_CFG_RAM_CTRL + amx->soc_data->reg_offset,
 		     TEGRA210_AMX_CFG_RAM_CTRL_SEQ_ACCESS_EN |
 		     TEGRA210_AMX_CFG_RAM_CTRL_ADDR_INIT_EN |
 		     TEGRA210_AMX_CFG_RAM_CTRL_RW_WRITE);
 
-	for (i = 0; i < amx->soc_data->ram_depth; i++)
+	for (i = 0; i < amx->soc_data->ram_depth; i++) {
+		u32 word = 0;
+		int b;
+
+		for (b = 0; b < TEGRA_AMX_SLOTS_PER_WORD; b++) {
+			unsigned int slot = i * TEGRA_AMX_SLOTS_PER_WORD + b;
+			u16 val = amx->map[slot];
+
+			if (val >= 256)
+				continue;
+
+			word |= (u32)val << (b * BITS_PER_BYTE);
+			byte_mask[slot / bits_per_mask] |= 1U << (slot % bits_per_mask);
+		}
 		regmap_write(amx->regmap, TEGRA210_AMX_CFG_RAM_DATA + amx->soc_data->reg_offset,
-			     amx->map[i]);
+			     word);
+	}
 
 	for (i = 0; i < amx->soc_data->byte_mask_size; i++)
 		regmap_write(amx->regmap,
 			     TEGRA210_AMX_OUT_BYTE_EN0 + (i * TEGRA210_AMX_AUDIOCIF_CH_STRIDE),
-			     amx->byte_mask[i]);
+			     byte_mask[i]);
+
+	kfree(byte_mask);
+
+	return 0;
 }
 
 static int tegra210_amx_startup(struct snd_pcm_substream *substream,
@@ -134,9 +161,7 @@ static int tegra210_amx_runtime_resume(struct device *dev)
 		TEGRA210_AMX_CTRL_RX_DEP_MASK,
 		TEGRA210_AMX_WAIT_ON_ANY << TEGRA210_AMX_CTRL_RX_DEP_SHIFT);
 
-	tegra210_amx_write_map_ram(amx);
-
-	return 0;
+	return tegra210_amx_write_map_ram(amx);
 }
 
 static int tegra210_amx_set_audio_cif(struct snd_soc_dai *dai,
@@ -212,26 +237,8 @@ static int tegra210_amx_get_byte_map(struct snd_kcontrol *kcontrol,
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
 	struct tegra210_amx *amx = snd_soc_component_get_drvdata(cmpnt);
-	unsigned char *bytes_map = (unsigned char *)amx->map;
-	int reg = mc->reg;
-	int enabled;
-
-	enabled = amx->byte_mask[reg / 32] & (1 << (reg % 32));
 
-	/*
-	 * TODO: Simplify this logic to just return from bytes_map[]
-	 *
-	 * Presently below is required since bytes_map[] is
-	 * tightly packed and cannot store the control value of 256.
-	 * Byte mask state is used to know if 256 needs to be returned.
-	 * Note that for control value of 256, the put() call stores 0
-	 * in the bytes_map[] and disables the corresponding bit in
-	 * byte_mask[].
-	 */
-	if (enabled)
-		ucontrol->value.integer.value[0] = bytes_map[reg];
-	else
-		ucontrol->value.integer.value[0] = 256;
+	ucontrol->value.integer.value[0] = amx->map[mc->reg];
 
 	return 0;
 }
@@ -243,22 +250,20 @@ static int tegra210_amx_put_byte_map(struct snd_kcontrol *kcontrol,
 		(struct soc_mixer_control *)kcontrol->private_value;
 	struct snd_soc_component *cmpnt = snd_kcontrol_chip(kcontrol);
 	struct tegra210_amx *amx = snd_soc_component_get_drvdata(cmpnt);
-	unsigned char *bytes_map = (unsigned char *)amx->map;
-	int reg = mc->reg;
-	int value = ucontrol->value.integer.value[0];
-	unsigned int mask_val = amx->byte_mask[reg / 32];
+	unsigned int value = ucontrol->value.integer.value[0];
 
-	if (value >= 0 && value <= 255)
-		mask_val |= (1 << (reg % 32));
-	else
-		mask_val &= ~(1 << (reg % 32));
+	/*
+	 * Match the previous behaviour: any value outside [0, 255] is
+	 * treated as the "disabled" sentinel (256). Negative values from
+	 * userspace fold in through the unsigned cast and are caught here.
+	 */
+	if (value > 255)
+		value = 256;
 
-	if (mask_val == amx->byte_mask[reg / 32])
+	if (amx->map[mc->reg] == value)
 		return 0;
 
-	/* Update byte map and slot */
-	bytes_map[reg] = value % 256;
-	amx->byte_mask[reg / 32] = mask_val;
+	amx->map[mc->reg] = value;
 
 	return 1;
 }
@@ -727,7 +732,7 @@ static int tegra210_amx_platform_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct tegra210_amx *amx;
 	void __iomem *regs;
-	int err;
+	int err, i;
 
 	amx = devm_kzalloc(dev, sizeof(*amx), GFP_KERNEL);
 	if (!amx)
@@ -750,16 +755,15 @@ static int tegra210_amx_platform_probe(struct platform_device *pdev)
 
 	regcache_cache_only(amx->regmap, true);
 
-	amx->map = devm_kzalloc(dev, amx->soc_data->ram_depth * sizeof(*amx->map),
-				GFP_KERNEL);
+	amx->map = devm_kcalloc(dev,
+				amx->soc_data->ram_depth * TEGRA_AMX_SLOTS_PER_WORD,
+				sizeof(*amx->map), GFP_KERNEL);
 	if (!amx->map)
 		return -ENOMEM;
 
-	amx->byte_mask = devm_kzalloc(dev,
-				      amx->soc_data->byte_mask_size * sizeof(*amx->byte_mask),
-				      GFP_KERNEL);
-	if (!amx->byte_mask)
-		return -ENOMEM;
+	/* Initialise all byte map slots as disabled (value 256). */
+	for (i = 0; i < amx->soc_data->ram_depth * TEGRA_AMX_SLOTS_PER_WORD; i++)
+		amx->map[i] = 256;
 
 	tegra210_amx_dais[TEGRA_AMX_OUT_DAI_ID].capture.channels_max =
 			amx->soc_data->max_ch;
diff --git a/sound/soc/tegra/tegra210_amx.h b/sound/soc/tegra/tegra210_amx.h
index 50a237b197ba..2c125a191a1a 100644
--- a/sound/soc/tegra/tegra210_amx.h
+++ b/sound/soc/tegra/tegra210_amx.h
@@ -8,6 +8,8 @@
 #ifndef __TEGRA210_AMX_H__
 #define __TEGRA210_AMX_H__
 
+#include <linux/types.h>
+
 /* Register offsets from TEGRA210_AMX*_BASE */
 #define TEGRA210_AMX_RX_STATUS			0x0c
 #define TEGRA210_AMX_RX_INT_STATUS		0x10
@@ -73,6 +75,7 @@
 #define TEGRA210_AMX_SOFT_RESET_SOFT_RESET_MASK		TEGRA210_AMX_SOFT_RESET_SOFT_EN
 
 #define TEGRA210_AMX_AUDIOCIF_CH_STRIDE		4
+#define TEGRA_AMX_SLOTS_PER_WORD		4
 #define TEGRA210_AMX_RAM_DEPTH			16
 #define TEGRA210_AMX_MAP_STREAM_NUM_SHIFT	6
 #define TEGRA210_AMX_MAP_WORD_NUM_SHIFT		2
@@ -105,8 +108,7 @@ struct tegra210_amx_soc_data {
 
 struct tegra210_amx {
 	const struct tegra210_amx_soc_data *soc_data;
-	unsigned int *map;
-	unsigned int *byte_mask;
+	u16 *map;
 	struct regmap *regmap;
 };
 
-- 
2.34.1

Re: [PATCH v2 2/2] ASoC: tegra210_amx: simplify byte map get/put logic

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 225 bytes --]

On Wed, Apr 08, 2026 at 10:38:18PM +0530, Piyush Patle wrote:
> The byte-map controls ("Byte Map N") already expose a value range of
> [0, 256] to userspace via SOC_SINGLE_EXT(), where 256 is the

Similar issues in this one.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] ASoC: tegra210: simplify byte map handling in ADX and AMX

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]

On Wed, Apr 08, 2026 at 10:38:16PM +0530, Piyush Patle wrote:
> The Tegra210 ADX and AMX drivers both keep their "Byte Map N" ALSA
> control state as a byte-packed u32 map[] array along with a separate
> byte_mask[] bitmap. This is because the control range exposed to
> userspace is [0, 256], where 256 is the "disabled" sentinel and
> does not fit in a byte, so the two arrays have to be cross-checked
> on every get()/put().

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]