Hi Ilan,
2011/9/13 Elias, Ilan <ilane@xxxxxx>:
> The NFC Controller Interface (NCI) is a standard
> communication protocol between an NFC Controller (NFCC)
> and a Device Host (DH), defined by the NFC Forum.
>
> Signed-off-by: Ilan Elias <ilane@xxxxxx>
> ---
> +
> +static void nci_init_complete_req(struct nci_dev *ndev, unsigned long opt)
> +{
> + struct nci_rf_disc_map_cmd cmd;
> + struct nci_core_conn_create_cmd conn_cmd;
> + int i;
> +
> + /* create static rf connection */
> + conn_cmd.target_handle = 0;
> + conn_cmd.num_target_specific_params = 0;
> + nci_send_cmd(ndev, NCI_OP_CORE_CONN_CREATE_CMD, 2, &conn_cmd);
> +
> + /* set rf mapping configurations */
> + cmd.num_mapping_configs = 0;
> +
> + /* by default mapping is set to NCI_RF_INTERFACE_FRAME */
> + for (i = 0; i < ndev->num_supported_rf_interfaces; i++) {
> + if (ndev->supported_rf_interfaces[i] ==
> + NCI_RF_INTERFACE_ISO_DEP) {
> + cmd.mapping_configs[cmd.num_mapping_configs]
> + .rf_protocol = NCI_RF_PROTOCOL_ISO_DEP;
> + cmd.mapping_configs[cmd.num_mapping_configs]
> + .mode = NCI_DISC_MAP_MODE_BOTH;
> + cmd.mapping_configs[cmd.num_mapping_configs]
> + .rf_interface_type = NCI_RF_INTERFACE_ISO_DEP;
> + cmd.num_mapping_configs++;
> + } else if (ndev->supported_rf_interfaces[i] ==
> + NCI_RF_INTERFACE_NFC_DEP) {
> + cmd.mapping_configs[cmd.num_mapping_configs]
> + .rf_protocol = NCI_RF_PROTOCOL_NFC_DEP;
> + cmd.mapping_configs[cmd.num_mapping_configs]
> + .mode = NCI_DISC_MAP_MODE_BOTH;
> + cmd.mapping_configs[cmd.num_mapping_configs]
> + .rf_interface_type = NCI_RF_INTERFACE_NFC_DEP;
> + cmd.num_mapping_configs++;
> + }
I think you should improve the readability in this 'if'.
> +
> +static int nci_start_poll(struct nfc_dev *nfc_dev, __u32 protocols)
> +{
> + struct nci_dev *ndev = nfc_get_drvdata(nfc_dev);
> + int rc;
> +
> + nfc_dbg("entry");
> +
> + if (test_bit(NCI_DISCOVERY, &ndev->flags)) {
> + nfc_err("unable to start poll, since poll is already active");
> + return -EBUSY;
> + }
> +
> + if (test_bit(NCI_POLL_ACTIVE, &ndev->flags)) {
> + nfc_dbg("target already active, first deactivate...");
> +
> + rc = nci_request(ndev, nci_rf_deactivate_req, 0,
> + msecs_to_jiffies(NCI_RF_DEACTIVATE_TIMEOUT));
> + if (rc)
> + return -EBUSY;
> + }
I don't think it is a good idea to implicitly deactivate a target. I
think you should just return EBUSY.
> +static int nci_data_exchange(struct nfc_dev *nfc_dev, __u32 target_idx,
> + struct sk_buff *skb,
> + data_exchange_cb_t cb,
> + void *cb_context)
> +{
> + struct nci_dev *ndev = nfc_get_drvdata(nfc_dev);
> +
> + nfc_dbg("entry, target_idx %d, len %d", target_idx, skb->len);
> +
> + if (!ndev->target_active_prot) {
> + nfc_err("unable to exchange data, no active target");
> + return -EINVAL;
> + }
> +
> + /* store cb and context to be used on receiving data */
> + ndev->data_exchange_cb = cb;
> + ndev->data_exchange_cb_context = cb_context;
This is wrong. The data exchange can be called again before completing
the previous call. So, you shouldn't store the cb and cb_context in
the device.
> +
> + return nci_send_data(ndev, ndev->conn_id, skb);
> +}
> +
Lauro
��.n��������+%������w��{.n�����{���zW����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
