Received-SPF: pass (google.com: domain of linux-kernel+bounces-180141-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 15 May 2024 18:29:54 +0200
From: Luca Ceresoli <luca.ceresoli@bootlin.com>
To: Dmitry Yashin <dmt.yashin@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>, Heiko Stuebner
 <heiko@sntech.de>, Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley
 <conor+dt@kernel.org>, Rob Herring <robh@kernel.org>, Jianqun Xu
 <jay.xu@rock-chips.com>, devicetree@vger.kernel.org,
 linux-gpio@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] pinctrl: rockchip: add rk3308b SoC support
Message-ID: <20240515182954.03c4a475@booty>
In-Reply-To: <20240515121634.23945-4-dmt.yashin@gmail.com>
References: <20240515121634.23945-1-dmt.yashin@gmail.com>
	<20240515121634.23945-4-dmt.yashin@gmail.com>
Organization: Bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

Hello Dmitry,

On Wed, 15 May 2024 17:16:34 +0500
Dmitry Yashin <dmt.yashin@gmail.com> wrote:

> Add pinctrl support for rk3308b. This pin controller much the same as
> rk3308's, but with additional iomux routes and 3bit iomuxes selected
> via gpio##_sel_src_ctrl registers. Set them up in the function
> rk3308b_soc_sel_src_init to use new 3bit iomuxes over some 2bit old ones.
> 
> Fixes: 1f3e25a06883 ("pinctrl: rockchip: fix RK3308 pinmux bits")
> Signed-off-by: Dmitry Yashin <dmt.yashin@gmail.com>

Thanks for the effort! I have one high-level remark, see below.
Otherwise at a superficial look it looks good.

> @@ -3952,6 +4150,8 @@ static const struct of_device_id rockchip_pinctrl_dt_match[] = {
>  		.data = &rk3288_pin_ctrl },
>  	{ .compatible = "rockchip,rk3308-pinctrl",
>  		.data = &rk3308_pin_ctrl },
> +	{ .compatible = "rockchip,rk3308b-pinctrl",
> +		.data = &rk3308b_pin_ctrl },

I'm skeptical about this being bound to a new DT compatible. As far as I
know the RK3308 and RK3308B are mostly equivalent, so it looks as the
pinctrl implementation could be detected at runtime. This would let
products to be built with either chip version and work on any without
any DT change.

Code for reading the chip ID is in the RK3308 codec driver [0].

[0] https://lore.kernel.org/all/20240305-rk3308-audio-codec-v4-4-312acdbe628f@bootlin.com/ -> search "GRF_CHIP_ID"

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com