On Tue, Dec 1, 2020 at 11:48 AM Jakub Kicinski <k...@kernel.org> wrote:
>
> On Fri, 27 Nov 2020 22:36:31 +0900 bongsu.je...@gmail.com wrote:
> > From: Bongsu Jeon <bongsu.j...@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.j...@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;
> > +
Thank you for your advice.
I will send a new version.
