Re: [PATCH v2 3/5] ASoC: mt6797: add mt6797 platform driver

[Mark Brown <broonie@kernel.org>]

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

On Mon, Apr 16, 2018 at 08:32:50AM +0800, KaiChieh Chuang wrote:
> add support for mt6797 playback and capture
> to MediaTek sound codec,
> through MediaTek Proprietary audio interface.
> add memory interface as FE dai.
> add ADDA interface as BE dai.

Again this looks pretty good, some small comments but nothing major:

> +		/* set output mode */
> +		if (rate == 96000) {
> +			dl_src2_con0 |= (0x2 << 24); /* UP_SAMPLING_RATE_X4 */
> +			dl_src2_con0 |= 1 << 14;
> +		} else if (rate == 192000) {
> +			dl_src2_con0 |= (0x1 << 24); /* UP_SAMPLING_RATE_X2 */
> +			dl_src2_con0 |= 1 << 14;
> +		} else {
> +			dl_src2_con0 |= (0x3 << 24); /* UP_SAMPLING_RATE_X8 */
> +		}

This looks like a switch statement.

> +		/* turn of mute function */
> +		dl_src2_con0 |= (0x03 << 11);

Is there any reason for not implementing this as a mute_stream()
callback?  That'll keep the DAI muted during startup and before teardown
which will do the most to avoid any noise making it into the output.

> +static irqreturn_t mt6797_afe_irq_handler(int irq_id, void *dev)
> +{
> +	struct mtk_base_afe *afe = dev;
> +	struct mtk_base_afe_irq *irq;
> +	unsigned int status;
> +	unsigned int mcu_en;
> +	int ret;
> +	int i;
> +
> +	/* get irq that is sent to MCU */
> +	regmap_read(afe->regmap, AFE_IRQ_MCU_EN, &mcu_en);

...

> +	/* clear irq */
> +	regmap_write(afe->regmap,
> +		     AFE_IRQ_MCU_CLR,
> +		     status & AFE_IRQ_STATUS_BITS);
> +
> +	return IRQ_HANDLED;
> +}

This unconditionally returns IRQ_HANDLED even if we didn't get an
interrupt - it's better to return IRQ_NONE unless we actually handled
something since that is more robust if something goes wrong, it lets the
IRQ core shut up interrupts that get latched on and works with shared
interrupts if any future designs do that.

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

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