Re: [PATCH v3 10/13] ASoC: tegra: Add audio graph based card driver
Hi Sameer
> > This is just an idea,
> > but can we use hooks here somehow ?
> >
> > .ops_hook_pre
> > .ops_hook_func
> > .ops_hook_post
> >
> > if (priv->ops_hook_pre->func)
> > priv->ops_hook_pre->func_pre(...);
> >
> > if (priv->ops_hook_func->func)
> > priv->ops_hook_func->func(...); /* driver's function */
> > else
> > graph_func(...);/* audio-graph function */
> >
> > if (priv->ops_hook_post->func)
> > priv->ops_hook_post->func(...);
>
> Right now I just required to populate some flags or structures and do
> not have any specific pre()/post() functions to be called. Can this be
> reserved for later?
Yeah, of course :)
> > These are almost same as graph_probe().
> > Maybe we can separate graph_probe() and export function ?
>
> Yes possible, I can move more stuff into graph_parse_of() which is
> already an exported function in the current series. This can be
> utilized by both generic audio graph and Tegra audio graph.
>
> Something like below,
>
> static int tegra_audio_graph_probe(struct platform_device *pdev)
> {
> struct tegra_audio_priv *priv;
> struct device *dev = >dev;
> struct snd_soc_card *card;
>
> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
>
> card = simple_priv_to_card(>simple);
>
> card->owner = THIS_MODULE;
> card->dev = dev;
> card->probe = tegra_audio_graph_card_probe;
>
> /* graph_parse_of() depends on below */
> card->component_chaining = 1;
> priv->simple.ops = _audio_graph_ops;
> priv->simple.force_dpcm = 1;
>
> return graph_parse_of(>simple);
> }
I think graph side can handle card->owner / card->dev,
but, it looks good to me
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v3 10/13] ASoC: tegra: Add audio graph based card driver
Add Tegra audio machine driver which is based on generic audio graph card
driver. It re-uses most of the common stuff from audio graph driver and
uses the same DT binding. Required Tegra specific customizations are done
in the driver.
(snip)
+static const struct snd_soc_ops tegra_audio_graph_ops = {
+ .startup= asoc_simple_startup,
+ .shutdown = asoc_simple_shutdown,
+ .hw_params = tegra_audio_graph_hw_params,
+};
This is just an idea,
but can we use hooks here somehow ?
.ops_hook_pre
.ops_hook_func
.ops_hook_post
if (priv->ops_hook_pre->func)
priv->ops_hook_pre->func_pre(...);
if (priv->ops_hook_func->func)
priv->ops_hook_func->func(...); /* driver's function */
else
graph_func(...);/* audio-graph function */
if (priv->ops_hook_post->func)
priv->ops_hook_post->func(...);
Right now I just required to populate some flags or structures and do
not have any specific pre()/post() functions to be called. Can this be
reserved for later?
+static int tegra_audio_graph_probe(struct platform_device *pdev)
+{
+ struct asoc_simple_priv *priv;
+ struct device *dev = >dev;
+ struct snd_soc_card *card;
+ struct link_info li;
+ int err;
+
+ /* Allocate the private data and the DAI link array */
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->data = (struct tegra_audio_graph_data *)
+ devm_kzalloc(dev, sizeof(*priv->data), GFP_KERNEL);
+ if (!priv->data)
+ return -ENOMEM;
+
+ card = simple_priv_to_card(priv);
+
+ card->owner = THIS_MODULE;
+ card->dev = dev;
+ card->component_chaining = true;
+ card->probe = tegra_audio_graph_card_probe;
+
+ priv->ops = _audio_graph_ops;
+ priv->force_dpcm = 1;
+
+ memset(, 0, sizeof(li));
+ graph_get_dais_count(priv, );
+ if (!li.link || !li.dais)
+ return -EINVAL;
+
+ err = asoc_simple_init_priv(priv, );
+ if (err < 0)
+ return err;
+
+ err = graph_parse_of(priv);
+ if (err < 0) {
+ if (err != -EPROBE_DEFER)
+ dev_err(dev, "Parse error %d\n", err);
+ goto cleanup;
+ }
+
+ snd_soc_card_set_drvdata(card, priv);
+
+ asoc_simple_debug_info(priv);
+
+ err = devm_snd_soc_register_card(dev, card);
+ if (err < 0)
+ goto cleanup;
+
+ return 0;
+
+cleanup:
+ asoc_simple_clean_reference(card);
+
+ return err;
+}
These are almost same as graph_probe().
Maybe we can separate graph_probe() and export function ?
Yes possible, I can move more stuff into graph_parse_of() which is
already an exported function in the current series. This can be utilized
by both generic audio graph and Tegra audio graph.
Something like below,
static int tegra_audio_graph_probe(struct platform_device *pdev)
{
struct tegra_audio_priv *priv;
struct device *dev = >dev;
struct snd_soc_card *card;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
card = simple_priv_to_card(>simple);
card->owner = THIS_MODULE;
card->dev = dev;
card->probe = tegra_audio_graph_card_probe;
/* graph_parse_of() depends on below */
card->component_chaining = 1;
priv->simple.ops = _audio_graph_ops;
priv->simple.force_dpcm = 1;
return graph_parse_of(>simple);
}
Does this sound fine?
Re: [PATCH v3 10/13] ASoC: tegra: Add audio graph based card driver
+/* Setup PLL clock as per the given sample rate */
+static int tegra_audio_graph_update_pll(struct snd_pcm_substream *substream,
+struct snd_pcm_hw_params *params)
+{
+struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+struct asoc_simple_priv *priv = snd_soc_card_get_drvdata(rtd->card);
+struct device *dev = rtd->card->dev;
+struct tegra_audio_graph_data *graph_data =
+(struct tegra_audio_graph_data *)priv->data;
+struct tegra_audio_chip_data *chip_data =
+(struct tegra_audio_chip_data *)of_device_get_match_data(dev);
void* doesn't need casting
There are several similar places in the code. Not a big deal, but this
makes code less readable than it could be.
I will drop these in next revision.
Re: [PATCH v3 10/13] ASoC: tegra: Add audio graph based card driver
Hi Sameer
> Add Tegra audio machine driver which is based on generic audio graph card
> driver. It re-uses most of the common stuff from audio graph driver and
> uses the same DT binding. Required Tegra specific customizations are done
> in the driver.
(snip)
> +static const struct snd_soc_ops tegra_audio_graph_ops = {
> + .startup= asoc_simple_startup,
> + .shutdown = asoc_simple_shutdown,
> + .hw_params = tegra_audio_graph_hw_params,
> +};
This is just an idea,
but can we use hooks here somehow ?
.ops_hook_pre
.ops_hook_func
.ops_hook_post
if (priv->ops_hook_pre->func)
priv->ops_hook_pre->func_pre(...);
if (priv->ops_hook_func->func)
priv->ops_hook_func->func(...); /* driver's function */
else
graph_func(...);/* audio-graph function */
if (priv->ops_hook_post->func)
priv->ops_hook_post->func(...);
> +static int tegra_audio_graph_probe(struct platform_device *pdev)
> +{
> + struct asoc_simple_priv *priv;
> + struct device *dev = >dev;
> + struct snd_soc_card *card;
> + struct link_info li;
> + int err;
> +
> + /* Allocate the private data and the DAI link array */
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->data = (struct tegra_audio_graph_data *)
> + devm_kzalloc(dev, sizeof(*priv->data), GFP_KERNEL);
> + if (!priv->data)
> + return -ENOMEM;
> +
> + card = simple_priv_to_card(priv);
> +
> + card->owner = THIS_MODULE;
> + card->dev = dev;
> + card->component_chaining = true;
> + card->probe = tegra_audio_graph_card_probe;
> +
> + priv->ops = _audio_graph_ops;
> + priv->force_dpcm = 1;
> +
> + memset(, 0, sizeof(li));
> + graph_get_dais_count(priv, );
> + if (!li.link || !li.dais)
> + return -EINVAL;
> +
> + err = asoc_simple_init_priv(priv, );
> + if (err < 0)
> + return err;
> +
> + err = graph_parse_of(priv);
> + if (err < 0) {
> + if (err != -EPROBE_DEFER)
> + dev_err(dev, "Parse error %d\n", err);
> + goto cleanup;
> + }
> +
> + snd_soc_card_set_drvdata(card, priv);
> +
> + asoc_simple_debug_info(priv);
> +
> + err = devm_snd_soc_register_card(dev, card);
> + if (err < 0)
> + goto cleanup;
> +
> + return 0;
> +
> +cleanup:
> + asoc_simple_clean_reference(card);
> +
> + return err;
> +}
These are almost same as graph_probe().
Maybe we can separate graph_probe() and export function ?
struct tegra_audio_graph_data
{
struct asoc_simple_priv simple;
...
};
#define simple_to_priv(_simple) container_of((_simple), struct my_priv,
simple)
static int tegra_audio_graph_probe(struct platform_device *pdev)
{
struct tegra_audio_graph_data *data;
struct asoc_simple_priv *priv;
/* Allocate the private data */
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
/* initial audio-graph */
ret = audio_graph_init(priv, pdev);
if (ret < 0)
return -xxx;
/* over-write for own settings */
card = simple_priv_to_card(priv);
card->component_chaining = true;
card->probe = tegra_audio_graph_card_probe;
priv = >simple;
priv->ops_hook_pre = _audio_graph_ops;
priv->force_dpcm = 1;
/* audio-graph remain */
return audio_graph_prove(priv, pdev);
}
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v3 10/13] ASoC: tegra: Add audio graph based card driver
01.10.2020 23:57, Dmitry Osipenko пишет:
> 01.10.2020 20:33, Sameer Pujar пишет:
>> +/* Setup PLL clock as per the given sample rate */
>> +static int tegra_audio_graph_update_pll(struct snd_pcm_substream *substream,
>> +struct snd_pcm_hw_params *params)
>> +{
>> +struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
>> +struct asoc_simple_priv *priv = snd_soc_card_get_drvdata(rtd->card);
>> +struct device *dev = rtd->card->dev;
>> +struct tegra_audio_graph_data *graph_data =
>> +(struct tegra_audio_graph_data *)priv->data;
>> +struct tegra_audio_chip_data *chip_data =
>> +(struct tegra_audio_chip_data *)of_device_get_match_data(dev);
>
> void* doesn't need casting
>
There are several similar places in the code. Not a big deal, but this
makes code less readable than it could be.
Re: [PATCH v3 10/13] ASoC: tegra: Add audio graph based card driver
01.10.2020 20:33, Sameer Pujar пишет:
> +/* Setup PLL clock as per the given sample rate */
> +static int tegra_audio_graph_update_pll(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params)
> +{
> + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> + struct asoc_simple_priv *priv = snd_soc_card_get_drvdata(rtd->card);
> + struct device *dev = rtd->card->dev;
> + struct tegra_audio_graph_data *graph_data =
> + (struct tegra_audio_graph_data *)priv->data;
> + struct tegra_audio_chip_data *chip_data =
> + (struct tegra_audio_chip_data *)of_device_get_match_data(dev);
void* doesn't need casting
Re: [PATCH v3 10/13] ASoC: tegra: Add audio graph based card driver
On 01/10/2020 20:07, Michał Mirosław wrote:
> On Thu, Oct 01, 2020 at 11:03:04PM +0530, Sameer Pujar wrote:
>> Add Tegra audio machine driver which is based on generic audio graph card
>> driver. It re-uses most of the common stuff from audio graph driver and
>> uses the same DT binding. Required Tegra specific customizations are done
>> in the driver.
> [...]
>> +switch (srate) {
>> +case 11025:
>> +case 22050:
>> +case 44100:
>> +case 88200:
>> +case 176400:
>> +plla_out0_rate = chip_data->plla_out0_rates[x11_RATE];
>> +plla_rate = chip_data->plla_rates[x11_RATE];
>> +break;
>> +case 8000:
>> +case 16000:
>> +case 32000:
>> +case 48000:
>> +case 96000:
>> +case 192000:
> [...]
>
> Do you really need to enumerate the frequencies? Wouldn't just checking
> srate % 11025 be enough to divide the set in two? Or just calculating
> the PLLA base rate by multiplying?
This is quite common among other ASoC drivers from what I can see. The
PLL rate does not scale with the srate, we just use a different PLL rate
depending on if the srate is 11025 Hz or 8000 Hz based. I don't see any
need to change the above.
Cheers
Jon
--
nvpublic
Re: [PATCH v3 10/13] ASoC: tegra: Add audio graph based card driver
On Thu, Oct 01, 2020 at 11:03:04PM +0530, Sameer Pujar wrote:
> Add Tegra audio machine driver which is based on generic audio graph card
> driver. It re-uses most of the common stuff from audio graph driver and
> uses the same DT binding. Required Tegra specific customizations are done
> in the driver.
[...]
> + switch (srate) {
> + case 11025:
> + case 22050:
> + case 44100:
> + case 88200:
> + case 176400:
> + plla_out0_rate = chip_data->plla_out0_rates[x11_RATE];
> + plla_rate = chip_data->plla_rates[x11_RATE];
> + break;
> + case 8000:
> + case 16000:
> + case 32000:
> + case 48000:
> + case 96000:
> + case 192000:
[...]
Do you really need to enumerate the frequencies? Wouldn't just checking
srate % 11025 be enough to divide the set in two? Or just calculating
the PLLA base rate by multiplying?
Best Regards,
Michał Mirosław
