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@xxxxxxxxxx> 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.