| Message ID | 20200203103806.29445-4-wolfgang.wallner@br-automation.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: intel_gpio: Fix Intel gpio driver | expand |
On Mon, Feb 3, 2020 at 12:38 PM Wolfgang Wallner <wolfgang.wallner at 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; > }
Hi Andy, On Mon, Feb 3, 2020 at 8:34 PM Andy Shevchenko <andy.shevchenko at gmail.com> wrote: > > On Mon, Feb 3, 2020 at 12:38 PM Wolfgang Wallner > <wolfgang.wallner at 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
On Mon, Feb 3, 2020 at 6:38 PM Wolfgang Wallner <wolfgang.wallner at 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 at br-automation.com> > Reviewed-by: Simon Glass <sjg at chromium.org> > Reviewed-by: Bin Meng <bmeng.cn at 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!
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;