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

[AngeloGioacchino Del Regno]

On 7/13/26 14:51, Nikolai Burov wrote:
> 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.

No worries, it happens.

> > > +    [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?

Eh, fair point. I guess it's fine then.

For the numbers, that's because the registers for the same region of the chip
(meaning physical region, really) are scattered all around... but bm2, bm, bm1
refer to the same physical region (again, really physical, look at the bga array).

> [...]
> > > +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?

If there are holes in between, yes, that wouldn't work.

Honestly though, this is starting to be a problem - as in - up until now,
practically all SoCs had EINTs for each pin, or no holes in between... but
I can see that this is changing, as MT6991, MT6993 and now MT6858 all have
those holes.

This is a note and I'm not asking you to do this but, at some point, we should
really add some logic to the mtk-eint driver so that it doesn't forcefully (and
at this point in time, even stupidly) map/expect pin data at the same array
index as the pin number.

That's probably more important than fixing mt6878, but well, I understand that
this stuff would take a bit more time than adding just a few entries in arrays
right now, and I also understand that time may be lacking, so I am definitely
not blocking the addition of 6858 for this (of course, as long as you fill up
the holes there)

Cheers,
Angelo