[PATCH v2 0/3] gpio: intel_gpio: Fix Intel gpio driver

[PATCH v2 0/3] gpio: intel_gpio: Fix Intel gpio driver

[Wolfgang Wallner]

This series fixes some issues in the Intel gpio driver.
I have tested it on an Apollo Lake device, where U-Boot is booted as a
coreboot payload.

Changes in v2:
- Fixed typo in the commit description
- Fixed the same error in both intel_gpio_direction_output() and
intel_gpio_set_value() (Thanks to Bin Meng for catching this)
- Reworded commit description
- Added reviewed-by tags

Wolfgang Wallner (3):
  gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()
  gpio: intel_gpio: Clear tx state bit when setting output
  gpio: intel_gpio: Fix register/bit offsets intel_gpio_get_value()

 drivers/gpio/intel_gpio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.25.0

[PATCH v2 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()

[Wolfgang Wallner]

The function pcr_clrsetbits32() expects a device with a P2SB parent
device. In intel_gpio_direction_output() and intel_gpio_set_value()
the device 'dev' is passed to pcr_clrsetbits32(), which is a
gpio-controller with a device 'pinctrl' as parent. This does not match
the expectations of pcr_clrsetbits32(). But the 'pinctrl' device has a
P2SB as parent.

Pass the 'pinctrl' device instead of the 'dev' device to
pcr_clrsetbits32().

Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>

---

Changes in v2:
- Fixed typo in the commit description
- Fixed the same error in both intel_gpio_direction_output() and
intel_gpio_set_value() (Thanks to Bin Meng for catching this)
- Reworded commit description

 drivers/gpio/intel_gpio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
index 4bf1c9ddc4..b05cfc4aed 100644
--- a/drivers/gpio/intel_gpio.c
+++ b/drivers/gpio/intel_gpio.c
@@ -39,7 +39,7 @@ static int intel_gpio_direction_output(struct udevice *dev, uint offset,
 	struct udevice *pinctrl = dev_get_parent(dev);
 	uint config_offset = intel_pinctrl_get_config_reg_addr(pinctrl, offset);
 
-	pcr_clrsetbits32(dev, config_offset,
+	pcr_clrsetbits32(pinctrl, config_offset,
 			 PAD_CFG0_MODE_MASK | PAD_CFG0_RX_STATE |
 				  PAD_CFG0_TX_DISABLE,
 			 PAD_CFG0_MODE_GPIO | PAD_CFG0_RX_DISABLE |
@@ -72,7 +72,7 @@ static int intel_gpio_set_value(struct udevice *dev, unsigned offset, int value)
 	struct udevice *pinctrl = dev_get_parent(dev);
 	uint config_offset = intel_pinctrl_get_config_reg_addr(pinctrl, offset);
 
-	pcr_clrsetbits32(dev, config_offset, PAD_CFG0_TX_STATE,
+	pcr_clrsetbits32(pinctrl, config_offset, PAD_CFG0_TX_STATE,
 			 value ? PAD_CFG0_TX_STATE : 0);
 
 	return 0;
-- 
2.25.0

[PATCH v2 2/3] gpio: intel_gpio: Clear tx state bit when setting output

[Wolfgang Wallner]

Add missing 'PAD_CFG0_TX_STATE' to the clear mask for pcr_clrsetbits32().
Otherwise this bit cannot be cleared again after it has been set once.

Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2:
- Added reviewed-by tags

 drivers/gpio/intel_gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
index b05cfc4aed..be91626cc5 100644
--- a/drivers/gpio/intel_gpio.c
+++ b/drivers/gpio/intel_gpio.c
@@ -41,7 +41,7 @@ static int intel_gpio_direction_output(struct udevice *dev, uint offset,
 
 	pcr_clrsetbits32(pinctrl, config_offset,
 			 PAD_CFG0_MODE_MASK | PAD_CFG0_RX_STATE |
-				  PAD_CFG0_TX_DISABLE,
+				  PAD_CFG0_TX_DISABLE | PAD_CFG0_TX_STATE,
 			 PAD_CFG0_MODE_GPIO | PAD_CFG0_RX_DISABLE |
 				  (value ? PAD_CFG0_TX_STATE : 0));
 
-- 
2.25.0

[PATCH v2 3/3] gpio: intel_gpio: Fix register/bit offsets intel_gpio_get_value()

[Wolfgang Wallner]

Fix the following in intel_gpio_get_value():

 * The value of the register is contained in the variable 'reg', not in
   'mode'. The variable 'mode' contains only the configuration whether
   the gpio is currently an input or an output.

 * The correct bitmasks for the input and output value are
   PAD_CFG0_RX_STATE and PAD_CFG0_TX_STATE.
   Use them instead of the currently used PAD_CFG0_RX_STATE_BIT and
   PAD_CFG0_TX_STATE_BIT.

Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2: None

 drivers/gpio/intel_gpio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
index be91626cc5..67b8b80b9d 100644
--- a/drivers/gpio/intel_gpio.c
+++ b/drivers/gpio/intel_gpio.c
@@ -59,9 +59,9 @@ static int intel_gpio_get_value(struct udevice *dev, uint offset)
 	if (!mode) {
 		rx_tx = reg & (PAD_CFG0_TX_DISABLE | PAD_CFG0_RX_DISABLE);
 		if (rx_tx == PAD_CFG0_TX_DISABLE)
-			return mode & PAD_CFG0_RX_STATE_BIT ? 1 : 0;
+			return reg & PAD_CFG0_RX_STATE ? 1 : 0;
 		else if (rx_tx == PAD_CFG0_RX_DISABLE)
-			return mode & PAD_CFG0_TX_STATE_BIT ? 1 : 0;
+			return reg & PAD_CFG0_TX_STATE ? 1 : 0;
 	}
 
 	return 0;
-- 
2.25.0

[PATCH v2 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()

[Bin Meng]

On Mon, Feb 3, 2020 at 6:38 PM Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> The function pcr_clrsetbits32() expects a device with a P2SB parent
> device. In intel_gpio_direction_output() and intel_gpio_set_value()
> the device 'dev' is passed to pcr_clrsetbits32(), which is a
> gpio-controller with a device 'pinctrl' as parent. This does not match
> the expectations of pcr_clrsetbits32(). But the 'pinctrl' device has a
> P2SB as parent.
>
> Pass the 'pinctrl' device instead of the 'dev' device to
> pcr_clrsetbits32().
>
> Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
>
> ---
>
> Changes in v2:
> - Fixed typo in the commit description
> - Fixed the same error in both intel_gpio_direction_output() and
> intel_gpio_set_value() (Thanks to Bin Meng for catching this)
> - Reworded commit description
>
>  drivers/gpio/intel_gpio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH v2 3/3] gpio: intel_gpio: Fix register/bit offsets intel_gpio_get_value()

[Andy Shevchenko]

On Mon, Feb 3, 2020 at 12:38 PM Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> Fix the following in intel_gpio_get_value():
>
>  * The value of the register is contained in the variable 'reg', not in
>    'mode'. The variable 'mode' contains only the configuration whether
>    the gpio is currently an input or an output.
>
>  * The correct bitmasks for the input and output value are
>    PAD_CFG0_RX_STATE and PAD_CFG0_TX_STATE.
>    Use them instead of the currently used PAD_CFG0_RX_STATE_BIT and
>    PAD_CFG0_TX_STATE_BIT.

...

>         if (!mode) {
>                 rx_tx = reg & (PAD_CFG0_TX_DISABLE | PAD_CFG0_RX_DISABLE);
>                 if (rx_tx == PAD_CFG0_TX_DISABLE)
> -                       return mode & PAD_CFG0_RX_STATE_BIT ? 1 : 0;
> +                       return reg & PAD_CFG0_RX_STATE ? 1 : 0;

Is it style of U-Boot? Because
return !!(...); will have same effect while consuming less characters.

>                 else if (rx_tx == PAD_CFG0_RX_DISABLE)

'else' is redundant here

> -                       return mode & PAD_CFG0_TX_STATE_BIT ? 1 : 0;
> +                       return reg & PAD_CFG0_TX_STATE ? 1 : 0;
>         }


-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 0/3] gpio: intel_gpio: Fix Intel gpio driver

[Andy Shevchenko]

On Mon, Feb 3, 2020 at 12:38 PM Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
>
> This series fixes some issues in the Intel gpio driver.
> I have tested it on an Apollo Lake device, where U-Boot is booted as a
> coreboot payload.
>

When you send series, be sure you put all maintainers (not me, but
like Bin) to *all* patches in it.

> Changes in v2:
> - Fixed typo in the commit description
> - Fixed the same error in both intel_gpio_direction_output() and
> intel_gpio_set_value() (Thanks to Bin Meng for catching this)
> - Reworded commit description
> - Added reviewed-by tags
>
> Wolfgang Wallner (3):
>   gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()
>   gpio: intel_gpio: Clear tx state bit when setting output
>   gpio: intel_gpio: Fix register/bit offsets intel_gpio_get_value()
>
>  drivers/gpio/intel_gpio.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> --
> 2.25.0
>
>


-- 
With Best Regards,
Andy Shevchenko

Antwort: Re: [PATCH v2 0/3] gpio: intel_gpio: Fix Intel gpio driver

[Wolfgang Wallner]

Hi Andy,

> -----"Andy Shevchenko" <andy.shevchenko@gmail.com> schrieb: -----
> On Mon, Feb 3, 2020 at 12:38 PM Wolfgang Wallner
> <wolfgang.wallner@br-automation.com> wrote:
> >
> >
> > This series fixes some issues in the Intel gpio driver.
> > I have tested it on an Apollo Lake device, where U-Boot is booted as a
> > coreboot payload.
> >

> When you send series, be sure you put all maintainers (not me, but
> like Bin) to *all* patches in it.

I can't follow you.
I have used the following configuration for patman:

  Series-to: u-boot
  Series-cc: bmeng, sjg

And according to my local mail client all patches in this series have
been sent with this configuration (U-Boot mailing list as receiver,
Bin Meng and Simon Glass as CC).

Does it look different for you?
Or did I miss something (e.g. other maintainers)?

regards, Wolfgang

[PATCH v2 0/3] gpio: intel_gpio: Fix Intel gpio driver

[Andy Shevchenko]

On Mon, Feb 3, 2020 at 3:19 PM Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> Hi Andy,
>
> > -----"Andy Shevchenko" <andy.shevchenko@gmail.com> schrieb: -----
> > On Mon, Feb 3, 2020 at 12:38 PM Wolfgang Wallner
> > <wolfgang.wallner@br-automation.com> wrote:
> > >
> > >
> > > This series fixes some issues in the Intel gpio driver.
> > > I have tested it on an Apollo Lake device, where U-Boot is booted as a
> > > coreboot payload.
> > >
>
> > When you send series, be sure you put all maintainers (not me, but
> > like Bin) to *all* patches in it.
>
> I can't follow you.
> I have used the following configuration for patman:
>
>   Series-to: u-boot
>   Series-cc: bmeng, sjg
>
> And according to my local mail client all patches in this series have
> been sent with this configuration (U-Boot mailing list as receiver,
> Bin Meng and Simon Glass as CC).
>
> Does it look different for you?

Yes, for example for this mail (initial cover letter above) I see only
ML in To and none in Cc.

from: Wolfgang Wallner <wolfgang.wallner@br-automation.com> via lists.denx.de
to: u-boot at lists.denx.de
date: Feb 3, 2020, 12:38 PM
subject: [PATCH v2 0/3] gpio: intel_gpio: Fix Intel gpio driver
mailing list: u-boot at lists.denx.de Filter messages from this mailing list
mailed-by: lists.denx.de
unsubscribe: Unsubscribe from this mailing list
security:  Standard encryption (TLS) Learn more

Are you sure you have Cc filled at the end before sending?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 0/3] gpio: intel_gpio: Fix Intel gpio driver

[Bin Meng]

Hi Andy,

On Mon, Feb 3, 2020 at 10:17 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Feb 3, 2020 at 3:19 PM Wolfgang Wallner
> <wolfgang.wallner@br-automation.com> wrote:
> >
> > Hi Andy,
> >
> > > -----"Andy Shevchenko" <andy.shevchenko@gmail.com> schrieb: -----
> > > On Mon, Feb 3, 2020 at 12:38 PM Wolfgang Wallner
> > > <wolfgang.wallner@br-automation.com> wrote:
> > > >
> > > >
> > > > This series fixes some issues in the Intel gpio driver.
> > > > I have tested it on an Apollo Lake device, where U-Boot is booted as a
> > > > coreboot payload.
> > > >
> >
> > > When you send series, be sure you put all maintainers (not me, but
> > > like Bin) to *all* patches in it.
> >
> > I can't follow you.
> > I have used the following configuration for patman:
> >
> >   Series-to: u-boot
> >   Series-cc: bmeng, sjg
> >
> > And according to my local mail client all patches in this series have
> > been sent with this configuration (U-Boot mailing list as receiver,
> > Bin Meng and Simon Glass as CC).
> >
> > Does it look different for you?
>
> Yes, for example for this mail (initial cover letter above) I see only
> ML in To and none in Cc.
>
> from: Wolfgang Wallner <wolfgang.wallner@br-automation.com> via lists.denx.de
> to: u-boot at lists.denx.de
> date: Feb 3, 2020, 12:38 PM
> subject: [PATCH v2 0/3] gpio: intel_gpio: Fix Intel gpio driver
> mailing list: u-boot at lists.denx.de Filter messages from this mailing list
> mailed-by: lists.denx.de
> unsubscribe: Unsubscribe from this mailing list
> security:  Standard encryption (TLS) Learn more

I can see both Simon and me are cc'ed.

From: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
To: u-boot at lists.denx.de
Cc: Simon Glass <sjg@chromium.org>, Bin Meng <bmeng.cn@gmail.com>,
Wolfgang Wallner <wolfgang.wallner@br-automation.com>
Subject: [PATCH v2 0/3] gpio: intel_gpio: Fix Intel gpio driver
Date: Mon, 3 Feb 2020 11:38:03 +0100
Message-Id: <20200203103806.29445-1-wolfgang.wallner@br-automation.com>

Regards,
Bin

[PATCH v2 0/3] gpio: intel_gpio: Fix Intel gpio driver

[Andy Shevchenko]

On Mon, Feb 3, 2020 at 4:27 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> On Mon, Feb 3, 2020 at 10:17 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Feb 3, 2020 at 3:19 PM Wolfgang Wallner
> > <wolfgang.wallner@br-automation.com> wrote:

...

> > > Does it look different for you?
> >
> > Yes, for example for this mail (initial cover letter above) I see only
> > ML in To and none in Cc.
> >
> > from: Wolfgang Wallner <wolfgang.wallner@br-automation.com> via lists.denx.de
> > to: u-boot at lists.denx.de
> > date: Feb 3, 2020, 12:38 PM
> > subject: [PATCH v2 0/3] gpio: intel_gpio: Fix Intel gpio driver
> > mailing list: u-boot at lists.denx.de Filter messages from this mailing list
> > mailed-by: lists.denx.de
> > unsubscribe: Unsubscribe from this mailing list
> > security:  Standard encryption (TLS) Learn more
>
> I can see both Simon and me are cc'ed.

Okay, that means Gmail is somehow broken. :-(

> From: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> To: u-boot at lists.denx.de
> Cc: Simon Glass <sjg@chromium.org>, Bin Meng <bmeng.cn@gmail.com>,
> Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> Subject: [PATCH v2 0/3] gpio: intel_gpio: Fix Intel gpio driver
> Date: Mon, 3 Feb 2020 11:38:03 +0100
> Message-Id: <20200203103806.29445-1-wolfgang.wallner@br-automation.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()

[Bin Meng]

On Mon, Feb 3, 2020 at 6:48 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Mon, Feb 3, 2020 at 6:38 PM Wolfgang Wallner
> <wolfgang.wallner@br-automation.com> wrote:
> >
> > The function pcr_clrsetbits32() expects a device with a P2SB parent
> > device. In intel_gpio_direction_output() and intel_gpio_set_value()
> > the device 'dev' is passed to pcr_clrsetbits32(), which is a
> > gpio-controller with a device 'pinctrl' as parent. This does not match
> > the expectations of pcr_clrsetbits32(). But the 'pinctrl' device has a
> > P2SB as parent.
> >
> > Pass the 'pinctrl' device instead of the 'dev' device to
> > pcr_clrsetbits32().
> >
> > Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> >
> > ---
> >
> > Changes in v2:
> > - Fixed typo in the commit description
> > - Fixed the same error in both intel_gpio_direction_output() and
> > intel_gpio_set_value() (Thanks to Bin Meng for catching this)
> > - Reworded commit description
> >
> >  drivers/gpio/intel_gpio.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

applied to u-boot-x86, thanks!

[PATCH v2 2/3] gpio: intel_gpio: Clear tx state bit when setting output

[Bin Meng]

On Mon, Feb 3, 2020 at 6:38 PM Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> Add missing 'PAD_CFG0_TX_STATE' to the clear mask for pcr_clrsetbits32().
> Otherwise this bit cannot be cleared again after it has been set once.
>
> Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2:
> - Added reviewed-by tags
>
>  drivers/gpio/intel_gpio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

applied to u-boot-x86, thanks!

[PATCH v2 3/3] gpio: intel_gpio: Fix register/bit offsets intel_gpio_get_value()

[Bin Meng]

Hi Andy,

On Mon, Feb 3, 2020 at 8:34 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Feb 3, 2020 at 12:38 PM Wolfgang Wallner
> <wolfgang.wallner@br-automation.com> wrote:
> >
> > Fix the following in intel_gpio_get_value():
> >
> >  * The value of the register is contained in the variable 'reg', not in
> >    'mode'. The variable 'mode' contains only the configuration whether
> >    the gpio is currently an input or an output.
> >
> >  * The correct bitmasks for the input and output value are
> >    PAD_CFG0_RX_STATE and PAD_CFG0_TX_STATE.
> >    Use them instead of the currently used PAD_CFG0_RX_STATE_BIT and
> >    PAD_CFG0_TX_STATE_BIT.
>
> ...
>
> >         if (!mode) {
> >                 rx_tx = reg & (PAD_CFG0_TX_DISABLE | PAD_CFG0_RX_DISABLE);
> >                 if (rx_tx == PAD_CFG0_TX_DISABLE)
> > -                       return mode & PAD_CFG0_RX_STATE_BIT ? 1 : 0;
> > +                       return reg & PAD_CFG0_RX_STATE ? 1 : 0;
>
> Is it style of U-Boot? Because
> return !!(...); will have same effect while consuming less characters.

checkpatch does not complain, so I assume it is okay for U-Boot.

>
> >                 else if (rx_tx == PAD_CFG0_RX_DISABLE)
>
> 'else' is redundant here
>
> > -                       return mode & PAD_CFG0_TX_STATE_BIT ? 1 : 0;
> > +                       return reg & PAD_CFG0_TX_STATE ? 1 : 0;
> >         }
>
>
> --

Regards,
Bin

[PATCH v2 3/3] gpio: intel_gpio: Fix register/bit offsets intel_gpio_get_value()

[Bin Meng]

On Mon, Feb 3, 2020 at 6:38 PM Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> Fix the following in intel_gpio_get_value():
>
>  * The value of the register is contained in the variable 'reg', not in
>    'mode'. The variable 'mode' contains only the configuration whether
>    the gpio is currently an input or an output.
>
>  * The correct bitmasks for the input and output value are
>    PAD_CFG0_RX_STATE and PAD_CFG0_TX_STATE.
>    Use them instead of the currently used PAD_CFG0_RX_STATE_BIT and
>    PAD_CFG0_TX_STATE_BIT.
>
> Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2: None
>
>  drivers/gpio/intel_gpio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

applied to u-boot-x86, thanks!