Re: [PATCH 2/4] Bluetooth: hci_qca: Don't vote for specific voltage

[Harish Bandi]

+ Bala

On 2019-10-18 23:52, Matthias Kaehlcke wrote:
> On Thu, Oct 17, 2019 at 10:24:02PM -0700, Bjorn Andersson wrote:
> > Devices with specific voltage requirements should not request voltage
> > from the driver, but instead rely on the system configuration to 
> > define
> > appropriate voltages for each rail.
> >
> > This ensures that PMIC and board variations are accounted for, 
> > something
> > that the 0.1V range in the hci_qca driver currently tries to address.
> > But on the Lenovo Yoga C630 (with wcn3990) vddch0 is 3.1V, which means
> > the driver will fail to set the voltage.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > ---
> >  drivers/bluetooth/hci_qca.c | 26 ++++++++------------------
> >  1 file changed, 8 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index c07c529b0d81..54aafcc69d06 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -130,8 +130,6 @@ enum qca_speed_type {
> >   */
> >  struct qca_vreg {
> >  	const char *name;
> > -	unsigned int min_uV;
> > -	unsigned int max_uV;
> >  	unsigned int load_uA;
> >  };
> >
> > @@ -1332,10 +1330,10 @@ static const struct hci_uart_proto qca_proto = 
> > {
> >  static const struct qca_vreg_data qca_soc_data_wcn3990 = {
> >  	.soc_type = QCA_WCN3990,
> >  	.vregs = (struct qca_vreg []) {
> > -		{ "vddio",   1800000, 1900000,  15000  },
> > -		{ "vddxo",   1800000, 1900000,  80000  },
> > -		{ "vddrf",   1300000, 1350000,  300000 },
> > -		{ "vddch0",  3300000, 3400000,  450000 },
> > +		{ "vddio", 15000  },
> > +		{ "vddxo", 80000  },
> > +		{ "vddrf", 300000 },
> > +		{ "vddch0", 450000 },
> >  	},
> >  	.num_vregs = 4,
> >  };
> > @@ -1343,10 +1341,10 @@ static const struct qca_vreg_data 
> > qca_soc_data_wcn3990 = {
> >  static const struct qca_vreg_data qca_soc_data_wcn3998 = {
> >  	.soc_type = QCA_WCN3998,
> >  	.vregs = (struct qca_vreg []) {
> > -		{ "vddio",   1800000, 1900000,  10000  },
> > -		{ "vddxo",   1800000, 1900000,  80000  },
> > -		{ "vddrf",   1300000, 1352000,  300000 },
> > -		{ "vddch0",  3300000, 3300000,  450000 },
> > +		{ "vddio", 10000  },
> > +		{ "vddxo", 80000  },
> > +		{ "vddrf", 300000 },
> > +		{ "vddch0", 450000 },
> >  	},
> >  	.num_vregs = 4,
> >  };
> > @@ -1386,13 +1384,6 @@ static int qca_power_off(struct hci_dev *hdev)
> >  static int qca_enable_regulator(struct qca_vreg vregs,
> >  				struct regulator *regulator)
> >  {
> > -	int ret;
> > -
> > -	ret = regulator_set_voltage(regulator, vregs.min_uV,
> > -				    vregs.max_uV);
> > -	if (ret)
> > -		return ret;
> > -
> >  	return regulator_enable(regulator);
> >
> >  }
> > @@ -1401,7 +1392,6 @@ static void qca_disable_regulator(struct 
> > qca_vreg vregs,
> >  				  struct regulator *regulator)
> >  {
> >  	regulator_disable(regulator);
> > -	regulator_set_voltage(regulator, 0, vregs.max_uV);
> >
> >  }
>
> This was brought up multiple times during the initial review, but
> wasn't addressed.
>
> Reviewed-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>