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

[Andrea della Porta <andrea.porta@suse.com>]

Hi Stefan,

On 11:57 Sun 24 Aug     , Stefan Wahren wrote:
> Hi Andrea,
> 
> Am 11.08.25 um 16:46 schrieb Andrea della Porta:
> > From: "Ivan T. Ivanov" <iivanov@suse.de>
> > 
> > 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.

[...]

> > +#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.

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

[...]

> > +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.

> > +
> > +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),

[...]

> > +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.

> > +};
> > +
> ...
> > +
> > +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