Messages in this thread Patch in this message | | | Date | Mon, 30 Nov 2020 18:48:45 -0800 | From | Jakub Kicinski <> | Subject | Re: [PATCH net-next v3] net/nfc/nci: Support NCI 2.x initial sequence |
| |
On Fri, 27 Nov 2020 22:36:31 +0900 bongsu.jeon2@gmail.com wrote: > From: Bongsu Jeon <bongsu.jeon@samsung.com> > > implement the NCI 2.x initial sequence to support NCI 2.x NFCC. > Since NCI 2.0, CORE_RESET and CORE_INIT sequence have been changed. > If NFCEE supports NCI 2.x, then NCI 2.x initial sequence will work. > > In NCI 1.0, Initial sequence and payloads are as below: > (DH) (NFCC) > | -- CORE_RESET_CMD --> | > | <-- CORE_RESET_RSP -- | > | -- CORE_INIT_CMD --> | > | <-- CORE_INIT_RSP -- | > CORE_RESET_RSP payloads are Status, NCI version, Configuration Status. > CORE_INIT_CMD payloads are empty. > CORE_INIT_RSP payloads are Status, NFCC Features, > Number of Supported RF Interfaces, Supported RF Interface, > Max Logical Connections, Max Routing table Size, > Max Control Packet Payload Size, Max Size for Large Parameters, > Manufacturer ID, Manufacturer Specific Information. > > In NCI 2.0, Initial Sequence and Parameters are as below: > (DH) (NFCC) > | -- CORE_RESET_CMD --> | > | <-- CORE_RESET_RSP -- | > | <-- CORE_RESET_NTF -- | > | -- CORE_INIT_CMD --> | > | <-- CORE_INIT_RSP -- | > CORE_RESET_RSP payloads are Status. > CORE_RESET_NTF payloads are Reset Trigger, > Configuration Status, NCI Version, Manufacturer ID, > Manufacturer Specific Information Length, > Manufacturer Specific Information. > CORE_INIT_CMD payloads are Feature1, Feature2. > CORE_INIT_RSP payloads are Status, NFCC Features, > Max Logical Connections, Max Routing Table Size, > Max Control Packet Payload Size, > Max Data Packet Payload Size of the Static HCI Connection, > Number of Credits of the Static HCI Connection, > Max NFC-V RF Frame Size, Number of Supported RF Interfaces, > Supported RF Interfaces. > > Signed-off-by: Bongsu Jeon <bongsu.jeon@samsung.com>
> static void nci_init_req(struct nci_dev *ndev, unsigned long opt) > { > - nci_send_cmd(ndev, NCI_OP_CORE_INIT_CMD, 0, NULL); > + struct nci_core_init_v2_cmd *cmd = (struct nci_core_init_v2_cmd *)opt; > + > + if (!cmd) > + nci_send_cmd(ndev, NCI_OP_CORE_INIT_CMD, 0, NULL); > + else > + /* if nci version is 2.0, then use the feature parameters */ > + nci_send_cmd(ndev, NCI_OP_CORE_INIT_CMD, > + sizeof(struct nci_core_init_v2_cmd), cmd);
This would be better written as:
u8 plen = 0;
if (opt) plen = sizeof(struct nci_core_init_v2_cmd); nci_send_cmd(ndev, NCI_OP_CORE_INIT_CMD, plen, (void *)opt);
> +
unnecessary empty line
> } > > static void nci_init_complete_req(struct nci_dev *ndev, unsigned long opt) > @@ -497,8 +505,18 @@ static int nci_open_device(struct nci_dev *ndev) > } > > if (!rc) { > - rc = __nci_request(ndev, nci_init_req, 0, > - msecs_to_jiffies(NCI_INIT_TIMEOUT)); > + if (!(ndev->nci_ver & NCI_VER_2_MASK)) { > + rc = __nci_request(ndev, nci_init_req, 0, > + msecs_to_jiffies(NCI_INIT_TIMEOUT)); > + } else { > + struct nci_core_init_v2_cmd nci_init_v2_cmd; > + > + nci_init_v2_cmd.feature1 = NCI_FEATURE_DISABLE; > + nci_init_v2_cmd.feature2 = NCI_FEATURE_DISABLE; > + > + rc = __nci_request(ndev, nci_init_req, (unsigned long)&nci_init_v2_cmd, > + msecs_to_jiffies(NCI_INIT_TIMEOUT)); > + }
again please try to pull out the common code:
struct nci_core_init_v2_cmd nci_init_v2_cmd = { .feature1 = NCI_FEATURE_DISABLE; .feature2 = NCI_FEATURE_DISABLE; }; unsigned long opt = 0; if (ndev->nci_ver & NCI_VER_2_MASK) opt = (unsigned long)&nci_init_v2_cmd;
rc = __nci_request(ndev, nci_init_req, opt, msecs_to_jiffies(NCI_INIT_TIMEOUT));
> }
> -static void nci_core_init_rsp_packet(struct nci_dev *ndev, struct sk_buff *skb) > +static unsigned char nci_core_init_rsp_packet_v1(struct nci_dev *ndev, struct sk_buff *skb) > { > struct nci_core_init_rsp_1 *rsp_1 = (void *) skb->data; > struct nci_core_init_rsp_2 *rsp_2; > @@ -48,16 +51,14 @@ static void nci_core_init_rsp_packet(struct nci_dev *ndev, struct sk_buff *skb) > pr_debug("status 0x%x\n", rsp_1->status); > > if (rsp_1->status != NCI_STATUS_OK) > - goto exit; > + return rsp_1->status; > > ndev->nfcc_features = __le32_to_cpu(rsp_1->nfcc_features); > ndev->num_supported_rf_interfaces = rsp_1->num_supported_rf_interfaces; > > - if (ndev->num_supported_rf_interfaces > > - NCI_MAX_SUPPORTED_RF_INTERFACES) { > - ndev->num_supported_rf_interfaces = > - NCI_MAX_SUPPORTED_RF_INTERFACES; > - } > + ndev->num_supported_rf_interfaces = > + min((int)ndev->num_supported_rf_interfaces, > + NCI_MAX_SUPPORTED_RF_INTERFACES); > > memcpy(ndev->supported_rf_interfaces, > rsp_1->supported_rf_interfaces, > @@ -77,6 +78,58 @@ static void nci_core_init_rsp_packet(struct nci_dev *ndev, struct sk_buff *skb) > ndev->manufact_specific_info = > __le32_to_cpu(rsp_2->manufact_specific_info); > > + return NCI_STATUS_OK; > +} > + > +static unsigned char nci_core_init_rsp_packet_v2(struct nci_dev *ndev, struct sk_buff *skb) > +{ > + struct nci_core_init_rsp_nci_ver2 *rsp = (void *)skb->data; > + unsigned char rf_interface_idx = 0;
Prefer the use of u8 type in the kernel
> + unsigned char rf_extension_cnt = 0; > + unsigned char *supported_rf_interface = rsp->supported_rf_interfaces;
Please order the variable declarations longest to shortest. Don't initialize them inline if that'd cause the order to break.
> + pr_debug("status %x\n", rsp->status); > + > + if (rsp->status != NCI_STATUS_OK) > + return rsp->status; > +
| |