| Message ID | 1426154203-11551-1-git-send-email-sonic.adi@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Mar 12, 2015 at 10:56 AM, Sonic Zhang <sonic.adi@gmail.com> wrote: > From: Sonic Zhang <sonic.zhang@analog.com> > > The blackfin pinmux and gpio controller doesn't allow user to set up 1 pin > for both GPIO and peripheral function. So, add flag strict in struct pinctrl > to check both gpio_owner and mux_owner before approving the pin request. > > Signed-off-by: Sonic Zhang <sonic.zhang@analog.com> Nice! But mention in the commit that ADI2 is also patched to use this. Do we have other candidates for strict GPIO/mux separation? What do people on the lists say? > +++ b/drivers/pinctrl/pinmux.c > @@ -99,24 +99,25 @@ static int pin_request(struct pinctrl_dev *pctldev, > dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n", > pin, desc->name, owner); > > + if ((gpio_range || pctldev->desc->strict) && desc->gpio_owner) { So either we find a range map or we are strict and there is also a previous owner of the pin. Is this correct? I think we should *always* find a range to request a pin. I think you should just leave this if()-statement alone and insert some new stuff inside the lower else()-clause. > + dev_err(pctldev->dev, > + "pin %s already requested by %s; cannot claim for %s\n", > + desc->name, desc->gpio_owner, owner); > + goto out; > + } > + > + if ((!gpio_range || pctldev->desc->strict) && > + desc->mux_usecount && strcmp(desc->mux_owner, owner)) { > + dev_err(pctldev->dev, > + "pin %s already requested by %s; cannot claim for %s\n", > + desc->name, desc->mux_owner, owner); > + goto out; > + } This is wrong. If the function is entered with gpio_range != NULL it is a request for a single GPIO line, else it is regular muxing. Keep the else() clause, just also include an explicit check to see if desc->gpio_owner is set, and in that case, if we are also strict, bail out. else { /* No gpio_range */ if (pctldev->desc->strict && desc->gpio_owner) { err "already used for GPIO..." } > + > if (gpio_range) { So just keep the whole thing inside if (gpio_range). > desc->mux_usecount++; > if (desc->mux_usecount > 1) > return 0; > diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h > index 66e4697..ca6c99c0 100644 > --- a/include/linux/pinctrl/pinctrl.h > +++ b/include/linux/pinctrl/pinctrl.h > @@ -132,6 +132,7 @@ struct pinctrl_desc { > const struct pinctrl_ops *pctlops; > const struct pinmux_ops *pmxops; > const struct pinconf_ops *confops; > + bool strict; Also update the kerneldoc above this struct. Also update examples and text in Documentation/pinctrl.txt so it is clear when to use this option and what it means. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On Wed, Mar 18, 2015 at 6:21 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Mar 12, 2015 at 10:56 AM, Sonic Zhang <sonic.adi@gmail.com> wrote: > >> From: Sonic Zhang <sonic.zhang@analog.com> >> >> The blackfin pinmux and gpio controller doesn't allow user to set up 1 pin >> for both GPIO and peripheral function. So, add flag strict in struct pinctrl >> to check both gpio_owner and mux_owner before approving the pin request. >> >> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com> > > Nice! > > But mention in the commit that ADI2 is also patched to use > this. OK > > Do we have other candidates for strict GPIO/mux separation? > What do people on the lists say? > >> +++ b/drivers/pinctrl/pinmux.c >> @@ -99,24 +99,25 @@ static int pin_request(struct pinctrl_dev *pctldev, >> dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n", >> pin, desc->name, owner); >> >> + if ((gpio_range || pctldev->desc->strict) && desc->gpio_owner) { > > So either we find a range map or we are strict and there is also a > previous owner of the pin. > > Is this correct? I think we should *always* find a range to request > a pin. When requesting regular muxing from function pinmux_enable_setting(), pin_request() is invoked with gpio_range = NULL. But, when requesting GPIO, function pinmux_request_gpio() always passes a valid range. So, if gpio_owner is set, it is correct to fail a request either the request is for this GPIO or the request is for regular muxing of this GPIO pin and the strict bit is set. > > I think you should just leave this if()-statement alone and insert > some new stuff inside the lower else()-clause. > > >> + dev_err(pctldev->dev, >> + "pin %s already requested by %s; cannot claim for %s\n", >> + desc->name, desc->gpio_owner, owner); >> + goto out; >> + } >> + >> + if ((!gpio_range || pctldev->desc->strict) && >> + desc->mux_usecount && strcmp(desc->mux_owner, owner)) { >> + dev_err(pctldev->dev, >> + "pin %s already requested by %s; cannot claim for %s\n", >> + desc->name, desc->mux_owner, owner); >> + goto out; >> + } > > This is wrong. > > If the function is entered with gpio_range != NULL it is a request > for a single GPIO line, else it is regular muxing. Why this is wrong? If gpio_range != NULL, the request of a GPIO is already checked in the first if clause. In strict case: Both mux_owner and gpio_owner are checked no matter whether GPIO or regular muxing is requested. If both checking pass, muxing_owner or gpio_owner is set according to the request type. In non strict case: Request of GPIO is checked in the first if clause against gpio_owner, while request of regular muxing is checked in the second if clause against mux_owner. If either checking passes, its owner is set which doesn't affect the checking of the other request type. > > Keep the else() clause, just also include an explicit check > to see if desc->gpio_owner is set, and in that case, if we > are also strict, bail out. Anyway, if you think doing the explicit check in both if() and else() clauses is better, I am fine to send a new patch. > > else { /* No gpio_range */ > if (pctldev->desc->strict && desc->gpio_owner) { > err "already used for GPIO..." > } > >> + >> if (gpio_range) { > > So just keep the whole thing inside if (gpio_range). > >> desc->mux_usecount++; >> if (desc->mux_usecount > 1) >> return 0; >> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h >> index 66e4697..ca6c99c0 100644 >> --- a/include/linux/pinctrl/pinctrl.h >> +++ b/include/linux/pinctrl/pinctrl.h >> @@ -132,6 +132,7 @@ struct pinctrl_desc { >> const struct pinctrl_ops *pctlops; >> const struct pinmux_ops *pmxops; >> const struct pinconf_ops *confops; >> + bool strict; > > Also update the kerneldoc above this struct. > > Also update examples and text in > Documentation/pinctrl.txt > so it is clear when to use this option and what it means. OK Regards, Sonic -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 19, 2015 at 11:06 AM, Sonic Zhang <sonic.adi@gmail.com> wrote: > Anyway, if you think doing the explicit check in both if() and else() > clauses is better, I am fine to send a new patch. I looked at it for half an hour and could not figure out if it was wrong or right really, eventually maybe got it wrong, maybe right. What is happening is that the code is getting so convoluted that I cannot follow the program flow, which is a clear sign that refactoring is needed. Either restructure, add comments or break our helper functions. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" 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-adi2.c b/drivers/pinctrl/pinctrl-adi2.c index 8434439..fbd4926 100644 --- a/drivers/pinctrl/pinctrl-adi2.c +++ b/drivers/pinctrl/pinctrl-adi2.c @@ -710,6 +710,7 @@ static struct pinctrl_desc adi_pinmux_desc = { .name = DRIVER_NAME, .pctlops = &adi_pctrl_ops, .pmxops = &adi_pinmux_ops, + .strict = true, .owner = THIS_MODULE, }; diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index b874458..dfa2b42 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -99,24 +99,25 @@ static int pin_request(struct pinctrl_dev *pctldev, dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n", pin, desc->name, owner); + if ((gpio_range || pctldev->desc->strict) && desc->gpio_owner) { + dev_err(pctldev->dev, + "pin %s already requested by %s; cannot claim for %s\n", + desc->name, desc->gpio_owner, owner); + goto out; + } + + if ((!gpio_range || pctldev->desc->strict) && + desc->mux_usecount && strcmp(desc->mux_owner, owner)) { + dev_err(pctldev->dev, + "pin %s already requested by %s; cannot claim for %s\n", + desc->name, desc->mux_owner, owner); + goto out; + } + if (gpio_range) { /* There's no need to support multiple GPIO requests */ - if (desc->gpio_owner) { - dev_err(pctldev->dev, - "pin %s already requested by %s; cannot claim for %s\n", - desc->name, desc->gpio_owner, owner); - goto out; - } - desc->gpio_owner = owner; } else { - if (desc->mux_usecount && strcmp(desc->mux_owner, owner)) { - dev_err(pctldev->dev, - "pin %s already requested by %s; cannot claim for %s\n", - desc->name, desc->mux_owner, owner); - goto out; - } - desc->mux_usecount++; if (desc->mux_usecount > 1) return 0; diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h index 66e4697..ca6c99c0 100644 --- a/include/linux/pinctrl/pinctrl.h +++ b/include/linux/pinctrl/pinctrl.h @@ -132,6 +132,7 @@ struct pinctrl_desc { const struct pinctrl_ops *pctlops; const struct pinmux_ops *pmxops; const struct pinconf_ops *confops; + bool strict; struct module *owner; #ifdef CONFIG_GENERIC_PINCONF unsigned int num_custom_params;