All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Maruthi Srinivas Bayyavarapu  <maruthi.srinivas.bayyavarapu@xilinx.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>,
	Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Vishal Sagar <vsagar@xilinx.com>,
	Michal Simek <michal.simek@xilinx.com>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	alsa-devel@alsa-project.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/3] ASoC: xlnx: add pcm formatter platform driver
Date: Thu, 20 Dec 2018 15:06:56 +0000	[thread overview]
Message-ID: <20181220150656.GB9325@sirena.org.uk> (raw)
In-Reply-To: <1545239949-112845-3-git-send-email-maruthi.srinivas.bayyavarapu@xilinx.com>

[-- Attachment #1: Type: text/plain, Size: 1657 bytes --]

On Wed, Dec 19, 2018 at 10:49:08PM +0530, Maruthi Srinivas Bayyavarapu wrote:

This looks mostly good, a few small things but all pretty trivial:

> +	switch (bits_per_sample) {
> +	case 8:
> +		val |= (BIT_DEPTH_8 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 16:
> +		val |= (BIT_DEPTH_16 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 20:
> +		val |= (BIT_DEPTH_20 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 24:
> +		val |= (BIT_DEPTH_24 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 32:
> +		val |= (BIT_DEPTH_32 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	}

It'd be better to have a default case returning an error for unsupported
sample sizes.

> +		ret = devm_request_irq(dev, aud_drv_data->mm2s_irq,
> +				       xlnx_mm2s_irq_handler, 0,
> +				       "xlnx_formatter_pcm_mm2s_irq", dev);
> +		if (ret) {
> +			dev_err(dev, "xlnx audio mm2s irq request failed\n");
> +			goto clk_err;
> +		}
> +		ret = xlnx_formatter_pcm_reset(aud_drv_data->mmio +
> +					       XLNX_MM2S_OFFSET);
> +		if (ret) {
> +			dev_err(dev, "audio formatter reset failed\n");
> +			goto clk_err;
> +		}
> +		xlnx_formatter_disable_irqs(aud_drv_data->mmio +
> +					    XLNX_MM2S_OFFSET,
> +					    SNDRV_PCM_STREAM_PLAYBACK);

This requests the interrupt before we've reset and quiesced the hardware
meaning the interrupt could fire immediately on request with the
hardware in an unknown state; it'd be better to reset the hardware first
then request the interrupt to minimize the risk that something goes
wrong with the handler.

> +	dev_info(dev, "pcm platform device registered\n");
> +	return 0;

This is just adding noise to the log.

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

WARNING: multiple messages have this Message-ID (diff)
From: Mark Brown <broonie@kernel.org>
To: Maruthi Srinivas Bayyavarapu <maruthi.srinivas.bayyavarapu@xilinx.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
	linux-kernel@vger.kernel.org, Takashi Iwai <tiwai@suse.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	Michal Simek <michal.simek@xilinx.com>,
	Vishal Sagar <vsagar@xilinx.com>
Subject: Re: [PATCH 2/3] ASoC: xlnx: add pcm formatter platform driver
Date: Thu, 20 Dec 2018 15:06:56 +0000	[thread overview]
Message-ID: <20181220150656.GB9325@sirena.org.uk> (raw)
In-Reply-To: <1545239949-112845-3-git-send-email-maruthi.srinivas.bayyavarapu@xilinx.com>


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

On Wed, Dec 19, 2018 at 10:49:08PM +0530, Maruthi Srinivas Bayyavarapu wrote:

This looks mostly good, a few small things but all pretty trivial:

> +	switch (bits_per_sample) {
> +	case 8:
> +		val |= (BIT_DEPTH_8 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 16:
> +		val |= (BIT_DEPTH_16 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 20:
> +		val |= (BIT_DEPTH_20 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 24:
> +		val |= (BIT_DEPTH_24 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 32:
> +		val |= (BIT_DEPTH_32 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	}

It'd be better to have a default case returning an error for unsupported
sample sizes.

> +		ret = devm_request_irq(dev, aud_drv_data->mm2s_irq,
> +				       xlnx_mm2s_irq_handler, 0,
> +				       "xlnx_formatter_pcm_mm2s_irq", dev);
> +		if (ret) {
> +			dev_err(dev, "xlnx audio mm2s irq request failed\n");
> +			goto clk_err;
> +		}
> +		ret = xlnx_formatter_pcm_reset(aud_drv_data->mmio +
> +					       XLNX_MM2S_OFFSET);
> +		if (ret) {
> +			dev_err(dev, "audio formatter reset failed\n");
> +			goto clk_err;
> +		}
> +		xlnx_formatter_disable_irqs(aud_drv_data->mmio +
> +					    XLNX_MM2S_OFFSET,
> +					    SNDRV_PCM_STREAM_PLAYBACK);

This requests the interrupt before we've reset and quiesced the hardware
meaning the interrupt could fire immediately on request with the
hardware in an unknown state; it'd be better to reset the hardware first
then request the interrupt to minimize the risk that something goes
wrong with the handler.

> +	dev_info(dev, "pcm platform device registered\n");
> +	return 0;

This is just adding noise to the log.

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

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



WARNING: multiple messages have this Message-ID (diff)
From: Mark Brown <broonie@kernel.org>
To: Maruthi Srinivas Bayyavarapu <maruthi.srinivas.bayyavarapu@xilinx.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
	linux-kernel@vger.kernel.org, Takashi Iwai <tiwai@suse.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	Jaroslav Kysela <perex@perex.cz>,
	Michal Simek <michal.simek@xilinx.com>,
	Vishal Sagar <vsagar@xilinx.com>
Subject: Re: [PATCH 2/3] ASoC: xlnx: add pcm formatter platform driver
Date: Thu, 20 Dec 2018 15:06:56 +0000	[thread overview]
Message-ID: <20181220150656.GB9325@sirena.org.uk> (raw)
In-Reply-To: <1545239949-112845-3-git-send-email-maruthi.srinivas.bayyavarapu@xilinx.com>


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

On Wed, Dec 19, 2018 at 10:49:08PM +0530, Maruthi Srinivas Bayyavarapu wrote:

This looks mostly good, a few small things but all pretty trivial:

> +	switch (bits_per_sample) {
> +	case 8:
> +		val |= (BIT_DEPTH_8 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 16:
> +		val |= (BIT_DEPTH_16 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 20:
> +		val |= (BIT_DEPTH_20 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 24:
> +		val |= (BIT_DEPTH_24 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 32:
> +		val |= (BIT_DEPTH_32 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	}

It'd be better to have a default case returning an error for unsupported
sample sizes.

> +		ret = devm_request_irq(dev, aud_drv_data->mm2s_irq,
> +				       xlnx_mm2s_irq_handler, 0,
> +				       "xlnx_formatter_pcm_mm2s_irq", dev);
> +		if (ret) {
> +			dev_err(dev, "xlnx audio mm2s irq request failed\n");
> +			goto clk_err;
> +		}
> +		ret = xlnx_formatter_pcm_reset(aud_drv_data->mmio +
> +					       XLNX_MM2S_OFFSET);
> +		if (ret) {
> +			dev_err(dev, "audio formatter reset failed\n");
> +			goto clk_err;
> +		}
> +		xlnx_formatter_disable_irqs(aud_drv_data->mmio +
> +					    XLNX_MM2S_OFFSET,
> +					    SNDRV_PCM_STREAM_PLAYBACK);

This requests the interrupt before we've reset and quiesced the hardware
meaning the interrupt could fire immediately on request with the
hardware in an unknown state; it'd be better to reset the hardware first
then request the interrupt to minimize the risk that something goes
wrong with the handler.

> +	dev_info(dev, "pcm platform device registered\n");
> +	return 0;

This is just adding noise to the log.

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2018-12-20 15:07 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-19 17:19 [PATCH 0/3] add Xilinx audio formatter driver Maruthi Srinivas Bayyavarapu
2018-12-19 17:19 ` Maruthi Srinivas Bayyavarapu
2018-12-19 17:19 ` Maruthi Srinivas Bayyavarapu
2018-12-19 17:19 ` [PATCH 1/3] dt-bindings: ASoC: xlnx,audio-formatter: Document audio formatter bindings Maruthi Srinivas Bayyavarapu
2018-12-19 17:19   ` [PATCH 1/3] dt-bindings: ASoC: xlnx, audio-formatter: " Maruthi Srinivas Bayyavarapu
2018-12-19 17:19   ` [PATCH 1/3] dt-bindings: ASoC: xlnx,audio-formatter: " Maruthi Srinivas Bayyavarapu
2018-12-19 17:19 ` [PATCH 2/3] ASoC: xlnx: add pcm formatter platform driver Maruthi Srinivas Bayyavarapu
2018-12-19 17:19   ` Maruthi Srinivas Bayyavarapu
2018-12-19 17:19   ` Maruthi Srinivas Bayyavarapu
2018-12-20 15:06   ` Mark Brown [this message]
2018-12-20 15:06     ` Mark Brown
2018-12-20 15:06     ` Mark Brown
2018-12-19 17:19 ` [PATCH 3/3] ASoC: xlnx: enable audio formatter driver build Maruthi Srinivas Bayyavarapu
2018-12-19 17:19   ` Maruthi Srinivas Bayyavarapu
2018-12-19 17:19   ` Maruthi Srinivas Bayyavarapu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181220150656.GB9325@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=alsa-devel@alsa-project.org \
    --cc=devicetree@vger.kernel.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=maruthi.srinivas.bayyavarapu@xilinx.com \
    --cc=michal.simek@xilinx.com \
    --cc=perex@perex.cz \
    --cc=robh+dt@kernel.org \
    --cc=tiwai@suse.com \
    --cc=vsagar@xilinx.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.