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;
Subject: Re: [PATCH v5 2/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     linus.walleij@linaro.org, robh+dt@kernel.org, agross@kernel.org,
        linux-arm-msm@vger.kernel.org, linux-gpio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20201201142830.13152-1-srinivas.kandagatla@linaro.org>
 <20201201142830.13152-3-srinivas.kandagatla@linaro.org>
 <X8ad9lw73ZKsqXKT@builder.lan>
 <c7f8e95d-21aa-b723-2b6e-0420cb400025@linaro.org>
Message-ID: <50411940-ded8-d5cc-c373-f1bbad2c02fe@linaro.org>
Date:   Wed, 2 Dec 2020 15:17:37 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <c7f8e95d-21aa-b723-2b6e-0420cb400025@linaro.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 02/12/2020 09:56, Srinivas Kandagatla wrote:
>>> +        case PIN_CONFIG_SLEW_RATE:
>>> +            if (arg > LPI_SLEW_RATE_MAX) {
>>> +                dev_err(pctldev->dev, "invalid slew rate %u for pin: 
>>> %d\n",
>>> +                    arg, group);
>>> +                return -EINVAL;
>>> +            }
>>> +
>>> +            slew_offset = g->slew_offset;
>>> +            if (slew_offset == NO_SLEW)
>>> +                break;
>>> +
>>> +            mutex_lock(&pctrl->slew_access_lock);
>>> +            sval = ioread32(pctrl->slew_base + LPI_SLEW_RATE_CTL_REG);
>>> +
>>> +            for (i = 0; i < LPI_SLEW_BITS_SIZE; i++) {
>>> +                assign_bit(slew_offset, &sval, arg & 0x01);
>>> +                slew_offset++;
>>> +                arg = arg >> 1;
>>> +            }
>>
>> Isn't this loop just the same as
>>
>>     FIELD_SET(3 << slew_offset, arg & 3, sval)

None of FIELD_* or replace_bits apis will work here, as the mask passed 
to those macros should be a constant #define. Passing variable to them 
in mask will result in compile error!

mask in this case is retrieved at runtime.

I think we should live with the existing code unless there is a strong 
reason for it to change! Or a better alternative.


--srini


> 

> This will not work FIELD_SET will not clear any bits wich are already 
> set! assing_bit will work, but we could do better by adding slew_mask 
> per pin rather than slew_offset which should allow us to use 
> replace_bits straight away.