Hi Lauro,
> > +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'.
OK, I'll try to improve 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.
The problem here is related to the NCI definition.
The NCI defines that if only 1 target is discovered (the common case),
it's automatically activated by the controller (i.e. in state POLL_ACTIVE).
On the other hand, if more than 1 target is discovered, the controller
sends RF_DISCOVER_NTF for each target, and the host need to select
one (this case is much less common and I didn't implemented it yet).
So, we're talking about the first case: once we start polling, and
discover only 1 target, it's automatically activated.
Now, if we call start_poll again, we have to deactivate the target before
starting the poll again (as dictated by the NCI spec).
I think this is the simplest solution to this issue: implicitly deactivate
the target before starting to poll.
Another alternative is to always deactivate the target after it was
automatically activated, and when activate_target is called, we'll have to
poll again and hope that the same target will be activated again...
I think this solution is much less preferred, since it's much more
complicated and contradictes the NCI idea.
Please advise.
>
>
>
> > +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.
You're correct, of course.
I see in your pn533 implementation that you also store the arguments
(in the field pn533.cmd_complete_arg), but you do protect them with the
'cmd_lock' semaphore.
If already in progress of data exchange transaction, you return EBUSY.
I can also protect the 'cb' and 'cb_context' with such a semaphore.
In fact, this is an implementation of a queue with max length of 1
data exchange transaction.
Please advise.
Thanks & BR,
Ilan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
