Re: [PATCH] pinctrl: mtmips: support requesting different functions for same group

[Shiji Yang <yangshiji66@xxxxxxxxxxx>]

On Sat, 29 Jul 2023 19:39:34 +0300, Arınç ÜNAL wrote:
> On 26.07.2023 03:48, Shiji Yang wrote:
> > Sometimes pinctrl consumers may request different functions for the
> > same pin group in different situations. This patch can help to reset
> > the group function flag when requesting a different function.
> >
> > Signed-off-by: Shiji Yang <yangshiji66@xxxxxxxxxxx>
> > ---
> >   drivers/pinctrl/mediatek/pinctrl-mtmips.c | 21 +++++++++++++++++----
> >   1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtmips.c b/drivers/pinctrl/mediatek/pinctrl-mtmips.c
> > index efd77b6c5..e5e085915 100644
> > --- a/drivers/pinctrl/mediatek/pinctrl-mtmips.c
> > +++ b/drivers/pinctrl/mediatek/pinctrl-mtmips.c
> > @@ -123,11 +123,24 @@ static int mtmips_pmx_group_enable(struct pinctrl_dev *pctrldev,
> >        int i;
> >        int shift;
> >  
> > -     /* dont allow double use */
> > +     /*
> > +      * for the same pin group, if request a different function,
> > +      * then clear the group function flag and continue, else exit.
> > +      */
> >        if (p->groups[group].enabled) {
> > -             dev_err(p->dev, "%s is already enabled\n",
> > -                     p->groups[group].name);
> > -             return 0;
> > +             for (i = 0; i < p->groups[group].func_count; i++) {
> > +                     if (p->groups[group].func[i].enabled == 1) {
> > +                             if (!strcmp(p->func[func]->name,
> > +                                     p->groups[group].func[i].name))
> > +                                     return 0;
> > +                             p->groups[group].func[i].enabled = 0;
> > +                             break;
> > +                     }
> > +             }
> > +
> > +             /* exit if request the "gpio" function again */
> > +             if (i == p->groups[group].func_count && func == 0)
> > +                     return 0;
> 
> Could you help me understand why? The @gpio_request_enable operation is
> not properly implemented on this driver so this check would never be
> true, no?

'.func_count' is the function number of a pin group. I will use MT7620's
'pa' group as an example to explain it. 'pa' group only has 1 function
'pa' ('gpio' function not included). When this group is first requested
as a gpio function, groups[pa].enabled will be set to 1, However,
groups[pa].func[i].enabled will still be 0 because 'gpio' is not a member
of groups[pa]. In this case, when we request gpio function again, for()
loop will do nothing but just increase 'i' to 1 (func_count). This will
make 'if (i == p->groups[group].func_count && func == 0)' to be true.


> 
> Even if it was, this makes it so that if a pin group is already given a
> function (meaning the pin group is enabled), it will never be given the
> gpio function when requested, unless I understand it wrong.
> 
> Arınç

When current function is pa, and we want to request a gpio function,
'if (p->groups[group].func[i].enabled == 1)' check will break the for()
loop and continue the pin configuration code.

If this 'if (p->groups[group].enabled)' check doesn't return, the pinmux
mode register will be reset and reconfigured.

	p->groups[group].enabled = 1;
	p->func[func]->enabled = 1;

	shift = p->groups[group].shift;
	if (shift >= 32) {
		shift -= 32;
		reg = SYSC_REG_GPIO_MODE2;
	}
	mode = rt_sysc_r32(reg);
	mode &= ~(p->groups[group].mask << shift);

	/* mark the pins as gpio */
	for (i = 0; i < p->groups[group].func[0].pin_count; i++)
		p->gpio[p->groups[group].func[0].pins[i]] = 1;

	/* function 0 is gpio and needs special handling */
	if (func == 0) {
		mode |= p->groups[group].gpio << shift;
	} else {
		for (i = 0; i < p->func[func]->pin_count; i++)
			p->gpio[p->func[func]->pins[i]] = 0;
		mode |= p->func[func]->value << shift;
	}
	rt_sysc_w32(mode, reg);


Ref:
MT7620 pa group switches between PA and GPIO functions during wireless calibration.
https://github.com/openwrt/openwrt/blob/main/package/kernel/mac80211/patches/rt2x00/994-rt2x00-import-support-for-external-LNA-on-MT7620.patch

Regards,
Shiji Yang