Hi Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Sent: Saturday, February 4, 2023 2:38 AM
> To: K, Kiran <kiran.k@xxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Srivatsa, Ravishankar
> <ravishankar.srivatsa@xxxxxxxxx>; Singh, Lokendra
> <lokendra.singh@xxxxxxxxx>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@xxxxxxxxx>; Sreemantha, Seema
> <seema.sreemantha@xxxxxxxxx>
> Subject: Re: [PATCH v1] Bluetooth: btintel: Set Per Platform Antenna
> Gain(PPAG)
>
> Hi Kiran, Seema,
>
> On Fri, Feb 3, 2023 at 1:28 AM Kiran K <kiran.k@xxxxxxxxx> wrote:
> >
> > From: Seema Sreemantha <seema.sreemantha@xxxxxxxxx>
> >
> > Antenna gain is defined as the antenna’s ability to increase the Tx
> > power in a given direction. Intel is certifying its products with
> > fixed reference antenna peak gain values (3/5dBi). The feature takes
> > into account the actual antenna gain, and increases output power
> > values, which results in a performance improvement.
> >
> > After firmware download is completed, driver reads from ACPI table and
> > configures PPAG as required. ACPI table entry for PPAG is defined as
> > below.
> >
> > Name (PPAG, Package (0x02)
> > {
> > 0x00000001,
> > Package (0x02)
> > {
> > 0x00000012, /* Bluetooth Domain */
> > 0x00000001 /* 1 - Enable PPAG, 0 - Disable PPAG */
> > }
> > })
> >
> > btmon log:
> >
> > < HCI Command: Vendor (0x3f|0x0219) plen 12
> > 00 00 00 00 00 00 00 00 00 00 00 00
> > > HCI Event: Command Complete (0x0e) plen 4
> > Vendor (0x3f|0x0219) ncmd 1
> > Status: Success (0x00)
>
> It would have been better if you guys had added a decoder for it.
Ack. I will publish the patch for the same.
>
> > Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
> > Signed-off-by: Seema Sreemantha <seema.sreemantha@xxxxxxxxx>
> > ---
> > drivers/bluetooth/btintel.c | 114
> > ++++++++++++++++++++++++++++++++++++
> > drivers/bluetooth/btintel.h | 13 ++++
> > 2 files changed, 127 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index d4e2cb9a4eb4..4d6d0dc10caa 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -9,6 +9,7 @@
> > #include <linux/module.h>
> > #include <linux/firmware.h>
> > #include <linux/regmap.h>
> > +#include <linux/acpi.h>
> > #include <asm/unaligned.h>
> >
> > #include <net/bluetooth/bluetooth.h>
> > @@ -24,6 +25,9 @@
> > #define ECDSA_OFFSET 644
> > #define ECDSA_HEADER_LEN 320
> >
> > +#define BTINTEL_PPAG_NAME "PPAG"
> > +#define BTINTEL_PPAG_PREFIX "\\_SB_.PCI0.XHCI.RHUB"
> > +
> > #define CMD_WRITE_BOOT_PARAMS 0xfc0e struct
> cmd_write_boot_params {
> > __le32 boot_addr;
> > @@ -1278,6 +1282,63 @@ static int btintel_read_debug_features(struct
> hci_dev *hdev,
> > return 0;
> > }
> >
> > +static acpi_status btintel_ppag_callback(acpi_handle handle, u32 lvl, void
> *data,
> > + void **ret) {
> > + acpi_status status;
> > + size_t len;
> > + struct btintel_ppag *ppag;
>
> You can probably just assign ppag = data above;
>
> > + union acpi_object *p, *elements;
> > + struct acpi_buffer string = {ACPI_ALLOCATE_BUFFER, NULL};
> > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > + struct hci_dev *hdev = ((struct btintel_ppag *)data)->hdev;
>
> Then use hdev = ppga->hdev
>
Ack.
> > +
> > + status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
> > + if (ACPI_FAILURE(status)) {
> > + bt_dev_warn(hdev, "ACPI Failure: %s",
> acpi_format_exception(status));
> > + return status;
> > + }
> > +
> > + if (strncmp(BTINTEL_PPAG_PREFIX, string.pointer,
> > + strlen(BTINTEL_PPAG_PREFIX))) {
> > + kfree(string.pointer);
> > + return AE_OK;
> > + }
> > +
> > + len = strlen(string.pointer);
> > + if (strncmp((char *)string.pointer + len - 4, BTINTEL_PPAG_NAME, 4)) {
> > + kfree(string.pointer);
> > + return AE_OK;
> > + }
> > + kfree(string.pointer);
> > +
> > + status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
> > + if (ACPI_FAILURE(status)) {
> > + bt_dev_warn(hdev, "ACPI Failure: %s",
> acpi_format_exception(status));
> > + return status;
> > + }
> > +
> > + p = buffer.pointer;
> > + ppag = (struct btintel_ppag *)data;
> > +
> > + if (p->type != ACPI_TYPE_PACKAGE || p->package.count != 2) {
> > + kfree(buffer.pointer);
> > + bt_dev_warn(hdev, "Invalid object type: %d or package count:
> %d",
> > + p->type, p->package.count);
> > + return AE_ERROR;
> > + }
> > +
> > + elements = p->package.elements;
> > +
> > + /* PPAG table is located at element[1] */
> > + p = &elements[1];
> > +
> > + ppag->domain = (u32)p->package.elements[0].integer.value;
> > + ppag->mode = (u32)p->package.elements[1].integer.value;
> > + kfree(buffer.pointer);
> > + return AE_CTRL_TERMINATE;
> > +}
> > +
> > static int btintel_set_debug_features(struct hci_dev *hdev,
> > const struct intel_debug_features
> > *features) { @@ -2251,6 +2312,56 @@ static int
> > btintel_configure_offload(struct hci_dev *hdev)
> > return err;
> > }
> >
> > +static int btintel_set_ppag(struct hci_dev *hdev, struct
> > +intel_version_tlv *ver) {
> > + acpi_status status;
> > + struct btintel_ppag ppag;
> > + struct sk_buff *skb;
> > + struct btintel_loc_aware_reg ppag_cmd;
> > +
> > + /* PPAG is not supported if CRF is HrP2, Jfp2, JfP1 */
> > + switch (ver->cnvr_top & 0xFFF) {
> > + case 0x504: /* Hrp2 */
> > + case 0x202: /* Jfp2 */
> > + case 0x201: /* Jfp1 */
> > + return 0;
> > + }
> > +
> > + memset(&ppag, 0, sizeof(ppag));
> > +
> > + ppag.hdev = hdev;
> > + status = acpi_walk_namespace(ACPI_TYPE_ANY, ACPI_ROOT_OBJECT,
> > + ACPI_UINT32_MAX, NULL,
> > + btintel_ppag_callback, &ppag,
> > + NULL);
> > +
> > + if (ACPI_FAILURE(status)) {
> > + bt_dev_warn(hdev, "PPAG: ACPI Failure: %s",
> > + acpi_format_exception(status));
>
> Shouldn't we consider it ok if the ACPI doesn't have an entry? I mean it
> should be possible to plug a new controller in an old laptop that possibly
> doesn't have this entry, right?
>
Ack. Older platform won't support this feature.
> > + return -1;
> > + }
> > +
> > + if (ppag.domain != 0x12) {
> > + bt_dev_warn(hdev, "PPAG-BT Domain disabled");
> > + return -1;
> > + }
> > +
> > + /* PPAG mode, BIT0 = 0 Disabled, BIT0 = 1 Enabled */
> > + if (!(ppag.mode & BIT(0))) {
> > + bt_dev_dbg(hdev, "PPAG disabled");
> > + return 0;
> > + }
> > +
> > + ppag_cmd.mcc = 0;
> > + ppag_cmd.sel = 0; /* 0 - Operational, 1 - Disable, 2 - Testing mode */
> > + ppag_cmd.delta = 0;
> > + skb = __hci_cmd_sync(hdev, 0xfe19, 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 PTR_ERR(skb);
> > + }
> > + kfree_skb(skb);
> > + return 0;
> > +}
> > +
> > static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
> > struct intel_version_tlv *ver)
> > { @@ -2297,6 +2408,9 @@ static int btintel_bootloader_setup_tlv(struct
> > hci_dev *hdev,
> >
> > hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);
> >
> > + /* Set PPAG feature */
> > + btintel_set_ppag(hdev, ver);
>
> Looks like you are not using the return above which I guess is fine since this
> step can be considered optional but I'd document it as such and probably
> make btintel_set_ppag return void as it shouldn't be considered as an error if
> it fails.
Ack.
>
> > +
....
Thanks,
Kiran
