[3/3] ASoC: rt5645: Wait for 400msec before concluding on value of RT5645_VENDOR_ID2
diff mbox

Message ID 1509741345-1589-4-git-send-email-alexander.deucher@amd.com
State New
Headers show

Commit Message

Alex Deucher Nov. 3, 2017, 8:35 p.m. UTC
From: Akshu Agrawal <akshu.agrawal@amd.com>

Minimum time required between power On of codec and read
of RT5645_VENDOR_ID2 is 400msec. We should wait and attempt
before erroring out.

BUG=b:66978383
TEST=Cold boot the device and check for sound device.

Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com>
Signed-off-by: Bard Liao <bardliao@realtek.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 sound/soc/codecs/rt5645.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Mark Brown Nov. 6, 2017, 4:24 p.m. UTC | #1
On Fri, Nov 03, 2017 at 04:35:45PM -0400, Alex Deucher wrote:

> Minimum time required between power On of codec and read
> of RT5645_VENDOR_ID2 is 400msec. We should wait and attempt
> before erroring out.

So the description says we have to wait 400ms before attempting a
read...

> BUG=b:66978383

What does this mean?

> @@ -3786,6 +3789,15 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
>  	}
>  	regmap_read(regmap, RT5645_VENDOR_ID2, &val);
>  
> +	/*
> +	 * Read for 400msec, as it is the interval required between
> +	 * read and power On.
> +	 */
> +	while (val != RT5645_DEVICE_ID && val != RT5650_DEVICE_ID && --timeout) {
> +		msleep(1);
> +		regmap_read(regmap, RT5645_VENDOR_ID2, &val);
> +	}
> +

...but what we actually do is try to read up to 400 times starting well
before that 400ms is up.  This directly contradicts what the commit
message said we needed, may take a lot longer if the chip misbehaves on
the I2C bus while it's not ready (which wouldn't be that much of a
surprise), might lead to us reporting before the chip is really stable
(if the read happens to work while the chip isn't yet stable) and could
cause lots of noise on the console if the I2C controller gets upset.
What are we actually waiting for here?

If we really need 400ms of dead reckoning time (which is a lot for a
modern chip, that feels more like a VMID ramp) then it's going to be
safer to just do that.
Akshu Agrawal Nov. 7, 2017, 7:12 a.m. UTC | #2
On 11/6/2017 9:54 PM, Mark Brown wrote:
> On Fri, Nov 03, 2017 at 04:35:45PM -0400, Alex Deucher wrote:
> 
>> Minimum time required between power On of codec and read
>> of RT5645_VENDOR_ID2 is 400msec. We should wait and attempt
>> before erroring out.
> 
> So the description says we have to wait 400ms before attempting a
> read...

Yes, that's true.

> 
>> BUG=b:66978383
> 
> What does this mean?

A bug ID. Removing it in V2.

> 
>> @@ -3786,6 +3789,15 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
>>   	}
>>   	regmap_read(regmap, RT5645_VENDOR_ID2, &val);
>>   
>> +	/*
>> +	 * Read for 400msec, as it is the interval required between
>> +	 * read and power On.
>> +	 */
>> +	while (val != RT5645_DEVICE_ID && val != RT5650_DEVICE_ID && --timeout) {
>> +		msleep(1);
>> +		regmap_read(regmap, RT5645_VENDOR_ID2, &val);
>> +	}
>> +
> 
> ...but what we actually do is try to read up to 400 times starting well
> before that 400ms is up.  This directly contradicts what the commit
> message said we needed, may take a lot longer if the chip misbehaves on
> the I2C bus while it's not ready (which wouldn't be that much of a
> surprise), might lead to us reporting before the chip is really stable
> (if the read happens to work while the chip isn't yet stable) and could
> cause lots of noise on the console if the I2C controller gets upset.
> What are we actually waiting for here?
> 

In my understanding if we get RT5645 or RT5650 DEVICE ID from register 
RT5645_VENDOR_ID2 then that means the chip is stable.

> If we really need 400ms of dead reckoning time (which is a lot for a
> modern chip, that feels more like a VMID ramp) then it's going to be
> safer to just do that.
> 

Agreed, will just sleep for 400ms before reading the register value.

Patch
diff mbox

diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index 23cc2cb..2da0b33 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -55,6 +55,8 @@  MODULE_PARM_DESC(quirk, "RT5645 pdata quirk override");
 
 #define RT5645_HWEQ_NUM 57
 
+#define TIME_TO_POWER_MS 400
+
 static const struct regmap_range_cfg rt5645_ranges[] = {
 	{
 		.name = "PR",
@@ -3712,6 +3714,7 @@  static int rt5645_i2c_probe(struct i2c_client *i2c,
 	int ret, i;
 	unsigned int val;
 	struct regmap *regmap;
+	int timeout = TIME_TO_POWER_MS;
 
 	rt5645 = devm_kzalloc(&i2c->dev, sizeof(struct rt5645_priv),
 				GFP_KERNEL);
@@ -3786,6 +3789,15 @@  static int rt5645_i2c_probe(struct i2c_client *i2c,
 	}
 	regmap_read(regmap, RT5645_VENDOR_ID2, &val);
 
+	/*
+	 * Read for 400msec, as it is the interval required between
+	 * read and power On.
+	 */
+	while (val != RT5645_DEVICE_ID && val != RT5650_DEVICE_ID && --timeout) {
+		msleep(1);
+		regmap_read(regmap, RT5645_VENDOR_ID2, &val);
+	}
+
 	switch (val) {
 	case RT5645_DEVICE_ID:
 		rt5645->regmap = devm_regmap_init_i2c(i2c, &rt5645_regmap);