| Message ID | 1397611505-17917-1-git-send-email-abrestic@chromium.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 04/15/2014 07:25 PM, Andrew Bresticker wrote: > The AS3722_GPIO_INV bit will always be blindly overwritten by > as3722_pinctrl_gpio_set_direction() and will be ignored when > setting the value of the GPIO in as3722_gpio_set() since the > enable_gpio_invert flag is never set. This will cause an > initially inverted GPIO to toggle when requested as an output, > which could be problematic if, for example, the GPIO controls > a critical regulator. > > Instead of setting up the enable_gpio_invert flag, uust leave > the invert bit alone and check it before setting the GPIO value. Reviewed-by: Stephen Warren <swarren@nvidia.com> Tested-by: Stephen Warren <swarren@nvidia.com> (not with a 'scope or anything, but an affected system boots in a stable fashion with this applied) Should this be CC: stable? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 16, 2014 at 9:21 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/15/2014 07:25 PM, Andrew Bresticker wrote: >> The AS3722_GPIO_INV bit will always be blindly overwritten by >> as3722_pinctrl_gpio_set_direction() and will be ignored when >> setting the value of the GPIO in as3722_gpio_set() since the >> enable_gpio_invert flag is never set. This will cause an >> initially inverted GPIO to toggle when requested as an output, >> which could be problematic if, for example, the GPIO controls >> a critical regulator. >> >> Instead of setting up the enable_gpio_invert flag, uust leave >> the invert bit alone and check it before setting the GPIO value. > > Reviewed-by: Stephen Warren <swarren@nvidia.com> > Tested-by: Stephen Warren <swarren@nvidia.com> > > (not with a 'scope or anything, but an affected system boots in a stable > fashion with this applied) > > Should this be CC: stable? I think so. Kernels with venice2 support should probably have this. I'll resend and fix the typo in the commit message. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew, On Wednesday 16 April 2014 06:55 AM, Andrew Bresticker wrote: > diff --git a/drivers/pinctrl/pinctrl-as3722.c b/drivers/pinctrl/pinctrl-as3722.c > index 92ed4b2..c862f9c0 100644 > --- a/drivers/pinctrl/pinctrl-as3722.c > +++ b/drivers/pinctrl/pinctrl-as3722.c > @@ -64,7 +64,6 @@ struct as3722_pin_function { > }; > > struct as3722_gpio_pin_control { > - bool enable_gpio_invert; > unsigned mode_prop; > int io_function; > }; Instead of removing this flag and calling read of register on every gpio set, better if we update this flag on probe once and use this for entire life. So adding the code in probe() as for (i = 0; i < ARRAY_SIZE(as3722_pingroups); ++i) { int gpio_cntr_reg = AS3722_GPIOn_CONTROL_REG(i); u32 val; ret = as3722_read(as3722, gpio_cntr_reg, &val); if (!ret) as_pci->gpio_control[i].enable_gpio_invert = !!(val & AS3722_GPIO_INV); } Thanks, Laxman -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 17, 2014 at 2:48 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote: > Hi Andrew, > > > On Wednesday 16 April 2014 06:55 AM, Andrew Bresticker wrote: >> >> diff --git a/drivers/pinctrl/pinctrl-as3722.c >> b/drivers/pinctrl/pinctrl-as3722.c >> index 92ed4b2..c862f9c0 100644 >> --- a/drivers/pinctrl/pinctrl-as3722.c >> +++ b/drivers/pinctrl/pinctrl-as3722.c >> @@ -64,7 +64,6 @@ struct as3722_pin_function { >> }; >> struct as3722_gpio_pin_control { >> - bool enable_gpio_invert; >> unsigned mode_prop; >> int io_function; >> }; > > > Instead of removing this flag and calling read of register on every gpio > set, better if we update this flag on probe once and use this for entire > life. Because of regcache, the reads won't result in any additional I2C transfers. I can respin though if you want. -Andrew > > So adding the code in probe() as > > for (i = 0; i < ARRAY_SIZE(as3722_pingroups); ++i) { > int gpio_cntr_reg = AS3722_GPIOn_CONTROL_REG(i); > u32 val; > > ret = as3722_read(as3722, gpio_cntr_reg, &val); > if (!ret) > as_pci->gpio_control[i].enable_gpio_invert = > !!(val & AS3722_GPIO_INV); > } > > > Thanks, > Laxman -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 17 April 2014 10:13 PM, Andrew Bresticker wrote: > On Thu, Apr 17, 2014 at 2:48 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote: >> Hi Andrew, >> >> >> On Wednesday 16 April 2014 06:55 AM, Andrew Bresticker wrote: >>> diff --git a/drivers/pinctrl/pinctrl-as3722.c >>> b/drivers/pinctrl/pinctrl-as3722.c >>> index 92ed4b2..c862f9c0 100644 >>> --- a/drivers/pinctrl/pinctrl-as3722.c >>> +++ b/drivers/pinctrl/pinctrl-as3722.c >>> @@ -64,7 +64,6 @@ struct as3722_pin_function { >>> }; >>> struct as3722_gpio_pin_control { >>> - bool enable_gpio_invert; >>> unsigned mode_prop; >>> int io_function; >>> }; >> >> Instead of removing this flag and calling read of register on every gpio >> set, better if we update this flag on probe once and use this for entire >> life. > Because of regcache, the reads won't result in any additional I2C > transfers. I can respin though if you want. Yaah, if regcache is enabled on this then it will reduce i2c calls. Just wanted to keep this invert bit so in future, if we need it, we can make it configurable. But not much issue here. I am acking it Acked-by: Laxman Dewangan <ldewangan@nvidia.com> -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/pinctrl-as3722.c b/drivers/pinctrl/pinctrl-as3722.c index 92ed4b2..c862f9c0 100644 --- a/drivers/pinctrl/pinctrl-as3722.c +++ b/drivers/pinctrl/pinctrl-as3722.c @@ -64,7 +64,6 @@ struct as3722_pin_function { }; struct as3722_gpio_pin_control { - bool enable_gpio_invert; unsigned mode_prop; int io_function; }; @@ -320,10 +319,8 @@ static int as3722_pinctrl_gpio_set_direction(struct pinctrl_dev *pctldev, return mode; } - if (as_pci->gpio_control[offset].enable_gpio_invert) - mode |= AS3722_GPIO_INV; - - return as3722_write(as3722, AS3722_GPIOn_CONTROL_REG(offset), mode); + return as3722_update_bits(as3722, AS3722_GPIOn_CONTROL_REG(offset), + AS3722_GPIO_MODE_MASK, mode); } static const struct pinmux_ops as3722_pinmux_ops = { @@ -496,10 +493,18 @@ static void as3722_gpio_set(struct gpio_chip *chip, unsigned offset, { struct as3722_pctrl_info *as_pci = to_as_pci(chip); struct as3722 *as3722 = as_pci->as3722; - int en_invert = as_pci->gpio_control[offset].enable_gpio_invert; + int en_invert; u32 val; int ret; + ret = as3722_read(as3722, AS3722_GPIOn_CONTROL_REG(offset), &val); + if (ret < 0) { + dev_err(as_pci->dev, + "GPIO_CONTROL%d_REG read failed: %d\n", offset, ret); + return; + } + en_invert = !!(val & AS3722_GPIO_INV); + if (value) val = (en_invert) ? 0 : AS3722_GPIOn_SIGNAL(offset); else
The AS3722_GPIO_INV bit will always be blindly overwritten by as3722_pinctrl_gpio_set_direction() and will be ignored when setting the value of the GPIO in as3722_gpio_set() since the enable_gpio_invert flag is never set. This will cause an initially inverted GPIO to toggle when requested as an output, which could be problematic if, for example, the GPIO controls a critical regulator. Instead of setting up the enable_gpio_invert flag, uust leave the invert bit alone and check it before setting the GPIO value. Signed-off-by: Andrew Bresticker <abrestic@chromium.org> --- drivers/pinctrl/pinctrl-as3722.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)