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

["Sheetal ." <sheetal@nvidia.com>]

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