Hi Iiro,
2011/8/3 <iiro.kaihlani...@nokia.com>:
> diff --git a/drivers/isimodem/network-registration.c
> b/drivers/isimodem/network-registration.c
> index cc48579..ffcfab4 100644
> --- a/drivers/isimodem/network-registration.c
> +++ b/drivers/isimodem/network-registration.c
> @@ -946,6 +946,84 @@ error:
> g_free(cbd);
> }
>
> +static void cs_access_config_resp_cb(const GIsiMessage *msg, void *data)
> +{
> + struct ofono_netreg *netreg = data;
> + struct netreg_data *nd = ofono_netreg_get_data(netreg);
> + GIsiSubBlockIter iter;
> +
> + DBG("");
> + if (g_isi_msg_id(msg) != NET_NW_ACCESS_CONF_RESP)
> + return;
Indent with a single tab, no spaces allowed. This applies to the rest
of the patch as well.
> + for (g_isi_sb_iter_init(&iter, msg, 2);
> + g_isi_sb_iter_is_valid(&iter);
> + g_isi_sb_iter_next(&iter))
> + {
> + uint8_t id = g_isi_sb_iter_get_id(&iter);
> + uint8_t mode;
> +
> + DBG("SB=%02X", id);
> + if ((id == 0x55) || (id == 0x59))
> + {
> + g_isi_sb_iter_get_byte(&iter, &mode, 2);
> + DBG("Reg %X", mode);
> + }
> + else if ((id == 0x56) || (id == 0x5A))
> + {
> + g_isi_sb_iter_get_byte(&iter, &mode, 2);
> + DBG("Roam %X", mode);
> + }
These subblock IDs need to be added in network.h. I would also
recommend a switch statement here for more readability.
> + else
> + {
> + DBG("Unknown subblock");
> + }
> + }
Regarding the entire for and if blocks above and elsewhere in the
patch, the style in oFono is to attach curly brackets on the same line
as the for/if/while statement. Please take a look at
doc/coding-style.txt.
To catch style errors early, I would recommend also using the
checkpatch.pl script before submitting.
> +}
> +
> +static void cs_state_resp_cb(const GIsiMessage *msg, void *data)
> +{
> + struct ofono_netreg *netreg = data;
> + struct netreg_data *nd = ofono_netreg_get_data(netreg);
> + uint8_t code;
> +
> + DBG("");
> + if (g_isi_msg_id(msg) != NET_CS_STATE_RESP)
> + return;
> +
> + if (!g_isi_msg_data_get_byte(msg, 0, &code))
> + return;
> +
> + if (code != NET_CAUSE_OK)
> + {
> + DBG("Failed with cause=%X", code);
> + return;
> + }
> +
> + if (!g_isi_msg_data_get_byte(msg, 1, &code))
> + return;
> + DBG("CS STATE=%X", code);
> +
> + if (code == NET_CS_INACTIVE)
> + {
> + DBG("CS INACTIVE - DO POWER ON NOT IMPLEMENTED!!!!!!");
No shouting please ;)
> + }
> + else
> + {
> + /* Enable registration and roaming */
> + const uint8_t req[] = {
> + NET_NW_ACCESS_CONF_REQ, 0, 2,
> + /* Subblock 1 */
> + 0x59, 4, 1, 0,
> + /* Subblock 2 */
> + 0x5A, 4, 1, 0,
Should really add these subblock IDs in network.h.
> + };
> +
> + DBG("CS ACTIVE - Check access config");
> + g_isi_client_send(nd->client, req, sizeof(req),
> cs_access_config_resp_cb, netreg, NULL);
Please take a look at coding style rule O2, and rather than putting
all this in an else statement, just return above if code ==
NET_CS_INACTIVE.
By the way, is CS access config ever set if when probing the CS state
is NET_CS_INACTIVE? Should we also subscribe to the equivalent
indication to drive this code?
> + }
> +}
> +
> static void subscribe_indications(GIsiClient *cl, void *data)
> {
> g_isi_client_ind_subscribe(cl, NET_RSSI_IND, rssi_ind_cb, data);
> @@ -994,6 +1072,11 @@ static void pn_modem_network_reachable_cb(const
> GIsiMessage *msg, void *data)
> struct ofono_netreg *netreg = data;
> struct netreg_data *nd = ofono_netreg_get_data(netreg);
>
> + const uint8_t req[] = {
> + NET_CS_STATE_REQ,
> + };
> +
> +
Indent with tabs here.
Cheers,
Aki
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
