[alsa-devel] [PATCH 3/3] ASoC: rt5645: Wait for 400msec before concluding on value of RT5645_VENDOR_ID2

[Agrawal, Akshu]

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.