Re: [PATCH v3 06/13] ASoC: simple-card-utils: Expose new members for asoc_simple_priv

2020-10-02 Thread Sameer Pujar




diff --git a/include/sound/simple_card_utils.h 
b/include/sound/simple_card_utils.h
index 86a1e95..9825308 100644
--- a/include/sound/simple_card_utils.h
+++ b/include/sound/simple_card_utils.h
@@ -56,6 +56,10 @@ struct asoc_simple_priv {
   struct asoc_simple_dai *dais;
   struct snd_soc_codec_conf *codec_conf;
   struct gpio_desc *pa_gpio;
+ const struct snd_soc_ops *ops;
+ unsigned int force_dpcm:1;
+ uintptr_t dpcm_selectable;
+ void *data;
  };

I have opinions about these.




About dpcm_selectable, indeed current audio-graph is using it as "uintptr_t",
but as you know, it checks whether it was non-NULL or not only.
This means we can use it as bit-field.


Yes that is true. Something like this would work?

graph_probe(...)
{
    ...

    if (of_device_get_match_data(dev))
    priv->dpcm_selectable = 1;

    ...
}




BTW, do we need to have dpcm_selectable at priv ?


Tegra audio graph driver does not require this because already it is 
populating 'force_dpcm' flag and having 'selectable' does not make much 
sense for it.




One note is that, -scu- user is only me (locally),
and it will be removed when audio-graph2 was created.
(My plan is keep code for you, but remove compatible)


Right now I am just keeping it to allow current code work. Later you can 
remove this during graph2.




About *data, I think we can avoid *data
if driver side priv includes asoc_simple_priv ?

 struct my_priv {
 struct asoc_simple_priv *simple;
 ...
 };

 #define simple_to_priv(_simple) container_of((_simple), struct 
my_priv, simple)



This seems like a better plan, will do this.


Re: [PATCH v3 06/13] ASoC: simple-card-utils: Expose new members for asoc_simple_priv

2020-10-01 Thread Kuninori Morimoto


Hi Sameer

Thank you for the patch

> Add new members in struct 'asoc_simple_priv'. Idea is to leverage
> simple or graph card driver as much as possible and vendor can
> maintain a thin driver to control the behavior by populating these
> newly exposed members.

re-use simple/audio-graph driver is nice idea.
My planning next new audio-graph2 can renuse it.

> diff --git a/include/sound/simple_card_utils.h 
> b/include/sound/simple_card_utils.h
> index 86a1e95..9825308 100644
> --- a/include/sound/simple_card_utils.h
> +++ b/include/sound/simple_card_utils.h
> @@ -56,6 +56,10 @@ struct asoc_simple_priv {
>   struct asoc_simple_dai *dais;
>   struct snd_soc_codec_conf *codec_conf;
>   struct gpio_desc *pa_gpio;
> + const struct snd_soc_ops *ops;
> + unsigned int force_dpcm:1;
> + uintptr_t dpcm_selectable;
> + void *data;
>  };

I have opinions about these.

About dpcm_selectable, indeed current audio-graph is using it as "uintptr_t",
but as you know, it checks whether it was non-NULL or not only.
This means we can use it as bit-field.

BTW, do we need to have dpcm_selectable at priv ?

One note is that, -scu- user is only me (locally),
and it will be removed when audio-graph2 was created.
(My plan is keep code for you, but remove compatible)

About *data, I think we can avoid *data
if driver side priv includes asoc_simple_priv ?

struct my_priv {
struct asoc_simple_priv *simple;
...
};

#define simple_to_priv(_simple) container_of((_simple), struct my_priv, 
simple)


Thank you for your help !!

Best regards
---
Kuninori Morimoto


[PATCH v3 06/13] ASoC: simple-card-utils: Expose new members for asoc_simple_priv

2020-10-01 Thread Sameer Pujar
Add new members in struct 'asoc_simple_priv'. Idea is to leverage
simple or graph card driver as much as possible and vendor can
maintain a thin driver to control the behavior by populating these
newly exposed members.

Following are the members added in 'asoc_simple_priv':

  - 'ops' struct: In some cases SoC vendor drivers may want to
implement 'snd_soc_ops' callbacks differently. In such cases
custom callbacks would be used.

  - 'force_dpcm' flag: Right now simple or graph card drivers
detect DAI links as DPCM links if:

  * The dpcm_selectable is set AND
  * Codec is connected to multiple CPU endpoints or aconvert
property is used for rate/channels.

So there is no way to directly specify usage of DPCM alone. So a
flag is exposed to mark all links as DPCM. Vendor driver can
set this if required.

  - 'dpcm_selectable': Currently simple or audio graph drivers
provide a way to enable this for specific compatibles. However
vendor driver may want to define some additional info. Thus
expose this variable where vendor drivers can set this if
required.

  - 'data': A void pointer member is provided. This would be useful
when vendor driver wants to define its own structure for internal
usage and still wants to rely on most of the other members from
'asoc_simple_priv'.

Subsequent patches in the series illustrate usage for above.

Signed-off-by: Sameer Pujar 
Cc: Kuninori Morimoto 
---
 include/sound/simple_card_utils.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/include/sound/simple_card_utils.h 
b/include/sound/simple_card_utils.h
index 86a1e95..9825308 100644
--- a/include/sound/simple_card_utils.h
+++ b/include/sound/simple_card_utils.h
@@ -56,6 +56,10 @@ struct asoc_simple_priv {
struct asoc_simple_dai *dais;
struct snd_soc_codec_conf *codec_conf;
struct gpio_desc *pa_gpio;
+   const struct snd_soc_ops *ops;
+   unsigned int force_dpcm:1;
+   uintptr_t dpcm_selectable;
+   void *data;
 };
 #define simple_priv_to_card(priv)  (&(priv)->snd_card)
 #define simple_priv_to_props(priv, i)  ((priv)->dai_props + (i))
-- 
2.7.4