Hi Balakrishna,
On Wed, May 09, 2018 at 03:37:34PM +0530, Balakrishna Godavarthi wrote:
> Hi,
> Please find my comments inline.
>
> On 2018-05-05 06:47, Matthias Kaehlcke wrote:
> > Hi,
> >
> > On Fri, May 04, 2018 at 09:05:05PM +0530, Balakrishna Godavarthi wrote:
> > > Add support to set voltage/current of various regulators
> > > to power up/down Bluetooth chip wcn3990.
> > > Add support to read baudrate from dts.
> > >
> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@xxxxxxxxxxxxxx>
> > > ---
> > > drivers/bluetooth/hci_qca.c | 555
> > > ++++++++++++++++++++++++++++++++++++++------
> > > 1 file changed, 483 insertions(+), 72 deletions(-)
> > >
> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index f05382b..075fab7 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > > ...
> > > - bt_dev_info(hdev, "ROME setup");
> > > + /* disable flow control, as chip is still not turned on */
> > > + hci_uart_set_flow_control(hu, true);
> >
> > The interface of this function is confusing. enable = true disables
> > flow control ... Not the fault of this driver though :)
> [Bala]: yes it is bit confusing. i taught to update.. but not sure of
> impact.
> so instead of updating, using the same function and written a
> comment above.
Sure, not much you can do about it. Just mentioned it since I was
about to comment that things are done inversely, but tean checked
hci_uart_set_flow_control() and saw that it has this odd
interface. Folks more familiar with the Bluetooth subsystem are
probably already used to it ;-)
> > > - if (speed)
> > > - host_set_baudrate(hu, speed);
> > > + /* Enable flow control */
> > > + hci_uart_set_flow_control(hu, false);
> > > + /* wait until flow control settled */
> > > + mdelay(100);
> >
> > A busy wait of 100ms doesn't seem a good idea. Use msleep()
> > instead. Is it really necessary to wait that long?
> [Bala]: yes this delay is required for BT Soc to bootup and settle down.
:(
> > What follows looks similar to the Cherokee path. I didn't look at all
> > the details, but it's probably possible to share some more code.
> [Bala]: yes these are two different chip.
> we also have difference in setup. the follwing are major differences
> Cherokee uses flow control. where ROME flow control is disabled.
> ON sequence also varies.
Ok, I'll give it another pass in the next revision to see if I have
suggestions to make it at least a bit less redundant.
> > > +static const struct btqca_vreg_data cherokee_data = {
> > > + .soc_type = BTQCA_CHEROKEE,
> > > + .vregs = (struct btqca_vreg []) {
> > > + { "vddio", 1352000, 1352000, 0 },
> > > + { "vddxtal", 1904000, 2040000, 0 },
> > > + { "vddcore", 1800000, 1800000, 1 },
> > > + { "vddpa", 130400, 1304000, 1 },
> >
> > 0 missing for min_v?
>
> [Bala]: min_v is the minimum voltage required for BT chip.
I referred to the value '130400', which happens to be exactly 1/10 of
the max_v '1304000', so I suspect the voltages are the same and a zero
is missing.
> > > + regulator_set_voltage(qca->vreg_bulk[i].consumer,
> > > + vregs[i].min_v,
> > > + vregs[i].max_v);
> >
> > Check return value.
> [Bala]: i am checking the status for regulator enable
> do we really require to check the status. as this same regulators
> are used by different sub systems.
> we will get an error during regulator enable when voltages are not
> settled down.
regulator_set_voltage() could fail due to requesting a voltage range
not supported by the regulator. In this case regulator_enable() could
still succeed, but not supply the requested voltage.
Thanks
Matthias
--
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
