Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20231002090434.1896-1-shenghao-ding@ti.com>
In-Reply-To: <20231002090434.1896-1-shenghao-ding@ti.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Mon, 2 Oct 2023 21:24:21 +0200
Message-ID: <CAMuHMdXr5oRgkFqYZnPe3Xdyq_QjtzhfL8Wa_e9JA0S1XhEhWw@mail.gmail.com>
Subject: Re: [PATCH v1] ASoC: tas2781: fixed compiling issue in m68k
To:     Shenghao Ding <shenghao-ding@ti.com>
Cc:     broonie@kernel.org, robh+dt@kernel.org,
        andriy.shevchenko@linux.intel.com, lgirdwood@gmail.com,
        perex@perex.cz, pierre-louis.bossart@linux.intel.com,
        kevin-lu@ti.com, 13916275206@139.com, alsa-devel@alsa-project.org,
        linux-kernel@vger.kernel.org, liam.r.girdwood@intel.com,
        mengdong.lin@intel.com, baojun.xu@ti.com,
        thomas.gfeller@q-drop.com, peeyush@ti.com, navada@ti.com,
        tiwai@suse.de
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

Hi Shenghao,

Thanks for your patch!

On Mon, Oct 2, 2023 at 4:38 PM Shenghao Ding <shenghao-ding@ti.com> wrote:
> fixed m68k compiling issue: mapping table can save code field; storing the

Please replicate the actual error message from the compiler, so it
is recorded in the commit description, and easy to find when someone
searches for the actual error message

From the Reported-by (which is not included in the actual description,
as it is below the "---"):

       {standard input}: Assembler messages:
    >> {standard input}:992: Error: value -148 out of range
       {standard input}:992: Error: value of ffffff6c too large for
field of 1 byte at 0000045f

> dev_idx as a member of block can reduce unnecessary  time and system
> resource comsumption of dev_idx mapping every time the block data writing
> to the dsp.

I am sorry, but I don't understand what this means.
Can you please elaborate?

Also, can you please explain what the real issue is?
(My first guess when seeing the error message before was that a  signed
 integer is truncated to an unsigned char or so, but I couldn't see
 immediately where that is happening)

> Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>
>
> ---
> Changes in v1:
>  - | Reported-by: kernel test robot <lkp@intel.com>
>    | Closes:
>    | https://lore.kernel.org/oe-kbuild-all/202309222048.RnSqEIK5-lkp@intel.
>    | com/

> --- a/include/sound/tas2781-dsp.h
> +++ b/include/sound/tas2781-dsp.h
> @@ -77,6 +77,11 @@ struct tasdev_blk {
>         unsigned int nr_cmds;
>         unsigned int blk_size;
>         unsigned int nr_subblocks;
> +       /* fixed m68k compiling issue, storing the dev_idx as a member of block
> +        * can reduce unnecessary timeand system resource comsumption of
> +        * dev_idx mapping every time the block data writing to the dsp.
> +        */

What is so special about "m68k" that it (1) fails there (and only there?
and only for some config/compiler combos, as it builds fine for me?),
and (2) is needed to mention this in comments all over the place?

> +       unsigned char dev_idx;
>         unsigned char *data;
>  };
>
> diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c
> index eb55abae0d7b..e27775d834e9 100644
> --- a/sound/soc/codecs/tas2781-fmwlib.c
> +++ b/sound/soc/codecs/tas2781-fmwlib.c
> @@ -80,10 +80,72 @@ struct tas_crc {
>         unsigned char len;
>  };
>
> +struct blktyp_devidx_map {
> +       unsigned char blktyp;
> +       unsigned char dev_idx;
> +};
> +
>  static const char deviceNumber[TASDEVICE_DSP_TAS_MAX_DEVICE] = {
>         1, 2, 1, 2, 1, 1, 0, 2, 4, 3, 1, 2, 3, 4
>  };
>
> +/* fixed m68k compiling issue: mapping table can save code field */
> +static const struct blktyp_devidx_map ppc3_tas2781_mapping_table[] = {
> +       { MAIN_ALL_DEVICES_1X, 0x80 },
> +       { MAIN_DEVICE_A_1X, 0x81 },
> +       { COEFF_DEVICE_A_1X, 0xC1 },
> +       { PRE_DEVICE_A_1X, 0xC1 },
> +       { PRE_SOFTWARE_RESET_DEVICE_A, 0xC1 },
> +       { POST_SOFTWARE_RESET_DEVICE_A, 0xC1 },
> +       { MAIN_DEVICE_B_1X, 0x82 },
> +       { COEFF_DEVICE_B_1X, 0xC2 },
> +       { PRE_DEVICE_B_1X, 0xC2 },
> +       { PRE_SOFTWARE_RESET_DEVICE_B, 0xC2 },
> +       { POST_SOFTWARE_RESET_DEVICE_B, 0xC2 },
> +       { MAIN_DEVICE_C_1X, 0x83 },
> +       { COEFF_DEVICE_C_1X, 0xC3 },
> +       { PRE_DEVICE_C_1X, 0xC3 },
> +       { PRE_SOFTWARE_RESET_DEVICE_C, 0xC3 },
> +       { POST_SOFTWARE_RESET_DEVICE_C, 0xC3 },
> +       { MAIN_DEVICE_D_1X, 0x84 },
> +       { COEFF_DEVICE_D_1X, 0xC4 },
> +       { PRE_DEVICE_D_1X, 0xC4 },
> +       { PRE_SOFTWARE_RESET_DEVICE_D, 0xC4 },
> +       { POST_SOFTWARE_RESET_DEVICE_D, 0xC4 },
> +};
> +
> +static const struct blktyp_devidx_map ppc3_mapping_table[] = {
> +       { MAIN_ALL_DEVICES_1X, 0x80 },
> +       { MAIN_DEVICE_A_1X, 0x81 },
> +       { COEFF_DEVICE_A_1X, 0xC1 },
> +       { PRE_DEVICE_A_1X, 0xC1 },
> +       { MAIN_DEVICE_B_1X, 0x82 },
> +       { COEFF_DEVICE_B_1X, 0xC2 },
> +       { PRE_DEVICE_B_1X, 0xC2 },
> +       { MAIN_DEVICE_C_1X, 0x83 },
> +       { COEFF_DEVICE_C_1X, 0xC3 },
> +       { PRE_DEVICE_C_1X, 0xC3 },
> +       { MAIN_DEVICE_D_1X, 0x84 },
> +       { COEFF_DEVICE_D_1X, 0xC4 },
> +       { PRE_DEVICE_D_1X, 0xC4 },
> +};
> +
> +static const struct blktyp_devidx_map non_ppc3_mapping_table[] = {
> +       { MAIN_ALL_DEVICES, 0x80 },
> +       { MAIN_DEVICE_A, 0x81 },
> +       { COEFF_DEVICE_A, 0xC1 },
> +       { PRE_DEVICE_A, 0xC1 },
> +       { MAIN_DEVICE_B, 0x82 },
> +       { COEFF_DEVICE_B, 0xC2 },
> +       { PRE_DEVICE_B, 0xC2 },
> +       { MAIN_DEVICE_C, 0x83 },
> +       { COEFF_DEVICE_C, 0xC3 },
> +       { PRE_DEVICE_C, 0xC3 },
> +       { MAIN_DEVICE_D, 0x84 },
> +       { COEFF_DEVICE_D, 0xC4 },
> +       { PRE_DEVICE_D, 0xC4 },
> +};
> +
>  static struct tasdevice_config_info *tasdevice_add_config(
>         struct tasdevice_priv *tas_priv, unsigned char *config_data,
>         unsigned int config_size, int *status)
> @@ -316,6 +378,37 @@ int tasdevice_rca_parser(void *context, const struct firmware *fmw)
>  }
>  EXPORT_SYMBOL_NS_GPL(tasdevice_rca_parser, SND_SOC_TAS2781_FMWLIB);
>
> +/* fixed m68k compiling issue: mapping table can save code field */
> +static unsigned char map_dev_idx(struct tasdevice_fw *tas_fmw,
> +       struct tasdev_blk *block)
> +{
> +
> +       struct blktyp_devidx_map *p =
> +               (struct blktyp_devidx_map *)non_ppc3_mapping_table;

Please do not cast away constness when not needed (also below).

> +       struct tasdevice_dspfw_hdr *fw_hdr = &(tas_fmw->fw_hdr);
> +       struct tasdevice_fw_fixed_hdr *fw_fixed_hdr = &(fw_hdr->fixed_hdr);
> +
> +       int i, n = ARRAY_SIZE(non_ppc3_mapping_table);

size_t

> +       unsigned char dev_idx = 0;
> +
> +       if (fw_fixed_hdr->ppcver >= PPC3_VERSION_TAS2781) {
> +               p = (struct blktyp_devidx_map *)ppc3_tas2781_mapping_table;
> +               n = ARRAY_SIZE(ppc3_tas2781_mapping_table);
> +       } else if (fw_fixed_hdr->ppcver >= PPC3_VERSION) {
> +               p = (struct blktyp_devidx_map *)ppc3_mapping_table;
> +               n = ARRAY_SIZE(ppc3_mapping_table);
> +       }
> +
> +       for (i = 0; i < n; i++) {
> +               if (block->type == p[i].blktyp) {
> +                       dev_idx = p[i].dev_idx;
> +                       break;
> +               }
> +       }
> +
> +       return dev_idx;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds