[PATCH 0/4] gpio: generic: add option to read output value from set register

[PATCH 0/4] gpio: generic: add option to read output value from set register

[Vladimir Zapolskiy]

The changeset introduces BGPIOF_READ_OUTPUT_REG_SET flag to
bgpio_init() function, which is if provided allows to get
stored value of output GPIO line in "set" register instead
of default "data" register.

The same functionality is already implemented in moxart driver by
a private function, move it to gpio-generic.c since at least iMX
GPIO controllers require it as well.

Fixes a problem on iMX platforms:

  % echo out > /sys/class/gpio/gpio9/direction
  % cat /sys/class/gpio/gpio9/value
  0
  % echo 1 > /sys/class/gpio/gpio9/value
  % cat /sys/class/gpio/gpio9/value
  0

Vladimir Zapolskiy (4):
  Revert "gpio: generic: clamp retured value to [0,1]"
  gpio: gpio-generic: add flag to read out output value from reg_set
  gpio: moxart: get value of output gpio from generic driver
  gpio: mxc: read output value from GPIO_DR register

 drivers/gpio/gpio-generic.c     |   24 ++++++++++++++++++++----
 drivers/gpio/gpio-moxart.c      |   17 +++--------------
 drivers/gpio/gpio-mxc.c         |    3 ++-
 include/linux/basic_mmio_gpio.h |    1 +
 4 files changed, 26 insertions(+), 19 deletions(-)

-- 
1.7.10.4

[PATCH 1/4] Revert "gpio: generic: clamp retured value to [0,1]"

[Vladimir Zapolskiy]

This reverts commit 25b35da7f4cce82271859f1b6eabd9f3bd41a2bb.

The original change is a fast workaround in GPIO generic driver,
which is properly fixed by Alexandre's 23600969ff centralized handling
of return values from GPIO chip drivers.

To avoid a redundant check and copy/paste confusion, it is better to
revert the change done in a particular driver.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/gpio/gpio-generic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index b92a690..3f90be8 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -139,7 +139,7 @@ static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
 
-	return !!(bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio));
+	return bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio);
 }
 
 static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
-- 
1.7.10.4

Re: [PATCH 1/4] Revert "gpio: generic: clamp retured value to [0,1]"

[Linus Walleij]

On Wed, Apr 29, 2015 at 5:34 PM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:

> This reverts commit 25b35da7f4cce82271859f1b6eabd9f3bd41a2bb.
>
> The original change is a fast workaround in GPIO generic driver,
> which is properly fixed by Alexandre's 23600969ff centralized handling
> of return values from GPIO chip drivers.
>
> To avoid a redundant check and copy/paste confusion, it is better to
> revert the change done in a particular driver.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

I'm gonna drop this patch, because the code is helpful like this,
clear to see what is going on.

In the long run I want to change the signature of the get/set functions
to take bools because that is what they actually handle.

Yours,
Linus Walleij

Re: [PATCH 1/4] Revert "gpio: generic: clamp retured value to [0,1]"

[Vladimir Zapolskiy]

Hi Linus,

On 11.05.2015 12:46, Linus Walleij wrote:
> On Wed, Apr 29, 2015 at 5:34 PM, Vladimir Zapolskiy
> <vladimir_zapolskiy@mentor.com> wrote:
> 
>> This reverts commit 25b35da7f4cce82271859f1b6eabd9f3bd41a2bb.
>>
>> The original change is a fast workaround in GPIO generic driver,
>> which is properly fixed by Alexandre's 23600969ff centralized handling
>> of return values from GPIO chip drivers.
>>
>> To avoid a redundant check and copy/paste confusion, it is better to
>> revert the change done in a particular driver.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> 
> I'm gonna drop this patch, because the code is helpful like this,
> clear to see what is going on.
> 
> In the long run I want to change the signature of the get/set functions
> to take bools because that is what they actually handle.

thank you for review.

In my opinion (and according to my tests of course) it is safe to apply
this revert commit, also the commit is anyway needed, when you switch to
bool return values.

I understand your wish to keep this code as a hint for the planned
updates in the future, if price of the dead code plus small deviation
between bgpio_get() and bgpio_get_set() is less than price of the hint
in your opinion, I have no objections to skip this patch.

--
With best wishes,
Vladimir

Re: [PATCH 1/4] Revert "gpio: generic: clamp retured value to [0,1]"

[Alexandre Courbot]

Hi Vladimir,

On Mon, May 11, 2015 at 10:12 PM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:
> Hi Linus,
>
> On 11.05.2015 12:46, Linus Walleij wrote:
>> On Wed, Apr 29, 2015 at 5:34 PM, Vladimir Zapolskiy
>> <vladimir_zapolskiy@mentor.com> wrote:
>>
>>> This reverts commit 25b35da7f4cce82271859f1b6eabd9f3bd41a2bb.
>>>
>>> The original change is a fast workaround in GPIO generic driver,
>>> which is properly fixed by Alexandre's 23600969ff centralized handling
>>> of return values from GPIO chip drivers.
>>>
>>> To avoid a redundant check and copy/paste confusion, it is better to
>>> revert the change done in a particular driver.
>>>
>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>
>> I'm gonna drop this patch, because the code is helpful like this,
>> clear to see what is going on.
>>
>> In the long run I want to change the signature of the get/set functions
>> to take bools because that is what they actually handle.
>
> thank you for review.
>
> In my opinion (and according to my tests of course) it is safe to apply
> this revert commit, also the commit is anyway needed, when you switch to
> bool return values.
>
> I understand your wish to keep this code as a hint for the planned
> updates in the future, if price of the dead code plus small deviation
> between bgpio_get() and bgpio_get_set() is less than price of the hint
> in your opinion, I have no objections to skip this patch.

I'd also agree with Linus that we could live without this patch until
we switch the functions to return bools.

Actually, with implicit type conversion doing most of the job, I
wonder if it would be that hard to do - maybe you will want to give it
a quick try?

Re: [PATCH 1/4] Revert "gpio: generic: clamp retured value to [0,1]"

[Vladimir Zapolskiy]

Hi Alexandre,

On 12.05.2015 10:45, Alexandre Courbot wrote:
> Hi Vladimir,
> 
> On Mon, May 11, 2015 at 10:12 PM, Vladimir Zapolskiy
> <vladimir_zapolskiy@mentor.com> wrote:
>> Hi Linus,
>>
>> On 11.05.2015 12:46, Linus Walleij wrote:
>>> On Wed, Apr 29, 2015 at 5:34 PM, Vladimir Zapolskiy
>>> <vladimir_zapolskiy@mentor.com> wrote:
>>>
>>>> This reverts commit 25b35da7f4cce82271859f1b6eabd9f3bd41a2bb.
>>>>
>>>> The original change is a fast workaround in GPIO generic driver,
>>>> which is properly fixed by Alexandre's 23600969ff centralized handling
>>>> of return values from GPIO chip drivers.
>>>>
>>>> To avoid a redundant check and copy/paste confusion, it is better to
>>>> revert the change done in a particular driver.
>>>>
>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>>
>>> I'm gonna drop this patch, because the code is helpful like this,
>>> clear to see what is going on.
>>>
>>> In the long run I want to change the signature of the get/set functions
>>> to take bools because that is what they actually handle.
>>
>> thank you for review.
>>
>> In my opinion (and according to my tests of course) it is safe to apply
>> this revert commit, also the commit is anyway needed, when you switch to
>> bool return values.
>>
>> I understand your wish to keep this code as a hint for the planned
>> updates in the future, if price of the dead code plus small deviation
>> between bgpio_get() and bgpio_get_set() is less than price of the hint
>> in your opinion, I have no objections to skip this patch.
> 
> I'd also agree with Linus that we could live without this patch until
> we switch the functions to return bools.
> 
> Actually, with implicit type conversion doing most of the job, I
> wonder if it would be that hard to do - maybe you will want to give it
> a quick try?
> 

it should be quite a simple and straightforward change, I'll prepare
patches for review, thank you for offering the task.

--
With best wishes,
Vladimir

Re: [PATCH 1/4] Revert "gpio: generic: clamp retured value to [0,1]"

[Linus Walleij]

On Tue, May 12, 2015 at 9:54 AM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:
> On 12.05.2015 10:45, Alexandre Courbot wrote:

>> Actually, with implicit type conversion doing most of the job, I
>> wonder if it would be that hard to do - maybe you will want to give it
>> a quick try?
>
> it should be quite a simple and straightforward change, I'll prepare
> patches for review, thank you for offering the task.

Awesome, thanks for proposing this to be done guys.

Yours,
Linus Walleij

[PATCH 2/4] gpio: gpio-generic: add flag to read out output value from reg_set

[Vladimir Zapolskiy]

The change introduces BGPIOF_READ_OUTPUT_REG_SET flag for gpio-generic
GPIO chip implementation, which allows to get correct configured value
from reg_set register, input value is still get from reg_dat.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/gpio/gpio-generic.c     |   22 +++++++++++++++++++---
 include/linux/basic_mmio_gpio.h |    1 +
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index 3f90be8..83258b6 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -135,6 +135,17 @@ static unsigned long bgpio_pin2mask_be(struct bgpio_chip *bgc,
 	return 1 << (bgc->bits - 1 - pin);
 }
 
+static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned long pinmask = bgc->pin2mask(bgc, gpio);
+
+	if (bgc->dir & pinmask)
+		return bgc->read_reg(bgc->reg_set) & pinmask;
+	else
+		return bgc->read_reg(bgc->reg_dat) & pinmask;
+}
+
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
@@ -416,7 +427,8 @@ static int bgpio_setup_accessors(struct device *dev,
 static int bgpio_setup_io(struct bgpio_chip *bgc,
 			  void __iomem *dat,
 			  void __iomem *set,
-			  void __iomem *clr)
+			  void __iomem *clr,
+			  unsigned long flags)
 {
 
 	bgc->reg_dat = dat;
@@ -437,7 +449,11 @@ static int bgpio_setup_io(struct bgpio_chip *bgc,
 		bgc->gc.set_multiple = bgpio_set_multiple;
 	}
 
-	bgc->gc.get = bgpio_get;
+	if (!(flags & BGPIOF_UNREADABLE_REG_SET) &&
+	    (flags & BGPIOF_READ_OUTPUT_REG_SET))
+		bgc->gc.get = bgpio_get_set;
+	else
+		bgc->gc.get = bgpio_get;
 
 	return 0;
 }
@@ -500,7 +516,7 @@ int bgpio_init(struct bgpio_chip *bgc, struct device *dev,
 	bgc->gc.ngpio = bgc->bits;
 	bgc->gc.request = bgpio_request;
 
-	ret = bgpio_setup_io(bgc, dat, set, clr);
+	ret = bgpio_setup_io(bgc, dat, set, clr, flags);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/basic_mmio_gpio.h b/include/linux/basic_mmio_gpio.h
index 0e97856..14eea94 100644
--- a/include/linux/basic_mmio_gpio.h
+++ b/include/linux/basic_mmio_gpio.h
@@ -74,5 +74,6 @@ int bgpio_init(struct bgpio_chip *bgc, struct device *dev,
 #define BGPIOF_UNREADABLE_REG_SET	BIT(1) /* reg_set is unreadable */
 #define BGPIOF_UNREADABLE_REG_DIR	BIT(2) /* reg_dir is unreadable */
 #define BGPIOF_BIG_ENDIAN_BYTE_ORDER	BIT(3)
+#define BGPIOF_READ_OUTPUT_REG_SET     BIT(4) /* reg_set stores output value */
 
 #endif /* __BASIC_MMIO_GPIO_H */
-- 
1.7.10.4

Re: [PATCH 2/4] gpio: gpio-generic: add flag to read out output value from reg_set

[Linus Walleij]

On Wed, Apr 29, 2015 at 5:34 PM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:

> The change introduces BGPIOF_READ_OUTPUT_REG_SET flag for gpio-generic
> GPIO chip implementation, which allows to get correct configured value
> from reg_set register, input value is still get from reg_dat.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

Looks reasonable. Patch applied.

Do we need device tree bindings for this?

Yours,
Linus Walleij

Re: [PATCH 2/4] gpio: gpio-generic: add flag to read out output value from reg_set

[Vladimir Zapolskiy]

On 11.05.2015 12:50, Linus Walleij wrote:
> On Wed, Apr 29, 2015 at 5:34 PM, Vladimir Zapolskiy
> <vladimir_zapolskiy@mentor.com> wrote:
> 
>> The change introduces BGPIOF_READ_OUTPUT_REG_SET flag for gpio-generic
>> GPIO chip implementation, which allows to get correct configured value
>> from reg_set register, input value is still get from reg_dat.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> 
> Looks reasonable. Patch applied.
> 
> Do we need device tree bindings for this?

I don't think so, particular drivers have this as a fixed property, and
"basic-mmio-gpio" has no device tree bindings.

OTOH let see on dynamics of the flag usage, if it grows, it may happen
that its negation is a better candidate for a quirk.

--
With best wishes,
Vladimir

[PATCH 3/4] gpio: moxart: get value of output gpio from generic driver

[Vladimir Zapolskiy]

Adding a BGPIOF_READ_OUTPUT_REG_SET initialization flag to GPIO
generic MMIO driver makes possible to remove a private get() value
function from the driver.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/gpio/gpio-moxart.c |   17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-moxart.c b/drivers/gpio/gpio-moxart.c
index c3ab46e..abd8676 100644
--- a/drivers/gpio/gpio-moxart.c
+++ b/drivers/gpio/gpio-moxart.c
@@ -39,17 +39,6 @@ static void moxart_gpio_free(struct gpio_chip *chip, unsigned offset)
 	pinctrl_free_gpio(offset);
 }
 
-static int moxart_gpio_get(struct gpio_chip *chip, unsigned offset)
-{
-	struct bgpio_chip *bgc = to_bgpio_chip(chip);
-	u32 ret = bgc->read_reg(bgc->reg_dir);
-
-	if (ret & BIT(offset))
-		return !!(bgc->read_reg(bgc->reg_set) & BIT(offset));
-	else
-		return !!(bgc->read_reg(bgc->reg_dat) & BIT(offset));
-}
-
 static int moxart_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -68,8 +57,9 @@ static int moxart_gpio_probe(struct platform_device *pdev)
 		return PTR_ERR(base);
 
 	ret = bgpio_init(bgc, dev, 4, base + GPIO_DATA_IN,
-		    base + GPIO_DATA_OUT, NULL,
-		    base + GPIO_PIN_DIRECTION, NULL, 0);
+			 base + GPIO_DATA_OUT, NULL,
+			 base + GPIO_PIN_DIRECTION, NULL,
+			 BGPIOF_READ_OUTPUT_REG_SET);
 	if (ret) {
 		dev_err(&pdev->dev, "bgpio_init failed\n");
 		return ret;
@@ -78,7 +68,6 @@ static int moxart_gpio_probe(struct platform_device *pdev)
 	bgc->gc.label = "moxart-gpio";
 	bgc->gc.request = moxart_gpio_request;
 	bgc->gc.free = moxart_gpio_free;
-	bgc->gc.get = moxart_gpio_get;
 	bgc->data = bgc->read_reg(bgc->reg_set);
 	bgc->gc.base = 0;
 	bgc->gc.ngpio = 32;
-- 
1.7.10.4

Re: [PATCH 3/4] gpio: moxart: get value of output gpio from generic driver

[Linus Walleij]

On Wed, Apr 29, 2015 at 5:35 PM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:

> Adding a BGPIOF_READ_OUTPUT_REG_SET initialization flag to GPIO
> generic MMIO driver makes possible to remove a private get() value
> function from the driver.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

Patch applied, unless Jonas has a problem with it.
Always CC: Jonas Jensen whenever patching Moxart stuff pls.

Yours,
Linus Walleij

Re: [PATCH 3/4] gpio: moxart: get value of output gpio from generic driver

[Jonas Jensen]

On 11 May 2015 at 11:52, Linus Walleij <linus.walleij@linaro.org> wrote:
> Patch applied, unless Jonas has a problem with it.
> Always CC: Jonas Jensen whenever patching Moxart stuff pls.

Thanks!

I just figured out I never completed my linux-gpio subscription though
I would have found this eventually.

Unfortunately my tree is not rebased since 3.17 so merged manually and
tested (verified OK on UC-7112-LX):

https://bitbucket.org/Kasreyn/linux-next/commits/5da653347ed802f5cdfd785d1dbaab2aa7432c1d


   Jonas

Re: [PATCH 3/4] gpio: moxart: get value of output gpio from generic driver

[Vladimir Zapolskiy]

Hi Jonas, Linus,

On 11.05.2015 15:17, Jonas Jensen wrote:
> On 11 May 2015 at 11:52, Linus Walleij <linus.walleij@linaro.org> wrote:
>> Patch applied, unless Jonas has a problem with it.
>> Always CC: Jonas Jensen whenever patching Moxart stuff pls.

will do, I got a list of addressees using scripts/get_maintainer.pl,
probably updating MAINTAINERS record may be a good option here.

> Thanks!
> 
> I just figured out I never completed my linux-gpio subscription though
> I would have found this eventually.
> 
> Unfortunately my tree is not rebased since 3.17 so merged manually and
> tested (verified OK on UC-7112-LX):
> 
> https://bitbucket.org/Kasreyn/linux-next/commits/5da653347ed802f5cdfd785d1dbaab2aa7432c1d
> 
> 
>    Jonas
> 

--
With best wishes,
Vladimir

[PATCH 4/4] gpio: mxc: read output value from GPIO_DR register

[Vladimir Zapolskiy]

All supported iMX GPIO controllers store configured GPIO output value
in GPIO_DR data register, which is represented by GPIO generic reg_set.
Provide a BGPIOF_READ_OUTPUT_REG_SET flag to bgpio_init() to allow
correct getting of previously set output value.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/gpio/gpio-mxc.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 9f7446a..38daa64 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -450,7 +450,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	err = bgpio_init(&port->bgc, &pdev->dev, 4,
 			 port->base + GPIO_PSR,
 			 port->base + GPIO_DR, NULL,
-			 port->base + GPIO_GDIR, NULL, 0);
+			 port->base + GPIO_GDIR, NULL,
+			 BGPIOF_READ_OUTPUT_REG_SET);
 	if (err)
 		goto out_bgio;
 
-- 
1.7.10.4

Re: [PATCH 4/4] gpio: mxc: read output value from GPIO_DR register

[Linus Walleij]

On Wed, Apr 29, 2015 at 5:35 PM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:

> All supported iMX GPIO controllers store configured GPIO output value
> in GPIO_DR data register, which is represented by GPIO generic reg_set.
> Provide a BGPIOF_READ_OUTPUT_REG_SET flag to bgpio_init() to allow
> correct getting of previously set output value.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

Patch applied, unless Shawn has some issue with it.
Add Shawn to CC when patching this driver.

Yours,
Linus Walleij