Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: rnayak=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.codeaurora.org DBEDBC4338F
Subject: Re: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     agross@kernel.org, linus.walleij@linaro.org,
        linux-arm-msm@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-kernel@vger.kernel.org, psodagud@codeaurora.org,
        dianders@chromium.org
References: <1635250056-20274-1-git-send-email-rnayak@codeaurora.org>
 <YXsrtTGZW66mUtkU@ripper>
From:   Rajendra Nayak <rnayak@codeaurora.org>
Message-ID: <4fda39ce-189e-4873-dd40-3219c0052ffd@codeaurora.org>
Date:   Fri, 29 Oct 2021 15:28:44 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <YXsrtTGZW66mUtkU@ripper>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 10/29/2021 4:31 AM, Bjorn Andersson wrote:
> On Tue 26 Oct 05:07 PDT 2021, Rajendra Nayak wrote:
> 
>> From: Prasad Sodagudi <psodagud@codeaurora.org>
>>
>> egpio is a scheme which allows special power Island Domain IOs
>> (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular
>> TLMM functions with Island Domain functions.
>> With this scheme, an IO can be controlled both by the cpu running
>> linux and the Island processor. This provides great flexibility to
>> re-purpose the Island IOs for regular TLMM usecases.
>>
>> 2 new bits are added to ctl_reg, egpio_present is a read only bit
>> which shows if egpio feature is available or not on a given gpio.
>> egpio_enable is the read/write bit and only effective if egpio_present
>> is 1. Once its set, the Island IO is controlled from Chip TLMM.
>> egpio_enable when set to 0 means the GPIO is used as Island Domain IO.
>>
>> To support this we add a new function 'egpio' which can be used to
>> set the egpio_enable to 0, for any other TLMM controlled functions
>> we set the egpio_enable to 1.
>>
>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>   drivers/pinctrl/qcom/pinctrl-msm.c | 17 +++++++++++++++--
>>   drivers/pinctrl/qcom/pinctrl-msm.h |  4 ++++
>>   2 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 8476a8a..bfdba3a 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -185,6 +185,7 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>>   	unsigned int irq = irq_find_mapping(gc->irq.domain, group);
>>   	struct irq_data *d = irq_get_irq_data(irq);
>>   	unsigned int gpio_func = pctrl->soc->gpio_func;
>> +	unsigned int egpio_func = pctrl->soc->egpio_func;
>>   	const struct msm_pingroup *g;
>>   	unsigned long flags;
>>   	u32 val, mask;
>> @@ -218,8 +219,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>>   	raw_spin_lock_irqsave(&pctrl->lock, flags);
>>   
>>   	val = msm_readl_ctl(pctrl, g);
>> -	val &= ~mask;
>> -	val |= i << g->mux_bit;
>> +
>> +	if (egpio_func && i == egpio_func) {
>> +		if (val & BIT(g->egpio_present))
>> +			val &= ~BIT(g->egpio_enable);
>> +		else
>> +			return -EINVAL;
> 
> You're returning here with pctrl->lock held and irqs disabled.

argh, right. I will fix that and repost.
I wonder if I should just drop that error handling completely,
we wouldn't end up here unless the platform driver wrongly populates
a pin which does not support egpio with a egpio function.

> 
>> +	} else {
>> +		val &= ~mask;
>> +		val |= i << g->mux_bit;
>> +		/* Check if egpio present and enable that feature */
> 
> I never remember if egpio_enable means apss or lpass, so I think this
> comment would be better as:
> 
> 		/* Claim ownership of pin if egpio capable */

:) makes sense

> 
>> +		if (egpio_func && (val & BIT(g->egpio_present)))
> 
> Can't you drop the parenthesis around the second expression?

yes, will do, thanks for the review.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation