2017-12-04 18:39:43

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver

On Wed, Nov 22, 2017 at 08:43:18PM +0900, Katsuhiro Suzuki wrote:

> sound/soc/uniphier/Makefile | 4 +
> sound/soc/uniphier/aio-core.c | 368 +++++++++++++++++++++
> sound/soc/uniphier/aio-dma.c | 266 +++++++++++++++
> sound/soc/uniphier/aio-regctrl.c | 699 +++++++++++++++++++++++++++++++++++++++
> sound/soc/uniphier/aio-regctrl.h | 495 +++++++++++++++++++++++++++
> sound/soc/uniphier/aio.h | 261 +++++++++++++++

Please split this up more, it looks like there's at least two or three
drivers in here and it winds up being quite large. There's at least a
DMA and a DAI driver. Looking through this my overall impression is
that this is a fairly large and complex audio subsystem with some DSP
and routing capacity which is being handled in a board specific fashion
rather than generically but it's kind of hard to tell as there's not
much description of what's going on so I'm needing to reverse engineer
things from the driver.

The code itself looks fairly clean, it's mainly a case of trying to
figure out if it's doing what it's supposed to with the limited
documentation.

> +int uniphier_aio_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + struct uniphier_aio *aio = uniphier_priv(dai);
> + struct uniphier_aio_sub *sub = &aio->sub[substream->stream];
> +
> + sub->params = *params;
> + sub->setting = 1;

So we don't validate the params at all?

> + uniphier_aio_port_reset(sub);
> + uniphier_aio_srcport_reset(sub);

Is there a mux in the SoC here?

> +static const struct of_device_id uniphier_aio_of_match[] = {
> +#ifdef CONFIG_SND_SOC_UNIPHIER_LD11
> + {
> + .compatible = "socionext,uniphier-ld11-aio",
> + .data = &uniphier_aio_ld11_spec,
> + },
> + {
> + .compatible = "socionext,uniphier-ld20-aio",
> + .data = &uniphier_aio_ld20_spec,
> + },
> +#endif /* CONFIG_SND_SOC_UNIPHIER_LD11 */

Why is there an ifdef here? There's no other conditional code in here,
it seems pointless.

> + for (j = 0; j < ARRAY_SIZE(aio->sub); j++) {
> + struct uniphier_aio_sub *sub = &aio->sub[j];
> +
> + if (!sub->running)
> + continue;
> +
> + spin_lock(&sub->spin);
> + uniphier_aio_rb_sync(sub);
> + uniphier_aio_rb_clear_int(sub);
> + spin_unlock(&sub->spin);

It's not 100% obvious what this does... a comment might help.

> +int uniphier_aio_chip_init(struct uniphier_aio_chip *chip)
> +{
> + struct regmap *r = chip->regmap;
> +
> + regmap_update_bits(r, A2APLLCTR0,
> + A2APLLCTR0_APLLXPOW_MASK,
> + A2APLLCTR0_APLLXPOW_PWON);
> +
> + regmap_update_bits(r, A2APLLCTR1, A2APLLCTR1_APLL_MASK,
> + A2APLLCTR1_APLLF2_33MHZ | A2APLLCTR1_APLLA2_33MHZ |
> + A2APLLCTR1_APLLF1_36MHZ | A2APLLCTR1_APLLA1_36MHZ);
> +
> + regmap_update_bits(r, A2EXMCLKSEL0,
> + A2EXMCLKSEL0_EXMCLK_MASK,
> + A2EXMCLKSEL0_EXMCLK_OUTPUT);
> +
> + regmap_update_bits(r, A2AIOINPUTSEL, A2AIOINPUTSEL_RXSEL_MASK,
> + A2AIOINPUTSEL_RXSEL_PCMI1_HDMIRX1 |
> + A2AIOINPUTSEL_RXSEL_PCMI2_SIF |
> + A2AIOINPUTSEL_RXSEL_PCMI3_EVEA |
> + A2AIOINPUTSEL_RXSEL_IECI1_HDMIRX1);

This definitely looks like there's some clocking and audio routing
within the SoC which should be exposed to userspace, or at the very
least machine driver configuration rather than being hard coded.

> + switch (pc) {
> + case IEC61937_PC_AC3:
> + repet = OPORTMXREPET_STRLENGTH_AC3 |
> + OPORTMXREPET_PMLENGTH_AC3;
> + pause |= OPORTMXPAUDAT_PAUSEPD_AC3;
> + break;
> + case IEC61937_PC_MPA:
> + repet = OPORTMXREPET_STRLENGTH_MPA |
> + OPORTMXREPET_PMLENGTH_MPA;
> + pause |= OPORTMXPAUDAT_PAUSEPD_MPA;
> + break;
> + case IEC61937_PC_MP3:
> + repet = OPORTMXREPET_STRLENGTH_MP3 |
> + OPORTMXREPET_PMLENGTH_MP3;
> + pause |= OPORTMXPAUDAT_PAUSEPD_MP3;
> + break;

This looks awfully like compressed audio support... should there be
integration with the compressed audio API/


Attachments:
(No filename) (3.83 kB)
signature.asc (488.00 B)
Download all attachments

2017-12-05 04:48:48

by Katsuhiro Suzuki

[permalink] [raw]
Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver

Hello,

Thanks a lot for your review.

> -----Original Message-----
> From: Mark Brown [mailto:[email protected]]
> Sent: Tuesday, December 5, 2017 3:40 AM
> To: Suzuki, Katsuhiro/$BNkLZ(B $B>!Gn(B <[email protected]>
> Cc: [email protected]; Rob Herring <[email protected]>; [email protected]; Yamada, Masahiro/$B;3(B
> $BED(B $B??90(B <[email protected]>; Masami Hiramatsu <[email protected]>; Jassi Brar
> <[email protected]>; [email protected]; [email protected]
> Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver
>
> On Wed, Nov 22, 2017 at 08:43:18PM +0900, Katsuhiro Suzuki wrote:
>
> > sound/soc/uniphier/Makefile | 4 +
> > sound/soc/uniphier/aio-core.c | 368 +++++++++++++++++++++
> > sound/soc/uniphier/aio-dma.c | 266 +++++++++++++++
> > sound/soc/uniphier/aio-regctrl.c | 699 +++++++++++++++++++++++++++++++++++++++
> > sound/soc/uniphier/aio-regctrl.h | 495 +++++++++++++++++++++++++++
> > sound/soc/uniphier/aio.h | 261 +++++++++++++++
>
> Please split this up more, it looks like there's at least two or three
> drivers in here and it winds up being quite large. There's at least a
> DMA and a DAI driver. Looking through this my overall impression is
> that this is a fairly large and complex audio subsystem with some DSP
> and routing capacity which is being handled in a board specific fashion
> rather than generically but it's kind of hard to tell as there's not
> much description of what's going on so I'm needing to reverse engineer
> things from the driver.
>
> The code itself looks fairly clean, it's mainly a case of trying to
> figure out if it's doing what it's supposed to with the limited
> documentation.
>
> > +int uniphier_aio_hw_params(struct snd_pcm_substream *substream,
> > + struct snd_pcm_hw_params *params,
> > + struct snd_soc_dai *dai)
> > +{
> > + struct uniphier_aio *aio = uniphier_priv(dai);
> > + struct uniphier_aio_sub *sub = &aio->sub[substream->stream];
> > +
> > + sub->params = *params;
> > + sub->setting = 1;
>
> So we don't validate the params at all?
>
> > + uniphier_aio_port_reset(sub);
> > + uniphier_aio_srcport_reset(sub);
>
> Is there a mux in the SoC here?
>

Sorry for confusing, It's not mux.

uniphier_srcport_reset() resets HW SRC (sampling rate converter) block.
Audio data out ports of UniPhier audio system have HW SRC.


> > +static const struct of_device_id uniphier_aio_of_match[] = {
> > +#ifdef CONFIG_SND_SOC_UNIPHIER_LD11
> > + {
> > + .compatible = "socionext,uniphier-ld11-aio",
> > + .data = &uniphier_aio_ld11_spec,
> > + },
> > + {
> > + .compatible = "socionext,uniphier-ld20-aio",
> > + .data = &uniphier_aio_ld20_spec,
> > + },
> > +#endif /* CONFIG_SND_SOC_UNIPHIER_LD11 */
>
> Why is there an ifdef here? There's no other conditional code in here,
> it seems pointless.
>

This config is used to support or not LD11 SoC.
aio-ld11.c is not build and 'uniphier_aio_ldxx_spec' is undefined if this config is disabled.

aio-ld11.c defines SoC dependent resources (port, HW ring buffer, DMA ch, etc.)
and fixed settings.
I know it's better to move such information into device-tree, but register areas of
UniPhier's audio system is very strange and interleaved. It's hard to split each nodes...


> > + for (j = 0; j < ARRAY_SIZE(aio->sub); j++) {
> > + struct uniphier_aio_sub *sub = &aio->sub[j];
> > +
> > + if (!sub->running)
> > + continue;
> > +
> > + spin_lock(&sub->spin);
> > + uniphier_aio_rb_sync(sub);
> > + uniphier_aio_rb_clear_int(sub);
> > + spin_unlock(&sub->spin);
>
> It's not 100% obvious what this does... a comment might help.
>
> > +int uniphier_aio_chip_init(struct uniphier_aio_chip *chip)
> > +{
> > + struct regmap *r = chip->regmap;
> > +
> > + regmap_update_bits(r, A2APLLCTR0,
> > + A2APLLCTR0_APLLXPOW_MASK,
> > + A2APLLCTR0_APLLXPOW_PWON);
> > +
> > + regmap_update_bits(r, A2APLLCTR1, A2APLLCTR1_APLL_MASK,
> > + A2APLLCTR1_APLLF2_33MHZ | A2APLLCTR1_APLLA2_33MHZ |
> > + A2APLLCTR1_APLLF1_36MHZ | A2APLLCTR1_APLLA1_36MHZ);
> > +
> > + regmap_update_bits(r, A2EXMCLKSEL0,
> > + A2EXMCLKSEL0_EXMCLK_MASK,
> > + A2EXMCLKSEL0_EXMCLK_OUTPUT);
> > +
> > + regmap_update_bits(r, A2AIOINPUTSEL, A2AIOINPUTSEL_RXSEL_MASK,
> > + A2AIOINPUTSEL_RXSEL_PCMI1_HDMIRX1 |
> > + A2AIOINPUTSEL_RXSEL_PCMI2_SIF |
> > + A2AIOINPUTSEL_RXSEL_PCMI3_EVEA |
> > + A2AIOINPUTSEL_RXSEL_IECI1_HDMIRX1);
>
> This definitely looks like there's some clocking and audio routing
> within the SoC which should be exposed to userspace, or at the very
> least machine driver configuration rather than being hard coded.
>
> > + switch (pc) {
> > + case IEC61937_PC_AC3:
> > + repet = OPORTMXREPET_STRLENGTH_AC3 |
> > + OPORTMXREPET_PMLENGTH_AC3;
> > + pause |= OPORTMXPAUDAT_PAUSEPD_AC3;
> > + break;
> > + case IEC61937_PC_MPA:
> > + repet = OPORTMXREPET_STRLENGTH_MPA |
> > + OPORTMXREPET_PMLENGTH_MPA;
> > + pause |= OPORTMXPAUDAT_PAUSEPD_MPA;
> > + break;
> > + case IEC61937_PC_MP3:
> > + repet = OPORTMXREPET_STRLENGTH_MP3 |
> > + OPORTMXREPET_PMLENGTH_MP3;
> > + pause |= OPORTMXPAUDAT_PAUSEPD_MP3;
> > + break;
>
> This looks awfully like compressed audio support... should there be
> integration with the compressed audio API/

Thanks, I'll try it. Is there Documentation in sound/designes/compress-offload.rst?
And best sample is... Intel's driver?


(Summary)
I think I should fix as follows:

- Split DMA, DAI patches from large one
- Validate parameters in hw_params
- Add description about HW SRC (or fix bad name 'srcport')
- Add comments about uniphier_aiodma_irq()
- Expose clocking and audio routing to userspace, or at the very
least machine driver configuration
- Support compress-audio API for S/PDIF

and post V2.


Regards,
--
Katsuhiro Suzuki




2017-12-05 12:40:11

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver

On Tue, Dec 05, 2017 at 01:48:39PM +0900, Katsuhiro Suzuki wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns. Doing this makes your messages much
easier to read and reply to.

> > Is there a mux in the SoC here?

> Sorry for confusing, It's not mux.

> uniphier_srcport_reset() resets HW SRC (sampling rate converter) block.
> Audio data out ports of UniPhier audio system have HW SRC.

Is the SRC just a single block sitting between the DMA and the external
audio port or is there more going on? Some of the other code made me
think the hardware was more flexible than this (all the writing to
registers with names like RXSEL for example).

> > > +#endif /* CONFIG_SND_SOC_UNIPHIER_LD11 */

> > Why is there an ifdef here? There's no other conditional code in here,
> > it seems pointless.

> This config is used to support or not LD11 SoC.
> aio-ld11.c is not build and 'uniphier_aio_ldxx_spec' is undefined if this config is disabled.
>
> aio-ld11.c defines SoC dependent resources (port, HW ring buffer, DMA ch, etc.)
> and fixed settings.
> I know it's better to move such information into device-tree, but register areas of
> UniPhier's audio system is very strange and interleaved. It's hard to split each nodes...

I'd expect this code to be structured more like a library - have a
driver that handles the specific IPs then have it call into a shared
block of code that does the generic bits. Though in this case the
device specific bit looks like a couple of tiny data tables so I'm not
sure it's worth making it conditional or separate at all.

> > This looks awfully like compressed audio support... should there be
> > integration with the compressed audio API/

> Thanks, I'll try it. Is there Documentation in sound/designes/compress-offload.rst?
> And best sample is... Intel's driver?

Yes.

> (Summary)
> I think I should fix as follows:

> - Split DMA, DAI patches from large one
> - Validate parameters in hw_params
> - Add description about HW SRC (or fix bad name 'srcport')
> - Add comments about uniphier_aiodma_irq()
> - Expose clocking and audio routing to userspace, or at the very
> least machine driver configuration
> - Support compress-audio API for S/PDIF

> and post V2.

At least. I do think we need to get to the bottom of how flexible the
hardware is first though.


Attachments:
(No filename) (2.33 kB)
signature.asc (488.00 B)
Download all attachments

2017-12-06 06:03:27

by Katsuhiro Suzuki

[permalink] [raw]
Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver

Hello,

> -----Original Message-----
> From: Mark Brown [mailto:[email protected]]
> Sent: Tuesday, December 5, 2017 9:15 PM
> To: Suzuki, Katsuhiro/$BNkLZ(B $B>!Gn(B <[email protected]>
> Cc: [email protected]; Rob Herring <[email protected]>;
> [email protected]; Yamada, Masahiro/$B;3ED(B $B??90(B
> <[email protected]>; Masami Hiramatsu
> <[email protected]>; Jassi Brar <[email protected]>;
> [email protected]; [email protected]
> Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver
>
> On Tue, Dec 05, 2017 at 01:48:39PM +0900, Katsuhiro Suzuki wrote:
>
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns. Doing this makes your messages much
> easier to read and reply to.
>

Thank you, I set it.


> > > Is there a mux in the SoC here?
>
> > Sorry for confusing, It's not mux.
>
> > uniphier_srcport_reset() resets HW SRC (sampling rate converter) block.
> > Audio data out ports of UniPhier audio system have HW SRC.
>
> Is the SRC just a single block sitting between the DMA and the external
> audio port or is there more going on? Some of the other code made me
> think the hardware was more flexible than this (all the writing to
> registers with names like RXSEL for example).
>

This hardware has 2 types of HW SRC. First type sit before audio port and
cannot change routing. I use it in this driver. Second type is like a loopback,
but this block is not used in this driver to simplify the first version.

Type 1:
Mem -> DMA -> SRC -> Out Port -> External pin
Type 2:
Mem -> DMA -> SRC -> Out Port -> In Port -> DMA -> Mem


> > > > +#endif /* CONFIG_SND_SOC_UNIPHIER_LD11 */
>
> > > Why is there an ifdef here? There's no other conditional code in here,
> > > it seems pointless.
>
> > This config is used to support or not LD11 SoC.
> > aio-ld11.c is not build and 'uniphier_aio_ldxx_spec' is undefined if this
config is
> disabled.
> >
> > aio-ld11.c defines SoC dependent resources (port, HW ring buffer, DMA ch,
etc.)
> > and fixed settings.
> > I know it's better to move such information into device-tree, but register
areas of
> > UniPhier's audio system is very strange and interleaved. It's hard to split
each
> nodes...
>
> I'd expect this code to be structured more like a library - have a
> driver that handles the specific IPs then have it call into a shared
> block of code that does the generic bits. Though in this case the
> device specific bit looks like a couple of tiny data tables so I'm not
> sure it's worth making it conditional or separate at all.
>

Sorry... I agree your opinion, but I can't imagine the detail.

I think my driver has structure as follows (ex. startup):
DAI: uniphier_aio_startup()@aio-core.c
Lib: uniphier_aio_init()@aio-regctrl.c
SoC specific: [email protected]

Am I wrong? Would you mean split the functions in aio-regctl.[ch] to other
kernel module? I wonder if you could tell me the example from existing
drivers. I'll try to fix my driver like as it.


> > > This looks awfully like compressed audio support... should there be
> > > integration with the compressed audio API/
>
> > Thanks, I'll try it. Is there Documentation in
> sound/designes/compress-offload.rst?
> > And best sample is... Intel's driver?
>
> Yes.
>
> > (Summary)
> > I think I should fix as follows:
>
> > - Split DMA, DAI patches from large one
> > - Validate parameters in hw_params
> > - Add description about HW SRC (or fix bad name 'srcport')
> > - Add comments about uniphier_aiodma_irq()
> > - Expose clocking and audio routing to userspace, or at the very
> > least machine driver configuration
> > - Support compress-audio API for S/PDIF
>
> > and post V2.
>
> At least. I do think we need to get to the bottom of how flexible the
> hardware is first though.

Yes, indeed. This hardware is more flexible and complex, but now I (and our
company) don't use it. Of course, I don't want to hide some features of this
hardware from ALSA people. I should try to upstream all features in the future,
I think.


Regards,
--
Katsuhiro Suzuki



2017-12-06 12:58:29

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver

On Wed, Dec 06, 2017 at 03:03:18PM +0900, Katsuhiro Suzuki wrote:

> > I'd expect this code to be structured more like a library - have a
> > driver that handles the specific IPs then have it call into a shared
> > block of code that does the generic bits. Though in this case the
> > device specific bit looks like a couple of tiny data tables so I'm not
> > sure it's worth making it conditional or separate at all.

> Sorry... I agree your opinion, but I can't imagine the detail.

> I think my driver has structure as follows (ex. startup):
> DAI: uniphier_aio_startup()@aio-core.c
> Lib: uniphier_aio_init()@aio-regctrl.c
> SoC specific: [email protected]

> Am I wrong? Would you mean split the functions in aio-regctl.[ch] to other
> kernel module? I wonder if you could tell me the example from existing
> drivers. I'll try to fix my driver like as it.

One example is how all the drivers that use the generic dmaengine code
instantiate their DMA drivers, or how all the drivers for CODECs that
have both I2C and SPIi control interfaces instantiate - given that the
device specific code here seems to be mostly data tables that's probably
the closest thing.

> > At least. I do think we need to get to the bottom of how flexible the
> > hardware is first though.

> Yes, indeed. This hardware is more flexible and complex, but now I (and our
> company) don't use it. Of course, I don't want to hide some features of this
> hardware from ALSA people. I should try to upstream all features in the future,
> I think.

My main concern here is to make sure that when you decide you need to
use the more complex hardware that this can be done without too much
pain to existing machines (and that they can benefit from as much of the
enhanced functionality as is possible).


Attachments:
(No filename) (1.76 kB)
signature.asc (488.00 B)
Download all attachments

2017-12-11 09:22:06

by Katsuhiro Suzuki

[permalink] [raw]
Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver

Hello,

> One example is how all the drivers that use the generic dmaengine code
> instantiate their DMA drivers, or how all the drivers for CODECs that
> have both I2C and SPIi control interfaces instantiate - given that the
> device specific code here seems to be mostly data tables that's probably
> the closest thing.

Thank you. I'm checking the ALSA drivers of other companies, I found Qualcomm's
QTi LPASS driver is similar with my wanted.


> > Thanks, I'll try it. Is there Documentation in
sound/designes/compress-offload.rst?
> > And best sample is... Intel's driver?
>
> Yes.

I read Intel's driver, I understand how to define the compress CPU DAI and
snd_compr_ops. The driver of Intel Atom (at sst-mfld-platform-pcm.c) defines
following DAI:
{
.name = "compress-cpu-dai",
.compress_new = snd_soc_new_compress,
.ops = &sst_compr_dai_ops,
.playback = {
.stream_name = "Compress Playback",
.channels_min = 1,
},
},

But I can't find how to use/map this DAI in machine driver or Device-Tree or
something. I think that it's same as PCM DAI, am I correct?

I read compress-offload.rst, but I can't find how do I test it. It seems aplay
of
alsa-util doesn't know compress audio formats. Should I use PulseAudio or
Android HAL to test compress audio APIs?


Regards,
--
Katsuhiro Suzuki


> -----Original Message-----
> From: Mark Brown [mailto:[email protected]]
> Sent: Wednesday, December 6, 2017 9:58 PM
> To: Suzuki, Katsuhiro/$BNkLZ(B $B>!Gn(B <[email protected]>
> Cc: [email protected]; Rob Herring <[email protected]>;
> [email protected]; Yamada, Masahiro/$B;3ED(B $B??90(B
> <[email protected]>; Masami Hiramatsu
> <[email protected]>; Jassi Brar <[email protected]>;
> [email protected]; [email protected]
> Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver
>
> On Wed, Dec 06, 2017 at 03:03:18PM +0900, Katsuhiro Suzuki wrote:
>
> > > I'd expect this code to be structured more like a library - have a
> > > driver that handles the specific IPs then have it call into a shared
> > > block of code that does the generic bits. Though in this case the
> > > device specific bit looks like a couple of tiny data tables so I'm not
> > > sure it's worth making it conditional or separate at all.
>
> > Sorry... I agree your opinion, but I can't imagine the detail.
>
> > I think my driver has structure as follows (ex. startup):
> > DAI: uniphier_aio_startup()@aio-core.c
> > Lib: uniphier_aio_init()@aio-regctrl.c
> > SoC specific: [email protected]
>
> > Am I wrong? Would you mean split the functions in aio-regctl.[ch] to other
> > kernel module? I wonder if you could tell me the example from existing
> > drivers. I'll try to fix my driver like as it.
>
> One example is how all the drivers that use the generic dmaengine code
> instantiate their DMA drivers, or how all the drivers for CODECs that
> have both I2C and SPIi control interfaces instantiate - given that the
> device specific code here seems to be mostly data tables that's probably
> the closest thing.
>
> > > At least. I do think we need to get to the bottom of how flexible the
> > > hardware is first though.
>
> > Yes, indeed. This hardware is more flexible and complex, but now I (and our
> > company) don't use it. Of course, I don't want to hide some features of this
> > hardware from ALSA people. I should try to upstream all features in the
future,
> > I think.
>
> My main concern here is to make sure that when you decide you need to
> use the more complex hardware that this can be done without too much
> pain to existing machines (and that they can benefit from as much of the
> enhanced functionality as is possible).


2017-12-11 15:16:45

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver

On Mon, Dec 11, 2017 at 06:21:58PM +0900, Katsuhiro Suzuki wrote:

> But I can't find how to use/map this DAI in machine driver or Device-Tree or
> something. I think that it's same as PCM DAI, am I correct?

Yes, that probably makes sense from a binding point of view.

> I read compress-offload.rst, but I can't find how do I test it. It seems aplay
> of
> alsa-util doesn't know compress audio formats. Should I use PulseAudio or
> Android HAL to test compress audio APIs?

IIRC tinyalsa has a compressed API test application - Vinod?


Attachments:
(No filename) (538.00 B)
signature.asc (488.00 B)
Download all attachments

2017-12-11 17:45:17

by Vinod Koul

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver

On Mon, Dec 11, 2017 at 03:16:29PM +0000, Mark Brown wrote:
> On Mon, Dec 11, 2017 at 06:21:58PM +0900, Katsuhiro Suzuki wrote:
>
> > But I can't find how to use/map this DAI in machine driver or Device-Tree or
> > something. I think that it's same as PCM DAI, am I correct?
>
> Yes, that probably makes sense from a binding point of view.
>
> > I read compress-offload.rst, but I can't find how do I test it. It seems aplay
> > of
> > alsa-util doesn't know compress audio formats. Should I use PulseAudio or
> > Android HAL to test compress audio APIs?
>
> IIRC tinyalsa has a compressed API test application - Vinod?

I guess it was sheer luck that i saw this :) email in CC reads
[email protected]! I don't work for Linaro, not yet :D

And to the answer the question, Yes we have compressed API test application
in tinycompress which is located at git.alsa-project.org:tinycompress.git

We have both compressed audio playback as well as record test app, cplay and
crecord.

HTH
--
~Vinod

2017-12-12 04:33:42

by Katsuhiro Suzuki

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver

Hello Vinod, Mark,

> -----Original Message-----
> From: Vinod Koul [mailto:[email protected]]
> Sent: Tuesday, December 12, 2017 2:49 AM
> To: Mark Brown <[email protected]>
> Cc: Suzuki, Katsuhiro/$BNkLZ(B $B>!Gn(B <[email protected]>;
> [email protected]; [email protected]; Masami Hiramatsu
> <[email protected]>; Yamada, Masahiro/$B;3ED(B $B??90(B
> <[email protected]>; [email protected]; Jassi Brar
> <[email protected]>; Rob Herring <[email protected]>;
> [email protected]
> Subject: Re: [alsa-devel] [PATCH 5/8] ASoC: uniphier: add support for UniPhier
> AIO driver
>
> On Mon, Dec 11, 2017 at 03:16:29PM +0000, Mark Brown wrote:
> > On Mon, Dec 11, 2017 at 06:21:58PM +0900, Katsuhiro Suzuki wrote:
> >
> > > But I can't find how to use/map this DAI in machine driver or Device-Tree
or
> > > something. I think that it's same as PCM DAI, am I correct?
> >
> > Yes, that probably makes sense from a binding point of view.
> >
> > > I read compress-offload.rst, but I can't find how do I test it. It seems
aplay
> > > of
> > > alsa-util doesn't know compress audio formats. Should I use PulseAudio or
> > > Android HAL to test compress audio APIs?
> >
> > IIRC tinyalsa has a compressed API test application - Vinod?
>
> I guess it was sheer luck that i saw this :) email in CC reads
> [email protected]! I don't work for Linaro, not yet :D
>
> And to the answer the question, Yes we have compressed API test application
> in tinycompress which is located at git.alsa-project.org:tinycompress.git
>
> We have both compressed audio playback as well as record test app, cplay and
> crecord.
>

Ah, I didn't check tinyalsa, thanks a lot!
I'll try it.


Regards,
--
Katsuhiro Suzuki


> HTH
> --
> ~Vinod