Re: [PATCH v2] NFC: update to NCI spec 1.0 draft 18

[Gustavo Padovan <padovan@profusion.mobi>]

Hi Elias,

* Elias, Ilan <ilane@ti.com> [2011-11-01 09:49:44 +0100]:

> > > -static void nci_rf_activate_ntf_packet(struct nci_dev *ndev,
> > > -					struct sk_buff *skb)
> > > +static void nci_rf_intf_activated_ntf_packet(struct nci_dev *ndev,
> > > +						struct sk_buff *skb)
> > >  {
> > > -	struct nci_rf_activate_ntf ntf;
> > > +	struct nci_rf_intf_activated_ntf ntf;
> > >  	__u8 *data = skb->data;
> > > -	int rc = -1;
> > > +	int err = 0;
> > >  
> > >  	clear_bit(NCI_DISCOVERY, &ndev->flags);
> > >  	set_bit(NCI_POLL_ACTIVE, &ndev->flags);
> > >  
> > > -	ntf.target_handle = *data++;
> > > +	ntf.rf_discovery_id = *data++;
> > > +	ntf.rf_interface_type = *data++;
> > >  	ntf.rf_protocol = *data++;
> > > -	ntf.rf_tech_and_mode = *data++;
> > > +	ntf.activation_rf_tech_and_mode = *data++;
> > >  	ntf.rf_tech_specific_params_len = *data++;
> > >  
> > > -	nfc_dbg("target_handle %d, rf_protocol 0x%x, 
> > rf_tech_and_mode 0x%x, rf_tech_specific_params_len %d",
> > > -		ntf.target_handle,
> > > -		ntf.rf_protocol,
> > > -		ntf.rf_tech_and_mode,
> > > +	nfc_dbg("rf_discovery_id %d", ntf.rf_discovery_id);
> > > +	nfc_dbg("rf_interface_type 0x%x", ntf.rf_interface_type);
> > > +	nfc_dbg("rf_protocol 0x%x", ntf.rf_protocol);
> > > +	nfc_dbg("activation_rf_tech_and_mode 0x%x",
> > > +		ntf.activation_rf_tech_and_mode);
> > > +	nfc_dbg("rf_tech_specific_params_len %d",
> > >  		ntf.rf_tech_specific_params_len);
> > >  
> > > -	switch (ntf.rf_tech_and_mode) {
> > > -	case NCI_NFC_A_PASSIVE_POLL_MODE:
> > > -		rc = nci_rf_activate_nfca_passive_poll(ndev, &ntf,
> > > -			data);
> > > -		break;
> > > +	if (ntf.rf_tech_specific_params_len > 0) {
> > > +		switch (ntf.activation_rf_tech_and_mode) {
> > > +		case NCI_NFC_A_PASSIVE_POLL_MODE:
> > > +			data = 
> > nci_extract_rf_params_nfca_passive_poll(ndev,
> > > +				&ntf, data);
> > > +			break;
> > > +
> > > +		default:
> > > +			nfc_err("unsupported 
> > activation_rf_tech_and_mode 0x%x",
> > > +				ntf.activation_rf_tech_and_mode);
> > > +			return;
> > > +		}
> > > +	}
> > >  
> > > -	default:
> > > -		nfc_err("unsupported rf_tech_and_mode 0x%x",
> > > -			ntf.rf_tech_and_mode);
> > > -		return;
> > > +	ntf.data_exchange_rf_tech_and_mode = *data++;
> > > +	ntf.data_exchange_tx_bit_rate = *data++;
> > > +	ntf.data_exchange_rx_bit_rate = *data++;
> > > +	ntf.activation_params_len = *data++;
> > > +
> > > +	nfc_dbg("data_exchange_rf_tech_and_mode 0x%x",
> > > +		ntf.data_exchange_rf_tech_and_mode);
> > > +	nfc_dbg("data_exchange_tx_bit_rate 0x%x",
> > > +		ntf.data_exchange_tx_bit_rate);
> > > +	nfc_dbg("data_exchange_rx_bit_rate 0x%x",
> > > +		ntf.data_exchange_rx_bit_rate);
> > > +	nfc_dbg("activation_params_len %d",
> > > +		ntf.activation_params_len);
> > > +
> > > +	if (ntf.activation_params_len > 0) {
> > 
> > Seems to me that ntf.activation_params_len > 0 can be a check 
> > in the beginning
> > of this function.
> Actually, each param is read in order (as you can see the pointer data is advanced on each param read).
> The order is important, since it's clearer and some parameters are depend on the previous ones (e.g. we must read activation_rf_tech_and_mode before we read the activation params).
> So, activation_params_len is read when we arrive to its location in the data stream.
> In any case, I don't see the benefit in checking the condition in the beginning of the function (since I still need to read the other params).

Sure, I didn't realize this in my quick look to this code. But this remind me
another thing. Why are you reading your data like this, each member at a time?
Can't you do something like this:

 	struct ntf *ntf = skb->data;

Then if you need to copy them to another places, or do memcpy or pick the ones
you want by assign them to other variables.

	Gustavo