Re: [PATCH 2/4]simutil: Changes to prepare for isimodem2.5

[Marcel Holtmann <marcel@holtmann.org>]

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

Hi Jessica,

> updates in src/simutil.h
> Adding some sim file id and sim dir id mostly related to phonebook usim/sim.

please write a proper commit message here. The subject is not acceptable
since it misleads what this patch is doing.

Also "updates in src/..." comments are not really useful. We have the
diffstat for this.

> ---
>  src/simutil.h |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/src/simutil.h b/src/simutil.h
> index 92b2e0f..bc53255 100644
> --- a/src/simutil.h
> +++ b/src/simutil.h
> @@ -19,6 +19,8 @@
>   *
>   */
>  
> +#include "types.h"
> +

What is this include for? Please remove it.

>  enum sim_fileid {
>  	SIM_EFPL_FILEID = 0x2f05,
>  	SIM_EF_ICCID_FILEID = 0x2fe2,
> @@ -49,6 +51,25 @@ enum sim_fileid {
>  	SIM_EFCBMIR_FILEID = 0x6f50,
>  	SIM_EFCBMI_FILEID = 0x6f45,
>  	SIM_EFCBMID_FILEID = 0x6f48,
> +	SIM_EFSMSP_FILEID = 0x6f42,
> +	SIM_EFIMSI_FILEID = 0x6F07,
> +
> +	/* Phonebook (USIM) */
> +	SIM_EFPBR_FILEID = 0x4f30,
> +	SIM_EFPSC_FILEID = 0x4F22,
> +	SIM_EFCC_FILEID = 0x4F23,
> +	SIM_EFPUID_FILEID = 0x4F24,
> +	/* Phonebook (SIM) */
> +	SIM_EFADN_SIM_FILEID = 0x6F3A,
> +	SIM_EFEXT1_SIM_FILEID = 0x6F4A,
> +};

So please be consistent with lower-case hex encoding. So 0x6f07 etc.

Also we should keep this information sorted. I know there is a sorting
bug in there that needs to be fixed as well.

And while at it, you could be the one that makes this enum finally
compliant to our coding style. This is one of our old left-overs that we
have get fixed. We just never got around to it. Are you up for such a
task?

So you could just fix the coding style in one patch, fix the sorting in
another and then add the new values.

> +enum sim_dirid {
> +	MF_FILEID = 0x3F00,
> +	DFTELECOM_FILEID = 0x7F10,
> +	DFGSM_FILEID = 0x7F20,
> +	DFPHONEBOOK_FILEID = 0x5F3A,
> +	DFMULTIMEDIA_FILEID = 0x5F3B
>  };

Please use a proper SIM_* prefix here and you can just put them into the
sim_fileid enum. However this should be a separate patch to get a clear
commit history of the changes.

Regards

Marcel