Re: [PATCH v4 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

[Balakrishna Godavarthi <bgodavar@codeaurora.org>]

Hi Johan,

On 2018-12-18 14:43, Johan Hovold wrote:
> On Mon, Dec 17, 2018 at 07:43:26PM +0530, Balakrishna Godavarthi wrote:
>> wcn3990 requires a power pulse to turn ON/OFF along with
>> regulators. Sometimes we are observing the power pulses are sent
>> out with some time delay, due to queuing these commands. This is
>> causing synchronization issues with chip, which intern delay the
>> chip setup or may end up with communication issues.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> v4:
>>  * used serdev_device_write_buf() instead of serdev_device_write()
>> 
>> v3:
>>   * no change.
>> v2:
>>   * Updated function qca_send_power_pulse()
>>   * addressed reviewer comments.
>> 
>> v1:
>>  * initial patch
>> 
>> ---
>>  drivers/bluetooth/hci_qca.c | 37 
>> +++++++++++++------------------------
>>  1 file changed, 13 insertions(+), 24 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index f036c8f98ea3..d8bc77c8c9b9 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct 
>> hci_uart *hu, unsigned int speed)
>>  		hci_uart_set_baudrate(hu, speed);
>>  }
>> 
>> -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>> +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
>>  {
>> -	struct hci_uart *hu = hci_get_drvdata(hdev);
>> -	struct qca_data *qca = hu->priv;
>> -	struct sk_buff *skb;
>> +	int ret;
>> 
>>  	/* These power pulses are single byte command which are sent
>>  	 * at required baudrate to wcn3990. On wcn3990, we have an external
>> @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct hci_dev 
>> *hdev, u8 cmd)
>>  	 * save power. Disabling hardware flow control is mandatory while
>>  	 * sending power pulses to SoC.
>>  	 */
>> -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
>> -
>> -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
>> -	if (!skb)
>> -		return -ENOMEM;
>> -
>> +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
>>  	hci_uart_set_flow_control(hu, true);
>> +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
>> +	if (ret < 0) {
>> +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
>> +			   cmd);
>> +		return ret;
>> +	}
> 
> As I mentioned earlier, serdev_device_write_buf() can buffer less than
> sizeof(cmd) bytes if the tty driver's write buffer is full (and return
> the number of bytes buffered).
> 
> How you want to deal with that is up to you and the bluetooth
> maintainers, but I think you want to at least log it even if you choose
> to ignore it.
> 
> Johan

[Bala]: thanks for reminding me for buffer size.
         we use the qca_send_power_pulse() where we use 
serdev_write_buf() to send
         a single byte commands to the chip during power ON or power OFF 
i.e. as soon as we open port or before close.

during power on:
         ideally open port can guarantee me to the have tty buffer empty 
as we didn't queued any data.
         let us assume that if the buffer is full(), 
serdev_device_write_buf() will return -1 (as  buffer is full)
         anyways i have check for return status to log the write_buf 
failure.

during power off:
         yes here we may face an issue as already we have some data 
queued in the buffer. i.e. due to previous transactions.
         I think we can achieve this by calling 
serdev_device_write_flush() and then calling serdev_device_write_buf().

-- 
Regards
Balakrishna.