Re: [Xen-devel] [PATCH v2 2/5] ALSA: xen-front: Read sound driver configuration from Xen store

2018-04-17 Thread Juergen Gross 
On 17/04/18 10:42, Oleksandr Andrushchenko wrote:
> On 04/16/2018 03:55 PM, Juergen Gross wrote:
>> On 16/04/18 08:24, Oleksandr Andrushchenko wrote:
>>> +    goto fail;
>>> +    }
>>> +
>>> +    if (!strncasecmp(str, XENSND_STREAM_TYPE_PLAYBACK,
>>> + sizeof(XENSND_STREAM_TYPE_PLAYBACK))) {
>>> +    stream = _instance->streams_pb[(*cur_pb)++];
>>> +    } else if (!strncasecmp(str, XENSND_STREAM_TYPE_CAPTURE,
>>> +  sizeof(XENSND_STREAM_TYPE_CAPTURE))) {
>>> +    stream = _instance->streams_cap[(*cur_cap)++];
>>> +    } else {
>>> +    ret = -EINVAL;
>>> +    goto fail;
>>> +    }
>> Until here this function looks very much like cfg_get_stream_type().
>> Can't they use a common sub-function?
> Not really, because cfg_get_stream_type uses kasprintf
> for strings and this one devm_kasprintf. Trying to make
> a common sub-func doesn't make sense to me

Aah, okay. Didn't spot that.

>>> +    /* start from default PCM HW configuration for the card */
>>> +    cfg_read_pcm_hw(xb_dev->nodename, NULL, >pcm_hw);
>>> +
>>> +    cfg->pcm_instances =
>>> +    devm_kcalloc(_info->xb_dev->dev, num_devices,
>>> + sizeof(struct xen_front_cfg_pcm_instance),
>>> + GFP_KERNEL);
>>> +    if (!cfg->pcm_instances)
>>> +    return -ENOMEM;
>>> +
>>> +    for (i = 0; i < num_devices; i++) {
>>> +    ret = cfg_device(front_info, >pcm_instances[i],
>>> + >pcm_hw, xb_dev->nodename, i, stream_cnt);
>>> +    if (ret < 0)
>>> +    return ret;
>> Who will free all the memory allocated until now in case of an error?
> The memory is allocated with devm_xxx functions, so it will
> be freed on device destructions automatically
>>
>> And I think when removing the device freeing the memory is missing, too.
> Same as above, the kernel will take care of it while destroying the device

Okay, thanks.


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/5] ALSA: xen-front: Read sound driver configuration from Xen store

2018-04-17 Thread Oleksandr Andrushchenko 

On 04/16/2018 03:55 PM, Juergen Gross wrote:

On 16/04/18 08:24, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Read configuration values from Xen store according
to xen/interface/io/sndif.h protocol:
   - introduce configuration structures for different
 components, e.g. sound card, device, stream
   - read PCM HW parameters, e.g rate, format etc.
   - detect stream type (capture/playback)
   - read device and card parameters

Signed-off-by: Oleksandr Andrushchenko 
---
  sound/xen/Makefile|   3 +-
  sound/xen/xen_snd_front.c |   7 +
  sound/xen/xen_snd_front.h |   4 +
  sound/xen/xen_snd_front_cfg.c | 517 ++
  sound/xen/xen_snd_front_cfg.h |  46 
  5 files changed, 576 insertions(+), 1 deletion(-)
  create mode 100644 sound/xen/xen_snd_front_cfg.c
  create mode 100644 sound/xen/xen_snd_front_cfg.h

diff --git a/sound/xen/Makefile b/sound/xen/Makefile
index 4507ef3c27fd..06705bef61fa 100644
--- a/sound/xen/Makefile
+++ b/sound/xen/Makefile
@@ -1,5 +1,6 @@
  # SPDX-License-Identifier: GPL-2.0 OR MIT
  
-snd_xen_front-objs := xen_snd_front.o

+snd_xen_front-objs := xen_snd_front.o \
+ xen_snd_front_cfg.o
  
  obj-$(CONFIG_SND_XEN_FRONTEND) += snd_xen_front.o

diff --git a/sound/xen/xen_snd_front.c b/sound/xen/xen_snd_front.c
index f406a8f52c51..65d2494a9d14 100644
--- a/sound/xen/xen_snd_front.c
+++ b/sound/xen/xen_snd_front.c
@@ -25,6 +25,13 @@ static void xen_snd_drv_fini(struct xen_snd_front_info 
*front_info)
  
  static int sndback_initwait(struct xen_snd_front_info *front_info)

  {
+   int num_streams;
+   int ret;
+
+   ret = xen_snd_front_cfg_card(front_info, _streams);
+   if (ret < 0)
+   return ret;
+
return 0;
  }
  
diff --git a/sound/xen/xen_snd_front.h b/sound/xen/xen_snd_front.h

index 4ae204b23d32..b52226cb30bc 100644
--- a/sound/xen/xen_snd_front.h
+++ b/sound/xen/xen_snd_front.h
@@ -11,8 +11,12 @@
  #ifndef __XEN_SND_FRONT_H
  #define __XEN_SND_FRONT_H
  
+#include "xen_snd_front_cfg.h"

+
  struct xen_snd_front_info {
struct xenbus_device *xb_dev;
+
+   struct xen_front_cfg_card cfg;
  };
  
  #endif /* __XEN_SND_FRONT_H */

diff --git a/sound/xen/xen_snd_front_cfg.c b/sound/xen/xen_snd_front_cfg.c
new file mode 100644
index ..d461985afffa
--- /dev/null
+++ b/sound/xen/xen_snd_front_cfg.c
@@ -0,0 +1,517 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+/*
+ * Xen para-virtual sound device
+ *
+ * Copyright (C) 2016-2018 EPAM Systems Inc.
+ *
+ * Author: Oleksandr Andrushchenko 
+ */
+
+#include 
+
+#include 
+
+#include "xen_snd_front.h"
+#include "xen_snd_front_cfg.h"
+
+/* maximum number of supported streams */

Comment style (multiple times below, too):
Start with a capital letter and end sentences with a full stop.

will fix

+#define VSND_MAX_STREAM8
+
+struct cfg_hw_sample_rate {
+   const char *name;
+   unsigned int mask;
+   unsigned int value;
+};
+
+static const struct cfg_hw_sample_rate CFG_HW_SUPPORTED_RATES[] = {
+   { .name = "5512",   .mask = SNDRV_PCM_RATE_5512,   .value = 5512 },
+   { .name = "8000",   .mask = SNDRV_PCM_RATE_8000,   .value = 8000 },
+   { .name = "11025",  .mask = SNDRV_PCM_RATE_11025,  .value = 11025 },
+   { .name = "16000",  .mask = SNDRV_PCM_RATE_16000,  .value = 16000 },
+   { .name = "22050",  .mask = SNDRV_PCM_RATE_22050,  .value = 22050 },
+   { .name = "32000",  .mask = SNDRV_PCM_RATE_32000,  .value = 32000 },
+   { .name = "44100",  .mask = SNDRV_PCM_RATE_44100,  .value = 44100 },
+   { .name = "48000",  .mask = SNDRV_PCM_RATE_48000,  .value = 48000 },
+   { .name = "64000",  .mask = SNDRV_PCM_RATE_64000,  .value = 64000 },
+   { .name = "96000",  .mask = SNDRV_PCM_RATE_96000,  .value = 96000 },
+   { .name = "176400", .mask = SNDRV_PCM_RATE_176400, .value = 176400 },
+   { .name = "192000", .mask = SNDRV_PCM_RATE_192000, .value = 192000 },
+};
+
+struct cfg_hw_sample_format {
+   const char *name;
+   u64 mask;
+};
+
+static const struct cfg_hw_sample_format CFG_HW_SUPPORTED_FORMATS[] = {
+   {
+   .name = XENSND_PCM_FORMAT_U8_STR,
+   .mask = SNDRV_PCM_FMTBIT_U8
+   },
+   {
+   .name = XENSND_PCM_FORMAT_S8_STR,
+   .mask = SNDRV_PCM_FMTBIT_S8
+   },
+   {
+   .name = XENSND_PCM_FORMAT_U16_LE_STR,
+   .mask = SNDRV_PCM_FMTBIT_U16_LE
+   },
+   {
+   .name = XENSND_PCM_FORMAT_U16_BE_STR,
+   .mask = SNDRV_PCM_FMTBIT_U16_BE
+   },
+   {
+   .name = XENSND_PCM_FORMAT_S16_LE_STR,
+   .mask = SNDRV_PCM_FMTBIT_S16_LE
+   },
+   {
+   .name = XENSND_PCM_FORMAT_S16_BE_STR,
+   .mask = SNDRV_PCM_FMTBIT_S16_BE
+

Re: [Xen-devel] [PATCH v2 2/5] ALSA: xen-front: Read sound driver configuration from Xen store

2018-04-16 Thread Juergen Gross 
On 16/04/18 08:24, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
> 
> Read configuration values from Xen store according
> to xen/interface/io/sndif.h protocol:
>   - introduce configuration structures for different
> components, e.g. sound card, device, stream
>   - read PCM HW parameters, e.g rate, format etc.
>   - detect stream type (capture/playback)
>   - read device and card parameters
> 
> Signed-off-by: Oleksandr Andrushchenko 
> ---
>  sound/xen/Makefile|   3 +-
>  sound/xen/xen_snd_front.c |   7 +
>  sound/xen/xen_snd_front.h |   4 +
>  sound/xen/xen_snd_front_cfg.c | 517 
> ++
>  sound/xen/xen_snd_front_cfg.h |  46 
>  5 files changed, 576 insertions(+), 1 deletion(-)
>  create mode 100644 sound/xen/xen_snd_front_cfg.c
>  create mode 100644 sound/xen/xen_snd_front_cfg.h
> 
> diff --git a/sound/xen/Makefile b/sound/xen/Makefile
> index 4507ef3c27fd..06705bef61fa 100644
> --- a/sound/xen/Makefile
> +++ b/sound/xen/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0 OR MIT
>  
> -snd_xen_front-objs := xen_snd_front.o
> +snd_xen_front-objs := xen_snd_front.o \
> +   xen_snd_front_cfg.o
>  
>  obj-$(CONFIG_SND_XEN_FRONTEND) += snd_xen_front.o
> diff --git a/sound/xen/xen_snd_front.c b/sound/xen/xen_snd_front.c
> index f406a8f52c51..65d2494a9d14 100644
> --- a/sound/xen/xen_snd_front.c
> +++ b/sound/xen/xen_snd_front.c
> @@ -25,6 +25,13 @@ static void xen_snd_drv_fini(struct xen_snd_front_info 
> *front_info)
>  
>  static int sndback_initwait(struct xen_snd_front_info *front_info)
>  {
> + int num_streams;
> + int ret;
> +
> + ret = xen_snd_front_cfg_card(front_info, _streams);
> + if (ret < 0)
> + return ret;
> +
>   return 0;
>  }
>  
> diff --git a/sound/xen/xen_snd_front.h b/sound/xen/xen_snd_front.h
> index 4ae204b23d32..b52226cb30bc 100644
> --- a/sound/xen/xen_snd_front.h
> +++ b/sound/xen/xen_snd_front.h
> @@ -11,8 +11,12 @@
>  #ifndef __XEN_SND_FRONT_H
>  #define __XEN_SND_FRONT_H
>  
> +#include "xen_snd_front_cfg.h"
> +
>  struct xen_snd_front_info {
>   struct xenbus_device *xb_dev;
> +
> + struct xen_front_cfg_card cfg;
>  };
>  
>  #endif /* __XEN_SND_FRONT_H */
> diff --git a/sound/xen/xen_snd_front_cfg.c b/sound/xen/xen_snd_front_cfg.c
> new file mode 100644
> index ..d461985afffa
> --- /dev/null
> +++ b/sound/xen/xen_snd_front_cfg.c
> @@ -0,0 +1,517 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +/*
> + * Xen para-virtual sound device
> + *
> + * Copyright (C) 2016-2018 EPAM Systems Inc.
> + *
> + * Author: Oleksandr Andrushchenko 
> + */
> +
> +#include 
> +
> +#include 
> +
> +#include "xen_snd_front.h"
> +#include "xen_snd_front_cfg.h"
> +
> +/* maximum number of supported streams */

Comment style (multiple times below, too):
Start with a capital letter and end sentences with a full stop.

> +#define VSND_MAX_STREAM  8
> +
> +struct cfg_hw_sample_rate {
> + const char *name;
> + unsigned int mask;
> + unsigned int value;
> +};
> +
> +static const struct cfg_hw_sample_rate CFG_HW_SUPPORTED_RATES[] = {
> + { .name = "5512",   .mask = SNDRV_PCM_RATE_5512,   .value = 5512 },
> + { .name = "8000",   .mask = SNDRV_PCM_RATE_8000,   .value = 8000 },
> + { .name = "11025",  .mask = SNDRV_PCM_RATE_11025,  .value = 11025 },
> + { .name = "16000",  .mask = SNDRV_PCM_RATE_16000,  .value = 16000 },
> + { .name = "22050",  .mask = SNDRV_PCM_RATE_22050,  .value = 22050 },
> + { .name = "32000",  .mask = SNDRV_PCM_RATE_32000,  .value = 32000 },
> + { .name = "44100",  .mask = SNDRV_PCM_RATE_44100,  .value = 44100 },
> + { .name = "48000",  .mask = SNDRV_PCM_RATE_48000,  .value = 48000 },
> + { .name = "64000",  .mask = SNDRV_PCM_RATE_64000,  .value = 64000 },
> + { .name = "96000",  .mask = SNDRV_PCM_RATE_96000,  .value = 96000 },
> + { .name = "176400", .mask = SNDRV_PCM_RATE_176400, .value = 176400 },
> + { .name = "192000", .mask = SNDRV_PCM_RATE_192000, .value = 192000 },
> +};
> +
> +struct cfg_hw_sample_format {
> + const char *name;
> + u64 mask;
> +};
> +
> +static const struct cfg_hw_sample_format CFG_HW_SUPPORTED_FORMATS[] = {
> + {
> + .name = XENSND_PCM_FORMAT_U8_STR,
> + .mask = SNDRV_PCM_FMTBIT_U8
> + },
> + {
> + .name = XENSND_PCM_FORMAT_S8_STR,
> + .mask = SNDRV_PCM_FMTBIT_S8
> + },
> + {
> + .name = XENSND_PCM_FORMAT_U16_LE_STR,
> + .mask = SNDRV_PCM_FMTBIT_U16_LE
> + },
> + {
> + .name = XENSND_PCM_FORMAT_U16_BE_STR,
> + .mask = SNDRV_PCM_FMTBIT_U16_BE
> + },
> + {
> + .name = XENSND_PCM_FORMAT_S16_LE_STR,
> + .mask = SNDRV_PCM_FMTBIT_S16_LE
> + },
> + {
> +