| Message ID | 20240214-mbly-gpio-v1-13-f88c0ccf372b@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rework Nomadik GPIO to add Mobileye EyeQ5 support | expand |
On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > Previously, the statement looked like: > > slpm[x] &= ~BIT(g->pins[i]); > > Where: > - slpm is a unsigned int pointer; > - g->pins[i] is a pin number which can grow to more than 32. > > The expected shift amount is a pin bank offset. > > This bug does not occur on every group or pin: the altsetting must be > NMK_GPIO_ALT_C and the pin must be 32 or above. It is possible that it > occurred. For example, in pinctrl-nomadik-db8500.c, pin group i2c3_c_2 > has the right altsetting and has pins 229 and 230. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Maybe add a Fixes: tag and put it at the beginning of the series so that it can go upstream earlier as a fix? Bart > --- > drivers/pinctrl/nomadik/pinctrl-nomadik.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/pinctrl/nomadik/pinctrl-nomadik.c b/drivers/pinctrl/nomadik/pinctrl-nomadik.c > index c7693fbc0484..99bdb25f358d 100644 > --- a/drivers/pinctrl/nomadik/pinctrl-nomadik.c > +++ b/drivers/pinctrl/nomadik/pinctrl-nomadik.c > @@ -913,8 +913,9 @@ static int nmk_pmx_set(struct pinctrl_dev *pctldev, unsigned int function, > */ > for (i = 0; i < g->grp.npins; i++) { > struct nmk_gpio_chip *nmk_chip; > + unsigned int bit; > > - nmk_chip = find_nmk_gpio_from_pin(g->grp.pins[i], NULL); > + nmk_chip = find_nmk_gpio_from_pin(g->grp.pins[i], &bit); > if (!nmk_chip) { > dev_err(npct->dev, > "invalid pin offset %d in group %s at index %d\n", > @@ -922,7 +923,7 @@ static int nmk_pmx_set(struct pinctrl_dev *pctldev, unsigned int function, > goto out_pre_slpm_init; > } > > - slpm[nmk_chip->bank] &= ~BIT(g->grp.pins[i]); > + slpm[nmk_chip->bank] &= ~BIT(bit); > } > nmk_gpio_glitch_slpm_init(slpm); > } > > -- > 2.43.1 >
On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > Previously, the statement looked like: > > slpm[x] &= ~BIT(g->pins[i]); > > Where: > - slpm is a unsigned int pointer; > - g->pins[i] is a pin number which can grow to more than 32. > > The expected shift amount is a pin bank offset. > > This bug does not occur on every group or pin: the altsetting must be > NMK_GPIO_ALT_C and the pin must be 32 or above. It is possible that it > occurred. For example, in pinctrl-nomadik-db8500.c, pin group i2c3_c_2 > has the right altsetting and has pins 229 and 230. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Ah good catch! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> I this something I could just apply as a fix or are there dependencies on other patches? Yours, Linus Walleij
Hello, On Mon Feb 19, 2024 at 4:54 PM CET, Bartosz Golaszewski wrote: > On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > > Previously, the statement looked like: > > > > slpm[x] &= ~BIT(g->pins[i]); > > > > Where: > > - slpm is a unsigned int pointer; > > - g->pins[i] is a pin number which can grow to more than 32. > > > > The expected shift amount is a pin bank offset. > > > > This bug does not occur on every group or pin: the altsetting must be > > NMK_GPIO_ALT_C and the pin must be 32 or above. It is possible that it > > occurred. For example, in pinctrl-nomadik-db8500.c, pin group i2c3_c_2 > > has the right altsetting and has pins 229 and 230. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > Maybe add a Fixes: tag and put it at the beginning of the series so > that it can go upstream earlier as a fix? I'll see how it works out because the fix depends on helpers added in this series. I'll be reworking that. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Mon Feb 19, 2024 at 10:56 PM CET, Linus Walleij wrote: > On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > Previously, the statement looked like: > > > > slpm[x] &= ~BIT(g->pins[i]); > > > > Where: > > - slpm is a unsigned int pointer; > > - g->pins[i] is a pin number which can grow to more than 32. > > > > The expected shift amount is a pin bank offset. > > > > This bug does not occur on every group or pin: the altsetting must be > > NMK_GPIO_ALT_C and the pin must be 32 or above. It is possible that it > > occurred. For example, in pinctrl-nomadik-db8500.c, pin group i2c3_c_2 > > has the right altsetting and has pins 229 and 230. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > Ah good catch! > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > I this something I could just apply as a fix or are there > dependencies on other patches? There are dependencies. As Bartosz and you asked, I'll try my best to avoid them and move this patch up in the series, with an appropriate Fixes: trailer (to the initial driver commit I believe). Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
diff --git a/drivers/pinctrl/nomadik/pinctrl-nomadik.c b/drivers/pinctrl/nomadik/pinctrl-nomadik.c index c7693fbc0484..99bdb25f358d 100644 --- a/drivers/pinctrl/nomadik/pinctrl-nomadik.c +++ b/drivers/pinctrl/nomadik/pinctrl-nomadik.c @@ -913,8 +913,9 @@ static int nmk_pmx_set(struct pinctrl_dev *pctldev, unsigned int function, */ for (i = 0; i < g->grp.npins; i++) { struct nmk_gpio_chip *nmk_chip; + unsigned int bit; - nmk_chip = find_nmk_gpio_from_pin(g->grp.pins[i], NULL); + nmk_chip = find_nmk_gpio_from_pin(g->grp.pins[i], &bit); if (!nmk_chip) { dev_err(npct->dev, "invalid pin offset %d in group %s at index %d\n", @@ -922,7 +923,7 @@ static int nmk_pmx_set(struct pinctrl_dev *pctldev, unsigned int function, goto out_pre_slpm_init; } - slpm[nmk_chip->bank] &= ~BIT(g->grp.pins[i]); + slpm[nmk_chip->bank] &= ~BIT(bit); } nmk_gpio_glitch_slpm_init(slpm); }
Previously, the statement looked like: slpm[x] &= ~BIT(g->pins[i]); Where: - slpm is a unsigned int pointer; - g->pins[i] is a pin number which can grow to more than 32. The expected shift amount is a pin bank offset. This bug does not occur on every group or pin: the altsetting must be NMK_GPIO_ALT_C and the pin must be 32 or above. It is possible that it occurred. For example, in pinctrl-nomadik-db8500.c, pin group i2c3_c_2 has the right altsetting and has pins 229 and 230. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- drivers/pinctrl/nomadik/pinctrl-nomadik.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)