linux-kernel - Re: [PATCH v2] sound/soc/adi/axi-spdif.c: Support programmable master clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <548813AC.1080105@metafoo.de>
Date:	Wed, 10 Dec 2014 10:34:36 +0100
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Mike Looijmans <mike.looijmans@...ic.nl>
CC:	alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] sound/soc/adi/axi-spdif.c: Support programmable master
 clock

On 12/05/2014 01:37 PM, Mike Looijmans wrote:
> If the master clock supports programmable rates, program it to generate
> the desired frequency. Only apply constraints when the clock is fixed.
> This allows proper clock generation for both 44100 and 48000 Hz based
> sampling rates if the platform supports it.
>
> The clock frequency must be set before enabling it. Enabling the clock
> was done in "startup", but that occurs before "hw_params" where the rate
> is known. Enabling a programmable clock without first setting a valid
> frequency may harm the system. Move the clock start to the hw_params
> routine, and keep track of whether the clock has been started, because
> shutdown may be called without having called hw_params first.
> Starting the clock and enabling the SPDIF output AFTER programming the
> dividers is a more logical order anyway.
>
> To detect if the source clock is fixed, the driver calls clk_round_rate
> for two frequencies. If the results are equal, or if the call returns
> an error, the driver assumes the clock is fixed.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@...ic.nl>

Hi,

Sorry for the delay.

[...]
>
> +	/* Try to set the master clock */
> +	clk_set_rate(spdif->clk_ref, rate * 128);
> +
>   	clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(spdif->clk_ref),
>   			rate * 64 * 2) - 1;
>   	clkdiv <<= AXI_SPDIF_CTRL_CLKDIV_OFFSET;
> @@ -103,6 +108,14 @@ static int axi_spdif_hw_params(struct snd_pcm_substream *substream,
>   	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
>   		AXI_SPDIF_CTRL_CLKDIV_MASK, clkdiv);
>
> +	ret = clk_prepare_enable(spdif->clk_ref);

I'm still not convinced this is the right place. I do see your point. But it 
just feels wrong to enable the clock in hw_params. It's a bit of a dilemma. 
the startup callback is to early, hw_params is the wrong place and we can't 
put it in the trigger callback as the trigger callback can not sleep.

But in any way hwparmas can be called multiple times, so you need to handle 
the case where the clock is already enabled.

> +	if (ret)
> +		return ret;
> +	spdif->clk_ref_running = true;
> +
> +	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
> +		AXI_SPDIF_CTRL_TXEN, AXI_SPDIF_CTRL_TXEN);

This should probably be moved to the trigger callback though.

> +
>   	return 0;
>   }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/