Re: [PATCH v3 14/33] drm/mediatek: mtk_hdmi: Move audio params selection to new function

[AngeloGioacchino Del Regno]

Il 20/12/24 04:47, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
>
> On Tue, 2024-12-17 at 16:43 +0100, AngeloGioacchino Del Regno wrote:
> > External email : Please do not click links or open attachments until you have verified the sender or the content.
> >
> >
> > In preparation for splitting common bits of this driver, move the
> > audio params (codec, sample rate/size, input type, i2s format, etc)
> > selection to a new function called mtk_hdmi_audio_params().
> >
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/mediatek/mtk_hdmi.c | 46 +++++++++++++++++------------
> >   1 file changed, 27 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> > index d2f1d6286fbc..8f5ab97a0261 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> > @@ -1479,12 +1479,11 @@ static int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi,
> >    * HDMI audio codec callbacks
> >    */
> >
> > -static int mtk_hdmi_audio_hw_params(struct device *dev, void *data,
> > -                                   struct hdmi_codec_daifmt *daifmt,
> > -                                   struct hdmi_codec_params *params)
> > +static int mtk_hdmi_audio_params(struct mtk_hdmi *hdmi,
> > +                                struct hdmi_codec_daifmt *daifmt,
> > +                                struct hdmi_codec_params *params)
> >   {
> > -       struct mtk_hdmi *hdmi = dev_get_drvdata(dev);
> > -       struct hdmi_audio_param hdmi_params;
> > +       struct hdmi_audio_param aud_params = { 0 };
> >          unsigned int chan = params->cea.channels;
> >
> >          dev_dbg(hdmi->dev, "%s: %u Hz, %d bit, %d channels\n", __func__,
> > @@ -1495,16 +1494,16 @@ static int mtk_hdmi_audio_hw_params(struct device *dev, void *data,
> >
> >          switch (chan) {
> >          case 2:
> > -               hdmi_params.aud_input_chan_type = HDMI_AUD_CHAN_TYPE_2_0;
> > +               aud_params.aud_input_chan_type = HDMI_AUD_CHAN_TYPE_2_0;
> >                  break;
> >          case 4:
> > -               hdmi_params.aud_input_chan_type = HDMI_AUD_CHAN_TYPE_4_0;
> > +               aud_params.aud_input_chan_type = HDMI_AUD_CHAN_TYPE_4_0;
> >                  break;
> >          case 6:
> > -               hdmi_params.aud_input_chan_type = HDMI_AUD_CHAN_TYPE_5_1;
> > +               aud_params.aud_input_chan_type = HDMI_AUD_CHAN_TYPE_5_1;
> >                  break;
> >          case 8:
> > -               hdmi_params.aud_input_chan_type = HDMI_AUD_CHAN_TYPE_7_1;
> > +               aud_params.aud_input_chan_type = HDMI_AUD_CHAN_TYPE_7_1;
> >                  break;
> >          default:
> >                  dev_err(hdmi->dev, "channel[%d] not supported!\n", chan);
> > @@ -1528,26 +1527,35 @@ static int mtk_hdmi_audio_hw_params(struct device *dev, void *data,
> >
> >          switch (daifmt->fmt) {
> >          case HDMI_I2S:
> > -               hdmi_params.aud_codec = HDMI_AUDIO_CODING_TYPE_PCM;
> > -               hdmi_params.aud_sample_size = HDMI_AUDIO_SAMPLE_SIZE_16;
> > -               hdmi_params.aud_input_type = HDMI_AUD_INPUT_I2S;
> > -               hdmi_params.aud_i2s_fmt = HDMI_I2S_MODE_I2S_24BIT;
> > -               hdmi_params.aud_mclk = HDMI_AUD_MCLK_128FS;
> > +               aud_params.aud_codec = HDMI_AUDIO_CODING_TYPE_PCM;
> > +               aud_params.aud_sample_size = HDMI_AUDIO_SAMPLE_SIZE_16;
> > +               aud_params.aud_input_type = HDMI_AUD_INPUT_I2S;
> > +               aud_params.aud_i2s_fmt = HDMI_I2S_MODE_I2S_24BIT;
> > +               aud_params.aud_mclk = HDMI_AUD_MCLK_128FS;
> >                  break;
> >          case HDMI_SPDIF:
> > -               hdmi_params.aud_codec = HDMI_AUDIO_CODING_TYPE_PCM;
> > -               hdmi_params.aud_sample_size = HDMI_AUDIO_SAMPLE_SIZE_16;
> > -               hdmi_params.aud_input_type = HDMI_AUD_INPUT_SPDIF;
> > +               aud_params.aud_codec = HDMI_AUDIO_CODING_TYPE_PCM;
> > +               aud_params.aud_sample_size = HDMI_AUDIO_SAMPLE_SIZE_16;
> > +               aud_params.aud_input_type = HDMI_AUD_INPUT_SPDIF;
> >                  break;
> >          default:
> >                  dev_err(hdmi->dev, "%s: Invalid DAI format %d\n", __func__,
> >                          daifmt->fmt);
> >                  return -EINVAL;
> >          }
> > +       memcpy(&aud_params.codec_params, params, sizeof(aud_params.codec_params));
>
> You copy to local variable, so this function does nothing.
>
> > +
> > +       return 0;
> > +}
> >
> > -       memcpy(&hdmi_params.codec_params, params,
> > -              sizeof(hdmi_params.codec_params));
> > +static int mtk_hdmi_audio_hw_params(struct device *dev, void *data,
> > +                                   struct hdmi_codec_daifmt *daifmt,
> > +                                   struct hdmi_codec_params *params)
> > +{
> > +       struct mtk_hdmi *hdmi = dev_get_drvdata(dev);
> > +       struct hdmi_audio_param hdmi_params;
> >
> > +       mtk_hdmi_audio_params(hdmi, daifmt, params);
> >          mtk_hdmi_audio_set_param(hdmi, &hdmi_params);
>
> hdmi_params has not been initialized.

That was done on purpose: we are anyway rewriting all fields of that structure
with the call to mtk_hdmi_audio_set_param(), so if that was stack-initialized
to zero, that would be a double initialization.

Cheers,
Angelo