pinctrl: tegra-xusb: Clean-up and fixes
Subsystems (3)
cover pinctrl: tegra-xusb: Clean-up and fixes
Some clean-up and fixes for tegra-xusb driver.
This had been boot tested on tegra20-trimslice, tegra30-beaver, tegra114-dalmore-a04 and tegra124-jetson-tk1 and no issues seen. Complete test information can be found below.
Jon Hunter (3):
pinctrl: tegra-xusb: Remove unused structure
pinctrl: tegra-xusb: Fix allocation of pins
pinctrl: tegra-xusb: Check that of_match_node returns a valid pointer
drivers/pinctrl/pinctrl-tegra-xusb.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)
Signature · 1 line
1.9.1
A message that is not in the archive (sent off-list or not yet received): 1432044863-2035-1-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
PATCH 1/3 pinctrl: tegra-xusb: Remove unused structure
The structure tegra_xusb_padctl_group is defined but never used and so remove this.
--- drivers/pinctrl/pinctrl-tegra-xusb.c | 5 ----- 1 file changed, 5 deletions(-)
drivers/pinctrl/pinctrl-tegra-xusb.c +0 -5
@@ -59,11 +59,6 @@ struct tegra_xusb_padctl_function {unsigned int num_groups; }; -struct tegra_xusb_padctl_group { -const unsigned int *funcs; -unsigned int num_funcs; -}; -struct tegra_xusb_padctl_soc {const struct pinctrl_pin_desc *pins;unsigned int num_pins;
Signature · 1 line
1.9.1
Re: [PATCH 1/3] pinctrl: tegra-xusb: Remove unused structure
Quoted 7 lines · Jon Hunter
The structure
tegra_xusb_padctl_groupis defined but never used and so remove this.Signed-off-by Jon Hunter <[email protected]>--- drivers/pinctrl/pinctrl-tegra-xusb.c | 5 ----- 1 file changed, 5 deletions(-)
Jon Hunter
maintainerPATCH 2/3 pinctrl: tegra-xusb: Fix allocation of pins
In reply to Jon HunterCommit 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
--- drivers/pinctrl/pinctrl-tegra-xusb.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
drivers/pinctrl/pinctrl-tegra-xusb.c +20 -0
@@ -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;
Signature · 1 line
1.9.1
A message that is not in the archive (sent off-list or not yet received): 1432044863-2035-3-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
Thierry Reding
Acked-byRe: [PATCH 2/3] pinctrl: tegra-xusb: Fix allocation of pins
Quoted 7 lines · Jon Hunter
Commit
e5b3b2d9ed20("pinctrl: allows not to define theget_group_pinsoperation") allows pin controllers not to register theget_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 theget_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?
Quoted 5 lines
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 callingpinctrl_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:
A message that is not in the archive (sent off-list or not yet received): 20150519143041.GD26748-AwZRO8vwLAwmlAP/[email protected]
Re: [PATCH 2/3] pinctrl: tegra-xusb: Fix allocation of pins
Quoted 11 lines · Thierry Reding
* PGP Signed by an unknown key
Quoted 7 lines · Jon Hunter
Commit
e5b3b2d9ed20("pinctrl: allows not to define theget_group_pinsoperation") allows pin controllers not to register theget_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 theget_group_pins()function.
Quoted 3 lines
If I understand correctly this effectively makes
get_group_pins()mandatory, doesn't it? In that case, shouldn't commite5b3b2d9ed20be reverted?
Yes may be it should. I will defer to Linus here.
Quoted 8 lines
Quoted 5 lines
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 callingpinctrl_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.
Quoted 1 line
Acked-by Thierry Reding <[email protected]>
Cheers
Jon
Re: [PATCH 2/3] pinctrl: tegra-xusb: Fix allocation of pins
In reply to Jon HunterOn Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote:
[...]
One more thing:
[...]
Quoted 10 lines
drivers/pinctrl/pinctrl-tegra-xusb.c +2
-0@@ -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,
drivers/pinctrl/pinctrl-tegra-xusb.c static const char *tegra_xusb_padctl_get_group_name(struct pinctrl_dev *pinctrl, · line 139
Quoted 10 lines
drivers/pinctrl/pinctrl-tegra-xusb.c +10
-0+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
A message that is not in the archive (sent off-list or not yet received): 20150519143340.GE26748-AwZRO8vwLAwmlAP/[email protected]
Re: [PATCH 2/3] pinctrl: tegra-xusb: Fix allocation of pins
Quoted 25 lines · Thierry Reding
* PGP Signed by an unknown key
On Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote:
[...]One more thing:
[...]
Quoted 17 lines
drivers/pinctrl/pinctrl-tegra-xusb.c +9
-0@@ -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
Quoted 8 lines
drivers/pinctrl/pinctrl-tegra-xusb.c static const char *tegra_xusb_padctl_get_group_name(struct pinctrl_dev *pinctrl, · line 139
Quoted 3 lines
drivers/pinctrl/pinctrl-tegra-xusb.c +3
-0+* 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
Re: [PATCH 2/3] pinctrl: tegra-xusb: Fix allocation of pins
Quoted 36 lines · Jon Hunter
Quoted 25 lines · Thierry Reding
* PGP Signed by an unknown key
On Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote:
[...]One more thing:
[...]
Quoted 17 lines
drivers/pinctrl/pinctrl-tegra-xusb.c +9
-0@@ -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 Quoted 8 lines
drivers/pinctrl/pinctrl-tegra-xusb.c static const char *tegra_xusb_padctl_get_group_name(struct pinctrl_dev *pinctrl, · line 139
Quoted 3 lines
drivers/pinctrl/pinctrl-tegra-xusb.c +3
-0+* 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?
Quoted 6 lines
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
Jon Hunter
maintainerPATCH 3/3 pinctrl: tegra-xusb: Check that of_match_node returns a valid pointer
In reply to Jon Hunterof_match_node() will return NULL if no match is found and so check for this before dereferencing the pointer.
--- drivers/pinctrl/pinctrl-tegra-xusb.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/pinctrl/pinctrl-tegra-xusb.c +3 -0
@@ -899,6 +899,9 @@ static int tegra_xusb_padctl_probe(struct platform_device *pdev) padctl->dev = &pdev->dev; match =of_match_node (tegra_xusb_padctl_of_match, pdev->dev.of_node); +if (!match) +return -EINVAL ; + padctl->soc = match->data; res =platform_get_resource (pdev,IORESOURCE_MEM ,0 );
Signature · 1 line
1.9.1
Re: [PATCH 3/3] pinctrl: tegra-xusb: Check that of_match_node returns a valid pointer
Quoted 7 lines · Jon Hunter
of_match_node()will return NULL if no match is found and so check for this before dereferencing the pointer.Signed-off-by Jon Hunter <[email protected]>--- drivers/pinctrl/pinctrl-tegra-xusb.c | 3 +++ 1 file changed, 3 insertions(+)
This isn't necessary. The driver is probed from DT only and therefore .probe() will only be called if there's a matching entry in the OF device ID table. match can never be NULL.
Thierry
A message that is not in the archive (sent off-list or not yet received): 20150519141836.GB26748-AwZRO8vwLAwmlAP/[email protected]
Re: [PATCH 3/3] pinctrl: tegra-xusb: Check that of_match_node returns a valid pointer
Quoted 6 lines · Thierry Reding
* PGP Signed by an unknown key
Quoted 3 lines · Jon Hunter
of_match_node()will return NULL if no match is found and so check for this before dereferencing the pointer.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Jon Hunter <[email protected]>--- drivers/pinctrl/pinctrl-tegra-xusb.c | 3 +++ 1 file changed, 3 insertions(+)This isn't necessary. The driver is probed from DT only and therefore .probe() will only be called if there's a matching entry in the OF device ID table. match can never be NULL.
Ah, ok that would make sense.
Cheers Jon