| Message ID | 1432044863-2035-3-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote: > Commit e5b3b2d9ed20 ("pinctrl: allows not to define the get_group_pins > operation") allows pin controllers not to register the get_group_pins() > function. However, a side-effect of not registering this function is > that pins are not allocated and potentially multiple devices could > attempt to configure the same pins [1]. Although this problem exists in > the pinctrl core, because only a few devices are impacted by this, fix > this for tegra-xusb by adding the get_group_pins() function. If I understand correctly this effectively makes get_group_pins() mandatory, doesn't it? In that case, shouldn't commit e5b3b2d9ed20 be reverted? > Please note that in addition to adding the get_group_pins() functions > the pins/lanes for the tegra-xusb also need to be registered when > calling pinctrl_register(). This also allows the current pinmux state > to be viewed by the debugfs node "pinmux-pins" for the tegra-xusb pad > controller. This sounds like a logically separate change and hence could warrant a separate patch. I'll defer to Linus on this, though, so: Acked-by: Thierry Reding <treding@nvidia.com>
On Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote: [...] One more thing: [...] > diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c b/drivers/pinctrl/pinctrl-tegra-xusb.c > index 3e8e4a914fb4..c61594066e26 100644 > --- a/drivers/pinctrl/pinctrl-tegra-xusb.c > +++ b/drivers/pinctrl/pinctrl-tegra-xusb.c > @@ -125,6 +125,23 @@ static const char *tegra_xusb_padctl_get_group_name(struct pinctrl_dev *pinctrl, > return padctl->soc->pins[group].name; > } > > +static int tegra_xusb_padctl_get_group_pins(struct pinctrl_dev *pinctrl, > + unsigned group, > + const unsigned **pins, > + unsigned *num_pins) > +{ > + struct tegra_xusb_padctl *padctl = pinctrl_dev_get_drvdata(pinctrl); > + > + /* > + * For the tegra-xusb pad controller groups are synonomous > + * with lanes/pins and there is always one lane/pin per group. > + */ > + *pins = &padctl->soc->pins[group].number; Shouldn't this be the same as pinctrl->desc->pins? In that case, maybe a better solution would be to make .get_group_pins() mandatory again and turn this into a pinctrl helper function that can be used by all group- only pinctrl drivers? Thierry
On 19/05/15 15:30, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote: >> Commit e5b3b2d9ed20 ("pinctrl: allows not to define the get_group_pins >> operation") allows pin controllers not to register the get_group_pins() >> function. However, a side-effect of not registering this function is >> that pins are not allocated and potentially multiple devices could >> attempt to configure the same pins [1]. Although this problem exists in >> the pinctrl core, because only a few devices are impacted by this, fix >> this for tegra-xusb by adding the get_group_pins() function. > > If I understand correctly this effectively makes get_group_pins() > mandatory, doesn't it? In that case, shouldn't commit e5b3b2d9ed20 be > reverted? Yes may be it should. I will defer to Linus here. >> Please note that in addition to adding the get_group_pins() functions >> the pins/lanes for the tegra-xusb also need to be registered when >> calling pinctrl_register(). This also allows the current pinmux state >> to be viewed by the debugfs node "pinmux-pins" for the tegra-xusb pad >> controller. > > This sounds like a logically separate change and hence could warrant a > separate patch. I'll defer to Linus on this, though, so: It needs to be registered as part of this patch otherwise the pins will not be registered and pin_request() will fail. Today pin_request() does not even get called for these pins. So I believe that this is needed as part of this patch. May be I was not clear above, but the debugfs "pinmux-pins" entry shows nothing today for these pins. A by-product of this fix is that now we can view the pinmux state of the pins via the debugfs. > Acked-by: Thierry Reding <treding@nvidia.com> Cheers Jon -- 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 19/05/15 15:33, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote: > [...] > > One more thing: > > [...] >> diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c b/drivers/pinctrl/pinctrl-tegra-xusb.c >> index 3e8e4a914fb4..c61594066e26 100644 >> --- a/drivers/pinctrl/pinctrl-tegra-xusb.c >> +++ b/drivers/pinctrl/pinctrl-tegra-xusb.c >> @@ -125,6 +125,23 @@ static const char *tegra_xusb_padctl_get_group_name(struct pinctrl_dev *pinctrl, >> return padctl->soc->pins[group].name; >> } >> >> +static int tegra_xusb_padctl_get_group_pins(struct pinctrl_dev *pinctrl, >> + unsigned group, >> + const unsigned **pins, >> + unsigned *num_pins) >> +{ >> + struct tegra_xusb_padctl *padctl = pinctrl_dev_get_drvdata(pinctrl); >> + >> + /* >> + * For the tegra-xusb pad controller groups are synonomous >> + * with lanes/pins and there is always one lane/pin per group. >> + */ >> + *pins = &padctl->soc->pins[group].number; > > Shouldn't this be the same as pinctrl->desc->pins? In that case, maybe a > better solution would be to make .get_group_pins() mandatory again and > turn this into a pinctrl helper function that can be used by all group- > only pinctrl drivers? Yes I believe that you are right. However, I don't think a helper function would work here, because this function returns a pointer to an array of pin numbers. In the case of xusb this is simplified a bit because we are only returning a single number here for the group. However, if you look at other pinctrl devices (such as pinctrl-tegra.c) there could be more than one number in the array. Cheers Jon -- 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 Tue, May 19, 2015 at 04:23:15PM +0100, Jon Hunter wrote: > > On 19/05/15 15:33, Thierry Reding wrote: > > * PGP Signed by an unknown key > > > > On Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote: > > [...] > > > > One more thing: > > > > [...] > >> diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c b/drivers/pinctrl/pinctrl-tegra-xusb.c > >> index 3e8e4a914fb4..c61594066e26 100644 > >> --- a/drivers/pinctrl/pinctrl-tegra-xusb.c > >> +++ b/drivers/pinctrl/pinctrl-tegra-xusb.c > >> @@ -125,6 +125,23 @@ static const char *tegra_xusb_padctl_get_group_name(struct pinctrl_dev *pinctrl, > >> return padctl->soc->pins[group].name; > >> } > >> > >> +static int tegra_xusb_padctl_get_group_pins(struct pinctrl_dev *pinctrl, > >> + unsigned group, > >> + const unsigned **pins, > >> + unsigned *num_pins) > >> +{ > >> + struct tegra_xusb_padctl *padctl = pinctrl_dev_get_drvdata(pinctrl); > >> + > >> + /* > >> + * For the tegra-xusb pad controller groups are synonomous > >> + * with lanes/pins and there is always one lane/pin per group. > >> + */ > >> + *pins = &padctl->soc->pins[group].number; > > > > Shouldn't this be the same as pinctrl->desc->pins? In that case, maybe a > > better solution would be to make .get_group_pins() mandatory again and > > turn this into a pinctrl helper function that can be used by all group- > > only pinctrl drivers? > > Yes I believe that you are right. However, I don't think a helper > function would work here, because this function returns a pointer to an > array of pin numbers. In the case of xusb this is simplified a bit > because we are only returning a single number here for the group. > However, if you look at other pinctrl devices (such as pinctrl-tegra.c) > there could be more than one number in the array. I see. Oh well, guess this is as good as it gets then. Thierry
diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c b/drivers/pinctrl/pinctrl-tegra-xusb.c index 3e8e4a914fb4..c61594066e26 100644 --- a/drivers/pinctrl/pinctrl-tegra-xusb.c +++ b/drivers/pinctrl/pinctrl-tegra-xusb.c @@ -125,6 +125,23 @@ static const char *tegra_xusb_padctl_get_group_name(struct pinctrl_dev *pinctrl, return padctl->soc->pins[group].name; } +static int tegra_xusb_padctl_get_group_pins(struct pinctrl_dev *pinctrl, + unsigned group, + const unsigned **pins, + unsigned *num_pins) +{ + struct tegra_xusb_padctl *padctl = pinctrl_dev_get_drvdata(pinctrl); + + /* + * For the tegra-xusb pad controller groups are synonomous + * with lanes/pins and there is always one lane/pin per group. + */ + *pins = &padctl->soc->pins[group].number; + *num_pins = 1; + + return 0; +} + enum tegra_xusb_padctl_param { TEGRA_XUSB_PADCTL_IDDQ, }; @@ -248,6 +265,7 @@ static int tegra_xusb_padctl_dt_node_to_map(struct pinctrl_dev *pinctrl, static const struct pinctrl_ops tegra_xusb_padctl_pinctrl_ops = { .get_groups_count = tegra_xusb_padctl_get_groups_count, .get_group_name = tegra_xusb_padctl_get_group_name, + .get_group_pins = tegra_xusb_padctl_get_group_pins, .dt_node_to_map = tegra_xusb_padctl_dt_node_to_map, .dt_free_map = pinctrl_utils_dt_free_map, }; @@ -898,6 +916,8 @@ static int tegra_xusb_padctl_probe(struct platform_device *pdev) memset(&padctl->desc, 0, sizeof(padctl->desc)); padctl->desc.name = dev_name(padctl->dev); + padctl->desc.pins = tegra124_pins; + padctl->desc.npins = ARRAY_SIZE(tegra124_pins); padctl->desc.pctlops = &tegra_xusb_padctl_pinctrl_ops; padctl->desc.pmxops = &tegra_xusb_padctl_pinmux_ops; padctl->desc.confops = &tegra_xusb_padctl_pinconf_ops;
Commit e5b3b2d9ed20 ("pinctrl: allows not to define the get_group_pins operation") allows pin controllers not to register the get_group_pins() function. However, a side-effect of not registering this function is that pins are not allocated and potentially multiple devices could attempt to configure the same pins [1]. Although this problem exists in the pinctrl core, because only a few devices are impacted by this, fix this for tegra-xusb by adding the get_group_pins() function. Please note that in addition to adding the get_group_pins() functions the pins/lanes for the tegra-xusb also need to be registered when calling pinctrl_register(). This also allows the current pinmux state to be viewed by the debugfs node "pinmux-pins" for the tegra-xusb pad controller. [1] http://www.spinics.net/lists/linux-gpio/msg05810.html Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/pinctrl/pinctrl-tegra-xusb.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)