Re: [PATCH v1] Bluetooth: btintel_pcie: Simplify MAC access request/release

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Kiran,


Thank yuo for your patch.

Am 11.05.26 um 15:39 schrieb Kiran K:
> Drop the STOP_MAC_ACCESS_DIS and XTAL_CLK_REQ bit manipulations from
> btintel_pcie_get_mac_access() and btintel_pcie_release_mac_access().
> These bits are no longer required to be toggled by the host driver for
> MAC access on the supported parts; the controller manages them
> internally.

Does the “no longer” refer to some controller firmware update? Please 
elaborate. (I personally prefer that as much as possible is done outside 
the firmware.) Also, maybe be explicit, and mention, if having the code 
has any practical downsides.

> Also fix the idempotency check in btintel_pcie_get_mac_access(): only
> assert MAC_ACCESS_REQ if it is not already set, instead of keying the
> read-modify-write off the MAC_ACCESS_STS bit (which reflects grant,
> not request, state).

*Also* is a good indicator to make something a separate commit. Any 
reason to not do it?

> Remove the now-unused STOP_MAC_ACCESS_DIS and XTAL_CLK_REQ register
> defines from btintel_pcie.h.
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>

No Fixes: tag? At least the second issue sounds like something to backport.

> ---
>   drivers/bluetooth/btintel_pcie.c | 10 +---------
>   drivers/bluetooth/btintel_pcie.h |  3 ---
>   2 files changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index fda474406003..53a4ad80b871 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -594,9 +594,7 @@ static int btintel_pcie_get_mac_access(struct btintel_pcie_data *data)
>   
>   	reg = btintel_pcie_rd_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
>   
> -	reg |= BTINTEL_PCIE_CSR_FUNC_CTRL_STOP_MAC_ACCESS_DIS;
> -	reg |= BTINTEL_PCIE_CSR_FUNC_CTRL_XTAL_CLK_REQ;
> -	if ((reg & BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_STS) == 0)
> +	if (!(reg & BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_REQ))
>   		reg |= BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_REQ;
>   
>   	btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG, reg);
> @@ -622,12 +620,6 @@ static void btintel_pcie_release_mac_access(struct btintel_pcie_data *data)
>   	if (reg & BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_REQ)
>   		reg &= ~BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_REQ;
>   
> -	if (reg & BTINTEL_PCIE_CSR_FUNC_CTRL_STOP_MAC_ACCESS_DIS)
> -		reg &= ~BTINTEL_PCIE_CSR_FUNC_CTRL_STOP_MAC_ACCESS_DIS;
> -
> -	if (reg & BTINTEL_PCIE_CSR_FUNC_CTRL_XTAL_CLK_REQ)
> -		reg &= ~BTINTEL_PCIE_CSR_FUNC_CTRL_XTAL_CLK_REQ;
> -
>   	btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG, reg);
>   }
>   
> diff --git a/drivers/bluetooth/btintel_pcie.h b/drivers/bluetooth/btintel_pcie.h
> index 2db85f71b2f8..7fc8c46ed689 100644
> --- a/drivers/bluetooth/btintel_pcie.h
> +++ b/drivers/bluetooth/btintel_pcie.h
> @@ -34,9 +34,6 @@
>   #define BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_STS	(BIT(20))
>   
>   #define BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_REQ	(BIT(21))
> -/* Stop MAC Access disconnection request */
> -#define BTINTEL_PCIE_CSR_FUNC_CTRL_STOP_MAC_ACCESS_DIS	(BIT(22))
> -#define BTINTEL_PCIE_CSR_FUNC_CTRL_XTAL_CLK_REQ		(BIT(23))
>   
>   #define BTINTEL_PCIE_CSR_FUNC_CTRL_BUS_MASTER_STS	(BIT(28))
>   #define BTINTEL_PCIE_CSR_FUNC_CTRL_BUS_MASTER_DISCON	(BIT(29))

The diff looks good.


Kind regards,

Paul