Re: [RFC PATCH v1 1/2] huawei: poll sim state

[Kalle Valo <kalle.valo@canonical.com>]

[-- Attachment #1: Type: text/plain, Size: 2077 bytes --]

Marcel Holtmann <marcel@holtmann.org> writes:

> Hi Kalle,
>
>> On my Huawei E1552 when I plug in the modem (ie. cold start) with PIN locked
>> SIM, the sim state is 255 (HUAWEI_SIM_STATE_NOT_EXISTENT). As the modem
>> doesn't send ^SIMST notifications, poll the sim state until it's ready.
>> 
>> In theory it might be possible to do this better, for example follow
>> ^BOOT notifications or something, but it's unknown what parameter we
>> should check for.
>

[...]

>> +	if (sim_state == HUAWEI_SIM_STATE_NOT_EXISTENT) {
>>  		ofono_sim_inserted_notify(data->sim, FALSE);
>> +
>> +		/* SIM is not ready, try again a bit later */
>> +		return TRUE;
>> +	}
>>  	else
>>  		ofono_sim_inserted_notify(data->sim, TRUE);
>
> Coding style. } else on the same line. You should know this by now ;)

Oops :)

> Also in this case since you do return TRUE anyway from the if block, why
> bother with else. Just don't.

Good point. I changed that.
>> +	if (rerun && data->sim_poll_count < MAX_SIM_POLL_COUNT) {
>> +		data->sim_poll_count++;
>> +		data->query_sim_state = g_timeout_add_seconds(2,
>> +								query_sim_state,
>> +								modem);
>
> Now I don't like the query_sim_state name. It should sim_poll_timeout or
> something similar. At least make it clear that it is timeout.

I didn't know if you talked only about about the name of structure field
or also about the function name? I only changed the structure name.

>> +
>> +	/* query_sim_state(modem); */
>>  }
>
> What is this ???

A leftover from my tests. Removed

>>  static GAtChat *create_port(const char *device)
>> @@ -546,6 +586,9 @@ static int huawei_disable(struct ofono_modem *modem)
>>  
>>  	DBG("%p", modem);
>>  
>> +	if (data->query_sim_state)
>> +		g_source_remove(data->query_sim_state);
>> +
>
> You need to set the timeout variable also to 0. Otherwise you have some
> funny side effect then next time enabling the modem.
>
> Also I prefer check for the timeout variables with > 0.

Fixed.

I'll send v2 shortly.

-- 
Kalle Valo