Re: [PATCH V3 4/8] i2c: i2c-qcom-geni: Add interconnect support

From: Akash Asthana <akashast@codeaurora.org>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: gregkh@linuxfoundation.org, agross@kernel.org,
	bjorn.andersson@linaro.org, wsa@the-dreams.de,
	broonie@kernel.org, mark.rutland@arm.com, robh+dt@kernel.org,
	georgi.djakov@linaro.org, linux-i2c@vger.kernel.org,
	linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
	swboyd@chromium.org, mgautam@codeaurora.org,
	linux-arm-msm@vger.kernel.org, linux-serial@vger.kernel.org,
	dianders@chromium.org, evgreen@chromium.org
Subject: Re: [PATCH V3 4/8] i2c: i2c-qcom-geni: Add interconnect support
Date: Tue, 7 Apr 2020 12:34:35 +0530	[thread overview]
Message-ID: <872c2c0a-6612-07db-abc2-2ee0c15fdbb2@codeaurora.org> (raw)
In-Reply-To: <20200331184939.GI199755@google.com>

Hi  Matthias,

On 4/1/2020 12:19 AM, Matthias Kaehlcke wrote:
> Hi Akash,
>
> On Tue, Mar 31, 2020 at 04:39:32PM +0530, Akash Asthana wrote:
>> Get the interconnect paths for I2C based Serial Engine device
>> and vote according to the bus speed of the driver.
>>
>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>> ---
>> Changes in V2:
>>   - As per Bjorn's comment, removed se == NULL check from geni_i2c_icc_get
>>   - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
>>   - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting
>>     path handle
>>   - As per Matthias comment, added error handling for icc_set_bw call
>>
>> Changes in V3:
>>   - As per Matthias comment, use common library APIs defined in geni-se
>>     driver for ICC functionality.
>>
>>   drivers/i2c/busses/i2c-qcom-geni.c | 30 +++++++++++++++++++++++++++++-
>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 18d1e4f..373c2ca 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -557,6 +557,26 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>   	gi2c->adap.dev.of_node = dev->of_node;
>>   	strlcpy(gi2c->adap.name, "Geni-I2C", sizeof(gi2c->adap.name));
>>   
>> +	ret = geni_icc_get(&gi2c->se, "qup-core", "qup-config", "qup-memory");
>> +	if (ret)
>> +		return ret;
>> +	/*
>> +	 * Set the bus quota for core and cpu to a reasonable value for
>> +	 * register access.
>> +	 * Set quota for DDR based on bus speed, assume peak requirement
>> +	 * as twice of avg bw.
>> +	 */
>> +	gi2c->se.to_core.avg_bw = GENI_DEFAULT_BW;
>> +	gi2c->se.to_core.peak_bw = Bps_to_icc(CORE_2X_100_MHZ);
>> +	gi2c->se.from_cpu.avg_bw = GENI_DEFAULT_BW;
>> +	gi2c->se.from_cpu.peak_bw = GENI_DEFAULT_BW;
>> +	gi2c->se.to_ddr.avg_bw = Bps_to_icc(gi2c->clk_freq_out);
>> +	gi2c->se.to_ddr.peak_bw = Bps_to_icc(2 * gi2c->clk_freq_out);
>> +
>> +	ret = geni_icc_vote_on(&gi2c->se);
>> +	if (ret)
>> +		return ret;
>> +
>>   	ret = geni_se_resources_on(&gi2c->se);
>>   	if (ret) {
>>   		dev_err(dev, "Error turning on resources %d\n", ret);
> I think you need to call geni_icc_vote_off() here and in other error paths.

No need to add geni_icc_vote_off() on error paths because we are using 
devm_of_icc_get to get ICC path handle.

If we are going to fail probe then "devm_icc_release---->icc_put" will 
set votes to 0 on each path. https://patchwork.kernel.org/patch/11467507/

Regards,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project