Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Mon, 21 Oct 2019 12:07:15 +0530
From:   Harish Bandi <c-hbandi@codeaurora.org>
To:     Matthias Kaehlcke <mka@chromium.org>,
        Balakrishna Godavarthi <bgodavar@codeaurora.org>
Cc:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jeffrey Hugo <jeffrey.l.hugo@gmail.com>,
        linux-arm-msm@vger.kernel.org,
        linux-bluetooth-owner@vger.kernel.org
Subject: Re: [PATCH 2/4] Bluetooth: hci_qca: Don't vote for specific voltage
In-Reply-To: <20191018182205.GA20212@google.com>
References: <20191018052405.3693555-1-bjorn.andersson@linaro.org>
 <20191018052405.3693555-3-bjorn.andersson@linaro.org>
 <20191018182205.GA20212@google.com>
Message-ID: <7f9a4de91f364a5f8ce707c8d8a2344d@codeaurora.org>
User-Agent: Roundcube Webmail/1.2.5
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

+ 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@linaro.org>
>> ---
>>  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@chromium.org>