Hi Matthias,
On 2018-06-22 02:46, Matthias Kaehlcke wrote:
Hi Balakrishna,
On Thu, Jun 21, 2018 at 07:30:25PM +0530, Balakrishna Godavarthi wrote:
Hi Matthias,
On 2018-06-20 03:23, Matthias Kaehlcke wrote:
> On Sat, Jun 16, 2018 at 11:57:18AM +0530, Balakrishna Godavarthi wrote:
> > Add support to set voltage/current of various regulators
> > to power up/down Bluetooth chip wcn3990.
> >
> > Signed-off-by: Balakrishna Godavarthi <bgodavar@xxxxxxxxxxxxxx>
> > ---
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index 28ae6a17a595..1961e313aae7 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> >
> > static int qca_setup(struct hci_uart *hu)
> > {
> > struct hci_dev *hdev = hu->hdev;
> > struct qca_data *qca = hu->priv;
> > + struct qca_serdev *qcadev;
> > unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
> > int ret;
> > int soc_ver = 0;
> >
> > - bt_dev_info(hdev, "ROME setup");
> > + qcadev = serdev_device_get_drvdata(hu->serdev);
> >
> > /* 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);
> > +
> > + if (qcadev->btsoc_type == QCA_WCN3990) {
> > + bt_dev_dbg(hdev, "setting up wcn3990");
> > + hci_uart_set_flow_control(hu, true);
> > + ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE);
> > + if (ret) {
> > + bt_dev_err(hdev, "failed to send power on command");
> > + return ret;
> > + }
> > + serdev_device_close(hu->serdev);
> > + ret = serdev_device_open(hu->serdev);
> > + if (ret) {
> > + bt_dev_err(hdev, "failed to open port");
> > + return ret;
> > + }
> > + msleep(100);
> > + qca_set_init_speed(hu);
> > + hci_uart_set_flow_control(hu, false);
> > + ret = qca_read_soc_version(hdev, &soc_ver);
> > + if (ret < 0 || soc_ver == 0) {
> > + bt_dev_err(hdev, "Failed to get version %d", ret);
>
> serdev_device_close() ?
>
> Also applies to other error paths in this function.
>
[Bala]: sorry, i didn't get you.
A few lines above serdev_device_open() is called, in the error paths
the device should be closed.
[Bala]: i don't think closing of port is required.
qca_send_vendor_cmd() fails if skb_alloc() fails. that is not
linked with serdev_close().
we need to close the port, if we have issues from reading or
writing into the port.
pls correct me if i am wrong.
> > static int qca_serdev_probe(struct serdev_device *serdev)
> > {
> > struct qca_serdev *qcadev;
> > + const struct qca_vreg_data *data;
> > int err;
> >
> > qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
> > @@ -1041,47 +1264,85 @@ static int qca_serdev_probe(struct
> > serdev_device *serdev)
> > return -ENOMEM;
> >
> > qcadev->serdev_hu.serdev = serdev;
> > + data = of_device_get_match_data(&serdev->dev);
> > + if (data && data->soc_type == QCA_WCN3990)
> > + qcadev->btsoc_type = QCA_WCN3990;
> > + else
> > + qcadev->btsoc_type = QCA_ROME;
> > +
> > serdev_device_set_drvdata(serdev, qcadev);
> > + if (qcadev->btsoc_type == QCA_WCN3990) {
>
> nit: the double "if (soc_type == QCA_WCN3990)" is a bit odd. Consider
> changing this condition to "if (data && data->soc_type == QCA_WCN3990)"
> and assign qcadev->btsoc_type in the corresponding branch.
[bala]: will update.
I have idea of removing flow control from qca_setup() and use them in
qca_set_speed()
"i need to disable hardware flow control when sending change baudrate
request to WCN3990.
enabling it after setting host baudrate. this is only for wcn3990.
"
I definitely think it's positive to hide the
hci_uart_set_flow_control() call in qca_set_speed(). A few comments
inline.
static int qca_set_speed(struct hci_uart *hu, unsigned int speed,
enum qca_speed_type speed_type,
enum qca_btsoc_type soc_type)
{
That's a lot of parameters, in particular 'soc_type' seems a bit
off-topic in a function called qca_set_speed(). Passing struct
qca_serdev instead of struct hci_uart would make the 'soc_type'
parameter unnecessary.
[Bala]: will update.
unsigned int qca_baudrate;
int ret;
if (speed_type == QCA_INIT_SPEED)
goto change_host_baudrate;
If the order of setting host and chip baudrate can't be changed I
think it's preferable to set the init speed right here in the if
branch instead of doing the goto. I earlier mentioned the short cut of
a return (if possible) to save a level of indentation, but a goto
IMO doesn't improve readability and it's the first level of
indentation anyway.
[Bala]: will update.
if (soc_type == QCA_WCN3990)
hci_uart_set_flow_control(hu, true);
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 baudrate
(%d)",
ret);
return ret;
}
change_host_baudrate:
host_set_baudrate(hu, speed);
if (soc_type == QCA_WCN3990)
hci_uart_set_flow_control(hu, false);
return ret;
}
is it good idea?
In general I think it is.
--
Regards
Balakrishna.
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
