Re: [PATCH v3 2/3] pinctrl: bcm: Add STB family pin controller driver

Hi Stefan,

On 11:57 Sun 24 Aug     , Stefan Wahren wrote:

Hi Andrea,

Am 11.08.25 um 16:46 schrieb Andrea della Porta:

quoted

From: "Ivan T. Ivanov" <redacted>

This driver provide pin muxing and configuration functionality
for BCM2712 SoC used by RPi5. According to [1] this chip is an
instance of the one used in Broadcom STB  product line.

[...]

quoted

+#define AGPIO_PIN(n)		PINCTRL_PIN(n, "aon_gpio" #n)
+#define SGPIO_PIN(n)		PINCTRL_PIN((n) + 32, "aon_sgpio" #n)

It would be great, if there is comment explaining the difference between a
AGPIO_PIN and a SGPIO_PIN?

In case AGPIO_PIN stands for AON_GPIO_PIN, i would prefer the latter.

I need to dig more on the SGPIO type, more on that as soon as I'll
find some more details. No problem in using more explanatory label,
of_course.

quoted

+
+struct pin_regs {
+	u16 mux_bit;
+	u16 pad_bit;
+};
+

[...]

quoted

+enum brcmstb_funcs {
+	func_gpio,
+	func_alt1,
+	func_alt2,
+	func_alt3,
+	func_alt4,
+	func_alt5,
+	func_alt6,
+	func_alt7,
+	func_alt8,
+	func_aon_cpu_standbyb,
+	func_aon_fp_4sec_resetb,
+	func_aon_gpclk,
+	func_aon_pwm,
+	func_arm_jtag,
+	func_aud_fs_clk0,
+	func_avs_pmu_bsc,
+	func_bsc_m0,
+	func_bsc_m1,
+	func_bsc_m2,
+	func_bsc_m3,
+	func_clk_observe,
+	func_ctl_hdmi_5v,
+	func_enet0,
+	func_enet0_mii,
+	func_enet0_rgmii,
+	func_ext_sc_clk,
+	func_fl0,
+	func_fl1,
+	func_gpclk0,
+	func_gpclk1,
+	func_gpclk2,
+	func_hdmi_tx0_auto_i2c,
+	func_hdmi_tx0_bsc,
+	func_hdmi_tx1_auto_i2c,
+	func_hdmi_tx1_bsc,
+	func_i2s_in,
+	func_i2s_out,
+	func_ir_in,
+	func_mtsif,
+	func_mtsif_alt,
+	func_mtsif_alt1,
+	func_pdm,
+	func_pkt,
+	func_pm_led_out,
+	func_sc0,
+	func_sd0,
+	func_sd2,
+	func_sd_card_a,
+	func_sd_card_b,
+	func_sd_card_c,
+	func_sd_card_d,
+	func_sd_card_e,
+	func_sd_card_f,
+	func_sd_card_g,
+	func_spdif_out,
+	func_spi_m,
+	func_spi_s,
+	func_sr_edm_sense,
+	func_te0,
+	func_te1,
+	func_tsio,
+	func_uart0,
+	func_uart1,
+	func_uart2,
+	func_usb_pwr,
+	func_usb_vbus,
+	func_uui,
+	func_vc_i2c0,
+	func_vc_i2c3,
+	func_vc_i2c4,
+	func_vc_i2c5,
+	func_vc_i2csl,
+	func_vc_pcm,
+	func_vc_pwm0,
+	func_vc_pwm1,
+	func_vc_spi0,
+	func_vc_spi3,
+	func_vc_spi4,
+	func_vc_spi5,
+	func_vc_uart0,
+	func_vc_uart2,
+	func_vc_uart3,
+	func_vc_uart4,
+	func__,
+	func_count = func__
+};

I'm very sceptical that this enum is generic. I would tend use to
bcm2712_funcs here.

Ack.

quoted

+
+static const struct pin_regs bcm2712_c0_gpio_pin_regs[] = {
+	GPIO_REGS(0, 0, 0, 7, 7),
+	GPIO_REGS(1, 0, 1, 7, 8),
+	GPIO_REGS(2, 0, 2, 7, 9),

[...]

quoted

+static const struct brcmstb_pin_funcs bcm2712_c0_aon_gpio_pin_funcs[] = {
+	PIN(0, ir_in, vc_spi0, vc_uart3, vc_i2c3, te0, vc_i2c0, _, _),
+	PIN(1, vc_pwm0, vc_spi0, vc_uart3, vc_i2c3, te1, aon_pwm, vc_i2c0, vc_pwm1),
+	PIN(2, vc_pwm0, vc_spi0, vc_uart3, ctl_hdmi_5v, fl0, aon_pwm, ir_in, vc_pwm1),
+	PIN(3, ir_in, vc_spi0, vc_uart3, aon_fp_4sec_resetb, fl1, sd_card_g, aon_gpclk, _),
+	PIN(4, gpclk0, vc_spi0, vc_i2csl, aon_gpclk, pm_led_out, aon_pwm, sd_card_g, vc_pwm0),
+	PIN(5, gpclk1, ir_in, vc_i2csl, clk_observe, aon_pwm, sd_card_g, vc_pwm0, _),
+	PIN(6, uart1, vc_uart4, gpclk2, ctl_hdmi_5v, vc_uart0, vc_spi3, _, _),
+	PIN(7, uart1, vc_uart4, gpclk0, aon_pwm, vc_uart0, vc_spi3, _, _),
+	PIN(8, uart1, vc_uart4, vc_i2csl, ctl_hdmi_5v, vc_uart0, vc_spi3, _, _),
+	PIN(9, uart1, vc_uart4, vc_i2csl, aon_pwm, vc_uart0, vc_spi3, _, _),
+	PIN(10, tsio, ctl_hdmi_5v, sc0, spdif_out, vc_spi5, usb_pwr, aon_gpclk, sd_card_f),
+	PIN(11, tsio, uart0, sc0, aud_fs_clk0, vc_spi5, usb_vbus, vc_uart2, sd_card_f),
+	PIN(12, tsio, uart0, vc_uart0, tsio, vc_spi5, usb_pwr, vc_uart2, sd_card_f),
+	PIN(13, bsc_m1, uart0, vc_uart0, uui, vc_spi5, arm_jtag, vc_uart2, vc_i2c3),
+	PIN(14, bsc_m1, uart0, vc_uart0, uui, vc_spi5, arm_jtag, vc_uart2, vc_i2c3),
+	PIN(15, ir_in, aon_fp_4sec_resetb, vc_uart0, pm_led_out, ctl_hdmi_5v, aon_pwm, aon_gpclk,
+	    _),
+	PIN(16, aon_cpu_standbyb, gpclk0, pm_led_out, ctl_hdmi_5v, vc_pwm0, usb_pwr, aud_fs_clk0,
+	    _),

I think it's okay to violate the 80 char limit in these both cases to
improve readability.

Ack.

quoted

+};
+

...

quoted

+
+static int brcmstb_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
+			       unsigned long *config)
+{
+	struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	u32 arg;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+		arg = (brcmstb_pull_config_get(pc, pin) == BRCMSTB_PULL_NONE);
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		arg = (brcmstb_pull_config_get(pc, pin) == BRCMSTB_PULL_DOWN);
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		arg = (brcmstb_pull_config_get(pc, pin) == BRCMSTB_PULL_UP);

I'm not sure this is correct. I would expect that "arg" contains the
resistance in Ohm for PULL_DOWN & PULL_UP.

In this case I don't have insight about the current impedance value. Since
this is easily something that changes between different implementations, I
would leave it as it is as it's more general, i.e. 0 for disabled and 1 for
pull up/down enabled (which seems to be the most common behaviour for pinconf
drivers anyway).

Once we know more details about the Ohm values or a new STB implementation
arises, we can quickly add the relevant data in the specific driver code.

Many thanks,
Andrea

Best regards