Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230810145653.1780-1-lokendra.singh@intel.com>
In-Reply-To: <20230810145653.1780-1-lokendra.singh@intel.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 10 Aug 2023 10:49:34 -0700
Message-ID: <CABBYNZJogALR3GwcR4CmsmjuhQaCytAHghgcaBLLeiu+QT+Zww@mail.gmail.com>
Subject: Re: [PATCH v2] Bluetooth: btintel: Send new command for PPAG
To:     Lokendra Singh <lokendra.singh@intel.com>
Cc:     linux-bluetooth@vger.kernel.org, ravishankar.srivatsa@intel.com,
        chethan.tumkur.narayan@intel.com, luiz.von.dentz@intel.com,
        tedd.an@intel.com, kiran.k@intel.com,
        Seema Sreemantha <seema.sreemantha@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Lokendra,

On Thu, Aug 10, 2023 at 8:18=E2=80=AFAM Lokendra Singh <lokendra.singh@inte=
l.com> 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@intel.com>
> Signed-off-by: Lokendra Singh <lokendra.singh@intel.com>
> ---
>  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.

>                 return;
>         }
>
> @@ -2434,24 +2436,32 @@ static void btintel_set_ppag(struct hci_dev *hdev=
, struct intel_version_tlv *ver
>         }
>
>         if (ppag.domain !=3D 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.

>                 return;
>         }
>
> -       /* PPAG mode, BIT0 =3D 0 Disabled, BIT0 =3D 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) !=3D BIT(0) && (ppag.mode & 0x02) !=3D BIT=
(1)) {
> +               bt_dev_warn(hdev, "PPAG-BT: EU, China mode are disabled i=
n CB/BIOS");

Ditto.

>                 return;
>         }
>
> -       ppag_cmd.mcc =3D cpu_to_le32(0);
> -       ppag_cmd.sel =3D cpu_to_le32(0); /* 0 - Enable , 1 - Disable, 2 -=
 Testing mode */
> -       ppag_cmd.delta =3D cpu_to_le32(0);
> -       skb =3D __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 =3D ppag.mode;
> +
> +       skb =3D __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.

>  #define INTEL_TLV_TYPE_ID              0x01
>
> --
> 2.25.1
>


--=20
Luiz Augusto von Dentz