Hi Bjorn,
Thanks for your comments.
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Thursday, March 28, 2024 10:39 PM
> To: K, Kiran <kiran.k@xxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Srivatsa, Ravishankar
> <ravishankar.srivatsa@xxxxxxxxx>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@xxxxxxxxx>; Devegowda, Chandrashekar
> <chandrashekar.devegowda@xxxxxxxxx>
> Subject: Re: [PATCH v1 3/3] Bluetooth: btintel_pcie: Add *setup* function to
> download firmware
>
> On Thu, Mar 28, 2024 at 04:49:04PM +0530, Kiran K wrote:
> > Add to support to download firmware.
>
> s/Add to/Add/
>
Ack.
> > +static void btintel_pcie_prepare_tx(struct txq *txq, u16 tfd_index,
> > + struct sk_buff *skb)
> > +{
> > + struct data_buf *buf;
> > + struct tfd *tfd;
> > +
> > + tfd = &txq->tfds[tfd_index];
> > + memset(tfd, 0, sizeof(*tfd));
> > +
> > + /* Get the buffer of the tfd index for DMA */
>
> s/tfd/TFD/ for consistency.
Ack.
>
> > +static int btintel_pcie_hci_send_frame(struct hci_dev *hdev,
> > + struct sk_buff *skb)
> > +{
> > + struct btintel_pcie_data *data = hci_get_drvdata(hdev);
> > + int ret;
> > + u32 type;
> > +
> > + /* Due to the fw limitation, the type header of the packet should be
> > + * 4 bytes unlikely 1 byte for UART. In UART, the firmware can reads
> > + * the first byte to get the packet type and redirect the rest of data
> > + * packet to the right handler. But for PCIe, THF(Transfer Flow Handler)
> > + * fetches the 4 bytes of data from DMA memory and by the time it
> reads
> > + * the first 4 bytes, it already consumes some part of packet. Thus
> > + * the packet type indicator for iBT PCIe is 4 bytes.
> > + * Luckily, when HCI core creates the skb, it allocated 8 bytes of
> > + * head room for profile and driver use, and before sending the data
> > + * to the device, append the iBT PCIe packet type in the front.
>
> s/unlikely/unlike/
> s/can reads/can read/
> s/it already consumes/it has already consumed/
>
> Add blank line between paragraphs.
Ack.
>
> > + */
> > + switch (hci_skb_pkt_type(skb)) {
> > + case HCI_COMMAND_PKT:
> > + type = BTINTEL_PCIE_HCI_CMD_PKT;
> > + if (btintel_test_flag(hdev, INTEL_BOOTLOADER)) {
> > + struct hci_command_hdr *cmd = (void *)skb->data;
> > + __u16 opcode = le16_to_cpu(cmd->opcode);
> > +
> > + /* When the 0xfc01 command is issued to boot into
> > + * the operational firmware, it will actually not
> > + * send a command complete event. To keep the flow
> > + * control working inject that event here.
> > + */
> > + if (opcode == 0xfc01)
> > + btintel_pcie_inject_cmd_complete(hdev,
> opcode);
> > + }
> > + hdev->stat.cmd_tx++;
> > + break;
> > + case HCI_ACLDATA_PKT:
> > + type = BTINTEL_PCIE_HCI_ACL_PKT;
> > + hdev->stat.acl_tx++;
> > + break;
> > + case HCI_SCODATA_PKT:
> > + type = BTINTEL_PCIE_HCI_SCO_PKT;
> > + hdev->stat.sco_tx++;
> > + break;
> > + default:
> > + bt_dev_err(hdev, "Unknown HCI packet type");
> > + ret = -EILSEQ;
> > + goto exit_error;
> > + }
> > + memcpy(skb_push(skb, BTINTEL_PCIE_HCI_TYPE_LEN), &type,
> > + BTINTEL_PCIE_HCI_TYPE_LEN);
> > +
> > + ret = btintel_pcie_send_sync(data, skb);
> > + if (ret) {
> > + hdev->stat.err_tx++;
> > + bt_dev_err(hdev, "Failed to send frame (%d)", ret);
> > + goto exit_error;
> > + } else {
> > + hdev->stat.byte_tx += skb->len;
> > + kfree_skb(skb);
> > + }
> > +
> > +exit_error:
> > +
> > + return ret;
>
> There's no cleanup here, so it would be simpler to omit "ret"
> completely and return directly above instead of using the goto.
>
Ack.
> > +}
Thanks,
Kiran
