Re: [PATCH v2 3/3] gemalto: add PLS8 support

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Sebastian,

On 08/30/2017 11:30 AM, Sebastian Arnd wrote:
> diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
> index 7c33c22..17d0be9 100644
> --- a/drivers/atmodem/sim.c
> +++ b/drivers/atmodem/sim.c

This patch should come before the plugins/gemalto.c changes...

> @@ -64,7 +64,7 @@ static const char *pinnum_prefix[] = { "%PINNUM:", NULL };
>   static const char *oercn_prefix[] = { "_OERCN:", NULL };
>   static const char *cpinr_prefixes[] = { "+CPINR:", "+CPINRE:", NULL };
>   static const char *epin_prefix[] = { "*EPIN:", NULL };
> -static const char *spic_prefix[] = { "+SPIC:", NULL };
> +static const char *spic_prefix[] = { "+SPIC:", "^SPIC:", NULL };

Okay, so looks like both Cinterion/Gemalto and SimCom use SPIC, except 
you use '^SPIC' vs '+SPIC' for SimCom.  While this change is likely 
harmless, I'd rather play it safe and use a separate variable.  Maybe 
caret_spic_prefix, or spic_gemalto_prefix?

>   static const char *pct_prefix[] = { "#PCT:", NULL };
>   static const char *pnnm_prefix[] = { "+PNNM:", NULL };
>   static const char *qpinc_prefix[] = { "+QPINC:", NULL };
> @@ -875,6 +875,62 @@ error:
>   	CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
>   }
>   
> +static void at_spic_gemalto_cb(gboolean ok, GAtResult *result,
> +							gpointer user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_sim_pin_retries_cb_t cb = cbd->cb;
> +	const char *final = g_at_result_final_response(result);
> +	GAtResultIter iter;
> +	struct ofono_error error;
> +	int retries[OFONO_SIM_PASSWORD_INVALID];
> +	size_t i;
> +	static enum ofono_sim_password_type password_types[] = {
> +		OFONO_SIM_PASSWORD_SIM_PIN,
> +		OFONO_SIM_PASSWORD_SIM_PUK,
> +		OFONO_SIM_PASSWORD_PHSIM_PIN,
> +		OFONO_SIM_PASSWORD_PHFSIM_PUK,
> +		OFONO_SIM_PASSWORD_SIM_PIN2,
> +		OFONO_SIM_PASSWORD_SIM_PUK2,
> +		OFONO_SIM_PASSWORD_PHNET_PIN,
> +		OFONO_SIM_PASSWORD_PHNET_PUK,
> +	};
> +
> +	decode_at_error(&error, final);
> +
> +	if (!ok) {
> +		cb(&error, NULL, cbd->data);
> +		return;
> +	}
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	for (i = 0; i < OFONO_SIM_PASSWORD_INVALID; i++)
> +		retries[i] = -1;
> +
> +	for (i = 0; i <  ARRAY_SIZE(password_types); i++) {
> +		int val;
> +
> +		if (!g_at_result_iter_next(&iter, "^SPIC:"))
> +			goto error;
> +
> +		if (!g_at_result_iter_next_number(&iter, &val))
> +			goto error;
> +
> +		retries[password_types[i]] = val;
> +
> +		DBG("retry counter id=%d, val=%d", password_types[i],
> +						retries[password_types[i]]);
> +	}
> +
> +	cb(&error, retries, cbd->data);
> +
> +	return;
> +
> +error:
> +	CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
> +}
> +
>   #define AT_PCT_SET_RETRIES(retries, pin_type, value) \
>   	retries[pin_type] = value; \
>   	DBG("retry counter id=%d, val=%d", pin_type, value);
> @@ -1104,6 +1160,18 @@ static void at_pin_retries_query(struct ofono_sim *sim,
>   					at_spic_cb, cbd, g_free) > 0)
>   			return;
>   		break;
> +	case OFONO_VENDOR_CINTERION:

Should we be introducing OFONO_VENDOR_GEMALTO?

> +		if (g_at_chat_send(sd->chat, "AT^SPIC=\"SC\",0;"
> +					 " ^SPIC=\"SC\",1;"
> +					 " ^SPIC=\"PS\",0;"
> +					 " ^SPIC=\"PS\",1;"
> +					 " ^SPIC=\"P2\",0;"
> +					 " ^SPIC=\"P2\",1;"
> +					 " ^SPIC=\"PN\",0;"
> +					 " ^SPIC=\"PN\",1",

Please just use tabs for indentation here, don't mix tabs + spaces.

> +			     spic_prefix, at_spic_gemalto_cb, cbd, g_free) > 0)
> +			return;
> +		break;
>   	case OFONO_VENDOR_TELIT:
>   		if (g_at_chat_send(sd->chat, "AT#PCT", pct_prefix,
>   					at_pct_cb, cbd, g_free) > 0)
> 

Regards,
-Denis