Re: [PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

[Matthias Kaehlcke]

On Tue, Jun 26, 2018 at 07:01:18AM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-06-26 05:13, Matthias Kaehlcke wrote:
> > This is a nice improvement, a few remaining questions inline.
> > 
> > On Mon, Jun 25, 2018 at 07:10:10PM +0530, Balakrishna Godavarthi wrote:
> > > In function 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@xxxxxxxxxxxxxx>
> > > ---
> > > Changes in v8:
> > >     * common function to set INIT and operating speeds.
> > >     * moved hardware flow control to qca_set_speed().
> > > 
> > > Changes in v7:
> > >     * initial patch
> > >     * created wrapper functions for init and operating speeds.
> > > ---
> > >  drivers/bluetooth/hci_qca.c | 89
> > > +++++++++++++++++++++++++++----------
> > >  1 file changed, 65 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index fe62420ef838..38b7dbe6c897 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > > @@ -119,6 +119,11 @@ struct qca_data {
> > >  	u64 votes_off;
> > >  };
> > > 
> > > +enum qca_speed_type {
> > > +	QCA_INIT_SPEED = 1,
> > > +	QCA_OPER_SPEED
> > > +};
> > > +
> > >  struct qca_serdev {
> > >  	struct hci_uart	 serdev_hu;
> > >  	struct gpio_desc *bt_en;
> > > @@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct
> > > hci_uart *hu, unsigned int speed)
> > >  		hci_uart_set_baudrate(hu, speed);
> > >  }
> > > 
> > > +static unsigned int qca_get_speed(struct hci_uart *hu,
> > > +				  enum qca_speed_type speed_type)
> > > +{
> > > +	unsigned int speed = 0;
> > > +
> > > +	if (speed_type == QCA_INIT_SPEED) {
> > > +		if (hu->init_speed)
> > > +			speed = hu->init_speed;
> > > +		else if (hu->proto->init_speed)
> > > +			speed = hu->proto->init_speed;
> > > +	} else {
> > > +		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_speed(struct hci_uart *hu, enum qca_speed_type
> > > speed_type)
> > > +{
> > > +	unsigned int speed, qca_baudrate;
> > > +	int ret;
> > > +
> > > +	if (speed_type == QCA_INIT_SPEED) {
> > > +		speed = qca_get_speed(hu, QCA_INIT_SPEED);
> > > +		if (speed)
> > > +			host_set_baudrate(hu, speed);
> > > +		else
> > > +			bt_dev_err(hu->hdev, "Init speed should be non zero");
> > 
> > The check for 'speed == 0' is done in multiple places. From this
> > code I deduce that it is expected that both INIT and OPER speed are
> > set to non-zero values. What happens if either of them is zero? Is the
> > driver still operational?
> > 
> [Bala]: yes in hci_uart_setup()(hci_serdev.c) function before calling
> qca_setup().
>         we actually set baudrate, but int  above code missed to restrict the
> further call to qca_setup()
>        if speed =0. so we are checking the same in the qca_setup().. i.e.
> qca_get_speed().

Sorry, didn't quite get you here. Yes, the driver is still operational?

Breaking it down in multiple questions:

1. Is it an error if one of the speeds isn't specified?

If yes we should probably check this early once and return an error
early, instead of doing the check repeatedly

2. If it is not an error, what is the driver supposed to do?

> > In the discussion on "[v7,8/8] Bluetooth: hci_qca: Add support for
> > Qualcomm Bluetooth chip wcn3990" you mentioned the possbility to move
> > the hci_uart_set_flow_control() calls into _set_speed(). This seemed
> > interesting but finally it isn't done in this series. Did you
> > encounter that it is not feasible/desirable for some reason?
> > 
> 
> [Bala]: this patch is for rome where flow control is not used.
>         after we integrate wcn3990, flow control is hidden in the
> qca_set_speed()
>         Pls check [v8 7/7] patch.

Sorry, my confusion

> > >  static int qca_setup(struct hci_uart *hu)
> > >  {
> > >  	struct hci_dev *hdev = hu->hdev;
> > > @@ -937,35 +996,17 @@ static int qca_setup(struct hci_uart *hu)
> > >  	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> > > 
> > >  	/* Setup initial baudrate */
> > > -	speed = 0;
> > > -	if (hu->init_speed)
> > > -		speed = hu->init_speed;
> > > -	else if (hu->proto->init_speed)
> > > -		speed = hu->proto->init_speed;
> > > -
> > > -	if (speed)
> > > -		host_set_baudrate(hu, speed);
> > > +	qca_set_speed(hu, QCA_INIT_SPEED);
> > > 
> > >  	/* 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;
> > > +	ret = qca_set_speed(hu, QCA_OPER_SPEED);
> > > +	if (ret)
> > > +		return ret;
> > > 
> > > -	if (speed) {
> > > +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
> > > +	if (speed)
> > >  		qca_baudrate = qca_get_baudrate_value(speed);
> > 
> > Is the check here necessary? qca_get_baudrate_value() returns
> > QCA_BAUDRATE_115200 for a zero speed.
> 
> this if for no zero operating speed,

My point is:

static int qca_setup(struct hci_uart *hu)
{
	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;

	...

	if (speed)
		qca_baudrate = qca_get_baudrate_value(speed);
}

static uint8_t qca_get_baudrate_value(int speed)
{
        switch (speed) {

	...

        default:
                return QCA_BAUDRATE_115200;
        }
}

If qca_get_baudrate_value() is called with 'speed == 0' it returns
QCA_BAUDRATE_115200, which is the same value with which
QCA_BAUDRATE_115200 is initialized.

It seems the initialization and the check for 'speed == 0' could be removed.