RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

["A.S. Dong" <aisheng.dong@xxxxxxx>]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan@xxxxxxxx]
> Sent: Tuesday, May 16, 2017 1:27 AM
> To: A.S. Dong
> Cc: linux-gpio@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linus.walleij@xxxxxxxxxx; shawnguo@xxxxxxxxxx; Jacky Bai; Andy Duan;
> kernel@xxxxxxxxxxxxxx; Alexandre Courbot
> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
> 
> On 2017-05-14 23:48, Dong Aisheng wrote:
> > Do not assume MUX 0 is GPIO function in core driver as a different SoC
> > may have different value. e.g. MX7ULP Mux 1 is GPIO.
> >
> > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > Cc: Alexandre Courbot <gnurou@xxxxxxxxx>
> > Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
> > Cc: Stefan Agner <stefan@xxxxxxxx>
> > Cc: Fugang Duan <fugang.duan@xxxxxxx>
> > Cc: Bai Ping <ping.bai@xxxxxxx>
> > Signed-off-by: Dong Aisheng <aisheng.dong@xxxxxxx>
> > ---
> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> > index 0d6aaca..ed8ea32 100644
> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> > @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
> > pinctrl_dev *pctldev,
> >  			continue;
> >  		for (pin = 0; pin < grp->num_pins; pin++) {
> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
> > +			if (imx_pin->pin == offset)
> >  				goto mux_pin;
> 
> The reason I added that check was to make sure we pick a mux option which
> is GPIO... With this change, any pinmux might be picked...
> 

First of all, this seems to be wrong to me that GPIO mux mode is SoC
Dependant and should not be put in pinctrl-imx core driver.

Secondly, I think we may be over worried and it's not quite necessary
As we did not do the sanity check for both raw config and mux data read
>From Device tree, why only do it for GPIO?

We may trust the data in device tree.

> >  		}
> >  	}
> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
> > pinctrl_dev *pctldev,
> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> >  	reg &= ~info->mux_mask;
> >  	reg |= imx_pin->config;
> > +	reg |= imx_pin->mux_mode << info->mux_shift;
> 
> ... and muxed...
> 
> Not sure if we want that.
> 
> I had to control GPIO output/input through pinctrl since Vybrid does not
> allow to control that from the GPIO block.
> 
> However, according to your GPIO patchset, the i.MX 7ULP has a new register
> GPIO_PDDR to control output from the GPIO block. Is controlling the output
> driver from IOMUXC still required? 

Yes, it's still required.

> If not, I really would just not use all
> that "find pinctrl config" hackery... e.g. add a new flag,
> USE_IOMUXC_FOR_GPIO_OUTPUT, and set that only for Vybrid.
> 
> This would also align much better with the other i.MX devices, where
> pinmuxing and GPIO is completely orthogonal.
> 

Actually this patch came only because to make the exist code
not break MX7ULP.

Actually I'm wondering why we need implement 
imx_pmx_gpio_request_enable function?

Per my understanding, IMX binding already set GPIO mux by
Parsing MUX mode from device tree, so why need gpio_request_enable
which looks like is duplicated.

Can you help explain it?

Regards
Dong Aisheng

> --
> Stefan
> 
> >  	writel(reg, ipctl->base + pin_reg->mux_reg);
> >
> >  	return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html