Re: [RFC PATCH 1/2] ASoC: mediatek: add btcvsd driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2443 bytes --]

On Thu, Jan 24, 2019 at 04:58:41PM +0800, KaiChieh Chuang wrote:

> The driver function for transferring/receiving
> BT encoded data to/from BT hardware.

It's really nice to see someone submitting a driver for actual radio
hardware, this is great to see!  

The driver looks pretty good, I've got a few quite minor comments below
but the main one is that given that this doesn't have any DAIs or DAPM
it looks like it doesn't integrate with the rest of the sound card much
so perhaps it could just be a free standing ALSA driver?  If there's
inputs that let you link it to the rest of the sound card which will be
added later then that's obviously a good reason to keep it in ASoC (I'd
not be surprised if there were) but otherwise I'm not sure what
advantage ASoC brings you.

> +static void mtk_btcvsd_snd_set_state(struct mtk_btcvsd_snd *bt,
> +				     struct mtk_btcvsd_snd_stream *bt_stream,
> +				     int state)
> +{
> +	dev_info(bt->dev, "%s(), stream %d, state %d, tx->state %d, rx->state %d, irq_disabled %d\n",
> +		 __func__,
> +		 bt_stream->stream, state,
> +		 bt->tx->state, bt->rx->state, bt->irq_disabled);

There's quite a few of these dev_info() calls which look like they might
be a bit noisy and could perhaps be dev_dbg() instead.

> +/* kcontrol */
> +static const char *const btsco_band_str[] = {"NB", "WB"};
> +static const char *const btsco_mute_str[] = {"Off", "On"};
> +static const char *const irq_received_str[] = {"No", "Yes"};
> +static const char *const rx_timeout_str[] = {"No", "Yes"};
> +
> +static const struct soc_enum btcvsd_enum[] = {
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(btsco_band_str), btsco_band_str),
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(btsco_mute_str), btsco_mute_str),
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(irq_received_str), irq_received_str),
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(rx_timeout_str), rx_timeout_str),
> +};

Some of these enums (all of them except the narrow/wide band one) should
be simple number controls ending with Switch as covered in
control-names.rst, this helps UIs display them better.

> +	SND_SOC_BYTES_TLV("btcvsd_tx_timestamp",
> +			  sizeof(struct mtk_btcvsd_snd_time_buffer_info),
> +			  btcvsd_tx_timestamp_get, NULL),

There's an ALSA timestamping API - see commit 4eeaaeaea1ce (ALSA: core:
add hooks for audio timestamps) and related commits.  I don't know if
that applies here and might be a better fit or not?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]