Re: [PATCH 1/3] ASoC: codecs: adau1701: allow configuration of PLL mode pins

[Daniel Mack <zonque@gmail.com>]

On 21.06.2013 06:46, Rajeev kumar wrote:
> Daniel,
> 
> On 6/20/2013 10:59 PM, Daniel Mack wrote:
>> The ADAU1701 has 2 hardware pins to configure the PLL mode in accordance
>> to the MCLK-to-LRCLK ratio. These pins have to be stable before the chip
>> is released from reset, and a full reset cycle, including a new firmware
>> download is needed whenever they change.
>>
>> This patch adds GPIO properties to the DT bindings of the Codec, and
>> implements makes the set_sysclk memorize the configured sysclk.
>>
>> To avoid excessive reset cycles and firmware downloads, the default
>> clock divider can be specified in DT as well. Whenever a ratio change is
>> detected in the hw_params callback, the PLL mode lines are updates and a
>> full reset cycle is issued.
>>
>> Signed-off-by: Daniel Mack<zonque@gmail.com>
>> ---
>>   .../devicetree/bindings/sound/adi,adau1701.txt     |  14 +++
>>   sound/soc/codecs/adau1701.c                        | 107 +++++++++++++++++----
>>   sound/soc/codecs/adau1701.h                        |   4 +
>>   3 files changed, 104 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/adi,adau1701.txt b/Documentation/devicetree/bindings/sound/adi,adau1701.txt
>> index 3afeda7..a0d7e92 100644
>> --- a/Documentation/devicetree/bindings/sound/adi,adau1701.txt
>> +++ b/Documentation/devicetree/bindings/sound/adi,adau1701.txt
>> @@ -11,6 +11,19 @@ Optional properties:
>>    - reset-gpio: 		A GPIO spec to define which pin is connected to the
>>   			chip's !RESET pin. If specified, the driver will
>>   			assert a hardware reset at probe time.
>> + - adi,pll-clkdiv: 	The PLL clock divider, specifing the ratio between
>> +			MCLK and fsclk. The value is used to determine the
>> +			correct state of the two mode pins below.
>> +			Note that this value can be overridden at runtime
>> +			by passing the ADAU1701_CLKDIV_MCLK_LRCLK divider
>> +			with ASoC calls. However, the chips needs a full
>> +			reset cycle and a new firmware download each time
>> +			the configuration changes.
>> + - adi,pll-mode-gpios:	An array of two GPIO specs to describe the GPIOs
>> +			the ADAU's PLL config pins are connected to.
>> +			The state of the pins are set according to the
>> +			configured clock divider on ASoC side before the
>> +			firmware is loaded.
>>
>>   Examples:
>>
>> @@ -19,5 +32,6 @@ Examples:
>>   			compatible = "adi,adau1701";
>>   			reg =<0x34>;
>>   			reset-gpio =<&gpio 23 0>;
>> +			adi,pll-mode-gpios =<&gpio 24 0&gpio 25 0>;
>>   		};
>>   	};
>> diff --git a/sound/soc/codecs/adau1701.c b/sound/soc/codecs/adau1701.c
>> index b6b1a77..e6ce4fe 100644
>> --- a/sound/soc/codecs/adau1701.c
>> +++ b/sound/soc/codecs/adau1701.c
>> @@ -91,7 +91,11 @@
>>
>>   struct adau1701 {
>>   	int gpio_nreset;
>> +	int gpio_pll_mode0;
>> +	int gpio_pll_mode1;
> 
> combine in single line.

I disagree for the sake of readability.

> 
>>   	unsigned int dai_fmt;
>> +	unsigned int pll_clkdiv;
>> +	unsigned int sysclk;
>>   };
>>
>>   static const struct snd_kcontrol_new adau1701_controls[] = {
>> @@ -184,13 +188,37 @@ static unsigned int adau1701_read(struct snd_soc_codec *codec, unsigned int reg)
>>   	return value;
>>   }
>>
>> -static void adau1701_reset(struct snd_soc_codec *codec)
>> +static void adau1701_reset(struct snd_soc_codec *codec, unsigned int clkdiv)
>>   {
>>   	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);
>>
>>   	if (!gpio_is_valid(adau1701->gpio_nreset))
>>   		return;
>>
>> +	if (gpio_is_valid(adau1701->gpio_pll_mode0)&&
>> +	    gpio_is_valid(adau1701->gpio_pll_mode1)) {
>> +		switch (adau1701->pll_clkdiv) {
>> +		case 64:
> 
> magic number?

That's a divider value. How and why would you possibly add a #define for
that?

> 
>> +			gpio_set_value(adau1701->gpio_pll_mode0, 0);
>> +			gpio_set_value(adau1701->gpio_pll_mode1, 0);
>> +			break;
>> +		case 256:
>> +			gpio_set_value(adau1701->gpio_pll_mode0, 0);
>> +			gpio_set_value(adau1701->gpio_pll_mode1, 1);
>> +			break;
>> +		case 384:
>> +			gpio_set_value(adau1701->gpio_pll_mode0, 1);
>> +			gpio_set_value(adau1701->gpio_pll_mode1, 0);
>> +			break;
>> +		case 512:
>> +			gpio_set_value(adau1701->gpio_pll_mode0, 1);
>> +			gpio_set_value(adau1701->gpio_pll_mode1, 1);
>> +			break;
>> +		}
>> +	}
>> +
>> +	adau1701->pll_clkdiv = clkdiv;
>> +
>>   	gpio_set_value(adau1701->gpio_nreset, 0);
>>   	/* minimum reset time is 20ns */
>>   	udelay(1);
>> @@ -199,24 +227,6 @@ static void adau1701_reset(struct snd_soc_codec *codec)
>>   	mdelay(85);
>>   }
>>
>> -static int adau1701_init(struct snd_soc_codec *codec)
>> -{
>> -	int ret;
>> -	struct i2c_client *client = to_i2c_client(codec->dev);
>> -
>> -	adau1701_reset(codec);
>> -
>> -	ret = process_sigma_firmware(client, ADAU1701_FIRMWARE);
>> -	if (ret) {
>> -		dev_warn(codec->dev, "Failed to load firmware\n");
>> -		return ret;
>> -	}
>> -
>> -	snd_soc_write(codec, ADAU1701_DACSET, ADAU1701_DACSET_DACINIT);
>> -
>> -	return 0;
>> -}
>> -
>>   static int adau1701_set_capture_pcm_format(struct snd_soc_codec *codec,
>>   		snd_pcm_format_t format)
>>   {
>> @@ -291,9 +301,22 @@ static int adau1701_hw_params(struct snd_pcm_substream *substream,
>>   		struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
>>   {
>>   	struct snd_soc_codec *codec = dai->codec;
>> +	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);
>>   	snd_pcm_format_t format;
>>   	unsigned int val;
>>
>> +	if (adau1701->sysclk) {
>> +		unsigned int clkdiv = adau1701->sysclk / params_rate(params);
> 
> It will give warning.

Please elaborate.



Thanks,
Daniel