Re: [PATCH v7 5/8] Bluetooth: hci_qca: Defined wrapper functions for setting UART speeds

[Matthias Kaehlcke <mka@chromium.org>]

On Sat, Jun 16, 2018 at 11:57:15AM +0530, Balakrishna Godavarthi wrote:
> Subject: Bluetooth: hci_qca: Defined wrapper functions for setting
>          UART speeds

s/Defined/Define/

or

s/Defined/Add/

>
> In qca_setup, we set initial and operating speeds for Qualcomm
> Bluetooth SoC's. This block of code is common across different
> Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
> a wrapper function to set the speeds. So that future coming SoC's
> can use these wrapper functions to set speeds.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> 
> Changes in v7:
> 	* initial patch
> 	* created wrapper functions for init and operating speeds.
> 
> ---
>  drivers/bluetooth/hci_qca.c | 64 ++++++++++++++++++++++++++-----------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index fe62420ef838..3e09c6223baf 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -923,21 +923,10 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
>  		hci_uart_set_baudrate(hu, speed);
>  }
>  
> -static int qca_setup(struct hci_uart *hu)
> +static void qca_set_init_speed(struct hci_uart *hu)
>  {
> -	struct hci_dev *hdev = hu->hdev;
> -	struct qca_data *qca = hu->priv;
> -	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
> -	int ret;
> -	int soc_ver = 0;
> -
> -	bt_dev_info(hdev, "ROME setup");
> -
> -	/* Patch downloading has to be done without IBS mode */
> -	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> +	unsigned int speed = 0;
>  
> -	/* Setup initial baudrate */
> -	speed = 0;
>  	if (hu->init_speed)
>  		speed = hu->init_speed;
>  	else if (hu->proto->init_speed)
> @@ -945,27 +934,64 @@ static int qca_setup(struct hci_uart *hu)
>  
>  	if (speed)
>  		host_set_baudrate(hu, speed);
> +}
> +
> +static int qca_get_oper_speed(struct hci_uart *hu)

Return type should be unsigned int.

> +{
> +	unsigned int speed = 0;
>  
> -	/* Setup user speed if needed */
> -	speed = 0;
>  	if (hu->oper_speed)
>  		speed = hu->oper_speed;
>  	else if (hu->proto->oper_speed)
>  		speed = hu->proto->oper_speed;
>  
> +	return speed;
> +}
> +
> +static int qca_set_oper_speed(struct hci_uart *hu)
> +{
> +	unsigned int speed = 0, qca_baudrate;

initialization is not needed.

> +	int ret = 0;
> +
> +	speed = qca_get_oper_speed(hu);
>  	if (speed) {

nit:
	if (!speed)
		return 0;


>  		qca_baudrate = qca_get_baudrate_value(speed);
> -
> -		bt_dev_info(hdev, "Set UART speed to %d", speed);
> -		ret = qca_set_baudrate(hdev, qca_baudrate);
> +		bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
> +		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>  		if (ret) {
> -			bt_dev_err(hdev, "Failed to change the baud rate (%d)",
> +			bt_dev_err(hu->hdev, "Failed to change the baud rate (%d)",
>  				   ret);
>  			return ret;
>  		}
>  		host_set_baudrate(hu, speed);
>  	}
>  
> +	return ret;
> +}
> +
> +static int qca_setup(struct hci_uart *hu)
> +{
> +	struct hci_dev *hdev = hu->hdev;
> +	struct qca_data *qca = hu->priv;
> +	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
> +	int ret;
> +	int soc_ver = 0;
> +
> +	bt_dev_info(hdev, "ROME setup");
> +
> +	/* Patch downloading has to be done without IBS mode */
> +	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> +
> +	/* Setup initial baudrate */
> +	qca_set_init_speed(hu);
> +	/* Setup user speed if needed */
> +	ret = qca_set_oper_speed(hu);
> +	if (ret)
> +		return ret;
> +	speed = qca_get_oper_speed(hu);
> +	if (speed)
> +		qca_baudrate = qca_get_baudrate_value(speed);

nit: the sequence

qca_set_oper_speed()
qca_get_oper_speed()

is slightly awkward to read. This could be avoided if the speed was
passed as parameter, though I understand this isn't done for symmetry
with qca_set_init_speed(). An alternative could be:

static int qca_set_speed(struct hci_uart *hu, unsigned int speed, enum
    qca_speed_type speed_type)
{
	unsigned int qca_baudrate;

	if (speed_type == QCA_OPER_SPEED) {
		qca_baudrate = qca_get_baudrate_value(speed);
		bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
		if (ret) {
			bt_dev_err(hu->hdev, "Failed to change the baud rate (%d)",
				ret);
			return ret;
		}
	}

	host_set_baudrate(hu, speed);

	/* If the order doesn't matter set the host baudrate first and
	   return if speed_type != QCA_OPER_SPEED */

	return 0;
}

static int qca_setup(struct hci_uart *hu)
{
	...
	speed = qca_get_init_speed(hu);
	if (speed)
		qca_set_speed(hu, speed, QCA_INIT_SPEED);

	speed = qca_get_oper_speed(hu);
	if (speed) {
	   	qca_set_speed(hu, speed, QCA_OPER_SPEED);
		qca_baudrate = qca_get_baudrate_value(speed);
	}
	...
}

Just a suggestion, ok for me if you prefer to keep it as is.