Add support function to get dma control and lpaif handle to avoid
repeated code in platform driver
Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
Co-developed-by: Venkata Prasad Potturu <[email protected]>
Signed-off-by: Venkata Prasad Potturu <[email protected]>
Reviewed-by: Srinivas Kandagatla <[email protected]>
---
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,
+ struct snd_soc_component *component,
+ struct lpaif_dmactl **dmactl, int *id, struct regmap **map)
+{
+ struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
+ struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
+ struct lpass_data *drvdata = snd_soc_component_get_drvdata(component);
+ struct snd_pcm_runtime *rt = substream->runtime;
+ struct lpass_pcm_data *pcm_data = rt->private_data;
+ struct lpass_variant *v = drvdata->variant;
+ int dir = substream->stream;
+ unsigned int dai_id = cpu_dai->driver->id;
+ struct lpaif_dmactl *l_dmactl = NULL;
+ struct regmap *l_map = NULL;
+ int l_id = 0;
+
+ switch (dai_id) {
+ case MI2S_PRIMARY ... MI2S_QUINARY:
+ if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
+ 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;
+}
+
static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component,
struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
@@ -191,21 +234,15 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component,
unsigned int channels = params_channels(params);
unsigned int regval;
struct lpaif_dmactl *dmactl;
- int id, dir = substream->stream;
+ int id;
int bitwidth;
int ret, dma_port = pcm_data->i2s_port + v->dmactl_audif_start;
unsigned int dai_id = cpu_dai->driver->id;
- if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
- id = pcm_data->dma_ch;
- if (dai_id == LPASS_DP_RX)
- dmactl = drvdata->hdmi_rd_dmactl;
- else
- dmactl = drvdata->rd_dmactl;
-
- } else {
- dmactl = drvdata->wr_dmactl;
- id = pcm_data->dma_ch - v->wrdma_channel_start;
+ __lpass_get_lpaif_handle(substream, component, &dmactl, &id, NULL);
+ if (!dmactl) {
+ dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
+ return -EINVAL;
}
bitwidth = snd_pcm_format_width(format);
@@ -350,10 +387,11 @@ static int lpass_platform_pcmops_hw_free(struct snd_soc_component *component,
struct regmap *map;
unsigned int dai_id = cpu_dai->driver->id;
- if (dai_id == LPASS_DP_RX)
- map = drvdata->hdmiif_map;
- else
- map = drvdata->lpaif_map;
+ __lpass_get_lpaif_handle(substream, component, NULL, NULL, &map);
+ if (!map) {
+ dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
+ return -EINVAL;
+ }
reg = LPAIF_DMACTL_REG(v, pcm_data->dma_ch, substream->stream, dai_id);
ret = regmap_write(map, reg, 0);
@@ -379,22 +417,12 @@ static int lpass_platform_pcmops_prepare(struct snd_soc_component *component,
int ret, id, ch, dir = substream->stream;
unsigned int dai_id = cpu_dai->driver->id;
-
ch = pcm_data->dma_ch;
- if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
- if (dai_id == LPASS_DP_RX) {
- dmactl = drvdata->hdmi_rd_dmactl;
- map = drvdata->hdmiif_map;
- } else {
- dmactl = drvdata->rd_dmactl;
- map = drvdata->lpaif_map;
- }
- id = pcm_data->dma_ch;
- } else {
- dmactl = drvdata->wr_dmactl;
- id = pcm_data->dma_ch - v->wrdma_channel_start;
- map = drvdata->lpaif_map;
+ __lpass_get_lpaif_handle(substream, component, &dmactl, &id, &map);
+ if (!dmactl) {
+ dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
+ return -EINVAL;
}
ret = regmap_write(map, LPAIF_DMABASE_REG(v, ch, dir, dai_id),
@@ -444,25 +472,15 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component,
struct lpaif_dmactl *dmactl;
struct regmap *map;
int ret, ch, id;
- int dir = substream->stream;
unsigned int reg_irqclr = 0, val_irqclr = 0;
unsigned int reg_irqen = 0, val_irqen = 0, val_mask = 0;
unsigned int dai_id = cpu_dai->driver->id;
ch = pcm_data->dma_ch;
- if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
- id = pcm_data->dma_ch;
- if (dai_id == LPASS_DP_RX) {
- dmactl = drvdata->hdmi_rd_dmactl;
- map = drvdata->hdmiif_map;
- } else {
- dmactl = drvdata->rd_dmactl;
- map = drvdata->lpaif_map;
- }
- } else {
- dmactl = drvdata->wr_dmactl;
- id = pcm_data->dma_ch - v->wrdma_channel_start;
- map = drvdata->lpaif_map;
+ __lpass_get_lpaif_handle(substream, component, &dmactl, &id, &map);
+ if (!dmactl) {
+ dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
+ return -EINVAL;
}
switch (cmd) {
@@ -597,10 +615,11 @@ static snd_pcm_uframes_t lpass_platform_pcmops_pointer(
struct regmap *map;
unsigned int dai_id = cpu_dai->driver->id;
- if (dai_id == LPASS_DP_RX)
- map = drvdata->hdmiif_map;
- else
- map = drvdata->lpaif_map;
+ __lpass_get_lpaif_handle(substream, component, NULL, NULL, &map);
+ if (!map) {
+ dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
+ return -EINVAL;
+ }
ch = pcm_data->dma_ch;
--
2.7.4
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 <[email protected]>
> Co-developed-by: Venkata Prasad Potturu <[email protected]>
> Signed-off-by: Venkata Prasad Potturu <[email protected]>
> Reviewed-by: Srinivas Kandagatla <[email protected]>
> ---
> 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?
> + struct snd_soc_component *component,
const?
> + struct lpaif_dmactl **dmactl, int *id, struct regmap **map)
> +{
> + struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
> + struct lpass_data *drvdata = snd_soc_component_get_drvdata(component);
> + struct snd_pcm_runtime *rt = substream->runtime;
> + struct lpass_pcm_data *pcm_data = rt->private_data;
> + struct lpass_variant *v = drvdata->variant;
> + int dir = substream->stream;
> + unsigned int dai_id = cpu_dai->driver->id;
> + struct lpaif_dmactl *l_dmactl = NULL;
> + struct regmap *l_map = NULL;
> + int l_id = 0;
> +
> + switch (dai_id) {
> + case MI2S_PRIMARY ... MI2S_QUINARY:
> + if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
Please write if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) and
drop 'dir' local variable.
> + 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.
> +}
> +
> static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component,
> struct snd_pcm_substream *substream,
> struct snd_pcm_hw_params *params)
> @@ -191,21 +234,15 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component,
> unsigned int channels = params_channels(params);
> unsigned int regval;
> struct lpaif_dmactl *dmactl;
> - int id, dir = substream->stream;
> + int id;
> int bitwidth;
> int ret, dma_port = pcm_data->i2s_port + v->dmactl_audif_start;
> unsigned int dai_id = cpu_dai->driver->id;
>
> - if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
> - id = pcm_data->dma_ch;
> - if (dai_id == LPASS_DP_RX)
> - dmactl = drvdata->hdmi_rd_dmactl;
> - else
> - dmactl = drvdata->rd_dmactl;
> -
> - } else {
> - dmactl = drvdata->wr_dmactl;
> - id = pcm_data->dma_ch - v->wrdma_channel_start;
> + __lpass_get_lpaif_handle(substream, component, &dmactl, &id, NULL);
> + if (!dmactl) {
> + dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
> + return -EINVAL;
> }
>
> bitwidth = snd_pcm_format_width(format);
> @@ -350,10 +387,11 @@ static int lpass_platform_pcmops_hw_free(struct snd_soc_component *component,
> struct regmap *map;
> unsigned int dai_id = cpu_dai->driver->id;
>
> - if (dai_id == LPASS_DP_RX)
> - map = drvdata->hdmiif_map;
> - else
> - map = drvdata->lpaif_map;
> + __lpass_get_lpaif_handle(substream, component, NULL, NULL, &map);
> + if (!map) {
> + dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
> + return -EINVAL;
> + }
>
> reg = LPAIF_DMACTL_REG(v, pcm_data->dma_ch, substream->stream, dai_id);
> ret = regmap_write(map, reg, 0);
> @@ -379,22 +417,12 @@ static int lpass_platform_pcmops_prepare(struct snd_soc_component *component,
> int ret, id, ch, dir = substream->stream;
> unsigned int dai_id = cpu_dai->driver->id;
>
> -
> ch = pcm_data->dma_ch;
> - if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
> - if (dai_id == LPASS_DP_RX) {
> - dmactl = drvdata->hdmi_rd_dmactl;
> - map = drvdata->hdmiif_map;
> - } else {
> - dmactl = drvdata->rd_dmactl;
> - map = drvdata->lpaif_map;
> - }
>
> - id = pcm_data->dma_ch;
> - } else {
> - dmactl = drvdata->wr_dmactl;
> - id = pcm_data->dma_ch - v->wrdma_channel_start;
> - map = drvdata->lpaif_map;
> + __lpass_get_lpaif_handle(substream, component, &dmactl, &id, &map);
> + if (!dmactl) {
> + dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
> + return -EINVAL;
> }
>
> ret = regmap_write(map, LPAIF_DMABASE_REG(v, ch, dir, dai_id),
> @@ -444,25 +472,15 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component,
> struct lpaif_dmactl *dmactl;
> struct regmap *map;
> int ret, ch, id;
> - int dir = substream->stream;
> unsigned int reg_irqclr = 0, val_irqclr = 0;
> unsigned int reg_irqen = 0, val_irqen = 0, val_mask = 0;
> unsigned int dai_id = cpu_dai->driver->id;
>
> ch = pcm_data->dma_ch;
> - if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
> - id = pcm_data->dma_ch;
> - if (dai_id == LPASS_DP_RX) {
> - dmactl = drvdata->hdmi_rd_dmactl;
> - map = drvdata->hdmiif_map;
> - } else {
> - dmactl = drvdata->rd_dmactl;
> - map = drvdata->lpaif_map;
> - }
> - } else {
> - dmactl = drvdata->wr_dmactl;
> - id = pcm_data->dma_ch - v->wrdma_channel_start;
> - map = drvdata->lpaif_map;
> + __lpass_get_lpaif_handle(substream, component, &dmactl, &id, &map);
> + if (!dmactl) {
> + dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
> + return -EINVAL;
> }
>
> switch (cmd) {
> @@ -597,10 +615,11 @@ static snd_pcm_uframes_t lpass_platform_pcmops_pointer(
> struct regmap *map;
> unsigned int dai_id = cpu_dai->driver->id;
>
> - if (dai_id == LPASS_DP_RX)
> - map = drvdata->hdmiif_map;
> - else
> - map = drvdata->lpaif_map;
> + __lpass_get_lpaif_handle(substream, component, NULL, NULL, &map);
> + if (!map) {
> + dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
> + return -EINVAL;
> + }
>
> ch = pcm_data->dma_ch;
>
> --
> 2.7.4
>
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 <[email protected]>
>> Co-developed-by: Venkata Prasad Potturu <[email protected]>
>> Signed-off-by: Venkata Prasad Potturu <[email protected]>
>> Reviewed-by: Srinivas Kandagatla <[email protected]>
>> ---
>> 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.
>
>> + struct lpaif_dmactl **dmactl, int *id, struct regmap **map)
>> +{
>> + struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
>> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
>> + struct lpass_data *drvdata = snd_soc_component_get_drvdata(component);
>> + struct snd_pcm_runtime *rt = substream->runtime;
>> + struct lpass_pcm_data *pcm_data = rt->private_data;
>> + struct lpass_variant *v = drvdata->variant;
>> + int dir = substream->stream;
>> + unsigned int dai_id = cpu_dai->driver->id;
>> + struct lpaif_dmactl *l_dmactl = NULL;
>> + struct regmap *l_map = NULL;
>> + int l_id = 0;
>> +
>> + switch (dai_id) {
>> + case MI2S_PRIMARY ... MI2S_QUINARY:
>> + if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
> Please write if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) and
> drop 'dir' local variable.
Okay. will change it.
>
>> + 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.
so check seems mandatory for updating only valid fields. Will remove
default case which may never occurs.
>
>> +}
>> +
>> static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component,
>> struct snd_pcm_substream *substream,
>> struct snd_pcm_hw_params *params)
>> @@ -191,21 +234,15 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component,
>> unsigned int channels = params_channels(params);
>> unsigned int regval;
>> struct lpaif_dmactl *dmactl;
>> - int id, dir = substream->stream;
>> + int id;
>> int bitwidth;
>> int ret, dma_port = pcm_data->i2s_port + v->dmactl_audif_start;
>> unsigned int dai_id = cpu_dai->driver->id;
>>
>> - if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
>> - id = pcm_data->dma_ch;
>> - if (dai_id == LPASS_DP_RX)
>> - dmactl = drvdata->hdmi_rd_dmactl;
>> - else
>> - dmactl = drvdata->rd_dmactl;
>> -
>> - } else {
>> - dmactl = drvdata->wr_dmactl;
>> - id = pcm_data->dma_ch - v->wrdma_channel_start;
>> + __lpass_get_lpaif_handle(substream, component, &dmactl, &id, NULL);
>> + if (!dmactl) {
>> + dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
>> + return -EINVAL;
>> }
>>
>> bitwidth = snd_pcm_format_width(format);
>> @@ -350,10 +387,11 @@ static int lpass_platform_pcmops_hw_free(struct snd_soc_component *component,
>> struct regmap *map;
>> unsigned int dai_id = cpu_dai->driver->id;
>>
>> - if (dai_id == LPASS_DP_RX)
>> - map = drvdata->hdmiif_map;
>> - else
>> - map = drvdata->lpaif_map;
>> + __lpass_get_lpaif_handle(substream, component, NULL, NULL, &map);
>> + if (!map) {
>> + dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
>> + return -EINVAL;
>> + }
>>
>> reg = LPAIF_DMACTL_REG(v, pcm_data->dma_ch, substream->stream, dai_id);
>> ret = regmap_write(map, reg, 0);
>> @@ -379,22 +417,12 @@ static int lpass_platform_pcmops_prepare(struct snd_soc_component *component,
>> int ret, id, ch, dir = substream->stream;
>> unsigned int dai_id = cpu_dai->driver->id;
>>
>> -
>> ch = pcm_data->dma_ch;
>> - if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
>> - if (dai_id == LPASS_DP_RX) {
>> - dmactl = drvdata->hdmi_rd_dmactl;
>> - map = drvdata->hdmiif_map;
>> - } else {
>> - dmactl = drvdata->rd_dmactl;
>> - map = drvdata->lpaif_map;
>> - }
>>
>> - id = pcm_data->dma_ch;
>> - } else {
>> - dmactl = drvdata->wr_dmactl;
>> - id = pcm_data->dma_ch - v->wrdma_channel_start;
>> - map = drvdata->lpaif_map;
>> + __lpass_get_lpaif_handle(substream, component, &dmactl, &id, &map);
>> + if (!dmactl) {
>> + dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
>> + return -EINVAL;
>> }
>>
>> ret = regmap_write(map, LPAIF_DMABASE_REG(v, ch, dir, dai_id),
>> @@ -444,25 +472,15 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component,
>> struct lpaif_dmactl *dmactl;
>> struct regmap *map;
>> int ret, ch, id;
>> - int dir = substream->stream;
>> unsigned int reg_irqclr = 0, val_irqclr = 0;
>> unsigned int reg_irqen = 0, val_irqen = 0, val_mask = 0;
>> unsigned int dai_id = cpu_dai->driver->id;
>>
>> ch = pcm_data->dma_ch;
>> - if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
>> - id = pcm_data->dma_ch;
>> - if (dai_id == LPASS_DP_RX) {
>> - dmactl = drvdata->hdmi_rd_dmactl;
>> - map = drvdata->hdmiif_map;
>> - } else {
>> - dmactl = drvdata->rd_dmactl;
>> - map = drvdata->lpaif_map;
>> - }
>> - } else {
>> - dmactl = drvdata->wr_dmactl;
>> - id = pcm_data->dma_ch - v->wrdma_channel_start;
>> - map = drvdata->lpaif_map;
>> + __lpass_get_lpaif_handle(substream, component, &dmactl, &id, &map);
>> + if (!dmactl) {
>> + dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
>> + return -EINVAL;
>> }
>>
>> switch (cmd) {
>> @@ -597,10 +615,11 @@ static snd_pcm_uframes_t lpass_platform_pcmops_pointer(
>> struct regmap *map;
>> unsigned int dai_id = cpu_dai->driver->id;
>>
>> - if (dai_id == LPASS_DP_RX)
>> - map = drvdata->hdmiif_map;
>> - else
>> - map = drvdata->lpaif_map;
>> + __lpass_get_lpaif_handle(substream, component, NULL, NULL, &map);
>> + if (!map) {
>> + dev_err(soc_runtime->dev, "failed to get dmactl handle\n");
>> + return -EINVAL;
>> + }
>>
>> ch = pcm_data->dma_ch;
>>
>> --
>> 2.7.4
>>
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 <[email protected]>
> >> Co-developed-by: Venkata Prasad Potturu <[email protected]>
> >> Signed-off-by: Venkata Prasad Potturu <[email protected]>
> >> Reviewed-by: Srinivas Kandagatla <[email protected]>
> >> ---
> >> 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().
On 2/18/2022 1:11 AM, Stephen Boyd wrote:
Thanks for Your time Stephen!!!
> 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 <[email protected]>
>>>> Co-developed-by: Venkata Prasad Potturu <[email protected]>
>>>> Signed-off-by: Venkata Prasad Potturu <[email protected]>
>>>> Reviewed-by: Srinivas Kandagatla <[email protected]>
>>>> ---
>>>> 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().
Okay. Agreed. But in initial review comments, asked to make common
function for code readability,
and to avoid duplicate code.
Anyway will change accordingly and re post it.