Re: [RESEND v13 04/10] ASoC: qcom: Add helper function to get dma control and lpaif handle

[Stephen Boyd]

Quoting Srinivasa Rao Mandadapu (2022-02-15 21:11:29)
>
> On 2/15/2022 6:40 AM, Stephen Boyd wrote:
> Thanks for your time Stephen!!!
> > Quoting Srinivasa Rao Mandadapu (2022-02-14 06:58:22)
> >> Add support function to get dma control and lpaif handle to avoid
> >> repeated code in platform driver
> >>
> >> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@xxxxxxxxxxx>
> >> Co-developed-by: Venkata Prasad Potturu <quic_potturu@xxxxxxxxxxx>
> >> Signed-off-by: Venkata Prasad Potturu <quic_potturu@xxxxxxxxxxx>
> >> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> >> ---
> >>   sound/soc/qcom/lpass-platform.c | 113 +++++++++++++++++++++++-----------------
> >>   1 file changed, 66 insertions(+), 47 deletions(-)
> >>
> >> diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c
> >> index a44162c..5d77240 100644
> >> --- a/sound/soc/qcom/lpass-platform.c
> >> +++ b/sound/soc/qcom/lpass-platform.c
> >> @@ -177,6 +177,49 @@ static int lpass_platform_pcmops_close(struct snd_soc_component *component,
> >>          return 0;
> >>   }
> >>
> >> +static void __lpass_get_lpaif_handle(struct snd_pcm_substream *substream,
> > const?
> Okay. will add const to substream pointer.
> >
> >> +                                    struct snd_soc_component *component,
> > const?
> Here const is giving compilation errors in below code.

Ok

> >
> >> +                       l_id = pcm_data->dma_ch;
> >> +                       l_dmactl = drvdata->rd_dmactl;
> >> +               } else {
> >> +                       l_dmactl = drvdata->wr_dmactl;
> >> +                       l_id = pcm_data->dma_ch - v->wrdma_channel_start;
> >> +               }
> >> +               l_map = drvdata->lpaif_map;
> >> +               break;
> >> +       case LPASS_DP_RX:
> >> +               l_id = pcm_data->dma_ch;
> >> +               l_dmactl = drvdata->hdmi_rd_dmactl;
> >> +               l_map = drvdata->hdmiif_map;
> >> +               break;
> >> +       default:
> >> +               break;
> >> +       }
> >> +       if (dmactl)
> >> +               *dmactl = l_dmactl;
> >> +       if (id)
> >> +               *id = l_id;
> >> +       if (map)
> >> +               *map = l_map;
> > Why not 'return 0' here and return -EINVAL in the default case above? Then
> > we can skip the checks for !map or !dmactl below and simply bail out if
> > it failed with an error value.
>
> Here the check is for input params. some users call for only damctl or
> only map.

Yes the check is to make sure there's a pointer to store the value into.
I get that. The users are all internal to this driver though because
the function is static. If the function returned an error because it
couldn't find something then we wouldn't have to test the resulting
pointers for success, instead we could check for a return value.
Similarly, it may be clearer to have a single get for each item and then
return a pointer from the function vs. passing it in to extract
something. It may duplicate some code but otherwise the code would be
clearer because we're getting one thing and can pass an error value
through the pointer with PTR_ERR().