Hi Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Sent: Thursday, 10 August, 2023 11:20 PM
> To: Singh, Lokendra <lokendra.singh@xxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Srivatsa, Ravishankar
> <ravishankar.srivatsa@xxxxxxxxx>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@xxxxxxxxx>; Von Dentz, Luiz
> <luiz.von.dentz@xxxxxxxxx>; An, Tedd <tedd.an@xxxxxxxxx>; K, Kiran
> <kiran.k@xxxxxxxxx>; Seema Sreemantha <seema.sreemantha@xxxxxxxxx>
> Subject: Re: [PATCH v2] Bluetooth: btintel: Send new command for PPAG
>
> Hi Lokendra,
>
> On Thu, Aug 10, 2023 at 8:18 AM Lokendra Singh
> <lokendra.singh@xxxxxxxxx> wrote:
> >
> > Added support for the new command opcode FE0B (HCI Intel PPAG Enable).
> >
> > btmon log:
> > < HCI Command: Intel PPAG Enable (0x3f|0x020b) plen 4
> > Enable: 0x00000002
> > > HCI Event: Command Complete (0x0e) plen 4
> > Intel PPAG Enable (0x3f|0x020b) ncmd 1
> > Status: Success (0x00)
> >
> > Signed-off-by: Seema Sreemantha <seema.sreemantha@xxxxxxxxx>
> > Signed-off-by: Lokendra Singh <lokendra.singh@xxxxxxxxx>
> > ---
> > drivers/bluetooth/btintel.c | 28 +++++++++++++++++++---------
> > drivers/bluetooth/btintel.h | 8 +++-----
> > 2 files changed, 22 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index 633e8d9bf58f..d2c93b88c527 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -2401,7 +2401,7 @@ static void btintel_set_ppag(struct hci_dev
> > *hdev, struct intel_version_tlv *ver {
> > struct btintel_ppag ppag;
> > struct sk_buff *skb;
> > - struct btintel_loc_aware_reg ppag_cmd;
> > + struct hci_ppag_enable_cmd ppag_cmd;
> > acpi_handle handle;
> >
> > /* PPAG is not supported if CRF is HrP2, Jfp2, JfP1 */ @@
> > -2409,6 +2409,8 @@ static void btintel_set_ppag(struct hci_dev *hdev,
> struct intel_version_tlv *ver
> > case 0x504: /* Hrp2 */
> > case 0x202: /* Jfp2 */
> > case 0x201: /* Jfp1 */
> > + bt_dev_warn(hdev, "PPAG not supported for Intel CNVr
> (0x%3x)",
> > + ver->cnvr_top & 0xFFF);
>
> If this doesn't change the functionality I'd recommend using bt_dev_dbg
> so we don't warn users for no reason.
Ack. I will update it in next patch.
>
> > return;
> > }
> >
> > @@ -2434,24 +2436,32 @@ static void btintel_set_ppag(struct hci_dev
> *hdev, struct intel_version_tlv *ver
> > }
> >
> > if (ppag.domain != 0x12) {
> > - bt_dev_warn(hdev, "PPAG-BT: domain is not bluetooth");
> > + bt_dev_warn(hdev, "PPAG-BT: Bluetooth domain is
> > + disabled in ACPI firmware");
>
> Ditto.
Ack. I will update it in next patch.
>
> > return;
> > }
> >
> > - /* PPAG mode, BIT0 = 0 Disabled, BIT0 = 1 Enabled */
> > - if (!(ppag.mode & BIT(0))) {
> > - bt_dev_dbg(hdev, "PPAG-BT: disabled");
> > + /* PPAG mode
> > + * BIT 0 : 0 Disabled in EU
> > + * 1 Enabled in EU
> > + * BIT 1 : 0 Disabled in China
> > + * 1 Enabled in China
> > + */
> > + if ((ppag.mode & 0x01) != BIT(0) && (ppag.mode & 0x02) !=
> BIT(1)) {
> > + bt_dev_warn(hdev, "PPAG-BT: EU, China mode are
> > + disabled in CB/BIOS");
>
> Ditto.
Ack. I will update it in next patch.
>
> > return;
> > }
> >
> > - ppag_cmd.mcc = cpu_to_le32(0);
> > - ppag_cmd.sel = cpu_to_le32(0); /* 0 - Enable , 1 - Disable, 2
> - Testing mode */
> > - ppag_cmd.delta = cpu_to_le32(0);
> > - skb = __hci_cmd_sync(hdev, 0xfe19, sizeof(ppag_cmd),
> &ppag_cmd, HCI_CMD_TIMEOUT);
> > + /* HCI_Intel_PpagEnable_CMD - opcode: 0xFE0B
> > + * ppag_enable_flags - ppag mode
> > + */
> > + ppag_cmd.ppag_enable_flags = ppag.mode;
> > +
> > + skb = __hci_cmd_sync(hdev, 0xfe0b, sizeof(ppag_cmd),
> > + &ppag_cmd, HCI_CMD_TIMEOUT);
> > if (IS_ERR(skb)) {
> > bt_dev_warn(hdev, "Failed to send PPAG Enable (%ld)",
> PTR_ERR(skb));
> > return;
> > }
> > + bt_dev_info(hdev, "PPAG-BT: Enabled (Mode %d)", ppag.mode);
> > kfree_skb(skb);
> > }
> >
> > diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> > index 2ed646609dee..01e87f68fab0 100644
> > --- a/drivers/bluetooth/btintel.h
> > +++ b/drivers/bluetooth/btintel.h
> > @@ -137,11 +137,9 @@ struct intel_offload_use_cases {
> > __u8 preset[8];
> > } __packed;
> >
> > -struct btintel_loc_aware_reg {
> > - __le32 mcc;
> > - __le32 sel;
> > - __le32 delta;
> > -} __packed;
> > +struct hci_ppag_enable_cmd {
> > + u32 ppag_enable_flags;
> > +};
>
> Lets also add defines to command opcodes so we don't have to add
> comments on what the opcode means around the codebase.
Ack. I will update it in next patch.
>
> > #define INTEL_TLV_TYPE_ID 0x01
> >
> > --
> > 2.25.1
> >
>
>
> --
> Luiz Augusto von Dentz
