[PATCH 1/1] iMX gpio: Allow reading back of pin status if configured as gpio output

[PATCH 1/1] iMX gpio: Allow reading back of pin status if configured as gpio output

[Waibel Georg]

>From cb384950a1153e856ec03109a5156e660a89bf6d Mon Sep 17 00:00:00 2001
From: Georg Waibel <georg.waibel@sensor-technik.de>
Date: Fri, 17 Jan 2014 14:51:38 +0100
Subject: [PATCH 1/1] iMX gpio: Allow reading back of pin status if configured
 as gpio output

Register PSR was used to read the pin status in the mxc_gpio driver. This
register reflects the pin status if a pin is configured as gpio input.
If a pin is configured as an gpio output register PSR is not updated and
returns 0 instead of the actual pin status. Thus attempting to read back the
status of an gpio output pin via PSR returns a wrong value.

Reading register DR instead of PSR fixes this issue:
- If pin is gpio output: DR returns the value written to DR by software
- If pin is gpio input: DR returns the value of register PSR und thus the
  pin status

This behaviour is valid for all processors >= iMX31. Verified on iMX6q.
See the iMX reference manuals gpio section for details.

Signed-off-by: Georg Waibel <georg.waibel@sensor-technik.de>
---
 drivers/gpio/gpio-mxc.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 3307f6d..3717e09 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -407,6 +407,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	struct resource *iores;
 	int irq_base;
 	int err;
+	int dat;
 
 	mxc_gpio_get_hw(pdev);
 
@@ -447,8 +448,9 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 		}
 	}
 
+	dat = (mxc_gpio_hwtype == IMX21_GPIO) ? GPIO_PSR : GPIO_DR;
 	err = bgpio_init(&port->bgc, &pdev->dev, 4,
-			 port->base + GPIO_PSR,
+			 port->base + dat,
 			 port->base + GPIO_DR, NULL,
 			 port->base + GPIO_GDIR, NULL, 0);
 	if (err)
-- 
1.7.9.5

Re: [PATCH 1/1] iMX gpio: Allow reading back of pin status if configured as gpio output

[Gerhard Sittig]

On Fri, Jan 17, 2014 at 14:11 +0000, Waibel Georg wrote:
> 
> From cb384950a1153e856ec03109a5156e660a89bf6d Mon Sep 17 00:00:00 2001
> From: Georg Waibel <georg.waibel@sensor-technik.de>
> Date: Fri, 17 Jan 2014 14:51:38 +0100
> Subject: [PATCH 1/1] iMX gpio: Allow reading back of pin status if configured
>  as gpio output
> 
> Register PSR was used to read the pin status in the mxc_gpio driver. This
> register reflects the pin status if a pin is configured as gpio input.
> If a pin is configured as an gpio output register PSR is not updated and
> returns 0 instead of the actual pin status. Thus attempting to read back the
> status of an gpio output pin via PSR returns a wrong value.
> 
> Reading register DR instead of PSR fixes this issue:
> - If pin is gpio output: DR returns the value written to DR by software
> - If pin is gpio input: DR returns the value of register PSR und thus the
>   pin status

Got curious because of your specific wording.  In the output
case, does the DR value reflect what you drive to the pin, or
what the pin's status is?  Because this need not be identical in
the open collector case (or other "weak" scenarios like bus
keeper, and what else HW development came up with).

If DR always reflects the pin's status, then the patch would be
OK but the commit message would need an update.  If DR does not
appropriately reflect the pin's status, then the patch would be
an improvement (would fix the totem-pole case), but still would
be wrong or incomplete in the open-collector case.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

AW: [PATCH 1/1] iMX gpio: Allow reading back of pin status if configured as gpio output

[Waibel Georg]

> On Fri, Jan 17, 2014 at 14:11 +0000, Waibel Georg wrote:
> >
> > From cb384950a1153e856ec03109a5156e660a89bf6d Mon Sep 17 00:00:00 2001
> > From: Georg Waibel <georg.waibel@sensor-technik.de>
> > Date: Fri, 17 Jan 2014 14:51:38 +0100
> > Subject: [PATCH 1/1] iMX gpio: Allow reading back of pin status if configured
> >  as gpio output
> >
> > Register PSR was used to read the pin status in the mxc_gpio driver. This
> > register reflects the pin status if a pin is configured as gpio input.
> > If a pin is configured as an gpio output register PSR is not updated and
> > returns 0 instead of the actual pin status. Thus attempting to read back the
> > status of an gpio output pin via PSR returns a wrong value.
> >
> > Reading register DR instead of PSR fixes this issue:
> > - If pin is gpio output: DR returns the value written to DR by software
> > - If pin is gpio input: DR returns the value of register PSR und thus the
> >   pin status
> 
> Got curious because of your specific wording.  In the output
> case, does the DR value reflect what you drive to the pin, or
> what the pin's status is?  Because this need not be identical in
> the open collector case (or other "weak" scenarios like bus
> keeper, and what else HW development came up with).
> 
> If DR always reflects the pin's status, then the patch would be
> OK but the commit message would need an update.  If DR does not
> appropriately reflect the pin's status, then the patch would be
> an improvement (would fix the totem-pole case), but still would
> be wrong or incomplete in the open-collector case.


Hi Gerhard,

actually I did not take the open-collector case into account. 
I checked the iMX6q reference manual and came to this conclusion:

In the output case, reading register DR returns the value I have written 
to DR, not the actual pad state. Thus may patch only works for the 
push-pull (totem-pole) output case, not for open-drain.

However, there is a solution to read back the pin status regardless of 
its input or output configuration. Its the SION (Software Input On Bit)
in the IOMUXC MUX control register. If SION is set, the pad status can be 
read back through register PSR. Thats exactly what I intended to do. 
Bit SION can be set in the device tree (IOMUXC: CONFIG filed in the 
fsl,pins binding, bit 30).
I will verify this on my iMX6 hardware next week.

Seems there is no need for my patch at all.

Thanks for your hint.
Regards
Georg

Re: [PATCH 1/1] iMX gpio: Allow reading back of pin status if configured as gpio output

[Linus Walleij]

On Fri, Jan 17, 2014 at 11:27 PM, Waibel Georg
<Georg.Waibel@sensor-technik.de> wrote:

> Seems there is no need for my patch at all.

I'm holding this off until you've made up your mind about
whether it's needed or not...

Yours,
Linus Walleij

AW: [PATCH 1/1] iMX gpio: Allow reading back of pin status if configured as gpio output

[Waibel Georg]

On Thu, Jan 23, 2014 at 08:40 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> I'm holding this off until you've made up your mind about whether it's needed or not...

By setting the SION bit the actual status of a gpio pin can be read back regardless of its driver configuration. 
You can drop my patch as there is no need for it.

Regards
Georg