Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i0ad843c9:Fastmail
Subject: Re: [PATCH 6/6] pinctrl: sunxi: Add driver for Allwinner D1/D1s
To:     Heiko Stuebner <heiko@sntech.de>, Chen-Yu Tsai <wens@csie.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        linux-arm-kernel@lists.infradead.org
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Maxime Ripard <mripard@kernel.org>, Ondrej Jirman <x@xff.cz>,
        Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-sunxi@lists.linux.dev
References: <20220626021148.56740-1-samuel@sholland.org>
 <20220626021148.56740-7-samuel@sholland.org> <17090319.JCcGWNJJiE@phil>
From:   Samuel Holland <samuel@sholland.org>
Message-ID: <e6d2d4e8-b385-11d6-4d41-454542735269@sholland.org>
Date:   Fri, 1 Jul 2022 10:16:07 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <17090319.JCcGWNJJiE@phil>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Heiko,

On 7/1/22 8:13 AM, Heiko Stuebner wrote:
> Am Sonntag, 26. Juni 2022, 04:11:47 CEST schrieb Samuel Holland:
>> These SoCs contain a pinctrl with a new register layout. Use the variant
>> parameter to set the right register offsets. This pinctrl also increases
>> the number of functions per pin from 8 to 16, taking advantage of all 4
>> bits in the mux config field (so far, only functions 0-8 and 14-15 are
>> used). This increases the maximum possible number of functions.
>>
>> D1s is a low pin count version of the D1 SoC, with some pins omitted.
>> The remaining pins have the same function assignments as D1.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
> 
> On a D1-Nezha
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> 
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> 
> with one remark below

Thanks for reviewing the series.

>> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> index ec7daaa5666b..350044d4c1b5 100644
>> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> @@ -1297,11 +1297,11 @@ static int sunxi_pinctrl_build_state(struct platform_device *pdev)
>>  
>>  	/*
>>  	 * Find an upper bound for the maximum number of functions: in
>> -	 * the worst case we have gpio_in, gpio_out, irq and up to four
>> +	 * the worst case we have gpio_in, gpio_out, irq and up to seven
>>  	 * special functions per pin, plus one entry for the sentinel.
>>  	 * We'll reallocate that later anyway.
>>  	 */
>> -	pctl->functions = kcalloc(4 * pctl->ngroups + 4,
>> +	pctl->functions = kcalloc(7 * pctl->ngroups + 4,
>>  				  sizeof(*pctl->functions),
>>  				  GFP_KERNEL);
>>  	if (!pctl->functions)
>> @@ -1494,9 +1494,15 @@ int sunxi_pinctrl_init_with_variant(struct platform_device *pdev,
>>  	pctl->dev = &pdev->dev;
>>  	pctl->desc = desc;
>>  	pctl->variant = variant;
>> -	pctl->bank_mem_size = BANK_MEM_SIZE;
>> -	pctl->pull_regs_offset = PULL_REGS_OFFSET;
>> -	pctl->dlevel_field_width = DLEVEL_FIELD_WIDTH;
>> +	if (pctl->variant >= PINCTRL_SUN20I_D1) {
>> +		pctl->bank_mem_size = D1_BANK_MEM_SIZE;
>> +		pctl->pull_regs_offset = D1_PULL_REGS_OFFSET;
>> +		pctl->dlevel_field_width = D1_DLEVEL_FIELD_WIDTH;
>> +	} else {
>> +		pctl->bank_mem_size = BANK_MEM_SIZE;
>> +		pctl->pull_regs_offset = PULL_REGS_OFFSET;
>> +		pctl->dlevel_field_width = DLEVEL_FIELD_WIDTH;
>> +	}
> 
> this is likely ok for _one_ variant (so for now this should be ok) but
> will get ugly when there are more of them.
> 
> So in the long term it might make sense to pass these values in from
> the soc-specific driver maybe?

Yes, in the long term. It took 10 years before the first layout change, so I do
not expect another layout change any time soon.

For now, I did not want to deal with the overhead of passing in the offsets
individually, nor coming up with a name for each layout to look up the offsets
from a table. (The BSP calls the variants "SUNXI_PCTL_HW_TYPE_0" and
"SUNXI_PCTL_HW_TYPE_1", which is... not descriptive.)

Regards,
Samuel