| Message ID | 1589817025-21886-1-git-send-email-groverm@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: msm: Add check for pinctrl group is valid | expand |
On Mon 18 May 08:50 PDT 2020, Mayank Grover wrote: > The list of reserved gpio pins for platform are populated > in gpiochip valid_mask. > > Here on MSM common driver introduce ability to check if > pingroup is valid, by parsing pins in pingroup against > reserved pins for gpios. This does not handle non-gpio > pingroups. > Thanks Mayank, I can confirm that this is a problem, but don't we need this on some of the pinmux_ops as well? @Linus, we started off with something similar for GPIOs and ended up with the logic in the core code. Should we somehow try to do the same for pinctrl? Regards, Bjorn > Signed-off-by: Mayank Grover <groverm@codeaurora.org> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 85858c1..b6ebe26 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -261,6 +261,24 @@ static unsigned msm_regval_to_drive(u32 val) > return (val + 1) * 2; > } > > +static bool msm_pingroup_is_valid(struct msm_pinctrl *pctrl, > + const struct msm_pingroup *g) > +{ > + const unsigned int *pins = g->pins; > + unsigned int num_pins = g->npins; > + struct gpio_chip *chip = &pctrl->chip; > + unsigned int max_gpios = chip->ngpio; > + unsigned int i; > + > + for (i = 0; i < num_pins; i++) { > + /* Doesn't handle non-gpio pingroups */ > + if (pins[i] < max_gpios && > + !gpiochip_line_is_valid(chip, pins[i])) > + return false; > + } > + return true; > +} > + > static int msm_config_group_get(struct pinctrl_dev *pctldev, > unsigned int group, > unsigned long *config) > @@ -276,6 +294,10 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev, > > g = &pctrl->soc->groups[group]; > > + /* Check if group has all valid pins */ > + if (!msm_pingroup_is_valid(pctrl, g)) > + return -EINVAL; > + > ret = msm_config_reg(pctrl, g, param, &mask, &bit); > if (ret < 0) > return ret; > @@ -355,6 +377,10 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev, > > g = &pctrl->soc->groups[group]; > > + /* Check if group has all valid pins */ > + if (!msm_pingroup_is_valid(pctrl, g)) > + return -EINVAL; > + > for (i = 0; i < num_configs; i++) { > param = pinconf_to_config_param(configs[i]); > arg = pinconf_to_config_argument(configs[i]); > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a > member of the Code Aurora Forum, hosted by The Linux Foundation
O Mon, May 18, 2020 at 06:38:13PM -0700, Bjorn Andersson wrote: > On Mon 18 May 08:50 PDT 2020, Mayank Grover wrote: > > > The list of reserved gpio pins for platform are populated > > in gpiochip valid_mask. > > > > Here on MSM common driver introduce ability to check if > > pingroup is valid, by parsing pins in pingroup against > > reserved pins for gpios. This does not handle non-gpio > > pingroups. > > > > Thanks Mayank, I can confirm that this is a problem, but don't we need > this on some of the pinmux_ops as well? > Thanks Bjorn, for quick reply. For pinmux ops, we already have check for validity in msm_pinmux_request function hook. request is getting called by core to check availabity of pin before acquiring the pin. Hence, I think we don't need this check there. Regards, Mayank > @Linus, we started off with something similar for GPIOs and ended up > with the logic in the core code. Should we somehow try to do the same > for pinctrl? > > Regards, > Bjorn > > > Signed-off-by: Mayank Grover <groverm@codeaurora.org> > > --- > > drivers/pinctrl/qcom/pinctrl-msm.c | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > > index 85858c1..b6ebe26 100644 > > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > > @@ -261,6 +261,24 @@ static unsigned msm_regval_to_drive(u32 val) > > return (val + 1) * 2; > > } > > > > +static bool msm_pingroup_is_valid(struct msm_pinctrl *pctrl, > > + const struct msm_pingroup *g) > > +{ > > + const unsigned int *pins = g->pins; > > + unsigned int num_pins = g->npins; > > + struct gpio_chip *chip = &pctrl->chip; > > + unsigned int max_gpios = chip->ngpio; > > + unsigned int i; > > + > > + for (i = 0; i < num_pins; i++) { > > + /* Doesn't handle non-gpio pingroups */ > > + if (pins[i] < max_gpios && > > + !gpiochip_line_is_valid(chip, pins[i])) > > + return false; > > + } > > + return true; > > +} > > + > > static int msm_config_group_get(struct pinctrl_dev *pctldev, > > unsigned int group, > > unsigned long *config) > > @@ -276,6 +294,10 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev, > > > > g = &pctrl->soc->groups[group]; > > > > + /* Check if group has all valid pins */ > > + if (!msm_pingroup_is_valid(pctrl, g)) > > + return -EINVAL; > > + > > ret = msm_config_reg(pctrl, g, param, &mask, &bit); > > if (ret < 0) > > return ret; > > @@ -355,6 +377,10 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev, > > > > g = &pctrl->soc->groups[group]; > > > > + /* Check if group has all valid pins */ > > + if (!msm_pingroup_is_valid(pctrl, g)) > > + return -EINVAL; > > + > > for (i = 0; i < num_configs; i++) { > > param = pinconf_to_config_param(configs[i]); > > arg = pinconf_to_config_argument(configs[i]); > > -- > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a > > member of the Code Aurora Forum, hosted by The Linux Foundation
On Tue, May 19, 2020 at 3:39 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > @Linus, we started off with something similar for GPIOs and ended up > with the logic in the core code. Should we somehow try to do the same > for pinctrl? msm_pingroup_is_valid() looks very reusable but I'm afraid I do not understand the implicit assumptions around it, but I guess yes, if it can be properly documented etc. Yours, Linus Walleij
On Mon, May 25, 2020 at 11:02:14AM +0200, Linus Walleij wrote: > On Tue, May 19, 2020 at 3:39 AM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > @Linus, we started off with something similar for GPIOs and ended up > > with the logic in the core code. Should we somehow try to do the same > > for pinctrl? > > msm_pingroup_is_valid() looks very reusable but I'm afraid I do not > understand the implicit assumptions around it, but I guess yes, > if it can be properly documented etc. > > Yours, > Linus Walleij Hi Bjorn, Can you please help guide further on this ? we can validate group using request calls similar to pinmux_ops here. -Mayank
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 85858c1..b6ebe26 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -261,6 +261,24 @@ static unsigned msm_regval_to_drive(u32 val) return (val + 1) * 2; } +static bool msm_pingroup_is_valid(struct msm_pinctrl *pctrl, + const struct msm_pingroup *g) +{ + const unsigned int *pins = g->pins; + unsigned int num_pins = g->npins; + struct gpio_chip *chip = &pctrl->chip; + unsigned int max_gpios = chip->ngpio; + unsigned int i; + + for (i = 0; i < num_pins; i++) { + /* Doesn't handle non-gpio pingroups */ + if (pins[i] < max_gpios && + !gpiochip_line_is_valid(chip, pins[i])) + return false; + } + return true; +} + static int msm_config_group_get(struct pinctrl_dev *pctldev, unsigned int group, unsigned long *config) @@ -276,6 +294,10 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev, g = &pctrl->soc->groups[group]; + /* Check if group has all valid pins */ + if (!msm_pingroup_is_valid(pctrl, g)) + return -EINVAL; + ret = msm_config_reg(pctrl, g, param, &mask, &bit); if (ret < 0) return ret; @@ -355,6 +377,10 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev, g = &pctrl->soc->groups[group]; + /* Check if group has all valid pins */ + if (!msm_pingroup_is_valid(pctrl, g)) + return -EINVAL; + for (i = 0; i < num_configs; i++) { param = pinconf_to_config_param(configs[i]); arg = pinconf_to_config_argument(configs[i]);
The list of reserved gpio pins for platform are populated in gpiochip valid_mask. Here on MSM common driver introduce ability to check if pingroup is valid, by parsing pins in pingroup against reserved pins for gpios. This does not handle non-gpio pingroups. Signed-off-by: Mayank Grover <groverm@codeaurora.org> --- drivers/pinctrl/qcom/pinctrl-msm.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)