Re: [PATCH v2 3/3] Bluetooth: hci_qca: Add serdev support

2018-03-13 Thread Bjorn Andersson 
On Tue 13 Mar 09:44 PDT 2018, Thierry Escande wrote:
> @@ -50,6 +54,9 @@
>  #define IBS_TX_IDLE_TIMEOUT_MS   2000
>  #define BAUDRATE_SETTLE_TIMEOUT_MS   300
>  
> +/* divclk4 rate */

The clock is called "susclk" in the BT chip, "divclk4" is the board
specific name for the clock source.

> +#define DIVCLK4_RATE_32KHZ   32768
> +
>  /* HCI_IBS transmit side sleep protocol states */
>  enum tx_ibs_states {
>   HCI_IBS_TX_ASLEEP,
> @@ -111,6 +118,12 @@ struct qca_data {
>   u64 votes_off;
>  };
>  
> +struct qca_serdev {
> + struct hci_uart  serdev_hu;
> + struct gpio_desc *bt_en;
> + struct clk   *divclk4;

Rename this to "susclk", or simply "clk".

> +};

Apart from this and Andy's comments this looks good.

Regards,
Bjorn

Re: [PATCH v2 3/3] Bluetooth: hci_qca: Add serdev support

2018-03-13 Thread Bjorn Andersson 
On Tue 13 Mar 09:44 PDT 2018, Thierry Escande wrote:
> @@ -50,6 +54,9 @@
>  #define IBS_TX_IDLE_TIMEOUT_MS   2000
>  #define BAUDRATE_SETTLE_TIMEOUT_MS   300
>  
> +/* divclk4 rate */

The clock is called "susclk" in the BT chip, "divclk4" is the board
specific name for the clock source.

> +#define DIVCLK4_RATE_32KHZ   32768
> +
>  /* HCI_IBS transmit side sleep protocol states */
>  enum tx_ibs_states {
>   HCI_IBS_TX_ASLEEP,
> @@ -111,6 +118,12 @@ struct qca_data {
>   u64 votes_off;
>  };
>  
> +struct qca_serdev {
> + struct hci_uart  serdev_hu;
> + struct gpio_desc *bt_en;
> + struct clk   *divclk4;

Rename this to "susclk", or simply "clk".

> +};

Apart from this and Andy's comments this looks good.

Regards,
Bjorn

Re: [PATCH v2 3/3] Bluetooth: hci_qca: Add serdev support

2018-03-13 Thread Andy Shevchenko 
On Tue, Mar 13, 2018 at 6:44 PM, Thierry Escande
 wrote:
> Add support for Qualcomm serial slave devices. Probe the serial device,
> retrieve its maximum speed and register a new hci uart device.

> +#include 

What exactly this is used for?

> +   qcadev->bt_en = devm_gpiod_get(>dev, "bt-disable-n",
> +  GPIOD_OUT_LOW);
> +   if (IS_ERR(qcadev->bt_en)) {
> +   dev_err(>dev, "failed to acquire bt-disable-n 
> gpio\n");
> +   return PTR_ERR(qcadev->bt_en);
> +   }

> +   gpiod_set_value(qcadev->bt_en, 0);

Redundant.

> +   clk_set_rate(qcadev->divclk4, DIVCLK4_RATE_32KHZ);

> +   clk_prepare_enable(qcadev->divclk4);

This may fail.

> +   return hci_uart_register_device(>serdev_hu, _proto);
> +}

> +   clk_disable(qcadev->divclk4);
> +   clk_unprepare(qcadev->divclk4);

One call.

> +}

> +static const struct of_device_id qca_bluetooth_of_match[] = {
> +   { .compatible = "qcom,qca6174-bt" },

> +   { },

No comma.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] Bluetooth: hci_qca: Add serdev support

2018-03-13 Thread Andy Shevchenko 
On Tue, Mar 13, 2018 at 6:44 PM, Thierry Escande
 wrote:
> Add support for Qualcomm serial slave devices. Probe the serial device,
> retrieve its maximum speed and register a new hci uart device.

> +#include 

What exactly this is used for?

> +   qcadev->bt_en = devm_gpiod_get(>dev, "bt-disable-n",
> +  GPIOD_OUT_LOW);
> +   if (IS_ERR(qcadev->bt_en)) {
> +   dev_err(>dev, "failed to acquire bt-disable-n 
> gpio\n");
> +   return PTR_ERR(qcadev->bt_en);
> +   }

> +   gpiod_set_value(qcadev->bt_en, 0);

Redundant.

> +   clk_set_rate(qcadev->divclk4, DIVCLK4_RATE_32KHZ);

> +   clk_prepare_enable(qcadev->divclk4);

This may fail.

> +   return hci_uart_register_device(>serdev_hu, _proto);
> +}

> +   clk_disable(qcadev->divclk4);
> +   clk_unprepare(qcadev->divclk4);

One call.

> +}

> +static const struct of_device_id qca_bluetooth_of_match[] = {
> +   { .compatible = "qcom,qca6174-bt" },

> +   { },

No comma.

> +};

-- 
With Best Regards,
Andy Shevchenko