Re: [PATCH 2/3] pinctrl: mediatek: Add driver for MT6858

[Nikolai Burov]

On 7/13/26 10:01 AM, AngeloGioacchino Del Regno wrote:
[...]
> > +	[PINCTRL_PIN_REG_MODE] = MTK_RANGE(mt6858_pin_mode_range),
> > +	[PINCTRL_PIN_REG_DIR] = MTK_RANGE(mt6858_pin_dir_range),
> > +	[PINCTRL_PIN_REG_DI] = MTK_RANGE(mt6858_pin_di_range),
> > +	[PINCTRL_PIN_REG_DO] = MTK_RANGE(mt6858_pin_do_range),
> > +	[PINCTRL_PIN_REG_SR] = MTK_RANGE(mt6858_pin_dir_range),
>
> This is Slew Rate, not Direction... And I think this is wrong.
>
> If you don't know where the slew rate registers are, or if that is an 
> unsupported operation, just omit PINCTRL_PIN_REG_SR.

Thanks for noticing this! This seems to be a mistake in the downstream 
driver that I unintentionally copied over. I will remove the slew rate.

> > +	[PINCTRL_PIN_REG_SMT] = MTK_RANGE(mt6858_pin_smt_range),
> > +	[PINCTRL_PIN_REG_IES] = MTK_RANGE(mt6858_pin_ies_range),
> > +	[PINCTRL_PIN_REG_PU] = MTK_RANGE(mt6858_pin_pu_range),
> > +	[PINCTRL_PIN_REG_PD] = MTK_RANGE(mt6858_pin_pd_range),
> > +	[PINCTRL_PIN_REG_DRV] = MTK_RANGE(mt6858_pin_drv_range),
> > +	[PINCTRL_PIN_REG_PUPD] = MTK_RANGE(mt6858_pin_pupd_range),
> > +	[PINCTRL_PIN_REG_R0] = MTK_RANGE(mt6858_pin_r0_range),
> > +	[PINCTRL_PIN_REG_R1] = MTK_RANGE(mt6858_pin_r1_range),
> > +	[PINCTRL_PIN_REG_DRV_ADV] = MTK_RANGE(mt6858_pin_drv_adv_range),
> > +	[PINCTRL_PIN_REG_RSEL] = MTK_RANGE(mt6858_pin_rsel_range),
> > +};
> > +
> > +static const char * const mt6858_pinctrl_register_base_names[] = {
> > +	"base", "lm", "rb", "bm2", "bm", "bm1", "lt", "lt1", "rt", "rt1",
>
> Fix ordering:
>
> "base", "lm", "rb", "bm", "bm1", "bm2", "lt", "lt1", "rt", "rt1",

The bases are already in ascending address order. I don't know what the 
1 and 2 suffixes in the names of the iocfg regions mean, they may just 
be a historical leftover. Are the names more important than the 
addresses here?

[...]
> > +static const struct of_device_id mt6858_pinctrl_of_match[] = {
> > +	{ .compatible = "mediatek,mt6858-pinctrl", .data = &mt6858_data },
> > +	{ }
>
>      { /* sentinel */ }

Thanks!

[...]
> > +	MTK_EINT_PIN(119, 0, 72, 0),
> > +	MTK_EINT_PIN(120, 0, 73, 0),
> > +	MTK_EINT_PIN(121, 0, 74, 0),
>
> Looks like you're missing ~70 pins here?
>
> If those have no EINT support, you can declare them as
>
>      MTK_EINT_PIN(122, EINT_INVALID_BASE, 0, 0),
>
> ...or you can add a macro to add a (sequential) range of pins with no 
> EINT support.

Adding a variable number of mtk_eint_pin structs (with a different pin 
number inside each struct) doesn't seem possible using a single macro, 
so I'll just manually add all of them to the list.

While writing this, I was looking at the mt6878 driver, which also omits 
a range of pins. Should that be fixed as well?

--
Best regards,
Nikolai